[PATCH v2 0/3] pcf857x: Add OF support

[PATCH v2 0/3] pcf857x: Add OF support

[Laurent Pinchart]

Hello,

Here's a small series of patches that add OF support to the pcf857x driver.
The first two patches prepare the driver for the third one that introduces DT
bindings and parses the device tree node.

Changes since v1:

- Add GPIO properties to the pcf8575 DT node in the bindings documentation

Laurent Pinchart (3):
  gpio: pcf857x: Sort headers alphabetically
  gpio: pcf857x: Remove pdata argument to pcf857x_irq_domain_init()
  gpio: pcf857x: Add OF support

 .../devicetree/bindings/gpio/gpio-pcf857x.txt      | 71 ++++++++++++++++++++++
 drivers/gpio/gpio-pcf857x.c                        | 70 ++++++++++++++++-----
 2 files changed, 125 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt

-- 
Regards,

Laurent Pinchart

[PATCH v2 1/3] gpio: pcf857x: Sort headers alphabetically

[Laurent Pinchart]

This makes checking for duplicates when adding a new #include easier.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpio/gpio-pcf857x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index e8faf53..ea9f3580 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -18,15 +18,15 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
-#include <linux/kernel.h>
-#include <linux/slab.h>
 #include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/i2c/pcf857x.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 
-- 
1.8.1.5

[PATCH v2 2/3] gpio: pcf857x: Remove pdata argument to pcf857x_irq_domain_init()

[Laurent Pinchart]

The argument is not used, remove it. No board registers a pcf857x device
with an IRQ without specifying platform data, IRQ domain registration
behaviour is thus not affected by this change.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpio/gpio-pcf857x.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index ea9f3580..070e81f 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -223,7 +223,6 @@ static void pcf857x_irq_domain_cleanup(struct pcf857x *gpio)
 }
 
 static int pcf857x_irq_domain_init(struct pcf857x *gpio,
-				   struct pcf857x_platform_data *pdata,
 				   struct i2c_client *client)
 {
 	int status;
@@ -286,8 +285,8 @@ static int pcf857x_probe(struct i2c_client *client,
 	gpio->chip.ngpio		= id->driver_data;
 
 	/* enable gpio_to_irq() if platform has settings */
-	if (pdata && client->irq) {
-		status = pcf857x_irq_domain_init(gpio, pdata, client);
+	if (client->irq) {
+		status = pcf857x_irq_domain_init(gpio, client);
 		if (status < 0) {
 			dev_err(&client->dev, "irq_domain init failed\n");
 			goto fail;
@@ -388,7 +387,7 @@ fail:
 	dev_dbg(&client->dev, "probe error %d for '%s'\n",
 			status, client->name);
 
-	if (pdata && client->irq)
+	if (client->irq)
 		pcf857x_irq_domain_cleanup(gpio);
 
 	return status;
@@ -411,7 +410,7 @@ static int pcf857x_remove(struct i2c_client *client)
 		}
 	}
 
-	if (pdata && client->irq)
+	if (client->irq)
 		pcf857x_irq_domain_cleanup(gpio);
 
 	status = gpiochip_remove(&gpio->chip);
-- 
1.8.1.5

[PATCH v2 3/3] 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                        | 57 ++++++++++++++---
 2 files changed, 119 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
new file mode 100644
index 0000000..df94462
--- /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 interrupts support is desired 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 070e81f..50a90f1 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,29 @@ 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 = client->dev.platform_data;
+	struct device_node		*np = client->dev.of_node;
 	struct pcf857x			*gpio;
+	unsigned int			n_latch = 0;
+	unsigned int			ngpio;
 	int				status;
 
-	pdata = client->dev.platform_data;
-	if (!pdata) {
+#ifdef CONFIG_OF
+	if (np) {
+		const struct of_device_id *of_id;
+
+		of_id = of_match_device(pcf857x_of_table, &client->dev);
+		ngpio = (unsigned int)of_id->data;
+	} else
+#endif
+		ngpio = id->driver_data;
+
+	if (pdata)
+		n_latch = pdata->n_latch;
+	else if (IS_ENABLED(CONFIG_OF) && np)
+		of_property_read_u32(np, "pins-initial-state", &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 +320,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 +395,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 +461,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,
-- 
1.8.1.5

Re: [PATCH v2 1/3] gpio: pcf857x: Sort headers alphabetically

[Linus Walleij]

On Tue, Aug 20, 2013 at 1:04 AM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:

> This makes checking for duplicates when adding a new #include easier.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

And I'm dreaming of patching all the GPIO drivers to have
the header includes in inverted christmas tree order. ;-)

Okay, applied anyway.

Yours,
Linus Walleij

Re: [PATCH v2 2/3] gpio: pcf857x: Remove pdata argument to pcf857x_irq_domain_init()

[Linus Walleij]

On Tue, Aug 20, 2013 at 1:04 AM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:

> The argument is not used, remove it. No board registers a pcf857x device
> with an IRQ without specifying platform data, IRQ domain registration
> behaviour is thus not affected by this change.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Patch applied, unless Kuninori has some objections.

Yours,
Linus Walleij

Re: [PATCH v2 3/3] gpio: pcf857x: Add OF support

[Linus Walleij]

On Tue, Aug 20, 2013 at 1:04 AM, 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>

I'm holding this until I get some reaction from the device tree reviewers,
or until I get unpatient, whichever comes first. It looks good to me.

Yours,
Linus Walleij

Re: [PATCH v2 3/3] gpio: pcf857x: Add OF support

[Laurent Pinchart]

Hi Linus,

On Friday 23 August 2013 19:54:03 Linus Walleij wrote:
> On Tue, Aug 20, 2013 at 1:04 AM, 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>
> 
> I'm holding this until I get some reaction from the device tree reviewers,
> or until I get unpatient, whichever comes first.

Will you get unpatient before the end of the v3.12 merge window ? ;-)

> It looks good to me.

Thanks.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] gpio: pcf857x: Add OF support

[Tomasz Figa]

Hi Laurent,

On Tuesday 20 of August 2013 01:04:54 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> ---
>  .../devicetree/bindings/gpio/gpio-pcf857x.txt      | 71
> ++++++++++++++++++++++ drivers/gpio/gpio-pcf857x.c                     
>   | 57 ++++++++++++++--- 2 files changed, 119 insertions(+), 9
> deletions(-)
>  create mode 100644
> Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt new file mode
> 100644
> index 0000000..df94462
> --- /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.

Hmm, do you actually need to know whether those pins are outputs or inputs 
before they get used for first time? I believe any driver using GPIO will 
call gpio_direction_{in,out}put() before it starts using the pin, which 
will initialize the pin to a known state.

What I'd suggest is making the driver handle this by having a bit mask 
that marks states of pins as defined and flagging all pins as undefined by 
default. Then any call to gpio_direction_output() or _input() would mark 
it as defined and direction of the pin could be stored in internal driver 
structures.

> +  The I/O expander can detect input state changes, and thus optionally
> act as +  an interrupt controller. When interrupts support is desired

I don't like this statement. Device tree should represent what the device 
allows you to do, not what you want the device to do.

My opinion on this is that if the chip supports interrupts then it should 
always be an interrupt-controller (unless its interrupt pin is not wired 
on the board, but this still conforms to what I wrote above).

> 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 070e81f..50a90f1 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,29 @@ 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 = client-
>dev.platform_data;
> +	struct device_node		*np = client->dev.of_node;
>  	struct pcf857x			*gpio;
> +	unsigned int			n_latch = 0;
> +	unsigned int			ngpio;
>  	int				status;
> 
> -	pdata = client->dev.platform_data;
> -	if (!pdata) {
> +#ifdef CONFIG_OF
> +	if (np) {

Wouldn't if (IS_ENABLED(CONFIG_OF) && np) be sufficient here, without the 
#ifdef? You would have to move the match table out of the #ifdef in this 
case, though...

Best regards,
Tomasz

> +		const struct of_device_id *of_id;
> +
> +		of_id = of_match_device(pcf857x_of_table, &client->dev);
> +		ngpio = (unsigned int)of_id->data;
> +	} else
> +#endif
> +		ngpio = id->driver_data;
> +

Re: [PATCH v2 3/3] gpio: pcf857x: Add OF support

[Laurent Pinchart]

Hi Tomasz,

Thank you for the review.

On Saturday 24 August 2013 02:41:59 Tomasz Figa wrote:
> On Tuesday 20 of August 2013 01:04:54 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> ---
> > 
> >  .../devicetree/bindings/gpio/gpio-pcf857x.txt      | 71 +++++++++++++++++
> >  drivers/gpio/gpio-pcf857x.c                        | 57 ++++++++++++++---
> >  2 files changed, 119 insertions(+), 9 deletions(-)
> > 
> >  create mode 100644
> > 
> > Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt new file mode
> > 100644
> > index 0000000..df94462
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > @@ -0,0 +1,71 @@

[snip]

> > +  - 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.
> 
> Hmm, do you actually need to know whether those pins are outputs or inputs
> before they get used for first time? I believe any driver using GPIO will
> call gpio_direction_{in,out}put() before it starts using the pin, which
> will initialize the pin to a known state.
> 
> What I'd suggest is making the driver handle this by having a bit mask
> that marks states of pins as defined and flagging all pins as undefined by
> default. Then any call to gpio_direction_output() or _input() would mark
> it as defined and direction of the pin could be stored in internal driver
> structures.

The problem is that all pins are controlled through a single I2C write. 
Setting the direction of a pin will set the direction of all other pins. I 
thus need to know what the initial settings are to avoid glitches.

> > +  The I/O expander can detect input state changes, and thus optionally
> > +  act as an interrupt controller. When interrupts support is desired
> 
> I don't like this statement. Device tree should represent what the device
> allows you to do, not what you want the device to do.
> 
> My opinion on this is that if the chip supports interrupts then it should
> always be an interrupt-controller (unless its interrupt pin is not wired
> on the board, but this still conforms to what I wrote above).

I agree. What about the following text then ?

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.

> > diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
> > index 070e81f..50a90f1 100644
> > --- a/drivers/gpio/gpio-pcf857x.c
> > +++ b/drivers/gpio/gpio-pcf857x.c

[snip]

> > @@ -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,29 @@ 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 = client->dev.platform_data;
> > +	struct device_node		*np = client->dev.of_node;
> >  	struct pcf857x			*gpio;
> > +	unsigned int			n_latch = 0;
> > +	unsigned int			ngpio;
> >  	int				status;
> > 
> > -	pdata = client->dev.platform_data;
> > -	if (!pdata) {
> > +#ifdef CONFIG_OF
> > +	if (np) {
> 
> Wouldn't if (IS_ENABLED(CONFIG_OF) && np) be sufficient here, without the
> #ifdef? You would have to move the match table out of the #ifdef in this
> case, though...

That's the exact reason why I've used #ifdef CONFIG_OF here, I didn't want to 
add the overhead of the pcf857x_of_table when CONFIG_OF isn't defined.

> > +		const struct of_device_id *of_id;
> > +
> > +		of_id = of_match_device(pcf857x_of_table, &client->dev);
> > +		ngpio = (unsigned int)of_id->data;
> > +	} else
> > +#endif
> > +		ngpio = id->driver_data;
> > +

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] gpio: pcf857x: Add OF support

[Tomasz Figa]

On Saturday 24 of August 2013 02:54:07 Laurent Pinchart wrote:
> Hi Tomasz,
> 
> Thank you for the review.
> 
> On Saturday 24 August 2013 02:41:59 Tomasz Figa wrote:
> > On Tuesday 20 of August 2013 01:04:54 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> ---
> > > 
> > >  .../devicetree/bindings/gpio/gpio-pcf857x.txt      | 71
> > >  +++++++++++++++++ drivers/gpio/gpio-pcf857x.c                     
> > >    | 57 ++++++++++++++--- 2 files changed, 119 insertions(+), 9
> > >  deletions(-)
> > >  
> > >  create mode 100644
> > > 
> > > Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt new file
> > > mode
> > > 100644
> > > index 0000000..df94462
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > @@ -0,0 +1,71 @@
> 
> [snip]
> 
> > > +  - 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.
> > 
> > Hmm, do you actually need to know whether those pins are outputs or
> > inputs before they get used for first time? I believe any driver
> > using GPIO will call gpio_direction_{in,out}put() before it starts
> > using the pin, which will initialize the pin to a known state.
> > 
> > What I'd suggest is making the driver handle this by having a bit mask
> > that marks states of pins as defined and flagging all pins as
> > undefined by default. Then any call to gpio_direction_output() or
> > _input() would mark it as defined and direction of the pin could be
> > stored in internal driver structures.
> 
> The problem is that all pins are controlled through a single I2C write.
> Setting the direction of a pin will set the direction of all other pins.
> I thus need to know what the initial settings are to avoid glitches.

Oh, that's a funny hardware, isn't it? :)

Well, I guess it can't be helped then. Sorry for the noise.

> > > +  The I/O expander can detect input state changes, and thus
> > > optionally
> > > +  act as an interrupt controller. When interrupts support is
> > > desired
> > 
> > I don't like this statement. Device tree should represent what the
> > device allows you to do, not what you want the device to do.
> > 
> > My opinion on this is that if the chip supports interrupts then it
> > should always be an interrupt-controller (unless its interrupt pin is
> > not wired on the board, but this still conforms to what I wrote
> > above).
> 
> I agree. What about the following text then ?
> 
> 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.

Sounds good.

> > > diff --git a/drivers/gpio/gpio-pcf857x.c
> > > b/drivers/gpio/gpio-pcf857x.c
> > > index 070e81f..50a90f1 100644
> > > --- a/drivers/gpio/gpio-pcf857x.c
> > > +++ b/drivers/gpio/gpio-pcf857x.c
> 
> [snip]
> 
> > > @@ -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,29 @@ 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 = client-
>dev.platform_data;
> > > +	struct device_node		*np = client->dev.of_node;
> > > 
> > >  	struct pcf857x			*gpio;
> > > 
> > > +	unsigned int			n_latch = 0;
> > > +	unsigned int			ngpio;
> > > 
> > >  	int				status;
> > > 
> > > -	pdata = client->dev.platform_data;
> > > -	if (!pdata) {
> > > +#ifdef CONFIG_OF
> > > +	if (np) {
> > 
> > Wouldn't if (IS_ENABLED(CONFIG_OF) && np) be sufficient here, without
> > the #ifdef? You would have to move the match table out of the #ifdef
> > in this case, though...
> 
> That's the exact reason why I've used #ifdef CONFIG_OF here, I didn't
> want to add the overhead of the pcf857x_of_table when CONFIG_OF isn't
> defined.

I'm not sure if I remember correctly, but I think there was something said 
in one of discussions some time ago, that we should be moving away from 
ifdef'ing such things, in favour of just having them compiled 
unconditionally.

[Adding DT maintainers on Cc for more opinions.]

Best regards,
Tomasz

Re: [PATCH v2 3/3] gpio: pcf857x: Add OF support

[Laurent Pinchart]

Hi Tomasz,

On Saturday 24 August 2013 16:13:11 Tomasz Figa wrote:
> On Saturday 24 of August 2013 02:54:07 Laurent Pinchart wrote:
> > On Saturday 24 August 2013 02:41:59 Tomasz Figa wrote:
> > > On Tuesday 20 of August 2013 01:04:54 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> ---
> > > > 
> > > >  .../devicetree/bindings/gpio/gpio-pcf857x.txt      | 71 +++++++++++++
> > > >  drivers/gpio/gpio-pcf857x.c                        | 57 ++++++++++---
> > > >  2 files changed, 119 insertions(+), 9 deletions(-)
> > > >  create mode 100644

[snip]

> > > > diff --git a/drivers/gpio/gpio-pcf857x.c
> > > > b/drivers/gpio/gpio-pcf857x.c
> > > > index 070e81f..50a90f1 100644
> > > > --- a/drivers/gpio/gpio-pcf857x.c
> > > > +++ b/drivers/gpio/gpio-pcf857x.c
> > 
> > [snip]
> > 
> > > > @@ -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
> > > > @@ -257,14 +280,29 @@ 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 = client->dev.platform_data;
> > > > +	struct device_node		*np = client->dev.of_node;
 > > >  	struct pcf857x			*gpio;
> > > > +	unsigned int			n_latch = 0;
> > > > +	unsigned int			ngpio;
> > > >  	int				status;
> > > > 
> > > > -	pdata = client->dev.platform_data;
> > > > -	if (!pdata) {
> > > > +#ifdef CONFIG_OF
> > > > +	if (np) {
> > > 
> > > Wouldn't if (IS_ENABLED(CONFIG_OF) && np) be sufficient here, without
> > > the #ifdef? You would have to move the match table out of the #ifdef
> > > in this case, though...
> > 
> > That's the exact reason why I've used #ifdef CONFIG_OF here, I didn't
> > want to add the overhead of the pcf857x_of_table when CONFIG_OF isn't
> > defined.
> 
> I'm not sure if I remember correctly, but I think there was something said
> in one of discussions some time ago, that we should be moving away from
> ifdef'ing such things, in favour of just having them compiled
> unconditionally.

There seems to be a general consensus to favor if (IS_ENABLED(CONFIG_OF)) 
instead of #ifdef CONFIG_OF when possible. I'm not sure what the opinion is 
regarding using conditional compilation to avoid compiling unnecessary data 
tables in. I would vote for using it (there's no need to bloat the kernel 
unnecessarily on non-OF platforms), but I'll conform to whatever is decided to 
be best.

> [Adding DT maintainers on Cc for more opinions.]

I'll resubmit the patch with the DT bindings documentation fixed, and will 
submit yet another version if I need to remove the #ifdef.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] gpio: pcf857x: Add OF support

[Sylwester Nawrocki]

On 08/25/2013 02:15 AM, Laurent Pinchart wrote:
> On Saturday 24 August 2013 16:13:11 Tomasz Figa wrote:
>> On Saturday 24 of August 2013 02:54:07 Laurent Pinchart wrote:
>>> On Saturday 24 August 2013 02:41:59 Tomasz Figa wrote:
>>>> On Tuesday 20 of August 2013 01:04:54 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>  ---
>>>>>
>>>>>   .../devicetree/bindings/gpio/gpio-pcf857x.txt      | 71 +++++++++++++
>>>>>   drivers/gpio/gpio-pcf857x.c                        | 57 ++++++++++---
>>>>>   2 files changed, 119 insertions(+), 9 deletions(-)
>>>>>   create mode 100644
>
> [snip]
>
>>>>> diff --git a/drivers/gpio/gpio-pcf857x.c
>>>>> b/drivers/gpio/gpio-pcf857x.c
>>>>> index 070e81f..50a90f1 100644
>>>>> --- a/drivers/gpio/gpio-pcf857x.c
>>>>> +++ b/drivers/gpio/gpio-pcf857x.c
>>>
>>> [snip]
>>>
>>>>> @@ -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
>>>>> @@ -257,14 +280,29 @@ 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 = client->dev.platform_data;
>>>>> +	struct device_node		*np = client->dev.of_node;
>   >  >  >   	struct pcf857x			*gpio;
>>>>> +	unsigned int			n_latch = 0;
>>>>> +	unsigned int			ngpio;
>>>>>   	int				status;
>>>>>
>>>>> -	pdata = client->dev.platform_data;
>>>>> -	if (!pdata) {
>>>>> +#ifdef CONFIG_OF
>>>>> +	if (np) {
>>>>
>>>> Wouldn't if (IS_ENABLED(CONFIG_OF)&&  np) be sufficient here, without
>>>> the #ifdef? You would have to move the match table out of the #ifdef
>>>> in this case, though...
>>>
>>> That's the exact reason why I've used #ifdef CONFIG_OF here, I didn't
>>> want to add the overhead of the pcf857x_of_table when CONFIG_OF isn't
>>> defined.
>>
>> I'm not sure if I remember correctly, but I think there was something said
>> in one of discussions some time ago, that we should be moving away from
>> ifdef'ing such things, in favour of just having them compiled
>> unconditionally.
>
> There seems to be a general consensus to favor if (IS_ENABLED(CONFIG_OF))
> instead of #ifdef CONFIG_OF when possible. I'm not sure what the opinion is
> regarding using conditional compilation to avoid compiling unnecessary data
> tables in. I would vote for using it (there's no need to bloat the kernel
> unnecessarily on non-OF platforms), but I'll conform to whatever is decided to
> be best.
>
>> [Adding DT maintainers on Cc for more opinions.]
>
> I'll resubmit the patch with the DT bindings documentation fixed, and will
> submit yet another version if I need to remove the #ifdef.

I think it makes sense to keep this table compiled in conditionally, 
size of
struct of_device_id is relatively large. While absolute increase in size
might not be that significant the relative increase is quite large - 
appr. 130%.


Before $subject patch:

$ size drivers/gpio/gpio-pcf857x.o
    text	   data	    bss	    dec	    hex	filename
    2228	    140	      0	   2368	    940	drivers/gpio/gpio-pcf857x.o

After applying the patch:

$ size drivers/gpio/gpio-pcf857x.o
    text	   data	    bss	    dec	    hex	filename
    5284	    140	      0	   5424	   1530	drivers/gpio/gpio-pcf857x.o

--
Regards,
Sylwester

Re: [PATCH v2 2/3] gpio: pcf857x: Remove pdata argument to pcf857x_irq_domain_init()

[Kuninori Morimoto]

Hi

> > The argument is not used, remove it. No board registers a pcf857x device
> > with an IRQ without specifying platform data, IRQ domain registration
> > behaviour is thus not affected by this change.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Patch applied, unless Kuninori has some objections.

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Re: [PATCH v2 3/3] gpio: pcf857x: Add OF support

[Mark Rutland]

On Sat, Aug 24, 2013 at 03:13:11PM +0100, Tomasz Figa wrote:
> On Saturday 24 of August 2013 02:54:07 Laurent Pinchart wrote:
> > Hi Tomasz,
> > 
> > Thank you for the review.
> > 
> > On Saturday 24 August 2013 02:41:59 Tomasz Figa wrote:
> > > On Tuesday 20 of August 2013 01:04:54 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> ---
> > > > 
> > > >  .../devicetree/bindings/gpio/gpio-pcf857x.txt      | 71
> > > >  +++++++++++++++++ drivers/gpio/gpio-pcf857x.c                     
> > > >    | 57 ++++++++++++++--- 2 files changed, 119 insertions(+), 9
> > > >  deletions(-)
> > > >  
> > > >  create mode 100644
> > > > 
> > > > Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > > b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt new file
> > > > mode
> > > > 100644
> > > > index 0000000..df94462
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > > @@ -0,0 +1,71 @@
> > 
> > [snip]
> > 
> > > > +  - 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.
> > > 
> > > Hmm, do you actually need to know whether those pins are outputs or
> > > inputs before they get used for first time? I believe any driver
> > > using GPIO will call gpio_direction_{in,out}put() before it starts
> > > using the pin, which will initialize the pin to a known state.
> > > 
> > > What I'd suggest is making the driver handle this by having a bit mask
> > > that marks states of pins as defined and flagging all pins as
> > > undefined by default. Then any call to gpio_direction_output() or
> > > _input() would mark it as defined and direction of the pin could be
> > > stored in internal driver structures.
> > 
> > The problem is that all pins are controlled through a single I2C write.
> > Setting the direction of a pin will set the direction of all other pins.
> > I thus need to know what the initial settings are to avoid glitches.

I guess it's not possible to read the initial state from the hardware?

> 
> Oh, that's a funny hardware, isn't it? :)
> 
> Well, I guess it can't be helped then. Sorry for the noise.
> 
> > > > +  The I/O expander can detect input state changes, and thus
> > > > optionally
> > > > +  act as an interrupt controller. When interrupts support is
> > > > desired
> > > 
> > > I don't like this statement. Device tree should represent what the
> > > device allows you to do, not what you want the device to do.
> > > 
> > > My opinion on this is that if the chip supports interrupts then it
> > > should always be an interrupt-controller (unless its interrupt pin is
> > > not wired on the board, but this still conforms to what I wrote
> > > above).
> > 
> > I agree. What about the following text then ?
> > 
> > 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.
> 
> Sounds good.
> 
> > > > diff --git a/drivers/gpio/gpio-pcf857x.c
> > > > b/drivers/gpio/gpio-pcf857x.c
> > > > index 070e81f..50a90f1 100644
> > > > --- a/drivers/gpio/gpio-pcf857x.c
> > > > +++ b/drivers/gpio/gpio-pcf857x.c
> > 
> > [snip]
> > 
> > > > @@ -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,29 @@ 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 = client-
> >dev.platform_data;
> > > > +	struct device_node		*np = client->dev.of_node;
> > > > 
> > > >  	struct pcf857x			*gpio;
> > > > 
> > > > +	unsigned int			n_latch = 0;
> > > > +	unsigned int			ngpio;
> > > > 
> > > >  	int				status;
> > > > 
> > > > -	pdata = client->dev.platform_data;
> > > > -	if (!pdata) {
> > > > +#ifdef CONFIG_OF
> > > > +	if (np) {
> > > 
> > > Wouldn't if (IS_ENABLED(CONFIG_OF) && np) be sufficient here, without
> > > the #ifdef? You would have to move the match table out of the #ifdef
> > > in this case, though...
> > 
> > That's the exact reason why I've used #ifdef CONFIG_OF here, I didn't
> > want to add the overhead of the pcf857x_of_table when CONFIG_OF isn't
> > defined.
> 
> I'm not sure if I remember correctly, but I think there was something said 
> in one of discussions some time ago, that we should be moving away from 
> ifdef'ing such things, in favour of just having them compiled 
> unconditionally.

I was also under this impression, but I have no strong feelings either
way.

Thanks,
Mark.

Re: [PATCH v2 3/3] gpio: pcf857x: Add OF support

[Laurent Pinchart]

Hi Mark,

On Tuesday 27 August 2013 11:39:49 Mark Rutland wrote:
> On Sat, Aug 24, 2013 at 03:13:11PM +0100, Tomasz Figa wrote:
> > On Saturday 24 of August 2013 02:54:07 Laurent Pinchart wrote:
> > > On Saturday 24 August 2013 02:41:59 Tomasz Figa wrote:
> > > > On Tuesday 20 of August 2013 01:04:54 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>
> > > > > ---
> > > > > 
> > > > >  .../devicetree/bindings/gpio/gpio-pcf857x.txt      | 71 +++++++++++
> > > > >  drivers/gpio/gpio-pcf857x.c                        | 57 +++++++++--
> > > > >  2 files changed, 119 insertions(+), 9 deletions(-)
> > > > >  create mode 100644
> > > > > Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > > > b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt new file
> > > > > mode 100644
> > > > > index 0000000..df94462
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > > > @@ -0,0 +1,71 @@
> > > 
> > > [snip]
> > > 
> > > > > +  - 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.
> > > > 
> > > > Hmm, do you actually need to know whether those pins are outputs or
> > > > inputs before they get used for first time? I believe any driver
> > > > using GPIO will call gpio_direction_{in,out}put() before it starts
> > > > using the pin, which will initialize the pin to a known state.
> > > > 
> > > > What I'd suggest is making the driver handle this by having a bit mask
> > > > that marks states of pins as defined and flagging all pins as
> > > > undefined by default. Then any call to gpio_direction_output() or
> > > > _input() would mark it as defined and direction of the pin could be
> > > > stored in internal driver structures.
> > > 
> > > The problem is that all pins are controlled through a single I2C write.
> > > Setting the direction of a pin will set the direction of all other pins.
> > > I thus need to know what the initial settings are to avoid glitches.
> 
> I guess it's not possible to read the initial state from the hardware?

I wish. Unfortunately it can only be written.

> > Oh, that's a funny hardware, isn't it? :)
> > 
> > Well, I guess it can't be helped then. Sorry for the noise.
> > 
> > > > > +  The I/O expander can detect input state changes, and thus
> > > > > optionally 
> > > > > +  act as an interrupt controller. When interrupts support is
> > > > > desired
> > > > 
> > > > I don't like this statement. Device tree should represent what the
> > > > device allows you to do, not what you want the device to do.
> > > > 
> > > > My opinion on this is that if the chip supports interrupts then it
> > > > should always be an interrupt-controller (unless its interrupt pin is
> > > > not wired on the board, but this still conforms to what I wrote
> > > > above).
> > > 
> > > I agree. What about the following text then ?
> > > 
> > > 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.
> > 
> > Sounds good.
> > 
> > > > > diff --git a/drivers/gpio/gpio-pcf857x.c
> > > > > b/drivers/gpio/gpio-pcf857x.c
> > > > > index 070e81f..50a90f1 100644
> > > > > --- a/drivers/gpio/gpio-pcf857x.c
> > > > > +++ b/drivers/gpio/gpio-pcf857x.c
> > > 
> > > [snip]
> > > 
> > > > > @@ -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,29 @@ 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 = client-
> > >
> > >dev.platform_data;
> > >
> > > > > +	struct device_node		*np = client->dev.of_node;
> > > > > 
> > > > >  	struct pcf857x			*gpio;
> > > > > 
> > > > > +	unsigned int			n_latch = 0;
> > > > > +	unsigned int			ngpio;
> > > > > 
> > > > >  	int				status;
> > > > > 
> > > > > -	pdata = client->dev.platform_data;
> > > > > -	if (!pdata) {
> > > > > +#ifdef CONFIG_OF
> > > > > +	if (np) {
> > > > 
> > > > Wouldn't if (IS_ENABLED(CONFIG_OF) && np) be sufficient here, without
> > > > the #ifdef? You would have to move the match table out of the #ifdef
> > > > in this case, though...
> > > 
> > > That's the exact reason why I've used #ifdef CONFIG_OF here, I didn't
> > > want to add the overhead of the pcf857x_of_table when CONFIG_OF isn't
> > > defined.
> > 
> > I'm not sure if I remember correctly, but I think there was something said
> > in one of discussions some time ago, that we should be moving away from
> > ifdef'ing such things, in favour of just having them compiled
> > unconditionally.
> 
> I was also under this impression, but I have no strong feelings either way.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] gpio: pcf857x: Add OF support

[Mark Rutland]

On Tue, Aug 27, 2013 at 11:50:40AM +0100, Laurent Pinchart wrote:
> Hi Mark,
> 
> On Tuesday 27 August 2013 11:39:49 Mark Rutland wrote:
> > On Sat, Aug 24, 2013 at 03:13:11PM +0100, Tomasz Figa wrote:
> > > On Saturday 24 of August 2013 02:54:07 Laurent Pinchart wrote:
> > > > On Saturday 24 August 2013 02:41:59 Tomasz Figa wrote:
> > > > > On Tuesday 20 of August 2013 01:04:54 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>
> > > > > > ---
> > > > > > 
> > > > > >  .../devicetree/bindings/gpio/gpio-pcf857x.txt      | 71 +++++++++++
> > > > > >  drivers/gpio/gpio-pcf857x.c                        | 57 +++++++++--
> > > > > >  2 files changed, 119 insertions(+), 9 deletions(-)
> > > > > >  create mode 100644
> > > > > > Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > > > > b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt new file
> > > > > > mode 100644
> > > > > > index 0000000..df94462
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > > > > > @@ -0,0 +1,71 @@
> > > > 
> > > > [snip]
> > > > 
> > > > > > +  - 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.
> > > > > 
> > > > > Hmm, do you actually need to know whether those pins are outputs or
> > > > > inputs before they get used for first time? I believe any driver
> > > > > using GPIO will call gpio_direction_{in,out}put() before it starts
> > > > > using the pin, which will initialize the pin to a known state.
> > > > > 
> > > > > What I'd suggest is making the driver handle this by having a bit mask
> > > > > that marks states of pins as defined and flagging all pins as
> > > > > undefined by default. Then any call to gpio_direction_output() or
> > > > > _input() would mark it as defined and direction of the pin could be
> > > > > stored in internal driver structures.
> > > > 
> > > > The problem is that all pins are controlled through a single I2C write.
> > > > Setting the direction of a pin will set the direction of all other pins.
> > > > I thus need to know what the initial settings are to avoid glitches.
> > 
> > I guess it's not possible to read the initial state from the hardware?
> 
> I wish. Unfortunately it can only be written.

What a shame :(

Thanks,
Mark.