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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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