From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Fri, 09 Jan 2015 09:57:06 +0100 Subject: [U-Boot] [PATCH 07/18] dm: i2c: s3c24x0: adjust to dm-i2c api In-Reply-To: <54AF75A7.1040801@denx.de> References: <1420716524-15969-1-git-send-email-p.marczak@samsung.com> <1420716809-16276-1-git-send-email-p.marczak@samsung.com> <1420716809-16276-7-git-send-email-p.marczak@samsung.com> <54AF75A7.1040801@denx.de> Message-ID: <54AF97E2.6090900@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> Cc: Simon Glass >> Cc: Heiko Schocher >> Cc: Minkyu Kang >> --- >> 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 >> +#include >> +#include >> #include >> #if (defined CONFIG_EXYNOS4 || defined CONFIG_EXYNOS5) >> #include >> @@ -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