[PATCH v2] gpio: mcp23s08: convert driver to DT

[PATCH v2] gpio: mcp23s08: convert driver to DT

[Lars Poeschel]

From: Lars Poeschel <poeschel-Xtl8qvBWbHwb1SvskN2V4Q@public.gmane.org>

This converts the mcp23s08 driver to be able to be used with device
tree.
Explicitly allow -1 as a legal value for the
mcp23s08_platform_data->base. This is the special value lets the
kernel choose a valid global gpio base number.
There is a "mcp,chips" device tree property, that correspond to the
chips member of the struct mcp23s08_platform_data. It can be used for
multiple mcp23s08/mc23s17 on the same spi chipselect.

Signed-off-by: Lars Poeschel <poeschel-Xtl8qvBWbHwb1SvskN2V4Q@public.gmane.org>
---
v2:
- squashed booth patches together
- fixed build warning, when CONFIG_OF is not defined
- use of_match_ptr macro for of_match_table

 .../devicetree/bindings/gpio/gpio-mcp23s08.txt     |   36 ++++++++
 drivers/gpio/gpio-mcp23s08.c                       |   95 ++++++++++++++++++--
 include/linux/spi/mcp23s08.h                       |    1 +
 3 files changed, 126 insertions(+), 6 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..17eb669
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
@@ -0,0 +1,36 @@
+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" for  8 GPIO SPI version
+    - "mcp,mcp23s17" for 16 GPIO SPI version
+    - "mcp,mcp23008" for  8 GPIO I2C version or
+    - "mcp,mcp23017" for 16 GPIO I2C version of the chip
+- #gpio-cells : Should be two.
+  - first cell is the pin number
+  - second cell is used to specify optional parameters (currently unused)
+- gpio-controller : Marks the device node as a GPIO controller.
+- reg : For an address on its bus
+
+Optional device specific properties:
+- mcp,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:
+gpiom1: gpio@20 {
+        compatible = "mcp,mcp23017";
+        gpio-controller;
+        #gpio-cells = <2>;
+        reg = <0x20>;
+	chips = <
+	/* is_present  pullups */
+		1	0x07	/* chip address 0 */
+	>;
+};
diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
index 3cea0ea..ad08a5a 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,17 +475,88 @@ fail:
 
 /*----------------------------------------------------------------------*/
 
+#ifdef CONFIG_OF
+static struct of_device_id mcp23s08_of_match[] = {
+#ifdef CONFIG_SPI_MASTER
+	{
+		.compatible = "mcp,mcp23s08",
+	},
+	{
+		.compatible = "mcp,mcp23s17",
+	},
+#endif
+#if IS_ENABLED(CONFIG_I2C)
+	{
+		.compatible = "mcp,mcp23008",
+	},
+	{
+		.compatible = "mcp,mcp23017",
+	},
+#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 chips[sizeof(pdata->chip)];
+	int ret, i, j;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	pdata->base = -1;
+
+	for (i = ARRAY_SIZE(pdata->chip) * MCP23S08_CHIP_INFO_MEMBERS;
+				i > 0; i -= MCP23S08_CHIP_INFO_MEMBERS) {
+		ret = of_property_read_u32_array(np, "mcp,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 device *dev)
+{
+	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;
 
-	pdata = client->dev.platform_data;
-	if (!pdata || !gpio_is_valid(pdata->base)) {
+	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;
+
+	if ((!pdata || !gpio_is_valid(pdata->base)) && pdata->base != -1) {
 		dev_dbg(&client->dev, "invalid or missing platform data\n");
 		return -EINVAL;
 	}
@@ -531,6 +604,7 @@ static struct i2c_driver mcp230xx_driver = {
 	.driver = {
 		.name	= "mcp230xx",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(mcp23s08_of_match),
 	},
 	.probe		= mcp230xx_probe,
 	.remove		= mcp230xx_remove,
@@ -560,17 +634,25 @@ 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;
-	if (!pdata || !gpio_is_valid(pdata->base)) {
+	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)) && pdata->base != -1) {
 		dev_dbg(&spi->dev, "invalid or missing platform data\n");
 		return -EINVAL;
 	}
@@ -668,6 +750,7 @@ static struct spi_driver mcp23s08_driver = {
 	.driver = {
 		.name	= "mcp23s08",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(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


------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb

Re: [PATCH v2] gpio: mcp23s08: convert driver to DT

[Grant Likely]

On Mon, 11 Feb 2013 12:52:42 +0100, Lars Poeschel <larsi@wh2.tu-dresden.de> wrote:
> From: Lars Poeschel <poeschel@lemonage.de>
> 
> This converts the mcp23s08 driver to be able to be used with device
> tree.
> Explicitly allow -1 as a legal value for the
> mcp23s08_platform_data->base. This is the special value lets the
> kernel choose a valid global gpio base number.
> There is a "mcp,chips" device tree property, that correspond to the
> chips member of the struct mcp23s08_platform_data. It can be used for
> multiple mcp23s08/mc23s17 on the same spi chipselect.
> 
> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> ---
> v2:
> - squashed booth patches together
> - fixed build warning, when CONFIG_OF is not defined
> - use of_match_ptr macro for of_match_table
> 
>  .../devicetree/bindings/gpio/gpio-mcp23s08.txt     |   36 ++++++++
>  drivers/gpio/gpio-mcp23s08.c                       |   95 ++++++++++++++++++--
>  include/linux/spi/mcp23s08.h                       |    1 +
>  3 files changed, 126 insertions(+), 6 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..17eb669
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> @@ -0,0 +1,36 @@
> +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" for  8 GPIO SPI version
> +    - "mcp,mcp23s17" for 16 GPIO SPI version
> +    - "mcp,mcp23008" for  8 GPIO I2C version or
> +    - "mcp,mcp23017" for 16 GPIO I2C version of the chip
> +- #gpio-cells : Should be two.
> +  - first cell is the pin number
> +  - second cell is used to specify optional parameters (currently unused)
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- reg : For an address on its bus
> +
> +Optional device specific properties:
> +- mcp,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.

Since these are two separate things, I would put them into separate
properties. Perhaps something like:
 - mcp,spi-present-mask = < mask >; /* one bit per chip */
 - mcp,pullup-enable-mask = < enable-value ... >;

However, is the pullup selection per-gpio line? If so, then why not
encode it into the flags field of the gpio specifier?

> +
> +Example:
> +gpiom1: gpio@20 {
> +        compatible = "mcp,mcp23017";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        reg = <0x20>;
> +	chips = <
> +	/* is_present  pullups */
> +		1	0x07	/* chip address 0 */
> +	>;
> +};
> diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
> index 3cea0ea..ad08a5a 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,17 +475,88 @@ fail:
>  
>  /*----------------------------------------------------------------------*/
>  
> +#ifdef CONFIG_OF
> +static struct of_device_id mcp23s08_of_match[] = {
> +#ifdef CONFIG_SPI_MASTER
> +	{
> +		.compatible = "mcp,mcp23s08",
> +	},
> +	{
> +		.compatible = "mcp,mcp23s17",
> +	},
> +#endif
> +#if IS_ENABLED(CONFIG_I2C)
> +	{
> +		.compatible = "mcp,mcp23008",
> +	},
> +	{
> +		.compatible = "mcp,mcp23017",
> +	},
> +#endif
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, mcp23s08_of_match);

I don't think this is actually what you want. You should use separate
match tables; one for I2C and one for SPI.

> +
> +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 chips[sizeof(pdata->chip)];
> +	int ret, i, j;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return NULL;
> +
> +	pdata->base = -1;
> +
> +	for (i = ARRAY_SIZE(pdata->chip) * MCP23S08_CHIP_INFO_MEMBERS;
> +				i > 0; i -= MCP23S08_CHIP_INFO_MEMBERS) {
> +		ret = of_property_read_u32_array(np, "mcp,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;

Using devm is probably overkill since the pdata structure is not touched
again after probe() exits. You could instead just put the
mcp23s08_platform_data structure into the stack of the probe hook.

> +}
> +#else
> +static struct mcp23s08_platform_data *
> +		of_get_mcp23s08_pdata(struct device *dev)
> +{
> +	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;
>  
> -	pdata = client->dev.platform_data;
> -	if (!pdata || !gpio_is_valid(pdata->base)) {
> +	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;
> +
> +	if ((!pdata || !gpio_is_valid(pdata->base)) && pdata->base != -1) {
>  		dev_dbg(&client->dev, "invalid or missing platform data\n");
>  		return -EINVAL;
>  	}
> @@ -531,6 +604,7 @@ static struct i2c_driver mcp230xx_driver = {
>  	.driver = {
>  		.name	= "mcp230xx",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(mcp23s08_of_match),
>  	},
>  	.probe		= mcp230xx_probe,
>  	.remove		= mcp230xx_remove,
> @@ -560,17 +634,25 @@ 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;
> -	if (!pdata || !gpio_is_valid(pdata->base)) {
> +	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)) && pdata->base != -1) {
>  		dev_dbg(&spi->dev, "invalid or missing platform data\n");
>  		return -EINVAL;
>  	}
> @@ -668,6 +750,7 @@ static struct spi_driver mcp23s08_driver = {
>  	.driver = {
>  		.name	= "mcp23s08",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(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
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH v2] gpio: mcp23s08: convert driver to DT

[Lars Poeschel]

On Monday 11 February 2013 at 22:25:51, Grant Likely wrote:
> On Mon, 11 Feb 2013 12:52:42 +0100, Lars Poeschel <larsi-myOXECIRRCL4ajHJ1XSv27NAH6kLmebB@public.gmane.org> 
wrote:

> > +Optional device specific properties:
> > +- mcp,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.
> 
> Since these are two separate things, I would put them into separate
> properties. Perhaps something like:
>  - mcp,spi-present-mask = < mask >; /* one bit per chip */
>  - mcp,pullup-enable-mask = < enable-value ... >;
> 
> However, is the pullup selection per-gpio line? If so, then why not
> encode it into the flags field of the gpio specifier?

Yes, the pullup is per-gpio line. I am working on that. It turns out, that 
this is a bit difficult for me, as there is no real documentation and no 
other driver is doing it or something similar yet. Exception are very few 
gpio soc drivers where situation is a bit different. They seem to rely an 
fixed global gpio numbers and they are always memory mapped.
But as I said I am working on it...

> > diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
> > index 3cea0ea..ad08a5a 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,17 +475,88 @@ fail:
> >  /*---------------------------------------------------------------------
> >  -*/
> > 
> > +#ifdef CONFIG_OF
> > +static struct of_device_id mcp23s08_of_match[] = {
> > +#ifdef CONFIG_SPI_MASTER
> > +	{
> > +		.compatible = "mcp,mcp23s08",
> > +	},
> > +	{
> > +		.compatible = "mcp,mcp23s17",
> > +	},
> > +#endif
> > +#if IS_ENABLED(CONFIG_I2C)
> > +	{
> > +		.compatible = "mcp,mcp23008",
> > +	},
> > +	{
> > +		.compatible = "mcp,mcp23017",
> > +	},
> > +#endif
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mcp23s08_of_match);
> 
> I don't think this is actually what you want. You should use separate
> match tables; one for I2C and one for SPI.

I am working on that.

> > +
> > +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 chips[sizeof(pdata->chip)];
> > +	int ret, i, j;
> > +
> > +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > +	if (!pdata)
> > +		return NULL;
> > +
> > +	pdata->base = -1;
> > +
> > +	for (i = ARRAY_SIZE(pdata->chip) * MCP23S08_CHIP_INFO_MEMBERS;
> > +				i > 0; i -= MCP23S08_CHIP_INFO_MEMBERS) {
> > +		ret = of_property_read_u32_array(np, "mcp,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;
> 
> Using devm is probably overkill since the pdata structure is not touched
> again after probe() exits. You could instead just put the
> mcp23s08_platform_data structure into the stack of the probe hook.

I wanted to keep the impact for the driver itself a minimum. But this is the 
better solution. Working on that too, ...


Thanks for your review,
Lars

------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb

Re: [PATCH v2] gpio: mcp23s08: convert driver to DT

[Linus Walleij]

On Wed, Feb 13, 2013 at 12:13 PM, Lars Poeschel <poeschel@lemonage.de> wrote:
> On Monday 11 February 2013 at 22:25:51, Grant Likely wrote:
>>
>> However, is the pullup selection per-gpio line? If so, then why not
>> encode it into the flags field of the gpio specifier?
>
> Yes, the pullup is per-gpio line. I am working on that. It turns out, that
> this is a bit difficult for me, as there is no real documentation and no
> other driver is doing it or something similar yet. Exception are very few
> gpio soc drivers where situation is a bit different. They seem to rely an
> fixed global gpio numbers and they are always memory mapped.
> But as I said I am working on it...

Part of your problem is that pull-up is pin control territory.

We invented that subsystem for a reason, and that reason
was that the GPIO subsystem had a hard time accomodating
things like this.

If you look in drivers/pinctrl/pinctrl-abx500.c which is my
latest submitted pinctrl driver you can see that this is basically
a quite simple GPIO chip, we just model it as a pin controller
with a GPIO front-end too exactly because it can do things
like multiplexing, pull-up and pull-down.

Have you considered this approach?

Yours,
Linus Walleij

Re: [PATCH v2] gpio: mcp23s08: convert driver to DT

[Lars Poeschel]

On Wednesday 13 February 2013 at 13:51:12, Linus Walleij wrote:
> On Wed, Feb 13, 2013 at 12:13 PM, Lars Poeschel <poeschel-Xtl8qvBWbHwb1SvskN2V4Q@public.gmane.org> 
wrote:
> > On Monday 11 February 2013 at 22:25:51, Grant Likely wrote:
> >> However, is the pullup selection per-gpio line? If so, then why not
> >> encode it into the flags field of the gpio specifier?
> > 
> > Yes, the pullup is per-gpio line. I am working on that. It turns out,
> > that this is a bit difficult for me, as there is no real documentation
> > and no other driver is doing it or something similar yet. Exception are
> > very few gpio soc drivers where situation is a bit different. They seem
> > to rely an fixed global gpio numbers and they are always memory mapped.
> > But as I said I am working on it...
> 
> Part of your problem is that pull-up is pin control territory.
> 
> We invented that subsystem for a reason, and that reason
> was that the GPIO subsystem had a hard time accomodating
> things like this.
> 
> If you look in drivers/pinctrl/pinctrl-abx500.c which is my
> latest submitted pinctrl driver you can see that this is basically
> a quite simple GPIO chip, we just model it as a pin controller
> with a GPIO front-end too exactly because it can do things
> like multiplexing, pull-up and pull-down.
> 
> Have you considered this approach?

No, I haven't. And although this doesn't solve all my problems, I like the 
idea very much! Thank you for this! But at the moment it looks to me that 
this could be a bit overkill for setting this single register and I don't 
think this is, what Grant meant me to do. 
Anyway, I will have a deeper look at this.

Thanks,
Lars

------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb

Re: [PATCH v2] gpio: mcp23s08: convert driver to DT

[Linus Walleij]

On Thu, Feb 14, 2013 at 1:22 PM, Lars Poeschel <poeschel@lemonage.de> wrote:
> On Wednesday 13 February 2013 at 13:51:12, Linus Walleij wrote:

>> Have you considered this [pinctrl] approach?
>
> No, I haven't. And although this doesn't solve all my problems, I like the
> idea very much! Thank you for this! But at the moment it looks to me that
> this could be a bit overkill for setting this single register and I don't
> think this is, what Grant meant me to do.

So there is this corner case where some GPIO driver does some
pinctrl business, I mean, really really little of it. And then it may be
overkill. Sometimes though, the hardware actually can do a lot of
this biasing and muxing it just wasn't part of the driver yet or done
in e.g. boot loaders, and then it's usually better to use pinctrl.

Adding custom APIs to the GPIO drivers will however always
cause a maintenance burden on the GPIO maintainers, so that's
why pinctrl & GPIO is nice.

I don't know which case this would be though.

Yours,
Linus Walleij