| Message ID | 20230926145943.42814-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | platform/x86: int3472: don't use gpiod_toggle_active_low() | expand |
On Tue, Sep 26, 2023 at 04:59:41PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use > temporary lookup tables with appropriate lookup flags. ... > + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( > + int3472->dev, path, agpio->pin_table[0], > + "int3472,privacy-led", polarity, > + GPIOD_OUT_LOW); Personally I found this style weird. I prefer to have longer line over the split on the parentheses.
On Tue, Sep 26, 2023 at 5:27 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Sep 26, 2023 at 04:59:41PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use > > temporary lookup tables with appropriate lookup flags. > > ... > > > + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( > > + int3472->dev, path, agpio->pin_table[0], > > + "int3472,privacy-led", polarity, > > + GPIOD_OUT_LOW); > > Personally I found this style weird. I prefer to have longer line over > the split on the parentheses. > I in turn prefer this one. Checkpatch doesn't complain either way so I'll leave it to the maintainers of this driver to decide. Bart
On Wed, Sep 27, 2023 at 10:38 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Bartosz, > > On 9/26/23 16:59, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > gpiod_toggle_active_low() is a badly designed API that should have never > > been used elsewhere then in the MMC code. And even there we should find > > a better solution. > > > > Replace the uses of it in the int3472 driver with the good old temporary > > lookup table trick. This is not very pretty either but it's the lesser > > evil. > > I saw your previous proposal which added a new api to directly set > the active_low flag, rather then toggle it. > > I intended to reply to that thread to say that I liked that approach, > but I don't remember if I actually did reply. > > I wonder what made you abandon the new function to directly set > the active-low flag on a gpio_desc? > > For the int3472 code that would work pretty well and it would > be much cleaner then the temp gpio-lookup approach. > You did reply, yes. Under one of the other patches Linus W stated that first: adding the ability for consumers to toggle the polarity was added to handle the MMC slot quirk, then it was used unknowingly to GPIO maintainers in other places (including this driver). I then acknowledged the fact that it should have never existed in the first place as this is HW description and should be defined in ACPI, DT or lookup flags. I'm not sure why this information needs to be hard-coded in the driver in int3472_get_func_and_polarity() but maybe it could be pulled into gpiolib-acpi.c with other quirks? Bart
Hi Bartosz, On 9/27/23 10:48, Bartosz Golaszewski wrote: > On Wed, Sep 27, 2023 at 10:38 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Bartosz, >> >> On 9/26/23 16:59, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>> >>> gpiod_toggle_active_low() is a badly designed API that should have never >>> been used elsewhere then in the MMC code. And even there we should find >>> a better solution. >>> >>> Replace the uses of it in the int3472 driver with the good old temporary >>> lookup table trick. This is not very pretty either but it's the lesser >>> evil. >> >> I saw your previous proposal which added a new api to directly set >> the active_low flag, rather then toggle it. >> >> I intended to reply to that thread to say that I liked that approach, >> but I don't remember if I actually did reply. >> >> I wonder what made you abandon the new function to directly set >> the active-low flag on a gpio_desc? >> >> For the int3472 code that would work pretty well and it would >> be much cleaner then the temp gpio-lookup approach. >> > > You did reply, yes. Under one of the other patches Linus W stated that > first: adding the ability for consumers to toggle the polarity was > added to handle the MMC slot quirk, then it was used unknowingly to > GPIO maintainers in other places (including this driver). I then > acknowledged the fact that it should have never existed in the first > place as this is HW description and should be defined in ACPI, DT or > lookup flags. I see and I understand. > I'm not sure why this information needs to be hard-coded in the driver > in int3472_get_func_and_polarity() but maybe it could be pulled into > gpiolib-acpi.c with other quirks? The problem is that for camera sensors Intel uses this special INT3472 ACPI device with a custom _DSM to list GPIOs, with the _DSM returning an u32 and one of the bits in the u32 is the polarity. We really do not want to deal with this Intel camera team hack inside gpiolib-acpi and I can understand why you and Linus W want to get rid of functions which allow drivers to meddle with a gpio_desc's active-low flag. So using a temporary gpio-lookup in the int3472 code as you are proposing is the best (least bad) thing to do here then. I'll try to make some time to test this sometime the coming days. Other then the discussion we just had is there any specific reason why this should be considered a RFC / why this would not be ready for merging? (I still need to review these, but lets assume that goes well) Regards, Hans
Hi Again, On 9/27/23 15:08, Hans de Goede wrote: > Hi Bart, > > On 9/27/23 12:44, Bartosz Golaszewski wrote: >> On Wed, Sep 27, 2023 at 11:40 AM Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> Hi, >>> >>> On 9/26/23 16:59, Bartosz Golaszewski wrote: >>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>>> >>>> Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use >>>> temporary lookup tables with appropriate lookup flags. >>>> >>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>>> --- >>>> drivers/platform/x86/intel/int3472/led.c | 12 ++++-------- >>>> 1 file changed, 4 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c >>>> index bca1ce7d0d0c..62e0cd5207a7 100644 >>>> --- a/drivers/platform/x86/intel/int3472/led.c >>>> +++ b/drivers/platform/x86/intel/int3472/led.c >>>> @@ -25,18 +25,14 @@ int skl_int3472_register_pled(struct int3472_discrete_device *int3472, >>>> if (int3472->pled.classdev.dev) >>>> return -EBUSY; >>>> >>>> - int3472->pled.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0], >>>> - "int3472,privacy-led"); >>>> + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( >>>> + int3472->dev, path, agpio->pin_table[0], >>>> + "int3472,privacy-led", polarity, >>>> + GPIOD_OUT_LOW); >>> >>> Yeah so this is not going to work, path here is an ACPI device path, e.g. >>> on my laptop (which actually uses the INT3472 glue code) the path-s of >>> the 2 GPIO controllers are: `\_SB_.GPI0` resp `\_SB_.PC00.XHCI.RHUB.HS08.VGPO` >>> >>> Where as skl_int3472_gpiod_get_from_temp_lookup() stores the passed in path >>> in gpiod_lookup_table.table[0].key, which is the dev_name() of the GPIO >>> controller's parent dev which are `INTC1055:00` resp. `INTC1096:00` . >>> >>> So we are going to need to add some code to INT3472 to go from path to >>> a correct value for gpiod_lookup_table.table[0].key which means partly >>> reproducing most of acpi_get_gpiod: >>> >>> struct gpio_chip *chip; >>> acpi_handle handle; >>> acpi_status status; >>> >>> status = acpi_get_handle(NULL, path, &handle); >>> if (ACPI_FAILURE(status)) >>> return ERR_PTR(-ENODEV); >>> >>> chip = gpiochip_find(handle, acpi_gpiochip_find); >>> if (!chip) >>> return ERR_PTR(-EPROBE_DEFER); >>> >>> And then get the key from the chip. Which means using gpiochip_find >>> in the int3472 code now, which does not sound like an improvement. >>> >>> I think that was is needed instead is adding an active_low flag >>> to acpi_get_and_request_gpiod() and then have that directly >>> set the active-low flag on the returned desc. >>> >> >> Ultimately I'd like everyone to use gpiod_get() for getting >> descriptors but for now I get it's enough. Are you find with this >> being done in a single patch across GPIO and this driver? > > Yes doing this in a single patch is fine. > > Also I'm fine with merging such a patch through the gpio tree . So thinking about this more I realized that the int3472 code already generates GPIO lookups for the sensor device for some (powerdown, reset) GPIOs, it only needs the gpio_desc for the case where the GPIO is turned into a regulator, clock or led. Since the int3472 code is already generating lookups it already has a way to go from path to a lookup "key": status = acpi_get_handle(NULL, path, &handle); if (ACPI_FAILURE(status)) return -EINVAL; adev = acpi_fetch_acpi_dev(handle); if (!adev) return -ENODEV; table_entry->key = acpi_dev_name(adev); So we can get the key without needing to call gpio_find_chip() So I do believe that the temp lookup approach should actually work. I'm currently traveling, so no promises but I should be able to rework your series in something which actually works and which will: 1. Stop using gpiod_toggle_active_low() 2. Allow dropping acpi_get_and_request_gpiod() So no need for a patch to add an active-low parameter to acpi_get_and_request_gpiod(), sorry about the noise. Regards, Hans >>>> if (IS_ERR(int3472->pled.gpio)) >>>> return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio), >>>> "getting privacy LED GPIO\n"); >>>> >>>> - if (polarity == GPIO_ACTIVE_LOW) >>>> - gpiod_toggle_active_low(int3472->pled.gpio); >>>> - >>>> - /* Ensure the pin is in output mode and non-active state */ >>>> - gpiod_direction_output(int3472->pled.gpio, 0); >>>> - >>>> /* Generate the name, replacing the ':' in the ACPI devname with '_' */ >>>> snprintf(int3472->pled.name, sizeof(int3472->pled.name), >>>> "%s::privacy_led", acpi_dev_name(int3472->sensor)); >>> >>
On Thu, 28 Sept 2023 at 14:40, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi All, > > Here is a v2 of Bartosz' "don't use gpiod_toggle_active_low()" series. > > New in v2: > - Rework to deal with ACPI path vs gpiod_lookup.key differences: > acpi_get_handle(path) -> acpi_fetch_acpi_dev(handle) -> acpi_dev_name(adev) > > Regards, > > Hans > > > Bartosz Golaszewski (2): > platform/x86: int3472: Add new > skl_int3472_gpiod_get_from_temp_lookup() helper > gpio: acpi: remove acpi_get_and_request_gpiod() > > Hans de Goede (3): > platform/x86: int3472: Add new skl_int3472_fill_gpiod_lookup() helper > platform/x86: int3472: Stop using gpiod_toggle_active_low() > platform/x86: int3472: Switch to devm_get_gpiod() > > drivers/gpio/gpiolib-acpi.c | 28 ----- > .../x86/intel/int3472/clk_and_regulator.c | 54 ++-------- > drivers/platform/x86/intel/int3472/common.h | 7 +- > drivers/platform/x86/intel/int3472/discrete.c | 101 ++++++++++++++---- > drivers/platform/x86/intel/int3472/led.c | 24 +---- > include/linux/gpio/consumer.h | 8 -- > 6 files changed, 93 insertions(+), 129 deletions(-) > > -- > 2.41.0 > Thanks Hans, this looks good to me. I'd let it sit on the list for a week. After that, do you want to take patches 1-4 and provide me with another tag? Bart
Hi, On 9/28/23 20:40, Bartosz Golaszewski wrote: > On Thu, 28 Sept 2023 at 14:40, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi All, >> >> Here is a v2 of Bartosz' "don't use gpiod_toggle_active_low()" series. >> >> New in v2: >> - Rework to deal with ACPI path vs gpiod_lookup.key differences: >> acpi_get_handle(path) -> acpi_fetch_acpi_dev(handle) -> acpi_dev_name(adev) >> >> Regards, >> >> Hans >> >> >> Bartosz Golaszewski (2): >> platform/x86: int3472: Add new >> skl_int3472_gpiod_get_from_temp_lookup() helper >> gpio: acpi: remove acpi_get_and_request_gpiod() >> >> Hans de Goede (3): >> platform/x86: int3472: Add new skl_int3472_fill_gpiod_lookup() helper >> platform/x86: int3472: Stop using gpiod_toggle_active_low() >> platform/x86: int3472: Switch to devm_get_gpiod() >> >> drivers/gpio/gpiolib-acpi.c | 28 ----- >> .../x86/intel/int3472/clk_and_regulator.c | 54 ++-------- >> drivers/platform/x86/intel/int3472/common.h | 7 +- >> drivers/platform/x86/intel/int3472/discrete.c | 101 ++++++++++++++---- >> drivers/platform/x86/intel/int3472/led.c | 24 +---- >> include/linux/gpio/consumer.h | 8 -- >> 6 files changed, 93 insertions(+), 129 deletions(-) >> >> -- >> 2.41.0 >> > > Thanks Hans, this looks good to me. I'd let it sit on the list for a > week. After that, do you want to take patches 1-4 and provide me with > another tag? That sounds like a good plan to me, will do. Regards, Hans
Hi Bart, On 9/28/23 20:40, Bartosz Golaszewski wrote: > On Thu, 28 Sept 2023 at 14:40, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi All, >> >> Here is a v2 of Bartosz' "don't use gpiod_toggle_active_low()" series. >> >> New in v2: >> - Rework to deal with ACPI path vs gpiod_lookup.key differences: >> acpi_get_handle(path) -> acpi_fetch_acpi_dev(handle) -> acpi_dev_name(adev) >> >> Regards, >> >> Hans >> >> >> Bartosz Golaszewski (2): >> platform/x86: int3472: Add new >> skl_int3472_gpiod_get_from_temp_lookup() helper >> gpio: acpi: remove acpi_get_and_request_gpiod() >> >> Hans de Goede (3): >> platform/x86: int3472: Add new skl_int3472_fill_gpiod_lookup() helper >> platform/x86: int3472: Stop using gpiod_toggle_active_low() >> platform/x86: int3472: Switch to devm_get_gpiod() >> >> drivers/gpio/gpiolib-acpi.c | 28 ----- >> .../x86/intel/int3472/clk_and_regulator.c | 54 ++-------- >> drivers/platform/x86/intel/int3472/common.h | 7 +- >> drivers/platform/x86/intel/int3472/discrete.c | 101 ++++++++++++++---- >> drivers/platform/x86/intel/int3472/led.c | 24 +---- >> include/linux/gpio/consumer.h | 8 -- >> 6 files changed, 93 insertions(+), 129 deletions(-) >> >> -- >> 2.41.0 >> > > Thanks Hans, this looks good to me. I'd let it sit on the list for a > week. After that, do you want to take patches 1-4 and provide me with > another tag? I have just send out a v3 to address Andy's remark about me somehow resetting the authorship to me on 2 patches from Bartosz. While working on this I noticed (and fixed) a bug in: [RFT PATCH 1/4] platform/x86: int3472: provide a helper for getting GPIOs from lookups https://lore.kernel.org/all/20230926145943.42814-2-brgl@bgdev.pl/ struct gpiod_lookup_table *lookup __free(kfree) = kzalloc(struct_size(lookup, table, 1), GFP_KERNEL); You are allocating an entry for the temp lookup, but the gpiolib core expects lookup tables to be terminated with an entry lookup, so this should alloc space for 2 entries: struct gpiod_lookup_table *lookup __free(kfree) = kzalloc(struct_size(lookup, table, 2), GFP_KERNEL); Despite this already being fixed now I wanted to explicitly point this out in case you have used the same construct elsewhere during your recent gpiolib cleanup efforts ? As for your request for a tag for the 4st 4 patches for you to merge into gpiolib. I'll go and work work on that. I need to coordinate this with Ilpo, with whom I now co-maintain pdx86 . Regards, Hans
On Wed, Oct 4, 2023 at 6:30 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Bart, > > On 9/28/23 20:40, Bartosz Golaszewski wrote: > > On Thu, 28 Sept 2023 at 14:40, Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi All, > >> > >> Here is a v2 of Bartosz' "don't use gpiod_toggle_active_low()" series. > >> > >> New in v2: > >> - Rework to deal with ACPI path vs gpiod_lookup.key differences: > >> acpi_get_handle(path) -> acpi_fetch_acpi_dev(handle) -> acpi_dev_name(adev) > >> > >> Regards, > >> > >> Hans > >> > >> > >> Bartosz Golaszewski (2): > >> platform/x86: int3472: Add new > >> skl_int3472_gpiod_get_from_temp_lookup() helper > >> gpio: acpi: remove acpi_get_and_request_gpiod() > >> > >> Hans de Goede (3): > >> platform/x86: int3472: Add new skl_int3472_fill_gpiod_lookup() helper > >> platform/x86: int3472: Stop using gpiod_toggle_active_low() > >> platform/x86: int3472: Switch to devm_get_gpiod() > >> > >> drivers/gpio/gpiolib-acpi.c | 28 ----- > >> .../x86/intel/int3472/clk_and_regulator.c | 54 ++-------- > >> drivers/platform/x86/intel/int3472/common.h | 7 +- > >> drivers/platform/x86/intel/int3472/discrete.c | 101 ++++++++++++++---- > >> drivers/platform/x86/intel/int3472/led.c | 24 +---- > >> include/linux/gpio/consumer.h | 8 -- > >> 6 files changed, 93 insertions(+), 129 deletions(-) > >> > >> -- > >> 2.41.0 > >> > > > > Thanks Hans, this looks good to me. I'd let it sit on the list for a > > week. After that, do you want to take patches 1-4 and provide me with > > another tag? > > I have just send out a v3 to address Andy's remark about me > somehow resetting the authorship to me on 2 patches from Bartosz. > > While working on this I noticed (and fixed) a bug in: > > [RFT PATCH 1/4] platform/x86: int3472: provide a helper for getting GPIOs from lookups > https://lore.kernel.org/all/20230926145943.42814-2-brgl@bgdev.pl/ > > struct gpiod_lookup_table *lookup __free(kfree) = > kzalloc(struct_size(lookup, table, 1), GFP_KERNEL); > > You are allocating an entry for the temp lookup, but the gpiolib > core expects lookup tables to be terminated with an entry lookup, > so this should alloc space for 2 entries: > > struct gpiod_lookup_table *lookup __free(kfree) = > kzalloc(struct_size(lookup, table, 2), GFP_KERNEL); > > Despite this already being fixed now I wanted to explicitly point > this out in case you have used the same construct elsewhere during > your recent gpiolib cleanup efforts ? > > As for your request for a tag for the 4st 4 patches for you to merge > into gpiolib. I'll go and work work on that. I need to coordinate > this with Ilpo, with whom I now co-maintain pdx86 . > > Regards, > > Hans > > Gah, thank you for bringing this up, I need one fix for a SPI driver. Bart
On Wed, 4 Oct 2023, Hans de Goede wrote: > Hi Bart, > > On 9/28/23 20:40, Bartosz Golaszewski wrote: > > On Thu, 28 Sept 2023 at 14:40, Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi All, > >> > >> Here is a v2 of Bartosz' "don't use gpiod_toggle_active_low()" series. > >> > >> New in v2: > >> - Rework to deal with ACPI path vs gpiod_lookup.key differences: > >> acpi_get_handle(path) -> acpi_fetch_acpi_dev(handle) -> acpi_dev_name(adev) > >> > >> Regards, > >> > >> Hans > >> > >> > >> Bartosz Golaszewski (2): > >> platform/x86: int3472: Add new > >> skl_int3472_gpiod_get_from_temp_lookup() helper > >> gpio: acpi: remove acpi_get_and_request_gpiod() > >> > >> Hans de Goede (3): > >> platform/x86: int3472: Add new skl_int3472_fill_gpiod_lookup() helper > >> platform/x86: int3472: Stop using gpiod_toggle_active_low() > >> platform/x86: int3472: Switch to devm_get_gpiod() > >> > >> drivers/gpio/gpiolib-acpi.c | 28 ----- > >> .../x86/intel/int3472/clk_and_regulator.c | 54 ++-------- > >> drivers/platform/x86/intel/int3472/common.h | 7 +- > >> drivers/platform/x86/intel/int3472/discrete.c | 101 ++++++++++++++---- > >> drivers/platform/x86/intel/int3472/led.c | 24 +---- > >> include/linux/gpio/consumer.h | 8 -- > >> 6 files changed, 93 insertions(+), 129 deletions(-) > >> > >> -- > >> 2.41.0 > >> > > > > Thanks Hans, this looks good to me. I'd let it sit on the list for a > > week. After that, do you want to take patches 1-4 and provide me with > > another tag? > > I have just send out a v3 to address Andy's remark about me > somehow resetting the authorship to me on 2 patches from Bartosz. > As for your request for a tag for the 4st 4 patches for you to merge > into gpiolib. I'll go and work work on that. I need to coordinate > this with Ilpo, with whom I now co-maintain pdx86 . Thanks all. I've applied patches 1-4 into platform-drivers-x86-int3472 and merged that into review-ilpo. I'll send the IB PR once LKP has done its thing for the branch.
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> gpiod_toggle_active_low() is a badly designed API that should have never been used elsewhere then in the MMC code. And even there we should find a better solution. Replace the uses of it in the int3472 driver with the good old temporary lookup table trick. This is not very pretty either but it's the lesser evil. Bartosz Golaszewski (4): platform/x86: int3472: provide a helper for getting GPIOs from lookups platform/x86: int3472: led: don't use gpiod_toggle_active_low() platform/x86: int3472: clk_and_regulator: use GPIO lookup tables gpio: acpi: remove acpi_get_and_request_gpiod() drivers/gpio/gpiolib-acpi.c | 28 ------------------ .../x86/intel/int3472/clk_and_regulator.c | 22 ++++++-------- drivers/platform/x86/intel/int3472/common.c | 29 +++++++++++++++++++ drivers/platform/x86/intel/int3472/common.h | 9 ++++++ drivers/platform/x86/intel/int3472/led.c | 12 +++----- include/linux/gpio/consumer.h | 8 ----- 6 files changed, 51 insertions(+), 57 deletions(-)