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

Message ID 20171102124731.10484-4-wsa+renesas@sang-engineering.com
State New
Headers show
Series
  • i2c: sh_mobile: refactor HW init/deinit
Related show

Commit Message

Wolfram Sang Nov. 2, 2017, 12:47 p.m.
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(-)

Comments

Geert Uytterhoeven Nov. 6, 2017, 8:35 a.m. | #1
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

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Wolfram Sang Nov. 6, 2017, 8:54 a.m. | #2
> > -       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!

Patch

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;