| Message ID | 20200717194043.1774643-1-drew@beagleboard.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] gpio: omap: handle pin config bias flags | expand |
Hi Drew, Somehow I ran into this patch in Linus' tree: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=for-next&id=75dec56710dfafd37daa95e756c5d1840932ba90 Please, see some comments below... On 7/17/20 14:40, Drew Fustini wrote: > Modify omap_gpio_set_config() to handle pin config bias flags by calling > gpiochip_generic_config(). > > The pin group for the gpio line must have the corresponding pinconf > properties: > > PIN_CONFIG_BIAS_PULL_UP requires "pinctrl-single,bias-pullup" > PIN_CONFIG_BIAS_PULL_DOWN requires "pinctrl-single,bias-pulldown" > > This is necessary for pcs_pinconf_set() to find the requested bias > parameter in the PIN_MAP_TYPE_CONFIGS_GROUP pinctrl map. > > Signed-off-by: Drew Fustini <drew@beagleboard.org> > Acked-by: Grygorii Strashko <grygorii.strashko@ti.com> > Acked-by: Tony Lindgren <tony@atomide.com> > Link: https://lore.kernel.org/r/20200715213738.1640030-1-drew@beagleboard.org > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/gpio/gpio-omap.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > v3 changes: > - adjust the braces to match the correct coding style > - note: I originally re-submitted this as v2 by accident when it should > have been v3. Sorry for the noise. > > v2 changes: > - simplify handling of -ENOTSUPP return value per Grygorii's suggestion > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index b8e2ecc3eade..0ccb31de0b67 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -896,12 +896,18 @@ static int omap_gpio_set_config(struct gpio_chip *chip, unsigned offset, > unsigned long config) > { > u32 debounce; > + int ret = -ENOTSUPP; > + > + if ((pinconf_to_config_param(config) == PIN_CONFIG_BIAS_DISABLE) || > + (pinconf_to_config_param(config) == PIN_CONFIG_BIAS_PULL_UP) || > + (pinconf_to_config_param(config) == PIN_CONFIG_BIAS_PULL_DOWN)) { > + ret = gpiochip_generic_config(chip, offset, config); > + } else if (pinconf_to_config_param(config) == PIN_CONFIG_INPUT_DEBOUNCE) { > + debounce = pinconf_to_config_argument(config); > + ret = omap_gpio_debounce(chip, offset, debounce); > + } > > - if (pinconf_to_config_param(config) != PIN_CONFIG_INPUT_DEBOUNCE) > - return -ENOTSUPP; > - > - debounce = pinconf_to_config_argument(config); > - return omap_gpio_debounce(chip, offset, debounce); > + return ret; > } > > static void omap_gpio_set(struct gpio_chip *chip, unsigned offset, int value) > Maybe next time you could consider coding something like this, instead: diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 8dd86b9fae53..7fbe0c9e1fc1 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -899,16 +899,18 @@ static int omap_gpio_set_config(struct gpio_chip *chip, unsigned offset, u32 debounce; int ret = -ENOTSUPP; - if ((pinconf_to_config_param(config) == PIN_CONFIG_BIAS_DISABLE) || - (pinconf_to_config_param(config) == PIN_CONFIG_BIAS_PULL_UP) || - (pinconf_to_config_param(config) == PIN_CONFIG_BIAS_PULL_DOWN)) - { + switch (pinconf_to_config_param(config)) { + case PIN_CONFIG_BIAS_DISABLE: + case PIN_CONFIG_BIAS_PULL_UP: + case PIN_CONFIG_BIAS_PULL_DOWN: ret = gpiochip_generic_config(chip, offset, config); - } - else if (pinconf_to_config_param(config) == PIN_CONFIG_INPUT_DEBOUNCE) - { + break; + case PIN_CONFIG_INPUT_DEBOUNCE: debounce = pinconf_to_config_argument(config); ret = omap_gpio_debounce(chip, offset, debounce); + break; + default: + break; } return ret; It looks a bit more readable and cleaner. :) Thanks -- Gustavo
On Mon, Jul 20, 2020 at 05:38:51PM -0500, Gustavo A. R. Silva wrote: > Hi Drew, > > Somehow I ran into this patch in Linus' tree: > > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=for-next&id=75dec56710dfafd37daa95e756c5d1840932ba90 > > Please, see some comments below... > > On 7/17/20 14:40, Drew Fustini wrote: > > Modify omap_gpio_set_config() to handle pin config bias flags by calling > > gpiochip_generic_config(). > > > > The pin group for the gpio line must have the corresponding pinconf > > properties: > > > > PIN_CONFIG_BIAS_PULL_UP requires "pinctrl-single,bias-pullup" > > PIN_CONFIG_BIAS_PULL_DOWN requires "pinctrl-single,bias-pulldown" > > > > This is necessary for pcs_pinconf_set() to find the requested bias > > parameter in the PIN_MAP_TYPE_CONFIGS_GROUP pinctrl map. > > > > Signed-off-by: Drew Fustini <drew@beagleboard.org> > > Acked-by: Grygorii Strashko <grygorii.strashko@ti.com> > > Acked-by: Tony Lindgren <tony@atomide.com> > > Link: https://lore.kernel.org/r/20200715213738.1640030-1-drew@beagleboard.org > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > --- > > drivers/gpio/gpio-omap.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > v3 changes: > > - adjust the braces to match the correct coding style > > - note: I originally re-submitted this as v2 by accident when it should > > have been v3. Sorry for the noise. > > > > v2 changes: > > - simplify handling of -ENOTSUPP return value per Grygorii's suggestion > > > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > > index b8e2ecc3eade..0ccb31de0b67 100644 > > --- a/drivers/gpio/gpio-omap.c > > +++ b/drivers/gpio/gpio-omap.c > > @@ -896,12 +896,18 @@ static int omap_gpio_set_config(struct gpio_chip *chip, unsigned offset, > > unsigned long config) > > { > > u32 debounce; > > + int ret = -ENOTSUPP; > > + > > + if ((pinconf_to_config_param(config) == PIN_CONFIG_BIAS_DISABLE) || > > + (pinconf_to_config_param(config) == PIN_CONFIG_BIAS_PULL_UP) || > > + (pinconf_to_config_param(config) == PIN_CONFIG_BIAS_PULL_DOWN)) { > > + ret = gpiochip_generic_config(chip, offset, config); > > + } else if (pinconf_to_config_param(config) == PIN_CONFIG_INPUT_DEBOUNCE) { > > + debounce = pinconf_to_config_argument(config); > > + ret = omap_gpio_debounce(chip, offset, debounce); > > + } > > > > - if (pinconf_to_config_param(config) != PIN_CONFIG_INPUT_DEBOUNCE) > > - return -ENOTSUPP; > > - > > - debounce = pinconf_to_config_argument(config); > > - return omap_gpio_debounce(chip, offset, debounce); > > + return ret; > > } > > > > static void omap_gpio_set(struct gpio_chip *chip, unsigned offset, int value) > > > > Maybe next time you could consider coding something like this, instead: > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 8dd86b9fae53..7fbe0c9e1fc1 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -899,16 +899,18 @@ static int omap_gpio_set_config(struct gpio_chip *chip, unsigned offset, > u32 debounce; > int ret = -ENOTSUPP; > > - if ((pinconf_to_config_param(config) == PIN_CONFIG_BIAS_DISABLE) || > - (pinconf_to_config_param(config) == PIN_CONFIG_BIAS_PULL_UP) || > - (pinconf_to_config_param(config) == PIN_CONFIG_BIAS_PULL_DOWN)) > - { > + switch (pinconf_to_config_param(config)) { > + case PIN_CONFIG_BIAS_DISABLE: > + case PIN_CONFIG_BIAS_PULL_UP: > + case PIN_CONFIG_BIAS_PULL_DOWN: > ret = gpiochip_generic_config(chip, offset, config); > - } > - else if (pinconf_to_config_param(config) == PIN_CONFIG_INPUT_DEBOUNCE) > - { > + break; > + case PIN_CONFIG_INPUT_DEBOUNCE: > debounce = pinconf_to_config_argument(config); > ret = omap_gpio_debounce(chip, offset, debounce); > + break; > + default: > + break; > } > > return ret; > > It looks a bit more readable and cleaner. :) > > Thanks > -- > Gustavo Gustavo - thanks very much for the feedback. I appreciate getting these insights into best practices. Linus - should I submit a patch? I'm not sure if it is better to limit churn, or make sure the code is structured as best is possible. Thanks, Drew
On Tue, Jul 21, 2020 at 12:02 PM Drew Fustini <drew@beagleboard.org> wrote:
> Linus - should I submit a patch?
Yeps, normally always submit a patch to fix something that is applied
already.
Yours,
Linus Walleij
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index b8e2ecc3eade..0ccb31de0b67 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -896,12 +896,18 @@ static int omap_gpio_set_config(struct gpio_chip *chip, unsigned offset, unsigned long config) { u32 debounce; + int ret = -ENOTSUPP; + + if ((pinconf_to_config_param(config) == PIN_CONFIG_BIAS_DISABLE) || + (pinconf_to_config_param(config) == PIN_CONFIG_BIAS_PULL_UP) || + (pinconf_to_config_param(config) == PIN_CONFIG_BIAS_PULL_DOWN)) { + ret = gpiochip_generic_config(chip, offset, config); + } else if (pinconf_to_config_param(config) == PIN_CONFIG_INPUT_DEBOUNCE) { + debounce = pinconf_to_config_argument(config); + ret = omap_gpio_debounce(chip, offset, debounce); + } - if (pinconf_to_config_param(config) != PIN_CONFIG_INPUT_DEBOUNCE) - return -ENOTSUPP; - - debounce = pinconf_to_config_argument(config); - return omap_gpio_debounce(chip, offset, debounce); + return ret; } static void omap_gpio_set(struct gpio_chip *chip, unsigned offset, int value)