| Message ID | 1435650327-2542-8-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Geert, Thank you for the patch. On Tuesday 30 June 2015 09:45:27 Geert Uytterhoeven wrote: > Legacy function GPIOs are no longer used on ARM since commit > a27c5cd1a08cc95c ("sh-pfc: sh73a0: Remove function GPIOs"). > Extract its setup code into a separate function, and make all function > GPIO related code and data depend on CONFIG_SUPERH. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Compile-tested on sh using se7724_defconfig. > --- > drivers/pinctrl/sh-pfc/core.h | 2 ++ > drivers/pinctrl/sh-pfc/gpio.c | 39 ++++++++++++++++++++++++++---------- > drivers/pinctrl/sh-pfc/sh_pfc.h | 2 ++ > 3 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h > index 4c3c37bf7161804d..c38ace46d7111b0d 100644 > --- a/drivers/pinctrl/sh-pfc/core.h > +++ b/drivers/pinctrl/sh-pfc/core.h > @@ -46,7 +46,9 @@ struct sh_pfc { > unsigned int nr_gpio_pins; > > struct sh_pfc_chip *gpio; > +#ifdef CONFIG_SUPERH > struct sh_pfc_chip *func; > +#endif > > struct sh_pfc_pinctrl *pinctrl; > }; > diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c > index 30654e800580524d..15f4eea186e2ba56 100644 > --- a/drivers/pinctrl/sh-pfc/gpio.c > +++ b/drivers/pinctrl/sh-pfc/gpio.c > @@ -290,6 +290,7 @@ static int gpio_pin_setup(struct sh_pfc_chip *chip) > * Function GPIOs > */ > > +#ifdef CONFIG_SUPERH > static int gpio_function_request(struct gpio_chip *gc, unsigned offset) > { > static bool __print_once; > @@ -330,6 +331,31 @@ static int gpio_function_setup(struct sh_pfc_chip > *chip) return 0; > } > > +static int sh_pfc_add_function_gpios(struct sh_pfc *pfc) > +{ > + struct sh_pfc_chip *chip; > + > + if (!pfc->info->nr_func_gpios) > + return 0; > + > + chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup, NULL); > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + > + pfc->func = chip; > + > + return 0; > +} > + > +static void sh_pfc_remove_function_gpios(struct sh_pfc *pfc) > +{ > + gpiochip_remove(&pfc->func->gpio_chip); > +} > +#else > +static inline int sh_pfc_add_function_gpios(struct sh_pfc *pfc) { return 0; > } > +static inline int sh_pfc_remove_function_gpios(struct sh_pfc *pfc) { > return 0; } Given that those functions are only called from a single location, would it make sense to just compile the calls conditionally instead ? I would find that slightly more readable as it would be more explicit. > +#endif > + > /* > --------------------------------------------------------------------------- > -- * Register/unregister > */ > @@ -397,22 +423,13 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc) > } > > /* Register the function GPIOs chip. */ > - if (pfc->info->nr_func_gpios == 0) > - return 0; > - > - chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup, NULL); > - if (IS_ERR(chip)) > - return PTR_ERR(chip); > - > - pfc->func = chip; > - > - return 0; > + return sh_pfc_add_function_gpios(pfc); > } > > int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc) > { > gpiochip_remove(&pfc->gpio->gpio_chip); > - gpiochip_remove(&pfc->func->gpio_chip); > + sh_pfc_remove_function_gpios(pfc); > > return 0; > } > diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h > b/drivers/pinctrl/sh-pfc/sh_pfc.h index c7508d5f688613b2..c6f9163b3e23041f > 100644 > --- a/drivers/pinctrl/sh-pfc/sh_pfc.h > +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h > @@ -138,8 +138,10 @@ struct sh_pfc_soc_info { > const struct sh_pfc_function *functions; > unsigned int nr_functions; > > +#ifdef CONFIG_SUPERH > const struct pinmux_func *func_gpios; > unsigned int nr_func_gpios; > +#endif > > const struct pinmux_cfg_reg *cfg_regs; > const struct pinmux_data_reg *data_regs;
Hi Laurent, On Tue, Jun 30, 2015 at 11:38 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> --- a/drivers/pinctrl/sh-pfc/gpio.c >> +++ b/drivers/pinctrl/sh-pfc/gpio.c >> @@ -290,6 +290,7 @@ static int gpio_pin_setup(struct sh_pfc_chip *chip) >> * Function GPIOs >> */ >> >> +#ifdef CONFIG_SUPERH >> static int gpio_function_request(struct gpio_chip *gc, unsigned offset) >> { >> static bool __print_once; >> @@ -330,6 +331,31 @@ static int gpio_function_setup(struct sh_pfc_chip >> *chip) return 0; >> } >> >> +static int sh_pfc_add_function_gpios(struct sh_pfc *pfc) >> +{ >> + struct sh_pfc_chip *chip; >> + >> + if (!pfc->info->nr_func_gpios) >> + return 0; >> + >> + chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup, NULL); >> + if (IS_ERR(chip)) >> + return PTR_ERR(chip); >> + >> + pfc->func = chip; >> + >> + return 0; >> +} >> + >> +static void sh_pfc_remove_function_gpios(struct sh_pfc *pfc) >> +{ >> + gpiochip_remove(&pfc->func->gpio_chip); >> +} >> +#else >> +static inline int sh_pfc_add_function_gpios(struct sh_pfc *pfc) { return 0; >> } >> +static inline int sh_pfc_remove_function_gpios(struct sh_pfc *pfc) { >> return 0; } > > Given that those functions are only called from a single location, would it > make sense to just compile the calls conditionally instead ? I would find that > slightly more readable as it would be more explicit. If that's acceptable for you, that would be fine for me, too. One (or two) more visual cues for future removal of legacy code. And then I can drop "[PATCH 6/7] pinctrl: sh-pfc: Move sh_pfc_add_gpiochip() up". 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h index 4c3c37bf7161804d..c38ace46d7111b0d 100644 --- a/drivers/pinctrl/sh-pfc/core.h +++ b/drivers/pinctrl/sh-pfc/core.h @@ -46,7 +46,9 @@ struct sh_pfc { unsigned int nr_gpio_pins; struct sh_pfc_chip *gpio; +#ifdef CONFIG_SUPERH struct sh_pfc_chip *func; +#endif struct sh_pfc_pinctrl *pinctrl; }; diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c index 30654e800580524d..15f4eea186e2ba56 100644 --- a/drivers/pinctrl/sh-pfc/gpio.c +++ b/drivers/pinctrl/sh-pfc/gpio.c @@ -290,6 +290,7 @@ static int gpio_pin_setup(struct sh_pfc_chip *chip) * Function GPIOs */ +#ifdef CONFIG_SUPERH static int gpio_function_request(struct gpio_chip *gc, unsigned offset) { static bool __print_once; @@ -330,6 +331,31 @@ static int gpio_function_setup(struct sh_pfc_chip *chip) return 0; } +static int sh_pfc_add_function_gpios(struct sh_pfc *pfc) +{ + struct sh_pfc_chip *chip; + + if (!pfc->info->nr_func_gpios) + return 0; + + chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup, NULL); + if (IS_ERR(chip)) + return PTR_ERR(chip); + + pfc->func = chip; + + return 0; +} + +static void sh_pfc_remove_function_gpios(struct sh_pfc *pfc) +{ + gpiochip_remove(&pfc->func->gpio_chip); +} +#else +static inline int sh_pfc_add_function_gpios(struct sh_pfc *pfc) { return 0; } +static inline int sh_pfc_remove_function_gpios(struct sh_pfc *pfc) { return 0; } +#endif + /* ----------------------------------------------------------------------------- * Register/unregister */ @@ -397,22 +423,13 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc) } /* Register the function GPIOs chip. */ - if (pfc->info->nr_func_gpios == 0) - return 0; - - chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup, NULL); - if (IS_ERR(chip)) - return PTR_ERR(chip); - - pfc->func = chip; - - return 0; + return sh_pfc_add_function_gpios(pfc); } int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc) { gpiochip_remove(&pfc->gpio->gpio_chip); - gpiochip_remove(&pfc->func->gpio_chip); + sh_pfc_remove_function_gpios(pfc); return 0; } diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h index c7508d5f688613b2..c6f9163b3e23041f 100644 --- a/drivers/pinctrl/sh-pfc/sh_pfc.h +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h @@ -138,8 +138,10 @@ struct sh_pfc_soc_info { const struct sh_pfc_function *functions; unsigned int nr_functions; +#ifdef CONFIG_SUPERH const struct pinmux_func *func_gpios; unsigned int nr_func_gpios; +#endif const struct pinmux_cfg_reg *cfg_regs; const struct pinmux_data_reg *data_regs;
Legacy function GPIOs are no longer used on ARM since commit a27c5cd1a08cc95c ("sh-pfc: sh73a0: Remove function GPIOs"). Extract its setup code into a separate function, and make all function GPIO related code and data depend on CONFIG_SUPERH. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Compile-tested on sh using se7724_defconfig. --- drivers/pinctrl/sh-pfc/core.h | 2 ++ drivers/pinctrl/sh-pfc/gpio.c | 39 ++++++++++++++++++++++++++++----------- drivers/pinctrl/sh-pfc/sh_pfc.h | 2 ++ 3 files changed, 32 insertions(+), 11 deletions(-)