[PATCH 0/5] i2c: cadence: Small cleanups

[PATCH 0/5] i2c: cadence: Small cleanups

[Lars-Peter Clausen]

This series has a set of small cleanups for the Cadence I2C driver. The
changes are mostly cosmetical and there is no change in behavior of the
driver.

Lars-Peter Clausen (5):
  i2c: cadence: Remove unused CDNS_I2C_DATA_INTR_DEPTH define
  i2c: cadence: Remove `irq` field from driver state struct
  i2c: cadence: Remove redundant expression in if clause
  i2c: cadence: Remove always false ternary operator
  i2c: cadence: Remove unnecessary register reads

 drivers/i2c/busses/i2c-cadence.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

-- 
2.30.2

[PATCH 1/5] i2c: cadence: Remove unused CDNS_I2C_DATA_INTR_DEPTH define

[Lars-Peter Clausen]

The CDNS_I2C_DATA_INTR_DEPTH is not used in the Cadence I2C driver. Remove
it.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/i2c/busses/i2c-cadence.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index f58943cb1341..71ea658f4bd3 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -115,8 +115,6 @@
 #define CNDS_I2C_PM_TIMEOUT		1000	/* ms */
 
 #define CDNS_I2C_FIFO_DEPTH		16
-/* FIFO depth at which the DATA interrupt occurs */
-#define CDNS_I2C_DATA_INTR_DEPTH	(CDNS_I2C_FIFO_DEPTH - 2)
 #define CDNS_I2C_MAX_TRANSFER_SIZE	255
 /* Transfer size in multiples of data interrupt depth */
 #define CDNS_I2C_TRANSFER_SIZE	(CDNS_I2C_MAX_TRANSFER_SIZE - 3)
-- 
2.30.2

Re: [PATCH 1/5] i2c: cadence: Remove unused CDNS_I2C_DATA_INTR_DEPTH define

[Michal Simek]

On 1/7/23 22:18, Lars-Peter Clausen wrote:
> 
> The CDNS_I2C_DATA_INTR_DEPTH is not used in the Cadence I2C driver. Remove
> it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>   drivers/i2c/busses/i2c-cadence.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index f58943cb1341..71ea658f4bd3 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -115,8 +115,6 @@
>   #define CNDS_I2C_PM_TIMEOUT            1000    /* ms */
> 
>   #define CDNS_I2C_FIFO_DEPTH            16
> -/* FIFO depth at which the DATA interrupt occurs */
> -#define CDNS_I2C_DATA_INTR_DEPTH       (CDNS_I2C_FIFO_DEPTH - 2)
>   #define CDNS_I2C_MAX_TRANSFER_SIZE     255
>   /* Transfer size in multiples of data interrupt depth */
>   #define CDNS_I2C_TRANSFER_SIZE (CDNS_I2C_MAX_TRANSFER_SIZE - 3)
> --
> 2.30.2
> 

Reviewed-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

Re: [PATCH 1/5] i2c: cadence: Remove unused CDNS_I2C_DATA_INTR_DEPTH define

[Wolfram Sang]

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

On Sat, Jan 07, 2023 at 01:18:10PM -0800, Lars-Peter Clausen wrote:
> The CDNS_I2C_DATA_INTR_DEPTH is not used in the Cadence I2C driver. Remove
> it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Applied to for-next, thanks!


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

[PATCH 2/5] i2c: cadence: Remove `irq` field from driver state struct

[Lars-Peter Clausen]

The irq field of the driver state struct is only used in the probe
function. There is no need to keep it around. Remove it from the state
struct and use a on-stack variable instead.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/i2c/busses/i2c-cadence.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 71ea658f4bd3..e2a4cb694cfb 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -173,7 +173,6 @@ enum cdns_i2c_slave_state {
  * @send_count:		Number of bytes still expected to send
  * @recv_count:		Number of bytes still expected to receive
  * @curr_recv_count:	Number of bytes to be received in current transfer
- * @irq:		IRQ number
  * @input_clk:		Input clock to I2C controller
  * @i2c_clk:		Maximum I2C clock speed
  * @bus_hold_flag:	Flag used in repeated start for clearing HOLD bit
@@ -198,7 +197,6 @@ struct cdns_i2c {
 	unsigned int send_count;
 	unsigned int recv_count;
 	unsigned int curr_recv_count;
-	int irq;
 	unsigned long input_clk;
 	unsigned int i2c_clk;
 	unsigned int bus_hold_flag;
@@ -1244,7 +1242,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 {
 	struct resource *r_mem;
 	struct cdns_i2c *id;
-	int ret;
+	int ret, irq;
 	const struct of_device_id *match;
 
 	id = devm_kzalloc(&pdev->dev, sizeof(*id), GFP_KERNEL);
@@ -1275,10 +1273,9 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR(id->membase))
 		return PTR_ERR(id->membase);
 
-	ret = platform_get_irq(pdev, 0);
-	if (ret < 0)
-		return ret;
-	id->irq = ret;
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
 
 	id->adap.owner = THIS_MODULE;
 	id->adap.dev.of_node = pdev->dev.of_node;
@@ -1329,10 +1326,10 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 		goto err_clk_dis;
 	}
 
-	ret = devm_request_irq(&pdev->dev, id->irq, cdns_i2c_isr, 0,
+	ret = devm_request_irq(&pdev->dev, irq, cdns_i2c_isr, 0,
 				 DRIVER_NAME, id);
 	if (ret) {
-		dev_err(&pdev->dev, "cannot get irq %d\n", id->irq);
+		dev_err(&pdev->dev, "cannot get irq %d\n", irq);
 		goto err_clk_dis;
 	}
 	cdns_i2c_init(id);
@@ -1342,7 +1339,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 		goto err_clk_dis;
 
 	dev_info(&pdev->dev, "%u kHz mmio %08lx irq %d\n",
-		 id->i2c_clk / 1000, (unsigned long)r_mem->start, id->irq);
+		 id->i2c_clk / 1000, (unsigned long)r_mem->start, irq);
 
 	return 0;
 
-- 
2.30.2

Re: [PATCH 2/5] i2c: cadence: Remove `irq` field from driver state struct

[Michal Simek]

On 1/7/23 22:18, Lars-Peter Clausen wrote:
> 
> The irq field of the driver state struct is only used in the probe
> function. There is no need to keep it around. Remove it from the state
> struct and use a on-stack variable instead.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>   drivers/i2c/busses/i2c-cadence.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index 71ea658f4bd3..e2a4cb694cfb 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -173,7 +173,6 @@ enum cdns_i2c_slave_state {
>    * @send_count:                Number of bytes still expected to send
>    * @recv_count:                Number of bytes still expected to receive
>    * @curr_recv_count:   Number of bytes to be received in current transfer
> - * @irq:               IRQ number
>    * @input_clk:         Input clock to I2C controller
>    * @i2c_clk:           Maximum I2C clock speed
>    * @bus_hold_flag:     Flag used in repeated start for clearing HOLD bit
> @@ -198,7 +197,6 @@ struct cdns_i2c {
>          unsigned int send_count;
>          unsigned int recv_count;
>          unsigned int curr_recv_count;
> -       int irq;
>          unsigned long input_clk;
>          unsigned int i2c_clk;
>          unsigned int bus_hold_flag;
> @@ -1244,7 +1242,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>   {
>          struct resource *r_mem;
>          struct cdns_i2c *id;
> -       int ret;
> +       int ret, irq;
>          const struct of_device_id *match;
> 
>          id = devm_kzalloc(&pdev->dev, sizeof(*id), GFP_KERNEL);
> @@ -1275,10 +1273,9 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>          if (IS_ERR(id->membase))
>                  return PTR_ERR(id->membase);
> 
> -       ret = platform_get_irq(pdev, 0);
> -       if (ret < 0)
> -               return ret;
> -       id->irq = ret;
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0)
> +               return irq;
> 
>          id->adap.owner = THIS_MODULE;
>          id->adap.dev.of_node = pdev->dev.of_node;
> @@ -1329,10 +1326,10 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>                  goto err_clk_dis;
>          }
> 
> -       ret = devm_request_irq(&pdev->dev, id->irq, cdns_i2c_isr, 0,
> +       ret = devm_request_irq(&pdev->dev, irq, cdns_i2c_isr, 0,
>                                   DRIVER_NAME, id);
>          if (ret) {
> -               dev_err(&pdev->dev, "cannot get irq %d\n", id->irq);
> +               dev_err(&pdev->dev, "cannot get irq %d\n", irq);
>                  goto err_clk_dis;
>          }
>          cdns_i2c_init(id);
> @@ -1342,7 +1339,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>                  goto err_clk_dis;
> 
>          dev_info(&pdev->dev, "%u kHz mmio %08lx irq %d\n",
> -                id->i2c_clk / 1000, (unsigned long)r_mem->start, id->irq);
> +                id->i2c_clk / 1000, (unsigned long)r_mem->start, irq);
> 
>          return 0;
> 
> --
> 2.30.2
> 

Reviewed-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

Re: [PATCH 2/5] i2c: cadence: Remove `irq` field from driver state struct

[Wolfram Sang]

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

On Sat, Jan 07, 2023 at 01:18:11PM -0800, Lars-Peter Clausen wrote:
> The irq field of the driver state struct is only used in the probe
> function. There is no need to keep it around. Remove it from the state
> struct and use a on-stack variable instead.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Applied to for-next, thanks!


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

[PATCH 3/5] i2c: cadence: Remove redundant expression in if clause

[Lars-Peter Clausen]

In the mrecv() function the Cadence I2C driver has the following expression
in an if clause.

	((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
	 (id->recv_count <= CDNS_I2C_FIFO_DEPTH))

Earlier in the same function when I2C_M_RECV_LEN is set the recv_count is
initialized to a value that is larger than CDNS_I2C_FIFO_DEPTH. This means
if the first expression is false the second expression is also false.
Checking the first expression is thus redundant and can be removed.

This slightly simplifies the logic.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/i2c/busses/i2c-cadence.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index e2a4cb694cfb..b5d22e7282c2 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -612,9 +612,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
 	}
 
 	/* Determine hold_clear based on number of bytes to receive and hold flag */
-	if (!id->bus_hold_flag &&
-	    ((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
-	    (id->recv_count <= CDNS_I2C_FIFO_DEPTH)) {
+	if (!id->bus_hold_flag && id->recv_count <= CDNS_I2C_FIFO_DEPTH) {
 		if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & CDNS_I2C_CR_HOLD) {
 			hold_clear = true;
 			if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT)
-- 
2.30.2

Re: [PATCH 3/5] i2c: cadence: Remove redundant expression in if clause

[Michal Simek]

On 1/7/23 22:18, Lars-Peter Clausen wrote:
> 
> 
> In the mrecv() function the Cadence I2C driver has the following expression
> in an if clause.
> 
>          ((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
>           (id->recv_count <= CDNS_I2C_FIFO_DEPTH))
> 
> Earlier in the same function when I2C_M_RECV_LEN is set the recv_count is
> initialized to a value that is larger than CDNS_I2C_FIFO_DEPTH. This means
> if the first expression is false the second expression is also false.
> Checking the first expression is thus redundant and can be removed.
> 
> This slightly simplifies the logic.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>   drivers/i2c/busses/i2c-cadence.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index e2a4cb694cfb..b5d22e7282c2 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -612,9 +612,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>          }
> 
>          /* Determine hold_clear based on number of bytes to receive and hold flag */
> -       if (!id->bus_hold_flag &&
> -           ((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
> -           (id->recv_count <= CDNS_I2C_FIFO_DEPTH)) {
> +       if (!id->bus_hold_flag && id->recv_count <= CDNS_I2C_FIFO_DEPTH) {
>                  if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & CDNS_I2C_CR_HOLD) {
>                          hold_clear = true;
>                          if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT)
> --
> 2.30.2
> 

Reviewed-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

Re: [PATCH 3/5] i2c: cadence: Remove redundant expression in if clause

[Wolfram Sang]

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

On Sat, Jan 07, 2023 at 01:18:12PM -0800, Lars-Peter Clausen wrote:
> In the mrecv() function the Cadence I2C driver has the following expression
> in an if clause.
> 
> 	((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
> 	 (id->recv_count <= CDNS_I2C_FIFO_DEPTH))
> 
> Earlier in the same function when I2C_M_RECV_LEN is set the recv_count is
> initialized to a value that is larger than CDNS_I2C_FIFO_DEPTH. This means
> if the first expression is false the second expression is also false.
> Checking the first expression is thus redundant and can be removed.
> 
> This slightly simplifies the logic.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Applied to for-next, thanks!


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

[PATCH 4/5] i2c: cadence: Remove always false ternary operator

[Lars-Peter Clausen]

When selecting the clock dividers the Cadence I2C driver skips settings
where the resulting I2C bus frequency is larger than the requested
frequency.

If the resulting frequency is lower it will compute the error to actual
frequency. When calculating the difference it also handles the case where
the resulting frequency is larger.

Since the resulting frequency is always smaller or equal the computation of
the error can be simplified by only considering this case.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/i2c/busses/i2c-cadence.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index b5d22e7282c2..bec50bfe7aad 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -1030,8 +1030,7 @@ static int cdns_i2c_calc_divs(unsigned long *f, unsigned long input_clk,
 		if (actual_fscl > fscl)
 			continue;
 
-		current_error = ((actual_fscl > fscl) ? (actual_fscl - fscl) :
-							(fscl - actual_fscl));
+		current_error = fscl - actual_fscl;
 
 		if (last_error > current_error) {
 			calc_div_a = div_a;
-- 
2.30.2

Re: [PATCH 4/5] i2c: cadence: Remove always false ternary operator

[Michal Simek]

On 1/7/23 22:18, Lars-Peter Clausen wrote:
> 
> When selecting the clock dividers the Cadence I2C driver skips settings
> where the resulting I2C bus frequency is larger than the requested
> frequency.
> 
> If the resulting frequency is lower it will compute the error to actual
> frequency. When calculating the difference it also handles the case where
> the resulting frequency is larger.
> 
> Since the resulting frequency is always smaller or equal the computation of
> the error can be simplified by only considering this case.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>   drivers/i2c/busses/i2c-cadence.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index b5d22e7282c2..bec50bfe7aad 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -1030,8 +1030,7 @@ static int cdns_i2c_calc_divs(unsigned long *f, unsigned long input_clk,
>                  if (actual_fscl > fscl)
>                          continue;
> 
> -               current_error = ((actual_fscl > fscl) ? (actual_fscl - fscl) :
> -                                                       (fscl - actual_fscl));
> +               current_error = fscl - actual_fscl;
> 
>                  if (last_error > current_error) {
>                          calc_div_a = div_a;
> --
> 2.30.2
> 

Shubhrajyoti: Please check this patch.

Thanks,
Michal

Re: [PATCH 4/5] i2c: cadence: Remove always false ternary operator

[Michal Simek]

On 1/7/23 22:18, Lars-Peter Clausen wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> 
> 
> When selecting the clock dividers the Cadence I2C driver skips settings
> where the resulting I2C bus frequency is larger than the requested
> frequency.
> 
> If the resulting frequency is lower it will compute the error to actual
> frequency. When calculating the difference it also handles the case where
> the resulting frequency is larger.
> 
> Since the resulting frequency is always smaller or equal the computation of
> the error can be simplified by only considering this case.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>   drivers/i2c/busses/i2c-cadence.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index b5d22e7282c2..bec50bfe7aad 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -1030,8 +1030,7 @@ static int cdns_i2c_calc_divs(unsigned long *f, unsigned long input_clk,
>                  if (actual_fscl > fscl)
>                          continue;
> 
> -               current_error = ((actual_fscl > fscl) ? (actual_fscl - fscl) :
> -                                                       (fscl - actual_fscl));
> +               current_error = fscl - actual_fscl;
> 
>                  if (last_error > current_error) {
>                          calc_div_a = div_a;
> --
> 2.30.2
> 

Mani has tested it that's why
Acked-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

Re: [PATCH 4/5] i2c: cadence: Remove always false ternary operator

[Wolfram Sang]

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

On Sat, Jan 07, 2023 at 01:18:13PM -0800, Lars-Peter Clausen wrote:
> When selecting the clock dividers the Cadence I2C driver skips settings
> where the resulting I2C bus frequency is larger than the requested
> frequency.
> 
> If the resulting frequency is lower it will compute the error to actual
> frequency. When calculating the difference it also handles the case where
> the resulting frequency is larger.
> 
> Since the resulting frequency is always smaller or equal the computation of
> the error can be simplified by only considering this case.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Applied to for-next, thanks!


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

[PATCH 5/5] i2c: cadence: Remove unnecessary register reads

[Lars-Peter Clausen]

In the `cdns_i2c_mrecv()` function the CTRL register of the Cadence I2C
controller is written and read back multiple times. The register value does
not change on its own. So it is possible to remember the just written value
instead of reading it back from the hardware.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/i2c/busses/i2c-cadence.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index bec50bfe7aad..93c6d0822468 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -613,7 +613,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
 
 	/* Determine hold_clear based on number of bytes to receive and hold flag */
 	if (!id->bus_hold_flag && id->recv_count <= CDNS_I2C_FIFO_DEPTH) {
-		if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & CDNS_I2C_CR_HOLD) {
+		if (ctrl_reg & CDNS_I2C_CR_HOLD) {
 			hold_clear = true;
 			if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT)
 				irq_save = true;
@@ -624,7 +624,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
 	addr &= CDNS_I2C_ADDR_MASK;
 
 	if (hold_clear) {
-		ctrl_reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & ~CDNS_I2C_CR_HOLD;
+		ctrl_reg &= ~CDNS_I2C_CR_HOLD;
 		/*
 		 * In case of Xilinx Zynq SOC, clear the HOLD bit before transfer size
 		 * register reaches '0'. This is an IP bug which causes transfer size
-- 
2.30.2

Re: [PATCH 5/5] i2c: cadence: Remove unnecessary register reads

[Michal Simek]

On 1/7/23 22:18, Lars-Peter Clausen wrote:
> 
> In the `cdns_i2c_mrecv()` function the CTRL register of the Cadence I2C
> controller is written and read back multiple times. The register value does
> not change on its own. So it is possible to remember the just written value
> instead of reading it back from the hardware.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>   drivers/i2c/busses/i2c-cadence.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index bec50bfe7aad..93c6d0822468 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -613,7 +613,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
> 
>          /* Determine hold_clear based on number of bytes to receive and hold flag */
>          if (!id->bus_hold_flag && id->recv_count <= CDNS_I2C_FIFO_DEPTH) {
> -               if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & CDNS_I2C_CR_HOLD) {
> +               if (ctrl_reg & CDNS_I2C_CR_HOLD) {
>                          hold_clear = true;
>                          if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT)
>                                  irq_save = true;
> @@ -624,7 +624,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>          addr &= CDNS_I2C_ADDR_MASK;
> 
>          if (hold_clear) {
> -               ctrl_reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & ~CDNS_I2C_CR_HOLD;
> +               ctrl_reg &= ~CDNS_I2C_CR_HOLD;
>                  /*
>                   * In case of Xilinx Zynq SOC, clear the HOLD bit before transfer size
>                   * register reaches '0'. This is an IP bug which causes transfer size
> --
> 2.30.2
> 

Logically this is fine but that additional read on CR register ensures that IP 
receive previous writes. The code itself is related to bug on Zynq SoC and that 
two additional readbacks can actually do something.

I think this should be properly tested on zynq to ensure that it doesn't break 
anything.

Shubhrajyoti: Can you please make sure that it is tested on Zynq?

Thanks,
Michal

Re: [PATCH 5/5] i2c: cadence: Remove unnecessary register reads

[Lars-Peter Clausen]

On 1/16/23 06:58, Michal Simek wrote:
>
>
> On 1/7/23 22:18, Lars-Peter Clausen wrote:
>>
>> In the `cdns_i2c_mrecv()` function the CTRL register of the Cadence I2C
>> controller is written and read back multiple times. The register 
>> value does
>> not change on its own. So it is possible to remember the just written 
>> value
>> instead of reading it back from the hardware.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>   drivers/i2c/busses/i2c-cadence.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-cadence.c 
>> b/drivers/i2c/busses/i2c-cadence.c
>> index bec50bfe7aad..93c6d0822468 100644
>> --- a/drivers/i2c/busses/i2c-cadence.c
>> +++ b/drivers/i2c/busses/i2c-cadence.c
>> @@ -613,7 +613,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>>
>>          /* Determine hold_clear based on number of bytes to receive 
>> and hold flag */
>>          if (!id->bus_hold_flag && id->recv_count <= 
>> CDNS_I2C_FIFO_DEPTH) {
>> -               if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & 
>> CDNS_I2C_CR_HOLD) {
>> +               if (ctrl_reg & CDNS_I2C_CR_HOLD) {
>>                          hold_clear = true;
>>                          if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT)
>>                                  irq_save = true;
>> @@ -624,7 +624,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>>          addr &= CDNS_I2C_ADDR_MASK;
>>
>>          if (hold_clear) {
>> -               ctrl_reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & 
>> ~CDNS_I2C_CR_HOLD;
>> +               ctrl_reg &= ~CDNS_I2C_CR_HOLD;
>>                  /*
>>                   * In case of Xilinx Zynq SOC, clear the HOLD bit 
>> before transfer size
>>                   * register reaches '0'. This is an IP bug which 
>> causes transfer size
>> -- 
>> 2.30.2
>>
>
> Logically this is fine but that additional read on CR register ensures 
> that IP receive previous writes. The code itself is related to bug on 
> Zynq SoC and that two additional readbacks can actually do something.
>
> I think this should be properly tested on zynq to ensure that it 
> doesn't break anything.
>
> Shubhrajyoti: Can you please make sure that it is tested on Zynq?
Maybe it is better to drop the patch then if it is used to enforce 
ordering in the hardware. But I guess we should add a comment to explain 
this.

Re: [PATCH 5/5] i2c: cadence: Remove unnecessary register reads

[Michal Simek]

On 1/16/23 18:14, Lars-Peter Clausen wrote:
> On 1/16/23 06:58, Michal Simek wrote:
>>
>>
>> On 1/7/23 22:18, Lars-Peter Clausen wrote:
>>>
>>> In the `cdns_i2c_mrecv()` function the CTRL register of the Cadence I2C
>>> controller is written and read back multiple times. The register value does
>>> not change on its own. So it is possible to remember the just written value
>>> instead of reading it back from the hardware.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>> ---
>>>   drivers/i2c/busses/i2c-cadence.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
>>> index bec50bfe7aad..93c6d0822468 100644
>>> --- a/drivers/i2c/busses/i2c-cadence.c
>>> +++ b/drivers/i2c/busses/i2c-cadence.c
>>> @@ -613,7 +613,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>>>
>>>          /* Determine hold_clear based on number of bytes to receive and hold 
>>> flag */
>>>          if (!id->bus_hold_flag && id->recv_count <= CDNS_I2C_FIFO_DEPTH) {
>>> -               if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & CDNS_I2C_CR_HOLD) {
>>> +               if (ctrl_reg & CDNS_I2C_CR_HOLD) {
>>>                          hold_clear = true;
>>>                          if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT)
>>>                                  irq_save = true;
>>> @@ -624,7 +624,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>>>          addr &= CDNS_I2C_ADDR_MASK;
>>>
>>>          if (hold_clear) {
>>> -               ctrl_reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & 
>>> ~CDNS_I2C_CR_HOLD;
>>> +               ctrl_reg &= ~CDNS_I2C_CR_HOLD;
>>>                  /*
>>>                   * In case of Xilinx Zynq SOC, clear the HOLD bit before 
>>> transfer size
>>>                   * register reaches '0'. This is an IP bug which causes 
>>> transfer size
>>> -- 
>>> 2.30.2
>>>
>>
>> Logically this is fine but that additional read on CR register ensures that IP 
>> receive previous writes. The code itself is related to bug on Zynq SoC and 
>> that two additional readbacks can actually do something.
>>
>> I think this should be properly tested on zynq to ensure that it doesn't break 
>> anything.
>>
>> Shubhrajyoti: Can you please make sure that it is tested on Zynq?
> Maybe it is better to drop the patch then if it is used to enforce ordering in 
> the hardware. But I guess we should add a comment to explain this.

Mani has tested it and he can't see any issue that's why I am fine both ways.

Thanks,
Michal

Re: [PATCH 5/5] i2c: cadence: Remove unnecessary register reads

[Wolfram Sang]

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


> Mani has tested it and he can't see any issue that's why I am fine both ways.

I read this as an ack.


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

Re: [PATCH 5/5] i2c: cadence: Remove unnecessary register reads

[Wolfram Sang]

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

On Sat, Jan 07, 2023 at 01:18:14PM -0800, Lars-Peter Clausen wrote:
> In the `cdns_i2c_mrecv()` function the CTRL register of the Cadence I2C
> controller is written and read back multiple times. The register value does
> not change on its own. So it is possible to remember the just written value
> instead of reading it back from the hardware.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Applied to for-next, thanks!


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

RE: [PATCH 0/5] i2c: cadence: Small cleanups

[Guntupalli, Manikanta]

Hi ,
> -----Original Message-----
> From: Lars-Peter Clausen <lars@metafoo.de>
> Sent: Sunday, January 8, 2023 2:48 AM
> To: Wolfram Sang <wsa@kernel.org>
> Cc: Michal Simek <michal.simek@xilinx.com>; Datta, Shubhrajyoti
> <shubhrajyoti.datta@amd.com>; linux-i2c@vger.kernel.org; Lars-Peter
> Clausen <lars@metafoo.de>
> Subject: [PATCH 0/5] i2c: cadence: Small cleanups
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> This series has a set of small cleanups for the Cadence I2C driver. The
> changes are mostly cosmetical and there is no change in behavior of the
> driver.
> 
Tested-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>

> Lars-Peter Clausen (5):
>   i2c: cadence: Remove unused CDNS_I2C_DATA_INTR_DEPTH define
>   i2c: cadence: Remove `irq` field from driver state struct
>   i2c: cadence: Remove redundant expression in if clause
>   i2c: cadence: Remove always false ternary operator
>   i2c: cadence: Remove unnecessary register reads
> 
>  drivers/i2c/busses/i2c-cadence.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> --
> 2.30.2