| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <AANLkTimS70uHqmv3jhr+7mPz4QFKXRXuFnWj8s3EpKsO@mail.gmail.com>
Date: Mon, 31 Jan 2011 01:09:02 -0700
From: Grant Likely <grant.likely@...retlab.ca>
To: Håvard Skinnemoen <hskinnemoen@...il.com>
Cc: Thomas Chou <thomas@...ron.com.tw>,
Ben Dooks <ben-linux@...ff.org>, linux-kernel@...r.kernel.org,
nios2-dev@...c.et.ntust.edu.tw,
devicetree-discuss@...ts.ozlabs.org, linux-i2c@...r.kernel.org,
Jean Delvare <khali@...ux-fr.org>,
Albert Herranz <albert_herranz@...oo.es>
Subject: Re: [PATCH] i2c-gpio: add devicetree support
On Sun, Jan 30, 2011 at 8:26 PM, Håvard Skinnemoen
<hskinnemoen@...il.com> wrote:
> Hi,
>
> On Sun, Jan 30, 2011 at 7:56 AM, Thomas Chou <thomas@...ron.com.tw> wrote:
>> From: Albert Herranz <albert_herranz@...oo.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@...oo.es>
>> Signed-off-by: Thomas Chou <thomas@...ron.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists