[PATCH] gpio: mcp23s08: Add option to configure pullups.

[PATCH] gpio: mcp23s08: Add option to configure pullups.

[Enric Balletbo i Serra]

Default is without pullups, but if property is specified in DT and the bit
is set, set a pullup on GPIO-n.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt | 1 +
 drivers/gpio/gpio-mcp23s08.c                             | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
index c934106..40306b1 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
@@ -57,6 +57,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 : If n-th bit is set, set a pullup on GPIO-n.
 
 Example I2C (with interrupt):
 gpiom1: gpio@20 {
diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
index 99d37b5..0cedf82 100644
--- a/drivers/gpio/gpio-mcp23s08.c
+++ b/drivers/gpio/gpio-mcp23s08.c
@@ -766,7 +766,8 @@ static int mcp230xx_probe(struct i2c_client *client,
 	if (match) {
 		pdata = &local_pdata;
 		pdata->base = -1;
-		pdata->chip[0].pullups = 0;
+		of_property_read_u32(client->dev.of_node, "microchip,pullups",
+				     &pdata->chip[0].pullups);
 		pdata->irq_controller =	of_property_read_bool(
 					client->dev.of_node,
 					"interrupt-controller");
-- 
2.1.0

Re: [PATCH] gpio: mcp23s08: Add option to configure pullups.

[Linus Walleij]

On Fri, Oct 21, 2016 at 5:00 PM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:

> Default is without pullups, but if property is specified in DT and the bit
> is set, set a pullup on GPIO-n.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

I don't generally like this.

In device tree it is the consumer that specifies how the line is used,
not the producer (gpiochip).

We currently only specify polarity, open drain and open source
for consumers though.

And the entire pin control system came into being *exactly* because
Grant didn't like me adding these things to the GPIO drivers.

So how many other things does the MCP support? Drive strength?
Schmitt trigger? Is there a datasheet?

Yours,
Linus Walleij

Re: [PATCH] gpio: mcp23s08: Add option to configure pullups.

[Alexander Stein]

On Monday 24 October 2016 02:53:31, Linus Walleij wrote:
> On Fri, Oct 21, 2016 at 5:00 PM, Enric Balletbo i Serra
> 
> <enric.balletbo@collabora.com> wrote:
> > Default is without pullups, but if property is specified in DT and the bit
> > is set, set a pullup on GPIO-n.
> > 
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> 
> I don't generally like this.
> 
> In device tree it is the consumer that specifies how the line is used,
> not the producer (gpiochip).
> 
> We currently only specify polarity, open drain and open source
> for consumers though.
> 
> And the entire pin control system came into being *exactly* because
> Grant didn't like me adding these things to the GPIO drivers.
> 
> So how many other things does the MCP support? Drive strength?
> Schmitt trigger? Is there a datasheet?

Some unsupported features (AFAIK):
* Input polarity (IPOL register)
* open-drain interrupt pin
* pullup on pins (proposed patch)

Datasheet is here: http://ww1.microchip.com/downloads/en/DeviceDoc/21919e.pdf

Best regards,
Alexander

Re: [PATCH] gpio: mcp23s08: Add option to configure pullups.

[Enric Balletbo Serra]

Hi Linus,

2016-10-24 8:13 GMT+02:00 Alexander Stein
<alexander.stein-93q1YBGzJSMe9JSWTWOYM3xStJ4P+DSV@public.gmane.org>:
> On Monday 24 October 2016 02:53:31, Linus Walleij wrote:
>> On Fri, Oct 21, 2016 at 5:00 PM, Enric Balletbo i Serra
>>
>> <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
>> > Default is without pullups, but if property is specified in DT and the bit
>> > is set, set a pullup on GPIO-n.
>> >
>> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>
>> I don't generally like this.
>>

I tried to use the same approach as used in gpio-twl4030.c because is
what I knew before. Maybe you can point me to a better approach?

>> In device tree it is the consumer that specifies how the line is used,
>> not the producer (gpiochip).
>>
>> We currently only specify polarity, open drain and open source
>> for consumers though.
>>
>> And the entire pin control system came into being *exactly* because
>> Grant didn't like me adding these things to the GPIO drivers.
>>
>> So how many other things does the MCP support? Drive strength?
>> Schmitt trigger? Is there a datasheet?
>
> Some unsupported features (AFAIK):
> * Input polarity (IPOL register)
> * open-drain interrupt pin
> * pullup on pins (proposed patch)
>
> Datasheet is here: http://ww1.microchip.com/downloads/en/DeviceDoc/21919e.pdf
>
> Best regards,
> Alexander
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gpio: mcp23s08: Add option to configure pullups.

[Linus Walleij]

On Wed, Oct 26, 2016 at 11:59 AM, Enric Balletbo Serra
<eballetbo@gmail.com> wrote:
> 2016-10-24 8:13 GMT+02:00 Alexander Stein
> <alexander.stein@systec-electronic.com>:
>> On Monday 24 October 2016 02:53:31, Linus Walleij wrote:
>>> On Fri, Oct 21, 2016 at 5:00 PM, Enric Balletbo i Serra
>>>
>>> <enric.balletbo@collabora.com> wrote:
>>> > Default is without pullups, but if property is specified in DT and the bit
>>> > is set, set a pullup on GPIO-n.
>>> >
>>> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>
>>> I don't generally like this.
>>>
>
> I tried to use the same approach as used in gpio-twl4030.c because is
> what I knew before. Maybe you can point me to a better approach?

Move the whole driver to a pin control variant:
Documentation/pinctrl.txt

We just did this with the sx150x, see
commit 9e80f9064e73f9f82679884ddf8b03ac3606cf4a
"pinctrl: Add SX150X GPIO Extender Pinctrl Driver"
in linux-next.

However that is a heavy operation. And a lot of upfront work.

If it is really *JUST* this one extra parameter, it's not a super-big
deal, but I do not like to create a plethora of custom attributes
to hack around stuff we already have generic solutions for.

And if it makes sense for clients to specify the pullup, it should
definately be done on the consumer side rather than on the
producer side as these approaches do. For that approach see
commit 69d301fdd19635a39cb2b78e53fdd625b7a27924
"gpio: add DT bindings for existing consumer flags"
commit c663e5f56737757db4d0b317c510ab505f93cecb
"gpio: support native single-ended hardware drivers"

Yours,
Linus Walleij