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

[Enric Balletbo Serra]

2017-06-01 12:25 GMT+02:00 Liam Breck <liam@xxxxxxxxxxxxxxxxx>:
> Hi Enric,
>
>
>
> On Thu, Jun 1, 2017 at 1:18 AM, Enric Balletbo Serra
> <eballetbo@xxxxxxxxx> wrote:
>> Hi Liam,
>>
>> 2017-06-01 9:01 GMT+02:00 Liam Breck <liam@xxxxxxxxxxxxxxxxx>:
>>> Hi Enric,
>>>
>>> On Sat, May 27, 2017 at 1:11 PM, Enric Balletbo Serra
>>> <eballetbo@xxxxxxxxx> wrote:
>>>> Hi Liam,
>>>>
>>>> 2017-05-26 23:20 GMT+02:00 Liam Breck <liam@xxxxxxxxxxxxxxxxx>:
>>>>> Hi Enric,
>>>>>
>>>>> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
>>>>> <enric.balletbo@xxxxxxxxxxxxx> 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.balletbo@xxxxxxxxxxxxx>
>>>>>> ---
>>>>>> 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/
>

Oh I missed that thread, please keep me in the loop for next versions
of the patchset :)

Best regards,
 Enric

>
>> 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 = <3200000>;
>>>>>>                 energy-full-design-microwatt-hours = <5290000>;
>>>>>>                 charge-full-design-microamp-hours = <1430000>;
>>>>>> +               charge-voltage-microvolt = <4100000>;
>>>>>> +               charge-current-microamp = <300000>;
>>>>>>         };
>>>>>>
>>>>>>         charger: charger@11 {
>>>>>> --
>>>>>> 2.9.3
>>>>>>