[U-Boot] [PATCH 07/18] dm: i2c: s3c24x0: adjust to dm-i2c api
Przemyslaw Marczak
p.marczak at samsung.com
Fri Jan 9 09:57:06 CET 2015
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 at samsung.com>
>> Cc: Simon Glass <sjg at chromium.org>
>> Cc: Heiko Schocher <hs at denx.de>
>> Cc: Minkyu Kang <mk7.kang at 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
More information about the U-Boot
mailing list