[PATCH 0/3] gpio/mcp23s08: add support for i2c variants

[PATCH 0/3] gpio/mcp23s08: add support for i2c variants

[Peter Korsgaard]

The Microchip mcp23xxx I/O expanders exists both in variants with SPI
(23S) and I2C (230) interfaces, with either 8 or 16 I/O pins.

Until now the mcp23s08 driver only worked with the SPI variant. Adjust
the driver to work with the I2C ones as well.

 drivers/gpio/Kconfig         |    7 +-
 drivers/gpio/mcp23s08.c      |  260 +++++++++++++++++++++++++++++++++++++-----
 include/linux/spi/mcp23s08.h |    4 +-
 3 files changed, 237 insertions(+), 34 deletions(-)

--
Bye, Peter Korsgaard

[PATCH 1/3] mcp23s08: remove unused work queue

[Peter Korsgaard]

Never accessed anywhere.

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 drivers/gpio/mcp23s08.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/mcp23s08.c b/drivers/gpio/mcp23s08.c
index 40e0760..242a8a2 100644
--- a/drivers/gpio/mcp23s08.c
+++ b/drivers/gpio/mcp23s08.c
@@ -4,7 +4,6 @@
 
 #include <linux/kernel.h>
 #include <linux/device.h>
-#include <linux/workqueue.h>
 #include <linux/mutex.h>
 #include <linux/gpio.h>
 #include <linux/spi/spi.h>
@@ -60,8 +59,6 @@ struct mcp23s08 {
 
 	struct gpio_chip	chip;
 
-	struct work_struct	work;
-
 	const struct mcp23s08_ops	*ops;
 };
 
-- 
1.7.5.4

[PATCH 2/3] mcp23s08: isolate spi specific parts

[Peter Korsgaard]

Change spi member of struct mcp23s08 to be a ops-specific opaque data
pointer, and move spi specific knowledge out of mcp23s08_probe_one().

No functional change, but is needed to add i2c support.

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 drivers/gpio/mcp23s08.c |   75 ++++++++++++++++++++++++++++++++--------------
 1 files changed, 52 insertions(+), 23 deletions(-)

diff --git a/drivers/gpio/mcp23s08.c b/drivers/gpio/mcp23s08.c
index 242a8a2..1494347 100644
--- a/drivers/gpio/mcp23s08.c
+++ b/drivers/gpio/mcp23s08.c
@@ -50,7 +50,6 @@ struct mcp23s08_ops {
 };
 
 struct mcp23s08 {
-	struct spi_device	*spi;
 	u8			addr;
 
 	u16			cache[11];
@@ -60,6 +59,7 @@ struct mcp23s08 {
 	struct gpio_chip	chip;
 
 	const struct mcp23s08_ops	*ops;
+	void			*data; /* ops specific data */
 };
 
 /* A given spi_device can represent up to eight mcp23sxx chips
@@ -73,6 +73,8 @@ struct mcp23s08_driver_data {
 	struct mcp23s08		chip[];
 };
 
+#ifdef CONFIG_SPI_MASTER
+
 static int mcp23s08_read(struct mcp23s08 *mcp, unsigned reg)
 {
 	u8	tx[2], rx[1];
@@ -80,7 +82,7 @@ static int mcp23s08_read(struct mcp23s08 *mcp, unsigned reg)
 
 	tx[0] = mcp->addr | 0x01;
 	tx[1] = reg;
-	status = spi_write_then_read(mcp->spi, tx, sizeof tx, rx, sizeof rx);
+	status = spi_write_then_read(mcp->data, tx, sizeof tx, rx, sizeof rx);
 	return (status < 0) ? status : rx[0];
 }
 
@@ -91,7 +93,7 @@ static int mcp23s08_write(struct mcp23s08 *mcp, unsigned reg, unsigned val)
 	tx[0] = mcp->addr;
 	tx[1] = reg;
 	tx[2] = val;
-	return spi_write_then_read(mcp->spi, tx, sizeof tx, NULL, 0);
+	return spi_write_then_read(mcp->data, tx, sizeof tx, NULL, 0);
 }
 
 static int
@@ -106,7 +108,7 @@ mcp23s08_read_regs(struct mcp23s08 *mcp, unsigned reg, u16 *vals, unsigned n)
 	tx[1] = reg;
 
 	tmp = (u8 *)vals;
-	status = spi_write_then_read(mcp->spi, tx, sizeof tx, tmp, n);
+	status = spi_write_then_read(mcp->data, tx, sizeof tx, tmp, n);
 	if (status >= 0) {
 		while (n--)
 			vals[n] = tmp[n]; /* expand to 16bit */
@@ -121,7 +123,7 @@ static int mcp23s17_read(struct mcp23s08 *mcp, unsigned reg)
 
 	tx[0] = mcp->addr | 0x01;
 	tx[1] = reg << 1;
-	status = spi_write_then_read(mcp->spi, tx, sizeof tx, rx, sizeof rx);
+	status = spi_write_then_read(mcp->data, tx, sizeof tx, rx, sizeof rx);
 	return (status < 0) ? status : (rx[0] | (rx[1] << 8));
 }
 
@@ -133,7 +135,7 @@ static int mcp23s17_write(struct mcp23s08 *mcp, unsigned reg, unsigned val)
 	tx[1] = reg << 1;
 	tx[2] = val;
 	tx[3] = val >> 8;
-	return spi_write_then_read(mcp->spi, tx, sizeof tx, NULL, 0);
+	return spi_write_then_read(mcp->data, tx, sizeof tx, NULL, 0);
 }
 
 static int
@@ -147,7 +149,7 @@ mcp23s17_read_regs(struct mcp23s08 *mcp, unsigned reg, u16 *vals, unsigned n)
 	tx[0] = mcp->addr | 0x01;
 	tx[1] = reg << 1;
 
-	status = spi_write_then_read(mcp->spi, tx, sizeof tx,
+	status = spi_write_then_read(mcp->data, tx, sizeof tx,
 				     (u8 *)vals, n * 2);
 	if (status >= 0) {
 		while (n--)
@@ -169,6 +171,7 @@ static const struct mcp23s08_ops mcp23s17_ops = {
 	.read_regs	= mcp23s17_read_regs,
 };
 
+#endif /* CONFIG_SPI_MASTER */
 
 /*----------------------------------------------------------------------*/
 
@@ -296,17 +299,16 @@ done:
 
 /*----------------------------------------------------------------------*/
 
-static int mcp23s08_probe_one(struct spi_device *spi, unsigned addr,
+static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
+			      void *data, unsigned addr,
 			      unsigned type, unsigned base, unsigned pullups)
 {
-	struct mcp23s08_driver_data	*data = spi_get_drvdata(spi);
-	struct mcp23s08			*mcp = data->mcp[addr];
-	int				status;
+	int status;
 
 	mutex_init(&mcp->lock);
 
-	mcp->spi = spi;
-	mcp->addr = 0x40 | (addr << 1);
+	mcp->data = data;
+	mcp->addr = addr;
 
 	mcp->chip.direction_input = mcp23s08_direction_input;
 	mcp->chip.get = mcp23s08_get;
@@ -314,18 +316,29 @@ static int mcp23s08_probe_one(struct spi_device *spi, unsigned addr,
 	mcp->chip.set = mcp23s08_set;
 	mcp->chip.dbg_show = mcp23s08_dbg_show;
 
-	if (type == MCP_TYPE_S17) {
-		mcp->ops = &mcp23s17_ops;
-		mcp->chip.ngpio = 16;
-		mcp->chip.label = "mcp23s17";
-	} else {
+	switch (type) {
+#ifdef CONFIG_SPI_MASTER
+	case MCP_TYPE_S08:
 		mcp->ops = &mcp23s08_ops;
 		mcp->chip.ngpio = 8;
 		mcp->chip.label = "mcp23s08";
+		break;
+
+	case MCP_TYPE_S17:
+		mcp->ops = &mcp23s17_ops;
+		mcp->chip.ngpio = 16;
+		mcp->chip.label = "mcp23s17";
+		break;
+#endif /* CONFIG_SPI_MASTER */
+
+	default:
+		dev_err(dev, "invalid device type (%d)\n", type);
+		return -EINVAL;
 	}
+
 	mcp->chip.base = base;
 	mcp->chip.can_sleep = 1;
-	mcp->chip.dev = &spi->dev;
+	mcp->chip.dev = dev;
 	mcp->chip.owner = THIS_MODULE;
 
 	/* verify MCP_IOCON.SEQOP = 0, so sequential reads work,
@@ -371,11 +384,13 @@ static int mcp23s08_probe_one(struct spi_device *spi, unsigned addr,
 	status = gpiochip_add(&mcp->chip);
 fail:
 	if (status < 0)
-		dev_dbg(&spi->dev, "can't setup chip %d, --> %d\n",
-				addr, status);
+		dev_dbg(dev, "can't setup chip %d, --> %d\n",
+			addr, status);
 	return status;
 }
 
+#ifdef CONFIG_SPI_MASTER
+
 static int mcp23s08_probe(struct spi_device *spi)
 {
 	struct mcp23s08_platform_data	*pdata;
@@ -418,7 +433,8 @@ static int mcp23s08_probe(struct spi_device *spi)
 			continue;
 		chips--;
 		data->mcp[addr] = &data->chip[chips];
-		status = mcp23s08_probe_one(spi, addr, type, base,
+		status = mcp23s08_probe_one(data->mcp[addr], &spi->dev, spi,
+					    0x40 | (addr << 1), type, base,
 					    pdata->chip[addr].pullups);
 		if (status < 0)
 			goto fail;
@@ -507,11 +523,21 @@ static struct spi_driver mcp23s08_driver = {
 	},
 };
 
+#endif /* CONFIG_SPI_MASTER */
+
 /*----------------------------------------------------------------------*/
 
 static int __init mcp23s08_init(void)
 {
-	return spi_register_driver(&mcp23s08_driver);
+	int ret = 0;
+
+#ifdef CONFIG_SPI_MASTER
+	ret = spi_register_driver(&mcp23s08_driver);
+	if (ret)
+		return ret;
+#endif /* CONFIG_SPI_MASTER */
+
+	return ret;
 }
 /* register after spi postcore initcall and before
  * subsys initcalls that may rely on these GPIOs
@@ -520,7 +546,10 @@ subsys_initcall(mcp23s08_init);
 
 static void __exit mcp23s08_exit(void)
 {
+#ifdef CONFIG_SPI_MASTER
 	spi_unregister_driver(&mcp23s08_driver);
+#endif /* CONFIG_SPI_MASTER */
+
 }
 module_exit(mcp23s08_exit);
 
-- 
1.7.5.4

[PATCH 3/3] mcp23s08: add i2c support

[Peter Korsgaard]

Add i2c bindings for the mcp230xx devices. This is quite a lot simpler
than the spi ones as there's no funky sub addressing done (one struct
i2c_client per struct gpio_chip).

The mcp23s08_platform_data structure is reused for i2c, even though
only a single mcp23s08_chip_info structure is needed.

To make the platform data bus independent, the setup/teardown prototypes
are slightly changed, so the bus specific struct (spi_device / i2c_client)
is passed as a void pointer instead.

There's no in-tree users of these callbacks.

To use, simply fill out a platform_data structure and pass it in
i2c_board_info, E.G.:

static const struct mcp23s08_platform_data mcp23017_data = {
	.chip[0] = {
		.pullups = 0x00ff,
	},
	.base = 240,
};

static struct i2c_board_info __initdata i2c_devs[] = {
	{ I2C_BOARD_INFO("mcp23017", 0x20),
	  .platform_data = &smartview_mcp23017_data, },
	...
};

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 drivers/gpio/Kconfig         |    7 +-
 drivers/gpio/mcp23s08.c      |  184 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/spi/mcp23s08.h |    4 +-
 3 files changed, 186 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 2967002..cf8a491 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -398,10 +398,11 @@ config GPIO_MAX7301
 	  GPIO driver for Maxim MAX7301 SPI-based GPIO expander.
 
 config GPIO_MCP23S08
-	tristate "Microchip MCP23Sxx I/O expander"
-	depends on SPI_MASTER
+	tristate "Microchip MCP23xxx I/O expander"
+	depends on SPI_MASTER || I2C
 	help
-	  SPI driver for Microchip MCP23S08/MPC23S17 I/O expanders.
+	  SPI/I2C driver for Microchip MCP23S08/MCP23S17/MCP23008/MCP23017
+	  I/O expanders.
 	  This provides a GPIO interface supporting inputs and outputs.
 
 config GPIO_MC33880
diff --git a/drivers/gpio/mcp23s08.c b/drivers/gpio/mcp23s08.c
index 1494347..5914018 100644
--- a/drivers/gpio/mcp23s08.c
+++ b/drivers/gpio/mcp23s08.c
@@ -1,11 +1,12 @@
 /*
- * mcp23s08.c - SPI gpio expander driver
+ * mcp23s08.c - SPI/I2C gpio expander driver
  */
 
 #include <linux/kernel.h>
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/gpio.h>
+#include <linux/i2c.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/mcp23s08.h>
 #include <linux/slab.h>
@@ -16,13 +17,13 @@
  */
 #define MCP_TYPE_S08	0
 #define MCP_TYPE_S17	1
+#define MCP_TYPE_008	2
+#define MCP_TYPE_017	3
 
 /* Registers are all 8 bits wide.
  *
  * The mcp23s17 has twice as many bits, and can be configured to work
  * with either 16 bit registers or with two adjacent 8 bit banks.
- *
- * Also, there are I2C versions of both chips.
  */
 #define MCP_IODIR	0x00		/* init/reset:  all ones */
 #define MCP_IPOL	0x01
@@ -73,6 +74,72 @@ struct mcp23s08_driver_data {
 	struct mcp23s08		chip[];
 };
 
+/*----------------------------------------------------------------------*/
+
+#ifdef CONFIG_I2C
+
+static int mcp23008_read(struct mcp23s08 *mcp, unsigned reg)
+{
+	return i2c_smbus_read_byte_data(mcp->data, reg);
+}
+
+static int mcp23008_write(struct mcp23s08 *mcp, unsigned reg, unsigned val)
+{
+	return i2c_smbus_write_byte_data(mcp->data, reg, val);
+}
+
+static int
+mcp23008_read_regs(struct mcp23s08 *mcp, unsigned reg, u16 *vals, unsigned n)
+{
+	while (n--) {
+		int ret = mcp23008_read(mcp, reg++);
+		if (ret < 0)
+			return ret;
+		*vals++ = ret;
+	}
+
+	return 0;
+}
+
+static int mcp23017_read(struct mcp23s08 *mcp, unsigned reg)
+{
+	return i2c_smbus_read_word_data(mcp->data, reg << 1);
+}
+
+static int mcp23017_write(struct mcp23s08 *mcp, unsigned reg, unsigned val)
+{
+	return i2c_smbus_write_word_data(mcp->data, reg << 1, val);
+}
+
+static int
+mcp23017_read_regs(struct mcp23s08 *mcp, unsigned reg, u16 *vals, unsigned n)
+{
+	while (n--) {
+		int ret = mcp23017_read(mcp, reg++);
+		if (ret < 0)
+			return ret;
+		*vals++ = ret;
+	}
+
+	return 0;
+}
+
+static const struct mcp23s08_ops mcp23008_ops = {
+	.read		= mcp23008_read,
+	.write		= mcp23008_write,
+	.read_regs	= mcp23008_read_regs,
+};
+
+static const struct mcp23s08_ops mcp23017_ops = {
+	.read		= mcp23017_read,
+	.write		= mcp23017_write,
+	.read_regs	= mcp23017_read_regs,
+};
+
+#endif /* CONFIG_I2C */
+
+/*----------------------------------------------------------------------*/
+
 #ifdef CONFIG_SPI_MASTER
 
 static int mcp23s08_read(struct mcp23s08 *mcp, unsigned reg)
@@ -331,6 +398,20 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 		break;
 #endif /* CONFIG_SPI_MASTER */
 
+#ifdef CONFIG_I2C
+	case MCP_TYPE_008:
+		mcp->ops = &mcp23008_ops;
+		mcp->chip.ngpio = 8;
+		mcp->chip.label = "mcp23008";
+		break;
+
+	case MCP_TYPE_017:
+		mcp->ops = &mcp23017_ops;
+		mcp->chip.ngpio = 16;
+		mcp->chip.label = "mcp23017";
+		break;
+#endif /* CONFIG_I2C */
+
 	default:
 		dev_err(dev, "invalid device type (%d)\n", type);
 		return -EINVAL;
@@ -389,6 +470,93 @@ fail:
 	return status;
 }
 
+/*----------------------------------------------------------------------*/
+
+#ifdef CONFIG_I2C
+
+static int __devinit mcp230xx_probe(struct i2c_client *client,
+				    const struct i2c_device_id *id)
+{
+	struct mcp23s08_platform_data *pdata;
+	struct mcp23s08 *mcp;
+	int status;
+
+	pdata = client->dev.platform_data;
+	if (!pdata || !gpio_is_valid(pdata->base)) {
+		dev_dbg(&client->dev, "invalid or missing platform data\n");
+		return -EINVAL;
+	}
+
+	mcp = kzalloc(sizeof *mcp, GFP_KERNEL);
+	if (!mcp)
+		return -ENOMEM;
+
+	status = mcp23s08_probe_one(mcp, &client->dev, client, client->addr,
+				    id->driver_data, pdata->base,
+				    pdata->chip[0].pullups);
+	if (status)
+		goto fail;
+
+	i2c_set_clientdata(client, mcp);
+
+	if (pdata->setup) {
+		status = pdata->setup(client, pdata->base, mcp->chip.ngpio,
+				      pdata->context);
+		if (status < 0)
+			dev_dbg(&client->dev, "setup --> %d\n", status);
+	}
+
+	return 0;
+
+fail:
+	kfree(mcp);
+
+	return status;
+}
+
+static int __devexit mcp230xx_remove(struct i2c_client *client)
+{
+	struct mcp23s08_platform_data *pdata = client->dev.platform_data;
+	struct mcp23s08 *mcp = i2c_get_clientdata(client);
+	int status;
+
+	if (pdata->teardown) {
+		status = pdata->teardown(client, pdata->base, mcp->chip.ngpio,
+					 pdata->context);
+		if (status < 0) {
+			dev_err(&client->dev, "teardown --> %d\n", status);
+			return status;
+		}
+	}
+
+	status = gpiochip_remove(&mcp->chip);
+	if (status == 0)
+		kfree(mcp);
+
+	return status;
+}
+
+static const struct i2c_device_id mcp230xx_id[] = {
+	{ "mcp23008", MCP_TYPE_008 },
+	{ "mcp23017", MCP_TYPE_017 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, mcp230xx_id);
+
+static struct i2c_driver mcp230xx_driver = {
+	.driver = {
+		.name	= "mcp230xx",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= mcp230xx_probe,
+	.remove		= __devexit_p(mcp230xx_remove),
+	.id_table	= mcp230xx_id,
+};
+
+#endif /* CONFIG_I2C */
+
+/*----------------------------------------------------------------------*/
+
 #ifdef CONFIG_SPI_MASTER
 
 static int mcp23s08_probe(struct spi_device *spi)
@@ -537,9 +705,13 @@ static int __init mcp23s08_init(void)
 		return ret;
 #endif /* CONFIG_SPI_MASTER */
 
+#ifdef CONFIG_I2C
+	ret = i2c_add_driver(&mcp230xx_driver);
+#endif /* CONFIG_I2C */
+
 	return ret;
 }
-/* register after spi postcore initcall and before
+/* register after spi/i2c postcore initcall and before
  * subsys initcalls that may rely on these GPIOs
  */
 subsys_initcall(mcp23s08_init);
@@ -550,6 +722,10 @@ static void __exit mcp23s08_exit(void)
 	spi_unregister_driver(&mcp23s08_driver);
 #endif /* CONFIG_SPI_MASTER */
 
+#ifdef CONFIG_I2C
+	i2c_del_driver(&mcp230xx_driver);
+#endif /* CONFIG_I2C */
+
 }
 module_exit(mcp23s08_exit);
 
diff --git a/include/linux/spi/mcp23s08.h b/include/linux/spi/mcp23s08.h
index c42cff8..2eb4fc1 100644
--- a/include/linux/spi/mcp23s08.h
+++ b/include/linux/spi/mcp23s08.h
@@ -25,10 +25,10 @@ struct mcp23s08_platform_data {
 
 	void		*context;	/* param to setup/teardown */
 
-	int		(*setup)(struct spi_device *spi,
+	int		(*setup)(void *data,
 					int gpio, unsigned ngpio,
 					void *context);
-	int		(*teardown)(struct spi_device *spi,
+	int		(*teardown)(void *data,
 					int gpio, unsigned ngpio,
 					void *context);
 };
-- 
1.7.5.4

Re: [PATCH 3/3] mcp23s08: add i2c support

[Grant Likely]

On Thu, Jul 14, 2011 at 09:59:28PM +0200, Peter Korsgaard wrote:
> Add i2c bindings for the mcp230xx devices. This is quite a lot simpler
> than the spi ones as there's no funky sub addressing done (one struct
> i2c_client per struct gpio_chip).
> 
> The mcp23s08_platform_data structure is reused for i2c, even though
> only a single mcp23s08_chip_info structure is needed.
> 
> To make the platform data bus independent, the setup/teardown prototypes
> are slightly changed, so the bus specific struct (spi_device / i2c_client)
> is passed as a void pointer instead.
> 
> There's no in-tree users of these callbacks.

Why don't we just remove them then?  The notifier mechanism is more
generic anyway.

[...]
> @@ -537,9 +705,13 @@ static int __init mcp23s08_init(void)
>  		return ret;
>  #endif /* CONFIG_SPI_MASTER */
>  
> +#ifdef CONFIG_I2C
> +	ret = i2c_add_driver(&mcp230xx_driver);
> +#endif /* CONFIG_I2C */
> +

Need to unwind on failure to register, or put the i2c driver into a
separate module so that each module gets it's own init/exit hooks.

>  	return ret;
>  }
> -/* register after spi postcore initcall and before
> +/* register after spi/i2c postcore initcall and before
>   * subsys initcalls that may rely on these GPIOs
>   */
>  subsys_initcall(mcp23s08_init);
> @@ -550,6 +722,10 @@ static void __exit mcp23s08_exit(void)
>  	spi_unregister_driver(&mcp23s08_driver);
>  #endif /* CONFIG_SPI_MASTER */
>  
> +#ifdef CONFIG_I2C
> +	i2c_del_driver(&mcp230xx_driver);
> +#endif /* CONFIG_I2C */
> +
>  }
>  module_exit(mcp23s08_exit);
>  
> diff --git a/include/linux/spi/mcp23s08.h b/include/linux/spi/mcp23s08.h
> index c42cff8..2eb4fc1 100644
> --- a/include/linux/spi/mcp23s08.h
> +++ b/include/linux/spi/mcp23s08.h
> @@ -25,10 +25,10 @@ struct mcp23s08_platform_data {
>  
>  	void		*context;	/* param to setup/teardown */
>  
> -	int		(*setup)(struct spi_device *spi,
> +	int		(*setup)(void *data,
>  					int gpio, unsigned ngpio,
>  					void *context);
> -	int		(*teardown)(struct spi_device *spi,
> +	int		(*teardown)(void *data,
>  					int gpio, unsigned ngpio,
>  					void *context);
>  };
> -- 
> 1.7.5.4
> 

Re: [PATCH 1/3] mcp23s08: remove unused work queue

[Grant Likely]

On Thu, Jul 14, 2011 at 09:59:26PM +0200, Peter Korsgaard wrote:
> Never accessed anywhere.
> 
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>

I've applied this, but I had to do it manually.  Please rebase the
remaining two patches on linux-next.

g.

> ---
>  drivers/gpio/mcp23s08.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/mcp23s08.c b/drivers/gpio/mcp23s08.c
> index 40e0760..242a8a2 100644
> --- a/drivers/gpio/mcp23s08.c
> +++ b/drivers/gpio/mcp23s08.c
> @@ -4,7 +4,6 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/device.h>
> -#include <linux/workqueue.h>
>  #include <linux/mutex.h>
>  #include <linux/gpio.h>
>  #include <linux/spi/spi.h>
> @@ -60,8 +59,6 @@ struct mcp23s08 {
>  
>  	struct gpio_chip	chip;
>  
> -	struct work_struct	work;
> -
>  	const struct mcp23s08_ops	*ops;
>  };
>  
> -- 
> 1.7.5.4
> 

Re: [PATCH 2/3] mcp23s08: isolate spi specific parts

[Grant Likely]

On Thu, Jul 14, 2011 at 09:59:27PM +0200, Peter Korsgaard wrote:
> Change spi member of struct mcp23s08 to be a ops-specific opaque data
> pointer, and move spi specific knowledge out of mcp23s08_probe_one().
> 
> No functional change, but is needed to add i2c support.
> 
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
> ---
>  drivers/gpio/mcp23s08.c |   75 ++++++++++++++++++++++++++++++++--------------
>  1 files changed, 52 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpio/mcp23s08.c b/drivers/gpio/mcp23s08.c
> index 242a8a2..1494347 100644
> --- a/drivers/gpio/mcp23s08.c
> +++ b/drivers/gpio/mcp23s08.c
> @@ -50,7 +50,6 @@ struct mcp23s08_ops {
>  };
>  
>  struct mcp23s08 {
> -	struct spi_device	*spi;
>  	u8			addr;
>  
>  	u16			cache[11];
> @@ -60,6 +59,7 @@ struct mcp23s08 {
>  	struct gpio_chip	chip;
>  
>  	const struct mcp23s08_ops	*ops;
> +	void			*data; /* ops specific data */
>  };
>  
>  /* A given spi_device can represent up to eight mcp23sxx chips
> @@ -73,6 +73,8 @@ struct mcp23s08_driver_data {
>  	struct mcp23s08		chip[];
>  };
>  
> +#ifdef CONFIG_SPI_MASTER
> +
>  static int mcp23s08_read(struct mcp23s08 *mcp, unsigned reg)
>  {
>  	u8	tx[2], rx[1];
> @@ -80,7 +82,7 @@ static int mcp23s08_read(struct mcp23s08 *mcp, unsigned reg)
>  
>  	tx[0] = mcp->addr | 0x01;
>  	tx[1] = reg;
> -	status = spi_write_then_read(mcp->spi, tx, sizeof tx, rx, sizeof rx);
> +	status = spi_write_then_read(mcp->data, tx, sizeof tx, rx, sizeof rx);
>  	return (status < 0) ? status : rx[0];
>  }
>  
> @@ -91,7 +93,7 @@ static int mcp23s08_write(struct mcp23s08 *mcp, unsigned reg, unsigned val)
>  	tx[0] = mcp->addr;
>  	tx[1] = reg;
>  	tx[2] = val;
> -	return spi_write_then_read(mcp->spi, tx, sizeof tx, NULL, 0);
> +	return spi_write_then_read(mcp->data, tx, sizeof tx, NULL, 0);
>  }
>  
>  static int
> @@ -106,7 +108,7 @@ mcp23s08_read_regs(struct mcp23s08 *mcp, unsigned reg, u16 *vals, unsigned n)
>  	tx[1] = reg;
>  
>  	tmp = (u8 *)vals;
> -	status = spi_write_then_read(mcp->spi, tx, sizeof tx, tmp, n);
> +	status = spi_write_then_read(mcp->data, tx, sizeof tx, tmp, n);
>  	if (status >= 0) {
>  		while (n--)
>  			vals[n] = tmp[n]; /* expand to 16bit */
> @@ -121,7 +123,7 @@ static int mcp23s17_read(struct mcp23s08 *mcp, unsigned reg)
>  
>  	tx[0] = mcp->addr | 0x01;
>  	tx[1] = reg << 1;
> -	status = spi_write_then_read(mcp->spi, tx, sizeof tx, rx, sizeof rx);
> +	status = spi_write_then_read(mcp->data, tx, sizeof tx, rx, sizeof rx);
>  	return (status < 0) ? status : (rx[0] | (rx[1] << 8));
>  }
>  
> @@ -133,7 +135,7 @@ static int mcp23s17_write(struct mcp23s08 *mcp, unsigned reg, unsigned val)
>  	tx[1] = reg << 1;
>  	tx[2] = val;
>  	tx[3] = val >> 8;
> -	return spi_write_then_read(mcp->spi, tx, sizeof tx, NULL, 0);
> +	return spi_write_then_read(mcp->data, tx, sizeof tx, NULL, 0);
>  }
>  
>  static int
> @@ -147,7 +149,7 @@ mcp23s17_read_regs(struct mcp23s08 *mcp, unsigned reg, u16 *vals, unsigned n)
>  	tx[0] = mcp->addr | 0x01;
>  	tx[1] = reg << 1;
>  
> -	status = spi_write_then_read(mcp->spi, tx, sizeof tx,
> +	status = spi_write_then_read(mcp->data, tx, sizeof tx,
>  				     (u8 *)vals, n * 2);
>  	if (status >= 0) {
>  		while (n--)
> @@ -169,6 +171,7 @@ static const struct mcp23s08_ops mcp23s17_ops = {
>  	.read_regs	= mcp23s17_read_regs,
>  };
>  
> +#endif /* CONFIG_SPI_MASTER */
>  
>  /*----------------------------------------------------------------------*/
>  
> @@ -296,17 +299,16 @@ done:
>  
>  /*----------------------------------------------------------------------*/
>  
> -static int mcp23s08_probe_one(struct spi_device *spi, unsigned addr,
> +static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
> +			      void *data, unsigned addr,
>  			      unsigned type, unsigned base, unsigned pullups)
>  {
> -	struct mcp23s08_driver_data	*data = spi_get_drvdata(spi);
> -	struct mcp23s08			*mcp = data->mcp[addr];
> -	int				status;
> +	int status;
>  
>  	mutex_init(&mcp->lock);
>  
> -	mcp->spi = spi;
> -	mcp->addr = 0x40 | (addr << 1);
> +	mcp->data = data;
> +	mcp->addr = addr;
>  
>  	mcp->chip.direction_input = mcp23s08_direction_input;
>  	mcp->chip.get = mcp23s08_get;
> @@ -314,18 +316,29 @@ static int mcp23s08_probe_one(struct spi_device *spi, unsigned addr,
>  	mcp->chip.set = mcp23s08_set;
>  	mcp->chip.dbg_show = mcp23s08_dbg_show;
>  
> -	if (type == MCP_TYPE_S17) {
> -		mcp->ops = &mcp23s17_ops;
> -		mcp->chip.ngpio = 16;
> -		mcp->chip.label = "mcp23s17";
> -	} else {
> +	switch (type) {
> +#ifdef CONFIG_SPI_MASTER
> +	case MCP_TYPE_S08:
>  		mcp->ops = &mcp23s08_ops;
>  		mcp->chip.ngpio = 8;
>  		mcp->chip.label = "mcp23s08";
> +		break;
> +
> +	case MCP_TYPE_S17:
> +		mcp->ops = &mcp23s17_ops;
> +		mcp->chip.ngpio = 16;
> +		mcp->chip.label = "mcp23s17";
> +		break;
> +#endif /* CONFIG_SPI_MASTER */
> +
> +	default:
> +		dev_err(dev, "invalid device type (%d)\n", type);
> +		return -EINVAL;
>  	}
> +
>  	mcp->chip.base = base;
>  	mcp->chip.can_sleep = 1;
> -	mcp->chip.dev = &spi->dev;
> +	mcp->chip.dev = dev;
>  	mcp->chip.owner = THIS_MODULE;
>  
>  	/* verify MCP_IOCON.SEQOP = 0, so sequential reads work,
> @@ -371,11 +384,13 @@ static int mcp23s08_probe_one(struct spi_device *spi, unsigned addr,
>  	status = gpiochip_add(&mcp->chip);
>  fail:
>  	if (status < 0)
> -		dev_dbg(&spi->dev, "can't setup chip %d, --> %d\n",
> -				addr, status);
> +		dev_dbg(dev, "can't setup chip %d, --> %d\n",
> +			addr, status);
>  	return status;
>  }
>  
> +#ifdef CONFIG_SPI_MASTER
> +
>  static int mcp23s08_probe(struct spi_device *spi)
>  {
>  	struct mcp23s08_platform_data	*pdata;
> @@ -418,7 +433,8 @@ static int mcp23s08_probe(struct spi_device *spi)
>  			continue;
>  		chips--;
>  		data->mcp[addr] = &data->chip[chips];
> -		status = mcp23s08_probe_one(spi, addr, type, base,
> +		status = mcp23s08_probe_one(data->mcp[addr], &spi->dev, spi,
> +					    0x40 | (addr << 1), type, base,
>  					    pdata->chip[addr].pullups);
>  		if (status < 0)
>  			goto fail;
> @@ -507,11 +523,21 @@ static struct spi_driver mcp23s08_driver = {
>  	},
>  };
>  
> +#endif /* CONFIG_SPI_MASTER */
> +
>  /*----------------------------------------------------------------------*/
>  
>  static int __init mcp23s08_init(void)
>  {
> -	return spi_register_driver(&mcp23s08_driver);
> +	int ret = 0;

'= 0' is redundant.

> +
> +#ifdef CONFIG_SPI_MASTER
> +	ret = spi_register_driver(&mcp23s08_driver);
> +	if (ret)
> +		return ret;
> +#endif /* CONFIG_SPI_MASTER */
> +
> +	return ret;

This change really belongs in the 3rd patch that adds the i2c
registration.

>  }
>  /* register after spi postcore initcall and before
>   * subsys initcalls that may rely on these GPIOs
> @@ -520,7 +546,10 @@ subsys_initcall(mcp23s08_init);
>  
>  static void __exit mcp23s08_exit(void)
>  {
> +#ifdef CONFIG_SPI_MASTER
>  	spi_unregister_driver(&mcp23s08_driver);
> +#endif /* CONFIG_SPI_MASTER */
> +

Extra blank line.
>  }
>  module_exit(mcp23s08_exit);
>  
> -- 
> 1.7.5.4
> 

Re: [PATCH 1/3] mcp23s08: remove unused work queue

[Peter Korsgaard]

>>>>> "Grant" == Grant Likely <grant.likely@secretlab.ca> writes:

 Grant> On Thu, Jul 14, 2011 at 09:59:26PM +0200, Peter Korsgaard wrote:
 >> Never accessed anywhere.
 >> 
 >> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>

 Grant> I've applied this, but I had to do it manually.  Please rebase the
 Grant> remaining two patches on linux-next.

Thanks, I will.

-- 
Bye, Peter Korsgaard

Re: [PATCH 2/3] mcp23s08: isolate spi specific parts

[Peter Korsgaard]

>>>>> "Grant" == Grant Likely <grant.likely@secretlab.ca> writes:

 Grant> On Thu, Jul 14, 2011 at 09:59:27PM +0200, Peter Korsgaard wrote:
 >> Change spi member of struct mcp23s08 to be a ops-specific opaque data
 >> pointer, and move spi specific knowledge out of mcp23s08_probe_one().
 >> 
 >> No functional change, but is needed to add i2c support.
 >> 
 >> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
 >> ---
 >> drivers/gpio/mcp23s08.c |   75 ++++++++++++++++++++++++++++++++--------------
 >> static int __init mcp23s08_init(void)
 >> {
 >> -	return spi_register_driver(&mcp23s08_driver);
 >> +	int ret = 0;

 Grant> '= 0' is redundant.

Will fix.

 >> +
 >> +#ifdef CONFIG_SPI_MASTER
 >> +	ret = spi_register_driver(&mcp23s08_driver);
 >> +	if (ret)
 >> +		return ret;
 >> +#endif /* CONFIG_SPI_MASTER */
 >> +
 >> +	return ret;

 Grant> This change really belongs in the 3rd patch that adds the i2c
 Grant> registration.

You can argue for both ways. With my approach the 3rd patch doesn't
touch any of the spi stuff, but OK - I'll change.


-- 
Bye, Peter Korsgaard

Re: [PATCH 3/3] mcp23s08: add i2c support

[Peter Korsgaard]

>>>>> "Grant" == Grant Likely <grant.likely@secretlab.ca> writes:

 Grant> On Thu, Jul 14, 2011 at 09:59:28PM +0200, Peter Korsgaard wrote:
 >> Add i2c bindings for the mcp230xx devices. This is quite a lot simpler
 >> than the spi ones as there's no funky sub addressing done (one struct
 >> i2c_client per struct gpio_chip).
 >> 
 >> The mcp23s08_platform_data structure is reused for i2c, even though
 >> only a single mcp23s08_chip_info structure is needed.
 >> 
 >> To make the platform data bus independent, the setup/teardown prototypes
 >> are slightly changed, so the bus specific struct (spi_device / i2c_client)
 >> is passed as a void pointer instead.
 >> 
 >> There's no in-tree users of these callbacks.

 Grant> Why don't we just remove them then?  The notifier mechanism is more
 Grant> generic anyway.

Ok, will do.

 Grant> [...]
 >> @@ -537,9 +705,13 @@ static int __init mcp23s08_init(void)
 >> return ret;
 >> #endif /* CONFIG_SPI_MASTER */
 >> 
 >> +#ifdef CONFIG_I2C
 >> +	ret = i2c_add_driver(&mcp230xx_driver);
 >> +#endif /* CONFIG_I2C */
 >> +

 Grant> Need to unwind on failure to register, or put the i2c driver into a
 Grant> separate module so that each module gets it's own init/exit hooks.

Ok, will unwind.

Thanks for the quick review, will send an updated patch series shortly.

-- 
Bye, Peter Korsgaard