[PATCH] gpio: mcp23s08: support setting pullups from device tree data

[PATCH] gpio: mcp23s08: support setting pullups from device tree data

[Timo Teräs]

Make pullup configurable from device tree data.

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
I'm not fully certain how the pullup data should be made configurable via OF.
gpio-twl4030.c uses similar approach as this, but gpio-samsung.c makes the
pullup configuration seems to be part of the custom data in gpio specifiers.

 Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt |  1 +
 drivers/gpio/gpio-mcp23s08.c                             | 14 +++++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
index f3332b9..c7d2128 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
@@ -59,6 +59,7 @@ Optional device specific properties:
         On devices with only one interrupt output this property is useless.
 - microchip,irq-active-high: Sets the INTPOL flag in the IOCON register. This
         configures the IRQ output polarity as active high.
+- microchip,pullups: Configuration value for GPPU register.
 
 Example I2C (with interrupt):
 gpiom1: gpio@20 {
diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
index 73db7ec..2320230 100644
--- a/drivers/gpio/gpio-mcp23s08.c
+++ b/drivers/gpio/gpio-mcp23s08.c
@@ -786,9 +786,12 @@ static int mcp230xx_probe(struct i2c_client *client,
 	match = of_match_device(of_match_ptr(mcp23s08_i2c_of_match),
 					&client->dev);
 	if (match) {
+		u32 pullups = 0;
 		pdata = &local_pdata;
 		pdata->base = -1;
-		pdata->chip[0].pullups = 0;
+		of_property_read_u32(client->dev.of_node, "microchip,pullups",
+				     &pullups);
+		pdata->chip[0].pullups = pullups;
 		pdata->irq_controller =	of_property_read_bool(
 					client->dev.of_node,
 					"interrupt-controller");
@@ -910,9 +913,14 @@ static int mcp23s08_probe(struct spi_device *spi)
 		pdata = &local_pdata;
 		pdata->base = -1;
 		for (addr = 0; addr < ARRAY_SIZE(pdata->chip); addr++) {
-			pdata->chip[addr].pullups = 0;
-			if (spi_present_mask & (1 << addr))
+			u32 pullups = 0;
+			if (spi_present_mask & (1 << addr)) {
+				of_property_read_u32_index(spi->dev.of_node,
+							   "microchip,pullups",
+							   addr, &pullups);
 				chips++;
+			}
+			pdata->chip[addr].pullups = pullups;
 		}
 		pdata->irq_controller =	of_property_read_bool(
 					spi->dev.of_node,
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gpio: mcp23s08: support setting pullups from device tree data

[Alexander Stein]

On Friday 04 December 2015 13:46:13, Timo Teräs wrote:
> Make pullup configurable from device tree data.
> 
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> ---
> I'm not fully certain how the pullup data should be made configurable via OF.
> gpio-twl4030.c uses similar approach as this, but gpio-samsung.c makes the
> pullup configuration seems to be part of the custom data in gpio specifiers.

I think this should be specified for each individual GPIO. e.g.
> gpios = <&gpio-ctrl 0 GPIO_ACTIVE_HIGH GPIO_PULLUP>;

You don't know (yet) which pins might need a internal pull-up when specifying the mcp23s08 node itself.
You might override the controller node in your dts, but so you need to adjust 2 entries, one for the user and one for the pullup, rather than have all options at one place.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:    +49 (0) 3765 38600-4100
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gpio: mcp23s08: support setting pullups from device tree data

[Timo Teras]

On Mon, 07 Dec 2015 07:53:08 +0100
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> On Friday 04 December 2015 13:46:13, Timo Teräs wrote:
> > Make pullup configurable from device tree data.
> > 
> > Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> > ---
> > I'm not fully certain how the pullup data should be made
> > configurable via OF. gpio-twl4030.c uses similar approach as this,
> > but gpio-samsung.c makes the pullup configuration seems to be part
> > of the custom data in gpio specifiers.  
> 
> I think this should be specified for each individual GPIO. e.g.
> > gpios = <&gpio-ctrl 0 GPIO_ACTIVE_HIGH GPIO_PULLUP>;  
> 
> You don't know (yet) which pins might need a internal pull-up when
> specifying the mcp23s08 node itself. You might override the
> controller node in your dts, but so you need to adjust 2 entries, one
> for the user and one for the pullup, rather than have all options at
> one place.

I think this is debatable. So consider this as opening a bit wider
discussion about the preferred way of exposing pullup/down configuration
via OF. And hopefully documenting it. (As mentioned there's already
code in for either way.)

Agreeably one might want to override it on PIN basis. Perhaps the GPIO
function can be changed or similar.

The other side is: Why would my gpio-keys input mapping specification
need know to what kind of GPIO input it is connected to? What if I
switch the GPIO expander - or the hardware schema?  I need to edit 10
different places rather than one - places that are about the high-level
functionality, not the hardware. Normally the pullup settings are
hardware layout dependant, so GPIO configuration would be the logical
place.

One generally wants to configure pullups correctly for all GPIOs
regardless of if they are connected or not; or if the relevant
high-level gpio driver is loaded or not. And this should be done as
early as possible - even before the driver for that specific GPIO pin
functionality is loaded.

This seems to be the way most platform data things work for different
GPIOs.

Perphaps others have more arguments for the per-pin configuration?
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gpio: mcp23s08: support setting pullups from device tree data

[Alexander Stein]

On Monday 07 December 2015 09:22:59, Timo Teras wrote:
> On Mon, 07 Dec 2015 07:53:08 +0100
> Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> 
> > On Friday 04 December 2015 13:46:13, Timo Teräs wrote:
> > > Make pullup configurable from device tree data.
> > > 
> > > Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> > > ---
> > > I'm not fully certain how the pullup data should be made
> > > configurable via OF. gpio-twl4030.c uses similar approach as this,
> > > but gpio-samsung.c makes the pullup configuration seems to be part
> > > of the custom data in gpio specifiers.  
> > 
> > I think this should be specified for each individual GPIO. e.g.
> > > gpios = <&gpio-ctrl 0 GPIO_ACTIVE_HIGH GPIO_PULLUP>;  
> > 
> > You don't know (yet) which pins might need a internal pull-up when
> > specifying the mcp23s08 node itself. You might override the
> > controller node in your dts, but so you need to adjust 2 entries, one
> > for the user and one for the pullup, rather than have all options at
> > one place.
> 
> I think this is debatable. So consider this as opening a bit wider
> discussion about the preferred way of exposing pullup/down configuration
> via OF. And hopefully documenting it. (As mentioned there's already
> code in for either way.)
> 
> Agreeably one might want to override it on PIN basis. Perhaps the GPIO
> function can be changed or similar.

I think for this you need only to add an additional cell entry or just add an additional mask bit like active low/high.
This would add the pull up/down feature to each GPIO user.

> The other side is: Why would my gpio-keys input mapping specification
> need know to what kind of GPIO input it is connected to? What if I
> switch the GPIO expander - or the hardware schema?  I need to edit 10
> different places rather than one - places that are about the high-level
> functionality, not the hardware. Normally the pullup settings are
> hardware layout dependant, so GPIO configuration would be the logical
> place.

gpio-keys need to know this when selecting active low or active high. You usually need the corresponding pull up/down for this to work.

> One generally wants to configure pullups correctly for all GPIOs
> regardless of if they are connected or not; or if the relevant
> high-level gpio driver is loaded or not. And this should be done as
> early as possible - even before the driver for that specific GPIO pin
> functionality is loaded.

This is also a valid point.

> Perphaps others have more arguments for the per-pin configuration?

I get the impression both ways would be needed... which is kinda error-prone.

Best regards,
Alexander
-- 
-----
Please note our closure period for Christmas vacation and turn of the year
We are closed from 21st December 2015 to 3rd January 2016
-----
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:    +49 (0) 3765 38600-4100
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gpio: mcp23s08: support setting pullups from device tree data

[Timo Teras]

On Tue, 15 Dec 2015 09:09:04 +0100
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> On Monday 07 December 2015 09:22:59, Timo Teras wrote:
> > On Mon, 07 Dec 2015 07:53:08 +0100
> > Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> >   
> > The other side is: Why would my gpio-keys input mapping
> > specification need know to what kind of GPIO input it is connected
> > to? What if I switch the GPIO expander - or the hardware schema?  I
> > need to edit 10 different places rather than one - places that are
> > about the high-level functionality, not the hardware. Normally the
> > pullup settings are hardware layout dependant, so GPIO
> > configuration would be the logical place.  
> 
> gpio-keys need to know this when selecting active low or active high.
> You usually need the corresponding pull up/down for this to work.

To me it looks active_low is just software invert. My understanding is
that pullups are practically always fixed configuration depending on how
the connection is physically wired. The only exception being that you
have development board where the physical wiring is subject to change.

> > One generally wants to configure pullups correctly for all GPIOs
> > regardless of if they are connected or not; or if the relevant
> > high-level gpio driver is loaded or not. And this should be done as
> > early as possible - even before the driver for that specific GPIO
> > pin functionality is loaded.  
> 
> This is also a valid point.
> 
> > Perphaps others have more arguments for the per-pin configuration?  
> 
> I get the impression both ways would be needed... which is kinda
> error-prone.

But yeah. If there's valid use cases for pullups to be changed, it
would justify both ways. But if needing to choose, I would prefer
specifying pullup register data in one place. Thus I chose that way
for the patch.

Perhaps this patch could be considered applied as-is then? Or is there
wishes to unify the OF property name?

Thanks,
Timo