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

[Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

Hi Liam,

On 29/03/2017 08:55, Liam Breck wrote:
> Hi Quentin,
> 
> On Thu, Mar 16, 2017 at 12:42 AM, Liam Breck <liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org> wrote:
>> On Thu, Mar 16, 2017 at 12:03 AM, Quentin Schulz
>> <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>> Hi Liam,
>>>
>>> On 16/03/2017 07:27, Liam Breck wrote:
>>>> Hi Rob,
>>>>
>>>> On Wed, Mar 15, 2017 at 6:10 AM, Quentin Schulz
>>>> <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>> Hi Liam,
>>>>>
>>>>> On 15/03/2017 13:08, Liam Breck wrote:
>>>>>> I dropped most of the CCs, pls re-add anyone essential. Use Cc lines
>>>>>> in patch description to direct a patch to interested parties and
>>>>>> relevant lists. I don't want to see all 19 patches in this series.
>>>>>>
>>>>>> On Wed, Mar 15, 2017 at 3:55 AM, Quentin Schulz
>>>>>> <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>>>>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>>>>> ---
>>>>>>>
>>>>>>> v4:
>>>>>>>  - switch from constant-charge-current-microamp to constant-charge-microamp,
>>>>
>>>> Sebastian is on record supporting name alignment between DT:battery
>>>> properties and enum power_supply_property.
>>>>
>>>> Could you OK a change back to constant-charge-current-microamp in this patch?
>>>>
>>>> Note that the following could appear in DT:battery in future:
>>>>
>>>> constant-charge-current-microamp
>>>> constant-charge-current-max-microamp
>>>
>>> OK. I am actually setting max constant charge current and constant
>>> current charge with the same DT property so I might need to adapt my
>>> patches.
>>
>> I suspect you should set battery:*-current-max with DT, as -current is
>> "programmed by charger" so seems to be a stat, not a fixed
>> characteristic.
> 
> Did you resolve this issue?
> 

Sorry, completely missed your first mail.

Constant charge current is actually what I set in the PMIC. The maximum
constant charge current is completely PMIC-agnostic in my driver as it
is used to tell the user that (s)he shouldn't really have a constant
charge current over the maximum limit as the battery might not be able
to handle such high current.

What I suggest is to have both properties in DT. I'll set the maximum as
I do today and we can give the user the ability to set a "default"
constant charge current (below maximum constant charge current) in the
DT. Maybe we do not want to recharge your battery with the maximum value
by default?

> Rob & Sebastian want me to merge all items for
> bindings/power/supply/battery.txt. If I do that I'll also roll up
> their counterparts in power_supply_core.c and submit a patchset for
> those two files.
> 

I guess that adding the two properties make sense, don't you think?

Also, could you Cc me on your next version of your patches? So I know
when to rebase and slightly rework my patches once your patch series is
merged.

Thanks,
Quentin

>> You may program your charger's -current with the battery's
>> -current-max, of course.
>>
>> Note that your charger also has a -current-max characteristic, likely
>> different than your battery. You may not need that in DT tho.
>>
>>>> constant-charge-voltage-microvolt
>>>> constant-charge-voltage-max-microvolt
>>>>
>>>> Thanks!
>>>>
>>>>>> Must be constant-charge-current-microamp for the reasons discussed in
>>>>>> battery.txt - consistency with sysfs names.
>>>>>>
>>>>>
>>>>> Hum. Just nitpicking, but I disagree with the use of 'must'. IIRC there
>>>>> is nothing in the code that would require the property to be named after
>>>>> a property from the enum power_supply_property.
>>>>>
>>>>> I would say that you _want_ it to be named like that because it
>>>>> underlines the relation between the DT property and the actual impacted
>>>>> property in the power supply subsystem. I'm fine with this reason but in
>>>>> the end, the maintainer's opinion prevails (if (s)he does not want it,
>>>>> (s)he will not take it). So, basically, I actually don't mind either
>>>>> option and I see arguments on each side.
>>>>>
>>>>> So, just waiting for maintainer's opinion to make the final version of
>>>>> this patch.
>>>>>
>>>>> Thanks,
>>>>> Quentin
>>>>>
>>>>>> Also applies to power_supply_core.c patch.
>>>>>>
>>>>>> Rob: enum power_supply_property also lists constant-charge-voltage,
>>>>>> hence the -current
>>>>>>
>>>>>>
>>>>>>> added in v3
>>>>>>>
>>>>>>>  Documentation/devicetree/bindings/power/supply/battery.txt | 2 ++
>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>>> index 0278617..9594e1e 100644
>>>>>>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>>> @@ -7,6 +7,7 @@ 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
>>>>>>> + - constant-charge-microamp: battery constant charge current
>>>>>>>
>>>>>>>  Because drivers surface properties in sysfs using names derived
>>>>>>>  from enum power_supply_property, e.g.
>>>>>>> @@ -30,6 +31,7 @@ Example:
>>>>>>>                 voltage-min-design-microvolt = <3200000>;
>>>>>>>                 energy-full-design-microwatt-hours = <5290000>;
>>>>>>>                 charge-full-design-microamp-hours = <1430000>;
>>>>>>> +               constant-charge-microamp = <300000>;
>>>>>>>         };
>>>>>>>
>>>>>>>         charger: charger@11 {
>>>>>>> --
>>>>>>> 2.9.3
>>>>>>>
>>>>>
>>>>> --
>>>>> Quentin Schulz, Free Electrons
>>>>> Embedded Linux and Kernel engineering
>>>>> http://free-electrons.com
>>>
>>> --
>>> Quentin Schulz, Free Electrons
>>> Embedded Linux and Kernel engineering
>>> http://free-electrons.com

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html