[PATCHv4 0/9] Add I2C support for Allwinner SoCs

[PATCHv4 0/9] Add I2C support for Allwinner SoCs

[Maxime Ripard]

Hi,

This patchset adds support for the I2C controller found on most of the
Allwinner SoCs, especially the already supported A10 and A13, and the
yet to come A31.

This driver leverages the Marvel mv64xxx i2c controller driver, that has
an almost identical logic, with a slightly different register layout.

It has been tested on a A13-Olinuxino and an A10s-Olinuxino.

Thanks,
Maxime

Changes from v3:
  * Merged the driver in the Marvell mv64xxx i2c controller

Changes from v2:
  * Slightly modified the switch comments again
  * Removed the of_* calls in favor of platform_get_* functions

Changes from v1:
  * Added comments to the switch statements to clarify when the fall through to
    the next case is made on purpose
  * Use devm_ioremap_resource instead of of_iomap
  * Moved the reset after enabling the clocks
  * Added Emilio Lopez' patch to add the available i2c controllers to the
    cubieboard

Emilio López (1):
  ARM: sun4i: cubieboard: Enable the i2c controllers

Maxime Ripard (8):
  i2c: mv64xxx: Add macros to access parts of registers
  i2c: mv64xxx: make the registers offset configurable
  ARM: orion: pass the i2c registers definition through the platform
    data
  i2c: mv64xxx: Add Allwinner sun4i compatible
  ARM: sunxi: dt: Add i2c controller nodes to the DTSI
  ARM: sun4i: dt: Add i2c muxing options
  ARM: sun5i: dt: Add i2c muxing options
  ARM: sun5i: olinuxino: Enable the i2c controllers

 arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 12 ++++
 arch/arm/boot/dts/sun4i-a10.dtsi           | 47 ++++++++++++++++
 arch/arm/boot/dts/sun5i-a13-olinuxino.dts  | 18 ++++++
 arch/arm/boot/dts/sun5i-a13.dtsi           | 48 ++++++++++++++++
 arch/arm/plat-orion/common.c               |  2 +
 drivers/i2c/busses/Kconfig                 |  3 +-
 drivers/i2c/busses/i2c-mv64xxx.c           | 88 ++++++++++++++++--------------
 include/linux/mv643xx_i2c.h                | 37 ++++++++++++-
 8 files changed, 211 insertions(+), 44 deletions(-)

-- 
1.8.3

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/groups/opt_out.

[PATCHv4 1/9] i2c: mv64xxx: Add macros to access parts of registers

[Maxime Ripard]

These macros make it more comprehensive to access to useful masked and
shifted area of the various registers used.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 6356439..d70a2fda 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -33,6 +33,10 @@
 #define	MV64XXX_I2C_REG_EXT_SLAVE_ADDR			0x10
 #define	MV64XXX_I2C_REG_SOFT_RESET			0x1c
 
+#define MV64XXX_I2C_ADDR_ADDR(val)			((val & 0x7f) << 1)
+#define MV64XXX_I2C_BAUD_DIV_N(val)			(val & 0x7)
+#define MV64XXX_I2C_BAUD_DIV_M(val)			((val & 0xf) << 3)
+
 #define	MV64XXX_I2C_REG_CONTROL_ACK			0x00000004
 #define	MV64XXX_I2C_REG_CONTROL_IFLG			0x00000008
 #define	MV64XXX_I2C_REG_CONTROL_STOP			0x00000010
@@ -133,7 +137,7 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 		drv_data->addr1 = 0xf0 | (((u32)msg->addr & 0x300) >> 7) | dir;
 		drv_data->addr2 = (u32)msg->addr & 0xff;
 	} else {
-		drv_data->addr1 = ((u32)msg->addr & 0x7f) << 1 | dir;
+		drv_data->addr1 = MV64XXX_I2C_ADDR_ADDR((u32)msg->addr) | dir;
 		drv_data->addr2 = 0;
 	}
 }
@@ -151,7 +155,7 @@ static void
 mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
 {
 	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SOFT_RESET);
-	writel((((drv_data->freq_m & 0xf) << 3) | (drv_data->freq_n & 0x7)),
+	writel(MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n),
 		drv_data->reg_base + MV64XXX_I2C_REG_BAUD);
 	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SLAVE_ADDR);
 	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_EXT_SLAVE_ADDR);
-- 
1.8.3

[PATCHv4 2/9] i2c: mv64xxx: make the registers offset configurable

[Maxime Ripard]

The Allwinner i2c controller uses the same logic as the Marvell one, but
with slightly different register offsets.

Introduce a structure that will be passed by either the pdata or
associated to the compatible strings, and that holds the various
registers that might be needed.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 81 +++++++++++++++++++++-------------------
 include/linux/mv643xx_i2c.h      | 27 ++++++++++++--
 2 files changed, 66 insertions(+), 42 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index d70a2fda..f9e076e 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -19,20 +19,12 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_device.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
-#define	MV64XXX_I2C_REG_DATA				0x04
-#define	MV64XXX_I2C_REG_CONTROL				0x08
-#define	MV64XXX_I2C_REG_STATUS				0x0c
-#define	MV64XXX_I2C_REG_BAUD				0x0c
-#define	MV64XXX_I2C_REG_EXT_SLAVE_ADDR			0x10
-#define	MV64XXX_I2C_REG_SOFT_RESET			0x1c
-
 #define MV64XXX_I2C_ADDR_ADDR(val)			((val & 0x7f) << 1)
 #define MV64XXX_I2C_BAUD_DIV_N(val)			(val & 0x7)
 #define MV64XXX_I2C_BAUD_DIV_M(val)			((val & 0xf) << 3)
@@ -98,6 +90,7 @@ struct mv64xxx_i2c_data {
 	u32			aborting;
 	u32			cntl_bits;
 	void __iomem		*reg_base;
+	struct mv64xxx_i2c_regs	*regs;
 	u32			addr1;
 	u32			addr2;
 	u32			bytes_left;
@@ -154,13 +147,13 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 static void
 mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
 {
-	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SOFT_RESET);
+	writel(0, drv_data->reg_base + drv_data->regs->soft_reset);
 	writel(MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n),
-		drv_data->reg_base + MV64XXX_I2C_REG_BAUD);
-	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SLAVE_ADDR);
-	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_EXT_SLAVE_ADDR);
+		drv_data->reg_base + drv_data->regs->clock);
+	writel(0, drv_data->reg_base + drv_data->regs->addr);
+	writel(0, drv_data->reg_base + drv_data->regs->ext_addr);
 	writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP,
-		drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+		drv_data->reg_base + drv_data->regs->control);
 	drv_data->state = MV64XXX_I2C_STATE_IDLE;
 }
 
@@ -282,7 +275,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 
 		drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
 		writel(drv_data->cntl_bits,
-			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+			drv_data->reg_base + drv_data->regs->control);
 
 		drv_data->msgs++;
 		drv_data->num_msgs--;
@@ -300,48 +293,48 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 
 	case MV64XXX_I2C_ACTION_CONTINUE:
 		writel(drv_data->cntl_bits,
-			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+			drv_data->reg_base + drv_data->regs->control);
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_START:
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
-			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+			drv_data->reg_base + drv_data->regs->control);
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
 		writel(drv_data->addr1,
-			drv_data->reg_base + MV64XXX_I2C_REG_DATA);
+			drv_data->reg_base + drv_data->regs->data);
 		writel(drv_data->cntl_bits,
-			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+			drv_data->reg_base + drv_data->regs->control);
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_ADDR_2:
 		writel(drv_data->addr2,
-			drv_data->reg_base + MV64XXX_I2C_REG_DATA);
+			drv_data->reg_base + drv_data->regs->data);
 		writel(drv_data->cntl_bits,
-			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+			drv_data->reg_base + drv_data->regs->control);
 		break;
 
 	case MV64XXX_I2C_ACTION_SEND_DATA:
 		writel(drv_data->msg->buf[drv_data->byte_posn++],
-			drv_data->reg_base + MV64XXX_I2C_REG_DATA);
+			drv_data->reg_base + drv_data->regs->data);
 		writel(drv_data->cntl_bits,
-			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+			drv_data->reg_base + drv_data->regs->control);
 		break;
 
 	case MV64XXX_I2C_ACTION_RCV_DATA:
 		drv_data->msg->buf[drv_data->byte_posn++] =
-			readl(drv_data->reg_base + MV64XXX_I2C_REG_DATA);
+			readl(drv_data->reg_base + drv_data->regs->data);
 		writel(drv_data->cntl_bits,
-			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+			drv_data->reg_base + drv_data->regs->control);
 		break;
 
 	case MV64XXX_I2C_ACTION_RCV_DATA_STOP:
 		drv_data->msg->buf[drv_data->byte_posn++] =
-			readl(drv_data->reg_base + MV64XXX_I2C_REG_DATA);
+			readl(drv_data->reg_base + drv_data->regs->data);
 		drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
-			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+			drv_data->reg_base + drv_data->regs->control);
 		drv_data->block = 0;
 		wake_up(&drv_data->waitq);
 		break;
@@ -356,7 +349,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 	case MV64XXX_I2C_ACTION_SEND_STOP:
 		drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
-			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
+			drv_data->reg_base + drv_data->regs->control);
 		drv_data->block = 0;
 		wake_up(&drv_data->waitq);
 		break;
@@ -372,9 +365,9 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
 	irqreturn_t	rc = IRQ_NONE;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
-	while (readl(drv_data->reg_base + MV64XXX_I2C_REG_CONTROL) &
+	while (readl(drv_data->reg_base + drv_data->regs->control) &
 						MV64XXX_I2C_REG_CONTROL_IFLG) {
-		status = readl(drv_data->reg_base + MV64XXX_I2C_REG_STATUS);
+		status = readl(drv_data->reg_base + drv_data->regs->status);
 		mv64xxx_i2c_fsm(drv_data, status);
 		mv64xxx_i2c_do_action(drv_data);
 		rc = IRQ_HANDLED;
@@ -496,6 +489,12 @@ static const struct i2c_algorithm mv64xxx_i2c_algo = {
  *****************************************************************************
  */
 #ifdef CONFIG_OF
+static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
+	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
+	{}
+};
+MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
+
 static int
 mv64xxx_calc_freq(const int tclk, const int n, const int m)
 {
@@ -528,8 +527,10 @@ mv64xxx_find_baud_factors(const u32 req_freq, const u32 tclk, u32 *best_n,
 
 static int
 mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
-		  struct device_node *np)
+		  struct device *dev)
 {
+	const struct of_device_id *device;
+	struct device_node *np = dev->of_node;
 	u32 bus_freq, tclk;
 	int rc = 0;
 
@@ -558,6 +559,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 	 * So hard code the value to 1 second.
 	 */
 	drv_data->adapter.timeout = HZ;
+
+	device = of_match_device(mv64xxx_i2c_of_match_table, dev);
+	if (!device)
+		return -ENODEV;
+
+	drv_data->regs = (struct mv64xxx_i2c_regs *)device->data;
+
 out:
 	return rc;
 #endif
@@ -565,7 +573,7 @@ out:
 #else /* CONFIG_OF */
 static int
 mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
-		  struct device_node *np)
+		  struct device *dev)
 {
 	return -ENODEV;
 }
@@ -611,8 +619,9 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 		drv_data->freq_n = pdata->freq_n;
 		drv_data->irq = platform_get_irq(pd, 0);
 		drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
+		drv_data->regs = pdata->regs;
 	} else if (pd->dev.of_node) {
-		rc = mv64xxx_of_config(drv_data, pd->dev.of_node);
+		rc = mv64xxx_of_config(drv_data, &pd->dev);
 		if (rc)
 			goto exit_clk;
 	}
@@ -680,12 +689,6 @@ mv64xxx_i2c_remove(struct platform_device *dev)
 	return 0;
 }
 
-static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
-	{ .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	= mv64xxx_i2c_remove,
diff --git a/include/linux/mv643xx_i2c.h b/include/linux/mv643xx_i2c.h
index 5db5152..9304c94 100644
--- a/include/linux/mv643xx_i2c.h
+++ b/include/linux/mv643xx_i2c.h
@@ -12,11 +12,32 @@
 
 #define MV64XXX_I2C_CTLR_NAME	"mv64xxx_i2c"
 
+struct mv64xxx_i2c_regs {
+	u8	addr;
+	u8	ext_addr;
+	u8	data;
+	u8	control;
+	u8	status;
+	u8	clock;
+	u8	soft_reset;
+};
+
+struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
+	.addr		= 0x00,
+	.ext_addr	= 0x10,
+	.data		= 0x04,
+	.control	= 0x08,
+	.status		= 0x0c,
+	.clock		= 0x0c,
+	.soft_reset	= 0x1c,
+};
+
 /* i2c Platform Device, Driver Data */
 struct mv64xxx_i2c_pdata {
-	u32	freq_m;
-	u32	freq_n;
-	u32	timeout;	/* In milliseconds */
+	u32			freq_m;
+	u32			freq_n;
+	struct mv64xxx_i2c_regs *regs;
+	u32			timeout;	/* In milliseconds */
 };
 
 #endif /*_MV64XXX_I2C_H_*/
-- 
1.8.3

Re: [PATCHv4 2/9] i2c: mv64xxx: make the registers offset configurable

[Andrew Lunn]

On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote:
> The Allwinner i2c controller uses the same logic as the Marvell one, but
> with slightly different register offsets.
> 
> Introduce a structure that will be passed by either the pdata or
> associated to the compatible strings, and that holds the various
> registers that might be needed.

Hi Maxime

I don't think this change is bisectable. It assumes the platform data
passes the registers, which is not true until the next patch.

Please can you re-arrange the order. Add the extra information to the
platform data, and then make use of it, not the other way around.

Thanks
	 Andrew


> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 81 +++++++++++++++++++++-------------------
>  include/linux/mv643xx_i2c.h      | 27 ++++++++++++--
>  2 files changed, 66 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index d70a2fda..f9e076e 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -19,20 +19,12 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
> +#include <linux/of_device.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
> -#define	MV64XXX_I2C_REG_DATA				0x04
> -#define	MV64XXX_I2C_REG_CONTROL				0x08
> -#define	MV64XXX_I2C_REG_STATUS				0x0c
> -#define	MV64XXX_I2C_REG_BAUD				0x0c
> -#define	MV64XXX_I2C_REG_EXT_SLAVE_ADDR			0x10
> -#define	MV64XXX_I2C_REG_SOFT_RESET			0x1c
> -
>  #define MV64XXX_I2C_ADDR_ADDR(val)			((val & 0x7f) << 1)
>  #define MV64XXX_I2C_BAUD_DIV_N(val)			(val & 0x7)
>  #define MV64XXX_I2C_BAUD_DIV_M(val)			((val & 0xf) << 3)
> @@ -98,6 +90,7 @@ struct mv64xxx_i2c_data {
>  	u32			aborting;
>  	u32			cntl_bits;
>  	void __iomem		*reg_base;
> +	struct mv64xxx_i2c_regs	*regs;
>  	u32			addr1;
>  	u32			addr2;
>  	u32			bytes_left;
> @@ -154,13 +147,13 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
>  static void
>  mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
>  {
> -	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SOFT_RESET);
> +	writel(0, drv_data->reg_base + drv_data->regs->soft_reset);
>  	writel(MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n),
> -		drv_data->reg_base + MV64XXX_I2C_REG_BAUD);
> -	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SLAVE_ADDR);
> -	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_EXT_SLAVE_ADDR);
> +		drv_data->reg_base + drv_data->regs->clock);
> +	writel(0, drv_data->reg_base + drv_data->regs->addr);
> +	writel(0, drv_data->reg_base + drv_data->regs->ext_addr);
>  	writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP,
> -		drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +		drv_data->reg_base + drv_data->regs->control);
>  	drv_data->state = MV64XXX_I2C_STATE_IDLE;
>  }
>  
> @@ -282,7 +275,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  
>  		drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
>  		writel(drv_data->cntl_bits,
> -			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +			drv_data->reg_base + drv_data->regs->control);
>  
>  		drv_data->msgs++;
>  		drv_data->num_msgs--;
> @@ -300,48 +293,48 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  
>  	case MV64XXX_I2C_ACTION_CONTINUE:
>  		writel(drv_data->cntl_bits,
> -			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +			drv_data->reg_base + drv_data->regs->control);
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_SEND_START:
>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
> -			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +			drv_data->reg_base + drv_data->regs->control);
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_SEND_ADDR_1:
>  		writel(drv_data->addr1,
> -			drv_data->reg_base + MV64XXX_I2C_REG_DATA);
> +			drv_data->reg_base + drv_data->regs->data);
>  		writel(drv_data->cntl_bits,
> -			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +			drv_data->reg_base + drv_data->regs->control);
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_SEND_ADDR_2:
>  		writel(drv_data->addr2,
> -			drv_data->reg_base + MV64XXX_I2C_REG_DATA);
> +			drv_data->reg_base + drv_data->regs->data);
>  		writel(drv_data->cntl_bits,
> -			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +			drv_data->reg_base + drv_data->regs->control);
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_SEND_DATA:
>  		writel(drv_data->msg->buf[drv_data->byte_posn++],
> -			drv_data->reg_base + MV64XXX_I2C_REG_DATA);
> +			drv_data->reg_base + drv_data->regs->data);
>  		writel(drv_data->cntl_bits,
> -			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +			drv_data->reg_base + drv_data->regs->control);
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_RCV_DATA:
>  		drv_data->msg->buf[drv_data->byte_posn++] =
> -			readl(drv_data->reg_base + MV64XXX_I2C_REG_DATA);
> +			readl(drv_data->reg_base + drv_data->regs->data);
>  		writel(drv_data->cntl_bits,
> -			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +			drv_data->reg_base + drv_data->regs->control);
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_RCV_DATA_STOP:
>  		drv_data->msg->buf[drv_data->byte_posn++] =
> -			readl(drv_data->reg_base + MV64XXX_I2C_REG_DATA);
> +			readl(drv_data->reg_base + drv_data->regs->data);
>  		drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
> -			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +			drv_data->reg_base + drv_data->regs->control);
>  		drv_data->block = 0;
>  		wake_up(&drv_data->waitq);
>  		break;
> @@ -356,7 +349,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  	case MV64XXX_I2C_ACTION_SEND_STOP:
>  		drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
>  		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
> -			drv_data->reg_base + MV64XXX_I2C_REG_CONTROL);
> +			drv_data->reg_base + drv_data->regs->control);
>  		drv_data->block = 0;
>  		wake_up(&drv_data->waitq);
>  		break;
> @@ -372,9 +365,9 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
>  	irqreturn_t	rc = IRQ_NONE;
>  
>  	spin_lock_irqsave(&drv_data->lock, flags);
> -	while (readl(drv_data->reg_base + MV64XXX_I2C_REG_CONTROL) &
> +	while (readl(drv_data->reg_base + drv_data->regs->control) &
>  						MV64XXX_I2C_REG_CONTROL_IFLG) {
> -		status = readl(drv_data->reg_base + MV64XXX_I2C_REG_STATUS);
> +		status = readl(drv_data->reg_base + drv_data->regs->status);
>  		mv64xxx_i2c_fsm(drv_data, status);
>  		mv64xxx_i2c_do_action(drv_data);
>  		rc = IRQ_HANDLED;
> @@ -496,6 +489,12 @@ static const struct i2c_algorithm mv64xxx_i2c_algo = {
>   *****************************************************************************
>   */
>  #ifdef CONFIG_OF
> +static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
> +	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
> +
>  static int
>  mv64xxx_calc_freq(const int tclk, const int n, const int m)
>  {
> @@ -528,8 +527,10 @@ mv64xxx_find_baud_factors(const u32 req_freq, const u32 tclk, u32 *best_n,
>  
>  static int
>  mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
> -		  struct device_node *np)
> +		  struct device *dev)
>  {
> +	const struct of_device_id *device;
> +	struct device_node *np = dev->of_node;
>  	u32 bus_freq, tclk;
>  	int rc = 0;
>  
> @@ -558,6 +559,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>  	 * So hard code the value to 1 second.
>  	 */
>  	drv_data->adapter.timeout = HZ;
> +
> +	device = of_match_device(mv64xxx_i2c_of_match_table, dev);
> +	if (!device)
> +		return -ENODEV;
> +
> +	drv_data->regs = (struct mv64xxx_i2c_regs *)device->data;
> +
>  out:
>  	return rc;
>  #endif
> @@ -565,7 +573,7 @@ out:
>  #else /* CONFIG_OF */
>  static int
>  mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
> -		  struct device_node *np)
> +		  struct device *dev)
>  {
>  	return -ENODEV;
>  }
> @@ -611,8 +619,9 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>  		drv_data->freq_n = pdata->freq_n;
>  		drv_data->irq = platform_get_irq(pd, 0);
>  		drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
> +		drv_data->regs = pdata->regs;
>  	} else if (pd->dev.of_node) {
> -		rc = mv64xxx_of_config(drv_data, pd->dev.of_node);
> +		rc = mv64xxx_of_config(drv_data, &pd->dev);
>  		if (rc)
>  			goto exit_clk;
>  	}
> @@ -680,12 +689,6 @@ mv64xxx_i2c_remove(struct platform_device *dev)
>  	return 0;
>  }
>  
> -static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
> -	{ .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	= mv64xxx_i2c_remove,
> diff --git a/include/linux/mv643xx_i2c.h b/include/linux/mv643xx_i2c.h
> index 5db5152..9304c94 100644
> --- a/include/linux/mv643xx_i2c.h
> +++ b/include/linux/mv643xx_i2c.h
> @@ -12,11 +12,32 @@
>  
>  #define MV64XXX_I2C_CTLR_NAME	"mv64xxx_i2c"
>  
> +struct mv64xxx_i2c_regs {
> +	u8	addr;
> +	u8	ext_addr;
> +	u8	data;
> +	u8	control;
> +	u8	status;
> +	u8	clock;
> +	u8	soft_reset;
> +};
> +
> +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
> +	.addr		= 0x00,
> +	.ext_addr	= 0x10,
> +	.data		= 0x04,
> +	.control	= 0x08,
> +	.status		= 0x0c,
> +	.clock		= 0x0c,
> +	.soft_reset	= 0x1c,
> +};
> +
>  /* i2c Platform Device, Driver Data */
>  struct mv64xxx_i2c_pdata {
> -	u32	freq_m;
> -	u32	freq_n;
> -	u32	timeout;	/* In milliseconds */
> +	u32			freq_m;
> +	u32			freq_n;
> +	struct mv64xxx_i2c_regs *regs;
> +	u32			timeout;	/* In milliseconds */
>  };
>  
>  #endif /*_MV64XXX_I2C_H_*/
> -- 
> 1.8.3
> 

Re: [PATCHv4 2/9] i2c: mv64xxx: make the registers offset configurable

[Maxime Ripard]

Hi Andrew,

On Wed, Jun 12, 2013 at 12:54:31PM +0200, Andrew Lunn wrote:
> On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote:
> > The Allwinner i2c controller uses the same logic as the Marvell one, but
> > with slightly different register offsets.
> > 
> > Introduce a structure that will be passed by either the pdata or
> > associated to the compatible strings, and that holds the various
> > registers that might be needed.
> 
> Hi Maxime
> 
> I don't think this change is bisectable. It assumes the platform data
> passes the registers, which is not true until the next patch.
> 
> Please can you re-arrange the order. Add the extra information to the
> platform data, and then make use of it, not the other way around.

Thanks to Tomasz comments, the next patch will be removed, and the whole
regs passing in the pdata as well, so it will become bisectable.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv4 2/9] i2c: mv64xxx: make the registers offset configurable

[Russell King - ARM Linux]

On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote:
> The Allwinner i2c controller uses the same logic as the Marvell one, but
> with slightly different register offsets.
> 
> Introduce a structure that will be passed by either the pdata or
> associated to the compatible strings, and that holds the various
> registers that might be needed.

I don't like this change.  It introduces further indirection where it's
not really necessary, and it's also using platform data to specify this
which is in the opposite direction to what's required for moving towards
DT.

> @@ -154,13 +147,13 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
>  static void
>  mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
>  {
> -	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SOFT_RESET);
> +	writel(0, drv_data->reg_base + drv_data->regs->soft_reset);

It'd be much better to copy the offsets themselves in drv_data.  You're
only talking about 7 bytes here, so there's no worry about bloating the
drv_data structure.

> @@ -611,8 +619,9 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>  		drv_data->freq_n = pdata->freq_n;
>  		drv_data->irq = platform_get_irq(pd, 0);
>  		drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
> +		drv_data->regs = pdata->regs;
>  	} else if (pd->dev.of_node) {
> -		rc = mv64xxx_of_config(drv_data, pd->dev.of_node);
> +		rc = mv64xxx_of_config(drv_data, &pd->dev);

I'd suggest making the default register offsets be the drivers existing
offsets, and allowing it to be overriden.  That nicely sorts out the
next comment below, and also gets rid of it in platform data.  Moreover,
if you're going to re-use this driver, you should do it via a different
"compatible" name in DT, which the driver can then use to identify the
different register set layout.

> +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
> +	.addr		= 0x00,
> +	.ext_addr	= 0x10,
> +	.data		= 0x04,
> +	.control	= 0x08,
> +	.status		= 0x0c,
> +	.clock		= 0x0c,
> +	.soft_reset	= 0x1c,
> +};

No, this means every file which includes this header ends up defining
this structure, which is globally visiable, and therefore its a recipe
for link failures.

Re: [PATCHv4 2/9] i2c: mv64xxx: make the registers offset configurable

[Maxime Ripard]

Hi Russel,

On Wed, Jun 12, 2013 at 02:57:35PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote:
> > The Allwinner i2c controller uses the same logic as the Marvell one, but
> > with slightly different register offsets.
> > 
> > Introduce a structure that will be passed by either the pdata or
> > associated to the compatible strings, and that holds the various
> > registers that might be needed.
> 
> I don't like this change.  It introduces further indirection where it's
> not really necessary, and it's also using platform data to specify this
> which is in the opposite direction to what's required for moving towards
> DT.

Well, some users of this aren't converted to DT, hence why I made the
changes to the platform_data.

> > @@ -154,13 +147,13 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
> >  static void
> >  mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
> >  {
> > -	writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SOFT_RESET);
> > +	writel(0, drv_data->reg_base + drv_data->regs->soft_reset);
> 
> It'd be much better to copy the offsets themselves in drv_data.  You're
> only talking about 7 bytes here, so there's no worry about bloating the
> drv_data structure.

It was more about keeping things separated. Moreover, the probe
function gets smaller, since you have only a pointer to pass on, instead
of assigning those 7 bytes.

And since Gregory Clement is working on adding more registers, I believe
it makes more sense to have things separate.

> 
> > @@ -611,8 +619,9 @@ mv64xxx_i2c_probe(struct platform_device *pd)
> >  		drv_data->freq_n = pdata->freq_n;
> >  		drv_data->irq = platform_get_irq(pd, 0);
> >  		drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
> > +		drv_data->regs = pdata->regs;
> >  	} else if (pd->dev.of_node) {
> > -		rc = mv64xxx_of_config(drv_data, pd->dev.of_node);
> > +		rc = mv64xxx_of_config(drv_data, &pd->dev);
> 
> I'd suggest making the default register offsets be the drivers existing
> offsets, and allowing it to be overriden.  That nicely sorts out the
> next comment below, and also gets rid of it in platform data.  Moreover,
> if you're going to re-use this driver, you should do it via a different
> "compatible" name in DT, which the driver can then use to identify the
> different register set layout.

The logic here will change quite a bit in the next iteration thanks to
the comments I received.

I'm now using a platform_device_id structure to match the name of the
driver just like what was done with the DT in that patchset. This also
removes the need to add the regs field to the platform data and ...

> 
> > +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
> > +	.addr		= 0x00,
> > +	.ext_addr	= 0x10,
> > +	.data		= 0x04,
> > +	.control	= 0x08,
> > +	.status		= 0x0c,
> > +	.clock		= 0x0c,
> > +	.soft_reset	= 0x1c,
> > +};
> 
> No, this means every file which includes this header ends up defining
> this structure, which is globally visiable, and therefore its a recipe
> for link failures.

solves this as well.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv4 2/9] i2c: mv64xxx: make the registers offset configurable

[Russell King - ARM Linux]

On Wed, Jun 12, 2013 at 04:44:47PM +0200, Maxime Ripard wrote:
> Hi Russel,
> 
> On Wed, Jun 12, 2013 at 02:57:35PM +0100, Russell King - ARM Linux wrote:
> > It'd be much better to copy the offsets themselves in drv_data.  You're
> > only talking about 7 bytes here, so there's no worry about bloating the
> > drv_data structure.
> 
> It was more about keeping things separated. Moreover, the probe
> function gets smaller, since you have only a pointer to pass on, instead
> of assigning those 7 bytes.

	struct driver_data {
		struct mv64xxx_i2c_regs reg_offsets;
	};

	struct driver_data *drv_data;

	memcpy(drv_data->reg_offsets, reg_offsets, sizeof(drv_data->reg_offsets));

No need to write it each member as a separate assignment.

Re: [PATCHv4 2/9] i2c: mv64xxx: make the registers offset configurable

[Maxime Ripard]

On Wed, Jun 12, 2013 at 03:51:39PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 12, 2013 at 04:44:47PM +0200, Maxime Ripard wrote:
> > Hi Russel,
> > 
> > On Wed, Jun 12, 2013 at 02:57:35PM +0100, Russell King - ARM Linux wrote:
> > > It'd be much better to copy the offsets themselves in drv_data.  You're
> > > only talking about 7 bytes here, so there's no worry about bloating the
> > > drv_data structure.
> > 
> > It was more about keeping things separated. Moreover, the probe
> > function gets smaller, since you have only a pointer to pass on, instead
> > of assigning those 7 bytes.
> 
> 	struct driver_data {
> 		struct mv64xxx_i2c_regs reg_offsets;
> 	};
> 
> 	struct driver_data *drv_data;
> 
> 	memcpy(drv_data->reg_offsets, reg_offsets, sizeof(drv_data->reg_offsets));
> 
> No need to write it each member as a separate assignment.

Ah right. I previously understood that you wanted a single variable for
each register in the driver data structure.

I'll do like you suggested.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv4 2/9] i2c: mv64xxx: make the registers offset configurable

[Sebastian Hesselbarth]

On 06/12/13 16:44, Maxime Ripard wrote:
> Hi Russel,
>
> On Wed, Jun 12, 2013 at 02:57:35PM +0100, Russell King - ARM Linux wrote:
>> On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote:
>>> The Allwinner i2c controller uses the same logic as the Marvell one, but
>>> with slightly different register offsets.
>>>
>>> Introduce a structure that will be passed by either the pdata or
>>> associated to the compatible strings, and that holds the various
>>> registers that might be needed.
>>
>> I don't like this change.  It introduces further indirection where it's
>> not really necessary, and it's also using platform data to specify this
>> which is in the opposite direction to what's required for moving towards
>> DT.
>
> Well, some users of this aren't converted to DT, hence why I made the
> changes to the platform_data.

Actually, this is not quite true. Yes of course, there are still users
of non-DT Marvell SoCs and it is still in the progress of full-DT. But
also ppc is using DT, except that they parse it and put in in
platform_data. Reasonable since back then, there was no global DT API
available.

IMHO for the time in between (i.e. now) check for pdev->dev.of_node
and !pdev->dev.platform_data will allow you to distinguish all users
perfectly:

- non-DT has platform_data set only
- ppc DT has of_node and platform_data set
- pure DT has of_node set only

This will allow you to limit your register offset modifications to
Allwinner exclusively and for pure DT (if that is what you want for
Allwinner).

Checkout mv643xx_eth in net-next where the above discrimination
strategy was chosen.

[...]
>> I'd suggest making the default register offsets be the drivers existing
>> offsets, and allowing it to be overriden.  That nicely sorts out the
>> next comment below, and also gets rid of it in platform data.  Moreover,
>> if you're going to re-use this driver, you should do it via a different
>> "compatible" name in DT, which the driver can then use to identify the
>> different register set layout.
>
> The logic here will change quite a bit in the next iteration thanks to
> the comments I received.
>
> I'm now using a platform_device_id structure to match the name of the
> driver just like what was done with the DT in that patchset. This also
> removes the need to add the regs field to the platform data and ...

Also here, if Allwinner is pure DT, you can call some
mv643xx_i2c_of_probe() for pure DT only with the above discrimination.

Sebastian

Re: [PATCHv4 2/9] i2c: mv64xxx: make the registers offset configurable

[Maxime Ripard]

Hi Sebastian,

On Wed, Jun 12, 2013 at 05:03:12PM +0200, Sebastian Hesselbarth wrote:
> On 06/12/13 16:44, Maxime Ripard wrote:
> >Hi Russel,
> >
> >On Wed, Jun 12, 2013 at 02:57:35PM +0100, Russell King - ARM Linux wrote:
> >>On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote:
> >>>The Allwinner i2c controller uses the same logic as the Marvell one, but
> >>>with slightly different register offsets.
> >>>
> >>>Introduce a structure that will be passed by either the pdata or
> >>>associated to the compatible strings, and that holds the various
> >>>registers that might be needed.
> >>
> >>I don't like this change.  It introduces further indirection where it's
> >>not really necessary, and it's also using platform data to specify this
> >>which is in the opposite direction to what's required for moving towards
> >>DT.
> >
> >Well, some users of this aren't converted to DT, hence why I made the
> >changes to the platform_data.
> 
> Actually, this is not quite true. Yes of course, there are still users
> of non-DT Marvell SoCs and it is still in the progress of full-DT. But
> also ppc is using DT, except that they parse it and put in in
> platform_data. Reasonable since back then, there was no global DT API
> available.

Ah, I see, thanks for the insight. I was here referring more
specifically to Orion that seems to be still stuck with !DT at the
moment, at least partially.

> IMHO for the time in between (i.e. now) check for pdev->dev.of_node
> and !pdev->dev.platform_data will allow you to distinguish all users
> perfectly:
> 
> - non-DT has platform_data set only
> - ppc DT has of_node and platform_data set
> - pure DT has of_node set only
> 
> This will allow you to limit your register offset modifications to
> Allwinner exclusively and for pure DT (if that is what you want for
> Allwinner).
> 
> Checkout mv643xx_eth in net-next where the above discrimination
> strategy was chosen.
> 
> [...]
> >>I'd suggest making the default register offsets be the drivers existing
> >>offsets, and allowing it to be overriden.  That nicely sorts out the
> >>next comment below, and also gets rid of it in platform data.  Moreover,
> >>if you're going to re-use this driver, you should do it via a different
> >>"compatible" name in DT, which the driver can then use to identify the
> >>different register set layout.
> >
> >The logic here will change quite a bit in the next iteration thanks to
> >the comments I received.
> >
> >I'm now using a platform_device_id structure to match the name of the
> >driver just like what was done with the DT in that patchset. This also
> >removes the need to add the regs field to the platform data and ...
> 
> Also here, if Allwinner is pure DT, you can call some
> mv643xx_i2c_of_probe() for pure DT only with the above discrimination.

Unless I'm missing something, isn't it what's already in place here?

We have:

if (pdata) {
    /* Fill in the driver data structure from pdata */
} else if (pd->dev.of_node) {
    /* Fill in the driver data structure from dt */
} else {
    return -EFAIL;
}

I guess that should cover all the cases you mentionned, even the PPC
one, right?

Now, the question about what content do we find in these platform_data
is actually a different one. This patch passed the regs offset as a
member of those. We all agreed that it was not the most elegant solution
(and like you mentionned, I will never use this pdata structure anyway
for the Allwinner stuff).

I guess we could just take the marvell offsets when using pdata, and use
different register offsets based on the compatibles when loading from
dt?

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCHv4 3/9] ARM: orion: pass the i2c registers definition through the platform data

[Maxime Ripard]

Convert the existing platform data users of the MV64XXX i2c driver to
pass the registers offset structure along with the platform data.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/plat-orion/common.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
index c019b7a..c166fc9 100644
--- a/arch/arm/plat-orion/common.c
+++ b/arch/arm/plat-orion/common.c
@@ -509,6 +509,7 @@ void __init orion_ge00_switch_init(struct dsa_platform_data *d, int irq)
  ****************************************************************************/
 static struct mv64xxx_i2c_pdata orion_i2c_pdata = {
 	.freq_n		= 3,
+	.regs		= &mv64xxx_i2c_regs_mv64xxx,
 	.timeout	= 1000, /* Default timeout of 1 second */
 };
 
@@ -524,6 +525,7 @@ static struct platform_device orion_i2c = {
 
 static struct mv64xxx_i2c_pdata orion_i2c_1_pdata = {
 	.freq_n		= 3,
+	.regs		= &mv64xxx_i2c_regs_mv64xxx,
 	.timeout	= 1000, /* Default timeout of 1 second */
 };
 
-- 
1.8.3

Re: [PATCHv4 3/9] ARM: orion: pass the i2c registers definition through the platform data

[Tomasz Figa]

Hi Maxime,

On Wednesday 12 of June 2013 10:07:12 Maxime Ripard wrote:
> Convert the existing platform data users of the MV64XXX i2c driver to
> pass the registers offset structure along with the platform data.

I'm not really convinced that platform data is the right way to pass such 
data.

IMHO driver/match data were supposed to contain variant-specific 
parameters, which the driver would receive based on matching platform 
device name (in non-DT case) or compatible string (in DT case).

Best regards,
Tomasz

> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/plat-orion/common.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
> index c019b7a..c166fc9 100644
> --- a/arch/arm/plat-orion/common.c
> +++ b/arch/arm/plat-orion/common.c
> @@ -509,6 +509,7 @@ void __init orion_ge00_switch_init(struct
> dsa_platform_data *d, int irq)
> ***********************************************************************
> *****/ static struct mv64xxx_i2c_pdata orion_i2c_pdata = {
>  	.freq_n		= 3,
> +	.regs		= &mv64xxx_i2c_regs_mv64xxx,
>  	.timeout	= 1000, /* Default timeout of 1 second */
>  };
> 
> @@ -524,6 +525,7 @@ static struct platform_device orion_i2c = {
> 
>  static struct mv64xxx_i2c_pdata orion_i2c_1_pdata = {
>  	.freq_n		= 3,
> +	.regs		= &mv64xxx_i2c_regs_mv64xxx,
>  	.timeout	= 1000, /* Default timeout of 1 second */
>  };

[PATCHv4 4/9] i2c: mv64xxx: Add Allwinner sun4i compatible

[Maxime Ripard]

Add the compatible string for the Allwinner A10 i2c controller and the
associated register layout.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/i2c/busses/Kconfig       |  3 ++-
 drivers/i2c/busses/i2c-mv64xxx.c |  1 +
 include/linux/mv643xx_i2c.h      | 10 ++++++++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 631736e..5dc4148 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -507,10 +507,11 @@ config I2C_MPC
 
 config I2C_MV64XXX
 	tristate "Marvell mv64xxx I2C Controller"
-	depends on (MV64X60 || PLAT_ORION)
+	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
 	help
 	  If you say yes to this option, support will be included for the
 	  built-in I2C interface on the Marvell 64xxx line of host bridges.
+	  This driver is also used for Allwinner SoCs I2C controllers.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mv64xxx.
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index f9e076e..febf5ba 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -490,6 +490,7 @@ static const struct i2c_algorithm mv64xxx_i2c_algo = {
  */
 #ifdef CONFIG_OF
 static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
+	{ .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
 	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
 	{}
 };
diff --git a/include/linux/mv643xx_i2c.h b/include/linux/mv643xx_i2c.h
index 9304c94..737108e 100644
--- a/include/linux/mv643xx_i2c.h
+++ b/include/linux/mv643xx_i2c.h
@@ -32,6 +32,16 @@ struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
 	.soft_reset	= 0x1c,
 };
 
+struct mv64xxx_i2c_regs mv64xxx_i2c_regs_sun4i = {
+	.addr		= 0x00,
+	.ext_addr	= 0x04,
+	.data		= 0x08,
+	.control	= 0x0c,
+	.status		= 0x10,
+	.clock		= 0x14,
+	.soft_reset	= 0x18,
+};
+
 /* i2c Platform Device, Driver Data */
 struct mv64xxx_i2c_pdata {
 	u32			freq_m;
-- 
1.8.3

Re: [PATCHv4 4/9] i2c: mv64xxx: Add Allwinner sun4i compatible

[Tomasz Figa]

Hi Maxime,

On Wednesday 12 of June 2013 10:07:13 Maxime Ripard wrote:
> Add the compatible string for the Allwinner A10 i2c controller and the
> associated register layout.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/i2c/busses/Kconfig       |  3 ++-
>  drivers/i2c/busses/i2c-mv64xxx.c |  1 +
>  include/linux/mv643xx_i2c.h      | 10 ++++++++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 631736e..5dc4148 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -507,10 +507,11 @@ config I2C_MPC
> 
>  config I2C_MV64XXX
>  	tristate "Marvell mv64xxx I2C Controller"
> -	depends on (MV64X60 || PLAT_ORION)
> +	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
>  	help
>  	  If you say yes to this option, support will be included for the
>  	  built-in I2C interface on the Marvell 64xxx line of host 
bridges.
> +	  This driver is also used for Allwinner SoCs I2C controllers.
> 
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mv64xxx.
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c
> b/drivers/i2c/busses/i2c-mv64xxx.c index f9e076e..febf5ba 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -490,6 +490,7 @@ static const struct i2c_algorithm mv64xxx_i2c_algo =
> { */
>  #ifdef CONFIG_OF
>  static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
> +	{ .compatible = "allwinner,sun4i-i2c", .data =
> &mv64xxx_i2c_regs_sun4i}, { .compatible = "marvell,mv64xxx-i2c", .data
> = &mv64xxx_i2c_regs_mv64xxx}, {}

Ah, OK, here you use compatible matching to get variant-specific data, 
fine. Still I think the same method (based on platform device name) should 
be used for non-DT variant.

>  };
> diff --git a/include/linux/mv643xx_i2c.h b/include/linux/mv643xx_i2c.h
> index 9304c94..737108e 100644
> --- a/include/linux/mv643xx_i2c.h
> +++ b/include/linux/mv643xx_i2c.h
> @@ -32,6 +32,16 @@ struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>  	.soft_reset	= 0x1c,
>  };
> 
> +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_sun4i = {
> +	.addr		= 0x00,
> +	.ext_addr	= 0x04,
> +	.data		= 0x08,
> +	.control	= 0x0c,
> +	.status		= 0x10,
> +	.clock		= 0x14,
> +	.soft_reset	= 0x18,
> +};
> +

Hmm, header doesn't look like a correct place for a structure definition, 
especially when the structure isn't even static.

Best regards,
Tomasz

>  /* i2c Platform Device, Driver Data */
>  struct mv64xxx_i2c_pdata {
>  	u32			freq_m;

Re: [PATCHv4 4/9] i2c: mv64xxx: Add Allwinner sun4i compatible

[Maxime Ripard]

Hi Tomasz,

On Wed, Jun 12, 2013 at 10:42:31AM +0200, Tomasz Figa wrote:
> Hi Maxime,
> 
> On Wednesday 12 of June 2013 10:07:13 Maxime Ripard wrote:
> > Add the compatible string for the Allwinner A10 i2c controller and the
> > associated register layout.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  drivers/i2c/busses/Kconfig       |  3 ++-
> >  drivers/i2c/busses/i2c-mv64xxx.c |  1 +
> >  include/linux/mv643xx_i2c.h      | 10 ++++++++++
> >  3 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 631736e..5dc4148 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -507,10 +507,11 @@ config I2C_MPC
> > 
> >  config I2C_MV64XXX
> >  	tristate "Marvell mv64xxx I2C Controller"
> > -	depends on (MV64X60 || PLAT_ORION)
> > +	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
> >  	help
> >  	  If you say yes to this option, support will be included for the
> >  	  built-in I2C interface on the Marvell 64xxx line of host 
> bridges.
> > +	  This driver is also used for Allwinner SoCs I2C controllers.
> > 
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called i2c-mv64xxx.
> > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c
> > b/drivers/i2c/busses/i2c-mv64xxx.c index f9e076e..febf5ba 100644
> > --- a/drivers/i2c/busses/i2c-mv64xxx.c
> > +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> > @@ -490,6 +490,7 @@ static const struct i2c_algorithm mv64xxx_i2c_algo =
> > { */
> >  #ifdef CONFIG_OF
> >  static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
> > +	{ .compatible = "allwinner,sun4i-i2c", .data =
> > &mv64xxx_i2c_regs_sun4i}, { .compatible = "marvell,mv64xxx-i2c", .data
> > = &mv64xxx_i2c_regs_mv64xxx}, {}
> 
> Ah, OK, here you use compatible matching to get variant-specific data, 
> fine. Still I think the same method (based on platform device name) should 
> be used for non-DT variant.

Yes, you're right. I wasn't aware that you could do something similar
with the platform device name, hence why I used the platform data.

I'll fix this in the next iteration.

> >  };
> > diff --git a/include/linux/mv643xx_i2c.h b/include/linux/mv643xx_i2c.h
> > index 9304c94..737108e 100644
> > --- a/include/linux/mv643xx_i2c.h
> > +++ b/include/linux/mv643xx_i2c.h
> > @@ -32,6 +32,16 @@ struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
> >  	.soft_reset	= 0x1c,
> >  };
> > 
> > +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_sun4i = {
> > +	.addr		= 0x00,
> > +	.ext_addr	= 0x04,
> > +	.data		= 0x08,
> > +	.control	= 0x0c,
> > +	.status		= 0x10,
> > +	.clock		= 0x14,
> > +	.soft_reset	= 0x18,
> > +};
> > +
> 
> Hmm, header doesn't look like a correct place for a structure definition, 
> especially when the structure isn't even static.

I put it there because it had to be used by the board files to fill
their plateform_data. Obviously, it will be moved back into the driver.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv4 4/9] i2c: mv64xxx: Add Allwinner sun4i compatible

[Andrew Lunn]

On Wed, Jun 12, 2013 at 10:07:13AM +0200, Maxime Ripard wrote:
> Add the compatible string for the Allwinner A10 i2c controller and the
> associated register layout.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/i2c/busses/Kconfig       |  3 ++-
>  drivers/i2c/busses/i2c-mv64xxx.c |  1 +
>  include/linux/mv643xx_i2c.h      | 10 ++++++++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 631736e..5dc4148 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -507,10 +507,11 @@ config I2C_MPC
>  
>  config I2C_MV64XXX
>  	tristate "Marvell mv64xxx I2C Controller"
> -	depends on (MV64X60 || PLAT_ORION)
> +	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)

Hi Maxime.

What about MV64X60? It also needs to pass the registers in its
platform data.

	 Andrew

Re: [PATCHv4 4/9] i2c: mv64xxx: Add Allwinner sun4i compatible

[Maxime Ripard]

On Wed, Jun 12, 2013 at 12:56:13PM +0200, Andrew Lunn wrote:
> On Wed, Jun 12, 2013 at 10:07:13AM +0200, Maxime Ripard wrote:
> > Add the compatible string for the Allwinner A10 i2c controller and the
> > associated register layout.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  drivers/i2c/busses/Kconfig       |  3 ++-
> >  drivers/i2c/busses/i2c-mv64xxx.c |  1 +
> >  include/linux/mv643xx_i2c.h      | 10 ++++++++++
> >  3 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 631736e..5dc4148 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -507,10 +507,11 @@ config I2C_MPC
> >  
> >  config I2C_MV64XXX
> >  	tristate "Marvell mv64xxx I2C Controller"
> > -	depends on (MV64X60 || PLAT_ORION)
> > +	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
> 
> Hi Maxime.
> 
> What about MV64X60? It also needs to pass the registers in its
> platform data.

I grep'ed for the users mv64xxx_i2c_pdata in arch/arm, and the
plat-orion/common.c was the only user. Obviously, I forgot to think
about the PPC architecture.

Anyway, this patch will be dropped.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCHv4 5/9] ARM: sunxi: dt: Add i2c controller nodes to the DTSI

[Maxime Ripard]

The Allwinner A10 and A13 both have 3 i2c controller embedded.
Add those to the common sunxi dtsi.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/sun4i-a10.dtsi | 27 +++++++++++++++++++++++++++
 arch/arm/boot/dts/sun5i-a13.dtsi | 27 +++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 04ff62a..f7e4a96 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -321,5 +321,32 @@
 			clocks = <&apb1_gates 23>;
 			status = "disabled";
 		};
+
+		i2c0: i2c@01c2ac00 {
+			compatible = "allwinner,sun4i-i2c";
+			reg = <0x01c2ac00 0x400>;
+			interrupts = <7>;
+			clocks = <&apb1_gates 0>;
+			clock-frequency = <100000>;
+			status = "disabled";
+		};
+
+		i2c1: i2c@01c2b000 {
+			compatible = "allwinner,sun4i-i2c";
+			reg = <0x01c2b000 0x400>;
+			interrupts = <8>;
+			clocks = <&apb1_gates 1>;
+			clock-frequency = <100000>;
+			status = "disabled";
+		};
+
+		i2c2: i2c@01c2b400 {
+			compatible = "allwinner,sun4i-i2c";
+			reg = <0x01c2b400 0x400>;
+			interrupts = <9>;
+			clocks = <&apb1_gates 2>;
+			clock-frequency = <100000>;
+			status = "disabled";
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index f34db19..31ebfd7 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -217,5 +217,32 @@
 			clocks = <&apb1_gates 19>;
 			status = "disabled";
 		};
+
+		i2c0: i2c@01c2ac00 {
+			compatible = "allwinner,sun4i-i2c";
+			reg = <0x01c2ac00 0x400>;
+			interrupts = <7>;
+			clocks = <&apb1_gates 0>;
+			clock-frequency = <100000>;
+			status = "disabled";
+		};
+
+		i2c1: i2c@01c2b000 {
+			compatible = "allwinner,sun4i-i2c";
+			reg = <0x01c2b000 0x400>;
+			interrupts = <8>;
+			clocks = <&apb1_gates 1>;
+			clock-frequency = <100000>;
+			status = "disabled";
+		};
+
+		i2c2: i2c@01c2b400 {
+			compatible = "allwinner,sun4i-i2c";
+			reg = <0x01c2b400 0x400>;
+			interrupts = <9>;
+			clocks = <&apb1_gates 2>;
+			clock-frequency = <100000>;
+			status = "disabled";
+		};
 	};
 };
-- 
1.8.3

[PATCHv4 6/9] ARM: sun4i: dt: Add i2c muxing options

[Maxime Ripard]

The i2c controller found on the Allwinner A10 has only one muxing option
available for each controller. Add them to the dtsi

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/sun4i-a10.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index f7e4a96..c2adc74 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -227,6 +227,26 @@
 				allwinner,function = "emac";
 				allwinner,drive = <0>;
 				allwinner,pull = <0>;
+
+			i2c0_pins_a: i2c0@0 {
+				allwinner,pins = "PB0", "PB1";
+				allwinner,function = "i2c0";
+				allwinner,drive = <0>;
+				allwinner,pull = <1>;
+			};
+
+			i2c1_pins_a: i2c1@0 {
+				allwinner,pins = "PB18", "PB19";
+				allwinner,function = "i2c1";
+				allwinner,drive = <0>;
+				allwinner,pull = <1>;
+			};
+
+			i2c2_pins_a: i2c2@0 {
+				allwinner,pins = "PB20", "PB21";
+				allwinner,function = "i2c2";
+				allwinner,drive = <0>;
+				allwinner,pull = <1>;
 			};
 		};
 
-- 
1.8.3

[PATCHv4 7/9] ARM: sun5i: dt: Add i2c muxing options

[Maxime Ripard]

The i2c controller found on the Allwinner A13 has only one muxing option
available for each controller. Add them to the dtsi

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/sun5i-a13.dtsi | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index 31ebfd7..bca0af3 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -184,6 +184,27 @@
 				allwinner,drive = <0>;
 				allwinner,pull = <0>;
 			};
+
+			i2c0_pins_a: i2c0@0 {
+				allwinner,pins = "PB0", "PB1";
+				allwinner,function = "i2c0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			i2c1_pins_a: i2c1@0 {
+				allwinner,pins = "PB15", "PB16";
+				allwinner,function = "i2c1";
+				allwinner,drive = <0>;
+				allwinner,pull = <1>;
+			};
+
+			i2c2_pins_a: i2c2@0 {
+				allwinner,pins = "PB17", "PB18";
+				allwinner,function = "i2c2";
+				allwinner,drive = <0>;
+				allwinner,pull = <1>;
+			};
 		};
 
 		timer@01c20c00 {
-- 
1.8.3

Re: [PATCHv4 7/9] ARM: sun5i: dt: Add i2c muxing options

[Henrik Nordström]

ons 2013-06-12 klockan 10:07 +0200 skrev Maxime Ripard:

> +			i2c0_pins_a: i2c0@0 {
> +				allwinner,pins = "PB0", "PB1";
> +				allwinner,function = "i2c0";
> +				allwinner,drive = <0>;
> +				allwinner,pull = <0>;

Is this right? All other use pull = 1.

I guess it's board dependent depending on if there is external pullups
or not on the i2c bus. I have seen Allwinner designs both with and
without external pullups.

It will not work well if there is neither internal or external pullup.
And may stress the i2c components a bit if there is both..

Regards
Henrik

Re: [PATCHv4 7/9] ARM: sun5i: dt: Add i2c muxing options

[Maxime Ripard]

Hi Henrik,

On Wed, Jun 12, 2013 at 10:29:48AM +0200, Henrik Nordström wrote:
> ons 2013-06-12 klockan 10:07 +0200 skrev Maxime Ripard:
> 
> > +			i2c0_pins_a: i2c0@0 {
> > +				allwinner,pins = "PB0", "PB1";
> > +				allwinner,function = "i2c0";
> > +				allwinner,drive = <0>;
> > +				allwinner,pull = <0>;
> 
> Is this right? All other use pull = 1.

Ah, true.

> I guess it's board dependent depending on if there is external pullups
> or not on the i2c bus. I have seen Allwinner designs both with and
> without external pullups.
> 
> It will not work well if there is neither internal or external pullup.
> And may stress the i2c components a bit if there is both..

Yes, but it's a default, so by definition, it won't suit everyone. Yet,
it has to be there. And every board is free to override that default in
its own DT anyway, so it's not really a big issue. But like you pointed
at, we have to be consistent.

I'll disable the pull-ups in the next iteration for every node.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/groups/opt_out.

[PATCHv4 8/9] ARM: sun5i: olinuxino: Enable the i2c controllers

[Maxime Ripard]

The A13-Olinuxino makes use of the 3 i2c controllers found on the Allwinner
A13. Enable them in the device tree.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/sun5i-a13-olinuxino.dts | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/sun5i-a13-olinuxino.dts b/arch/arm/boot/dts/sun5i-a13-olinuxino.dts
index 3ca5506..80497e3 100644
--- a/arch/arm/boot/dts/sun5i-a13-olinuxino.dts
+++ b/arch/arm/boot/dts/sun5i-a13-olinuxino.dts
@@ -37,6 +37,24 @@
 			pinctrl-0 = <&uart1_pins_b>;
 			status = "okay";
 		};
+
+		i2c0: i2c@01c2ac00 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2c0_pins_a>;
+			status = "okay";
+		};
+
+		i2c1: i2c@01c2b000 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2c1_pins_a>;
+			status = "okay";
+		};
+
+		i2c2: i2c@01c2b400 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2c2_pins_a>;
+			status = "okay";
+		};
 	};
 
 	leds {
-- 
1.8.3

[PATCHv4 9/9] ARM: sun4i: cubieboard: Enable the i2c controllers

[Maxime Ripard]

From: Emilio López <emilio-0Z03zUJReD5OxF6Tv1QG9Q@public.gmane.org>

The Cubieboard makes use of the first two i2c controllers found on the
Allwinner A10; i2c-0 is used internally for the PMIC, while i2c-1
is exposed on the board headers. This patch enables them in the device
tree.

Signed-off-by: Emilio López <emilio-0Z03zUJReD5OxF6Tv1QG9Q@public.gmane.org>
Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
index e752b57..757c4cd 100644
--- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
+++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
@@ -56,6 +56,18 @@
 			pinctrl-0 = <&uart0_pins_a>;
 			status = "okay";
 		};
+
+		i2c0: i2c@01c2ac00 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2c0_pins_a>;
+			status = "okay";
+		};
+
+		i2c1: i2c@01c2b000 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2c1_pins_a>;
+			status = "okay";
+		};
 	};
 
 	leds {
-- 
1.8.3

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCHv4 0/9] Add I2C support for Allwinner SoCs

[Arnd Bergmann]

On Wednesday 12 June 2013 10:07:09 Maxime Ripard wrote:
> 
> This patchset adds support for the I2C controller found on most of the
> Allwinner SoCs, especially the already supported A10 and A13, and the
> yet to come A31.
> 
> This driver leverages the Marvel mv64xxx i2c controller driver, that has
> an almost identical logic, with a slightly different register layout.
> 
> It has been tested on a A13-Olinuxino and an A10s-Olinuxino.

It doesn't really matter, but can someone clarify why this driver can
be reused? Did marvell and allwinner license the same hardware block
or is just a really simple driver that does things in an obvious way?

	Arnd

Re: [PATCHv4 0/9] Add I2C support for Allwinner SoCs

[Maxime Ripard]

Hi Arnd,

On Wed, Jun 12, 2013 at 01:26:56PM +0200, Arnd Bergmann wrote:
> On Wednesday 12 June 2013 10:07:09 Maxime Ripard wrote:
> > 
> > This patchset adds support for the I2C controller found on most of the
> > Allwinner SoCs, especially the already supported A10 and A13, and the
> > yet to come A31.
> > 
> > This driver leverages the Marvel mv64xxx i2c controller driver, that has
> > an almost identical logic, with a slightly different register layout.
> > 
> > It has been tested on a A13-Olinuxino and an A10s-Olinuxino.
> 
> It doesn't really matter, but can someone clarify why this driver can
> be reused? Did marvell and allwinner license the same hardware block
> or is just a really simple driver that does things in an obvious way?

The Marvell and Allwinner controllers share the exact same logic (which
is definitely not trivial), based on a finite state machine that
triggers interrupts at each change of state, each state being a state in
the I2C protocol (like address sent, data received with an ACK, etc.).

The weird thing is that the only difference between the two controllers
is the register offsets, and that's it. The state numbers, bit index,
etc, are exactly the same.

So yes, I think they both licensed the same IP.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv4 0/9] Add I2C support for Allwinner SoCs

[Arnd Bergmann]

On Wednesday 12 June 2013, Maxime Ripard wrote:
> The Marvell and Allwinner controllers share the exact same logic (which
> is definitely not trivial), based on a finite state machine that
> triggers interrupts at each change of state, each state being a state in
> the I2C protocol (like address sent, data received with an ACK, etc.).
> 
> The weird thing is that the only difference between the two controllers
> is the register offsets, and that's it. The state numbers, bit index,
> etc, are exactly the same.

Ok, cool. Great someone noticed!

> So yes, I think they both licensed the same IP.

I wonder if it's the Mentor Graphics Inventra mi2c block, which
would make sense given that Allwinner also uses musb.

	Arnd

Re: [PATCHv4 0/9] Add I2C support for Allwinner SoCs

[Maxime Ripard]

On Wed, Jun 12, 2013 at 02:38:12PM +0200, Arnd Bergmann wrote:
> On Wednesday 12 June 2013, Maxime Ripard wrote:
> > The Marvell and Allwinner controllers share the exact same logic (which
> > is definitely not trivial), based on a finite state machine that
> > triggers interrupts at each change of state, each state being a state in
> > the I2C protocol (like address sent, data received with an ACK, etc.).
> > 
> > The weird thing is that the only difference between the two controllers
> > is the register offsets, and that's it. The state numbers, bit index,
> > etc, are exactly the same.
> 
> Ok, cool. Great someone noticed!

Kudos to Wolfram :)

> > So yes, I think they both licensed the same IP.
> 
> I wonder if it's the Mentor Graphics Inventra mi2c block, which
> would make sense given that Allwinner also uses musb.

The only datasheet or manual I have been able to find is
http://www.mentor.com/products/ip/peripheral/ip_interface/upload/mi2c_pd.pdf
which doesn't give a lot of details. Yet, from what is shown and
explained in the second page, it looks like it could be this IP.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv4 0/9] Add I2C support for Allwinner SoCs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]


> > The weird thing is that the only difference between the two controllers
> > is the register offsets, and that's it. The state numbers, bit index,
> > etc, are exactly the same.
> 
> Ok, cool. Great someone noticed!

:) First thing I do when reviewing new drivers is a manual fuzzy search
for already known register sets.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]