[PATCH v3] dt-bindings: power: supply: bq24735: reverse the polarity of ac-detect

[PATCH v3] dt-bindings: power: supply: bq24735: reverse the polarity of ac-detect

[Peter Rosin]

The ACOK pin on the bq24735 is active-high, of course meaning that when
AC is OK the pin is high. However, all Tegra dts files have incorrectly
specified active-high even though the signal is inverted on the Tegra
boards. This has worked since the Linux driver has also inverted the
meaning of the GPIO. Fix this situation by simply specifying in the
bindings what everybody else agrees on; that the ti,ac-detect-gpios is
active on AC adapter absence.

Signed-off-by: Peter Rosin <peda@axentia.se>
---

Hi!

This patch is the result of this discussion:
http://marc.info/?t=148152531800002

I don't like how it changes the one thing that is seems correct, but
what to do?

Cheers,
Peter

 Documentation/devicetree/bindings/power/supply/ti,bq24735.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt b/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
index 3bf55757ceec..43d56c49455b 100644
--- a/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
+++ b/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
@@ -8,8 +8,9 @@ Optional properties :
  - interrupts : Specify the interrupt to be used to trigger when the AC
    adapter is either plugged in or removed.
  - ti,ac-detect-gpios : This GPIO is optionally used to read the AC adapter
-   presence. This is a Host GPIO that is configured as an input and
-   connected to the bq24735.
+   status. This is a Host GPIO that is configured as an input and
+   connected to the bq24735, typically the ACOK pin (note: the GPIO should
+   be active on AC adapter absence).
  - ti,charge-current : Used to control and set the charging current. This value
    must be between 128mA and 8.128A with a 64mA step resolution. The POR value
    is 0x0000h. This number is in mA (e.g. 8192), see spec for more information
-- 
2.1.4

Re: [PATCH v3] dt-bindings: power: supply: bq24735: reverse the polarity of ac-detect

[Stephen Warren]

On 12/15/2016 05:21 AM, Peter Rosin wrote:
> The ACOK pin on the bq24735 is active-high, of course meaning that when
> AC is OK the pin is high. However, all Tegra dts files have incorrectly
> specified active-high even though the signal is inverted on the Tegra
> boards. This has worked since the Linux driver has also inverted the
> meaning of the GPIO. Fix this situation by simply specifying in the
> bindings what everybody else agrees on; that the ti,ac-detect-gpios is
> active on AC adapter absence.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>
> Hi!
>
> This patch is the result of this discussion:
> http://marc.info/?t=148152531800002
>
> I don't like how it changes the one thing that is seems correct, but
> what to do?

I haven't followed this thread so hopefully what I say is relevant. My 
take is:

If the DT binding is correct or reasonable, keep it.

If the Tegra DTs contain incorrect content, and never worked correctly 
in this aspect, then fix them. We do need to maintain DT 
ABI/compatibility, but I believe only with stuff that actually worked 
correctly. If the DT has a bug, just fix it.

That said, if ti,ac-detect-gpios is describing a host GPIO, then it's 
entirely arbitrary which polarity it should have, i.e. the polarity is 
not something specified by the bq24735 HW. In that case, feel free to 
change either the binding to match the DT or the DT to match the 
binding. Changing the DT to match the binding might still be better 
since there could be other users you're not aware of, and they might 
have written their DT correctly, and you don't want to break them.

Re: [PATCH v3] dt-bindings: power: supply: bq24735: reverse the polarity of ac-detect

[Peter Rosin]

On 2016-12-15 18:32, Stephen Warren wrote:
> On 12/15/2016 05:21 AM, Peter Rosin wrote:
>> The ACOK pin on the bq24735 is active-high, of course meaning that when
>> AC is OK the pin is high. However, all Tegra dts files have incorrectly
>> specified active-high even though the signal is inverted on the Tegra
>> boards. This has worked since the Linux driver has also inverted the
>> meaning of the GPIO. Fix this situation by simply specifying in the
>> bindings what everybody else agrees on; that the ti,ac-detect-gpios is
>> active on AC adapter absence.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>
>> Hi!
>>
>> This patch is the result of this discussion:
>> http://marc.info/?t=148152531800002
>>
>> I don't like how it changes the one thing that is seems correct, but
>> what to do?
> 
> I haven't followed this thread so hopefully what I say is relevant. My 
> take is:
> 
> If the DT binding is correct or reasonable, keep it.
> 
> If the Tegra DTs contain incorrect content, and never worked correctly 
> in this aspect, then fix them. We do need to maintain DT 
> ABI/compatibility, but I believe only with stuff that actually worked 
> correctly. If the DT has a bug, just fix it.
> 
> That said, if ti,ac-detect-gpios is describing a host GPIO, then it's 
> entirely arbitrary which polarity it should have, i.e. the polarity is 
> not something specified by the bq24735 HW. In that case, feel free to 
> change either the binding to match the DT or the DT to match the 
> binding. Changing the DT to match the binding might still be better 
> since there could be other users you're not aware of, and they might 
> have written their DT correctly, and you don't want to break them.

The bindings are fine.

The Tegra dts files are buggy, but the driver is also buggy, so those
two bugs cancel each other. So, the option is to either introduce
regressions by fixing the two bugs thus creating a flag day where
the kernel and dt needs to match. Or, just document what is going on
and change the bindings even if they are not wrong.

I suggest you read the discussion. We covered all this already, and it
is also in the commit message.

Cheers,
peda

Re: [PATCH v3] dt-bindings: power: supply: bq24735: reverse the polarity of ac-detect

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

Le jeudi 15 décembre 2016 à 18:50 +0100, Peter Rosin a écrit :
> The bindings are fine.
> 
> The Tegra dts files are buggy, but the driver is also buggy, so those
> two bugs cancel each other. So, the option is to either introduce
> regressions by fixing the two bugs thus creating a flag day where
> the kernel and dt needs to match. Or, just document what is going on
> and change the bindings even if they are not wrong.

After reading the discussion, I would rather be in favor of fixing the driver
and the tegra dts files, which are both wrong.

Keeping things as-is is very counter-intuitive: the GPIO on nyan boards is
active-low and should be described as such (think of other projects, like
U-Boot, reusing the dts). It's also very counter-intuitive to require that any
new board using that driver use active-low polarity in the GPIO declaration when
the line is really active-high.

So yes, it means that older dtbs won't work with new kernels and vice-versa, but
as it was pointed out, this is a bug fix, not even a cosmetic change.

Is anyone strongly opposed to that solution? I'd really rather see the issue
fixed that way instead of the current proposal (this patch).

I'd also be happy to implement and test that solution on nyans, as I've done
other bq24735-related work for nyans recently.

-- 
Paul Kocialkowski, developer of free digital technology at the lower levels

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] dt-bindings: power: supply: bq24735: reverse the polarity of ac-detect

[Peter Rosin]

On 2017-01-24 17:24, Paul Kocialkowski wrote:
> Le jeudi 15 décembre 2016 à 18:50 +0100, Peter Rosin a écrit :
>> The bindings are fine.
>>
>> The Tegra dts files are buggy, but the driver is also buggy, so those
>> two bugs cancel each other. So, the option is to either introduce
>> regressions by fixing the two bugs thus creating a flag day where
>> the kernel and dt needs to match. Or, just document what is going on
>> and change the bindings even if they are not wrong.
> 
> After reading the discussion, I would rather be in favor of fixing the driver
> and the tegra dts files, which are both wrong.
> 
> Keeping things as-is is very counter-intuitive: the GPIO on nyan boards is
> active-low and should be described as such (think of other projects, like
> U-Boot, reusing the dts). It's also very counter-intuitive to require that any
> new board using that driver use active-low polarity in the GPIO declaration when
> the line is really active-high.

Agreed, it very counter-intuitive. I have a board w/o an invert and it
does look odd with active-low in the dts. It really should be active-high.

The (new) binding helps a bit though.

But that said, for various reasons I ended up not using that binding anyway
and instead rely on polling the internal register for the ACOK bit.

> So yes, it means that older dtbs won't work with new kernels and vice-versa, but
> as it was pointed out, this is a bug fix, not even a cosmetic change.
> 
> Is anyone strongly opposed to that solution? I'd really rather see the issue
> fixed that way instead of the current proposal (this patch).

It's a little bit more than a proposal since it is in linux-next. But not set
in stone of course. I personally do not care as long as it is changed before
hitting Linus' tree as I have no deployed devices. But docs are just that.
Docs. Anything that worked before is going to break with the change you are
proposing. Are you really willing to break who knows how many tegra boards?

What will you do a few months down the line when the regression reports start
to creep in??? Argue about it with Linus? And then revert and create an even
bigger mess? You could skip the arguing step and revert right away, but how
is that helpful?

Or do you somehow know that *all* tegra users will always update their kernel
and dtb in sync, so that regressions will not happen?

> I'd also be happy to implement and test that solution on nyans, as I've done
> other bq24735-related work for nyans recently.

Changing this is trivial. Testing that the change does what it is supposed to
is not the main obstacle...

Cheers,
peda

Re: [PATCH v3] dt-bindings: power: supply: bq24735: reverse the polarity of ac-detect

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 3976 bytes --]

On 25/01/17 13:20, Peter Rosin wrote:
> On 2017-01-24 17:24, Paul Kocialkowski wrote:
> > Le jeudi 15 décembre 2016 à 18:50 +0100, Peter Rosin a écrit :
> > > The bindings are fine.
> > > 
> > > The Tegra dts files are buggy, but the driver is also buggy, so those
> > > two bugs cancel each other. So, the option is to either introduce
> > > regressions by fixing the two bugs thus creating a flag day where
> > > the kernel and dt needs to match. Or, just document what is going on
> > > and change the bindings even if they are not wrong.
> > 
> > After reading the discussion, I would rather be in favor of fixing the
> > driver
> > and the tegra dts files, which are both wrong.
> > 
> > Keeping things as-is is very counter-intuitive: the GPIO on nyan boards is
> > active-low and should be described as such (think of other projects, like
> > U-Boot, reusing the dts). It's also very counter-intuitive to require that
> > any
> > new board using that driver use active-low polarity in the GPIO declaration
> > when
> > the line is really active-high.
> 
> Agreed, it very counter-intuitive. I have a board w/o an invert and it
> does look odd with active-low in the dts. It really should be active-high.
> 
> The (new) binding helps a bit though.

Yeah, it's a solution anyway. I'm mostly worried that it means all other code
(especially not in Linux) using the dts will be plagued by this, which seems a
bit unfair. I still see this as an error not being fixed.

> > Is anyone strongly opposed to that solution? I'd really rather see the issue
> > fixed that way instead of the current proposal (this patch).
> 
> It's a little bit more than a proposal since it is in linux-next. But not set
> in stone of course.

Oh, I thought it was still up for review when sending my previous message.

> I personally do not care as long as it is changed before
> hitting Linus' tree as I have no deployed devices. But docs are just that.
> Docs. Anything that worked before is going to break with the change you are
> proposing. Are you really willing to break who knows how many tegra boards?

From my perspective, fixing counter-intuitive behaviour justifies that.

But you're probably right to point out that others down the line will most
likely prefer to keep the change you introduced. So I'll put it this way:
if everyone agrees that it's okay to break support for older device-trees,
then I'm available to craft a patch. Otherwise, let's just stick with you
changes you got in.

> Or do you somehow know that *all* tegra users will always update their kernel
> and dtb in sync, so that regressions will not happen?

Frankly I've never understood why kernel maintainers expect this is not the
case, and this is quite frequently a pain when trying to address issues such as
this one. While I reckon that device-trees shouldn't be tied to the kernel in
particular (both are independent), I think it's fair to except that users use
device-trees matching the release date of the kernel.

It's usually accepted that projects have dependencies on certain versions of
libraries and explicitly do not support older versions of these. It would be
nice to adopt the same attitude towards device-tree. These are my 2 cents on the
issue, of course this isn't up for me to decide.

> > I'd also be happy to implement and test that solution on nyans, as I've done
> > other bq24735-related work for nyans recently.
> 
> Changing this is trivial. Testing that the change does what it is supposed to
> is not the main obstacle...

Right. Note that I'll be testing your bq24735 patches on nyans soon, so perhaps
I'll have an upcoming series in case I spot any regression.

Cheers!

-- 
Paul Kocialkowski, developer of free digital technology at the lower levels

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] dt-bindings: power: supply: bq24735: reverse the polarity of ac-detect

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5883 bytes --]

On Thu, Jan 26, 2017 at 11:09:52AM +0100, Paul Kocialkowski wrote:
> On 25/01/17 13:20, Peter Rosin wrote:
> > On 2017-01-24 17:24, Paul Kocialkowski wrote:
> > > Le jeudi 15 décembre 2016 à 18:50 +0100, Peter Rosin a écrit :
> > > > The bindings are fine.
> > > > 
> > > > The Tegra dts files are buggy, but the driver is also buggy, so those
> > > > two bugs cancel each other. So, the option is to either introduce
> > > > regressions by fixing the two bugs thus creating a flag day where
> > > > the kernel and dt needs to match. Or, just document what is going on
> > > > and change the bindings even if they are not wrong.
> > > 
> > > After reading the discussion, I would rather be in favor of fixing the
> > > driver
> > > and the tegra dts files, which are both wrong.
> > > 
> > > Keeping things as-is is very counter-intuitive: the GPIO on nyan boards is
> > > active-low and should be described as such (think of other projects, like
> > > U-Boot, reusing the dts). It's also very counter-intuitive to require that
> > > any
> > > new board using that driver use active-low polarity in the GPIO declaration
> > > when
> > > the line is really active-high.
> > 
> > Agreed, it very counter-intuitive. I have a board w/o an invert and it
> > does look odd with active-low in the dts. It really should be active-high.
> > 
> > The (new) binding helps a bit though.
> 
> Yeah, it's a solution anyway. I'm mostly worried that it means all other code
> (especially not in Linux) using the dts will be plagued by this, which seems a
> bit unfair. I still see this as an error not being fixed.
> 
> > > Is anyone strongly opposed to that solution? I'd really rather see the issue
> > > fixed that way instead of the current proposal (this patch).
> > 
> > It's a little bit more than a proposal since it is in linux-next. But not set
> > in stone of course.
> 
> Oh, I thought it was still up for review when sending my previous message.
> 
> > I personally do not care as long as it is changed before
> > hitting Linus' tree as I have no deployed devices. But docs are just that.
> > Docs. Anything that worked before is going to break with the change you are
> > proposing. Are you really willing to break who knows how many tegra boards?
> 
> From my perspective, fixing counter-intuitive behaviour justifies that.
> 
> But you're probably right to point out that others down the line will most
> likely prefer to keep the change you introduced. So I'll put it this way:
> if everyone agrees that it's okay to break support for older device-trees,
> then I'm available to craft a patch. Otherwise, let's just stick with you
> changes you got in.

I do think this case is exceptional because both the DTS and driver code
are wrong, whereas the binding is correct.

So the reasonable thing to do here is to fix the code and DTS files. The
only thing I'm still somewhat concerned about is the fallout from this.

Looking at the code it seems that the ACOK GPIO is primarily used to
report the "charger present" and "charging/discharging" status. However
that status is also used to enable or disable charging. So from what I
can tell breaking compatibility here would cause device to no longer be
able to charge, which could potentially brick users' devices.

Then again, I'm not sure how the hardware works exactly. If the device
will still function with an empty battery but with a charger connected,
and I suspect it will, then users would still be able to hook up their
chargers, which they eventually have to do anyway, to boot and update
the DTB to fix this.

Now as for the amount of users concerned: I see this charger used only
on Tegra boards. Of those I don't think Venice2 and Tegra132 Norrin have
ever been available outside NVIDIA, so I don't think we'd have to
concern ourselves with them. Even if somebody was still using them
they'd have to be developers that would likely start out by copying new
kernel *and* DTB to the device.

Nyan-based devices, on the other hand, are fairly popular. I suspect
that not many actually run upstream kernels on them, and those that do
will certainly know how to update the DTS along with the kernel.

> > Or do you somehow know that *all* tegra users will always update their kernel
> > and dtb in sync, so that regressions will not happen?
> 
> Frankly I've never understood why kernel maintainers expect this is not the
> case, and this is quite frequently a pain when trying to address issues such as
> this one. While I reckon that device-trees shouldn't be tied to the kernel in
> particular (both are independent), I think it's fair to except that users use
> device-trees matching the release date of the kernel.
> 
> It's usually accepted that projects have dependencies on certain versions of
> libraries and explicitly do not support older versions of these. It would be
> nice to adopt the same attitude towards device-tree. These are my 2 cents on the
> issue, of course this isn't up for me to decide.

I think the primary reason for requiring backwards-compatibility is that
the DTB could be stored in a read-only location. So if there is no way
to upgrade the DTB it's fairly natural that we can't ever break ABI with
it.

Anyway, I think I've captured in full above what the potential fallout
from this break would be. Essentially things will continue to work as-is
but if users end up not updating their DTB along with the kernel their
battery will drain. The fix for this would be to update the DTB, which
on Nyan boards can be easily done along with the kernel.

The advantages would be that other boards and OSes wouldn't have to deal
with a trivial logical error that was made a long time ago.

Rob, any chance you could grant an exception under the above
circumstances?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] dt-bindings: power: supply: bq24735: reverse the polarity of ac-detect

[Jon Hunter]

On 15/12/16 12:21, Peter Rosin wrote:
> The ACOK pin on the bq24735 is active-high, of course meaning that when
> AC is OK the pin is high. However, all Tegra dts files have incorrectly
> specified active-high even though the signal is inverted on the Tegra
> boards. This has worked since the Linux driver has also inverted the
> meaning of the GPIO. Fix this situation by simply specifying in the
> bindings what everybody else agrees on; that the ti,ac-detect-gpios is
> active on AC adapter absence.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> 
> Hi!
> 
> This patch is the result of this discussion:
> http://marc.info/?t=148152531800002
> 
> I don't like how it changes the one thing that is seems correct, but
> what to do?
> 
> Cheers,
> Peter
> 
>  Documentation/devicetree/bindings/power/supply/ti,bq24735.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt b/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
> index 3bf55757ceec..43d56c49455b 100644
> --- a/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
> +++ b/Documentation/devicetree/bindings/power/supply/ti,bq24735.txt
> @@ -8,8 +8,9 @@ Optional properties :
>   - interrupts : Specify the interrupt to be used to trigger when the AC
>     adapter is either plugged in or removed.
>   - ti,ac-detect-gpios : This GPIO is optionally used to read the AC adapter
> -   presence. This is a Host GPIO that is configured as an input and
> -   connected to the bq24735.
> +   status. This is a Host GPIO that is configured as an input and
> +   connected to the bq24735, typically the ACOK pin (note: the GPIO should
> +   be active on AC adapter absence).
>   - ti,charge-current : Used to control and set the charging current. This value
>     must be between 128mA and 8.128A with a 64mA step resolution. The POR value
>     is 0x0000h. This number is in mA (e.g. 8192), see spec for more information

I don't think you need the 'typically' above, and I may have added ...

"the GPIO must be active on AC adapter absence despite the ACOK being
active-high on AC presence to be compatible with legacy boards".

... but otherwise ...

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic