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
