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.
On 4/19/2015 9:53 PM, Simon Glass wrote:
Hi Peng,
On 15 April 2015 at 03:35, Peng Fan <peng....@freescale.com> wrote:
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@30000000
simple_bus [ ] | | |-- anatop@30360000
simple_bus [ ] | | `-- snvs@30370000
simple_bus [ ] | |-- aips-bus@30400000
simple_bus [ + ] | `-- aips-bus@30800000
i2c [ + ] | |-- i2c@30a20000
i2c_generic [ + ] | | |-- generic_8
i2c_generic [ + ] | | `-- generic_50
i2c [ ] | |-- i2c@30a40000
spi [ ] | `-- qspi@30bb0000
simple_bus [ ] `-- regulators
Signed-off-by: Peng Fan <peng....@freescale.com>
---
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(+)
diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c
index 1a632e7..99fe112 100644
--- a/arch/arm/imx-common/i2c-mxv7.c
+++ b/arch/arm/imx-common/i2c-mxv7.c
@@ -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.
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.
+#endif
return 0;
diff --git a/arch/arm/include/asm/imx-common/mxc_i2c.h
b/arch/arm/include/asm/imx-common/mxc_i2c.h
index af86163..8f9c83e 100644
--- a/arch/arm/include/asm/imx-common/mxc_i2c.h
+++ b/arch/arm/include/asm/imx-common/mxc_i2c.h
@@ -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,
diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index fc5ee35..9488e24 100644
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -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
*/
@@ -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.
+ * @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.
\> + * @regs: Pointer, the address of i2c bus
+ * @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.
+ * @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__);
+ 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.
+
+ return 0;
+}
+
+static void i2c_init_controller(struct i2c_bus *i2c_bus)
Drop this function? It seems to do nothing.
ok.
+{
+ 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;
+ /*
+ * 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.
+
+ 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;
+ }
+
+ 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
+
+ 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 */
+#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.
+ 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)
+ 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.
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.
+
+
+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 *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.
+ 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)
+ 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.
+
+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,
+ 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.
+ }
+ }
+
+ 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
--
1.8.4
Regards,
Simon
.
Regards,
Peng.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
