Re: [PATCH] i2c-gpio: add devicetree support

[Grant Likely <grant.likely@secretlab.ca>]

On Sun, Jan 30, 2011 at 8:26 PM, Håvard Skinnemoen
<hskinnemoen@gmail.com> wrote:
> Hi,
>
> On Sun, Jan 30, 2011 at 7:56 AM, Thomas Chou <thomas@wytron.com.tw> wrote:
>> From: Albert Herranz <albert_herranz@yahoo.es>
>>
>> This patch is based on an earlier patch from Albert Herranz,
>> http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/
>> 9854eb78607c641ab5ae85bcbe3c9d14ac113733
>
> That commit has a single-line description of which I don't understand
> a single word (unless "wii" is what I think it is, which seems
> likely). Could you please explain how that commit relates to this
> patch?

The URL got wrapped.  Try this one (assuming my mailer doesn't wrap it):

http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/9854eb78607c641ab5ae85bcbe3c9d14ac113733

>
>> The dts binding is modified as Grant suggested. The of probing
>> is merged inline instead of a separate file. It uses the newer
>> of gpio probe.
>
> It seems like a terrible idea to merge firmware-specific code into the
> driver. Is there are reason why of-based platforms can't just pass the
> data they need in pdata like everyone else?

Overall Thomas is doing the right thing here.  The driver data has to
be decoded *somewhere*, but since that code is definitely
driver-specific (as opposed to platform, subsystem, or arch specific)
putting it into the driver is absolutely the right thing to do.  Quite
a few drivers now do exactly this.

It is however generally a wise practice to limit the of-support code
to a hook in the drivers probe hook, and as you point out, care must
be taken to make sure both CONFIG_OF and !CONFIG_OF kernel builds
work.

>
> Not saying that it necessarily _is_ a terrible idea, but I think the
> reasoning behind it needs to be included in the patch description.

Nah, he doesn't really need to defend this since it is a well
established pattern.  device tree support is in core code now (see
of_node an of_match_table in include/linux/device.h), and other
drivers do exactly this.

>> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
>> ---
>>  Documentation/devicetree/bindings/gpio/i2c.txt |   39 ++++++++++++++
>>  drivers/i2c/busses/i2c-gpio.c                  |   67 ++++++++++++++++++++++-
>>  2 files changed, 103 insertions(+), 3 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/i2c.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/i2c.txt b/Documentation/devicetree/bindings/gpio/i2c.txt
>> new file mode 100644
>> index 0000000..402569e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/i2c.txt
>
> This looks a bit backwards. i2c-gpio is a i2c driver which happens to
> utilize the gpio framework, not the other way around.

Yes, this should be in devicetree/bindings/i2c/i2c-gpio.txt

>
>> @@ -0,0 +1,39 @@
>> +GPIO-based I2C
>> +
>> +Required properties:
>> +- compatible : should be "i2c-gpio".
>> +- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
>> +Optional properties:
>> +- sda-is-open-drain : present if SDA gpio is open-drain.
>> +- scl-is-open-drain : present if SCL gpio is open-drain.
>> +- scl-is-output-only : present if SCL is an output gpio only.
>
> I think "present if the output driver for SCL cannot be turned off" is
> more accurate. Might also be worth mentioning that this will prevent
> clock stretching from working.
>
>> --- a/drivers/i2c/busses/i2c-gpio.c
>> +++ b/drivers/i2c/busses/i2c-gpio.c
>> @@ -14,6 +14,9 @@
>>  #include <linux/module.h>
>>  #include <linux/slab.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_i2c.h>
>
> Do these headers provide stubs so non-of platforms won't break?

yes.

>
>> @@ -83,11 +86,52 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>>        struct i2c_gpio_platform_data *pdata;
>>        struct i2c_algo_bit_data *bit_data;
>>        struct i2c_adapter *adap;
>> +       struct device_node *np = pdev->dev.of_node;
>
> Would be nice if this could be eliminated on non-of platforms.

It's pretty benign.  However, for current mainline this needs to be
protected with a #ifdef CONFIG_OF.  In 2.6.29, the conditional can be
removed since of_node will be a permanent part of struct device.

>
>>        int ret;
>>
>>        pdata = pdev->dev.platform_data;
>> -       if (!pdata)
>> -               return -ENXIO;
>> +       if (!pdata) {
>> +               if (np && of_gpio_count(np) >= 2) {
>
> If that expression somehow always evaluates to false on non-of
> platforms, this might be ok. But please confirm if this is the case;
> otherwise, it looks like a pretty large addition to an otherwise very
> small driver.
>
> How about a tiny bit of restructuring: Move the block below into a
> separate function, which is only called if some constant expression
> says that of is enabled. Then you can move the declaration above
> either into the if block or into the function, depending on where you
> want to do the conditional above.

Yes, moving the dt decoding code to a separate function would keep the
dt-support better isolated from the core of the driver and would make
the CONFIG_OF/!CONFIG_OF handling better.

>
>>  static struct platform_driver i2c_gpio_driver = {
>>        .driver         = {
>>                .name   = "i2c-gpio",
>>                .owner  = THIS_MODULE,
>> +               .of_match_table = i2c_gpio_match,
>
> Is this field always present even when of is disabled?

No, not yet.  It will be in 2.6.29.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.