[v2,5/8] dt-bindings: power: supply: max77658: Add ADI MAX77658 Battery

Add ADI MAX77658 power supply devicetree document.

Signed-off-by: Nurettin Bolucu <Nurettin.Bolucu@analog.com>
Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
---
 .../power/supply/adi,max77658-battery.yaml    | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/adi,max77658-battery.yaml

On Wed, 22 Mar 2023, Krzysztof Kozlowski wrote:
>On 22/03/2023 06:56, Zeynep Arslanbenzer wrote:
>> Add ADI MAX77658 power supply devicetree document.
>> 
>> Signed-off-by: Nurettin Bolucu <Nurettin.Bolucu@analog.com>
>> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
>> ---
>>  .../power/supply/adi,max77658-battery.yaml    | 58 +++++++++++++++++++
>>  1 file changed, 58 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/power/supply/adi,max77658-battery.ya
>> ml
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/power/supply/adi,max77658-battery.
>> yaml 
>> b/Documentation/devicetree/bindings/power/supply/adi,max77658-battery.
>> yaml
>> new file mode 100644
>> index 000000000000..0b696f7c4d1b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/adi,max77658-batt
>> +++ ery.yaml
>> @@ -0,0 +1,58 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2
>> +---
>> +$id: 
>> +https://urldefense.com/v3/__http://devicetree.org/schemas/power/suppl
>> +y/adi,max77658-battery.yaml*__;Iw!!A3Ni8CS0y2Y!7jzMr8UalEjjYfmquE6Iqt
>> +SndU7f-c9va789cC2VmSpvstAZ-AokoftF1vX_ZdeLxGuE455k4EMaG0BdyEAEeqCT4rs
>> +zrkvmwS9F$
>> +$schema: 
>> +https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.y
>> +aml*__;Iw!!A3Ni8CS0y2Y!7jzMr8UalEjjYfmquE6IqtSndU7f-c9va789cC2VmSpvst
>> +AZ-AokoftF1vX_ZdeLxGuE455k4EMaG0BdyEAEeqCT4rszromzOD1g$
>> +
>> +title: Battery for MAX77658 PMIC from ADI.
>
>Implement all previous comments, not just some.
>
>
>> +
>> +maintainers:
>> +  - Nurettin Bolucu <Nurettin.Bolucu@analog.com>
>> +  - Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
>> +
>> +description: |
>> +  This module is part of the MAX77658 MFD device. For more details
>> +  see Documentation/devicetree/bindings/mfd/adi,max77658.yaml.
>> +
>> +  The fuel gauge is represented as a sub-node of the PMIC node on the device tree.
>> +
>> +properties:
>> +  compatible:
>> +    const:
>> +      adi,max77658-battery
>
>It's one line.
>
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  adi,valrt-min-microvolt:
>> +    description: Minimum voltage value that triggers the alarm.
>> +
>> +  adi,valrt-max-microvolt:
>> +    description: Maximum voltage value that triggers the alarm.
>
>Use the same syntax as battery.yaml
>
>> +
>> +  adi,salrt-min-percent:
>> +    description: Minimum percentage of battery that triggers the alarm.
>> +
>> +  adi,salrt-max-percent:
>> +    description: Maximum percentage of battery that triggers the alarm.
>
>That's not suitable for DT. Do not encode policies into DT.
>
>> +
>> +  adi,ialrt-min-microamp:
>> +    description: Minimum current value that triggers the alarm.
>> +
>> +  adi,ialrt-max-microamp:
>> +    description: Maximum current value that triggers the alarm.
>> +
>> +  monitored-battery:
>> +    description: >
>> +      phandle to a "simple-battery" compatible node.
>> +
>> +      This property must be a phandle to a node using the format 
>> + described
>
>You already said it above.
>
>> +      in battery.yaml, with the following properties being required:
>> +      - alert-celsius
>> +
>> +required:
>> +  - compatible
>
>Why reg and monitored-batter are not required?
>
If no monitored-battery information is supplied, we set default values for alert-celsius. The reg property is the I2C address of the device and it is part of the parent schema. Therefore, both are not required in this file.

Best regards,
Zeynep

On Tue, 2 May 2023, Krzysztof Kozlowski wrote:
>On 01/05/2023 22:20, Arslanbenzer, Zeynep wrote:
>
>>> You already said it above.
>>>
>>>> +      in battery.yaml, with the following properties being required:
>>>> +      - alert-celsius
>>>> +
>>>> +required:
>>>> +  - compatible
>>>
>>> Why reg and monitored-batter are not required?
>>>
>> If no monitored-battery information is supplied, we set default values for alert-celsius.
>
>So the device can operate without battery? Interesting... are you sure, you can physically remove battery and device will work?
>

I mean if battery info is not supplied, we use default values.

ret = power_supply_get_battery_info(fg->battery, &info);
if (ret) {
    dev_err(fg->dev, "Unable to get battery info\n");
    fg->temp_alert_min = info->temp_alert_min;
    fg->temp_alert_max = info->temp_alert_max;
} else {
    fg->temp_alert_min = -128;
    fg->temp_alert_max = 127;
}


>> The reg property is the I2C address of the device and it is part of the parent schema. Therefore, both are not required in this file.
>
>What does it mean "in parent schema"? You have reg here, so how parent schema is related to this?

It is my mistake, I will remove reg from here.

Best regards,
Zeynep

On 02/05/2023 17:05, Arslanbenzer, Zeynep wrote:
> On Tue, 2 May 2023, Krzysztof Kozlowski wrote:
>> On 01/05/2023 22:20, Arslanbenzer, Zeynep wrote:
>>
>>>> You already said it above.
>>>>
>>>>> +      in battery.yaml, with the following properties being required:
>>>>> +      - alert-celsius
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>
>>>> Why reg and monitored-batter are not required?
>>>>
>>> If no monitored-battery information is supplied, we set default values for alert-celsius.
>>
>> So the device can operate without battery? Interesting... are you sure, you can physically remove battery and device will work?
>>
> 
> I mean if battery info is not supplied, we use default values.
> 
> ret = power_supply_get_battery_info(fg->battery, &info);
> if (ret) {
>     dev_err(fg->dev, "Unable to get battery info\n");
>     fg->temp_alert_min = info->temp_alert_min;
>     fg->temp_alert_max = info->temp_alert_max;
> } else {
>     fg->temp_alert_min = -128;
>     fg->temp_alert_max = 127;
> }

You speak about driver, but I speak about hardware. The bindings are for
the latter.

Best regards,
Krzysztof