From: Lee Jones <hidden> Date: 2012-08-23 15:01:48
Since initial support was provided for the Nomadik I2C driver, it
has been converted to an AMBA device. AMBA devices are probed in
a slightly different way to other devices, so we have to identify
them using an "arm,primecell" compatible string. As well as doing
just that, this patch specifies which regulators the controller
should use and requests a clock-speed. The latter is provided as
more of an example, as it's the same as the recently changed
default configuration.
Acked-by: Linus Walleij <redacted>
Signed-off-by: Lee Jones <redacted>
---
arch/arm/boot/dts/dbx5x0.dtsi | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
From: Lee Jones <hidden> Date: 2012-08-23 15:01:50
Here we apply the bindings required for successful Device Tree
probing of the i2c-nomadik driver.
Cc: linux-i2c at vger.kernel.org
Acked-by: srinidhi kasagar <redacted>
Signed-off-by: Lee Jones <redacted>
---
drivers/i2c/busses/i2c-nomadik.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
@@ -920,15 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {.sm=I2C_FREQ_MODE_FAST,};+staticvoidnmk_i2c_of_probe(structdevice_node*np,+structnmk_i2c_controller*pdata)+{+/* Provide the default configuration as a base. */+pdata=&u8500_i2c;++of_property_read_u32(np,"clock-frequency",(u32*)&pdata->clk_freq);++/* This driver only supports 'standard' and 'fast' modes of operation. */+if(pdata->clk_freq<=100000)+pdata->sm=I2C_FREQ_MODE_STANDARD;+else+pdata->sm=I2C_FREQ_MODE_FAST;+}+staticatomic_tadapter_id=ATOMIC_INIT(0);staticintnmk_i2c_probe(structamba_device*adev,conststructamba_id*id){intret=0;structnmk_i2c_controller*pdata=adev->dev.platform_data;+structdevice_node*np=adev->dev.of_node;structnmk_i2c_dev*dev;structi2c_adapter*adap;+if(np){+if(!pdata){+pdata=devm_kzalloc(&adev->dev,sizeof(*pdata),GFP_KERNEL);+if(!pdata){+ret=-ENOMEM;+gotoerr_no_mem;+}+}+nmk_i2c_of_probe(np,pdata);+}+if(!pdata)/* No i2c configuration found, using the default. */pdata=&u8500_i2c;
From: Lee Jones <hidden> Date: 2012-08-23 15:02:13
This document describes each of the non-standard Device Tree bindings
used in the Nomadik I2C driver.
Acked-by: Linus Walleij <redacted>
Signed-off-by: Lee Jones <redacted>
---
Documentation/devicetree/bindings/i2c/nomadik.txt | 23 +++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/nomadik.txt
On Thu, Aug 23, 2012 at 8:01 AM, Lee Jones [off-list ref] wrote:
Here we apply the bindings required for successful Device Tree
probing of the i2c-nomadik driver.
Cc: linux-i2c at vger.kernel.org
Acked-by: srinidhi kasagar <redacted>
Signed-off-by: Lee Jones <redacted>
From: Lee Jones <hidden> Date: 2012-08-31 10:36:44
On Mon, Aug 27, 2012 at 04:42:49PM -0700, Linus Walleij wrote:
On Thu, Aug 23, 2012 at 8:01 AM, Lee Jones [off-list ref] wrote:
quoted
Here we apply the bindings required for successful Device Tree
probing of the i2c-nomadik driver.
Cc: linux-i2c at vger.kernel.org
Acked-by: srinidhi kasagar <redacted>
Signed-off-by: Lee Jones <redacted>
Acked-by: Linus Walleij <redacted>
I'm assuming I still need Wolfram's Ack on this?
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
From: Wolfram Sang <hidden> Date: 2012-08-31 11:23:05
On Thu, Aug 23, 2012 at 04:01:27PM +0100, Lee Jones wrote:
Here we apply the bindings required for successful Device Tree
probing of the i2c-nomadik driver.
Cc: linux-i2c at vger.kernel.org
Acked-by: srinidhi kasagar <redacted>
Two acks? I'd think this cannot work for multiple reasons.
BTW, patch 2 and 3 should be merged. It is a lot easier to review the
code with the binding description together.
Is there some dependency other than updating the dts files? If not, I'd
like to pick up the patch via I2C.
quoted hunk
Signed-off-by: Lee Jones <redacted>
---
drivers/i2c/busses/i2c-nomadik.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
@@ -920,15 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {.sm=I2C_FREQ_MODE_FAST,};+staticvoidnmk_i2c_of_probe(structdevice_node*np,+structnmk_i2c_controller*pdata)+{+/* Provide the default configuration as a base. */+pdata=&u8500_i2c;
?????? I wonder how that could work... have you tested the patch?
+
+ /* This driver only supports 'standard' and 'fast' modes of operation. */
+ if (pdata->clk_freq <= 100000)
+ pdata->sm = I2C_FREQ_MODE_STANDARD;
Is standard == 100000 Hz?
+ else
+ pdata->sm = I2C_FREQ_MODE_FAST;
If those two are fixed frequencies, you should omit a warning if the
devicetree has a different frequency set and report which one is going
to be used actually.
+}
+
static atomic_t adapter_id = ATOMIC_INIT(0);
static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
{
int ret = 0;
struct nmk_i2c_controller *pdata = adev->dev.platform_data;
+ struct device_node *np = adev->dev.of_node;
struct nmk_i2c_dev *dev;
struct i2c_adapter *adap;
+ if (np) {
+ if (!pdata) {
+ pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ ret = -ENOMEM;
+ goto err_no_mem;
+ }
+ }
+ nmk_i2c_of_probe(np, pdata);
+ }
+
if (!pdata)
/* No i2c configuration found, using the default. */
pdata = &u8500_i2c;
--
1.7.9.5
From: Lee Jones <hidden> Date: 2012-08-31 12:04:21
Is there some dependency other than updating the dts files? If not, I'd
like to pick up the patch via I2C.
There's no other dependency. Feel free to take them into your tree.
quoted
+static void nmk_i2c_of_probe(struct device_node *np,
+ struct nmk_i2c_controller *pdata)
+{
+ /* Provide the default configuration as a base. */
+ pdata = &u8500_i2c;
?????? I wonder how that could work... have you tested the patch?
Wow, that's a great spot!
I have tested the patch, but I don't have any i2c devices, so can't
test full functionality. I, wrongly it seems, assumed there would be
a complaint from the I2C subsystem if any of the values seemed wrong.
What I will do before next submission is print out the entire pdata
structure to ensure it's populated in the correct way.
+
+ /* This driver only supports 'standard' and 'fast' modes of operation. */
+ if (pdata->clk_freq <= 100000)
+ pdata->sm = I2C_FREQ_MODE_STANDARD;
Is standard == 100000 Hz?
Well it depends on how you interpret the comments in:
include/linux/platform_data/i2c-nomadik.h
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 */
};
I guess your guess would be better than mine.
quoted
+ else
+ pdata->sm = I2C_FREQ_MODE_FAST;
If those two are fixed frequencies, you should omit a warning if the
devicetree has a different frequency set and report which one is going
to be used actually.
Well, again by the comments above I would say that in between values
were valid, but I'm willing to bow down to your knowledge if you think
they are fixed values?
Thanks for reviewing.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
From: Lee Jones <hidden> Date: 2012-08-31 12:23:34
Hopefully this is more to your liking:
Author: Lee Jones [off-list ref]
Date: Mon Aug 6 11:09:57 2012 +0100
i2c: nomadik: 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.
Cc: linux-i2c at vger.kernel.org
Signed-off-by: Lee Jones [off-list ref]
@@ -920,15 +921,41 @@ static struct nmk_i2c_controller u8500_i2c = {.sm=I2C_FREQ_MODE_FAST,};+staticvoidnmk_i2c_of_probe(structdevice_node*np,+structnmk_i2c_controller*pdata)+{+of_property_read_u32(np,"clock-frequency",&pdata->clk_freq);++/* This driver only supports 'standard' and 'fast' modes of operation. */+if(pdata->clk_freq<=100000)+pdata->sm=I2C_FREQ_MODE_STANDARD;+else+pdata->sm=I2C_FREQ_MODE_FAST;+}+staticatomic_tadapter_id=ATOMIC_INIT(0);staticintnmk_i2c_probe(structamba_device*adev,conststructamba_id*id){intret=0;structnmk_i2c_controller*pdata=adev->dev.platform_data;+structdevice_node*np=adev->dev.of_node;structnmk_i2c_dev*dev;structi2c_adapter*adap;+if(np){+if(!pdata){+pdata=devm_kzalloc(&adev->dev,sizeof(*pdata),GFP_KERNEL);+if(!pdata){+ret=-ENOMEM;+gotoerr_no_mem;+}+}+/* Provide the default configuration as a base. */+pdata=&u8500_i2c;+nmk_i2c_of_probe(np,pdata);+}+if(!pdata)/* No i2c configuration found, using the default. */pdata=&u8500_i2c;
On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones [off-list ref] wrote:
(...)
static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
{
int ret = 0;
struct nmk_i2c_controller *pdata = adev->dev.platform_data;
+ struct device_node *np = adev->dev.of_node;
struct nmk_i2c_dev *dev;
struct i2c_adapter *adap;
+ if (np) {
+ if (!pdata) {
So, if no pdata is provided, we go on to allocate some ...
+ pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ ret = -ENOMEM;
+ goto err_no_mem;
+ }
+ }
+ /* Provide the default configuration as a base. */
+ pdata = &u8500_i2c;
Then you just override that pointer with a pointer to the local config.
+ nmk_i2c_of_probe(np, pdata);
+ }
+
if (!pdata)
/* No i2c configuration found, using the default. */
pdata = &u8500_i2c;
This in it's entirety does not look sound. I *think* this is what you
want to do,
replace all of the above codde (including the last if (!pdata) clause) with:
if (!pdata) {
/* If no platform data passed in, use the default configuration as
a base. */
pdata = &u8500_i2c;
if (np)
/* Further, if we have a DT node, override the default with this */
nmk_i2c_of_probe(np, pdata);
}
This makes any passed pdata take precedence, else default pdata
complemented with DT info. Which is what we want.
Yours,
Linus Walleij
From: Wolfram Sang <hidden> Date: 2012-09-03 09:44:56
On Mon, Sep 03, 2012 at 11:22:28AM +0200, Linus Walleij wrote:
On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones [off-list ref] wrote:
(...)
quoted
static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
{
int ret = 0;
struct nmk_i2c_controller *pdata = adev->dev.platform_data;
+ struct device_node *np = adev->dev.of_node;
struct nmk_i2c_dev *dev;
struct i2c_adapter *adap;
+ if (np) {
+ if (!pdata) {
So, if no pdata is provided, we go on to allocate some ...
quoted
+ pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ ret = -ENOMEM;
+ goto err_no_mem;
+ }
+ }
+ /* Provide the default configuration as a base. */
+ pdata = &u8500_i2c;
Then you just override that pointer with a pointer to the local config.
quoted
+ nmk_i2c_of_probe(np, pdata);
+ }
+
if (!pdata)
/* No i2c configuration found, using the default. */
pdata = &u8500_i2c;
This in it's entirety does not look sound. I *think* this is what you
want to do,
replace all of the above codde (including the last if (!pdata) clause) with:
if (!pdata) {
/* If no platform data passed in, use the default configuration as
a base. */
pdata = &u8500_i2c;
if (np)
/* Further, if we have a DT node, override the default with this */
nmk_i2c_of_probe(np, pdata);
}
This makes any passed pdata take precedence, else default pdata
complemented with DT info. Which is what we want.
No. of_probe modifies pdata which in this case the default config which
might already be in use. So, you will get problems if you have two
instances with different configuration. So, we need to allocate memory
but copy the content of the default data. The patch above just copies
the pointer which is bogus.
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120903/e9711070/attachment-0001.sig>
From: Lee Jones <hidden> Date: 2012-09-03 09:50:16
On Mon, Sep 03, 2012 at 11:44:48AM +0200, Wolfram Sang wrote:
On Mon, Sep 03, 2012 at 11:22:28AM +0200, Linus Walleij wrote:
quoted
On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones [off-list ref] wrote:
(...)
quoted
static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
{
int ret = 0;
struct nmk_i2c_controller *pdata = adev->dev.platform_data;
+ struct device_node *np = adev->dev.of_node;
struct nmk_i2c_dev *dev;
struct i2c_adapter *adap;
+ if (np) {
+ if (!pdata) {
So, if no pdata is provided, we go on to allocate some ...
quoted
+ pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ ret = -ENOMEM;
+ goto err_no_mem;
+ }
+ }
+ /* Provide the default configuration as a base. */
+ pdata = &u8500_i2c;
Then you just override that pointer with a pointer to the local config.
quoted
+ nmk_i2c_of_probe(np, pdata);
+ }
+
if (!pdata)
/* No i2c configuration found, using the default. */
pdata = &u8500_i2c;
This in it's entirety does not look sound. I *think* this is what you
want to do,
replace all of the above codde (including the last if (!pdata) clause) with:
if (!pdata) {
/* If no platform data passed in, use the default configuration as
a base. */
pdata = &u8500_i2c;
if (np)
/* Further, if we have a DT node, override the default with this */
nmk_i2c_of_probe(np, pdata);
}
This makes any passed pdata take precedence, else default pdata
complemented with DT info. Which is what we want.
No. of_probe modifies pdata which in this case the default config which
might already be in use. So, you will get problems if you have two
instances with different configuration. So, we need to allocate memory
but copy the content of the default data. The patch above just copies
the pointer which is bogus.
Agreed. I'll fixup.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
From: Lee Jones <hidden> Date: 2012-09-03 10:07:10
Author: Lee Jones [off-list ref]
Date: Mon Aug 6 11:09:57 2012 +0100
i2c: nomadik: 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.
Cc: linux-i2c at vger.kernel.org
Signed-off-by: Lee Jones [off-list ref]
@@ -920,15 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {.sm=I2C_FREQ_MODE_FAST,};+staticvoidnmk_i2c_of_probe(structdevice_node*np,+structnmk_i2c_controller*pdata)+{+of_property_read_u32(np,"clock-frequency",&pdata->clk_freq);++/* This driver only supports 'standard' and 'fast' modes of operation. */+if(pdata->clk_freq<=100000)+pdata->sm=I2C_FREQ_MODE_STANDARD;+else+pdata->sm=I2C_FREQ_MODE_FAST;+}+staticatomic_tadapter_id=ATOMIC_INIT(0);staticintnmk_i2c_probe(structamba_device*adev,conststructamba_id*id){intret=0;structnmk_i2c_controller*pdata=adev->dev.platform_data;+structdevice_node*np=adev->dev.of_node;structnmk_i2c_dev*dev;structi2c_adapter*adap;+if(np){+if(!pdata){+pdata=devm_kzalloc(&adev->dev,sizeof(*pdata),GFP_KERNEL);+if(!pdata){+ret=-ENOMEM;+gotoerr_no_mem;+}+}+/* Provide the default configuration as a base. */+memcpy(pdata,&u8500_i2c,sizeof(structnmk_i2c_controller));++nmk_i2c_of_probe(np,pdata);+}+if(!pdata)/* No i2c configuration found, using the default. */pdata=&u8500_i2c;
On Mon, Sep 3, 2012 at 12:07 PM, Lee Jones [off-list ref] wrote:
(...)
+ if (np) {
+ if (!pdata) {
+ pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ ret = -ENOMEM;
+ goto err_no_mem;
+ }
+ }
+ /* Provide the default configuration as a base. */
+ memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));
Here you blank out any pdata passed from say a board file or
whatever if pdata != NULL.
+
+ nmk_i2c_of_probe(np, pdata);
+ }
+
if (!pdata)
/* No i2c configuration found, using the default. */
pdata = &u8500_i2c;
So this is still wrong, if pdata is passed to the driver it will
not override the DT, you have the semantics the other way
around, DT overrides pdata.
Look at the switch statement in my previous comment,
just add the allocations and a memcpy() and it still holds:
if (!pdata) {
if (np) {
pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata) {
ret = -ENOMEM;
goto err_no_mem;
}
}
/* Provide the default configuration as a base. */
memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));
nmk_i2c_of_probe(np, pdata);
} else
/* Just use the static pdata */
pdata = &u8500_i2c;
}
Yours,
Linus Walleij
From: Lee Jones <hidden> Date: 2012-09-03 11:11:07
You're right. I rushed it to get it out the door, as I'm working on
something else.
Leave it with me. I'll spend more time on the semantics before posting
again.
On 3 September 2012 12:07, Linus Walleij [off-list ref] wrote:
On Mon, Sep 3, 2012 at 12:07 PM, Lee Jones [off-list ref] wrote:
(...)
quoted
+ if (np) {
+ if (!pdata) {
+ pdata = devm_kzalloc(&adev->dev, sizeof(*pdata),
GFP_KERNEL);
quoted
+ if (!pdata) {
+ ret = -ENOMEM;
+ goto err_no_mem;
+ }
+ }
+ /* Provide the default configuration as a base. */
+ memcpy(pdata, &u8500_i2c, sizeof(struct
nmk_i2c_controller));
Here you blank out any pdata passed from say a board file or
whatever if pdata != NULL.
quoted
+
+ nmk_i2c_of_probe(np, pdata);
+ }
+
if (!pdata)
/* No i2c configuration found, using the default. */
pdata = &u8500_i2c;
So this is still wrong, if pdata is passed to the driver it will
not override the DT, you have the semantics the other way
around, DT overrides pdata.
Look at the switch statement in my previous comment,
just add the allocations and a memcpy() and it still holds:
if (!pdata) {
if (np) {
pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata) {
ret = -ENOMEM;
goto err_no_mem;
}
}
/* Provide the default configuration as a base. */
memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));
nmk_i2c_of_probe(np, pdata);
} else
/* Just use the static pdata */
pdata = &u8500_i2c;
}
Yours,
Linus Walleij
From: Lee Jones <hidden> Date: 2012-09-03 11:32:35
Looking at this again ...
On 3 September 2012 12:07, Linus Walleij [off-list ref] wrote:
On Mon, Sep 3, 2012 at 12:07 PM, Lee Jones [off-list ref] wrote:
(...)
quoted
+ if (np) {
+ if (!pdata) {
+ pdata = devm_kzalloc(&adev->dev, sizeof(*pdata),
GFP_KERNEL);
quoted
+ if (!pdata) {
+ ret = -ENOMEM;
+ goto err_no_mem;
+ }
+ }
+ /* Provide the default configuration as a base. */
+ memcpy(pdata, &u8500_i2c, sizeof(struct
nmk_i2c_controller));
Here you blank out any pdata passed from say a board file or
whatever if pdata != NULL.
Right, this should be in 'if (!pdata) {}' . I'll correct this now.
quoted
+
+ nmk_i2c_of_probe(np, pdata);
+ }
+
if (!pdata)
/* No i2c configuration found, using the default. */
pdata = &u8500_i2c;
So this is still wrong, if pdata is passed to the driver it will
not override the DT, you have the semantics the other way
around, DT overrides pdata.
This is correct, however. If there is DT, it should override any platform
data.
Actually, if there is DT then no platform data should be passed in the
first place.
Look at the switch statement in my previous comment,
just add the allocations and a memcpy() and it still holds:
if (!pdata) {
if (np) {
pdata = devm_kzalloc(&adev->dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata) {
ret = -ENOMEM;
goto err_no_mem;
}
}
/* Provide the default configuration as a base. */
memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));
nmk_i2c_of_probe(np, pdata);
} else
/* Just use the static pdata */
pdata = &u8500_i2c;
}
No, this is wrong. Platform data should not override DT.
If DT is enabled and passed, it should have highest priority.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120903/8487d887/attachment-0001.html>
On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones [off-list ref] wrote:
No, this is wrong. Platform data should not override DT.
If DT is enabled and passed, it should have highest priority.
Oh is that so.
Rob: do we have a clear consensus on this? Then we should document
it in Documentation/devicetree/usage-model.txt.
(That document isn't part of the binding I believe, so we could define
Linux-specific behaviours in it.)
I always thought it was the other way around, that pdata took priority.
Usecase: hardcoded bootloader passer erroneous DT to a platform.
No way out. What to do? Override with pdata.
Yours,
Linus Walleij
From: Lee Jones <hidden> Date: 2012-09-03 12:34:39
On Mon, Sep 03, 2012 at 01:58:04PM +0200, Linus Walleij wrote:
On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones [off-list ref] wrote:
quoted
No, this is wrong. Platform data should not override DT.
If DT is enabled and passed, it should have highest priority.
Oh is that so.
:)
Rob: do we have a clear consensus on this? Then we should document
it in Documentation/devicetree/usage-model.txt.
(That document isn't part of the binding I believe, so we could define
Linux-specific behaviours in it.)
I always thought it was the other way around, that pdata took priority.
Usecase: hardcoded bootloader passer erroneous DT to a platform.
No way out. What to do? Override with pdata.
Yours,
Linus Walleij
Hmmm... I see your point, but this won't work.
When booting DT booting take a different path and no platform data
is passed. We can't boot DT AND register devices with platform data
or else we will double probe every device. The only way to pass
pdata when booting with DT is with AUX_DATA() and that's a hack to
get around things we don't have support for yet. Up until now that
has been DMA bindings, clock and pinctrl names and call-backs.
If DT is corrupt or missing the kernel will boot using platform
data, but np will always be NULL, so we don't have the problem you
were alluding to above.
Let me know if I didn't explain that well enough and I will have
another go.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Sep 3, 2012 at 2:34 PM, Lee Jones [off-list ref] wrote:
When booting DT booting take a different path and no platform data
is passed. We can't boot DT AND register devices with platform data
or else we will double probe every device. The only way to pass
pdata when booting with DT is with AUX_DATA() and that's a hack to
get around things we don't have support for yet. Up until now that
has been DMA bindings, clock and pinctrl names and call-backs.
So if we pass some augmented platform data using AUX_DATA()
that appears as pdata in this case, and gets discarded.
Thus we cannot use AUX_DATA() to override a broken, as in
"the interrupt number is wrong" device tree.
If DT is corrupt or missing the kernel will boot using platform
data, but np will always be NULL, so we don't have the problem you
were alluding to above.
That was not the problem I had in mind.
I had a valid, but incorrect device tree in mind. I.e the device
is there, but with wrong base address, or wrong IRQ number.
If pdata takes precedence, we can use AUX_DATA() to
override such errors from the platform, since drivers/of/platform.c
helpfully pokes in the auxdata as the platform data.
I thought this was one of the reasons why auxdata exist
at all.
Or is the proper solution to runtime-patch the device tree
per se in such cases? How is that actually done then?
Yours,
Linus Walleij
From: Lee Jones <hidden> Date: 2012-09-03 13:28:57
On Mon, Sep 03, 2012 at 03:19:13PM +0200, Linus Walleij wrote:
On Mon, Sep 3, 2012 at 2:34 PM, Lee Jones [off-list ref] wrote:
quoted
When booting DT booting take a different path and no platform data
is passed. We can't boot DT AND register devices with platform data
or else we will double probe every device. The only way to pass
pdata when booting with DT is with AUX_DATA() and that's a hack to
get around things we don't have support for yet. Up until now that
has been DMA bindings, clock and pinctrl names and call-backs.
So if we pass some augmented platform data using AUX_DATA()
that appears as pdata in this case, and gets discarded.
Thus we cannot use AUX_DATA() to override a broken, as in
"the interrupt number is wrong" device tree.
No, you cannot to that. You'd have to fix it in DT (which is easier).
quoted
If DT is corrupt or missing the kernel will boot using platform
data, but np will always be NULL, so we don't have the problem you
were alluding to above.
That was not the problem I had in mind.
I had a valid, but incorrect device tree in mind. I.e the device
is there, but with wrong base address, or wrong IRQ number.
If pdata takes precedence, we can use AUX_DATA() to
override such errors from the platform, since drivers/of/platform.c
helpfully pokes in the auxdata as the platform data.
Yes, that's what happens.
I thought this was one of the reasons why auxdata exist
at all.
I don't think so. I've been told that AUXDATA is just a hack. My
aim is to rid the boardfile of all AUXDATA entries.
Or is the proper solution to runtime-patch the device tree
per se in such cases? How is that actually done then?
No, you can't do that either. DT is only read once at boot-time.
The correct solution would be to fix the broken DT.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren [off-list ref] wrote:
On 09/03/2012 05:58 AM, Linus Walleij wrote:
quoted
On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones [off-list ref] wrote:
quoted
No, this is wrong. Platform data should not override DT.
If DT is enabled and passed, it should have highest priority.
No, that's wrong. If platform data is specified, it overrides DT, so
that if the DT needs any fixup, it can be provided using platform data.
Thanks Stephen, now there are two of us saying this, Lee please
follow this design pattern.
(Unless Rob/Grant start shouting counter-orders...)
Yours,
Linus Walleij
From: Rob Herring <hidden> Date: 2012-09-03 15:09:42
On 09/03/2012 09:35 AM, Linus Walleij wrote:
On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren [off-list ref] wrote:
quoted
On 09/03/2012 05:58 AM, Linus Walleij wrote:
quoted
On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones [off-list ref] wrote:
quoted
No, this is wrong. Platform data should not override DT.
If DT is enabled and passed, it should have highest priority.
No, that's wrong. If platform data is specified, it overrides DT, so
that if the DT needs any fixup, it can be provided using platform data.
Thanks Stephen, now there are two of us saying this, Lee please
follow this design pattern.
(Unless Rob/Grant start shouting counter-orders...)
Ideally, you only use DT or platform_data and you override DT with a new
DTB. Hopefully we can ultimately remove platform_data or all but parts
that can't be described in DT (i.e. function callouts).
But if you are handling both, then I agree that platform_data should
override DT.
Rob
From: Lee Jones <hidden> Date: 2012-09-03 15:20:23
On Mon, Sep 03, 2012 at 10:09:34AM -0500, Rob Herring wrote:
On 09/03/2012 09:35 AM, Linus Walleij wrote:
quoted
On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren [off-list ref] wrote:
quoted
On 09/03/2012 05:58 AM, Linus Walleij wrote:
quoted
On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones [off-list ref] wrote:
quoted
No, this is wrong. Platform data should not override DT.
If DT is enabled and passed, it should have highest priority.
No, that's wrong. If platform data is specified, it overrides DT, so
that if the DT needs any fixup, it can be provided using platform data.
Thanks Stephen, now there are two of us saying this, Lee please
follow this design pattern.
(Unless Rob/Grant start shouting counter-orders...)
Ideally, you only use DT or platform_data and you override DT with a new
DTB. Hopefully we can ultimately remove platform_data or all but parts
that can't be described in DT (i.e. function callouts).
Exactly. I don't believe that AUX_DATA() should be used as a facility to
override DT settings from platform_data.
But if you are handling both, then I agree that platform_data should
override DT.
I do agree with this, but I haven't stumbled over such a use-case yet.
I have only provided; clock names, DMA settings and call-back information
via AUX_DATA() thus far, and those are being removed too when a) the
correct bindings are mainlined and b) I have the time.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
But if you are handling both, then I agree that platform_data should
override DT.
I do agree with this, but I haven't stumbled over such a use-case yet.
I have only provided; clock names, DMA settings and call-back information
via AUX_DATA() thus far, and those are being removed too when a) the
correct bindings are mainlined and b) I have the time.
I'd prefer if you just disallow the case where pdata and DT have conflicting
information. We don't seem to have a clear rule that is enforced over the
kernel, so I don't think we can rely on either one taking precedence over
the other in general.
In this particular case, we don't have a single board file providing a
struct nmk_i2c_controller definition for platform data, so the best way
to handle this IMHO is to remove the header file with the platform
data definition, and just encode the defaults in the driver.
Arnd
On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann [off-list ref] wrote:
In this particular case, we don't have a single board file providing a
struct nmk_i2c_controller definition for platform data, so the best way
to handle this IMHO is to remove the header file with the platform
data definition, and just encode the defaults in the driver.
Alessandro Rubini is actively working on bridging this (and
other amba_device primecells) to PCI, that is the reason why it
was recently converted to an amba_device. How is he then supposed to
get the proper parameters into the driver? Note that the PCI ID
is no help at all since the parameters depend on what is connected
to the I2C bus, not on what it itself is connected to. Isn't platform data
used in such cases?
Yours,
Linus Walleij
Alessandro Rubini is actively working on bridging this (and
other amba_device primecells) to PCI, that is the reason why it
was recently converted to an amba_device.
Yes, I've been inactive for a while but I'm on it right now.
How is he then supposed to get the proper parameters into the
driver? Note that the PCI ID is no help at all since the parameters
depend on what is connected to the I2C bus, not on what it itself is
connected to. Isn't platform data used in such cases?
I'm using platform data currently, but Davide Ciminaghi is actively
working to convert the configuration to device-tree: the way we pass
platform data to the pci device (and thus amba) is not considered
acceptable by Peter Anvin.
I'm thus asking Davide if he's happy to remove the platform data
configuration path right now (I personally wouldn't be very happy, but
I acknowledge it should happen, sooner or later).
/alessandro
From: Lee Jones <hidden> Date: 2012-09-05 06:41:14
On Tue, Sep 04, 2012 at 07:27:10PM +0200, Linus Walleij wrote:
On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann [off-list ref] wrote:
quoted
In this particular case, we don't have a single board file providing a
struct nmk_i2c_controller definition for platform data, so the best way
to handle this IMHO is to remove the header file with the platform
data definition, and just encode the defaults in the driver.
Alessandro Rubini is actively working on bridging this (and
other amba_device primecells) to PCI, that is the reason why it
was recently converted to an amba_device. How is he then supposed to
get the proper parameters into the driver? Note that the PCI ID
is no help at all since the parameters depend on what is connected
to the I2C bus, not on what it itself is connected to. Isn't platform data
used in such cases?
So why can't Alessandro continue to use Platform Data in the normal way?
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Sep 5, 2012 at 8:41 AM, Lee Jones [off-list ref] wrote:
On Tue, Sep 04, 2012 at 07:27:10PM +0200, Linus Walleij wrote:
quoted
On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann [off-list ref] wrote:
quoted
In this particular case, we don't have a single board file providing a
struct nmk_i2c_controller definition for platform data, so the best way
to handle this IMHO is to remove the header file with the platform
data definition, and just encode the defaults in the driver.
Alessandro Rubini is actively working on bridging this (and
other amba_device primecells) to PCI, that is the reason why it
was recently converted to an amba_device. How is he then supposed to
get the proper parameters into the driver? Note that the PCI ID
is no help at all since the parameters depend on what is connected
to the I2C bus, not on what it itself is connected to. Isn't platform data
used in such cases?
So why can't Alessandro continue to use Platform Data in the normal way?
He probably can, this is not an argument about that, what I am worried
about is Arnd's suggestion to delete the platform data header if there
are potential users of it.
Yours,
Linus Walleij
From: Lee Jones <hidden> Date: 2012-09-05 07:33:27
Author: Lee Jones [off-list ref]
Date: Mon Aug 6 11:09:57 2012 +0100
i2c: nomadik: 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.
Cc: linux-i2c at vger.kernel.org
Signed-off-by: Lee Jones [off-list ref]
@@ -920,18 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {.sm=I2C_FREQ_MODE_FAST,};+staticvoidnmk_i2c_of_probe(structdevice_node*np,+structnmk_i2c_controller*pdata)+{+of_property_read_u32(np,"clock-frequency",&pdata->clk_freq);++/* This driver only supports 'standard' and 'fast' modes of operation. */+if(pdata->clk_freq<=100000)+pdata->sm=I2C_FREQ_MODE_STANDARD;+else+pdata->sm=I2C_FREQ_MODE_FAST;+}+staticatomic_tadapter_id=ATOMIC_INIT(0);staticintnmk_i2c_probe(structamba_device*adev,conststructamba_id*id){intret=0;structnmk_i2c_controller*pdata=adev->dev.platform_data;+structdevice_node*np=adev->dev.of_node;structnmk_i2c_dev*dev;structi2c_adapter*adap;-if(!pdata)-/* No i2c configuration found, using the default. */-pdata=&u8500_i2c;+if(!pdata){+if(np){+pdata=devm_kzalloc(&adev->dev,sizeof(*pdata),GFP_KERNEL);+if(!pdata){+ret=-ENOMEM;+gotoerr_no_mem;+}+/* Provide the default configuration as a base. */+memcpy(pdata,&u8500_i2c,sizeof(structnmk_i2c_controller));+nmk_i2c_of_probe(np,pdata);+}else+/* No i2c configuration found, using the default. */+pdata=&u8500_i2c;+}dev=kzalloc(sizeof(structnmk_i2c_dev),GFP_KERNEL);if(!dev){
On Wed, Sep 5, 2012 at 9:33 AM, Lee Jones [off-list ref] wrote:
Author: Lee Jones [off-list ref]
Date: Mon Aug 6 11:09:57 2012 +0100
i2c: nomadik: 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.
Cc: linux-i2c at vger.kernel.org
Signed-off-by: Lee Jones [off-list ref]
Excellent :-)
Reviewed-by: Linus Walleij <redacted>
Wolfram are you picking this up?
Yours,
Linus Walleij