[PATCH RFT 0/5] i2c: sh_mobile: refactor HW init/deinit

[PATCH RFT 0/5] i2c: sh_mobile: refactor HW init/deinit

[Wolfram Sang]

When clearing the ICE bit, all registers fall back to their defaut value. That
allows for some simplifications in the code.

Tested on a Renesas Lager board (R-Car H2) doing a bunch of consecutive
commands. No spurious interrupts have been observed and the signals look
exactly the same when visualized with sigrok.

According to the docs, the ICE bit behaviour is the same since the beginning of
this driver (sh7722) for the migo-r board.

jacopo: can you please test this series while you work on migo-r anyhow? Thank
you a ton for that!

Looking forward to other comments, as well...


Wolfram Sang (5):
  i2c: sh_mobile: remove redundant initialization
  i2c: sh_mobile: remove redundant deinitialization
  i2c: sh_mobile: manually "inline" two short functions
  i2c: sh_mobile: use direct writes when accessing ICE bit
  i2c: sh_mobile: shorten exit of xfer routine

 drivers/i2c/busses/i2c-sh_mobile.c | 50 +++++++++-----------------------------
 1 file changed, 12 insertions(+), 38 deletions(-)

-- 
2.11.0

[PATCH RFT 1/5] i2c: sh_mobile: remove redundant initialization

[Wolfram Sang]

Following the documentation, we initialize the HW before each START in
start_ch(). No need to do the same in activate_ch().

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 6f2aaeb7c4fa15..9d073e99c7e72d 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -303,16 +303,6 @@ static void activate_ch(struct sh_mobile_i2c_data *pd)
 	/* Wake up device and enable clock */
 	pm_runtime_get_sync(pd->dev);
 	clk_prepare_enable(pd->clk);
-
-	/* Enable channel and configure rx ack */
-	iic_set_clr(pd, ICCR, ICCR_ICE, 0);
-
-	/* Mask all interrupts */
-	iic_wr(pd, ICIC, 0);
-
-	/* Set the clock */
-	iic_wr(pd, ICCL, pd->iccl & 0xff);
-	iic_wr(pd, ICCH, pd->icch & 0xff);
 }
 
 static void deactivate_ch(struct sh_mobile_i2c_data *pd)
-- 
2.11.0

[PATCH RFT 2/5] i2c: sh_mobile: remove redundant deinitialization

[Wolfram Sang]

No need to clear the interrupt registers because right after that we
disable the IP core which will reload registers with their initial
values anyhow.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 9d073e99c7e72d..a1253df9574594 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -307,10 +307,6 @@ static void activate_ch(struct sh_mobile_i2c_data *pd)
 
 static void deactivate_ch(struct sh_mobile_i2c_data *pd)
 {
-	/* Clear/disable interrupts */
-	iic_wr(pd, ICSR, 0);
-	iic_wr(pd, ICIC, 0);
-
 	/* Disable channel */
 	iic_set_clr(pd, ICCR, 0, ICCR_ICE);
 
-- 
2.11.0

[PATCH RFT 3/5] i2c: sh_mobile: manually "inline" two short functions

[Wolfram Sang]

Those two functions are very short and only called once. The code
becomes easier to understand if the code is directly put into the main
xfer function.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index a1253df9574594..cdd7a746b9d1ed 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -298,23 +298,6 @@ static int sh_mobile_i2c_init(struct sh_mobile_i2c_data *pd)
 	return 0;
 }
 
-static void activate_ch(struct sh_mobile_i2c_data *pd)
-{
-	/* Wake up device and enable clock */
-	pm_runtime_get_sync(pd->dev);
-	clk_prepare_enable(pd->clk);
-}
-
-static void deactivate_ch(struct sh_mobile_i2c_data *pd)
-{
-	/* Disable channel */
-	iic_set_clr(pd, ICCR, 0, ICCR_ICE);
-
-	/* Disable clock and mark device as idle */
-	clk_disable_unprepare(pd->clk);
-	pm_runtime_put_sync(pd->dev);
-}
-
 static unsigned char i2c_op(struct sh_mobile_i2c_data *pd,
 			    enum sh_mobile_i2c_op op, unsigned char data)
 {
@@ -717,7 +700,9 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
 	int i;
 	long timeout;
 
-	activate_ch(pd);
+	/* Wake up device and enable clock */
+	pm_runtime_get_sync(pd->dev);
+	clk_prepare_enable(pd->clk);
 
 	/* Process all messages */
 	for (i = 0; i < num; i++) {
@@ -754,7 +739,12 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
 			break;
 	}
 
-	deactivate_ch(pd);
+	/* Disable channel */
+	iic_set_clr(pd, ICCR, 0, ICCR_ICE);
+
+	/* Disable clock and mark device as idle */
+	clk_disable_unprepare(pd->clk);
+	pm_runtime_put_sync(pd->dev);
 
 	if (!err)
 		err = num;
-- 
2.11.0

Re: [PATCH RFT 3/5] i2c: sh_mobile: manually "inline" two short functions

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Nov 2, 2017 at 1:47 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Those two functions are very short and only called once. The code
> becomes easier to understand if the code is directly put into the main
> xfer function.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-sh_mobile.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
> index a1253df9574594..cdd7a746b9d1ed 100644
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -298,23 +298,6 @@ static int sh_mobile_i2c_init(struct sh_mobile_i2c_data *pd)
>         return 0;
>  }
>
> -static void activate_ch(struct sh_mobile_i2c_data *pd)
> -{
> -       /* Wake up device and enable clock */
> -       pm_runtime_get_sync(pd->dev);
> -       clk_prepare_enable(pd->clk);
> -}
> -
> -static void deactivate_ch(struct sh_mobile_i2c_data *pd)
> -{
> -       /* Disable channel */
> -       iic_set_clr(pd, ICCR, 0, ICCR_ICE);
> -
> -       /* Disable clock and mark device as idle */
> -       clk_disable_unprepare(pd->clk);
> -       pm_runtime_put_sync(pd->dev);
> -}
> -
>  static unsigned char i2c_op(struct sh_mobile_i2c_data *pd,
>                             enum sh_mobile_i2c_op op, unsigned char data)
>  {
> @@ -717,7 +700,9 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
>         int i;
>         long timeout;
>
> -       activate_ch(pd);
> +       /* Wake up device and enable clock */
> +       pm_runtime_get_sync(pd->dev);
> +       clk_prepare_enable(pd->clk);

Suggestion for further cleanup: the call to clk_prepare_enable() should
not be needed, due to Runtime PM taking care of that.
This is also the case on SH, which uses the legacy clock domain
(drivers/sh/pm_runtime.c).

>
>         /* Process all messages */
>         for (i = 0; i < num; i++) {
> @@ -754,7 +739,12 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
>                         break;
>         }
>
> -       deactivate_ch(pd);
> +       /* Disable channel */
> +       iic_set_clr(pd, ICCR, 0, ICCR_ICE);
> +
> +       /* Disable clock and mark device as idle */
> +       clk_disable_unprepare(pd->clk);

Likewise for clk_disable_unprepare().

> +       pm_runtime_put_sync(pd->dev);

Gr{oetje,eeting}s,

                        Geert

Re: [PATCH RFT 3/5] i2c: sh_mobile: manually "inline" two short functions

[Wolfram Sang]

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

> > -       activate_ch(pd);
> > +       /* Wake up device and enable clock */
> > +       pm_runtime_get_sync(pd->dev);
> > +       clk_prepare_enable(pd->clk);
> 
> Suggestion for further cleanup: the call to clk_prepare_enable() should
> not be needed, due to Runtime PM taking care of that.
> This is also the case on SH, which uses the legacy clock domain
> (drivers/sh/pm_runtime.c).

Ah, I didn't know that about SH. Thanks for the heads up!


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

[PATCH RFT 4/5] i2c: sh_mobile: use direct writes when accessing ICE bit

[Wolfram Sang]

ICE bit is for resetting the module. Other bits don't matter then, so we
don't need to use the iic_set_clr() function but can use iic_wr().

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index cdd7a746b9d1ed..1701f193d94acf 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -620,10 +620,10 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg,
 
 	if (do_init) {
 		/* Initialize channel registers */
-		iic_set_clr(pd, ICCR, 0, ICCR_ICE);
+		iic_wr(pd, ICCR, 0);
 
 		/* Enable channel and configure rx ack */
-		iic_set_clr(pd, ICCR, ICCR_ICE, 0);
+		iic_wr(pd, ICCR, ICCR_ICE);
 
 		/* Set the clock */
 		iic_wr(pd, ICCL, pd->iccl & 0xff);
@@ -740,7 +740,7 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
 	}
 
 	/* Disable channel */
-	iic_set_clr(pd, ICCR, 0, ICCR_ICE);
+	iic_wr(pd, ICCR, 0);
 
 	/* Disable clock and mark device as idle */
 	clk_disable_unprepare(pd->clk);
-- 
2.11.0

[PATCH RFT 5/5] i2c: sh_mobile: shorten exit of xfer routine

[Wolfram Sang]

We can use the ternary operator for easier reading.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 1701f193d94acf..39f0ee1cd78c74 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -746,9 +746,7 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
 	clk_disable_unprepare(pd->clk);
 	pm_runtime_put_sync(pd->dev);
 
-	if (!err)
-		err = num;
-	return err;
+	return err ?: num;
 }
 
 static u32 sh_mobile_i2c_func(struct i2c_adapter *adapter)
-- 
2.11.0

Re: [PATCH RFT 0/5] i2c: sh_mobile: refactor HW init/deinit

[jacopo mondi]

Hi Wolfram,

On Thu, Nov 02, 2017 at 01:47:26PM +0100, Wolfram Sang wrote:
> When clearing the ICE bit, all registers fall back to their defaut value. That
> allows for some simplifications in the code.
>
> Tested on a Renesas Lager board (R-Car H2) doing a bunch of consecutive
> commands. No spurious interrupts have been observed and the signals look
> exactly the same when visualized with sigrok.
>
> According to the docs, the ICE bit behaviour is the same since the beginning of
> this driver (sh7722) for the migo-r board.
>
> jacopo: can you please test this series while you work on migo-r anyhow? Thank
> you a ton for that!

I have applied your series on top of my developments on Migo-R

------------------------------------------------------------
57d007c i2c: sh_mobile: shorten exit of xfer routine
67dfa18 i2c: sh_mobile: use direct writes when accessing ICE bit
f44a2df i2c: sh_mobile: manually "inline" two short functions
5d656b6 i2c: sh_mobile: remove redundant deinitialization
9d2b8cc i2c: sh_mobile: remove redundant initialization
cac9723 arch: sh: migor: Use new CEU camera driver
...
------------------------------------------------------------

And I can successfully probe the camera sensor

------------------------------------------------------------
i2c i2c-0: master_xfer[0] W, addr=0x21, len=1
i2c i2c-0: master_xfer[1] R, addr=0x21, len=1
i2c i2c-0: master_xfer[0] W, addr=0x21, len=1
i2c i2c-0: master_xfer[1] R, addr=0x21, len=1
i2c i2c-0: master_xfer[0] W, addr=0x21, len=1
i2c i2c-0: master_xfer[1] R, addr=0x21, len=1
i2c i2c-0: master_xfer[0] W, addr=0x21, len=1
i2c i2c-0: master_xfer[1] R, addr=0x21, len=1
ov772x 0-0021: ov7725 Product ID 77:21 Manufacturer ID 7f:a2
------------------------------------------------------------

As well as set format on it without noticeable errors

------------------------------------------------------------
i2c i2c-0: master_xfer[0] W, addr=0x21, len=1
i2c i2c-0: master_xfer[1] R, addr=0x21, len=1
i2c i2c-0: master_xfer[0] W, addr=0x21, len=2
i2c i2c-0: master_xfer[0] W, addr=0x21, len=2
i2c i2c-0: master_xfer[0] W, addr=0x21, len=2
i2c i2c-0: master_xfer[0] W, addr=0x21, len=2
i2c i2c-0: master_xfer[0] W, addr=0x21, len=2
i2c i2c-0: master_xfer[0] W, addr=0x21, len=2
i2c i2c-0: master_xfer[0] W, addr=0x21, len=2
i2c i2c-0: master_xfer[0] W, addr=0x21, len=2
i2c i2c-0: master_xfer[0] W, addr=0x21, len=2
i2c i2c-0: master_xfer[0] W, addr=0x21, len=1
i2c i2c-0: master_xfer[1] R, addr=0x21, len=1
i2c i2c-0: master_xfer[0] W, addr=0x21, len=2
i2c i2c-0: master_xfer[0] W, addr=0x21, len=2
------------------------------------------------------------

I cannot try capture, as I currently have issues with mmap on SH4 :(

If you want me to run some specific tests, let me know.
Otherwise you can add my

Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Cheers!
   j
>
> Looking forward to other comments, as well...
>
>
> Wolfram Sang (5):
>   i2c: sh_mobile: remove redundant initialization
>   i2c: sh_mobile: remove redundant deinitialization
>   i2c: sh_mobile: manually "inline" two short functions
>   i2c: sh_mobile: use direct writes when accessing ICE bit
>   i2c: sh_mobile: shorten exit of xfer routine
>
>  drivers/i2c/busses/i2c-sh_mobile.c | 50 +++++++++-----------------------------
>  1 file changed, 12 insertions(+), 38 deletions(-)
>
> --
> 2.11.0
>

Re: [PATCH RFT 0/5] i2c: sh_mobile: refactor HW init/deinit

[Wolfram Sang]

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

On Thu, Nov 02, 2017 at 01:47:26PM +0100, Wolfram Sang wrote:
> When clearing the ICE bit, all registers fall back to their defaut value. That
> allows for some simplifications in the code.
> 
> Tested on a Renesas Lager board (R-Car H2) doing a bunch of consecutive
> commands. No spurious interrupts have been observed and the signals look
> exactly the same when visualized with sigrok.
> 
> According to the docs, the ICE bit behaviour is the same since the beginning of
> this driver (sh7722) for the migo-r board.
> 
> jacopo: can you please test this series while you work on migo-r anyhow? Thank
> you a ton for that!
> 
> Looking forward to other comments, as well...

Whole series applied to i2c/for-next!


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