[U-Boot] [PATCH v2 13/40] i2c: Add high-level API

[Heiko Schocher]

Hello Thierry,

Am 26.08.2014 17:34, schrieb Thierry Reding:
> From: Thierry Reding<treding at nvidia.com>
>
> This API operates on I2C adapters or I2C clients (a new type of object

which is a bad idea ...

> that refers to a particular slave connected to an adapter). This is
> useful to avoid having to call i2c_set_bus_num() whenever a device is
> being accessed.

But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus,
you must check before every access, if you are on it, if not, you must
switch back to it...

This is collected in i2c_set_bus_num() ... before, every "user" did
this on his own ... if you are on the bus you want to access, the
overhead is not so big, see:

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278

  278 int i2c_set_bus_num(unsigned int bus)
  279 {
  280         int max;
  281
  282         if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))
  283                 return 0;

And you must be aware of i2c muxes! You directly use the read/write
functions from the i2c adapter, but what is if you have i2c muxes?

Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is
an eeprom, on the other is a PMIC ... your code in patch
"power: Add AMS AS3722 PMIC support" does access with your functions
the PMIC ... what is, if between this accesses someone accesses the eeprom?
If he switches the mux, you never switch back!

Your code did not check this!

Why is i2c_set_bus_num() such a problem?

> Drivers for I2C devices are supposed to embed a struct i2c_client within
> a driver-specific data structure and call i2c_client_init() on it,
> passing in a pointer to the parent I2C adapter and the slave address of
> the device.
>
> Signed-off-by: Thierry Reding<treding at nvidia.com>
> ---
>   drivers/i2c/i2c_core.c | 53 ++++++++++++++++++++++++++++
>   include/i2c.h          | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 149 insertions(+)
>
> diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c
> index f6179a16244b..88c7af546b0b 100644
> --- a/drivers/i2c/i2c_core.c
> +++ b/drivers/i2c/i2c_core.c
> @@ -410,3 +410,56 @@ void __i2c_init(int speed, int slaveaddr)
>   }
>   void i2c_init(int speed, int slaveaddr)
>   	__attribute__((weak, alias("__i2c_init")));
> +
> +struct i2c_adapter *i2c_adapter_get(unsigned int index)
> +{
> +	struct i2c_adapter *adapter = ll_entry_start(struct i2c_adapter, i2c);
> +	unsigned int num = ll_entry_count(struct i2c_adapter, i2c);
> +	unsigned int i;
> +
> +	if (index>= num)
> +		return NULL;
> +
> +	for (i = 0; i<  index; i++)
> +		adapter++;
> +
> +	i2c_adapter_init(adapter, adapter->speed, adapter->slaveaddr);
> +	return adapter;
> +}
> +
> +int i2c_adapter_read(struct i2c_adapter *adapter, uint8_t chip,
> +		     unsigned int address, size_t alen, void *buffer,
> +		     size_t size)
> +{
> +	return adapter->read(adapter, chip, address, alen, buffer, size);
> +}
> +
> +int i2c_adapter_write(struct i2c_adapter *adapter, uint8_t chip,
> +		      unsigned int address, size_t alen, void *buffer,
> +		      size_t size)
> +{
> +	return adapter->write(adapter, chip, address, alen, buffer, size);
> +}
> +
> +int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter,
> +		    uint8_t address)
> +{
> +	client->adapter = adapter;
> +	client->address = address;
> +
> +	return 0;
> +}
> +
> +int i2c_client_read(struct i2c_client *client, unsigned int address,
> +		    size_t alen, void *buffer, size_t size)
> +{
> +	return i2c_adapter_read(client->adapter, client->address, address,
> +				alen, buffer, size);
> +}
> +
> +int i2c_client_write(struct i2c_client *client, unsigned int address,
> +		     size_t alen, void *buffer, size_t size)
> +{
> +	return i2c_adapter_write(client->adapter, client->address, address,
> +				 alen, buffer, size);
> +}
> diff --git a/include/i2c.h b/include/i2c.h
> index 1b4078ed62fe..8f73ba93c614 100644
> --- a/include/i2c.h
> +++ b/include/i2c.h
> @@ -55,6 +55,20 @@
>   #define CONFIG_SYS_SPD_BUS_NUM		0
>   #endif
>
> +/**
> + * struct i2c_adapter - I2C adapter
> + * @init: initialize I2C adapter
> + * @probe: probe for a device on the I2C bus
> + * @read: read data from a device on the I2C bus
> + * @write: write data to a device on the I2C bus
> + * @set_bus_speed: configure the I2C bus speed
> + * @speed: current I2C bus speed
> + * @waitdelay:
> + * @slaveaddr: own address (when used as a slave)
> + * @init_done: flag to indicate whether or not the adapter has been initialized
> + * @hwadapnr: the hardware number of this adapter
> + * @name: name of this adapter
> + */
>   struct i2c_adapter {
>   	void		(*init)(struct i2c_adapter *adap, int speed,
>   				int slaveaddr);
> @@ -75,6 +89,16 @@ struct i2c_adapter {
>   	char		*name;
>   };
>
> +/**
> + * struct i2c_client - I2C slave
> + * @adapter: I2C adapter providing the slave's parent bus
> + * address: address of the I2C slave on the parent bus
> + */
> +struct i2c_client {
> +	struct i2c_adapter *adapter;
> +	unsigned int address;
> +};
> +
>   #define U_BOOT_I2C_MKENT_COMPLETE(_init, _probe, _read, _write, \
>   		_set_speed, _speed, _slaveaddr, _hwadapnr, _name) \
>   	{ \
> @@ -271,6 +295,78 @@ unsigned int i2c_get_bus_speed(void);
>    * Adjusts I2C pointers after U-Boot is relocated to DRAM
>    */
>   void i2c_reloc_fixup(void);
> +
> +/*
> + * i2c_adapter_get() - get the I2C adapter associated with a given index
> + * @index: index of the I2C adapter
> + */
> +struct i2c_adapter *i2c_adapter_get(unsigned int index);
> +
> +/*
> + * i2c_adapter_read() - read data from an I2C slave at a given address
> + * @adapter: I2C adapter
> + * @chip: address of the I2C slave to read from
> + * @address: address within the I2C slave to read from
> + * @alen: length of address
> + * @buffer: buffer to receive data from I2C slave
> + * @size: number of bytes to read
> + */
> +int i2c_adapter_read(struct i2c_adapter *adapter, uint8_t chip,
> +		     unsigned int address, size_t alen, void *buffer,
> +		     size_t size);
> +
> +/*
> + * i2c_adapter_write() - write data to an I2C slave at a given address
> + * @adapter: I2C adapter
> + * @chip: address of the I2C slave to write to
> + * @address: address within the I2C slave to write to
> + * @alen: length of address
> + * @buffer: buffer containing the data to write
> + * @size: number of bytes to write
> + *
> + * Ideally the function would take a const void * buffer, but the underlying
> + * infrastructure doesn't properly propagate const and adding it here would
> + * cause a lot of build warnings.
> + */
> +int i2c_adapter_write(struct i2c_adapter *adapter, uint8_t chip,
> +		      unsigned int address, size_t alen, void *buffer,
> +		      size_t size);
> +
> +/*
> + * i2c_client_init() - initialize an I2C slave
> + * @client: I2C slave
> + * @adapter: parent I2C adapter
> + * @address: address of I2C slave
> + */
> +int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter,
> +		    uint8_t address);
> +
> +/*
> + * i2c_client_read() - read data from an I2C slave
> + * @client: I2C slave
> + * @address: address within the I2C slave to read from
> + * @alen: length of address
> + * @buffer: buffer to receive data from I2C slave
> + * @size: number of bytes to read
> + */
> +int i2c_client_read(struct i2c_client *client, unsigned int address,
> +		    size_t alen, void *buffer, size_t size);
> +
> +/*
> + * i2c_client_write() - write data to an I2C slave
> + * @client: I2C slave
> + * @address: address within the I2C slave to write to
> + * @alen: length of address
> + * @buffer: buffer containing the data to write
> + * @size: number of bytes to write
> + *
> + * Ideally the function would take a const void * buffer, but the underlying
> + * infrastructure doesn't properly propagate const and adding it here would
> + * cause a lot of build warnings.
> + */
> +int i2c_client_write(struct i2c_client *client, unsigned int address,
> +		     size_t alen, void *buffer, size_t size);
> +
>   #if defined(CONFIG_SYS_I2C_SOFT)
>   void i2c_soft_init(void);
>   void i2c_soft_active(void);

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany