Re: [U-Boot] [PATCH v2 07/10] dm: i2c: s3c24x0: adjust to dm-i2c api

Hi Przemyslaw,

On 26 January 2015 at 08:21, Przemyslaw Marczak p.marczak@samsung.com wrote:

This commit adjusts the s3c24x0 driver to new i2c api based on driver-model. The driver supports standard and high-speed i2c as previous.

Tested on Trats2, Odroid U3, Arndale, Odroid XU3

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Cc: Simon Glass sjg@chromium.org Cc: Heiko Schocher hs@denx.de Cc: Minkyu Kang mk7.kang@samsung.com

Tested on snow:

Tested-by: Simon Glass sjg@chromium.org

This looks right to me, but I have a number of nits, mostly code that can be deleted, Please see below.

If you can respin this I will pick it up.

---

Changes v2:

• use consistent return values on errors
• decrease transaction status timeout, because the previous one was too big

---

drivers/i2c/s3c24x0_i2c.c | 275 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 233 insertions(+), 42 deletions(-)

diff --git a/drivers/i2c/s3c24x0_i2c.c b/drivers/i2c/s3c24x0_i2c.c index fd328f0..c82640d 100644 --- a/drivers/i2c/s3c24x0_i2c.c +++ b/drivers/i2c/s3c24x0_i2c.c @@ -9,8 +9,9 @@

• as they seem to have the same I2C controller inside.
• The different address mapping is handled by the s3c24xx.h files below.

*/

#include <common.h> +#include <errno.h> +#include <dm.h> #include <fdtdec.h> #if (defined CONFIG_EXYNOS4 || defined CONFIG_EXYNOS5) #include <asm/arch/clk.h> @@ -111,9 +112,9 @@ #define I2C_START_STOP 0x20 /* START / STOP */ #define I2C_TXRX_ENA 0x10 /* I2C Tx/Rx enable */

-#define I2C_TIMEOUT_MS 1000 /* 1 second */ +#define I2C_TIMEOUT_MS 10 /* 10 ms */

-#define HSI2C_TIMEOUT_US 100000 /* 100 ms, finer granularity */ +#define HSI2C_TIMEOUT_US 10000 /* 10 ms, finer granularity */

Why change the timeouts? In any case that should be a separate patch.

/* To support VCMA9 boards and other who dont define max_i2c_num */ @@ -121,13 +122,23 @@ #define CONFIG_MAX_I2C_NUM 1 #endif

+DECLARE_GLOBAL_DATA_PTR;

/*

• For SPL boot some boards need i2c before SDRAM is initialised so force
• variables to live in SRAM

*/ +#ifdef CONFIG_SYS_I2C static struct s3c24x0_i2c_bus i2c_bus[CONFIG_MAX_I2C_NUM] __attribute__((section(".data"))); +#endif

+enum exynos_i2c_type {

•   EXYNOS_I2C_STD,
  

•   EXYNOS_I2C_HS,
  

+};

+#ifdef CONFIG_SYS_I2C /**

• Get a pointer to the given bus index

@@ -147,6 +158,7 @@ static struct s3c24x0_i2c_bus *get_bus(unsigned int bus_idx) debug("Undefined bus: %d\n", bus_idx); return NULL; } +#endif

#if !(defined CONFIG_EXYNOS4 || defined CONFIG_EXYNOS5) static int GetI2CSDA(void) @@ -251,6 +263,7 @@ static void ReadWriteByte(struct s3c24x0_i2c *i2c) writel(readl(&i2c->iiccon) & ~I2CCON_IRPND, &i2c->iiccon); }

+#ifdef CONFIG_SYS_I2C static struct s3c24x0_i2c *get_base_i2c(int bus) { #ifdef CONFIG_EXYNOS4 @@ -267,6 +280,7 @@ static struct s3c24x0_i2c *get_base_i2c(int bus) return s3c24x0_get_base_i2c(); #endif } +#endif

static void i2c_ch_init(struct s3c24x0_i2c *i2c, int speed, int slaveadd) { @@ -326,7 +340,7 @@ static int hsi2c_get_clk_details(struct s3c24x0_i2c_bus *i2c_bus) return 0; } }

•   return -1;
  

•   return -EINVAL;
  

}

static void hsi2c_ch_init(struct s3c24x0_i2c_bus *i2c_bus) @@ -398,18 +412,20 @@ static void exynos5_i2c_reset(struct s3c24x0_i2c_bus *i2c_bus) hsi2c_ch_init(i2c_bus); }

+#ifdef CONFIG_SYS_I2C static void s3c24x0_i2c_init(struct i2c_adapter *adap, int speed, int slaveadd) { struct s3c24x0_i2c *i2c; struct s3c24x0_i2c_bus *bus;

#if !(defined CONFIG_EXYNOS4 || defined CONFIG_EXYNOS5) struct s3c24x0_gpio *gpio = s3c24x0_get_base_gpio(); #endif ulong start_time = get_timer(0);

•   /* By default i2c channel 0 is the current bus */
    i2c = get_base_i2c(adap->hwadapnr);
  

•   bus = &i2c_bus[adap->hwadapnr];
  

•   if (!bus)
  

•           return;
  
    /*
     * In case the previous transfer is still going, wait to give it a
  

@@ -470,12 +486,13 @@ static void s3c24x0_i2c_init(struct i2c_adapter *adap, int speed, int slaveadd) #endif } #endif /* #if !(defined CONFIG_EXYNOS4 || defined CONFIG_EXYNOS5) */

•   i2c_ch_init(i2c, speed, slaveadd);
  
  

•   bus = &i2c_bus[adap->hwadapnr];
    bus->active = true;
    bus->regs = i2c;
  

} +#endif /* CONFIG_SYS_I2C */

/*

• Poll the appropriate bit of the fifo status register until the interface is

@@ -698,40 +715,40 @@ static int hsi2c_read(struct exynos5_hsi2c *i2c, return rv; }

+#ifdef CONFIG_SYS_I2C static unsigned int s3c24x0_i2c_set_bus_speed(struct i2c_adapter *adap,

•                                     unsigned int speed)
  

•                                         unsigned int speed)
  

+#else +static int s3c24x0_i2c_set_bus_speed(struct udevice *dev, unsigned int speed) +#endif { struct s3c24x0_i2c_bus *i2c_bus;

+#ifdef CONFIG_SYS_I2C i2c_bus = get_bus(adap->hwadapnr); +#else

•   if (!dev)
  

•           return -ENODEV;
  

This can't happen, you can drop this check.

•   i2c_bus = dev_get_priv(dev);
  

+#endif if (!i2c_bus)

•           return -1;
  

•           return -EFAULT;
  

This can't happen for driver model, so move this check up into the #ifdef CONFIG_SYS_I2C

    i2c_bus->clock_frequency = speed;

    if (i2c_bus->is_highspeed) {
            if (hsi2c_get_clk_details(i2c_bus))

•                   return -1;
  

•                   return -EFAULT;
            hsi2c_ch_init(i2c_bus);
    } else {
            i2c_ch_init(i2c_bus->regs, i2c_bus->clock_frequency,
  

•                       CONFIG_SYS_I2C_S3C24X0_SLAVE);
  

•                               CONFIG_SYS_I2C_S3C24X0_SLAVE);
  

Should leave this line alone.

    }

    return 0;

}

-#ifdef CONFIG_EXYNOS5 -static void exynos_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) -{

•   /* This will override the speed selected in the fdt for that port */
  

•   debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
  

•   if (i2c_set_bus_speed(speed))
  

•           printf("i2c_init: failed to init bus %d for speed = %d\n",
  

•                                           adap->hwadapnr, speed);
  

-} -#endif

/*

• cmd_type is 0 for write, 1 for read.

@@ -844,15 +861,24 @@ bailout: return result; }

+#ifdef CONFIG_SYS_I2C static int s3c24x0_i2c_probe(struct i2c_adapter *adap, uchar chip) +#else +static int s3c24x0_i2c_probe(struct udevice *dev, uint chip, uint chip_flags) +#endif { struct s3c24x0_i2c_bus *i2c_bus; uchar buf[1]; int ret;

+#ifdef CONFIG_SYS_I2C i2c_bus = get_bus(adap->hwadapnr); +#else

•   i2c_bus = dev_get_priv(dev);
  

+#endif if (!i2c_bus)

•           return -1;
  

•           return -EFAULT;
  

Same comments as above.

    buf[0] = 0;

    /*

@@ -871,6 +897,7 @@ static int s3c24x0_i2c_probe(struct i2c_adapter *adap, uchar chip) return ret != I2C_OK; }

+#ifdef CONFIG_SYS_I2C static int s3c24x0_i2c_read(struct i2c_adapter *adap, uchar chip, uint addr, int alen, uchar *buffer, int len) { @@ -878,9 +905,13 @@ static int s3c24x0_i2c_read(struct i2c_adapter *adap, uchar chip, uint addr, uchar xaddr[4]; int ret;

•   i2c_bus = get_bus(adap->hwadapnr);
  

•   if (!i2c_bus)
  

•           return -EFAULT;
  

•   if (alen > 4) {
            debug("I2C read: addr len %d not supported\n", alen);
  

•           return 1;
  

•           return -EFBIG;
  

I've been using -EADDRNOTAVAIL

    }

    if (alen > 0) {

@@ -906,10 +937,6 @@ static int s3c24x0_i2c_read(struct i2c_adapter *adap, uchar chip, uint addr, chip |= ((addr >> (alen * 8)) & CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW); #endif

•   i2c_bus = get_bus(adap->hwadapnr);
  

•   if (!i2c_bus)
  

•           return -1;
  

•   if (i2c_bus->is_highspeed)
            ret = hsi2c_read(i2c_bus->hsregs, chip, &xaddr[4 - alen],
                             alen, buffer, len);
  

@@ -921,7 +948,7 @@ static int s3c24x0_i2c_read(struct i2c_adapter *adap, uchar chip, uint addr, if (i2c_bus->is_highspeed) exynos5_i2c_reset(i2c_bus); debug("I2c read failed %d\n", ret);

•           return 1;
  

•           return -EIO;
    }
    return 0;
  

} @@ -933,9 +960,13 @@ static int s3c24x0_i2c_write(struct i2c_adapter *adap, uchar chip, uint addr, uchar xaddr[4]; int ret;

•   i2c_bus = get_bus(adap->hwadapnr);
  

•   if (!i2c_bus)
  

•           return -EFAULT;
  

•   if (alen > 4) {
            debug("I2C write: addr len %d not supported\n", alen);
  

•           return 1;
  

•           return -EINVAL;
    }
  
    if (alen > 0) {
  

@@ -960,10 +991,6 @@ static int s3c24x0_i2c_write(struct i2c_adapter *adap, uchar chip, uint addr, chip |= ((addr >> (alen * 8)) & CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW); #endif

•   i2c_bus = get_bus(adap->hwadapnr);
  

•   if (!i2c_bus)
  

•           return -1;
  

•   if (i2c_bus->is_highspeed)
            ret = hsi2c_write(i2c_bus->hsregs, chip, &xaddr[4 - alen],
                              alen, buffer, len, true);
  

@@ -1010,7 +1037,7 @@ static void process_nodes(const void *blob, int node_list[], int count, CONFIG_SYS_I2C_S3C24X0_SPEED); bus->node = node; bus->bus_num = i;

•           exynos_pinmux_config(bus->id, 0);
  

•           exynos_pinmux_config(PERIPH_ID_I2C0 + bus->id, 0);
  
            /* Mark position as used */
            node_list[i] = -1;
  

@@ -1033,7 +1060,6 @@ void board_i2c_init(const void *blob) COMPAT_SAMSUNG_EXYNOS5_I2C, node_list, CONFIG_MAX_I2C_NUM); process_nodes(blob, node_list, count, 1);

}

int i2c_get_bus_num_fdt(int node) @@ -1046,7 +1072,7 @@ int i2c_get_bus_num_fdt(int node) }

    debug("%s: Can't find any matched I2C bus\n", __func__);

•   return -1;
  

•   return -EINVAL;
  

}

int i2c_reset_port_fdt(const void *blob, int node) @@ -1057,18 +1083,18 @@ int i2c_reset_port_fdt(const void *blob, int node) bus = i2c_get_bus_num_fdt(node); if (bus < 0) { debug("could not get bus for node %d\n", node);

•           return -1;
  

•           return bus;
    }
  
    i2c_bus = get_bus(bus);
    if (!i2c_bus) {
  

•           debug("get_bus() failed for node node %d\n", node);
  

•           return -1;
  

•           debug("get_bus() failed for node %d\n", node);
  

•           return -EFAULT;
    }
  
    if (i2c_bus->is_highspeed) {
            if (hsi2c_get_clk_details(i2c_bus))
  

•                   return -1;
  

•                   return -EINVAL;
            hsi2c_ch_init(i2c_bus);
    } else {
            i2c_ch_init(i2c_bus->regs, i2c_bus->clock_frequency,
  

@@ -1077,7 +1103,17 @@ int i2c_reset_port_fdt(const void *blob, int node)

    return 0;

} -#endif +#endif /* CONFIG_OF_CONTROL */

+#ifdef CONFIG_EXYNOS5 +static void exynos_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) +{

•   /* This will override the speed selected in the fdt for that port */
  

•   debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);
  

•   if (i2c_set_bus_speed(speed))
  

•           error("i2c_init: failed to init bus for speed = %d", speed);
  

+} +#endif /* CONFIG_EXYNOS5 */

/*

• Register s3c24x0 i2c adapters

@@ -1247,3 +1283,158 @@ U_BOOT_I2C_ADAP_COMPLETE(s3c0, s3c24x0_i2c_init, s3c24x0_i2c_probe, CONFIG_SYS_I2C_S3C24X0_SPEED, CONFIG_SYS_I2C_S3C24X0_SLAVE, 0) #endif +#endif /* CONFIG_SYS_I2C */

+#ifdef CONFIG_DM_I2C +static int i2c_write_data(struct s3c24x0_i2c_bus *i2c_bus, uchar chip,

•                     uchar *buffer, int len, bool end_with_repeated_start)
  

+{

•   int ret;
  

•   if (!i2c_bus)
  

•           return -EFAULT;
  

Can't happen, drop this check.

•   if (i2c_bus->is_highspeed) {
  

•           ret = hsi2c_write(i2c_bus->hsregs, chip, 0, 0,
  

•                             buffer, len, true);
  

•           if (ret)
  

•                   exynos5_i2c_reset(i2c_bus);
  

•   } else {
  

•           ret = i2c_transfer(i2c_bus->regs, I2C_WRITE,
  

•                              chip << 1, 0, 0, buffer, len);
  

•   }
  

•   return ret != I2C_OK;
  

+}

+static int i2c_read_data(struct s3c24x0_i2c_bus *i2c_bus, uchar chip,

•                    uchar *buffer, int len)
  

+{

•   int ret;
  

•   if (!i2c_bus)
  

•           return -EFAULT;
  

Can't happen, drop this check.

•   if (i2c_bus->is_highspeed) {
  

•           ret = hsi2c_read(i2c_bus->hsregs, chip, 0, 0, buffer, len);
  

•           if (ret)
  

•                   exynos5_i2c_reset(i2c_bus);
  

•   } else {
  

•           ret = i2c_transfer(i2c_bus->regs, I2C_READ,
  

•                              chip << 1, 0, 0, buffer, len);
  

•   }
  

•   return ret != I2C_OK;
  

+}

+static int s3c24x0_i2c_xfer(struct udevice *dev, struct i2c_msg *msg,

•                       int nmsgs)
  

+{

•   struct s3c24x0_i2c_bus *i2c_bus;
  

•   int ret;
  

•   if (!dev)
  

•           return -ENODEV;
  

•   i2c_bus = dev_get_priv(dev);
  

•   if (!i2c_bus)
  

•           return -EFAULT;
  

Can't happen, drop this check.

•   for (; nmsgs > 0; nmsgs--, msg++) {
  

•           bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD);
  

•           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)
  

•                   return -EREMOTEIO;
  

•   }
  

•   return 0;
  

+}

+static int s3c_i2c_ofdata_to_platdata(struct udevice *dev) +{

•   const void *blob = gd->fdt_blob;
  

•   struct s3c24x0_i2c_bus *i2c_bus;
  

•   int node;
  

•   if (!dev) {
  

•           error("%s: no such device!", dev->name);
  

•           return -ENODEV;
  

•   }
  

Can't happen, drop this check.

•   i2c_bus = dev_get_priv(dev);
  

•   if (!i2c_bus) {
  

•           error("%s: i2c bus not allocated!", dev->name);
  

•           return -EFAULT;
  

•   }
  

Can't happen, drop this check.

•   if (!dev->of_id) {
  

•           error("%s: no compat ids!", dev->name);
  

•           return -EINVAL;
  

•   }
  

Can't happen, drop this check.

•   i2c_bus->is_highspeed = dev->of_id->data;
  

Remove blank line

•   node = dev->of_offset;
  

•   if (i2c_bus->is_highspeed) {
  

•           i2c_bus->hsregs = (struct exynos5_hsi2c *)
  

•                           fdtdec_get_addr(blob, node, "reg");
  

•   } else {
  

•           i2c_bus->regs = (struct s3c24x0_i2c *)
  

•                           fdtdec_get_addr(blob, node, "reg");
  

•   }
  

•   i2c_bus->id = pinmux_decode_periph_id(blob, node);
  

•   i2c_bus->clock_frequency = fdtdec_get_int(blob, node,
  

•                                           "clock-frequency",
  

•                                           CONFIG_SYS_I2C_S3C24X0_SPEED);
  

•   i2c_bus->node = node;
  

•   i2c_bus->bus_num = dev->seq;
  

•   exynos_pinmux_config(i2c_bus->id, i2c_bus->is_highspeed);
  

You should add a flag to pinmux.h and then do something like:

exynos_pinmux_config(i2c_bus->id, i2c_bus->is_highspeed ? PINMUX_FLAG_I2C_HIGH_SPEED : 0);

•   i2c_bus->active = true;
  

•   return 0;
  

+}

+static int s3c_i2c_child_pre_probe(struct udevice *dev) +{

•   struct dm_i2c_chip *i2c_chip = dev_get_parentdata(dev);
  

•   if (dev->of_offset == -1)
  

•           return 0;
  

•   return i2c_chip_ofdata_to_platdata(gd->fdt_blob, dev->of_offset,
  

•                                      i2c_chip);
  

+}

Not needed, drop this functoin.

+static const struct dm_i2c_ops s3c_i2c_ops = {

•   .xfer           = s3c24x0_i2c_xfer,
  

•   .probe_chip     = s3c24x0_i2c_probe,
  

•   .set_bus_speed  = s3c24x0_i2c_set_bus_speed,
  

+};

+static const struct udevice_id s3c_i2c_ids[] = {

•   { .compatible = "samsung,s3c2440-i2c", .data = EXYNOS_I2C_STD },
  

•   { .compatible = "samsung,exynos5-hsi2c", .data = EXYNOS_I2C_HS },
  

•   { }
  

+};

+U_BOOT_DRIVER(i2c_s3c) = {

•   .name   = "i2c_s3c",
  

•   .id     = UCLASS_I2C,
  

•   .of_match = s3c_i2c_ids,
  

•   .ofdata_to_platdata = s3c_i2c_ofdata_to_platdata,
  

•   .per_child_auto_alloc_size = sizeof(struct dm_i2c_chip),
  

Not needed, drop this member.

•   .child_pre_probe = s3c_i2c_child_pre_probe,
  

Not needed, drop this functoin.

•   .priv_auto_alloc_size = sizeof(struct s3c24x0_i2c_bus),
  

•   .ops    = &s3c_i2c_ops,
  

+};

+#endif /* CONFIG_DM_I2C */

1.9.1

Regards, Simon