| Message ID | e6817d30-b443-1a73-efae-84415604b19f@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | platform/x86: int3472: don't use gpiod_toggle_active_low() | expand |
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
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.
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(-)