On Wed, Mar 29, 2017 at 12:09 AM, Quentin Schulz
<quentin.schulz@xxxxxxxxxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxxxxx> wrote:
>>> On Thu, Mar 16, 2017 at 12:03 AM, Quentin Schulz
>>> <quentin.schulz@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>>>>>>>> Acked-by: Rob Herring <robh@xxxxxxxxxx>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> 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?
The scope here is just static battery characteristics. For the battery
node, I think constant-charge-current-max-microamp is the relevant
term. If we added both properties, one would be an alias for the
other, which doesn't seem useful.
The pmic can report both independently of the battery characteristic,
and take a driver-specific DT property for
constant-charge-current-microamp which it applies if less than the
battery's -max setting. Or you could set that in userspace via
/sys/class.../constant_charge_current. The charger's -max value would
be a characteristic of its regulator.
Or am I missing something...?
>> 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.
Surely.
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
