[RFT PATCH 0/4] platform/x86: int3472: don't use gpiod_toggle_active_low()

[RFT PATCH 0/4] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Bartosz Golaszewski]

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(-)

-- 
2.39.2

[RFT PATCH 1/4] platform/x86: int3472: provide a helper for getting GPIOs from lookups

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

gpiod_toggle_active_low() should have never existed in the first place
and once it was added, it should have never been used outside the MMC
slot code.

Stop using it in the int3472 driver and use temporary lookup tables
instead. First: add a helper wrapping the common code in one function.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/platform/x86/intel/int3472/common.c | 29 +++++++++++++++++++++
 drivers/platform/x86/intel/int3472/common.h |  9 +++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c
index 9db2bb0bbba4..61a8240ed6db 100644
--- a/drivers/platform/x86/intel/int3472/common.c
+++ b/drivers/platform/x86/intel/int3472/common.c
@@ -2,6 +2,10 @@
 /* Author: Dan Scally <djrscally@gmail.com> */
 
 #include <linux/acpi.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/gpio/machine.h>
+#include <linux/overflow.h>
 #include <linux/slab.h>
 
 #include "common.h"
@@ -80,3 +84,28 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev,
 
 	return ret;
 }
+
+/* This should *really* only be used when there's no other way... */
+struct gpio_desc *
+skl_int3472_gpiod_get_from_temp_lookup(struct device *dev,
+				       const char *chip_label, u16 hwnum,
+				       const char *con_id,
+				       enum gpio_lookup_flags lflags,
+				       enum gpiod_flags rflags)
+{
+	struct gpio_desc *desc;
+
+	struct gpiod_lookup_table *lookup __free(kfree) =
+			kzalloc(struct_size(lookup, table, 1), GFP_KERNEL);
+	if (!lookup)
+		return ERR_PTR(-ENOMEM);
+
+	lookup->dev_id = dev_name(dev);
+	lookup->table[0] = GPIO_LOOKUP(chip_label, hwnum, con_id, lflags);
+
+	gpiod_add_lookup_table(lookup);
+
+	desc = gpiod_get(dev, con_id, rflags);
+	gpiod_remove_lookup_table(lookup);
+	return desc;
+}
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 9f29baa13860..85ef9b630044 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -5,6 +5,8 @@
 #define _INTEL_SKL_INT3472_H
 
 #include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
 #include <linux/gpio/machine.h>
 #include <linux/leds.h>
 #include <linux/regulator/driver.h>
@@ -129,4 +131,11 @@ int skl_int3472_register_pled(struct int3472_discrete_device *int3472,
 			      struct acpi_resource_gpio *agpio, u32 polarity);
 void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472);
 
+struct gpio_desc *
+skl_int3472_gpiod_get_from_temp_lookup(struct device *dev,
+				       const char *chip_label, u16 hwnum,
+				       const char *con_id,
+				       enum gpio_lookup_flags lflags,
+				       enum gpiod_flags rflags);
+
 #endif
-- 
2.39.2

Re: [RFT PATCH 1/4] platform/x86: int3472: provide a helper for getting GPIOs from lookups

[Andy Shevchenko]

On Tue, Sep 26, 2023 at 04:59:40PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> gpiod_toggle_active_low() should have never existed in the first place
> and once it was added, it should have never been used outside the MMC
> slot code.
> 
> Stop using it in the int3472 driver and use temporary lookup tables
> instead. First: add a helper wrapping the common code in one function.

>  #include <linux/clk-provider.h>

...

> +#include <linux/device.h>

No need to have this in the header, use forward declaration instead.

-- 
With Best Regards,
Andy Shevchenko

[RFT PATCH 2/4] platform/x86: int3472: led: don't use gpiod_toggle_active_low()

[Bartosz Golaszewski]

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);
 	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));
-- 
2.39.2

Re: [RFT PATCH 2/4] platform/x86: int3472: led: don't use gpiod_toggle_active_low()

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFT PATCH 2/4] platform/x86: int3472: led: don't use gpiod_toggle_active_low()

[Bartosz Golaszewski]

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

Re: [RFT PATCH 2/4] platform/x86: int3472: led: don't use gpiod_toggle_active_low()

[Hans de Goede]

HI,

On 9/27/23 09:02, Bartosz Golaszewski wrote:
> 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.

I'm fine with keeping this as is, using longer lines does not seem to make
things better here.

Regards,

Hans

Re: [RFT PATCH 2/4] platform/x86: int3472: led: don't use gpiod_toggle_active_low()

[Hans de Goede]

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.

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));

Re: [RFT PATCH 2/4] platform/x86: int3472: led: don't use gpiod_toggle_active_low()

[Bartosz Golaszewski]

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?

Bart

> 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));
>

Re: [RFT PATCH 2/4] platform/x86: int3472: led: don't use gpiod_toggle_active_low()

[Hans de Goede]

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 .

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));
>>
> 

Re: [RFT PATCH 2/4] platform/x86: int3472: led: don't use gpiod_toggle_active_low()

[Hans de Goede]

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));
>>>
>>

[RFT PATCH 3/4] platform/x86: int3472: clk_and_regulator: use GPIO lookup tables

[Bartosz Golaszewski]

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>
---
 .../x86/intel/int3472/clk_and_regulator.c     | 22 ++++++++-----------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index ef4b3141efcd..ec4c4848a2c4 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -174,20 +174,16 @@ int skl_int3472_register_gpio_clock(struct int3472_discrete_device *int3472,
 	if (int3472->clock.cl)
 		return -EBUSY;
 
-	int3472->clock.ena_gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
-							     "int3472,clk-enable");
+	int3472->clock.ena_gpio = skl_int3472_gpiod_get_from_temp_lookup(
+					int3472->dev, path, agpio->pin_table[0],
+					"int3472,clk-enable", polarity,
+					GPIOD_OUT_LOW);
 	if (IS_ERR(int3472->clock.ena_gpio)) {
 		ret = PTR_ERR(int3472->clock.ena_gpio);
 		int3472->clock.ena_gpio = NULL;
 		return dev_err_probe(int3472->dev, ret, "getting clk-enable GPIO\n");
 	}
 
-	if (polarity == GPIO_ACTIVE_LOW)
-		gpiod_toggle_active_low(int3472->clock.ena_gpio);
-
-	/* Ensure the pin is in output mode and non-active state */
-	gpiod_direction_output(int3472->clock.ena_gpio, 0);
-
 	init.name = kasprintf(GFP_KERNEL, "%s-clk",
 			      acpi_dev_name(int3472->adev));
 	if (!init.name) {
@@ -314,17 +310,17 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 						int3472->regulator.supply_name,
 						&int3472_gpio_regulator_ops);
 
-	int3472->regulator.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
-							     "int3472,regulator");
+	/* Ensure the pin is in output mode and non-active state */
+	int3472->regulator.gpio = skl_int3472_gpiod_get_from_temp_lookup(
+					int3472->dev, path, agpio->pin_table[0],
+					"int3472,regulator", GPIO_ACTIVE_HIGH,
+					GPIOD_OUT_LOW);
 	if (IS_ERR(int3472->regulator.gpio)) {
 		ret = PTR_ERR(int3472->regulator.gpio);
 		int3472->regulator.gpio = NULL;
 		return dev_err_probe(int3472->dev, ret, "getting regulator GPIO\n");
 	}
 
-	/* Ensure the pin is in output mode and non-active state */
-	gpiod_direction_output(int3472->regulator.gpio, 0);
-
 	cfg.dev = &int3472->adev->dev;
 	cfg.init_data = &init_data;
 	cfg.ena_gpiod = int3472->regulator.gpio;
-- 
2.39.2

Re: [RFT PATCH 3/4] platform/x86: int3472: clk_and_regulator: use GPIO lookup tables

[Andy Shevchenko]

On Tue, Sep 26, 2023 at 04:59:42PM +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.

As per previous patch.

-- 
With Best Regards,
Andy Shevchenko

[RFT PATCH 4/4] gpio: acpi: remove acpi_get_and_request_gpiod()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With no more users, we can remove acpi_get_and_request_gpiod().

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-acpi.c   | 28 ----------------------------
 include/linux/gpio/consumer.h |  8 --------
 2 files changed, 36 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 17a86bdd9609..89ff93f3b579 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -158,34 +158,6 @@ static struct gpio_desc *acpi_get_gpiod(char *path, unsigned int pin)
 	return gpiochip_get_desc(chip, pin);
 }
 
-/**
- * acpi_get_and_request_gpiod - Translate ACPI GPIO pin to GPIO descriptor and
- *                              hold a refcount to the GPIO device.
- * @path:      ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
- * @pin:       ACPI GPIO pin number (0-based, controller-relative)
- * @label:     Label to pass to gpiod_request()
- *
- * This function is a simple pass-through to acpi_get_gpiod(), except that
- * as it is intended for use outside of the GPIO layer (in a similar fashion to
- * gpiod_get_index() for example) it also holds a reference to the GPIO device.
- */
-struct gpio_desc *acpi_get_and_request_gpiod(char *path, unsigned int pin, char *label)
-{
-	struct gpio_desc *gpio;
-	int ret;
-
-	gpio = acpi_get_gpiod(path, pin);
-	if (IS_ERR(gpio))
-		return gpio;
-
-	ret = gpiod_request(gpio, label);
-	if (ret)
-		return ERR_PTR(ret);
-
-	return gpio;
-}
-EXPORT_SYMBOL_GPL(acpi_get_and_request_gpiod);
-
 static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
 {
 	struct acpi_gpio_event *event = data;
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 6cc345440a5b..db2dfbae8edb 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -606,8 +606,6 @@ void acpi_dev_remove_driver_gpios(struct acpi_device *adev);
 int devm_acpi_dev_add_driver_gpios(struct device *dev,
 				   const struct acpi_gpio_mapping *gpios);
 
-struct gpio_desc *acpi_get_and_request_gpiod(char *path, unsigned int pin, char *label);
-
 #else  /* CONFIG_GPIOLIB && CONFIG_ACPI */
 
 #include <linux/err.h>
@@ -625,12 +623,6 @@ static inline int devm_acpi_dev_add_driver_gpios(struct device *dev,
 	return -ENXIO;
 }
 
-static inline struct gpio_desc *acpi_get_and_request_gpiod(char *path, unsigned int pin,
-							   char *label)
-{
-	return ERR_PTR(-ENOSYS);
-}
-
 #endif /* CONFIG_GPIOLIB && CONFIG_ACPI */
 
 
-- 
2.39.2

Re: [RFT PATCH 4/4] gpio: acpi: remove acpi_get_and_request_gpiod()

[Andy Shevchenko]

On Tue, Sep 26, 2023 at 04:59:43PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> With no more users, we can remove acpi_get_and_request_gpiod().

The best patch in the series!
Reviewed-by: From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [RFT PATCH 4/4] gpio: acpi: remove acpi_get_and_request_gpiod()

[Mika Westerberg]

On Tue, Sep 26, 2023 at 06:27:54PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 26, 2023 at 04:59:43PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > 
> > With no more users, we can remove acpi_get_and_request_gpiod().
> 
> The best patch in the series!
> Reviewed-by: From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Fully agree!

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [RFT PATCH 4/4] gpio: acpi: remove acpi_get_and_request_gpiod()

[Bartosz Golaszewski]

On Tue, Sep 26, 2023 at 4:59 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> With no more users, we can remove acpi_get_and_request_gpiod().
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

With Hans' patches applied, I queued this on top.

Bart

Re: [RFT PATCH 0/4] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Andy Shevchenko]

On Tue, Sep 26, 2023 at 04:59:39PM +0200, 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.

Good jon!
I have only style issues, otherwise
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [RFT PATCH 0/4] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Hans de Goede]

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.

Regards,

Hans



> 
> 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(-)
> 

Re: [RFT PATCH 0/4] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Hans de Goede]

Hi,

On 9/27/23 10:38, Hans de Goede 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.

I missed that 4/4 removes acpi_get_and_request_gpiod(),
so I guess that this is not just only about removing gpiod_toggle_active_low()
but also about removing gpiod_toggle_active_low() ?

Regards,

Hans



>>
>> 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(-)
>>

Re: [RFT PATCH 0/4] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Bartosz Golaszewski]

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

Re: [RFT PATCH 0/4] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Hans de Goede]

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

Re: [RFT PATCH 0/4] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Bartosz Golaszewski]

On Wed, Sep 27, 2023 at 11:02 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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)
>

This is not an RFC but rather RFT - Request For Testing. I don't have
any HW to test those with so I only built it.

Bart

[PATCH v2 0/5] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Hans de Goede]

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

[PATCH v2 1/5] platform/x86: int3472: Add new skl_int3472_fill_gpiod_lookup() helper

[Hans de Goede]

Add a new skl_int3472_fill_gpiod_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: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 32 +++++++++++++------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index e33c2d75975c..351ecf047944 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -52,21 +52,15 @@ static void skl_int3472_log_sensor_module_name(struct int3472_discrete_device *i
 	}
 }
 
-static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int3472,
-					  struct acpi_resource_gpio *agpio,
-					  const char *func, u32 polarity)
+static int skl_int3472_fill_gpiod_lookup(struct gpiod_lookup *table_entry,
+					 struct acpi_resource_gpio *agpio,
+					 const char *func, u32 polarity)
 {
 	char *path = agpio->resource_source.string_ptr;
-	struct gpiod_lookup *table_entry;
 	struct acpi_device *adev;
 	acpi_handle handle;
 	acpi_status status;
 
-	if (int3472->n_sensor_gpios >= INT3472_MAX_SENSOR_GPIOS) {
-		dev_warn(int3472->dev, "Too many GPIOs mapped\n");
-		return -EINVAL;
-	}
-
 	status = acpi_get_handle(NULL, path, &handle);
 	if (ACPI_FAILURE(status))
 		return -EINVAL;
@@ -75,13 +69,31 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 	if (!adev)
 		return -ENODEV;
 
-	table_entry = &int3472->gpios.table[int3472->n_sensor_gpios];
 	table_entry->key = acpi_dev_name(adev);
 	table_entry->chip_hwnum = agpio->pin_table[0];
 	table_entry->con_id = func;
 	table_entry->idx = 0;
 	table_entry->flags = polarity;
 
+	return 0;
+}
+
+static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int3472,
+					  struct acpi_resource_gpio *agpio,
+					  const char *func, u32 polarity)
+{
+	int ret;
+
+	if (int3472->n_sensor_gpios >= INT3472_MAX_SENSOR_GPIOS) {
+		dev_warn(int3472->dev, "Too many GPIOs mapped\n");
+		return -EINVAL;
+	}
+
+	ret = skl_int3472_fill_gpiod_lookup(&int3472->gpios.table[int3472->n_sensor_gpios],
+					    agpio, func, polarity);
+	if (ret)
+		return ret;
+
 	int3472->n_sensor_gpios++;
 
 	return 0;
-- 
2.41.0

[PATCH v2 2/5] platform/x86: int3472: Add new skl_int3472_gpiod_get_from_temp_lookup() helper

[Hans de Goede]

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>
---
 drivers/platform/x86/intel/int3472/discrete.c | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 351ecf047944..a46c3a206aa3 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -99,6 +99,32 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 	return 0;
 }
 
+/* This should *really* only be used when there's no other way... */
+static struct gpio_desc *
+skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
+				       struct acpi_resource_gpio *agpio,
+				       const char *func, u32 polarity)
+{
+	struct gpio_desc *desc;
+	int ret;
+
+	struct gpiod_lookup_table *lookup __free(kfree) =
+			kzalloc(struct_size(lookup, table, 1), GFP_KERNEL);
+	if (!lookup)
+		return ERR_PTR(-ENOMEM);
+
+	lookup->dev_id = dev_name(int3472->dev);
+	ret = skl_int3472_fill_gpiod_lookup(&lookup->table[0], agpio, func, polarity);
+	if (ret)
+		return ERR_PTR(ret);
+
+	gpiod_add_lookup_table(lookup);
+	desc = gpiod_get(int3472->dev, func, GPIOD_OUT_LOW);
+	gpiod_remove_lookup_table(lookup);
+
+	return desc;
+}
+
 static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
 {
 	switch (type) {
-- 
2.41.0

Re: [PATCH v2 2/5] platform/x86: int3472: Add new skl_int3472_gpiod_get_from_temp_lookup() helper

[Andy Shevchenko]

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: ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] platform/x86: int3472: Add new skl_int3472_gpiod_get_from_temp_lookup() helper

[Hans de Goede]

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

[PATCH v2 3/5] platform/x86: int3472: Stop using gpiod_toggle_active_low()

[Hans de Goede]

Use the new skl_int3472_gpiod_get_from_temp_lookup() helper to get
a gpio to pass to register_gpio_clock(), skl_int3472_register_regulator()
and skl_int3472_register_pled().

This removes all use of the deprecated gpiod_toggle_active_low() and
acpi_get_and_request_gpiod() functions.

Suggested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../x86/intel/int3472/clk_and_regulator.c     | 31 ++-----------
 drivers/platform/x86/intel/int3472/common.h   |  7 ++-
 drivers/platform/x86/intel/int3472/discrete.c | 43 +++++++++++++------
 drivers/platform/x86/intel/int3472/led.c      | 17 ++------
 4 files changed, 40 insertions(+), 58 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index ef4b3141efcd..459f96c04ca1 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -162,9 +162,8 @@ int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472)
 }
 
 int skl_int3472_register_gpio_clock(struct int3472_discrete_device *int3472,
-				    struct acpi_resource_gpio *agpio, u32 polarity)
+				    struct gpio_desc *gpio)
 {
-	char *path = agpio->resource_source.string_ptr;
 	struct clk_init_data init = {
 		.ops = &skl_int3472_clock_ops,
 		.flags = CLK_GET_RATE_NOCACHE,
@@ -174,19 +173,7 @@ int skl_int3472_register_gpio_clock(struct int3472_discrete_device *int3472,
 	if (int3472->clock.cl)
 		return -EBUSY;
 
-	int3472->clock.ena_gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
-							     "int3472,clk-enable");
-	if (IS_ERR(int3472->clock.ena_gpio)) {
-		ret = PTR_ERR(int3472->clock.ena_gpio);
-		int3472->clock.ena_gpio = NULL;
-		return dev_err_probe(int3472->dev, ret, "getting clk-enable GPIO\n");
-	}
-
-	if (polarity == GPIO_ACTIVE_LOW)
-		gpiod_toggle_active_low(int3472->clock.ena_gpio);
-
-	/* Ensure the pin is in output mode and non-active state */
-	gpiod_direction_output(int3472->clock.ena_gpio, 0);
+	int3472->clock.ena_gpio = gpio;
 
 	init.name = kasprintf(GFP_KERNEL, "%s-clk",
 			      acpi_dev_name(int3472->adev));
@@ -273,9 +260,8 @@ static const struct dmi_system_id skl_int3472_regulator_second_sensor[] = {
 };
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
-				   struct acpi_resource_gpio *agpio)
+				   struct gpio_desc *gpio)
 {
-	char *path = agpio->resource_source.string_ptr;
 	struct regulator_init_data init_data = { };
 	struct regulator_config cfg = { };
 	const char *second_sensor = NULL;
@@ -314,16 +300,7 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 						int3472->regulator.supply_name,
 						&int3472_gpio_regulator_ops);
 
-	int3472->regulator.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
-							     "int3472,regulator");
-	if (IS_ERR(int3472->regulator.gpio)) {
-		ret = PTR_ERR(int3472->regulator.gpio);
-		int3472->regulator.gpio = NULL;
-		return dev_err_probe(int3472->dev, ret, "getting regulator GPIO\n");
-	}
-
-	/* Ensure the pin is in output mode and non-active state */
-	gpiod_direction_output(int3472->regulator.gpio, 0);
+	int3472->regulator.gpio = gpio;
 
 	cfg.dev = &int3472->adev->dev;
 	cfg.init_data = &init_data;
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 9f29baa13860..145dec66df64 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -117,16 +117,15 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev,
 					 const char **name_ret);
 
 int skl_int3472_register_gpio_clock(struct int3472_discrete_device *int3472,
-				    struct acpi_resource_gpio *agpio, u32 polarity);
+				    struct gpio_desc *gpio);
 int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472);
 void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472);
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
-				   struct acpi_resource_gpio *agpio);
+				   struct gpio_desc *gpio);
 void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472);
 
-int skl_int3472_register_pled(struct int3472_discrete_device *int3472,
-			      struct acpi_resource_gpio *agpio, u32 polarity);
+int skl_int3472_register_pled(struct int3472_discrete_device *int3472, struct gpio_desc *gpio);
 void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472);
 
 #endif
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index a46c3a206aa3..eb0cded5b92a 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -194,6 +194,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	struct acpi_resource_gpio *agpio;
 	u8 active_value, pin, type;
 	union acpi_object *obj;
+	struct gpio_desc *gpio;
 	const char *err_msg;
 	const char *func;
 	u32 polarity;
@@ -244,22 +245,38 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
-		ret = skl_int3472_register_gpio_clock(int3472, agpio, polarity);
-		if (ret)
-			err_msg = "Failed to register clock\n";
-
-		break;
 	case INT3472_GPIO_TYPE_PRIVACY_LED:
-		ret = skl_int3472_register_pled(int3472, agpio, polarity);
-		if (ret)
-			err_msg = "Failed to register LED\n";
-
-		break;
 	case INT3472_GPIO_TYPE_POWER_ENABLE:
-		ret = skl_int3472_register_regulator(int3472, agpio);
-		if (ret)
-			err_msg = "Failed to map regulator to sensor\n";
+		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, func, polarity);
+		if (IS_ERR(gpio)) {
+			ret = PTR_ERR(gpio);
+			err_msg = "Failed to get GPIO\n";
+			break;
+		}
 
+		switch (type) {
+		case INT3472_GPIO_TYPE_CLK_ENABLE:
+			ret = skl_int3472_register_gpio_clock(int3472, gpio);
+			if (ret)
+				err_msg = "Failed to register clock\n";
+
+			break;
+		case INT3472_GPIO_TYPE_PRIVACY_LED:
+			ret = skl_int3472_register_pled(int3472, gpio);
+			if (ret)
+				err_msg = "Failed to register LED\n";
+
+			break;
+		case INT3472_GPIO_TYPE_POWER_ENABLE:
+			ret = skl_int3472_register_regulator(int3472, gpio);
+			if (ret)
+				err_msg = "Failed to map regulator to sensor\n";
+
+			break;
+		default: /* Never reached */
+			ret = -EINVAL;
+			break;
+		}
 		break;
 	default:
 		dev_warn(int3472->dev,
diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c
index bca1ce7d0d0c..476cd637fc51 100644
--- a/drivers/platform/x86/intel/int3472/led.c
+++ b/drivers/platform/x86/intel/int3472/led.c
@@ -16,26 +16,15 @@ static int int3472_pled_set(struct led_classdev *led_cdev,
 	return 0;
 }
 
-int skl_int3472_register_pled(struct int3472_discrete_device *int3472,
-			      struct acpi_resource_gpio *agpio, u32 polarity)
+int skl_int3472_register_pled(struct int3472_discrete_device *int3472, struct gpio_desc *gpio)
 {
-	char *p, *path = agpio->resource_source.string_ptr;
+	char *p;
 	int ret;
 
 	if (int3472->pled.classdev.dev)
 		return -EBUSY;
 
-	int3472->pled.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
-							     "int3472,privacy-led");
-	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);
+	int3472->pled.gpio = gpio;
 
 	/* Generate the name, replacing the ':' in the ACPI devname with '_' */
 	snprintf(int3472->pled.name, sizeof(int3472->pled.name),
-- 
2.41.0

[PATCH v2 4/5] platform/x86: int3472: Switch to devm_get_gpiod()

[Hans de Goede]

Switch to devm_get_gpiod() for discrete GPIOs for clks / regulators / LEDs
and let devm do the cleanup for us.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../x86/intel/int3472/clk_and_regulator.c     | 23 ++++---------------
 drivers/platform/x86/intel/int3472/discrete.c |  2 +-
 drivers/platform/x86/intel/int3472/led.c      |  7 +-----
 3 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 459f96c04ca1..16e36ac0a7b4 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -177,10 +177,8 @@ int skl_int3472_register_gpio_clock(struct int3472_discrete_device *int3472,
 
 	init.name = kasprintf(GFP_KERNEL, "%s-clk",
 			      acpi_dev_name(int3472->adev));
-	if (!init.name) {
-		ret = -ENOMEM;
-		goto out_put_gpio;
-	}
+	if (!init.name)
+		return -ENOMEM;
 
 	int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
 
@@ -206,8 +204,6 @@ int skl_int3472_register_gpio_clock(struct int3472_discrete_device *int3472,
 	clk_unregister(int3472->clock.clk);
 out_free_init_name:
 	kfree(init.name);
-out_put_gpio:
-	gpiod_put(int3472->clock.ena_gpio);
 
 	return ret;
 }
@@ -219,7 +215,6 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
 
 	clkdev_drop(int3472->clock.cl);
 	clk_unregister(int3472->clock.clk);
-	gpiod_put(int3472->clock.ena_gpio);
 }
 
 /*
@@ -266,7 +261,7 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	struct regulator_config cfg = { };
 	const char *second_sensor = NULL;
 	const struct dmi_system_id *id;
-	int i, j, ret;
+	int i, j;
 
 	id = dmi_first_match(skl_int3472_regulator_second_sensor);
 	if (id)
@@ -309,21 +304,11 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	int3472->regulator.rdev = regulator_register(int3472->dev,
 						     &int3472->regulator.rdesc,
 						     &cfg);
-	if (IS_ERR(int3472->regulator.rdev)) {
-		ret = PTR_ERR(int3472->regulator.rdev);
-		goto err_free_gpio;
-	}
 
-	return 0;
-
-err_free_gpio:
-	gpiod_put(int3472->regulator.gpio);
-
-	return ret;
+	return PTR_ERR_OR_ZERO(int3472->regulator.rdev);
 }
 
 void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472)
 {
 	regulator_unregister(int3472->regulator.rdev);
-	gpiod_put(int3472->regulator.gpio);
 }
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index eb0cded5b92a..8171b16882b7 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -119,7 +119,7 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
 		return ERR_PTR(ret);
 
 	gpiod_add_lookup_table(lookup);
-	desc = gpiod_get(int3472->dev, func, GPIOD_OUT_LOW);
+	desc = devm_gpiod_get(int3472->dev, func, GPIOD_OUT_LOW);
 	gpiod_remove_lookup_table(lookup);
 
 	return desc;
diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c
index 476cd637fc51..9cbed694e2ca 100644
--- a/drivers/platform/x86/intel/int3472/led.c
+++ b/drivers/platform/x86/intel/int3472/led.c
@@ -39,7 +39,7 @@ int skl_int3472_register_pled(struct int3472_discrete_device *int3472, struct gp
 
 	ret = led_classdev_register(int3472->dev, &int3472->pled.classdev);
 	if (ret)
-		goto err_free_gpio;
+		return ret;
 
 	int3472->pled.lookup.provider = int3472->pled.name;
 	int3472->pled.lookup.dev_id = int3472->sensor_name;
@@ -47,10 +47,6 @@ int skl_int3472_register_pled(struct int3472_discrete_device *int3472, struct gp
 	led_add_lookup(&int3472->pled.lookup);
 
 	return 0;
-
-err_free_gpio:
-	gpiod_put(int3472->pled.gpio);
-	return ret;
 }
 
 void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
@@ -60,5 +56,4 @@ void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
 
 	led_remove_lookup(&int3472->pled.lookup);
 	led_classdev_unregister(&int3472->pled.classdev);
-	gpiod_put(int3472->pled.gpio);
 }
-- 
2.41.0

[PATCH v2 5/5] gpio: acpi: remove acpi_get_and_request_gpiod()

[Hans de Goede]

With no more users, we can remove acpi_get_and_request_gpiod().

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Link: https://lore.kernel.org/r/20230926145943.42814-5-brgl@bgdev.pl
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib-acpi.c   | 28 ----------------------------
 include/linux/gpio/consumer.h |  8 --------
 2 files changed, 36 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 17a86bdd9609..89ff93f3b579 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -158,34 +158,6 @@ static struct gpio_desc *acpi_get_gpiod(char *path, unsigned int pin)
 	return gpiochip_get_desc(chip, pin);
 }
 
-/**
- * acpi_get_and_request_gpiod - Translate ACPI GPIO pin to GPIO descriptor and
- *                              hold a refcount to the GPIO device.
- * @path:      ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
- * @pin:       ACPI GPIO pin number (0-based, controller-relative)
- * @label:     Label to pass to gpiod_request()
- *
- * This function is a simple pass-through to acpi_get_gpiod(), except that
- * as it is intended for use outside of the GPIO layer (in a similar fashion to
- * gpiod_get_index() for example) it also holds a reference to the GPIO device.
- */
-struct gpio_desc *acpi_get_and_request_gpiod(char *path, unsigned int pin, char *label)
-{
-	struct gpio_desc *gpio;
-	int ret;
-
-	gpio = acpi_get_gpiod(path, pin);
-	if (IS_ERR(gpio))
-		return gpio;
-
-	ret = gpiod_request(gpio, label);
-	if (ret)
-		return ERR_PTR(ret);
-
-	return gpio;
-}
-EXPORT_SYMBOL_GPL(acpi_get_and_request_gpiod);
-
 static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
 {
 	struct acpi_gpio_event *event = data;
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 1c4385a00f88..9d1f598b8971 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -614,8 +614,6 @@ void acpi_dev_remove_driver_gpios(struct acpi_device *adev);
 int devm_acpi_dev_add_driver_gpios(struct device *dev,
 				   const struct acpi_gpio_mapping *gpios);
 
-struct gpio_desc *acpi_get_and_request_gpiod(char *path, unsigned int pin, char *label);
-
 #else  /* CONFIG_GPIOLIB && CONFIG_ACPI */
 
 #include <linux/err.h>
@@ -633,12 +631,6 @@ static inline int devm_acpi_dev_add_driver_gpios(struct device *dev,
 	return -ENXIO;
 }
 
-static inline struct gpio_desc *acpi_get_and_request_gpiod(char *path, unsigned int pin,
-							   char *label)
-{
-	return ERR_PTR(-ENOSYS);
-}
-
 #endif /* CONFIG_GPIOLIB && CONFIG_ACPI */
 
 
-- 
2.41.0

Re: [PATCH v2 5/5] gpio: acpi: remove acpi_get_and_request_gpiod()

[Andy Shevchenko]

On Thu, Sep 28, 2023 at 02:45:51PM +0200, Hans de Goede wrote:
> With no more users, we can remove acpi_get_and_request_gpiod().

> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Link: https://lore.kernel.org/r/20230926145943.42814-5-brgl@bgdev.pl
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Authorship?

Missing Mika's tag?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/5] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Bartosz Golaszewski]

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

Re: [PATCH v2 0/5] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Hans de Goede]

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

Re: [PATCH v2 0/5] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Hans de Goede]

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

Re: [PATCH v2 0/5] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Bartosz Golaszewski]

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

Re: [PATCH v2 0/5] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Ilpo Järvinen]

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.


-- 
 i.

Re: [PATCH v2 0/5] platform/x86: int3472: don't use gpiod_toggle_active_low()

[Andy Shevchenko]

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...

-- 
With Best Regards,
Andy Shevchenko