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.marc...@samsung.com> Cc: Simon Glass <s...@chromium.org> Cc: Heiko Schocher <h...@denx.de> Cc: Minkyu Kang <mk7.k...@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 ...
+#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?
+ 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!
+ } + + 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 -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot