Re: [PATCH] coccinelle: api: detect duplicate chip data arrays

[Liam Breck]

Hi Joe,

On Thu, Oct 5, 2017 at 12:30 PM, Joe Perches  wrote:
> On Thu, 2017-10-05 at 21:19 +0200, Julia Lawall wrote:
>> On Thu, 5 Oct 2017, Joe Perches wrote:
>> > On Thu, 2017-10-05 at 21:13 +0200, Julia Lawall wrote:
>> > > On Fri, 6 Oct 2017, Masahiro Yamada wrote:
>> > > > 2017-10-01 21:42 GMT+09:00 Julia Lawall :
>> > > > > This semantic patch detects duplicate arrays declared using 
>> > > > > BQ27XXX_DATA
>> > > > > within a single structure.  It is currently specific to the file
>> > > > > drivers/power/supply/bq27xxx_battery.c.
>> > > > > Signed-off-by: Julia Lawall 
>> > > > Applied to linux-kbuild/misc.
>> > > Thanks for picking it up.
>> > If it is specific to one file, why not just run it
>> > and post the resultant patch?  Why have it in tree?
>> I guess that they anticipate that the data may change in the future?
>
> Aren't you the script author Julia?  Who is the "they"?
>
> I think having a script for a single file unnecessary.

Sebastian Reichel, the power-supply maintainer, requested this script.
I previously implemented this as a run-time check within ifdef DEBUG,
but that was rejected.

Re: [PATCH] coccinelle: api: detect duplicate chip data arrays

[Liam Breck]

Hi Joe,

On Thu, Oct 5, 2017 at 12:30 PM, Joe Perches  wrote:
> On Thu, 2017-10-05 at 21:19 +0200, Julia Lawall wrote:
>> On Thu, 5 Oct 2017, Joe Perches wrote:
>> > On Thu, 2017-10-05 at 21:13 +0200, Julia Lawall wrote:
>> > > On Fri, 6 Oct 2017, Masahiro Yamada wrote:
>> > > > 2017-10-01 21:42 GMT+09:00 Julia Lawall :
>> > > > > This semantic patch detects duplicate arrays declared using 
>> > > > > BQ27XXX_DATA
>> > > > > within a single structure.  It is currently specific to the file
>> > > > > drivers/power/supply/bq27xxx_battery.c.
>> > > > > Signed-off-by: Julia Lawall 
>> > > > Applied to linux-kbuild/misc.
>> > > Thanks for picking it up.
>> > If it is specific to one file, why not just run it
>> > and post the resultant patch?  Why have it in tree?
>> I guess that they anticipate that the data may change in the future?
>
> Aren't you the script author Julia?  Who is the "they"?
>
> I think having a script for a single file unnecessary.

Sebastian Reichel, the power-supply maintainer, requested this script.
I previously implemented this as a run-time check within ifdef DEBUG,
but that was rejected.

Re: [PATCH] coccinelle: api: detect duplicate chip data arrays

[Liam Breck]

Hi Julia,

On Thu, Oct 5, 2017 at 12:25 PM, Julia Lawall <julia.law...@lip6.fr> wrote:
>
>
> On Thu, 5 Oct 2017, Liam Breck wrote:
>
>> Hi, sorry for slow reply...
>>
>> Can we patch something to make this script run by default on
>> bq7_battery_i2c build? If so let's do that.
>
> I don't think anything is set up for that.  But any changes to the file
> should be checked by the 0-day bot.

How do we ask the 0-day bot team to configure their build to run this
script? Will they just pick it up?

>> Also maybe the name of the script should include "bq27xxx_data"?
>
> OK
>
>> Few more comments below...
>>
>> On Sun, Oct 1, 2017 at 5:42 AM, Julia Lawall <julia.law...@lip6.fr> wrote:
>> > This semantic patch detects duplicate arrays declared using BQ27XXX_DATA
>> > within a single structure.  It is currently specific to the file
>> > drivers/power/supply/bq27xxx_battery.c.
>> >
>> > Signed-off-by: Julia Lawall <julia.law...@lip6.fr>
>> >
>> > ---
>> >  scripts/coccinelle/api/battery.cocci |  161 
>> > +++
>> >  1 file changed, 161 insertions(+)
>> >
>> > diff --git a/scripts/coccinelle/api/battery.cocci 
>> > b/scripts/coccinelle/api/battery.cocci
>> > new file mode 100644
>> > index 000..77c145a
>> > --- /dev/null
>> > +++ b/scripts/coccinelle/api/battery.cocci
>> > @@ -0,0 +1,161 @@
>> > +/// Detect BQ27XXX_DATA structures with identical registers, dm registers 
>> > or
>> > +/// properties.
>> > +//# Doesn't unfold macros used in register or property fields.
>> > +//# Requires OCaml scripting
>> > +///
>> > +// Confidence: High
>> > +// Copyright: (C) 2017 Julia Lawall, Inria/LIP6, GPLv2.
>> > +// URL: http://coccinelle.lip6.fr/
>> > +// Requires: 1.0.7
>> > +// Keywords: BQ27XXX_DATA
>> > +
>> > +virtual report
>> > +
>> > +@initialize:ocaml@
>> > +@@
>> > +
>> > +let print_report p msg =
>> > +  let p = List.hd p in
>> > +  Printf.printf "%s:%d:%d-%d: %s" p.file p.line p.col p.col_end msg
>> > +
>> > +@str depends on report@
>> > +type t;
>> > +identifier i,i1,i2;
>> > +expression e1,e2;
>> > +@@
>> > +
>> > +t i[] = {
>> > +  ...,
>> > +  [e1] = BQ27XXX_DATA(i1,...),
>> > +  ...,
>> > +  [e2] = BQ27XXX_DATA(i2,...),
>> > +  ...,
>> > +};
>> > +
>> > +@script:ocaml tocheck@
>> > +i1 << str.i1;
>> > +i2 << str.i2;
>> > +i1regs; i2regs;
>> > +i1dmregs; i2dmregs;
>> > +i1props; i2props;
>> > +@@
>> > +
>> > +if not(i1 = i2)
>> > +then
>> > +  begin
>> > +i1regs := make_ident (i1 ^ "_regs");
>> > +i2regs := make_ident (i2 ^ "_regs");
>> > +i1dmregs := make_ident (i1 ^ "_dm_regs");
>> > +i2dmregs := make_ident (i2 ^ "_dm_regs");
>> > +i1props := make_ident (i1 ^ "_props");
>> > +i2props := make_ident (i2 ^ "_props")
>> > +  end
>> > +
>> > +(*  *)
>> > +
>> > +@getregs1@
>> > +typedef u8;
>> > +identifier tocheck.i1regs;
>> > +initializer list i1regs_vals;
>> > +position p1;
>> > +@@
>> > +
>> > +u8 i1regs@p1[...] = { i1regs_vals, };
>> > +
>> > +@getregs2@
>> > +identifier tocheck.i2regs;
>> > +initializer list i2regs_vals;
>> > +position p2;
>> > +@@
>> > +
>> > +u8 i2regs@p2[...] = { i2regs_vals, };
>> > +
>> > +@script:ocaml@
>> > +(_,i1regs_vals) << getregs1.i1regs_vals;
>> > +(_,i2regs_vals) << getregs2.i2regs_vals;
>> > +i1regs << tocheck.i1regs;
>> > +i2regs << tocheck.i2regs;
>> > +p1 << getregs1.p1;
>> > +p2 << getregs2.p2;
>> > +@@
>> > +
>> > +if i1regs < i2regs &&
>> > +   List.sort compare i1regs_vals = List.sort compare i2regs_vals
>> > +then
>> > +  let msg =
>> > +Printf.sprintf
>> > +  "WARNING %s and %s (line %d) have the same registers\n"
>>
>> "are identical" vs "have the same..."
>
> OK, I guess identical would be appropriate for regsand dm_r

Re: [PATCH] coccinelle: api: detect duplicate chip data arrays

[Liam Breck]

Hi Julia,

On Thu, Oct 5, 2017 at 12:25 PM, Julia Lawall  wrote:
>
>
> On Thu, 5 Oct 2017, Liam Breck wrote:
>
>> Hi, sorry for slow reply...
>>
>> Can we patch something to make this script run by default on
>> bq7_battery_i2c build? If so let's do that.
>
> I don't think anything is set up for that.  But any changes to the file
> should be checked by the 0-day bot.

How do we ask the 0-day bot team to configure their build to run this
script? Will they just pick it up?

>> Also maybe the name of the script should include "bq27xxx_data"?
>
> OK
>
>> Few more comments below...
>>
>> On Sun, Oct 1, 2017 at 5:42 AM, Julia Lawall  wrote:
>> > This semantic patch detects duplicate arrays declared using BQ27XXX_DATA
>> > within a single structure.  It is currently specific to the file
>> > drivers/power/supply/bq27xxx_battery.c.
>> >
>> > Signed-off-by: Julia Lawall 
>> >
>> > ---
>> >  scripts/coccinelle/api/battery.cocci |  161 
>> > +++
>> >  1 file changed, 161 insertions(+)
>> >
>> > diff --git a/scripts/coccinelle/api/battery.cocci 
>> > b/scripts/coccinelle/api/battery.cocci
>> > new file mode 100644
>> > index 000..77c145a
>> > --- /dev/null
>> > +++ b/scripts/coccinelle/api/battery.cocci
>> > @@ -0,0 +1,161 @@
>> > +/// Detect BQ27XXX_DATA structures with identical registers, dm registers 
>> > or
>> > +/// properties.
>> > +//# Doesn't unfold macros used in register or property fields.
>> > +//# Requires OCaml scripting
>> > +///
>> > +// Confidence: High
>> > +// Copyright: (C) 2017 Julia Lawall, Inria/LIP6, GPLv2.
>> > +// URL: http://coccinelle.lip6.fr/
>> > +// Requires: 1.0.7
>> > +// Keywords: BQ27XXX_DATA
>> > +
>> > +virtual report
>> > +
>> > +@initialize:ocaml@
>> > +@@
>> > +
>> > +let print_report p msg =
>> > +  let p = List.hd p in
>> > +  Printf.printf "%s:%d:%d-%d: %s" p.file p.line p.col p.col_end msg
>> > +
>> > +@str depends on report@
>> > +type t;
>> > +identifier i,i1,i2;
>> > +expression e1,e2;
>> > +@@
>> > +
>> > +t i[] = {
>> > +  ...,
>> > +  [e1] = BQ27XXX_DATA(i1,...),
>> > +  ...,
>> > +  [e2] = BQ27XXX_DATA(i2,...),
>> > +  ...,
>> > +};
>> > +
>> > +@script:ocaml tocheck@
>> > +i1 << str.i1;
>> > +i2 << str.i2;
>> > +i1regs; i2regs;
>> > +i1dmregs; i2dmregs;
>> > +i1props; i2props;
>> > +@@
>> > +
>> > +if not(i1 = i2)
>> > +then
>> > +  begin
>> > +i1regs := make_ident (i1 ^ "_regs");
>> > +i2regs := make_ident (i2 ^ "_regs");
>> > +i1dmregs := make_ident (i1 ^ "_dm_regs");
>> > +i2dmregs := make_ident (i2 ^ "_dm_regs");
>> > +i1props := make_ident (i1 ^ "_props");
>> > +i2props := make_ident (i2 ^ "_props")
>> > +  end
>> > +
>> > +(*  *)
>> > +
>> > +@getregs1@
>> > +typedef u8;
>> > +identifier tocheck.i1regs;
>> > +initializer list i1regs_vals;
>> > +position p1;
>> > +@@
>> > +
>> > +u8 i1regs@p1[...] = { i1regs_vals, };
>> > +
>> > +@getregs2@
>> > +identifier tocheck.i2regs;
>> > +initializer list i2regs_vals;
>> > +position p2;
>> > +@@
>> > +
>> > +u8 i2regs@p2[...] = { i2regs_vals, };
>> > +
>> > +@script:ocaml@
>> > +(_,i1regs_vals) << getregs1.i1regs_vals;
>> > +(_,i2regs_vals) << getregs2.i2regs_vals;
>> > +i1regs << tocheck.i1regs;
>> > +i2regs << tocheck.i2regs;
>> > +p1 << getregs1.p1;
>> > +p2 << getregs2.p2;
>> > +@@
>> > +
>> > +if i1regs < i2regs &&
>> > +   List.sort compare i1regs_vals = List.sort compare i2regs_vals
>> > +then
>> > +  let msg =
>> > +Printf.sprintf
>> > +  "WARNING %s and %s (line %d) have the same registers\n"
>>
>> "are identical" vs "have the same..."
>
> OK, I guess identical would be appropriate for regsand dm_regs, but
> perhaps not for properties because there the same values might be in 

Re: [PATCH] coccinelle: api: detect duplicate chip data arrays

[Liam Breck]

Hi Joe,

On Thu, Oct 5, 2017 at 12:15 PM, Joe Perches  wrote:
> On Thu, 2017-10-05 at 21:13 +0200, Julia Lawall wrote:
>>
>> On Fri, 6 Oct 2017, Masahiro Yamada wrote:
>>
>> > 2017-10-01 21:42 GMT+09:00 Julia Lawall :
>> > > This semantic patch detects duplicate arrays declared using BQ27XXX_DATA
>> > > within a single structure.  It is currently specific to the file
>> > > drivers/power/supply/bq27xxx_battery.c.
>> > >
>> > > Signed-off-by: Julia Lawall 
>> > >
>> > > ---
>> >
>> > Applied to linux-kbuild/misc.
>>
>> Thanks for picking it up.
>
> If it is specific to one file, why not just run it
> and post the resultant patch?  Why have it in tree?

The driver which this script analyzes supports more than a dozen
different chips, which require different parameters. This script
checks for duplicate parameter arrays.

Re: [PATCH] coccinelle: api: detect duplicate chip data arrays

[Liam Breck]

Hi Joe,

On Thu, Oct 5, 2017 at 12:15 PM, Joe Perches  wrote:
> On Thu, 2017-10-05 at 21:13 +0200, Julia Lawall wrote:
>>
>> On Fri, 6 Oct 2017, Masahiro Yamada wrote:
>>
>> > 2017-10-01 21:42 GMT+09:00 Julia Lawall :
>> > > This semantic patch detects duplicate arrays declared using BQ27XXX_DATA
>> > > within a single structure.  It is currently specific to the file
>> > > drivers/power/supply/bq27xxx_battery.c.
>> > >
>> > > Signed-off-by: Julia Lawall 
>> > >
>> > > ---
>> >
>> > Applied to linux-kbuild/misc.
>>
>> Thanks for picking it up.
>
> If it is specific to one file, why not just run it
> and post the resultant patch?  Why have it in tree?

The driver which this script analyzes supports more than a dozen
different chips, which require different parameters. This script
checks for duplicate parameter arrays.

Re: [PATCH] coccinelle: api: detect duplicate chip data arrays

[Liam Breck]

Hi, sorry for slow reply...

Can we patch something to make this script run by default on
bq7_battery_i2c build? If so let's do that.

Also maybe the name of the script should include "bq27xxx_data"?

Few more comments below...

On Sun, Oct 1, 2017 at 5:42 AM, Julia Lawall  wrote:
> This semantic patch detects duplicate arrays declared using BQ27XXX_DATA
> within a single structure.  It is currently specific to the file
> drivers/power/supply/bq27xxx_battery.c.
>
> Signed-off-by: Julia Lawall 
>
> ---
>  scripts/coccinelle/api/battery.cocci |  161 
> +++
>  1 file changed, 161 insertions(+)
>
> diff --git a/scripts/coccinelle/api/battery.cocci 
> b/scripts/coccinelle/api/battery.cocci
> new file mode 100644
> index 000..77c145a
> --- /dev/null
> +++ b/scripts/coccinelle/api/battery.cocci
> @@ -0,0 +1,161 @@
> +/// Detect BQ27XXX_DATA structures with identical registers, dm registers or
> +/// properties.
> +//# Doesn't unfold macros used in register or property fields.
> +//# Requires OCaml scripting
> +///
> +// Confidence: High
> +// Copyright: (C) 2017 Julia Lawall, Inria/LIP6, GPLv2.
> +// URL: http://coccinelle.lip6.fr/
> +// Requires: 1.0.7
> +// Keywords: BQ27XXX_DATA
> +
> +virtual report
> +
> +@initialize:ocaml@
> +@@
> +
> +let print_report p msg =
> +  let p = List.hd p in
> +  Printf.printf "%s:%d:%d-%d: %s" p.file p.line p.col p.col_end msg
> +
> +@str depends on report@
> +type t;
> +identifier i,i1,i2;
> +expression e1,e2;
> +@@
> +
> +t i[] = {
> +  ...,
> +  [e1] = BQ27XXX_DATA(i1,...),
> +  ...,
> +  [e2] = BQ27XXX_DATA(i2,...),
> +  ...,
> +};
> +
> +@script:ocaml tocheck@
> +i1 << str.i1;
> +i2 << str.i2;
> +i1regs; i2regs;
> +i1dmregs; i2dmregs;
> +i1props; i2props;
> +@@
> +
> +if not(i1 = i2)
> +then
> +  begin
> +i1regs := make_ident (i1 ^ "_regs");
> +i2regs := make_ident (i2 ^ "_regs");
> +i1dmregs := make_ident (i1 ^ "_dm_regs");
> +i2dmregs := make_ident (i2 ^ "_dm_regs");
> +i1props := make_ident (i1 ^ "_props");
> +i2props := make_ident (i2 ^ "_props")
> +  end
> +
> +(*  *)
> +
> +@getregs1@
> +typedef u8;
> +identifier tocheck.i1regs;
> +initializer list i1regs_vals;
> +position p1;
> +@@
> +
> +u8 i1regs@p1[...] = { i1regs_vals, };
> +
> +@getregs2@
> +identifier tocheck.i2regs;
> +initializer list i2regs_vals;
> +position p2;
> +@@
> +
> +u8 i2regs@p2[...] = { i2regs_vals, };
> +
> +@script:ocaml@
> +(_,i1regs_vals) << getregs1.i1regs_vals;
> +(_,i2regs_vals) << getregs2.i2regs_vals;
> +i1regs << tocheck.i1regs;
> +i2regs << tocheck.i2regs;
> +p1 << getregs1.p1;
> +p2 << getregs2.p2;
> +@@
> +
> +if i1regs < i2regs &&
> +   List.sort compare i1regs_vals = List.sort compare i2regs_vals
> +then
> +  let msg =
> +Printf.sprintf
> +  "WARNING %s and %s (line %d) have the same registers\n"

"are identical" vs "have the same..."

> +  i1regs i2regs (List.hd p2).line in
> +  print_report p1 msg
> +
> +(*  *)
> +
> +@getdmregs1@
> +identifier tocheck.i1dmregs;
> +initializer list i1dmregs_vals;
> +position p1;
> +@@
> +
> +struct bq27xxx_dm_reg i1dmregs@p1[] = { i1dmregs_vals, };
> +
> +@getdmregs2@
> +identifier tocheck.i2dmregs;
> +initializer list i2dmregs_vals;
> +position p2;
> +@@
> +
> +struct bq27xxx_dm_reg i2dmregs@p2[] = { i2dmregs_vals, };
> +
> +@script:ocaml@
> +(_,i1dmregs_vals) << getdmregs1.i1dmregs_vals;
> +(_,i2dmregs_vals) << getdmregs2.i2dmregs_vals;
> +i1dmregs << tocheck.i1dmregs;
> +i2dmregs << tocheck.i2dmregs;
> +p1 << getdmregs1.p1;
> +p2 << getdmregs2.p2;
> +@@
> +
> +if i1dmregs < i2dmregs &&
> +   List.sort compare i1dmregs_vals = List.sort compare i2dmregs_vals
> +then
> +  let msg =
> +Printf.sprintf
> +  "WARNING %s and %s (line %d) have the same dm registers\n"

"are identical" vs "have the same..."

> +  i1dmregs i2dmregs (List.hd p2).line in
> +  print_report p1 msg
> +
> +(*  *)
> +
> +@getprops1@
> +identifier tocheck.i1props;
> +initializer list[n1] i1props_vals;
> +position p1;
> +@@
> +
> +enum power_supply_property i1props@p1[] = { i1props_vals, };
> +
> +@getprops2@
> +identifier tocheck.i2props;
> +initializer list[n2] i2props_vals;
> +position p2;
> +@@
> +
> +enum power_supply_property i2props@p2[] = { i2props_vals, };
> +
> +@script:ocaml@
> +(_,i1props_vals) << getprops1.i1props_vals;
> +(_,i2props_vals) << getprops2.i2props_vals;
> +i1props << tocheck.i1props;
> +i2props << tocheck.i2props;
> +p1 << getprops1.p1;
> +p2 << getprops2.p2;
> +@@
> +
> +if i1props < i2props &&
> +   List.sort compare i1props_vals = List.sort compare i2props_vals
> +then
> +  let msg =
> +Printf.sprintf
> +  "WARNING %s and %s (line %d) have the same properties\n"

"are identical" vs "have the same..."


> +  i1props i2props 

Re: [PATCH] coccinelle: api: detect duplicate chip data arrays

[Liam Breck]

Hi, sorry for slow reply...

Can we patch something to make this script run by default on
bq7_battery_i2c build? If so let's do that.

Also maybe the name of the script should include "bq27xxx_data"?

Few more comments below...

On Sun, Oct 1, 2017 at 5:42 AM, Julia Lawall  wrote:
> This semantic patch detects duplicate arrays declared using BQ27XXX_DATA
> within a single structure.  It is currently specific to the file
> drivers/power/supply/bq27xxx_battery.c.
>
> Signed-off-by: Julia Lawall 
>
> ---
>  scripts/coccinelle/api/battery.cocci |  161 
> +++
>  1 file changed, 161 insertions(+)
>
> diff --git a/scripts/coccinelle/api/battery.cocci 
> b/scripts/coccinelle/api/battery.cocci
> new file mode 100644
> index 000..77c145a
> --- /dev/null
> +++ b/scripts/coccinelle/api/battery.cocci
> @@ -0,0 +1,161 @@
> +/// Detect BQ27XXX_DATA structures with identical registers, dm registers or
> +/// properties.
> +//# Doesn't unfold macros used in register or property fields.
> +//# Requires OCaml scripting
> +///
> +// Confidence: High
> +// Copyright: (C) 2017 Julia Lawall, Inria/LIP6, GPLv2.
> +// URL: http://coccinelle.lip6.fr/
> +// Requires: 1.0.7
> +// Keywords: BQ27XXX_DATA
> +
> +virtual report
> +
> +@initialize:ocaml@
> +@@
> +
> +let print_report p msg =
> +  let p = List.hd p in
> +  Printf.printf "%s:%d:%d-%d: %s" p.file p.line p.col p.col_end msg
> +
> +@str depends on report@
> +type t;
> +identifier i,i1,i2;
> +expression e1,e2;
> +@@
> +
> +t i[] = {
> +  ...,
> +  [e1] = BQ27XXX_DATA(i1,...),
> +  ...,
> +  [e2] = BQ27XXX_DATA(i2,...),
> +  ...,
> +};
> +
> +@script:ocaml tocheck@
> +i1 << str.i1;
> +i2 << str.i2;
> +i1regs; i2regs;
> +i1dmregs; i2dmregs;
> +i1props; i2props;
> +@@
> +
> +if not(i1 = i2)
> +then
> +  begin
> +i1regs := make_ident (i1 ^ "_regs");
> +i2regs := make_ident (i2 ^ "_regs");
> +i1dmregs := make_ident (i1 ^ "_dm_regs");
> +i2dmregs := make_ident (i2 ^ "_dm_regs");
> +i1props := make_ident (i1 ^ "_props");
> +i2props := make_ident (i2 ^ "_props")
> +  end
> +
> +(*  *)
> +
> +@getregs1@
> +typedef u8;
> +identifier tocheck.i1regs;
> +initializer list i1regs_vals;
> +position p1;
> +@@
> +
> +u8 i1regs@p1[...] = { i1regs_vals, };
> +
> +@getregs2@
> +identifier tocheck.i2regs;
> +initializer list i2regs_vals;
> +position p2;
> +@@
> +
> +u8 i2regs@p2[...] = { i2regs_vals, };
> +
> +@script:ocaml@
> +(_,i1regs_vals) << getregs1.i1regs_vals;
> +(_,i2regs_vals) << getregs2.i2regs_vals;
> +i1regs << tocheck.i1regs;
> +i2regs << tocheck.i2regs;
> +p1 << getregs1.p1;
> +p2 << getregs2.p2;
> +@@
> +
> +if i1regs < i2regs &&
> +   List.sort compare i1regs_vals = List.sort compare i2regs_vals
> +then
> +  let msg =
> +Printf.sprintf
> +  "WARNING %s and %s (line %d) have the same registers\n"

"are identical" vs "have the same..."

> +  i1regs i2regs (List.hd p2).line in
> +  print_report p1 msg
> +
> +(*  *)
> +
> +@getdmregs1@
> +identifier tocheck.i1dmregs;
> +initializer list i1dmregs_vals;
> +position p1;
> +@@
> +
> +struct bq27xxx_dm_reg i1dmregs@p1[] = { i1dmregs_vals, };
> +
> +@getdmregs2@
> +identifier tocheck.i2dmregs;
> +initializer list i2dmregs_vals;
> +position p2;
> +@@
> +
> +struct bq27xxx_dm_reg i2dmregs@p2[] = { i2dmregs_vals, };
> +
> +@script:ocaml@
> +(_,i1dmregs_vals) << getdmregs1.i1dmregs_vals;
> +(_,i2dmregs_vals) << getdmregs2.i2dmregs_vals;
> +i1dmregs << tocheck.i1dmregs;
> +i2dmregs << tocheck.i2dmregs;
> +p1 << getdmregs1.p1;
> +p2 << getdmregs2.p2;
> +@@
> +
> +if i1dmregs < i2dmregs &&
> +   List.sort compare i1dmregs_vals = List.sort compare i2dmregs_vals
> +then
> +  let msg =
> +Printf.sprintf
> +  "WARNING %s and %s (line %d) have the same dm registers\n"

"are identical" vs "have the same..."

> +  i1dmregs i2dmregs (List.hd p2).line in
> +  print_report p1 msg
> +
> +(*  *)
> +
> +@getprops1@
> +identifier tocheck.i1props;
> +initializer list[n1] i1props_vals;
> +position p1;
> +@@
> +
> +enum power_supply_property i1props@p1[] = { i1props_vals, };
> +
> +@getprops2@
> +identifier tocheck.i2props;
> +initializer list[n2] i2props_vals;
> +position p2;
> +@@
> +
> +enum power_supply_property i2props@p2[] = { i2props_vals, };
> +
> +@script:ocaml@
> +(_,i1props_vals) << getprops1.i1props_vals;
> +(_,i2props_vals) << getprops2.i2props_vals;
> +i1props << tocheck.i1props;
> +i2props << tocheck.i2props;
> +p1 << getprops1.p1;
> +p2 << getprops2.p2;
> +@@
> +
> +if i1props < i2props &&
> +   List.sort compare i1props_vals = List.sort compare i2props_vals
> +then
> +  let msg =
> +Printf.sprintf
> +  "WARNING %s and %s (line %d) have the same properties\n"

"are identical" vs "have the same..."


> +  i1props i2props (List.hd p2).line in
> +  print_report p1 

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

Howdy,

On Fri, Sep 8, 2017 at 1:28 PM, H. Nikolaus Schaller <h...@goldelico.com> wrote:
> Hi Liam,
>
>> Am 08.09.2017 um 22:19 schrieb Liam Breck <l...@networkimprov.net>:
>>
>> Hi Nikolaus,
>>
>> On Fri, Sep 8, 2017 at 10:38 AM, H. Nikolaus Schaller <h...@goldelico.com> 
>> wrote:
>>> Hi Liam,
>>> I finally continues testing on OpenPandora.
>>>
>>>> Am 31.08.2017 um 22:19 schrieb Liam Breck <l...@networkimprov.net>:
>>>>
>>>> Hi,
>>>>
>>>> This may be a fix that allows >0 input from DT, but won't try to
>>>> program the register since the first 3 fields aren't compatible:
>>>>
>>>> ... bq27500_dm_regs[] = {
>>>> ...
>>>> [bq27xxx_dm_design_energy] = {  0,  0, 0,0, 65535 }, /* missing on 
>>>> chip */
>>>> NB: align columns with other rows
>>>
>>> I have tried with this DT
>>>
>>>bat: battery {
>>>compatible = "simple-battery", "openpandora-battery";
>>>voltage-min-design-microvolt = <325>;
>>>energy-full-design-microwatt-hours = <1480>;
>>>charge-full-design-microamp-hours = <410>;
>>>};
>>>
>>> and here is the result:
>>>
>>>> root@letux:~# dmesg|fgrep bq27
>>>> [   10.391235] bq27xxx_battery_setup
>>>> [   10.391265] bq27xxx_battery_setup: dm_regs=bf0520e0
>>>> [   10.393798] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name 
>>>> attribute, please fix
>>>> [   10.432678] bq27xxx_battery_settings
>>>> [   10.432952] bq27xxx_battery_set_config
>>>> [   10.432952] bq27xxx_battery_unseal
>>>> [   10.485168] bq27xxx_battery_update_dm_block
>>>> [   10.485198] bq27xxx-battery 2-0055: update design-capacity to 4100
>>>
>>> looks good
>>>
>>>> [   10.485229] bq27xxx_battery_update_dm_block
>>>> [   10.485229] bq27xxx-battery 2-0055: buffer does not match design-energy 
>>>> dm spec
>>>
>>> ok, ignored
>>>
>>>> [   10.511718] bq27xxx_battery_update_dm_block
>>>> [   10.511749] bq27xxx-battery 2-0055: update terminate-voltage to 3250
>>>
>>> looks good
>>>
>>>> [   10.826446] bq27xxx_battery_seal
>>>> [   12.150939] bq27xxx_battery_platform_probe
>>>> [   12.151031] bq27xxx_battery_setup
>>>> [   12.151062] bq27xxx_battery_setup: dm_regs=  (null)
>>>> [   12.153411] (NULL device *): hwmon: 'bq27000-battery' is not a valid 
>>>> name attribute, please fix
>>>> [   12.154327] bq27xxx_battery_settings
>>>> [   12.154357] power_supply bq27000-battery: power_supply_get_battery_info 
>>>> currently only supports devicetree
>>>> [   12.154388] bq27xxx_battery_settings: power_supply_get_battery_info 
>>>> failed ret=-6
>>
>> Is there also a bq27000 chip on this device, or a battery with it
>> embedded? If so, it doesn't appear to have a DT node (correct for
>> embedded), hence that warning, which isn't useful in that case. Prob
>> battery_settings() should do:
>>if (!di->dev->of_node || power_supply_get_battery_info(...) < 0) return;
>> altho that would be wrong once get_battery_info() supports ACPI config...
>
> No it is not available, but could be. The bq27000 can be connected through
> HDQ to an OMAP3 and therefore there is no auto-detection. If it is configured
> the driver will be loaded. Even if there is no battery_info.
>
> In fact we use the same defconfig for several devices and there is one (GTA04)
> which only has a bq27000 inside the battery.

Any idea why the driver detects a non-present bq27000 and calls
probe() for it? defconfig with battery_bq27xxx=y ?

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

Howdy,

On Fri, Sep 8, 2017 at 1:28 PM, H. Nikolaus Schaller  wrote:
> Hi Liam,
>
>> Am 08.09.2017 um 22:19 schrieb Liam Breck :
>>
>> Hi Nikolaus,
>>
>> On Fri, Sep 8, 2017 at 10:38 AM, H. Nikolaus Schaller  
>> wrote:
>>> Hi Liam,
>>> I finally continues testing on OpenPandora.
>>>
>>>> Am 31.08.2017 um 22:19 schrieb Liam Breck :
>>>>
>>>> Hi,
>>>>
>>>> This may be a fix that allows >0 input from DT, but won't try to
>>>> program the register since the first 3 fields aren't compatible:
>>>>
>>>> ... bq27500_dm_regs[] = {
>>>> ...
>>>> [bq27xxx_dm_design_energy] = {  0,  0, 0,0, 65535 }, /* missing on 
>>>> chip */
>>>> NB: align columns with other rows
>>>
>>> I have tried with this DT
>>>
>>>bat: battery {
>>>compatible = "simple-battery", "openpandora-battery";
>>>voltage-min-design-microvolt = <325>;
>>>energy-full-design-microwatt-hours = <1480>;
>>>charge-full-design-microamp-hours = <410>;
>>>};
>>>
>>> and here is the result:
>>>
>>>> root@letux:~# dmesg|fgrep bq27
>>>> [   10.391235] bq27xxx_battery_setup
>>>> [   10.391265] bq27xxx_battery_setup: dm_regs=bf0520e0
>>>> [   10.393798] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name 
>>>> attribute, please fix
>>>> [   10.432678] bq27xxx_battery_settings
>>>> [   10.432952] bq27xxx_battery_set_config
>>>> [   10.432952] bq27xxx_battery_unseal
>>>> [   10.485168] bq27xxx_battery_update_dm_block
>>>> [   10.485198] bq27xxx-battery 2-0055: update design-capacity to 4100
>>>
>>> looks good
>>>
>>>> [   10.485229] bq27xxx_battery_update_dm_block
>>>> [   10.485229] bq27xxx-battery 2-0055: buffer does not match design-energy 
>>>> dm spec
>>>
>>> ok, ignored
>>>
>>>> [   10.511718] bq27xxx_battery_update_dm_block
>>>> [   10.511749] bq27xxx-battery 2-0055: update terminate-voltage to 3250
>>>
>>> looks good
>>>
>>>> [   10.826446] bq27xxx_battery_seal
>>>> [   12.150939] bq27xxx_battery_platform_probe
>>>> [   12.151031] bq27xxx_battery_setup
>>>> [   12.151062] bq27xxx_battery_setup: dm_regs=  (null)
>>>> [   12.153411] (NULL device *): hwmon: 'bq27000-battery' is not a valid 
>>>> name attribute, please fix
>>>> [   12.154327] bq27xxx_battery_settings
>>>> [   12.154357] power_supply bq27000-battery: power_supply_get_battery_info 
>>>> currently only supports devicetree
>>>> [   12.154388] bq27xxx_battery_settings: power_supply_get_battery_info 
>>>> failed ret=-6
>>
>> Is there also a bq27000 chip on this device, or a battery with it
>> embedded? If so, it doesn't appear to have a DT node (correct for
>> embedded), hence that warning, which isn't useful in that case. Prob
>> battery_settings() should do:
>>if (!di->dev->of_node || power_supply_get_battery_info(...) < 0) return;
>> altho that would be wrong once get_battery_info() supports ACPI config...
>
> No it is not available, but could be. The bq27000 can be connected through
> HDQ to an OMAP3 and therefore there is no auto-detection. If it is configured
> the driver will be loaded. Even if there is no battery_info.
>
> In fact we use the same defconfig for several devices and there is one (GTA04)
> which only has a bq27000 inside the battery.

Any idea why the driver detects a non-present bq27000 and calls
probe() for it? defconfig with battery_bq27xxx=y ?

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

Hi Nikolaus,

On Fri, Sep 8, 2017 at 10:38 AM, H. Nikolaus Schaller <h...@goldelico.com> 
wrote:
> Hi Liam,
> I finally continues testing on OpenPandora.
>
>> Am 31.08.2017 um 22:19 schrieb Liam Breck <l...@networkimprov.net>:
>>
>> Hi,
>>
>> This may be a fix that allows >0 input from DT, but won't try to
>> program the register since the first 3 fields aren't compatible:
>>
>> ... bq27500_dm_regs[] = {
>> ...
>>  [bq27xxx_dm_design_energy] = {  0,  0, 0,0, 65535 }, /* missing on chip 
>> */
>> NB: align columns with other rows
>
> I have tried with this DT
>
> bat: battery {
> compatible = "simple-battery", "openpandora-battery";
> voltage-min-design-microvolt = <325>;
> energy-full-design-microwatt-hours = <1480>;
> charge-full-design-microamp-hours = <410>;
> };
>
> and here is the result:
>
>> root@letux:~# dmesg|fgrep bq27
>> [   10.391235] bq27xxx_battery_setup
>> [   10.391265] bq27xxx_battery_setup: dm_regs=bf0520e0
>> [   10.393798] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name 
>> attribute, please fix
>> [   10.432678] bq27xxx_battery_settings
>> [   10.432952] bq27xxx_battery_set_config
>> [   10.432952] bq27xxx_battery_unseal
>> [   10.485168] bq27xxx_battery_update_dm_block
>> [   10.485198] bq27xxx-battery 2-0055: update design-capacity to 4100
>
> looks good
>
>> [   10.485229] bq27xxx_battery_update_dm_block
>> [   10.485229] bq27xxx-battery 2-0055: buffer does not match design-energy 
>> dm spec
>
> ok, ignored
>
>> [   10.511718] bq27xxx_battery_update_dm_block
>> [   10.511749] bq27xxx-battery 2-0055: update terminate-voltage to 3250
>
> looks good
>
>> [   10.826446] bq27xxx_battery_seal
>> [   12.150939] bq27xxx_battery_platform_probe
>> [   12.151031] bq27xxx_battery_setup
>> [   12.151062] bq27xxx_battery_setup: dm_regs=  (null)
>> [   12.153411] (NULL device *): hwmon: 'bq27000-battery' is not a valid name 
>> attribute, please fix
>> [   12.154327] bq27xxx_battery_settings
>> [   12.154357] power_supply bq27000-battery: power_supply_get_battery_info 
>> currently only supports devicetree
>> [   12.154388] bq27xxx_battery_settings: power_supply_get_battery_info 
>> failed ret=-6

Is there also a bq27000 chip on this device, or a battery with it
embedded? If so, it doesn't appear to have a DT node (correct for
embedded), hence that warning, which isn't useful in that case. Prob
battery_settings() should do:
if (!di->dev->of_node || power_supply_get_battery_info(...) < 0) return;
altho that would be wrong once get_battery_info() supports ACPI config...

> ... login:
>
>> root@letux:~# cat /sys/class/power_supply/bq27500-1-0/uevent
>> POWER_SUPPLY_NAME=bq27500-1-0
>> POWER_SUPPLY_STATUS=Discharging
>> POWER_SUPPLY_PRESENT=1
>> POWER_SUPPLY_VOLTAGE_NOW=3892000
>> POWER_SUPPLY_CURRENT_NOW=-317000
>> POWER_SUPPLY_CAPACITY=0
>
> oops.
>
>> POWER_SUPPLY_CAPACITY_LEVEL=Low
>> POWER_SUPPLY_TEMP=223
>> POWER_SUPPLY_TIME_TO_EMPTY_NOW=0
>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>> POWER_SUPPLY_CHARGE_FULL=1147000
>> POWER_SUPPLY_CHARGE_NOW=2665000
>> POWER_SUPPLY_CHARGE_FULL_DESIGN=410
>> POWER_SUPPLY_CYCLE_COUNT=6
>> POWER_SUPPLY_ENERGY_NOW=0
>> POWER_SUPPLY_POWER_AVG=64395
>> POWER_SUPPLY_HEALTH=Dead
>
> oops. But maybe the bq27500 needs a full cycle first

I wouldn't guess that the above state is due to the DM update
sequence, but I suppose that's possible. Another update pass might
clarify that.

>> POWER_SUPPLY_MANUFACTURER=Texas Instruments
>
> vvv after plugging in charger
>
>> root@letux:~# cat /sys/class/power_supply/bq27500-1-0/uevent
>> POWER_SUPPLY_NAME=bq27500-1-0
>> POWER_SUPPLY_STATUS=Charging
>> POWER_SUPPLY_PRESENT=1
>> POWER_SUPPLY_VOLTAGE_NOW=3923000
>> POWER_SUPPLY_CURRENT_NOW=204000
>> POWER_SUPPLY_CAPACITY=0
>> POWER_SUPPLY_CAPACITY_LEVEL=Low
>> POWER_SUPPLY_TEMP=249
>> POWER_SUPPLY_TIME_TO_EMPTY_NOW=0
>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>> POWER_SUPPLY_CHARGE_FULL=1147000
>> POWER_SUPPLY_CHARGE_NOW=2665000
>> POWER_SUPPLY_CHARGE_FULL_DESIGN=410
>> POWER_SUPPLY_CYCLE_COUNT=6
>> POWER_SUPPLY_ENERGY_NOW=0
>> POWER_SUPPLY_POWER_AVG=800
>> POWER_SUPPLY_HEALTH=Dead
>> POWER_SUPPLY_MANUFACTURER=Texas Instruments
>> root@letux:~#
>
> Now a reboot after removing charger and battery for several minutes:
>
>> root@letux:~# dmesg|fgrep bq27
&g

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

Hi Nikolaus,

On Fri, Sep 8, 2017 at 10:38 AM, H. Nikolaus Schaller  
wrote:
> Hi Liam,
> I finally continues testing on OpenPandora.
>
>> Am 31.08.2017 um 22:19 schrieb Liam Breck :
>>
>> Hi,
>>
>> This may be a fix that allows >0 input from DT, but won't try to
>> program the register since the first 3 fields aren't compatible:
>>
>> ... bq27500_dm_regs[] = {
>> ...
>>  [bq27xxx_dm_design_energy] = {  0,  0, 0,0, 65535 }, /* missing on chip 
>> */
>> NB: align columns with other rows
>
> I have tried with this DT
>
> bat: battery {
> compatible = "simple-battery", "openpandora-battery";
> voltage-min-design-microvolt = <325>;
> energy-full-design-microwatt-hours = <1480>;
> charge-full-design-microamp-hours = <410>;
> };
>
> and here is the result:
>
>> root@letux:~# dmesg|fgrep bq27
>> [   10.391235] bq27xxx_battery_setup
>> [   10.391265] bq27xxx_battery_setup: dm_regs=bf0520e0
>> [   10.393798] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name 
>> attribute, please fix
>> [   10.432678] bq27xxx_battery_settings
>> [   10.432952] bq27xxx_battery_set_config
>> [   10.432952] bq27xxx_battery_unseal
>> [   10.485168] bq27xxx_battery_update_dm_block
>> [   10.485198] bq27xxx-battery 2-0055: update design-capacity to 4100
>
> looks good
>
>> [   10.485229] bq27xxx_battery_update_dm_block
>> [   10.485229] bq27xxx-battery 2-0055: buffer does not match design-energy 
>> dm spec
>
> ok, ignored
>
>> [   10.511718] bq27xxx_battery_update_dm_block
>> [   10.511749] bq27xxx-battery 2-0055: update terminate-voltage to 3250
>
> looks good
>
>> [   10.826446] bq27xxx_battery_seal
>> [   12.150939] bq27xxx_battery_platform_probe
>> [   12.151031] bq27xxx_battery_setup
>> [   12.151062] bq27xxx_battery_setup: dm_regs=  (null)
>> [   12.153411] (NULL device *): hwmon: 'bq27000-battery' is not a valid name 
>> attribute, please fix
>> [   12.154327] bq27xxx_battery_settings
>> [   12.154357] power_supply bq27000-battery: power_supply_get_battery_info 
>> currently only supports devicetree
>> [   12.154388] bq27xxx_battery_settings: power_supply_get_battery_info 
>> failed ret=-6

Is there also a bq27000 chip on this device, or a battery with it
embedded? If so, it doesn't appear to have a DT node (correct for
embedded), hence that warning, which isn't useful in that case. Prob
battery_settings() should do:
if (!di->dev->of_node || power_supply_get_battery_info(...) < 0) return;
altho that would be wrong once get_battery_info() supports ACPI config...

> ... login:
>
>> root@letux:~# cat /sys/class/power_supply/bq27500-1-0/uevent
>> POWER_SUPPLY_NAME=bq27500-1-0
>> POWER_SUPPLY_STATUS=Discharging
>> POWER_SUPPLY_PRESENT=1
>> POWER_SUPPLY_VOLTAGE_NOW=3892000
>> POWER_SUPPLY_CURRENT_NOW=-317000
>> POWER_SUPPLY_CAPACITY=0
>
> oops.
>
>> POWER_SUPPLY_CAPACITY_LEVEL=Low
>> POWER_SUPPLY_TEMP=223
>> POWER_SUPPLY_TIME_TO_EMPTY_NOW=0
>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>> POWER_SUPPLY_CHARGE_FULL=1147000
>> POWER_SUPPLY_CHARGE_NOW=2665000
>> POWER_SUPPLY_CHARGE_FULL_DESIGN=410
>> POWER_SUPPLY_CYCLE_COUNT=6
>> POWER_SUPPLY_ENERGY_NOW=0
>> POWER_SUPPLY_POWER_AVG=64395
>> POWER_SUPPLY_HEALTH=Dead
>
> oops. But maybe the bq27500 needs a full cycle first

I wouldn't guess that the above state is due to the DM update
sequence, but I suppose that's possible. Another update pass might
clarify that.

>> POWER_SUPPLY_MANUFACTURER=Texas Instruments
>
> vvv after plugging in charger
>
>> root@letux:~# cat /sys/class/power_supply/bq27500-1-0/uevent
>> POWER_SUPPLY_NAME=bq27500-1-0
>> POWER_SUPPLY_STATUS=Charging
>> POWER_SUPPLY_PRESENT=1
>> POWER_SUPPLY_VOLTAGE_NOW=3923000
>> POWER_SUPPLY_CURRENT_NOW=204000
>> POWER_SUPPLY_CAPACITY=0
>> POWER_SUPPLY_CAPACITY_LEVEL=Low
>> POWER_SUPPLY_TEMP=249
>> POWER_SUPPLY_TIME_TO_EMPTY_NOW=0
>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>> POWER_SUPPLY_CHARGE_FULL=1147000
>> POWER_SUPPLY_CHARGE_NOW=2665000
>> POWER_SUPPLY_CHARGE_FULL_DESIGN=410
>> POWER_SUPPLY_CYCLE_COUNT=6
>> POWER_SUPPLY_ENERGY_NOW=0
>> POWER_SUPPLY_POWER_AVG=800
>> POWER_SUPPLY_HEALTH=Dead
>> POWER_SUPPLY_MANUFACTURER=Texas Instruments
>> root@letux:~#
>
> Now a reboot after removing charger and battery for several minutes:
>
>> root@letux:~# dmesg|fgrep bq27
>> [   10.482818] bq27xxx_battery_setup
>

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

Hi,

This may be a fix that allows >0 input from DT, but won't try to
program the register since the first 3 fields aren't compatible:

... bq27500_dm_regs[] = {
...
  [bq27xxx_dm_design_energy] = {  0,  0, 0,0, 65535 }, /* missing on chip */
NB: align columns with other rows

Pls CC me on patch posts. You left me off your last series.

Cpl more comments below...

On Thu, Aug 31, 2017 at 2:35 AM, H. Nikolaus Schaller <h...@goldelico.com> 
wrote:
> Hi Liam,
>
>> Am 31.08.2017 um 10:58 schrieb Liam Breck <l...@networkimprov.net>:
>>
>> On Wed, Aug 30, 2017 at 11:58 PM, H. Nikolaus Schaller
>> <h...@goldelico.com> wrote:
>>> Hi Liam,
>>>
>>>> Am 30.08.2017 um 13:24 schrieb Liam Breck <l...@networkimprov.net>:
>>>>
>>>> Hi Nikolaus,
>>>>
>>>> On Wed, Aug 30, 2017 at 2:30 AM, H. Nikolaus Schaller <h...@goldelico.com> 
>>>> wrote:
>>>>> Hi Liam and Sebastian,
>>>>>
>>>>>> Am 29.08.2017 um 22:20 schrieb Liam Breck <l...@networkimprov.net>:
>>>>>>
>>>>>> Hi Nikolaus, thanks for the patch...
>>>>>>
>>>>>> On Tue, Aug 29, 2017 at 1:10 PM, H. Nikolaus Schaller 
>>>>>> <h...@goldelico.com> wrote:
>>>>>>> Tested on Pyra prototype with bq27421.
>>>>>>>
>>>>>>> Signed-off-by: H. Nikolaus Schaller <h...@goldelico.com>
>>>>>>> ---
>>>>>>> drivers/power/supply/bq27xxx_battery.c | 2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c 
>>>>>>> b/drivers/power/supply/bq27xxx_battery.c
>>>>>>> index b6c3120ca5ad..e3c878ef7c7e 100644
>>>>>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>>>>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>>>>>> @@ -688,7 +688,7 @@ static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>>>>>>> #define bq27545_dm_regs 0
>>>>>>> #endif
>>>>>>>
>>>>>>> -#if 0 /* not yet tested */
>>>>>>> +#if 1 /* tested on Pyra */
>>>>>>> static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
>>>>>>
>>>>>> I think we can drop the #if and #else...#endif so it's just the
>>>>>> dm_regs table, as with bq27425.
>>>>>
>>>>> I have fixed that and did take the chance to update my OpenPandora
>>>>> kernel so that I also could test and make the bq27500 working.
>>>>
>>>> Is this gauge on the board or in the battery?
>>>
>>> It is on the board.
>>>
>>>> If in the battery,
>>>> monitored-battery should not be used.
>>>>
>>>> For a gauge on the board, if a user changes the battery to a different
>>>> type, they need to update the dtb.
>>>
>>> Well, that is a little difficult to control, but here we have only
>>> one standard Pandora cell. There may exist replacements with different
>>> capacity, but that should not be our problem...
>>>
>>>>
>>>> I assume you built with config_battery_bq27xxx_dt_updates_nvm?
>>>
>>> Yes.
>>>
>>>>
>>>>> The biggest hurdle was to find out that I have to change the
>>>>> compatible string to "ti,bq27500-1" to get non-NULL dm_regs...
>>>>>
>>>>> [   12.765930] bq27xxx_battery_i2c_probe name=bq27500-1-0
>>>>> [   12.771392] bq27xxx_battery_i2c_probe call setup
>>>>> [   12.874816] bq27xxx_battery_setup
>>>>> [   12.904266] bq27xxx_battery_setup: dm_regs=bf0520e0
>>>>> [   12.933380] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name 
>>>>> attribute, please fix
>>>>> [   13.234893] bq27xxx_battery_settings
>>>>> [   13.238891] bq27xxx-battery 2-0055: invalid 
>>>>> battery:energy-full-design-microwatt-hours 1480
>>>>
>>>> The chip does not support this param, so it should be zero, as it has
>>>> to be set if charge-full-design-* is set. Can you try that?
>>>
>>> Hm. IMHO the DT should describe what the battery has and the driver should 
>>> simply ignore
>>> values it can't handle or reject them but do the best out of it. Telling 
>>> the 

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

Hi,

This may be a fix that allows >0 input from DT, but won't try to
program the register since the first 3 fields aren't compatible:

... bq27500_dm_regs[] = {
...
  [bq27xxx_dm_design_energy] = {  0,  0, 0,0, 65535 }, /* missing on chip */
NB: align columns with other rows

Pls CC me on patch posts. You left me off your last series.

Cpl more comments below...

On Thu, Aug 31, 2017 at 2:35 AM, H. Nikolaus Schaller  
wrote:
> Hi Liam,
>
>> Am 31.08.2017 um 10:58 schrieb Liam Breck :
>>
>> On Wed, Aug 30, 2017 at 11:58 PM, H. Nikolaus Schaller
>>  wrote:
>>> Hi Liam,
>>>
>>>> Am 30.08.2017 um 13:24 schrieb Liam Breck :
>>>>
>>>> Hi Nikolaus,
>>>>
>>>> On Wed, Aug 30, 2017 at 2:30 AM, H. Nikolaus Schaller  
>>>> wrote:
>>>>> Hi Liam and Sebastian,
>>>>>
>>>>>> Am 29.08.2017 um 22:20 schrieb Liam Breck :
>>>>>>
>>>>>> Hi Nikolaus, thanks for the patch...
>>>>>>
>>>>>> On Tue, Aug 29, 2017 at 1:10 PM, H. Nikolaus Schaller 
>>>>>>  wrote:
>>>>>>> Tested on Pyra prototype with bq27421.
>>>>>>>
>>>>>>> Signed-off-by: H. Nikolaus Schaller 
>>>>>>> ---
>>>>>>> drivers/power/supply/bq27xxx_battery.c | 2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c 
>>>>>>> b/drivers/power/supply/bq27xxx_battery.c
>>>>>>> index b6c3120ca5ad..e3c878ef7c7e 100644
>>>>>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>>>>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>>>>>> @@ -688,7 +688,7 @@ static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>>>>>>> #define bq27545_dm_regs 0
>>>>>>> #endif
>>>>>>>
>>>>>>> -#if 0 /* not yet tested */
>>>>>>> +#if 1 /* tested on Pyra */
>>>>>>> static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
>>>>>>
>>>>>> I think we can drop the #if and #else...#endif so it's just the
>>>>>> dm_regs table, as with bq27425.
>>>>>
>>>>> I have fixed that and did take the chance to update my OpenPandora
>>>>> kernel so that I also could test and make the bq27500 working.
>>>>
>>>> Is this gauge on the board or in the battery?
>>>
>>> It is on the board.
>>>
>>>> If in the battery,
>>>> monitored-battery should not be used.
>>>>
>>>> For a gauge on the board, if a user changes the battery to a different
>>>> type, they need to update the dtb.
>>>
>>> Well, that is a little difficult to control, but here we have only
>>> one standard Pandora cell. There may exist replacements with different
>>> capacity, but that should not be our problem...
>>>
>>>>
>>>> I assume you built with config_battery_bq27xxx_dt_updates_nvm?
>>>
>>> Yes.
>>>
>>>>
>>>>> The biggest hurdle was to find out that I have to change the
>>>>> compatible string to "ti,bq27500-1" to get non-NULL dm_regs...
>>>>>
>>>>> [   12.765930] bq27xxx_battery_i2c_probe name=bq27500-1-0
>>>>> [   12.771392] bq27xxx_battery_i2c_probe call setup
>>>>> [   12.874816] bq27xxx_battery_setup
>>>>> [   12.904266] bq27xxx_battery_setup: dm_regs=bf0520e0
>>>>> [   12.933380] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name 
>>>>> attribute, please fix
>>>>> [   13.234893] bq27xxx_battery_settings
>>>>> [   13.238891] bq27xxx-battery 2-0055: invalid 
>>>>> battery:energy-full-design-microwatt-hours 1480
>>>>
>>>> The chip does not support this param, so it should be zero, as it has
>>>> to be set if charge-full-design-* is set. Can you try that?
>>>
>>> Hm. IMHO the DT should describe what the battery has and the driver should 
>>> simply ignore
>>> values it can't handle or reject them but do the best out of it. Telling 
>>> the driver to ignore
>>> a value by setting to 0 would IMHO be the wrong approach.
>>
>> The driver requires that if either charge- or energy-full-design is
>> set, then the other must be. Setting one

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

On Wed, Aug 30, 2017 at 11:58 PM, H. Nikolaus Schaller
<h...@goldelico.com> wrote:
> Hi Liam,
>
>> Am 30.08.2017 um 13:24 schrieb Liam Breck <l...@networkimprov.net>:
>>
>> Hi Nikolaus,
>>
>> On Wed, Aug 30, 2017 at 2:30 AM, H. Nikolaus Schaller <h...@goldelico.com> 
>> wrote:
>>> Hi Liam and Sebastian,
>>>
>>>> Am 29.08.2017 um 22:20 schrieb Liam Breck <l...@networkimprov.net>:
>>>>
>>>> Hi Nikolaus, thanks for the patch...
>>>>
>>>> On Tue, Aug 29, 2017 at 1:10 PM, H. Nikolaus Schaller <h...@goldelico.com> 
>>>> wrote:
>>>>> Tested on Pyra prototype with bq27421.
>>>>>
>>>>> Signed-off-by: H. Nikolaus Schaller <h...@goldelico.com>
>>>>> ---
>>>>> drivers/power/supply/bq27xxx_battery.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c 
>>>>> b/drivers/power/supply/bq27xxx_battery.c
>>>>> index b6c3120ca5ad..e3c878ef7c7e 100644
>>>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>>>> @@ -688,7 +688,7 @@ static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>>>>> #define bq27545_dm_regs 0
>>>>> #endif
>>>>>
>>>>> -#if 0 /* not yet tested */
>>>>> +#if 1 /* tested on Pyra */
>>>>> static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
>>>>
>>>> I think we can drop the #if and #else...#endif so it's just the
>>>> dm_regs table, as with bq27425.
>>>
>>> I have fixed that and did take the chance to update my OpenPandora
>>> kernel so that I also could test and make the bq27500 working.
>>
>> Is this gauge on the board or in the battery?
>
> It is on the board.
>
>> If in the battery,
>> monitored-battery should not be used.
>>
>> For a gauge on the board, if a user changes the battery to a different
>> type, they need to update the dtb.
>
> Well, that is a little difficult to control, but here we have only
> one standard Pandora cell. There may exist replacements with different
> capacity, but that should not be our problem...
>
>>
>> I assume you built with config_battery_bq27xxx_dt_updates_nvm?
>
> Yes.
>
>>
>>> The biggest hurdle was to find out that I have to change the
>>> compatible string to "ti,bq27500-1" to get non-NULL dm_regs...
>>>
>>> [   12.765930] bq27xxx_battery_i2c_probe name=bq27500-1-0
>>> [   12.771392] bq27xxx_battery_i2c_probe call setup
>>> [   12.874816] bq27xxx_battery_setup
>>> [   12.904266] bq27xxx_battery_setup: dm_regs=bf0520e0
>>> [   12.933380] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name 
>>> attribute, please fix
>>> [   13.234893] bq27xxx_battery_settings
>>> [   13.238891] bq27xxx-battery 2-0055: invalid 
>>> battery:energy-full-design-microwatt-hours 1480
>>
>> The chip does not support this param, so it should be zero, as it has
>> to be set if charge-full-design-* is set. Can you try that?
>
> Hm. IMHO the DT should describe what the battery has and the driver should 
> simply ignore
> values it can't handle or reject them but do the best out of it. Telling the 
> driver to ignore
> a value by setting to 0 would IMHO be the wrong approach.

The driver requires that if either charge- or energy-full-design is
set, then the other must be. Setting one and leaving the other at
default would be an error. Allowing any value for a field unsupported
by the chip, when supported field values must be within a range, isn't
useful for hw development or production scenarios. The solution for
shipped equipment is to drop the monitored-battery ref; see below.

> We are also working on the generic-adc-battery driver that makes use of the 
> same battery DT
> node so that people can choose if they want to configure a kernel for fuel 
> gauge or
> g-a-b or even both.
>
>>
>>> [   13.312469] bq27xxx_battery_set_config
>>> [   13.407745] bq27xxx_battery_unseal
>>> [   13.455993] bq27xxx_battery_update_dm_block
>>> [   13.460418] bq27xxx-battery 2-0055: update terminate-voltage to 3200
>>> [   13.694885] bq27xxx_battery_seal
>>
>> We need to see output after a reboot.
>
> This was the value after first boot with the new driver enabled.
>
> A second boot after removing and reinserting battery shows:
>
> [   11.2

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

On Wed, Aug 30, 2017 at 11:58 PM, H. Nikolaus Schaller
 wrote:
> Hi Liam,
>
>> Am 30.08.2017 um 13:24 schrieb Liam Breck :
>>
>> Hi Nikolaus,
>>
>> On Wed, Aug 30, 2017 at 2:30 AM, H. Nikolaus Schaller  
>> wrote:
>>> Hi Liam and Sebastian,
>>>
>>>> Am 29.08.2017 um 22:20 schrieb Liam Breck :
>>>>
>>>> Hi Nikolaus, thanks for the patch...
>>>>
>>>> On Tue, Aug 29, 2017 at 1:10 PM, H. Nikolaus Schaller  
>>>> wrote:
>>>>> Tested on Pyra prototype with bq27421.
>>>>>
>>>>> Signed-off-by: H. Nikolaus Schaller 
>>>>> ---
>>>>> drivers/power/supply/bq27xxx_battery.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c 
>>>>> b/drivers/power/supply/bq27xxx_battery.c
>>>>> index b6c3120ca5ad..e3c878ef7c7e 100644
>>>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>>>> @@ -688,7 +688,7 @@ static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>>>>> #define bq27545_dm_regs 0
>>>>> #endif
>>>>>
>>>>> -#if 0 /* not yet tested */
>>>>> +#if 1 /* tested on Pyra */
>>>>> static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
>>>>
>>>> I think we can drop the #if and #else...#endif so it's just the
>>>> dm_regs table, as with bq27425.
>>>
>>> I have fixed that and did take the chance to update my OpenPandora
>>> kernel so that I also could test and make the bq27500 working.
>>
>> Is this gauge on the board or in the battery?
>
> It is on the board.
>
>> If in the battery,
>> monitored-battery should not be used.
>>
>> For a gauge on the board, if a user changes the battery to a different
>> type, they need to update the dtb.
>
> Well, that is a little difficult to control, but here we have only
> one standard Pandora cell. There may exist replacements with different
> capacity, but that should not be our problem...
>
>>
>> I assume you built with config_battery_bq27xxx_dt_updates_nvm?
>
> Yes.
>
>>
>>> The biggest hurdle was to find out that I have to change the
>>> compatible string to "ti,bq27500-1" to get non-NULL dm_regs...
>>>
>>> [   12.765930] bq27xxx_battery_i2c_probe name=bq27500-1-0
>>> [   12.771392] bq27xxx_battery_i2c_probe call setup
>>> [   12.874816] bq27xxx_battery_setup
>>> [   12.904266] bq27xxx_battery_setup: dm_regs=bf0520e0
>>> [   12.933380] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name 
>>> attribute, please fix
>>> [   13.234893] bq27xxx_battery_settings
>>> [   13.238891] bq27xxx-battery 2-0055: invalid 
>>> battery:energy-full-design-microwatt-hours 1480
>>
>> The chip does not support this param, so it should be zero, as it has
>> to be set if charge-full-design-* is set. Can you try that?
>
> Hm. IMHO the DT should describe what the battery has and the driver should 
> simply ignore
> values it can't handle or reject them but do the best out of it. Telling the 
> driver to ignore
> a value by setting to 0 would IMHO be the wrong approach.

The driver requires that if either charge- or energy-full-design is
set, then the other must be. Setting one and leaving the other at
default would be an error. Allowing any value for a field unsupported
by the chip, when supported field values must be within a range, isn't
useful for hw development or production scenarios. The solution for
shipped equipment is to drop the monitored-battery ref; see below.

> We are also working on the generic-adc-battery driver that makes use of the 
> same battery DT
> node so that people can choose if they want to configure a kernel for fuel 
> gauge or
> g-a-b or even both.
>
>>
>>> [   13.312469] bq27xxx_battery_set_config
>>> [   13.407745] bq27xxx_battery_unseal
>>> [   13.455993] bq27xxx_battery_update_dm_block
>>> [   13.460418] bq27xxx-battery 2-0055: update terminate-voltage to 3200
>>> [   13.694885] bq27xxx_battery_seal
>>
>> We need to see output after a reboot.
>
> This was the value after first boot with the new driver enabled.
>
> A second boot after removing and reinserting battery shows:
>
> [   11.256713] bq27xxx_battery_setup
> [   11.256744] bq27xxx_battery_setup: dm_regs=bf05b0e0
> [   11.257690] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

Hi Nikolaus,

On Wed, Aug 30, 2017 at 2:30 AM, H. Nikolaus Schaller <h...@goldelico.com> 
wrote:
> Hi Liam and Sebastian,
>
>> Am 29.08.2017 um 22:20 schrieb Liam Breck <l...@networkimprov.net>:
>>
>> Hi Nikolaus, thanks for the patch...
>>
>> On Tue, Aug 29, 2017 at 1:10 PM, H. Nikolaus Schaller <h...@goldelico.com> 
>> wrote:
>>> Tested on Pyra prototype with bq27421.
>>>
>>> Signed-off-by: H. Nikolaus Schaller <h...@goldelico.com>
>>> ---
>>> drivers/power/supply/bq27xxx_battery.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/power/supply/bq27xxx_battery.c 
>>> b/drivers/power/supply/bq27xxx_battery.c
>>> index b6c3120ca5ad..e3c878ef7c7e 100644
>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>> @@ -688,7 +688,7 @@ static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>>> #define bq27545_dm_regs 0
>>> #endif
>>>
>>> -#if 0 /* not yet tested */
>>> +#if 1 /* tested on Pyra */
>>> static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
>>
>> I think we can drop the #if and #else...#endif so it's just the
>> dm_regs table, as with bq27425.
>
> I have fixed that and did take the chance to update my OpenPandora
> kernel so that I also could test and make the bq27500 working.

Is this gauge on the board or in the battery? If in the battery,
monitored-battery should not be used.

For a gauge on the board, if a user changes the battery to a different
type, they need to update the dtb.

I assume you built with config_battery_bq27xxx_dt_updates_nvm?

> The biggest hurdle was to find out that I have to change the
> compatible string to "ti,bq27500-1" to get non-NULL dm_regs...
>
> [   12.765930] bq27xxx_battery_i2c_probe name=bq27500-1-0
> [   12.771392] bq27xxx_battery_i2c_probe call setup
> [   12.874816] bq27xxx_battery_setup
> [   12.904266] bq27xxx_battery_setup: dm_regs=bf0520e0
> [   12.933380] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name 
> attribute, please fix
> [   13.234893] bq27xxx_battery_settings
> [   13.238891] bq27xxx-battery 2-0055: invalid 
> battery:energy-full-design-microwatt-hours 1480

The chip does not support this param, so it should be zero, as it has
to be set if charge-full-design-* is set. Can you try that?

> [   13.312469] bq27xxx_battery_set_config
> [   13.407745] bq27xxx_battery_unseal
> [   13.455993] bq27xxx_battery_update_dm_block
> [   13.460418] bq27xxx-battery 2-0055: update terminate-voltage to 3200
> [   13.694885] bq27xxx_battery_seal

We need to see output after a reboot. Note that this chip has NVM so
these settings persist without power.

> Does this look ok for
>
> bat: battery {
> compatible = "simple-battery", "openpandora-battery";
> voltage-min-design-microvolt = <320>;
> energy-full-design-microwatt-hours = <1480>;
> charge-full-design-microamp-hours = <400>;
> };
>
> ?
>
> I will send a patch for enabling both fuel gauges and the 
> omap3-pandora-common.dtsi asap.

You probably don't want this in upstream dts, as it only programs the
gauge if the kernel has the above config option. If the box runs a
stock distro, it won't work. A custom-built kernel with the above dts
programs the gauge unless the user sets the module param
dt_monitored_battery_updates_nvm=0. The requisite dtb should be
packaged with the custom-built kernel, and deployed with awareness of
the actual battery present.

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

Hi Nikolaus,

On Wed, Aug 30, 2017 at 2:30 AM, H. Nikolaus Schaller  
wrote:
> Hi Liam and Sebastian,
>
>> Am 29.08.2017 um 22:20 schrieb Liam Breck :
>>
>> Hi Nikolaus, thanks for the patch...
>>
>> On Tue, Aug 29, 2017 at 1:10 PM, H. Nikolaus Schaller  
>> wrote:
>>> Tested on Pyra prototype with bq27421.
>>>
>>> Signed-off-by: H. Nikolaus Schaller 
>>> ---
>>> drivers/power/supply/bq27xxx_battery.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/power/supply/bq27xxx_battery.c 
>>> b/drivers/power/supply/bq27xxx_battery.c
>>> index b6c3120ca5ad..e3c878ef7c7e 100644
>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>> @@ -688,7 +688,7 @@ static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>>> #define bq27545_dm_regs 0
>>> #endif
>>>
>>> -#if 0 /* not yet tested */
>>> +#if 1 /* tested on Pyra */
>>> static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
>>
>> I think we can drop the #if and #else...#endif so it's just the
>> dm_regs table, as with bq27425.
>
> I have fixed that and did take the chance to update my OpenPandora
> kernel so that I also could test and make the bq27500 working.

Is this gauge on the board or in the battery? If in the battery,
monitored-battery should not be used.

For a gauge on the board, if a user changes the battery to a different
type, they need to update the dtb.

I assume you built with config_battery_bq27xxx_dt_updates_nvm?

> The biggest hurdle was to find out that I have to change the
> compatible string to "ti,bq27500-1" to get non-NULL dm_regs...
>
> [   12.765930] bq27xxx_battery_i2c_probe name=bq27500-1-0
> [   12.771392] bq27xxx_battery_i2c_probe call setup
> [   12.874816] bq27xxx_battery_setup
> [   12.904266] bq27xxx_battery_setup: dm_regs=bf0520e0
> [   12.933380] (NULL device *): hwmon: 'bq27500-1-0' is not a valid name 
> attribute, please fix
> [   13.234893] bq27xxx_battery_settings
> [   13.238891] bq27xxx-battery 2-0055: invalid 
> battery:energy-full-design-microwatt-hours 1480

The chip does not support this param, so it should be zero, as it has
to be set if charge-full-design-* is set. Can you try that?

> [   13.312469] bq27xxx_battery_set_config
> [   13.407745] bq27xxx_battery_unseal
> [   13.455993] bq27xxx_battery_update_dm_block
> [   13.460418] bq27xxx-battery 2-0055: update terminate-voltage to 3200
> [   13.694885] bq27xxx_battery_seal

We need to see output after a reboot. Note that this chip has NVM so
these settings persist without power.

> Does this look ok for
>
> bat: battery {
> compatible = "simple-battery", "openpandora-battery";
> voltage-min-design-microvolt = <320>;
> energy-full-design-microwatt-hours = <1480>;
> charge-full-design-microamp-hours = <400>;
> };
>
> ?
>
> I will send a patch for enabling both fuel gauges and the 
> omap3-pandora-common.dtsi asap.

You probably don't want this in upstream dts, as it only programs the
gauge if the kernel has the above config option. If the box runs a
stock distro, it won't work. A custom-built kernel with the above dts
programs the gauge unless the user sets the module param
dt_monitored_battery_updates_nvm=0. The requisite dtb should be
packaged with the custom-built kernel, and deployed with awareness of
the actual battery present.

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

Hi Nikolaus, thanks for the patch...

On Tue, Aug 29, 2017 at 1:10 PM, H. Nikolaus Schaller  
wrote:
> Tested on Pyra prototype with bq27421.
>
> Signed-off-by: H. Nikolaus Schaller 
> ---
>  drivers/power/supply/bq27xxx_battery.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c 
> b/drivers/power/supply/bq27xxx_battery.c
> index b6c3120ca5ad..e3c878ef7c7e 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -688,7 +688,7 @@ static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>  #define bq27545_dm_regs 0
>  #endif
>
> -#if 0 /* not yet tested */
> +#if 1 /* tested on Pyra */
>  static struct bq27xxx_dm_reg bq27421_dm_regs[] = {

I think we can drop the #if and #else...#endif so it's just the
dm_regs table, as with bq27425.

Re: [PATCH resend v2] power: supply: bq27xxx: enable writing capacity values for bq27421

[Liam Breck]

Hi Nikolaus, thanks for the patch...

On Tue, Aug 29, 2017 at 1:10 PM, H. Nikolaus Schaller  
wrote:
> Tested on Pyra prototype with bq27421.
>
> Signed-off-by: H. Nikolaus Schaller 
> ---
>  drivers/power/supply/bq27xxx_battery.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c 
> b/drivers/power/supply/bq27xxx_battery.c
> index b6c3120ca5ad..e3c878ef7c7e 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -688,7 +688,7 @@ static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>  #define bq27545_dm_regs 0
>  #endif
>
> -#if 0 /* not yet tested */
> +#if 1 /* tested on Pyra */
>  static struct bq27xxx_dm_reg bq27421_dm_regs[] = {

I think we can drop the #if and #else...#endif so it's just the
dm_regs table, as with bq27425.

Re: dt-bindings: power: supply: bq27xxx: Add monitored-battery documentation

[Liam Breck]

Hi,

On Tue, Aug 29, 2017 at 12:50 PM, H. Nikolaus Schaller
<h...@goldelico.com> wrote:
> Hi Liam,
>
>> Am 29.08.2017 um 20:18 schrieb Liam Breck <l...@networkimprov.net>:
>>
>> Hi Nikolaus, thanks for the testing report...
>>
>> On Tue, Aug 29, 2017 at 7:24 AM, H. Nikolaus Schaller <h...@goldelico.com> 
>> wrote:
>>> Hi Liam,
>>> seems to work as expected :)
>>>
>>> First boot:
>>>
>>> [6.096336] bq27xxx_battery_settings
>>> [6.097987] bq27xxx_battery_set_config
>>> [6.097990] bq27xxx_battery_unseal
>>> [6.107987] bq27xxx-battery 1-0055: update design-capacity to 6000
>>> [6.107992] bq27xxx-battery 1-0055: update design-energy to 22200
>>> [6.107997] bq27xxx-battery 1-0055: terminate-voltage has 3200
>>> [6.382048] bq27xxx-battery 1-0055: cfgupdate 0, retries 7
>>> [6.382054] bq27xxx_battery_seal
>>>
>>> Second boot (w/o removing battery in between):
>>>
>>> [6.008883] bq27xxx_battery_settings
>>> [6.013280] bq27xxx_battery_set_config
>>> [6.018761] bq27xxx_battery_unseal
>>> [6.050694] bq27xxx-battery 1-0055: design-capacity has 6000
>>> [6.059913] bq27xxx-battery 1-0055: design-energy has 22200
>>> [6.067113] bq27xxx-battery 1-0055: terminate-voltage has 3200
>>> [6.075803] bq27xxx_battery_seal
>>
>> I'd be interested to hear what happens after poweroff & boot (ie does
>> it retain config)?
>
> second boot was simple poweroff + reboot and it changes from "update 
> design-capacity" to "design-capacity has"
> which IMHO means that the bq27421 did retain settings and was therefore not 
> reprogrammed.

That implies that the board is keeping the battery gauge powered
during poweroff. That's not a problem per se, but could indicate that
other subsystems are also powered...

>>
>>> Third boot (with removing battery in between):
>>>
>>> [6.161085] bq27xxx_battery_settings
>>> [6.161162] bq27xxx_battery_set_config
>>> [6.161165] bq27xxx_battery_unseal
>>> [6.177904] bq27xxx-battery 1-0055: update design-capacity to 6000
>>> [6.177909] bq27xxx-battery 1-0055: update design-energy to 22200
>>> [6.177914] bq27xxx-battery 1-0055: terminate-voltage has 3200
>>> [6.440592] bq27xxx-battery 1-0055: cfgupdate 0, retries 7
>>> [6.440597] bq27xxx_battery_seal
>
> this was with removing all power which makes the bq27421 loosing the settings.
> And next reboot does another "update design-capacity".
>
>>>
>>> (I have added some printk to trace seal/unseal etc.)
>>>
>>> Values match the 6000mAh specified by DT:
>>>
>>>bat: battery {
>>>compatible = "simple-battery", "pyra-battery";
>>>voltage-min-design-microvolt = <320>;
>>>energy-full-design-microwatt-hours = <2220>;
>>>charge-full-design-microamp-hours = <600>;
>>>};
>>>
>>> root@letux:~# cat /sys/class/power_supply/bq27421-0/uevent
>>> POWER_SUPPLY_NAME=bq27421-0
>>> POWER_SUPPLY_STATUS=Discharging
>>> POWER_SUPPLY_PRESENT=1
>>> POWER_SUPPLY_VOLTAGE_NOW=3943000
>>> POWER_SUPPLY_CURRENT_NOW=-634000
>>> POWER_SUPPLY_CAPACITY=82
>>> POWER_SUPPLY_CAPACITY_LEVEL=Normal
>>> POWER_SUPPLY_TEMP=319
>>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>>> POWER_SUPPLY_CHARGE_FULL=5467000
>>> POWER_SUPPLY_CHARGE_NOW=4611000
>>> POWER_SUPPLY_CHARGE_FULL_DESIGN=600
>>> POWER_SUPPLY_MANUFACTURER=Texas Instruments
>>> root@letux:~#
>>>
>>> Anything else I should test?
>>
>> Could you also test voltage-min-design-microvolt at 3177 to make sure
>> that's configurable too?
>
> Yes:
>
> bat: battery {
> compatible = "simple-battery", "pyra-battery";
> voltage-min-design-microvolt = <3177000>;
> energy-full-design-microwatt-hours = <2220>;
>     charge-full-design-microamp-hours = <600>;
> };
>
> gives
>
> [6.241009] bq27xxx_battery_settings
> [6.241055] bq27xxx_battery_set_config
> [6.241058] bq27xxx_battery_unseal
> [6.257073] bq27xxx-battery 1-0055: update design-capacity to 6000
> [6.257078] bq27xxx-battery 1-0055: update design-energy to 22200
> [6.257084] bq27xxx-battery

Re: dt-bindings: power: supply: bq27xxx: Add monitored-battery documentation

[Liam Breck]

Hi,

On Tue, Aug 29, 2017 at 12:50 PM, H. Nikolaus Schaller
 wrote:
> Hi Liam,
>
>> Am 29.08.2017 um 20:18 schrieb Liam Breck :
>>
>> Hi Nikolaus, thanks for the testing report...
>>
>> On Tue, Aug 29, 2017 at 7:24 AM, H. Nikolaus Schaller  
>> wrote:
>>> Hi Liam,
>>> seems to work as expected :)
>>>
>>> First boot:
>>>
>>> [6.096336] bq27xxx_battery_settings
>>> [6.097987] bq27xxx_battery_set_config
>>> [6.097990] bq27xxx_battery_unseal
>>> [6.107987] bq27xxx-battery 1-0055: update design-capacity to 6000
>>> [6.107992] bq27xxx-battery 1-0055: update design-energy to 22200
>>> [6.107997] bq27xxx-battery 1-0055: terminate-voltage has 3200
>>> [6.382048] bq27xxx-battery 1-0055: cfgupdate 0, retries 7
>>> [6.382054] bq27xxx_battery_seal
>>>
>>> Second boot (w/o removing battery in between):
>>>
>>> [6.008883] bq27xxx_battery_settings
>>> [6.013280] bq27xxx_battery_set_config
>>> [6.018761] bq27xxx_battery_unseal
>>> [6.050694] bq27xxx-battery 1-0055: design-capacity has 6000
>>> [6.059913] bq27xxx-battery 1-0055: design-energy has 22200
>>> [6.067113] bq27xxx-battery 1-0055: terminate-voltage has 3200
>>> [6.075803] bq27xxx_battery_seal
>>
>> I'd be interested to hear what happens after poweroff & boot (ie does
>> it retain config)?
>
> second boot was simple poweroff + reboot and it changes from "update 
> design-capacity" to "design-capacity has"
> which IMHO means that the bq27421 did retain settings and was therefore not 
> reprogrammed.

That implies that the board is keeping the battery gauge powered
during poweroff. That's not a problem per se, but could indicate that
other subsystems are also powered...

>>
>>> Third boot (with removing battery in between):
>>>
>>> [6.161085] bq27xxx_battery_settings
>>> [6.161162] bq27xxx_battery_set_config
>>> [6.161165] bq27xxx_battery_unseal
>>> [6.177904] bq27xxx-battery 1-0055: update design-capacity to 6000
>>> [6.177909] bq27xxx-battery 1-0055: update design-energy to 22200
>>> [6.177914] bq27xxx-battery 1-0055: terminate-voltage has 3200
>>> [6.440592] bq27xxx-battery 1-0055: cfgupdate 0, retries 7
>>> [6.440597] bq27xxx_battery_seal
>
> this was with removing all power which makes the bq27421 loosing the settings.
> And next reboot does another "update design-capacity".
>
>>>
>>> (I have added some printk to trace seal/unseal etc.)
>>>
>>> Values match the 6000mAh specified by DT:
>>>
>>>bat: battery {
>>>compatible = "simple-battery", "pyra-battery";
>>>voltage-min-design-microvolt = <320>;
>>>energy-full-design-microwatt-hours = <2220>;
>>>charge-full-design-microamp-hours = <600>;
>>>};
>>>
>>> root@letux:~# cat /sys/class/power_supply/bq27421-0/uevent
>>> POWER_SUPPLY_NAME=bq27421-0
>>> POWER_SUPPLY_STATUS=Discharging
>>> POWER_SUPPLY_PRESENT=1
>>> POWER_SUPPLY_VOLTAGE_NOW=3943000
>>> POWER_SUPPLY_CURRENT_NOW=-634000
>>> POWER_SUPPLY_CAPACITY=82
>>> POWER_SUPPLY_CAPACITY_LEVEL=Normal
>>> POWER_SUPPLY_TEMP=319
>>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>>> POWER_SUPPLY_CHARGE_FULL=5467000
>>> POWER_SUPPLY_CHARGE_NOW=4611000
>>> POWER_SUPPLY_CHARGE_FULL_DESIGN=600
>>> POWER_SUPPLY_MANUFACTURER=Texas Instruments
>>> root@letux:~#
>>>
>>> Anything else I should test?
>>
>> Could you also test voltage-min-design-microvolt at 3177 to make sure
>> that's configurable too?
>
> Yes:
>
> bat: battery {
> compatible = "simple-battery", "pyra-battery";
> voltage-min-design-microvolt = <3177000>;
> energy-full-design-microwatt-hours = <2220>;
> charge-full-design-microamp-hours = <600>;
> };
>
> gives
>
> [6.241009] bq27xxx_battery_settings
> [6.241055] bq27xxx_battery_set_config
> [6.241058] bq27xxx_battery_unseal
> [6.257073] bq27xxx-battery 1-0055: update design-capacity to 6000
> [6.257078] bq27xxx-battery 1-0055: update design-energy to 22200
> [6.257084] bq27xxx-battery 1-0055: update terminate-voltage to 3177
> [6.379230] bq27xxx_battery_sea

Re: dt-bindings: power: supply: bq27xxx: Add monitored-battery documentation

[Liam Breck]

Hi Nikolaus, thanks for the testing report...

On Tue, Aug 29, 2017 at 7:24 AM, H. Nikolaus Schaller <h...@goldelico.com> 
wrote:
> Hi Liam,
> seems to work as expected :)
>
> First boot:
>
> [6.096336] bq27xxx_battery_settings
> [6.097987] bq27xxx_battery_set_config
> [6.097990] bq27xxx_battery_unseal
> [6.107987] bq27xxx-battery 1-0055: update design-capacity to 6000
> [6.107992] bq27xxx-battery 1-0055: update design-energy to 22200
> [6.107997] bq27xxx-battery 1-0055: terminate-voltage has 3200
> [6.382048] bq27xxx-battery 1-0055: cfgupdate 0, retries 7
> [6.382054] bq27xxx_battery_seal
>
> Second boot (w/o removing battery in between):
>
> [6.008883] bq27xxx_battery_settings
> [6.013280] bq27xxx_battery_set_config
> [6.018761] bq27xxx_battery_unseal
> [6.050694] bq27xxx-battery 1-0055: design-capacity has 6000
> [6.059913] bq27xxx-battery 1-0055: design-energy has 22200
> [6.067113] bq27xxx-battery 1-0055: terminate-voltage has 3200
> [6.075803] bq27xxx_battery_seal

I'd be interested to hear what happens after poweroff & boot (ie does
it retain config)?


> Third boot (with removing battery in between):
>
> [6.161085] bq27xxx_battery_settings
> [6.161162] bq27xxx_battery_set_config
> [6.161165] bq27xxx_battery_unseal
> [6.177904] bq27xxx-battery 1-0055: update design-capacity to 6000
> [6.177909] bq27xxx-battery 1-0055: update design-energy to 22200
> [6.177914] bq27xxx-battery 1-0055: terminate-voltage has 3200
> [6.440592] bq27xxx-battery 1-0055: cfgupdate 0, retries 7
> [6.440597] bq27xxx_battery_seal
>
> (I have added some printk to trace seal/unseal etc.)
>
> Values match the 6000mAh specified by DT:
>
> bat: battery {
> compatible = "simple-battery", "pyra-battery";
> voltage-min-design-microvolt = <320>;
> energy-full-design-microwatt-hours = <2220>;
> charge-full-design-microamp-hours = <600>;
> };
>
> root@letux:~# cat /sys/class/power_supply/bq27421-0/uevent
> POWER_SUPPLY_NAME=bq27421-0
> POWER_SUPPLY_STATUS=Discharging
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_VOLTAGE_NOW=3943000
> POWER_SUPPLY_CURRENT_NOW=-634000
> POWER_SUPPLY_CAPACITY=82
> POWER_SUPPLY_CAPACITY_LEVEL=Normal
> POWER_SUPPLY_TEMP=319
> POWER_SUPPLY_TECHNOLOGY=Li-ion
> POWER_SUPPLY_CHARGE_FULL=5467000
> POWER_SUPPLY_CHARGE_NOW=4611000
> POWER_SUPPLY_CHARGE_FULL_DESIGN=600
> POWER_SUPPLY_MANUFACTURER=Texas Instruments
> root@letux:~#
>
> Anything else I should test?

Could you also test voltage-min-design-microvolt at 3177 to make sure
that's configurable too?

Sebastian just queued my series to -next. Would you be inclined to
submit a new patch enabling bq27421? If so, the sooner the better
since merge window is imminent :-)

Liam


>
>
>> Am 29.08.2017 um 12:40 schrieb Liam Breck <l...@networkimprov.net>:
>>
>> Hi Nikolaus,
>>
>> You need the patchset which enables this feature, which was delayed
>> for further work. It's now ready...
>>
>> https://patchwork.kernel.org/patch/9918947/
>> https://patchwork.kernel.org/patch/9918949/
>> https://patchwork.kernel.org/patch/9918951/
>> https://patchwork.kernel.org/patch/9918953/
>> https://patchwork.kernel.org/patch/9918955/
>>
>> And flip the #if 0 protecting bq27421_dm_regs
>>
>> I'd love to support that chip in this patchset if you can test
>> non-default settings for all 3 options this week?
>>
>> Thanks,
>> Liam
>>
>>
>> On Tue, Aug 29, 2017 at 2:43 AM, H. Nikolaus Schaller <h...@goldelico.com> 
>> wrote:
>>> Hi,
>>> I am trying to get this working on our bq27421.
>>>
>>> But the only message I get is:
>>>
>>> [6.086407] bq27xxx-battery 1-0055: data memory update not supported for 
>>> chip
>>>
>>> A little research shows that this message comes from
>>>
>>>
>>> http://elixir.free-electrons.com/linux/v4.13-rc7/source/drivers/power/supply/bq27xxx_battery.c#L1279
>>>
>>> So di->dm_regs is NULL.
>>>
>>> But doing an fgrep for dm_regs shows no line of code where the pointer is 
>>> set to a non-null value:
>>>
>>> master hns$ fgrep -R dm_regs *
>>> drivers/power/supply/bq27xxx_battery.c: .class = 
>>> (di)->dm_regs[i].subclass_id, \
>>> drivers/power/supply/bq27xxx_battery.c: .block = (di)->dm_regs[i].offset / 
>>> BQ27XXX_DM_SZ, \
>>> drivers/power/supply/bq27xxx_battery.

Re: dt-bindings: power: supply: bq27xxx: Add monitored-battery documentation

[Liam Breck]

Hi Nikolaus, thanks for the testing report...

On Tue, Aug 29, 2017 at 7:24 AM, H. Nikolaus Schaller  
wrote:
> Hi Liam,
> seems to work as expected :)
>
> First boot:
>
> [6.096336] bq27xxx_battery_settings
> [6.097987] bq27xxx_battery_set_config
> [6.097990] bq27xxx_battery_unseal
> [6.107987] bq27xxx-battery 1-0055: update design-capacity to 6000
> [6.107992] bq27xxx-battery 1-0055: update design-energy to 22200
> [6.107997] bq27xxx-battery 1-0055: terminate-voltage has 3200
> [6.382048] bq27xxx-battery 1-0055: cfgupdate 0, retries 7
> [6.382054] bq27xxx_battery_seal
>
> Second boot (w/o removing battery in between):
>
> [6.008883] bq27xxx_battery_settings
> [6.013280] bq27xxx_battery_set_config
> [6.018761] bq27xxx_battery_unseal
> [6.050694] bq27xxx-battery 1-0055: design-capacity has 6000
> [6.059913] bq27xxx-battery 1-0055: design-energy has 22200
> [6.067113] bq27xxx-battery 1-0055: terminate-voltage has 3200
> [6.075803] bq27xxx_battery_seal

I'd be interested to hear what happens after poweroff & boot (ie does
it retain config)?


> Third boot (with removing battery in between):
>
> [6.161085] bq27xxx_battery_settings
> [6.161162] bq27xxx_battery_set_config
> [6.161165] bq27xxx_battery_unseal
> [6.177904] bq27xxx-battery 1-0055: update design-capacity to 6000
> [6.177909] bq27xxx-battery 1-0055: update design-energy to 22200
> [6.177914] bq27xxx-battery 1-0055: terminate-voltage has 3200
> [6.440592] bq27xxx-battery 1-0055: cfgupdate 0, retries 7
> [6.440597] bq27xxx_battery_seal
>
> (I have added some printk to trace seal/unseal etc.)
>
> Values match the 6000mAh specified by DT:
>
> bat: battery {
> compatible = "simple-battery", "pyra-battery";
> voltage-min-design-microvolt = <320>;
> energy-full-design-microwatt-hours = <2220>;
> charge-full-design-microamp-hours = <600>;
> };
>
> root@letux:~# cat /sys/class/power_supply/bq27421-0/uevent
> POWER_SUPPLY_NAME=bq27421-0
> POWER_SUPPLY_STATUS=Discharging
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_VOLTAGE_NOW=3943000
> POWER_SUPPLY_CURRENT_NOW=-634000
> POWER_SUPPLY_CAPACITY=82
> POWER_SUPPLY_CAPACITY_LEVEL=Normal
> POWER_SUPPLY_TEMP=319
> POWER_SUPPLY_TECHNOLOGY=Li-ion
> POWER_SUPPLY_CHARGE_FULL=5467000
> POWER_SUPPLY_CHARGE_NOW=4611000
> POWER_SUPPLY_CHARGE_FULL_DESIGN=600
> POWER_SUPPLY_MANUFACTURER=Texas Instruments
> root@letux:~#
>
> Anything else I should test?

Could you also test voltage-min-design-microvolt at 3177 to make sure
that's configurable too?

Sebastian just queued my series to -next. Would you be inclined to
submit a new patch enabling bq27421? If so, the sooner the better
since merge window is imminent :-)

Liam


>
>
>> Am 29.08.2017 um 12:40 schrieb Liam Breck :
>>
>> Hi Nikolaus,
>>
>> You need the patchset which enables this feature, which was delayed
>> for further work. It's now ready...
>>
>> https://patchwork.kernel.org/patch/9918947/
>> https://patchwork.kernel.org/patch/9918949/
>> https://patchwork.kernel.org/patch/9918951/
>> https://patchwork.kernel.org/patch/9918953/
>> https://patchwork.kernel.org/patch/9918955/
>>
>> And flip the #if 0 protecting bq27421_dm_regs
>>
>> I'd love to support that chip in this patchset if you can test
>> non-default settings for all 3 options this week?
>>
>> Thanks,
>> Liam
>>
>>
>> On Tue, Aug 29, 2017 at 2:43 AM, H. Nikolaus Schaller  
>> wrote:
>>> Hi,
>>> I am trying to get this working on our bq27421.
>>>
>>> But the only message I get is:
>>>
>>> [6.086407] bq27xxx-battery 1-0055: data memory update not supported for 
>>> chip
>>>
>>> A little research shows that this message comes from
>>>
>>>
>>> http://elixir.free-electrons.com/linux/v4.13-rc7/source/drivers/power/supply/bq27xxx_battery.c#L1279
>>>
>>> So di->dm_regs is NULL.
>>>
>>> But doing an fgrep for dm_regs shows no line of code where the pointer is 
>>> set to a non-null value:
>>>
>>> master hns$ fgrep -R dm_regs *
>>> drivers/power/supply/bq27xxx_battery.c: .class = 
>>> (di)->dm_regs[i].subclass_id, \
>>> drivers/power/supply/bq27xxx_battery.c: .block = (di)->dm_regs[i].offset / 
>>> BQ27XXX_DM_SZ, \
>>> drivers/power/supply/bq27xxx_battery.c: struct bq27xxx_dm_reg *reg = 
>>> >dm_regs[reg_id];
>>> d

Re: dt-bindings: power: supply: bq27xxx: Add monitored-battery documentation

[Liam Breck]

Hi Nikolaus,

You need the patchset which enables this feature, which was delayed
for further work. It's now ready...

https://patchwork.kernel.org/patch/9918947/
https://patchwork.kernel.org/patch/9918949/
https://patchwork.kernel.org/patch/9918951/
https://patchwork.kernel.org/patch/9918953/
https://patchwork.kernel.org/patch/9918955/

And flip the #if 0 protecting bq27421_dm_regs

I'd love to support that chip in this patchset if you can test
non-default settings for all 3 options this week?

Thanks,
Liam


On Tue, Aug 29, 2017 at 2:43 AM, H. Nikolaus Schaller  
wrote:
> Hi,
> I am trying to get this working on our bq27421.
>
> But the only message I get is:
>
> [6.086407] bq27xxx-battery 1-0055: data memory update not supported for 
> chip
>
> A little research shows that this message comes from
>
> 
> http://elixir.free-electrons.com/linux/v4.13-rc7/source/drivers/power/supply/bq27xxx_battery.c#L1279
>
> So di->dm_regs is NULL.
>
> But doing an fgrep for dm_regs shows no line of code where the pointer is set 
> to a non-null value:
>
> master hns$ fgrep -R dm_regs *
> drivers/power/supply/bq27xxx_battery.c: .class = 
> (di)->dm_regs[i].subclass_id, \
> drivers/power/supply/bq27xxx_battery.c: .block = (di)->dm_regs[i].offset / 
> BQ27XXX_DM_SZ, \
> drivers/power/supply/bq27xxx_battery.c: struct bq27xxx_dm_reg *reg = 
> >dm_regs[reg_id];
> drivers/power/supply/bq27xxx_battery.c: if (!di->dm_regs) {
> drivers/power/supply/bq27xxx_battery.c: max = 
> di->dm_regs[BQ27XXX_DM_DESIGN_ENERGY].max;
> drivers/power/supply/bq27xxx_battery.c: max = 
> di->dm_regs[BQ27XXX_DM_DESIGN_CAPACITY].max;
> drivers/power/supply/bq27xxx_battery.c: min = 
> di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].min;
> drivers/power/supply/bq27xxx_battery.c: max = 
> di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].max;
> Binary file drivers/power/supply/bq27xxx_battery.ko matches
> Binary file drivers/power/supply/bq27xxx_battery.o matches
> Binary file drivers/power/supply/bq27xxx_battery_i2c.ko matches
> Binary file drivers/power/supply/bq27xxx_battery_i2c.o matches
> include/linux/power/bq27xxx_battery.h:  struct bq27xxx_dm_reg *dm_regs;
> master hns$
>
> What am I doing wrong here?
>
> BR and thanks,
> Nikolaus
>

Re: dt-bindings: power: supply: bq27xxx: Add monitored-battery documentation

[Liam Breck]

Hi Nikolaus,

You need the patchset which enables this feature, which was delayed
for further work. It's now ready...

https://patchwork.kernel.org/patch/9918947/
https://patchwork.kernel.org/patch/9918949/
https://patchwork.kernel.org/patch/9918951/
https://patchwork.kernel.org/patch/9918953/
https://patchwork.kernel.org/patch/9918955/

And flip the #if 0 protecting bq27421_dm_regs

I'd love to support that chip in this patchset if you can test
non-default settings for all 3 options this week?

Thanks,
Liam


On Tue, Aug 29, 2017 at 2:43 AM, H. Nikolaus Schaller  
wrote:
> Hi,
> I am trying to get this working on our bq27421.
>
> But the only message I get is:
>
> [6.086407] bq27xxx-battery 1-0055: data memory update not supported for 
> chip
>
> A little research shows that this message comes from
>
> 
> http://elixir.free-electrons.com/linux/v4.13-rc7/source/drivers/power/supply/bq27xxx_battery.c#L1279
>
> So di->dm_regs is NULL.
>
> But doing an fgrep for dm_regs shows no line of code where the pointer is set 
> to a non-null value:
>
> master hns$ fgrep -R dm_regs *
> drivers/power/supply/bq27xxx_battery.c: .class = 
> (di)->dm_regs[i].subclass_id, \
> drivers/power/supply/bq27xxx_battery.c: .block = (di)->dm_regs[i].offset / 
> BQ27XXX_DM_SZ, \
> drivers/power/supply/bq27xxx_battery.c: struct bq27xxx_dm_reg *reg = 
> >dm_regs[reg_id];
> drivers/power/supply/bq27xxx_battery.c: if (!di->dm_regs) {
> drivers/power/supply/bq27xxx_battery.c: max = 
> di->dm_regs[BQ27XXX_DM_DESIGN_ENERGY].max;
> drivers/power/supply/bq27xxx_battery.c: max = 
> di->dm_regs[BQ27XXX_DM_DESIGN_CAPACITY].max;
> drivers/power/supply/bq27xxx_battery.c: min = 
> di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].min;
> drivers/power/supply/bq27xxx_battery.c: max = 
> di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].max;
> Binary file drivers/power/supply/bq27xxx_battery.ko matches
> Binary file drivers/power/supply/bq27xxx_battery.o matches
> Binary file drivers/power/supply/bq27xxx_battery_i2c.ko matches
> Binary file drivers/power/supply/bq27xxx_battery_i2c.o matches
> include/linux/power/bq27xxx_battery.h:  struct bq27xxx_dm_reg *dm_regs;
> master hns$
>
> What am I doing wrong here?
>
> BR and thanks,
> Nikolaus
>

Re: [PATCH v2 11/14] power: supply: bq24190_charger: Get input_current_limit from our supplier

[Liam Breck]

Hi Hans, I sent too soon...

On Mon, Aug 28, 2017 at 9:04 AM, Hans de Goede <hdego...@redhat.com> wrote:
> Hi,
>
>
> On 16-08-17 22:28, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Tue, Aug 15, 2017 at 1:04 PM, Hans de Goede <hdego...@redhat.com>
>> wrote:
>>>
>>> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
>>> done by a separate port-controller IC, while the current limit is
>>> controlled through another (charger) IC.
>>>
>>> It has been decided to model this by modelling the external Type-C
>>> power brick (adapter/charger) as a power-supply class device which
>>> supplies the charger-IC, with its voltage-now and current-max
>>> representing
>>> the negotiated voltage and max current draw.
>>>
>>> This commit adds support for this to the bq24190_charger driver by
>>> calling
>>> power_supply_set_input_current_limit_from_supplier helper if the
>>> "input-current-limit-from-supplier" device-property is set.
>>>
>>> Note this replaces the functionality to get the current-limit from an
>>> extcon device, which will be removed in a follow-up commit.
>>>
>>> Signed-off-by: Hans de Goede <hdego...@redhat.com>
>>> ---
>>> Changes in v2:
>>> -Wait a bit before applying current-max from our supplier as
>>> input-current-limit
>>>   the bq24190 may still be in its power-good wait-state while our
>>> supplier is
>>>   done negotating current-max and if we apply the limit to early then the
>>>   input-current-limit will be reset to 0.5A by the bq24190 after its
>>>   power-good wait is done.
>>> ---
>>>   drivers/power/supply/bq24190_charger.c | 35
>>> ++
>>>   1 file changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index f13f892..6f75c8e 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -159,9 +159,11 @@ struct bq24190_dev_info {
>>>  struct extcon_dev   *extcon;
>>>  struct notifier_block   extcon_nb;
>>>  struct delayed_work extcon_work;
>>> +   struct delayed_work input_current_limit_work;
>>>  charmodel_name[I2C_NAME_SIZE];
>>>  boolinitialized;
>>>  boolirq_event;
>>> +   bool
>>> input_current_limit_from_supplier;
>>>  struct mutexf_reg_lock;
>>>  u8  f_reg;
>>>  u8  ss_reg;
>>> @@ -1142,6 +1144,32 @@ static int
>>> bq24190_charger_property_is_writeable(struct power_supply *psy,
>>>  return ret;
>>>   }
>>>
>>> +static void bq24190_input_current_limit_work(struct work_struct *work)
>>> +{
>>> +   struct bq24190_dev_info *bdi =
>>> +   container_of(work, struct bq24190_dev_info,
>>> +input_current_limit_work.work);
>>> +
>>> +   power_supply_set_input_current_limit_from_supplier(bdi->charger);
>>> +}
>>> +
>>> +static void bq24190_charger_external_power_changed(struct power_supply
>>> *psy)
>>> +{
>>> +   struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>>> +
>>> +   /*
>>> +* The Power-Good detection may take up to 220ms, sometimes
>>> +* the external charger detection is quicker, and the bq24190
>>> will
>>> +* reset to iinlim based on its own charger detection (which is
>>> not
>>> +* hooked up when using external charger detection) resulting in
>>> a
>>> +* too low default 500mA iinlim. Delay setting the
>>> input-current-limit
>>> +* for 300ms to avoid this.
>>> +*/
>>> +   if (bdi->input_current_limit_from_supplier)
>>> +   queue_delayed_work(system_wq,
>>> >input_current_limit_work,
>>> +  msecs_to_jiffies(300));
>>> +}
>>> +
>>>   static enum power_supply_property bq24190_charger_properties[] = {
>>>  POWER_SUPPLY_PROP_CHARGE_TYPE,
>>> 

Re: [PATCH v2 11/14] power: supply: bq24190_charger: Get input_current_limit from our supplier

[Liam Breck]

Hi Hans, I sent too soon...

On Mon, Aug 28, 2017 at 9:04 AM, Hans de Goede  wrote:
> Hi,
>
>
> On 16-08-17 22:28, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Tue, Aug 15, 2017 at 1:04 PM, Hans de Goede 
>> wrote:
>>>
>>> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
>>> done by a separate port-controller IC, while the current limit is
>>> controlled through another (charger) IC.
>>>
>>> It has been decided to model this by modelling the external Type-C
>>> power brick (adapter/charger) as a power-supply class device which
>>> supplies the charger-IC, with its voltage-now and current-max
>>> representing
>>> the negotiated voltage and max current draw.
>>>
>>> This commit adds support for this to the bq24190_charger driver by
>>> calling
>>> power_supply_set_input_current_limit_from_supplier helper if the
>>> "input-current-limit-from-supplier" device-property is set.
>>>
>>> Note this replaces the functionality to get the current-limit from an
>>> extcon device, which will be removed in a follow-up commit.
>>>
>>> Signed-off-by: Hans de Goede 
>>> ---
>>> Changes in v2:
>>> -Wait a bit before applying current-max from our supplier as
>>> input-current-limit
>>>   the bq24190 may still be in its power-good wait-state while our
>>> supplier is
>>>   done negotating current-max and if we apply the limit to early then the
>>>   input-current-limit will be reset to 0.5A by the bq24190 after its
>>>   power-good wait is done.
>>> ---
>>>   drivers/power/supply/bq24190_charger.c | 35
>>> ++
>>>   1 file changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index f13f892..6f75c8e 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -159,9 +159,11 @@ struct bq24190_dev_info {
>>>  struct extcon_dev   *extcon;
>>>  struct notifier_block   extcon_nb;
>>>  struct delayed_work extcon_work;
>>> +   struct delayed_work input_current_limit_work;
>>>  charmodel_name[I2C_NAME_SIZE];
>>>  boolinitialized;
>>>  boolirq_event;
>>> +   bool
>>> input_current_limit_from_supplier;
>>>  struct mutexf_reg_lock;
>>>  u8  f_reg;
>>>  u8  ss_reg;
>>> @@ -1142,6 +1144,32 @@ static int
>>> bq24190_charger_property_is_writeable(struct power_supply *psy,
>>>  return ret;
>>>   }
>>>
>>> +static void bq24190_input_current_limit_work(struct work_struct *work)
>>> +{
>>> +   struct bq24190_dev_info *bdi =
>>> +   container_of(work, struct bq24190_dev_info,
>>> +input_current_limit_work.work);
>>> +
>>> +   power_supply_set_input_current_limit_from_supplier(bdi->charger);
>>> +}
>>> +
>>> +static void bq24190_charger_external_power_changed(struct power_supply
>>> *psy)
>>> +{
>>> +   struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>>> +
>>> +   /*
>>> +* The Power-Good detection may take up to 220ms, sometimes
>>> +* the external charger detection is quicker, and the bq24190
>>> will
>>> +* reset to iinlim based on its own charger detection (which is
>>> not
>>> +* hooked up when using external charger detection) resulting in
>>> a
>>> +* too low default 500mA iinlim. Delay setting the
>>> input-current-limit
>>> +* for 300ms to avoid this.
>>> +*/
>>> +   if (bdi->input_current_limit_from_supplier)
>>> +   queue_delayed_work(system_wq,
>>> >input_current_limit_work,
>>> +  msecs_to_jiffies(300));
>>> +}
>>> +
>>>   static enum power_supply_property bq24190_charger_properties[] = {
>>>  POWER_SUPPLY_PROP_CHARGE_TYPE,
>>>  POWER_SUPPLY_PROP_HEALTH,
>>> @@ -1170,6 +1198,7 @@ static const s

Re: [PATCH v2 11/14] power: supply: bq24190_charger: Get input_current_limit from our supplier

[Liam Breck]

Hi Hans,

On Mon, Aug 28, 2017 at 9:04 AM, Hans de Goede <hdego...@redhat.com> wrote:
> Hi,
>
>
> On 16-08-17 22:28, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Tue, Aug 15, 2017 at 1:04 PM, Hans de Goede <hdego...@redhat.com>
>> wrote:
>>>
>>> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
>>> done by a separate port-controller IC, while the current limit is
>>> controlled through another (charger) IC.
>>>
>>> It has been decided to model this by modelling the external Type-C
>>> power brick (adapter/charger) as a power-supply class device which
>>> supplies the charger-IC, with its voltage-now and current-max
>>> representing
>>> the negotiated voltage and max current draw.
>>>
>>> This commit adds support for this to the bq24190_charger driver by
>>> calling
>>> power_supply_set_input_current_limit_from_supplier helper if the
>>> "input-current-limit-from-supplier" device-property is set.
>>>
>>> Note this replaces the functionality to get the current-limit from an
>>> extcon device, which will be removed in a follow-up commit.
>>>
>>> Signed-off-by: Hans de Goede <hdego...@redhat.com>
>>> ---
>>> Changes in v2:
>>> -Wait a bit before applying current-max from our supplier as
>>> input-current-limit
>>>   the bq24190 may still be in its power-good wait-state while our
>>> supplier is
>>>   done negotating current-max and if we apply the limit to early then the
>>>   input-current-limit will be reset to 0.5A by the bq24190 after its
>>>   power-good wait is done.
>>> ---
>>>   drivers/power/supply/bq24190_charger.c | 35
>>> ++
>>>   1 file changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index f13f892..6f75c8e 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -159,9 +159,11 @@ struct bq24190_dev_info {
>>>  struct extcon_dev   *extcon;
>>>  struct notifier_block   extcon_nb;
>>>  struct delayed_work extcon_work;
>>> +   struct delayed_work input_current_limit_work;
>>>  charmodel_name[I2C_NAME_SIZE];
>>>  boolinitialized;
>>>  boolirq_event;
>>> +   bool
>>> input_current_limit_from_supplier;
>>>  struct mutexf_reg_lock;
>>>  u8  f_reg;
>>>  u8  ss_reg;
>>> @@ -1142,6 +1144,32 @@ static int
>>> bq24190_charger_property_is_writeable(struct power_supply *psy,
>>>  return ret;
>>>   }
>>>
>>> +static void bq24190_input_current_limit_work(struct work_struct *work)
>>> +{
>>> +   struct bq24190_dev_info *bdi =
>>> +   container_of(work, struct bq24190_dev_info,
>>> +input_current_limit_work.work);
>>> +
>>> +   power_supply_set_input_current_limit_from_supplier(bdi->charger);
>>> +}
>>> +
>>> +static void bq24190_charger_external_power_changed(struct power_supply
>>> *psy)
>>> +{
>>> +   struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>>> +
>>> +   /*
>>> +* The Power-Good detection may take up to 220ms, sometimes
>>> +* the external charger detection is quicker, and the bq24190
>>> will
>>> +* reset to iinlim based on its own charger detection (which is
>>> not
>>> +* hooked up when using external charger detection) resulting in
>>> a
>>> +* too low default 500mA iinlim. Delay setting the
>>> input-current-limit
>>> +* for 300ms to avoid this.
>>> +*/
>>> +   if (bdi->input_current_limit_from_supplier)
>>> +   queue_delayed_work(system_wq,
>>> >input_current_limit_work,
>>> +  msecs_to_jiffies(300));
>>> +}
>>> +
>>>   static enum power_supply_property bq24190_charger_properties[] = {
>>>  POWER_SUPPLY_PROP_CHARGE_TYPE,
>>>  POWER_SUPPLY_

Re: [PATCH v2 11/14] power: supply: bq24190_charger: Get input_current_limit from our supplier

[Liam Breck]

Hi Hans,

On Mon, Aug 28, 2017 at 9:04 AM, Hans de Goede  wrote:
> Hi,
>
>
> On 16-08-17 22:28, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Tue, Aug 15, 2017 at 1:04 PM, Hans de Goede 
>> wrote:
>>>
>>> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
>>> done by a separate port-controller IC, while the current limit is
>>> controlled through another (charger) IC.
>>>
>>> It has been decided to model this by modelling the external Type-C
>>> power brick (adapter/charger) as a power-supply class device which
>>> supplies the charger-IC, with its voltage-now and current-max
>>> representing
>>> the negotiated voltage and max current draw.
>>>
>>> This commit adds support for this to the bq24190_charger driver by
>>> calling
>>> power_supply_set_input_current_limit_from_supplier helper if the
>>> "input-current-limit-from-supplier" device-property is set.
>>>
>>> Note this replaces the functionality to get the current-limit from an
>>> extcon device, which will be removed in a follow-up commit.
>>>
>>> Signed-off-by: Hans de Goede 
>>> ---
>>> Changes in v2:
>>> -Wait a bit before applying current-max from our supplier as
>>> input-current-limit
>>>   the bq24190 may still be in its power-good wait-state while our
>>> supplier is
>>>   done negotating current-max and if we apply the limit to early then the
>>>   input-current-limit will be reset to 0.5A by the bq24190 after its
>>>   power-good wait is done.
>>> ---
>>>   drivers/power/supply/bq24190_charger.c | 35
>>> ++
>>>   1 file changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index f13f892..6f75c8e 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -159,9 +159,11 @@ struct bq24190_dev_info {
>>>  struct extcon_dev   *extcon;
>>>  struct notifier_block   extcon_nb;
>>>  struct delayed_work extcon_work;
>>> +   struct delayed_work input_current_limit_work;
>>>  charmodel_name[I2C_NAME_SIZE];
>>>  boolinitialized;
>>>  boolirq_event;
>>> +   bool
>>> input_current_limit_from_supplier;
>>>  struct mutexf_reg_lock;
>>>  u8  f_reg;
>>>  u8  ss_reg;
>>> @@ -1142,6 +1144,32 @@ static int
>>> bq24190_charger_property_is_writeable(struct power_supply *psy,
>>>  return ret;
>>>   }
>>>
>>> +static void bq24190_input_current_limit_work(struct work_struct *work)
>>> +{
>>> +   struct bq24190_dev_info *bdi =
>>> +   container_of(work, struct bq24190_dev_info,
>>> +input_current_limit_work.work);
>>> +
>>> +   power_supply_set_input_current_limit_from_supplier(bdi->charger);
>>> +}
>>> +
>>> +static void bq24190_charger_external_power_changed(struct power_supply
>>> *psy)
>>> +{
>>> +   struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>>> +
>>> +   /*
>>> +* The Power-Good detection may take up to 220ms, sometimes
>>> +* the external charger detection is quicker, and the bq24190
>>> will
>>> +* reset to iinlim based on its own charger detection (which is
>>> not
>>> +* hooked up when using external charger detection) resulting in
>>> a
>>> +* too low default 500mA iinlim. Delay setting the
>>> input-current-limit
>>> +* for 300ms to avoid this.
>>> +*/
>>> +   if (bdi->input_current_limit_from_supplier)
>>> +   queue_delayed_work(system_wq,
>>> >input_current_limit_work,
>>> +  msecs_to_jiffies(300));
>>> +}
>>> +
>>>   static enum power_supply_property bq24190_charger_properties[] = {
>>>  POWER_SUPPLY_PROP_CHARGE_TYPE,
>>>  POWER_SUPPLY_PROP_HEALTH,
>>> @@ -1170,6 +1198,7 @@ static const struct power_supply_desc
>&

Re: [PATCH v2 11/14] power: supply: bq24190_charger: Get input_current_limit from our supplier

[Liam Breck]

Hi Hans,

On Tue, Aug 15, 2017 at 1:04 PM, Hans de Goede  wrote:
> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
> done by a separate port-controller IC, while the current limit is
> controlled through another (charger) IC.
>
> It has been decided to model this by modelling the external Type-C
> power brick (adapter/charger) as a power-supply class device which
> supplies the charger-IC, with its voltage-now and current-max representing
> the negotiated voltage and max current draw.
>
> This commit adds support for this to the bq24190_charger driver by calling
> power_supply_set_input_current_limit_from_supplier helper if the
> "input-current-limit-from-supplier" device-property is set.
>
> Note this replaces the functionality to get the current-limit from an
> extcon device, which will be removed in a follow-up commit.
>
> Signed-off-by: Hans de Goede 
> ---
> Changes in v2:
> -Wait a bit before applying current-max from our supplier as 
> input-current-limit
>  the bq24190 may still be in its power-good wait-state while our supplier is
>  done negotating current-max and if we apply the limit to early then the
>  input-current-limit will be reset to 0.5A by the bq24190 after its
>  power-good wait is done.
> ---
>  drivers/power/supply/bq24190_charger.c | 35 
> ++
>  1 file changed, 35 insertions(+)
>
> diff --git a/drivers/power/supply/bq24190_charger.c 
> b/drivers/power/supply/bq24190_charger.c
> index f13f892..6f75c8e 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -159,9 +159,11 @@ struct bq24190_dev_info {
> struct extcon_dev   *extcon;
> struct notifier_block   extcon_nb;
> struct delayed_work extcon_work;
> +   struct delayed_work input_current_limit_work;
> charmodel_name[I2C_NAME_SIZE];
> boolinitialized;
> boolirq_event;
> +   boolinput_current_limit_from_supplier;
> struct mutexf_reg_lock;
> u8  f_reg;
> u8  ss_reg;
> @@ -1142,6 +1144,32 @@ static int 
> bq24190_charger_property_is_writeable(struct power_supply *psy,
> return ret;
>  }
>
> +static void bq24190_input_current_limit_work(struct work_struct *work)
> +{
> +   struct bq24190_dev_info *bdi =
> +   container_of(work, struct bq24190_dev_info,
> +input_current_limit_work.work);
> +
> +   power_supply_set_input_current_limit_from_supplier(bdi->charger);
> +}
> +
> +static void bq24190_charger_external_power_changed(struct power_supply *psy)
> +{
> +   struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
> +
> +   /*
> +* The Power-Good detection may take up to 220ms, sometimes
> +* the external charger detection is quicker, and the bq24190 will
> +* reset to iinlim based on its own charger detection (which is not
> +* hooked up when using external charger detection) resulting in a
> +* too low default 500mA iinlim. Delay setting the input-current-limit
> +* for 300ms to avoid this.
> +*/
> +   if (bdi->input_current_limit_from_supplier)
> +   queue_delayed_work(system_wq, >input_current_limit_work,
> +  msecs_to_jiffies(300));
> +}
> +
>  static enum power_supply_property bq24190_charger_properties[] = {
> POWER_SUPPLY_PROP_CHARGE_TYPE,
> POWER_SUPPLY_PROP_HEALTH,
> @@ -1170,6 +1198,7 @@ static const struct power_supply_desc 
> bq24190_charger_desc = {
> .get_property   = bq24190_charger_get_property,
> .set_property   = bq24190_charger_set_property,
> .property_is_writeable  = bq24190_charger_property_is_writeable,
> +   .external_power_changed = bq24190_charger_external_power_changed,
>  };
>
>  /* Battery power supply property routines */
> @@ -1651,6 +1680,8 @@ static int bq24190_probe(struct i2c_client *client,
> mutex_init(>f_reg_lock);
> bdi->f_reg = 0;
> bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
> +   INIT_DELAYED_WORK(>input_current_limit_work,
> + bq24190_input_current_limit_work);
>
> i2c_set_clientdata(client, bdi);
>
> @@ -1659,6 +1690,10 @@ static int bq24190_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> +   bdi->input_current_limit_from_supplier =
> +   device_property_read_bool(dev,
> + 
> "input-current-limit-from-supplier");
> +

Maybe
  if (device_property_read_bool(dev,
"linux,input-current-limit-from-supplier")) {
   

Re: [PATCH v2 11/14] power: supply: bq24190_charger: Get input_current_limit from our supplier

[Liam Breck]

Hi Hans,

On Tue, Aug 15, 2017 at 1:04 PM, Hans de Goede  wrote:
> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
> done by a separate port-controller IC, while the current limit is
> controlled through another (charger) IC.
>
> It has been decided to model this by modelling the external Type-C
> power brick (adapter/charger) as a power-supply class device which
> supplies the charger-IC, with its voltage-now and current-max representing
> the negotiated voltage and max current draw.
>
> This commit adds support for this to the bq24190_charger driver by calling
> power_supply_set_input_current_limit_from_supplier helper if the
> "input-current-limit-from-supplier" device-property is set.
>
> Note this replaces the functionality to get the current-limit from an
> extcon device, which will be removed in a follow-up commit.
>
> Signed-off-by: Hans de Goede 
> ---
> Changes in v2:
> -Wait a bit before applying current-max from our supplier as 
> input-current-limit
>  the bq24190 may still be in its power-good wait-state while our supplier is
>  done negotating current-max and if we apply the limit to early then the
>  input-current-limit will be reset to 0.5A by the bq24190 after its
>  power-good wait is done.
> ---
>  drivers/power/supply/bq24190_charger.c | 35 
> ++
>  1 file changed, 35 insertions(+)
>
> diff --git a/drivers/power/supply/bq24190_charger.c 
> b/drivers/power/supply/bq24190_charger.c
> index f13f892..6f75c8e 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -159,9 +159,11 @@ struct bq24190_dev_info {
> struct extcon_dev   *extcon;
> struct notifier_block   extcon_nb;
> struct delayed_work extcon_work;
> +   struct delayed_work input_current_limit_work;
> charmodel_name[I2C_NAME_SIZE];
> boolinitialized;
> boolirq_event;
> +   boolinput_current_limit_from_supplier;
> struct mutexf_reg_lock;
> u8  f_reg;
> u8  ss_reg;
> @@ -1142,6 +1144,32 @@ static int 
> bq24190_charger_property_is_writeable(struct power_supply *psy,
> return ret;
>  }
>
> +static void bq24190_input_current_limit_work(struct work_struct *work)
> +{
> +   struct bq24190_dev_info *bdi =
> +   container_of(work, struct bq24190_dev_info,
> +input_current_limit_work.work);
> +
> +   power_supply_set_input_current_limit_from_supplier(bdi->charger);
> +}
> +
> +static void bq24190_charger_external_power_changed(struct power_supply *psy)
> +{
> +   struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
> +
> +   /*
> +* The Power-Good detection may take up to 220ms, sometimes
> +* the external charger detection is quicker, and the bq24190 will
> +* reset to iinlim based on its own charger detection (which is not
> +* hooked up when using external charger detection) resulting in a
> +* too low default 500mA iinlim. Delay setting the input-current-limit
> +* for 300ms to avoid this.
> +*/
> +   if (bdi->input_current_limit_from_supplier)
> +   queue_delayed_work(system_wq, >input_current_limit_work,
> +  msecs_to_jiffies(300));
> +}
> +
>  static enum power_supply_property bq24190_charger_properties[] = {
> POWER_SUPPLY_PROP_CHARGE_TYPE,
> POWER_SUPPLY_PROP_HEALTH,
> @@ -1170,6 +1198,7 @@ static const struct power_supply_desc 
> bq24190_charger_desc = {
> .get_property   = bq24190_charger_get_property,
> .set_property   = bq24190_charger_set_property,
> .property_is_writeable  = bq24190_charger_property_is_writeable,
> +   .external_power_changed = bq24190_charger_external_power_changed,
>  };
>
>  /* Battery power supply property routines */
> @@ -1651,6 +1680,8 @@ static int bq24190_probe(struct i2c_client *client,
> mutex_init(>f_reg_lock);
> bdi->f_reg = 0;
> bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
> +   INIT_DELAYED_WORK(>input_current_limit_work,
> + bq24190_input_current_limit_work);
>
> i2c_set_clientdata(client, bdi);
>
> @@ -1659,6 +1690,10 @@ static int bq24190_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> +   bdi->input_current_limit_from_supplier =
> +   device_property_read_bool(dev,
> + 
> "input-current-limit-from-supplier");
> +

Maybe
  if (device_property_read_bool(dev,
"linux,input-current-limit-from-supplier")) {
   INIT_DELAYED_WORK(>input_current_limit_work,
 

Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

[Liam Breck]

On Tue, Aug 8, 2017 at 2:00 AM, Hans de Goede <hdego...@redhat.com> wrote:
> Hi,
>
> On 08-08-17 10:39, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede <hdego...@redhat.com> wrote:
>>>
>>> Register the 5V boost converter as a regulator named
>>> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
>>> the bq24190 family is also used on ACPI devices where there are no
>>> device-tree phandles, so regulator_get will fallback to the name and thus
>>> it must be unique on the system.
>>
>>
>> What we're enabling here is 5V boost for otg host mode, not vbus
>> generally, so maybe the name should indicate that...
>>
>> regulator-bq24190-usb-5volt
>> regulator-bq24190-usb-host
>> regulator-bq24190-usb-otg-5v
>
>
> I picked vbus because that gets used a lot already in similar cases,
> but I agree that we should probably come up with a better name.
>
> I like "regulator-bq24190-usb-otg-5v", shall I use that for v2?

There is this upstream, with "otg-vbus":
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=61274eff0ddee8f10deaa5f79085e981db52930a

Related search:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/log/?qt=grep=otg+regulator

I don't think it needs a "regulator-" prefix, and maybe the driver
name is a suffix. We could also recommend the "regulator-name" value
for OTG here:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/Documentation/devicetree/bindings/regulator/regulator.txt

a. Make the above patch wrong :-)
usb-otg-5v (generic)
usb-otg-5v-bq2419x (specific)

b. Follow a weak precedent
otg-vbus (generic)
otg-vbus-bq2419x (specific)

Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

[Liam Breck]

On Tue, Aug 8, 2017 at 2:00 AM, Hans de Goede  wrote:
> Hi,
>
> On 08-08-17 10:39, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede  wrote:
>>>
>>> Register the 5V boost converter as a regulator named
>>> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
>>> the bq24190 family is also used on ACPI devices where there are no
>>> device-tree phandles, so regulator_get will fallback to the name and thus
>>> it must be unique on the system.
>>
>>
>> What we're enabling here is 5V boost for otg host mode, not vbus
>> generally, so maybe the name should indicate that...
>>
>> regulator-bq24190-usb-5volt
>> regulator-bq24190-usb-host
>> regulator-bq24190-usb-otg-5v
>
>
> I picked vbus because that gets used a lot already in similar cases,
> but I agree that we should probably come up with a better name.
>
> I like "regulator-bq24190-usb-otg-5v", shall I use that for v2?

There is this upstream, with "otg-vbus":
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=61274eff0ddee8f10deaa5f79085e981db52930a

Related search:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/log/?qt=grep=otg+regulator

I don't think it needs a "regulator-" prefix, and maybe the driver
name is a suffix. We could also recommend the "regulator-name" value
for OTG here:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/Documentation/devicetree/bindings/regulator/regulator.txt

a. Make the above patch wrong :-)
usb-otg-5v (generic)
usb-otg-5v-bq2419x (specific)

b. Follow a weak precedent
otg-vbus (generic)
otg-vbus-bq2419x (specific)

Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

[Liam Breck]

Hi Hans,

On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede  wrote:
> Register the 5V boost converter as a regulator named
> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
> the bq24190 family is also used on ACPI devices where there are no
> device-tree phandles, so regulator_get will fallback to the name and thus
> it must be unique on the system.

What we're enabling here is 5V boost for otg host mode, not vbus
generally, so maybe the name should indicate that...

regulator-bq24190-usb-5volt
regulator-bq24190-usb-host
regulator-bq24190-usb-otg-5v

Tony, thoughts?

Re: [PATCH 13/18] power: supply: bq24190_charger: Export 5V boost converter as regulator

[Liam Breck]

Hi Hans,

On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede  wrote:
> Register the 5V boost converter as a regulator named
> "regulator-bq24190-usb-vbus". Note the name includes "bq24190" because
> the bq24190 family is also used on ACPI devices where there are no
> device-tree phandles, so regulator_get will fallback to the name and thus
> it must be unique on the system.

What we're enabling here is 5V boost for otg host mode, not vbus
generally, so maybe the name should indicate that...

regulator-bq24190-usb-5volt
regulator-bq24190-usb-host
regulator-bq24190-usb-otg-5v

Tony, thoughts?

Re: [PATCH 16/18] power: supply: bq24190_charger: Remove extcon handling

[Liam Breck]

Hi Hans,

On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede  wrote:
> Now that drivers/i2c/busses/i2c-cht-wc.c uses
> "input-current-limit-from-supplier" instead of "extcon-name" the last
> user of the bq24190 extcon code is gone, remove it.
>
> Signed-off-by: Hans de Goede 
> ---
>  drivers/power/supply/bq24190_charger.c | 107 
> -
>  1 file changed, 107 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c 
> b/drivers/power/supply/bq24190_charger.c
> index 1f6424f0772f..0376de6d8e70 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -11,7 +11,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -155,9 +154,6 @@ struct bq24190_dev_info {
> struct device   *dev;
> struct power_supply *charger;
> struct power_supply *battery;
> -   struct extcon_dev   *extcon;
> -   struct notifier_block   extcon_nb;
> -   struct delayed_work extcon_work;
> charmodel_name[I2C_NAME_SIZE];
> boolinitialized;
> boolirq_event;
> @@ -1530,75 +1526,6 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, 
> void *data)
> return IRQ_HANDLED;
>  }
>
> -static void bq24190_extcon_work(struct work_struct *work)
> -{
> -   struct bq24190_dev_info *bdi =
> -   container_of(work, struct bq24190_dev_info, extcon_work.work);
> -   int error, iinlim = 0;
> -   u8 v;
> -
> -   error = pm_runtime_get_sync(bdi->dev);
> -   if (error < 0) {
> -   dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> -   pm_runtime_put_noidle(bdi->dev);
> -   return;
> -   }
> -
> -   if  (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
> -   iinlim =  50;
> -   else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
> -extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
> -   iinlim = 150;
> -   else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1)
> -   iinlim = 200;
> -
> -   if (iinlim) {
> -   error = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
> - BQ24190_REG_ISC_IINLIM_MASK,
> - BQ24190_REG_ISC_IINLIM_SHIFT,
> - bq24190_isc_iinlim_values,
> - 
> ARRAY_SIZE(bq24190_isc_iinlim_values),
> - iinlim);
> -   if (error < 0)
> -   dev_err(bdi->dev, "Can't set IINLIM: %d\n", error);
> -   }
> -
> -   /* if no charger found and in USB host mode, set OTG 5V boost, else 
> normal */
> -   if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1)
> -   v = BQ24190_REG_POC_CHG_CONFIG_OTG;
> -   else
> -   v = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
> -
> -   error = bq24190_write_mask(bdi, BQ24190_REG_POC,
> -  BQ24190_REG_POC_CHG_CONFIG_MASK,
> -  BQ24190_REG_POC_CHG_CONFIG_SHIFT,
> -  v);
> -   if (error < 0)
> -   dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
> -
> -   pm_runtime_mark_last_busy(bdi->dev);
> -   pm_runtime_put_autosuspend(bdi->dev);
> -}
> -
> -static int bq24190_extcon_event(struct notifier_block *nb, unsigned long 
> event,
> -   void *param)
> -{
> -   struct bq24190_dev_info *bdi =
> -   container_of(nb, struct bq24190_dev_info, extcon_nb);
> -
> -   /*
> -* The Power-Good detection may take up to 220ms, sometimes
> -* the external charger detection is quicker, and the bq24190 will
> -* reset to iinlim based on its own charger detection (which is not
> -* hooked up when using external charger detection) resulting in
> -* a too low default 500mA iinlim. Delay applying the extcon value
> -* for 300ms to avoid this.
> -*/
> -   queue_delayed_work(system_wq, >extcon_work, 
> msecs_to_jiffies(300));
> -
> -   return NOTIFY_OK;
> -}
> -
>  static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>  {
> u8 v;
> @@ -1636,7 +1563,6 @@ static int bq24190_probe(struct i2c_client *client,
> struct device *dev = >dev;
> struct power_supply_config charger_cfg = {}, battery_cfg = {};
> struct bq24190_dev_info *bdi;
> -   const char *name;
> int ret;
>
> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> @@ -1668,25 +1594,6 @@ static int 

Re: [PATCH 16/18] power: supply: bq24190_charger: Remove extcon handling

[Liam Breck]

Hi Hans,

On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede  wrote:
> Now that drivers/i2c/busses/i2c-cht-wc.c uses
> "input-current-limit-from-supplier" instead of "extcon-name" the last
> user of the bq24190 extcon code is gone, remove it.
>
> Signed-off-by: Hans de Goede 
> ---
>  drivers/power/supply/bq24190_charger.c | 107 
> -
>  1 file changed, 107 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c 
> b/drivers/power/supply/bq24190_charger.c
> index 1f6424f0772f..0376de6d8e70 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -11,7 +11,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -155,9 +154,6 @@ struct bq24190_dev_info {
> struct device   *dev;
> struct power_supply *charger;
> struct power_supply *battery;
> -   struct extcon_dev   *extcon;
> -   struct notifier_block   extcon_nb;
> -   struct delayed_work extcon_work;
> charmodel_name[I2C_NAME_SIZE];
> boolinitialized;
> boolirq_event;
> @@ -1530,75 +1526,6 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, 
> void *data)
> return IRQ_HANDLED;
>  }
>
> -static void bq24190_extcon_work(struct work_struct *work)
> -{
> -   struct bq24190_dev_info *bdi =
> -   container_of(work, struct bq24190_dev_info, extcon_work.work);
> -   int error, iinlim = 0;
> -   u8 v;
> -
> -   error = pm_runtime_get_sync(bdi->dev);
> -   if (error < 0) {
> -   dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> -   pm_runtime_put_noidle(bdi->dev);
> -   return;
> -   }
> -
> -   if  (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
> -   iinlim =  50;
> -   else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
> -extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
> -   iinlim = 150;
> -   else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1)
> -   iinlim = 200;
> -
> -   if (iinlim) {
> -   error = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
> - BQ24190_REG_ISC_IINLIM_MASK,
> - BQ24190_REG_ISC_IINLIM_SHIFT,
> - bq24190_isc_iinlim_values,
> - 
> ARRAY_SIZE(bq24190_isc_iinlim_values),
> - iinlim);
> -   if (error < 0)
> -   dev_err(bdi->dev, "Can't set IINLIM: %d\n", error);
> -   }
> -
> -   /* if no charger found and in USB host mode, set OTG 5V boost, else 
> normal */
> -   if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1)
> -   v = BQ24190_REG_POC_CHG_CONFIG_OTG;
> -   else
> -   v = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
> -
> -   error = bq24190_write_mask(bdi, BQ24190_REG_POC,
> -  BQ24190_REG_POC_CHG_CONFIG_MASK,
> -  BQ24190_REG_POC_CHG_CONFIG_SHIFT,
> -  v);
> -   if (error < 0)
> -   dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
> -
> -   pm_runtime_mark_last_busy(bdi->dev);
> -   pm_runtime_put_autosuspend(bdi->dev);
> -}
> -
> -static int bq24190_extcon_event(struct notifier_block *nb, unsigned long 
> event,
> -   void *param)
> -{
> -   struct bq24190_dev_info *bdi =
> -   container_of(nb, struct bq24190_dev_info, extcon_nb);
> -
> -   /*
> -* The Power-Good detection may take up to 220ms, sometimes
> -* the external charger detection is quicker, and the bq24190 will
> -* reset to iinlim based on its own charger detection (which is not
> -* hooked up when using external charger detection) resulting in
> -* a too low default 500mA iinlim. Delay applying the extcon value
> -* for 300ms to avoid this.
> -*/
> -   queue_delayed_work(system_wq, >extcon_work, 
> msecs_to_jiffies(300));
> -
> -   return NOTIFY_OK;
> -}
> -
>  static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>  {
> u8 v;
> @@ -1636,7 +1563,6 @@ static int bq24190_probe(struct i2c_client *client,
> struct device *dev = >dev;
> struct power_supply_config charger_cfg = {}, battery_cfg = {};
> struct bq24190_dev_info *bdi;
> -   const char *name;
> int ret;
>
> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> @@ -1668,25 +1594,6 @@ static int bq24190_probe(struct i2c_client *client,
>  

Re: [PATCH 15/18] power: supply: bq24190_charger: Get input_current_limit from our supplier

[Liam Breck]

Hallo Hans :-)


On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede  wrote:
> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
> done by a separate port-controller IC, while the current limit is
> controlled through another (charger) IC.
>
> It has been decided to model this by modelling the external Type-C
> power brick (adapter/charger) as a power-supply class device which
> supplies the charger-IC, with its voltage-now and current-max representing
> the negotiated voltage and max current draw.
>
> This commit adds support for this to the bq24190_charger driver by calling
> power_supply_set_input_current_limit_from_supplier helper if the
> "input-current-limit-from-supplier" device-property is set.
>
> Note this replaces the functionality to get the current-limit from an
> extcon device, which will be removed in a follow-up commit.
>
> Signed-off-by: Hans de Goede 
> ---
>  drivers/power/supply/bq24190_charger.c | 14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/power/supply/bq24190_charger.c 
> b/drivers/power/supply/bq24190_charger.c
> index d78e2c6dc127..1f6424f0772f 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -161,6 +161,7 @@ struct bq24190_dev_info {
> charmodel_name[I2C_NAME_SIZE];
> boolinitialized;
> boolirq_event;
> +   boolinput_current_limit_from_supplier;
> struct mutexf_reg_lock;
> u8  f_reg;
> u8  ss_reg;
> @@ -1137,6 +1138,14 @@ static int 
> bq24190_charger_property_is_writeable(struct power_supply *psy,
> return ret;
>  }
>
> +static void bq24190_charger_external_power_changed(struct power_supply *psy)
> +{
> +   struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
> +
> +   if (bdi->input_current_limit_from_supplier)
> +   power_supply_set_input_current_limit_from_supplier(psy);
> +}
> +
>  static enum power_supply_property bq24190_charger_properties[] = {
> POWER_SUPPLY_PROP_CHARGE_TYPE,
> POWER_SUPPLY_PROP_HEALTH,
> @@ -1165,6 +1174,7 @@ static const struct power_supply_desc 
> bq24190_charger_desc = {
> .get_property   = bq24190_charger_get_property,
> .set_property   = bq24190_charger_set_property,
> .property_is_writeable  = bq24190_charger_property_is_writeable,
> +   .external_power_changed = bq24190_charger_external_power_changed,
>  };
>
>  /* Battery power supply property routines */
> @@ -1654,6 +1664,10 @@ static int bq24190_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> +   bdi->input_current_limit_from_supplier =
> +   device_property_read_bool(dev,
> + 
> "input-current-limit-from-supplier");

Since this invokes the new power_supply_class function, should this be
a devicetree property, not just a driver-to-driver switch? If so, the
property name should probably be defined in
Doc...bindings/power/supply/power_supply.txt.

My latest bq24190 series has a new DT doc, which should ref a new DT property.

Re: [PATCH 15/18] power: supply: bq24190_charger: Get input_current_limit from our supplier

[Liam Breck]

Hallo Hans :-)


On Sun, Aug 6, 2017 at 5:35 AM, Hans de Goede  wrote:
> On some devices the USB Type-C port power (USB PD 2.0) negotiation is
> done by a separate port-controller IC, while the current limit is
> controlled through another (charger) IC.
>
> It has been decided to model this by modelling the external Type-C
> power brick (adapter/charger) as a power-supply class device which
> supplies the charger-IC, with its voltage-now and current-max representing
> the negotiated voltage and max current draw.
>
> This commit adds support for this to the bq24190_charger driver by calling
> power_supply_set_input_current_limit_from_supplier helper if the
> "input-current-limit-from-supplier" device-property is set.
>
> Note this replaces the functionality to get the current-limit from an
> extcon device, which will be removed in a follow-up commit.
>
> Signed-off-by: Hans de Goede 
> ---
>  drivers/power/supply/bq24190_charger.c | 14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/power/supply/bq24190_charger.c 
> b/drivers/power/supply/bq24190_charger.c
> index d78e2c6dc127..1f6424f0772f 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -161,6 +161,7 @@ struct bq24190_dev_info {
> charmodel_name[I2C_NAME_SIZE];
> boolinitialized;
> boolirq_event;
> +   boolinput_current_limit_from_supplier;
> struct mutexf_reg_lock;
> u8  f_reg;
> u8  ss_reg;
> @@ -1137,6 +1138,14 @@ static int 
> bq24190_charger_property_is_writeable(struct power_supply *psy,
> return ret;
>  }
>
> +static void bq24190_charger_external_power_changed(struct power_supply *psy)
> +{
> +   struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
> +
> +   if (bdi->input_current_limit_from_supplier)
> +   power_supply_set_input_current_limit_from_supplier(psy);
> +}
> +
>  static enum power_supply_property bq24190_charger_properties[] = {
> POWER_SUPPLY_PROP_CHARGE_TYPE,
> POWER_SUPPLY_PROP_HEALTH,
> @@ -1165,6 +1174,7 @@ static const struct power_supply_desc 
> bq24190_charger_desc = {
> .get_property   = bq24190_charger_get_property,
> .set_property   = bq24190_charger_set_property,
> .property_is_writeable  = bq24190_charger_property_is_writeable,
> +   .external_power_changed = bq24190_charger_external_power_changed,
>  };
>
>  /* Battery power supply property routines */
> @@ -1654,6 +1664,10 @@ static int bq24190_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> +   bdi->input_current_limit_from_supplier =
> +   device_property_read_bool(dev,
> + 
> "input-current-limit-from-supplier");

Since this invokes the new power_supply_class function, should this be
a devicetree property, not just a driver-to-driver switch? If so, the
property name should probably be defined in
Doc...bindings/power/supply/power_supply.txt.

My latest bq24190 series has a new DT doc, which should ref a new DT property.

[PATCH v14 01/11] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring <r...@kernel.org>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <ker...@networkimprov.net>
Acked-by: Sebastian Reichel <s...@kernel.org>
Acked-by: Rob Herring <r...@kernel.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d79..0849618a 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.13.0

[PATCH v14 01/11] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay 

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring 
Cc: Mark Rutland 
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay 
Signed-off-by: Liam Breck 
Acked-by: Sebastian Reichel 
Acked-by: Rob Herring 
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d79..0849618a 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.13.0

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Liam Breck]

Hi Enric,



On Thu, Jun 1, 2017 at 1:18 AM, Enric Balletbo Serra
<eballe...@gmail.com> wrote:
> Hi Liam,
>
> 2017-06-01 9:01 GMT+02:00 Liam Breck <l...@networkimprov.net>:
>> Hi Enric,
>>
>> On Sat, May 27, 2017 at 1:11 PM, Enric Balletbo Serra
>> <eballe...@gmail.com> wrote:
>>> Hi Liam,
>>>
>>> 2017-05-26 23:20 GMT+02:00 Liam Breck <l...@networkimprov.net>:
>>>> Hi Enric,
>>>>
>>>> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
>>>> <enric.balle...@collabora.com> wrote:
>>>>> Add charging voltage and current characteristics to the battery DT for
>>>>> proper handling of the battery by fuel-gauge and charger chips.
>>>>>
>>>>> Signed-off-by: Enric Balletbo i Serra <enric.balle...@collabora.com>
>>>>> ---
>>>>> Changes since v2:
>>>>>  - Requested by Sebastian Reichel
>>>>>- Move to its own patch and apply to simple-battery framework.
>>>>> Changes since v1:
>>>>>  - Requested by Rob Herring
>>>>>- Rename ti,charge-* to charge-* to be standard properties.
>>>>>- Use unit suffixes as per bindings/property-units.txt
>>>>>
>>>>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt 
>>>>> b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>> index 63a7028..c87a439 100644
>>>>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>> @@ -12,6 +12,8 @@ Optional Properties:
>>>>>   - voltage-min-design-microvolt: drained battery voltage
>>>>>   - energy-full-design-microwatt-hours: battery design energy
>>>>>   - charge-full-design-microamp-hours: battery design capacity
>>>>> + - charge-voltage-microvolt: battery charging voltage
>>>>> + - charge-current-microamp: battery charging current
>>>>
>>>> I think you mean constant-charge-* which is how you surface these
>>>> properties in your tps65217_charger patch.
>>>>
>>>
>>> Yes, to be strict this is constant-charge-*
>>
>> The DT battery node should carry static battery characteristics. So on
>> reflection, I think you want
>>
>> constant-charge-current-max-microamp
>> constant-charge-voltage-max-microvolt
>>
>> The charger or the user could then safely apply any value <= those.
>>
>> Thoughts?

I'm curious to hear how your hw config requires constant-charge settings?

> Hmm I see your point and I'm thinking now that actually this is not
> what I wanted to do. What I wanted is set the charger voltage and the
> current hence my first patchset was setting these properties in the
> charger node not the battery.

You could certainly support constant-charge-* params in the charger
node. And it's easy to enable userspace to set these via sysfs.

> Said that, I think that you have reason and what we want in battery
> node is the current/voltage max values but we also need to implement a
> mechanism to set the charging voltage/current from userspace or from
> the DT for the charger. The charger should be able to set these values
> and fail if, based in the battery specs, is not supported.

The charger could default constant-charge-* to the battery node's max values.

See also this discussion about similar issues.
https://patchwork.kernel.org/patch/9625331/


> More thoughts?
>
>>>> I'll add these to v14 of my patchset which adds simple-battery
>>>> support. Rob requested a single patch for this file.
>>>>
>>>
>>> Ok, I'll send the tps charger series without this patch, so please,
>>> include this patch in your series.
>>>
>>>> I've been waiting for feedback on v13.2 from Sebastian. If I don't
>>>> hear from him within a few days, I'll post v14.
>>>>
>>>>>  Batteries must be referenced by chargers and/or fuel-gauges
>>>>>  using a phandle. The phandle's property should be named
>>>>> @@ -24,6 +26,8 @@ Example:
>>>>> voltage-min-design-microvolt = <320>;
>>>>> energy-full-design-microwatt-hours = <529>;
>>>>> charge-full-design-microamp-hours = <143>;
>>>>> +   charge-voltage-microvolt = <410>;
>>>>> +   charge-current-microamp = <30>;
>>>>> };
>>>>>
>>>>> charger: charger@11 {
>>>>> --
>>>>> 2.9.3
>>>>>

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Liam Breck]

Hi Enric,



On Thu, Jun 1, 2017 at 1:18 AM, Enric Balletbo Serra
 wrote:
> Hi Liam,
>
> 2017-06-01 9:01 GMT+02:00 Liam Breck :
>> Hi Enric,
>>
>> On Sat, May 27, 2017 at 1:11 PM, Enric Balletbo Serra
>>  wrote:
>>> Hi Liam,
>>>
>>> 2017-05-26 23:20 GMT+02:00 Liam Breck :
>>>> Hi Enric,
>>>>
>>>> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
>>>>  wrote:
>>>>> Add charging voltage and current characteristics to the battery DT for
>>>>> proper handling of the battery by fuel-gauge and charger chips.
>>>>>
>>>>> Signed-off-by: Enric Balletbo i Serra 
>>>>> ---
>>>>> Changes since v2:
>>>>>  - Requested by Sebastian Reichel
>>>>>- Move to its own patch and apply to simple-battery framework.
>>>>> Changes since v1:
>>>>>  - Requested by Rob Herring
>>>>>- Rename ti,charge-* to charge-* to be standard properties.
>>>>>- Use unit suffixes as per bindings/property-units.txt
>>>>>
>>>>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt 
>>>>> b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>> index 63a7028..c87a439 100644
>>>>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>> @@ -12,6 +12,8 @@ Optional Properties:
>>>>>   - voltage-min-design-microvolt: drained battery voltage
>>>>>   - energy-full-design-microwatt-hours: battery design energy
>>>>>   - charge-full-design-microamp-hours: battery design capacity
>>>>> + - charge-voltage-microvolt: battery charging voltage
>>>>> + - charge-current-microamp: battery charging current
>>>>
>>>> I think you mean constant-charge-* which is how you surface these
>>>> properties in your tps65217_charger patch.
>>>>
>>>
>>> Yes, to be strict this is constant-charge-*
>>
>> The DT battery node should carry static battery characteristics. So on
>> reflection, I think you want
>>
>> constant-charge-current-max-microamp
>> constant-charge-voltage-max-microvolt
>>
>> The charger or the user could then safely apply any value <= those.
>>
>> Thoughts?

I'm curious to hear how your hw config requires constant-charge settings?

> Hmm I see your point and I'm thinking now that actually this is not
> what I wanted to do. What I wanted is set the charger voltage and the
> current hence my first patchset was setting these properties in the
> charger node not the battery.

You could certainly support constant-charge-* params in the charger
node. And it's easy to enable userspace to set these via sysfs.

> Said that, I think that you have reason and what we want in battery
> node is the current/voltage max values but we also need to implement a
> mechanism to set the charging voltage/current from userspace or from
> the DT for the charger. The charger should be able to set these values
> and fail if, based in the battery specs, is not supported.

The charger could default constant-charge-* to the battery node's max values.

See also this discussion about similar issues.
https://patchwork.kernel.org/patch/9625331/


> More thoughts?
>
>>>> I'll add these to v14 of my patchset which adds simple-battery
>>>> support. Rob requested a single patch for this file.
>>>>
>>>
>>> Ok, I'll send the tps charger series without this patch, so please,
>>> include this patch in your series.
>>>
>>>> I've been waiting for feedback on v13.2 from Sebastian. If I don't
>>>> hear from him within a few days, I'll post v14.
>>>>
>>>>>  Batteries must be referenced by chargers and/or fuel-gauges
>>>>>  using a phandle. The phandle's property should be named
>>>>> @@ -24,6 +26,8 @@ Example:
>>>>> voltage-min-design-microvolt = <320>;
>>>>> energy-full-design-microwatt-hours = <529>;
>>>>> charge-full-design-microamp-hours = <143>;
>>>>> +   charge-voltage-microvolt = <410>;
>>>>> +   charge-current-microamp = <30>;
>>>>> };
>>>>>
>>>>> charger: charger@11 {
>>>>> --
>>>>> 2.9.3
>>>>>

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Liam Breck]

Hi Enric,

On Sat, May 27, 2017 at 1:11 PM, Enric Balletbo Serra
<eballe...@gmail.com> wrote:
> Hi Liam,
>
> 2017-05-26 23:20 GMT+02:00 Liam Breck <l...@networkimprov.net>:
>> Hi Enric,
>>
>> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
>> <enric.balle...@collabora.com> wrote:
>>> Add charging voltage and current characteristics to the battery DT for
>>> proper handling of the battery by fuel-gauge and charger chips.
>>>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balle...@collabora.com>
>>> ---
>>> Changes since v2:
>>>  - Requested by Sebastian Reichel
>>>- Move to its own patch and apply to simple-battery framework.
>>> Changes since v1:
>>>  - Requested by Rob Herring
>>>- Rename ti,charge-* to charge-* to be standard properties.
>>>- Use unit suffixes as per bindings/property-units.txt
>>>
>>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt 
>>> b/Documentation/devicetree/bindings/power/supply/battery.txt
>>> index 63a7028..c87a439 100644
>>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>> @@ -12,6 +12,8 @@ Optional Properties:
>>>   - voltage-min-design-microvolt: drained battery voltage
>>>   - energy-full-design-microwatt-hours: battery design energy
>>>   - charge-full-design-microamp-hours: battery design capacity
>>> + - charge-voltage-microvolt: battery charging voltage
>>> + - charge-current-microamp: battery charging current
>>
>> I think you mean constant-charge-* which is how you surface these
>> properties in your tps65217_charger patch.
>>
>
> Yes, to be strict this is constant-charge-*

The DT battery node should carry static battery characteristics. So on
reflection, I think you want

constant-charge-current-max-microamp
constant-charge-voltage-max-microvolt

The charger or the user could then safely apply any value <= those.

Thoughts?

>> I'll add these to v14 of my patchset which adds simple-battery
>> support. Rob requested a single patch for this file.
>>
>
> Ok, I'll send the tps charger series without this patch, so please,
> include this patch in your series.
>
>> I've been waiting for feedback on v13.2 from Sebastian. If I don't
>> hear from him within a few days, I'll post v14.
>>
>>>  Batteries must be referenced by chargers and/or fuel-gauges
>>>  using a phandle. The phandle's property should be named
>>> @@ -24,6 +26,8 @@ Example:
>>> voltage-min-design-microvolt = <320>;
>>> energy-full-design-microwatt-hours = <529>;
>>> charge-full-design-microamp-hours = <143>;
>>> +   charge-voltage-microvolt = <410>;
>>> +   charge-current-microamp = <30>;
>>> };
>>>
>>> charger: charger@11 {
>>> --
>>> 2.9.3
>>>

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Liam Breck]

Hi Enric,

On Sat, May 27, 2017 at 1:11 PM, Enric Balletbo Serra
 wrote:
> Hi Liam,
>
> 2017-05-26 23:20 GMT+02:00 Liam Breck :
>> Hi Enric,
>>
>> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
>>  wrote:
>>> Add charging voltage and current characteristics to the battery DT for
>>> proper handling of the battery by fuel-gauge and charger chips.
>>>
>>> Signed-off-by: Enric Balletbo i Serra 
>>> ---
>>> Changes since v2:
>>>  - Requested by Sebastian Reichel
>>>- Move to its own patch and apply to simple-battery framework.
>>> Changes since v1:
>>>  - Requested by Rob Herring
>>>- Rename ti,charge-* to charge-* to be standard properties.
>>>- Use unit suffixes as per bindings/property-units.txt
>>>
>>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt 
>>> b/Documentation/devicetree/bindings/power/supply/battery.txt
>>> index 63a7028..c87a439 100644
>>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>> @@ -12,6 +12,8 @@ Optional Properties:
>>>   - voltage-min-design-microvolt: drained battery voltage
>>>   - energy-full-design-microwatt-hours: battery design energy
>>>   - charge-full-design-microamp-hours: battery design capacity
>>> + - charge-voltage-microvolt: battery charging voltage
>>> + - charge-current-microamp: battery charging current
>>
>> I think you mean constant-charge-* which is how you surface these
>> properties in your tps65217_charger patch.
>>
>
> Yes, to be strict this is constant-charge-*

The DT battery node should carry static battery characteristics. So on
reflection, I think you want

constant-charge-current-max-microamp
constant-charge-voltage-max-microvolt

The charger or the user could then safely apply any value <= those.

Thoughts?

>> I'll add these to v14 of my patchset which adds simple-battery
>> support. Rob requested a single patch for this file.
>>
>
> Ok, I'll send the tps charger series without this patch, so please,
> include this patch in your series.
>
>> I've been waiting for feedback on v13.2 from Sebastian. If I don't
>> hear from him within a few days, I'll post v14.
>>
>>>  Batteries must be referenced by chargers and/or fuel-gauges
>>>  using a phandle. The phandle's property should be named
>>> @@ -24,6 +26,8 @@ Example:
>>> voltage-min-design-microvolt = <320>;
>>> energy-full-design-microwatt-hours = <529>;
>>> charge-full-design-microamp-hours = <143>;
>>> +   charge-voltage-microvolt = <410>;
>>> +   charge-current-microamp = <30>;
>>> };
>>>
>>> charger: charger@11 {
>>> --
>>> 2.9.3
>>>

Re: [PATCH v3 3/4] dt-bindings: power: tps65217_charger: add NTC type for battery temperature measurement.

[Liam Breck]

Hi Enric,

On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
 wrote:
> The TS pin of the TPS56217 connects to the NTC resistor in the battery
> pack. By default the device is setup to support a 10-kohm but can also
> be configured to support a 100-kohm. Add a propietry to configure the
> connected NTC resistor. Therefore, the charger would get the wrong
> temperature information.

This also needs a monitored-battery property, and a battery node in
the example. See https://patchwork.kernel.org/patch/9710803/


> Signed-off-by: Enric Balletbo i Serra 
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>- Get rid of common properties and only maintain ntc-type.
> Changes since v1:
>  - None
>
>  .../devicetree/bindings/power/supply/tps65217_charger.txt  | 7 
> +++
>  1 file changed, 7 insertions(+)
>
> diff --git 
> a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt 
> b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
> index a11072c..851f5c7 100644
> --- a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
> +++ b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
> @@ -6,6 +6,12 @@ Required Properties:
>   Should be <0> for the USB charger and <1> for the AC adapter.
>  -interrupt-names: Should be "USB" and "AC"
>
> +Optional properties:
> +-ti,ntc-type: set the NTC type for battery temperature measurement. The value
> +   must be 0 or 1, where:
> + 0 – 100k (curve 1, B = 3960)
> + 1 – 10k  (curve 2, B = 3480) (default)
> +
>  This node is a subnode of the tps65217 PMIC.
>
>  Example:
> @@ -14,4 +20,5 @@ Example:
> compatible = "ti,tps65217-charger";
> interrupts = <0>, <1>;
> interrupt-names = "USB", "AC";
> +   ti,ntc-type = <1>;
> };
> --
> 2.9.3
>

Re: [PATCH v3 3/4] dt-bindings: power: tps65217_charger: add NTC type for battery temperature measurement.

[Liam Breck]

Hi Enric,

On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
 wrote:
> The TS pin of the TPS56217 connects to the NTC resistor in the battery
> pack. By default the device is setup to support a 10-kohm but can also
> be configured to support a 100-kohm. Add a propietry to configure the
> connected NTC resistor. Therefore, the charger would get the wrong
> temperature information.

This also needs a monitored-battery property, and a battery node in
the example. See https://patchwork.kernel.org/patch/9710803/


> Signed-off-by: Enric Balletbo i Serra 
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>- Get rid of common properties and only maintain ntc-type.
> Changes since v1:
>  - None
>
>  .../devicetree/bindings/power/supply/tps65217_charger.txt  | 7 
> +++
>  1 file changed, 7 insertions(+)
>
> diff --git 
> a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt 
> b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
> index a11072c..851f5c7 100644
> --- a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
> +++ b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
> @@ -6,6 +6,12 @@ Required Properties:
>   Should be <0> for the USB charger and <1> for the AC adapter.
>  -interrupt-names: Should be "USB" and "AC"
>
> +Optional properties:
> +-ti,ntc-type: set the NTC type for battery temperature measurement. The value
> +   must be 0 or 1, where:
> + 0 – 100k (curve 1, B = 3960)
> + 1 – 10k  (curve 2, B = 3480) (default)
> +
>  This node is a subnode of the tps65217 PMIC.
>
>  Example:
> @@ -14,4 +20,5 @@ Example:
> compatible = "ti,tps65217-charger";
> interrupts = <0>, <1>;
> interrupt-names = "USB", "AC";
> +   ti,ntc-type = <1>;
> };
> --
> 2.9.3
>

Re: [PATCH v3 2/4] power: supply: core: add charging voltage/current battery info

[Liam Breck]

Hi Enric,

I'll also incorporate these changes into v14 of my patchset. (See my prev msg.)


On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
 wrote:
> Add the parameters to define the battery charging voltage and charging
> current. Charger driver can get this information from the struct
> power_supply_battery_info and apply the desired value.
>
> Signed-off-by: Enric Balletbo i Serra 
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>- Move to its own patch and apply to simple-battery framework.
> Changes since v1:
>  - Requested by Rob Herring
>- Rename ti,charge-* to charge-* to be standard properties.
>- Use unit suffixes as per bindings/property-units.txt
>
>  drivers/power/supply/power_supply_core.c | 6 ++
>  include/linux/power_supply.h | 2 ++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/power/supply/power_supply_core.c 
> b/drivers/power/supply/power_supply_core.c
> index 862fa8fc..a6857c2 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -530,6 +530,8 @@ int power_supply_get_battery_info(struct power_supply 
> *psy,
> info->energy_full_design_uwh = -EINVAL;
> info->charge_full_design_uah = -EINVAL;
> info->voltage_min_design_uv  = -EINVAL;
> +   info->charge_voltage_uv = -EINVAL;
> +   info->charge_current_ua = -EINVAL;
>
> if (!psy->of_node) {
> dev_warn(>dev, "%s currently only supports devicetree\n",
> @@ -559,6 +561,10 @@ int power_supply_get_battery_info(struct power_supply 
> *psy,
>  >charge_full_design_uah);
> of_property_read_u32(battery_np, "voltage-min-design-microvolt",
>  >voltage_min_design_uv);
> +   of_property_read_u32(battery_np, "charge-voltage-microvolt",
> +>charge_voltage_uv);
> +   of_property_read_u32(battery_np, "charge-current-microamp",
> +>charge_current_ua);
>
> return 0;
>  }
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 8220f7b..3eea323 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -302,6 +302,8 @@ struct power_supply_battery_info {
> int energy_full_design_uwh; /* microWatt-hours */
> int charge_full_design_uah; /* microAmp-hours */
> int voltage_min_design_uv;  /* microVolts */
> +   int charge_voltage_uv;  /* microVolts */
> +   int charge_current_ua;  /* microAmp */
>  };
>
>  extern struct atomic_notifier_head power_supply_notifier;
> --
> 2.9.3
>

Re: [PATCH v3 2/4] power: supply: core: add charging voltage/current battery info

[Liam Breck]

Hi Enric,

I'll also incorporate these changes into v14 of my patchset. (See my prev msg.)


On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
 wrote:
> Add the parameters to define the battery charging voltage and charging
> current. Charger driver can get this information from the struct
> power_supply_battery_info and apply the desired value.
>
> Signed-off-by: Enric Balletbo i Serra 
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>- Move to its own patch and apply to simple-battery framework.
> Changes since v1:
>  - Requested by Rob Herring
>- Rename ti,charge-* to charge-* to be standard properties.
>- Use unit suffixes as per bindings/property-units.txt
>
>  drivers/power/supply/power_supply_core.c | 6 ++
>  include/linux/power_supply.h | 2 ++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/power/supply/power_supply_core.c 
> b/drivers/power/supply/power_supply_core.c
> index 862fa8fc..a6857c2 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -530,6 +530,8 @@ int power_supply_get_battery_info(struct power_supply 
> *psy,
> info->energy_full_design_uwh = -EINVAL;
> info->charge_full_design_uah = -EINVAL;
> info->voltage_min_design_uv  = -EINVAL;
> +   info->charge_voltage_uv = -EINVAL;
> +   info->charge_current_ua = -EINVAL;
>
> if (!psy->of_node) {
> dev_warn(>dev, "%s currently only supports devicetree\n",
> @@ -559,6 +561,10 @@ int power_supply_get_battery_info(struct power_supply 
> *psy,
>  >charge_full_design_uah);
> of_property_read_u32(battery_np, "voltage-min-design-microvolt",
>  >voltage_min_design_uv);
> +   of_property_read_u32(battery_np, "charge-voltage-microvolt",
> +>charge_voltage_uv);
> +   of_property_read_u32(battery_np, "charge-current-microamp",
> +>charge_current_ua);
>
> return 0;
>  }
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 8220f7b..3eea323 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -302,6 +302,8 @@ struct power_supply_battery_info {
> int energy_full_design_uwh; /* microWatt-hours */
> int charge_full_design_uah; /* microAmp-hours */
> int voltage_min_design_uv;  /* microVolts */
> +   int charge_voltage_uv;  /* microVolts */
> +   int charge_current_ua;  /* microAmp */
>  };
>
>  extern struct atomic_notifier_head power_supply_notifier;
> --
> 2.9.3
>

Re: [PATCH v3 4/4] power: tps65217_charger: add support for NTC type, voltage and current charge

[Liam Breck]

Hi Enric,


On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
 wrote:
> Allow the possibility to configure the charge and the current voltage of
> the charger and also the NTC type for battery temperature measurement.
>
> Signed-off-by: Enric Balletbo i Serra 
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>- Use the simple-battery framework
>- Use device_property_read_u32 instead of of_property_read_u32
> Changes since v1:
>  - None
>
>  drivers/power/supply/tps65217_charger.c | 194 
> ++--
>  include/linux/mfd/tps65217.h|   2 +
>  2 files changed, 188 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/power/supply/tps65217_charger.c 
> b/drivers/power/supply/tps65217_charger.c
> index 1f52340..5939e77 100644
> --- a/drivers/power/supply/tps65217_charger.c
> +++ b/drivers/power/supply/tps65217_charger.c
> @@ -39,6 +39,12 @@
>  #define NUM_CHARGER_IRQS   2
>  #define POLL_INTERVAL  (HZ * 2)
>
> +struct tps65217_charger_platform_data {
> +   int charge_voltage_uv;
> +   int charge_current_ua;
> +   int ntc_type;
> +};
> +

It's not my department, but maybe this should be called devprops or
similar, since platform_data refers to the pre-devicetree config
scheme?


>  struct tps65217_charger {
> struct tps65217 *tps;
> struct device *dev;
> @@ -48,16 +54,84 @@ struct tps65217_charger {
> int prev_online;
>
> struct task_struct  *poll_task;
> +   struct tps65217_charger_platform_data *pdata;
>  };
>
>  static enum power_supply_property tps65217_charger_props[] = {
> POWER_SUPPLY_PROP_ONLINE,
> +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
>  };
>
> -static int tps65217_config_charger(struct tps65217_charger *charger)
> +static int tps65217_set_charge_current(struct tps65217_charger *charger,
> +  unsigned int uamp)
> +{
> +   int ret, val;
> +
> +   dev_dbg(charger->dev, "setting charge current to %d uA\n", uamp);
> +
> +   if (uamp == 30)
> +   val = 0x00;
> +   else if (uamp == 40)
> +   val = 0x01;
> +   else if (uamp == 50)
> +   val = 0x02;
> +   else if (uamp == 70)
> +   val = 0x03;
> +   else
> +   return -EINVAL;
> +
> +   ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG3,
> +   TPS65217_CHGCONFIG3_ICHRG_MASK,
> +   val << TPS65217_CHGCONFIG3_ICHRG_SHIFT,
> +   TPS65217_PROTECT_NONE);
> +   if (ret) {
> +   dev_err(charger->dev,
> +   "failed to set ICHRG setting to 0x%02x (err: %d)\n",
> +   val, ret);
> +   return ret;
> +   }
> +
> +   return 0;
> +}
> +
> +static int tps65217_set_charge_voltage(struct tps65217_charger *charger,
> +  unsigned int uvolt)
> +{
> +   int ret, val;
> +
> +   dev_dbg(charger->dev, "setting charge voltage to %d uV\n", uvolt);
> +
> +   if (uvolt != 410 && uvolt != 415 &&
> +   uvolt != 420 && uvolt != 425)
> +   return -EINVAL;
> +
> +   val = (uvolt - 410) / 5;
> +
> +   ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG2,
> +   TPS65217_CHGCONFIG2_VOREG_MASK,
> +   val << TPS65217_CHGCONFIG2_VOREG_SHIFT,
> +   TPS65217_PROTECT_NONE);
> +   if (ret) {
> +   dev_err(charger->dev,
> +   "failed to set VOCHG setting to 0x%02x (err: %d)\n",
> +   val, ret);
> +   return ret;
> +   }
> +
> +   return 0;
> +}
> +
> +static int tps65217_set_ntc_type(struct tps65217_charger *charger,
> +unsigned int ntc)
>  {
> int ret;
>
> +   dev_dbg(charger->dev, "setting NTC type to %d\n", ntc);
> +
> +   if (ntc != 0 && ntc != 1)
> +   return -EINVAL;
> +
> /*
>  * tps65217 rev. G, p. 31 (see p. 32 for NTC schematic)
>  *
> @@ -74,14 +148,57 @@ static int tps65217_config_charger(struct 
> tps65217_charger *charger)
>  * NTC TYPE (for battery temperature measurement)
>  *   0 – 100k (curve 1, B = 3960)
>  *   1 – 10k  (curve 2, B = 3480) (default on reset)
> -*
>  */
> -   ret = tps65217_clear_bits(charger->tps, TPS65217_REG_CHGCONFIG1,
> - TPS65217_CHGCONFIG1_NTC_TYPE,
> - TPS65217_PROTECT_NONE);
> +   if (ntc) {
> +   ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG1,
> +   

Re: [PATCH v3 4/4] power: tps65217_charger: add support for NTC type, voltage and current charge

[Liam Breck]

Hi Enric,


On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
 wrote:
> Allow the possibility to configure the charge and the current voltage of
> the charger and also the NTC type for battery temperature measurement.
>
> Signed-off-by: Enric Balletbo i Serra 
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>- Use the simple-battery framework
>- Use device_property_read_u32 instead of of_property_read_u32
> Changes since v1:
>  - None
>
>  drivers/power/supply/tps65217_charger.c | 194 
> ++--
>  include/linux/mfd/tps65217.h|   2 +
>  2 files changed, 188 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/power/supply/tps65217_charger.c 
> b/drivers/power/supply/tps65217_charger.c
> index 1f52340..5939e77 100644
> --- a/drivers/power/supply/tps65217_charger.c
> +++ b/drivers/power/supply/tps65217_charger.c
> @@ -39,6 +39,12 @@
>  #define NUM_CHARGER_IRQS   2
>  #define POLL_INTERVAL  (HZ * 2)
>
> +struct tps65217_charger_platform_data {
> +   int charge_voltage_uv;
> +   int charge_current_ua;
> +   int ntc_type;
> +};
> +

It's not my department, but maybe this should be called devprops or
similar, since platform_data refers to the pre-devicetree config
scheme?


>  struct tps65217_charger {
> struct tps65217 *tps;
> struct device *dev;
> @@ -48,16 +54,84 @@ struct tps65217_charger {
> int prev_online;
>
> struct task_struct  *poll_task;
> +   struct tps65217_charger_platform_data *pdata;
>  };
>
>  static enum power_supply_property tps65217_charger_props[] = {
> POWER_SUPPLY_PROP_ONLINE,
> +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> +   POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
>  };
>
> -static int tps65217_config_charger(struct tps65217_charger *charger)
> +static int tps65217_set_charge_current(struct tps65217_charger *charger,
> +  unsigned int uamp)
> +{
> +   int ret, val;
> +
> +   dev_dbg(charger->dev, "setting charge current to %d uA\n", uamp);
> +
> +   if (uamp == 30)
> +   val = 0x00;
> +   else if (uamp == 40)
> +   val = 0x01;
> +   else if (uamp == 50)
> +   val = 0x02;
> +   else if (uamp == 70)
> +   val = 0x03;
> +   else
> +   return -EINVAL;
> +
> +   ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG3,
> +   TPS65217_CHGCONFIG3_ICHRG_MASK,
> +   val << TPS65217_CHGCONFIG3_ICHRG_SHIFT,
> +   TPS65217_PROTECT_NONE);
> +   if (ret) {
> +   dev_err(charger->dev,
> +   "failed to set ICHRG setting to 0x%02x (err: %d)\n",
> +   val, ret);
> +   return ret;
> +   }
> +
> +   return 0;
> +}
> +
> +static int tps65217_set_charge_voltage(struct tps65217_charger *charger,
> +  unsigned int uvolt)
> +{
> +   int ret, val;
> +
> +   dev_dbg(charger->dev, "setting charge voltage to %d uV\n", uvolt);
> +
> +   if (uvolt != 410 && uvolt != 415 &&
> +   uvolt != 420 && uvolt != 425)
> +   return -EINVAL;
> +
> +   val = (uvolt - 410) / 5;
> +
> +   ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG2,
> +   TPS65217_CHGCONFIG2_VOREG_MASK,
> +   val << TPS65217_CHGCONFIG2_VOREG_SHIFT,
> +   TPS65217_PROTECT_NONE);
> +   if (ret) {
> +   dev_err(charger->dev,
> +   "failed to set VOCHG setting to 0x%02x (err: %d)\n",
> +   val, ret);
> +   return ret;
> +   }
> +
> +   return 0;
> +}
> +
> +static int tps65217_set_ntc_type(struct tps65217_charger *charger,
> +unsigned int ntc)
>  {
> int ret;
>
> +   dev_dbg(charger->dev, "setting NTC type to %d\n", ntc);
> +
> +   if (ntc != 0 && ntc != 1)
> +   return -EINVAL;
> +
> /*
>  * tps65217 rev. G, p. 31 (see p. 32 for NTC schematic)
>  *
> @@ -74,14 +148,57 @@ static int tps65217_config_charger(struct 
> tps65217_charger *charger)
>  * NTC TYPE (for battery temperature measurement)
>  *   0 – 100k (curve 1, B = 3960)
>  *   1 – 10k  (curve 2, B = 3480) (default on reset)
> -*
>  */
> -   ret = tps65217_clear_bits(charger->tps, TPS65217_REG_CHGCONFIG1,
> - TPS65217_CHGCONFIG1_NTC_TYPE,
> - TPS65217_PROTECT_NONE);
> +   if (ntc) {
> +   ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG1,
> +   TPS65217_CHGCONFIG1_NTC_TYPE,
> +   

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Liam Breck]

Hi Enric,

On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
 wrote:
> Add charging voltage and current characteristics to the battery DT for
> proper handling of the battery by fuel-gauge and charger chips.
>
> Signed-off-by: Enric Balletbo i Serra 
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>- Move to its own patch and apply to simple-battery framework.
> Changes since v1:
>  - Requested by Rob Herring
>- Rename ti,charge-* to charge-* to be standard properties.
>- Use unit suffixes as per bindings/property-units.txt
>
>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt 
> b/Documentation/devicetree/bindings/power/supply/battery.txt
> index 63a7028..c87a439 100644
> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> @@ -12,6 +12,8 @@ Optional Properties:
>   - voltage-min-design-microvolt: drained battery voltage
>   - energy-full-design-microwatt-hours: battery design energy
>   - charge-full-design-microamp-hours: battery design capacity
> + - charge-voltage-microvolt: battery charging voltage
> + - charge-current-microamp: battery charging current

I think you mean constant-charge-* which is how you surface these
properties in your tps65217_charger patch.

I'll add these to v14 of my patchset which adds simple-battery
support. Rob requested a single patch for this file.

I've been waiting for feedback on v13.2 from Sebastian. If I don't
hear from him within a few days, I'll post v14.

>  Batteries must be referenced by chargers and/or fuel-gauges
>  using a phandle. The phandle's property should be named
> @@ -24,6 +26,8 @@ Example:
> voltage-min-design-microvolt = <320>;
> energy-full-design-microwatt-hours = <529>;
> charge-full-design-microamp-hours = <143>;
> +   charge-voltage-microvolt = <410>;
> +   charge-current-microamp = <30>;
> };
>
> charger: charger@11 {
> --
> 2.9.3
>

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Liam Breck]

Hi Enric,

On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
 wrote:
> Add charging voltage and current characteristics to the battery DT for
> proper handling of the battery by fuel-gauge and charger chips.
>
> Signed-off-by: Enric Balletbo i Serra 
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>- Move to its own patch and apply to simple-battery framework.
> Changes since v1:
>  - Requested by Rob Herring
>- Rename ti,charge-* to charge-* to be standard properties.
>- Use unit suffixes as per bindings/property-units.txt
>
>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt 
> b/Documentation/devicetree/bindings/power/supply/battery.txt
> index 63a7028..c87a439 100644
> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> @@ -12,6 +12,8 @@ Optional Properties:
>   - voltage-min-design-microvolt: drained battery voltage
>   - energy-full-design-microwatt-hours: battery design energy
>   - charge-full-design-microamp-hours: battery design capacity
> + - charge-voltage-microvolt: battery charging voltage
> + - charge-current-microamp: battery charging current

I think you mean constant-charge-* which is how you surface these
properties in your tps65217_charger patch.

I'll add these to v14 of my patchset which adds simple-battery
support. Rob requested a single patch for this file.

I've been waiting for feedback on v13.2 from Sebastian. If I don't
hear from him within a few days, I'll post v14.

>  Batteries must be referenced by chargers and/or fuel-gauges
>  using a phandle. The phandle's property should be named
> @@ -24,6 +26,8 @@ Example:
> voltage-min-design-microvolt = <320>;
> energy-full-design-microwatt-hours = <529>;
> charge-full-design-microamp-hours = <143>;
> +   charge-voltage-microvolt = <410>;
> +   charge-current-microamp = <30>;
> };
>
> charger: charger@11 {
> --
> 2.9.3
>

[PATCH v13 01/11] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring <r...@kernel.org>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <ker...@networkimprov.net>
Acked-by: Sebastian Reichel <s...@kernel.org>
Acked-by: Rob Herring <r...@kernel.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v13 01/11] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay 

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring 
Cc: Mark Rutland 
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay 
Signed-off-by: Liam Breck 
Acked-by: Sebastian Reichel 
Acked-by: Rob Herring 
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

Re: [PATCH] power: bq24190_charger: mark PM functions as __maybe_unused

[Liam Breck]

On Mon, Mar 20, 2017 at 6:14 AM, Arnd Bergmann <a...@arndb.de> wrote:
> Without CONFIG_PM, we get a harmless warning:
>
> drivers/power/supply/bq24190_charger.c:1514:12: error: 
> 'bq24190_runtime_resume' defined but not used [-Werror=unused-function]
> drivers/power/supply/bq24190_charger.c:1501:12: error: 
> 'bq24190_runtime_suspend' defined but not used [-Werror=unused-function]
>
> To avoid the warning, we can mark all four PM functions as __maybe_unused,
> which also lets us remove the incorrect #ifdef.
>
> Fixes: 3d8090cba638 ("power: bq24190_charger: Check the interrupt status on 
> resume")
> Signed-off-by: Arnd Bergmann <a...@arndb.de>

Cool, thx!

Acked-by: Liam Breck <ker...@networkimprov.net>

> ---
>  drivers/power/supply/bq24190_charger.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c 
> b/drivers/power/supply/bq24190_charger.c
> index 6d80670586eb..451f2bc05ea5 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1498,7 +1498,7 @@ static int bq24190_remove(struct i2c_client *client)
> return 0;
>  }
>
> -static int bq24190_runtime_suspend(struct device *dev)
> +static __maybe_unused int bq24190_runtime_suspend(struct device *dev)
>  {
> struct i2c_client *client = to_i2c_client(dev);
> struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> @@ -1511,7 +1511,7 @@ static int bq24190_runtime_suspend(struct device *dev)
> return 0;
>  }
>
> -static int bq24190_runtime_resume(struct device *dev)
> +static __maybe_unused int bq24190_runtime_resume(struct device *dev)
>  {
> struct i2c_client *client = to_i2c_client(dev);
> struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> @@ -1527,8 +1527,7 @@ static int bq24190_runtime_resume(struct device *dev)
> return 0;
>  }
>
> -#ifdef CONFIG_PM_SLEEP
> -static int bq24190_pm_suspend(struct device *dev)
> +static __maybe_unused int bq24190_pm_suspend(struct device *dev)
>  {
> struct i2c_client *client = to_i2c_client(dev);
> struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> @@ -1550,7 +1549,7 @@ static int bq24190_pm_suspend(struct device *dev)
> return 0;
>  }
>
> -static int bq24190_pm_resume(struct device *dev)
> +static __maybe_unused int bq24190_pm_resume(struct device *dev)
>  {
> struct i2c_client *client = to_i2c_client(dev);
> struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> @@ -1580,7 +1579,6 @@ static int bq24190_pm_resume(struct device *dev)
>
> return 0;
>  }
> -#endif
>
>  static const struct dev_pm_ops bq24190_pm_ops = {
> SET_RUNTIME_PM_OPS(bq24190_runtime_suspend, bq24190_runtime_resume,
> --
> 2.9.0
>

Re: [PATCH] power: bq24190_charger: mark PM functions as __maybe_unused

[Liam Breck]

On Mon, Mar 20, 2017 at 6:14 AM, Arnd Bergmann  wrote:
> Without CONFIG_PM, we get a harmless warning:
>
> drivers/power/supply/bq24190_charger.c:1514:12: error: 
> 'bq24190_runtime_resume' defined but not used [-Werror=unused-function]
> drivers/power/supply/bq24190_charger.c:1501:12: error: 
> 'bq24190_runtime_suspend' defined but not used [-Werror=unused-function]
>
> To avoid the warning, we can mark all four PM functions as __maybe_unused,
> which also lets us remove the incorrect #ifdef.
>
> Fixes: 3d8090cba638 ("power: bq24190_charger: Check the interrupt status on 
> resume")
> Signed-off-by: Arnd Bergmann 

Cool, thx!

Acked-by: Liam Breck 

> ---
>  drivers/power/supply/bq24190_charger.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c 
> b/drivers/power/supply/bq24190_charger.c
> index 6d80670586eb..451f2bc05ea5 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1498,7 +1498,7 @@ static int bq24190_remove(struct i2c_client *client)
> return 0;
>  }
>
> -static int bq24190_runtime_suspend(struct device *dev)
> +static __maybe_unused int bq24190_runtime_suspend(struct device *dev)
>  {
> struct i2c_client *client = to_i2c_client(dev);
> struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> @@ -1511,7 +1511,7 @@ static int bq24190_runtime_suspend(struct device *dev)
> return 0;
>  }
>
> -static int bq24190_runtime_resume(struct device *dev)
> +static __maybe_unused int bq24190_runtime_resume(struct device *dev)
>  {
> struct i2c_client *client = to_i2c_client(dev);
> struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> @@ -1527,8 +1527,7 @@ static int bq24190_runtime_resume(struct device *dev)
> return 0;
>  }
>
> -#ifdef CONFIG_PM_SLEEP
> -static int bq24190_pm_suspend(struct device *dev)
> +static __maybe_unused int bq24190_pm_suspend(struct device *dev)
>  {
> struct i2c_client *client = to_i2c_client(dev);
> struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> @@ -1550,7 +1549,7 @@ static int bq24190_pm_suspend(struct device *dev)
> return 0;
>  }
>
> -static int bq24190_pm_resume(struct device *dev)
> +static __maybe_unused int bq24190_pm_resume(struct device *dev)
>  {
> struct i2c_client *client = to_i2c_client(dev);
> struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> @@ -1580,7 +1579,6 @@ static int bq24190_pm_resume(struct device *dev)
>
> return 0;
>  }
> -#endif
>
>  static const struct dev_pm_ops bq24190_pm_ops = {
> SET_RUNTIME_PM_OPS(bq24190_runtime_suspend, bq24190_runtime_resume,
> --
> 2.9.0
>

[PATCH v11 02/10] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring <r...@kernel.org>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <ker...@networkimprov.net>
Acked-by: Sebastian Reichel <s...@kernel.org>
Acked-by: Rob Herring <r...@kernel.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v11 02/10] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay 

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring 
Cc: Mark Rutland 
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay 
Signed-off-by: Liam Breck 
Acked-by: Sebastian Reichel 
Acked-by: Rob Herring 
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v10 2/8] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring <r...@kernel.org>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <ker...@networkimprov.net>
Acked-by: Sebastian Reichel <s...@kernel.org>
Acked-by: Rob Herring <r...@kernel.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v10 2/8] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay 

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring 
Cc: Mark Rutland 
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay 
Signed-off-by: Liam Breck 
Acked-by: Sebastian Reichel 
Acked-by: Rob Herring 
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v9 2/8] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring <r...@kernel.org>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <ker...@networkimprov.net>
Acked-by: Sebastian Reichel <s...@kernel.org>
Acked-by: Rob Herring <r...@kernel.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v9 2/8] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay 

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring 
Cc: Mark Rutland 
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay 
Signed-off-by: Liam Breck 
Acked-by: Sebastian Reichel 
Acked-by: Rob Herring 
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v8 2/9] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring <r...@kernel.org>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <ker...@networkimprov.net>
Acked-by: Sebastian Reichel <s...@kernel.org>
Acked-by: Rob Herring <r...@kernel.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v8 2/9] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay 

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring 
Cc: Mark Rutland 
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay 
Signed-off-by: Liam Breck 
Acked-by: Sebastian Reichel 
Acked-by: Rob Herring 
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v7 2/9] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring <r...@kernel.org>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <ker...@networkimprov.net>
Acked-by: Sebastian Reichel <s...@kernel.org>
Acked-by: Rob Herring <r...@kernel.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v7 2/9] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay 

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring 
Cc: Mark Rutland 
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay 
Signed-off-by: Liam Breck 
Acked-by: Sebastian Reichel 
Acked-by: Rob Herring 
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

Re: [PATCH v3 01/18] dt-bindings: power: battery: add constant-charge-current property

[Liam Breck]

On Wed, Feb 15, 2017 at 12:53 AM, Quentin Schulz
<quentin.sch...@free-electrons.com> wrote:
> Hi,
>
> On 15/02/2017 01:46, Liam Breck wrote:
>>
>> On Tue, 14 Feb 2017 10:40:55 +0100 Quentin Schulz wrote:
>>> This adds the constant-charge-current property to the list of optional
>>> properties of the battery.
>>>
>>> The constant charge current is critical for batteries as they can't
>>> handle all charge currents.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.sch...@free-electrons.com>
>>> ---
>>>
>>> added in v3
>>>
>>>  Documentation/devicetree/bindings/power/supply/battery.txt | 2 ++
>>
>> Is constant-charge-current dependent on the battery (e.g. capacity, nominal 
>> voltage, etc) or the
>> system (charger chip, input current/voltage, etc)?
>>
>> It belongs in Doc.../power/supply/battery.txt if it's a characteristic of 
>> the battery.
>>
>> Note, this page asserts that constant-current charging applies to NiMH 
>> batteries:
>> http://power-topics.blogspot.com/2016/05/constant-voltage-constant-current.html
>>
>> Related properties to be added to battery.txt near-future in a patchset for 
>> the BQ24190
>> charger are as follows. These are not currently in enum 
>> power_supply_property, so the actual names
>> are still to be decided.
>>
>> precharge-current-microamp:
>>maximum charge current during precharge phase (typically 20% of battery 
>> capacity)
>>
>> termination-current-microamp (or endcharge-current):
>>a charge cycle terminates when the battery voltage is above recharge 
>> threshold,
>>and the current is below this setting (typically 10% of battery capacity)
>>
>
> We have a client with a board whose battery accepts a maximum of 300mA
> for charging. So depending on the battery, we cannot have any charging
> current we want. The AXP PmMICs set constant charge current in a range
> of 300mA-1800mA, so it is enforced by the charger but needs to be
> adapted depending on the battery present in the system.
>
> The AXP PMICs charge battery with constant current (Ichrg) between the
> trickle voltage (Vtrkl which is ~3.0V) and the targeted voltage (Vtrgt;
> which seems to be the voltage telling the battery is fully charged).
>
> So if I understand correctly, "my" constant-charge-current would be
> located in the charging cycle between your precharge-current-microamp
> and the termination-current-microamp as it is the current for the
> charging process as a whole.
>
> See here[1] for the explanation in the datasheet (page 20).
>
> That would definitely match what is explained in your link for constant
> current.
>
> [1] http://dl.linux-sunxi.org/AXP/AXP209_Datasheet_v1.0en.pdf
>
> Let me know if something seems odd,

Sounds OK to me. I'm happy to ack this, after my patchset goes in.
I'll be posting v7 this weekend, with a minor addition to battery.txt.

BTW there's rather a lot of ppl and lists CC'd on this, none of which
are listed in the patch comment...

> Thanks,
> Quentin
>
> --
> Quentin Schulz, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH v3 01/18] dt-bindings: power: battery: add constant-charge-current property

[Liam Breck]

On Wed, Feb 15, 2017 at 12:53 AM, Quentin Schulz
 wrote:
> Hi,
>
> On 15/02/2017 01:46, Liam Breck wrote:
>>
>> On Tue, 14 Feb 2017 10:40:55 +0100 Quentin Schulz wrote:
>>> This adds the constant-charge-current property to the list of optional
>>> properties of the battery.
>>>
>>> The constant charge current is critical for batteries as they can't
>>> handle all charge currents.
>>>
>>> Signed-off-by: Quentin Schulz 
>>> ---
>>>
>>> added in v3
>>>
>>>  Documentation/devicetree/bindings/power/supply/battery.txt | 2 ++
>>
>> Is constant-charge-current dependent on the battery (e.g. capacity, nominal 
>> voltage, etc) or the
>> system (charger chip, input current/voltage, etc)?
>>
>> It belongs in Doc.../power/supply/battery.txt if it's a characteristic of 
>> the battery.
>>
>> Note, this page asserts that constant-current charging applies to NiMH 
>> batteries:
>> http://power-topics.blogspot.com/2016/05/constant-voltage-constant-current.html
>>
>> Related properties to be added to battery.txt near-future in a patchset for 
>> the BQ24190
>> charger are as follows. These are not currently in enum 
>> power_supply_property, so the actual names
>> are still to be decided.
>>
>> precharge-current-microamp:
>>maximum charge current during precharge phase (typically 20% of battery 
>> capacity)
>>
>> termination-current-microamp (or endcharge-current):
>>a charge cycle terminates when the battery voltage is above recharge 
>> threshold,
>>and the current is below this setting (typically 10% of battery capacity)
>>
>
> We have a client with a board whose battery accepts a maximum of 300mA
> for charging. So depending on the battery, we cannot have any charging
> current we want. The AXP PmMICs set constant charge current in a range
> of 300mA-1800mA, so it is enforced by the charger but needs to be
> adapted depending on the battery present in the system.
>
> The AXP PMICs charge battery with constant current (Ichrg) between the
> trickle voltage (Vtrkl which is ~3.0V) and the targeted voltage (Vtrgt;
> which seems to be the voltage telling the battery is fully charged).
>
> So if I understand correctly, "my" constant-charge-current would be
> located in the charging cycle between your precharge-current-microamp
> and the termination-current-microamp as it is the current for the
> charging process as a whole.
>
> See here[1] for the explanation in the datasheet (page 20).
>
> That would definitely match what is explained in your link for constant
> current.
>
> [1] http://dl.linux-sunxi.org/AXP/AXP209_Datasheet_v1.0en.pdf
>
> Let me know if something seems odd,

Sounds OK to me. I'm happy to ack this, after my patchset goes in.
I'll be posting v7 this weekend, with a minor addition to battery.txt.

BTW there's rather a lot of ppl and lists CC'd on this, none of which
are listed in the patch comment...

> Thanks,
> Quentin
>
> --
> Quentin Schulz, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH v3 01/18] dt-bindings: power: battery: add constant-charge-current property

[Liam Breck]

On Tue, 14 Feb 2017 10:40:55 +0100 Quentin Schulz wrote:
> This adds the constant-charge-current property to the list of optional
> properties of the battery.
>
> The constant charge current is critical for batteries as they can't
> handle all charge currents.
>
> Signed-off-by: Quentin Schulz 
> ---
>
> added in v3
>
>  Documentation/devicetree/bindings/power/supply/battery.txt | 2 ++

Is constant-charge-current dependent on the battery (e.g. capacity, nominal 
voltage, etc) or the 
system (charger chip, input current/voltage, etc)?

It belongs in Doc.../power/supply/battery.txt if it's a characteristic of the 
battery.

Note, this page asserts that constant-current charging applies to NiMH 
batteries:
http://power-topics.blogspot.com/2016/05/constant-voltage-constant-current.html

Related properties to be added to battery.txt near-future in a patchset for the 
BQ24190 
charger are as follows. These are not currently in enum power_supply_property, 
so the actual names 
are still to be decided.

precharge-current-microamp:
   maximum charge current during precharge phase (typically 20% of battery 
capacity)

termination-current-microamp (or endcharge-current):
   a charge cycle terminates when the battery voltage is above recharge 
threshold,
   and the current is below this setting (typically 10% of battery capacity)


~.~

Re: [PATCH v3 01/18] dt-bindings: power: battery: add constant-charge-current property

[Liam Breck]

On Tue, 14 Feb 2017 10:40:55 +0100 Quentin Schulz wrote:
> This adds the constant-charge-current property to the list of optional
> properties of the battery.
>
> The constant charge current is critical for batteries as they can't
> handle all charge currents.
>
> Signed-off-by: Quentin Schulz 
> ---
>
> added in v3
>
>  Documentation/devicetree/bindings/power/supply/battery.txt | 2 ++

Is constant-charge-current dependent on the battery (e.g. capacity, nominal 
voltage, etc) or the 
system (charger chip, input current/voltage, etc)?

It belongs in Doc.../power/supply/battery.txt if it's a characteristic of the 
battery.

Note, this page asserts that constant-current charging applies to NiMH 
batteries:
http://power-topics.blogspot.com/2016/05/constant-voltage-constant-current.html

Related properties to be added to battery.txt near-future in a patchset for the 
BQ24190 
charger are as follows. These are not currently in enum power_supply_property, 
so the actual names 
are still to be decided.

precharge-current-microamp:
   maximum charge current during precharge phase (typically 20% of battery 
capacity)

termination-current-microamp (or endcharge-current):
   a charge cycle terminates when the battery voltage is above recharge 
threshold,
   and the current is below this setting (typically 10% of battery capacity)


~.~

[PATCH v6 2/8] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring <r...@kernel.org>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <ker...@networkimprov.net>
Acked-by: Sebastian Reichel <s...@kernel.org>
Acked-by: Rob Herring <r...@kernel.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v6 2/8] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay 

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring 
Cc: Mark Rutland 
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay 
Signed-off-by: Liam Breck 
Acked-by: Sebastian Reichel 
Acked-by: Rob Herring 
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

Re: [PATCH] power: supply: Add driver for TI BQ2416X battery charger

[Liam Breck]

G'day,

On 7 Feb 2017 at 01:09:09, Wojciech Ziemba wrote:
> diff --git a/Documentation/devicetree/bindings/power/supply/bq2416x.txt 
> b/Documentation/devicetree/bindings/power/supply/bq2416x.txt
> new file mode 100644
> index 000..8f4b814
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/bq2416x.txt

DT docs are usually added/amended in a separate patch.

> @@ -0,0 +1,71 @@ 
> +Binding for TI bq2416x Li-Ion Charger
> ...
> +Optional properties:
> +===
> +- interrupt-names:   Meanigfull irq name.
> +- ti,charge-voltage: Charge volatge [mV].
> +- ti,charge-current: Charge current [mA].
> +- ti,termination-current:Termination current [mA}.
> +- ti,in-current-limit:   IN source current limit. enum:
> + - IN_CURR_LIM_1500MA (0)
> + - IN_CURR_LIM_2500MA (1)
> +
> +- ti,usb-current-limit:  USB source current limit. enum:
> + - USB_CURR_LIM_100MA (0)
> + - USB_CURR_LIM_150MA (1)
> + - USB_CURR_LIM_500MA (2)
> + - USB_CURR_LIM_800MA (3)
> + - USB_CURR_LIM_900MA (4)
> + - USB_CURR_LIM_1500MA (5)

DT values are usually uA, uV, uAh, etc.
DT properties usually get a unit suffix, e.g. -microamp.
See Documentation/devicetree/bindings/property-units.txt
and https://patchwork.kernel.org/patch/941/

> +- ti,status-pin-enable:  0 or 1. Enable charge status output 
> STAT pin.
> +- ti,current-termination-enable:0 or 1. Enable charge current termination.
> +- ti,usb-dpm-voltage:USB dpm voltage [mV]. Refer to 
> datasheet.
> +- ti,in-dpm-voltage: IN dpm voltage [mV].
> +- ti,safety-timer:   Safety timer. enum:
> + - TMR_27MIN (0)
> + - TMR_6H (1)
> + - TMR_9H (2)
> + - TMR_OFF (3)
> +
> +- ti,supplied-to:string array: max 4. Names of devices to which
> + the charger supplies.

You might want to use and/or add to power_supply_battery_info, which
Sebastian asked for, see:
https://patchwork.kernel.org/patch/939/
https://patchwork.kernel.org/patch/945/
https://patchwork.kernel.org/patch/947/
https://patchwork.kernel.org/patch/949/

I'm also at work on a patch for BQ24190 which adds:
  battery:precharge-current-microamp -> info.precharge_current_ua ->
POWER_SUPPLY_PROP_PRECHARGE_CURRENT
  battery:endcharge-current-microamp -> info.endcharge_current_ua ->
POWER_SUPPLY_PROP_ENDCHARGE_CURRENT

Hope that helps!

Re: [PATCH] power: supply: Add driver for TI BQ2416X battery charger

[Liam Breck]

G'day,

On 7 Feb 2017 at 01:09:09, Wojciech Ziemba wrote:
> diff --git a/Documentation/devicetree/bindings/power/supply/bq2416x.txt 
> b/Documentation/devicetree/bindings/power/supply/bq2416x.txt
> new file mode 100644
> index 000..8f4b814
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/bq2416x.txt

DT docs are usually added/amended in a separate patch.

> @@ -0,0 +1,71 @@ 
> +Binding for TI bq2416x Li-Ion Charger
> ...
> +Optional properties:
> +===
> +- interrupt-names:   Meanigfull irq name.
> +- ti,charge-voltage: Charge volatge [mV].
> +- ti,charge-current: Charge current [mA].
> +- ti,termination-current:Termination current [mA}.
> +- ti,in-current-limit:   IN source current limit. enum:
> + - IN_CURR_LIM_1500MA (0)
> + - IN_CURR_LIM_2500MA (1)
> +
> +- ti,usb-current-limit:  USB source current limit. enum:
> + - USB_CURR_LIM_100MA (0)
> + - USB_CURR_LIM_150MA (1)
> + - USB_CURR_LIM_500MA (2)
> + - USB_CURR_LIM_800MA (3)
> + - USB_CURR_LIM_900MA (4)
> + - USB_CURR_LIM_1500MA (5)

DT values are usually uA, uV, uAh, etc.
DT properties usually get a unit suffix, e.g. -microamp.
See Documentation/devicetree/bindings/property-units.txt
and https://patchwork.kernel.org/patch/941/

> +- ti,status-pin-enable:  0 or 1. Enable charge status output 
> STAT pin.
> +- ti,current-termination-enable:0 or 1. Enable charge current termination.
> +- ti,usb-dpm-voltage:USB dpm voltage [mV]. Refer to 
> datasheet.
> +- ti,in-dpm-voltage: IN dpm voltage [mV].
> +- ti,safety-timer:   Safety timer. enum:
> + - TMR_27MIN (0)
> + - TMR_6H (1)
> + - TMR_9H (2)
> + - TMR_OFF (3)
> +
> +- ti,supplied-to:string array: max 4. Names of devices to which
> + the charger supplies.

You might want to use and/or add to power_supply_battery_info, which
Sebastian asked for, see:
https://patchwork.kernel.org/patch/939/
https://patchwork.kernel.org/patch/945/
https://patchwork.kernel.org/patch/947/
https://patchwork.kernel.org/patch/949/

I'm also at work on a patch for BQ24190 which adds:
  battery:precharge-current-microamp -> info.precharge_current_ua ->
POWER_SUPPLY_PROP_PRECHARGE_CURRENT
  battery:endcharge-current-microamp -> info.endcharge_current_ua ->
POWER_SUPPLY_PROP_ENDCHARGE_CURRENT

Hope that helps!

[PATCH v5 2/8] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

From: Matt Ranostay <matt@ranostay.consulting>

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring <r...@kernel.org>
Cc: Mark Rutland <mark.rutl...@arm.com>
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <ker...@networkimprov.net>
Acked-by: Sebastian Reichel <s...@kernel.org>
Acked-by: Rob Herring <r...@kernel.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3

[PATCH v5 2/8] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay 

From: Matt Ranostay 

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring 
Cc: Mark Rutland 
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay 
Signed-off-by: Liam Breck 
Acked-by: Sebastian Reichel 
Acked-by: Rob Herring 
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt 
b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 
 -microamp  : micro amps
+-microamp-hours : micro amp-hours
 -ohms  : Ohms
 -micro-ohms: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt : micro volts
 
 Temperature
-- 
2.9.3