LTC3651 and other GPIO chargers

LTC3651 and other GPIO chargers

[Ladislav Michl]

Hi there!

A driver for LTC3651 was recently added to -next
https://patchwork.kernel.org/patch/9717049/
which brings a question whenever we want to add separate driver
(or vendor specific bindings) for ever growing list of similar
chargers. For example those using the same status output lines
are: BQ24232HA, LTC4007, LTM8061, RT9502, LT3651, LT3650, TP4056,
MAX1737...
Others for example BQ24032A and LM3658 (which I have to support)
are using different status encoding, ie. LM3658:

stat1	stat2		Condition
  0	  0	Power-Down, charging is suspended or interrupted
  1	  0	Pre-qualification mode, CC and CV charging
  0	  1	Charge is completed
  1	  1	Bad battery (Safety timer expired), or LDO mode

What about extending gpio-charger instead?
- allow gpio list fdt subnode
  - consider each gpio line to represent bit in a word
- provide per property subnodes
  - each subnode holds a mask and mapping to property values
This way we should be able to cover most chargers providing status
using gpios. Comments welcome and appreciated - those will turn into
implementation.

Thank you,
	ladis

Re: LTC3651 and other GPIO chargers

[Mike Looijmans]

On 26-06-17 18:21, Ladislav Michl wrote:
> Hi there!
> 
> A driver for LTC3651 was recently added to -next
> https://patchwork.kernel.org/patch/9717049/
> which brings a question whenever we want to add separate driver
> (or vendor specific bindings) for ever growing list of similar
> chargers. For example those using the same status output lines
> are: BQ24232HA, LTC4007, LTM8061, RT9502, LT3651, LT3650, TP4056,
> MAX1737...
> Others for example BQ24032A and LM3658 (which I have to support)
> are using different status encoding, ie. LM3658:
> 
> stat1	stat2		Condition
>    0	  0	Power-Down, charging is suspended or interrupted
>    1	  0	Pre-qualification mode, CC and CV charging
>    0	  1	Charge is completed
>    1	  1	Bad battery (Safety timer expired), or LDO mode
> 
> What about extending gpio-charger instead?
> - allow gpio list fdt subnode
>    - consider each gpio line to represent bit in a word
> - provide per property subnodes
>    - each subnode holds a mask and mapping to property values
> This way we should be able to cover most chargers providing status
> using gpios. Comments welcome and appreciated - those will turn into
> implementation.

It's an interesting idea, and while writing the ltc3651 driver, I was thinking 
along those lines already. However, there are only two gpio charger drivers 
now, so I didn't see the need yet.

The table is an interesting approach. Something else to consider: Some gpio 
chargers allow control as well, e.g. pulling the "charging" ping low 
externally forces the charging to stop.

Something that might help is to create a "GPIO charger" sub-menu in Kconfig, 
to make hunting for these drivers a bit easier.


Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Re: LTC3651 and other GPIO chargers

[Ladislav Michl]

On Tue, Jun 27, 2017 at 07:39:21AM +0200, Mike Looijmans wrote:
> On 26-06-17 18:21, Ladislav Michl wrote:
> > Hi there!
> > 
> > A driver for LTC3651 was recently added to -next
> > https://patchwork.kernel.org/patch/9717049/
> > which brings a question whenever we want to add separate driver
> > (or vendor specific bindings) for ever growing list of similar
> > chargers. For example those using the same status output lines
> > are: BQ24232HA, LTC4007, LTM8061, RT9502, LT3651, LT3650, TP4056,
> > MAX1737...
> > Others for example BQ24032A and LM3658 (which I have to support)
> > are using different status encoding, ie. LM3658:
> > 
> > stat1	stat2		Condition
> >    0	  0	Power-Down, charging is suspended or interrupted
> >    1	  0	Pre-qualification mode, CC and CV charging
> >    0	  1	Charge is completed
> >    1	  1	Bad battery (Safety timer expired), or LDO mode
> > 
> > What about extending gpio-charger instead?
> > - allow gpio list fdt subnode
> >    - consider each gpio line to represent bit in a word
> > - provide per property subnodes
> >    - each subnode holds a mask and mapping to property values
> > This way we should be able to cover most chargers providing status
> > using gpios. Comments welcome and appreciated - those will turn into
> > implementation.
> 
> It's an interesting idea, and while writing the ltc3651 driver, I was
> thinking along those lines already. However, there are only two gpio charger
> drivers now, so I didn't see the need yet.
> 
> The table is an interesting approach. Something else to consider: Some gpio
> chargers allow control as well, e.g. pulling the "charging" ping low
> externally forces the charging to stop.

Yes, the aforementioned LM3658 chip is one of such chargers. We already
have charger-manager and gpio-regulator, but this doesn't seem to solve
this situation as it provides "software charger ic". So for now I'm
just misusing led class to enable charger - something which indeed needs
to be solved properly.

> Something that might help is to create a "GPIO charger" sub-menu in Kconfig,
> to make hunting for these drivers a bit easier.

I hope number of these driver will stop rising after we have one to rule
them all :-)

Best regards,
	ladis

Re: LTC3651 and other GPIO chargers

[Mike Looijmans]

On 27-06-17 08:38, Ladislav Michl wrote:
> On Tue, Jun 27, 2017 at 07:39:21AM +0200, Mike Looijmans wrote:
>> On 26-06-17 18:21, Ladislav Michl wrote:
>>> Hi there!
>>>
>>> A driver for LTC3651 was recently added to -next
>>> https://patchwork.kernel.org/patch/9717049/
>>> which brings a question whenever we want to add separate driver
>>> (or vendor specific bindings) for ever growing list of similar
>>> chargers. For example those using the same status output lines
>>> are: BQ24232HA, LTC4007, LTM8061, RT9502, LT3651, LT3650, TP4056,
>>> MAX1737...
>>> Others for example BQ24032A and LM3658 (which I have to support)
>>> are using different status encoding, ie. LM3658:
>>>
>>> stat1	stat2		Condition
>>>     0	  0	Power-Down, charging is suspended or interrupted
>>>     1	  0	Pre-qualification mode, CC and CV charging
>>>     0	  1	Charge is completed
>>>     1	  1	Bad battery (Safety timer expired), or LDO mode
>>>
>>> What about extending gpio-charger instead?
>>> - allow gpio list fdt subnode
>>>     - consider each gpio line to represent bit in a word
>>> - provide per property subnodes
>>>     - each subnode holds a mask and mapping to property values
>>> This way we should be able to cover most chargers providing status
>>> using gpios. Comments welcome and appreciated - those will turn into
>>> implementation.
>>
>> It's an interesting idea, and while writing the ltc3651 driver, I was
>> thinking along those lines already. However, there are only two gpio charger
>> drivers now, so I didn't see the need yet.
>>
>> The table is an interesting approach. Something else to consider: Some gpio
>> chargers allow control as well, e.g. pulling the "charging" ping low
>> externally forces the charging to stop.
> 
> Yes, the aforementioned LM3658 chip is one of such chargers. We already
> have charger-manager and gpio-regulator, but this doesn't seem to solve
> this situation as it provides "software charger ic". So for now I'm
> just misusing led class to enable charger - something which indeed needs
> to be solved properly.

Yeah, LED is great for (mis)using for all kinds of generic GPIO switches. Been 
there, done that (controlling model trains through relais for example...)

The problem goes a bit deeper. There's a bit of support for everything, and 
laptops and tablets seem to have it worked out reasonably, but for embedded 
devices, we somehow always end up with some very bare drivers for the charger, 
fuel gauge, charge detector, monitors, controllers and whatnot, and we always 
need a dedicated user-mode application to stitch it all together, for example 
to reset the battery gauge to full when the charger reports that charge has 
completed, or changing the max charger current from 0.5 to 1.5A when a 
dedicated charger has been detected on the USB wires. would be nice if that 
application could get that information from the drivers so we could make a 
generic power manager...


>> Something that might help is to create a "GPIO charger" sub-menu in Kconfig,
>> to make hunting for these drivers a bit easier.
> 
> I hope number of these driver will stop rising after we have one to rule
> them all :-)

True.



Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Re: LTC3651 and other GPIO chargers

[Ladislav Michl]

Hello Mike,

On Tue, Jun 27, 2017 at 07:39:21AM +0200, Mike Looijmans wrote:
> On 26-06-17 18:21, Ladislav Michl wrote:
> > Hi there!
> > 
> > A driver for LTC3651 was recently added to -next
> > https://patchwork.kernel.org/patch/9717049/
> > which brings a question whenever we want to add separate driver
> > (or vendor specific bindings) for ever growing list of similar
> > chargers. For example those using the same status output lines
> > are: BQ24232HA, LTC4007, LTM8061, RT9502, LT3651, LT3650, TP4056,
> > MAX1737...
> > Others for example BQ24032A and LM3658 (which I have to support)
> > are using different status encoding, ie. LM3658:
> > 
> > stat1	stat2		Condition
> >    0	  0	Power-Down, charging is suspended or interrupted
> >    1	  0	Pre-qualification mode, CC and CV charging
> >    0	  1	Charge is completed
> >    1	  1	Bad battery (Safety timer expired), or LDO mode
> > 
> > What about extending gpio-charger instead?
> > - allow gpio list fdt subnode
> >    - consider each gpio line to represent bit in a word
> > - provide per property subnodes
> >    - each subnode holds a mask and mapping to property values
> > This way we should be able to cover most chargers providing status
> > using gpios. Comments welcome and appreciated - those will turn into
> > implementation.
> 
> It's an interesting idea, and while writing the ltc3651 driver, I was
> thinking along those lines already. However, there are only two gpio charger
> drivers now, so I didn't see the need yet.
> 
> The table is an interesting approach. Something else to consider: Some gpio
> chargers allow control as well, e.g. pulling the "charging" ping low
> externally forces the charging to stop.

Just one more question before submitting "one driver for all" patch...
In LTC3651 you made acpr_gpio mandatory while chrg_gpio and fault_gpio
are optional. I quite do not get it as those later two gpios make
driver LTC3651 specific, while without them it is just another gpio-charger
driver. Also I have a board here, where acpr_gpio is not routed at all
and ADC is used to measure Vin, which makes this driver imposible to use.

Any specific reason you implemented it this way? As once 4.13 is out,
devicetree binding will be set in stone and I'm out of luck...

(based on that I exteded gpio-charger to take into account optional
gpios as well)

> Something that might help is to create a "GPIO charger" sub-menu in Kconfig,
> to make hunting for these drivers a bit easier.

Thank you,
	ladis

Re: LTC3651 and other GPIO chargers

[Mike Looijmans]

On 28-07-17 03:04, Ladislav Michl wrote:
> Hello Mike,
> 
> On Tue, Jun 27, 2017 at 07:39:21AM +0200, Mike Looijmans wrote:
>> On 26-06-17 18:21, Ladislav Michl wrote:
>>> Hi there!
>>>
>>> A driver for LTC3651 was recently added to -next
>>> https://patchwork.kernel.org/patch/9717049/
>>> which brings a question whenever we want to add separate driver
>>> (or vendor specific bindings) for ever growing list of similar
>>> chargers. For example those using the same status output lines
>>> are: BQ24232HA, LTC4007, LTM8061, RT9502, LT3651, LT3650, TP4056,
>>> MAX1737...
>>> Others for example BQ24032A and LM3658 (which I have to support)
>>> are using different status encoding, ie. LM3658:
>>>
>>> stat1	stat2		Condition
>>>     0	  0	Power-Down, charging is suspended or interrupted
>>>     1	  0	Pre-qualification mode, CC and CV charging
>>>     0	  1	Charge is completed
>>>     1	  1	Bad battery (Safety timer expired), or LDO mode
>>>
>>> What about extending gpio-charger instead?
>>> - allow gpio list fdt subnode
>>>     - consider each gpio line to represent bit in a word
>>> - provide per property subnodes
>>>     - each subnode holds a mask and mapping to property values
>>> This way we should be able to cover most chargers providing status
>>> using gpios. Comments welcome and appreciated - those will turn into
>>> implementation.
>>
>> It's an interesting idea, and while writing the ltc3651 driver, I was
>> thinking along those lines already. However, there are only two gpio charger
>> drivers now, so I didn't see the need yet.
>>
>> The table is an interesting approach. Something else to consider: Some gpio
>> chargers allow control as well, e.g. pulling the "charging" ping low
>> externally forces the charging to stop.
> 
> Just one more question before submitting "one driver for all" patch...
> In LTC3651 you made acpr_gpio mandatory while chrg_gpio and fault_gpio
> are optional. I quite do not get it as those later two gpios make
> driver LTC3651 specific, while without them it is just another gpio-charger
> driver. Also I have a board here, where acpr_gpio is not routed at all
> and ADC is used to measure Vin, which makes this driver imposible to use.
> 
> Any specific reason you implemented it this way? As once 4.13 is out,
> devicetree binding will be set in stone and I'm out of luck...
> 
> (based on that I exteded gpio-charger to take into account optional
> gpios as well)

I only made acpr_gpio mandatory out of a feeling that at least one of the 
three should be connected, and it makes things slightly simpler. Without it, 
the "online" property isn't available.

It can be made optional, but it requires a few changes in the driver.

> 
>> Something that might help is to create a "GPIO charger" sub-menu in Kconfig,
>> to make hunting for these drivers a bit easier.
> 
> Thank you,
> 	ladis
> 



Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Re: LTC3651 and other GPIO chargers

[Sebastian Reichel]

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

Hi,

On Mon, Jun 26, 2017 at 06:21:06PM +0200, Ladislav Michl wrote:
> A driver for LTC3651 was recently added to -next
> https://patchwork.kernel.org/patch/9717049/
> which brings a question whenever we want to add separate driver
> (or vendor specific bindings) for ever growing list of similar
> chargers. For example those using the same status output lines
> are: BQ24232HA, LTC4007, LTM8061, RT9502, LT3651, LT3650, TP4056,
> MAX1737...
> Others for example BQ24032A and LM3658 (which I have to support)
> are using different status encoding, ie. LM3658:
> 
> stat1	stat2		Condition
>   0	  0	Power-Down, charging is suspended or interrupted
>   1	  0	Pre-qualification mode, CC and CV charging
>   0	  1	Charge is completed
>   1	  1	Bad battery (Safety timer expired), or LDO mode
> 
> What about extending gpio-charger instead?
> - allow gpio list fdt subnode
>   - consider each gpio line to represent bit in a word
> - provide per property subnodes
>   - each subnode holds a mask and mapping to property values
> This way we should be able to cover most chargers providing status
> using gpios. Comments welcome and appreciated - those will turn into
> implementation.
> 
> Thank you,
> 	ladis

I think you mean something like this?

charger {
    compatible = "vendor,chip", "gpio-charger;

    status-gpios = <status-gpio1>, <status-gpio2>;

    status-mapping-0 {
        mask = <00>;
        type = "exploded";
    };

    status-mapping-1 {
        mask = <01>;
        type = "frozen";
    };

    ...
};

In that case: NAK. I like the general idea, but the DT binding looks
like a mess. Instead of providing the mapping in DT, it should be
provided by the driver and selected based on the compatible value.

-- Sebastian

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

Re: LTC3651 and other GPIO chargers

[Ladislav Michl]

On Mon, Jul 03, 2017 at 03:52:38PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, Jun 26, 2017 at 06:21:06PM +0200, Ladislav Michl wrote:
> > A driver for LTC3651 was recently added to -next
> > https://patchwork.kernel.org/patch/9717049/
> > which brings a question whenever we want to add separate driver
> > (or vendor specific bindings) for ever growing list of similar
> > chargers. For example those using the same status output lines
> > are: BQ24232HA, LTC4007, LTM8061, RT9502, LT3651, LT3650, TP4056,
> > MAX1737...
> > Others for example BQ24032A and LM3658 (which I have to support)
> > are using different status encoding, ie. LM3658:
> > 
> > stat1	stat2		Condition
> >   0	  0	Power-Down, charging is suspended or interrupted
> >   1	  0	Pre-qualification mode, CC and CV charging
> >   0	  1	Charge is completed
> >   1	  1	Bad battery (Safety timer expired), or LDO mode
> > 
> > What about extending gpio-charger instead?
> > - allow gpio list fdt subnode
> >   - consider each gpio line to represent bit in a word
> > - provide per property subnodes
> >   - each subnode holds a mask and mapping to property values
> > This way we should be able to cover most chargers providing status
> > using gpios. Comments welcome and appreciated - those will turn into
> > implementation.
> > 
> > Thank you,
> > 	ladis
> 
> I think you mean something like this?

Not exatly, but you are pretty close.

> charger {
>     compatible = "vendor,chip", "gpio-charger;
> 
>     status-gpios = <status-gpio1>, <status-gpio2>;
> 
>     status-mapping-0 {
>         mask = <00>;
>         type = "exploded";
>     };
> 
>     status-mapping-1 {
>         mask = <01>;
>         type = "frozen";
>     };
> 
>     ...
> };
> 
> In that case: NAK. I like the general idea, but the DT binding looks
> like a mess. Instead of providing the mapping in DT, it should be
> provided by the driver and selected based on the compatible value.

I won't pretend I'm not disapointed, but you are propably right.
Mapping in DT would bind DT with Linux specific implementation details.

Now testing patched gpio-charger. Patch will be send once I'm happy
with implementation.

Thank you,
	ladis