diff mbox series

[v1,1/1] pinctrl: tangier: Move default strength assignment to a switch-case

Message ID 20231030155340.3468528-1-andriy.shevchenko@linux.intel.com
State New
Headers
Series [v1,1/1] pinctrl: tangier: Move default strength assignment to a switch-case |

Commit Message

Andy Shevchenko Oct. 30, 2023, 3:53 p.m. UTC 
  iWhen ->pin_config_set() is called from the GPIO library (assumed
GpioIo() ACPI resource), the argument can be 1, when, for example,
PullDefault is provided. In such case we supply sane default in
the driver. Move that default assingment to a switch-case, so
it will be consolidated in one place.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-tangier.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Comments

Raag Jadav Oct. 30, 2023, 9:14 p.m. UTC | #1 
On Mon, Oct 30, 2023 at 05:53:40PM +0200, Andy Shevchenko wrote:
> iWhen ->pin_config_set() is called from the GPIO library (assumed
> GpioIo() ACPI resource), the argument can be 1, when, for example,
> PullDefault is provided. In such case we supply sane default in
> the driver. Move that default assingment to a switch-case, so
> it will be consolidated in one place.

Looks good.

iWhen -> When

Raag
Andy Shevchenko Oct. 31, 2023, 10:22 a.m. UTC | #2 
On Mon, Oct 30, 2023 at 11:14:11PM +0200, Raag Jadav wrote:
> On Mon, Oct 30, 2023 at 05:53:40PM +0200, Andy Shevchenko wrote:
> > iWhen ->pin_config_set() is called from the GPIO library (assumed
> > GpioIo() ACPI resource), the argument can be 1, when, for example,
> > PullDefault is provided. In such case we supply sane default in
> > the driver. Move that default assingment to a switch-case, so
> > it will be consolidated in one place.
> 
> Looks good.

Thank you for review. Can you give your Rb tag then?

> iWhen -> When

I'll fix it locally.
Raag Jadav Oct. 31, 2023, 1:57 p.m. UTC | #3 
On Tue, Oct 31, 2023 at 12:22:47PM +0200, Andy Shevchenko wrote:
> On Mon, Oct 30, 2023 at 11:14:11PM +0200, Raag Jadav wrote:
> > On Mon, Oct 30, 2023 at 05:53:40PM +0200, Andy Shevchenko wrote:
> > > iWhen ->pin_config_set() is called from the GPIO library (assumed
> > > GpioIo() ACPI resource), the argument can be 1, when, for example,
> > > PullDefault is provided. In such case we supply sane default in
> > > the driver. Move that default assingment to a switch-case, so
> > > it will be consolidated in one place.
> > 
> > Looks good.
> 
> Thank you for review. Can you give your Rb tag then?

Since I'm not a maintainer, I'm not sure if I qualify.
But anyway, here you go.

Reviewed-by: Raag Jadav <raag.jadav@intel.com>
Mika Westerberg Nov. 1, 2023, 6:35 a.m. UTC | #4 
On Mon, Oct 30, 2023 at 05:53:40PM +0200, Andy Shevchenko wrote:
> iWhen ->pin_config_set() is called from the GPIO library (assumed
> GpioIo() ACPI resource), the argument can be 1, when, for example,
> PullDefault is provided. In such case we supply sane default in
> the driver. Move that default assingment to a switch-case, so
> it will be consolidated in one place.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-tangier.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-tangier.c b/drivers/pinctrl/intel/pinctrl-tangier.c
> index 007bca1cf224..26e34ec0a972 100644
> --- a/drivers/pinctrl/intel/pinctrl-tangier.c
> +++ b/drivers/pinctrl/intel/pinctrl-tangier.c
> @@ -368,14 +368,11 @@ static int tng_config_set_pin(struct tng_pinctrl *tp, unsigned int pin,
>  		break;
>  
>  	case PIN_CONFIG_BIAS_PULL_UP:
> -		/* Set default strength value in case none is given */
> -		if (arg == 1)
> -			arg = 20000;
> -
>  		switch (arg) {
>  		case 50000:
>  			term = BUFCFG_PUPD_VAL_50K;
>  			break;
> +		case 1: /* Set default strength value in case none is given */

The comment is good but I think would it make sense to have constant for
this instead?

>  		case 20000:
>  			term = BUFCFG_PUPD_VAL_20K;
>  			break;
> @@ -394,14 +391,11 @@ static int tng_config_set_pin(struct tng_pinctrl *tp, unsigned int pin,
>  		break;
>  
>  	case PIN_CONFIG_BIAS_PULL_DOWN:
> -		/* Set default strength value in case none is given */
> -		if (arg == 1)
> -			arg = 20000;
> -
>  		switch (arg) {
>  		case 50000:
>  			term = BUFCFG_PUPD_VAL_50K;
>  			break;
> +		case 1: /* Set default strength value in case none is given */
>  		case 20000:
>  			term = BUFCFG_PUPD_VAL_20K;
>  			break;
> -- 
> 2.40.0.1.gaa8946217a0b
Andy Shevchenko Nov. 1, 2023, 9:34 a.m. UTC | #5 
On Wed, Nov 01, 2023 at 08:35:20AM +0200, Mika Westerberg wrote:
> On Mon, Oct 30, 2023 at 05:53:40PM +0200, Andy Shevchenko wrote:

...

> > +		case 1: /* Set default strength value in case none is given */
> 
> The comment is good but I think would it make sense to have constant for
> this instead?

If so, it makes sense to get it via entire GPIO library, and not locally for
Intel stuff. That said, I prefer to do that separately. Do you agree?
Mika Westerberg Nov. 1, 2023, 10:44 a.m. UTC | #6 
On Wed, Nov 01, 2023 at 11:34:58AM +0200, Andy Shevchenko wrote:
> On Wed, Nov 01, 2023 at 08:35:20AM +0200, Mika Westerberg wrote:
> > On Mon, Oct 30, 2023 at 05:53:40PM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > > +		case 1: /* Set default strength value in case none is given */
> > 
> > The comment is good but I think would it make sense to have constant for
> > this instead?
> 
> If so, it makes sense to get it via entire GPIO library, and not locally for
> Intel stuff. That said, I prefer to do that separately. Do you agree?

Yes, agree.
Linus Walleij Nov. 2, 2023, 7:36 a.m. UTC | #7 
On Mon, Oct 30, 2023 at 4:54 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> iWhen ->pin_config_set() is called from the GPIO library (assumed
> GpioIo() ACPI resource), the argument can be 1, when, for example,
> PullDefault is provided. In such case we supply sane default in
> the driver. Move that default assingment to a switch-case, so
> it will be consolidated in one place.
(...)
> +               case 1: /* Set default strength value in case none is given */

So where does this 1 come from in the end? That's the piece I
am missing in this explanation. Somewhere, someone decided
to pass 1 to indicate "pull to default resistance".

Is it coming from ACPI firmware?

Then a comment such as "the firmware author chose to pass 1
for default pull" should be added to the constant definition in the
code.

Other than that it looks good!

Yours,
Linus Walleij
Andy Shevchenko Nov. 2, 2023, 12:34 p.m. UTC | #8 
On Thu, Nov 02, 2023 at 08:36:11AM +0100, Linus Walleij wrote:
> On Mon, Oct 30, 2023 at 4:54 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

(...)

> > +               case 1: /* Set default strength value in case none is given */
> 
> So where does this 1 come from in the end? That's the piece I
> am missing in this explanation. Somewhere, someone decided
> to pass 1 to indicate "pull to default resistance".
> 
> Is it coming from ACPI firmware?

No, it's pure Linux kernel decision.
gpio_set_bias() is who made that. That's why it needs to be chosen on global
level.

We may even document somewhere that arguments let's say up to 10 do not make
any sense in real life, as even for 1.2 v it will give 120 mA current on a single
pin. Yet, theoretically that's possible for discrete industrial GPIOs, so we
can choose "very big number" if such case appears in the future. I don't want
to change 1 to something else right now as it may break things.

> for default pull" should be added to the constant definition in the
> code.
Andy Shevchenko Nov. 2, 2023, 12:37 p.m. UTC | #9 
On Thu, Nov 02, 2023 at 02:34:30PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 02, 2023 at 08:36:11AM +0100, Linus Walleij wrote:
> > On Mon, Oct 30, 2023 at 4:54 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:

(...)

> > > +               case 1: /* Set default strength value in case none is given */
> > 
> > So where does this 1 come from in the end? That's the piece I
> > am missing in this explanation. Somewhere, someone decided
> > to pass 1 to indicate "pull to default resistance".
> > 
> > Is it coming from ACPI firmware?
> 
> No, it's pure Linux kernel decision.
> gpio_set_bias() is who made that. That's why it needs to be chosen on global
> level.
> 
> We may even document somewhere that arguments let's say up to 10 do not make
> any sense in real life, as even for 1.2 v it will give 120 mA current on a single
> pin. Yet, theoretically that's possible for discrete industrial GPIOs, so we
> can choose "very big number" if such case appears in the future. I don't want

Just realized that "very big number" is limited to 16-bit value right now and
65 kOhm is quite reasonable value for the pull bias (yet we can use exact
0xffff for the "special" case).

> to change 1 to something else right now as it may break things.
> 
> > for default pull" should be added to the constant definition in the
> > code.
Linus Walleij Nov. 2, 2023, 12:56 p.m. UTC | #10 
On Thu, Nov 2, 2023 at 1:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, Nov 02, 2023 at 08:36:11AM +0100, Linus Walleij wrote:

> > So where does this 1 come from in the end? That's the piece I
> > am missing in this explanation. Somewhere, someone decided
> > to pass 1 to indicate "pull to default resistance".
> >
> > Is it coming from ACPI firmware?
>
> No, it's pure Linux kernel decision.
> gpio_set_bias() is who made that. That's why it needs to be chosen on global
> level.

Aha I see, that makes sense.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Andy Shevchenko Nov. 13, 2023, 11:53 a.m. UTC | #11 
On Thu, Nov 02, 2023 at 01:56:08PM +0100, Linus Walleij wrote:
> On Thu, Nov 2, 2023 at 1:34 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Nov 02, 2023 at 08:36:11AM +0100, Linus Walleij wrote:
> 
> > > So where does this 1 come from in the end? That's the piece I
> > > am missing in this explanation. Somewhere, someone decided
> > > to pass 1 to indicate "pull to default resistance".
> > >
> > > Is it coming from ACPI firmware?
> >
> > No, it's pure Linux kernel decision.
> > gpio_set_bias() is who made that. That's why it needs to be chosen on global
> > level.
> 
> Aha I see, that makes sense.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Pushed to my review and testing queue, thanks!
diff mbox series

Patch

diff --git a/drivers/pinctrl/intel/pinctrl-tangier.c b/drivers/pinctrl/intel/pinctrl-tangier.c
index 007bca1cf224..26e34ec0a972 100644
--- a/drivers/pinctrl/intel/pinctrl-tangier.c
+++ b/drivers/pinctrl/intel/pinctrl-tangier.c
@@ -368,14 +368,11 @@  static int tng_config_set_pin(struct tng_pinctrl *tp, unsigned int pin,
 		break;
 
 	case PIN_CONFIG_BIAS_PULL_UP:
-		/* Set default strength value in case none is given */
-		if (arg == 1)
-			arg = 20000;
-
 		switch (arg) {
 		case 50000:
 			term = BUFCFG_PUPD_VAL_50K;
 			break;
+		case 1: /* Set default strength value in case none is given */
 		case 20000:
 			term = BUFCFG_PUPD_VAL_20K;
 			break;
@@ -394,14 +391,11 @@  static int tng_config_set_pin(struct tng_pinctrl *tp, unsigned int pin,
 		break;
 
 	case PIN_CONFIG_BIAS_PULL_DOWN:
-		/* Set default strength value in case none is given */
-		if (arg == 1)
-			arg = 20000;
-
 		switch (arg) {
 		case 50000:
 			term = BUFCFG_PUPD_VAL_50K;
 			break;
+		case 1: /* Set default strength value in case none is given */
 		case 20000:
 			term = BUFCFG_PUPD_VAL_20K;
 			break;