On Wed, Sep 28, 2022 at 12:00:51PM +0100, Daniel Thompson wrote:
> On Tue, Sep 27, 2022 at 03:32:35PM -0700, Dmitry Torokhov wrote:
> > Properties describing GPIOs should be named as "<property>-gpios" or
> > "<property>-gpio", and that is what gpiod API expects, however the
> > driver uses non-standard "gpios-reset" name. Let's adjust this, and also
> > note that the reset line is active low as that is also important to
> > gpiod API.
>
> No objections to the goal but...
>
>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > ---
> >
> > Another option is to add another quirk into gpiolib-of.c, but we
> > may end up with a ton of them once we convert everything away from
> > of_get_named_gpio() to gpiod API, so I'd prefer not doing that.
>
> ... it is unusual to permit backwards incompatible changes to the DT
> bindings[1]: creating "flag days" where hardware stops functioning if
> you boot an new kernel with an old DT is a known annoyance to users.
>
> I usually favour quirks tables or similar[2] rather than break legacy
> DTs. Very occasionally I accept (believable) arguments that no legacy
> DTs actually exist but that can very difficult to verify.
>
> Overall I'd like to solicit views from both GPIO and DT maintainers
> before rejecting quirks tables as a way to help smooth these sort of
> changes (or links to ML archives if this has already been discussed).
I believe I was able to convince Rob once or twice that keeping
compatibility was not worth it (not in general but in a couple of
concrete cases), at least while device tree bindings are part of the
kernel. Can't find the emails though...
I think we should consider several options:
1. DTS/DTB is in firmware. In this case absolutely, we need to keep
binary compatibility as we can not expect users to reflash firmware
(there might not even be a new firmware). This situation matches what we
have with ACPI systems where we are trying to work around issues
2. DTS is shipped with the kernel:
2a. DTS is present in upstream kernel - awesome, we can patch it
as needed and have one less thing to worry about.
2b. DTS is not upstream. Vendor did not bother sending it. In
this case it is extremely unlikely that an upstream kernel
will work on such system out of the box, and updating the
kernel is a large engineering task where you pull down new
kernel, rework and apply non-upstream patches, rework kernel
config (new Kconfig options can be introduced, old options
can be renamed, etc). And then spend several weeks
stabilizing the system and tracking down regressions (in
general, not DTS-related ones)
3. DTS is not in firmware and not in kernel. Are there such systems?
So my opinion is that while device trees are part of kernel code and
have not been split into a separate project they are a fair game. If the
change can be handled in the driver without much effort (something like
"wakeup-source" vs "linux,wakeup" vs "linux,keypad-wakeup") - fine, we
can just put a tiny quirk in the driver, but if we need something more
substantial we need to think long and hard if we should implement a
fallback and how much effort there is to maintain/test it so it does not
bitrot.
Anyway, I hope Rob, Linux and Krzysztof to chime in on this exciting
topic once again ;)
>
> [1] For this particular driver the situation is muddied slightly
> because it looks like complex since it looks the bindings for
> himax,hx8357 and himax,hx8369 are undocumented (and badly named).
>
> [2] When the property is not parsed by library code mostly we handle
> legacy by consuming both new or old names in the parser code.
>
>
> > diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
> > index 9b50bc96e00f..41332f48b2df 100644
> > --- a/drivers/video/backlight/hx8357.c
> > +++ b/drivers/video/backlight/hx8357.c
> > @@ -601,7 +601,7 @@ static int hx8357_probe(struct spi_device *spi)
> > if (!match || !match->data)
> > return -EINVAL;
> >
> > - lcd->reset = of_get_named_gpio(spi->dev.of_node, "gpios-reset", 0);
> > + lcd->reset = of_get_named_gpio(spi->dev.of_node, "reset-gpios", 0);
> > if (!gpio_is_valid(lcd->reset)) {
> > dev_err(&spi->dev, "Missing dt property: gpios-reset\n");
> > return -EINVAL;
>
> Daniel.
Thanks.
--
Dmitry
