dm: i2c: mxc support DM

PATCH New Peng Fan 3 participants 6 messages Apr 15, 2015 – Apr 26, 2015

Subsystems (3)

Arm (32-bit) Arm i.MX and Layerscape I2C

lore

Share & tools

lore

Peng Fan

author

2015-04-15 09:35 UTC #0 lore raw

PATCH 1/1 dm: i2c: mxc support DM

Add support when CONFIG_DM_I2C configured.

Test results:
=> i2c dev 0
Setting bus to 0
=> i2c probe
Valid chip addresses: 08 50
=> i2c md 8 38
0038: 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 ................
=> i2c mw 8 38 5 1
=> i2c md 8 38
0038: 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 ................
=> dm tree
Class Probed Name
----------------------------------------

 root        [ + ]    root_driver
 thermal     [   ]    |-- imx_thermal
 simple_bus  [ + ]    |-- soc
 simple_bus  [   ]    |   |-- aips-bus at 30000000
 simple_bus  [   ]    |   |   |-- anatop at 30360000
 simple_bus  [   ]    |   |   `-- snvs at 30370000
 simple_bus  [   ]    |   |-- aips-bus at 30400000
 simple_bus  [ + ]    |   `-- aips-bus at 30800000
 i2c         [ + ]    |       |-- i2c at 30a20000
 i2c_generic [ + ]    |       |   |-- generic_8
 i2c_generic [ + ]    |       |   `-- generic_50
 i2c         [   ]    |       |-- i2c at 30a40000
 spi         [   ]    |       `-- qspi at 30bb0000
 simple_bus  [   ]    `-- regulators

Signed-off-by Peng Fan <[email protected]>

---
 arch/arm/imx-common/i2c-mxv7.c            |   4 +
 arch/arm/include/asm/imx-common/mxc_i2c.h |   5 +
 drivers/i2c/mxc_i2c.c                     | 354 ++++++++++++++++++++++++++++++
 3 files changed, 363 insertions(+)

arch/arm/imx-common/i2c-mxv7.c +4 -0

index 1a632e7..99fe112 100644
@@ -99,8 +99,12 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
 	if (ret)
 		goto err_idle;
 
+#ifndef CONFIG_DM_I2C
 	bus_i2c_init(i2c_bases[i2c_index], speed, slave_addr,
 			force_idle_bus, p);
+#else
+	bus_i2c_init(i2c_index, speed, slave_addr, force_idle_bus, p);
+#endif
 
 	return 0;

arch/arm/include/asm/imx-common/mxc_i2c.h +5 -0

index af86163..8f9c83e 100644
@@ -54,8 +54,13 @@ struct i2c_pads_info {
 
 int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
 	      struct i2c_pads_info *p);
+#ifndef CONFIG_DM_I2C
 void bus_i2c_init(void *base, int speed, int slave_addr,
 		int (*idle_bus_fn)(void *p), void *p);
+#else
+void bus_i2c_init(int index, int speed, int slave_addr,
+		int (*idle_bus_fn)(void *p), void *p);
+#endif
 int bus_i2c_read(void *base, uchar chip, uint addr, int alen, uchar *buf,
 		int len);
 int bus_i2c_write(void *base, uchar chip, uint addr, int alen,

drivers/i2c/mxc_i2c.c +354 -0

369 lines not on this page · raw message

Signature · 1 line

1.8.4

Marek Vasut

2015-04-15 11:38 UTC #1 lore raw

PATCH 1/1 dm: i2c: mxc support DM

Quoted 30 lines · Peng Fan

Add support when CONFIG_DM_I2C configured.
Test results:
=> i2c dev 0
Setting bus to 0
=> i2c probe
Valid chip addresses: 08 50
=> i2c md 8 38
0038: 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 ................
=> i2c mw 8 38 5 1
=> i2c md 8 38
0038: 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 ................
=> dm tree
Class Probed Name
----------------------------------------
 root        [ + ]    root_driver
 thermal     [   ]    |-- imx_thermal
 simple_bus  [ + ]    |-- soc
 simple_bus  [   ]    |   |-- aips-bus at 30000000
 simple_bus  [   ]    |   |   |-- anatop at 30360000
 simple_bus  [   ]    |   |   `-- snvs at 30370000
 simple_bus  [   ]    |   |-- aips-bus at 30400000
 simple_bus  [ + ]    |   `-- aips-bus at 30800000
 i2c         [ + ]    |       |-- i2c at 30a20000
 i2c_generic [ + ]    |       |   |-- generic_8
 i2c_generic [ + ]    |       |   `-- generic_50
 i2c         [   ]    |       |-- i2c at 30a40000
 spi         [   ]    |       `-- qspi at 30bb0000
 simple_bus  [   ]    `-- regulators

Quoted 1 line

Signed-off-by Peng Fan <[email protected]>

Hi,

[...]

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 692

Quoted 13 lines

drivers/i2c/mxc_i2c.c +13 -0

+static int i2c_init_transfer_(struct i2c_bus *i2c_bus, u32 chip,
+			      bool read)
+{
+	unsigned int temp;
+	int ret;
+	struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+
+	/* Enable I2C controller */
+#ifdef I2C_QUIRK_REG
+	if (readb(&i2c_regs->i2cr) & I2CR_IDIS) {
+#else
+	if (!(readb(&i2c_regs->i2cr) & I2CR_IEN)) {
+#endif

This is a little odd, maybe you can rename this to some sensible name since it's IMX specific ?

Simon Glass

2015-04-19 13:53 UTC #2 lore raw

PATCH 1/1 dm: i2c: mxc support DM

In reply to Peng Fan

Hi Peng,

Quoted 42 lines · Peng Fan

Add support when CONFIG_DM_I2C configured.
Test results:
=> i2c dev 0
Setting bus to 0
=> i2c probe
Valid chip addresses: 08 50
=> i2c md 8 38
0038: 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 ................
=> i2c mw 8 38 5 1
=> i2c md 8 38
0038: 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 ................
=> dm tree
Class Probed Name
----------------------------------------
 root        [ + ]    root_driver
 thermal     [   ]    |-- imx_thermal
 simple_bus  [ + ]    |-- soc
 simple_bus  [   ]    |   |-- aips-bus at 30000000
 simple_bus  [   ]    |   |   |-- anatop at 30360000
 simple_bus  [   ]    |   |   `-- snvs at 30370000
 simple_bus  [   ]    |   |-- aips-bus at 30400000
 simple_bus  [ + ]    |   `-- aips-bus at 30800000
 i2c         [ + ]    |       |-- i2c at 30a20000
 i2c_generic [ + ]    |       |   |-- generic_8
 i2c_generic [ + ]    |       |   `-- generic_50
 i2c         [   ]    |       |-- i2c at 30a40000
 spi         [   ]    |       `-- qspi at 30bb0000
 simple_bus  [   ]    `-- regulators
Signed-off-by Peng Fan <[email protected]>
---
 arch/arm/imx-common/i2c-mxv7.c            |   4 +
 arch/arm/include/asm/imx-common/mxc_i2c.h |   5 +
 drivers/i2c/mxc_i2c.c                     | 354 ++++++++++++++++++++++++++++++
 3 files changed, 363 insertions(+)
arch/arm/imx-common/i2c-mxv7.c +0 -0
index 1a632e7..99fe112 100644

arch/arm/imx-common/i2c-mxv7.c int setup_i2c(unsigned i2c_index, int speed, int slave_addr, · line 106

Quoted 8 lines

arch/arm/imx-common/i2c-mxv7.c +3 -0

@@ -99,8 +99,12 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
        if (ret)
                goto err_idle;
 
+#ifndef CONFIG_DM_I2C
        bus_i2c_init(i2c_bases[i2c_index], speed, slave_addr,
                        force_idle_bus, p);
+#else
+       bus_i2c_init(i2c_index, speed, slave_addr, force_idle_bus, p);

One of the goals of driver model is to remove code like this, and have the devices init themselves when they are used. Here you are probing each device and then changing its configuration outside the device's probe() method. This should not be needed with driver model. See below.

Quoted 44 lines

arch/arm/imx-common/i2c-mxv7.c +1 -0

+#endif
 
        return 0;

arch/arm/include/asm/imx-common/mxc_i2c.h +5 -0

index af86163..8f9c83e 100644
@@ -54,8 +54,13 @@ struct i2c_pads_info {
 
 int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
              struct i2c_pads_info *p);
+#ifndef CONFIG_DM_I2C
 void bus_i2c_init(void *base, int speed, int slave_addr,
                int (*idle_bus_fn)(void *p), void *p);
+#else
+void bus_i2c_init(int index, int speed, int slave_addr,
+               int (*idle_bus_fn)(void *p), void *p);
+#endif
 int bus_i2c_read(void *base, uchar chip, uint addr, int alen, uchar *buf,
                int len);
 int bus_i2c_write(void *base, uchar chip, uint addr, int alen,

drivers/i2c/mxc_i2c.c +3 -0

index fc5ee35..9488e24 100644
@@ -21,6 +21,8 @@
 #include <asm/io.h>
 #include <i2c.h>
 #include <watchdog.h>
+#include <dm.h>
+#include <fdtdec.h>
 
 DECLARE_GLOBAL_DATA_PTR;

@@ -224,6 +226,7 @@ static int tx_byte(struct mxc_i2c_regs *i2c_regs, u8 byte)
        return 0;
 }
 
+#ifndef CONFIG_DM_I2C
 /*
  * Stop I2C transaction
  */

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 563

Quoted 9 lines

drivers/i2c/mxc_i2c.c +6 -0

@@ -552,3 +555,354 @@ U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe,
                         CONFIG_SYS_MXC_I2C3_SPEED,
                         CONFIG_SYS_MXC_I2C3_SLAVE, 2)
 #endif
+#else
+/*
+ * Information about one i2c bus
+ * struct i2c_bus - information about the i2c[x] bus
+ *
+ * @id: Index of i2c bus

What do you mean by 'index'? Is this actually used? I think you should drop this. See dev->seq for a probed device.

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 565

Quoted 2 lines

drivers/i2c/mxc_i2c.c +2 -0

+ * @speed: Speed of i2c bus
+ * @driver_data: Flags for different platforms, not used now.

You could drop it, or change to ulong.

\> + * @regs: Pointer, the address of i2c bus

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 567

Quoted 1 line

drivers/i2c/mxc_i2c.c +1 -0
+ * @idle_bus_fn: function to force bus idle

We should not call functions like this in driver model.

Quoted 23 lines

drivers/i2c/mxc_i2c.c +23 -0

+ * @idle_bus_data: parameter for idle_bus_fun
+ */
+struct i2c_bus {
+       int id;
+       int speed;
+       int pinmux_config;
+       int driver_data;
+       struct mxc_i2c_regs *regs;
+       int (*idle_bus_fn)(void *p);
+       void *idle_bus_data;
+};
+
+/*
+ * Stop I2C transaction
+ */
+static void i2c_imx_stop(struct i2c_bus *i2c_bus)
+{
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+       int ret;
+       unsigned int temp = readb(&i2c_regs->i2cr);
+
+       temp &= ~(I2CR_MSTA | I2CR_MTX);
+       writeb(temp, &i2c_regs->i2cr);

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 600

Quoted 10 lines

drivers/i2c/mxc_i2c.c +10 -0

+       ret = wait_for_sr_state(i2c_regs, ST_BUS_IDLE);
+       if (ret < 0)
+               debug("%s:trigger stop failed\n", __func__);
+       return;
+}
+
+static int i2c_idle_bus(struct i2c_bus *i2c_bus)
+{
+       if (i2c_bus && i2c_bus->idle_bus_fn)
+               return i2c_bus->idle_bus_fn(i2c_bus->idle_bus_data);

Can you explain what this does?

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 605

Quoted 5 lines

drivers/i2c/mxc_i2c.c +5 -0

+
+       return 0;
+}
+
+static void i2c_init_controller(struct i2c_bus *i2c_bus)

Drop this function? It seems to do nothing.

Quoted 19 lines

drivers/i2c/mxc_i2c.c +19 -0

+{
+       if (!i2c_bus->speed)
+               return;
+
+       debug("%s: speed=%d\n", __func__, i2c_bus->speed);
+
+       return;
+}
+
+static int mxc_i2c_set_bus_speed(struct udevice *bus, unsigned int speed)
+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+
+       return bus_i2c_set_bus_speed(i2c_bus->regs, speed);
+}
+
+static int mxc_i2c_probe(struct udevice *bus)
+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 634

Quoted 10 lines

drivers/i2c/mxc_i2c.c +10 -0

+       fdt_addr_t addr;
+
+       i2c_bus->id = bus->seq;
+       /*
+        * driver_dats is not used now, later we can use driver data
+        * to cover I2C_QUIRK_REG and etc.
+        *
+        * TODO
+        */
+       i2c_bus->driver_data = dev_get_of_data(bus);

dev_get_driver_data() now.

Quoted 31 lines

drivers/i2c/mxc_i2c.c +31 -0

+
+       addr = dev_get_addr(bus);
+       if (addr == FDT_ADDR_T_NONE)
+               return -ENODEV;
+
+       i2c_bus->regs = (struct mxc_i2c_regs *)addr;
+
+       /*
+        * Pinmux settings are in board file now, until pinmux is supported,
+        * we can set pinmux here in probe function.
+        *
+        * TODO: Pinmux
+        */
+
+       i2c_init_controller(i2c_bus);
+       debug("i2c : controller bus %d at %p , speed %d: ",
+             bus->seq, i2c_bus->regs,
+             i2c_bus->speed);
+
+       return 0;
+}
+
+void bus_i2c_init(int busnum, int speed, int slave, int (*idle_bus_fn)(void *p),
+                 void *idle_bus_data)
+{
+       struct udevice *bus;
+       struct i2c_bus *i2c_bus;
+       int ret;
+
+       ret = uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus);
+       if (ret) {

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 675

Quoted 10 lines

drivers/i2c/mxc_i2c.c +10 -0

+               debug("Cannot find I2C bus %d\n", busnum);
+               return;
+       }
+
+       i2c_bus = dev_get_priv(bus);
+
+       i2c_bus->idle_bus_fn = idle_bus_fn;
+       i2c_bus->idle_bus_data = idle_bus_data;
+
+       mxc_i2c_set_bus_speed(bus, speed);

This should move into your probe function. You should not need bus_i2c_init(). See for example tegra_i2c_probe().

Quoted 7 lines

drivers/i2c/mxc_i2c.c +7 -0

+
+       return;
+}
+
+static int i2c_init_transfer_(struct i2c_bus *i2c_bus, u32 chip,
+                             bool read)
+{

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 692

Quoted 10 lines

drivers/i2c/mxc_i2c.c +10 -0

+       unsigned int temp;
+       int ret;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+
+       /* Enable I2C controller */
+#ifdef I2C_QUIRK_REG
+       if (readb(&i2c_regs->i2cr) & I2CR_IDIS) {
+#else
+       if (!(readb(&i2c_regs->i2cr) & I2CR_IEN)) {
+#endif

Can you work this out from the device tree or the driver data, and avoid the #ifdef?

Quoted 50 lines

drivers/i2c/mxc_i2c.c +50 -0

+               writeb(I2CR_IEN, &i2c_regs->i2cr);
+               /* Wait for controller to be stable */
+               udelay(50);
+       }
+       if (readb(&i2c_regs->iadr) == (chip << 1))
+               writeb((chip << 1) ^ 2, &i2c_regs->iadr);
+       writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+       ret = wait_for_sr_state(i2c_regs, ST_BUS_IDLE);
+       if (ret < 0)
+               return ret;
+
+       /* Start I2C transaction */
+       temp = readb(&i2c_regs->i2cr);
+       temp |= I2CR_MSTA;
+       writeb(temp, &i2c_regs->i2cr);
+
+       ret = wait_for_sr_state(i2c_regs, ST_BUS_BUSY);
+       if (ret < 0)
+               return ret;
+
+       temp |= I2CR_MTX | I2CR_TX_NO_AK;
+       writeb(temp, &i2c_regs->i2cr);
+
+       /* write slave address */
+       ret = tx_byte(i2c_regs, chip << 1 | read);
+       if (ret < 0) {
+               i2c_imx_stop(i2c_bus);
+               return ret;
+       }
+
+       return 0;
+}
+
+static int i2c_init_transfer(struct i2c_bus *i2c_bus, u32 chip, bool read)
+{
+       int retry;
+       int ret;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+
+       for (retry = 0; retry < 3; retry++) {
+               ret = i2c_init_transfer_(i2c_bus, chip, read);
+               if (ret >= 0)
+                       return 0;
+               i2c_imx_stop(i2c_bus);
+               if (ret == -ENODEV)
+                       return ret;
+
+               debug("%s: failed for chip 0x%x retry=%d\n", __func__, chip,
+                     retry);
+               if (ret != -ERESTART)

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 752

Quoted 10 lines

drivers/i2c/mxc_i2c.c +10 -0

+                       /* Disable controller */
+                       writeb(I2CR_IDIS, &i2c_regs->i2cr);
+               udelay(100);
+               if (i2c_idle_bus(i2c_bus) < 0)
+                       break;
+       }
+
+       debug("%s: give up i2c_regs=%p\n", __func__, i2c_regs);
+       return ret;
+}

This looks very similar to the old i2c_init_transfer(). Can you create a common function and avoid copying the code? Also in the old function you dealt with 'alen' (now called offset length) but I don't see that code here.

Quoted 13 lines

drivers/i2c/mxc_i2c.c +13 -0

+
+
+static int mxc_i2c_probe_chip(struct udevice *bus, u32 chip_addr,
+                             u32 chip_flags)
+{
+       int ret;
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+
+       ret = i2c_init_transfer(i2c_bus, chip_addr, false);
+       if (ret < 0)
+               return ret;
+
+       i2c_imx_stop(i2c_bus);

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 775

Quoted 10 lines

drivers/i2c/mxc_i2c.c +10 -0

+
+       return 0;
+}
+
+static int i2c_write_data(struct i2c_bus *i2c_bus, uchar chip, uchar *buffer,
+                         int len, bool end_with_repeated_start)
+{
+       int i, ret = 0;
+
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;

blank line should move up one.

Quoted 72 lines

drivers/i2c/mxc_i2c.c +72 -0

+       debug("i2c_write_data: chip=0x%x, len=0x%x\n", chip, len);
+       debug("write_data: ");
+       /* use rc for counter */
+       for (i = 0; i < len; ++i)
+               debug(" 0x%02x", buffer[i]);
+       debug("\n");
+
+       for (i = 0; i < len; i++) {
+               ret = tx_byte(i2c_regs, buffer[i]);
+               if (ret < 0) {
+                       debug("i2c_write_data(): rc=%d\n", ret);
+                       break;
+               }
+       }
+
+       if (end_with_repeated_start) {
+               /* Reuse ret */
+               ret = readb(&i2c_regs->i2cr);
+               ret |= I2CR_RSTA;
+               writeb(ret, &i2c_regs->i2cr);
+
+               ret = tx_byte(i2c_regs, (chip << 1) | 1);
+               if (ret < 0) {
+                       i2c_imx_stop(i2c_bus);
+                       return ret;
+               }
+       }
+
+       return ret;
+}
+
+static int i2c_read_data(struct i2c_bus *i2c_bus, uchar chip, uchar *buf,
+                        int len)
+{
+       int ret;
+       unsigned int temp;
+       int i;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+
+       debug("i2c_read_data: chip=0x%x, len=0x%x\n", chip, len);
+
+       /* setup bus to read data */
+       temp = readb(&i2c_regs->i2cr);
+       temp &= ~(I2CR_MTX | I2CR_TX_NO_AK);
+       if (len == 1)
+               temp |= I2CR_TX_NO_AK;
+       writeb(temp, &i2c_regs->i2cr);
+       writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+       readb(&i2c_regs->i2dr);         /* dummy read to clear ICF */
+
+       /* read data */
+       for (i = 0; i < len; i++) {
+               ret = wait_for_sr_state(i2c_regs, ST_IIF);
+               if (ret < 0) {
+                       debug("i2c_read_data(): ret=%d\n", ret);
+                       i2c_imx_stop(i2c_bus);
+                       return ret;
+               }
+
+               /*
+                * It must generate STOP before read I2DR to prevent
+                * controller from generating another clock cycle
+                */
+               if (i == (len - 1)) {
+                       i2c_imx_stop(i2c_bus);
+               } else if (i == (len - 2)) {
+                       temp = readb(&i2c_regs->i2cr);
+                       temp |= I2CR_TX_NO_AK;
+                       writeb(temp, &i2c_regs->i2cr);
+               }
+               writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+               buf[i] = readb(&i2c_regs->i2dr);

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 857

Quoted 10 lines

drivers/i2c/mxc_i2c.c +10 -0

+       }
+
+       /* reuse ret for counter*/
+       for (ret = 0; ret < len; ++ret)
+               debug(" 0x%02x", buf[ret]);
+       debug("\n");
+
+       i2c_imx_stop(i2c_bus);
+       return 0;
+}

Again we have duplicated code. While we have both driver model and non-drivel-model code co-existing, we should try to avoid this.

Quoted 12 lines

drivers/i2c/mxc_i2c.c +12 -0

+
+static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+       int ret;
+
+       ret = i2c_init_transfer(i2c_bus, msg->addr, false);
+       if (ret < 0)
+               return ret;
+
+       for (; nmsgs > 0; nmsgs--, msg++) {
+               bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD);

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 879

Quoted 10 lines

drivers/i2c/mxc_i2c.c +10 -0

+               debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len);
+               if (msg->flags & I2C_M_RD)
+                       ret = i2c_read_data(i2c_bus, msg->addr, msg->buf,
+                                           msg->len);
+               else
+                       ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
+                                            msg->len, next_is_read);
+               if (ret) {
+                       debug("i2c_write: error sending\n");
+                       return -EREMOTEIO;

return ret? Is the error the wrong value?

Quoted 30 lines

drivers/i2c/mxc_i2c.c +29 -1

+               }
+       }
+
+       i2c_imx_stop(i2c_bus);
+
+       return 0;
+}
+
+static const struct dm_i2c_ops mxc_i2c_ops = {
+       .xfer           = mxc_i2c_xfer,
+       .probe_chip     = mxc_i2c_probe_chip,
+       .set_bus_speed  = mxc_i2c_set_bus_speed,
+};
+
+static const struct udevice_id mxc_i2c_ids[] = {
+       { .compatible = "fsl,imx21-i2c", },
+       { .compatible = "fsl,vf610-i2c", },
+       {}
+};
+
+U_BOOT_DRIVER(i2c_mxc) = {
+       .name = "i2c_mxc",
+       .id = UCLASS_I2C,
+       .of_match = mxc_i2c_ids,
+       .probe = mxc_i2c_probe,
+       .priv_auto_alloc_size = sizeof(struct i2c_bus),
+       .ops = &mxc_i2c_ops,
+};
+#endif
--

Quoted 3 lines

1.8.4

Regards,
Simon

Peng Fan

author

2015-04-20 05:49 UTC #3 lore raw

PATCH 1/1 dm: i2c: mxc support DM

Hi Simon,

Thanks for reviewing. I'll address most comments and try to merge DM and non-DM part into one. will send out v2 for review.
The only unsure part is bus_i2c_init, I also reply you inline. I want to pass force_idle_bus and pinmux setting to i2c driver, so i use bus_i2c_init, same with non-DM way.

Quoted 48 lines · Simon Glass

Hi Peng,
Quoted 45 lines · Peng Fan
Add support when CONFIG_DM_I2C configured.
Test results:
=> i2c dev 0
Setting bus to 0
=> i2c probe
Valid chip addresses: 08 50
=> i2c md 8 38
0038: 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 ................
=> i2c mw 8 38 5 1
=> i2c md 8 38
0038: 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 ................
=> dm tree
Class Probed Name
----------------------------------------
  root        [ + ]    root_driver
  thermal     [   ]    |-- imx_thermal
  simple_bus  [ + ]    |-- soc
  simple_bus  [   ]    |   |-- aips-bus at 30000000
  simple_bus  [   ]    |   |   |-- anatop at 30360000
  simple_bus  [   ]    |   |   `-- snvs at 30370000
  simple_bus  [   ]    |   |-- aips-bus at 30400000
  simple_bus  [ + ]    |   `-- aips-bus at 30800000
  i2c         [ + ]    |       |-- i2c at 30a20000
  i2c_generic [ + ]    |       |   |-- generic_8
  i2c_generic [ + ]    |       |   `-- generic_50
  i2c         [   ]    |       |-- i2c at 30a40000
  spi         [   ]    |       `-- qspi at 30bb0000
  simple_bus  [   ]    `-- regulators
Signed-off-by Peng Fan <[email protected]>
---
  arch/arm/imx-common/i2c-mxv7.c            |   4 +
  arch/arm/include/asm/imx-common/mxc_i2c.h |   5 +
  drivers/i2c/mxc_i2c.c                     | 354 ++++++++++++++++++++++++++++++
  3 files changed, 363 insertions(+)
arch/arm/imx-common/i2c-mxv7.c +0 -0
index 1a632e7..99fe112 100644
@@ -99,8 +99,12 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
         if (ret)
                 goto err_idle;
 

Quoted 10 lines

arch/arm/imx-common/i2c-mxv7.c int setup_i2c(unsigned i2c_index, int speed, int slave_addr, · line 106
Quoted 5 lines
arch/arm/imx-common/i2c-mxv7.c +3 -0
+#ifndef CONFIG_DM_I2C
         bus_i2c_init(i2c_bases[i2c_index], speed, slave_addr,
                         force_idle_bus, p);
+#else
+       bus_i2c_init(i2c_index, speed, slave_addr, force_idle_bus, p);
One of the goals of driver model is to remove code like this, and have the devices init themselves when they are used. Here you are probing each device and then changing its configuration outside the device's probe() method. This should not be needed with driver model. See below.

Agree. But i want to pass force_idle_bus and pinmux settings(parameter
p) to i2c driver. I did not find a good way how to pass these two to DM mxc_i2c driver.

Quoted 40 lines

arch/arm/imx-common/i2c-mxv7.c +1 -0

+#endif
 
         return 0;

arch/arm/include/asm/imx-common/mxc_i2c.h +5 -0

index af86163..8f9c83e 100644
@@ -54,8 +54,13 @@ struct i2c_pads_info {
 
  int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
               struct i2c_pads_info *p);
+#ifndef CONFIG_DM_I2C
  void bus_i2c_init(void *base, int speed, int slave_addr,
                 int (*idle_bus_fn)(void *p), void *p);
+#else
+void bus_i2c_init(int index, int speed, int slave_addr,
+               int (*idle_bus_fn)(void *p), void *p);
+#endif
  int bus_i2c_read(void *base, uchar chip, uint addr, int alen, uchar *buf,
                 int len);
  int bus_i2c_write(void *base, uchar chip, uint addr, int alen,

drivers/i2c/mxc_i2c.c +2 -0

index fc5ee35..9488e24 100644
@@ -21,6 +21,8 @@
  #include <asm/io.h>
  #include <i2c.h>
  #include <watchdog.h>
+#include <dm.h>
+#include <fdtdec.h>
 
  DECLARE_GLOBAL_DATA_PTR;

@@ -224,6 +226,7 @@ static int tx_byte(struct mxc_i2c_regs *i2c_regs, u8 byte)
         return 0;
  }

Quoted 16 lines

Quoted 5 lines
drivers/i2c/mxc_i2c.c +1 -0
+#ifndef CONFIG_DM_I2C
  /*
   * Stop I2C transaction
   */
drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 563
Quoted 9 lines
drivers/i2c/mxc_i2c.c +6 -0
@@ -552,3 +555,354 @@ U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe,
                          CONFIG_SYS_MXC_I2C3_SPEED,
                          CONFIG_SYS_MXC_I2C3_SLAVE, 2)
  #endif
+#else
+/*
+ * Information about one i2c bus
+ * struct i2c_bus - information about the i2c[x] bus
+ *
+ * @id: Index of i2c bus
What do you mean by 'index'? Is this actually used? I think you should drop this. See dev->seq for a probed device.

Yeah, it is not used. Will remove it.

Quoted 4 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 565
Quoted 3 lines
drivers/i2c/mxc_i2c.c +2 -0
+ * @speed: Speed of i2c bus
+ * @driver_data: Flags for different platforms, not used now.
You could drop it, or change to ulong.

Will change it to ulong, and use it cover the I2C_QUIRK_REG.

Quoted 4 lines

\> + * @regs: Pointer, the address of i2c bus
drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 567
Quoted 1 line
drivers/i2c/mxc_i2c.c +1 -0
+ * @idle_bus_fn: function to force bus idle
We should not call functions like this in driver model.

I can not follow you about this. idle_bus_fn is used to force bus idle, when something err during data transfer.

Quoted 27 lines

drivers/i2c/mxc_i2c.c +26 -0

+ * @idle_bus_data: parameter for idle_bus_fun
+ */
+struct i2c_bus {
+       int id;
+       int speed;
+       int pinmux_config;
+       int driver_data;
+       struct mxc_i2c_regs *regs;
+       int (*idle_bus_fn)(void *p);
+       void *idle_bus_data;
+};
+
+/*
+ * Stop I2C transaction
+ */
+static void i2c_imx_stop(struct i2c_bus *i2c_bus)
+{
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+       int ret;
+       unsigned int temp = readb(&i2c_regs->i2cr);
+
+       temp &= ~(I2CR_MSTA | I2CR_MTX);
+       writeb(temp, &i2c_regs->i2cr);
+       ret = wait_for_sr_state(i2c_regs, ST_BUS_IDLE);
+       if (ret < 0)
+               debug("%s:trigger stop failed\n", __func__);

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 600
Quoted 7 lines
drivers/i2c/mxc_i2c.c +7 -0
+       return;
+}
+
+static int i2c_idle_bus(struct i2c_bus *i2c_bus)
+{
+       if (i2c_bus && i2c_bus->idle_bus_fn)
+               return i2c_bus->idle_bus_fn(i2c_bus->idle_bus_data);
Can you explain what this does?

Used to force bus idle when something err during data transfer. You can see arch/arm/imx-common/i2c-mxv7.c, there is a function "force_idle_bus" which is used to force i2c bus idle using gpio mode.

Quoted 7 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 605
Quoted 6 lines
drivers/i2c/mxc_i2c.c +5 -0
+
+       return 0;
+}
+
+static void i2c_init_controller(struct i2c_bus *i2c_bus)
Drop this function? It seems to do nothing.

ok.

Quoted 23 lines

drivers/i2c/mxc_i2c.c +22 -0

+{
+       if (!i2c_bus->speed)
+               return;
+
+       debug("%s: speed=%d\n", __func__, i2c_bus->speed);
+
+       return;
+}
+
+static int mxc_i2c_set_bus_speed(struct udevice *bus, unsigned int speed)
+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+
+       return bus_i2c_set_bus_speed(i2c_bus->regs, speed);
+}
+
+static int mxc_i2c_probe(struct udevice *bus)
+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+       fdt_addr_t addr;
+
+       i2c_bus->id = bus->seq;

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 634
Quoted 7 lines
drivers/i2c/mxc_i2c.c +7 -0
+       /*
+        * driver_dats is not used now, later we can use driver data
+        * to cover I2C_QUIRK_REG and etc.
+        *
+        * TODO
+        */
+       i2c_bus->driver_data = dev_get_of_data(bus);
dev_get_driver_data() now.

ok.

Quoted 36 lines

drivers/i2c/mxc_i2c.c +35 -0

+
+       addr = dev_get_addr(bus);
+       if (addr == FDT_ADDR_T_NONE)
+               return -ENODEV;
+
+       i2c_bus->regs = (struct mxc_i2c_regs *)addr;
+
+       /*
+        * Pinmux settings are in board file now, until pinmux is supported,
+        * we can set pinmux here in probe function.
+        *
+        * TODO: Pinmux
+        */
+
+       i2c_init_controller(i2c_bus);
+       debug("i2c : controller bus %d at %p , speed %d: ",
+             bus->seq, i2c_bus->regs,
+             i2c_bus->speed);
+
+       return 0;
+}
+
+void bus_i2c_init(int busnum, int speed, int slave, int (*idle_bus_fn)(void *p),
+                 void *idle_bus_data)
+{
+       struct udevice *bus;
+       struct i2c_bus *i2c_bus;
+       int ret;
+
+       ret = uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus);
+       if (ret) {
+               debug("Cannot find I2C bus %d\n", busnum);
+               return;
+       }
+

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 675
Quoted 6 lines
drivers/i2c/mxc_i2c.c +6 -0
+       i2c_bus = dev_get_priv(bus);
+
+       i2c_bus->idle_bus_fn = idle_bus_fn;
+       i2c_bus->idle_bus_data = idle_bus_data;
+
+       mxc_i2c_set_bus_speed(bus, speed);
This should move into your probe function. You should not need bus_i2c_init(). See for example tegra_i2c_probe().

I want to use "force_bus_idle" and pinmux settings, but I did not find a good way to pass force_bus_idle to the i2c driver. bus_i2c_init is called from arch/arm/imx-common/i2c-mxv7.c

Quoted 12 lines

drivers/i2c/mxc_i2c.c +11 -0

+
+       return;
+}
+
+static int i2c_init_transfer_(struct i2c_bus *i2c_bus, u32 chip,
+                             bool read)
+{
+       unsigned int temp;
+       int ret;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 692
Quoted 6 lines
drivers/i2c/mxc_i2c.c +6 -0
+       /* Enable I2C controller */
+#ifdef I2C_QUIRK_REG
+       if (readb(&i2c_regs->i2cr) & I2CR_IDIS) {
+#else
+       if (!(readb(&i2c_regs->i2cr) & I2CR_IEN)) {
+#endif
Can you work this out from the device tree or the driver data, and avoid the #ifdef?

will use driver data to work this out.

Quoted 55 lines

drivers/i2c/mxc_i2c.c +54 -0

+               writeb(I2CR_IEN, &i2c_regs->i2cr);
+               /* Wait for controller to be stable */
+               udelay(50);
+       }
+       if (readb(&i2c_regs->iadr) == (chip << 1))
+               writeb((chip << 1) ^ 2, &i2c_regs->iadr);
+       writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+       ret = wait_for_sr_state(i2c_regs, ST_BUS_IDLE);
+       if (ret < 0)
+               return ret;
+
+       /* Start I2C transaction */
+       temp = readb(&i2c_regs->i2cr);
+       temp |= I2CR_MSTA;
+       writeb(temp, &i2c_regs->i2cr);
+
+       ret = wait_for_sr_state(i2c_regs, ST_BUS_BUSY);
+       if (ret < 0)
+               return ret;
+
+       temp |= I2CR_MTX | I2CR_TX_NO_AK;
+       writeb(temp, &i2c_regs->i2cr);
+
+       /* write slave address */
+       ret = tx_byte(i2c_regs, chip << 1 | read);
+       if (ret < 0) {
+               i2c_imx_stop(i2c_bus);
+               return ret;
+       }
+
+       return 0;
+}
+
+static int i2c_init_transfer(struct i2c_bus *i2c_bus, u32 chip, bool read)
+{
+       int retry;
+       int ret;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+
+       for (retry = 0; retry < 3; retry++) {
+               ret = i2c_init_transfer_(i2c_bus, chip, read);
+               if (ret >= 0)
+                       return 0;
+               i2c_imx_stop(i2c_bus);
+               if (ret == -ENODEV)
+                       return ret;
+
+               debug("%s: failed for chip 0x%x retry=%d\n", __func__, chip,
+                     retry);
+               if (ret != -ERESTART)
+                       /* Disable controller */
+                       writeb(I2CR_IDIS, &i2c_regs->i2cr);
+               udelay(100);
+               if (i2c_idle_bus(i2c_bus) < 0)

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 752
Quoted 6 lines
drivers/i2c/mxc_i2c.c +6 -0
+                       break;
+       }
+
+       debug("%s: give up i2c_regs=%p\n", __func__, i2c_regs);
+       return ret;
+}
This looks very similar to the old i2c_init_transfer(). Can you create a common function and avoid copying the code?

Just want to not mix with non-DM part. Since we are migrating to DM, mix DM and non-DM will introuce more #ifdefs. I'll look into whether can use a common function to cover DM and non-DM part.

Quoted 3 lines

Also in the old function
you dealt with 'alen' (now called offset length) but I don't see that code here.

Here I suppose only support 7 bit, so I just removed alen, seems better to keep it. Will add it back and use the flags from chip->flags to determine alen.

Quoted 17 lines

drivers/i2c/mxc_i2c.c +16 -0

+
+
+static int mxc_i2c_probe_chip(struct udevice *bus, u32 chip_addr,
+                             u32 chip_flags)
+{
+       int ret;
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+
+       ret = i2c_init_transfer(i2c_bus, chip_addr, false);
+       if (ret < 0)
+               return ret;
+
+       i2c_imx_stop(i2c_bus);
+
+       return 0;
+}

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 775
Quoted 7 lines
drivers/i2c/mxc_i2c.c +7 -0
+
+static int i2c_write_data(struct i2c_bus *i2c_bus, uchar chip, uchar *buffer,
+                         int len, bool end_with_repeated_start)
+{
+       int i, ret = 0;
+
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
blank line should move up one.

ok.

Quoted 77 lines

drivers/i2c/mxc_i2c.c +76 -0

+       debug("i2c_write_data: chip=0x%x, len=0x%x\n", chip, len);
+       debug("write_data: ");
+       /* use rc for counter */
+       for (i = 0; i < len; ++i)
+               debug(" 0x%02x", buffer[i]);
+       debug("\n");
+
+       for (i = 0; i < len; i++) {
+               ret = tx_byte(i2c_regs, buffer[i]);
+               if (ret < 0) {
+                       debug("i2c_write_data(): rc=%d\n", ret);
+                       break;
+               }
+       }
+
+       if (end_with_repeated_start) {
+               /* Reuse ret */
+               ret = readb(&i2c_regs->i2cr);
+               ret |= I2CR_RSTA;
+               writeb(ret, &i2c_regs->i2cr);
+
+               ret = tx_byte(i2c_regs, (chip << 1) | 1);
+               if (ret < 0) {
+                       i2c_imx_stop(i2c_bus);
+                       return ret;
+               }
+       }
+
+       return ret;
+}
+
+static int i2c_read_data(struct i2c_bus *i2c_bus, uchar chip, uchar *buf,
+                        int len)
+{
+       int ret;
+       unsigned int temp;
+       int i;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+
+       debug("i2c_read_data: chip=0x%x, len=0x%x\n", chip, len);
+
+       /* setup bus to read data */
+       temp = readb(&i2c_regs->i2cr);
+       temp &= ~(I2CR_MTX | I2CR_TX_NO_AK);
+       if (len == 1)
+               temp |= I2CR_TX_NO_AK;
+       writeb(temp, &i2c_regs->i2cr);
+       writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+       readb(&i2c_regs->i2dr);         /* dummy read to clear ICF */
+
+       /* read data */
+       for (i = 0; i < len; i++) {
+               ret = wait_for_sr_state(i2c_regs, ST_IIF);
+               if (ret < 0) {
+                       debug("i2c_read_data(): ret=%d\n", ret);
+                       i2c_imx_stop(i2c_bus);
+                       return ret;
+               }
+
+               /*
+                * It must generate STOP before read I2DR to prevent
+                * controller from generating another clock cycle
+                */
+               if (i == (len - 1)) {
+                       i2c_imx_stop(i2c_bus);
+               } else if (i == (len - 2)) {
+                       temp = readb(&i2c_regs->i2cr);
+                       temp |= I2CR_TX_NO_AK;
+                       writeb(temp, &i2c_regs->i2cr);
+               }
+               writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+               buf[i] = readb(&i2c_regs->i2dr);
+       }
+
+       /* reuse ret for counter*/
+       for (ret = 0; ret < len; ++ret)

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 857
Quoted 6 lines
drivers/i2c/mxc_i2c.c +6 -0
+               debug(" 0x%02x", buf[ret]);
+       debug("\n");
+
+       i2c_imx_stop(i2c_bus);
+       return 0;
+}
Again we have duplicated code. While we have both driver model and non-drivel-model code co-existing, we should try to avoid this.

ok. will try to merge them into a common one.

Quoted 16 lines

drivers/i2c/mxc_i2c.c +15 -0

+
+static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+       int ret;
+
+       ret = i2c_init_transfer(i2c_bus, msg->addr, false);
+       if (ret < 0)
+               return ret;
+
+       for (; nmsgs > 0; nmsgs--, msg++) {
+               bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD);
+               debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len);
+               if (msg->flags & I2C_M_RD)
+                       ret = i2c_read_data(i2c_bus, msg->addr, msg->buf,

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 879

Quoted 7 lines

drivers/i2c/mxc_i2c.c +7 -0

+                                           msg->len);
+               else
+                       ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
+                                            msg->len, next_is_read);
+               if (ret) {
+                       debug("i2c_write: error sending\n");
+                       return -EREMOTEIO;

return ret? Is the error the wrong value?

should return ret. Thanks for correcting me.

Quoted 29 lines

drivers/i2c/mxc_i2c.c +28 -0

+               }
+       }
+
+       i2c_imx_stop(i2c_bus);
+
+       return 0;
+}
+
+static const struct dm_i2c_ops mxc_i2c_ops = {
+       .xfer           = mxc_i2c_xfer,
+       .probe_chip     = mxc_i2c_probe_chip,
+       .set_bus_speed  = mxc_i2c_set_bus_speed,
+};
+
+static const struct udevice_id mxc_i2c_ids[] = {
+       { .compatible = "fsl,imx21-i2c", },
+       { .compatible = "fsl,vf610-i2c", },
+       {}
+};
+
+U_BOOT_DRIVER(i2c_mxc) = {
+       .name = "i2c_mxc",
+       .id = UCLASS_I2C,
+       .of_match = mxc_i2c_ids,
+       .probe = mxc_i2c_probe,
+       .priv_auto_alloc_size = sizeof(struct i2c_bus),
+       .ops = &mxc_i2c_ops,
+};

Quoted 9 lines

Quoted 5 lines
drivers/i2c/mxc_i2c.c +1 -1
+#endif
--
1.8.4
Regards,
Simon
.

Regards,
Peng.

Simon Glass

2015-04-24 04:40 UTC #4 lore raw

PATCH 1/1 dm: i2c: mxc support DM

Hi Peng,

Quoted 7 lines · Peng Fan

Hi Simon,
Thanks for reviewing. I'll address most comments and try to merge DM and non-DM part into one. will send out v2 for review.
The only unsure part is bus_i2c_init, I also reply you inline. I want to pass force_idle_bus and pinmux setting to i2c driver, so i use bus_i2c_init, same with non-DM way.

We should not pass functions to driver model. It is better to just have a weak function or something like that, that your driver calls out to.

With pinmux, you should be able to encode it in the device tree. If not, again I think it is the lesser of two evils to call out to a separate function.

If we get a pinctl uclass one day then you could move it over then.

Quoted 191 lines · Simon Glass

Quoted 53 lines
Hi Peng,
Quoted 49 lines · Peng Fan
Add support when CONFIG_DM_I2C configured.
Test results:
=> i2c dev 0
Setting bus to 0
=> i2c probe
Valid chip addresses: 08 50
=> i2c md 8 38
0038: 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 ................
=> i2c mw 8 38 5 1
=> i2c md 8 38
0038: 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 ................
=> dm tree
Class Probed Name
----------------------------------------
  root        [ + ]    root_driver
  thermal     [   ]    |-- imx_thermal
  simple_bus  [ + ]    |-- soc
  simple_bus  [   ]    |   |-- aips-bus at 30000000
  simple_bus  [   ]    |   |   |-- anatop at 30360000
  simple_bus  [   ]    |   |   `-- snvs at 30370000
  simple_bus  [   ]    |   |-- aips-bus at 30400000
  simple_bus  [ + ]    |   `-- aips-bus at 30800000
  i2c         [ + ]    |       |-- i2c at 30a20000
  i2c_generic [ + ]    |       |   |-- generic_8
  i2c_generic [ + ]    |       |   `-- generic_50
  i2c         [   ]    |       |-- i2c at 30a40000
  spi         [   ]    |       `-- qspi at 30bb0000
  simple_bus  [   ]    `-- regulators
Signed-off-by Peng Fan <[email protected]>
---
  arch/arm/imx-common/i2c-mxv7.c            |   4 +
  arch/arm/include/asm/imx-common/mxc_i2c.h |   5 +
  drivers/i2c/mxc_i2c.c                     | 354
++++++++++++++++++++++++++++++
  3 files changed, 363 insertions(+)
arch/arm/imx-common/i2c-mxv7.c +0 -0
index 1a632e7..99fe112 100644
@@ -99,8 +99,12 @@ int setup_i2c(unsigned i2c_index, int speed, int
 slave_addr,
         if (ret)
                 goto err_idle;
 
Quoted 11 lines
arch/arm/imx-common/i2c-mxv7.c int setup_i2c(unsigned i2c_index, int speed, int slave_addr, · line 106
Quoted 5 lines
arch/arm/imx-common/i2c-mxv7.c +3 -0
+#ifndef CONFIG_DM_I2C
         bus_i2c_init(i2c_bases[i2c_index], speed, slave_addr,
                         force_idle_bus, p);
+#else
+       bus_i2c_init(i2c_index, speed, slave_addr, force_idle_bus, p);
One of the goals of driver model is to remove code like this, and have the devices init themselves when they are used. Here you are probing each device and then changing its configuration outside the device's probe() method. This should not be needed with driver model. See below.
Agree. But i want to pass force_idle_bus and pinmux settings(parameter p) to i2c driver. I did not find a good way how to pass these two to DM mxc_i2c driver.
Quoted 53 lines
Quoted 53 lines
arch/arm/imx-common/i2c-mxv7.c +1 -0
+#endif
 
         return 0;
arch/arm/include/asm/imx-common/mxc_i2c.h +5 -0
index af86163..8f9c83e 100644
@@ -54,8 +54,13 @@ struct i2c_pads_info {
 
  int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
               struct i2c_pads_info *p);
+#ifndef CONFIG_DM_I2C
  void bus_i2c_init(void *base, int speed, int slave_addr,
                 int (*idle_bus_fn)(void *p), void *p);
+#else
+void bus_i2c_init(int index, int speed, int slave_addr,
+               int (*idle_bus_fn)(void *p), void *p);
+#endif
  int bus_i2c_read(void *base, uchar chip, uint addr, int alen, uchar
 *buf,
                 int len);
int bus_i2c_write(void *base, uchar chip, uint addr, int alen,
drivers/i2c/mxc_i2c.c +3 -0
index fc5ee35..9488e24 100644
@@ -21,6 +21,8 @@
  #include <asm/io.h>
  #include <i2c.h>
  #include <watchdog.h>
+#include <dm.h>
+#include <fdtdec.h>
 
  DECLARE_GLOBAL_DATA_PTR;
 
@@ -224,6 +226,7 @@ static int tx_byte(struct mxc_i2c_regs *i2c_regs, u8
 byte)
         return 0;
  }
 
+#ifndef CONFIG_DM_I2C
  /*
   * Stop I2C transaction
   */
drivers/i2c/mxc_i2c.c +1 -0
@@ -552,3 +555,354 @@ U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init,
 mxc_i2c_probe,
                          CONFIG_SYS_MXC_I2C3_SPEED,
                          CONFIG_SYS_MXC_I2C3_SLAVE, 2)
  #endif
+#else
Quoted 8 lines
Quoted 5 lines
+/*
+ * Information about one i2c bus
+ * struct i2c_bus - information about the i2c[x] bus
+ *
+ * @id: Index of i2c bus
What do you mean by 'index'? Is this actually used? I think you should drop this. See dev->seq for a probed device.
Yeah, it is not used. Will remove it.
Quoted 6 lines
Quoted 4 lines
+ * @speed: Speed of i2c bus
+ * @driver_data: Flags for different platforms, not used now.
You could drop it, or change to ulong.
Will change it to ulong, and use it cover the I2C_QUIRK_REG.
Quoted 6 lines
\> + * @regs: Pointer, the address of i2c bus
Quoted 2 lines
+ * @idle_bus_fn: function to force bus idle
We should not call functions like this in driver model.
I can not follow you about this. idle_bus_fn is used to force bus idle, when something err during data transfer.
Quoted 28 lines
Quoted 28 lines
+ * @idle_bus_data: parameter for idle_bus_fun
+ */
drivers/i2c/mxc_i2c.c +25 -0
+struct i2c_bus {
+       int id;
+       int speed;
+       int pinmux_config;
+       int driver_data;
+       struct mxc_i2c_regs *regs;
+       int (*idle_bus_fn)(void *p);
+       void *idle_bus_data;
+};
+
+/*
+ * Stop I2C transaction
+ */
+static void i2c_imx_stop(struct i2c_bus *i2c_bus)
+{
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+       int ret;
+       unsigned int temp = readb(&i2c_regs->i2cr);
+
+       temp &= ~(I2CR_MSTA | I2CR_MTX);
+       writeb(temp, &i2c_regs->i2cr);
+       ret = wait_for_sr_state(i2c_regs, ST_BUS_IDLE);
+       if (ret < 0)
+               debug("%s:trigger stop failed\n", __func__);
+       return;
Quoted 8 lines
drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 600
Quoted 6 lines
drivers/i2c/mxc_i2c.c +6 -0
+}
+
+static int i2c_idle_bus(struct i2c_bus *i2c_bus)
+{
+       if (i2c_bus && i2c_bus->idle_bus_fn)
+               return i2c_bus->idle_bus_fn(i2c_bus->idle_bus_data);
Can you explain what this does?

Quoted 3 lines

Used to force bus idle when something err during data transfer. You can see arch/arm/imx-common/i2c-mxv7.c, there is a function "force_idle_bus" which is used to force i2c bus idle using gpio mode.

This could in principle be done generically. If you specify the pins which are used by the I2C, these could be flipped to GPIO mode for the force_bus_idle, then flipped back to I2C mode later, using a pinctl library. Anyway, for now just calling the function seems OK, and no need for a function pointer as it just pretends that this is the way we want it ultimately IMO.

Quoted 195 lines

Quoted 9 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 605
Quoted 7 lines
drivers/i2c/mxc_i2c.c +5 -0
+
+       return 0;
+}
+
+static void i2c_init_controller(struct i2c_bus *i2c_bus)
Drop this function? It seems to do nothing.

ok.

Quoted 25 lines

drivers/i2c/mxc_i2c.c +10 -0

+{
+       if (!i2c_bus->speed)
+               return;
+
+       debug("%s: speed=%d\n", __func__, i2c_bus->speed);
+
+       return;
+}
+
+static int mxc_i2c_set_bus_speed(struct udevice *bus, unsigned int

speed)

drivers/i2c/mxc_i2c.c +13 -0

+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+
+       return bus_i2c_set_bus_speed(i2c_bus->regs, speed);
+}
+
+static int mxc_i2c_probe(struct udevice *bus)
+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+       fdt_addr_t addr;
+
+       i2c_bus->id = bus->seq;
+       /*

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 634
Quoted 6 lines
drivers/i2c/mxc_i2c.c +6 -0
+        * driver_dats is not used now, later we can use driver data
+        * to cover I2C_QUIRK_REG and etc.
+        *
+        * TODO
+        */
+       i2c_bus->driver_data = dev_get_of_data(bus);
dev_get_driver_data() now.

ok.

Quoted 39 lines

drivers/i2c/mxc_i2c.c +9 -0

+
+       addr = dev_get_addr(bus);
+       if (addr == FDT_ADDR_T_NONE)
+               return -ENODEV;
+
+       i2c_bus->regs = (struct mxc_i2c_regs *)addr;
+
+       /*
+        * Pinmux settings are in board file now, until pinmux is

supported,

drivers/i2c/mxc_i2c.c +14 -0

+        * we can set pinmux here in probe function.
+        *
+        * TODO: Pinmux
+        */
+
+       i2c_init_controller(i2c_bus);
+       debug("i2c : controller bus %d at %p , speed %d: ",
+             bus->seq, i2c_bus->regs,
+             i2c_bus->speed);
+
+       return 0;
+}
+
+void bus_i2c_init(int busnum, int speed, int slave, int

(*idle_bus_fn)(void *p),

drivers/i2c/mxc_i2c.c +13 -0

+                 void *idle_bus_data)
+{
+       struct udevice *bus;
+       struct i2c_bus *i2c_bus;
+       int ret;
+
+       ret = uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus);
+       if (ret) {
+               debug("Cannot find I2C bus %d\n", busnum);
+               return;
+       }
+
+       i2c_bus = dev_get_priv(bus);

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 675
Quoted 5 lines
drivers/i2c/mxc_i2c.c +5 -0
+
+       i2c_bus->idle_bus_fn = idle_bus_fn;
+       i2c_bus->idle_bus_data = idle_bus_data;
+
+       mxc_i2c_set_bus_speed(bus, speed);
This should move into your probe function. You should not need bus_i2c_init(). See for example tegra_i2c_probe().

I want to use "force_bus_idle" and pinmux settings, but I did not find a good way to pass force_bus_idle to the i2c driver. bus_i2c_init is called from arch/arm/imx-common/i2c-mxv7.c

Quoted 14 lines

drivers/i2c/mxc_i2c.c +12 -0

+
+       return;
+}
+
+static int i2c_init_transfer_(struct i2c_bus *i2c_bus, u32 chip,
+                             bool read)
+{
+       unsigned int temp;
+       int ret;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+
+       /* Enable I2C controller */

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 692
Quoted 5 lines
drivers/i2c/mxc_i2c.c +5 -0
+#ifdef I2C_QUIRK_REG
+       if (readb(&i2c_regs->i2cr) & I2CR_IDIS) {
+#else
+       if (!(readb(&i2c_regs->i2cr) & I2CR_IEN)) {
+#endif
Can you work this out from the device tree or the driver data, and avoid the #ifdef?

will use driver data to work this out.

Quoted 58 lines

drivers/i2c/mxc_i2c.c +34 -0

+               writeb(I2CR_IEN, &i2c_regs->i2cr);
+               /* Wait for controller to be stable */
+               udelay(50);
+       }
+       if (readb(&i2c_regs->iadr) == (chip << 1))
+               writeb((chip << 1) ^ 2, &i2c_regs->iadr);
+       writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+       ret = wait_for_sr_state(i2c_regs, ST_BUS_IDLE);
+       if (ret < 0)
+               return ret;
+
+       /* Start I2C transaction */
+       temp = readb(&i2c_regs->i2cr);
+       temp |= I2CR_MSTA;
+       writeb(temp, &i2c_regs->i2cr);
+
+       ret = wait_for_sr_state(i2c_regs, ST_BUS_BUSY);
+       if (ret < 0)
+               return ret;
+
+       temp |= I2CR_MTX | I2CR_TX_NO_AK;
+       writeb(temp, &i2c_regs->i2cr);
+
+       /* write slave address */
+       ret = tx_byte(i2c_regs, chip << 1 | read);
+       if (ret < 0) {
+               i2c_imx_stop(i2c_bus);
+               return ret;
+       }
+
+       return 0;
+}
+
+static int i2c_init_transfer(struct i2c_bus *i2c_bus, u32 chip, bool

read)

drivers/i2c/mxc_i2c.c +14 -0

+{
+       int retry;
+       int ret;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+
+       for (retry = 0; retry < 3; retry++) {
+               ret = i2c_init_transfer_(i2c_bus, chip, read);
+               if (ret >= 0)
+                       return 0;
+               i2c_imx_stop(i2c_bus);
+               if (ret == -ENODEV)
+                       return ret;
+
+               debug("%s: failed for chip 0x%x retry=%d\n", __func__,

chip,

drivers/i2c/mxc_i2c.c +7 -0

+                     retry);
+               if (ret != -ERESTART)
+                       /* Disable controller */
+                       writeb(I2CR_IDIS, &i2c_regs->i2cr);
+               udelay(100);
+               if (i2c_idle_bus(i2c_bus) < 0)
+                       break;

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 752
Quoted 5 lines
drivers/i2c/mxc_i2c.c +5 -0
+       }
+
+       debug("%s: give up i2c_regs=%p\n", __func__, i2c_regs);
+       return ret;
+}
This looks very similar to the old i2c_init_transfer(). Can you create a common function and avoid copying the code?

Just want to not mix with non-DM part. Since we are migrating to DM, mix DM and non-DM will introuce more #ifdefs. I'll look into whether can use a common function to cover DM and non-DM part.

Quoted 1 line

Quoted 8 lines

Quoted 3 lines
Also in the old function
you dealt with 'alen' (now called offset length) but I don't see that code here.
Here I suppose only support 7 bit, so I just removed alen, seems better to keep it. Will add it back and use the flags from chip->flags to determine alen.

I think alen is actually the number of bytes needed to specify a register address in the I2C chip. With driver model I called that offset_length to avoid confusion with the 7/10-bit address. Shoult if I have this wrong.

Quoted 176 lines

Quoted 19 lines

drivers/i2c/mxc_i2c.c +18 -0

+
+
+static int mxc_i2c_probe_chip(struct udevice *bus, u32 chip_addr,
+                             u32 chip_flags)
+{
+       int ret;
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+
+       ret = i2c_init_transfer(i2c_bus, chip_addr, false);
+       if (ret < 0)
+               return ret;
+
+       i2c_imx_stop(i2c_bus);
+
+       return 0;
+}
+
+static int i2c_write_data(struct i2c_bus *i2c_bus, uchar chip, uchar

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 775
Quoted 6 lines
*buffer,
drivers/i2c/mxc_i2c.c +5 -0
+                         int len, bool end_with_repeated_start)
+{
+       int i, ret = 0;
+
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
blank line should move up one.

ok.

Quoted 79 lines

drivers/i2c/mxc_i2c.c +32 -0

+       debug("i2c_write_data: chip=0x%x, len=0x%x\n", chip, len);
+       debug("write_data: ");
+       /* use rc for counter */
+       for (i = 0; i < len; ++i)
+               debug(" 0x%02x", buffer[i]);
+       debug("\n");
+
+       for (i = 0; i < len; i++) {
+               ret = tx_byte(i2c_regs, buffer[i]);
+               if (ret < 0) {
+                       debug("i2c_write_data(): rc=%d\n", ret);
+                       break;
+               }
+       }
+
+       if (end_with_repeated_start) {
+               /* Reuse ret */
+               ret = readb(&i2c_regs->i2cr);
+               ret |= I2CR_RSTA;
+               writeb(ret, &i2c_regs->i2cr);
+
+               ret = tx_byte(i2c_regs, (chip << 1) | 1);
+               if (ret < 0) {
+                       i2c_imx_stop(i2c_bus);
+                       return ret;
+               }
+       }
+
+       return ret;
+}
+
+static int i2c_read_data(struct i2c_bus *i2c_bus, uchar chip, uchar

*buf,

drivers/i2c/mxc_i2c.c +45 -0

+                        int len)
+{
+       int ret;
+       unsigned int temp;
+       int i;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+
+       debug("i2c_read_data: chip=0x%x, len=0x%x\n", chip, len);
+
+       /* setup bus to read data */
+       temp = readb(&i2c_regs->i2cr);
+       temp &= ~(I2CR_MTX | I2CR_TX_NO_AK);
+       if (len == 1)
+               temp |= I2CR_TX_NO_AK;
+       writeb(temp, &i2c_regs->i2cr);
+       writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+       readb(&i2c_regs->i2dr);         /* dummy read to clear ICF */
+
+       /* read data */
+       for (i = 0; i < len; i++) {
+               ret = wait_for_sr_state(i2c_regs, ST_IIF);
+               if (ret < 0) {
+                       debug("i2c_read_data(): ret=%d\n", ret);
+                       i2c_imx_stop(i2c_bus);
+                       return ret;
+               }
+
+               /*
+                * It must generate STOP before read I2DR to prevent
+                * controller from generating another clock cycle
+                */
+               if (i == (len - 1)) {
+                       i2c_imx_stop(i2c_bus);
+               } else if (i == (len - 2)) {
+                       temp = readb(&i2c_regs->i2cr);
+                       temp |= I2CR_TX_NO_AK;
+                       writeb(temp, &i2c_regs->i2cr);
+               }
+               writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+               buf[i] = readb(&i2c_regs->i2dr);
+       }
+
+       /* reuse ret for counter*/
+       for (ret = 0; ret < len; ++ret)
+               debug(" 0x%02x", buf[ret]);

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 857
Quoted 5 lines
drivers/i2c/mxc_i2c.c +5 -0
+       debug("\n");
+
+       i2c_imx_stop(i2c_bus);
+       return 0;
+}
Again we have duplicated code. While we have both driver model and non-drivel-model code co-existing, we should try to avoid this.

ok. will try to merge them into a common one.

Quoted 22 lines

drivers/i2c/mxc_i2c.c +2 -0

+
+static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int

nmsgs)

drivers/i2c/mxc_i2c.c +10 -0

+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+       int ret;
+
+       ret = i2c_init_transfer(i2c_bus, msg->addr, false);
+       if (ret < 0)
+               return ret;
+
+       for (; nmsgs > 0; nmsgs--, msg++) {
+               bool next_is_read = nmsgs > 1 && (msg[1].flags &

I2C_M_RD);
+ debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr,
msg->len);

drivers/i2c/mxc_i2c.c +4 -0

+               if (msg->flags & I2C_M_RD)
+                       ret = i2c_read_data(i2c_bus, msg->addr, msg->buf,
+                                           msg->len);
+               else

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 879
Quoted 6 lines
+ ret = i2c_write_data(i2c_bus, msg->addr,
msg->buf,
drivers/i2c/mxc_i2c.c +4 -0
+                                            msg->len, next_is_read);
+               if (ret) {
+                       debug("i2c_write: error sending\n");
+                       return -EREMOTEIO;
return ret? Is the error the wrong value?

should return ret. Thanks for correcting me.

Quoted 25 lines

drivers/i2c/mxc_i2c.c +23 -0

+               }
+       }
+
+       i2c_imx_stop(i2c_bus);
+
+       return 0;
+}
+
+static const struct dm_i2c_ops mxc_i2c_ops = {
+       .xfer           = mxc_i2c_xfer,
+       .probe_chip     = mxc_i2c_probe_chip,
+       .set_bus_speed  = mxc_i2c_set_bus_speed,
+};
+
+static const struct udevice_id mxc_i2c_ids[] = {
+       { .compatible = "fsl,imx21-i2c", },
+       { .compatible = "fsl,vf610-i2c", },
+       {}
+};
+
+U_BOOT_DRIVER(i2c_mxc) = {
+       .name = "i2c_mxc",
+       .id = UCLASS_I2C,

Quoted 8 lines

drivers/i2c/mxc_i2c.c +6 -1

+       .of_match = mxc_i2c_ids,
+       .probe = mxc_i2c_probe,
+       .priv_auto_alloc_size = sizeof(struct i2c_bus),
+       .ops = &mxc_i2c_ops,
+};
+#endif
--

1.8.4

Regards,
Simon

Peng Fan

author

2015-04-26 07:37 UTC #5 lore raw

PATCH 1/1 dm: i2c: mxc support DM

Hi Simon,

I missed this mail, and sent out a V2 version patch which still has bus_i2c_init there, but refactors the driver and support dm,
https://patchwork.ozlabs.org/patch/464544/.
I'll send out a V3 version soon.

Quoted 5 lines · Simon Glass

Hi Peng,
Quoted 2 lines · Peng Fan
Hi Simon,

Quoted 8 lines

Quoted 5 lines
Thanks for reviewing. I'll address most comments and try to merge DM and non-DM part into one. will send out v2 for review.
The only unsure part is bus_i2c_init, I also reply you inline. I want to pass force_idle_bus and pinmux setting to i2c driver, so i use bus_i2c_init, same with non-DM way.
We should not pass functions to driver model. It is better to just have a weak function or something like that, that your driver calls out to.

Agree.

Quoted 6 lines

With pinmux, you should be able to encode it in the device tree. If not, again I think it is the lesser of two evils to call out to a separate function.
If we get a pinctl uclass one day then you could move it over then.

Yeah.
About the weak function, I reply inline.

Quoted 177 lines

Quoted 174 lines · Simon Glass
Quoted 51 lines
Hi Peng,
Quoted 48 lines · Peng Fan
Add support when CONFIG_DM_I2C configured.
Test results:
=> i2c dev 0
Setting bus to 0
=> i2c probe
Valid chip addresses: 08 50
=> i2c md 8 38
0038: 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 ................
=> i2c mw 8 38 5 1
=> i2c md 8 38
0038: 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 ................
=> dm tree
Class Probed Name
----------------------------------------
   root        [ + ]    root_driver
   thermal     [   ]    |-- imx_thermal
   simple_bus  [ + ]    |-- soc
   simple_bus  [   ]    |   |-- aips-bus at 30000000
   simple_bus  [   ]    |   |   |-- anatop at 30360000
   simple_bus  [   ]    |   |   `-- snvs at 30370000
   simple_bus  [   ]    |   |-- aips-bus at 30400000
   simple_bus  [ + ]    |   `-- aips-bus at 30800000
   i2c         [ + ]    |       |-- i2c at 30a20000
   i2c_generic [ + ]    |       |   |-- generic_8
   i2c_generic [ + ]    |       |   `-- generic_50
   i2c         [   ]    |       |-- i2c at 30a40000
   spi         [   ]    |       `-- qspi at 30bb0000
   simple_bus  [   ]    `-- regulators
Signed-off-by Peng Fan <[email protected]>
---
   arch/arm/imx-common/i2c-mxv7.c            |   4 +
   arch/arm/include/asm/imx-common/mxc_i2c.h |   5 +
   drivers/i2c/mxc_i2c.c                     | 354
++++++++++++++++++++++++++++++
   3 files changed, 363 insertions(+)
arch/arm/imx-common/i2c-mxv7.c +0 -0
index 1a632e7..99fe112 100644
@@ -99,8 +99,12 @@ int setup_i2c(unsigned i2c_index, int speed, int
 slave_addr,
          if (ret)
                  goto err_idle;
 
Quoted 10 lines
arch/arm/imx-common/i2c-mxv7.c int setup_i2c(unsigned i2c_index, int speed, int slave_addr, · line 106
Quoted 5 lines
arch/arm/imx-common/i2c-mxv7.c +3 -0
+#ifndef CONFIG_DM_I2C
          bus_i2c_init(i2c_bases[i2c_index], speed, slave_addr,
                          force_idle_bus, p);
+#else
+       bus_i2c_init(i2c_index, speed, slave_addr, force_idle_bus, p);
One of the goals of driver model is to remove code like this, and have the devices init themselves when they are used. Here you are probing each device and then changing its configuration outside the device's probe() method. This should not be needed with driver model. See below.
Agree. But i want to pass force_idle_bus and pinmux settings(parameter p) to i2c driver. I did not find a good way how to pass these two to DM mxc_i2c driver.
Quoted 51 lines
Quoted 51 lines
arch/arm/imx-common/i2c-mxv7.c +1 -0
+#endif
 
          return 0;
arch/arm/include/asm/imx-common/mxc_i2c.h +5 -0
index af86163..8f9c83e 100644
@@ -54,8 +54,13 @@ struct i2c_pads_info {
 
   int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
                struct i2c_pads_info *p);
+#ifndef CONFIG_DM_I2C
   void bus_i2c_init(void *base, int speed, int slave_addr,
                  int (*idle_bus_fn)(void *p), void *p);
+#else
+void bus_i2c_init(int index, int speed, int slave_addr,
+               int (*idle_bus_fn)(void *p), void *p);
+#endif
   int bus_i2c_read(void *base, uchar chip, uint addr, int alen, uchar
 *buf,
                  int len);
int bus_i2c_write(void *base, uchar chip, uint addr, int alen,
drivers/i2c/mxc_i2c.c +3 -0
index fc5ee35..9488e24 100644
@@ -21,6 +21,8 @@
   #include <asm/io.h>
   #include <i2c.h>
   #include <watchdog.h>
+#include <dm.h>
+#include <fdtdec.h>
 
   DECLARE_GLOBAL_DATA_PTR;
 
@@ -224,6 +226,7 @@ static int tx_byte(struct mxc_i2c_regs *i2c_regs, u8
 byte)
          return 0;
   }
 
+#ifndef CONFIG_DM_I2C
   /*
    * Stop I2C transaction
    */
drivers/i2c/mxc_i2c.c +0 -0
@@ -552,3 +555,354 @@ U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init,
 mxc_i2c_probe,
                           CONFIG_SYS_MXC_I2C3_SPEED,
                           CONFIG_SYS_MXC_I2C3_SLAVE, 2)
   #endif
Quoted 8 lines
drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 563
Quoted 6 lines
drivers/i2c/mxc_i2c.c +6 -0
+#else
+/*
+ * Information about one i2c bus
+ * struct i2c_bus - information about the i2c[x] bus
+ *
+ * @id: Index of i2c bus
What do you mean by 'index'? Is this actually used? I think you should drop this. See dev->seq for a probed device.
Yeah, it is not used. Will remove it.
Quoted 4 lines
Quoted 3 lines
+ * @speed: Speed of i2c bus
+ * @driver_data: Flags for different platforms, not used now.
You could drop it, or change to ulong.
Will change it to ulong, and use it cover the I2C_QUIRK_REG.
Quoted 3 lines
\> + * @regs: Pointer, the address of i2c bus
Quoted 1 line
+ * @idle_bus_fn: function to force bus idle
We should not call functions like this in driver model.
I can not follow you about this. idle_bus_fn is used to force bus idle, when something err during data transfer.
Quoted 26 lines
Quoted 26 lines
+ * @idle_bus_data: parameter for idle_bus_fun
+ */
drivers/i2c/mxc_i2c.c +24 -0
+struct i2c_bus {
+       int id;
+       int speed;
+       int pinmux_config;
+       int driver_data;
+       struct mxc_i2c_regs *regs;
+       int (*idle_bus_fn)(void *p);
+       void *idle_bus_data;
+};
+
+/*
+ * Stop I2C transaction
+ */
+static void i2c_imx_stop(struct i2c_bus *i2c_bus)
+{
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+       int ret;
+       unsigned int temp = readb(&i2c_regs->i2cr);
+
+       temp &= ~(I2CR_MSTA | I2CR_MTX);
+       writeb(temp, &i2c_regs->i2cr);
+       ret = wait_for_sr_state(i2c_regs, ST_BUS_IDLE);
+       if (ret < 0)
+               debug("%s:trigger stop failed\n", __func__);
Quoted 8 lines
drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 600
Quoted 7 lines
drivers/i2c/mxc_i2c.c +7 -0
+       return;
+}
+
+static int i2c_idle_bus(struct i2c_bus *i2c_bus)
+{
+       if (i2c_bus && i2c_bus->idle_bus_fn)
+               return i2c_bus->idle_bus_fn(i2c_bus->idle_bus_data);
Can you explain what this does?
Quoted 3 lines
Used to force bus idle when something err during data transfer. You can see arch/arm/imx-common/i2c-mxv7.c, there is a function "force_idle_bus" which is used to force i2c bus idle using gpio mode.

Quoted 6 lines

This could in principle be done generically. If you specify the pins which are used by the I2C, these could be flipped to GPIO mode for the force_bus_idle, then flipped back to I2C mode later, using a pinctl library. Anyway, for now just calling the function seems OK, and no need for a function pointer as it just pretends that this is the way we want it ultimately IMO.

The i.MX linux i2c driver using this way:
504 pinctrl_i2c1: i2c1grp {
505 fsl,pins = <
506 MX6SX_PAD_GPIO1_IO01__I2C1_SDA 0x4001b8b1
507 MX6SX_PAD_GPIO1_IO00__I2C1_SCL 0x4001b8b1
508 >;
509 }
It does not contain GPIO mode. But uboot mxc_i2c is different from linux imx i2c driver, I think better add GPIO mode for uboot mxc_i2c like this:
504 pinctrl_i2c1: i2c1grp {
505 fsl,pins = <
506 MX6SX_PAD_GPIO1_IO01__I2C1_SDA 0x4001b8b1
507 MX6SX_PAD_GPIO1_IO01__GPIO1_IO01 0x001b8b1
508 MX6SX_PAD_GPIO1_IO00__I2C1_SCL 0x4001b8b1
509 MX6SX_PAD_GPIO1_IO00__GPIO1_IO00 0x001b8b1
510 >;
509 }
To mxc_i2c DM, we can convert this dts to i2c_pad_info structure. And pass i2c_pad_info to force_bus_idle(void *priv), like this:
"force_bus_idle((void*) i2c_bus->pad_info)". Since force_bus_idle is static function in arch/arm/imx-common/i2c-mxc7.c, need to discard the static type.

I agree to introduce a weak function for force_bus_idle.

Quoted 370 lines

Quoted 180 lines

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 605
Quoted 7 lines
drivers/i2c/mxc_i2c.c +5 -0
+
+       return 0;
+}
+
+static void i2c_init_controller(struct i2c_bus *i2c_bus)
Drop this function? It seems to do nothing.

ok.

Quoted 23 lines

drivers/i2c/mxc_i2c.c +10 -0

+{
+       if (!i2c_bus->speed)
+               return;
+
+       debug("%s: speed=%d\n", __func__, i2c_bus->speed);
+
+       return;
+}
+
+static int mxc_i2c_set_bus_speed(struct udevice *bus, unsigned int

speed)

drivers/i2c/mxc_i2c.c +12 -0

+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+
+       return bus_i2c_set_bus_speed(i2c_bus->regs, speed);
+}
+
+static int mxc_i2c_probe(struct udevice *bus)
+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+       fdt_addr_t addr;
+
+       i2c_bus->id = bus->seq;

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 634
Quoted 7 lines
drivers/i2c/mxc_i2c.c +7 -0
+       /*
+        * driver_dats is not used now, later we can use driver data
+        * to cover I2C_QUIRK_REG and etc.
+        *
+        * TODO
+        */
+       i2c_bus->driver_data = dev_get_of_data(bus);
dev_get_driver_data() now.

ok.

Quoted 37 lines

drivers/i2c/mxc_i2c.c +9 -0

+
+       addr = dev_get_addr(bus);
+       if (addr == FDT_ADDR_T_NONE)
+               return -ENODEV;
+
+       i2c_bus->regs = (struct mxc_i2c_regs *)addr;
+
+       /*
+        * Pinmux settings are in board file now, until pinmux is

supported,

drivers/i2c/mxc_i2c.c +14 -0

+        * we can set pinmux here in probe function.
+        *
+        * TODO: Pinmux
+        */
+
+       i2c_init_controller(i2c_bus);
+       debug("i2c : controller bus %d at %p , speed %d: ",
+             bus->seq, i2c_bus->regs,
+             i2c_bus->speed);
+
+       return 0;
+}
+
+void bus_i2c_init(int busnum, int speed, int slave, int

(*idle_bus_fn)(void *p),

drivers/i2c/mxc_i2c.c +12 -0

+                 void *idle_bus_data)
+{
+       struct udevice *bus;
+       struct i2c_bus *i2c_bus;
+       int ret;
+
+       ret = uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus);
+       if (ret) {
+               debug("Cannot find I2C bus %d\n", busnum);
+               return;
+       }
+

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 675
Quoted 6 lines
drivers/i2c/mxc_i2c.c +6 -0
+       i2c_bus = dev_get_priv(bus);
+
+       i2c_bus->idle_bus_fn = idle_bus_fn;
+       i2c_bus->idle_bus_data = idle_bus_data;
+
+       mxc_i2c_set_bus_speed(bus, speed);
This should move into your probe function. You should not need bus_i2c_init(). See for example tegra_i2c_probe().

I want to use "force_bus_idle" and pinmux settings, but I did not find a good way to pass force_bus_idle to the i2c driver. bus_i2c_init is called from arch/arm/imx-common/i2c-mxv7.c

Quoted 12 lines

drivers/i2c/mxc_i2c.c +11 -0

+
+       return;
+}
+
+static int i2c_init_transfer_(struct i2c_bus *i2c_bus, u32 chip,
+                             bool read)
+{
+       unsigned int temp;
+       int ret;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 692
Quoted 6 lines
drivers/i2c/mxc_i2c.c +6 -0
+       /* Enable I2C controller */
+#ifdef I2C_QUIRK_REG
+       if (readb(&i2c_regs->i2cr) & I2CR_IDIS) {
+#else
+       if (!(readb(&i2c_regs->i2cr) & I2CR_IEN)) {
+#endif
Can you work this out from the device tree or the driver data, and avoid the #ifdef?

will use driver data to work this out.

Quoted 56 lines

drivers/i2c/mxc_i2c.c +34 -0

+               writeb(I2CR_IEN, &i2c_regs->i2cr);
+               /* Wait for controller to be stable */
+               udelay(50);
+       }
+       if (readb(&i2c_regs->iadr) == (chip << 1))
+               writeb((chip << 1) ^ 2, &i2c_regs->iadr);
+       writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+       ret = wait_for_sr_state(i2c_regs, ST_BUS_IDLE);
+       if (ret < 0)
+               return ret;
+
+       /* Start I2C transaction */
+       temp = readb(&i2c_regs->i2cr);
+       temp |= I2CR_MSTA;
+       writeb(temp, &i2c_regs->i2cr);
+
+       ret = wait_for_sr_state(i2c_regs, ST_BUS_BUSY);
+       if (ret < 0)
+               return ret;
+
+       temp |= I2CR_MTX | I2CR_TX_NO_AK;
+       writeb(temp, &i2c_regs->i2cr);
+
+       /* write slave address */
+       ret = tx_byte(i2c_regs, chip << 1 | read);
+       if (ret < 0) {
+               i2c_imx_stop(i2c_bus);
+               return ret;
+       }
+
+       return 0;
+}
+
+static int i2c_init_transfer(struct i2c_bus *i2c_bus, u32 chip, bool

read)

drivers/i2c/mxc_i2c.c +14 -0

+{
+       int retry;
+       int ret;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+
+       for (retry = 0; retry < 3; retry++) {
+               ret = i2c_init_transfer_(i2c_bus, chip, read);
+               if (ret >= 0)
+                       return 0;
+               i2c_imx_stop(i2c_bus);
+               if (ret == -ENODEV)
+                       return ret;
+
+               debug("%s: failed for chip 0x%x retry=%d\n", __func__,

chip,

drivers/i2c/mxc_i2c.c +6 -0

+                     retry);
+               if (ret != -ERESTART)
+                       /* Disable controller */
+                       writeb(I2CR_IDIS, &i2c_regs->i2cr);
+               udelay(100);
+               if (i2c_idle_bus(i2c_bus) < 0)

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 752
Quoted 6 lines
drivers/i2c/mxc_i2c.c +6 -0
+                       break;
+       }
+
+       debug("%s: give up i2c_regs=%p\n", __func__, i2c_regs);
+       return ret;
+}
This looks very similar to the old i2c_init_transfer(). Can you create a common function and avoid copying the code?

Just want to not mix with non-DM part. Since we are migrating to DM, mix DM and non-DM will introuce more #ifdefs. I'll look into whether can use a common function to cover DM and non-DM part.

Quoted 7 lines

Quoted 3 lines
Also in the old function
you dealt with 'alen' (now called offset length) but I don't see that code here.
Here I suppose only support 7 bit, so I just removed alen, seems better to keep it. Will add it back and use the flags from chip->flags to determine alen.

Quoted 166 lines

Quoted 17 lines

drivers/i2c/mxc_i2c.c +17 -0

+
+
+static int mxc_i2c_probe_chip(struct udevice *bus, u32 chip_addr,
+                             u32 chip_flags)
+{
+       int ret;
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+
+       ret = i2c_init_transfer(i2c_bus, chip_addr, false);
+       if (ret < 0)
+               return ret;
+
+       i2c_imx_stop(i2c_bus);
+
+       return 0;
+}
+

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 775
Quoted 7 lines
+static int i2c_write_data(struct i2c_bus *i2c_bus, uchar chip, uchar *buffer,
drivers/i2c/mxc_i2c.c +5 -0
+                         int len, bool end_with_repeated_start)
+{
+       int i, ret = 0;
+
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
blank line should move up one.

ok.

Quoted 77 lines

drivers/i2c/mxc_i2c.c +32 -0

+       debug("i2c_write_data: chip=0x%x, len=0x%x\n", chip, len);
+       debug("write_data: ");
+       /* use rc for counter */
+       for (i = 0; i < len; ++i)
+               debug(" 0x%02x", buffer[i]);
+       debug("\n");
+
+       for (i = 0; i < len; i++) {
+               ret = tx_byte(i2c_regs, buffer[i]);
+               if (ret < 0) {
+                       debug("i2c_write_data(): rc=%d\n", ret);
+                       break;
+               }
+       }
+
+       if (end_with_repeated_start) {
+               /* Reuse ret */
+               ret = readb(&i2c_regs->i2cr);
+               ret |= I2CR_RSTA;
+               writeb(ret, &i2c_regs->i2cr);
+
+               ret = tx_byte(i2c_regs, (chip << 1) | 1);
+               if (ret < 0) {
+                       i2c_imx_stop(i2c_bus);
+                       return ret;
+               }
+       }
+
+       return ret;
+}
+
+static int i2c_read_data(struct i2c_bus *i2c_bus, uchar chip, uchar

*buf,

drivers/i2c/mxc_i2c.c +44 -0

+                        int len)
+{
+       int ret;
+       unsigned int temp;
+       int i;
+       struct mxc_i2c_regs *i2c_regs = i2c_bus->regs;
+
+       debug("i2c_read_data: chip=0x%x, len=0x%x\n", chip, len);
+
+       /* setup bus to read data */
+       temp = readb(&i2c_regs->i2cr);
+       temp &= ~(I2CR_MTX | I2CR_TX_NO_AK);
+       if (len == 1)
+               temp |= I2CR_TX_NO_AK;
+       writeb(temp, &i2c_regs->i2cr);
+       writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+       readb(&i2c_regs->i2dr);         /* dummy read to clear ICF */
+
+       /* read data */
+       for (i = 0; i < len; i++) {
+               ret = wait_for_sr_state(i2c_regs, ST_IIF);
+               if (ret < 0) {
+                       debug("i2c_read_data(): ret=%d\n", ret);
+                       i2c_imx_stop(i2c_bus);
+                       return ret;
+               }
+
+               /*
+                * It must generate STOP before read I2DR to prevent
+                * controller from generating another clock cycle
+                */
+               if (i == (len - 1)) {
+                       i2c_imx_stop(i2c_bus);
+               } else if (i == (len - 2)) {
+                       temp = readb(&i2c_regs->i2cr);
+                       temp |= I2CR_TX_NO_AK;
+                       writeb(temp, &i2c_regs->i2cr);
+               }
+               writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr);
+               buf[i] = readb(&i2c_regs->i2dr);
+       }
+
+       /* reuse ret for counter*/
+       for (ret = 0; ret < len; ++ret)

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 857
Quoted 6 lines
drivers/i2c/mxc_i2c.c +6 -0
+               debug(" 0x%02x", buf[ret]);
+       debug("\n");
+
+       i2c_imx_stop(i2c_bus);
+       return 0;
+}
Again we have duplicated code. While we have both driver model and non-drivel-model code co-existing, we should try to avoid this.

ok. will try to merge them into a common one.

Quoted 20 lines

drivers/i2c/mxc_i2c.c +2 -0

+
+static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int

nmsgs)

drivers/i2c/mxc_i2c.c +10 -0

+{
+       struct i2c_bus *i2c_bus = dev_get_priv(bus);
+       int ret;
+
+       ret = i2c_init_transfer(i2c_bus, msg->addr, false);
+       if (ret < 0)
+               return ret;
+
+       for (; nmsgs > 0; nmsgs--, msg++) {
+               bool next_is_read = nmsgs > 1 && (msg[1].flags &

I2C_M_RD);
+ debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr,
msg->len);

drivers/i2c/mxc_i2c.c +3 -0

+               if (msg->flags & I2C_M_RD)
+                       ret = i2c_read_data(i2c_bus, msg->addr, msg->buf,
+                                           msg->len);

Quoted 8 lines

drivers/i2c/mxc_i2c.c U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, · line 879

Quoted 7 lines

drivers/i2c/mxc_i2c.c +2 -0

+               else
+                       ret = i2c_write_data(i2c_bus, msg->addr,

msg->buf,

drivers/i2c/mxc_i2c.c +4 -0

+                                            msg->len, next_is_read);
+               if (ret) {
+                       debug("i2c_write: error sending\n");
+                       return -EREMOTEIO;

return ret? Is the error the wrong value?

should return ret. Thanks for correcting me.

Quoted 24 lines

drivers/i2c/mxc_i2c.c +23 -0

+               }
+       }
+
+       i2c_imx_stop(i2c_bus);
+
+       return 0;
+}
+
+static const struct dm_i2c_ops mxc_i2c_ops = {
+       .xfer           = mxc_i2c_xfer,
+       .probe_chip     = mxc_i2c_probe_chip,
+       .set_bus_speed  = mxc_i2c_set_bus_speed,
+};
+
+static const struct udevice_id mxc_i2c_ids[] = {
+       { .compatible = "fsl,imx21-i2c", },
+       { .compatible = "fsl,vf610-i2c", },
+       {}
+};
+
+U_BOOT_DRIVER(i2c_mxc) = {
+       .name = "i2c_mxc",
+       .id = UCLASS_I2C,

Quoted 8 lines

drivers/i2c/mxc_i2c.c +6 -1

+       .of_match = mxc_i2c_ids,
+       .probe = mxc_i2c_probe,
+       .priv_auto_alloc_size = sizeof(struct i2c_bus),
+       .ops = &mxc_i2c_ops,
+};
+#endif
--

1.8.4

Regards,
Simon
.