Re: [PATCH 4/9] dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx

[Dmitry Osipenko]

09.05.2020 04:14, Sebastian Reichel ÐÐÑÐÑ:
> Hi,
> 
> On Wed, Apr 15, 2020 at 06:30:02PM +0300, Dmitry Osipenko wrote:
>> 15.04.2020 17:27, Rob Herring ÐÐÑÐÑ:
>>> On Fri, Apr 10, 2020 at 2:02 PM Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>>>>
>>>> 10.04.2020 19:49, Rob Herring ÐÐÑÐÑ:
>>>> ...
>>>>>> +  summit,max-chg-curr:
>>>>>> +    description: Maximum current for charging (in uA)
>>>>>> +    allOf:
>>>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +
>>>>>> +  summit,max-chg-volt:
>>>>>> +    description: Maximum voltage for charging (in uV)
>>>>>> +    allOf:
>>>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +    minimum: 3500000
>>>>>> +    maximum: 4500000
>>>>>> +
>>>>>> +  summit,pre-chg-curr:
>>>>>> +    description: Pre-charging current for charging (in uA)
>>>>>> +    allOf:
>>>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +
>>>>>> +  summit,term-curr:
>>>>>> +    description: Charging cycle termination current (in uA)
>>>>>> +    allOf:
>>>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>> ...
>>>>> These are all properties of the battery attached and we have standard
>>>>> properties for some/all of these.
>>>>
>>>> Looks like only four properties seem to be matching the properties of
>>>> the battery.txt binding.
>>>>
>>>> Are you suggesting that these matching properties should be renamed
>>>> after the properties in battery.txt?
>>>
>>> Yes, and that there should be a battery node.
>>
>> Usually, it's a battery that has a phandle to the power-supply. Isn't it?
> 
> There are two things: The infrastructure described by 
> Documentation/devicetree/bindings/power/supply/power-supply.yaml is
> used for telling the operating system, that a battery is charged
> by some charger. This is done by adding a power-supplies = <&phandle>
> in the battery fuel gauge node referencing the charger and probably
> what you mean here.
> 
> Then we have the infrastructure described by 
> Documentation/devicetree/bindings/power/supply/battery.txt, which
> provides data about the battery cell. In an ideal world we would
> have only smart batteries providing this data, but we don't live
> in such a world. So what we currently have is a binding looking
> like this:
> 
> bat: dumb-battery {
>     compatible = "simple-battery";
> 
>     // data about battery cell(s)
> };
> 
> fuel-gauge {
>     // fuel-gauge specific data
> 
>     supplies = <&charger>;
>     monitored-battery = <&bat>;
> };
> 
> charger: charger {
>     // charger specific data
> 
>     monitored-battery = <&bat>;
> };
> 
> In an ideal world, charger should possibly reference fuel-gauge
> node, which could provide combined data. Right now we do not have
> the infrastructure for that, so it needs to directly reference
> the simple-battery node.
> 
>>> Possibly you should add
>>> new properties battery.txt. It's curious that different properties are
>>> needed.
>>
>> I guess it should be possible to make all these properties generic.
>>
>> Sebastian, will you be okay if we will add all the required properties
>> to the power_supply_core?
> 
> Extending battery.txt is possible when something is missing. As Rob
> mentioned quite a few are already described, though:
> 
> summit,max-chg-curr => constant-charge-current-max-microamp
> summit,max-chg-volt => constant-charge-voltage-max-microvolt
> summit,pre-chg-curr => precharge-current-microamp
> summit,term-curr => charge-term-current-microamp
> 
> I think at least the battery temperature limits are something, that
> should be added to the generic code.
> 
>>> Ultimately, for a given battery technology I would expect
>>> there's a fixed set of properties needed to describe how to charge
>>> them.
>>
>> Please notice that the charger doesn't "only charge" the battery,
>> usually it also supplies power to the whole device.
>>
>> For example, when battery is fully-charged and charger is connected to
>> the power source (USB or mains), then battery may not draw any current
>> at all.
> 
> It is also a question of how good the charging process should be.
> Technically I can charge a single cell Li-ion battery without
> knowing much, but it can reduce battery life and/or be very slow.
> It might even be dangerous, if charging is done at high
> temperatures. Also some of the properties in the battery binding
> are not about charging, but about gauging. Some devices basically
> have only options to measure voltage and voltage drop over a
> resistor and everything else must be done by the operating system.
> 
>>> Perhaps some of these properties can just be derived from other
>>> properties and folks are just picking what a specific charger wants.
>>
>> Could be so, but I don't know for sure.
> 
> I don't think we have things, that can be derived with a reasonable
> amount of effort in the existing simple-battery binding, except for
> energy-full-design-microwatt-hours & charge-full-design-microamp-hours.
> 
>> Even if some properties could be derived from the others, it won't hurt
>> if we will specify everything explicitly in the device-tree.
>>
>>> Unfortunately, we have just a mess of stuff made up for each charger
>>> out there. I don't have the time nor the experience in this area to do
>>> much more than say do better.
>>
>> I don't think it's a mess in the kernel. For example, it's common that
>> embedded controllers are exposed to the system as "just a battery",
>> while in fact it's a combined charger + battery controller and the
>> charger parameters just couldn't be changed by SW.
> 
> A good EC driver exposes a charger and a battery device, so that
> userspace can easily identify if a charger is connected.
> 
>> In a case of the Nexus 7 devices, the battery controller and charger
>> controller are two separate entities in the system. The battery
>> controller (bq27541) only monitors status of the battery (charge level,
>> temperature and etc).

Hello Sebastian,

Thank you very much for the comments! We'll prepare the v2.