[v1,1/7] devicetree: power: battery: Add properties for pre-charge and end-charge current

From: Liam Breck <kernel@networkimprov.net>

precharge-current-microamp and endcharge-current-microamp are used
by battery chargers at the beginning and end of a charging cycle.

Depends-on: https://patchwork.kernel.org/patch/9633605/
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
 1 file changed, 4 insertions(+)

Hi,

On 21-03-17 23:09, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> precharge-current-microamp and endcharge-current-microamp are used
> by battery chargers at the beginning and end of a charging cycle.
>
> Depends-on: https://patchwork.kernel.org/patch/9633605/
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  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 53a68c0..494374a 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
> + - precharge-current-microamp: current for pre-charge phase
> + - endcharge-current-microamp: current for charge termination phase
>
>  Battery properties are named, where possible, for the corresponding
>  elements in enum power_supply_property, defined in
> @@ -28,6 +30,8 @@ Example:
>  		voltage-min-design-microvolt = <3200000>;
>  		energy-full-design-microwatt-hours = <5290000>;
>  		charge-full-design-microamp-hours = <1430000>;
> +		precharge-current-microamp = <256000>;
> +		endcharge-current-microamp = <128000>;
>  	};
>
>  	charger: charger@11 {
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi,

On Tue, Mar 21, 2017 at 03:09:15PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> precharge-current-microamp and endcharge-current-microamp are used
> by battery chargers at the beginning and end of a charging cycle.
> 
> Depends-on: https://patchwork.kernel.org/patch/9633605/
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

Acked-by: Sebastian Reichel <sre@kernel.org>

I think it makes sense to merge this into the patch adding
simple-battery.

>  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 53a68c0..494374a 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
> + - precharge-current-microamp: current for pre-charge phase
> + - endcharge-current-microamp: current for charge termination phase
>  
>  Battery properties are named, where possible, for the corresponding 
>  elements in enum power_supply_property, defined in
> @@ -28,6 +30,8 @@ Example:
>  		voltage-min-design-microvolt = <3200000>;
>  		energy-full-design-microwatt-hours = <5290000>;
>  		charge-full-design-microamp-hours = <1430000>;
> +		precharge-current-microamp = <256000>;
> +		endcharge-current-microamp = <128000>;
>  	};
>  
>  	charger: charger@11 {
> -- 
> 2.9.3
>

On Fri, Mar 24, 2017 at 2:01 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Tue, Mar 21, 2017 at 03:09:15PM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> precharge-current-microamp and endcharge-current-microamp are used
>> by battery chargers at the beginning and end of a charging cycle.
>>
>> Depends-on: https://patchwork.kernel.org/patch/9633605/
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>
> Acked-by: Sebastian Reichel <sre@kernel.org>
>
> I think it makes sense to merge this into the patch adding
> simple-battery.

It would make sense, but it means a new _prop_precharge/endcharge
patch in that patchset, and I am trying to constrain it at this stage.

So if it's OK, I'd like to keep all that in this patchset.

>>  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 53a68c0..494374a 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
>> + - precharge-current-microamp: current for pre-charge phase
>> + - endcharge-current-microamp: current for charge termination phase
>>
>>  Battery properties are named, where possible, for the corresponding
>>  elements in enum power_supply_property, defined in
>> @@ -28,6 +30,8 @@ Example:
>>               voltage-min-design-microvolt = <3200000>;
>>               energy-full-design-microwatt-hours = <5290000>;
>>               charge-full-design-microamp-hours = <1430000>;
>> +             precharge-current-microamp = <256000>;
>> +             endcharge-current-microamp = <128000>;
>>       };
>>
>>       charger: charger@11 {
>> --
>> 2.9.3
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Mar 21, 2017 at 03:09:15PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> precharge-current-microamp and endcharge-current-microamp are used
> by battery chargers at the beginning and end of a charging cycle.
> 
> Depends-on: https://patchwork.kernel.org/patch/9633605/
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  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 53a68c0..494374a 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
> + - precharge-current-microamp: current for pre-charge phase
> + - endcharge-current-microamp: current for charge termination phase

current is implied by microamp, so perhaps just pre-charge-microamp and 
end-charge-microamp.

I know little about batteries, but don't you also need to know when each 
phase starts/ends? 

I mainly ask because we just added the previous properties and now we're 
adding 2 more. While fine to add features to a driver one by one, we 
really shouldn't for bindings. The h/w is not evolving (in a month).

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Mar 24, 2017 at 05:34:26PM -0700, Liam Breck wrote:
> On Fri, Mar 24, 2017 at 2:01 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi,
> >
> > On Tue, Mar 21, 2017 at 03:09:15PM -0700, Liam Breck wrote:
> >> From: Liam Breck <kernel@networkimprov.net>
> >>
> >> precharge-current-microamp and endcharge-current-microamp are used
> >> by battery chargers at the beginning and end of a charging cycle.
> >>
> >> Depends-on: https://patchwork.kernel.org/patch/9633605/
> >> Cc: Rob Herring <robh@kernel.org>
> >> Cc: devicetree@vger.kernel.org
> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> >
> > Acked-by: Sebastian Reichel <sre@kernel.org>
> >
> > I think it makes sense to merge this into the patch adding
> > simple-battery.

Agreed.

> 
> It would make sense, but it means a new _prop_precharge/endcharge
> patch in that patchset, and I am trying to constrain it at this stage.

Please make bindings complete as possible. You don't have to have the 
driver side.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Rob,

On Tue, Mar 28, 2017 at 5:39 PM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Mar 21, 2017 at 03:09:15PM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> precharge-current-microamp and endcharge-current-microamp are used
>> by battery chargers at the beginning and end of a charging cycle.
>>
>> Depends-on: https://patchwork.kernel.org/patch/9633605/
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> ---
>>  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 53a68c0..494374a 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
>> + - precharge-current-microamp: current for pre-charge phase
>> + - endcharge-current-microamp: current for charge termination phase
>
> current is implied by microamp, so perhaps just pre-charge-microamp and
> end-charge-microamp.

Ah, this is why I want to document the naming scheme for battery node
properties in battery.txt, by referring to a linux header file :-)

The names mirror enum power_supply_property elements wherever
possible. Shortening them as you suggest makes dts code a little more
terse, but obscures their relationship with power_supply sysfs
attributes. I prefer brevity myself, but there is no strong case to
reword the names for DT use.

> I know little about batteries, but don't you also need to know when each
> phase starts/ends?

Meaning at what voltage levels? We don't need it for this
battery/charger pair; no idea about others...

> I mainly ask because we just added the previous properties and now we're
> adding 2 more. While fine to add features to a driver one by one, we
> really shouldn't for bindings. The h/w is not evolving (in a month).

And a third patchset from Quentin Schulz adds another :-)

I think you may need to accept piecemeal assembly in this case... No
one has made a study of all properties that should be in the battery
node. (precharge_current wasn't even in power_supply_property until
this patchset.) Sebastian requested we create this binding in the
process of adding DT support to a fuel gauge, so we coded what that
called for.

I hope that helps...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html