On 08/04/2011 10:45 AM, Rob Herring wrote:
> Ben,
>
> On 08/04/2011 04:12 AM, Ben Dooks wrote:
>> On Wed, Aug 03, 2011 at 03:04:23PM -0500, Rob Herring wrote:
>>> From: Rob Herring <rob.herring@xxxxxxxxxxx>
>>>
>>> Add of_match_table and DT style i2c registration to designware i2c
>>> driver.
>>>
>>> Signed-off-by: Rob Herring <rob.herring@xxxxxxxxxxx>
>>> Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
>>> Cc: devicetree-discuss@xxxxxxxxxxxxxxxx
>>> Cc: Ben Dooks <ben-linux@xxxxxxxxx>
>>> Cc: linux-i2c@xxxxxxxxxxxxxxx
>>> ---
>>> Documentation/devicetree/bindings/i2c/dw-i2c.txt | 23 ++++++++++++++++++++++
>>> drivers/i2c/busses/i2c-designware.c | 13 ++++++++++++
>>> 2 files changed, 36 insertions(+), 0 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/i2c/dw-i2c.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/dw-i2c.txt b/Documentation/devicetree/bindings/i2c/dw-i2c.txt
>>> new file mode 100644
>>> index 0000000..cbcb404
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/i2c/dw-i2c.txt
>>> @@ -0,0 +1,23 @@
>>> +* Synopsys DesignWare I2C
>>> +
>>> +Required properties :
>>> +
>>> + - compatible : should be "snps,designware-i2c"
>>> + - reg : Offset and length of the register set for the device
>>> + - interrupts : <IRQ> where IRQ is the interrupt number.
>>> +
>>> +Recommended properties :
>>> +
>>> + - clock-frequency : desired I2C bus clock frequency in Hz.
>>> +
>>> +Example :
>>> +
>>> + i2c@f0000 {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + compatible = "snps,designware-i2c";
>>> + reg = <0xf0000 0x1000>;
>>> + interrupts = <11>;
>>> + clock-frequency = <400000>;
>>> + };
>>> +
>>
>> looks good to me.
>>
>>> diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
>>> index b7a51c4..2911a49 100644
>>> --- a/drivers/i2c/busses/i2c-designware.c
>>> +++ b/drivers/i2c/busses/i2c-designware.c
>>> @@ -37,6 +37,7 @@
>>> #include <linux/platform_device.h>
>>> #include <linux/io.h>
>>> #include <linux/slab.h>
>>> +#include <linux/of_i2c.h>
>>>
>>> /*
>>> * Registers offset
>>> @@ -770,12 +771,17 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
>>> adap->algo = &i2c_dw_algo;
>>> adap->dev.parent = &pdev->dev;
>>>
>>> +#ifdef CONFIG_OF
>>> + r = i2c_add_adapter(adap);
>>> +#else
>>> adap->nr = pdev->id;
>>> r = i2c_add_numbered_adapter(adap);
>>> +#endif
>>
>> I would say that doing the #ifdef CONFIG_OF is dangerous here when we
>> are in a mixed OF/platform enviromnent as we're depending on compile
>> time selection.
>>
>> I'm also wondering whether we have an of helper macro which takes
>> a pdev and gives you an adapter number either given on pdev->id or
>> -1 for the case when we're using the OF bindings.
>>
>> It might be worth talking to Grant about setting pdev->id to -1 if we
>> are using an OF device.
>>
>
> As Grant said, that's already done and this hunk is not needed.
>
>>> if (r) {
>>> dev_err(&pdev->dev, "failure adding adapter\n");
>>> goto err_free_irq;
>>> }
>>> + of_i2c_register_devices(adap);
>>
>> If we did that, we could add a of_i2c_register_adapter() call which
>> would take the platform device and then do the of_i2c_register_devices()
>> and do these steps.
>>
>
> Better yet, how about putting of_i2c_register_devices into
> i2c_register_adapter? Everywhere that calls of_i2c_register_devices is
> preceded by a call to i2c_add_numbered_adapter or i2c_add_adapter. It
> seems logical to put it with i2c_scan_static_board_info. I'll prepare a
> patch to add that and remove all the other callers unless you think
> that's a bad idea.
>
Nevermind. That would be undoing this commit:
of/i2c: Fix module load order issue caused by of_i2c.c
Commit 959e85f7, "i2c: add OF-style registration and binding" caused a
module dependency loop where of_i2c.c calls functions in i2c-core, and
i2c-core calls of_i2c_register_devices() in of_i2c. This means that
when i2c support is built as a module when CONFIG_OF is set, then
neither i2c_core nor of_i2c are able to be loaded.
This patch fixes the problem by moving the of_i2c_register_devices()
calls back into the device drivers. Device drivers already
specifically request the core code to parse the device tree for
devices anyway by setting the of_node pointer, so it isn't a big
deal to also call the registration function. The drivers just become
slightly more verbose.
Signed-off-by: Grant Likely <grant.likely@xxxxxxxxxxxx>
Signed-off-by: Jean Delvare <khali@xxxxxxxxxxxx>
Rob
>>> return 0;
>>>
>>> @@ -819,6 +825,12 @@ static int __devexit dw_i2c_remove(struct platform_device *pdev)
>>> return 0;
>>> }
>>>
>>> +static const struct of_device_id dw_i2c_of_match[] = {
>>> + { .compatible = "snps,designware-i2c", },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
>>> +
>>> /* work with hotplug and coldplug */
>>> MODULE_ALIAS("platform:i2c_designware");
>>>
>>> @@ -827,6 +839,7 @@ static struct platform_driver dw_i2c_driver = {
>>> .driver = {
>>> .name = "i2c_designware",
>>> .owner = THIS_MODULE,
>>> + .of_match_table = dw_i2c_of_match,
>>
>> If my patch for of_match_ptr() is accepted, it will be needed here
>> otherwise you will need to do something about remvoing the of table
>> above if not config of.
>
> There's really not much harm with having the table. If you match the
> device in the non-DT way, the table is not used. Drivers should never
> directly access the table either, but use the helpers to get their data.
>
> Rob
>
>
--
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
