Re: [PATCH] mcp23s08: support mcp23s17 variant

[Grant Likely]

On Mon, Feb 14, 2011 at 9:12 AM, Peter Korsgaard <jacmet@xxxxxxxxxx> wrote:
> mpc23s17 is very similar to the mcp23s08, except that registers are 16bit
> wide, so extend the interface to work with both variants.
>
> The s17 variant also has an additional address pin, so adjust platform
> data structure to support up to 8 devices per SPI chipselect.
>
> Signed-off-by: Peter Korsgaard <jacmet@xxxxxxxxxx>
> ---
>  drivers/gpio/Kconfig         |    6 +-
>  drivers/gpio/mcp23s08.c      |  147 +++++++++++++++++++++++++++++-------------
>  include/linux/spi/mcp23s08.h |   15 +++--
>  3 files changed, 113 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 664660e..9573b33 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -368,11 +368,11 @@ config GPIO_MAX7301
>          GPIO driver for Maxim MAX7301 SPI-based GPIO expander.
>
>  config GPIO_MCP23S08
> -       tristate "Microchip MCP23S08 I/O expander"
> +       tristate "Microchip MCP23Sxx I/O expander"
>        depends on SPI_MASTER
>        help
> -         SPI driver for Microchip MCP23S08 I/O expander.  This provides
> -         a GPIO interface supporting inputs and outputs.
> +         SPI driver for Microchip MCP23S08/MPC23S17 I/O expanders.
> +         This provides a GPIO interface supporting inputs and outputs.
>
>  config GPIO_MC33880
>        tristate "Freescale MC33880 high-side/low-side switch"
> diff --git a/drivers/gpio/mcp23s08.c b/drivers/gpio/mcp23s08.c
> index 69f6f19..497e527 100644
> --- a/drivers/gpio/mcp23s08.c
> +++ b/drivers/gpio/mcp23s08.c
> @@ -10,7 +10,13 @@
>  #include <linux/spi/spi.h>
>  #include <linux/spi/mcp23s08.h>
>  #include <linux/slab.h>
> +#include <asm/byteorder.h>
>
> +/**
> + * MCP types supported by driver
> + */
> +#define MCP_TYPE_S08   0
> +#define MCP_TYPE_S17   1
>
>  /* Registers are all 8 bits wide.
>  *
> @@ -38,8 +44,9 @@
>  struct mcp23s08 {
>        struct spi_device       *spi;
>        u8                      addr;
> +       u8                      type;
>
> -       u8                      cache[11];
> +       u16                     cache[11];
>        /* lock protects the cached values */
>        struct mutex            lock;
>
> @@ -48,48 +55,83 @@ struct mcp23s08 {
>        struct work_struct      work;
>  };
>
> -/* A given spi_device can represent up to four mcp23s08 chips
> +/* A given spi_device can represent up to eight mcp23sxx chips
>  * sharing the same chipselect but using different addresses
>  * (e.g. chips #0 and #3 might be populated, but not #1 or $2).
>  * Driver data holds all the per-chip data.
>  */
>  struct mcp23s08_driver_data {
>        unsigned                ngpio;
> -       struct mcp23s08         *mcp[4];
> +       struct mcp23s08         *mcp[8];
>        struct mcp23s08         chip[];
>  };
>
>  static int mcp23s08_read(struct mcp23s08 *mcp, unsigned reg)
>  {
> -       u8      tx[2], rx[1];
> +       u8      tx[2], rx[2];
>        int     status;
>
>        tx[0] = mcp->addr | 0x01;
>        tx[1] = reg;
> -       status = spi_write_then_read(mcp->spi, tx, sizeof tx, rx, sizeof rx);
> -       return (status < 0) ? status : rx[0];
> +
> +       if (mcp->type == MCP_TYPE_S17) {
> +               tx[1] <<= 1;
> +
> +               status = spi_write_then_read(mcp->spi, tx, sizeof tx, rx, 2);
> +               return (status < 0) ? status : (rx[0] | (rx[1] << 8));
> +       } else {
> +               status = spi_write_then_read(mcp->spi, tx, sizeof tx, rx, 1);
> +               return (status < 0) ? status : rx[0];
> +       }

Rather than checking ->type for every transaction, would a set of
callbacks for each type be better?  It would probably have lower
overhead and be simpler to maintain.

[...]
> diff --git a/include/linux/spi/mcp23s08.h b/include/linux/spi/mcp23s08.h
> index 22ef107..c42cff8 100644
> --- a/include/linux/spi/mcp23s08.h
> +++ b/include/linux/spi/mcp23s08.h
> @@ -2,21 +2,24 @@
>  /* FIXME driver should be able to handle IRQs...  */
>
>  struct mcp23s08_chip_info {
> -       bool    is_present;             /* true iff populated */
> -       u8      pullups;                /* BIT(x) means enable pullup x */
> +       bool            is_present;     /* true if populated */
> +       unsigned        pullups;        /* BIT(x) means enable pullup x */
>  };

Unrelated whitespace changes.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/