[...]
>
>
> From: Lee Jones <[email protected]>
> Date: Tue, 17 Apr 2012 16:04:13 +0100
> Subject: [PATCH 1/1] i2c: Add Device Tree support to the Nomadik I2C driver
>
> Here we apply the bindings required for successful Device Tree
> probing of the i2c-nomadik driver. We also apply a fall-back
> configuration in case either one is not provided, or a required
> element is missing from the one supplied.
>
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index a92440d..58e8114 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -23,6 +23,7 @@
> #include <linux/io.h>
> #include <linux/regulator/consumer.h>
> #include <linux/pm_runtime.h>
> +#include <linux/of.h>
>
> #include <plat/i2c.h>
>
> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
> .functionality = nmk_i2c_functionality
> };
>
> +static struct nmk_i2c_controller u8500_i2c = {
> + /*
> + * Slave data setup time; 250ns, 100ns, and 10ns, which
> + * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
> + */
> + .slsu = 0xe,
> + .tft = 1, /* Tx FIFO threshold */
> + .rft = 8, /* Rx FIFO threshold */
> + .clk_freq = 100000, /* std. mode operation */
> + .timeout = 200, /* Slave response timeout(ms) */
> + .sm = I2C_FREQ_MODE_FAST,
How is this possible? you are setting clk_freq as 100kb/s and mode
as fast mode which is supposed to be 400kb/s.
> +};
> +
> +static int __devinit
> +nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
> +{
> + of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
> + if (!pdata->clk_freq) {
> + pr_warn("%s: Clock frequency not found\n", np->full_name);
> + return -EINVAL;
> + }
> +
> + of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
> + if (!pdata->slsu) {
> + pr_warn("%s: Data line delay not found\n", np->full_name);
> + return -EINVAL;
> + }
> +
> + of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
> + if (!pdata->tft) {
> + pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
> + return -EINVAL;
> + }
> +
> + of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
> + if (!pdata->rft) {
> + pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
> + return -EINVAL;
> + }
> +
> + of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
> + if (!pdata->timeout) {
> + pr_warn("%s: Timeout not found\n", np->full_name);
> + return -EINVAL;
> + }
> +
> + if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
> + pdata->sm = I2C_FREQ_MODE_FAST;
> + else
> + pdata->sm = I2C_FREQ_MODE_STANDARD;
The controller has more modes like fast mode plus and high speed mode. You can't make
assumptions like this.
> +
> + return 0;
> +}
> +
> static int __devinit nmk_i2c_probe(struct platform_device *pdev)
> {
> int ret = 0;
> struct resource *res;
> - struct nmk_i2c_controller *pdata =
> - pdev->dev.platform_data;
> + struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
> + struct device_node *np = pdev->dev.of_node;
> struct nmk_i2c_dev *dev;
> struct i2c_adapter *adap;
>
> + if (np) {
> + if (!pdata) {
> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) {
> + ret = -ENOMEM;
> + goto err_no_mem;
> + }
> + }
> + ret = nmk_i2c_of_probe(np, pdata);
> + if (ret)
> + kfree(pdata);
> + }
> +
> + if (!pdata)
> + /* No i2c configuration found, using the default. */
> + pdata = &u8500_i2c;
There are redundant fallback configurations exists in the driver. You might need to remove
those, if you want to fallback here.
> +
> dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
> if (!dev) {
> dev_err(&pdev->dev, "cannot allocate memory\n");
> @@ -1038,11 +1110,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct of_device_id nmk_gpio_match[] = {
> + { .compatible = "st,nomadik-i2c", },
> + {},
> +};
> +
> static struct platform_driver nmk_i2c_driver = {
> .driver = {
> .owner = THIS_MODULE,
> .name = DRIVER_NAME,
> .pm = &nmk_i2c_pm,
> + .of_match_table = nmk_gpio_match,
> },
> .probe = nmk_i2c_probe,
> .remove = __devexit_p(nmk_i2c_remove),
> --
> 1.7.9.5
>
/srinidhi
On 15/06/12 12:50, Srinidhi Kasagar wrote:
> [...]
>>
>>
>> From: Lee Jones<[email protected]>
>> Date: Tue, 17 Apr 2012 16:04:13 +0100
>> Subject: [PATCH 1/1] i2c: Add Device Tree support to the Nomadik I2C driver
>>
>> Here we apply the bindings required for successful Device Tree
>> probing of the i2c-nomadik driver. We also apply a fall-back
>> configuration in case either one is not provided, or a required
>> element is missing from the one supplied.
>>
>> Cc: [email protected]
>> Signed-off-by: Lee Jones<[email protected]>
>> ---
>> drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 80 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
>> index a92440d..58e8114 100644
>> --- a/drivers/i2c/busses/i2c-nomadik.c
>> +++ b/drivers/i2c/busses/i2c-nomadik.c
>> @@ -23,6 +23,7 @@
>> #include<linux/io.h>
>> #include<linux/regulator/consumer.h>
>> #include<linux/pm_runtime.h>
>> +#include<linux/of.h>
>>
>> #include<plat/i2c.h>
>>
>> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
>> .functionality = nmk_i2c_functionality
>> };
>>
>> +static struct nmk_i2c_controller u8500_i2c = {
>> + /*
>> + * Slave data setup time; 250ns, 100ns, and 10ns, which
>> + * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>> + */
>> + .slsu = 0xe,
>> + .tft = 1, /* Tx FIFO threshold */
>> + .rft = 8, /* Rx FIFO threshold */
>> + .clk_freq = 100000, /* std. mode operation */
>> + .timeout = 200, /* Slave response timeout(ms) */
>> + .sm = I2C_FREQ_MODE_FAST,
>
> How is this possible? you are setting clk_freq as 100kb/s and mode
> as fast mode which is supposed to be 400kb/s.
That's not how I read it:
> enum i2c_freq_mode {
> I2C_FREQ_MODE_STANDARD, /* up to 100 Kb/s */
> I2C_FREQ_MODE_FAST, /* up to 400 Kb/s */
> I2C_FREQ_MODE_HIGH_SPEED, /* up to 3.4 Mb/s */
> I2C_FREQ_MODE_FAST_PLUS, /* up to 1 Mb/s */
> };
The operative words here are "up to".
>> +};
>> +
>> +static int __devinit
>> +nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
>> +{
>> + of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
>> + if (!pdata->clk_freq) {
>> + pr_warn("%s: Clock frequency not found\n", np->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
>> + if (!pdata->slsu) {
>> + pr_warn("%s: Data line delay not found\n", np->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
>> + if (!pdata->tft) {
>> + pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
>> + if (!pdata->rft) {
>> + pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
>> + if (!pdata->timeout) {
>> + pr_warn("%s: Timeout not found\n", np->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
>> + pdata->sm = I2C_FREQ_MODE_FAST;
>> + else
>> + pdata->sm = I2C_FREQ_MODE_STANDARD;
>
> The controller has more modes like fast mode plus and high speed mode. You can't make
> assumptions like this.
These are currently unsupported. Read the comments in the file:
> /*
> * set the speed mode. Currently we support
> * only standard and fast mode of operation
> * TODO - support for fast mode plus (up to 1Mb/s)
> * and high speed (up to 3.4 Mb/s)
> */
I can add a similar comment here if it make you feel better?
>> +
>> + return 0;
>> +}
>> +
>> static int __devinit nmk_i2c_probe(struct platform_device *pdev)
>> {
>> int ret = 0;
>> struct resource *res;
>> - struct nmk_i2c_controller *pdata =
>> - pdev->dev.platform_data;
>> + struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
>> + struct device_node *np = pdev->dev.of_node;
>> struct nmk_i2c_dev *dev;
>> struct i2c_adapter *adap;
>>
>> + if (np) {
>> + if (!pdata) {
>> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata) {
>> + ret = -ENOMEM;
>> + goto err_no_mem;
>> + }
>> + }
>> + ret = nmk_i2c_of_probe(np, pdata);
>> + if (ret)
>> + kfree(pdata);
>> + }
>> +
>> + if (!pdata)
>> + /* No i2c configuration found, using the default. */
>> + pdata =&u8500_i2c;
>
> There are redundant fallback configurations exists in the driver. You might need to remove
> those, if you want to fallback here.
I only see a fall-back for clk_freq. Are there others?
>> +
>> dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
>> if (!dev) {
>> dev_err(&pdev->dev, "cannot allocate memory\n");
>> @@ -1038,11 +1110,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static const struct of_device_id nmk_gpio_match[] = {
>> + { .compatible = "st,nomadik-i2c", },
>> + {},
>> +};
>> +
>> static struct platform_driver nmk_i2c_driver = {
>> .driver = {
>> .owner = THIS_MODULE,
>> .name = DRIVER_NAME,
>> .pm =&nmk_i2c_pm,
>> + .of_match_table = nmk_gpio_match,
>> },
>> .probe = nmk_i2c_probe,
>> .remove = __devexit_p(nmk_i2c_remove),
>> --
>> 1.7.9.5
>>
Kind regards,
Lee
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
> On 15/06/12 12:50, Srinidhi Kasagar wrote:
> > [...]
> >>
> >>
> >> From: Lee Jones<[email protected]>
> >> Date: Tue, 17 Apr 2012 16:04:13 +0100
> >> Subject: [PATCH 1/1] i2c: Add Device Tree support to the Nomadik I2C driver
> >>
> >> Here we apply the bindings required for successful Device Tree
> >> probing of the i2c-nomadik driver. We also apply a fall-back
> >> configuration in case either one is not provided, or a required
> >> element is missing from the one supplied.
> >>
> >> Cc: [email protected]
> >> Signed-off-by: Lee Jones<[email protected]>
> >> ---
> >> drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 80 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> >> index a92440d..58e8114 100644
> >> --- a/drivers/i2c/busses/i2c-nomadik.c
> >> +++ b/drivers/i2c/busses/i2c-nomadik.c
> >> @@ -23,6 +23,7 @@
> >> #include<linux/io.h>
> >> #include<linux/regulator/consumer.h>
> >> #include<linux/pm_runtime.h>
> >> +#include<linux/of.h>
> >>
> >> #include<plat/i2c.h>
> >>
> >> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
> >> .functionality = nmk_i2c_functionality
> >> };
> >>
> >> +static struct nmk_i2c_controller u8500_i2c = {
> >> + /*
> >> + * Slave data setup time; 250ns, 100ns, and 10ns, which
> >> + * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
> >> + */
> >> + .slsu = 0xe,
> >> + .tft = 1, /* Tx FIFO threshold */
> >> + .rft = 8, /* Rx FIFO threshold */
> >> + .clk_freq = 100000, /* std. mode operation */
> >> + .timeout = 200, /* Slave response timeout(ms) */
> >> + .sm = I2C_FREQ_MODE_FAST,
> >
> > How is this possible? you are setting clk_freq as 100kb/s and mode
> > as fast mode which is supposed to be 400kb/s.
>
> That's not how I read it:
But it is not readable. It confuses people.
>
> > enum i2c_freq_mode {
> > I2C_FREQ_MODE_STANDARD, /* up to 100 Kb/s */
> > I2C_FREQ_MODE_FAST, /* up to 400 Kb/s */
> > I2C_FREQ_MODE_HIGH_SPEED, /* up to 3.4 Mb/s */
> > I2C_FREQ_MODE_FAST_PLUS, /* up to 1 Mb/s */
> > };
>
> The operative words here are "up to".
I know, then none of these modes does not make sense.
>
> >> +};
> >> +
> >> +static int __devinit
> >> +nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
> >> +{
> >> + of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
> >> + if (!pdata->clk_freq) {
> >> + pr_warn("%s: Clock frequency not found\n", np->full_name);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
> >> + if (!pdata->slsu) {
> >> + pr_warn("%s: Data line delay not found\n", np->full_name);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
> >> + if (!pdata->tft) {
> >> + pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
> >> + if (!pdata->rft) {
> >> + pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
> >> + if (!pdata->timeout) {
> >> + pr_warn("%s: Timeout not found\n", np->full_name);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
> >> + pdata->sm = I2C_FREQ_MODE_FAST;
> >> + else
> >> + pdata->sm = I2C_FREQ_MODE_STANDARD;
> >
> > The controller has more modes like fast mode plus and high speed mode. You can't make
> > assumptions like this.
>
> These are currently unsupported. Read the comments in the file:
I wrote this code, I remember these comments very well :)
>
> > /*
> > * set the speed mode. Currently we support
> > * only standard and fast mode of operation
> > * TODO - support for fast mode plus (up to 1Mb/s)
> > * and high speed (up to 3.4 Mb/s)
> > */
>
> I can add a similar comment here if it make you feel better?
Please.
>
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int __devinit nmk_i2c_probe(struct platform_device *pdev)
> >> {
> >> int ret = 0;
> >> struct resource *res;
> >> - struct nmk_i2c_controller *pdata =
> >> - pdev->dev.platform_data;
> >> + struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
> >> + struct device_node *np = pdev->dev.of_node;
> >> struct nmk_i2c_dev *dev;
> >> struct i2c_adapter *adap;
> >>
> >> + if (np) {
> >> + if (!pdata) {
> >> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> >> + if (!pdata) {
> >> + ret = -ENOMEM;
> >> + goto err_no_mem;
> >> + }
> >> + }
> >> + ret = nmk_i2c_of_probe(np, pdata);
> >> + if (ret)
> >> + kfree(pdata);
> >> + }
> >> +
> >> + if (!pdata)
> >> + /* No i2c configuration found, using the default. */
> >> + pdata =&u8500_i2c;
> >
> > There are redundant fallback configurations exists in the driver. You might need to remove
> > those, if you want to fallback here.
>
> I only see a fall-back for clk_freq. Are there others?
No, I can see that it only exists in setup_i2c_controller(). Please fix.
>
> >> +
> >> dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
> >> if (!dev) {
> >> dev_err(&pdev->dev, "cannot allocate memory\n");
> >> @@ -1038,11 +1110,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
> >> return 0;
> >> }
> >>
> >> +static const struct of_device_id nmk_gpio_match[] = {
> >> + { .compatible = "st,nomadik-i2c", },
> >> + {},
> >> +};
> >> +
> >> static struct platform_driver nmk_i2c_driver = {
> >> .driver = {
> >> .owner = THIS_MODULE,
> >> .name = DRIVER_NAME,
> >> .pm =&nmk_i2c_pm,
> >> + .of_match_table = nmk_gpio_match,
> >> },
> >> .probe = nmk_i2c_probe,
> >> .remove = __devexit_p(nmk_i2c_remove),
> >> --
> >> 1.7.9.5
> >>
>
> Kind regards,
> Lee
>
> --
> Lee Jones
> Linaro ST-Ericsson Landing Team Lead
> M: +44 77 88 633 515
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
On 15/06/12 14:05, Srinidhi Kasagar wrote:
> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
>> On 15/06/12 12:50, Srinidhi Kasagar wrote:
>>> [...]
>>>>
>>>>
>>>> From: Lee Jones<[email protected]>
>>>> Date: Tue, 17 Apr 2012 16:04:13 +0100
>>>> Subject: [PATCH 1/1] i2c: Add Device Tree support to the Nomadik I2C driver
>>>>
>>>> Here we apply the bindings required for successful Device Tree
>>>> probing of the i2c-nomadik driver. We also apply a fall-back
>>>> configuration in case either one is not provided, or a required
>>>> element is missing from the one supplied.
>>>>
>>>> Cc: [email protected]
>>>> Signed-off-by: Lee Jones<[email protected]>
>>>> ---
>>>> drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 80 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
>>>> index a92440d..58e8114 100644
>>>> --- a/drivers/i2c/busses/i2c-nomadik.c
>>>> +++ b/drivers/i2c/busses/i2c-nomadik.c
>>>> @@ -23,6 +23,7 @@
>>>> #include<linux/io.h>
>>>> #include<linux/regulator/consumer.h>
>>>> #include<linux/pm_runtime.h>
>>>> +#include<linux/of.h>
>>>>
>>>> #include<plat/i2c.h>
>>>>
>>>> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
>>>> .functionality = nmk_i2c_functionality
>>>> };
>>>>
>>>> +static struct nmk_i2c_controller u8500_i2c = {
>>>> + /*
>>>> + * Slave data setup time; 250ns, 100ns, and 10ns, which
>>>> + * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>>>> + */
>>>> + .slsu = 0xe,
>>>> + .tft = 1, /* Tx FIFO threshold */
>>>> + .rft = 8, /* Rx FIFO threshold */
>>>> + .clk_freq = 100000, /* std. mode operation */
>>>> + .timeout = 200, /* Slave response timeout(ms) */
>>>> + .sm = I2C_FREQ_MODE_FAST,
>>>
>>> How is this possible? you are setting clk_freq as 100kb/s and mode
>>> as fast mode which is supposed to be 400kb/s.
>>
>> That's not how I read it:
>
> But it is not readable. It confuses people.
I understood it. :)
If you think it's unclear speak to the author, Linus. He's CC'ed.
>>> enum i2c_freq_mode {
>>> I2C_FREQ_MODE_STANDARD, /* up to 100 Kb/s */
>>> I2C_FREQ_MODE_FAST, /* up to 400 Kb/s */
>>> I2C_FREQ_MODE_HIGH_SPEED, /* up to 3.4 Mb/s */
>>> I2C_FREQ_MODE_FAST_PLUS, /* up to 1 Mb/s */
>>> };
>>
>> The operative words here are "up to".
>
> I know, then none of these modes does not make sense.
>
>>>> +};
>>>> +
>>>> +static int __devinit
>>>> +nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
>>>> +{
>>>> + of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
>>>> + if (!pdata->clk_freq) {
>>>> + pr_warn("%s: Clock frequency not found\n", np->full_name);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
>>>> + if (!pdata->slsu) {
>>>> + pr_warn("%s: Data line delay not found\n", np->full_name);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
>>>> + if (!pdata->tft) {
>>>> + pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
>>>> + if (!pdata->rft) {
>>>> + pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
>>>> + if (!pdata->timeout) {
>>>> + pr_warn("%s: Timeout not found\n", np->full_name);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
>>>> + pdata->sm = I2C_FREQ_MODE_FAST;
>>>> + else
>>>> + pdata->sm = I2C_FREQ_MODE_STANDARD;
>>>
>>> The controller has more modes like fast mode plus and high speed mode. You can't make
>>> assumptions like this.
>>
>> These are currently unsupported. Read the comments in the file:
>
> I wrote this code, I remember these comments very well :)
Yes, I saw. I thought that perhaps you'd forgotten. ;)
>>> /*
>>> * set the speed mode. Currently we support
>>> * only standard and fast mode of operation
>>> * TODO - support for fast mode plus (up to 1Mb/s)
>>> * and high speed (up to 3.4 Mb/s)
>>> */
>>
>> I can add a similar comment here if it make you feel better?
>
> Please.
No problem.
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int __devinit nmk_i2c_probe(struct platform_device *pdev)
>>>> {
>>>> int ret = 0;
>>>> struct resource *res;
>>>> - struct nmk_i2c_controller *pdata =
>>>> - pdev->dev.platform_data;
>>>> + struct nmk_i2c_controller *pdata = pdev->dev.platform_data;
>>>> + struct device_node *np = pdev->dev.of_node;
>>>> struct nmk_i2c_dev *dev;
>>>> struct i2c_adapter *adap;
>>>>
>>>> + if (np) {
>>>> + if (!pdata) {
>>>> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>>> + if (!pdata) {
>>>> + ret = -ENOMEM;
>>>> + goto err_no_mem;
>>>> + }
>>>> + }
>>>> + ret = nmk_i2c_of_probe(np, pdata);
>>>> + if (ret)
>>>> + kfree(pdata);
>>>> + }
>>>> +
>>>> + if (!pdata)
>>>> + /* No i2c configuration found, using the default. */
>>>> + pdata =&u8500_i2c;
>>>
>>> There are redundant fallback configurations exists in the driver. You might need to remove
>>> those, if you want to fallback here.
>>
>> I only see a fall-back for clk_freq. Are there others?
>
> No, I can see that it only exists in setup_i2c_controller(). Please fix.
I can remove it, no problem.
>>>> +
>>>> dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
>>>> if (!dev) {
>>>> dev_err(&pdev->dev, "cannot allocate memory\n");
>>>> @@ -1038,11 +1110,17 @@ static int __devexit nmk_i2c_remove(struct platform_device *pdev)
>>>> return 0;
>>>> }
>>>>
>>>> +static const struct of_device_id nmk_gpio_match[] = {
>>>> + { .compatible = "st,nomadik-i2c", },
>>>> + {},
>>>> +};
>>>> +
>>>> static struct platform_driver nmk_i2c_driver = {
>>>> .driver = {
>>>> .owner = THIS_MODULE,
>>>> .name = DRIVER_NAME,
>>>> .pm =&nmk_i2c_pm,
>>>> + .of_match_table = nmk_gpio_match,
>>>> },
>>>> .probe = nmk_i2c_probe,
>>>> .remove = __devexit_p(nmk_i2c_remove),
>>>> --
>>>> 1.7.9.5
>>>>
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, Jun 15, 2012 at 15:18:58 +0200, Lee Jones wrote:
> On 15/06/12 14:05, Srinidhi Kasagar wrote:
> > On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
> >> On 15/06/12 12:50, Srinidhi Kasagar wrote:
> >>> [...]
> >>>>
> >>>>
> >>>> From: Lee Jones<[email protected]>
> >>>> Date: Tue, 17 Apr 2012 16:04:13 +0100
> >>>> Subject: [PATCH 1/1] i2c: Add Device Tree support to the Nomadik I2C driver
> >>>>
> >>>> Here we apply the bindings required for successful Device Tree
> >>>> probing of the i2c-nomadik driver. We also apply a fall-back
> >>>> configuration in case either one is not provided, or a required
> >>>> element is missing from the one supplied.
> >>>>
> >>>> Cc: [email protected]
> >>>> Signed-off-by: Lee Jones<[email protected]>
> >>>> ---
> >>>> drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
> >>>> 1 file changed, 80 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> >>>> index a92440d..58e8114 100644
> >>>> --- a/drivers/i2c/busses/i2c-nomadik.c
> >>>> +++ b/drivers/i2c/busses/i2c-nomadik.c
> >>>> @@ -23,6 +23,7 @@
> >>>> #include<linux/io.h>
> >>>> #include<linux/regulator/consumer.h>
> >>>> #include<linux/pm_runtime.h>
> >>>> +#include<linux/of.h>
> >>>>
> >>>> #include<plat/i2c.h>
> >>>>
> >>>> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
> >>>> .functionality = nmk_i2c_functionality
> >>>> };
> >>>>
> >>>> +static struct nmk_i2c_controller u8500_i2c = {
> >>>> + /*
> >>>> + * Slave data setup time; 250ns, 100ns, and 10ns, which
> >>>> + * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
> >>>> + */
> >>>> + .slsu = 0xe,
And BTW, I forgot to mention: This slsu stuff is not needed at all.
It is required only in case of slave mode operation, which the
driver does not support. You can perhaps consider deprecating
this parameter from device tree list as well as from platform data.
> >>>> + .tft = 1, /* Tx FIFO threshold */
> >>>> + .rft = 8, /* Rx FIFO threshold */
> >>>> + .clk_freq = 100000, /* std. mode operation */
> >>>> + .timeout = 200, /* Slave response timeout(ms) */
> >>>> + .sm = I2C_FREQ_MODE_FAST,
> >>>
> >>> How is this possible? you are setting clk_freq as 100kb/s and mode
> >>> as fast mode which is supposed to be 400kb/s.
> >>
> >> That's not how I read it:
> >
> > But it is not readable. It confuses people.
>
> I understood it. :)
>
> If you think it's unclear speak to the author, Linus. He's CC'ed.
It is!
/srinidhi
On 15/06/12 14:37, Srinidhi Kasagar wrote:
> On Fri, Jun 15, 2012 at 15:18:58 +0200, Lee Jones wrote:
>> On 15/06/12 14:05, Srinidhi Kasagar wrote:
>>> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
>>>> On 15/06/12 12:50, Srinidhi Kasagar wrote:
>>>>> [...]
>>>>>>
>>>>>>
>>>>>> From: Lee Jones<[email protected]>
>>>>>> Date: Tue, 17 Apr 2012 16:04:13 +0100
>>>>>> Subject: [PATCH 1/1] i2c: Add Device Tree support to the Nomadik I2C driver
>>>>>>
>>>>>> Here we apply the bindings required for successful Device Tree
>>>>>> probing of the i2c-nomadik driver. We also apply a fall-back
>>>>>> configuration in case either one is not provided, or a required
>>>>>> element is missing from the one supplied.
>>>>>>
>>>>>> Cc: [email protected]
>>>>>> Signed-off-by: Lee Jones<[email protected]>
>>>>>> ---
>>>>>> drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
>>>>>> 1 file changed, 80 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
>>>>>> index a92440d..58e8114 100644
>>>>>> --- a/drivers/i2c/busses/i2c-nomadik.c
>>>>>> +++ b/drivers/i2c/busses/i2c-nomadik.c
>>>>>> @@ -23,6 +23,7 @@
>>>>>> #include<linux/io.h>
>>>>>> #include<linux/regulator/consumer.h>
>>>>>> #include<linux/pm_runtime.h>
>>>>>> +#include<linux/of.h>
>>>>>>
>>>>>> #include<plat/i2c.h>
>>>>>>
>>>>>> @@ -899,15 +900,86 @@ static const struct i2c_algorithm nmk_i2c_algo = {
>>>>>> .functionality = nmk_i2c_functionality
>>>>>> };
>>>>>>
>>>>>> +static struct nmk_i2c_controller u8500_i2c = {
>>>>>> + /*
>>>>>> + * Slave data setup time; 250ns, 100ns, and 10ns, which
>>>>>> + * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>>>>>> + */
>>>>>> + .slsu = 0xe,
>
> And BTW, I forgot to mention: This slsu stuff is not needed at all.
> It is required only in case of slave mode operation, which the
> driver does not support. You can perhaps consider deprecating
> this parameter from device tree list as well as from platform data.
Thanks. I will add it to my TODO list for a latter patch-set.
>>>>>> + .tft = 1, /* Tx FIFO threshold */
>>>>>> + .rft = 8, /* Rx FIFO threshold */
>>>>>> + .clk_freq = 100000, /* std. mode operation */
>>>>>> + .timeout = 200, /* Slave response timeout(ms) */
>>>>>> + .sm = I2C_FREQ_MODE_FAST,
>>>>>
>>>>> How is this possible? you are setting clk_freq as 100kb/s and mode
>>>>> as fast mode which is supposed to be 400kb/s.
>>>>
>>>> That's not how I read it:
>>>
>>> But it is not readable. It confuses people.
>>
>> I understood it. :)
>>
>> If you think it's unclear speak to the author, Linus. He's CC'ed.
>
> It is!
>
> /srinidhi
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Gah, what a thread...
On Fri, Jun 15, 2012 at 3:18 PM, Lee Jones <[email protected]> wrote:
> On 15/06/12 14:05, Srinidhi Kasagar wrote:
>> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
>>> On 15/06/12 12:50, Srinidhi Kasagar wrote:
>>>>> +static struct nmk_i2c_controller u8500_i2c = {
>>>>> + ? ? ? /*
>>>>> + ? ? ? ?* Slave data setup time; 250ns, 100ns, and 10ns, which
>>>>> + ? ? ? ?* is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>>>>> + ? ? ? ?*/
>>>>> + ? ? ? .slsu ? ? ? ? ? = 0xe,
>>>>> + ? ? ? .tft ? ? ? ? ? ?= 1, ? ? ?/* Tx FIFO threshold */
>>>>> + ? ? ? .rft ? ? ? ? ? ?= 8, ? ? ?/* Rx FIFO threshold */
>>>>> + ? ? ? .clk_freq ? ? ? = 100000, /* std. mode operation */
>>>>> + ? ? ? .timeout ? ? ? ?= 200, ? ?/* Slave response timeout(ms) */
>>>>> + ? ? ? .sm ? ? ? ? ? ? = I2C_FREQ_MODE_FAST,
>>>>
>>>> How is this possible? you are setting clk_freq as 100kb/s and mode
>>>> as fast mode which is supposed to be 400kb/s.
>>>
>>> That's not how I read it:
>>
>>
>> But it is not readable. It confuses people.
>
> I understood it. :)
>
> If you think it's unclear speak to the author, Linus. He's CC'ed.
Author of what? The i2c driver was written by Srinidhi. (The
MODULE_AUTHOR() clause should be a good hint...)
But it's true that board data for the ux500 kernel use
100000 Hz and I2C_FREQ_MODE_FAST, in the board-mop500.c
file, like this:
U8500_I2C_CONTROLLER(0, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(1, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(2, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(3, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
This is indeed a bit odd. But obviously it works... But reading some
fixes in another tree it seems it should look like this:
/*
* The board uses 4 i2c controllers, initialize all of
* them with slave data setup time of 250 ns,
* Tx & Rx FIFO threshold values as 1 and standard
* mode of operation
*/
U8500_I2C_CONTROLLER(0, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(1, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(2, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
U8500_I2C_CONTROLLER(3, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
Which makes *much* more sense.
I'll cook a separate patch for this or something.
Thanks,
Linus Walleij
On 17/06/12 18:43, Linus Walleij wrote:
> Gah, what a thread...
>
> On Fri, Jun 15, 2012 at 3:18 PM, Lee Jones<[email protected]> wrote:
>> On 15/06/12 14:05, Srinidhi Kasagar wrote:
>>> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
>>>> On 15/06/12 12:50, Srinidhi Kasagar wrote:
>
>>>>>> +static struct nmk_i2c_controller u8500_i2c = {
>>>>>> + /*
>>>>>> + * Slave data setup time; 250ns, 100ns, and 10ns, which
>>>>>> + * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>>>>>> + */
>>>>>> + .slsu = 0xe,
>>>>>> + .tft = 1, /* Tx FIFO threshold */
>>>>>> + .rft = 8, /* Rx FIFO threshold */
>>>>>> + .clk_freq = 100000, /* std. mode operation */
>>>>>> + .timeout = 200, /* Slave response timeout(ms) */
>>>>>> + .sm = I2C_FREQ_MODE_FAST,
>>>>>
>>>>> How is this possible? you are setting clk_freq as 100kb/s and mode
>>>>> as fast mode which is supposed to be 400kb/s.
>>>>
>>>> That's not how I read it:
>>>
>>> But it is not readable. It confuses people.
>>
>> I understood it. :)
>>
>> If you think it's unclear speak to the author, Linus. He's CC'ed.
>
> Author of what? The i2c driver was written by Srinidhi. (The
> MODULE_AUTHOR() clause should be a good hint...)
Actually, we appear to have our wires crossed. You wrote the board data
stuff below, which changes the mode from STANDARD to FAST, no doubt
using Srinidhi's struct explanation comments found in
arch/arm/plat-nomadik/include/plat/i2c.h.
> I'll cook a separate patch for this or something.
Cool, thanks Linus.
But you're correct in what you say Linus. Srinidhi, the comments which
are you say are confusing are the bits you are the author of:
e208c447bd728920e4f3d438a706344ea31249b9?
> But it's true that board data for the ux500 kernel use
> 100000 Hz and I2C_FREQ_MODE_FAST, in the board-mop500.c
> file, like this:
>
> U8500_I2C_CONTROLLER(0, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(1, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(2, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(3, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
>
> This is indeed a bit odd. But obviously it works... But reading some
> fixes in another tree it seems it should look like this:
>
> /*
> * The board uses 4 i2c controllers, initialize all of
> * them with slave data setup time of 250 ns,
> * Tx& Rx FIFO threshold values as 1 and standard
> * mode of operation
> */
> U8500_I2C_CONTROLLER(0, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(1, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(2, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
> U8500_I2C_CONTROLLER(3, 0xe, 1, 8, 400000, 200, I2C_FREQ_MODE_FAST);
>
> Which makes *much* more sense.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Jun 18, 2012 at 09:18:05 +0200, Lee Jones wrote:
> On 17/06/12 18:43, Linus Walleij wrote:
> > Gah, what a thread...
> >
> > On Fri, Jun 15, 2012 at 3:18 PM, Lee Jones<[email protected]> wrote:
> >> On 15/06/12 14:05, Srinidhi Kasagar wrote:
> >>> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
> >>>> On 15/06/12 12:50, Srinidhi Kasagar wrote:
> >
> >>>>>> +static struct nmk_i2c_controller u8500_i2c = {
> >>>>>> + /*
> >>>>>> + * Slave data setup time; 250ns, 100ns, and 10ns, which
> >>>>>> + * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
> >>>>>> + */
> >>>>>> + .slsu = 0xe,
> >>>>>> + .tft = 1, /* Tx FIFO threshold */
> >>>>>> + .rft = 8, /* Rx FIFO threshold */
> >>>>>> + .clk_freq = 100000, /* std. mode operation */
> >>>>>> + .timeout = 200, /* Slave response timeout(ms) */
> >>>>>> + .sm = I2C_FREQ_MODE_FAST,
> >>>>>
> >>>>> How is this possible? you are setting clk_freq as 100kb/s and mode
> >>>>> as fast mode which is supposed to be 400kb/s.
> >>>>
> >>>> That's not how I read it:
> >>>
> >>> But it is not readable. It confuses people.
> >>
> >> I understood it. :)
> >>
> >> If you think it's unclear speak to the author, Linus. He's CC'ed.
> >
> > Author of what? The i2c driver was written by Srinidhi. (The
> > MODULE_AUTHOR() clause should be a good hint...)
>
> Actually, we appear to have our wires crossed. You wrote the board data
> stuff below, which changes the mode from STANDARD to FAST, no doubt
> using Srinidhi's struct explanation comments found in
> arch/arm/plat-nomadik/include/plat/i2c.h.
>
> > I'll cook a separate patch for this or something.
>
> Cool, thanks Linus.
>
> But you're correct in what you say Linus. Srinidhi, the comments which
> are you say are confusing are the bits you are the author of:
> e208c447bd728920e4f3d438a706344ea31249b9?
yes, it is of course evident that it's my commit from git.
I think you are not catching my point. What I mean is:
fast mode devices are downward compatible and can communicate
with the devices with standard only mode. Those comments in that
enum says exactly that (with "up to" keyword). However the code
which configures the bus in fast mode and restrict the frequency
to operate in 100 kb/s without any reason, and this creates
confusion..
Perhaps, I think we need to remove one of these
parameters as configurable options and derive out the
frequency based on the mode of operation..
/srinidhi
On 18/06/12 08:58, Srinidhi Kasagar wrote:
> On Mon, Jun 18, 2012 at 09:18:05 +0200, Lee Jones wrote:
>> On 17/06/12 18:43, Linus Walleij wrote:
>>> Gah, what a thread...
>>>
>>> On Fri, Jun 15, 2012 at 3:18 PM, Lee Jones<[email protected]> wrote:
>>>> On 15/06/12 14:05, Srinidhi Kasagar wrote:
>>>>> On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
>>>>>> On 15/06/12 12:50, Srinidhi Kasagar wrote:
>>>
>>>>>>>> +static struct nmk_i2c_controller u8500_i2c = {
>>>>>>>> + /*
>>>>>>>> + * Slave data setup time; 250ns, 100ns, and 10ns, which
>>>>>>>> + * is 14, 6 and 2 respectively for a 48Mhz i2c clock.
>>>>>>>> + */
>>>>>>>> + .slsu = 0xe,
>>>>>>>> + .tft = 1, /* Tx FIFO threshold */
>>>>>>>> + .rft = 8, /* Rx FIFO threshold */
>>>>>>>> + .clk_freq = 100000, /* std. mode operation */
>>>>>>>> + .timeout = 200, /* Slave response timeout(ms) */
>>>>>>>> + .sm = I2C_FREQ_MODE_FAST,
>>>>>>>
>>>>>>> How is this possible? you are setting clk_freq as 100kb/s and mode
>>>>>>> as fast mode which is supposed to be 400kb/s.
>>>>>>
>>>>>> That's not how I read it:
>>>>>
>>>>> But it is not readable. It confuses people.
>>>>
>>>> I understood it. :)
>>>>
>>>> If you think it's unclear speak to the author, Linus. He's CC'ed.
>>>
>>> Author of what? The i2c driver was written by Srinidhi. (The
>>> MODULE_AUTHOR() clause should be a good hint...)
>>
>> Actually, we appear to have our wires crossed. You wrote the board data
>> stuff below, which changes the mode from STANDARD to FAST, no doubt
>> using Srinidhi's struct explanation comments found in
>> arch/arm/plat-nomadik/include/plat/i2c.h.
>>
>> > I'll cook a separate patch for this or something.
>>
>> Cool, thanks Linus.
>>
>> But you're correct in what you say Linus. Srinidhi, the comments which
>> are you say are confusing are the bits you are the author of:
>> e208c447bd728920e4f3d438a706344ea31249b9?
>
> yes, it is of course evident that it's my commit from git.
> I think you are not catching my point. What I mean is:
> fast mode devices are downward compatible and can communicate
> with the devices with standard only mode. Those comments in that
> enum says exactly that (with "up to" keyword). However the code
> which configures the bus in fast mode and restrict the frequency
> to operate in 100 kb/s without any reason, and this creates
> confusion..
>
> Perhaps, I think we need to remove one of these
> parameters as configurable options and derive out the
> frequency based on the mode of operation..
I think Linus is fixing the frequency issues in platform code and I have
done so from Device Tree, so I think we're hot-to-trot.
Linus, Srinidhi,
If you're happy with the patch now (not withstanding the removal of
slsu, which I'll do in a separate patch), can you ack it please? By the
way, which tree will this go through?
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog