* [PATCHv3] I2C: MV64XXX: Add Device Tree support
@ 2012-07-16 9:16 ` Andrew Lunn
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2012-07-16 9:16 UTC (permalink / raw)
To: w.sang
Cc: Andrew Lunn, sebastian.hesselbarth, linux-i2c, linux ARM, Jason Cooper
This is one patch out of a larger patchset adding DT support to a
number of Orion drivers. All the other patches have been acked-by,
etc, so i'm only re-posting the one which has been modified.
The I2C driver now uses the clock-frequency property.
Andrew Lunn (1):
I2C: MV64XXX: Add Device Tree support
Documentation/devicetree/bindings/i2c/mrvl-i2c.txt | 21 +++-
arch/arm/boot/dts/kirkwood.dtsi | 11 ++
arch/arm/mach-kirkwood/board-dt.c | 2 +
arch/arm/mach-kirkwood/common.c | 2 +
arch/arm/plat-orion/common.c | 1 +
drivers/i2c/busses/i2c-mv64xxx.c | 107 +++++++++++++++++++-
6 files changed, 138 insertions(+), 6 deletions(-)
--
1.7.10
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCHv3] I2C: MV64XXX: Add Device Tree support
@ 2012-07-16 9:16 ` Andrew Lunn
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2012-07-16 9:16 UTC (permalink / raw)
To: linux-arm-kernel
This is one patch out of a larger patchset adding DT support to a
number of Orion drivers. All the other patches have been acked-by,
etc, so i'm only re-posting the one which has been modified.
The I2C driver now uses the clock-frequency property.
Andrew Lunn (1):
I2C: MV64XXX: Add Device Tree support
Documentation/devicetree/bindings/i2c/mrvl-i2c.txt | 21 +++-
arch/arm/boot/dts/kirkwood.dtsi | 11 ++
arch/arm/mach-kirkwood/board-dt.c | 2 +
arch/arm/mach-kirkwood/common.c | 2 +
arch/arm/plat-orion/common.c | 1 +
drivers/i2c/busses/i2c-mv64xxx.c | 107 +++++++++++++++++++-
6 files changed, 138 insertions(+), 6 deletions(-)
--
1.7.10
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCHv3] I2C: MV64XXX: Add Device Tree support
2012-07-16 9:16 ` Andrew Lunn
@ 2012-07-16 9:16 ` Andrew Lunn
-1 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2012-07-16 9:16 UTC (permalink / raw)
To: w.sang
Cc: Andrew Lunn, sebastian.hesselbarth, linux-i2c, linux ARM, Jason Cooper
Extends the driver to get properties from device tree. Rather than
pass the N & M factors in DT, use the more standard clock-frequency
property. Calculate N & M at run time. In order to do this, we need to
know tclk. So the driver uses clk_get() etc in order to get the clock
and clk_get_rate() to determine the tclk rate. Not all platforms
however have CLK, so some #ifdefery is needed to ensure the driver
still compiles when CLK is not available.
Also extend the kirkwood DT support to supply the needed properties. A
default clock-frequency is 400KHz, which some platforms might need to
override to 100KHz if they have devices which do not support fast
mode.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@googlemail.com>
---
v2: Removed duplicate interrupt in DT.
v3: Use clock-frequency property, instead of explicit n & m baud factors.
i2c fix.
---
Documentation/devicetree/bindings/i2c/mrvl-i2c.txt | 21 +++-
arch/arm/boot/dts/kirkwood.dtsi | 11 ++
arch/arm/mach-kirkwood/board-dt.c | 2 +
arch/arm/mach-kirkwood/common.c | 2 +
arch/arm/plat-orion/common.c | 1 +
drivers/i2c/busses/i2c-mv64xxx.c | 107 +++++++++++++++++++-
6 files changed, 138 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt b/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
index b891ee2..885f340 100644
--- a/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
@@ -1,4 +1,4 @@
-* I2C
+* Marvell MMP I2C controller
Required properties :
@@ -32,3 +32,22 @@ Examples:
interrupts = <58>;
};
+* Marvell MV64XXX I2C controller
+
+Required properties :
+
+ - reg : Offset and length of the register set for the device
+ - compatible : Should be "marvell,mv64xxx-i2c"
+ - interrupts : Te interrupt number
+ - clock-frequency : Desired I2C bus clock frequency in Hz.
+ - timeout-ms : How long to wait for a transaction to complete
+
+Examples:
+
+ i2c@11000 {
+ compatible = "marvell,mv64xxx-i2c";
+ reg = <0x11000 0x20>;
+ interrupts = <29>;
+ clock-frequency = <100000>;
+ timeout-ms = <1000>;
+ };
diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
index a3350fd..bc57d81 100644
--- a/arch/arm/boot/dts/kirkwood.dtsi
+++ b/arch/arm/boot/dts/kirkwood.dtsi
@@ -82,5 +82,16 @@
/* set partition map and/or chip-delay in board dts */
status = "disabled";
};
+
+ i2c@11000 {
+ compatible = "marvell,mv64xxx-i2c";
+ reg = <0x11000 0x20>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = <29>;
+ clock-frequency = <400000>;
+ timeout-ms = <1000>;
+ status = "disable";
+ };
};
};
diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
index 8f8da5d..24c8fdd 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -28,6 +28,8 @@ static struct of_device_id kirkwood_dt_match_table[] __initdata = {
struct of_dev_auxdata kirkwood_auxdata_lookup[] __initdata = {
OF_DEV_AUXDATA("marvell,orion-spi", 0xf1010600, "orion_spi.0", NULL),
+ OF_DEV_AUXDATA("marvell,mv64xxx-i2c", 0xf1011000, "mv64xxx_i2c.0",
+ NULL),
{},
};
diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
index f261cd2..078d9c6 100644
--- a/arch/arm/mach-kirkwood/common.c
+++ b/arch/arm/mach-kirkwood/common.c
@@ -17,6 +17,7 @@
#include <linux/dma-mapping.h>
#include <linux/clk-provider.h>
#include <linux/spinlock.h>
+#include <linux/mv643xx_i2c.h>
#include <net/dsa.h>
#include <asm/page.h>
#include <asm/timex.h>
@@ -241,6 +242,7 @@ void __init kirkwood_clk_init(void)
orion_clkdev_add("0", "pcie", pex0);
orion_clkdev_add("1", "pcie", pex1);
orion_clkdev_add(NULL, "kirkwood-i2s", audio);
+ orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".0", runit);
}
/*****************************************************************************
diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
index c179378..d245a87 100644
--- a/arch/arm/plat-orion/common.c
+++ b/arch/arm/plat-orion/common.c
@@ -47,6 +47,7 @@ void __init orion_clkdev_init(struct clk *tclk)
orion_clkdev_add(NULL, MV643XX_ETH_NAME ".2", tclk);
orion_clkdev_add(NULL, MV643XX_ETH_NAME ".3", tclk);
orion_clkdev_add(NULL, "orion_wdt", tclk);
+ orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".0", tclk);
}
/* Fill in the resources structure and link it into the platform
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 4f44a33..e97246b 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -18,6 +18,11 @@
#include <linux/mv643xx_i2c.h>
#include <linux/platform_device.h>
#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_i2c.h>
+#include <linux/clk.h>
+#include <linux/err.h>
/* Register defines */
#define MV64XXX_I2C_REG_SLAVE_ADDR 0x00
@@ -98,6 +103,7 @@ struct mv64xxx_i2c_data {
int rc;
u32 freq_m;
u32 freq_n;
+ struct clk *clk;
wait_queue_head_t waitq;
spinlock_t lock;
struct i2c_msg *msg;
@@ -521,6 +527,36 @@ mv64xxx_i2c_unmap_regs(struct mv64xxx_i2c_data *drv_data)
drv_data->reg_base_p = 0;
}
+#ifdef CONFIG_OF
+static int __devinit
+calc_freq(const int tclk, const int n, const int m)
+{
+ return tclk / (10 * (m + 1) * (2 << n));
+}
+
+static bool __devinit
+find_baud_factors(const int req_freq, const int tclk, int *best_n, int *best_m)
+{
+ int freq, delta, best_delta = INT_MAX;
+ int m, n;
+
+ for (n = 0; n <= 7; n++)
+ for (m = 0; m <= 15; m++) {
+ freq = calc_freq(tclk, n, m);
+ delta = req_freq - freq;
+ if (delta >= 0 && delta < best_delta) {
+ *best_m = m;
+ *best_n = n;
+ best_delta = delta;
+ }
+ if (best_delta == 0)
+ return true;
+ }
+ if (best_delta == INT_MAX)
+ return false;
+ return true;
+}
+#endif
static int __devinit
mv64xxx_i2c_probe(struct platform_device *pd)
{
@@ -528,7 +564,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
struct mv64xxx_i2c_pdata *pdata = pd->dev.platform_data;
int rc;
- if ((pd->id != 0) || !pdata)
+ if ((!pdata && !pd->dev.of_node) || (pdata && (pd->id != 0)))
return -ENODEV;
drv_data = kzalloc(sizeof(struct mv64xxx_i2c_data), GFP_KERNEL);
@@ -546,19 +582,64 @@ mv64xxx_i2c_probe(struct platform_device *pd)
init_waitqueue_head(&drv_data->waitq);
spin_lock_init(&drv_data->lock);
- drv_data->freq_m = pdata->freq_m;
- drv_data->freq_n = pdata->freq_n;
- drv_data->irq = platform_get_irq(pd, 0);
+#if defined(CONFIG_HAVE_CLK)
+ /* Not all platforms have a clk */
+ drv_data->clk = clk_get(&pd->dev, NULL);
+ if (!IS_ERR(drv_data->clk)) {
+ clk_prepare(drv_data->clk);
+ clk_enable(drv_data->clk);
+ }
+#endif
+ if (pdata) {
+ drv_data->freq_m = pdata->freq_m;
+ drv_data->freq_n = pdata->freq_n;
+ drv_data->irq = platform_get_irq(pd, 0);
+ }
+#ifdef CONFIG_OF
+ if (pd->dev.of_node) {
+ int bus_freq;
+ int tclk;
+ /* CLK is mandatory when using DT to describe the i2c
+ * bus. We need to know tclk in order to calculate bus
+ * clock factors. */
+#if defined(CONFIG_HAVE_CLK)
+ if (IS_ERR(drv_data->clk)) {
+ rc = -ENODEV;
+ goto exit_unmap_regs;
+ }
+ tclk = clk_get_rate(drv_data->clk);
+ of_property_read_u32(pd->dev.of_node, "clock-frequency",
+ &bus_freq);
+ if (!find_baud_factors(bus_freq, tclk,
+ &drv_data->freq_n, &drv_data->freq_m)) {
+ rc = -EINVAL;
+ goto exit_unmap_regs;
+ }
+ drv_data->irq = irq_of_parse_and_map(pd->dev.of_node, 0);
+#else
+ /* Have OF but no CLK */
+ rc = -ENODEV;
+ goto exit_unmap_regs;
+#endif
+ }
+#endif
if (drv_data->irq < 0) {
rc = -ENXIO;
goto exit_unmap_regs;
}
+
drv_data->adapter.dev.parent = &pd->dev;
drv_data->adapter.algo = &mv64xxx_i2c_algo;
drv_data->adapter.owner = THIS_MODULE;
drv_data->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
- drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
+ if (pd->dev.of_node)
+ drv_data->adapter.timeout = msecs_to_jiffies(
+ of_property_read_u32(pd->dev.of_node, "timeout-ms",
+ &drv_data->freq_n));
+ else
+ drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
drv_data->adapter.nr = pd->id;
+ drv_data->adapter.dev.of_node = pd->dev.of_node;
platform_set_drvdata(pd, drv_data);
i2c_set_adapdata(&drv_data->adapter, drv_data);
@@ -577,6 +658,8 @@ mv64xxx_i2c_probe(struct platform_device *pd)
goto exit_free_irq;
}
+ of_i2c_register_devices(&drv_data->adapter);
+
return 0;
exit_free_irq:
@@ -597,17 +680,31 @@ mv64xxx_i2c_remove(struct platform_device *dev)
rc = i2c_del_adapter(&drv_data->adapter);
free_irq(drv_data->irq, drv_data);
mv64xxx_i2c_unmap_regs(drv_data);
+#if defined(CONFIG_HAVE_CLK)
+ /* Not all platforms have a clk */
+ if (!IS_ERR(drv_data->clk)) {
+ clk_disable(drv_data->clk);
+ clk_unprepare(drv_data->clk);
+ }
+#endif
kfree(drv_data);
return rc;
}
+static const struct of_device_id mv64xxx_i2c_of_match_table[] __devinitdata = {
+ { .compatible = "marvell,mv64xxx-i2c", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
+
static struct platform_driver mv64xxx_i2c_driver = {
.probe = mv64xxx_i2c_probe,
.remove = __devexit_p(mv64xxx_i2c_remove),
.driver = {
.owner = THIS_MODULE,
.name = MV64XXX_I2C_CTLR_NAME,
+ .of_match_table = of_match_ptr(mv64xxx_i2c_of_match_table),
},
};
--
1.7.10
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCHv3] I2C: MV64XXX: Add Device Tree support
@ 2012-07-16 9:16 ` Andrew Lunn
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2012-07-16 9:16 UTC (permalink / raw)
To: linux-arm-kernel
Extends the driver to get properties from device tree. Rather than
pass the N & M factors in DT, use the more standard clock-frequency
property. Calculate N & M at run time. In order to do this, we need to
know tclk. So the driver uses clk_get() etc in order to get the clock
and clk_get_rate() to determine the tclk rate. Not all platforms
however have CLK, so some #ifdefery is needed to ensure the driver
still compiles when CLK is not available.
Also extend the kirkwood DT support to supply the needed properties. A
default clock-frequency is 400KHz, which some platforms might need to
override to 100KHz if they have devices which do not support fast
mode.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@googlemail.com>
---
v2: Removed duplicate interrupt in DT.
v3: Use clock-frequency property, instead of explicit n & m baud factors.
i2c fix.
---
Documentation/devicetree/bindings/i2c/mrvl-i2c.txt | 21 +++-
arch/arm/boot/dts/kirkwood.dtsi | 11 ++
arch/arm/mach-kirkwood/board-dt.c | 2 +
arch/arm/mach-kirkwood/common.c | 2 +
arch/arm/plat-orion/common.c | 1 +
drivers/i2c/busses/i2c-mv64xxx.c | 107 +++++++++++++++++++-
6 files changed, 138 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt b/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
index b891ee2..885f340 100644
--- a/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
@@ -1,4 +1,4 @@
-* I2C
+* Marvell MMP I2C controller
Required properties :
@@ -32,3 +32,22 @@ Examples:
interrupts = <58>;
};
+* Marvell MV64XXX I2C controller
+
+Required properties :
+
+ - reg : Offset and length of the register set for the device
+ - compatible : Should be "marvell,mv64xxx-i2c"
+ - interrupts : Te interrupt number
+ - clock-frequency : Desired I2C bus clock frequency in Hz.
+ - timeout-ms : How long to wait for a transaction to complete
+
+Examples:
+
+ i2c at 11000 {
+ compatible = "marvell,mv64xxx-i2c";
+ reg = <0x11000 0x20>;
+ interrupts = <29>;
+ clock-frequency = <100000>;
+ timeout-ms = <1000>;
+ };
diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
index a3350fd..bc57d81 100644
--- a/arch/arm/boot/dts/kirkwood.dtsi
+++ b/arch/arm/boot/dts/kirkwood.dtsi
@@ -82,5 +82,16 @@
/* set partition map and/or chip-delay in board dts */
status = "disabled";
};
+
+ i2c at 11000 {
+ compatible = "marvell,mv64xxx-i2c";
+ reg = <0x11000 0x20>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = <29>;
+ clock-frequency = <400000>;
+ timeout-ms = <1000>;
+ status = "disable";
+ };
};
};
diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
index 8f8da5d..24c8fdd 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -28,6 +28,8 @@ static struct of_device_id kirkwood_dt_match_table[] __initdata = {
struct of_dev_auxdata kirkwood_auxdata_lookup[] __initdata = {
OF_DEV_AUXDATA("marvell,orion-spi", 0xf1010600, "orion_spi.0", NULL),
+ OF_DEV_AUXDATA("marvell,mv64xxx-i2c", 0xf1011000, "mv64xxx_i2c.0",
+ NULL),
{},
};
diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
index f261cd2..078d9c6 100644
--- a/arch/arm/mach-kirkwood/common.c
+++ b/arch/arm/mach-kirkwood/common.c
@@ -17,6 +17,7 @@
#include <linux/dma-mapping.h>
#include <linux/clk-provider.h>
#include <linux/spinlock.h>
+#include <linux/mv643xx_i2c.h>
#include <net/dsa.h>
#include <asm/page.h>
#include <asm/timex.h>
@@ -241,6 +242,7 @@ void __init kirkwood_clk_init(void)
orion_clkdev_add("0", "pcie", pex0);
orion_clkdev_add("1", "pcie", pex1);
orion_clkdev_add(NULL, "kirkwood-i2s", audio);
+ orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".0", runit);
}
/*****************************************************************************
diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
index c179378..d245a87 100644
--- a/arch/arm/plat-orion/common.c
+++ b/arch/arm/plat-orion/common.c
@@ -47,6 +47,7 @@ void __init orion_clkdev_init(struct clk *tclk)
orion_clkdev_add(NULL, MV643XX_ETH_NAME ".2", tclk);
orion_clkdev_add(NULL, MV643XX_ETH_NAME ".3", tclk);
orion_clkdev_add(NULL, "orion_wdt", tclk);
+ orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".0", tclk);
}
/* Fill in the resources structure and link it into the platform
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 4f44a33..e97246b 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -18,6 +18,11 @@
#include <linux/mv643xx_i2c.h>
#include <linux/platform_device.h>
#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_i2c.h>
+#include <linux/clk.h>
+#include <linux/err.h>
/* Register defines */
#define MV64XXX_I2C_REG_SLAVE_ADDR 0x00
@@ -98,6 +103,7 @@ struct mv64xxx_i2c_data {
int rc;
u32 freq_m;
u32 freq_n;
+ struct clk *clk;
wait_queue_head_t waitq;
spinlock_t lock;
struct i2c_msg *msg;
@@ -521,6 +527,36 @@ mv64xxx_i2c_unmap_regs(struct mv64xxx_i2c_data *drv_data)
drv_data->reg_base_p = 0;
}
+#ifdef CONFIG_OF
+static int __devinit
+calc_freq(const int tclk, const int n, const int m)
+{
+ return tclk / (10 * (m + 1) * (2 << n));
+}
+
+static bool __devinit
+find_baud_factors(const int req_freq, const int tclk, int *best_n, int *best_m)
+{
+ int freq, delta, best_delta = INT_MAX;
+ int m, n;
+
+ for (n = 0; n <= 7; n++)
+ for (m = 0; m <= 15; m++) {
+ freq = calc_freq(tclk, n, m);
+ delta = req_freq - freq;
+ if (delta >= 0 && delta < best_delta) {
+ *best_m = m;
+ *best_n = n;
+ best_delta = delta;
+ }
+ if (best_delta == 0)
+ return true;
+ }
+ if (best_delta == INT_MAX)
+ return false;
+ return true;
+}
+#endif
static int __devinit
mv64xxx_i2c_probe(struct platform_device *pd)
{
@@ -528,7 +564,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
struct mv64xxx_i2c_pdata *pdata = pd->dev.platform_data;
int rc;
- if ((pd->id != 0) || !pdata)
+ if ((!pdata && !pd->dev.of_node) || (pdata && (pd->id != 0)))
return -ENODEV;
drv_data = kzalloc(sizeof(struct mv64xxx_i2c_data), GFP_KERNEL);
@@ -546,19 +582,64 @@ mv64xxx_i2c_probe(struct platform_device *pd)
init_waitqueue_head(&drv_data->waitq);
spin_lock_init(&drv_data->lock);
- drv_data->freq_m = pdata->freq_m;
- drv_data->freq_n = pdata->freq_n;
- drv_data->irq = platform_get_irq(pd, 0);
+#if defined(CONFIG_HAVE_CLK)
+ /* Not all platforms have a clk */
+ drv_data->clk = clk_get(&pd->dev, NULL);
+ if (!IS_ERR(drv_data->clk)) {
+ clk_prepare(drv_data->clk);
+ clk_enable(drv_data->clk);
+ }
+#endif
+ if (pdata) {
+ drv_data->freq_m = pdata->freq_m;
+ drv_data->freq_n = pdata->freq_n;
+ drv_data->irq = platform_get_irq(pd, 0);
+ }
+#ifdef CONFIG_OF
+ if (pd->dev.of_node) {
+ int bus_freq;
+ int tclk;
+ /* CLK is mandatory when using DT to describe the i2c
+ * bus. We need to know tclk in order to calculate bus
+ * clock factors. */
+#if defined(CONFIG_HAVE_CLK)
+ if (IS_ERR(drv_data->clk)) {
+ rc = -ENODEV;
+ goto exit_unmap_regs;
+ }
+ tclk = clk_get_rate(drv_data->clk);
+ of_property_read_u32(pd->dev.of_node, "clock-frequency",
+ &bus_freq);
+ if (!find_baud_factors(bus_freq, tclk,
+ &drv_data->freq_n, &drv_data->freq_m)) {
+ rc = -EINVAL;
+ goto exit_unmap_regs;
+ }
+ drv_data->irq = irq_of_parse_and_map(pd->dev.of_node, 0);
+#else
+ /* Have OF but no CLK */
+ rc = -ENODEV;
+ goto exit_unmap_regs;
+#endif
+ }
+#endif
if (drv_data->irq < 0) {
rc = -ENXIO;
goto exit_unmap_regs;
}
+
drv_data->adapter.dev.parent = &pd->dev;
drv_data->adapter.algo = &mv64xxx_i2c_algo;
drv_data->adapter.owner = THIS_MODULE;
drv_data->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
- drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
+ if (pd->dev.of_node)
+ drv_data->adapter.timeout = msecs_to_jiffies(
+ of_property_read_u32(pd->dev.of_node, "timeout-ms",
+ &drv_data->freq_n));
+ else
+ drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
drv_data->adapter.nr = pd->id;
+ drv_data->adapter.dev.of_node = pd->dev.of_node;
platform_set_drvdata(pd, drv_data);
i2c_set_adapdata(&drv_data->adapter, drv_data);
@@ -577,6 +658,8 @@ mv64xxx_i2c_probe(struct platform_device *pd)
goto exit_free_irq;
}
+ of_i2c_register_devices(&drv_data->adapter);
+
return 0;
exit_free_irq:
@@ -597,17 +680,31 @@ mv64xxx_i2c_remove(struct platform_device *dev)
rc = i2c_del_adapter(&drv_data->adapter);
free_irq(drv_data->irq, drv_data);
mv64xxx_i2c_unmap_regs(drv_data);
+#if defined(CONFIG_HAVE_CLK)
+ /* Not all platforms have a clk */
+ if (!IS_ERR(drv_data->clk)) {
+ clk_disable(drv_data->clk);
+ clk_unprepare(drv_data->clk);
+ }
+#endif
kfree(drv_data);
return rc;
}
+static const struct of_device_id mv64xxx_i2c_of_match_table[] __devinitdata = {
+ { .compatible = "marvell,mv64xxx-i2c", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
+
static struct platform_driver mv64xxx_i2c_driver = {
.probe = mv64xxx_i2c_probe,
.remove = __devexit_p(mv64xxx_i2c_remove),
.driver = {
.owner = THIS_MODULE,
.name = MV64XXX_I2C_CTLR_NAME,
+ .of_match_table = of_match_ptr(mv64xxx_i2c_of_match_table),
},
};
--
1.7.10
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCHv3] I2C: MV64XXX: Add Device Tree support
2012-07-16 9:16 ` Andrew Lunn
@ 2012-07-16 12:20 ` Thomas Petazzoni
-1 siblings, 0 replies; 12+ messages in thread
From: Thomas Petazzoni @ 2012-07-16 12:20 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jason Cooper, sebastian.hesselbarth, w.sang, linux ARM, linux-i2c
Le Mon, 16 Jul 2012 11:16:45 +0200,
Andrew Lunn <andrew@lunn.ch> a écrit :
> + - reg : Offset and length of the register set for the device
> + - compatible : Should be "marvell,mv64xxx-i2c"
> + - interrupts : Te interrupt number
The
> + i2c@11000 {
> + compatible = "marvell,mv64xxx-i2c";
> + reg = <0x11000 0x20>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <29>;
> + clock-frequency = <400000>;
> + timeout-ms = <1000>;
> + status = "disable";
This should be "disabled". Any other status than "okay" is considered
as disabled, if I'm correct, but the right opposite to "okay" is
"disabled", as far as I'm aware.
The #ifdef machinery in the ->probe() function looks a bit ugly, but I
guess this can be cleaned up once all platforms using this driver will
have been converted to DT + clock framework.
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCHv3] I2C: MV64XXX: Add Device Tree support
@ 2012-07-16 12:20 ` Thomas Petazzoni
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Petazzoni @ 2012-07-16 12:20 UTC (permalink / raw)
To: linux-arm-kernel
Le Mon, 16 Jul 2012 11:16:45 +0200,
Andrew Lunn <andrew@lunn.ch> a ?crit :
> + - reg : Offset and length of the register set for the device
> + - compatible : Should be "marvell,mv64xxx-i2c"
> + - interrupts : Te interrupt number
The
> + i2c at 11000 {
> + compatible = "marvell,mv64xxx-i2c";
> + reg = <0x11000 0x20>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <29>;
> + clock-frequency = <400000>;
> + timeout-ms = <1000>;
> + status = "disable";
This should be "disabled". Any other status than "okay" is considered
as disabled, if I'm correct, but the right opposite to "okay" is
"disabled", as far as I'm aware.
The #ifdef machinery in the ->probe() function looks a bit ugly, but I
guess this can be cleaned up once all platforms using this driver will
have been converted to DT + clock framework.
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv3] I2C: MV64XXX: Add Device Tree support
2012-07-16 12:20 ` Thomas Petazzoni
@ 2012-07-16 12:37 ` Andrew Lunn
-1 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2012-07-16 12:37 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Andrew Lunn, Jason Cooper, sebastian.hesselbarth, w.sang,
linux-i2c, linux ARM
On Mon, Jul 16, 2012 at 02:20:19PM +0200, Thomas Petazzoni wrote:
> Le Mon, 16 Jul 2012 11:16:45 +0200,
> Andrew Lunn <andrew@lunn.ch> a ??crit :
>
> > + - reg : Offset and length of the register set for the device
> > + - compatible : Should be "marvell,mv64xxx-i2c"
> > + - interrupts : Te interrupt number
>
> The
>
> > + i2c@11000 {
> > + compatible = "marvell,mv64xxx-i2c";
> > + reg = <0x11000 0x20>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + interrupts = <29>;
> > + clock-frequency = <400000>;
> > + timeout-ms = <1000>;
> > + status = "disable";
>
> This should be "disabled".
Thanks.
> The #ifdef machinery in the ->probe() function looks a bit ugly, but I
> guess this can be cleaned up once all platforms using this driver will
> have been converted to DT + clock framework.
Yes, it is ugly :-(
The ARM platforms are moving in the right direction. They all have
clock framework support and only mv78xx0 has nobody working on DT, as
far as i know.
I've no idea if anybody is still working on the powerpc platforms.
There are two _defconfig files that use it.
Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCHv3] I2C: MV64XXX: Add Device Tree support
@ 2012-07-16 12:37 ` Andrew Lunn
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2012-07-16 12:37 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 16, 2012 at 02:20:19PM +0200, Thomas Petazzoni wrote:
> Le Mon, 16 Jul 2012 11:16:45 +0200,
> Andrew Lunn <andrew@lunn.ch> a ??crit :
>
> > + - reg : Offset and length of the register set for the device
> > + - compatible : Should be "marvell,mv64xxx-i2c"
> > + - interrupts : Te interrupt number
>
> The
>
> > + i2c at 11000 {
> > + compatible = "marvell,mv64xxx-i2c";
> > + reg = <0x11000 0x20>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + interrupts = <29>;
> > + clock-frequency = <400000>;
> > + timeout-ms = <1000>;
> > + status = "disable";
>
> This should be "disabled".
Thanks.
> The #ifdef machinery in the ->probe() function looks a bit ugly, but I
> guess this can be cleaned up once all platforms using this driver will
> have been converted to DT + clock framework.
Yes, it is ugly :-(
The ARM platforms are moving in the right direction. They all have
clock framework support and only mv78xx0 has nobody working on DT, as
far as i know.
I've no idea if anybody is still working on the powerpc platforms.
There are two _defconfig files that use it.
Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv3] I2C: MV64XXX: Add Device Tree support
2012-07-16 9:16 ` Andrew Lunn
@ 2012-07-20 10:35 ` Wolfram Sang
-1 siblings, 0 replies; 12+ messages in thread
From: Wolfram Sang @ 2012-07-20 10:35 UTC (permalink / raw)
To: Andrew Lunn; +Cc: sebastian.hesselbarth, linux-i2c, linux ARM, Jason Cooper
[-- Attachment #1.1: Type: text/plain, Size: 11940 bytes --]
Hi Andrew,
On Mon, Jul 16, 2012 at 11:16:45AM +0200, Andrew Lunn wrote:
> Extends the driver to get properties from device tree. Rather than
> pass the N & M factors in DT, use the more standard clock-frequency
> property. Calculate N & M at run time. In order to do this, we need to
Thanks.
> know tclk. So the driver uses clk_get() etc in order to get the clock
> and clk_get_rate() to determine the tclk rate. Not all platforms
> however have CLK, so some #ifdefery is needed to ensure the driver
> still compiles when CLK is not available.
>
> Also extend the kirkwood DT support to supply the needed properties. A
> default clock-frequency is 400KHz, which some platforms might need to
> override to 100KHz if they have devices which do not support fast
> mode.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@googlemail.com>
Has Sebastian acked THIS patch? Didn't saw such a mail? If it was a
previous patch which he acked, I'd like it to be resent, since the
divider calculation is new.
> ---
> v2: Removed duplicate interrupt in DT.
> v3: Use clock-frequency property, instead of explicit n & m baud factors.
>
> i2c fix.
> ---
> Documentation/devicetree/bindings/i2c/mrvl-i2c.txt | 21 +++-
> arch/arm/boot/dts/kirkwood.dtsi | 11 ++
> arch/arm/mach-kirkwood/board-dt.c | 2 +
> arch/arm/mach-kirkwood/common.c | 2 +
> arch/arm/plat-orion/common.c | 1 +
> drivers/i2c/busses/i2c-mv64xxx.c | 107 +++++++++++++++++++-
As said in another mail, please split up arch/arm and i2c-stuff.
> 6 files changed, 138 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt b/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
> index b891ee2..885f340 100644
> --- a/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
> +++ b/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
> @@ -1,4 +1,4 @@
> -* I2C
> +* Marvell MMP I2C controller
>
> Required properties :
>
> @@ -32,3 +32,22 @@ Examples:
> interrupts = <58>;
> };
>
> +* Marvell MV64XXX I2C controller
> +
> +Required properties :
> +
> + - reg : Offset and length of the register set for the device
> + - compatible : Should be "marvell,mv64xxx-i2c"
> + - interrupts : Te interrupt number
> + - clock-frequency : Desired I2C bus clock frequency in Hz.
> + - timeout-ms : How long to wait for a transaction to complete
If you don't really need "timeout-ms", I'd ask you to drop it. I haven't
made up my mind how to handle those timeouts yet. They should be
per-slave and not per-bus, but the core cannot handle this yet. And even
though I am tempted to accept a per-bus-timeout, getting the binding
right is not something we can come up with before the next merge window.
I am suspecting a default value could do for now?
> +
> +Examples:
> +
> + i2c@11000 {
> + compatible = "marvell,mv64xxx-i2c";
> + reg = <0x11000 0x20>;
> + interrupts = <29>;
> + clock-frequency = <100000>;
> + timeout-ms = <1000>;
> + };
> diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> index a3350fd..bc57d81 100644
> --- a/arch/arm/boot/dts/kirkwood.dtsi
> +++ b/arch/arm/boot/dts/kirkwood.dtsi
> @@ -82,5 +82,16 @@
> /* set partition map and/or chip-delay in board dts */
> status = "disabled";
> };
> +
> + i2c@11000 {
> + compatible = "marvell,mv64xxx-i2c";
> + reg = <0x11000 0x20>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <29>;
> + clock-frequency = <400000>;
> + timeout-ms = <1000>;
> + status = "disable";
> + };
> };
> };
> diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
> index 8f8da5d..24c8fdd 100644
> --- a/arch/arm/mach-kirkwood/board-dt.c
> +++ b/arch/arm/mach-kirkwood/board-dt.c
> @@ -28,6 +28,8 @@ static struct of_device_id kirkwood_dt_match_table[] __initdata = {
>
> struct of_dev_auxdata kirkwood_auxdata_lookup[] __initdata = {
> OF_DEV_AUXDATA("marvell,orion-spi", 0xf1010600, "orion_spi.0", NULL),
> + OF_DEV_AUXDATA("marvell,mv64xxx-i2c", 0xf1011000, "mv64xxx_i2c.0",
> + NULL),
> {},
> };
>
> diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
> index f261cd2..078d9c6 100644
> --- a/arch/arm/mach-kirkwood/common.c
> +++ b/arch/arm/mach-kirkwood/common.c
> @@ -17,6 +17,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/clk-provider.h>
> #include <linux/spinlock.h>
> +#include <linux/mv643xx_i2c.h>
> #include <net/dsa.h>
> #include <asm/page.h>
> #include <asm/timex.h>
> @@ -241,6 +242,7 @@ void __init kirkwood_clk_init(void)
> orion_clkdev_add("0", "pcie", pex0);
> orion_clkdev_add("1", "pcie", pex1);
> orion_clkdev_add(NULL, "kirkwood-i2s", audio);
> + orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".0", runit);
> }
>
> /*****************************************************************************
> diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
> index c179378..d245a87 100644
> --- a/arch/arm/plat-orion/common.c
> +++ b/arch/arm/plat-orion/common.c
> @@ -47,6 +47,7 @@ void __init orion_clkdev_init(struct clk *tclk)
> orion_clkdev_add(NULL, MV643XX_ETH_NAME ".2", tclk);
> orion_clkdev_add(NULL, MV643XX_ETH_NAME ".3", tclk);
> orion_clkdev_add(NULL, "orion_wdt", tclk);
> + orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".0", tclk);
> }
>
> /* Fill in the resources structure and link it into the platform
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 4f44a33..e97246b 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -18,6 +18,11 @@
> #include <linux/mv643xx_i2c.h>
> #include <linux/platform_device.h>
> #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_i2c.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
>
> /* Register defines */
> #define MV64XXX_I2C_REG_SLAVE_ADDR 0x00
> @@ -98,6 +103,7 @@ struct mv64xxx_i2c_data {
> int rc;
> u32 freq_m;
> u32 freq_n;
> + struct clk *clk;
> wait_queue_head_t waitq;
> spinlock_t lock;
> struct i2c_msg *msg;
> @@ -521,6 +527,36 @@ mv64xxx_i2c_unmap_regs(struct mv64xxx_i2c_data *drv_data)
> drv_data->reg_base_p = 0;
> }
>
> +#ifdef CONFIG_OF
Hooray, we have one big CONFIG_OF block, so we could move other stuff
here? See below.
> +static int __devinit
> +calc_freq(const int tclk, const int n, const int m)
> +{
> + return tclk / (10 * (m + 1) * (2 << n));
> +}
> +
> +static bool __devinit
> +find_baud_factors(const int req_freq, const int tclk, int *best_n, int *best_m)
> +{
> + int freq, delta, best_delta = INT_MAX;
> + int m, n;
> +
> + for (n = 0; n <= 7; n++)
> + for (m = 0; m <= 15; m++) {
> + freq = calc_freq(tclk, n, m);
> + delta = req_freq - freq;
> + if (delta >= 0 && delta < best_delta) {
> + *best_m = m;
> + *best_n = n;
> + best_delta = delta;
> + }
> + if (best_delta == 0)
> + return true;
> + }
> + if (best_delta == INT_MAX)
> + return false;
> + return true;
> +}
> +#endif
> static int __devinit
> mv64xxx_i2c_probe(struct platform_device *pd)
> {
> @@ -528,7 +564,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> struct mv64xxx_i2c_pdata *pdata = pd->dev.platform_data;
> int rc;
>
> - if ((pd->id != 0) || !pdata)
> + if ((!pdata && !pd->dev.of_node) || (pdata && (pd->id != 0)))
> return -ENODEV;
(extra points if you rebase your patch on top of this patch
d61a9095155e832287552a9e565b8756ee293c46 from my next tree:
git://git.pengutronix.de/git/wsa/linux.git i2c-embedded/for-next)
>
> drv_data = kzalloc(sizeof(struct mv64xxx_i2c_data), GFP_KERNEL);
> @@ -546,19 +582,64 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> init_waitqueue_head(&drv_data->waitq);
> spin_lock_init(&drv_data->lock);
>
> - drv_data->freq_m = pdata->freq_m;
> - drv_data->freq_n = pdata->freq_n;
> - drv_data->irq = platform_get_irq(pd, 0);
> +#if defined(CONFIG_HAVE_CLK)
> + /* Not all platforms have a clk */
> + drv_data->clk = clk_get(&pd->dev, NULL);
> + if (!IS_ERR(drv_data->clk)) {
> + clk_prepare(drv_data->clk);
> + clk_enable(drv_data->clk);
> + }
Call this code only when CONFIG_OF?
> +#endif
> + if (pdata) {
> + drv_data->freq_m = pdata->freq_m;
> + drv_data->freq_n = pdata->freq_n;
> + drv_data->irq = platform_get_irq(pd, 0);
> + }
> +#ifdef CONFIG_OF
> + if (pd->dev.of_node) {
Move this stuff into a seperate function and put it into the big
CONFIG_OF block? That should make the code a lot more readable?
> + int bus_freq;
> + int tclk;
> + /* CLK is mandatory when using DT to describe the i2c
> + * bus. We need to know tclk in order to calculate bus
> + * clock factors. */
> +#if defined(CONFIG_HAVE_CLK)
> + if (IS_ERR(drv_data->clk)) {
> + rc = -ENODEV;
> + goto exit_unmap_regs;
> + }
> + tclk = clk_get_rate(drv_data->clk);
> + of_property_read_u32(pd->dev.of_node, "clock-frequency",
> + &bus_freq);
> + if (!find_baud_factors(bus_freq, tclk,
> + &drv_data->freq_n, &drv_data->freq_m)) {
> + rc = -EINVAL;
> + goto exit_unmap_regs;
> + }
> + drv_data->irq = irq_of_parse_and_map(pd->dev.of_node, 0);
> +#else
> + /* Have OF but no CLK */
> + rc = -ENODEV;
> + goto exit_unmap_regs;
> +#endif
> + }
> +#endif
> if (drv_data->irq < 0) {
> rc = -ENXIO;
> goto exit_unmap_regs;
> }
> +
> drv_data->adapter.dev.parent = &pd->dev;
> drv_data->adapter.algo = &mv64xxx_i2c_algo;
> drv_data->adapter.owner = THIS_MODULE;
> drv_data->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> - drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
> + if (pd->dev.of_node)
> + drv_data->adapter.timeout = msecs_to_jiffies(
> + of_property_read_u32(pd->dev.of_node, "timeout-ms",
> + &drv_data->freq_n));
> + else
> + drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
This should also go into the new function.
> drv_data->adapter.nr = pd->id;
> + drv_data->adapter.dev.of_node = pd->dev.of_node;
> platform_set_drvdata(pd, drv_data);
> i2c_set_adapdata(&drv_data->adapter, drv_data);
>
> @@ -577,6 +658,8 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> goto exit_free_irq;
> }
>
> + of_i2c_register_devices(&drv_data->adapter);
> +
> return 0;
>
> exit_free_irq:
> @@ -597,17 +680,31 @@ mv64xxx_i2c_remove(struct platform_device *dev)
> rc = i2c_del_adapter(&drv_data->adapter);
> free_irq(drv_data->irq, drv_data);
> mv64xxx_i2c_unmap_regs(drv_data);
> +#if defined(CONFIG_HAVE_CLK)
> + /* Not all platforms have a clk */
> + if (!IS_ERR(drv_data->clk)) {
> + clk_disable(drv_data->clk);
> + clk_unprepare(drv_data->clk);
> + }
> +#endif
> kfree(drv_data);
>
> return rc;
> }
>
> +static const struct of_device_id mv64xxx_i2c_of_match_table[] __devinitdata = {
> + { .compatible = "marvell,mv64xxx-i2c", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
> +
> static struct platform_driver mv64xxx_i2c_driver = {
> .probe = mv64xxx_i2c_probe,
> .remove = __devexit_p(mv64xxx_i2c_remove),
> .driver = {
> .owner = THIS_MODULE,
> .name = MV64XXX_I2C_CTLR_NAME,
> + .of_match_table = of_match_ptr(mv64xxx_i2c_of_match_table),
> },
> };
>
> --
> 1.7.10
>
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCHv3] I2C: MV64XXX: Add Device Tree support
@ 2012-07-20 10:35 ` Wolfram Sang
0 siblings, 0 replies; 12+ messages in thread
From: Wolfram Sang @ 2012-07-20 10:35 UTC (permalink / raw)
To: linux-arm-kernel
Hi Andrew,
On Mon, Jul 16, 2012 at 11:16:45AM +0200, Andrew Lunn wrote:
> Extends the driver to get properties from device tree. Rather than
> pass the N & M factors in DT, use the more standard clock-frequency
> property. Calculate N & M at run time. In order to do this, we need to
Thanks.
> know tclk. So the driver uses clk_get() etc in order to get the clock
> and clk_get_rate() to determine the tclk rate. Not all platforms
> however have CLK, so some #ifdefery is needed to ensure the driver
> still compiles when CLK is not available.
>
> Also extend the kirkwood DT support to supply the needed properties. A
> default clock-frequency is 400KHz, which some platforms might need to
> override to 100KHz if they have devices which do not support fast
> mode.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@googlemail.com>
Has Sebastian acked THIS patch? Didn't saw such a mail? If it was a
previous patch which he acked, I'd like it to be resent, since the
divider calculation is new.
> ---
> v2: Removed duplicate interrupt in DT.
> v3: Use clock-frequency property, instead of explicit n & m baud factors.
>
> i2c fix.
> ---
> Documentation/devicetree/bindings/i2c/mrvl-i2c.txt | 21 +++-
> arch/arm/boot/dts/kirkwood.dtsi | 11 ++
> arch/arm/mach-kirkwood/board-dt.c | 2 +
> arch/arm/mach-kirkwood/common.c | 2 +
> arch/arm/plat-orion/common.c | 1 +
> drivers/i2c/busses/i2c-mv64xxx.c | 107 +++++++++++++++++++-
As said in another mail, please split up arch/arm and i2c-stuff.
> 6 files changed, 138 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt b/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
> index b891ee2..885f340 100644
> --- a/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
> +++ b/Documentation/devicetree/bindings/i2c/mrvl-i2c.txt
> @@ -1,4 +1,4 @@
> -* I2C
> +* Marvell MMP I2C controller
>
> Required properties :
>
> @@ -32,3 +32,22 @@ Examples:
> interrupts = <58>;
> };
>
> +* Marvell MV64XXX I2C controller
> +
> +Required properties :
> +
> + - reg : Offset and length of the register set for the device
> + - compatible : Should be "marvell,mv64xxx-i2c"
> + - interrupts : Te interrupt number
> + - clock-frequency : Desired I2C bus clock frequency in Hz.
> + - timeout-ms : How long to wait for a transaction to complete
If you don't really need "timeout-ms", I'd ask you to drop it. I haven't
made up my mind how to handle those timeouts yet. They should be
per-slave and not per-bus, but the core cannot handle this yet. And even
though I am tempted to accept a per-bus-timeout, getting the binding
right is not something we can come up with before the next merge window.
I am suspecting a default value could do for now?
> +
> +Examples:
> +
> + i2c at 11000 {
> + compatible = "marvell,mv64xxx-i2c";
> + reg = <0x11000 0x20>;
> + interrupts = <29>;
> + clock-frequency = <100000>;
> + timeout-ms = <1000>;
> + };
> diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> index a3350fd..bc57d81 100644
> --- a/arch/arm/boot/dts/kirkwood.dtsi
> +++ b/arch/arm/boot/dts/kirkwood.dtsi
> @@ -82,5 +82,16 @@
> /* set partition map and/or chip-delay in board dts */
> status = "disabled";
> };
> +
> + i2c at 11000 {
> + compatible = "marvell,mv64xxx-i2c";
> + reg = <0x11000 0x20>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <29>;
> + clock-frequency = <400000>;
> + timeout-ms = <1000>;
> + status = "disable";
> + };
> };
> };
> diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
> index 8f8da5d..24c8fdd 100644
> --- a/arch/arm/mach-kirkwood/board-dt.c
> +++ b/arch/arm/mach-kirkwood/board-dt.c
> @@ -28,6 +28,8 @@ static struct of_device_id kirkwood_dt_match_table[] __initdata = {
>
> struct of_dev_auxdata kirkwood_auxdata_lookup[] __initdata = {
> OF_DEV_AUXDATA("marvell,orion-spi", 0xf1010600, "orion_spi.0", NULL),
> + OF_DEV_AUXDATA("marvell,mv64xxx-i2c", 0xf1011000, "mv64xxx_i2c.0",
> + NULL),
> {},
> };
>
> diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
> index f261cd2..078d9c6 100644
> --- a/arch/arm/mach-kirkwood/common.c
> +++ b/arch/arm/mach-kirkwood/common.c
> @@ -17,6 +17,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/clk-provider.h>
> #include <linux/spinlock.h>
> +#include <linux/mv643xx_i2c.h>
> #include <net/dsa.h>
> #include <asm/page.h>
> #include <asm/timex.h>
> @@ -241,6 +242,7 @@ void __init kirkwood_clk_init(void)
> orion_clkdev_add("0", "pcie", pex0);
> orion_clkdev_add("1", "pcie", pex1);
> orion_clkdev_add(NULL, "kirkwood-i2s", audio);
> + orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".0", runit);
> }
>
> /*****************************************************************************
> diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
> index c179378..d245a87 100644
> --- a/arch/arm/plat-orion/common.c
> +++ b/arch/arm/plat-orion/common.c
> @@ -47,6 +47,7 @@ void __init orion_clkdev_init(struct clk *tclk)
> orion_clkdev_add(NULL, MV643XX_ETH_NAME ".2", tclk);
> orion_clkdev_add(NULL, MV643XX_ETH_NAME ".3", tclk);
> orion_clkdev_add(NULL, "orion_wdt", tclk);
> + orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".0", tclk);
> }
>
> /* Fill in the resources structure and link it into the platform
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 4f44a33..e97246b 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -18,6 +18,11 @@
> #include <linux/mv643xx_i2c.h>
> #include <linux/platform_device.h>
> #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_i2c.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
>
> /* Register defines */
> #define MV64XXX_I2C_REG_SLAVE_ADDR 0x00
> @@ -98,6 +103,7 @@ struct mv64xxx_i2c_data {
> int rc;
> u32 freq_m;
> u32 freq_n;
> + struct clk *clk;
> wait_queue_head_t waitq;
> spinlock_t lock;
> struct i2c_msg *msg;
> @@ -521,6 +527,36 @@ mv64xxx_i2c_unmap_regs(struct mv64xxx_i2c_data *drv_data)
> drv_data->reg_base_p = 0;
> }
>
> +#ifdef CONFIG_OF
Hooray, we have one big CONFIG_OF block, so we could move other stuff
here? See below.
> +static int __devinit
> +calc_freq(const int tclk, const int n, const int m)
> +{
> + return tclk / (10 * (m + 1) * (2 << n));
> +}
> +
> +static bool __devinit
> +find_baud_factors(const int req_freq, const int tclk, int *best_n, int *best_m)
> +{
> + int freq, delta, best_delta = INT_MAX;
> + int m, n;
> +
> + for (n = 0; n <= 7; n++)
> + for (m = 0; m <= 15; m++) {
> + freq = calc_freq(tclk, n, m);
> + delta = req_freq - freq;
> + if (delta >= 0 && delta < best_delta) {
> + *best_m = m;
> + *best_n = n;
> + best_delta = delta;
> + }
> + if (best_delta == 0)
> + return true;
> + }
> + if (best_delta == INT_MAX)
> + return false;
> + return true;
> +}
> +#endif
> static int __devinit
> mv64xxx_i2c_probe(struct platform_device *pd)
> {
> @@ -528,7 +564,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> struct mv64xxx_i2c_pdata *pdata = pd->dev.platform_data;
> int rc;
>
> - if ((pd->id != 0) || !pdata)
> + if ((!pdata && !pd->dev.of_node) || (pdata && (pd->id != 0)))
> return -ENODEV;
(extra points if you rebase your patch on top of this patch
d61a9095155e832287552a9e565b8756ee293c46 from my next tree:
git://git.pengutronix.de/git/wsa/linux.git i2c-embedded/for-next)
>
> drv_data = kzalloc(sizeof(struct mv64xxx_i2c_data), GFP_KERNEL);
> @@ -546,19 +582,64 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> init_waitqueue_head(&drv_data->waitq);
> spin_lock_init(&drv_data->lock);
>
> - drv_data->freq_m = pdata->freq_m;
> - drv_data->freq_n = pdata->freq_n;
> - drv_data->irq = platform_get_irq(pd, 0);
> +#if defined(CONFIG_HAVE_CLK)
> + /* Not all platforms have a clk */
> + drv_data->clk = clk_get(&pd->dev, NULL);
> + if (!IS_ERR(drv_data->clk)) {
> + clk_prepare(drv_data->clk);
> + clk_enable(drv_data->clk);
> + }
Call this code only when CONFIG_OF?
> +#endif
> + if (pdata) {
> + drv_data->freq_m = pdata->freq_m;
> + drv_data->freq_n = pdata->freq_n;
> + drv_data->irq = platform_get_irq(pd, 0);
> + }
> +#ifdef CONFIG_OF
> + if (pd->dev.of_node) {
Move this stuff into a seperate function and put it into the big
CONFIG_OF block? That should make the code a lot more readable?
> + int bus_freq;
> + int tclk;
> + /* CLK is mandatory when using DT to describe the i2c
> + * bus. We need to know tclk in order to calculate bus
> + * clock factors. */
> +#if defined(CONFIG_HAVE_CLK)
> + if (IS_ERR(drv_data->clk)) {
> + rc = -ENODEV;
> + goto exit_unmap_regs;
> + }
> + tclk = clk_get_rate(drv_data->clk);
> + of_property_read_u32(pd->dev.of_node, "clock-frequency",
> + &bus_freq);
> + if (!find_baud_factors(bus_freq, tclk,
> + &drv_data->freq_n, &drv_data->freq_m)) {
> + rc = -EINVAL;
> + goto exit_unmap_regs;
> + }
> + drv_data->irq = irq_of_parse_and_map(pd->dev.of_node, 0);
> +#else
> + /* Have OF but no CLK */
> + rc = -ENODEV;
> + goto exit_unmap_regs;
> +#endif
> + }
> +#endif
> if (drv_data->irq < 0) {
> rc = -ENXIO;
> goto exit_unmap_regs;
> }
> +
> drv_data->adapter.dev.parent = &pd->dev;
> drv_data->adapter.algo = &mv64xxx_i2c_algo;
> drv_data->adapter.owner = THIS_MODULE;
> drv_data->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> - drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
> + if (pd->dev.of_node)
> + drv_data->adapter.timeout = msecs_to_jiffies(
> + of_property_read_u32(pd->dev.of_node, "timeout-ms",
> + &drv_data->freq_n));
> + else
> + drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
This should also go into the new function.
> drv_data->adapter.nr = pd->id;
> + drv_data->adapter.dev.of_node = pd->dev.of_node;
> platform_set_drvdata(pd, drv_data);
> i2c_set_adapdata(&drv_data->adapter, drv_data);
>
> @@ -577,6 +658,8 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> goto exit_free_irq;
> }
>
> + of_i2c_register_devices(&drv_data->adapter);
> +
> return 0;
>
> exit_free_irq:
> @@ -597,17 +680,31 @@ mv64xxx_i2c_remove(struct platform_device *dev)
> rc = i2c_del_adapter(&drv_data->adapter);
> free_irq(drv_data->irq, drv_data);
> mv64xxx_i2c_unmap_regs(drv_data);
> +#if defined(CONFIG_HAVE_CLK)
> + /* Not all platforms have a clk */
> + if (!IS_ERR(drv_data->clk)) {
> + clk_disable(drv_data->clk);
> + clk_unprepare(drv_data->clk);
> + }
> +#endif
> kfree(drv_data);
>
> return rc;
> }
>
> +static const struct of_device_id mv64xxx_i2c_of_match_table[] __devinitdata = {
> + { .compatible = "marvell,mv64xxx-i2c", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
> +
> static struct platform_driver mv64xxx_i2c_driver = {
> .probe = mv64xxx_i2c_probe,
> .remove = __devexit_p(mv64xxx_i2c_remove),
> .driver = {
> .owner = THIS_MODULE,
> .name = MV64XXX_I2C_CTLR_NAME,
> + .of_match_table = of_match_ptr(mv64xxx_i2c_of_match_table),
> },
> };
>
> --
> 1.7.10
>
--
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/20120720/2e239bae/attachment-0001.sig>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv3] I2C: MV64XXX: Add Device Tree support
2012-07-20 10:35 ` Wolfram Sang
@ 2012-07-20 11:17 ` Andrew Lunn
-1 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2012-07-20 11:17 UTC (permalink / raw)
To: Wolfram Sang
Cc: Andrew Lunn, sebastian.hesselbarth, linux-i2c, linux ARM, Jason Cooper
On Fri, Jul 20, 2012 at 12:35:13PM +0200, Wolfram Sang wrote:
> Hi Andrew,
>
> On Mon, Jul 16, 2012 at 11:16:45AM +0200, Andrew Lunn wrote:
> > Extends the driver to get properties from device tree. Rather than
> > pass the N & M factors in DT, use the more standard clock-frequency
> > property. Calculate N & M at run time. In order to do this, we need to
>
> Thanks.
>
> > know tclk. So the driver uses clk_get() etc in order to get the clock
> > and clk_get_rate() to determine the tclk rate. Not all platforms
> > however have CLK, so some #ifdefery is needed to ensure the driver
> > still compiles when CLK is not available.
> >
> > Also extend the kirkwood DT support to supply the needed properties. A
> > default clock-frequency is 400KHz, which some platforms might need to
> > override to 100KHz if they have devices which do not support fast
> > mode.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@googlemail.com>
>
> Has Sebastian acked THIS patch? Didn't saw such a mail? If it was a
> previous patch which he acked, I'd like it to be resent, since the
> divider calculation is new.
Yes and the version that came after it, fixing the timeout code, which
at your request i will rip out. It has also been tested by a couple of
other people on different kirkwood boards.
> > v2: Removed duplicate interrupt in DT.
> > v3: Use clock-frequency property, instead of explicit n & m baud factors.
> >
> > i2c fix.
> > ---
> > Documentation/devicetree/bindings/i2c/mrvl-i2c.txt | 21 +++-
> > arch/arm/boot/dts/kirkwood.dtsi | 11 ++
> > arch/arm/mach-kirkwood/board-dt.c | 2 +
> > arch/arm/mach-kirkwood/common.c | 2 +
> > arch/arm/plat-orion/common.c | 1 +
> > drivers/i2c/busses/i2c-mv64xxx.c | 107 +++++++++++++++++++-
>
> As said in another mail, please split up arch/arm and i2c-stuff.
Sure, no problem.
>
> > +Required properties :
> > +
> > + - reg : Offset and length of the register set for the device
> > + - compatible : Should be "marvell,mv64xxx-i2c"
> > + - interrupts : Te interrupt number
> > + - clock-frequency : Desired I2C bus clock frequency in Hz.
> > + - timeout-ms : How long to wait for a transaction to complete
>
> If you don't really need "timeout-ms", I'd ask you to drop it.
I can hard code it to 1 second. This should have no impact on any
plat-orion boards, since they all use 1 second.
> > +#ifdef CONFIG_OF
>
> Hooray, we have one big CONFIG_OF block, so we could move other stuff
> here? See below.
>
> > +static int __devinit
> > +calc_freq(const int tclk, const int n, const int m)
> > +{
> > + return tclk / (10 * (m + 1) * (2 << n));
> > +}
> > +
> > +static bool __devinit
> > +find_baud_factors(const int req_freq, const int tclk, int *best_n, int *best_m)
> > +{
> > + int freq, delta, best_delta = INT_MAX;
> > + int m, n;
> > +
> > + for (n = 0; n <= 7; n++)
> > + for (m = 0; m <= 15; m++) {
> > + freq = calc_freq(tclk, n, m);
> > + delta = req_freq - freq;
> > + if (delta >= 0 && delta < best_delta) {
> > + *best_m = m;
> > + *best_n = n;
> > + best_delta = delta;
> > + }
> > + if (best_delta == 0)
> > + return true;
> > + }
> > + if (best_delta == INT_MAX)
> > + return false;
> > + return true;
> > +}
> > +#endif
> > static int __devinit
> > mv64xxx_i2c_probe(struct platform_device *pd)
> > {
> > @@ -528,7 +564,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> > struct mv64xxx_i2c_pdata *pdata = pd->dev.platform_data;
> > int rc;
> >
> > - if ((pd->id != 0) || !pdata)
> > + if ((!pdata && !pd->dev.of_node) || (pdata && (pd->id != 0)))
> > return -ENODEV;
>
> (extra points if you rebase your patch on top of this patch
> d61a9095155e832287552a9e565b8756ee293c46 from my next tree:
> git://git.pengutronix.de/git/wsa/linux.git i2c-embedded/for-next)
Yes, i know of this patch from Florian Fainelli. I was planning on
warning you about the merge conflict, but maybe i will try for the
rebase.
> > drv_data = kzalloc(sizeof(struct mv64xxx_i2c_data), GFP_KERNEL);
> > @@ -546,19 +582,64 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> > init_waitqueue_head(&drv_data->waitq);
> > spin_lock_init(&drv_data->lock);
> >
> > - drv_data->freq_m = pdata->freq_m;
> > - drv_data->freq_n = pdata->freq_n;
> > - drv_data->irq = platform_get_irq(pd, 0);
> > +#if defined(CONFIG_HAVE_CLK)
> > + /* Not all platforms have a clk */
> > + drv_data->clk = clk_get(&pd->dev, NULL);
> > + if (!IS_ERR(drv_data->clk)) {
> > + clk_prepare(drv_data->clk);
> > + clk_enable(drv_data->clk);
> > + }
>
> Call this code only when CONFIG_OF?
Nope. There are lots of kirkwood boards who need to turn this clock
on, but are not yet converted to DT. I suspect Dove is similar. I also
don't particularly like all this #ifdefery, but there is not much i
can do about it :-(
>
> > +#endif
> > + if (pdata) {
> > + drv_data->freq_m = pdata->freq_m;
> > + drv_data->freq_n = pdata->freq_n;
> > + drv_data->irq = platform_get_irq(pd, 0);
> > + }
> > +#ifdef CONFIG_OF
> > + if (pd->dev.of_node) {
>
> Move this stuff into a seperate function and put it into the big
> CONFIG_OF block? That should make the code a lot more readable?
O.K.
> > + int bus_freq;
> > + int tclk;
> > + /* CLK is mandatory when using DT to describe the i2c
> > + * bus. We need to know tclk in order to calculate bus
> > + * clock factors. */
> > +#if defined(CONFIG_HAVE_CLK)
> > + if (IS_ERR(drv_data->clk)) {
> > + rc = -ENODEV;
> > + goto exit_unmap_regs;
> > + }
> > + tclk = clk_get_rate(drv_data->clk);
> > + of_property_read_u32(pd->dev.of_node, "clock-frequency",
> > + &bus_freq);
> > + if (!find_baud_factors(bus_freq, tclk,
> > + &drv_data->freq_n, &drv_data->freq_m)) {
> > + rc = -EINVAL;
> > + goto exit_unmap_regs;
> > + }
> > + drv_data->irq = irq_of_parse_and_map(pd->dev.of_node, 0);
> > +#else
> > + /* Have OF but no CLK */
> > + rc = -ENODEV;
> > + goto exit_unmap_regs;
> > +#endif
> > + }
> > +#endif
> > if (drv_data->irq < 0) {
> > rc = -ENXIO;
> > goto exit_unmap_regs;
> > }
> > +
> > drv_data->adapter.dev.parent = &pd->dev;
> > drv_data->adapter.algo = &mv64xxx_i2c_algo;
> > drv_data->adapter.owner = THIS_MODULE;
> > drv_data->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> > - drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
> > + if (pd->dev.of_node)
> > + drv_data->adapter.timeout = msecs_to_jiffies(
> > + of_property_read_u32(pd->dev.of_node, "timeout-ms",
> > + &drv_data->freq_n));
> > + else
> > + drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
>
> This should also go into the new function.
Actually, its totally broken, and is what i will replace with a hard
coded value...
Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCHv3] I2C: MV64XXX: Add Device Tree support
@ 2012-07-20 11:17 ` Andrew Lunn
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2012-07-20 11:17 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jul 20, 2012 at 12:35:13PM +0200, Wolfram Sang wrote:
> Hi Andrew,
>
> On Mon, Jul 16, 2012 at 11:16:45AM +0200, Andrew Lunn wrote:
> > Extends the driver to get properties from device tree. Rather than
> > pass the N & M factors in DT, use the more standard clock-frequency
> > property. Calculate N & M at run time. In order to do this, we need to
>
> Thanks.
>
> > know tclk. So the driver uses clk_get() etc in order to get the clock
> > and clk_get_rate() to determine the tclk rate. Not all platforms
> > however have CLK, so some #ifdefery is needed to ensure the driver
> > still compiles when CLK is not available.
> >
> > Also extend the kirkwood DT support to supply the needed properties. A
> > default clock-frequency is 400KHz, which some platforms might need to
> > override to 100KHz if they have devices which do not support fast
> > mode.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@googlemail.com>
>
> Has Sebastian acked THIS patch? Didn't saw such a mail? If it was a
> previous patch which he acked, I'd like it to be resent, since the
> divider calculation is new.
Yes and the version that came after it, fixing the timeout code, which
at your request i will rip out. It has also been tested by a couple of
other people on different kirkwood boards.
> > v2: Removed duplicate interrupt in DT.
> > v3: Use clock-frequency property, instead of explicit n & m baud factors.
> >
> > i2c fix.
> > ---
> > Documentation/devicetree/bindings/i2c/mrvl-i2c.txt | 21 +++-
> > arch/arm/boot/dts/kirkwood.dtsi | 11 ++
> > arch/arm/mach-kirkwood/board-dt.c | 2 +
> > arch/arm/mach-kirkwood/common.c | 2 +
> > arch/arm/plat-orion/common.c | 1 +
> > drivers/i2c/busses/i2c-mv64xxx.c | 107 +++++++++++++++++++-
>
> As said in another mail, please split up arch/arm and i2c-stuff.
Sure, no problem.
>
> > +Required properties :
> > +
> > + - reg : Offset and length of the register set for the device
> > + - compatible : Should be "marvell,mv64xxx-i2c"
> > + - interrupts : Te interrupt number
> > + - clock-frequency : Desired I2C bus clock frequency in Hz.
> > + - timeout-ms : How long to wait for a transaction to complete
>
> If you don't really need "timeout-ms", I'd ask you to drop it.
I can hard code it to 1 second. This should have no impact on any
plat-orion boards, since they all use 1 second.
> > +#ifdef CONFIG_OF
>
> Hooray, we have one big CONFIG_OF block, so we could move other stuff
> here? See below.
>
> > +static int __devinit
> > +calc_freq(const int tclk, const int n, const int m)
> > +{
> > + return tclk / (10 * (m + 1) * (2 << n));
> > +}
> > +
> > +static bool __devinit
> > +find_baud_factors(const int req_freq, const int tclk, int *best_n, int *best_m)
> > +{
> > + int freq, delta, best_delta = INT_MAX;
> > + int m, n;
> > +
> > + for (n = 0; n <= 7; n++)
> > + for (m = 0; m <= 15; m++) {
> > + freq = calc_freq(tclk, n, m);
> > + delta = req_freq - freq;
> > + if (delta >= 0 && delta < best_delta) {
> > + *best_m = m;
> > + *best_n = n;
> > + best_delta = delta;
> > + }
> > + if (best_delta == 0)
> > + return true;
> > + }
> > + if (best_delta == INT_MAX)
> > + return false;
> > + return true;
> > +}
> > +#endif
> > static int __devinit
> > mv64xxx_i2c_probe(struct platform_device *pd)
> > {
> > @@ -528,7 +564,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> > struct mv64xxx_i2c_pdata *pdata = pd->dev.platform_data;
> > int rc;
> >
> > - if ((pd->id != 0) || !pdata)
> > + if ((!pdata && !pd->dev.of_node) || (pdata && (pd->id != 0)))
> > return -ENODEV;
>
> (extra points if you rebase your patch on top of this patch
> d61a9095155e832287552a9e565b8756ee293c46 from my next tree:
> git://git.pengutronix.de/git/wsa/linux.git i2c-embedded/for-next)
Yes, i know of this patch from Florian Fainelli. I was planning on
warning you about the merge conflict, but maybe i will try for the
rebase.
> > drv_data = kzalloc(sizeof(struct mv64xxx_i2c_data), GFP_KERNEL);
> > @@ -546,19 +582,64 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> > init_waitqueue_head(&drv_data->waitq);
> > spin_lock_init(&drv_data->lock);
> >
> > - drv_data->freq_m = pdata->freq_m;
> > - drv_data->freq_n = pdata->freq_n;
> > - drv_data->irq = platform_get_irq(pd, 0);
> > +#if defined(CONFIG_HAVE_CLK)
> > + /* Not all platforms have a clk */
> > + drv_data->clk = clk_get(&pd->dev, NULL);
> > + if (!IS_ERR(drv_data->clk)) {
> > + clk_prepare(drv_data->clk);
> > + clk_enable(drv_data->clk);
> > + }
>
> Call this code only when CONFIG_OF?
Nope. There are lots of kirkwood boards who need to turn this clock
on, but are not yet converted to DT. I suspect Dove is similar. I also
don't particularly like all this #ifdefery, but there is not much i
can do about it :-(
>
> > +#endif
> > + if (pdata) {
> > + drv_data->freq_m = pdata->freq_m;
> > + drv_data->freq_n = pdata->freq_n;
> > + drv_data->irq = platform_get_irq(pd, 0);
> > + }
> > +#ifdef CONFIG_OF
> > + if (pd->dev.of_node) {
>
> Move this stuff into a seperate function and put it into the big
> CONFIG_OF block? That should make the code a lot more readable?
O.K.
> > + int bus_freq;
> > + int tclk;
> > + /* CLK is mandatory when using DT to describe the i2c
> > + * bus. We need to know tclk in order to calculate bus
> > + * clock factors. */
> > +#if defined(CONFIG_HAVE_CLK)
> > + if (IS_ERR(drv_data->clk)) {
> > + rc = -ENODEV;
> > + goto exit_unmap_regs;
> > + }
> > + tclk = clk_get_rate(drv_data->clk);
> > + of_property_read_u32(pd->dev.of_node, "clock-frequency",
> > + &bus_freq);
> > + if (!find_baud_factors(bus_freq, tclk,
> > + &drv_data->freq_n, &drv_data->freq_m)) {
> > + rc = -EINVAL;
> > + goto exit_unmap_regs;
> > + }
> > + drv_data->irq = irq_of_parse_and_map(pd->dev.of_node, 0);
> > +#else
> > + /* Have OF but no CLK */
> > + rc = -ENODEV;
> > + goto exit_unmap_regs;
> > +#endif
> > + }
> > +#endif
> > if (drv_data->irq < 0) {
> > rc = -ENXIO;
> > goto exit_unmap_regs;
> > }
> > +
> > drv_data->adapter.dev.parent = &pd->dev;
> > drv_data->adapter.algo = &mv64xxx_i2c_algo;
> > drv_data->adapter.owner = THIS_MODULE;
> > drv_data->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> > - drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
> > + if (pd->dev.of_node)
> > + drv_data->adapter.timeout = msecs_to_jiffies(
> > + of_property_read_u32(pd->dev.of_node, "timeout-ms",
> > + &drv_data->freq_n));
> > + else
> > + drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
>
> This should also go into the new function.
Actually, its totally broken, and is what i will replace with a hard
coded value...
Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-07-20 11:17 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-16 9:16 [PATCHv3] I2C: MV64XXX: Add Device Tree support Andrew Lunn
2012-07-16 9:16 ` Andrew Lunn
2012-07-16 9:16 ` Andrew Lunn
2012-07-16 9:16 ` Andrew Lunn
2012-07-16 12:20 ` Thomas Petazzoni
2012-07-16 12:20 ` Thomas Petazzoni
2012-07-16 12:37 ` Andrew Lunn
2012-07-16 12:37 ` Andrew Lunn
2012-07-20 10:35 ` Wolfram Sang
2012-07-20 10:35 ` Wolfram Sang
2012-07-20 11:17 ` Andrew Lunn
2012-07-20 11:17 ` Andrew Lunn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.