From: Lee Jones <redacted>
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: linux-i2c at vger.kernel.org
Signed-off-by: Lee Jones <redacted>
---
drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
1 file changed, 80 insertions(+), 2 deletions(-)
From: Lee Jones <hidden> Date: 2012-06-15 12:45:18
On 15/06/12 12:50, Srinidhi Kasagar wrote:
[...]
quoted
From: Lee Jones<redacted>
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: linux-i2c at vger.kernel.org
Signed-off-by: Lee Jones<redacted>
---
drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
1 file changed, 80 insertions(+), 2 deletions(-)
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".
quoted
+};
+
+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?
quoted
+
+ 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?
quoted
+
dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
if (!dev) {
dev_err(&pdev->dev, "cannot allocate memory\n");
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:
quoted
[...]
quoted
From: Lee Jones<redacted>
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: linux-i2c at vger.kernel.org
Signed-off-by: Lee Jones<redacted>
---
drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
1 file changed, 80 insertions(+), 2 deletions(-)
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.
quoted
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.
quoted
quoted
+};
+
+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 :)
quoted
/*
* 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.
quoted
quoted
+
+ 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.
quoted
quoted
+
dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
if (!dev) {
dev_err(&pdev->dev, "cannot allocate memory\n");
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
From: Lee Jones <hidden> Date: 2012-06-15 13:19:05
On 15/06/12 14:05, Srinidhi Kasagar wrote:
On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
quoted
On 15/06/12 12:50, Srinidhi Kasagar wrote:
quoted
[...]
quoted
From: Lee Jones<redacted>
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: linux-i2c at vger.kernel.org
Signed-off-by: Lee Jones<redacted>
---
drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
1 file changed, 80 insertions(+), 2 deletions(-)
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.
quoted
quoted
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.
>
quoted
quoted
quoted
+};
+
+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. ;)
quoted
quoted
/*
* 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.
quoted
quoted
quoted
+
+ 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.
quoted
quoted
quoted
+
dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
if (!dev) {
dev_err(&pdev->dev, "cannot allocate memory\n");
--
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:
quoted
On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
quoted
On 15/06/12 12:50, Srinidhi Kasagar wrote:
quoted
[...]
quoted
From: Lee Jones<redacted>
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: linux-i2c at vger.kernel.org
Signed-off-by: Lee Jones<redacted>
---
drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
1 file changed, 80 insertions(+), 2 deletions(-)
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.
From: Lee Jones <hidden> Date: 2012-06-15 13:58:12
On 15/06/12 14:37, Srinidhi Kasagar wrote:
On Fri, Jun 15, 2012 at 15:18:58 +0200, Lee Jones wrote:
quoted
On 15/06/12 14:05, Srinidhi Kasagar wrote:
quoted
On Fri, Jun 15, 2012 at 14:45:11 +0200, Lee Jones wrote:
quoted
On 15/06/12 12:50, Srinidhi Kasagar wrote:
quoted
[...]
quoted
From: Lee Jones<redacted>
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: linux-i2c at vger.kernel.org
Signed-off-by: Lee Jones<redacted>
---
drivers/i2c/busses/i2c-nomadik.c | 82 +++++++++++++++++++++++++++++++++++++-
1 file changed, 80 insertions(+), 2 deletions(-)
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.
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
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
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
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
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