Re: bq24735 charger and ac-detect

Peter Rosin
2016-12-15T12:04:41Z

On 2016-12-15 11:45, Jon Hunter wrote:
>
> On 14/12/16 21:22, Peter Rosin wrote:
>> On 2016-12-14 18:54, Jon Hunter wrote:
>
> ...
>
>>> Hmmm ... looking closer at the schematics I see a power-MOSFET in the
>>> path of the ACOK to the Tegra and it looks like this could be inverting
>>> the signal! Seems this was not caught when the code was submitted. This
>>> should have been described in the device-tree somewhere :-(
>>
>> That's what I suspected...
>>
>> Ok, I expect the ship has sailed and that it's too late to actually fix
>> this up with active-low in various Tegra dts files and removing the negation
>> in the code, without causing ripples and regressions? Because that would
>> be the clean fix!
>
> Yes, unfortunately it has sailed as we cannot break compatibility AFAIK.
> However, that said, I am not 100% certain for cases where there is a bug
> like this.
>
> I did double check this morning and verified that the gpio is low when
> the charger is connected and so it is inverted.

Thanks!

>> One other thing to possibly do is add a new binding that is not negated,
>> and perhaps named after the pin in the data-sheet, i.e. ti,acok-gpios,
>> and deprecate ti,ac-detect-gpios. But that's no fun as it leaves a bunch of
>> compat code that wastes space and some lucky person has to maintain it.
>>
>> Perhaps the best thing to do is to just re-document ti,ac-detect-gpios
>> to be the inverse of what is currently described, i.e. to be AC absence
>> instead of AC presence? But that feels like giving up and it isn't really
>> nice from a device-tree point of view either, but I guess those points are
>> fairly accademic? Or are there any non-Linux users of this binding? How are
>> these things handled when they pop up elsewhere?
>
> I guess that ideally the polarity should have been specified correctly
> by the gpio property, but even this looks wrong for Tegra as it is
> GPIO_ACTIVE_HIGH and not LOW.

It's exactly this GPIO_ACTIVE_HIGH that is wrong. It should have been
active low since the signal is active high from the charger, but inverted
on its way to the gpio pin, so from the gpio point of view the signal
is active-low.

What a mess, the only thing that is right is the bindings. It says that the
gpio is active when AC is present, which makes perfect sense and matches the
name of the property.

Then all the Tegra dts users violate that and say active-high when in fact
the signal is inverted on the path to the pin, so should have been active-low.

Then the driver code "saves the day" for the Tegra users by reversing the
inversion in the signal-path, but that is of course only helping Tegra and
and others not following the dt specification.

> The only other option is to add another
> property called something like 'ti,ac-detect-override-pol' to specify
> the polarity you want.

How is that helping? It's no different that just saying active-low for
boards that do not invert ACOK (which is what I currently do in my dts,
but I hate doing it since it doesn't match dt docs and is therefore just
wrong).

> To be honest, I am not sure how this type of thing is normally handled.
> So probably best to put together a patch with whatever option you feel
> best and explain why this is needed and see what the dev-tree folks say.

I suspect that at the end of the day documentation is less important than
regressions. But if there are more than one implementation of the same
spec and Linux is not following it, it's kind of harsh to change the spec
to match Linux. I doubt that there are any other users in this case though,
but what do I know?

I'll send a patch re-documenting ti,ac-detect-gpios to specify AC absence
instead of AC presence, let's see what the dt people thinks...

Cheers,
peda