[U-Boot] [PATCH 07/18] dm: i2c: s3c24x0: adjust to dm-i2c api

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello Heiko Schocher,

On 01/09/2015 07:31 AM, Heiko Schocher wrote:
> Hello Przemyslaw Marczak,
>
> just some nitpick ...
>
> Am 08.01.2015 12:33, schrieb Przemyslaw Marczak:
>> 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 and Arndale (also with HS).
>>
>> 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>
>> ---
>>   drivers/i2c/s3c24x0_i2c.c | 254
>> ++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 222 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/i2c/s3c24x0_i2c.c b/drivers/i2c/s3c24x0_i2c.c
>> index fd328f0..c21d479 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>
>> @@ -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)
>>   {
>> @@ -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
>> @@ -545,7 +562,6 @@ static int hsi2c_prepare_transaction(struct
>> exynos5_hsi2c *i2c,
>>                        bool issue_stop)
>>   {
>>       u32 conf;
>> -
>
> why do you delete this empty line?
>
>>       conf = len | HSI2C_MASTER_RUN;
>>
>>       if (issue_stop)
>> @@ -698,14 +714,24 @@ 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;
>> -
>
> here too ...
>

Oh, I missed this after removing the debug code. Will fix in both cases.

>> +#ifdef CONFIG_SYS_I2C
>>       i2c_bus = get_bus(adap->hwadapnr);
>> +#else
>> +    if (!dev)
>> +        return -ENODEV;
>> +
>> +    i2c_bus = dev_get_priv(dev);
>> +#endif
>>       if (!i2c_bus)
>> -        return -1;
>> +        return -ENODEV;
>>
>>       i2c_bus->clock_frequency = speed;
>>
>> @@ -715,23 +741,12 @@ static unsigned int
>> s3c24x0_i2c_set_bus_speed(struct i2c_adapter *adap,
>>           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);
>>       }
>>
>>       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 +859,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 -ENODEV;
>> +
>>       buf[0] = 0;
>>
>>       /*
>> @@ -871,6 +895,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,6 +903,10 @@ 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 -1;
>
> above you change "return -1" to "return -ENODEV" ...
> Shouldn;t we use here also an errno? Suggestion -EFAULT?
>

Yes, you are right, I will fix this issue globally.

>> +
>>       if (alen > 4) {
>>           debug("I2C read: addr len %d not supported\n", alen);
>>           return 1;
>> @@ -906,10 +935,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);
>> @@ -933,6 +958,10 @@ 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 -1;
>
> here too ...
>
>> +
>>       if (alen > 4) {
>>           debug("I2C write: addr len %d not supported\n", alen);
>>           return 1;
>> @@ -960,10 +989,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 +1035,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 +1058,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)
>> @@ -1077,7 +1101,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 +1281,159 @@ 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 -1;
>
> and here
>
>> +
>> +    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 -1;
>
> and here
>
>> +
>> +    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 -1;
>
> and here...
>
>> +
>> +    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;
>> +    }
>> +
>> +    i2c_bus = dev_get_priv(dev);
>> +    if (!i2c_bus) {
>> +        error("%s: i2c bus not allocated!", dev->name);
>> +        return -EINVAL;
>
> ah, here if no bus is found you return -EINVAL ... as EINVAL is used
> below again, I prefer to use another errno here ... as suggested EFAULT
> or maybe ENOENT? ... but please for all occurrences of "if (!i2c_bus) {"
> the same value, thanks!
>

Right, this was not good.

>> +    }
>> +
>> +    if (!dev->of_id) {
>> +        error("%s: no compat ids!", dev->name);
>> +        return -EINVAL;
>> +    }
>> +    i2c_bus->is_highspeed = dev->of_id->data;
>> +
>> +    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);
>> +
>> +    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);
>> +}
>> +
>> +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),
>> +    .child_pre_probe = s3c_i2c_child_pre_probe,
>> +    .priv_auto_alloc_size = sizeof(struct s3c24x0_i2c_bus),
>> +    .ops    = &s3c_i2c_ops,
>> +};
>> +#endif /* CONFIG_DM_I2C */
>>
>
> Thanks for your work!
>
> bye,
> Heiko

Thank you for the review, I will fix this in the next patchset version.
Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com