[PATCH RESEND] power: bq24190_charger: Fix charge type sysfs property

[PATCH RESEND] power: bq24190_charger: Fix charge type sysfs property

[Andreas Dannenberg]

Access to the BQ24190's configurable charge type property (none, trickle,
fast) is being masked by an incorrect power_supply_property entry. After
applying this patch a new 'charge_type' property will appear in the
bq24190-charger sysfs folder backed up by getters/setters already
present in the driver.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
This is a RESEND of yesterday's patch that for some reason did not seem to
have gotten picked up correctly...

The current driver incorrectly tries to declare the charge type property
using POWER_SUPPLY_PROP_TYPE which is the wrong declaration for this
(actually POWER_SUPPLY_PROP_TYPE itself has been superseeded by
power_supply.type and isn't even used anymore as part of the power
supply property declarations).

After implementing the fix tested this aspect of the driver using an
actual BQ24190 device and could witness the impact the change of charge
type has on the battery (writing '1' to the new sysfs property means
charging off, '2' will do trickle charging, and '3' will result in
fast charging).


 drivers/power/bq24190_charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/bq24190_charger.c b/drivers/power/bq24190_charger.c
index 052db78..40ae47c 100644
--- a/drivers/power/bq24190_charger.c
+++ b/drivers/power/bq24190_charger.c
@@ -902,7 +902,7 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
 }
 
 static enum power_supply_property bq24190_charger_properties[] = {
-	POWER_SUPPLY_PROP_TYPE,
+	POWER_SUPPLY_PROP_CHARGE_TYPE,
 	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
-- 
1.9.1

Re: [PATCH RESEND] power: bq24190_charger: Fix charge type sysfs property

[Dan Murphy]

On 08/04/2015 11:36 AM, Andreas Dannenberg wrote:
> Access to the BQ24190's configurable charge type property (none, trickle,
> fast) is being masked by an incorrect power_supply_property entry. After
> applying this patch a new 'charge_type' property will appear in the
> bq24190-charger sysfs folder backed up by getters/setters already
> present in the driver.
>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> This is a RESEND of yesterday's patch that for some reason did not seem to
> have gotten picked up correctly...
>
> The current driver incorrectly tries to declare the charge type property
> using POWER_SUPPLY_PROP_TYPE which is the wrong declaration for this
> (actually POWER_SUPPLY_PROP_TYPE itself has been superseeded by
> power_supply.type and isn't even used anymore as part of the power
> supply property declarations).
>
> After implementing the fix tested this aspect of the driver using an
> actual BQ24190 device and could witness the impact the change of charge
> type has on the battery (writing '1' to the new sysfs property means
> charging off, '2' will do trickle charging, and '3' will result in
> fast charging).
>
>
>  drivers/power/bq24190_charger.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/power/bq24190_charger.c b/drivers/power/bq24190_charger.c
> index 052db78..40ae47c 100644
> --- a/drivers/power/bq24190_charger.c
> +++ b/drivers/power/bq24190_charger.c
> @@ -902,7 +902,7 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
>  }
>  
>  static enum power_supply_property bq24190_charger_properties[] = {
> -	POWER_SUPPLY_PROP_TYPE,
> +	POWER_SUPPLY_PROP_CHARGE_TYPE,
>  	POWER_SUPPLY_PROP_HEALTH,
>  	POWER_SUPPLY_PROP_ONLINE,
>  	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,

Acked-by: Dan Murphy <dmurphy@ti.com>

-- 
------------------
Dan Murphy

Re: [PATCH RESEND] power: bq24190_charger: Fix charge type sysfs property

[Andrew F. Davis]

On 08/04/2015 11:36 AM, Andreas Dannenberg wrote:
> Access to the BQ24190's configurable charge type property (none, trickle,
> fast) is being masked by an incorrect power_supply_property entry. After
> applying this patch a new 'charge_type' property will appear in the
> bq24190-charger sysfs folder backed up by getters/setters already
> present in the driver.
>
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
> This is a RESEND of yesterday's patch that for some reason did not seem to
> have gotten picked up correctly...
>
> The current driver incorrectly tries to declare the charge type property
> using POWER_SUPPLY_PROP_TYPE which is the wrong declaration for this
> (actually POWER_SUPPLY_PROP_TYPE itself has been superseeded by
> power_supply.type and isn't even used anymore as part of the power
> supply property declarations).
>
> After implementing the fix tested this aspect of the driver using an
> actual BQ24190 device and could witness the impact the change of charge
> type has on the battery (writing '1' to the new sysfs property means
> charging off, '2' will do trickle charging, and '3' will result in
> fast charging).
>
>
>   drivers/power/bq24190_charger.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/power/bq24190_charger.c b/drivers/power/bq24190_charger.c
> index 052db78..40ae47c 100644
> --- a/drivers/power/bq24190_charger.c
> +++ b/drivers/power/bq24190_charger.c
> @@ -902,7 +902,7 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
>   }
>
>   static enum power_supply_property bq24190_charger_properties[] = {
> -	POWER_SUPPLY_PROP_TYPE,
> +	POWER_SUPPLY_PROP_CHARGE_TYPE,
>   	POWER_SUPPLY_PROP_HEALTH,
>   	POWER_SUPPLY_PROP_ONLINE,
>   	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
>

Acked-by: Andrew F. Davis <afd@ti.com>

-- 
Andrew F. Davis

Re: [PATCH RESEND] power: bq24190_charger: Fix charge type sysfs property

[Sebastian Reichel]

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

Hi,

On Tue, Aug 04, 2015 at 11:36:17AM -0500, Andreas Dannenberg wrote:
> Access to the BQ24190's configurable charge type property (none, trickle,
> fast) is being masked by an incorrect power_supply_property entry. After
> applying this patch a new 'charge_type' property will appear in the
> bq24190-charger sysfs folder backed up by getters/setters already
> present in the driver.

Thanks, queued.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC] TI BQ242xx Battery Charger Development/Consolidation Plans

[Andreas Dannenberg]

Not too long I ago started working on a new driver called bq242xx_charger.c 
that will add support for TI's BQ24250, BQ24251, BQ24261M, and BQ24262 Li-Ion
battery charger/power-path management ICs and that work is about 50% done. I 
chose a working name of bq242xx_charger.c in the hopes that in addition to
providing immediate support for said devices that this driver can later be used
as a platform to add other devices of the BQ242xx sub-family in an effort to
re-use/consolidate and help keep drivers/power organized.

Long story short I also realized that we just got a new bq24257_charger.c
driver (for the BQ24257) already in the Linux 4.2 tree which creates somewhat
of a conflict with the bq242xx_charger.c work in progress.

I would like to solicit ideas/comments regarding the following plan of attack:

- Continue bq242xx_charger.c development as scoped in the first paragraph
- Work on getting it accepted into upstream
- As future work, absorb the already existing bq24257_charger.c driver into the
  bq242xx_charger.c driver, and deprecate its usage

There might be a potential risk for user confusion for a transitional time if
somebody was just looking at the filenames to select their driver due to part
number overlap. However I argue folks are more likely to go the Kconfig route
to select their driver so this may not be a problem after all.

Thanks!

-- 
Andreas Dannenberg
Texas Instruments Inc.

Re: [RFC] TI BQ242xx Battery Charger Development/Consolidation Plans

[Laurentiu Palcu]

Hi Andreas,

On 5 Aug 2015 19:11, "Andreas Dannenberg" <dannenberg@ti.com> wrote:
>
> Not too long I ago started working on a new driver called bq242xx_charger.c
> that will add support for TI's BQ24250, BQ24251, BQ24261M, and BQ24262 Li-Ion
> battery charger/power-path management ICs and that work is about 50% done. I
> chose a working name of bq242xx_charger.c in the hopes that in addition to
> providing immediate support for said devices that this driver can later be used
> as a platform to add other devices of the BQ242xx sub-family in an effort to
> re-use/consolidate and help keep drivers/power organized.
>
> Long story short I also realized that we just got a new bq24257_charger.c
> driver (for the BQ24257) already in the Linux 4.2 tree which creates somewhat
> of a conflict with the bq242xx_charger.c work in progress.
>
> I would like to solicit ideas/comments regarding the following plan of attack:
>
> - Continue bq242xx_charger.c development as scoped in the first paragraph
> - Work on getting it accepted into upstream
> - As future work, absorb the already existing bq24257_charger.c driver into the
>   bq242xx_charger.c driver, and deprecate its usage

Using wildcards in the driver name looks like a good idea until a new
part that fits the naming scheme appears but it's a totally different
product... Assuming those chips have a similar register layout as
BQ24257, why don't you start with the existing bq24257_charger driver
and build on that? Then, in Kconfig, make it clear what parts are
supported by the driver. This practice is quite common in Linux
kernel.

>
> There might be a potential risk for user confusion for a transitional time if
> somebody was just looking at the filenames to select their driver due to part
> number overlap. However I argue folks are more likely to go the Kconfig route
> to select their driver so this may not be a problem after all.

This one is tricky. If the BQ24257 chip became part of a commercial
product, then the customers will have to change the .config after
doing a kernel upgrade. Customers are not keen on doing that for a
commercially released product.

Sebastian, what do you think?

laurentiu

RE: [RFC] TI BQ242xx Battery Charger Development/Consolidation Plans

[Pallala, Ramakrishna]

Hi,

> Hi Andreas,
> 
> On 5 Aug 2015 19:11, "Andreas Dannenberg" <dannenberg@ti.com> wrote:
> >
> > Not too long I ago started working on a new driver called
> > bq242xx_charger.c that will add support for TI's BQ24250, BQ24251,
> > BQ24261M, and BQ24262 Li-Ion battery charger/power-path management ICs
> > and that work is about 50% done. I chose a working name of
> > bq242xx_charger.c in the hopes that in addition to providing immediate
> > support for said devices that this driver can later be used as a
> > platform to add other devices of the BQ242xx sub-family in an effort to re-
> use/consolidate and help keep drivers/power organized.
> >
> > Long story short I also realized that we just got a new
> > bq24257_charger.c driver (for the BQ24257) already in the Linux 4.2
> > tree which creates somewhat of a conflict with the bq242xx_charger.c work in
> progress.
> >
> > I would like to solicit ideas/comments regarding the following plan of attack:
> >
> > - Continue bq242xx_charger.c development as scoped in the first
> > paragraph
> > - Work on getting it accepted into upstream
> > - As future work, absorb the already existing bq24257_charger.c driver into the
> >   bq242xx_charger.c driver, and deprecate its usage
> 
> Using wildcards in the driver name looks like a good idea until a new part that
> fits the naming scheme appears but it's a totally different product... Assuming
> those chips have a similar register layout as BQ24257, why don't you start with
> the existing bq24257_charger driver and build on that? Then, in Kconfig, make it
> clear what parts are supported by the driver. This practice is quite common in
> Linux kernel.
> 
> >
> > There might be a potential risk for user confusion for a transitional
> > time if somebody was just looking at the filenames to select their
> > driver due to part number overlap. However I argue folks are more
> > likely to go the Kconfig route to select their driver so this may not be a
> problem after all.
> 
> This one is tricky. If the BQ24257 chip became part of a commercial product,
> then the customers will have to change the .config after doing a kernel upgrade.
> Customers are not keen on doing that for a commercially released product.
> 
> Sebastian, what do you think?
> 
> laurentiu

I was on vacation for some time and didn’t check this thread.

I have a separate driver for bq24261 charger and it is ready for upstream. Can I push the patch?

Thanks,
Ram

Re: [RFC] TI BQ242xx Battery Charger Development/Consolidation Plans

[Andreas Dannenberg]

Hi Ram,

On 08/11/2015 06:12 AM, Pallala, Ramakrishna wrote:
> I have a separate driver for bq24261 charger and it is ready for upstream. Can I push the patch?
> 
> Thanks,
> Ram
> 

Wow I'm impressed! You are doing all the TI drivers so there is nothing left
for me to do :) On a more serious note thanks for chiming in here.

Could you please share/post the driver that you have?

The bq24261 is basically the same as the bq24261M and bq24262 that I'm working
on so I would like to check feature completeness and how I could potentially
integrate pieces of my work into yours.

Also while we are at it, please note the bq24261 is "not recommended for new
designs" by TI. What should be used instead moving forward is the bq24261M.

Thanks and Regards,

-- 
Andreas Dannenberg
Texas Instruments Inc.

RE: [RFC] TI BQ242xx Battery Charger Development/Consolidation Plans

[Pallala, Ramakrishna]

Hi Andreas,

> On 08/11/2015 06:12 AM, Pallala, Ramakrishna wrote:
> > I have a separate driver for bq24261 charger and it is ready for upstream. Can I
> push the patch?
> >
> > Thanks,
> > Ram
> >
> 
> Wow I'm impressed! You are doing all the TI drivers so there is nothing left for
> me to do :) On a more serious note thanks for chiming in here.
> 
> Could you please share/post the driver that you have?
> 
> The bq24261 is basically the same as the bq24261M and bq24262 that I'm
> working on so I would like to check feature completeness and how I could
> potentially integrate pieces of my work into yours.
> 
> Also while we are at it, please note the bq24261 is "not recommended for new
> designs" by TI. What should be used instead moving forward is the bq24261M.

I will push the RFC patch in a day or two.

Thanks,
Ram

Re: [RFC] TI BQ242xx Battery Charger Development/Consolidation Plans

[Andreas Dannenberg]

Hi Palcu,

On 08/07/2015 03:58 AM, Laurentiu Palcu wrote:
> Hi Andreas,
> 
> On 5 Aug 2015 19:11, "Andreas Dannenberg" <dannenberg@ti.com> wrote:
>>
>> Not too long I ago started working on a new driver called bq242xx_charger.c
>> that will add support for TI's BQ24250, BQ24251, BQ24261M, and BQ24262 Li-Ion
>> battery charger/power-path management ICs and that work is about 50% done. I
>> chose a working name of bq242xx_charger.c in the hopes that in addition to
>> providing immediate support for said devices that this driver can later be used
>> as a platform to add other devices of the BQ242xx sub-family in an effort to
>> re-use/consolidate and help keep drivers/power organized.
>>
>> Long story short I also realized that we just got a new bq24257_charger.c
>> driver (for the BQ24257) already in the Linux 4.2 tree which creates somewhat
>> of a conflict with the bq242xx_charger.c work in progress.
>>
>> I would like to solicit ideas/comments regarding the following plan of attack:
>>
>> - Continue bq242xx_charger.c development as scoped in the first paragraph
>> - Work on getting it accepted into upstream
>> - As future work, absorb the already existing bq24257_charger.c driver into the
>>   bq242xx_charger.c driver, and deprecate its usage
> 
> Using wildcards in the driver name looks like a good idea until a new
> part that fits the naming scheme appears but it's a totally different
> product...

Thanks for your comments. It's really good and timely to have this discussion.

Generally I would agree but since this driver would encompass several devices from
the start on, all from a rather narrowly defined sub-family IMHO it would make
sense to give it a wildcard name rather than a single part number which won't
apply from the get-go to most what the driver would support. But this all might
be a moot point...

> Assuming those chips have a similar register layout as
> BQ24257, why don't you start with the existing bq24257_charger driver
> and build on that? Then, in Kconfig, make it clear what parts are
> supported by the driver. This practice is quite common in Linux
> kernel.

The BQ24257 is more or less a subset of the BQ24250/BQ24251 devices I'm working
on (once of the main difference is that the latter support dynamic power path
management which is a pretty useful feature) so generally speaking yes they
could sit in the same driver. The register maps look exactly the same so it
definitely makes sense to consolidate here.

I'll do some more digging in BQ24257_charger.c to see how feature-complete it
is and what it would mean adding support for "my" BQ24250/bq24251 devices.

One concern I have with bq24257_charger.c is the extensive use of regmap fields.
While certainly elegant I'm worried about the overhead/memory it consumes vs.
simple #defines. For example the charger code creates 27 instances of struct
regmap_field at runtime which consumes ~650 bytes of memory, and another 27
instances of struct reg_field allocated at load time consuming 540 byte, bringing
the total to almost 1.2KB (along the lines, bq25890_charger.c has 72 register
fields and consumes almost 3x that amount of memory for the same reasons).
Maybe it's because I'm from an MCU background but this sounds like a lot of
memory to me to hold 6 bytes worth of actual data (the device registers)...

>> There might be a potential risk for user confusion for a transitional time if
>> somebody was just looking at the filenames to select their driver due to part
>> number overlap. However I argue folks are more likely to go the Kconfig route
>> to select their driver so this may not be a problem after all.
> 
> This one is tricky. If the BQ24257 chip became part of a commercial
> product, then the customers will have to change the .config after
> doing a kernel upgrade. Customers are not keen on doing that for a
> commercially released product.

Understood. Migrating stuff is never fun. If I can find a good way to split/merge
my work into yours (and Ram's) hopefully it won't come to that.

Thanks and Regards,

-- 
Andreas Dannenberg
Texas Instruments Inc.

> 
> Sebastian, what do you think?
> 
> laurentiu
>