| Message ID | 20231030155340.3468528-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 0a4cfed79e4f4498c5f9d76c7e149e51a32d5be5 |
| Headers | show |
| Series | [v1,1/1] pinctrl: tangier: Move default strength assignment to a switch-case | expand |
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
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>
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
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?
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.
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
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.
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.
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
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 --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;
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(-)