| Message ID | 20201109205332.19592-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | gpiolib: acpi: pin configuration fixes | expand |
On Tue, Nov 10, 2020 at 4:10 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/9/20 9:53 PM, Andy Shevchenko wrote: > > There are fixes (and plenty cleanups) that allow to take into consideration > > more parameters in ACPI, i.e. bias for GpioInt() and debounce timeout > > for Operation Regions, Events and GpioInt() resources. > > > > During review Hans noted, that gpiod_set_debounce() returns -ENOTSUPP for > > the cases when feature is not supported either by driver or a controller. > > > > It appears that we have slightly messy API here: > > > > FUNC Relation with ENOTSUPP > > > > gpiod_set_config() returns if not supported > > gpiod_set_debounce() as gpiod_set_config() above > > gpio_set_debounce() legacy wrapper on top of gpiod_set_debounce() > > gpiod_set_transitory() skips it (returns okay) with debug message > > gpio_set_config() returns if not supported > > gpio_set_bias() skips it (returns okay) > > > > Last two functions are internal to GPIO library, while the rest is > > exported API. In order to be consistent with both naming schemas > > the series introduces gpio_set_debounce_timeout() that considers > > the feature optional. New API is only for internal use. > > > > While at it, the few first patches do clean up the current GPIO library > > code to unify it to some extend. > > > > The above is followed by changes made in ACPI GPIO library part. > > > > The bias patch highly depends on Intel pin control driver changes > > (they are material for v5.10 [1]), due to this and amount of the > > prerequisite changes this series is probably not supposed to be > > backported (at least right now). > > The entire series looks good to me, feel free to add my: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> Thanks! I still would like Mika to have a look and perform a couple more tests myself. And I have one emergency task right now, so I think we have time to let others have a look and comment / test / etc. > > to patches 1-16 (patch 17 makes sense to me, but I do not feel > I'm the right person to ack it). > > Regards, > > Hans > > > > > > > > The last patch adds Intel GPIO tree as official one for ACPI GPIO > > changes. > > > > Assuming [1] makes v5.10 this series can be sent as PR to Linus > > for v5.11 cycle. > > > > Note, some patches are also depend to the code from GPIO fixes / for-next > > repositories. Unfortunately there is no one repository which contains all > > up to date for-next changes against GPIO subsystem. That's why I have merged > > Bart's for-current followed by Linus' fixes followed by Bart's for-next > > followed by Linus' for-next branches as prerequisites to the series. > > > > Cc: Jamie McClymont <jamie@kwiius.com> > > > > [1]: https://lore.kernel.org/linux-gpio/20201106181938.GA41213@black.fi.intel.com/ > > > > Changelog v5: > > - introduced gpio_set_debounce_timeout() > > - made a prerequisite refactoring in GPIO library code > > - updated the rest accordingly > > > > Changelog v4: > > - extended debounce setting to ACPI events and Operation Regions > > - added Ack (Linus) > > - added few more cleanup patches, including MAINTAINERS update > > > > Changelog v3: > > - dropped upstreamed OF patch > > - added debounce fix > > > > Andy Shevchenko (17): > > gpiolib: Replace unsigned by unsigned int > > gpiolib: add missed break statement > > gpiolib: use proper API to pack pin configuration parameters > > gpiolib: Add temporary variable to gpiod_set_transitory() for cleaner > > code > > gpiolib: Extract gpio_set_config_with_argument() for future use > > gpiolib: move bias related code from gpio_set_config() to > > gpio_set_bias() > > gpiolib: Extract gpio_set_config_with_argument_optional() helper > > gpiolib: Extract gpio_set_debounce_timeout() for internal use > > gpiolib: acpi: Respect bias settings for GpioInt() resource > > gpiolib: acpi: Use named item for enum gpiod_flags variable > > gpiolib: acpi: Take into account debounce settings > > gpiolib: acpi: Move acpi_gpio_to_gpiod_flags() upper in the code > > gpiolib: acpi: Make acpi_gpio_to_gpiod_flags() usable for GpioInt() > > gpiolib: acpi: Extract acpi_request_own_gpiod() helper > > gpiolib: acpi: Convert pin_index to be u16 > > gpiolib: acpi: Use BIT() macro to increase readability > > gpiolib: acpi: Make Intel GPIO tree official for GPIO ACPI work > > > > MAINTAINERS | 1 + > > drivers/gpio/gpiolib-acpi.c | 130 ++++++++++++++++++++-------------- > > drivers/gpio/gpiolib-acpi.h | 2 + > > drivers/gpio/gpiolib.c | 97 ++++++++++++++----------- > > drivers/gpio/gpiolib.h | 1 + > > include/linux/gpio/consumer.h | 4 +- > > 6 files changed, 141 insertions(+), 94 deletions(-) > > > -- With Best Regards, Andy Shevchenko
On Mon, Nov 09, 2020 at 10:53:16PM +0200, Andy Shevchenko wrote: > Replace unsigned by unsigned int in GPIO library code. > Note, legacy API left untouched. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Mon, Nov 09, 2020 at 10:53:18PM +0200, Andy Shevchenko wrote: > Instead of open coded macro use, call pinconf_to_config_packed(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Mon, Nov 09, 2020 at 10:53:19PM +0200, Andy Shevchenko wrote: > Temporary variable that keeps mode allows to neat the code a bit. > It will also help for the future changes. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Not really sure if this is good change or not. Instead of constant you now introduce a useless variable just to get them to the same line. To me this looks better and reads easier: packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE, !transitory); But not insisting so if GPIO maintainers are fine then no objections :)
On Mon, Nov 09, 2020 at 10:53:20PM +0200, Andy Shevchenko wrote: > In the future we will need to have a separate function > that takes an arbitrary argument value. > > Extract gpio_set_config_with_argument() for that purpose. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Mon, Nov 09, 2020 at 10:53:23PM +0200, Andy Shevchenko wrote: > In some cases we would like to have debounce setter which doesn't fail > when feature is not supported by a controller. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib.c | 7 +++++++ > drivers/gpio/gpiolib.h | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 4558af08df23..9d23fbf1f7cd 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -2162,6 +2162,13 @@ static int gpio_set_bias(struct gpio_desc *desc) > return gpio_set_config_with_argument_optional(desc, bias, arg); > } > > +int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce) > +{ > + enum pin_config_param mode = PIN_CONFIG_PERSIST_STATE; > + > + return gpio_set_config_with_argument_optional(desc, mode, debounce); Again I think mode variable is pretty useless here and does not improve readability.
On Wed, Nov 11, 2020 at 5:33 PM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Mon, Nov 09, 2020 at 10:53:19PM +0200, Andy Shevchenko wrote: > > Temporary variable that keeps mode allows to neat the code a bit. > > It will also help for the future changes. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Not really sure if this is good change or not. Instead of constant you > now introduce a useless variable just to get them to the same line. No, it's useful in a patch in the series as promised in the commit message. That variable reduces a line length a lot there. > To me this looks better and reads easier: > > packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE, !transitory); > > But not insisting so if GPIO maintainers are fine then no objections :) -- With Best Regards, Andy Shevchenko
On Mon, Nov 09, 2020 at 10:53:26PM +0200, Andy Shevchenko wrote: > We didn't take into account the debounce settings supplied by ACPI. > This change is targeting the mentioned gap. > > Reported-by: Coiby Xu <coiby.xu@gmail.com> > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/gpio/gpiolib-acpi.c | 18 ++++++++++++++++++ > drivers/gpio/gpiolib-acpi.h | 2 ++ > 2 files changed, 20 insertions(+) > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > index c127b410a7a2..6cbad96be866 100644 > --- a/drivers/gpio/gpiolib-acpi.c > +++ b/drivers/gpio/gpiolib-acpi.c > @@ -299,6 +299,10 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares, > return AE_OK; > } > > + ret = gpio_set_debounce_timeout(desc, agpio->debounce_timeout); > + if (ret) > + goto fail_free_desc; > + > ret = gpiochip_lock_as_irq(chip, pin); > if (ret) { > dev_err(chip->parent, > @@ -664,6 +668,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data) > lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr, > agpio->pin_table[pin_index]); > lookup->info.pin_config = agpio->pin_config; > + lookup->info.debounce = agpio->debounce_timeout; > lookup->info.gpioint = gpioint; > > /* > @@ -961,6 +966,10 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) > if (ret < 0) > return ret; > > + ret = gpio_set_debounce_timeout(desc, info.debounce); > + if (ret) > + return ret; > + > irq_flags = acpi_dev_get_irq_type(info.triggering, > info.polarity); > > @@ -1048,6 +1057,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, > if (!found) { > enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio); > const char *label = "ACPI:OpRegion"; > + int ret; > > desc = gpiochip_request_own_desc(chip, pin, label, > GPIO_ACTIVE_HIGH, > @@ -1058,6 +1068,14 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, > goto out; > } > > + ret = gpio_set_debounce_timeout(desc, agpio->debounce_timeout); > + if (ret) { > + status = AE_ERROR; > + gpiochip_free_own_desc(desc); > + mutex_unlock(&achip->conn_lock); Nit: I think you can set status outside of the critical section. Otherwise looks good, Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Wed, Nov 11, 2020 at 5:40 PM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Mon, Nov 09, 2020 at 10:53:23PM +0200, Andy Shevchenko wrote: ... > Again I think mode variable is pretty useless here and does not improve > readability. You mean something like return gpio_set_config_with_argument_optional(desc, PIN_CONFIG_INPUT_DEBOUNCE, debounce); is better? -- With Best Regards, Andy Shevchenko
On Mon, Nov 09, 2020 at 10:53:28PM +0200, Andy Shevchenko wrote: > GpioInt() implies input configuration of the pin. Add this to > the acpi_gpio_to_gpiod_flags() and make usable for GpioInt(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Mon, Nov 09, 2020 at 10:53:29PM +0200, Andy Shevchenko wrote: > It appears that we are using similar code excerpts for ACPI OpRegion > and event handling. Deduplicate those excerpts by extracting a new > acpi_request_own_gpiod() helper. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Wed, Nov 11, 2020 at 05:46:55PM +0200, Andy Shevchenko wrote: > On Wed, Nov 11, 2020 at 5:40 PM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Mon, Nov 09, 2020 at 10:53:23PM +0200, Andy Shevchenko wrote: > > ... > > > Again I think mode variable is pretty useless here and does not improve > > readability. > > You mean something like > > return gpio_set_config_with_argument_optional(desc, > PIN_CONFIG_INPUT_DEBOUNCE, debounce); > > is better? Yes.
On Mon, Nov 09, 2020 at 10:53:30PM +0200, Andy Shevchenko wrote: > As defined by ACPI the pin index is 16-bit unsigned integer. > Define the variable, which holds it, accordingly. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Wed, Nov 11, 2020 at 5:54 PM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Wed, Nov 11, 2020 at 05:46:55PM +0200, Andy Shevchenko wrote: > > On Wed, Nov 11, 2020 at 5:40 PM Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > On Mon, Nov 09, 2020 at 10:53:23PM +0200, Andy Shevchenko wrote: ... > > > Again I think mode variable is pretty useless here and does not improve > > > readability. > > > > You mean something like > > > > return gpio_set_config_with_argument_optional(desc, > > PIN_CONFIG_INPUT_DEBOUNCE, debounce); > > > > is better? Hmm... Not sure I can agree with this, but I can change. -- With Best Regards, Andy Shevchenko
On Wed, Nov 11, 2020 at 05:40:16PM +0200, Andy Shevchenko wrote: > On Wed, Nov 11, 2020 at 5:33 PM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Mon, Nov 09, 2020 at 10:53:19PM +0200, Andy Shevchenko wrote: ... > > To me this looks better and reads easier: > > > > packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE, !transitory); > > > > But not insisting so if GPIO maintainers are fine then no objections :) I have dropped this patch. -- With Best Regards, Andy Shevchenko
There are fixes (and plenty cleanups) that allow to take into consideration more parameters in ACPI, i.e. bias for GpioInt() and debounce timeout for Operation Regions, Events and GpioInt() resources. During review Hans noted, that gpiod_set_debounce() returns -ENOTSUPP for the cases when feature is not supported either by driver or a controller. It appears that we have slightly messy API here: FUNC Relation with ENOTSUPP gpiod_set_config() returns if not supported gpiod_set_debounce() as gpiod_set_config() above gpio_set_debounce() legacy wrapper on top of gpiod_set_debounce() gpiod_set_transitory() skips it (returns okay) with debug message gpio_set_config() returns if not supported gpio_set_bias() skips it (returns okay) Last two functions are internal to GPIO library, while the rest is exported API. In order to be consistent with both naming schemas the series introduces gpio_set_debounce_timeout() that considers the feature optional. New API is only for internal use. While at it, the few first patches do clean up the current GPIO library code to unify it to some extend. The above is followed by changes made in ACPI GPIO library part. The bias patch highly depends on Intel pin control driver changes (they are material for v5.10 [1]), due to this and amount of the prerequisite changes this series is probably not supposed to be backported (at least right now). The last patch adds Intel GPIO tree as official one for ACPI GPIO changes. Assuming [1] makes v5.10 this series can be sent as PR to Linus for v5.11 cycle. Note, some patches are also depend to the code from GPIO fixes / for-next repositories. Unfortunately there is no one repository which contains all up to date for-next changes against GPIO subsystem. That's why I have merged Bart's for-current followed by Linus' fixes followed by Bart's for-next followed by Linus' for-next branches as prerequisites to the series. Cc: Jamie McClymont <jamie@kwiius.com> [1]: https://lore.kernel.org/linux-gpio/20201106181938.GA41213@black.fi.intel.com/ Changelog v5: - introduced gpio_set_debounce_timeout() - made a prerequisite refactoring in GPIO library code - updated the rest accordingly Changelog v4: - extended debounce setting to ACPI events and Operation Regions - added Ack (Linus) - added few more cleanup patches, including MAINTAINERS update Changelog v3: - dropped upstreamed OF patch - added debounce fix Andy Shevchenko (17): gpiolib: Replace unsigned by unsigned int gpiolib: add missed break statement gpiolib: use proper API to pack pin configuration parameters gpiolib: Add temporary variable to gpiod_set_transitory() for cleaner code gpiolib: Extract gpio_set_config_with_argument() for future use gpiolib: move bias related code from gpio_set_config() to gpio_set_bias() gpiolib: Extract gpio_set_config_with_argument_optional() helper gpiolib: Extract gpio_set_debounce_timeout() for internal use gpiolib: acpi: Respect bias settings for GpioInt() resource gpiolib: acpi: Use named item for enum gpiod_flags variable gpiolib: acpi: Take into account debounce settings gpiolib: acpi: Move acpi_gpio_to_gpiod_flags() upper in the code gpiolib: acpi: Make acpi_gpio_to_gpiod_flags() usable for GpioInt() gpiolib: acpi: Extract acpi_request_own_gpiod() helper gpiolib: acpi: Convert pin_index to be u16 gpiolib: acpi: Use BIT() macro to increase readability gpiolib: acpi: Make Intel GPIO tree official for GPIO ACPI work MAINTAINERS | 1 + drivers/gpio/gpiolib-acpi.c | 130 ++++++++++++++++++++-------------- drivers/gpio/gpiolib-acpi.h | 2 + drivers/gpio/gpiolib.c | 97 ++++++++++++++----------- drivers/gpio/gpiolib.h | 1 + include/linux/gpio/consumer.h | 4 +- 6 files changed, 141 insertions(+), 94 deletions(-)