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.
>
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.
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.
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).
> 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.
> 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
--
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
