[U-Boot] [PATCH] fsl_i2c: Add write-then-read transaction interface for I2C slave

[York Sun]

On 03/09/2014 10:44 PM, Leekha Shaveta-B20052 wrote:
> 
> 
> -----Original Message-----
> From: Sun York-R58495 
> Sent: Saturday, March 08, 2014 3:10 AM
> To: Leekha Shaveta-B20052; u-boot at lists.denx.de
> Cc: Aggrwal Poonam-B10812; Aggrwal Poonam-B10812
> Subject: Re: [PATCH] fsl_i2c: Add write-then-read transaction interface for I2C slave
> 
> On 03/03/2014 12:58 AM, Shaveta Leekha wrote:
>> Most of the I2C slaves support accesses in the typical style 
>> viz.read/write series of bytes at particular address offset.
>> These transactions are currently supportd in the i2c driver using 
>> i2c_read and i2c_write APIs. I2C EEPROMs, RTC, etc fall in this 
>> category.
>> The transactions look like:"
>> START:Address:Tx:Offset:RESTART:Address[0..4]:Tx/Rx:data[0..n]:STOP"
>>
>> However there are certain devices which support accesses in terms of 
>> the transactions as follows:
>> "START:Address:Tx:Txdata[0..n1]:Clock_stretching:
>>         RESTART:Address:Rx:data[0..n2]"
>>
>> The Txdata is typically a command and some associated data, similarly 
>> Rxdata could be command status plus some data received as a response 
>> to the command sent.
>> i2c_write_read() function provides support for such transactions 
>> (multiple bytes write followed by read)
>>
>> Signed-off-by: Poonam Aggrwal <poonam.aggrwal at freescale.com>
>> Signed-off-by: Shaveta Leekha <shaveta at freescale.com>
>> ---
>>  drivers/i2c/fsl_i2c.c  |   64 ++++++++++++++++++++++++++++++++++++++++++-----
>>  drivers/i2c/i2c_core.c |    7 +++++
>>  include/i2c.h          |   19 ++++++++++---
>>  3 files changed, 78 insertions(+), 12 deletions(-)
> 
> <snip>
> 
>> diff --git a/include/i2c.h b/include/i2c.h index 1b4078e..7bac20a 
>> 100644
>> --- a/include/i2c.h
>> +++ b/include/i2c.h
>> @@ -65,6 +65,9 @@ struct i2c_adapter {
>>  	int		(*write)(struct i2c_adapter *adap, uint8_t chip,
>>  				uint addr, int alen, uint8_t *buffer,
>>  				int len);
>> +	int		(*write_read)(struct i2c_adapter *adap, uint8_t chip,
>> +				uint8_t *wbuffer, int wlength, uint8_t *rbuffer,
>> +				int rlength);
>>  	uint		(*set_bus_speed)(struct i2c_adapter *adap,
>>  				uint speed);
>>  	int		speed;
>> @@ -75,13 +78,14 @@ struct i2c_adapter {
>>  	char		*name;
>>  };
>>  
>> -#define U_BOOT_I2C_MKENT_COMPLETE(_init, _probe, _read, _write, \
>> +#define U_BOOT_I2C_MKENT_COMPLETE(_init, _probe, _read, _write, 
>> +_write_read, \
>>  		_set_speed, _speed, _slaveaddr, _hwadapnr, _name) \
>>  	{ \
>>  		.init		=	_init, \
>>  		.probe		=	_probe, \
>>  		.read		=	_read, \
>>  		.write		=	_write, \
>> +		.write_read	=	_write_read, \
>>  		.set_bus_speed	=	_set_speed, \
>>  		.speed		=	_speed, \
>>  		.slaveaddr	=	_slaveaddr, \
>> @@ -90,10 +94,11 @@ struct i2c_adapter {
>>  		.name		=	#_name \
>>  };
>>  
>> -#define U_BOOT_I2C_ADAP_COMPLETE(_name, _init, _probe, _read, _write, \
>> -			_set_speed, _speed, _slaveaddr, _hwadapnr) \
>> -	ll_entry_declare(struct i2c_adapter, _name, i2c) = \
>> -	U_BOOT_I2C_MKENT_COMPLETE(_init, _probe, _read, _write, \
>> +#define U_BOOT_I2C_ADAP_COMPLETE(_name, _init, _probe, _read, _write,	\
>> +			_write_read, _set_speed, _speed, _slaveaddr,	\
>> +			_hwadapnr)					\
>> +	ll_entry_declare(struct i2c_adapter, _name, i2c) =		\
>> +	U_BOOT_I2C_MKENT_COMPLETE(_init, _probe, _read, _write,  
>> +_write_read, \
>>  		 _set_speed, _speed, _slaveaddr, _hwadapnr, _name);
>>  
>>  struct i2c_adapter *i2c_get_adapter(int index); @@ -237,6 +242,8 @@ 
>> int i2c_read(uint8_t chip, unsigned int addr, int alen,
>>  
>>  int i2c_write(uint8_t chip, unsigned int addr, int alen,
>>  				uint8_t *buffer, int len);
>> +int i2c_write_read(uint8_t chip, uchar *wbuffer, int wlen, uchar *rbuffer,
>> +								int rlen);
>>  
>>  /*
>>   * Utility routines to read/write registers.
>> @@ -302,6 +309,8 @@ int i2c_probe(uchar chip);
>>   */
>>  int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int 
>> len);  int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, 
>> int len);
>> +int i2c_write_read(uchar chip, uchar *wbuffer, int wlen, uchar *rbuffer,
>> +								int rlen);
> 
> 
> You need to be careful when changing the header file. If you compile other platforms, you will see the error. Try ./MAKEALL -a powerpc. I don't know how bad you broke other architectures.
> 
> [Leekha Shaveta-B20052] Agree. This may break i2c driver code for other architectures.
> For one of the I2C device for B4 platform, the read/write transaction cycle is little different from
> Other conventional I2C devices like EEPROM, PCA etc. 
> As its not right to change the header file for i2c as the same is being shared by other architectures/drivers also.
> So should we write a separate header file for FSL I2C driver fsl_i2c.h that support this
> New write_read API for I2C? 
> 
> What do you suggest here?
> 

You are adding a new operation to i2c adapters. There are different ways.

1) Update all code to support this new operation
2) Use ifdef to activate this operation only on selected platforms
3) Look for other examples to do this safely. There may be some better ways to
do so.

York