lkml.org 
[lkml]   [2014]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] i2c: davinci: Add block read functionality for IPMI
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


\
 
 \ /
  Last update: 2014-06-02 19:21    [W:1.234 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site