subject:"\[PATCH\] i2c\: davinci\: Add block read functionality for IPMI"

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

2014-06-02 Thread Murali Karicheri


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 = 0x as flag */
+   if (msg-flags  I2C_M_RECV_LEN)
+   dev-buf_len = 0x;

a) this magic value should be a define instead of a comment
b) i2c messages easily have a 16 bit range, so 0x 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 == 0x)
+   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
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

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

2014-06-02 Thread Murali Karicheri


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-d...@ti.com
Signed-off-by: Murali Karicheri m-kariche...@ti.com

I tested this on a DA850 using i2cdetect and it did not seem to break
anything so:

Tested-by: Sekhar Nori nsek...@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
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

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

2014-05-22 Thread 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 = 0x as flag */
 +if (msg-flags  I2C_M_RECV_LEN)
 +dev-buf_len = 0x;

a) this magic value should be a define instead of a comment
b) i2c messages easily have a 16 bit range, so 0x 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 == 0x)
 +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


signature.asc
Description: Digital signature
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

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

2014-05-22 Thread Sekhar Nori

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-d...@ti.com
 Signed-off-by: Murali Karicheri m-kariche...@ti.com

I tested this on a DA850 using i2cdetect and it did not seem to break 
anything so:

Tested-by: Sekhar Nori nsek...@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

___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

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

2014-05-05 Thread Karicheri, Muralidharan

-Original Message-
From: Karicheri, Muralidharan
Sent: Thursday, May 01, 2014 2:50 PM
To: davinci-linux-open-source@linux.davincidsp.com; linux-...@vger.kernel.org; 
linux-
ker...@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-d...@ti.com
Signed-off-by: Murali Karicheri m-kariche...@ti.com
Tested-by: Garrett Ding g-d...@ti.com
CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Wolfram Sang w...@the-dreams.de
CC: Santosh Shilimkar santosh.shilim...@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_ALBIT(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 = 0x as flag */
+  if (msg-flags  I2C_M_RECV_LEN)
+  dev-buf_len = 0x;
+  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 == 0x)
+  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

___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

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

2014-05-01 Thread Murali Karicheri

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-d...@ti.com
Signed-off-by: Murali Karicheri m-kariche...@ti.com
Tested-by: Garrett Ding g-d...@ti.com
CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Wolfram Sang w...@the-dreams.de
CC: Santosh Shilimkar santosh.shilim...@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 = 0x as flag */
+   if (msg-flags  I2C_M_RECV_LEN)
+   dev-buf_len = 0x;
+   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 == 0x)
+   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

___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

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

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

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

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

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

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

6 matches

Site Navigation

Mail list logo

Footer information