[PATCH v4] gpio: pcf857x: Add OF support

[PATCH v4] gpio: pcf857x: Add OF support

[Laurent Pinchart]

Add DT bindings for the pcf857x-compatible chips and parse the device
tree node in the driver.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../devicetree/bindings/gpio/gpio-pcf857x.txt      | 71 ++++++++++++++++++++++
 drivers/gpio/gpio-pcf857x.c                        | 54 +++++++++++++---
 2 files changed, 116 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt

Changes since v3:

- Get rid of the #ifdef CONFIG_OF in the probe function
- Give DT node priority over platform data

Changes since v2

- Replace mention about interrupts software configuration in DT bindings
  documentation with an explanation of the hardware configuration.

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
new file mode 100644
index 0000000..d261391
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
@@ -0,0 +1,71 @@
+* PCF857x-compatible I/O expanders
+
+The PCF857x-compatible chips have "quasi-bidirectional" I/O pins that can be
+driven high by a pull-up current source or driven low to ground. This combines
+the direction and output level into a single bit per pin, which can't be read
+back. We can't actually know at initialization time whether a pin is configured
+(a) as output and driving the signal low/high, or (b) as input and reporting a
+low/high value, without knowing the last value written since the chip came out
+of reset (if any). The only reliable solution for setting up pin direction is
+thus to do it explicitly.
+
+Required Properties:
+
+  - compatible: should be one of the following.
+    - "maxim,max7328": For the Maxim MAX7378
+    - "maxim,max7329": For the Maxim MAX7329
+    - "nxp,pca8574": For the NXP PCA8574
+    - "nxp,pca8575": For the NXP PCA8575
+    - "nxp,pca9670": For the NXP PCA9670
+    - "nxp,pca9671": For the NXP PCA9671
+    - "nxp,pca9672": For the NXP PCA9672
+    - "nxp,pca9673": For the NXP PCA9673
+    - "nxp,pca9674": For the NXP PCA9674
+    - "nxp,pca9675": For the NXP PCA9675
+    - "nxp,pcf8574": For the NXP PCF8574
+    - "nxp,pcf8574a": For the NXP PCF8574A
+    - "nxp,pcf8575": For the NXP PCF8575
+    - "ti,tca9554": For the TI TCA9554
+
+  - reg: I2C slave address.
+
+  - gpio-controller: Marks the device node as a gpio controller.
+  - #gpio-cells: Should be 2. The first cell is the GPIO number and the second
+    cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. Only the
+    GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
+
+Optional Properties:
+
+  - pins-initial-state: Bitmask that specifies the initial state of each pin.
+  When a bit is set to zero, the corresponding pin will be initialized to the
+  input (pulled-up) state. When the  bit is set to one, the pin will be
+  initialized the the low-level output state. If the property is not specified
+  all pins will be initialized to the input state.
+
+  The I/O expander can detect input state changes, and thus optionally act as
+  an interrupt controller. When the expander interrupt pin is connected all the
+  following properties must be set. For more information please see the
+  interrupt controller device tree bindings documentation available at
+  Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.
+
+  - interrupt-controller: Identifies the node as an interrupt controller.
+  - #interrupt-cells: Number of cells to encode an interrupt source, shall be 2.
+  - interrupt-parent: phandle of the parent interrupt controller.
+  - interrupts: Interrupt specifier for the controllers interrupt.
+
+
+Please refer to gpio.txt in this directory for details of the common GPIO
+bindings used by client devices.
+
+Example: PCF8575 I/O expander node
+
+	pcf8575: gpio@20 {
+		compatible = "nxp,pcf8575";
+		reg = <0x20>;
+		interrupt-parent = <&irqpin2>;
+		interrupts = <3 0>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index 9e61bb0..f652bb4 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -26,6 +26,8 @@
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
@@ -50,6 +52,27 @@ static const struct i2c_device_id pcf857x_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, pcf857x_id);
 
+#ifdef CONFIG_OF
+static const struct of_device_id pcf857x_of_table[] = {
+	{ .compatible = "nxp,pcf8574", .data = (void *)8 },
+	{ .compatible = "nxp,pcf8574a", .data = (void *)8 },
+	{ .compatible = "nxp,pca8574", .data = (void *)8 },
+	{ .compatible = "nxp,pca9670", .data = (void *)8 },
+	{ .compatible = "nxp,pca9672", .data = (void *)8 },
+	{ .compatible = "nxp,pca9674", .data = (void *)8 },
+	{ .compatible = "nxp,pcf8575", .data = (void *)16 },
+	{ .compatible = "nxp,pca8575", .data = (void *)16 },
+	{ .compatible = "nxp,pca9671", .data = (void *)16 },
+	{ .compatible = "nxp,pca9673", .data = (void *)16 },
+	{ .compatible = "nxp,pca9675", .data = (void *)16 },
+	{ .compatible = "maxim,max7328", .data = (void *)8 },
+	{ .compatible = "maxim,max7329", .data = (void *)8 },
+	{ .compatible = "ti,tca9554", .data = (void *)8 },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pcf857x_of_table);
+#endif
+
 /*
  * The pcf857x, pca857x, and pca967x chips only expose one read and one
  * write register.  Writing a "one" bit (to match the reset state) lets
@@ -257,14 +280,26 @@ fail:
 static int pcf857x_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
-	struct pcf857x_platform_data	*pdata;
+	struct pcf857x_platform_data	*pdata = dev_get_platdata(&client->dev);
+	struct device_node		*np = client->dev.of_node;
+	const struct  of_device_id	*of_id;
 	struct pcf857x			*gpio;
+	unsigned int			n_latch = 0;
+	unsigned int			ngpio;
 	int				status;
 
-	pdata = dev_get_platdata(&client->dev);
-	if (!pdata) {
+	of_id = of_match_device(of_match_ptr(pcf857x_of_table), &client->dev);
+	if (of_id)
+		ngpio = (unsigned int)of_id->data;
+	else
+		ngpio = id->driver_data;
+
+	if (IS_ENABLED(CONFIG_OF) && np)
+		of_property_read_u32(np, "pins-initial-state", &n_latch);
+	else if (pdata)
+		n_latch = pdata->n_latch;
+	else
 		dev_dbg(&client->dev, "no platform data\n");
-	}
 
 	/* Allocate, initialize, and register this gpio_chip. */
 	gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
@@ -282,7 +317,7 @@ static int pcf857x_probe(struct i2c_client *client,
 	gpio->chip.set			= pcf857x_set;
 	gpio->chip.direction_input	= pcf857x_input;
 	gpio->chip.direction_output	= pcf857x_output;
-	gpio->chip.ngpio		= id->driver_data;
+	gpio->chip.ngpio		= ngpio;
 
 	/* enable gpio_to_irq() if platform has settings */
 	if (client->irq) {
@@ -357,11 +392,11 @@ static int pcf857x_probe(struct i2c_client *client,
 	 * may cause transient glitching since it can't know the last value
 	 * written (some pins may need to be driven low).
 	 *
-	 * Using pdata->n_latch avoids that trouble.  When left initialized
-	 * to zero, our software copy of the "latch" then matches the chip's
-	 * all-ones reset state.  Otherwise it flags pins to be driven low.
+	 * Using n_latch avoids that trouble.  When left initialized to zero,
+	 * our software copy of the "latch" then matches the chip's all-ones
+	 * reset state.  Otherwise it flags pins to be driven low.
 	 */
-	gpio->out = pdata ? ~pdata->n_latch : ~0;
+	gpio->out = ~n_latch;
 	gpio->status = gpio->out;
 
 	status = gpiochip_add(&gpio->chip);
@@ -423,6 +458,7 @@ static struct i2c_driver pcf857x_driver = {
 	.driver = {
 		.name	= "pcf857x",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(pcf857x_of_table),
 	},
 	.probe	= pcf857x_probe,
 	.remove	= pcf857x_remove,
-- 
Regards,

Laurent Pinchart

Re: [PATCH v4] gpio: pcf857x: Add OF support

[Linus Walleij]

On Mon, Aug 26, 2013 at 3:45 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:

> Add DT bindings for the pcf857x-compatible chips and parse the device
> tree node in the driver.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

First: can I get an ACK from some DT-bindings maintainer?

I think you may need to CC them all individually to get some
response.

> +++ b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> @@ -0,0 +1,71 @@
> +* PCF857x-compatible I/O expanders
> +
> +The PCF857x-compatible chips have "quasi-bidirectional" I/O pins that can be
> +driven high by a pull-up current source or driven low to ground. This combines
> +the direction and output level into a single bit per pin, which can't be read
> +back. We can't actually know at initialization time whether a pin is configured
> +(a) as output and driving the signal low/high, or (b) as input and reporting a
> +low/high value, without knowing the last value written since the chip came out
> +of reset (if any). The only reliable solution for setting up pin direction is
> +thus to do it explicitly.

Nitpick: I prefer that wrt gpio we talk about "lines" rather than "pins"
to separate it from the pin control concept. Just
s/pin/line/g

(...)
> +Optional Properties:
> +
> +  - pins-initial-state: Bitmask that specifies the initial state of each pin.
> +  When a bit is set to zero, the corresponding pin will be initialized to the
> +  input (pulled-up) state. When the  bit is set to one, the pin will be
> +  initialized the the low-level output state. If the property is not specified
> +  all pins will be initialized to the input state.

Name this lines-initial-states (pluralis).

Don't we want to do this generic if we shall do it?

Like for *any* GPIO chips we provide lines-initial state in the device
tree and some code in the gpiochip with a callback in struct gpio_chip
that can be called by the gpiolib core to set this up? Then we don't
have to reimplement this for every GPIO controller that needs it.

Sorry for not noticing this earlier...

Yours,
Linus Walleij

Re: [PATCH v4] gpio: pcf857x: Add OF support

[Laurent Pinchart]

Hi Linus,

On Thursday 29 August 2013 14:16:59 Linus Walleij wrote:
> On Mon, Aug 26, 2013 at 3:45 PM, Laurent Pinchart wrote:
> > Add DT bindings for the pcf857x-compatible chips and parse the device
> > tree node in the driver.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> First: can I get an ACK from some DT-bindings maintainer?
> 
> I think you may need to CC them all individually to get some response.

I'll make sure to get an ACK for v6 (or a more recent version). Reviewers are 
probably busy with the merge window about to open, delaying this patch to 
v3.13 should help.

I'll post v6 after receiving your answer to the comment below, as well as to 
the issue raised by Tomasz on v5.

> > +++ b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > @@ -0,0 +1,71 @@
> > +* PCF857x-compatible I/O expanders
> > +
> > +The PCF857x-compatible chips have "quasi-bidirectional" I/O pins that can
> > be +driven high by a pull-up current source or driven low to ground. This
> > combines +the direction and output level into a single bit per pin, which
> > can't be read +back. We can't actually know at initialization time
> > whether a pin is configured +(a) as output and driving the signal
> > low/high, or (b) as input and reporting a +low/high value, without
> > knowing the last value written since the chip came out +of reset (if
> > any). The only reliable solution for setting up pin direction is +thus to
> > do it explicitly.
> 
> Nitpick: I prefer that wrt gpio we talk about "lines" rather than "pins"
> to separate it from the pin control concept. Just
> s/pin/line/g

OK.

> (...)
> 
> > +Optional Properties:
> > +
> > +  - pins-initial-state: Bitmask that specifies the initial state of each
> > pin. +  When a bit is set to zero, the corresponding pin will be
> > initialized to the +  input (pulled-up) state. When the  bit is set to
> > one, the pin will be +  initialized the the low-level output state. If
> > the property is not specified +  all pins will be initialized to the
> > input state.
> 
> Name this lines-initial-states (pluralis).

OK.

> Don't we want to do this generic if we shall do it?
> 
> Like for *any* GPIO chips we provide lines-initial state in the device
> tree and some code in the gpiochip with a callback in struct gpio_chip
> that can be called by the gpiolib core to set this up? Then we don't
> have to reimplement this for every GPIO controller that needs it.

Most GPIO chips will provide a way to read back the current state. The initial 
state only needs to be provided for write-only chips. This is (luckily) rather 
an exception, so I don't think we should implement it in the core, at least 
not yet. We can always refactor the code later if needed, the proposed DT 
binding is generic enough.

> Sorry for not noticing this earlier...

No worries.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4] gpio: pcf857x: Add OF support

[Linus Walleij]

On Fri, Aug 30, 2013 at 2:05 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thursday 29 August 2013 14:16:59 Linus Walleij wrote:
>> On Mon, Aug 26, 2013 at 3:45 PM, Laurent Pinchart wrote:

>> Don't we want to do this generic if we shall do it?
>>
>> Like for *any* GPIO chips we provide lines-initial state in the device
>> tree and some code in the gpiochip with a callback in struct gpio_chip
>> that can be called by the gpiolib core to set this up? Then we don't
>> have to reimplement this for every GPIO controller that needs it.
>
> Most GPIO chips will provide a way to read back the current state. The initial
> state only needs to be provided for write-only chips. This is (luckily) rather
> an exception, so I don't think we should implement it in the core, at least
> not yet. We can always refactor the code later if needed, the proposed DT
> binding is generic enough.

But I think this can be useful on any GPIO chip.

For someone deploying some system and hacking around in the
device tree to set the GPIOs up properly at boot it can be a
real useful tool.

Or is that giving them too much rope? :-D

Yours,
Linus Walleij

Re: [PATCH v4] gpio: pcf857x: Add OF support

[Laurent Pinchart]

On Friday 30 August 2013 10:07:23 Linus Walleij wrote:
> On Fri, Aug 30, 2013 at 2:05 AM, Laurent Pinchart wrote:
> > On Thursday 29 August 2013 14:16:59 Linus Walleij wrote:
> >> On Mon, Aug 26, 2013 at 3:45 PM, Laurent Pinchart wrote:
> >> 
> >> Don't we want to do this generic if we shall do it?
> >> 
> >> Like for *any* GPIO chips we provide lines-initial state in the device
> >> tree and some code in the gpiochip with a callback in struct gpio_chip
> >> that can be called by the gpiolib core to set this up? Then we don't
> >> have to reimplement this for every GPIO controller that needs it.
> > 
> > Most GPIO chips will provide a way to read back the current state. The
> > initial state only needs to be provided for write-only chips. This is
> > (luckily) rather an exception, so I don't think we should implement it in
> > the core, at least not yet. We can always refactor the code later if
> > needed, the proposed DT binding is generic enough.
> 
> But I think this can be useful on any GPIO chip.
> 
> For someone deploying some system and hacking around in the
> device tree to set the GPIOs up properly at boot it can be a
> real useful tool.
> 
> Or is that giving them too much rope? :-D

That's the job of the boot loader, isn't it ? :-)

-- 
Regards,

Laurent Pinchart