Hi,
On 29/03/2017 11:26, Liam Breck wrote:
> On Wed, Mar 29, 2017 at 12:54 AM, Quentin Schulz
> <quentin.schulz@xxxxxxxxxxxxxxxxxx> wrote:
>> Hi,
>>
>> On 29/03/2017 09:39, Liam Breck wrote:
>>> 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 it's only for static battery characteristics, I agree.
>>
>>
>>> If we added both properties, one would be an alias for the
>>> other, which doesn't seem useful.
>
> Meaning if both properties describe static characteristics of the
> battery, they wouldn't have different meaning.
>
>> Hum. Not really an alias. In my driver, I have a maximum which avoids to
>> set a constant charge current higher than what the battery is supposed
>> to be able to handle. The maximum can be increased via sysfs if ever the
>> user decides to go wild (or if (s)he changes the battery for example).
>> The maximum is never input in any of PMIC registers, it is not dealing
>> with the PMIC at all.
>
> Using the battery's -max as the default for your user-adjustable max
> sounds sensible. However the user-adjustable max should perhaps appear
> in a custom sysfs attribute, not
> /sys/class.../constant_charge_current_max (see comment re regulator).
>
Why?
> If the battery changes, the DT battery node should also change, esp if
> its -max drops. I realize that's not nec trivial.
>
Disagree. That's definitely not user friendly.
>> If I have only the max constant charge current in the DT, I'll set the
>> max and constant charge current with the same DT entry. That's perfectly
>> fine.
>
> I assume the pmic ccc setting is adjustable via
> /sys/class.../constant_charge_current?
>
Yes, it is then limited by a user-modifiable constant_charge_current_max.
>> However, what I was thinking is to have a "default" value taken from DT
>> for constant charge current (which will be below maximum ccc) if ever
>> someone wants to set a maximum but use a lower ccc value. Anyway, I
>> guess it's more a "fancy" feature than a requirement (unlike max-ccc).
>
> Maybe a lower default ccc than battery's -max could be provided via a
> % value in the pmic DT node? Then multiply battery -max by that to set
> ccc.
>
That's battery-specific, why would you want to put it in the PMIC node?
>>> 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.
>>
>> Indeed.
>>
>>> Or you could set that in userspace via
>>> /sys/class.../constant_charge_current.
>>
>> The goal is to have both. The DT for default values (given by the
>> vendor/upstream) and sysfs for custom values (interesting if you don't
>> want to/can't recompile a DT.
>>
>>> The charger's -max value would
>>> be a characteristic of its regulator.
>>>
>>
>> I don't really understand what you mean by that.
>
> I believe the pmic's -max value is a characteristic of its electrical
> design, the max current it can deliver. The power_supply_class docs
> are not very clear - "maximum charge current supported by the power
> supply object".
>
This PMIC can deliver a maximum of 1.8A or 2.5A depending on the variant
for constant charge current but you do not want to easily allow your
user to put such a high value as the battery is most likely not able to
handle such a high current.
>
>>> 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
>>
>> --
>> 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
