bq24735 charger and ac-detect

bq24735 charger and ac-detect

[Peter Rosin]

Hi!

I'm wondering about the dt bindings for the bq24735 charger. Specifically
the ac-detect property. The bindings say:

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

The only way I can make sense of that is if this is the pin on the bq24735
that is named ACOK. But that pin is active high, and the code has this:

static bool bq24735_charger_is_present(struct bq24735 *charger)
{
	if (charger->status_gpio)
		return !gpiod_get_value_cansleep(charger->status_gpio);
...


(status_gpio is what holds the gpio_desc of ac-detect)

In other words, the code seems to want a signal that is effectively
active low (the code negates the signal and thus returns "present"
when the signal is zero). The existing dts users all have active high
in their bindings, so it's not like they say active low to work around
the negation in the code...

This just makes no sense to me and I'm wondering what I'm missing and
what pin is really meant to be fed to ac-detect???

Yes, there is a pin on the bq24735 that is named ACDET which seems like
a good match, but that is a signal that is entering the bq24735. And it
is also active high, at least the way I read it so the problem persists...

Cheers,
Peter

Re: bq24735 charger and ac-detect

[Sebastian Reichel]

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

Hi Peter,

On Mon, Dec 12, 2016 at 12:12:47AM +0100, Peter Rosin wrote:
> Hi!
> 
> I'm wondering about the dt bindings for the bq24735 charger. Specifically
> the ac-detect property. The bindings say:

I don't have a device with bq24735 and the driver has been added
before I was the maintainer of the power-supply subsystem.

>  - 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.
> 
> The only way I can make sense of that is if this is the pin on the bq24735
> that is named ACOK.

Yes. I would expect the same.

> But that pin is active high, and the code has this:
> 
> static bool bq24735_charger_is_present(struct bq24735 *charger)
> {
> 	if (charger->status_gpio)
> 		return !gpiod_get_value_cansleep(charger->status_gpio);
> ...
> 
> 
> (status_gpio is what holds the gpio_desc of ac-detect)
> 
> In other words, the code seems to want a signal that is effectively
> active low (the code negates the signal and thus returns "present"
> when the signal is zero). The existing dts users all have active high
> in their bindings, so it's not like they say active low to work around
> the negation in the code...
> 
> This just makes no sense to me and I'm wondering what I'm missing and
> what pin is really meant to be fed to ac-detect???
> 
> Yes, there is a pin on the bq24735 that is named ACDET which seems like
> a good match, but that is a signal that is entering the bq24735. And it
> is also active high, at least the way I read it so the problem persists...
> 
> Cheers,
> Peter

As far as I can see all current users are Tegra boards and the
driver is also from Nvidia, so let's ask them directly. I guess
the driver was ported incorrectly and the "!" should be removed.

-- Sebastian

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

Re: bq24735 charger and ac-detect

[Sebastian Reichel]

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

Hi,

Ok, Darbha Sriharsha's mail address no longer works, so I Cc'd
linux-tegra instead. Maybe some of the people there have the
schematics for one of the Tegra boards using the bq24735 and/or
could check if charger presence detection actually works on
those boards with current mainline kernel.

-- Sebastian

On Wed, Dec 14, 2016 at 03:25:02PM +0100, Sebastian Reichel wrote:
> Hi Peter,
> 
> On Mon, Dec 12, 2016 at 12:12:47AM +0100, Peter Rosin wrote:
> > Hi!
> > 
> > I'm wondering about the dt bindings for the bq24735 charger. Specifically
> > the ac-detect property. The bindings say:
> 
> I don't have a device with bq24735 and the driver has been added
> before I was the maintainer of the power-supply subsystem.
> 
> >  - 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.
> > 
> > The only way I can make sense of that is if this is the pin on the bq24735
> > that is named ACOK.
> 
> Yes. I would expect the same.
> 
> > But that pin is active high, and the code has this:
> > 
> > static bool bq24735_charger_is_present(struct bq24735 *charger)
> > {
> > 	if (charger->status_gpio)
> > 		return !gpiod_get_value_cansleep(charger->status_gpio);
> > ...
> > 
> > 
> > (status_gpio is what holds the gpio_desc of ac-detect)
> > 
> > In other words, the code seems to want a signal that is effectively
> > active low (the code negates the signal and thus returns "present"
> > when the signal is zero). The existing dts users all have active high
> > in their bindings, so it's not like they say active low to work around
> > the negation in the code...
> > 
> > This just makes no sense to me and I'm wondering what I'm missing and
> > what pin is really meant to be fed to ac-detect???
> > 
> > Yes, there is a pin on the bq24735 that is named ACDET which seems like
> > a good match, but that is a signal that is entering the bq24735. And it
> > is also active high, at least the way I read it so the problem persists...
> > 
> > Cheers,
> > Peter
> 
> As far as I can see all current users are Tegra boards and the
> driver is also from Nvidia, so let's ask them directly. I guess
> the driver was ported incorrectly and the "!" should be removed.
> 
> -- Sebastian



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

Re: bq24735 charger and ac-detect

[Jon Hunter]

On 14/12/16 14:59, Sebastian Reichel wrote:
> * PGP Signed by an unknown key
> 
> Hi,
> 
> Ok, Darbha Sriharsha's mail address no longer works, so I Cc'd
> linux-tegra instead. Maybe some of the people there have the
> schematics for one of the Tegra boards using the bq24735 and/or
> could check if charger presence detection actually works on
> those boards with current mainline kernel.

I just gave this a quick test on my tegra124-nyan-big with v4.9 and I do
see the status for the bq24735 switching from "Charging" to "Not
charging" when plugging and unplugging the charger.

Please note for this board, the bq24735 is connected to the Google
chrome EC (embedded controller) and not directly to Tegra. If you look
at the device-tree source, tegra124-nyan.dtsi, you will see there is
this ec-i2c-tunnel that is exposing the device to the kernel.

Cheers
Jon

-- 
nvpublic

Re: bq24735 charger and ac-detect

[Jon Hunter]

On 14/12/16 14:59, Sebastian Reichel wrote:
> * PGP Signed by an unknown key
> 
> Hi,
> 
> Ok, Darbha Sriharsha's mail address no longer works, so I Cc'd
> linux-tegra instead. Maybe some of the people there have the
> schematics for one of the Tegra boards using the bq24735 and/or
> could check if charger presence detection actually works on
> those boards with current mainline kernel.
> 
> -- Sebastian
> 
> On Wed, Dec 14, 2016 at 03:25:02PM +0100, Sebastian Reichel wrote:
>> Hi Peter,
>>
>> On Mon, Dec 12, 2016 at 12:12:47AM +0100, Peter Rosin wrote:
>>> Hi!
>>>
>>> I'm wondering about the dt bindings for the bq24735 charger. Specifically
>>> the ac-detect property. The bindings say:
>>
>> I don't have a device with bq24735 and the driver has been added
>> before I was the maintainer of the power-supply subsystem.
>>
>>>  - 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.
>>>
>>> The only way I can make sense of that is if this is the pin on the bq24735
>>> that is named ACOK.
>>
>> Yes. I would expect the same.

I just checked the schematic and it is indeed the ACOK.

>>> But that pin is active high, and the code has this:

Yes I see the same in the TI datasheet.

>>> static bool bq24735_charger_is_present(struct bq24735 *charger)
>>> {
>>> 	if (charger->status_gpio)
>>> 		return !gpiod_get_value_cansleep(charger->status_gpio);
>>> ...
>>>
>>>
>>> (status_gpio is what holds the gpio_desc of ac-detect)
>>>
>>> In other words, the code seems to want a signal that is effectively
>>> active low (the code negates the signal and thus returns "present"
>>> when the signal is zero). The existing dts users all have active high
>>> in their bindings, so it's not like they say active low to work around
>>> the negation in the code...
>>>
>>> This just makes no sense to me and I'm wondering what I'm missing and
>>> what pin is really meant to be fed to ac-detect???
>>>
>>> Yes, there is a pin on the bq24735 that is named ACDET which seems like
>>> a good match, but that is a signal that is entering the bq24735. And it
>>> is also active high, at least the way I read it so the problem persists...

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 :-(

Jon

-- 
nvpublic

Re: bq24735 charger and ac-detect

[Peter Rosin]

On 2016-12-14 18:54, Jon Hunter wrote:
> 
> On 14/12/16 14:59, Sebastian Reichel wrote:
>> * PGP Signed by an unknown key
>>
>> Hi,
>>
>> Ok, Darbha Sriharsha's mail address no longer works, so I Cc'd
>> linux-tegra instead. Maybe some of the people there have the
>> schematics for one of the Tegra boards using the bq24735 and/or
>> could check if charger presence detection actually works on
>> those boards with current mainline kernel.
>>
>> -- Sebastian
>>
>> On Wed, Dec 14, 2016 at 03:25:02PM +0100, Sebastian Reichel wrote:
>>> Hi Peter,
>>>
>>> On Mon, Dec 12, 2016 at 12:12:47AM +0100, Peter Rosin wrote:
>>>> Hi!
>>>>
>>>> I'm wondering about the dt bindings for the bq24735 charger. Specifically
>>>> the ac-detect property. The bindings say:
>>>
>>> I don't have a device with bq24735 and the driver has been added
>>> before I was the maintainer of the power-supply subsystem.
>>>
>>>>  - 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.
>>>>
>>>> The only way I can make sense of that is if this is the pin on the bq24735
>>>> that is named ACOK.
>>>
>>> Yes. I would expect the same.
> 
> I just checked the schematic and it is indeed the ACOK.

Good to know, thanks!

>>>> But that pin is active high, and the code has this:
> 
> Yes I see the same in the TI datasheet.
> 
>>>> static bool bq24735_charger_is_present(struct bq24735 *charger)
>>>> {
>>>> 	if (charger->status_gpio)
>>>> 		return !gpiod_get_value_cansleep(charger->status_gpio);
>>>> ...
>>>>
>>>>
>>>> (status_gpio is what holds the gpio_desc of ac-detect)
>>>>
>>>> In other words, the code seems to want a signal that is effectively
>>>> active low (the code negates the signal and thus returns "present"
>>>> when the signal is zero). The existing dts users all have active high
>>>> in their bindings, so it's not like they say active low to work around
>>>> the negation in the code...
> 
> 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!

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?

Cheers,
peda

Re: bq24735 charger and ac-detect

[Jon Hunter]

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.

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

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.

Cheers
Jon

-- 
nvpublic

Re: bq24735 charger and ac-detect

[Peter Rosin]

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

Re: bq24735 charger and ac-detect

[Jon Hunter]

On 15/12/16 12:04, Peter Rosin wrote:

...

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

By providing a means for the user to specify the polarity for their
board. Of course the documentation would need to be updated as well. I
think all solutions will be ugly if we need to preserve compatibility.

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

Fine with me and of course that works for Tegra, but how does that
ultimately help you? How do you tell the driver to use active-high
instead of the default which is not active-low?

Jon

-- 
nvpublic

Re: bq24735 charger and ac-detect

[Jon Hunter]

On 15/12/16 14:04, Jon Hunter wrote:
> 
> On 15/12/16 12:04, Peter Rosin wrote:
> 
> ...
> 
>>>                               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).
> 
> By providing a means for the user to specify the polarity for their
> board. Of course the documentation would need to be updated as well. I
> think all solutions will be ugly if we need to preserve compatibility.
> 
>>> 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...
> 
> Fine with me and of course that works for Tegra, but how does that
> ultimately help you? How do you tell the driver to use active-high
> instead of the default which is not active-low?

s/which is not/which is now/

Jon

-- 
nvpublic

Re: bq24735 charger and ac-detect

[Peter Rosin]

On 2016-12-15 15:04, Jon Hunter wrote:
> 
> On 15/12/16 12:04, Peter Rosin wrote:
> 
> ...
> 
>>>                               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).
> 
> By providing a means for the user to specify the polarity for their
> board. Of course the documentation would need to be updated as well. I
> think all solutions will be ugly if we need to preserve compatibility.
> 
>>> 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...
> 
> Fine with me and of course that works for Tegra, but how does that
> ultimately help you? How do you tell the driver to use active-high
> instead of the default which is not active-low?

Oh, you didn't know that gpiod_set_value/gpiod_get_value takes care of
active high/low automatically. That explains the disconnect.

If I have this in my device tree:

bq24735-charger@9{
	compatible = "ti,bq24735";
	ti,ac-detect-gpios = <&ioexp 3 GPIO_ACTIVE_LOW>;
};

and do
	gpiod_get(dev, "ti,ac-detect", GPIOD_IN);

then gpiod_get_value() on that gpio descriptor will return 1 (as in
active) when the signal is low.

You have to use gpiod_get_raw_value to see what's on the actual line.
But that is almost always not interesting, the reasons for that should
be obvious from this discussion...

So, I lie and say GPIO_ACTIVE_LOW so that gpiod_get_value returns 0 when
AC is there (signal is high), and the driver inverts that and gets that
AC is present and starts charging.

Cheers,
peda

Re: bq24735 charger and ac-detect

[Jon Hunter]

On 15/12/16 15:34, Peter Rosin wrote:
> On 2016-12-15 15:04, Jon Hunter wrote:
>>
>> On 15/12/16 12:04, Peter Rosin wrote:
>>
>> ...
>>
>>>>                               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).
>>
>> By providing a means for the user to specify the polarity for their
>> board. Of course the documentation would need to be updated as well. I
>> think all solutions will be ugly if we need to preserve compatibility.
>>
>>>> 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...
>>
>> Fine with me and of course that works for Tegra, but how does that
>> ultimately help you? How do you tell the driver to use active-high
>> instead of the default which is not active-low?
> 
> Oh, you didn't know that gpiod_set_value/gpiod_get_value takes care of
> active high/low automatically. That explains the disconnect.

Ah I see. Yes I see that now in the gpiod_get_value().

Thanks!
Jon

-- 
nvpublic