* bq24735 charger and ac-detect @ 2016-12-11 23:12 Peter Rosin 2016-12-14 14:25 ` Sebastian Reichel 0 siblings, 1 reply; 12+ messages in thread From: Peter Rosin @ 2016-12-11 23:12 UTC (permalink / raw) To: linux-pm, Sebastian Reichel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bq24735 charger and ac-detect 2016-12-11 23:12 bq24735 charger and ac-detect Peter Rosin @ 2016-12-14 14:25 ` Sebastian Reichel 2016-12-14 14:59 ` Sebastian Reichel 0 siblings, 1 reply; 12+ messages in thread From: Sebastian Reichel @ 2016-12-14 14:25 UTC (permalink / raw) To: Peter Rosin; +Cc: linux-pm, Darbha Sriharsha [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bq24735 charger and ac-detect 2016-12-14 14:25 ` Sebastian Reichel @ 2016-12-14 14:59 ` Sebastian Reichel 2016-12-14 17:22 ` Jon Hunter 2016-12-14 17:54 ` Jon Hunter 0 siblings, 2 replies; 12+ messages in thread From: Sebastian Reichel @ 2016-12-14 14:59 UTC (permalink / raw) To: Peter Rosin, linux-tegra-u79uwXL29TY76Z2rM5mHXA Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bq24735 charger and ac-detect 2016-12-14 14:59 ` Sebastian Reichel @ 2016-12-14 17:22 ` Jon Hunter 2016-12-14 17:54 ` Jon Hunter 1 sibling, 0 replies; 12+ messages in thread From: Jon Hunter @ 2016-12-14 17:22 UTC (permalink / raw) To: Sebastian Reichel, Peter Rosin, linux-tegra-u79uwXL29TY76Z2rM5mHXA Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bq24735 charger and ac-detect 2016-12-14 14:59 ` Sebastian Reichel 2016-12-14 17:22 ` Jon Hunter @ 2016-12-14 17:54 ` Jon Hunter 2016-12-14 21:22 ` Peter Rosin 1 sibling, 1 reply; 12+ messages in thread From: Jon Hunter @ 2016-12-14 17:54 UTC (permalink / raw) To: Sebastian Reichel, Peter Rosin, linux-tegra-u79uwXL29TY76Z2rM5mHXA Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bq24735 charger and ac-detect 2016-12-14 17:54 ` Jon Hunter @ 2016-12-14 21:22 ` Peter Rosin [not found] ` <84ede737-9e4c-b0ba-5a37-a96a7e86c3ec-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> 0 siblings, 1 reply; 12+ messages in thread From: Peter Rosin @ 2016-12-14 21:22 UTC (permalink / raw) To: Jon Hunter, Sebastian Reichel, linux-tegra; +Cc: linux-pm 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <84ede737-9e4c-b0ba-5a37-a96a7e86c3ec-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>]
* Re: bq24735 charger and ac-detect [not found] ` <84ede737-9e4c-b0ba-5a37-a96a7e86c3ec-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> @ 2016-12-15 10:45 ` Jon Hunter [not found] ` <21e09552-96b9-fb72-60be-7a4b9347b761-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 12+ messages in thread From: Jon Hunter @ 2016-12-15 10:45 UTC (permalink / raw) To: Peter Rosin, Sebastian Reichel, linux-tegra-u79uwXL29TY76Z2rM5mHXA Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <21e09552-96b9-fb72-60be-7a4b9347b761-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* Re: bq24735 charger and ac-detect [not found] ` <21e09552-96b9-fb72-60be-7a4b9347b761-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> @ 2016-12-15 12:04 ` Peter Rosin 2016-12-15 14:04 ` Jon Hunter 0 siblings, 1 reply; 12+ messages in thread From: Peter Rosin @ 2016-12-15 12:04 UTC (permalink / raw) To: Jon Hunter, Sebastian Reichel, linux-tegra-u79uwXL29TY76Z2rM5mHXA Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bq24735 charger and ac-detect 2016-12-15 12:04 ` Peter Rosin @ 2016-12-15 14:04 ` Jon Hunter [not found] ` <721122dd-dda8-1830-9224-40b5ed79e463-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 12+ messages in thread From: Jon Hunter @ 2016-12-15 14:04 UTC (permalink / raw) To: Peter Rosin, Sebastian Reichel, linux-tegra; +Cc: linux-pm 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <721122dd-dda8-1830-9224-40b5ed79e463-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* Re: bq24735 charger and ac-detect [not found] ` <721122dd-dda8-1830-9224-40b5ed79e463-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> @ 2016-12-15 14:07 ` Jon Hunter 2016-12-15 15:34 ` Peter Rosin 1 sibling, 0 replies; 12+ messages in thread From: Jon Hunter @ 2016-12-15 14:07 UTC (permalink / raw) To: Peter Rosin, Sebastian Reichel, linux-tegra-u79uwXL29TY76Z2rM5mHXA Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: bq24735 charger and ac-detect [not found] ` <721122dd-dda8-1830-9224-40b5ed79e463-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-12-15 14:07 ` Jon Hunter @ 2016-12-15 15:34 ` Peter Rosin [not found] ` <1316ab3d-8a75-6a03-e1d0-7dee55f2c90f-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> 1 sibling, 1 reply; 12+ messages in thread From: Peter Rosin @ 2016-12-15 15:34 UTC (permalink / raw) To: Jon Hunter, Sebastian Reichel, linux-tegra-u79uwXL29TY76Z2rM5mHXA Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <1316ab3d-8a75-6a03-e1d0-7dee55f2c90f-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>]
* Re: bq24735 charger and ac-detect [not found] ` <1316ab3d-8a75-6a03-e1d0-7dee55f2c90f-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> @ 2016-12-15 16:10 ` Jon Hunter 0 siblings, 0 replies; 12+ messages in thread From: Jon Hunter @ 2016-12-15 16:10 UTC (permalink / raw) To: Peter Rosin, Sebastian Reichel, linux-tegra-u79uwXL29TY76Z2rM5mHXA Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-12-15 16:10 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-11 23:12 bq24735 charger and ac-detect Peter Rosin 2016-12-14 14:25 ` Sebastian Reichel 2016-12-14 14:59 ` Sebastian Reichel 2016-12-14 17:22 ` Jon Hunter 2016-12-14 17:54 ` Jon Hunter 2016-12-14 21:22 ` Peter Rosin [not found] ` <84ede737-9e4c-b0ba-5a37-a96a7e86c3ec-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> 2016-12-15 10:45 ` Jon Hunter [not found] ` <21e09552-96b9-fb72-60be-7a4b9347b761-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-12-15 12:04 ` Peter Rosin 2016-12-15 14:04 ` Jon Hunter [not found] ` <721122dd-dda8-1830-9224-40b5ed79e463-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-12-15 14:07 ` Jon Hunter 2016-12-15 15:34 ` Peter Rosin [not found] ` <1316ab3d-8a75-6a03-e1d0-7dee55f2c90f-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> 2016-12-15 16:10 ` Jon Hunter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.