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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
