| Message ID | 20240828184018.3097386-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | pinctrl: intel: High impedance impl. and cleanups | expand |
+Cc: Heiner On Wed, Aug 28, 2024 at 09:38:33PM +0300, Andy Shevchenko wrote: > We would need a high impedance implementation for a quirk, so here it > is. While doing this series I also noticed a couple of opportunities > to clean up, hence two more patches (1st and 5th). Sorry it took a while to actually start implementing the quirk for your case. Here I'm asking for the following things: 1) what is the marketing name of the device you have problems with? (I believe it's available on the free market, correct?); 2) does it have any BIOS updates and, if it has, does it fix the issue? 3) can you apply patches 2,3,4,5 from this series (the first one is buggy and not needed for you) and replace the hack I mentioned earlier with ret = intel_gpio_set_high_impedance(pctrl, 3); if (ret) return ret; somewhere at the end of intel_pinctrl_probe()? Does it still work as expected?
On Wed, Aug 28, 2024 at 09:38:37PM +0300, Andy Shevchenko wrote: > Implement high impedance support for Intel pin control hardware. > It allows to set high impedance and check it. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pinctrl/intel/pinctrl-intel.c | 46 +++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c > index 3a135cfe435f..ae30969b2dee 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -78,6 +78,7 @@ > #define PADCFG0_GPIODIS_FULL 3 > #define PADCFG0_GPIORXDIS BIT(9) > #define PADCFG0_GPIOTXDIS BIT(8) > +#define PADCFG0_GPIODIS (BIT(9) | BIT(8)) > #define PADCFG0_GPIORXSTATE BIT(1) > #define PADCFG0_GPIOTXSTATE BIT(0) > > @@ -654,6 +655,23 @@ static int intel_config_get_pull(struct intel_pinctrl *pctrl, unsigned int pin, > return 0; > } > > +static int intel_config_get_high_impedance(struct intel_pinctrl *pctrl, unsigned int pin, > + enum pin_config_param param, u32 *arg) > +{ > + void __iomem *padcfg0; > + u32 value; > + > + padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0); > + > + scoped_guard(raw_spinlock_irqsave, &pctrl->lock) > + value = readl(padcfg0); > + > + if (__intel_gpio_get_direction(value) != PAD_CONNECT_NONE) > + return -EINVAL; > + > + return 0; > +} > + > static int intel_config_get_debounce(struct intel_pinctrl *pctrl, unsigned int pin, > enum pin_config_param param, u32 *arg) > { > @@ -697,6 +715,12 @@ static int intel_config_get(struct pinctrl_dev *pctldev, unsigned int pin, > return ret; > break; > > + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE: > + ret = intel_config_get_high_impedance(pctrl, pin, param, &arg); > + if (ret) > + return ret; > + break; > + > case PIN_CONFIG_INPUT_DEBOUNCE: > ret = intel_config_get_debounce(pctrl, pin, param, &arg); > if (ret) > @@ -795,6 +819,22 @@ static int intel_config_set_pull(struct intel_pinctrl *pctrl, unsigned int pin, > return 0; > } > > +static int intel_gpio_set_high_impedance(struct intel_pinctrl *pctrl, unsigned int pin) > +{ > + void __iomem *padcfg0; > + u32 value; > + > + padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0); > + > + guard(raw_spinlock_irqsave)(&pctrl->lock); > + > + value = readl(padcfg0); > + value = __intel_gpio_set_direction(value, false, false); > + writel(value, padcfg0); > + > + return 0; Why not make this return void? > +} > + > static int intel_config_set_debounce(struct intel_pinctrl *pctrl, > unsigned int pin, unsigned int debounce) > { > @@ -857,6 +897,12 @@ static int intel_config_set(struct pinctrl_dev *pctldev, unsigned int pin, > return ret; > break; > > + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE: > + ret = intel_gpio_set_high_impedance(pctrl, pin); > + if (ret) > + return ret; Then this becomes simpler too. > + break; > + > case PIN_CONFIG_INPUT_DEBOUNCE: > ret = intel_config_set_debounce(pctrl, pin, > pinconf_to_config_argument(configs[i])); > -- > 2.43.0.rc1.1336.g36b5255a03ac
On Wed, Aug 28, 2024 at 09:38:38PM +0300, Andy Shevchenko wrote: > Introduce a helper macro for_each_intel_gpio_group(). > With that in place, update users. > > It reduces the C code base as well as shrinks the binary: > > add/remove: 0/0 grow/shrink: 1/8 up/down: 37/-106 (-69) > Total: Before=15611, After=15542, chg -0.44% > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pinctrl/intel/pinctrl-intel.c | 89 +++++++++------------------ > 1 file changed, 29 insertions(+), 60 deletions(-) > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c > index ae30969b2dee..143174abda32 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -931,6 +931,15 @@ static const struct pinctrl_desc intel_pinctrl_desc = { > .owner = THIS_MODULE, > }; > > +#define for_each_intel_gpio_group(pctrl, community, grp) \ > + for (unsigned int __i = 0; \ > + __i < pctrl->ncommunities && (community = &pctrl->communities[__i]); \ > + __i++) \ > + for (unsigned int __j = 0; \ > + __j < community->ngpps && (grp = &community->gpps[__j]); \ > + __j++) \ > + if (grp->gpio_base == INTEL_GPIO_BASE_NOMAP) {} else > + This looks absolutely grotesque. I hope that you can debug this still after couple of months has passed because I cannot ;-) I wonder if there is a way to make it more readable by adding some sort of helpers? Or perhaps we don't need to make the whole thing as macro and just provide helpers we can use in the otherwise open-coded callers. > /** > * intel_gpio_to_pin() - Translate from GPIO offset to pin number > * @pctrl: Pinctrl structure > @@ -949,30 +958,17 @@ static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned int offset, > const struct intel_community **community, > const struct intel_padgroup **padgrp) > { > - int i; > + const struct intel_community *c; > + const struct intel_padgroup *gpp; > > - for (i = 0; i < pctrl->ncommunities; i++) { > - const struct intel_community *comm = &pctrl->communities[i]; > - int j; > + for_each_intel_gpio_group(pctrl, c, gpp) { > + if (offset >= gpp->gpio_base && offset < gpp->gpio_base + gpp->size) { > + if (community) > + *community = c; > + if (padgrp) > + *padgrp = gpp; > > - for (j = 0; j < comm->ngpps; j++) { > - const struct intel_padgroup *pgrp = &comm->gpps[j]; > - > - if (pgrp->gpio_base == INTEL_GPIO_BASE_NOMAP) > - continue; > - > - if (offset >= pgrp->gpio_base && > - offset < pgrp->gpio_base + pgrp->size) { > - int pin; > - > - pin = pgrp->base + offset - pgrp->gpio_base; > - if (community) > - *community = comm; > - if (padgrp) > - *padgrp = pgrp; > - > - return pin; > - } Because I think this open-coded one is still at least readable. Of course if there is duplication we should try to get rid of it but not in expense of readability IMHO. > + return gpp->base + offset - gpp->gpio_base; > } > } > > @@ -1348,36 +1344,17 @@ static int intel_gpio_irq_init_hw(struct gpio_chip *gc) > return 0; > } > > -static int intel_gpio_add_community_ranges(struct intel_pinctrl *pctrl, > - const struct intel_community *community) > -{ > - int ret = 0, i; > - > - for (i = 0; i < community->ngpps; i++) { > - const struct intel_padgroup *gpp = &community->gpps[i]; > - > - if (gpp->gpio_base == INTEL_GPIO_BASE_NOMAP) > - continue; > - > - ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), > - gpp->gpio_base, gpp->base, > - gpp->size); > - if (ret) > - return ret; > - } > - > - return ret; > -} > - > static int intel_gpio_add_pin_ranges(struct gpio_chip *gc) > { > struct intel_pinctrl *pctrl = gpiochip_get_data(gc); > - int ret, i; > + struct intel_community *community; > + const struct intel_padgroup *gpp; > + int ret; > > - for (i = 0; i < pctrl->ncommunities; i++) { > - struct intel_community *community = &pctrl->communities[i]; > - > - ret = intel_gpio_add_community_ranges(pctrl, community); > + for_each_intel_gpio_group(pctrl, community, gpp) { > + ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), > + gpp->gpio_base, gpp->base, > + gpp->size); > if (ret) { > dev_err(pctrl->dev, "failed to add GPIO pin range\n"); > return ret; > @@ -1390,20 +1367,12 @@ static int intel_gpio_add_pin_ranges(struct gpio_chip *gc) > static unsigned int intel_gpio_ngpio(const struct intel_pinctrl *pctrl) > { > const struct intel_community *community; > + const struct intel_padgroup *gpp; > unsigned int ngpio = 0; > - int i, j; > > - for (i = 0; i < pctrl->ncommunities; i++) { > - community = &pctrl->communities[i]; > - for (j = 0; j < community->ngpps; j++) { > - const struct intel_padgroup *gpp = &community->gpps[j]; > - > - if (gpp->gpio_base == INTEL_GPIO_BASE_NOMAP) > - continue; > - > - if (gpp->gpio_base + gpp->size > ngpio) > - ngpio = gpp->gpio_base + gpp->size; > - } > + for_each_intel_gpio_group(pctrl, community, gpp) { > + if (gpp->gpio_base + gpp->size > ngpio) > + ngpio = gpp->gpio_base + gpp->size; > } > > return ngpio; > -- > 2.43.0.rc1.1336.g36b5255a03ac
On Thu, Aug 29, 2024 at 7:53 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Wed, Aug 28, 2024 at 09:38:38PM +0300, Andy Shevchenko wrote: > > Introduce a helper macro for_each_intel_gpio_group(). > > With that in place, update users. > > > > It reduces the C code base as well as shrinks the binary: > > > > add/remove: 0/0 grow/shrink: 1/8 up/down: 37/-106 (-69) > > Total: Before=15611, After=15542, chg -0.44% ... > > +#define for_each_intel_gpio_group(pctrl, community, grp) \ > > + for (unsigned int __i = 0; \ > > + __i < pctrl->ncommunities && (community = &pctrl->communities[__i]); \ > > + __i++) \ > > + for (unsigned int __j = 0; \ > > + __j < community->ngpps && (grp = &community->gpps[__j]); \ > > + __j++) \ > > + if (grp->gpio_base == INTEL_GPIO_BASE_NOMAP) {} else > > + > > This looks absolutely grotesque. I hope that you can debug this still > after couple of months has passed because I cannot ;-) Yes, I can. > I wonder if there is a way to make it more readable by adding some sort > of helpers? Or perhaps we don't need to make the whole thing as macro > and just provide helpers we can use in the otherwise open-coded callers. Yes, I can split it into two for-loops. But note, each of them a quite standard how we define for_each macro with and without conditional, see the jernel full of them (PCI, GPIOLIB, i915, ...). ... > > - for (i = 0; i < pctrl->ncommunities; i++) { > > - const struct intel_community *comm = &pctrl->communities[i]; > > - int j; > > + for_each_intel_gpio_group(pctrl, c, gpp) { > > + if (offset >= gpp->gpio_base && offset < gpp->gpio_base + gpp->size) { > > + if (community) > > + *community = c; > > + if (padgrp) > > + *padgrp = gpp; > > > > - for (j = 0; j < comm->ngpps; j++) { > > - const struct intel_padgroup *pgrp = &comm->gpps[j]; > > - > > - if (pgrp->gpio_base == INTEL_GPIO_BASE_NOMAP) > > - continue; > > - > > - if (offset >= pgrp->gpio_base && > > - offset < pgrp->gpio_base + pgrp->size) { > > - int pin; > > - > > - pin = pgrp->base + offset - pgrp->gpio_base; > > - if (community) > > - *community = comm; > > - if (padgrp) > > - *padgrp = pgrp; > > - > > - return pin; > > - } > > Because I think this open-coded one is still at least readable. Of > course if there is duplication we should try to get rid of it but not in > expense of readability IMHO. The result I think is more readable as it's pretty clear from the macro name what is iterating over. It also hides unneeded detail, i.e. iterator variable. > > > + return gpp->base + offset - gpp->gpio_base; > > } > > }
On Thu, Aug 29, 2024 at 07:48:19AM +0300, Mika Westerberg wrote: > On Wed, Aug 28, 2024 at 09:38:37PM +0300, Andy Shevchenko wrote: ... > > +static int intel_gpio_set_high_impedance(struct intel_pinctrl *pctrl, unsigned int pin) > > +{ > > + void __iomem *padcfg0; > > + u32 value; > > + > > + padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0); > > + > > + guard(raw_spinlock_irqsave)(&pctrl->lock); > > + > > + value = readl(padcfg0); > > + value = __intel_gpio_set_direction(value, false, false); > > + writel(value, padcfg0); > > + > > + return 0; > > Why not make this return void? No special needs, can be void. > > +} ... Thank you for the review! I'll send a v2 soon.
On 28.08.2024 22:00, Andy Shevchenko wrote: > +Cc: Heiner > > On Wed, Aug 28, 2024 at 09:38:33PM +0300, Andy Shevchenko wrote: >> We would need a high impedance implementation for a quirk, so here it >> is. While doing this series I also noticed a couple of opportunities >> to clean up, hence two more patches (1st and 5th). > > Sorry it took a while to actually start implementing the quirk for your case. > Here I'm asking for the following things: > > 1) what is the marketing name of the device you have problems with? > (I believe it's available on the free market, correct?); > Device is a dirt-cheap mini pc, marketed as Chatreey T9. It's available on the free market, right. Dmesg says: DMI: Default string Default string/Default string, BIOS ADLN.M6.SODIMM.ZB.CY.015 08/08/2023 > 2) does it have any BIOS updates and, if it has, does it fix the issue? > No BIOS updates. > 3) can you apply patches 2,3,4,5 from this series (the first one is buggy and > not needed for you) and replace the hack I mentioned earlier with > > ret = intel_gpio_set_high_impedance(pctrl, 3); > if (ret) > return ret; > > somewhere at the end of intel_pinctrl_probe()? > > Does it still work as expected? > > I will check.
On Fri, Aug 30, 2024 at 07:56:11AM +0200, Heiner Kallweit wrote: > On 28.08.2024 22:00, Andy Shevchenko wrote: > > On Wed, Aug 28, 2024 at 09:38:33PM +0300, Andy Shevchenko wrote: > >> We would need a high impedance implementation for a quirk, so here it > >> is. While doing this series I also noticed a couple of opportunities > >> to clean up, hence two more patches (1st and 5th). > > > > Sorry it took a while to actually start implementing the quirk for your case. > > Here I'm asking for the following things: > > > > 1) what is the marketing name of the device you have problems with? > > (I believe it's available on the free market, correct?); > > Device is a dirt-cheap mini pc, marketed as Chatreey T9. It's available > on the free market, right. Dmesg says: > DMI: Default string Default string/Default string, BIOS ADLN.M6.SODIMM.ZB.CY.015 08/08/2023 > > > 2) does it have any BIOS updates and, if it has, does it fix the issue? > > > No BIOS updates. > > > 3) can you apply patches 2,3,4,5 from this series (the first one is buggy and > > not needed for you) and replace the hack I mentioned earlier with > > > > ret = intel_gpio_set_high_impedance(pctrl, 3); > > if (ret) > > return ret; > > > > somewhere at the end of intel_pinctrl_probe()? > > > > Does it still work as expected? > > > I will check. There is a v2 to test, you can take entire series, or for-next branch of Intel pin control tree https://git.kernel.org/pub/scm/linux/kernel/git/pinctrl/intel.git/log/?h=for-next Thanks!
On 28.08.2024 22:00, Andy Shevchenko wrote: > +Cc: Heiner > > On Wed, Aug 28, 2024 at 09:38:33PM +0300, Andy Shevchenko wrote: >> We would need a high impedance implementation for a quirk, so here it >> is. While doing this series I also noticed a couple of opportunities >> to clean up, hence two more patches (1st and 5th). > > Sorry it took a while to actually start implementing the quirk for your case. > Here I'm asking for the following things: > > 1) what is the marketing name of the device you have problems with? > (I believe it's available on the free market, correct?); > > 2) does it have any BIOS updates and, if it has, does it fix the issue? > > 3) can you apply patches 2,3,4,5 from this series (the first one is buggy and > not needed for you) and replace the hack I mentioned earlier with > > ret = intel_gpio_set_high_impedance(pctrl, 3); > if (ret) > return ret; > > somewhere at the end of intel_pinctrl_probe()? > > Does it still work as expected? > > In latest series the return value of intel_gpio_set_high_impedance() has been removed. With the call to intel_gpio_set_high_impedance() changed accordingly this fixes the problem on my system. Thanks a lot for your efforts!
On Fri, Aug 30, 2024 at 11:24:06PM +0200, Heiner Kallweit wrote: > On 28.08.2024 22:00, Andy Shevchenko wrote: > > On Wed, Aug 28, 2024 at 09:38:33PM +0300, Andy Shevchenko wrote: > >> We would need a high impedance implementation for a quirk, so here it > >> is. While doing this series I also noticed a couple of opportunities > >> to clean up, hence two more patches (1st and 5th). > > > > Sorry it took a while to actually start implementing the quirk for your case. > > Here I'm asking for the following things: > > > > 1) what is the marketing name of the device you have problems with? > > (I believe it's available on the free market, correct?); > > > > 2) does it have any BIOS updates and, if it has, does it fix the issue? > > > > 3) can you apply patches 2,3,4,5 from this series (the first one is buggy and > > not needed for you) and replace the hack I mentioned earlier with > > > > ret = intel_gpio_set_high_impedance(pctrl, 3); > > if (ret) > > return ret; > > > > somewhere at the end of intel_pinctrl_probe()? > > > > Does it still work as expected? > > > > > In latest series the return value of intel_gpio_set_high_impedance() > has been removed. With the call to intel_gpio_set_high_impedance() > changed accordingly this fixes the problem on my system. > Thanks a lot for your efforts! Thank you! I will think now, how to make the real quirk (which I may ask you to test in the future, when it will be ready) that looks and feels not as an ugly hack.
We would need a high impedance implementation for a quirk, so here it is. While doing this series I also noticed a couple of opportunities to clean up, hence two more patches (1st and 5th). Andy Shevchenko (5): pinctrl: intel: Move debounce validation out of the lock pinctrl: intel: Refactor __intel_gpio_set_direction() to be more useful pinctrl: intel: Add __intel_gpio_get_direction() helper pinctrl: intel: Implement high impedance support pinctrl: intel: Introduce for_each_intel_gpio_group() helper drivers/pinctrl/intel/pinctrl-intel.c | 239 ++++++++++++++++---------- 1 file changed, 150 insertions(+), 89 deletions(-)