[PATCH v2 0/2] add PEC support on slave side

[PATCH v2 0/2] add PEC support on slave side

[Rayagonda Kokatanur]

This patch set adds support for PEC on Slave side.

Changes from v1:
 -Address review comments from Andy Shevchenko
  Update commit message,
  Rewrite bcm_iproc_smbus_check_slave_pec() to remove local
  variable ret and type casting,
  Use positive condition.

Rayagonda Kokatanur (2):
  i2c: add PEC error event
  i2c: iproc: add slave pec support

 drivers/i2c/busses/i2c-bcm-iproc.c | 49 +++++++++++++++++++++++++++---
 include/linux/i2c.h                |  1 +
 2 files changed, 46 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH v2 0/2] add PEC support on slave side

[Rayagonda Kokatanur]

This patch set adds support for PEC on Slave side.

Changes from v1:
 -Address review comments from Andy Shevchenko
  Update commit message,
  Rewrite bcm_iproc_smbus_check_slave_pec() to remove local
  variable ret and type casting,
  Use positive condition.

Rayagonda Kokatanur (2):
  i2c: add PEC error event
  i2c: iproc: add slave pec support

 drivers/i2c/busses/i2c-bcm-iproc.c | 49 +++++++++++++++++++++++++++---
 include/linux/i2c.h                |  1 +
 2 files changed, 46 insertions(+), 4 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] i2c: add PEC error event

[Rayagonda Kokatanur]

Add new event I2C_SLAVE_PEC_ERR to list of slave events.
This event will be used by slave bus driver to indicate
PEC error to slave client or backend driver.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 include/linux/i2c.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b8b8963f8bb9..e04acd04eb48 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -370,6 +370,7 @@ enum i2c_slave_event {
 	I2C_SLAVE_READ_PROCESSED,
 	I2C_SLAVE_WRITE_RECEIVED,
 	I2C_SLAVE_STOP,
+	I2C_SLAVE_PEC_ERR,
 };
 
 int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
-- 
2.17.1

[PATCH v2 1/2] i2c: add PEC error event

[Rayagonda Kokatanur]

Add new event I2C_SLAVE_PEC_ERR to list of slave events.
This event will be used by slave bus driver to indicate
PEC error to slave client or backend driver.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 include/linux/i2c.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b8b8963f8bb9..e04acd04eb48 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -370,6 +370,7 @@ enum i2c_slave_event {
 	I2C_SLAVE_READ_PROCESSED,
 	I2C_SLAVE_WRITE_RECEIVED,
 	I2C_SLAVE_STOP,
+	I2C_SLAVE_PEC_ERR,
 };
 
 int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] i2c: iproc: add slave pec support

[Rayagonda Kokatanur]

Iproc supports PEC computation and checking in both Master
and Slave mode.

This patch adds support for PEC in slave mode.

As per hw spec, PEC ERROR status bits are [29:28] in S_RX_OFFSET
register, hence this patch corrects the S_RX_PEC_ERR_SHIFT.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
Changes from v1:
 -Address review comments from Andy Shevchenko
  Update commit message,
  Rewrite bcm_iproc_smbus_check_slave_pec() to remove local
  variable ret and type casting,
  Use positive condition.

 drivers/i2c/busses/i2c-bcm-iproc.c | 49 +++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 8a3c98866fb7..cfa7b044209e 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -93,6 +93,7 @@
 #define S_CMD_STATUS_MASK            0x07
 #define S_CMD_STATUS_SUCCESS         0x0
 #define S_CMD_STATUS_TIMEOUT         0x5
+#define S_CMD_PEC_SHIFT              8
 
 #define IE_OFFSET                    0x38
 #define IE_M_RX_FIFO_FULL_SHIFT      31
@@ -138,7 +139,9 @@
 #define S_RX_OFFSET                  0x4c
 #define S_RX_STATUS_SHIFT            30
 #define S_RX_STATUS_MASK             0x03
-#define S_RX_PEC_ERR_SHIFT           29
+#define S_RX_PEC_ERR_SHIFT           28
+#define S_RX_PEC_ERR_MASK            0x3
+#define S_RX_PEC_ERR                 0x1
 #define S_RX_DATA_SHIFT              0
 #define S_RX_DATA_MASK               0xff
 
@@ -205,6 +208,8 @@ struct bcm_iproc_i2c_dev {
 	/* bytes that have been read */
 	unsigned int rx_bytes;
 	unsigned int thld_bytes;
+
+	bool en_s_pec;
 };
 
 /*
@@ -321,6 +326,23 @@ static void bcm_iproc_i2c_check_slave_status(
 	}
 }
 
+static int bcm_iproc_smbus_check_slave_pec(struct bcm_iproc_i2c_dev *iproc_i2c,
+					   u32 val)
+{
+	u8 err_status;
+
+	if (!iproc_i2c->en_s_pec)
+		return 0;
+
+	err_status = (val >> S_RX_PEC_ERR_SHIFT) & S_RX_PEC_ERR_MASK;
+	if (err_status == S_RX_PEC_ERR) {
+		dev_err(iproc_i2c->device, "Slave PEC error\n");
+		return -EBADMSG;
+	}
+
+	return 0;
+}
+
 static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 				    u32 status)
 {
@@ -347,6 +369,8 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 			iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
 
 			val = BIT(S_CMD_START_BUSY_SHIFT);
+			if (iproc_i2c->en_s_pec)
+				val |= BIT(S_CMD_PEC_SHIFT);
 			iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
 
 			/*
@@ -361,9 +385,19 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 			value = (u8)((val >> S_RX_DATA_SHIFT) & S_RX_DATA_MASK);
 			i2c_slave_event(iproc_i2c->slave,
 					I2C_SLAVE_WRITE_RECEIVED, &value);
-			if (rx_status == I2C_SLAVE_RX_END)
-				i2c_slave_event(iproc_i2c->slave,
-						I2C_SLAVE_STOP, &value);
+			if (rx_status == I2C_SLAVE_RX_END) {
+				int ret;
+
+				ret = bcm_iproc_smbus_check_slave_pec(iproc_i2c,
+								      val);
+				if (ret)
+					i2c_slave_event(iproc_i2c->slave,
+							I2C_SLAVE_PEC_ERR,
+							&value);
+				else
+					i2c_slave_event(iproc_i2c->slave,
+							I2C_SLAVE_STOP, &value);
+			}
 		}
 	} else if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
 		/* Master read other than start */
@@ -372,6 +406,8 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 
 		iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
 		val = BIT(S_CMD_START_BUSY_SHIFT);
+		if (iproc_i2c->en_s_pec)
+			val |= BIT(S_CMD_PEC_SHIFT);
 		iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
 	}
 
@@ -1065,6 +1101,11 @@ static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave)
 	if (slave->flags & I2C_CLIENT_TEN)
 		return -EAFNOSUPPORT;
 
+	/* Enable partial slave HW PEC support if requested by the client */
+	iproc_i2c->en_s_pec = !!(slave->flags & I2C_CLIENT_PEC);
+	if (iproc_i2c->en_s_pec)
+		dev_info(iproc_i2c->device, "Enable PEC\n");
+
 	iproc_i2c->slave = slave;
 	bcm_iproc_i2c_slave_init(iproc_i2c, false);
 	return 0;
-- 
2.17.1

[PATCH v2 2/2] i2c: iproc: add slave pec support

[Rayagonda Kokatanur]

Iproc supports PEC computation and checking in both Master
and Slave mode.

This patch adds support for PEC in slave mode.

As per hw spec, PEC ERROR status bits are [29:28] in S_RX_OFFSET
register, hence this patch corrects the S_RX_PEC_ERR_SHIFT.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
Changes from v1:
 -Address review comments from Andy Shevchenko
  Update commit message,
  Rewrite bcm_iproc_smbus_check_slave_pec() to remove local
  variable ret and type casting,
  Use positive condition.

 drivers/i2c/busses/i2c-bcm-iproc.c | 49 +++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 8a3c98866fb7..cfa7b044209e 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -93,6 +93,7 @@
 #define S_CMD_STATUS_MASK            0x07
 #define S_CMD_STATUS_SUCCESS         0x0
 #define S_CMD_STATUS_TIMEOUT         0x5
+#define S_CMD_PEC_SHIFT              8
 
 #define IE_OFFSET                    0x38
 #define IE_M_RX_FIFO_FULL_SHIFT      31
@@ -138,7 +139,9 @@
 #define S_RX_OFFSET                  0x4c
 #define S_RX_STATUS_SHIFT            30
 #define S_RX_STATUS_MASK             0x03
-#define S_RX_PEC_ERR_SHIFT           29
+#define S_RX_PEC_ERR_SHIFT           28
+#define S_RX_PEC_ERR_MASK            0x3
+#define S_RX_PEC_ERR                 0x1
 #define S_RX_DATA_SHIFT              0
 #define S_RX_DATA_MASK               0xff
 
@@ -205,6 +208,8 @@ struct bcm_iproc_i2c_dev {
 	/* bytes that have been read */
 	unsigned int rx_bytes;
 	unsigned int thld_bytes;
+
+	bool en_s_pec;
 };
 
 /*
@@ -321,6 +326,23 @@ static void bcm_iproc_i2c_check_slave_status(
 	}
 }
 
+static int bcm_iproc_smbus_check_slave_pec(struct bcm_iproc_i2c_dev *iproc_i2c,
+					   u32 val)
+{
+	u8 err_status;
+
+	if (!iproc_i2c->en_s_pec)
+		return 0;
+
+	err_status = (val >> S_RX_PEC_ERR_SHIFT) & S_RX_PEC_ERR_MASK;
+	if (err_status == S_RX_PEC_ERR) {
+		dev_err(iproc_i2c->device, "Slave PEC error\n");
+		return -EBADMSG;
+	}
+
+	return 0;
+}
+
 static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 				    u32 status)
 {
@@ -347,6 +369,8 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 			iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
 
 			val = BIT(S_CMD_START_BUSY_SHIFT);
+			if (iproc_i2c->en_s_pec)
+				val |= BIT(S_CMD_PEC_SHIFT);
 			iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
 
 			/*
@@ -361,9 +385,19 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 			value = (u8)((val >> S_RX_DATA_SHIFT) & S_RX_DATA_MASK);
 			i2c_slave_event(iproc_i2c->slave,
 					I2C_SLAVE_WRITE_RECEIVED, &value);
-			if (rx_status == I2C_SLAVE_RX_END)
-				i2c_slave_event(iproc_i2c->slave,
-						I2C_SLAVE_STOP, &value);
+			if (rx_status == I2C_SLAVE_RX_END) {
+				int ret;
+
+				ret = bcm_iproc_smbus_check_slave_pec(iproc_i2c,
+								      val);
+				if (ret)
+					i2c_slave_event(iproc_i2c->slave,
+							I2C_SLAVE_PEC_ERR,
+							&value);
+				else
+					i2c_slave_event(iproc_i2c->slave,
+							I2C_SLAVE_STOP, &value);
+			}
 		}
 	} else if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
 		/* Master read other than start */
@@ -372,6 +406,8 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 
 		iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
 		val = BIT(S_CMD_START_BUSY_SHIFT);
+		if (iproc_i2c->en_s_pec)
+			val |= BIT(S_CMD_PEC_SHIFT);
 		iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
 	}
 
@@ -1065,6 +1101,11 @@ static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave)
 	if (slave->flags & I2C_CLIENT_TEN)
 		return -EAFNOSUPPORT;
 
+	/* Enable partial slave HW PEC support if requested by the client */
+	iproc_i2c->en_s_pec = !!(slave->flags & I2C_CLIENT_PEC);
+	if (iproc_i2c->en_s_pec)
+		dev_info(iproc_i2c->device, "Enable PEC\n");
+
 	iproc_i2c->slave = slave;
 	bcm_iproc_i2c_slave_init(iproc_i2c, false);
 	return 0;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] add PEC support on slave side

[Andy Shevchenko]

On Fri, Jul 17, 2020 at 02:31:53PM +0530, Rayagonda Kokatanur wrote:
> This patch set adds support for PEC on Slave side.

LGTM! FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Changes from v1:
>  -Address review comments from Andy Shevchenko
>   Update commit message,
>   Rewrite bcm_iproc_smbus_check_slave_pec() to remove local
>   variable ret and type casting,
>   Use positive condition.
> 
> Rayagonda Kokatanur (2):
>   i2c: add PEC error event
>   i2c: iproc: add slave pec support
> 
>  drivers/i2c/busses/i2c-bcm-iproc.c | 49 +++++++++++++++++++++++++++---
>  include/linux/i2c.h                |  1 +
>  2 files changed, 46 insertions(+), 4 deletions(-)
> 
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] add PEC support on slave side

[Andy Shevchenko]

On Fri, Jul 17, 2020 at 02:31:53PM +0530, Rayagonda Kokatanur wrote:
> This patch set adds support for PEC on Slave side.

LGTM! FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Changes from v1:
>  -Address review comments from Andy Shevchenko
>   Update commit message,
>   Rewrite bcm_iproc_smbus_check_slave_pec() to remove local
>   variable ret and type casting,
>   Use positive condition.
> 
> Rayagonda Kokatanur (2):
>   i2c: add PEC error event
>   i2c: iproc: add slave pec support
> 
>  drivers/i2c/busses/i2c-bcm-iproc.c | 49 +++++++++++++++++++++++++++---
>  include/linux/i2c.h                |  1 +
>  2 files changed, 46 insertions(+), 4 deletions(-)
> 
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] i2c: add PEC error event

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 959 bytes --]

On Fri, Jul 17, 2020 at 02:31:54PM +0530, Rayagonda Kokatanur wrote:
> Add new event I2C_SLAVE_PEC_ERR to list of slave events.
> This event will be used by slave bus driver to indicate
> PEC error to slave client or backend driver.
> 
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

Definately needs documentation in Documentation/i2c/slave-interface.rst.

What is a backend supposed to do? Does 'value' have a meaning?

> ---
>  include/linux/i2c.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index b8b8963f8bb9..e04acd04eb48 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -370,6 +370,7 @@ enum i2c_slave_event {
>  	I2C_SLAVE_READ_PROCESSED,
>  	I2C_SLAVE_WRITE_RECEIVED,
>  	I2C_SLAVE_STOP,
> +	I2C_SLAVE_PEC_ERR,
>  };
>  
>  int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] i2c: add PEC error event

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 959 bytes --]

On Fri, Jul 17, 2020 at 02:31:54PM +0530, Rayagonda Kokatanur wrote:
> Add new event I2C_SLAVE_PEC_ERR to list of slave events.
> This event will be used by slave bus driver to indicate
> PEC error to slave client or backend driver.
> 
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

Definately needs documentation in Documentation/i2c/slave-interface.rst.

What is a backend supposed to do? Does 'value' have a meaning?

> ---
>  include/linux/i2c.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index b8b8963f8bb9..e04acd04eb48 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -370,6 +370,7 @@ enum i2c_slave_event {
>  	I2C_SLAVE_READ_PROCESSED,
>  	I2C_SLAVE_WRITE_RECEIVED,
>  	I2C_SLAVE_STOP,
> +	I2C_SLAVE_PEC_ERR,
>  };
>  
>  int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
> -- 
> 2.17.1
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] i2c: iproc: add slave pec support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]


> +	/* Enable partial slave HW PEC support if requested by the client */
> +	iproc_i2c->en_s_pec = !!(slave->flags & I2C_CLIENT_PEC);
> +	if (iproc_i2c->en_s_pec)
> +		dev_info(iproc_i2c->device, "Enable PEC\n");

Where do you set the I2C_CLIENT_PEC flag for the slave? Is your backend
code publicly available?

I may need a second thought here, but I am not sure I2C_CLIENT_PEC is
the right way to enable PEC. Isn't it actually depending on the backend
if PEC is needed? I.e. is the backend an I2C device or an SMBus device?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/2] i2c: iproc: add slave pec support

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 528 bytes --]


> +	/* Enable partial slave HW PEC support if requested by the client */
> +	iproc_i2c->en_s_pec = !!(slave->flags & I2C_CLIENT_PEC);
> +	if (iproc_i2c->en_s_pec)
> +		dev_info(iproc_i2c->device, "Enable PEC\n");

Where do you set the I2C_CLIENT_PEC flag for the slave? Is your backend
code publicly available?

I may need a second thought here, but I am not sure I2C_CLIENT_PEC is
the right way to enable PEC. Isn't it actually depending on the backend
if PEC is needed? I.e. is the backend an I2C device or an SMBus device?


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] i2c: add PEC error event

[Rayagonda Kokatanur]

On Fri, Jul 24, 2020 at 1:46 AM Wolfram Sang <wsa@kernel.org> wrote:
>
> On Fri, Jul 17, 2020 at 02:31:54PM +0530, Rayagonda Kokatanur wrote:
> > Add new event I2C_SLAVE_PEC_ERR to list of slave events.
> > This event will be used by slave bus driver to indicate
> > PEC error to slave client or backend driver.
> >
> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>
> Definately needs documentation in Documentation/i2c/slave-interface.rst.

Okay, I will update the doc.

>
> What is a backend supposed to do? Does 'value' have a meaning?

According to SMBUS spec, during slave receive transfer, it is
encouraged to issue NACK if the received PEC is not correct.
Also PEC errors discovered above the data link layer may also be
indicated with a NACK if the device is fast enough to discover and
indicate the error when the NACK is due.

If a device doesn't support issuing NACK and issuing NACK above the
data link layer is not possible because the device is not fast then in
that case we can send error code (I2C_SLAVE_PEC_ERR ) to the backend
driver. Backend drivers which support PEC should check for this error
code and take action such as discarding the data.

Best regards,
Rayagonda

>
> > ---
> >  include/linux/i2c.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index b8b8963f8bb9..e04acd04eb48 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -370,6 +370,7 @@ enum i2c_slave_event {
> >       I2C_SLAVE_READ_PROCESSED,
> >       I2C_SLAVE_WRITE_RECEIVED,
> >       I2C_SLAVE_STOP,
> > +     I2C_SLAVE_PEC_ERR,
> >  };
> >
> >  int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
> > --
> > 2.17.1
> >

Re: [PATCH v2 1/2] i2c: add PEC error event

[Rayagonda Kokatanur]

On Fri, Jul 24, 2020 at 1:46 AM Wolfram Sang <wsa@kernel.org> wrote:
>
> On Fri, Jul 17, 2020 at 02:31:54PM +0530, Rayagonda Kokatanur wrote:
> > Add new event I2C_SLAVE_PEC_ERR to list of slave events.
> > This event will be used by slave bus driver to indicate
> > PEC error to slave client or backend driver.
> >
> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>
> Definately needs documentation in Documentation/i2c/slave-interface.rst.

Okay, I will update the doc.

>
> What is a backend supposed to do? Does 'value' have a meaning?

According to SMBUS spec, during slave receive transfer, it is
encouraged to issue NACK if the received PEC is not correct.
Also PEC errors discovered above the data link layer may also be
indicated with a NACK if the device is fast enough to discover and
indicate the error when the NACK is due.

If a device doesn't support issuing NACK and issuing NACK above the
data link layer is not possible because the device is not fast then in
that case we can send error code (I2C_SLAVE_PEC_ERR ) to the backend
driver. Backend drivers which support PEC should check for this error
code and take action such as discarding the data.

Best regards,
Rayagonda

>
> > ---
> >  include/linux/i2c.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index b8b8963f8bb9..e04acd04eb48 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -370,6 +370,7 @@ enum i2c_slave_event {
> >       I2C_SLAVE_READ_PROCESSED,
> >       I2C_SLAVE_WRITE_RECEIVED,
> >       I2C_SLAVE_STOP,
> > +     I2C_SLAVE_PEC_ERR,
> >  };
> >
> >  int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
> > --
> > 2.17.1
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] i2c: iproc: add slave pec support

[Rayagonda Kokatanur]

On Fri, Jul 24, 2020 at 1:50 AM Wolfram Sang <wsa@kernel.org> wrote:
>
>
> > +     /* Enable partial slave HW PEC support if requested by the client */
> > +     iproc_i2c->en_s_pec = !!(slave->flags & I2C_CLIENT_PEC);
> > +     if (iproc_i2c->en_s_pec)
> > +             dev_info(iproc_i2c->device, "Enable PEC\n");
>
> Where do you set the I2C_CLIENT_PEC flag for the slave? Is your backend
> code publicly available?

I2C_CLIENT_PEC should be set by backend before calling i2c_slave_register() ie

client->flags |= I2C_CLIENT_PEC;
ret = i2c_slave_register(client, i2c_slave_eeprom_slave_cb);
------
------
------

My backend code is not yet publicly available.

>
> I may need a second thought here, but I am not sure I2C_CLIENT_PEC is
> the right way to enable PEC. Isn't it actually depending on the backend
> if PEC is needed? I.e. is the backend an I2C device or an SMBus device?
>
Yes, it depends on the backend. If backend is SMBUS device and
supports PEC then it should set client->flags |= I2C_CLIENT_PEC,
before calling i2c_slave_register(), so that the slave bus driver will
enable PEC in device.

Best regards,
Rayagonda

Re: [PATCH v2 2/2] i2c: iproc: add slave pec support

[Rayagonda Kokatanur]

On Fri, Jul 24, 2020 at 1:50 AM Wolfram Sang <wsa@kernel.org> wrote:
>
>
> > +     /* Enable partial slave HW PEC support if requested by the client */
> > +     iproc_i2c->en_s_pec = !!(slave->flags & I2C_CLIENT_PEC);
> > +     if (iproc_i2c->en_s_pec)
> > +             dev_info(iproc_i2c->device, "Enable PEC\n");
>
> Where do you set the I2C_CLIENT_PEC flag for the slave? Is your backend
> code publicly available?

I2C_CLIENT_PEC should be set by backend before calling i2c_slave_register() ie

client->flags |= I2C_CLIENT_PEC;
ret = i2c_slave_register(client, i2c_slave_eeprom_slave_cb);
------
------
------

My backend code is not yet publicly available.

>
> I may need a second thought here, but I am not sure I2C_CLIENT_PEC is
> the right way to enable PEC. Isn't it actually depending on the backend
> if PEC is needed? I.e. is the backend an I2C device or an SMBus device?
>
Yes, it depends on the backend. If backend is SMBUS device and
supports PEC then it should set client->flags |= I2C_CLIENT_PEC,
before calling i2c_slave_register(), so that the slave bus driver will
enable PEC in device.

Best regards,
Rayagonda

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel