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

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

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 ...

> +#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

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,

Hi Przemyslaw,

On 9 January 2015 at 01:57, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> Hello Heiko Schocher,
>
>
> On 01/09/2015 07:31 AM, Heiko Schocher wrote:
>>
>> Hello Przemyslaw Marczak,
>>
>> just some nitpick ...
>>

[snip]

> Thank you for the review, I will fix this in the next patchset version.

I'd like to apply this to u-boot-dm soon - can you please resend this
patch with the nits fixed? I don't have any comments beyond what Heiko
says.

Also, you probably saw the compatibility layer for I2C which should
make it possible for you to run the old PMIC framework with I2C using
driver model. It should make it easier to get everything done. It is
in u-boot-dm/testing.

Regards,
Simon

Hello Simon,

On 01/20/2015 04:18 AM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 9 January 2015 at 01:57, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> Hello Heiko Schocher,
>>
>>
>> On 01/09/2015 07:31 AM, Heiko Schocher wrote:
>>>
>>> Hello Przemyslaw Marczak,
>>>
>>> just some nitpick ...
>>>
>
> [snip]
>
>> Thank you for the review, I will fix this in the next patchset version.
>
> I'd like to apply this to u-boot-dm soon - can you please resend this
> patch with the nits fixed? I don't have any comments beyond what Heiko
> says.
>
> Also, you probably saw the compatibility layer for I2C which should
> make it possible for you to run the old PMIC framework with I2C using
> driver model. It should make it easier to get everything done. It is
> in u-boot-dm/testing.
>
> Regards,
> Simon
>

I will check this and send the second patch version probably on Monday.

Best regards,