Re: [PATCH] i2c: davinci: Add block read functionality for IPMI

[Murali Karicheri <m-karicheri2@ti.com>]

On 5/22/2014 5:00 AM, Wolfram Sang wrote:
> Hi,
>
> thanks for the patch.
>
>>> +/* capabilities */
>>> +#define I2C_CAPABILITIES	(I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | \
>>> +				 I2C_FUNC_SMBUS_READ_BLOCK_DATA)
> I don't see the need for a seperate define.
>
>>> +
>>> struct davinci_i2c_dev {
>>> 	struct device           *dev;
>>> 	void __iomem		*base;
>>> @@ -318,7 +322,13 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg
>>> *msg, int stop)
>>> 	davinci_i2c_write_reg(dev, DAVINCI_I2C_SAR_REG, msg->addr);
>>>
>>> 	dev->buf = msg->buf;
>>> -	dev->buf_len = msg->len;
>>> +
>>> +	 /* if first received byte is length, set buf_len = 0xffff as flag */
>>> +	if (msg->flags & I2C_M_RECV_LEN)
>>> +		dev->buf_len = 0xffff;
> a) this magic value should be a define instead of a comment
> b) i2c messages easily have a 16 bit range, so 0xffff is a troublesome
> choice.
>
>>> +	else
>>> +		dev->buf_len = msg->len;
>>> +
>>> 	dev->stop = stop;
>>>
>>> 	davinci_i2c_write_reg(dev, DAVINCI_I2C_CNT_REG, dev->buf_len); @@ -456,7
>>> +466,7 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>>>
>>> static u32 i2c_davinci_func(struct i2c_adapter *adap)  {
>>> -	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>>> +	return I2C_CAPABILITIES;
>>> }
>>>
>>> static void terminate_read(struct davinci_i2c_dev *dev) @@ -528,10 +538,32 @@ static
>>> irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>>>
>>> 		case DAVINCI_I2C_IVR_RDR:
>>> 			if (dev->buf_len) {
>>> -				*dev->buf++ =
>>> -				    davinci_i2c_read_reg(dev,
>>> -							 DAVINCI_I2C_DRR_REG);
>>> +				*dev->buf++ = davinci_i2c_read_reg(dev,
>>> +							DAVINCI_I2C_DRR_REG);
>>> +				/*
>>> +				 * check if the first received byte is message
>>> +				 * length, i.e, I2C_M_RECV_LEN
>>> +				 */
>>> +				if (dev->buf_len == 0xffff)
>>> +					dev->buf_len = *(dev->buf - 1) + 1;
> Please rework the code to get rid of the '- 1' and '+ 1'. They look
> hackish and make the code less readable.
>
>>> +
>>> 				dev->buf_len--;
>>> +				/*
>>> +				 * send NACK/STOP bits BEFORE last byte is
>>> +				 * received
>>> +				 */
>>> +				if (dev->buf_len == 1) {
>>> +					w = davinci_i2c_read_reg(dev,
>>> +							DAVINCI_I2C_MDR_REG);
>>> +					w |= DAVINCI_I2C_MDR_NACK;
>>> +					davinci_i2c_write_reg(dev,
>>> +							DAVINCI_I2C_MDR_REG, w);
>>> +
>>> +					w |= DAVINCI_I2C_MDR_STP;
>>> +					davinci_i2c_write_reg(dev,
>>> +							DAVINCI_I2C_MDR_REG, w);
>>> +				}
>>> +
> Looks like an unreleated change to me? Why is this I2C_M_RECV_LEN
> specific?
>
> Kind regards,
>
>     Wolfram
Wolfram,

Thanks for reviewing the patch. I will review your comments and get back 
to you. This patch
is tested on a customer board and thus it might be a while before I can 
incorporate these changes
and provide an updated patch to the list. Meanwhile I will be reviewing 
the comment in the
next few weeks and get back to you in case I have questions.

Thanks and regards,

Murali