Message ID | 20171102124731.10484-4-wsa+renesas@sang-engineering.com |
---|---|
State | Accepted |
Headers | show |
Series | i2c: sh_mobile: refactor HW init/deinit | expand |
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
> > - 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!
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;
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(-)