| Message ID | 20230926145943.42814-3-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/x86: int3472: don't use gpiod_toggle_active_low() | expand |
On Tue, Sep 26, 2023 at 04:59:41PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use > temporary lookup tables with appropriate lookup flags. ... > + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( > + int3472->dev, path, agpio->pin_table[0], > + "int3472,privacy-led", polarity, > + GPIOD_OUT_LOW); Personally I found this style weird. I prefer to have longer line over the split on the parentheses.
On Tue, Sep 26, 2023 at 5:27 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Sep 26, 2023 at 04:59:41PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Instead of acpi_get_and_request_gpiod() + gpiod_toggle_active_low(), use > > temporary lookup tables with appropriate lookup flags. > > ... > > > + int3472->pled.gpio = skl_int3472_gpiod_get_from_temp_lookup( > > + int3472->dev, path, agpio->pin_table[0], > > + "int3472,privacy-led", polarity, > > + GPIOD_OUT_LOW); > > Personally I found this style weird. I prefer to have longer line over > the split on the parentheses. > I in turn prefer this one. Checkpatch doesn't complain either way so I'll leave it to the maintainers of this driver to decide. Bart
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
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));
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)); >
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)); >> >
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)); >>> >>
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));