Re: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Apr 12, 2024 at 5:25 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, Apr 12, 2024 at 10:20:24AM +0200, Linus Walleij wrote:
> > On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko
> > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> >
> > > The GPIO_* flag definitions are *almost* duplicated in two files
> > > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies
> > > on one set of definitions while the rest is on the other. Clean up
> > > this mess by providing only one source of the definitions to all.
> > >
> > > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors")
> > > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own")
> > > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent")
> > > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high")
> > > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags")
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> >
> > The way the line lookup flags ("lflags") were conceived was through
> > support for non-DT systems using descriptor tables, and that is how
> > enum gpio_lookup_flags came to be.
> >
> > When OF support was added it was bolted on on the side, in essence
> > assuming that the DT/OF ABI was completely separate (and they/we
> > sure like to think about it that way...) and thus needed translation from
> > OF flags to kernel-internal enum gpio_lookup_flags.
> >
> > The way *I* thought about this when writing it was certainly that the
> > DT bindings was a separate thing (<dt-bindings/*.h> didn't even exist
> > at the time I think) and that translation from OF to kernel-internal
> > lflags would happen in *one* place.
> >
> > The main reasoning still holds: the OF define is an ABI, so it can
> > *never* be changed, but the enum gpio_lookup_flags is subject to
> > Documentation/process/stable-api-nonsense.rst and that means
> > that if we want to swap around the order of the definitions we can.
> >
> > But admittedly this is a bit over-belief in process and separation of
> > concerns and practical matters may be something else...
>
> Got it. But we have a name clash and the mess added to the users.
> I can redo this to separate these entities.
>
> Note, that there is code in the kernel that *does* use
> #include <dt-bindings/*.h>
> for Linux internals.
>

Well, then they are wrong. We should convert them to using
linux/gpio/machine.h first. Or even put these defines elsewhere
depending on what these drivers are using it for in general. Maybe
machine.h is not the right place. Then once that's figured out, we can
start renaming the constants.

IIUC include/dt-bindings/ headers should only be used by DT sources
and code that parses the OF properties.

But it seems to me that we need to inspect these users, we cannot just
automatically convert them at once, this is asking for trouble IMO.

Bart