| Message ID | 20230926145943.42814-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | platform/x86: int3472: don't use gpiod_toggle_active_low() | expand |
On Thu, Sep 28, 2023 at 02:42:50PM +0200, Hans de Goede wrote: > Add a new skl_int3472_gpiod_get_from_temp_lookup() helper. > > This is a preparation patch for removing usage of the deprecated > gpiod_toggle_active_low() and acpi_get_and_request_gpiod() functions. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > [hdegoede@redhat.com] use the new skl_int3472_fill_gpiod_lookup() helper > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Something wrong between authorship and committer and SoB chain. I believe you need to preserve the authorship and add yourself as Co-developed-by: ?
Hi, On 10/1/23 10:42, Andy Shevchenko wrote: > On Thu, Sep 28, 2023 at 02:42:50PM +0200, Hans de Goede wrote: >> Add a new skl_int3472_gpiod_get_from_temp_lookup() helper. >> >> This is a preparation patch for removing usage of the deprecated >> gpiod_toggle_active_low() and acpi_get_and_request_gpiod() functions. >> >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >> [hdegoede@redhat.com] use the new skl_int3472_fill_gpiod_lookup() helper >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Something wrong between authorship and committer and SoB chain. > I believe you need to preserve the authorship and add yourself as > Co-developed-by: ? Yes you are correct, I'll prepare a new version of the series with this fixed. Regards, hans
On Thu, Sep 28, 2023 at 02:40:03PM +0200, Hans de Goede 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) Code-wise LGTM, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> But please fix tags here and there...
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(-)