From: Bartosz Golaszewski <[email protected]>
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
On Tue, Sep 26, 2023 at 04:59:39PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
--
With Best Regards,
Andy Shevchenko
From: Bartosz Golaszewski <[email protected]>
Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use
temporary lookup tables with appropriate lookup flags.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
Hi,
On 9/26/23 16:59, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use
> temporary lookup tables with appropriate lookup flags.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> 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));
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 <[email protected]>
>>
>> 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(-)
>>
Hi Bartosz,
On 9/27/23 10:48, Bartosz Golaszewski wrote:
> On Wed, Sep 27, 2023 at 10:38 AM Hans de Goede <[email protected]> wrote:
>>
>> Hi Bartosz,
>>
>> On 9/26/23 16:59, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <[email protected]>
>>>
>>> gpiod_toggle_active_low() is a badly designed API that should have never
>>> been used elsewhere then in the MMC code. And even there we should find
>>> a better solution.
>>>
>>> Replace the uses of it in the int3472 driver with the good old temporary
>>> lookup table trick. This is not very pretty either but it's the lesser
>>> evil.
>>
>> I saw your previous proposal which added a new api to directly set
>> the active_low flag, rather then toggle it.
>>
>> I intended to reply to that thread to say that I liked that approach,
>> but I don't remember if I actually did reply.
>>
>> I wonder what made you abandon the new function to directly set
>> the active-low flag on a gpio_desc?
>>
>> For the int3472 code that would work pretty well and it would
>> be much cleaner then the temp gpio-lookup approach.
>>
>
> You did reply, yes. Under one of the other patches Linus W stated that
> first: adding the ability for consumers to toggle the polarity was
> added to handle the MMC slot quirk, then it was used unknowingly to
> GPIO maintainers in other places (including this driver). I then
> acknowledged the fact that it should have never existed in the first
> place as this is HW description and should be defined in ACPI, DT or
> lookup flags.
I see and I understand.
> I'm not sure why this information needs to be hard-coded in the driver
> in int3472_get_func_and_polarity() but maybe it could be pulled into
> gpiolib-acpi.c with other quirks?
The problem is that for camera sensors Intel uses this special
INT3472 ACPI device with a custom _DSM to list GPIOs, with the _DSM
returning an u32 and one of the bits in the u32 is the polarity.
We really do not want to deal with this Intel camera team hack
inside gpiolib-acpi and I can understand why you and Linus W
want to get rid of functions which allow drivers to meddle
with a gpio_desc's active-low flag.
So using a temporary gpio-lookup in the int3472 code as
you are proposing is the best (least bad) thing to do
here then.
I'll try to make some time to test this sometime
the coming days.
Other then the discussion we just had is there any specific
reason why this should be considered a RFC / why this would
not be ready for merging? (I still need to review these,
but lets assume that goes well)
Regards,
Hans
Hi Bart,
On 9/27/23 12:44, Bartosz Golaszewski wrote:
> On Wed, Sep 27, 2023 at 11:40 AM Hans de Goede <[email protected]> wrote:
>>
>> Hi,
>>
>> On 9/26/23 16:59, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <[email protected]>
>>>
>>> Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use
>>> temporary lookup tables with appropriate lookup flags.
>>>
>>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>>> ---
>>> 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));
>>
>
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 <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>> On 9/26/23 16:59, Bartosz Golaszewski wrote:
>>>> From: Bartosz Golaszewski <[email protected]>
>>>>
>>>> Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use
>>>> temporary lookup tables with appropriate lookup flags.
>>>>
>>>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>>>> ---
>>>> drivers/platform/x86/intel/int3472/led.c | 12 ++++--------
>>>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c
>>>> index bca1ce7d0d0c..62e0cd5207a7 100644
>>>> --- a/drivers/platform/x86/intel/int3472/led.c
>>>> +++ b/drivers/platform/x86/intel/int3472/led.c
>>>> @@ -25,18 +25,14 @@ int skl_int3472_register_pled(struct int3472_discrete_device *int3472,
>>>> if (int3472->pled.classdev.dev)
>>>> return -EBUSY;
>>>>
>>>> - int3472->pled.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
>>>> - "int3472,privacy-led");
>>>> + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup(
>>>> + int3472->dev, path, agpio->pin_table[0],
>>>> + "int3472,privacy-led", polarity,
>>>> + GPIOD_OUT_LOW);
>>>
>>> Yeah so this is not going to work, path here is an ACPI device path, e.g.
>>> on my laptop (which actually uses the INT3472 glue code) the path-s of
>>> the 2 GPIO controllers are: `\_SB_.GPI0` resp `\_SB_.PC00.XHCI.RHUB.HS08.VGPO`
>>>
>>> Where as skl_int3472_gpiod_get_from_temp_lookup() stores the passed in path
>>> in gpiod_lookup_table.table[0].key, which is the dev_name() of the GPIO
>>> controller's parent dev which are `INTC1055:00` resp. `INTC1096:00` .
>>>
>>> So we are going to need to add some code to INT3472 to go from path to
>>> a correct value for gpiod_lookup_table.table[0].key which means partly
>>> reproducing most of acpi_get_gpiod:
>>>
>>> struct gpio_chip *chip;
>>> acpi_handle handle;
>>> acpi_status status;
>>>
>>> status = acpi_get_handle(NULL, path, &handle);
>>> if (ACPI_FAILURE(status))
>>> return ERR_PTR(-ENODEV);
>>>
>>> chip = gpiochip_find(handle, acpi_gpiochip_find);
>>> if (!chip)
>>> return ERR_PTR(-EPROBE_DEFER);
>>>
>>> And then get the key from the chip. Which means using gpiochip_find
>>> in the int3472 code now, which does not sound like an improvement.
>>>
>>> I think that was is needed instead is adding an active_low flag
>>> to acpi_get_and_request_gpiod() and then have that directly
>>> set the active-low flag on the returned desc.
>>>
>>
>> Ultimately I'd like everyone to use gpiod_get() for getting
>> descriptors but for now I get it's enough. Are you find with this
>> being done in a single patch across GPIO and this driver?
>
> Yes doing this in a single patch is fine.
>
> Also I'm fine with merging such a patch through the gpio tree .
So thinking about this more I realized that the int3472 code already
generates GPIO lookups for the sensor device for some
(powerdown, reset) GPIOs, it only needs the gpio_desc for
the case where the GPIO is turned into a regulator, clock or led.
Since the int3472 code is already generating lookups it already
has a way to go from path to a lookup "key":
status = acpi_get_handle(NULL, path, &handle);
if (ACPI_FAILURE(status))
return -EINVAL;
adev = acpi_fetch_acpi_dev(handle);
if (!adev)
return -ENODEV;
table_entry->key = acpi_dev_name(adev);
So we can get the key without needing to call gpio_find_chip()
So I do believe that the temp lookup approach should actually
work. I'm currently traveling, so no promises but I should
be able to rework your series in something which actually
works and which will:
1. Stop using gpiod_toggle_active_low()
2. Allow dropping acpi_get_and_request_gpiod()
So no need for a patch to add an active-low parameter to
acpi_get_and_request_gpiod(), sorry about the noise.
Regards,
Hans
>>>> if (IS_ERR(int3472->pled.gpio))
>>>> return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio),
>>>> "getting privacy LED GPIO\n");
>>>>
>>>> - if (polarity == GPIO_ACTIVE_LOW)
>>>> - gpiod_toggle_active_low(int3472->pled.gpio);
>>>> -
>>>> - /* Ensure the pin is in output mode and non-active state */
>>>> - gpiod_direction_output(int3472->pled.gpio, 0);
>>>> -
>>>> /* Generate the name, replacing the ':' in the ACPI devname with '_' */
>>>> snprintf(int3472->pled.name, sizeof(int3472->pled.name),
>>>> "%s::privacy_led", acpi_dev_name(int3472->sensor));
>>>
>>
On Wed, Sep 27, 2023 at 10:38 AM Hans de Goede <[email protected]> wrote:
>
> Hi Bartosz,
>
> On 9/26/23 16:59, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > 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
On Wed, Sep 27, 2023 at 11:02 AM Hans de Goede <[email protected]> wrote:
>
> Hi Bartosz,
>
> On 9/27/23 10:48, Bartosz Golaszewski wrote:
> > On Wed, Sep 27, 2023 at 10:38 AM Hans de Goede <[email protected]> wrote:
> >>
> >> Hi Bartosz,
> >>
> >> On 9/26/23 16:59, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <[email protected]>
> >>>
> >>> 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
Hi Bartosz,
On 9/26/23 16:59, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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(-)
>
On Wed, Sep 27, 2023 at 11:40 AM Hans de Goede <[email protected]> wrote:
>
> Hi,
>
> On 9/26/23 16:59, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use
> > temporary lookup tables with appropriate lookup flags.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > ---
> > 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));
>
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
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 <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
.../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
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 <[email protected]>
But please fix tags here and there...
--
With Best Regards,
Andy Shevchenko