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

[Wolfram Sang]

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

Attachment: signature.asc
Description: Digital signature