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

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

Hi Tomasz,

On Tuesday 27 August 2013 13:55:00 Tomasz Figa wrote:
> On Tuesday 27 of August 2013 14:00:24 Archit Taneja wrote:
> > On Tuesday 27 August 2013 01:44 PM, Tomasz Figa wrote:
> > > On Tuesday 27 of August 2013 10:02:39 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                        | 44 ++++++++++---
> > >>   2 files changed, 107 insertions(+), 8 deletions(-)
> > >>   create mode 100644
> > >> Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
> > >> 
> > >> Changes since v4:
> > >> 
> > >> - Don't try to get ngpio from of_device_id data, we already get it
> > >>   from i2c_device_id
> > > 
> > > Hmm, I'm not sure how this is supposed to work.
> > > 
> > > How does the I2C core resolve OF compatible name to particular entry in
> > > id_table? I believe it simply passes NULL as the second argument of
> > > .probe() if the device is instantiated based on OF compatible string
> > > and not one in the legacy ID table.
> > 
> > It doesn't pass the second argument as NULL. If you look at
> > i2c_device_probe() in drivers/i2c/i2c-core.c, the second argument to
> > probe is passed as: i2c_match_id(driver->id_table, client)
> > 
> > This will extract the i2c_device_id pointer from the id_table.
> 
> Yes, there is a chance that it will not return NULL, but I think that
> relying on this is rather flawed.
> 
> If you look at the whole code path, you can see that it's only a
> coincidence that this works. See the execution flow:
>  - I2C adapter driver calls of_i2c_register_devices(),
>  - of_i2c_register_devices() calls of_modalias_node() for every device on
> this bus,
>  - of_modalias_node() stores the second substring of compatible string
> separated by a comma, if there is one or the whole compatible otherwise to
> the output buffer (type field of i2c_board_info struct, as passed by
> of_i2c_register_devices()),
>  - of_i2c_register_devices() then calls i2c_new_device() with the resulting
> info struct,
>  - i2c_new_device() takes info->type and copies its contents to client->name
>  - then a bit later, I2C core calls i2c_match_id(), which does matching of
> client->name against id_table of the driver and the resulting i2c_device_id
> entry (or NULL) is then passed to driver's .probe() callback.
> 
> So if it happens that compatible is not equal to "<vendor>,<ID from legacy
> I2C table>", then the matching will fail and NULL will be passed.

The driver should support the same chip models reardless of whether it's used 
with or without DT. If an entry in the OF table has no corresponding entry in 
the I2C table I would consider that as a driver bug. It would be caught early, 
as the driver would crash at probe time, so it will likely not make it to 
mainline (unless we merge untested code, but that's another issue :-)).

> [CCing Wolfram and Grant, as they should now more about this behavior and
> whether it's intentional or no]

-- 
Regards,

Laurent Pinchart