* [PATCH 1/3] ARM: ux500: Add i2c configurations to the Device Tree for DB8500 based devices
@ 2012-08-23 15:01 Lee Jones
2012-08-23 15:01 ` [PATCH 2/3] Documentation: Device Tree binding information for i2c-nomadik driver Lee Jones
2012-08-23 15:01 ` [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver Lee Jones
0 siblings, 2 replies; 28+ messages in thread
From: Lee Jones @ 2012-08-23 15:01 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: STEricsson_nomadik_linux, linus.walleij, arnd, w.sang, Lee Jones
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 <linus.walleij@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
arch/arm/boot/dts/dbx5x0.dtsi | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/arch/arm/boot/dts/dbx5x0.dtsi b/arch/arm/boot/dts/dbx5x0.dtsi
index 51209a8..3708a6b 100644
--- a/arch/arm/boot/dts/dbx5x0.dtsi
+++ b/arch/arm/boot/dts/dbx5x0.dtsi
@@ -480,43 +480,58 @@
};
i2c@80004000 {
- compatible = "stericsson,db8500-i2c", "st,nomadik-i2c";
+ compatible = "stericsson,db8500-i2c", "st,nomadik-i2c", "arm,primecell";
reg = <0x80004000 0x1000>;
interrupts = <0 21 0x4>;
#address-cells = <1>;
#size-cells = <0>;
+ v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <400000>;
};
i2c@80122000 {
- compatible = "stericsson,db8500-i2c", "st,nomadik-i2c";
+ compatible = "stericsson,db8500-i2c", "st,nomadik-i2c", "arm,primecell";
reg = <0x80122000 0x1000>;
interrupts = <0 22 0x4>;
#address-cells = <1>;
#size-cells = <0>;
+ v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <400000>;
};
i2c@80128000 {
- compatible = "stericsson,db8500-i2c", "st,nomadik-i2c";
+ compatible = "stericsson,db8500-i2c", "st,nomadik-i2c", "arm,primecell";
reg = <0x80128000 0x1000>;
interrupts = <0 55 0x4>;
#address-cells = <1>;
#size-cells = <0>;
+ v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <400000>;
};
i2c@80110000 {
- compatible = "stericsson,db8500-i2c", "st,nomadik-i2c";
+ compatible = "stericsson,db8500-i2c", "st,nomadik-i2c", "arm,primecell";
reg = <0x80110000 0x1000>;
interrupts = <0 12 0x4>;
#address-cells = <1>;
#size-cells = <0>;
+ v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <400000>;
};
i2c@8012a000 {
- compatible = "stericsson,db8500-i2c", "st,nomadik-i2c";
+ compatible = "stericsson,db8500-i2c", "st,nomadik-i2c", "arm,primecell";
reg = <0x8012a000 0x1000>;
interrupts = <0 51 0x4>;
#address-cells = <1>;
#size-cells = <0>;
+ v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <400000>;
};
ssp@80002000 {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/3] Documentation: Device Tree binding information for i2c-nomadik driver
2012-08-23 15:01 [PATCH 1/3] ARM: ux500: Add i2c configurations to the Device Tree for DB8500 based devices Lee Jones
@ 2012-08-23 15:01 ` Lee Jones
2012-08-23 15:01 ` [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver Lee Jones
1 sibling, 0 replies; 28+ messages in thread
From: Lee Jones @ 2012-08-23 15:01 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: STEricsson_nomadik_linux, linus.walleij, arnd, w.sang, Lee Jones
This document describes each of the non-standard Device Tree bindings
used in the Nomadik I2C driver.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
Documentation/devicetree/bindings/i2c/nomadik.txt | 23 +++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/nomadik.txt
diff --git a/Documentation/devicetree/bindings/i2c/nomadik.txt b/Documentation/devicetree/bindings/i2c/nomadik.txt
new file mode 100644
index 0000000..72065b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/nomadik.txt
@@ -0,0 +1,23 @@
+I2C for Nomadik based systems
+
+Required (non-standard) properties:
+ - Nil
+
+Recommended (non-standard) properties:
+ - clock-frequency : Maximum bus clock frequency for the device
+
+Optional (non-standard) properties:
+ - Nil
+
+Example :
+
+i2c@80004000 {
+ compatible = "stericsson,db8500-i2c", "st,nomadik-i2c";
+ reg = <0x80004000 0x1000>;
+ interrupts = <0 21 0x4>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ v-i2c-supply = <&db8500_vape_reg>;
+
+ clock-frequency = <400000>;
+};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-08-23 15:01 [PATCH 1/3] ARM: ux500: Add i2c configurations to the Device Tree for DB8500 based devices Lee Jones
2012-08-23 15:01 ` [PATCH 2/3] Documentation: Device Tree binding information for i2c-nomadik driver Lee Jones
@ 2012-08-23 15:01 ` Lee Jones
2012-08-27 23:42 ` Linus Walleij
2012-08-31 11:22 ` Wolfram Sang
1 sibling, 2 replies; 28+ messages in thread
From: Lee Jones @ 2012-08-23 15:01 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: STEricsson_nomadik_linux, linus.walleij, arnd, w.sang, Lee Jones,
linux-i2c
Here we apply the bindings required for successful Device Tree
probing of the i2c-nomadik driver.
Cc: linux-i2c@vger.kernel.org
Acked-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
drivers/i2c/busses/i2c-nomadik.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 61b00ed..8168389 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -25,6 +25,7 @@
#include <linux/regulator/consumer.h>
#include <linux/pm_runtime.h>
#include <linux/platform_data/i2c-nomadik.h>
+#include <linux/of.h>
#define DRIVER_NAME "nmk-i2c"
@@ -920,15 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {
.sm = I2C_FREQ_MODE_FAST,
};
+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;
+
+ 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;
+}
+
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-08-23 15:01 ` [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver Lee Jones
@ 2012-08-27 23:42 ` Linus Walleij
2012-08-31 10:36 ` Lee Jones
2012-08-31 11:22 ` Wolfram Sang
1 sibling, 1 reply; 28+ messages in thread
From: Linus Walleij @ 2012-08-27 23:42 UTC (permalink / raw)
To: Lee Jones
Cc: linux-arm-kernel, linux-kernel, STEricsson_nomadik_linux,
linus.walleij, arnd, w.sang, linux-i2c
On Thu, Aug 23, 2012 at 8:01 AM, Lee Jones <lee.jones@linaro.org> wrote:
> Here we apply the bindings required for successful Device Tree
> probing of the i2c-nomadik driver.
>
> Cc: linux-i2c@vger.kernel.org
> Acked-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-08-27 23:42 ` Linus Walleij
@ 2012-08-31 10:36 ` Lee Jones
0 siblings, 0 replies; 28+ messages in thread
From: Lee Jones @ 2012-08-31 10:36 UTC (permalink / raw)
To: Linus Walleij, w.sang
Cc: linux-arm-kernel, linux-kernel, STEricsson_nomadik_linux,
linus.walleij, arnd, linux-i2c
On Mon, Aug 27, 2012 at 04:42:49PM -0700, Linus Walleij wrote:
> On Thu, Aug 23, 2012 at 8:01 AM, Lee Jones <lee.jones@linaro.org> wrote:
>
> > Here we apply the bindings required for successful Device Tree
> > probing of the i2c-nomadik driver.
> >
> > Cc: linux-i2c@vger.kernel.org
> > Acked-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-08-23 15:01 ` [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver Lee Jones
2012-08-27 23:42 ` Linus Walleij
@ 2012-08-31 11:22 ` Wolfram Sang
2012-08-31 12:04 ` Lee Jones
` (2 more replies)
1 sibling, 3 replies; 28+ messages in thread
From: Wolfram Sang @ 2012-08-31 11:22 UTC (permalink / raw)
To: Lee Jones
Cc: linux-arm-kernel, linux-kernel, STEricsson_nomadik_linux,
linus.walleij, arnd, linux-i2c
[-- Attachment #1: Type: text/plain, Size: 2938 bytes --]
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@vger.kernel.org
> Acked-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
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.
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> drivers/i2c/busses/i2c-nomadik.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 61b00ed..8168389 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -25,6 +25,7 @@
> #include <linux/regulator/consumer.h>
> #include <linux/pm_runtime.h>
> #include <linux/platform_data/i2c-nomadik.h>
> +#include <linux/of.h>
>
> #define DRIVER_NAME "nmk-i2c"
>
> @@ -920,15 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {
> .sm = I2C_FREQ_MODE_FAST,
> };
>
> +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?
> +
> + of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
Might be worth changing clk_freq to u32?
> +
> + /* 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
>
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-08-31 11:22 ` Wolfram Sang
@ 2012-08-31 12:04 ` Lee Jones
2012-08-31 12:23 ` Lee Jones
2012-09-05 7:33 ` Lee Jones
2 siblings, 0 replies; 28+ messages in thread
From: Lee Jones @ 2012-08-31 12:04 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-arm-kernel, linux-kernel, STEricsson_nomadik_linux,
linus.walleij, arnd, linux-i2c
> 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.
> > +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.
> > +
> > + of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
>
> Might be worth changing clk_freq to u32?
Yes, makes sense.
> > +
> > + /* 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.
> > + 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-08-31 11:22 ` Wolfram Sang
2012-08-31 12:04 ` Lee Jones
@ 2012-08-31 12:23 ` Lee Jones
2012-09-03 9:22 ` Linus Walleij
2012-09-05 7:33 ` Lee Jones
2 siblings, 1 reply; 28+ messages in thread
From: Lee Jones @ 2012-08-31 12:23 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-arm-kernel, linux-kernel, STEricsson_nomadik_linux,
linus.walleij, arnd, linux-i2c
Hopefully this is more to your liking:
Author: Lee Jones <lee.jones@linaro.org>
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@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 61b00ed..2c85de1 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -25,6 +25,7 @@
#include <linux/regulator/consumer.h>
#include <linux/pm_runtime.h>
#include <linux/platform_data/i2c-nomadik.h>
+#include <linux/of.h>
#define DRIVER_NAME "nmk-i2c"
@@ -920,15 +921,41 @@ static struct nmk_i2c_controller u8500_i2c = {
.sm = I2C_FREQ_MODE_FAST,
};
+static void nmk_i2c_of_probe(struct device_node *np,
+ struct nmk_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;
+}
+
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;
+ }
+ }
+ /* 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;
diff --git a/include/linux/platform_data/i2c-nomadik.h b/include/linux/platform_data/i2c-nomadik.h
index c2303c3..3a8be9c 100644
--- a/include/linux/platform_data/i2c-nomadik.h
+++ b/include/linux/platform_data/i2c-nomadik.h
@@ -28,7 +28,7 @@ enum i2c_freq_mode {
* @sm: speed mode
*/
struct nmk_i2c_controller {
- unsigned long clk_freq;
+ u32 clk_freq;
unsigned short slsu;
unsigned char tft;
unsigned char rft;
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-08-31 12:23 ` Lee Jones
@ 2012-09-03 9:22 ` Linus Walleij
2012-09-03 9:44 ` Wolfram Sang
0 siblings, 1 reply; 28+ messages in thread
From: Linus Walleij @ 2012-09-03 9:22 UTC (permalink / raw)
To: Lee Jones
Cc: Wolfram Sang, linux-arm-kernel, linux-kernel,
STEricsson_nomadik_linux, linus.walleij, arnd, linux-i2c
On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones <lee.jones@linaro.org> 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-09-03 9:22 ` Linus Walleij
@ 2012-09-03 9:44 ` Wolfram Sang
2012-09-03 9:50 ` Lee Jones
2012-09-03 10:07 ` Lee Jones
0 siblings, 2 replies; 28+ messages in thread
From: Wolfram Sang @ 2012-09-03 9:44 UTC (permalink / raw)
To: Linus Walleij
Cc: Lee Jones, linux-arm-kernel, linux-kernel,
STEricsson_nomadik_linux, linus.walleij, arnd, linux-i2c
[-- Attachment #1: Type: text/plain, Size: 2362 bytes --]
On Mon, Sep 03, 2012 at 11:22:28AM +0200, Linus Walleij wrote:
> On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones <lee.jones@linaro.org> 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.
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/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-09-03 9:44 ` Wolfram Sang
@ 2012-09-03 9:50 ` Lee Jones
2012-09-03 10:07 ` Lee Jones
1 sibling, 0 replies; 28+ messages in thread
From: Lee Jones @ 2012-09-03 9:50 UTC (permalink / raw)
To: Wolfram Sang
Cc: Linus Walleij, linux-arm-kernel, linux-kernel,
STEricsson_nomadik_linux, linus.walleij, arnd, linux-i2c
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:
> > On Fri, Aug 31, 2012 at 2:23 PM, Lee Jones <lee.jones@linaro.org> 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.
>
> 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-09-03 9:44 ` Wolfram Sang
2012-09-03 9:50 ` Lee Jones
@ 2012-09-03 10:07 ` Lee Jones
2012-09-03 11:07 ` Linus Walleij
1 sibling, 1 reply; 28+ messages in thread
From: Lee Jones @ 2012-09-03 10:07 UTC (permalink / raw)
To: Wolfram Sang
Cc: Linus Walleij, linux-arm-kernel, linux-kernel,
STEricsson_nomadik_linux, linus.walleij, arnd, linux-i2c
Author: Lee Jones <lee.jones@linaro.org>
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@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 61b00ed..c2da711 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -25,6 +25,7 @@
#include <linux/regulator/consumer.h>
#include <linux/pm_runtime.h>
#include <linux/platform_data/i2c-nomadik.h>
+#include <linux/of.h>
#define DRIVER_NAME "nmk-i2c"
@@ -920,15 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {
.sm = I2C_FREQ_MODE_FAST,
};
+static void nmk_i2c_of_probe(struct device_node *np,
+ struct nmk_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;
+}
+
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;
+ }
+ }
+ /* Provide the default configuration as a base. */
+ memcpy(pdata, &u8500_i2c, sizeof(struct nmk_i2c_controller));
+
+ nmk_i2c_of_probe(np, pdata);
+ }
+
if (!pdata)
/* No i2c configuration found, using the default. */
pdata = &u8500_i2c;
diff --git a/include/linux/platform_data/i2c-nomadik.h b/include/linux/platform_data/i2c-nomadik.h
index c2303c3..3a8be9c 100644
--- a/include/linux/platform_data/i2c-nomadik.h
+++ b/include/linux/platform_data/i2c-nomadik.h
@@ -28,7 +28,7 @@ enum i2c_freq_mode {
* @sm: speed mode
*/
struct nmk_i2c_controller {
- unsigned long clk_freq;
+ u32 clk_freq;
unsigned short slsu;
unsigned char tft;
unsigned char rft;
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-09-03 10:07 ` Lee Jones
@ 2012-09-03 11:07 ` Linus Walleij
[not found] ` <CAF2Aj3j25w1Nn9O6hV+=i-j1ts_p_Ucswk_M7r04S7i5BzPkHg@mail.gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Linus Walleij @ 2012-09-03 11:07 UTC (permalink / raw)
To: Lee Jones
Cc: Wolfram Sang, linux-arm-kernel, linux-kernel,
STEricsson_nomadik_linux, linus.walleij, arnd, linux-i2c
On Mon, Sep 3, 2012 at 12:07 PM, Lee Jones <lee.jones@linaro.org> 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-08-31 11:22 ` Wolfram Sang
2012-08-31 12:04 ` Lee Jones
2012-08-31 12:23 ` Lee Jones
@ 2012-09-05 7:33 ` Lee Jones
2012-09-05 8:22 ` Linus Walleij
2 siblings, 1 reply; 28+ messages in thread
From: Lee Jones @ 2012-09-05 7:33 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-arm-kernel, linux-kernel, STEricsson_nomadik_linux,
linus.walleij, arnd, linux-i2c
Author: Lee Jones <lee.jones@linaro.org>
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@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 61b00ed..5d1a970 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -25,6 +25,7 @@
#include <linux/regulator/consumer.h>
#include <linux/pm_runtime.h>
#include <linux/platform_data/i2c-nomadik.h>
+#include <linux/of.h>
#define DRIVER_NAME "nmk-i2c"
@@ -920,18 +921,42 @@ static struct nmk_i2c_controller u8500_i2c = {
.sm = I2C_FREQ_MODE_FAST,
};
+static void nmk_i2c_of_probe(struct device_node *np,
+ struct nmk_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;
+}
+
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 (!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;
+ 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
+ /* No i2c configuration found, using the default. */
+ pdata = &u8500_i2c;
+ }
dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
if (!dev) {
diff --git a/include/linux/platform_data/i2c-nomadik.h b/include/linux/platform_data/i2c-nomadik.h
index c2303c3..3a8be9c 100644
--- a/include/linux/platform_data/i2c-nomadik.h
+++ b/include/linux/platform_data/i2c-nomadik.h
@@ -28,7 +28,7 @@ enum i2c_freq_mode {
* @sm: speed mode
*/
struct nmk_i2c_controller {
- unsigned long clk_freq;
+ u32 clk_freq;
unsigned short slsu;
unsigned char tft;
unsigned char rft;
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver
2012-09-05 7:33 ` Lee Jones
@ 2012-09-05 8:22 ` Linus Walleij
0 siblings, 0 replies; 28+ messages in thread
From: Linus Walleij @ 2012-09-05 8:22 UTC (permalink / raw)
To: linux-arm-kernel, Wolfram Sang
Cc: linux-kernel, STEricsson_nomadik_linux, linus.walleij, arnd, linux-i2c
On Wed, Sep 5, 2012 at 9:33 AM, Lee Jones <lee.jones@linaro.org> wrote:
> Author: Lee Jones <lee.jones@linaro.org>
> 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@vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
Excellent :-)
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Wolfram are you picking this up?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2012-09-05 8:22 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-23 15:01 [PATCH 1/3] ARM: ux500: Add i2c configurations to the Device Tree for DB8500 based devices Lee Jones
2012-08-23 15:01 ` [PATCH 2/3] Documentation: Device Tree binding information for i2c-nomadik driver Lee Jones
2012-08-23 15:01 ` [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver Lee Jones
2012-08-27 23:42 ` Linus Walleij
2012-08-31 10:36 ` Lee Jones
2012-08-31 11:22 ` Wolfram Sang
2012-08-31 12:04 ` Lee Jones
2012-08-31 12:23 ` Lee Jones
2012-09-03 9:22 ` Linus Walleij
2012-09-03 9:44 ` Wolfram Sang
2012-09-03 9:50 ` Lee Jones
2012-09-03 10:07 ` Lee Jones
2012-09-03 11:07 ` Linus Walleij
[not found] ` <CAF2Aj3j25w1Nn9O6hV+=i-j1ts_p_Ucswk_M7r04S7i5BzPkHg@mail.gmail.com>
2012-09-03 11:58 ` Linus Walleij
2012-09-03 12:34 ` Lee Jones
2012-09-03 13:19 ` Linus Walleij
2012-09-03 13:28 ` Lee Jones
2012-09-03 14:33 ` Stephen Warren
2012-09-03 14:35 ` Linus Walleij
2012-09-03 15:09 ` Rob Herring
2012-09-03 15:20 ` Lee Jones
2012-09-04 14:28 ` Arnd Bergmann
2012-09-04 17:27 ` Linus Walleij
2012-09-05 6:41 ` Lee Jones
2012-09-05 6:53 ` Linus Walleij
2012-09-04 17:35 ` Alessandro Rubini
2012-09-05 7:33 ` Lee Jones
2012-09-05 8:22 ` Linus Walleij
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).