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@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> >>
> >> Hi Bartosz,
> >>
> >> On 9/26/23 16:59, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >>>
> >>> 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