| Message ID | 1398970186-12204-1-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>-----Original Message----- >From: Karicheri, Muralidharan >Sent: Thursday, May 01, 2014 2:50 PM >To: davinci-linux-open-source@linux.davincidsp.com; linux-i2c@vger.kernel.org; linux- >kernel@vger.kernel.org >Cc: Karicheri, Muralidharan; Ding, Garrett; Nori, Sekhar; Kevin Hilman; Wolfram Sang; >Shilimkar, Santosh >Subject: [PATCH] i2c: davinci: Add block read functionality for IPMI > >Intelligent Plaform Management Interface (IPMI) requires I2C driver to support block read, >where the first byte received from slave is the length of following data:- Added length >check if the read type is block read (I2C_M_RECV_LEN) Send NACK/STOP bits before last >byte is received > >Signed-off-by: Garrett Ding <g-ding@ti.com> >Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >Tested-by: Garrett Ding <g-ding@ti.com> >CC: Sekhar Nori <nsekhar@ti.com> >CC: Kevin Hilman <khilman@deeprootsystems.com> >CC: Wolfram Sang <wsa@the-dreams.de> >CC: Santosh Shilimkar <santosh.shilimkar@ti.com> >--- > Tested on a customer board based on K2HK SoC > drivers/i2c/busses/i2c-davinci.c | 42 +++++++++++++++++++++++++++++++++- >---- > 1 file changed, 37 insertions(+), 5 deletions(-) > >diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c >index 389bc68..cd97920 100644 >--- a/drivers/i2c/busses/i2c-davinci.c >+++ b/drivers/i2c/busses/i2c-davinci.c >@@ -97,6 +97,10 @@ > #define DAVINCI_I2C_IMR_NACK BIT(1) > #define DAVINCI_I2C_IMR_AL BIT(0) > >+/* capabilities */ >+#define I2C_CAPABILITIES (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | \ >+ I2C_FUNC_SMBUS_READ_BLOCK_DATA) >+ > 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; >+ 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; >+ > 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); >+ } >+ > if (dev->buf_len) > continue; > >-- >1.7.9.5 All, Please review this when you get a chance. Murali
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
On Friday 02 May 2014 12:19 AM, Murali Karicheri wrote: > Intelligent Plaform Management Interface (IPMI) requires I2C driver > to support block read, where the first byte received from slave is > the length of following data:- > Added length check if the read type is block read (I2C_M_RECV_LEN) > Send NACK/STOP bits before last byte is received > > Signed-off-by: Garrett Ding <g-ding@ti.com> > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> I tested this on a DA850 using i2cdetect and it did not seem to break anything so: Tested-by: Sekhar Nori <nsekhar@ti.com> There are some checks that were triggered in checkpatch. You may want to fix them up. Thanks, Sekhar CHECK: Alignment should match open parenthesis #112: FILE: drivers/i2c/busses/i2c-davinci.c:557: + w = davinci_i2c_read_reg(dev, + DAVINCI_I2C_MDR_REG); CHECK: Alignment should match open parenthesis #115: FILE: drivers/i2c/busses/i2c-davinci.c:560: + davinci_i2c_write_reg(dev, + DAVINCI_I2C_MDR_REG, w); CHECK: Alignment should match open parenthesis #119: FILE: drivers/i2c/busses/i2c-davinci.c:564: + davinci_i2c_write_reg(dev, + DAVINCI_I2C_MDR_REG, w); total: 0 errors, 0 warnings, 3 checks, 67 lines checked
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
On 5/22/2014 6:51 AM, Sekhar Nori wrote: > On Friday 02 May 2014 12:19 AM, Murali Karicheri wrote: >> Intelligent Plaform Management Interface (IPMI) requires I2C driver >> to support block read, where the first byte received from slave is >> the length of following data:- >> Added length check if the read type is block read (I2C_M_RECV_LEN) >> Send NACK/STOP bits before last byte is received >> >> Signed-off-by: Garrett Ding <g-ding@ti.com> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > I tested this on a DA850 using i2cdetect and it did not seem to break > anything so: > > Tested-by: Sekhar Nori <nsekhar@ti.com> > > There are some checks that were triggered in checkpatch. You may want > to fix them up. > > Thanks, > Sekhar > > CHECK: Alignment should match open parenthesis > #112: FILE: drivers/i2c/busses/i2c-davinci.c:557: > + w = davinci_i2c_read_reg(dev, > + DAVINCI_I2C_MDR_REG); > > CHECK: Alignment should match open parenthesis > #115: FILE: drivers/i2c/busses/i2c-davinci.c:560: > + davinci_i2c_write_reg(dev, > + DAVINCI_I2C_MDR_REG, w); > > CHECK: Alignment should match open parenthesis > #119: FILE: drivers/i2c/busses/i2c-davinci.c:564: > + davinci_i2c_write_reg(dev, > + DAVINCI_I2C_MDR_REG, w); > > total: 0 errors, 0 warnings, 3 checks, 67 lines checked > Sekhar, Thanks for testing this. I have other comments against this patch and will be sending an updated patch in few weeks. Thanks and regards, Murali
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index 389bc68..cd97920 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -97,6 +97,10 @@ #define DAVINCI_I2C_IMR_NACK BIT(1) #define DAVINCI_I2C_IMR_AL BIT(0) +/* capabilities */ +#define I2C_CAPABILITIES (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | \ + I2C_FUNC_SMBUS_READ_BLOCK_DATA) + 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; + 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; + 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); + } + if (dev->buf_len) continue;