diff mbox series

[RFC/PATCH] backlight: hx8357: prepare to conversion to gpiod API

Message ID YzN6A9Y20Ea1LdEz@google.com (mailing list archive)
State New, archived
Headers show
Series [RFC/PATCH] backlight: hx8357: prepare to conversion to gpiod API | expand

Commit Message

Dmitry Torokhov Sept. 27, 2022, 10:32 p.m. UTC
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.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

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.

 arch/arm/boot/dts/imx28-cfa10049.dts | 7 +++++--
 arch/arm/boot/dts/imx28-cfa10055.dts | 3 ++-
 arch/arm/boot/dts/imx28-cfa10056.dts | 3 ++-
 drivers/video/backlight/hx8357.c     | 2 +-
 4 files changed, 10 insertions(+), 5 deletions(-)

Comments

Daniel Thompson Sept. 28, 2022, 11 a.m. UTC | #1
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@gmail.com>
> ---
>
> 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).

[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.
Dmitry Torokhov Sept. 28, 2022, 6:33 p.m. UTC | #2
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@gmail.com>
> > ---
> >
> > 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.
Daniel Thompson Oct. 3, 2022, 1:32 p.m. UTC | #3
On Wed, Sep 28, 2022 at 11:33:52AM -0700, Dmitry Torokhov wrote:
> 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@gmail.com>
> > > ---
> > >
> > > 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:

I have to note that these are *non-exclusive* 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.

I don't think the presence of a DT within the kernel can be the basis
for any useful reasoning.

a. "Better" firmware projects aimed are likely to consume a DT that is
   shipped with the kernel and pin it (meaning the kernel cannot solve
   version skew problems by updating it's copies of the DT). I think
   tow-boot to be a specific example of this.

b. The fact there are are consumers of the binding shipped with the
   kernel isn't sufficient to show that *all* consumers of the binding
   are shipped with the kernel.

On other words I don't think the presence of a DT in the kernel is
especially useful to showing that neither #1 nor #3 apply.


> 	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?

DT overlays strike me are an example of this case. I'm particularly
thinking of daughterboard/expansion card examples here where the DT
overlay could be any several different places: firmware, an add on
boards I2C FLASH, daughterboard documentation, blog posts, etc.

That is especially relevant to this specific patch since HX8357 is found
on several widely available add-on boards.


> 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.

To be honest my original thoughts were that for simple renames, a rename
quirk table shared by all renames needed to introduce libgpiod would
probably have a lower impact than all the "tiny" per-driver quirks (because
it could share code across multiple names).


> Anyway, I hope Rob, Linux and Krzysztof to chime in on this exciting
> topic once again ;)

I'm especially interested in a gpiod point of view! I have invested
quite a few characters in this thread. That is because, if accepted,
adding strings to a quirks table is much less effort for patch
submitters than having to demonstrate on a per-patch basis the due
diligence that has been undertaken to show that cases #1 and #3 do not
apply to the particular rename being sought.


Daniel.
Linus Walleij Oct. 4, 2022, 9:02 a.m. UTC | #4
On Wed, Sep 28, 2022 at 12:32 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> 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.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

I think the gods of Open Firmware will try to punish you for such
incompatible changes. But I have long since renounced them.

> 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.

We need to know if i.MX is shipping device trees stored in flash,
or if they bundle it with the kernel.

In the former case, you have to add quirks, in the latter case this
patch is fine.

Sascha, what does the Freescale maintainer say?

Yours,
Linus Walleij
Daniel Thompson Oct. 4, 2022, 12:54 p.m. UTC | #5
On Tue, Oct 04, 2022 at 11:02:06AM +0200, Linus Walleij wrote:
> On Wed, Sep 28, 2022 at 12:32 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> 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.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> I think the gods of Open Firmware will try to punish you for such
> incompatible changes. But I have long since renounced them.
>
> > 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.
>
> We need to know if i.MX is shipping device trees stored in flash,
> or if they bundle it with the kernel.

This part is frequently found in add-on boards so it's not purely an
i.MX-only question.


> In the former case, you have to add quirks, in the latter case this
> patch is fine.
>
> Sascha, what does the Freescale maintainer say?

IMHO for not-in-the-soc devices like this the presence of in-kernel DTs
isn't enough to make a decision. What is needed is a degree of
due-diligence to show that there are no obvious out-of-kernel users.

To be honest, I suspect the due-diligence checks will probably yield a
green light for this one. Most of the tutorials for the popular HX8357
devices, show how to run python code in userspace that sends raw SPI
commands. That sucks but at least it doesn't raise any concerns about
bindings maintenance.


Daniel.
Linus Walleij Oct. 4, 2022, 7:50 p.m. UTC | #6
On Tue, Oct 4, 2022 at 2:54 PM Daniel Thompson
<daniel.thompson@linaro.org> wrote:

> > We need to know if i.MX is shipping device trees stored in flash,
> > or if they bundle it with the kernel.
>
> This part is frequently found in add-on boards so it's not purely an
> i.MX-only question.

Oh

> IMHO for not-in-the-soc devices like this the presence of in-kernel DTs
> isn't enough to make a decision. What is needed is a degree of
> due-diligence to show that there are no obvious out-of-kernel users.

OK I think this is a good case to use a special quirk in this case.
I actually envisioned keeping piling them up, and that they would
not be innumerable.

Dmitry, could you fix this? Just patch away in gpiolib-of.c.

Yours,
Linus Walleij
Dmitry Torokhov Oct. 4, 2022, 8:35 p.m. UTC | #7
On Tue, Oct 04, 2022 at 09:50:25PM +0200, Linus Walleij wrote:
> On Tue, Oct 4, 2022 at 2:54 PM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> 
> > > We need to know if i.MX is shipping device trees stored in flash,
> > > or if they bundle it with the kernel.
> >
> > This part is frequently found in add-on boards so it's not purely an
> > i.MX-only question.
> 
> Oh
> 
> > IMHO for not-in-the-soc devices like this the presence of in-kernel DTs
> > isn't enough to make a decision. What is needed is a degree of
> > due-diligence to show that there are no obvious out-of-kernel users.
> 
> OK I think this is a good case to use a special quirk in this case.
> I actually envisioned keeping piling them up, and that they would
> not be innumerable.
> 
> Dmitry, could you fix this? Just patch away in gpiolib-of.c.

Sure, I'll add a few quirks. I wonder what is the best way to merge
this? I can create a bunch of IBs to be pulled, or I can send quirks to
you/Bartosz and once they land send the patches to drivers...

Thanks.
Linus Walleij Oct. 6, 2022, 9:03 a.m. UTC | #8
On Tue, Oct 4, 2022 at 10:35 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> > Dmitry, could you fix this? Just patch away in gpiolib-of.c.
>
> Sure, I'll add a few quirks. I wonder what is the best way to merge
> this? I can create a bunch of IBs to be pulled, or I can send quirks to
> you/Bartosz and once they land send the patches to drivers...

When I did it I was sufficiently convinced that I was the only one patching
the quirks in gpiolib-of.c that merge window so I just included it as
a hunk in the driver patch. If there will be some more patches to that
file I guess some separate patch(es) for gpiolib-of.c is needed, maybe
an immutable branch for those if it becomes a lot.

Yours,
Linus Walleij
Daniel Thompson Oct. 6, 2022, 10:04 a.m. UTC | #9
On Thu, Oct 06, 2022 at 11:03:15AM +0200, Linus Walleij wrote:
> On Tue, Oct 4, 2022 at 10:35 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>
> > > Dmitry, could you fix this? Just patch away in gpiolib-of.c.
> >
> > Sure, I'll add a few quirks. I wonder what is the best way to merge
> > this? I can create a bunch of IBs to be pulled, or I can send quirks to
> > you/Bartosz and once they land send the patches to drivers...
>
> When I did it I was sufficiently convinced that I was the only one patching
> the quirks in gpiolib-of.c that merge window so I just included it as
> a hunk in the driver patch. If there will be some more patches to that
> file I guess some separate patch(es) for gpiolib-of.c is needed, maybe
> an immutable branch for those if it becomes a lot.

Are renames likely to be a common quirk on the road to libgpiod
conversion?

I admit I sort of expected it to be common enough that there would be
one rename quirk in the code supported by an alphabetized string table.
Such a table would certainly still provoke troublesome merges but ones
that are trivially resolved.


Daniel.
Linus Walleij Oct. 10, 2022, 8:36 p.m. UTC | #10
On Thu, Oct 6, 2022 at 12:05 PM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
> On Thu, Oct 06, 2022 at 11:03:15AM +0200, Linus Walleij wrote:
> > On Tue, Oct 4, 2022 at 10:35 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >
> > > > Dmitry, could you fix this? Just patch away in gpiolib-of.c.
> > >
> > > Sure, I'll add a few quirks. I wonder what is the best way to merge
> > > this? I can create a bunch of IBs to be pulled, or I can send quirks to
> > > you/Bartosz and once they land send the patches to drivers...
> >
> > When I did it I was sufficiently convinced that I was the only one patching
> > the quirks in gpiolib-of.c that merge window so I just included it as
> > a hunk in the driver patch. If there will be some more patches to that
> > file I guess some separate patch(es) for gpiolib-of.c is needed, maybe
> > an immutable branch for those if it becomes a lot.
>
> Are renames likely to be a common quirk on the road to libgpiod
> conversion?
>
> I admit I sort of expected it to be common enough that there would be
> one rename quirk in the code supported by an alphabetized string table.
> Such a table would certainly still provoke troublesome merges but ones
> that are trivially resolved.

Dmitry added a table of sorts, the problems are usually a bit unique
for each instance of nonstandard DT GPIO bindings, that's why I
mostly solved it with open coding in gpiolib-of.c.

Yours,
Linus Walleij
Dmitry Torokhov Oct. 12, 2022, 8:34 p.m. UTC | #11
On Mon, Oct 10, 2022 at 10:36:00PM +0200, Linus Walleij wrote:
> On Thu, Oct 6, 2022 at 12:05 PM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> > On Thu, Oct 06, 2022 at 11:03:15AM +0200, Linus Walleij wrote:
> > > On Tue, Oct 4, 2022 at 10:35 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > > > Dmitry, could you fix this? Just patch away in gpiolib-of.c.
> > > >
> > > > Sure, I'll add a few quirks. I wonder what is the best way to merge
> > > > this? I can create a bunch of IBs to be pulled, or I can send quirks to
> > > > you/Bartosz and once they land send the patches to drivers...
> > >
> > > When I did it I was sufficiently convinced that I was the only one patching
> > > the quirks in gpiolib-of.c that merge window so I just included it as
> > > a hunk in the driver patch. If there will be some more patches to that
> > > file I guess some separate patch(es) for gpiolib-of.c is needed, maybe
> > > an immutable branch for those if it becomes a lot.
> >
> > Are renames likely to be a common quirk on the road to libgpiod
> > conversion?
> >
> > I admit I sort of expected it to be common enough that there would be
> > one rename quirk in the code supported by an alphabetized string table.
> > Such a table would certainly still provoke troublesome merges but ones
> > that are trivially resolved.
> 
> Dmitry added a table of sorts, the problems are usually a bit unique
> for each instance of nonstandard DT GPIO bindings, that's why I
> mostly solved it with open coding in gpiolib-of.c.

OK, so I sent out the patch adding "reset-gpios" -> "gpios-reset"
translation quirk to keep compatibility with the legacy names, but
we still need to figure out what to do with incorrect line polarity
in current DTses in tree. Unlike with names we have no indication
if out of tree DTSes specify correct polarity or not, so we can't
reasonably come up with workarounds that are guaranteed to work for
everyone. I see several options:

- the driver could continue ignoring line polarity specified in DTS
  (and use gpiod_set_value_raw()) and hope that they all/will be
  wired in the same way?

- we could fix up in-kernel DTSes, allow flexibility of connection
  in new designs and respect polarity specified in out of tree DTSes,
  but accept that there can be breakages with old DTSes not contributed
  to the mainline or DTSes that were "cached" from an older kernel
  release

- add another quirk forcing "active low" polarity for the legacy
  "gpios-reset" name, which will allow us respecting polarity in
  new "reset-gpios" name.

Thanks.
Daniel Thompson Oct. 13, 2022, 12:43 p.m. UTC | #12
On Wed, Oct 12, 2022 at 01:34:38PM -0700, Dmitry Torokhov wrote:
> On Mon, Oct 10, 2022 at 10:36:00PM +0200, Linus Walleij wrote:
> > On Thu, Oct 6, 2022 at 12:05 PM Daniel Thompson
> > <daniel.thompson@linaro.org> wrote:
> > > On Thu, Oct 06, 2022 at 11:03:15AM +0200, Linus Walleij wrote:
> > > > On Tue, Oct 4, 2022 at 10:35 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > > > Dmitry, could you fix this? Just patch away in gpiolib-of.c.
> > > > >
> > > > > Sure, I'll add a few quirks. I wonder what is the best way to merge
> > > > > this? I can create a bunch of IBs to be pulled, or I can send quirks to
> > > > > you/Bartosz and once they land send the patches to drivers...
> > > >
> > > > When I did it I was sufficiently convinced that I was the only one patching
> > > > the quirks in gpiolib-of.c that merge window so I just included it as
> > > > a hunk in the driver patch. If there will be some more patches to that
> > > > file I guess some separate patch(es) for gpiolib-of.c is needed, maybe
> > > > an immutable branch for those if it becomes a lot.
> > >
> > > Are renames likely to be a common quirk on the road to libgpiod
> > > conversion?
> > >
> > > I admit I sort of expected it to be common enough that there would be
> > > one rename quirk in the code supported by an alphabetized string table.
> > > Such a table would certainly still provoke troublesome merges but ones
> > > that are trivially resolved.
> >
> > Dmitry added a table of sorts, the problems are usually a bit unique
> > for each instance of nonstandard DT GPIO bindings, that's why I
> > mostly solved it with open coding in gpiolib-of.c.
>
> OK, so I sent out the patch adding "reset-gpios" -> "gpios-reset"
> translation quirk to keep compatibility with the legacy names, but
> we still need to figure out what to do with incorrect line polarity
> in current DTses in tree. Unlike with names we have no indication
> if out of tree DTSes specify correct polarity or not, so we can't
> reasonably come up with workarounds that are guaranteed to work for
> everyone. I see several options:
>
> 1 the driver could continue ignoring line polarity specified in DTS
>   (and use gpiod_set_value_raw()) and hope that they all/will be
>   wired in the same way?
>
> 2 we could fix up in-kernel DTSes, allow flexibility of connection
>   in new designs and respect polarity specified in out of tree DTSes,
>   but accept that there can be breakages with old DTSes not contributed
>   to the mainline or DTSes that were "cached" from an older kernel
>   release
>
> 3 add another quirk forcing "active low" polarity for the legacy
>   "gpios-reset" name, which will allow us respecting polarity in
>   new "reset-gpios" name.

I don't think they is any point in having a rename if legacy DTs break
anyway! Thus if there is a rename then there should also be an active low
quirk applied when the old name is used.


Daniel.
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/imx28-cfa10049.dts b/arch/arm/boot/dts/imx28-cfa10049.dts
index 9ef0d567ea48..ae51a2aa2028 100644
--- a/arch/arm/boot/dts/imx28-cfa10049.dts
+++ b/arch/arm/boot/dts/imx28-cfa10049.dts
@@ -3,6 +3,7 @@ 
  * Copyright 2012 Free Electrons
  */
 
+#include <dt-bindings/gpio/gpio.h>
 /*
  * The CFA-10049 is an expansion board for the CFA-10036 module, thus we
  * need to include the CFA-10036 DTS.
@@ -346,8 +347,10 @@  hx8357: hx8357@0 {
 			spi-max-frequency = <100000>;
 			spi-cpol;
 			spi-cpha;
-			gpios-reset = <&gpio3 30 0>;
-			im-gpios = <&gpio5 4 0 &gpio5 5 0 &gpio5 6 0>;
+			reset-gpios = <&gpio3 30 GPIO_ACTIVE_LOW>;
+			im-gpios = <&gpio5 4 GPIO_ACTIVE_HIGH
+				    &gpio5 5 GPIO_ACTIVE_HIGH
+				    &gpio5 6 GPIO_ACTIVE_HIGH>;
 		};
 	};
 
diff --git a/arch/arm/boot/dts/imx28-cfa10055.dts b/arch/arm/boot/dts/imx28-cfa10055.dts
index fac5bbda7a93..70e4dc67f7d2 100644
--- a/arch/arm/boot/dts/imx28-cfa10055.dts
+++ b/arch/arm/boot/dts/imx28-cfa10055.dts
@@ -4,6 +4,7 @@ 
  * 				  Free Electrons
  */
 
+#include <dt-bindings/gpio/gpio.h>
 /*
  * The CFA-10055 is an expansion board for the CFA-10036 module and
  * CFA-10037, thus we need to include the CFA-10037 DTS.
@@ -148,7 +149,7 @@  hx8357: hx8357@0 {
 			spi-max-frequency = <100000>;
 			spi-cpol;
 			spi-cpha;
-			gpios-reset = <&gpio3 30 0>;
+			reset-gpios = <&gpio3 30 GPIO_ACTIVE_LOW>;
 		};
 	};
 
diff --git a/arch/arm/boot/dts/imx28-cfa10056.dts b/arch/arm/boot/dts/imx28-cfa10056.dts
index c5f3337e8b39..687eaa555a15 100644
--- a/arch/arm/boot/dts/imx28-cfa10056.dts
+++ b/arch/arm/boot/dts/imx28-cfa10056.dts
@@ -3,6 +3,7 @@ 
  * Copyright 2013 Free Electrons
  */
 
+#include <dt-bindings/gpio/gpio.h>
 /*
  * The CFA-10055 is an expansion board for the CFA-10036 module and
  * CFA-10037, thus we need to include the CFA-10037 DTS.
@@ -107,7 +108,7 @@  hx8369: hx8369@0 {
 			spi-max-frequency = <100000>;
 			spi-cpol;
 			spi-cpha;
-			gpios-reset = <&gpio3 30 0>;
+			reset-gpios = <&gpio3 30 GPIO_ACTIVE_LOW>;
 		};
 	};
 };
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;