[PATCH RFC 0/1] Convert mcp23s08 to DT usage

[PATCH RFC 0/1] Convert mcp23s08 to DT usage

[Lars Poeschel]

From: Lars Poeschel <poeschel@lemonage.de>

I wanted to use mcp23s08 driver with a device that boots using device tree.
Therefore I had to modify the driver to be able to take it's platform_data
from the device tree.
I am using a mcp23017 (I2C) device successfully with the patch booted over
device tree. I am not sure if I did this right and I am not able to test the
SPI part of the driver, so I am requesting for comments.

Thank you,
Lars

Lars Poeschel (1):
  gpio: mcp23s08: convert driver to DT

 .../devicetree/bindings/gpio/gpio-mcp23s08.txt     |   27 ++++++
 drivers/gpio/gpio-mcp23s08.c                       |   93 +++++++++++++++++++-
 include/linux/spi/mcp23s08.h                       |    1 +
 3 files changed, 117 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt

-- 
1.7.10.4

[PATCH RFC 1/1] gpio: mcp23s08: convert driver to DT

[Lars Poeschel]

From: Lars Poeschel <poeschel@lemonage.de>

This converts the mcp23s08 driver to be able to be used with device
tree.
There are two properties taken, that correspond to the members of
the struct mcp23s08_platform_data, that is the base member and the
chip array member.

Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
 .../devicetree/bindings/gpio/gpio-mcp23s08.txt     |   27 ++++++
 drivers/gpio/gpio-mcp23s08.c                       |   93 +++++++++++++++++++-
 include/linux/spi/mcp23s08.h                       |    1 +
 3 files changed, 117 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
new file mode 100644
index 0000000..572bc87
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
@@ -0,0 +1,27 @@
+Microchip MCP2308/MCP23S08/MCP23017/MCP23S17 driver for
+8-/16-bit I/O expander with serial interface (I2C/SPI)
+
+Required properties:
+- compatible : Should be "mcp,mcp23s08-gpio", "mcp,mcp23s17-gpio",
+			"mcp,mcp23008-gpio" or "mcp,mcp23017-gpio"
+- base : The first gpio number that should be assigned by this chip.
+
+Optional properties:
+- chips : This is a table with 2 columns and up to 8 entries. The first column
+	is a is_present flag, that makes only sense for SPI chips. Multiple
+	chips can share the same chipselect. This flag can be 0 or 1 depending
+	if there is a chip at this address or not.
+	The second column is written to the GPPU register, selecting a 100k
+	pullup resistor or not. Setting a 1 is activating the pullup.
+	For I2C chips only the first line in this table is used. Further chips
+	are registered at different addresses at the I2C bus.
+
+Example:
+&mcp0 {
+	compatible = "mcp,mcp23017";
+	base = <128>;
+	chips = <
+	/* is_present  pullups */
+		1	0x07	/* chip addr 0 */
+	>;
+};
diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
index 3cea0ea..7f90d11 100644
--- a/drivers/gpio/gpio-mcp23s08.c
+++ b/drivers/gpio/gpio-mcp23s08.c
@@ -12,6 +12,8 @@
 #include <linux/spi/mcp23s08.h>
 #include <linux/slab.h>
 #include <asm/byteorder.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 /**
  * MCP types supported by driver
@@ -473,16 +475,89 @@ fail:
 
 /*----------------------------------------------------------------------*/
 
+#ifdef CONFIG_OF
+static struct of_device_id mcp23s08_of_match[] = {
+#ifdef CONFIG_SPI_MASTER
+	{
+		.compatible = "mcp,mcp23s08-gpio",
+	},
+	{
+		.compatible = "mcp,mcp23s17-gpio",
+	},
+#endif
+#if IS_ENABLED(CONFIG_I2C)
+	{
+		.compatible = "mcp,mcp23008-gpio",
+	},
+	{
+		.compatible = "mcp,mcp23017-gpio",
+	},
+#endif
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mcp23s08_of_match);
+
+static struct mcp23s08_platform_data *
+		of_get_mcp23s08_pdata(struct device *dev)
+{
+	struct mcp23s08_platform_data *pdata;
+	struct device_node *np = dev->of_node;
+	u32 gpio_base;
+	u32 chips[sizeof(pdata->chip)];
+	int ret, i, j;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	ret = of_property_read_u32(np, "base", &gpio_base);
+	if (!ret)
+		pdata->base = gpio_base;
+
+	for (i = ARRAY_SIZE(pdata->chip) * MCP23S08_CHIP_INFO_MEMBERS;
+				i > 0; i -= MCP23S08_CHIP_INFO_MEMBERS) {
+		ret = of_property_read_u32_array(np, "chips", chips, i);
+		if (ret == -EOVERFLOW)
+			continue;
+		break;
+	}
+	if (!ret) {
+		for (j = 0; j < i / MCP23S08_CHIP_INFO_MEMBERS ; j++) {
+			pdata->chip[j].is_present =
+				chips[j * MCP23S08_CHIP_INFO_MEMBERS];
+			pdata->chip[j].pullups =
+				chips[j * MCP23S08_CHIP_INFO_MEMBERS + 1];
+		}
+	}
+
+	return pdata;
+}
+#else
+static struct mcp23s08_platform_data *
+		of_get_mcp23s08_pdata(struct i2c_client *client, int *chip_id)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF */
+
 #if IS_ENABLED(CONFIG_I2C)
 
 static int mcp230xx_probe(struct i2c_client *client,
 				    const struct i2c_device_id *id)
 {
-	struct mcp23s08_platform_data *pdata;
+	struct mcp23s08_platform_data *pdata = NULL;
 	struct mcp23s08 *mcp;
 	int status;
+	const struct of_device_id *match;
+
+	match = of_match_device(of_match_ptr(mcp23s08_of_match), &client->dev);
+	if (match)
+		pdata = of_get_mcp23s08_pdata(&client->dev);
+
+	/* if there was no pdata in DT, take it the legacy way */
+	if (!pdata)
+		pdata = client->dev.platform_data;
 
-	pdata = client->dev.platform_data;
 	if (!pdata || !gpio_is_valid(pdata->base)) {
 		dev_dbg(&client->dev, "invalid or missing platform data\n");
 		return -EINVAL;
@@ -531,6 +606,7 @@ static struct i2c_driver mcp230xx_driver = {
 	.driver = {
 		.name	= "mcp230xx",
 		.owner	= THIS_MODULE,
+		.of_match_table = mcp23s08_of_match,
 	},
 	.probe		= mcp230xx_probe,
 	.remove		= mcp230xx_remove,
@@ -560,16 +636,24 @@ static void mcp23s08_i2c_exit(void) { }
 
 static int mcp23s08_probe(struct spi_device *spi)
 {
-	struct mcp23s08_platform_data	*pdata;
+	struct mcp23s08_platform_data	*pdata = NULL;
 	unsigned			addr;
 	unsigned			chips = 0;
 	struct mcp23s08_driver_data	*data;
 	int				status, type;
 	unsigned			base;
+	const struct			of_device_id *match;
 
 	type = spi_get_device_id(spi)->driver_data;
 
-	pdata = spi->dev.platform_data;
+	match = of_match_device(of_match_ptr(mcp23s08_of_match), &spi->dev);
+	if (match)
+		pdata = of_get_mcp23s08_pdata(&spi->dev);
+
+	/* if there was no pdata in DT, take it the legacy way */
+	if (!pdata)
+		pdata = spi->dev.platform_data;
+
 	if (!pdata || !gpio_is_valid(pdata->base)) {
 		dev_dbg(&spi->dev, "invalid or missing platform data\n");
 		return -EINVAL;
@@ -668,6 +752,7 @@ static struct spi_driver mcp23s08_driver = {
 	.driver = {
 		.name	= "mcp23s08",
 		.owner	= THIS_MODULE,
+		.of_match_table = mcp23s08_of_match,
 	},
 };
 
diff --git a/include/linux/spi/mcp23s08.h b/include/linux/spi/mcp23s08.h
index 2d676d5..3969e12 100644
--- a/include/linux/spi/mcp23s08.h
+++ b/include/linux/spi/mcp23s08.h
@@ -1,3 +1,4 @@
+#define MCP23S08_CHIP_INFO_MEMBERS	2
 
 /* FIXME driver should be able to handle IRQs...  */
 
-- 
1.7.10.4

Re: [PATCH RFC 1/1] gpio: mcp23s08: convert driver to DT

[Linus Walleij]

On Thu, Jan 31, 2013 at 4:58 PM, Lars Poeschel <larsi@wh2.tu-dresden.de> wrote:

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> @@ -0,0 +1,27 @@
> +Microchip MCP2308/MCP23S08/MCP23017/MCP23S17 driver for
> +8-/16-bit I/O expander with serial interface (I2C/SPI)
> +
> +Required properties:
> +- compatible : Should be "mcp,mcp23s08-gpio", "mcp,mcp23s17-gpio",
> +                       "mcp,mcp23008-gpio" or "mcp,mcp23017-gpio"
> +- base : The first gpio number that should be assigned by this chip.

No. We do not tie the global GPIO numbers into the device tree.

In the DT GPIOs are referenced by ampersand <&gpio0 1 2>
notation referring to the instance, so as you realize DT itself
has no need for that number.

Further it is not OS-neutral.

You have to find another way to handle this in the driver code.
In worst case: use AUXDATA.

Yours,
Linus Walleij

Re: [PATCH RFC 1/1] gpio: mcp23s08: convert driver to DT

[Grant Likely]

On Thu, 31 Jan 2013 21:51:36 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jan 31, 2013 at 4:58 PM, Lars Poeschel <larsi@wh2.tu-dresden.de> wrote:
> 
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> > @@ -0,0 +1,27 @@
> > +Microchip MCP2308/MCP23S08/MCP23017/MCP23S17 driver for
> > +8-/16-bit I/O expander with serial interface (I2C/SPI)
> > +
> > +Required properties:
> > +- compatible : Should be "mcp,mcp23s08-gpio", "mcp,mcp23s17-gpio",
> > +                       "mcp,mcp23008-gpio" or "mcp,mcp23017-gpio"
> > +- base : The first gpio number that should be assigned by this chip.
> 
> No. We do not tie the global GPIO numbers into the device tree.
> 
> In the DT GPIOs are referenced by ampersand <&gpio0 1 2>
> notation referring to the instance, so as you realize DT itself
> has no need for that number.
> 
> Further it is not OS-neutral.
> 
> You have to find another way to handle this in the driver code.
> In worst case: use AUXDATA.

Hi Lars,

The trick is to declare the io expander to be a "gpio-controller" and
use the #gpio-cells property to declare how many cells (32-bit numbers)
are need to specify a single gpio line. Most gpio controllers use
"gpio-cells=<2>"; The first cell is the *controller local* gpio
number, and the second cell is used for flags. That way your gpio
controller can be referenced by other nodes in the tree with a "gpios"
property.

You can find lots of examples of this in the tree.

g.

Re: [PATCH RFC 1/1] gpio: mcp23s08: convert driver to DT

[Lars Poeschel]

On Tuesday 05 February 2013 at 15:29:09, Grant Likely wrote:
> On Thu, 31 Jan 2013 21:51:36 +0100, Linus Walleij 
<linus.walleij@linaro.org> wrote:
> > On Thu, Jan 31, 2013 at 4:58 PM, Lars Poeschel <larsi@wh2.tu-dresden.de> 
wrote:
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> > > @@ -0,0 +1,27 @@
> > > +Microchip MCP2308/MCP23S08/MCP23017/MCP23S17 driver for
> > > +8-/16-bit I/O expander with serial interface (I2C/SPI)
> > > +
> > > +Required properties:
> > > +- compatible : Should be "mcp,mcp23s08-gpio", "mcp,mcp23s17-gpio",
> > > +                       "mcp,mcp23008-gpio" or "mcp,mcp23017-gpio"
> > > +- base : The first gpio number that should be assigned by this chip.
> > 
> > No. We do not tie the global GPIO numbers into the device tree.
> > 
> > In the DT GPIOs are referenced by ampersand <&gpio0 1 2>
> > notation referring to the instance, so as you realize DT itself
> > has no need for that number.
> > 
> > Further it is not OS-neutral.
> > 
> > You have to find another way to handle this in the driver code.
> > In worst case: use AUXDATA.
> 
> Hi Lars,
> 
> The trick is to declare the io expander to be a "gpio-controller" and
> use the #gpio-cells property to declare how many cells (32-bit numbers)
> are need to specify a single gpio line. Most gpio controllers use
> "gpio-cells=<2>"; The first cell is the *controller local* gpio
> number, and the second cell is used for flags. That way your gpio
> controller can be referenced by other nodes in the tree with a "gpios"
> property.
> 
> You can find lots of examples of this in the tree.

Linus, Grant, thanks for the explanations. I think I have catched where it 
should go.
The thing that confused me was, that the platform_data for the chip has a 
mandatory "base" member, that sets the linux global gpio number at which the 
chip should appear. A value of -1 for automatic assigning gpio number is not 
allowed, the chip will not probe.
I have to change the driver to allow at least this -1 as an additional value.
As Linus pointed out, it is not desirable to set the global gpio base number 
from device tree, right ? If I have 3 instances of this chips then, how can 
userspace sw distinguish then to which one it is talking ?

This is an example for a DT entry (for i2c version) of the chip as I am 
targetting it now:

gpiom1: gpio@20 {
	reg = <0x20>;
	compatible = "mcp,mcp23017-gpio";
	gpio-controller;
	#gpio-cells = <2>;
};

I am working on this but I have some strange issues with the driver 
probing/not probing and kernel debug output. I hope I will solve this today. 
I will send a new patch then.

Lars

Re: [PATCH RFC 1/1] gpio: mcp23s08: convert driver to DT

[Linus Walleij]

On Wed, Feb 6, 2013 at 10:31 AM, Lars Poeschel <poeschel@lemonage.de> wrote:

> The thing that confused me was, that the platform_data for the chip has a
> mandatory "base" member, that sets the linux global gpio number at which the
> chip should appear.

Yes this is common. I think you should look at other drivers
under drivers/gpio using device tree, and how they work around
this.

As stated, as a last resort you can use AUXDATA to anyway assign
a piece of platform data per instance.

In the Nomadik driver, we use the block instance ID and multiply
by a factor of the numbers of GPIOs on each instance.
And luckily the base is zero. Not elegant maybe, but the
global GPIO numberspace is not elegant by nature.

Yours,
Linus Walleij

Re: [PATCH RFC 1/1] gpio: mcp23s08: convert driver to DT

[Grant Likely]

On Wed, 6 Feb 2013 10:31:04 +0100, Lars Poeschel <poeschel@lemonage.de> wrote:
> On Tuesday 05 February 2013 at 15:29:09, Grant Likely wrote:
> > On Thu, 31 Jan 2013 21:51:36 +0100, Linus Walleij 
> <linus.walleij@linaro.org> wrote:
> > > On Thu, Jan 31, 2013 at 4:58 PM, Lars Poeschel <larsi@wh2.tu-dresden.de> 
> wrote:
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> > > > @@ -0,0 +1,27 @@
> > > > +Microchip MCP2308/MCP23S08/MCP23017/MCP23S17 driver for
> > > > +8-/16-bit I/O expander with serial interface (I2C/SPI)
> > > > +
> > > > +Required properties:
> > > > +- compatible : Should be "mcp,mcp23s08-gpio", "mcp,mcp23s17-gpio",
> > > > +                       "mcp,mcp23008-gpio" or "mcp,mcp23017-gpio"
> > > > +- base : The first gpio number that should be assigned by this chip.
> > > 
> > > No. We do not tie the global GPIO numbers into the device tree.
> > > 
> > > In the DT GPIOs are referenced by ampersand <&gpio0 1 2>
> > > notation referring to the instance, so as you realize DT itself
> > > has no need for that number.
> > > 
> > > Further it is not OS-neutral.
> > > 
> > > You have to find another way to handle this in the driver code.
> > > In worst case: use AUXDATA.
> > 
> > Hi Lars,
> > 
> > The trick is to declare the io expander to be a "gpio-controller" and
> > use the #gpio-cells property to declare how many cells (32-bit numbers)
> > are need to specify a single gpio line. Most gpio controllers use
> > "gpio-cells=<2>"; The first cell is the *controller local* gpio
> > number, and the second cell is used for flags. That way your gpio
> > controller can be referenced by other nodes in the tree with a "gpios"
> > property.
> > 
> > You can find lots of examples of this in the tree.
> 
> Linus, Grant, thanks for the explanations. I think I have catched where it 
> should go.
> The thing that confused me was, that the platform_data for the chip has a 
> mandatory "base" member, that sets the linux global gpio number at which the 
> chip should appear. A value of -1 for automatic assigning gpio number is not 
> allowed, the chip will not probe.
> I have to change the driver to allow at least this -1 as an additional value.
> As Linus pointed out, it is not desirable to set the global gpio base number 
> from device tree, right ? If I have 3 instances of this chips then, how can 
> userspace sw distinguish then to which one it is talking ?

You look in sysfs to find the chip you are interested in. That is the
place to find out how dynamic numbers have been assigned.

g.