Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] gpio: omap: handle pin config bias flags
@ 2020-07-17 19:40 Drew Fustini
  2020-07-20 22:38 ` Gustavo A. R. Silva
  0 siblings, 1 reply; 4+ messages in thread
From: Drew Fustini @ 2020-07-17 19:40 UTC (permalink / raw)
  To: Tony Lindgren, Haojian Zhuang, Linus Walleij, Grygorii Strashko,
	linux-omap, linux-gpio, Jason Kridner, Robert Nelson
  Cc: Drew Fustini

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)
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] gpio: omap: handle pin config bias flags
  2020-07-17 19:40 [PATCH v3] gpio: omap: handle pin config bias flags Drew Fustini
@ 2020-07-20 22:38 ` Gustavo A. R. Silva
  2020-07-21 10:02   ` Drew Fustini
  0 siblings, 1 reply; 4+ messages in thread
From: Gustavo A. R. Silva @ 2020-07-20 22:38 UTC (permalink / raw)
  To: Drew Fustini, Tony Lindgren, Haojian Zhuang, Linus Walleij,
	Grygorii Strashko, linux-omap, linux-gpio, Jason Kridner,
	Robert Nelson

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] gpio: omap: handle pin config bias flags
  2020-07-20 22:38 ` Gustavo A. R. Silva
@ 2020-07-21 10:02   ` Drew Fustini
  2020-07-21 22:07     ` Linus Walleij
  0 siblings, 1 reply; 4+ messages in thread
From: Drew Fustini @ 2020-07-21 10:02 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Linus Walleij
  Cc: Tony Lindgren, Haojian Zhuang, Grygorii Strashko, linux-omap,
	linux-gpio, Jason Kridner, Robert Nelson

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] gpio: omap: handle pin config bias flags
  2020-07-21 10:02   ` Drew Fustini
@ 2020-07-21 22:07     ` Linus Walleij
  0 siblings, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2020-07-21 22:07 UTC (permalink / raw)
  To: Drew Fustini
  Cc: Gustavo A. R. Silva, Tony Lindgren, Haojian Zhuang,
	Grygorii Strashko, Linux-OMAP, open list:GPIO SUBSYSTEM,
	Jason Kridner, Robert Nelson

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17 19:40 [PATCH v3] gpio: omap: handle pin config bias flags Drew Fustini
2020-07-20 22:38 ` Gustavo A. R. Silva
2020-07-21 10:02   ` Drew Fustini
2020-07-21 22:07     ` Linus Walleij

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git