[PATCH v3 0/4] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[PATCH v3 0/4] i2c-mv64xxx: Fixes and new feature for controlers embedded in Aramda XP

[Gregory CLEMENT]

Hello,

this patch set adds support for the I2C Transaction Generator which
offloads CPU from managing I2C transfer step by step. This feature is
currently only available on the I2C controller IP embedded in the
Armada XP SoC.

This series also contains a real fix for the I2C controller of the
Armada XP SoC and a patch closer to an improvement than a fix.

The first three patches modify the driver itself and should go through
i2c subsystem.

The last patch updates the device tree to be able to use this new
feature and should go through mvebu subsystem.

Due to the recent changes in i2c-mv64xxx.c file, I based these patches
on the for-next branch of the i2c git repository.

Thanks,

Change log:

v2->v3:

 - Introduces a new compatible string mv78230-i2c which will be used
   for the fix and for the offload feature which are only present on
   the Armada XP SoCs

 - Removes the unneeded spin_lock_irqsave pointed by Russell King

 - The offload mechanism is now port of the fsm and handle the
   multiple messages.

 - The flag bridge-enabled is renamed to offload_enabled, but the
   register name stills contains the BRIDGE word to match the
   datasheet.

 - Uses writel_relaxed on the place pointed by Russell King

 - Uses the bool type for the flag (pointed by Thomas Petazzoni)

 - Removes useless code (pointed by Thomas Petazzoni)

 - Updates the bindings documentation

v1->v2:
 - Move the flag for the timing issue from global scope to per device
   scope
 - Assignment is no more done in if condition


Gregory CLEMENT (4):
  i2c-mv64xxx: Set bus frequency to 100kHz if clock-frequency is not
    provided
  i2c-mv64xxx: Add I2C Transaction Generator support
  i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889)
  ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

 .../devicetree/bindings/i2c/i2c-mv64xxx.txt        |  19 +-
 arch/arm/boot/dts/armada-370-xp.dtsi               |   2 -
 arch/arm/boot/dts/armada-370.dtsi                  |   8 +
 arch/arm/boot/dts/armada-xp.dtsi                   |  10 +
 drivers/i2c/busses/i2c-mv64xxx.c                   | 222 ++++++++++++++++++++-
 5 files changed, 246 insertions(+), 15 deletions(-)

-- 
1.8.1.2

[PATCH v3 1/4] i2c-mv64xxx: Set bus frequency to 100kHz if clock-frequency is not provided

[Gregory CLEMENT]

This commit adds checking whether clock-frequency property acquisition
has succeeded. If not, the frequency is set to 100kHz by default.

The Device Tree binding documentation is updated accordingly.

Based on the intials patches from Zbigniew Bodek

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Signed-off-by: Zbigniew Bodek <zbb@semihalf.com>
---
 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 6 +++++-
 drivers/i2c/busses/i2c-mv64xxx.c                      | 6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index f46d928..a1ee681 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -6,7 +6,11 @@ Required properties :
  - reg             : Offset and length of the register set for the device
  - compatible      : Should be "marvell,mv64xxx-i2c"
  - interrupts      : The interrupt number
- - clock-frequency : Desired I2C bus clock frequency in Hz.
+
+Optional properties :
+
+ - clock-frequency : Desired I2C bus clock frequency in Hz. If not set the
+default frequency is 100kHz
 
 Examples:
 
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 7a0e39b..d5d46db 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -578,7 +578,11 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 		goto out;
 	}
 	tclk = clk_get_rate(drv_data->clk);
-	of_property_read_u32(np, "clock-frequency", &bus_freq);
+
+	rc = of_property_read_u32(np, "clock-frequency", &bus_freq);
+	if (rc)
+		bus_freq = 100000; /* 100kHz by default */
+
 	if (!mv64xxx_find_baud_factors(bus_freq, tclk,
 				       &drv_data->freq_n, &drv_data->freq_m)) {
 		rc = -EINVAL;
-- 
1.8.1.2

[PATCH v3 1/4] i2c-mv64xxx: Set bus frequency to 100kHz if clock-frequency is not provided

[Wolfram Sang]

On Fri, Jun 21, 2013 at 03:32:06PM +0200, Gregory CLEMENT wrote:
> This commit adds checking whether clock-frequency property acquisition
> has succeeded. If not, the frequency is set to 100kHz by default.
> 
> The Device Tree binding documentation is updated accordingly.
> 
> Based on the intials patches from Zbigniew Bodek
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Signed-off-by: Zbigniew Bodek <zbb@semihalf.com>

Applied to for-next, thanks! Rest of the series is 3.12 material.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130625/707c68b3/attachment.sig>

[PATCH v3 1/4] i2c-mv64xxx: Set bus frequency to 100kHz if clock-frequency is not provided

[Gregory CLEMENT]

On 06/25/2013 11:44 PM, Wolfram Sang wrote:
> On Fri, Jun 21, 2013 at 03:32:06PM +0200, Gregory CLEMENT wrote:
>> This commit adds checking whether clock-frequency property acquisition has succeeded. If not, the frequency is set to 100kHz by default.
>> 
>> The Device Tree binding documentation is updated accordingly.
>> 
>> Based on the intials patches from Zbigniew Bodek
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> Signed-off-by: Zbigniew Bodek <zbb@semihalf.com>
> 
> Applied to for-next, thanks! Rest of the series is 3.12 material.
> 
Thanks!

For the rest of the series, does it mean that you agree with it, or
that you didn't review it yet?

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3 1/4] i2c-mv64xxx: Set bus frequency to 100kHz if clock-frequency is not provided

[Wolfram Sang]

> For the rest of the series, does it mean that you agree with it, or
> that you didn't review it yet?

Didn't review. So, other people can still join in ;)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130626/19840cf2/attachment-0001.sig>

[PATCH v3 2/4] i2c-mv64xxx: Add I2C Transaction Generator support

[Gregory CLEMENT]

The I2C Transaction Generator offloads CPU from managing I2C
transfer step by step.

This feature is currently only available on Armada XP, so usage of
this mechanism is activated through device tree.

Based on the work of Piotr Ziecik and rewrote to use the new way of
handling multiples i2c messages.

Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 207 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 196 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index d5d46db..c07e742 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -55,6 +55,32 @@
 #define	MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK	0xe8
 #define	MV64XXX_I2C_STATUS_NO_STATUS			0xf8
 
+/* Register defines (I2C bridge) */
+#define	MV64XXX_I2C_REG_TX_DATA_LO			0xc0
+#define	MV64XXX_I2C_REG_TX_DATA_HI			0xc4
+#define	MV64XXX_I2C_REG_RX_DATA_LO			0xc8
+#define	MV64XXX_I2C_REG_RX_DATA_HI			0xcc
+#define	MV64XXX_I2C_REG_BRIDGE_CONTROL			0xd0
+#define	MV64XXX_I2C_REG_BRIDGE_STATUS			0xd4
+#define	MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE		0xd8
+#define	MV64XXX_I2C_REG_BRIDGE_INTR_MASK		0xdC
+#define	MV64XXX_I2C_REG_BRIDGE_TIMING			0xe0
+
+/* Bridge Control values */
+#define	MV64XXX_I2C_BRIDGE_CONTROL_WR			0x00000001
+#define	MV64XXX_I2C_BRIDGE_CONTROL_RD			0x00000002
+#define	MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT		2
+#define	MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT		0x00001000
+#define	MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT	13
+#define	MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT	16
+#define	MV64XXX_I2C_BRIDGE_CONTROL_ENABLE		0x00080000
+
+/* Bridge Status values */
+#define	MV64XXX_I2C_BRIDGE_STATUS_ERROR			0x00000001
+#define	MV64XXX_I2C_STATUS_OFFLOAD_ERROR		0xf0000001
+#define	MV64XXX_I2C_STATUS_OFFLOAD_OK			0xf0000000
+
+
 /* Driver states */
 enum {
 	MV64XXX_I2C_STATE_INVALID,
@@ -71,14 +97,17 @@ enum {
 enum {
 	MV64XXX_I2C_ACTION_INVALID,
 	MV64XXX_I2C_ACTION_CONTINUE,
+	MV64XXX_I2C_ACTION_OFFLOAD_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_START,
 	MV64XXX_I2C_ACTION_SEND_RESTART,
+	MV64XXX_I2C_ACTION_OFFLOAD_RESTART,
 	MV64XXX_I2C_ACTION_SEND_ADDR_1,
 	MV64XXX_I2C_ACTION_SEND_ADDR_2,
 	MV64XXX_I2C_ACTION_SEND_DATA,
 	MV64XXX_I2C_ACTION_RCV_DATA,
 	MV64XXX_I2C_ACTION_RCV_DATA_STOP,
 	MV64XXX_I2C_ACTION_SEND_STOP,
+	MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP,
 };
 
 struct mv64xxx_i2c_regs {
@@ -117,6 +146,7 @@ struct mv64xxx_i2c_data {
 	spinlock_t		lock;
 	struct i2c_msg		*msg;
 	struct i2c_adapter	adapter;
+	bool			offload_enabled;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -165,6 +195,79 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 	}
 }
 
+static int mv64xxx_i2c_offload_msg(struct mv64xxx_i2c_data *drv_data)
+{
+	unsigned long data_reg_hi = 0;
+	unsigned long data_reg_lo = 0;
+	unsigned long ctrl_reg;
+	unsigned int i;
+	struct i2c_msg *msg = drv_data->msgs;
+
+	drv_data->msg = msg;
+	drv_data->byte_posn = 0;
+	drv_data->bytes_left = msg->len;
+	drv_data->aborting = 0;
+	drv_data->rc = 0;
+	/* Only regular transactions can be offloaded */
+	if ((msg->flags & ~(I2C_M_TEN | I2C_M_RD)) != 0)
+		return 1;
+
+	/* Only 1-8 byte transfers can be offloaded */
+	if (msg->len < 1 || msg->len > 8)
+		return 1;
+
+	/* Build transaction */
+	ctrl_reg = MV64XXX_I2C_BRIDGE_CONTROL_ENABLE |
+		   (msg->addr << MV64XXX_I2C_BRIDGE_CONTROL_ADDR_SHIFT);
+
+	if ((msg->flags & I2C_M_TEN) != 0)
+		ctrl_reg |=  MV64XXX_I2C_BRIDGE_CONTROL_ADDR_EXT;
+
+	if ((msg->flags & I2C_M_RD) == 0) {
+		for (i = 0; i < 4 && i < msg->len; i++)
+			data_reg_lo = data_reg_lo |
+					(msg->buf[i] << ((i & 0x3) * 8));
+
+		for (i = 4; i < 8 && i < msg->len; i++)
+			data_reg_hi = data_reg_hi |
+					(msg->buf[i] << ((i & 0x3) * 8));
+
+		ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_WR |
+		    (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_TX_SIZE_SHIFT;
+	} else {
+		ctrl_reg |= MV64XXX_I2C_BRIDGE_CONTROL_RD |
+		    (msg->len - 1) << MV64XXX_I2C_BRIDGE_CONTROL_RX_SIZE_SHIFT;
+	}
+
+	/* Execute transaction */
+	writel_relaxed(data_reg_lo,
+		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_LO);
+	writel_relaxed(data_reg_hi,
+		drv_data->reg_base + MV64XXX_I2C_REG_TX_DATA_HI);
+	writel(ctrl_reg, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+
+	return 0;
+}
+
+static void
+mv64xxx_i2c_update_offload_data(struct i2c_msg *msg, unsigned long data_reg_hi,
+	unsigned long data_reg_lo)
+{
+	int i;
+
+	if ((msg->flags & I2C_M_RD) != 0) {
+		for (i = 0; i < 4 && i < msg->len; i++) {
+			msg->buf[i] = data_reg_lo & 0xFF;
+			data_reg_lo >>= 8;
+		}
+
+		for (i = 4; i < 8 && i < msg->len; i++) {
+			msg->buf[i] = data_reg_hi & 0xFF;
+			data_reg_hi >>= 8;
+		}
+	}
+
+}
 /*
  *****************************************************************************
  *
@@ -177,6 +280,15 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
 static void
 mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
 {
+	if (drv_data->offload_enabled) {
+		writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_CONTROL);
+		writel(0, drv_data->reg_base + MV64XXX_I2C_REG_BRIDGE_TIMING);
+		writel(0, drv_data->reg_base +
+			MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+		writel(0, drv_data->reg_base +
+			MV64XXX_I2C_REG_BRIDGE_INTR_MASK);
+	}
+
 	writel(0, drv_data->reg_base + drv_data->reg_offsets.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 + drv_data->reg_offsets.clock);
@@ -283,6 +395,16 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 		drv_data->rc = -ENODEV;
 		break;
 
+	case MV64XXX_I2C_STATUS_OFFLOAD_OK:
+		if (drv_data->send_stop || drv_data->aborting) {
+			drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP;
+			drv_data->state = MV64XXX_I2C_STATE_IDLE;
+		} else {
+			drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_RESTART;
+			drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_RESTART;
+		}
+		break;
+
 	default:
 		dev_err(&drv_data->adapter.dev,
 			"mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, "
@@ -298,21 +420,36 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
 static void
 mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 {
+	unsigned long data_reg_hi = 0;
+	unsigned long data_reg_lo = 0;
+
 	switch(drv_data->action) {
+	case MV64XXX_I2C_ACTION_OFFLOAD_RESTART:
+		data_reg_lo = readl(drv_data->reg_base +
+				MV64XXX_I2C_REG_RX_DATA_LO);
+		data_reg_hi = readl(drv_data->reg_base +
+				MV64XXX_I2C_REG_RX_DATA_HI);
+		mv64xxx_i2c_update_offload_data(drv_data->msg, data_reg_hi,
+						data_reg_lo);
+		writel(0, drv_data->reg_base +	MV64XXX_I2C_REG_BRIDGE_CONTROL);
+		writel(0, drv_data->reg_base +
+			MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_RESTART:
 		/* We should only get here if we have further messages */
 		BUG_ON(drv_data->num_msgs == 0);
 
-		drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
-		writel(drv_data->cntl_bits,
-			drv_data->reg_base + drv_data->reg_offsets.control);
-
 		drv_data->msgs++;
 		drv_data->num_msgs--;
+		if (!(drv_data->offload_enabled &&
+				mv64xxx_i2c_offload_msg(drv_data))) {
+			drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START;
+			writel(drv_data->cntl_bits,
+			drv_data->reg_base + drv_data->reg_offsets.control);
 
-		/* Setup for the next message */
-		mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
-
+			/* Setup for the next message */
+			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
+		}
 		/*
 		 * We're never at the start of the message here, and by this
 		 * time it's already too late to do any protocol mangling.
@@ -326,6 +463,12 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		break;
 
+	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_START:
+		if (mv64xxx_i2c_offload_msg(drv_data) <= 0)
+			break;
+		else
+			drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_START:
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START,
 			drv_data->reg_base + drv_data->reg_offsets.control);
@@ -375,6 +518,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			"mv64xxx_i2c_do_action: Invalid action: %d\n",
 			drv_data->action);
 		drv_data->rc = -EIO;
+
 		/* FALLTHRU */
 	case MV64XXX_I2C_ACTION_SEND_STOP:
 		drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
@@ -383,6 +527,20 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		drv_data->block = 0;
 		wake_up(&drv_data->waitq);
 		break;
+
+	case MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP:
+		data_reg_lo = readl(drv_data->reg_base +
+				MV64XXX_I2C_REG_RX_DATA_LO);
+		data_reg_hi = readl(drv_data->reg_base +
+				MV64XXX_I2C_REG_RX_DATA_HI);
+		mv64xxx_i2c_update_offload_data(drv_data->msg, data_reg_hi,
+						data_reg_lo);
+		writel(0, drv_data->reg_base +	MV64XXX_I2C_REG_BRIDGE_CONTROL);
+		writel(0, drv_data->reg_base +
+			MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE);
+		drv_data->block = 0;
+		wake_up(&drv_data->waitq);
+		break;
 	}
 }
 
@@ -395,6 +553,21 @@ mv64xxx_i2c_intr(int irq, void *dev_id)
 	irqreturn_t	rc = IRQ_NONE;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
+
+	if (drv_data->offload_enabled) {
+		while (readl(drv_data->reg_base +
+				MV64XXX_I2C_REG_BRIDGE_INTR_CAUSE)) {
+			int reg_status = readl(drv_data->reg_base +
+					MV64XXX_I2C_REG_BRIDGE_STATUS);
+			if (reg_status & MV64XXX_I2C_BRIDGE_STATUS_ERROR)
+				status = MV64XXX_I2C_STATUS_OFFLOAD_ERROR;
+			else
+				status = MV64XXX_I2C_STATUS_OFFLOAD_OK;
+			mv64xxx_i2c_fsm(drv_data, status);
+			mv64xxx_i2c_do_action(drv_data);
+			rc = IRQ_HANDLED;
+		}
+	}
 	while (readl(drv_data->reg_base + drv_data->reg_offsets.control) &
 						MV64XXX_I2C_REG_CONTROL_IFLG) {
 		status = readl(drv_data->reg_base + drv_data->reg_offsets.status);
@@ -459,11 +632,15 @@ mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg,
 	unsigned long	flags;
 
 	spin_lock_irqsave(&drv_data->lock, flags);
-	mv64xxx_i2c_prepare_for_io(drv_data, msg);
-
-	drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
-	drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+	if (drv_data->offload_enabled) {
+		drv_data->action = MV64XXX_I2C_ACTION_OFFLOAD_SEND_START;
+		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+	} else {
+		mv64xxx_i2c_prepare_for_io(drv_data, msg);
 
+		drv_data->action = MV64XXX_I2C_ACTION_SEND_START;
+		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND;
+	}
 	drv_data->send_stop = is_last;
 	drv_data->block = 1;
 	mv64xxx_i2c_do_action(drv_data);
@@ -601,6 +778,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 
 	memcpy(&drv_data->reg_offsets, device->data, sizeof(drv_data->reg_offsets));
 
+	/*
+	 * For controllers embedded in new SoCs activate the
+	 * Transaction Generator support.
+	 */
+	if (of_device_is_compatible(np, "marvell,mv78230-i2c"))
+		drv_data->offload_enabled = true;
+
 out:
 	return rc;
 #endif
@@ -654,6 +838,7 @@ 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->offload_enabled = 0;
 		memcpy(&drv_data->reg_offsets, &mv64xxx_i2c_regs_mv64xxx, sizeof(drv_data->reg_offsets));
 	} else if (pd->dev.of_node) {
 		rc = mv64xxx_of_config(drv_data, &pd->dev);
-- 
1.8.1.2

[PATCH v3 3/4] i2c-mv64xxx: Fix timing issue on Armada XP (errata FE-8471889)

[Gregory CLEMENT]

All the Armada XP (mv78230, mv78260 and mv78460) have a silicon issue
in the I2C controller which violate the i2c repeated start
timing. The I2C standard requires a minimum of 4.7us for the repeated
start condition whereas the I2C controller of the Armada XP this time
is 2.9us.

So this patch adds a 5us delay for the start case only if the
the compatible i2c-mv78230 is set.

Based on the initals patches from Zbigniew Bodek

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Signed-off-by: Zbigniew Bodek <zbb@semihalf.com>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index c07e742..e5b9c42 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -24,6 +24,7 @@
 #include <linux/of_i2c.h>
 #include <linux/clk.h>
 #include <linux/err.h>
+#include <linux/delay.h>
 
 #define MV64XXX_I2C_ADDR_ADDR(val)			((val & 0x7f) << 1)
 #define MV64XXX_I2C_BAUD_DIV_N(val)			(val & 0x7)
@@ -147,6 +148,8 @@ struct mv64xxx_i2c_data {
 	struct i2c_msg		*msg;
 	struct i2c_adapter	adapter;
 	bool			offload_enabled;
+/* 5us delay in order to avoid repeated start timing violation */
+	bool			errata_delay;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -450,6 +453,9 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 			/* Setup for the next message */
 			mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs);
 		}
+		if (drv_data->errata_delay)
+			udelay(5);
+
 		/*
 		 * We're never at the start of the message here, and by this
 		 * time it's already too late to do any protocol mangling.
@@ -509,6 +515,9 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
 		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
 			drv_data->reg_base + drv_data->reg_offsets.control);
 		drv_data->block = 0;
+		if (drv_data->errata_delay)
+			udelay(5);
+
 		wake_up(&drv_data->waitq);
 		break;
 
@@ -780,10 +789,12 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 
 	/*
 	 * For controllers embedded in new SoCs activate the
-	 * Transaction Generator support.
+	 * Transaction Generator support and the errata fix.
 	 */
-	if (of_device_is_compatible(np, "marvell,mv78230-i2c"))
+	if (of_device_is_compatible(np, "marvell,mv78230-i2c")) {
 		drv_data->offload_enabled = true;
+		drv_data->errata_delay = true;
+	}
 
 out:
 	return rc;
-- 
1.8.1.2

[PATCH v3 4/4] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Gregory CLEMENT]

The mv64xxx-i2c embedded in the Armada XP have a new feature to
offload i2c transaction. This new version of the IP come also with
some errata. This lead to the introduction to a another compatible
string.

This commit split the i2c information into armada-370.dtsi and
armada-xp.dtsi. Most of the data remains the same and stay in the
common file Armada-370-xp.dtsi. With this new feature the size of the
registers are bigger for Armada XP and the new compatible string is
used.

The Device Tree binding documentation is updated accordingly.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
 arch/arm/boot/dts/armada-370-xp.dtsi                  |  2 --
 arch/arm/boot/dts/armada-370.dtsi                     |  8 ++++++++
 arch/arm/boot/dts/armada-xp.dtsi                      | 10 ++++++++++
 4 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index a1ee681..ce7af6a 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -4,7 +4,8 @@
 Required properties :
 
  - reg             : Offset and length of the register set for the device
- - compatible      : Should be "marvell,mv64xxx-i2c"
+ - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
+for controller which support the I2C Transaction Generator
  - interrupts      : The interrupt number
 
 Optional properties :
@@ -20,3 +21,13 @@ Examples:
 		interrupts = <29>;
 		clock-frequency = <100000>;
 	};
+
+For a controller which support the I2C Transaction Generator:
+
+	i2c at 11000 {
+		compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
+		reg = <0x11000 0x100>;
+		compatible = "marvell,mv64xxx-i2c";
+		interrupts = <29>;
+		clock-frequency = <100000>;
+	};
diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 550eb77..b6f475c 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -112,7 +112,6 @@
 
 			i2c0: i2c at 11000 {
 				compatible = "marvell,mv64xxx-i2c";
-				reg = <0x11000 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				interrupts = <31>;
@@ -123,7 +122,6 @@
 
 			i2c1: i2c at 11100 {
 				compatible = "marvell,mv64xxx-i2c";
-				reg = <0x11100 0x20>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				interrupts = <32>;
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index aee2b18..39b26d6 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -155,6 +155,14 @@
 				};
 			};
 
+			i2c0: i2c at 11000 {
+				reg = <0x11000 0x20>;
+			};
+
+			i2c1: i2c at 11100 {
+				reg = <0x11100 0x20>;
+			};
+
 			usb at 50000 {
 				clocks = <&coreclk 0>;
 			};
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 5b902f9..c2c357e 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -134,6 +134,16 @@
 				};
 			};
 
+			i2c0: i2c at 11000 {
+				compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
+				reg = <0x11000 0x100>;
+			};
+
+			i2c1: i2c at 11100 {
+				compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
+				reg = <0x11100 0x100>;
+			};
+
 			usb at 50000 {
 				clocks = <&gateclk 18>;
 			};
-- 
1.8.1.2

[PATCH v3 4/4] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Jason Cooper]

On Fri, Jun 21, 2013 at 03:32:09PM +0200, Gregory CLEMENT wrote:
> The mv64xxx-i2c embedded in the Armada XP have a new feature to
> offload i2c transaction. This new version of the IP come also with
> some errata. This lead to the introduction to a another compatible
> string.
> 
> This commit split the i2c information into armada-370.dtsi and
> armada-xp.dtsi. Most of the data remains the same and stay in the
> common file Armada-370-xp.dtsi. With this new feature the size of the
> registers are bigger for Armada XP and the new compatible string is
> used.
> 
> The Device Tree binding documentation is updated accordingly.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
>  arch/arm/boot/dts/armada-370-xp.dtsi                  |  2 --
>  arch/arm/boot/dts/armada-370.dtsi                     |  8 ++++++++
>  arch/arm/boot/dts/armada-xp.dtsi                      | 10 ++++++++++
>  4 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index a1ee681..ce7af6a 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -4,7 +4,8 @@
>  Required properties :
>  
>   - reg             : Offset and length of the register set for the device
> - - compatible      : Should be "marvell,mv64xxx-i2c"
> + - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
> +for controller which support the I2C Transaction Generator
>   - interrupts      : The interrupt number
>  
>  Optional properties :
> @@ -20,3 +21,13 @@ Examples:
>  		interrupts = <29>;
>  		clock-frequency = <100000>;
>  	};
> +
> +For a controller which support the I2C Transaction Generator:
> +
> +	i2c at 11000 {
> +		compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
> +		reg = <0x11000 0x100>;
> +		compatible = "marvell,mv64xxx-i2c";

extra compatible line.  If there's nothing else, I'll fix this up when I
pull it in.

thx,

Jason.

> +		interrupts = <29>;
> +		clock-frequency = <100000>;
> +	};
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 550eb77..b6f475c 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -112,7 +112,6 @@
>  
>  			i2c0: i2c at 11000 {
>  				compatible = "marvell,mv64xxx-i2c";
> -				reg = <0x11000 0x20>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  				interrupts = <31>;
> @@ -123,7 +122,6 @@
>  
>  			i2c1: i2c at 11100 {
>  				compatible = "marvell,mv64xxx-i2c";
> -				reg = <0x11100 0x20>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  				interrupts = <32>;
> diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
> index aee2b18..39b26d6 100644
> --- a/arch/arm/boot/dts/armada-370.dtsi
> +++ b/arch/arm/boot/dts/armada-370.dtsi
> @@ -155,6 +155,14 @@
>  				};
>  			};
>  
> +			i2c0: i2c at 11000 {
> +				reg = <0x11000 0x20>;
> +			};
> +
> +			i2c1: i2c at 11100 {
> +				reg = <0x11100 0x20>;
> +			};
> +
>  			usb at 50000 {
>  				clocks = <&coreclk 0>;
>  			};
> diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
> index 5b902f9..c2c357e 100644
> --- a/arch/arm/boot/dts/armada-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-xp.dtsi
> @@ -134,6 +134,16 @@
>  				};
>  			};
>  
> +			i2c0: i2c at 11000 {
> +				compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
> +				reg = <0x11000 0x100>;
> +			};
> +
> +			i2c1: i2c at 11100 {
> +				compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
> +				reg = <0x11100 0x100>;
> +			};
> +
>  			usb at 50000 {
>  				clocks = <&gateclk 18>;
>  			};
> -- 
> 1.8.1.2
> 

[PATCH v3 4/4] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Sebastian Hesselbarth]

On 06/21/13 16:07, Jason Cooper wrote:
> On Fri, Jun 21, 2013 at 03:32:09PM +0200, Gregory CLEMENT wrote:
>> The mv64xxx-i2c embedded in the Armada XP have a new feature to
>> offload i2c transaction. This new version of the IP come also with
>> some errata. This lead to the introduction to a another compatible
>> string.
>>
>> This commit split the i2c information into armada-370.dtsi and
>> armada-xp.dtsi. Most of the data remains the same and stay in the
>> common file Armada-370-xp.dtsi. With this new feature the size of the
>> registers are bigger for Armada XP and the new compatible string is
>> used.
>>
>> The Device Tree binding documentation is updated accordingly.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>   Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
>>   arch/arm/boot/dts/armada-370-xp.dtsi                  |  2 --
>>   arch/arm/boot/dts/armada-370.dtsi                     |  8 ++++++++
>>   arch/arm/boot/dts/armada-xp.dtsi                      | 10 ++++++++++
>>   4 files changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> index a1ee681..ce7af6a 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> @@ -4,7 +4,8 @@
>>   Required properties :
>>
>>    - reg             : Offset and length of the register set for the device
>> - - compatible      : Should be "marvell,mv64xxx-i2c"
>> + - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
>> +for controller which support the I2C Transaction Generator

Jason, Gregory,

Also, s/mv7230/mv78230/

from a quick check of the patch set (which you forgot to send to LKML)
I am wondering why you didn't update the of matches struct with the new
compatible for "marvell,mv78230-i2c"? This will save you from still
having "marvell,mv64xxx-i2c" as additional compatible to match device
and driver. With that the above should also be s/and/or/.

>>    - interrupts      : The interrupt number
>>
>>   Optional properties :
>> @@ -20,3 +21,13 @@ Examples:
>>   		interrupts = <29>;
>>   		clock-frequency = <100000>;
>>   	};
>> +
>> +For a controller which support the I2C Transaction Generator:
>> +
>> +	i2c at 11000 {
>> +		compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
>> +		reg = <0x11000 0x100>;
>> +		compatible = "marvell,mv64xxx-i2c";
>
> extra compatible line.  If there's nothing else, I'll fix this up when I
> pull it in.

[PATCH v3 4/4] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Gregory CLEMENT]

On 06/21/2013 04:15 PM, Sebastian Hesselbarth wrote:
> On 06/21/13 16:07, Jason Cooper wrote:
>> On Fri, Jun 21, 2013 at 03:32:09PM +0200, Gregory CLEMENT wrote:
>>> The mv64xxx-i2c embedded in the Armada XP have a new feature to
>>> offload i2c transaction. This new version of the IP come also with
>>> some errata. This lead to the introduction to a another compatible
>>> string.
>>>
>>> This commit split the i2c information into armada-370.dtsi and
>>> armada-xp.dtsi. Most of the data remains the same and stay in the
>>> common file Armada-370-xp.dtsi. With this new feature the size of the
>>> registers are bigger for Armada XP and the new compatible string is
>>> used.
>>>
>>> The Device Tree binding documentation is updated accordingly.
>>>
>>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>> ---
>>>   Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
>>>   arch/arm/boot/dts/armada-370-xp.dtsi                  |  2 --
>>>   arch/arm/boot/dts/armada-370.dtsi                     |  8 ++++++++
>>>   arch/arm/boot/dts/armada-xp.dtsi                      | 10 ++++++++++
>>>   4 files changed, 30 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>>> index a1ee681..ce7af6a 100644
>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>>> @@ -4,7 +4,8 @@
>>>   Required properties :
>>>
>>>    - reg             : Offset and length of the register set for the device
>>> - - compatible      : Should be "marvell,mv64xxx-i2c"
>>> + - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
>>> +for controller which support the I2C Transaction Generator
> 
> Jason, Gregory,
> 
> Also, s/mv7230/mv78230/

You are right, thanks for both of you to notice it.

> 
> from a quick check of the patch set (which you forgot to send to LKML)
> I am wondering why you didn't update the of matches struct with the new
> compatible for "marvell,mv78230-i2c"? This will save you from still
> having "marvell,mv64xxx-i2c" as additional compatible to match device
> and driver. With that the above should also be s/and/or/.

>From my point of view it is more an extension than an other kind of controller like
the one embedded in the AllWinner SoCs. So I thought it was more in the spirit of
the way you describe hardware in the device tree, but I am far for being an expert
so I maybe wrong.

> 
>>>    - interrupts      : The interrupt number
>>>
>>>   Optional properties :
>>> @@ -20,3 +21,13 @@ Examples:
>>>   		interrupts = <29>;
>>>   		clock-frequency = <100000>;
>>>   	};
>>> +
>>> +For a controller which support the I2C Transaction Generator:
>>> +
>>> +	i2c at 11000 {
>>> +		compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
>>> +		reg = <0x11000 0x100>;
>>> +		compatible = "marvell,mv64xxx-i2c";
>>
>> extra compatible line.  If there's nothing else, I'll fix this up when I
>> pull it in.
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3 4/4] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Jason Cooper]

On Fri, Jun 21, 2013 at 04:15:29PM +0200, Sebastian Hesselbarth wrote:
> On 06/21/13 16:07, Jason Cooper wrote:
> >On Fri, Jun 21, 2013 at 03:32:09PM +0200, Gregory CLEMENT wrote:
> >>The mv64xxx-i2c embedded in the Armada XP have a new feature to
> >>offload i2c transaction. This new version of the IP come also with
> >>some errata. This lead to the introduction to a another compatible
> >>string.
> >>
> >>This commit split the i2c information into armada-370.dtsi and
> >>armada-xp.dtsi. Most of the data remains the same and stay in the
> >>common file Armada-370-xp.dtsi. With this new feature the size of the
> >>registers are bigger for Armada XP and the new compatible string is
> >>used.
> >>
> >>The Device Tree binding documentation is updated accordingly.
> >>
> >>Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> >>---
> >>  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 13 ++++++++++++-
> >>  arch/arm/boot/dts/armada-370-xp.dtsi                  |  2 --
> >>  arch/arm/boot/dts/armada-370.dtsi                     |  8 ++++++++
> >>  arch/arm/boot/dts/armada-xp.dtsi                      | 10 ++++++++++
> >>  4 files changed, 30 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> >>index a1ee681..ce7af6a 100644
> >>--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> >>+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> >>@@ -4,7 +4,8 @@
> >>  Required properties :
> >>
> >>   - reg             : Offset and length of the register set for the device
> >>- - compatible      : Should be "marvell,mv64xxx-i2c"
> >>+ - compatible      : Should be "marvell,mv64xxx-i2c" and "marvell,mv7230-i2c"
> >>+for controller which support the I2C Transaction Generator
> 
> Jason, Gregory,
> 
> Also, s/mv7230/mv78230/

noted.

> from a quick check of the patch set (which you forgot to send to LKML)
> I am wondering why you didn't update the of matches struct with the new
> compatible for "marvell,mv78230-i2c"? This will save you from still
> having "marvell,mv64xxx-i2c" as additional compatible to match device
> and driver. With that the above should also be s/and/or/.

agreed, good point.

Gregory,

I'm sending the last PRs for mvebu today.  I'll include this one with my
suggestion and Sebastian's if you're ok with it.  That means you'll have
to respin the series for the i2c folks.  Does that work for you?

thx,

Jason.

> >>   - interrupts      : The interrupt number
> >>
> >>  Optional properties :
> >>@@ -20,3 +21,13 @@ Examples:
> >>  		interrupts = <29>;
> >>  		clock-frequency = <100000>;
> >>  	};
> >>+
> >>+For a controller which support the I2C Transaction Generator:
> >>+
> >>+	i2c at 11000 {
> >>+		compatible = "marvell,mv64xxx-i2c", "marvell,mv78230-i2c";
> >>+		reg = <0x11000 0x100>;
> >>+		compatible = "marvell,mv64xxx-i2c";
> >
> >extra compatible line.  If there's nothing else, I'll fix this up when I
> >pull it in.
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 4/4] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Wolfram Sang]

> > from a quick check of the patch set (which you forgot to send to LKML)
> > I am wondering why you didn't update the of matches struct with the new
> > compatible for "marvell,mv78230-i2c"? This will save you from still
> > having "marvell,mv64xxx-i2c" as additional compatible to match device
> > and driver. With that the above should also be s/and/or/.
> 
> agreed, good point.
> 
> Gregory,
> 
> I'm sending the last PRs for mvebu today.  I'll include this one with my
> suggestion and Sebastian's if you're ok with it.  That means you'll have
> to respin the series for the i2c folks.  Does that work for you?

No hurry, please. Most of the patches are 3.12 material anyhow, so we
can as well just wait until the I2C patches are okay and then update the
dts later.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130621/0fc4b799/attachment.sig>

[PATCH v3 4/4] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Jason Cooper]

On Fri, Jun 21, 2013 at 05:03:23PM +0200, Wolfram Sang wrote:
> 
> > > from a quick check of the patch set (which you forgot to send to LKML)
> > > I am wondering why you didn't update the of matches struct with the new
> > > compatible for "marvell,mv78230-i2c"? This will save you from still
> > > having "marvell,mv64xxx-i2c" as additional compatible to match device
> > > and driver. With that the above should also be s/and/or/.
> > 
> > agreed, good point.
> > 
> > Gregory,
> > 
> > I'm sending the last PRs for mvebu today.  I'll include this one with my
> > suggestion and Sebastian's if you're ok with it.  That means you'll have
> > to respin the series for the i2c folks.  Does that work for you?
> 
> No hurry, please. Most of the patches are 3.12 material anyhow, so we
> can as well just wait until the I2C patches are okay and then update the
> dts later.

Ok, thanks for the heads up.  I'll put this in my v3.12 stack.

thx,

Jason.

[PATCH v3 4/4] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Jason Gunthorpe]

On Fri, Jun 21, 2013 at 04:15:29PM +0200, Sebastian Hesselbarth wrote:

> from a quick check of the patch set (which you forgot to send to LKML)
> I am wondering why you didn't update the of matches struct with the new
> compatible for "marvell,mv78230-i2c"? This will save you from still
> having "marvell,mv64xxx-i2c" as additional compatible to match device
> and driver. With that the above should also be s/and/or/.

Agree, I noticed the same things.

Alos, the compatible string should be:

  compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";

(note the order reversal, most specific is first)

and I think it is better to use the 'data' field of of_device_id than
a of_is_compatible call. The former is sensitive to order of the
compatible string, the latter is not.

Jason

[PATCH v3 4/4] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Sebastian Hesselbarth]

On 06/21/2013 07:18 PM, Jason Gunthorpe wrote:
> On Fri, Jun 21, 2013 at 04:15:29PM +0200, Sebastian Hesselbarth wrote:
>> from a quick check of the patch set (which you forgot to send to LKML)
>> I am wondering why you didn't update the of matches struct with the new
>> compatible for "marvell,mv78230-i2c"? This will save you from still
>> having "marvell,mv64xxx-i2c" as additional compatible to match device
>> and driver. With that the above should also be s/and/or/.
>
> Agree, I noticed the same things.
>
> Alos, the compatible string should be:
>
>    compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>
> (note the order reversal, most specific is first)

Actually, I suggest to use "marvell,mv78230-i2c" alone. With a new
entry in the match table it will be sufficient for matching driver and
device. BTW, is mv78230 sufficient to match all SoC variants having
this feature? Maybe mv78xx0 but that could interfere with Discovery
Innovation or even better armada-xp-i2c (or armada-370-i2c)?

> and I think it is better to use the 'data' field of of_device_id than
> a of_is_compatible call. The former is sensitive to order of the
> compatible string, the latter is not.

IMHO as long as there is only one compatible and only a bool to set,
using of_device_is_compatible without data pointer is fine here.
IIRC sunxi i2c isn't using data pointer the different register offset
struct, so we shouldn't for a simple bool. And casting a bool to
(void *) looks awkward.

Sebastian

[PATCH v3 4/4] ARM: dts: mvebu: Introduce a new compatible string for mv64xxx-i2c

[Gregory CLEMENT]

On 06/22/2013 06:14 PM, Sebastian Hesselbarth wrote:
> On 06/21/2013 07:18 PM, Jason Gunthorpe wrote:
>> On Fri, Jun 21, 2013 at 04:15:29PM +0200, Sebastian Hesselbarth wrote:
>>> from a quick check of the patch set (which you forgot to send to LKML)
>>> I am wondering why you didn't update the of matches struct with the new
>>> compatible for "marvell,mv78230-i2c"? This will save you from still
>>> having "marvell,mv64xxx-i2c" as additional compatible to match device
>>> and driver. With that the above should also be s/and/or/.
>>
>> Agree, I noticed the same things.
>>
>> Alos, the compatible string should be:
>>
>>    compatible = "marvell,mv78230-i2c", "marvell,mv64xxx-i2c";
>>
>> (note the order reversal, most specific is first)
> 
> Actually, I suggest to use "marvell,mv78230-i2c" alone. With a new
> entry in the match table it will be sufficient for matching driver and

I would prefer using a specialization rather than a new entry, so I can just
use the "marvell,mv64xxx-i2c" entry for the data pointer .

> device. BTW, is mv78230 sufficient to match all SoC variants having
> this feature? Maybe mv78xx0 but that could interfere with Discovery
> Innovation or even better armada-xp-i2c (or armada-370-i2c)?

I am pretty sure that the convention in the device tree is to always use
specific product numbers, rather than wildcards. So, here, the choice of
mv78230 was just that we can see the mv78260 and mv78460 as extension of
the mv78230, so I chose the smallest common denominator.

> 
>> and I think it is better to use the 'data' field of of_device_id than
>> a of_is_compatible call. The former is sensitive to order of the
>> compatible string, the latter is not.
> 
> IMHO as long as there is only one compatible and only a bool to set,
> using of_device_is_compatible without data pointer is fine here.
> IIRC sunxi i2c isn't using data pointer the different register offset
> struct, so we shouldn't for a simple bool. And casting a bool to
> (void *) looks awkward.
> 
> Sebastian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com