On Sun, Jan 30, 2011 at 8:26 PM, Håvard Skinnemoen <hskinnemoen@xxxxxxxxx> wrote: > Hi, > > On Sun, Jan 30, 2011 at 7:56 AM, Thomas Chou <thomas@xxxxxxxxxxxxx> wrote: >> From: Albert Herranz <albert_herranz@xxxxxxxx> >> >> 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@xxxxxxxx> >> Signed-off-by: Thomas Chou <thomas@xxxxxxxxxxxxx> >> --- >> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
