[PATCH RESEND 1/2] ARM: dts: vf610-twr: enable i2c0 device

[PATCH RESEND 1/2] ARM: dts: vf610-twr: enable i2c0 device

[Jingchang Lu]

enable i2c0 device on Vybrid VF610 Tower Board

Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 arch/arm/boot/dts/vf610-twr.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index b3905f5..1a58678 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -50,6 +50,13 @@
 	status = "okay";
 };
 
+&i2c0 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c0_1>;
+	status = "okay";
+};
+
 &uart1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart1_1>;
-- 
1.8.0

[PATCH RESEND 1/2] ARM: dts: vf610-twr: enable i2c0 device

[Jingchang Lu]

enable i2c0 device on Vybrid VF610 Tower Board

Signed-off-by: Jingchang Lu <b35083@freescale.com>
---
 arch/arm/boot/dts/vf610-twr.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index b3905f5..1a58678 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -50,6 +50,13 @@
 	status = "okay";
 };
 
+&i2c0 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c0_1>;
+	status = "okay";
+};
+
 &uart1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart1_1>;
-- 
1.8.0

[PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Jingchang Lu]

  Add Freescale Vybrid VF610 I2C controller support to
imx I2C driver framework.
  Some operation is different from imx I2C controller.
The register offset, the i2c clock divider value table,
the module enabling(I2CR_IEN) which is just invert with imx,
and the interrupt flag(I2SR) clearing opcode is w1c on VF610
but w0c on imx.

Signed-off-by: Jason Jin <Jason.jin-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Signed-off-by: Xiaochun Li <b41219-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
changes in v3:
  Using struct naming the i2c clock {div, regval} pair.
  Using address shift handling registers address difference.

changes in v2:
  Fix building section mismatch(es) warning.

 drivers/i2c/busses/i2c-imx.c | 146 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 122 insertions(+), 24 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index e242797..8f9981c 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -30,6 +30,8 @@
  *	Copyright (C) 2007 RightHand Technologies, Inc.
  *	Copyright (C) 2008 Darius Augulis <darius.augulis at teltonika.lt>
  *
+ *	Copyright 2013 Freescale Semiconductor, Inc.
+ *
  */
 
 /** Includes *******************************************************************
@@ -62,13 +64,6 @@
 /* Default value */
 #define IMX_I2C_BIT_RATE	100000	/* 100kHz */
 
-/* IMX I2C registers */
-#define IMX_I2C_IADR	0x00	/* i2c slave address */
-#define IMX_I2C_IFDR	0x04	/* i2c frequency divider */
-#define IMX_I2C_I2CR	0x08	/* i2c control */
-#define IMX_I2C_I2SR	0x0C	/* i2c status */
-#define IMX_I2C_I2DR	0x10	/* i2c transfer data */
-
 /* Bits of IMX I2C registers */
 #define I2SR_RXAK	0x01
 #define I2SR_IIF	0x02
@@ -95,8 +90,12 @@
  *
  * Duplicated divider values removed from list
  */
+struct imx_i2c_clk_pair {
+	u16	div;
+	u16	regval;
+};
 
-static u16 __initdata i2c_clk_div[50][2] = {
+static struct imx_i2c_clk_pair imx_i2c_clk_div[] = {
 	{ 22,	0x20 }, { 24,	0x21 }, { 26,	0x22 }, { 28,	0x23 },
 	{ 30,	0x00 },	{ 32,	0x24 }, { 36,	0x25 }, { 40,	0x26 },
 	{ 42,	0x03 }, { 44,	0x27 },	{ 48,	0x28 }, { 52,	0x05 },
@@ -112,11 +111,63 @@ static u16 __initdata i2c_clk_div[50][2] = {
 	{ 3072,	0x1E }, { 3840,	0x1F }
 };
 
+/* Vybrid VF610 clock divider, register value pairs */
+static struct imx_i2c_clk_pair vf610_i2c_clk_div[] = {
+	{ 20,   0x00 }, { 22,   0x01 }, { 24,   0x02 }, { 26,   0x03 },
+	{ 28,   0x04 }, { 30,   0x05 }, { 32,   0x09 }, { 34,   0x06 },
+	{ 36,   0x0A }, { 40,   0x07 }, { 44,   0x0C }, { 48,   0x0D },
+	{ 52,   0x43 }, { 56,   0x0E }, { 60,   0x45 }, { 64,   0x12 },
+	{ 68,   0x0F }, { 72,   0x13 }, { 80,   0x14 }, { 88,   0x15 },
+	{ 96,   0x19 }, { 104,  0x16 }, { 112,  0x1A }, { 128,  0x17 },
+	{ 136,  0x4F }, { 144,  0x1C }, { 160,  0x1D }, { 176,  0x55 },
+	{ 192,  0x1E }, { 208,  0x56 }, { 224,  0x22 }, { 228,  0x24 },
+	{ 240,  0x1F }, { 256,  0x23 }, { 288,  0x5C }, { 320,  0x25 },
+	{ 384,  0x26 }, { 448,  0x2A }, { 480,  0x27 }, { 512,  0x2B },
+	{ 576,  0x2C }, { 640,  0x2D }, { 768,  0x31 }, { 896,  0x32 },
+	{ 960,  0x2F }, { 1024, 0x33 }, { 1152, 0x34 }, { 1280, 0x35 },
+	{ 1536, 0x36 }, { 1792, 0x3A }, { 1920, 0x37 }, { 2048, 0x3B },
+	{ 2304, 0x3C }, { 2560, 0x3D }, { 3072, 0x3E }, { 3584, 0x7A },
+	{ 3840, 0x3F }, { 4096, 0x7B }, { 5120, 0x7D }, { 6144, 0x7E },
+};
+
 enum imx_i2c_type {
 	IMX1_I2C,
 	IMX21_I2C,
+	VF610_I2C,
 };
 
+struct imx_i2c_hwdata {
+	unsigned		regshift;
+	struct imx_i2c_clk_pair	*clk_div;
+	unsigned		ndivs;
+	unsigned		i2sr_clr_opcode;
+	unsigned		i2cr_ien_opcode;
+};
+
+struct imx_i2c_drvdata {
+	enum imx_i2c_type	devtype;
+	struct imx_i2c_hwdata	*hwdata;
+};
+
+#define IMX_I2C_IADR	(0x00 << i2c_imx->i2c_hwdata->regshift)
+#define IMX_I2C_IFDR	(0x01 << i2c_imx->i2c_hwdata->regshift)
+#define IMX_I2C_I2CR	(0x02 << i2c_imx->i2c_hwdata->regshift)
+#define IMX_I2C_I2SR	(0x03 << i2c_imx->i2c_hwdata->regshift)
+#define IMX_I2C_I2DR	(0x04 << i2c_imx->i2c_hwdata->regshift)
+
+/*
+ * Interrupt flags clear operation differ between SoCs:
+ * - write zero to clear(w0c) INT flag on I.MX,
+ * - but write one to clear(w1c) INT flag on Vybrid.
+ * I2C module enable operation also differ bteween SoCs:
+ * - set I2CR_IEN bit enable the module on I.MX,
+ * - but clear I2CR_IEN bit enable the module on Vybrid.
+ */
+#define I2SR_CLR_OPCODE_W0C		0x0
+#define I2SR_CLR_OPCODE_W1C		(I2SR_IAL | I2SR_IIF)
+#define I2CR_IEN_OPCODE_0		0x0
+#define I2CR_IEN_OPCODE_1		I2CR_IEN
+
 struct imx_i2c_struct {
 	struct i2c_adapter	adapter;
 	struct clk		*clk;
@@ -127,15 +178,52 @@ struct imx_i2c_struct {
 	int			stopped;
 	unsigned int		ifdr; /* IMX_I2C_IFDR */
 	enum imx_i2c_type	devtype;
+	struct imx_i2c_hwdata	*i2c_hwdata;
+};
+
+static struct imx_i2c_hwdata imx_i2c_hwdata = {
+	.regshift		= 2,
+	.clk_div		= imx_i2c_clk_div,
+	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
+	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
+	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
+
+};
+
+static struct imx_i2c_hwdata vf610_i2c_hwdata = {
+	.regshift		= 0,
+	.clk_div		= vf610_i2c_clk_div,
+	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
+	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
+	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
+
+};
+
+static struct imx_i2c_drvdata imx_i2c_drvdata[] = {
+	[IMX1_I2C] = {
+		.devtype = IMX1_I2C,
+		.hwdata = &imx_i2c_hwdata
+	},
+	[IMX21_I2C] = {
+		.devtype = IMX21_I2C,
+		.hwdata = &imx_i2c_hwdata
+	},
+	[VF610_I2C] = {
+		.devtype = VF610_I2C,
+		.hwdata = &vf610_i2c_hwdata
+	},
 };
 
 static struct platform_device_id imx_i2c_devtype[] = {
 	{
 		.name = "imx1-i2c",
-		.driver_data = IMX1_I2C,
+		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[IMX1_I2C],
 	}, {
 		.name = "imx21-i2c",
-		.driver_data = IMX21_I2C,
+		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[IMX21_I2C],
+	}, {
+		.name = "vf610-i2c",
+		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[VF610_I2C],
 	}, {
 		/* sentinel */
 	}
@@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
 static const struct of_device_id i2c_imx_dt_ids[] = {
 	{ .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
 	{ .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
+	{ .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, i2c_imx_dt_ids);
@@ -215,9 +304,8 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
 	clk_prepare_enable(i2c_imx->clk);
 	writeb(i2c_imx->ifdr, i2c_imx->base + IMX_I2C_IFDR);
 	/* Enable I2C controller */
-	writeb(0, i2c_imx->base + IMX_I2C_I2SR);
-	writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
-
+	writeb(i2c_imx->i2c_hwdata->i2sr_clr_opcode, i2c_imx->base + IMX_I2C_I2SR);
+	writeb(i2c_imx->i2c_hwdata->i2cr_ien_opcode, i2c_imx->base + IMX_I2C_I2CR);
 	/* Wait controller to be stable */
 	udelay(50);
 
@@ -260,7 +348,8 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
 	}
 
 	/* Disable I2C controller */
-	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
+	writeb(i2c_imx->i2c_hwdata->i2cr_ien_opcode ^ I2CR_IEN,
+						i2c_imx->base + IMX_I2C_I2CR);
 	clk_disable_unprepare(i2c_imx->clk);
 }
 
@@ -270,19 +359,20 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
 	unsigned int i2c_clk_rate;
 	unsigned int div;
 	int i;
+	struct imx_i2c_clk_pair *i2c_clk_div = i2c_imx->i2c_hwdata->clk_div;
 
 	/* Divider value calculation */
 	i2c_clk_rate = clk_get_rate(i2c_imx->clk);
 	div = (i2c_clk_rate + rate - 1) / rate;
-	if (div < i2c_clk_div[0][0])
+	if (div < i2c_clk_div[0].div)
 		i = 0;
-	else if (div > i2c_clk_div[ARRAY_SIZE(i2c_clk_div) - 1][0])
-		i = ARRAY_SIZE(i2c_clk_div) - 1;
+	else if (div > i2c_clk_div[i2c_imx->i2c_hwdata->ndivs - 1].div)
+		i = i2c_imx->i2c_hwdata->ndivs - 1;
 	else
-		for (i = 0; i2c_clk_div[i][0] < div; i++);
+		for (i = 0; i2c_clk_div[i].div < div; i++);
 
 	/* Store divider value */
-	i2c_imx->ifdr = i2c_clk_div[i][1];
+	i2c_imx->ifdr = i2c_clk_div[i].regval;
 
 	/*
 	 * There dummy delay is calculated.
@@ -290,7 +380,7 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
 	 * This delay is used in I2C bus disable function
 	 * to fix chip hardware bug.
 	 */
-	i2c_imx->disable_delay = (500000U * i2c_clk_div[i][0]
+	i2c_imx->disable_delay = (500000U * i2c_clk_div[i].div
 		+ (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2);
 
 	/* dev_dbg() can't be used, because adapter is not yet registered */
@@ -298,7 +388,7 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
 	dev_dbg(&i2c_imx->adapter.dev, "<%s> I2C_CLK=%d, REQ DIV=%d\n",
 		__func__, i2c_clk_rate, div);
 	dev_dbg(&i2c_imx->adapter.dev, "<%s> IFDR[IC]=0x%x, REAL DIV=%d\n",
-		__func__, i2c_clk_div[i][1], i2c_clk_div[i][0]);
+		__func__, i2c_clk_div[i].regval, i2c_clk_div[i].div);
 #endif
 }
 
@@ -312,6 +402,7 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
 		/* save status register */
 		i2c_imx->i2csr = temp;
 		temp &= ~I2SR_IIF;
+		temp |= (i2c_imx->i2c_hwdata->i2sr_clr_opcode & I2SR_IIF);
 		writeb(temp, i2c_imx->base + IMX_I2C_I2SR);
 		wake_up(&i2c_imx->queue);
 		return IRQ_HANDLED;
@@ -493,6 +584,7 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
 	struct imx_i2c_struct *i2c_imx;
 	struct resource *res;
 	struct imxi2c_platform_data *pdata = pdev->dev.platform_data;
+	struct imx_i2c_drvdata *drvdata;
 	void __iomem *base;
 	int irq, ret;
 	u32 bitrate;
@@ -519,7 +611,9 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
 
 	if (of_id)
 		pdev->id_entry = of_id->data;
-	i2c_imx->devtype = pdev->id_entry->driver_data;
+	drvdata = (struct imx_i2c_drvdata *)pdev->id_entry->driver_data;
+	i2c_imx->devtype = drvdata->devtype;
+	i2c_imx->i2c_hwdata = drvdata->hwdata;
 
 	/* Setup i2c_imx driver structure */
 	strlcpy(i2c_imx->adapter.name, pdev->name, sizeof(i2c_imx->adapter.name));
@@ -560,8 +654,12 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
 	i2c_imx_set_clk(i2c_imx, bitrate);
 
 	/* Set up chip registers to defaults */
-	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
-	writeb(0, i2c_imx->base + IMX_I2C_I2SR);
+	clk_prepare_enable(i2c_imx->clk);
+	writeb(i2c_imx->i2c_hwdata->i2cr_ien_opcode ^ I2CR_IEN,
+						i2c_imx->base + IMX_I2C_I2CR);
+	writeb(i2c_imx->i2c_hwdata->i2sr_clr_opcode,
+						i2c_imx->base + IMX_I2C_I2SR);
+	clk_disable_unprepare(i2c_imx->clk);
 
 	/* Add I2C adapter */
 	ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
-- 
1.8.0

[PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Jingchang Lu]

  Add Freescale Vybrid VF610 I2C controller support to
imx I2C driver framework.
  Some operation is different from imx I2C controller.
The register offset, the i2c clock divider value table,
the module enabling(I2CR_IEN) which is just invert with imx,
and the interrupt flag(I2SR) clearing opcode is w1c on VF610
but w0c on imx.

Signed-off-by: Jason Jin <Jason.jin@freescale.com>
Signed-off-by: Xiaochun Li <b41219@freescale.com>
Signed-off-by: Jingchang Lu <b35083@freescale.com>
---
changes in v3:
  Using struct naming the i2c clock {div, regval} pair.
  Using address shift handling registers address difference.

changes in v2:
  Fix building section mismatch(es) warning.

 drivers/i2c/busses/i2c-imx.c | 146 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 122 insertions(+), 24 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index e242797..8f9981c 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -30,6 +30,8 @@
  *	Copyright (C) 2007 RightHand Technologies, Inc.
  *	Copyright (C) 2008 Darius Augulis <darius.augulis@teltonika.lt>
  *
+ *	Copyright 2013 Freescale Semiconductor, Inc.
+ *
  */
 
 /** Includes *******************************************************************
@@ -62,13 +64,6 @@
 /* Default value */
 #define IMX_I2C_BIT_RATE	100000	/* 100kHz */
 
-/* IMX I2C registers */
-#define IMX_I2C_IADR	0x00	/* i2c slave address */
-#define IMX_I2C_IFDR	0x04	/* i2c frequency divider */
-#define IMX_I2C_I2CR	0x08	/* i2c control */
-#define IMX_I2C_I2SR	0x0C	/* i2c status */
-#define IMX_I2C_I2DR	0x10	/* i2c transfer data */
-
 /* Bits of IMX I2C registers */
 #define I2SR_RXAK	0x01
 #define I2SR_IIF	0x02
@@ -95,8 +90,12 @@
  *
  * Duplicated divider values removed from list
  */
+struct imx_i2c_clk_pair {
+	u16	div;
+	u16	regval;
+};
 
-static u16 __initdata i2c_clk_div[50][2] = {
+static struct imx_i2c_clk_pair imx_i2c_clk_div[] = {
 	{ 22,	0x20 }, { 24,	0x21 }, { 26,	0x22 }, { 28,	0x23 },
 	{ 30,	0x00 },	{ 32,	0x24 }, { 36,	0x25 }, { 40,	0x26 },
 	{ 42,	0x03 }, { 44,	0x27 },	{ 48,	0x28 }, { 52,	0x05 },
@@ -112,11 +111,63 @@ static u16 __initdata i2c_clk_div[50][2] = {
 	{ 3072,	0x1E }, { 3840,	0x1F }
 };
 
+/* Vybrid VF610 clock divider, register value pairs */
+static struct imx_i2c_clk_pair vf610_i2c_clk_div[] = {
+	{ 20,   0x00 }, { 22,   0x01 }, { 24,   0x02 }, { 26,   0x03 },
+	{ 28,   0x04 }, { 30,   0x05 }, { 32,   0x09 }, { 34,   0x06 },
+	{ 36,   0x0A }, { 40,   0x07 }, { 44,   0x0C }, { 48,   0x0D },
+	{ 52,   0x43 }, { 56,   0x0E }, { 60,   0x45 }, { 64,   0x12 },
+	{ 68,   0x0F }, { 72,   0x13 }, { 80,   0x14 }, { 88,   0x15 },
+	{ 96,   0x19 }, { 104,  0x16 }, { 112,  0x1A }, { 128,  0x17 },
+	{ 136,  0x4F }, { 144,  0x1C }, { 160,  0x1D }, { 176,  0x55 },
+	{ 192,  0x1E }, { 208,  0x56 }, { 224,  0x22 }, { 228,  0x24 },
+	{ 240,  0x1F }, { 256,  0x23 }, { 288,  0x5C }, { 320,  0x25 },
+	{ 384,  0x26 }, { 448,  0x2A }, { 480,  0x27 }, { 512,  0x2B },
+	{ 576,  0x2C }, { 640,  0x2D }, { 768,  0x31 }, { 896,  0x32 },
+	{ 960,  0x2F }, { 1024, 0x33 }, { 1152, 0x34 }, { 1280, 0x35 },
+	{ 1536, 0x36 }, { 1792, 0x3A }, { 1920, 0x37 }, { 2048, 0x3B },
+	{ 2304, 0x3C }, { 2560, 0x3D }, { 3072, 0x3E }, { 3584, 0x7A },
+	{ 3840, 0x3F }, { 4096, 0x7B }, { 5120, 0x7D }, { 6144, 0x7E },
+};
+
 enum imx_i2c_type {
 	IMX1_I2C,
 	IMX21_I2C,
+	VF610_I2C,
 };
 
+struct imx_i2c_hwdata {
+	unsigned		regshift;
+	struct imx_i2c_clk_pair	*clk_div;
+	unsigned		ndivs;
+	unsigned		i2sr_clr_opcode;
+	unsigned		i2cr_ien_opcode;
+};
+
+struct imx_i2c_drvdata {
+	enum imx_i2c_type	devtype;
+	struct imx_i2c_hwdata	*hwdata;
+};
+
+#define IMX_I2C_IADR	(0x00 << i2c_imx->i2c_hwdata->regshift)
+#define IMX_I2C_IFDR	(0x01 << i2c_imx->i2c_hwdata->regshift)
+#define IMX_I2C_I2CR	(0x02 << i2c_imx->i2c_hwdata->regshift)
+#define IMX_I2C_I2SR	(0x03 << i2c_imx->i2c_hwdata->regshift)
+#define IMX_I2C_I2DR	(0x04 << i2c_imx->i2c_hwdata->regshift)
+
+/*
+ * Interrupt flags clear operation differ between SoCs:
+ * - write zero to clear(w0c) INT flag on I.MX,
+ * - but write one to clear(w1c) INT flag on Vybrid.
+ * I2C module enable operation also differ bteween SoCs:
+ * - set I2CR_IEN bit enable the module on I.MX,
+ * - but clear I2CR_IEN bit enable the module on Vybrid.
+ */
+#define I2SR_CLR_OPCODE_W0C		0x0
+#define I2SR_CLR_OPCODE_W1C		(I2SR_IAL | I2SR_IIF)
+#define I2CR_IEN_OPCODE_0		0x0
+#define I2CR_IEN_OPCODE_1		I2CR_IEN
+
 struct imx_i2c_struct {
 	struct i2c_adapter	adapter;
 	struct clk		*clk;
@@ -127,15 +178,52 @@ struct imx_i2c_struct {
 	int			stopped;
 	unsigned int		ifdr; /* IMX_I2C_IFDR */
 	enum imx_i2c_type	devtype;
+	struct imx_i2c_hwdata	*i2c_hwdata;
+};
+
+static struct imx_i2c_hwdata imx_i2c_hwdata = {
+	.regshift		= 2,
+	.clk_div		= imx_i2c_clk_div,
+	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
+	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
+	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
+
+};
+
+static struct imx_i2c_hwdata vf610_i2c_hwdata = {
+	.regshift		= 0,
+	.clk_div		= vf610_i2c_clk_div,
+	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
+	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
+	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
+
+};
+
+static struct imx_i2c_drvdata imx_i2c_drvdata[] = {
+	[IMX1_I2C] = {
+		.devtype = IMX1_I2C,
+		.hwdata = &imx_i2c_hwdata
+	},
+	[IMX21_I2C] = {
+		.devtype = IMX21_I2C,
+		.hwdata = &imx_i2c_hwdata
+	},
+	[VF610_I2C] = {
+		.devtype = VF610_I2C,
+		.hwdata = &vf610_i2c_hwdata
+	},
 };
 
 static struct platform_device_id imx_i2c_devtype[] = {
 	{
 		.name = "imx1-i2c",
-		.driver_data = IMX1_I2C,
+		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[IMX1_I2C],
 	}, {
 		.name = "imx21-i2c",
-		.driver_data = IMX21_I2C,
+		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[IMX21_I2C],
+	}, {
+		.name = "vf610-i2c",
+		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[VF610_I2C],
 	}, {
 		/* sentinel */
 	}
@@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
 static const struct of_device_id i2c_imx_dt_ids[] = {
 	{ .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
 	{ .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
+	{ .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, i2c_imx_dt_ids);
@@ -215,9 +304,8 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
 	clk_prepare_enable(i2c_imx->clk);
 	writeb(i2c_imx->ifdr, i2c_imx->base + IMX_I2C_IFDR);
 	/* Enable I2C controller */
-	writeb(0, i2c_imx->base + IMX_I2C_I2SR);
-	writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
-
+	writeb(i2c_imx->i2c_hwdata->i2sr_clr_opcode, i2c_imx->base + IMX_I2C_I2SR);
+	writeb(i2c_imx->i2c_hwdata->i2cr_ien_opcode, i2c_imx->base + IMX_I2C_I2CR);
 	/* Wait controller to be stable */
 	udelay(50);
 
@@ -260,7 +348,8 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
 	}
 
 	/* Disable I2C controller */
-	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
+	writeb(i2c_imx->i2c_hwdata->i2cr_ien_opcode ^ I2CR_IEN,
+						i2c_imx->base + IMX_I2C_I2CR);
 	clk_disable_unprepare(i2c_imx->clk);
 }
 
@@ -270,19 +359,20 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
 	unsigned int i2c_clk_rate;
 	unsigned int div;
 	int i;
+	struct imx_i2c_clk_pair *i2c_clk_div = i2c_imx->i2c_hwdata->clk_div;
 
 	/* Divider value calculation */
 	i2c_clk_rate = clk_get_rate(i2c_imx->clk);
 	div = (i2c_clk_rate + rate - 1) / rate;
-	if (div < i2c_clk_div[0][0])
+	if (div < i2c_clk_div[0].div)
 		i = 0;
-	else if (div > i2c_clk_div[ARRAY_SIZE(i2c_clk_div) - 1][0])
-		i = ARRAY_SIZE(i2c_clk_div) - 1;
+	else if (div > i2c_clk_div[i2c_imx->i2c_hwdata->ndivs - 1].div)
+		i = i2c_imx->i2c_hwdata->ndivs - 1;
 	else
-		for (i = 0; i2c_clk_div[i][0] < div; i++);
+		for (i = 0; i2c_clk_div[i].div < div; i++);
 
 	/* Store divider value */
-	i2c_imx->ifdr = i2c_clk_div[i][1];
+	i2c_imx->ifdr = i2c_clk_div[i].regval;
 
 	/*
 	 * There dummy delay is calculated.
@@ -290,7 +380,7 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
 	 * This delay is used in I2C bus disable function
 	 * to fix chip hardware bug.
 	 */
-	i2c_imx->disable_delay = (500000U * i2c_clk_div[i][0]
+	i2c_imx->disable_delay = (500000U * i2c_clk_div[i].div
 		+ (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2);
 
 	/* dev_dbg() can't be used, because adapter is not yet registered */
@@ -298,7 +388,7 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
 	dev_dbg(&i2c_imx->adapter.dev, "<%s> I2C_CLK=%d, REQ DIV=%d\n",
 		__func__, i2c_clk_rate, div);
 	dev_dbg(&i2c_imx->adapter.dev, "<%s> IFDR[IC]=0x%x, REAL DIV=%d\n",
-		__func__, i2c_clk_div[i][1], i2c_clk_div[i][0]);
+		__func__, i2c_clk_div[i].regval, i2c_clk_div[i].div);
 #endif
 }
 
@@ -312,6 +402,7 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
 		/* save status register */
 		i2c_imx->i2csr = temp;
 		temp &= ~I2SR_IIF;
+		temp |= (i2c_imx->i2c_hwdata->i2sr_clr_opcode & I2SR_IIF);
 		writeb(temp, i2c_imx->base + IMX_I2C_I2SR);
 		wake_up(&i2c_imx->queue);
 		return IRQ_HANDLED;
@@ -493,6 +584,7 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
 	struct imx_i2c_struct *i2c_imx;
 	struct resource *res;
 	struct imxi2c_platform_data *pdata = pdev->dev.platform_data;
+	struct imx_i2c_drvdata *drvdata;
 	void __iomem *base;
 	int irq, ret;
 	u32 bitrate;
@@ -519,7 +611,9 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
 
 	if (of_id)
 		pdev->id_entry = of_id->data;
-	i2c_imx->devtype = pdev->id_entry->driver_data;
+	drvdata = (struct imx_i2c_drvdata *)pdev->id_entry->driver_data;
+	i2c_imx->devtype = drvdata->devtype;
+	i2c_imx->i2c_hwdata = drvdata->hwdata;
 
 	/* Setup i2c_imx driver structure */
 	strlcpy(i2c_imx->adapter.name, pdev->name, sizeof(i2c_imx->adapter.name));
@@ -560,8 +654,12 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
 	i2c_imx_set_clk(i2c_imx, bitrate);
 
 	/* Set up chip registers to defaults */
-	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
-	writeb(0, i2c_imx->base + IMX_I2C_I2SR);
+	clk_prepare_enable(i2c_imx->clk);
+	writeb(i2c_imx->i2c_hwdata->i2cr_ien_opcode ^ I2CR_IEN,
+						i2c_imx->base + IMX_I2C_I2CR);
+	writeb(i2c_imx->i2c_hwdata->i2sr_clr_opcode,
+						i2c_imx->base + IMX_I2C_I2SR);
+	clk_disable_unprepare(i2c_imx->clk);
 
 	/* Add I2C adapter */
 	ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
-- 
1.8.0

Re: [PATCH RESEND 1/2] ARM: dts: vf610-twr: enable i2c0 device

[Shawn Guo]

On Fri, Aug 02, 2013 at 12:44:07PM +0800, Jingchang Lu wrote:
> enable i2c0 device on Vybrid VF610 Tower Board
> 
> Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

I suggest you send me the patch after the driver part gets accepted by
subsystem maintainer.

Shawn

> ---
>  arch/arm/boot/dts/vf610-twr.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
> index b3905f5..1a58678 100644
> --- a/arch/arm/boot/dts/vf610-twr.dts
> +++ b/arch/arm/boot/dts/vf610-twr.dts
> @@ -50,6 +50,13 @@
>  	status = "okay";
>  };
>  
> +&i2c0 {
> +	clock-frequency = <100000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c0_1>;
> +	status = "okay";
> +};
> +
>  &uart1 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_uart1_1>;
> -- 
> 1.8.0
> 
> 

[PATCH RESEND 1/2] ARM: dts: vf610-twr: enable i2c0 device

[Shawn Guo]

On Fri, Aug 02, 2013 at 12:44:07PM +0800, Jingchang Lu wrote:
> enable i2c0 device on Vybrid VF610 Tower Board
> 
> Signed-off-by: Jingchang Lu <b35083@freescale.com>

I suggest you send me the patch after the driver part gets accepted by
subsystem maintainer.

Shawn

> ---
>  arch/arm/boot/dts/vf610-twr.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
> index b3905f5..1a58678 100644
> --- a/arch/arm/boot/dts/vf610-twr.dts
> +++ b/arch/arm/boot/dts/vf610-twr.dts
> @@ -50,6 +50,13 @@
>  	status = "okay";
>  };
>  
> +&i2c0 {
> +	clock-frequency = <100000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c0_1>;
> +	status = "okay";
> +};
> +
>  &uart1 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_uart1_1>;
> -- 
> 1.8.0
> 
> 

Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Sascha Hauer]

On Fri, Aug 02, 2013 at 12:44:08PM +0800, Jingchang Lu wrote:
>   Add Freescale Vybrid VF610 I2C controller support to
> imx I2C driver framework.
>   Some operation is different from imx I2C controller.
> The register offset, the i2c clock divider value table,
> the module enabling(I2CR_IEN) which is just invert with imx,
> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
> but w0c on imx.
> 
> Signed-off-by: Jason Jin <Jason.jin-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Signed-off-by: Xiaochun Li <b41219-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
> changes in v3:
>   Using struct naming the i2c clock {div, regval} pair.
>   Using address shift handling registers address difference.
> 
> changes in v2:
>   Fix building section mismatch(es) warning.
> 
>  drivers/i2c/busses/i2c-imx.c | 146 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 122 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index e242797..8f9981c 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -30,6 +30,8 @@
>   *	Copyright (C) 2007 RightHand Technologies, Inc.
>   *	Copyright (C) 2008 Darius Augulis <darius.augulis at teltonika.lt>
>   *
> + *	Copyright 2013 Freescale Semiconductor, Inc.
> + *
>   */
>  
>  /** Includes *******************************************************************
> @@ -62,13 +64,6 @@
>  /* Default value */
>  #define IMX_I2C_BIT_RATE	100000	/* 100kHz */
>  
> -/* IMX I2C registers */
> -#define IMX_I2C_IADR	0x00	/* i2c slave address */
> -#define IMX_I2C_IFDR	0x04	/* i2c frequency divider */
> -#define IMX_I2C_I2CR	0x08	/* i2c control */
> -#define IMX_I2C_I2SR	0x0C	/* i2c status */
> -#define IMX_I2C_I2DR	0x10	/* i2c transfer data */
> -
>  /* Bits of IMX I2C registers */
>  #define I2SR_RXAK	0x01
>  #define I2SR_IIF	0x02
> @@ -95,8 +90,12 @@
>   *
>   * Duplicated divider values removed from list
>   */
> +struct imx_i2c_clk_pair {
> +	u16	div;
> +	u16	regval;
> +}i;

Converting this to a struct should be a separate patch. This is an
obvious cleanup which makes maintainers happy and more willing to take
the features you are adding later.

>  
> -static u16 __initdata i2c_clk_div[50][2] = {
> +static struct imx_i2c_clk_pair imx_i2c_clk_div[] = {
>  	{ 22,	0x20 }, { 24,	0x21 }, { 26,	0x22 }, { 28,	0x23 },
>  	{ 30,	0x00 },	{ 32,	0x24 }, { 36,	0x25 }, { 40,	0x26 },
>  	{ 42,	0x03 }, { 44,	0x27 },	{ 48,	0x28 }, { 52,	0x05 },
> @@ -112,11 +111,63 @@ static u16 __initdata i2c_clk_div[50][2] = {
>  	{ 3072,	0x1E }, { 3840,	0x1F }
>  };
>  
> +/* Vybrid VF610 clock divider, register value pairs */
> +static struct imx_i2c_clk_pair vf610_i2c_clk_div[] = {
> +	{ 20,   0x00 }, { 22,   0x01 }, { 24,   0x02 }, { 26,   0x03 },
> +	{ 28,   0x04 }, { 30,   0x05 }, { 32,   0x09 }, { 34,   0x06 },
> +	{ 36,   0x0A }, { 40,   0x07 }, { 44,   0x0C }, { 48,   0x0D },
> +	{ 52,   0x43 }, { 56,   0x0E }, { 60,   0x45 }, { 64,   0x12 },
> +	{ 68,   0x0F }, { 72,   0x13 }, { 80,   0x14 }, { 88,   0x15 },
> +	{ 96,   0x19 }, { 104,  0x16 }, { 112,  0x1A }, { 128,  0x17 },
> +	{ 136,  0x4F }, { 144,  0x1C }, { 160,  0x1D }, { 176,  0x55 },
> +	{ 192,  0x1E }, { 208,  0x56 }, { 224,  0x22 }, { 228,  0x24 },
> +	{ 240,  0x1F }, { 256,  0x23 }, { 288,  0x5C }, { 320,  0x25 },
> +	{ 384,  0x26 }, { 448,  0x2A }, { 480,  0x27 }, { 512,  0x2B },
> +	{ 576,  0x2C }, { 640,  0x2D }, { 768,  0x31 }, { 896,  0x32 },
> +	{ 960,  0x2F }, { 1024, 0x33 }, { 1152, 0x34 }, { 1280, 0x35 },
> +	{ 1536, 0x36 }, { 1792, 0x3A }, { 1920, 0x37 }, { 2048, 0x3B },
> +	{ 2304, 0x3C }, { 2560, 0x3D }, { 3072, 0x3E }, { 3584, 0x7A },
> +	{ 3840, 0x3F }, { 4096, 0x7B }, { 5120, 0x7D }, { 6144, 0x7E },
> +};
> +
>  enum imx_i2c_type {
>  	IMX1_I2C,
>  	IMX21_I2C,
> +	VF610_I2C,
>  };
>  
> +struct imx_i2c_hwdata {
> +	unsigned		regshift;
> +	struct imx_i2c_clk_pair	*clk_div;
> +	unsigned		ndivs;
> +	unsigned		i2sr_clr_opcode;
> +	unsigned		i2cr_ien_opcode;
> +};
> +
> +struct imx_i2c_drvdata {
> +	enum imx_i2c_type	devtype;
> +	struct imx_i2c_hwdata	*hwdata;
> +};
> +
> +#define IMX_I2C_IADR	(0x00 << i2c_imx->i2c_hwdata->regshift)
> +#define IMX_I2C_IFDR	(0x01 << i2c_imx->i2c_hwdata->regshift)
> +#define IMX_I2C_I2CR	(0x02 << i2c_imx->i2c_hwdata->regshift)
> +#define IMX_I2C_I2SR	(0x03 << i2c_imx->i2c_hwdata->regshift)
> +#define IMX_I2C_I2DR	(0x04 << i2c_imx->i2c_hwdata->regshift)

Macros should not assume there is a i2c_imx variable present in a
function. Instead, add static inline accessor functions.

> +
> +/*
> + * Interrupt flags clear operation differ between SoCs:
> + * - write zero to clear(w0c) INT flag on I.MX,

It's i.MX. With a Freescale mail address you should get this right ;)

> + * - but write one to clear(w1c) INT flag on Vybrid.
> + * I2C module enable operation also differ bteween SoCs:

s/bteween/between/

> + * - set I2CR_IEN bit enable the module on I.MX,
> + * - but clear I2CR_IEN bit enable the module on Vybrid.
> + */
> +#define I2SR_CLR_OPCODE_W0C		0x0
> +#define I2SR_CLR_OPCODE_W1C		(I2SR_IAL | I2SR_IIF)
> +#define I2CR_IEN_OPCODE_0		0x0
> +#define I2CR_IEN_OPCODE_1		I2CR_IEN
> +
>  struct imx_i2c_struct {
>  	struct i2c_adapter	adapter;
>  	struct clk		*clk;
> @@ -127,15 +178,52 @@ struct imx_i2c_struct {
>  	int			stopped;
>  	unsigned int		ifdr; /* IMX_I2C_IFDR */
>  	enum imx_i2c_type	devtype;
> +	struct imx_i2c_hwdata	*i2c_hwdata;
> +};
> +
> +static struct imx_i2c_hwdata imx_i2c_hwdata = {
> +	.regshift		= 2,
> +	.clk_div		= imx_i2c_clk_div,
> +	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
> +	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> +	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> +
> +};

unnecessary blank line

> +
> +static struct imx_i2c_hwdata vf610_i2c_hwdata = {
> +	.regshift		= 0,
> +	.clk_div		= vf610_i2c_clk_div,
> +	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
> +	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
> +	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> +
> +};

ditto

> +
> +static struct imx_i2c_drvdata imx_i2c_drvdata[] = {
> +	[IMX1_I2C] = {
> +		.devtype = IMX1_I2C,
> +		.hwdata = &imx_i2c_hwdata
> +	},
> +	[IMX21_I2C] = {
> +		.devtype = IMX21_I2C,
> +		.hwdata = &imx_i2c_hwdata
> +	},
> +	[VF610_I2C] = {
> +		.devtype = VF610_I2C,
> +		.hwdata = &vf610_i2c_hwdata
> +	},
>  };

Why add this array?

>  
>  static struct platform_device_id imx_i2c_devtype[] = {
>  	{
>  		.name = "imx1-i2c",
> -		.driver_data = IMX1_I2C,
> +		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[IMX1_I2C],

You should reference imx_i2c_hwdata/vf610_i2c_hwdata directly here.

>  	}, {
>  		.name = "imx21-i2c",
> -		.driver_data = IMX21_I2C,
> +		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[IMX21_I2C],
> +	}, {
> +		.name = "vf610-i2c",
> +		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[VF610_I2C],
>  	}, {
>  		/* sentinel */
>  	}
> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
>  static const struct of_device_id i2c_imx_dt_ids[] = {
>  	{ .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
>  	{ .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
> +	{ .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, i2c_imx_dt_ids);
> @@ -215,9 +304,8 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>  	clk_prepare_enable(i2c_imx->clk);
>  	writeb(i2c_imx->ifdr, i2c_imx->base + IMX_I2C_IFDR);
>  	/* Enable I2C controller */
> -	writeb(0, i2c_imx->base + IMX_I2C_I2SR);
> -	writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> -
> +	writeb(i2c_imx->i2c_hwdata->i2sr_clr_opcode, i2c_imx->base + IMX_I2C_I2SR);
> +	writeb(i2c_imx->i2c_hwdata->i2cr_ien_opcode, i2c_imx->base + IMX_I2C_I2CR);
>  	/* Wait controller to be stable */
>  	udelay(50);
>  
> @@ -260,7 +348,8 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
>  	}
>  
>  	/* Disable I2C controller */
> -	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
> +	writeb(i2c_imx->i2c_hwdata->i2cr_ien_opcode ^ I2CR_IEN,
> +						i2c_imx->base + IMX_I2C_I2CR);
>  	clk_disable_unprepare(i2c_imx->clk);
>  }
>  
> @@ -270,19 +359,20 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
>  	unsigned int i2c_clk_rate;
>  	unsigned int div;
>  	int i;
> +	struct imx_i2c_clk_pair *i2c_clk_div = i2c_imx->i2c_hwdata->clk_div;
>  
>  	/* Divider value calculation */
>  	i2c_clk_rate = clk_get_rate(i2c_imx->clk);
>  	div = (i2c_clk_rate + rate - 1) / rate;
> -	if (div < i2c_clk_div[0][0])
> +	if (div < i2c_clk_div[0].div)
>  		i = 0;
> -	else if (div > i2c_clk_div[ARRAY_SIZE(i2c_clk_div) - 1][0])
> -		i = ARRAY_SIZE(i2c_clk_div) - 1;
> +	else if (div > i2c_clk_div[i2c_imx->i2c_hwdata->ndivs - 1].div)
> +		i = i2c_imx->i2c_hwdata->ndivs - 1;
>  	else
> -		for (i = 0; i2c_clk_div[i][0] < div; i++);
> +		for (i = 0; i2c_clk_div[i].div < div; i++);
>  
>  	/* Store divider value */
> -	i2c_imx->ifdr = i2c_clk_div[i][1];
> +	i2c_imx->ifdr = i2c_clk_div[i].regval;
>  
>  	/*
>  	 * There dummy delay is calculated.
> @@ -290,7 +380,7 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
>  	 * This delay is used in I2C bus disable function
>  	 * to fix chip hardware bug.
>  	 */
> -	i2c_imx->disable_delay = (500000U * i2c_clk_div[i][0]
> +	i2c_imx->disable_delay = (500000U * i2c_clk_div[i].div
>  		+ (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2);
>  
>  	/* dev_dbg() can't be used, because adapter is not yet registered */
> @@ -298,7 +388,7 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s> I2C_CLK=%d, REQ DIV=%d\n",
>  		__func__, i2c_clk_rate, div);
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s> IFDR[IC]=0x%x, REAL DIV=%d\n",
> -		__func__, i2c_clk_div[i][1], i2c_clk_div[i][0]);
> +		__func__, i2c_clk_div[i].regval, i2c_clk_div[i].div);
>  #endif
>  }
>  
> @@ -312,6 +402,7 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  		/* save status register */
>  		i2c_imx->i2csr = temp;
>  		temp &= ~I2SR_IIF;
> +		temp |= (i2c_imx->i2c_hwdata->i2sr_clr_opcode & I2SR_IIF);
>  		writeb(temp, i2c_imx->base + IMX_I2C_I2SR);
>  		wake_up(&i2c_imx->queue);
>  		return IRQ_HANDLED;
> @@ -493,6 +584,7 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
>  	struct imx_i2c_struct *i2c_imx;
>  	struct resource *res;
>  	struct imxi2c_platform_data *pdata = pdev->dev.platform_data;
> +	struct imx_i2c_drvdata *drvdata;
>  	void __iomem *base;
>  	int irq, ret;
>  	u32 bitrate;
> @@ -519,7 +611,9 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
>  
>  	if (of_id)
>  		pdev->id_entry = of_id->data;

This is wrong in the original driver code. This field should be changed
in a driver.

> -	i2c_imx->devtype = pdev->id_entry->driver_data;
> +	drvdata = (struct imx_i2c_drvdata *)pdev->id_entry->driver_data;

Unnecessary cast.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Sascha Hauer]

On Fri, Aug 02, 2013 at 12:44:08PM +0800, Jingchang Lu wrote:
>   Add Freescale Vybrid VF610 I2C controller support to
> imx I2C driver framework.
>   Some operation is different from imx I2C controller.
> The register offset, the i2c clock divider value table,
> the module enabling(I2CR_IEN) which is just invert with imx,
> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
> but w0c on imx.
> 
> Signed-off-by: Jason Jin <Jason.jin@freescale.com>
> Signed-off-by: Xiaochun Li <b41219@freescale.com>
> Signed-off-by: Jingchang Lu <b35083@freescale.com>
> ---
> changes in v3:
>   Using struct naming the i2c clock {div, regval} pair.
>   Using address shift handling registers address difference.
> 
> changes in v2:
>   Fix building section mismatch(es) warning.
> 
>  drivers/i2c/busses/i2c-imx.c | 146 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 122 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index e242797..8f9981c 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -30,6 +30,8 @@
>   *	Copyright (C) 2007 RightHand Technologies, Inc.
>   *	Copyright (C) 2008 Darius Augulis <darius.augulis@teltonika.lt>
>   *
> + *	Copyright 2013 Freescale Semiconductor, Inc.
> + *
>   */
>  
>  /** Includes *******************************************************************
> @@ -62,13 +64,6 @@
>  /* Default value */
>  #define IMX_I2C_BIT_RATE	100000	/* 100kHz */
>  
> -/* IMX I2C registers */
> -#define IMX_I2C_IADR	0x00	/* i2c slave address */
> -#define IMX_I2C_IFDR	0x04	/* i2c frequency divider */
> -#define IMX_I2C_I2CR	0x08	/* i2c control */
> -#define IMX_I2C_I2SR	0x0C	/* i2c status */
> -#define IMX_I2C_I2DR	0x10	/* i2c transfer data */
> -
>  /* Bits of IMX I2C registers */
>  #define I2SR_RXAK	0x01
>  #define I2SR_IIF	0x02
> @@ -95,8 +90,12 @@
>   *
>   * Duplicated divider values removed from list
>   */
> +struct imx_i2c_clk_pair {
> +	u16	div;
> +	u16	regval;
> +}i;

Converting this to a struct should be a separate patch. This is an
obvious cleanup which makes maintainers happy and more willing to take
the features you are adding later.

>  
> -static u16 __initdata i2c_clk_div[50][2] = {
> +static struct imx_i2c_clk_pair imx_i2c_clk_div[] = {
>  	{ 22,	0x20 }, { 24,	0x21 }, { 26,	0x22 }, { 28,	0x23 },
>  	{ 30,	0x00 },	{ 32,	0x24 }, { 36,	0x25 }, { 40,	0x26 },
>  	{ 42,	0x03 }, { 44,	0x27 },	{ 48,	0x28 }, { 52,	0x05 },
> @@ -112,11 +111,63 @@ static u16 __initdata i2c_clk_div[50][2] = {
>  	{ 3072,	0x1E }, { 3840,	0x1F }
>  };
>  
> +/* Vybrid VF610 clock divider, register value pairs */
> +static struct imx_i2c_clk_pair vf610_i2c_clk_div[] = {
> +	{ 20,   0x00 }, { 22,   0x01 }, { 24,   0x02 }, { 26,   0x03 },
> +	{ 28,   0x04 }, { 30,   0x05 }, { 32,   0x09 }, { 34,   0x06 },
> +	{ 36,   0x0A }, { 40,   0x07 }, { 44,   0x0C }, { 48,   0x0D },
> +	{ 52,   0x43 }, { 56,   0x0E }, { 60,   0x45 }, { 64,   0x12 },
> +	{ 68,   0x0F }, { 72,   0x13 }, { 80,   0x14 }, { 88,   0x15 },
> +	{ 96,   0x19 }, { 104,  0x16 }, { 112,  0x1A }, { 128,  0x17 },
> +	{ 136,  0x4F }, { 144,  0x1C }, { 160,  0x1D }, { 176,  0x55 },
> +	{ 192,  0x1E }, { 208,  0x56 }, { 224,  0x22 }, { 228,  0x24 },
> +	{ 240,  0x1F }, { 256,  0x23 }, { 288,  0x5C }, { 320,  0x25 },
> +	{ 384,  0x26 }, { 448,  0x2A }, { 480,  0x27 }, { 512,  0x2B },
> +	{ 576,  0x2C }, { 640,  0x2D }, { 768,  0x31 }, { 896,  0x32 },
> +	{ 960,  0x2F }, { 1024, 0x33 }, { 1152, 0x34 }, { 1280, 0x35 },
> +	{ 1536, 0x36 }, { 1792, 0x3A }, { 1920, 0x37 }, { 2048, 0x3B },
> +	{ 2304, 0x3C }, { 2560, 0x3D }, { 3072, 0x3E }, { 3584, 0x7A },
> +	{ 3840, 0x3F }, { 4096, 0x7B }, { 5120, 0x7D }, { 6144, 0x7E },
> +};
> +
>  enum imx_i2c_type {
>  	IMX1_I2C,
>  	IMX21_I2C,
> +	VF610_I2C,
>  };
>  
> +struct imx_i2c_hwdata {
> +	unsigned		regshift;
> +	struct imx_i2c_clk_pair	*clk_div;
> +	unsigned		ndivs;
> +	unsigned		i2sr_clr_opcode;
> +	unsigned		i2cr_ien_opcode;
> +};
> +
> +struct imx_i2c_drvdata {
> +	enum imx_i2c_type	devtype;
> +	struct imx_i2c_hwdata	*hwdata;
> +};
> +
> +#define IMX_I2C_IADR	(0x00 << i2c_imx->i2c_hwdata->regshift)
> +#define IMX_I2C_IFDR	(0x01 << i2c_imx->i2c_hwdata->regshift)
> +#define IMX_I2C_I2CR	(0x02 << i2c_imx->i2c_hwdata->regshift)
> +#define IMX_I2C_I2SR	(0x03 << i2c_imx->i2c_hwdata->regshift)
> +#define IMX_I2C_I2DR	(0x04 << i2c_imx->i2c_hwdata->regshift)

Macros should not assume there is a i2c_imx variable present in a
function. Instead, add static inline accessor functions.

> +
> +/*
> + * Interrupt flags clear operation differ between SoCs:
> + * - write zero to clear(w0c) INT flag on I.MX,

It's i.MX. With a Freescale mail address you should get this right ;)

> + * - but write one to clear(w1c) INT flag on Vybrid.
> + * I2C module enable operation also differ bteween SoCs:

s/bteween/between/

> + * - set I2CR_IEN bit enable the module on I.MX,
> + * - but clear I2CR_IEN bit enable the module on Vybrid.
> + */
> +#define I2SR_CLR_OPCODE_W0C		0x0
> +#define I2SR_CLR_OPCODE_W1C		(I2SR_IAL | I2SR_IIF)
> +#define I2CR_IEN_OPCODE_0		0x0
> +#define I2CR_IEN_OPCODE_1		I2CR_IEN
> +
>  struct imx_i2c_struct {
>  	struct i2c_adapter	adapter;
>  	struct clk		*clk;
> @@ -127,15 +178,52 @@ struct imx_i2c_struct {
>  	int			stopped;
>  	unsigned int		ifdr; /* IMX_I2C_IFDR */
>  	enum imx_i2c_type	devtype;
> +	struct imx_i2c_hwdata	*i2c_hwdata;
> +};
> +
> +static struct imx_i2c_hwdata imx_i2c_hwdata = {
> +	.regshift		= 2,
> +	.clk_div		= imx_i2c_clk_div,
> +	.ndivs			= ARRAY_SIZE(imx_i2c_clk_div),
> +	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W0C,
> +	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_1,
> +
> +};

unnecessary blank line

> +
> +static struct imx_i2c_hwdata vf610_i2c_hwdata = {
> +	.regshift		= 0,
> +	.clk_div		= vf610_i2c_clk_div,
> +	.ndivs			= ARRAY_SIZE(vf610_i2c_clk_div),
> +	.i2sr_clr_opcode	= I2SR_CLR_OPCODE_W1C,
> +	.i2cr_ien_opcode	= I2CR_IEN_OPCODE_0,
> +
> +};

ditto

> +
> +static struct imx_i2c_drvdata imx_i2c_drvdata[] = {
> +	[IMX1_I2C] = {
> +		.devtype = IMX1_I2C,
> +		.hwdata = &imx_i2c_hwdata
> +	},
> +	[IMX21_I2C] = {
> +		.devtype = IMX21_I2C,
> +		.hwdata = &imx_i2c_hwdata
> +	},
> +	[VF610_I2C] = {
> +		.devtype = VF610_I2C,
> +		.hwdata = &vf610_i2c_hwdata
> +	},
>  };

Why add this array?

>  
>  static struct platform_device_id imx_i2c_devtype[] = {
>  	{
>  		.name = "imx1-i2c",
> -		.driver_data = IMX1_I2C,
> +		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[IMX1_I2C],

You should reference imx_i2c_hwdata/vf610_i2c_hwdata directly here.

>  	}, {
>  		.name = "imx21-i2c",
> -		.driver_data = IMX21_I2C,
> +		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[IMX21_I2C],
> +	}, {
> +		.name = "vf610-i2c",
> +		.driver_data = (kernel_ulong_t)&imx_i2c_drvdata[VF610_I2C],
>  	}, {
>  		/* sentinel */
>  	}
> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
>  static const struct of_device_id i2c_imx_dt_ids[] = {
>  	{ .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
>  	{ .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
> +	{ .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, i2c_imx_dt_ids);
> @@ -215,9 +304,8 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>  	clk_prepare_enable(i2c_imx->clk);
>  	writeb(i2c_imx->ifdr, i2c_imx->base + IMX_I2C_IFDR);
>  	/* Enable I2C controller */
> -	writeb(0, i2c_imx->base + IMX_I2C_I2SR);
> -	writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> -
> +	writeb(i2c_imx->i2c_hwdata->i2sr_clr_opcode, i2c_imx->base + IMX_I2C_I2SR);
> +	writeb(i2c_imx->i2c_hwdata->i2cr_ien_opcode, i2c_imx->base + IMX_I2C_I2CR);
>  	/* Wait controller to be stable */
>  	udelay(50);
>  
> @@ -260,7 +348,8 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
>  	}
>  
>  	/* Disable I2C controller */
> -	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
> +	writeb(i2c_imx->i2c_hwdata->i2cr_ien_opcode ^ I2CR_IEN,
> +						i2c_imx->base + IMX_I2C_I2CR);
>  	clk_disable_unprepare(i2c_imx->clk);
>  }
>  
> @@ -270,19 +359,20 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
>  	unsigned int i2c_clk_rate;
>  	unsigned int div;
>  	int i;
> +	struct imx_i2c_clk_pair *i2c_clk_div = i2c_imx->i2c_hwdata->clk_div;
>  
>  	/* Divider value calculation */
>  	i2c_clk_rate = clk_get_rate(i2c_imx->clk);
>  	div = (i2c_clk_rate + rate - 1) / rate;
> -	if (div < i2c_clk_div[0][0])
> +	if (div < i2c_clk_div[0].div)
>  		i = 0;
> -	else if (div > i2c_clk_div[ARRAY_SIZE(i2c_clk_div) - 1][0])
> -		i = ARRAY_SIZE(i2c_clk_div) - 1;
> +	else if (div > i2c_clk_div[i2c_imx->i2c_hwdata->ndivs - 1].div)
> +		i = i2c_imx->i2c_hwdata->ndivs - 1;
>  	else
> -		for (i = 0; i2c_clk_div[i][0] < div; i++);
> +		for (i = 0; i2c_clk_div[i].div < div; i++);
>  
>  	/* Store divider value */
> -	i2c_imx->ifdr = i2c_clk_div[i][1];
> +	i2c_imx->ifdr = i2c_clk_div[i].regval;
>  
>  	/*
>  	 * There dummy delay is calculated.
> @@ -290,7 +380,7 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
>  	 * This delay is used in I2C bus disable function
>  	 * to fix chip hardware bug.
>  	 */
> -	i2c_imx->disable_delay = (500000U * i2c_clk_div[i][0]
> +	i2c_imx->disable_delay = (500000U * i2c_clk_div[i].div
>  		+ (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2);
>  
>  	/* dev_dbg() can't be used, because adapter is not yet registered */
> @@ -298,7 +388,7 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s> I2C_CLK=%d, REQ DIV=%d\n",
>  		__func__, i2c_clk_rate, div);
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s> IFDR[IC]=0x%x, REAL DIV=%d\n",
> -		__func__, i2c_clk_div[i][1], i2c_clk_div[i][0]);
> +		__func__, i2c_clk_div[i].regval, i2c_clk_div[i].div);
>  #endif
>  }
>  
> @@ -312,6 +402,7 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  		/* save status register */
>  		i2c_imx->i2csr = temp;
>  		temp &= ~I2SR_IIF;
> +		temp |= (i2c_imx->i2c_hwdata->i2sr_clr_opcode & I2SR_IIF);
>  		writeb(temp, i2c_imx->base + IMX_I2C_I2SR);
>  		wake_up(&i2c_imx->queue);
>  		return IRQ_HANDLED;
> @@ -493,6 +584,7 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
>  	struct imx_i2c_struct *i2c_imx;
>  	struct resource *res;
>  	struct imxi2c_platform_data *pdata = pdev->dev.platform_data;
> +	struct imx_i2c_drvdata *drvdata;
>  	void __iomem *base;
>  	int irq, ret;
>  	u32 bitrate;
> @@ -519,7 +611,9 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
>  
>  	if (of_id)
>  		pdev->id_entry = of_id->data;

This is wrong in the original driver code. This field should be changed
in a driver.

> -	i2c_imx->devtype = pdev->id_entry->driver_data;
> +	drvdata = (struct imx_i2c_drvdata *)pdev->id_entry->driver_data;

Unnecessary cast.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Lu Jingchang-B35083]

> Subject: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller
> > +struct imx_i2c_clk_pair {
> > +	u16	div;
> > +	u16	regval;
> > +}i;
> 
> Converting this to a struct should be a separate patch. This is an
> obvious cleanup which makes maintainers happy and more willing to take
> the features you are adding later.
> 
[Lu Jingchang-B35083] 
Do you mean all changes to i.MX common should be a separate patch including the clk div pair, and 
the struct imx_i2c_hwdata, struct imx_i2c_drvdata and other things changed to be easier to
use adding other SoCs support later. Thanks.
> >
> > +#define IMX_I2C_IADR	(0x00 << i2c_imx->i2c_hwdata->regshift)
> > +#define IMX_I2C_IFDR	(0x01 << i2c_imx->i2c_hwdata->regshift)
> > +#define IMX_I2C_I2CR	(0x02 << i2c_imx->i2c_hwdata->regshift)
> > +#define IMX_I2C_I2SR	(0x03 << i2c_imx->i2c_hwdata->regshift)
> > +#define IMX_I2C_I2DR	(0x04 << i2c_imx->i2c_hwdata->regshift)
> 
> Macros should not assume there is a i2c_imx variable present in a
> function. Instead, add static inline accessor functions.
[Lu Jingchang-B35083] 
Ok, I will add inline functions instead. Thanks.
> 
> 
> > +	[VF610_I2C] = {
> > +		.devtype = VF610_I2C,
> > +		.hwdata = &vf610_i2c_hwdata
> > +	},
> >  };
> 
> Why add this array?
> You should reference imx_i2c_hwdata/vf610_i2c_hwdata directly here.
[Lu Jingchang-B35083] 
The "is_imx1_i2c()"function need the devtype variable the test the arch, so the devtype is out of the
imx_i2c_hwdata struct to let all other i.MX i2c device can sharing the same imx_i2c_hwdata setting. Thanks.
> 
> >
> >  	if (of_id)
> >  		pdev->id_entry = of_id->data;
> 
> This is wrong in the original driver code. This field should be changed
> in a driver.
[Lu Jingchang-B35083] 
Could you please give some more comments on this? Thanks.



Best Regards

[PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Lu Jingchang-B35083]

> Subject: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller
> > +struct imx_i2c_clk_pair {
> > +	u16	div;
> > +	u16	regval;
> > +}i;
> 
> Converting this to a struct should be a separate patch. This is an
> obvious cleanup which makes maintainers happy and more willing to take
> the features you are adding later.
> 
[Lu Jingchang-B35083] 
Do you mean all changes to i.MX common should be a separate patch including the clk div pair, and 
the struct imx_i2c_hwdata, struct imx_i2c_drvdata and other things changed to be easier to
use adding other SoCs support later. Thanks.
> >
> > +#define IMX_I2C_IADR	(0x00 << i2c_imx->i2c_hwdata->regshift)
> > +#define IMX_I2C_IFDR	(0x01 << i2c_imx->i2c_hwdata->regshift)
> > +#define IMX_I2C_I2CR	(0x02 << i2c_imx->i2c_hwdata->regshift)
> > +#define IMX_I2C_I2SR	(0x03 << i2c_imx->i2c_hwdata->regshift)
> > +#define IMX_I2C_I2DR	(0x04 << i2c_imx->i2c_hwdata->regshift)
> 
> Macros should not assume there is a i2c_imx variable present in a
> function. Instead, add static inline accessor functions.
[Lu Jingchang-B35083] 
Ok, I will add inline functions instead. Thanks.
> 
> 
> > +	[VF610_I2C] = {
> > +		.devtype = VF610_I2C,
> > +		.hwdata = &vf610_i2c_hwdata
> > +	},
> >  };
> 
> Why add this array?
> You should reference imx_i2c_hwdata/vf610_i2c_hwdata directly here.
[Lu Jingchang-B35083] 
The "is_imx1_i2c()"function need the devtype variable the test the arch, so the devtype is out of the
imx_i2c_hwdata struct to let all other i.MX i2c device can sharing the same imx_i2c_hwdata setting. Thanks.
> 
> >
> >  	if (of_id)
> >  		pdev->id_entry = of_id->data;
> 
> This is wrong in the original driver code. This field should be changed
> in a driver.
[Lu Jingchang-B35083] 
Could you please give some more comments on this? Thanks.



Best Regards

Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Sascha Hauer]

On Mon, Aug 05, 2013 at 09:32:11AM +0000, Lu Jingchang-B35083 wrote:
> > Subject: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller
> > > +struct imx_i2c_clk_pair {
> > > +	u16	div;
> > > +	u16	regval;
> > > +}i;
> > 
> > Converting this to a struct should be a separate patch. This is an
> > obvious cleanup which makes maintainers happy and more willing to take
> > the features you are adding later.
> > 
> [Lu Jingchang-B35083] 
> Do you mean all changes to i.MX common should be a separate patch including the clk div pair, and 
> the struct imx_i2c_hwdata, struct imx_i2c_drvdata and other things changed to be easier to
> use adding other SoCs support later. Thanks.

I mean that each patch should only contain a single logical change.

Converting the two dimensional array into an array of struct is such a
logical change and thus should be a separate patch.

The smaller a patch is the easier it is for people to review it.

> > >
> > > +#define IMX_I2C_IADR	(0x00 << i2c_imx->i2c_hwdata->regshift)
> > > +#define IMX_I2C_IFDR	(0x01 << i2c_imx->i2c_hwdata->regshift)
> > > +#define IMX_I2C_I2CR	(0x02 << i2c_imx->i2c_hwdata->regshift)
> > > +#define IMX_I2C_I2SR	(0x03 << i2c_imx->i2c_hwdata->regshift)
> > > +#define IMX_I2C_I2DR	(0x04 << i2c_imx->i2c_hwdata->regshift)
> > 
> > Macros should not assume there is a i2c_imx variable present in a
> > function. Instead, add static inline accessor functions.
> [Lu Jingchang-B35083] 
> Ok, I will add inline functions instead. Thanks.
> > 
> > 
> > > +	[VF610_I2C] = {
> > > +		.devtype = VF610_I2C,
> > > +		.hwdata = &vf610_i2c_hwdata
> > > +	},
> > >  };
> > 
> > Why add this array?
> > You should reference imx_i2c_hwdata/vf610_i2c_hwdata directly here.
> [Lu Jingchang-B35083] 
> The "is_imx1_i2c()"function need the devtype variable the test the arch, so the devtype is out of the
> imx_i2c_hwdata struct to let all other i.MX i2c device can sharing the same imx_i2c_hwdata setting. Thanks.

Then add the devtype variable to struct imx_i2c_hwdata and split
imx_i2c_hwdata into imx1_i2c_hwdata and imx21_i2c_hwdata.

> > 
> > >
> > >  	if (of_id)
> > >  		pdev->id_entry = of_id->data;
> > 
> > This is wrong in the original driver code. This field should be changed
> > in a driver.
> [Lu Jingchang-B35083] 
> Could you please give some more comments on this? Thanks.

It's not the drivers business of changing pdev->id_entry. This should
only be modified by the driver core.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Sascha Hauer]

On Mon, Aug 05, 2013 at 09:32:11AM +0000, Lu Jingchang-B35083 wrote:
> > Subject: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller
> > > +struct imx_i2c_clk_pair {
> > > +	u16	div;
> > > +	u16	regval;
> > > +}i;
> > 
> > Converting this to a struct should be a separate patch. This is an
> > obvious cleanup which makes maintainers happy and more willing to take
> > the features you are adding later.
> > 
> [Lu Jingchang-B35083] 
> Do you mean all changes to i.MX common should be a separate patch including the clk div pair, and 
> the struct imx_i2c_hwdata, struct imx_i2c_drvdata and other things changed to be easier to
> use adding other SoCs support later. Thanks.

I mean that each patch should only contain a single logical change.

Converting the two dimensional array into an array of struct is such a
logical change and thus should be a separate patch.

The smaller a patch is the easier it is for people to review it.

> > >
> > > +#define IMX_I2C_IADR	(0x00 << i2c_imx->i2c_hwdata->regshift)
> > > +#define IMX_I2C_IFDR	(0x01 << i2c_imx->i2c_hwdata->regshift)
> > > +#define IMX_I2C_I2CR	(0x02 << i2c_imx->i2c_hwdata->regshift)
> > > +#define IMX_I2C_I2SR	(0x03 << i2c_imx->i2c_hwdata->regshift)
> > > +#define IMX_I2C_I2DR	(0x04 << i2c_imx->i2c_hwdata->regshift)
> > 
> > Macros should not assume there is a i2c_imx variable present in a
> > function. Instead, add static inline accessor functions.
> [Lu Jingchang-B35083] 
> Ok, I will add inline functions instead. Thanks.
> > 
> > 
> > > +	[VF610_I2C] = {
> > > +		.devtype = VF610_I2C,
> > > +		.hwdata = &vf610_i2c_hwdata
> > > +	},
> > >  };
> > 
> > Why add this array?
> > You should reference imx_i2c_hwdata/vf610_i2c_hwdata directly here.
> [Lu Jingchang-B35083] 
> The "is_imx1_i2c()"function need the devtype variable the test the arch, so the devtype is out of the
> imx_i2c_hwdata struct to let all other i.MX i2c device can sharing the same imx_i2c_hwdata setting. Thanks.

Then add the devtype variable to struct imx_i2c_hwdata and split
imx_i2c_hwdata into imx1_i2c_hwdata and imx21_i2c_hwdata.

> > 
> > >
> > >  	if (of_id)
> > >  		pdev->id_entry = of_id->data;
> > 
> > This is wrong in the original driver code. This field should be changed
> > in a driver.
> [Lu Jingchang-B35083] 
> Could you please give some more comments on this? Thanks.

It's not the drivers business of changing pdev->id_entry. This should
only be modified by the driver core.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Lu Jingchang-B35083]

> > > + [VF610_I2C] = {
> > > +         .devtype = VF610_I2C,
> > > +         .hwdata = &vf610_i2c_hwdata
> > > + },
> > >  };
> >
> > Why add this array?
> > You should reference imx_i2c_hwdata/vf610_i2c_hwdata directly here.
[Lu Jingchang-B35083]
The imx_i2c_devtype[] array is used to support platform that doesn't support dts currently. So the array is added to make the driver consistent. Thanks.

> >
> > >
> > >   if (of_id)
> > >           pdev->id_entry = of_id->data;
> >
> > This is wrong in the original driver code. This field should be changed
> > in a driver.
 [Lu Jingchang-B35083]
The assignment to id_entry is due to support both dts device and non-dts devices, just as you said, maybe this field shouldn't be changed out of platform driver core,
so I will try to change this but make the drive for both dts and non-dts still work well. Thanks.



Best Regards,
Jingchang

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Lu Jingchang-B35083]

> > > + [VF610_I2C] = {
> > > +         .devtype = VF610_I2C,
> > > +         .hwdata = &vf610_i2c_hwdata
> > > + },
> > >  };
> >
> > Why add this array?
> > You should reference imx_i2c_hwdata/vf610_i2c_hwdata directly here.
[Lu Jingchang-B35083]
The imx_i2c_devtype[] array is used to support platform that doesn't support dts currently. So the array is added to make the driver consistent. Thanks.

> >
> > >
> > >   if (of_id)
> > >           pdev->id_entry = of_id->data;
> >
> > This is wrong in the original driver code. This field should be changed
> > in a driver.
 [Lu Jingchang-B35083]
The assignment to id_entry is due to support both dts device and non-dts devices, just as you said, maybe this field shouldn't be changed out of platform driver core,
so I will try to change this but make the drive for both dts and non-dts still work well. Thanks.



Best Regards,
Jingchang

Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Mark Rutland]

On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
>   Add Freescale Vybrid VF610 I2C controller support to
> imx I2C driver framework.
>   Some operation is different from imx I2C controller.
> The register offset, the i2c clock divider value table,
> the module enabling(I2CR_IEN) which is just invert with imx,
> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
> but w0c on imx.
> 
> Signed-off-by: Jason Jin <Jason.jin-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Signed-off-by: Xiaochun Li <b41219-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
> changes in v3:
>   Using struct naming the i2c clock {div, regval} pair.
>   Using address shift handling registers address difference.
> 
> changes in v2:
>   Fix building section mismatch(es) warning.
> 
>  drivers/i2c/busses/i2c-imx.c | 146 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 122 insertions(+), 24 deletions(-)

[...]

> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
>  static const struct of_device_id i2c_imx_dt_ids[] = {
>         { .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
>         { .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
> +       { .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
>         { /* sentinel */ }
>  };


That string doesn't seem to be documented anywhere (from a quick grep of
Documentation/devicetree), and there's no binding update included
here. It would be nice for that to be fixed :)

Thanks,
Mark.

[PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Mark Rutland]

On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
>   Add Freescale Vybrid VF610 I2C controller support to
> imx I2C driver framework.
>   Some operation is different from imx I2C controller.
> The register offset, the i2c clock divider value table,
> the module enabling(I2CR_IEN) which is just invert with imx,
> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
> but w0c on imx.
> 
> Signed-off-by: Jason Jin <Jason.jin@freescale.com>
> Signed-off-by: Xiaochun Li <b41219@freescale.com>
> Signed-off-by: Jingchang Lu <b35083@freescale.com>
> ---
> changes in v3:
>   Using struct naming the i2c clock {div, regval} pair.
>   Using address shift handling registers address difference.
> 
> changes in v2:
>   Fix building section mismatch(es) warning.
> 
>  drivers/i2c/busses/i2c-imx.c | 146 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 122 insertions(+), 24 deletions(-)

[...]

> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
>  static const struct of_device_id i2c_imx_dt_ids[] = {
>         { .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
>         { .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
> +       { .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
>         { /* sentinel */ }
>  };


That string doesn't seem to be documented anywhere (from a quick grep of
Documentation/devicetree), and there's no binding update included
here. It would be nice for that to be fixed :)

Thanks,
Mark.

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Lu Jingchang-B35083]

________________________________________
>发件人: Mark Rutland [mark.rutland@arm.com]
>发送时间: 2013年8月10日 22:08
>收件人: Lu Jingchang-B35083
>抄送: wsa@the-dreams.de; Estevam Fabio-R49496; Li Xiaochun-B41219; s.hauer@pengutronix.de; linux-i2c@vger.kernel.org; Jin Zhengxiong-R64188; shawn.guo@linaro.org; linux-arm-kernel@lists.infradead.org
>主题: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

>On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
>>   Add Freescale Vybrid VF610 I2C controller support to
>> imx I2C driver framework.
>>   Some operation is different from imx I2C controller.
>> The register offset, the i2c clock divider value table,
>> the module enabling(I2CR_IEN) which is just invert with imx,
>> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
>> but w0c on imx.
>>
>> Signed-off-by: Jason Jin <Jason.jin@freescale.com>
>> Signed-off-by: Xiaochun Li <b41219@freescale.com>
>> Signed-off-by: Jingchang Lu <b35083@freescale.com>
>> ---
>> changes in v3:
>>   Using struct naming the i2c clock {div, regval} pair.
>>  Using address shift handling registers address difference.
>>
>> changes in v2:
>>   Fix building section mismatch(es) warning.
>>
>>  drivers/i2c/busses/i2c-imx.c | 146 ++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 122 insertions(+), 24 deletions(-)

>[...]

>> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
>>  static const struct of_device_id i2c_imx_dt_ids[] = {
>>         { .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
>>         { .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
>> +       { .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
>>         { /* sentinel */ }
>>  };


>That string doesn't seem to be documented anywhere (from a quick grep of
>Documentation/devicetree), and there's no binding update included
>here. It would be nice for that to be fixed :)
[Lu Jingchang]
The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
is described in the binding document. So I just leave the vf610 i2c compatible with this. 
Thanks.


Best Regards,
Jingchang

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Lu Jingchang-B35083]

________________________________________
>???: Mark Rutland [mark.rutland at arm.com]
>????: 2013?8?10? 22:08
>???: Lu Jingchang-B35083
>??: wsa at the-dreams.de; Estevam Fabio-R49496; Li Xiaochun-B41219; s.hauer at pengutronix.de; linux-i2c at vger.kernel.org; Jin Zhengxiong-R64188; shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org
>??: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

>On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
>>   Add Freescale Vybrid VF610 I2C controller support to
>> imx I2C driver framework.
>>   Some operation is different from imx I2C controller.
>> The register offset, the i2c clock divider value table,
>> the module enabling(I2CR_IEN) which is just invert with imx,
>> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
>> but w0c on imx.
>>
>> Signed-off-by: Jason Jin <Jason.jin@freescale.com>
>> Signed-off-by: Xiaochun Li <b41219@freescale.com>
>> Signed-off-by: Jingchang Lu <b35083@freescale.com>
>> ---
>> changes in v3:
>>   Using struct naming the i2c clock {div, regval} pair.
>>  Using address shift handling registers address difference.
>>
>> changes in v2:
>>   Fix building section mismatch(es) warning.
>>
>>  drivers/i2c/busses/i2c-imx.c | 146 ++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 122 insertions(+), 24 deletions(-)

>[...]

>> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
>>  static const struct of_device_id i2c_imx_dt_ids[] = {
>>         { .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
>>         { .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
>> +       { .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
>>         { /* sentinel */ }
>>  };


>That string doesn't seem to be documented anywhere (from a quick grep of
>Documentation/devicetree), and there's no binding update included
>here. It would be nice for that to be fixed :)
[Lu Jingchang]
The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
is described in the binding document. So I just leave the vf610 i2c compatible with this. 
Thanks.


Best Regards,
Jingchang

Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Mark Rutland]

[Adding other devicetree maintainers to Cc]

On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
> 
> ________________________________________
> >发件人: Mark Rutland [mark.rutland-5wv7dgnIgG8@public.gmane.org]
> >发送时间: 2013年8月10日 22:08
> >收件人: Lu Jingchang-B35083
> >抄送: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org; Estevam Fabio-R49496; Li Xiaochun-B41219; s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Jin Zhengxiong-R64188; shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XQ@public.gmane.orgorg
> >主题: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
> 
> >On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
> >>   Add Freescale Vybrid VF610 I2C controller support to
> >> imx I2C driver framework.
> >>   Some operation is different from imx I2C controller.
> >> The register offset, the i2c clock divider value table,
> >> the module enabling(I2CR_IEN) which is just invert with imx,
> >> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
> >> but w0c on imx.
> >>
> >> Signed-off-by: Jason Jin <Jason.jin-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >> Signed-off-by: Xiaochun Li <b41219-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >> Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >> ---
> >> changes in v3:
> >>   Using struct naming the i2c clock {div, regval} pair.
> >>  Using address shift handling registers address difference.
> >>
> >> changes in v2:
> >>   Fix building section mismatch(es) warning.
> >>
> >>  drivers/i2c/busses/i2c-imx.c | 146 ++++++++++++++++++++++++++++++++++++-------
> >>  1 file changed, 122 insertions(+), 24 deletions(-)
> 
> >[...]
> 
> >> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
> >>  static const struct of_device_id i2c_imx_dt_ids[] = {
> >>         { .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
> >>         { .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
> >> +       { .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
> >>         { /* sentinel */ }
> >>  };
> 
> 
> >That string doesn't seem to be documented anywhere (from a quick grep of
> >Documentation/devicetree), and there's no binding update included
> >here. It would be nice for that to be fixed :)
> [Lu Jingchang]
> The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
> of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
> is described in the binding document. So I just leave the vf610 i2c compatible with this. 

I'm not a big fan on wildcards in bindings, as it leaves people free to
put anything in and claim it's a documented binding, and makes it far
harder for an os to actually implement drivers for said binding, as
there's no canonical reference for the set of valid variations.

Obviously there is some precedent, but I'm not sure it's something we
want to stick with, and we can prevent it my updating the documentation
now.

Does anyone else have an opinion?

Mark.

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Mark Rutland]

[Adding other devicetree maintainers to Cc]

On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
> 
> ________________________________________
> >???: Mark Rutland [mark.rutland at arm.com]
> >????: 2013?8?10? 22:08
> >???: Lu Jingchang-B35083
> >??: wsa at the-dreams.de; Estevam Fabio-R49496; Li Xiaochun-B41219; s.hauer at pengutronix.de; linux-i2c at vger.kernel.org; Jin Zhengxiong-R64188; shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org
> >??: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
> 
> >On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
> >>   Add Freescale Vybrid VF610 I2C controller support to
> >> imx I2C driver framework.
> >>   Some operation is different from imx I2C controller.
> >> The register offset, the i2c clock divider value table,
> >> the module enabling(I2CR_IEN) which is just invert with imx,
> >> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
> >> but w0c on imx.
> >>
> >> Signed-off-by: Jason Jin <Jason.jin@freescale.com>
> >> Signed-off-by: Xiaochun Li <b41219@freescale.com>
> >> Signed-off-by: Jingchang Lu <b35083@freescale.com>
> >> ---
> >> changes in v3:
> >>   Using struct naming the i2c clock {div, regval} pair.
> >>  Using address shift handling registers address difference.
> >>
> >> changes in v2:
> >>   Fix building section mismatch(es) warning.
> >>
> >>  drivers/i2c/busses/i2c-imx.c | 146 ++++++++++++++++++++++++++++++++++++-------
> >>  1 file changed, 122 insertions(+), 24 deletions(-)
> 
> >[...]
> 
> >> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
> >>  static const struct of_device_id i2c_imx_dt_ids[] = {
> >>         { .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
> >>         { .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
> >> +       { .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
> >>         { /* sentinel */ }
> >>  };
> 
> 
> >That string doesn't seem to be documented anywhere (from a quick grep of
> >Documentation/devicetree), and there's no binding update included
> >here. It would be nice for that to be fixed :)
> [Lu Jingchang]
> The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
> of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
> is described in the binding document. So I just leave the vf610 i2c compatible with this. 

I'm not a big fan on wildcards in bindings, as it leaves people free to
put anything in and claim it's a documented binding, and makes it far
harder for an os to actually implement drivers for said binding, as
there's no canonical reference for the set of valid variations.

Obviously there is some precedent, but I'm not sure it's something we
want to stick with, and we can prevent it my updating the documentation
now.

Does anyone else have an opinion?

Mark.

Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Tomasz Figa]

On Monday 12 of August 2013 17:43:54 Mark Rutland wrote:
> [Adding other devicetree maintainers to Cc]
> 
> On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
> > ________________________________________
> > 
> > >发件人: Mark Rutland [mark.rutland-5wv7dgnIgG8@public.gmane.org]
> > >发送时间: 2013年8月10日 22:08
> > >收件人: Lu Jingchang-B35083
> > >抄送: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org; Estevam Fabio-R49496; Li Xiaochun-B41219;
> > >s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Jin
> > >Zhengxiong-R64188; shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org;
> > >linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org 主题: Re: [PATCH v3 2/2] i2c:
> > >imx: Add Vybrid VF610 I2C controller support> >
> > >On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
> > >>   Add Freescale Vybrid VF610 I2C controller support to
> > >> 
> > >> imx I2C driver framework.
> > >> 
> > >>   Some operation is different from imx I2C controller.
> > >> 
> > >> The register offset, the i2c clock divider value table,
> > >> the module enabling(I2CR_IEN) which is just invert with imx,
> > >> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
> > >> but w0c on imx.
> > >> 
> > >> Signed-off-by: Jason Jin <Jason.jin-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > >> Signed-off-by: Xiaochun Li <b41219-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > >> Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > >> ---
> > >> 
> > >> changes in v3:
> > >>   Using struct naming the i2c clock {div, regval} pair.
> > >>  
> > >>  Using address shift handling registers address difference.
> > >> 
> > >> changes in v2:
> > >>   Fix building section mismatch(es) warning.
> > >>  
> > >>  drivers/i2c/busses/i2c-imx.c | 146
> > >>  ++++++++++++++++++++++++++++++++++++------- 1 file changed, 122
> > >>  insertions(+), 24 deletions(-)
> > >
> > >[...]
> > >
> > >> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
> > >> 
> > >>  static const struct of_device_id i2c_imx_dt_ids[] = {
> > >>  
> > >>         { .compatible = "fsl,imx1-i2c", .data =
> > >>         &imx_i2c_devtype[IMX1_I2C], },
> > >>         { .compatible = "fsl,imx21-i2c", .data =
> > >>         &imx_i2c_devtype[IMX21_I2C], },> >> 
> > >> +       { .compatible = "fsl,vf610-i2c", .data =
> > >> &imx_i2c_devtype[VF610_I2C], },> >> 
> > >>         { /* sentinel */ }
> > >>  
> > >>  };
> > >
> > >That string doesn't seem to be documented anywhere (from a quick grep
> > >of Documentation/devicetree), and there's no binding update included
> > >here. It would be nice for that to be fixed :)
> > 
> > [Lu Jingchang]
> > The binding string for i2c-imx driver in
> > Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard
> > format of "- compatible : Should be "fsl,<chip>-i2c" " for device
> > using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c is
> > described in the binding document. So I just leave the vf610 i2c
> > compatible with this.
> I'm not a big fan on wildcards in bindings, as it leaves people free to
> put anything in and claim it's a documented binding, and makes it far
> harder for an os to actually implement drivers for said binding, as
> there's no canonical reference for the set of valid variations.
> 
> Obviously there is some precedent, but I'm not sure it's something we
> want to stick with, and we can prevent it my updating the documentation
> now.
> 
> Does anyone else have an opinion?

In case of Samsung platforms we decided to always use the name of first 
SoC in which given IP appeared and list all compatible SoCs in binding 
documentation. This is IMHO the most consistent way, as there is no 
confusion about IP versions (not always listed in documentation, not even 
saying about unavailable documentation) or potential problems with new 
SoCs matching the wildcard, but having different IP.

In this particular case, the <chip> wildcard can be easily transformed 
into a non-wildcard binding, by listing all supported <chip> values, i.e. 
adding following text to binding documentation:

8<--
The <chip> can be one of:
 - imx1 - for i.MX1 and compatible SoCs,
 - imx21 - for i.MX21 and compatible SoCs,
 - vf610 - for Vybrid VF610 and compatible SoCs.
-->8

Best regards,
Tomasz

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Tomasz Figa]

On Monday 12 of August 2013 17:43:54 Mark Rutland wrote:
> [Adding other devicetree maintainers to Cc]
> 
> On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
> > ________________________________________
> > 
> > >???: Mark Rutland [mark.rutland at arm.com]
> > >????: 2013?8?10? 22:08
> > >???: Lu Jingchang-B35083
> > >??: wsa at the-dreams.de; Estevam Fabio-R49496; Li Xiaochun-B41219;
> > >s.hauer at pengutronix.de; linux-i2c at vger.kernel.org; Jin
> > >Zhengxiong-R64188; shawn.guo at linaro.org;
> > >linux-arm-kernel at lists.infradead.org ??: Re: [PATCH v3 2/2] i2c:
> > >imx: Add Vybrid VF610 I2C controller support> >
> > >On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
> > >>   Add Freescale Vybrid VF610 I2C controller support to
> > >> 
> > >> imx I2C driver framework.
> > >> 
> > >>   Some operation is different from imx I2C controller.
> > >> 
> > >> The register offset, the i2c clock divider value table,
> > >> the module enabling(I2CR_IEN) which is just invert with imx,
> > >> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
> > >> but w0c on imx.
> > >> 
> > >> Signed-off-by: Jason Jin <Jason.jin@freescale.com>
> > >> Signed-off-by: Xiaochun Li <b41219@freescale.com>
> > >> Signed-off-by: Jingchang Lu <b35083@freescale.com>
> > >> ---
> > >> 
> > >> changes in v3:
> > >>   Using struct naming the i2c clock {div, regval} pair.
> > >>  
> > >>  Using address shift handling registers address difference.
> > >> 
> > >> changes in v2:
> > >>   Fix building section mismatch(es) warning.
> > >>  
> > >>  drivers/i2c/busses/i2c-imx.c | 146
> > >>  ++++++++++++++++++++++++++++++++++++------- 1 file changed, 122
> > >>  insertions(+), 24 deletions(-)
> > >
> > >[...]
> > >
> > >> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
> > >> 
> > >>  static const struct of_device_id i2c_imx_dt_ids[] = {
> > >>  
> > >>         { .compatible = "fsl,imx1-i2c", .data =
> > >>         &imx_i2c_devtype[IMX1_I2C], },
> > >>         { .compatible = "fsl,imx21-i2c", .data =
> > >>         &imx_i2c_devtype[IMX21_I2C], },> >> 
> > >> +       { .compatible = "fsl,vf610-i2c", .data =
> > >> &imx_i2c_devtype[VF610_I2C], },> >> 
> > >>         { /* sentinel */ }
> > >>  
> > >>  };
> > >
> > >That string doesn't seem to be documented anywhere (from a quick grep
> > >of Documentation/devicetree), and there's no binding update included
> > >here. It would be nice for that to be fixed :)
> > 
> > [Lu Jingchang]
> > The binding string for i2c-imx driver in
> > Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard
> > format of "- compatible : Should be "fsl,<chip>-i2c" " for device
> > using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c is
> > described in the binding document. So I just leave the vf610 i2c
> > compatible with this.
> I'm not a big fan on wildcards in bindings, as it leaves people free to
> put anything in and claim it's a documented binding, and makes it far
> harder for an os to actually implement drivers for said binding, as
> there's no canonical reference for the set of valid variations.
> 
> Obviously there is some precedent, but I'm not sure it's something we
> want to stick with, and we can prevent it my updating the documentation
> now.
> 
> Does anyone else have an opinion?

In case of Samsung platforms we decided to always use the name of first 
SoC in which given IP appeared and list all compatible SoCs in binding 
documentation. This is IMHO the most consistent way, as there is no 
confusion about IP versions (not always listed in documentation, not even 
saying about unavailable documentation) or potential problems with new 
SoCs matching the wildcard, but having different IP.

In this particular case, the <chip> wildcard can be easily transformed 
into a non-wildcard binding, by listing all supported <chip> values, i.e. 
adding following text to binding documentation:

8<--
The <chip> can be one of:
 - imx1 - for i.MX1 and compatible SoCs,
 - imx21 - for i.MX21 and compatible SoCs,
 - vf610 - for Vybrid VF610 and compatible SoCs.
-->8

Best regards,
Tomasz

Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Kumar Gala]

On Aug 12, 2013, at 11:59 AM, Tomasz Figa wrote:

> On Monday 12 of August 2013 17:43:54 Mark Rutland wrote:
>> [Adding other devicetree maintainers to Cc]
>> 
>> On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
>>> ________________________________________
>>> 
>>>> 发件人: Mark Rutland [mark.rutland-5wv7dgnIgG8@public.gmane.org]
>>>> 发送时间: 2013年8月10日 22:08
>>>> 收件人: Lu Jingchang-B35083
>>>> 抄送: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org; Estevam Fabio-R49496; Li Xiaochun-B41219;
>>>> s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Jin
>>>> Zhengxiong-R64188; shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org;
>>>> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org 主题: Re: [PATCH v3 2/2] i2c:
>>>> imx: Add Vybrid VF610 I2C controller support> >
>>>> On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
>>>>>  Add Freescale Vybrid VF610 I2C controller support to
>>>>> 
>>>>> imx I2C driver framework.
>>>>> 
>>>>>  Some operation is different from imx I2C controller.
>>>>> 
>>>>> The register offset, the i2c clock divider value table,
>>>>> the module enabling(I2CR_IEN) which is just invert with imx,
>>>>> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
>>>>> but w0c on imx.
>>>>> 
>>>>> Signed-off-by: Jason Jin <Jason.jin-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>> Signed-off-by: Xiaochun Li <b41219-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>> Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>> ---
>>>>> 
>>>>> changes in v3:
>>>>>  Using struct naming the i2c clock {div, regval} pair.
>>>>> 
>>>>> Using address shift handling registers address difference.
>>>>> 
>>>>> changes in v2:
>>>>>  Fix building section mismatch(es) warning.
>>>>> 
>>>>> drivers/i2c/busses/i2c-imx.c | 146
>>>>> ++++++++++++++++++++++++++++++++++++------- 1 file changed, 122
>>>>> insertions(+), 24 deletions(-)
>>>> 
>>>> [...]
>>>> 
>>>>> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
>>>>> 
>>>>> static const struct of_device_id i2c_imx_dt_ids[] = {
>>>>> 
>>>>>        { .compatible = "fsl,imx1-i2c", .data =
>>>>>        &imx_i2c_devtype[IMX1_I2C], },
>>>>>        { .compatible = "fsl,imx21-i2c", .data =
>>>>>        &imx_i2c_devtype[IMX21_I2C], },> >> 
>>>>> +       { .compatible = "fsl,vf610-i2c", .data =
>>>>> &imx_i2c_devtype[VF610_I2C], },> >> 
>>>>>        { /* sentinel */ }
>>>>> 
>>>>> };
>>>> 
>>>> That string doesn't seem to be documented anywhere (from a quick grep
>>>> of Documentation/devicetree), and there's no binding update included
>>>> here. It would be nice for that to be fixed :)
>>> 
>>> [Lu Jingchang]
>>> The binding string for i2c-imx driver in
>>> Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard
>>> format of "- compatible : Should be "fsl,<chip>-i2c" " for device
>>> using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c is
>>> described in the binding document. So I just leave the vf610 i2c
>>> compatible with this.
>> I'm not a big fan on wildcards in bindings, as it leaves people free to
>> put anything in and claim it's a documented binding, and makes it far
>> harder for an os to actually implement drivers for said binding, as
>> there's no canonical reference for the set of valid variations.
>> 
>> Obviously there is some precedent, but I'm not sure it's something we
>> want to stick with, and we can prevent it my updating the documentation
>> now.
>> 
>> Does anyone else have an opinion?
> 
> In case of Samsung platforms we decided to always use the name of first 
> SoC in which given IP appeared and list all compatible SoCs in binding 
> documentation. This is IMHO the most consistent way, as there is no 
> confusion about IP versions (not always listed in documentation, not even 
> saying about unavailable documentation) or potential problems with new 
> SoCs matching the wildcard, but having different IP.
> 
> In this particular case, the <chip> wildcard can be easily transformed 
> into a non-wildcard binding, by listing all supported <chip> values, i.e. 
> adding following text to binding documentation:
> 
> 8<--
> The <chip> can be one of:
> - imx1 - for i.MX1 and compatible SoCs,
> - imx21 - for i.MX21 and compatible SoCs,
> - vf610 - for Vybrid VF610 and compatible SoCs.
> -->8
> 
> Best regards,
> Tomasz


One way to handle this is to introduce a SoC Family <chip> name doc with the possible list of names and than reference that doc in the binding.  Kinda like how we have vendor-prefixes.txt.

And maybe instead of doing <chip> its <imx-chip> or some reference that isn't too generic such that in the future a .dts could be validated.

- k

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Kumar Gala]

On Aug 12, 2013, at 11:59 AM, Tomasz Figa wrote:

> On Monday 12 of August 2013 17:43:54 Mark Rutland wrote:
>> [Adding other devicetree maintainers to Cc]
>> 
>> On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
>>> ________________________________________
>>> 
>>>> ???: Mark Rutland [mark.rutland at arm.com]
>>>> ????: 2013?8?10? 22:08
>>>> ???: Lu Jingchang-B35083
>>>> ??: wsa at the-dreams.de; Estevam Fabio-R49496; Li Xiaochun-B41219;
>>>> s.hauer at pengutronix.de; linux-i2c at vger.kernel.org; Jin
>>>> Zhengxiong-R64188; shawn.guo at linaro.org;
>>>> linux-arm-kernel at lists.infradead.org ??: Re: [PATCH v3 2/2] i2c:
>>>> imx: Add Vybrid VF610 I2C controller support> >
>>>> On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
>>>>>  Add Freescale Vybrid VF610 I2C controller support to
>>>>> 
>>>>> imx I2C driver framework.
>>>>> 
>>>>>  Some operation is different from imx I2C controller.
>>>>> 
>>>>> The register offset, the i2c clock divider value table,
>>>>> the module enabling(I2CR_IEN) which is just invert with imx,
>>>>> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
>>>>> but w0c on imx.
>>>>> 
>>>>> Signed-off-by: Jason Jin <Jason.jin@freescale.com>
>>>>> Signed-off-by: Xiaochun Li <b41219@freescale.com>
>>>>> Signed-off-by: Jingchang Lu <b35083@freescale.com>
>>>>> ---
>>>>> 
>>>>> changes in v3:
>>>>>  Using struct naming the i2c clock {div, regval} pair.
>>>>> 
>>>>> Using address shift handling registers address difference.
>>>>> 
>>>>> changes in v2:
>>>>>  Fix building section mismatch(es) warning.
>>>>> 
>>>>> drivers/i2c/busses/i2c-imx.c | 146
>>>>> ++++++++++++++++++++++++++++++++++++------- 1 file changed, 122
>>>>> insertions(+), 24 deletions(-)
>>>> 
>>>> [...]
>>>> 
>>>>> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
>>>>> 
>>>>> static const struct of_device_id i2c_imx_dt_ids[] = {
>>>>> 
>>>>>        { .compatible = "fsl,imx1-i2c", .data =
>>>>>        &imx_i2c_devtype[IMX1_I2C], },
>>>>>        { .compatible = "fsl,imx21-i2c", .data =
>>>>>        &imx_i2c_devtype[IMX21_I2C], },> >> 
>>>>> +       { .compatible = "fsl,vf610-i2c", .data =
>>>>> &imx_i2c_devtype[VF610_I2C], },> >> 
>>>>>        { /* sentinel */ }
>>>>> 
>>>>> };
>>>> 
>>>> That string doesn't seem to be documented anywhere (from a quick grep
>>>> of Documentation/devicetree), and there's no binding update included
>>>> here. It would be nice for that to be fixed :)
>>> 
>>> [Lu Jingchang]
>>> The binding string for i2c-imx driver in
>>> Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard
>>> format of "- compatible : Should be "fsl,<chip>-i2c" " for device
>>> using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c is
>>> described in the binding document. So I just leave the vf610 i2c
>>> compatible with this.
>> I'm not a big fan on wildcards in bindings, as it leaves people free to
>> put anything in and claim it's a documented binding, and makes it far
>> harder for an os to actually implement drivers for said binding, as
>> there's no canonical reference for the set of valid variations.
>> 
>> Obviously there is some precedent, but I'm not sure it's something we
>> want to stick with, and we can prevent it my updating the documentation
>> now.
>> 
>> Does anyone else have an opinion?
> 
> In case of Samsung platforms we decided to always use the name of first 
> SoC in which given IP appeared and list all compatible SoCs in binding 
> documentation. This is IMHO the most consistent way, as there is no 
> confusion about IP versions (not always listed in documentation, not even 
> saying about unavailable documentation) or potential problems with new 
> SoCs matching the wildcard, but having different IP.
> 
> In this particular case, the <chip> wildcard can be easily transformed 
> into a non-wildcard binding, by listing all supported <chip> values, i.e. 
> adding following text to binding documentation:
> 
> 8<--
> The <chip> can be one of:
> - imx1 - for i.MX1 and compatible SoCs,
> - imx21 - for i.MX21 and compatible SoCs,
> - vf610 - for Vybrid VF610 and compatible SoCs.
> -->8
> 
> Best regards,
> Tomasz


One way to handle this is to introduce a SoC Family <chip> name doc with the possible list of names and than reference that doc in the binding.  Kinda like how we have vendor-prefixes.txt.

And maybe instead of doing <chip> its <imx-chip> or some reference that isn't too generic such that in the future a .dts could be validated.

- k

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Stephen Warren]

On 08/12/2013 10:43 AM, Mark Rutland wrote:
> [Adding other devicetree maintainers to Cc]
> 
> On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
>>
>> ________________________________________
>>> 发件人: Mark Rutland [mark.rutland-5wv7dgnIgG8@public.gmane.org]
>>> 发送时间: 2013年8月10日 22:08
>>> 收件人: Lu Jingchang-B35083
>>> 抄送: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org; Estevam Fabio-R49496; Li Xiaochun-B41219; s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Jin Zhengxiong-R64188; shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; linux-arm-kernel-IAPFreCvJWM1dQTEkxZZdg@public.gmane.org.org
>>> 主题: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
>>
>>> On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
>>>>   Add Freescale Vybrid VF610 I2C controller support to
>>>> imx I2C driver framework.
>>>>   Some operation is different from imx I2C controller.
>>>> The register offset, the i2c clock divider value table,
>>>> the module enabling(I2CR_IEN) which is just invert with imx,
>>>> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
>>>> but w0c on imx.

>>>>  static const struct of_device_id i2c_imx_dt_ids[] = {
>>>>         { .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
>>>>         { .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
>>>> +       { .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
>>>
>>> That string doesn't seem to be documented anywhere (from a quick grep of
>>> Documentation/devicetree), and there's no binding update included
>>> here. It would be nice for that to be fixed :)
>>
>> [Lu Jingchang]
>> The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
>> of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
>> is described in the binding document. So I just leave the vf610 i2c compatible with this. 
> 
> I'm not a big fan on wildcards in bindings, as it leaves people free to
> put anything in and claim it's a documented binding, and makes it far
> harder for an os to actually implement drivers for said binding, as
> there's no canonical reference for the set of valid variations.
> 
> Obviously there is some precedent, but I'm not sure it's something we
> want to stick with, and we can prevent it my updating the documentation
> now.
> 
> Does anyone else have an opinion?

I suppose technically we should list out every exact string in the
binding, but it's a little annoying to have to update the binding doc
every time a new chip comes out (and I expect that'll happen more and
more!) just to add a new compatible value since all the differences are
known internally to the driver and don't impact the binding...

Kumar's idea could be a reasonable compromise, although I guess it
doesn't technically cover the case of there being 10 chips, each of
which has *some* new/different IP block, yet some IP blocks not changing
in every chip, and hence drivers might only directly binding to 5 of the
10 possible compatible values in some cases...

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Stephen Warren]

On 08/12/2013 10:43 AM, Mark Rutland wrote:
> [Adding other devicetree maintainers to Cc]
> 
> On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
>>
>> ________________________________________
>>> ???: Mark Rutland [mark.rutland at arm.com]
>>> ????: 2013?8?10? 22:08
>>> ???: Lu Jingchang-B35083
>>> ??: wsa at the-dreams.de; Estevam Fabio-R49496; Li Xiaochun-B41219; s.hauer at pengutronix.de; linux-i2c at vger.kernel.org; Jin Zhengxiong-R64188; shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org
>>> ??: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
>>
>>> On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
>>>>   Add Freescale Vybrid VF610 I2C controller support to
>>>> imx I2C driver framework.
>>>>   Some operation is different from imx I2C controller.
>>>> The register offset, the i2c clock divider value table,
>>>> the module enabling(I2CR_IEN) which is just invert with imx,
>>>> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
>>>> but w0c on imx.

>>>>  static const struct of_device_id i2c_imx_dt_ids[] = {
>>>>         { .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
>>>>         { .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
>>>> +       { .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
>>>
>>> That string doesn't seem to be documented anywhere (from a quick grep of
>>> Documentation/devicetree), and there's no binding update included
>>> here. It would be nice for that to be fixed :)
>>
>> [Lu Jingchang]
>> The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
>> of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
>> is described in the binding document. So I just leave the vf610 i2c compatible with this. 
> 
> I'm not a big fan on wildcards in bindings, as it leaves people free to
> put anything in and claim it's a documented binding, and makes it far
> harder for an os to actually implement drivers for said binding, as
> there's no canonical reference for the set of valid variations.
> 
> Obviously there is some precedent, but I'm not sure it's something we
> want to stick with, and we can prevent it my updating the documentation
> now.
> 
> Does anyone else have an opinion?

I suppose technically we should list out every exact string in the
binding, but it's a little annoying to have to update the binding doc
every time a new chip comes out (and I expect that'll happen more and
more!) just to add a new compatible value since all the differences are
known internally to the driver and don't impact the binding...

Kumar's idea could be a reasonable compromise, although I guess it
doesn't technically cover the case of there being 10 chips, each of
which has *some* new/different IP block, yet some IP blocks not changing
in every chip, and hence drivers might only directly binding to 5 of the
10 possible compatible values in some cases...

Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ]

On Mon, Aug 12, 2013 at 05:23:35PM -0600, Stephen Warren wrote:
> On 08/12/2013 10:43 AM, Mark Rutland wrote:
> >> The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
> >> of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
> >> is described in the binding document. So I just leave the vf610 i2c compatible with this. 
> > 
> > I'm not a big fan on wildcards in bindings, as it leaves people free to
> > put anything in and claim it's a documented binding, and makes it far
> > harder for an os to actually implement drivers for said binding, as
> > there's no canonical reference for the set of valid variations.
> > 
> > Obviously there is some precedent, but I'm not sure it's something we
> > want to stick with, and we can prevent it my updating the documentation
> > now.
> > 
> > Does anyone else have an opinion?
> 
> I suppose technically we should list out every exact string in the
> binding, but it's a little annoying to have to update the binding doc
> every time a new chip comes out (and I expect that'll happen more and
> more!) just to add a new compatible value since all the differences are
> known internally to the driver and don't impact the binding...

We would only have to update the the docs when an incompatible SoC comes
out. For this particular driver this would be all marked with a star:

* i.MX1
* i.MX21
  i.MX25
  i.MX27
  i.MX31
  i.MX35
  i.MX51
  i.MX53
  i.MX6
* Vybrid

That's not too many updates to the binding docs since 2001.
(The SPI core changed with nearly every SoC version though)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[s.hauer at pengutronix.de]

On Mon, Aug 12, 2013 at 05:23:35PM -0600, Stephen Warren wrote:
> On 08/12/2013 10:43 AM, Mark Rutland wrote:
> >> The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
> >> of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
> >> is described in the binding document. So I just leave the vf610 i2c compatible with this. 
> > 
> > I'm not a big fan on wildcards in bindings, as it leaves people free to
> > put anything in and claim it's a documented binding, and makes it far
> > harder for an os to actually implement drivers for said binding, as
> > there's no canonical reference for the set of valid variations.
> > 
> > Obviously there is some precedent, but I'm not sure it's something we
> > want to stick with, and we can prevent it my updating the documentation
> > now.
> > 
> > Does anyone else have an opinion?
> 
> I suppose technically we should list out every exact string in the
> binding, but it's a little annoying to have to update the binding doc
> every time a new chip comes out (and I expect that'll happen more and
> more!) just to add a new compatible value since all the differences are
> known internally to the driver and don't impact the binding...

We would only have to update the the docs when an incompatible SoC comes
out. For this particular driver this would be all marked with a star:

* i.MX1
* i.MX21
  i.MX25
  i.MX27
  i.MX31
  i.MX35
  i.MX51
  i.MX53
  i.MX6
* Vybrid

That's not too many updates to the binding docs since 2001.
(The SPI core changed with nearly every SoC version though)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Stephen Warren]

On 08/13/2013 01:46 AM, s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
> On Mon, Aug 12, 2013 at 05:23:35PM -0600, Stephen Warren wrote:
>> On 08/12/2013 10:43 AM, Mark Rutland wrote:
>>>> The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
>>>> of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
>>>> is described in the binding document. So I just leave the vf610 i2c compatible with this. 
>>>
>>> I'm not a big fan on wildcards in bindings, as it leaves people free to
>>> put anything in and claim it's a documented binding, and makes it far
>>> harder for an os to actually implement drivers for said binding, as
>>> there's no canonical reference for the set of valid variations.
>>>
>>> Obviously there is some precedent, but I'm not sure it's something we
>>> want to stick with, and we can prevent it my updating the documentation
>>> now.
>>>
>>> Does anyone else have an opinion?
>>
>> I suppose technically we should list out every exact string in the
>> binding, but it's a little annoying to have to update the binding doc
>> every time a new chip comes out (and I expect that'll happen more and
>> more!) just to add a new compatible value since all the differences are
>> known internally to the driver and don't impact the binding...
> 
> We would only have to update the the docs when an incompatible SoC comes
> out. For this particular driver this would be all marked with a star:
> 
> * i.MX1
> * i.MX21
>   i.MX25
>   i.MX27
>   i.MX31
>   i.MX35
>   i.MX51
>   i.MX53
>   i.MX6
> * Vybrid
> 
> That's not too many updates to the binding docs since 2001.
> (The SPI core changed with nearly every SoC version though)

So, the SPI core changed its HW implementation, or changed its
SW-visible interface? If the latter, then you need a separate compatible
value for each, which was my point.

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Stephen Warren]

On 08/13/2013 01:46 AM, s.hauer at pengutronix.de wrote:
> On Mon, Aug 12, 2013 at 05:23:35PM -0600, Stephen Warren wrote:
>> On 08/12/2013 10:43 AM, Mark Rutland wrote:
>>>> The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
>>>> of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
>>>> is described in the binding document. So I just leave the vf610 i2c compatible with this. 
>>>
>>> I'm not a big fan on wildcards in bindings, as it leaves people free to
>>> put anything in and claim it's a documented binding, and makes it far
>>> harder for an os to actually implement drivers for said binding, as
>>> there's no canonical reference for the set of valid variations.
>>>
>>> Obviously there is some precedent, but I'm not sure it's something we
>>> want to stick with, and we can prevent it my updating the documentation
>>> now.
>>>
>>> Does anyone else have an opinion?
>>
>> I suppose technically we should list out every exact string in the
>> binding, but it's a little annoying to have to update the binding doc
>> every time a new chip comes out (and I expect that'll happen more and
>> more!) just to add a new compatible value since all the differences are
>> known internally to the driver and don't impact the binding...
> 
> We would only have to update the the docs when an incompatible SoC comes
> out. For this particular driver this would be all marked with a star:
> 
> * i.MX1
> * i.MX21
>   i.MX25
>   i.MX27
>   i.MX31
>   i.MX35
>   i.MX51
>   i.MX53
>   i.MX6
> * Vybrid
> 
> That's not too many updates to the binding docs since 2001.
> (The SPI core changed with nearly every SoC version though)

So, the SPI core changed its HW implementation, or changed its
SW-visible interface? If the latter, then you need a separate compatible
value for each, which was my point.

Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ]

On Tue, Aug 13, 2013 at 09:48:40AM -0600, Stephen Warren wrote:
> On 08/13/2013 01:46 AM, s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org wrote:
> > On Mon, Aug 12, 2013 at 05:23:35PM -0600, Stephen Warren wrote:
> >> On 08/12/2013 10:43 AM, Mark Rutland wrote:
> >>>> The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
> >>>> of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
> >>>> is described in the binding document. So I just leave the vf610 i2c compatible with this. 
> >>>
> >>> I'm not a big fan on wildcards in bindings, as it leaves people free to
> >>> put anything in and claim it's a documented binding, and makes it far
> >>> harder for an os to actually implement drivers for said binding, as
> >>> there's no canonical reference for the set of valid variations.
> >>>
> >>> Obviously there is some precedent, but I'm not sure it's something we
> >>> want to stick with, and we can prevent it my updating the documentation
> >>> now.
> >>>
> >>> Does anyone else have an opinion?
> >>
> >> I suppose technically we should list out every exact string in the
> >> binding, but it's a little annoying to have to update the binding doc
> >> every time a new chip comes out (and I expect that'll happen more and
> >> more!) just to add a new compatible value since all the differences are
> >> known internally to the driver and don't impact the binding...
> > 
> > We would only have to update the the docs when an incompatible SoC comes
> > out. For this particular driver this would be all marked with a star:
> > 
> > * i.MX1
> > * i.MX21
> >   i.MX25
> >   i.MX27
> >   i.MX31
> >   i.MX35
> >   i.MX51
> >   i.MX53
> >   i.MX6
> > * Vybrid
> > 
> > That's not too many updates to the binding docs since 2001.
> > (The SPI core changed with nearly every SoC version though)
> 
> So, the SPI core changed its HW implementation, or changed its
> SW-visible interface? If the latter, then you need a separate compatible
> value for each, which was my point.

It changed the SW-visible interface.

I vote for having the exact SoC revision in the binding documentation
rather than wildcards or references to the list of i.MX SoCs. Otherwise
only the driver code gives a clue that the i2c driver matches imx1-i2c,
imx21-i2c and vf610-i2c, but not imx31-i2c.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[s.hauer at pengutronix.de]

On Tue, Aug 13, 2013 at 09:48:40AM -0600, Stephen Warren wrote:
> On 08/13/2013 01:46 AM, s.hauer at pengutronix.de wrote:
> > On Mon, Aug 12, 2013 at 05:23:35PM -0600, Stephen Warren wrote:
> >> On 08/12/2013 10:43 AM, Mark Rutland wrote:
> >>>> The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
> >>>> of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
> >>>> is described in the binding document. So I just leave the vf610 i2c compatible with this. 
> >>>
> >>> I'm not a big fan on wildcards in bindings, as it leaves people free to
> >>> put anything in and claim it's a documented binding, and makes it far
> >>> harder for an os to actually implement drivers for said binding, as
> >>> there's no canonical reference for the set of valid variations.
> >>>
> >>> Obviously there is some precedent, but I'm not sure it's something we
> >>> want to stick with, and we can prevent it my updating the documentation
> >>> now.
> >>>
> >>> Does anyone else have an opinion?
> >>
> >> I suppose technically we should list out every exact string in the
> >> binding, but it's a little annoying to have to update the binding doc
> >> every time a new chip comes out (and I expect that'll happen more and
> >> more!) just to add a new compatible value since all the differences are
> >> known internally to the driver and don't impact the binding...
> > 
> > We would only have to update the the docs when an incompatible SoC comes
> > out. For this particular driver this would be all marked with a star:
> > 
> > * i.MX1
> > * i.MX21
> >   i.MX25
> >   i.MX27
> >   i.MX31
> >   i.MX35
> >   i.MX51
> >   i.MX53
> >   i.MX6
> > * Vybrid
> > 
> > That's not too many updates to the binding docs since 2001.
> > (The SPI core changed with nearly every SoC version though)
> 
> So, the SPI core changed its HW implementation, or changed its
> SW-visible interface? If the latter, then you need a separate compatible
> value for each, which was my point.

It changed the SW-visible interface.

I vote for having the exact SoC revision in the binding documentation
rather than wildcards or references to the list of i.MX SoCs. Otherwise
only the driver code gives a clue that the i2c driver matches imx1-i2c,
imx21-i2c and vf610-i2c, but not imx31-i2c.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Shawn Guo]

On Tue, Aug 13, 2013 at 06:12:14PM +0200, s.hauer@pengutronix.de wrote:
> I vote for having the exact SoC revision in the binding documentation
> rather than wildcards or references to the list of i.MX SoCs. Otherwise
> only the driver code gives a clue that the i2c driver matches imx1-i2c,
> imx21-i2c and vf610-i2c, but not imx31-i2c.

+1

Shawn

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Shawn Guo]

On Tue, Aug 13, 2013 at 06:12:14PM +0200, s.hauer at pengutronix.de wrote:
> I vote for having the exact SoC revision in the binding documentation
> rather than wildcards or references to the list of i.MX SoCs. Otherwise
> only the driver code gives a clue that the i2c driver matches imx1-i2c,
> imx21-i2c and vf610-i2c, but not imx31-i2c.

+1

Shawn

Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Wolfram Sang]

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


> I vote for having the exact SoC revision in the binding documentation
> rather than wildcards or references to the list of i.MX SoCs. Otherwise
> only the driver code gives a clue that the i2c driver matches imx1-i2c,
> imx21-i2c and vf610-i2c, but not imx31-i2c.

Dunno if I got all right, so adding my 2 cents:

Yes to adding each SoC to the binding docs. No to adding each SoC to the
driver as a seperate 'compatible' entry if not really needed to
distinguish IP versions. I mean imx31 should have two compatible entries
anyhow, one for imx31 and one for imx21 as fallback, no?

That all being said: Unless somebody objects, I'll pick the most recent
VF610 series today and leave the doc fixup for later.

Thanks,

   Wolfram


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

答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Wolfram Sang]

> I vote for having the exact SoC revision in the binding documentation
> rather than wildcards or references to the list of i.MX SoCs. Otherwise
> only the driver code gives a clue that the i2c driver matches imx1-i2c,
> imx21-i2c and vf610-i2c, but not imx31-i2c.

Dunno if I got all right, so adding my 2 cents:

Yes to adding each SoC to the binding docs. No to adding each SoC to the
driver as a seperate 'compatible' entry if not really needed to
distinguish IP versions. I mean imx31 should have two compatible entries
anyhow, one for imx31 and one for imx21 as fallback, no?

That all being said: Unless somebody objects, I'll pick the most recent
VF610 series today and leave the doc fixup for later.

Thanks,

   Wolfram

-------------- 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/20130815/bc41abc3/attachment.sig>