diff mbox series

[v1,1/4] dt-bindings: power: supply: maxim,max17040: update properties

Message ID 20230308084419.11934-2-clamor95@gmail.com
State New
Headers
Series Add optional properties to MAX17040 |

Commit Message

Svyatoslav Ryhel March 8, 2023, 8:44 a.m. UTC 
  Add simple cell, status, health and temperature properties.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../bindings/power/supply/maxim,max17040.yaml | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Krzysztof Kozlowski March 8, 2023, 9:04 a.m. UTC | #1 
On 08/03/2023 09:44, Svyatoslav Ryhel wrote:
> Add simple cell, status, health and temperature properties.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  .../bindings/power/supply/maxim,max17040.yaml | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> index 3a529326ecbd..6f1c25b4729f 100644
> --- a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> @@ -55,6 +55,20 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  monitored-battery:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to the battery node being monitored
> +
> +  power-supplies: true

This should be rather specific input name, e.g. vdd-supply.

> +
> +  io-channels:
> +    items:
> +      - description: battery temperature



max17040 does not have ADC temperature input... so is it system
configuration?


> +
> +  io-channel-names:
> +    items:
> +      - const: temp

Drop the names property, not needed for one item.

> +
>    wakeup-source:
>      type: boolean
>      description: |
> @@ -95,3 +109,26 @@ examples:
>          wakeup-source;
>        };
>      };
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c0 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      fuel-gauge@36 {
> +        compatible = "maxim,max17043";
> +        reg = <0x36>;
> +
> +        interrupt-parent = <&gpio>;
> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
> +
> +        monitored-battery = <&battery>;
> +        power-supplies = <&charger>;

But here you suggests something else than VDD... The hardware does not
take charger as input. It takes power supply - vdd.

> +
> +        io-channels = <&adc 8>;

Just add these to existing example.

> +        io-channel-names = "temp";
> +
> +        maxim,alert-low-soc-level = <10>;
> +        wakeup-source;
> +      };
> +    };

Best regards,
Krzysztof
Svyatoslav Ryhel March 8, 2023, 9:15 a.m. UTC | #2 
ср, 8 бер. 2023 р. о 11:04 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 09:44, Svyatoslav Ryhel wrote:
> > Add simple cell, status, health and temperature properties.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  .../bindings/power/supply/maxim,max17040.yaml | 37 +++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> > index 3a529326ecbd..6f1c25b4729f 100644
> > --- a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> > +++ b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> > @@ -55,6 +55,20 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >
> > +  monitored-battery:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: phandle to the battery node being monitored
> > +
> > +  power-supplies: true
>
> This should be rather specific input name, e.g. vdd-supply.
>

it is not vdd it is actual charger device

> > +
> > +  io-channels:
> > +    items:
> > +      - description: battery temperature
>
>
>
> max17040 does not have ADC temperature input... so is it system
> configuration?
>

yes, I own a device (LG Optimus Vu P895) which uses max17043
coupled with ADC thermal sensor

> > +
> > +  io-channel-names:
> > +    items:
> > +      - const: temp
>
> Drop the names property, not needed for one item.
>

Alright, but driver patch expects temp name. I will look if this
is adjustable.

> > +
> >    wakeup-source:
> >      type: boolean
> >      description: |
> > @@ -95,3 +109,26 @@ examples:
> >          wakeup-source;
> >        };
> >      };
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c0 {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      fuel-gauge@36 {
> > +        compatible = "maxim,max17043";
> > +        reg = <0x36>;
> > +
> > +        interrupt-parent = <&gpio>;
> > +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
> > +
> > +        monitored-battery = <&battery>;
> > +        power-supplies = <&charger>;
>
> But here you suggests something else than VDD... The hardware does not
> take charger as input. It takes power supply - vdd.
>

Power system allows passing properties from other power devices.
In this case battery health and status are passed from charger.

> > +
> > +        io-channels = <&adc 8>;
>
> Just add these to existing example.
>

Not sure if it is a good idea, as you wish.

> > +        io-channel-names = "temp";
> > +
> > +        maxim,alert-low-soc-level = <10>;
> > +        wakeup-source;
> > +      };
> > +    };
>
> Best regards,
> Krzysztof
>

Best regards,
Svyatoslav R.
Krzysztof Kozlowski March 8, 2023, 10:44 a.m. UTC | #3 
On 08/03/2023 10:15, Svyatoslav Ryhel wrote:

>> max17040 does not have ADC temperature input... so is it system
>> configuration?
>>
> 
> yes, I own a device (LG Optimus Vu P895) which uses max17043
> coupled with ADC thermal sensor
> 
>>> +
>>> +  io-channel-names:
>>> +    items:
>>> +      - const: temp
>>
>> Drop the names property, not needed for one item.
>>
> 
> Alright, but driver patch expects temp name. I will look if this
> is adjustable.

I think I saw cases without names.

> 
>>> +
>>>    wakeup-source:
>>>      type: boolean
>>>      description: |
>>> @@ -95,3 +109,26 @@ examples:
>>>          wakeup-source;
>>>        };
>>>      };
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>> +    i2c0 {
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +
>>> +      fuel-gauge@36 {
>>> +        compatible = "maxim,max17043";
>>> +        reg = <0x36>;
>>> +
>>> +        interrupt-parent = <&gpio>;
>>> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
>>> +
>>> +        monitored-battery = <&battery>;
>>> +        power-supplies = <&charger>;
>>
>> But here you suggests something else than VDD... The hardware does not
>> take charger as input. It takes power supply - vdd.
>>
> 
> Power system allows passing properties from other power devices.
> In this case battery health and status are passed from charger.

So this is not an input to device? Then it does not really look like
property of this hardware. Fuel gauge does not control the charger, also
from system configuration point of view.

Best regards,
Krzysztof
Svyatoslav Ryhel March 8, 2023, 10:51 a.m. UTC | #4 
ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 10:15, Svyatoslav Ryhel wrote:
>
> >> max17040 does not have ADC temperature input... so is it system
> >> configuration?
> >>
> >
> > yes, I own a device (LG Optimus Vu P895) which uses max17043
> > coupled with ADC thermal sensor
> >
> >>> +
> >>> +  io-channel-names:
> >>> +    items:
> >>> +      - const: temp
> >>
> >> Drop the names property, not needed for one item.
> >>
> >
> > Alright, but driver patch expects temp name. I will look if this
> > is adjustable.
>
> I think I saw cases without names.
>

There is no io-channel without a name. And io-channels are mostly used
by power supply devices.

> >
> >>> +
> >>>    wakeup-source:
> >>>      type: boolean
> >>>      description: |
> >>> @@ -95,3 +109,26 @@ examples:
> >>>          wakeup-source;
> >>>        };
> >>>      };
> >>> +  - |
> >>> +    #include <dt-bindings/interrupt-controller/irq.h>
> >>> +    i2c0 {
> >>> +      #address-cells = <1>;
> >>> +      #size-cells = <0>;
> >>> +
> >>> +      fuel-gauge@36 {
> >>> +        compatible = "maxim,max17043";
> >>> +        reg = <0x36>;
> >>> +
> >>> +        interrupt-parent = <&gpio>;
> >>> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
> >>> +
> >>> +        monitored-battery = <&battery>;
> >>> +        power-supplies = <&charger>;
> >>
> >> But here you suggests something else than VDD... The hardware does not
> >> take charger as input. It takes power supply - vdd.
> >>
> >
> > Power system allows passing properties from other power devices.
> > In this case battery health and status are passed from charger.
>
> So this is not an input to device? Then it does not really look like
> property of this hardware. Fuel gauge does not control the charger, also
> from system configuration point of view.
>

It is not controlling charger, the charger provides the status and
health of the battery to the fuel gauge. This option is also used in
other fuel gauges.

> Best regards,
> Krzysztof
>
Krzysztof Kozlowski March 8, 2023, 10:53 a.m. UTC | #5 
On 08/03/2023 11:51, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
>>
>> On 08/03/2023 10:15, Svyatoslav Ryhel wrote:
>>
>>>> max17040 does not have ADC temperature input... so is it system
>>>> configuration?
>>>>
>>>
>>> yes, I own a device (LG Optimus Vu P895) which uses max17043
>>> coupled with ADC thermal sensor
>>>
>>>>> +
>>>>> +  io-channel-names:
>>>>> +    items:
>>>>> +      - const: temp
>>>>
>>>> Drop the names property, not needed for one item.
>>>>
>>>
>>> Alright, but driver patch expects temp name. I will look if this
>>> is adjustable.
>>
>> I think I saw cases without names.
>>
> 
> There is no io-channel without a name. And io-channels are mostly used
> by power supply devices.
> 
>>>
>>>>> +
>>>>>    wakeup-source:
>>>>>      type: boolean
>>>>>      description: |
>>>>> @@ -95,3 +109,26 @@ examples:
>>>>>          wakeup-source;
>>>>>        };
>>>>>      };
>>>>> +  - |
>>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>>> +    i2c0 {
>>>>> +      #address-cells = <1>;
>>>>> +      #size-cells = <0>;
>>>>> +
>>>>> +      fuel-gauge@36 {
>>>>> +        compatible = "maxim,max17043";
>>>>> +        reg = <0x36>;
>>>>> +
>>>>> +        interrupt-parent = <&gpio>;
>>>>> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
>>>>> +
>>>>> +        monitored-battery = <&battery>;
>>>>> +        power-supplies = <&charger>;
>>>>
>>>> But here you suggests something else than VDD... The hardware does not
>>>> take charger as input. It takes power supply - vdd.
>>>>
>>>
>>> Power system allows passing properties from other power devices.
>>> In this case battery health and status are passed from charger.
>>
>> So this is not an input to device? Then it does not really look like
>> property of this hardware. Fuel gauge does not control the charger, also
>> from system configuration point of view.
>>
> 
> It is not controlling charger, the charger provides the status and
> health of the battery to the fuel gauge. This option is also used in
> other fuel gauges.

How regulator provides health and status of the battery? I don't understand.

Best regards,
Krzysztof
Svyatoslav Ryhel March 8, 2023, 11:06 a.m. UTC | #6 
ср, 8 бер. 2023 р. о 12:53 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 11:51, Svyatoslav Ryhel wrote:
> > ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> пише:
> >>
> >> On 08/03/2023 10:15, Svyatoslav Ryhel wrote:
> >>
> >>>> max17040 does not have ADC temperature input... so is it system
> >>>> configuration?
> >>>>
> >>>
> >>> yes, I own a device (LG Optimus Vu P895) which uses max17043
> >>> coupled with ADC thermal sensor
> >>>
> >>>>> +
> >>>>> +  io-channel-names:
> >>>>> +    items:
> >>>>> +      - const: temp
> >>>>
> >>>> Drop the names property, not needed for one item.
> >>>>
> >>>
> >>> Alright, but driver patch expects temp name. I will look if this
> >>> is adjustable.
> >>
> >> I think I saw cases without names.
> >>
> >
> > There is no io-channel without a name. And io-channels are mostly used
> > by power supply devices.
> >
> >>>
> >>>>> +
> >>>>>    wakeup-source:
> >>>>>      type: boolean
> >>>>>      description: |
> >>>>> @@ -95,3 +109,26 @@ examples:
> >>>>>          wakeup-source;
> >>>>>        };
> >>>>>      };
> >>>>> +  - |
> >>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
> >>>>> +    i2c0 {
> >>>>> +      #address-cells = <1>;
> >>>>> +      #size-cells = <0>;
> >>>>> +
> >>>>> +      fuel-gauge@36 {
> >>>>> +        compatible = "maxim,max17043";
> >>>>> +        reg = <0x36>;
> >>>>> +
> >>>>> +        interrupt-parent = <&gpio>;
> >>>>> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
> >>>>> +
> >>>>> +        monitored-battery = <&battery>;
> >>>>> +        power-supplies = <&charger>;
> >>>>
> >>>> But here you suggests something else than VDD... The hardware does not
> >>>> take charger as input. It takes power supply - vdd.
> >>>>
> >>>
> >>> Power system allows passing properties from other power devices.
> >>> In this case battery health and status are passed from charger.
> >>
> >> So this is not an input to device? Then it does not really look like
> >> property of this hardware. Fuel gauge does not control the charger, also
> >> from system configuration point of view.
> >>
> >
> > It is not controlling charger, the charger provides the status and
> > health of the battery to the fuel gauge. This option is also used in
> > other fuel gauges.
>
> How regulator provides health and status of the battery? I don't understand.
>

It is not a regulator, it is a charger! Dedicated chip responsible for
controlling charging. And its configuration allows it to get battery
health and status, because this fuel gauge does not have this
function.

> Best regards,
> Krzysztof
>
Krzysztof Kozlowski March 8, 2023, 11:12 a.m. UTC | #7 
On 08/03/2023 12:06, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:53 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
>>
>> On 08/03/2023 11:51, Svyatoslav Ryhel wrote:
>>> ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> пише:
>>>>
>>>> On 08/03/2023 10:15, Svyatoslav Ryhel wrote:
>>>>
>>>>>> max17040 does not have ADC temperature input... so is it system
>>>>>> configuration?
>>>>>>
>>>>>
>>>>> yes, I own a device (LG Optimus Vu P895) which uses max17043
>>>>> coupled with ADC thermal sensor
>>>>>
>>>>>>> +
>>>>>>> +  io-channel-names:
>>>>>>> +    items:
>>>>>>> +      - const: temp
>>>>>>
>>>>>> Drop the names property, not needed for one item.
>>>>>>
>>>>>
>>>>> Alright, but driver patch expects temp name. I will look if this
>>>>> is adjustable.
>>>>
>>>> I think I saw cases without names.
>>>>
>>>
>>> There is no io-channel without a name. And io-channels are mostly used
>>> by power supply devices.
>>>
>>>>>
>>>>>>> +
>>>>>>>    wakeup-source:
>>>>>>>      type: boolean
>>>>>>>      description: |
>>>>>>> @@ -95,3 +109,26 @@ examples:
>>>>>>>          wakeup-source;
>>>>>>>        };
>>>>>>>      };
>>>>>>> +  - |
>>>>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>>>>> +    i2c0 {
>>>>>>> +      #address-cells = <1>;
>>>>>>> +      #size-cells = <0>;
>>>>>>> +
>>>>>>> +      fuel-gauge@36 {
>>>>>>> +        compatible = "maxim,max17043";
>>>>>>> +        reg = <0x36>;
>>>>>>> +
>>>>>>> +        interrupt-parent = <&gpio>;
>>>>>>> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
>>>>>>> +
>>>>>>> +        monitored-battery = <&battery>;
>>>>>>> +        power-supplies = <&charger>;
>>>>>>
>>>>>> But here you suggests something else than VDD... The hardware does not
>>>>>> take charger as input. It takes power supply - vdd.
>>>>>>
>>>>>
>>>>> Power system allows passing properties from other power devices.
>>>>> In this case battery health and status are passed from charger.
>>>>
>>>> So this is not an input to device? Then it does not really look like
>>>> property of this hardware. Fuel gauge does not control the charger, also
>>>> from system configuration point of view.
>>>>
>>>
>>> It is not controlling charger, the charger provides the status and
>>> health of the battery to the fuel gauge. This option is also used in
>>> other fuel gauges.
>>
>> How regulator provides health and status of the battery? I don't understand.
>>
> 
> It is not a regulator, it is a charger! Dedicated chip responsible for
> controlling charging. And its configuration allows it to get battery
> health and status, because this fuel gauge does not have this
> function.


Ah, you are right. I confused with power-supply. It is fine.


Best regards,
Krzysztof
Sebastian Reichel May 15, 2023, 10:17 p.m. UTC | #8 
Hi,

On Wed, Mar 08, 2023 at 10:44:16AM +0200, Svyatoslav Ryhel wrote:
> Add simple cell, status, health and temperature properties.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  .../bindings/power/supply/maxim,max17040.yaml | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> index 3a529326ecbd..6f1c25b4729f 100644
> --- a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
> @@ -55,6 +55,20 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  monitored-battery:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to the battery node being monitored
> +
> +  power-supplies: true

The above two should not be needed, since the binding inherits them:

```
allOf:
  - $ref: power-supply.yaml#

unevaluatedProperties: false
```

Otherwise LGTM.

-- Sebastian

> +
> +  io-channels:
> +    items:
> +      - description: battery temperature
> +
> +  io-channel-names:
> +    items:
> +      - const: temp
> +
>    wakeup-source:
>      type: boolean
>      description: |
> @@ -95,3 +109,26 @@ examples:
>          wakeup-source;
>        };
>      };
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c0 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      fuel-gauge@36 {
> +        compatible = "maxim,max17043";
> +        reg = <0x36>;
> +
> +        interrupt-parent = <&gpio>;
> +        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
> +
> +        monitored-battery = <&battery>;
> +        power-supplies = <&charger>;
> +
> +        io-channels = <&adc 8>;
> +        io-channel-names = "temp";
> +
> +        maxim,alert-low-soc-level = <10>;
> +        wakeup-source;
> +      };
> +    };
> -- 
> 2.37.2
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
index 3a529326ecbd..6f1c25b4729f 100644
--- a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
+++ b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml
@@ -55,6 +55,20 @@  properties:
   interrupts:
     maxItems: 1
 
+  monitored-battery:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to the battery node being monitored
+
+  power-supplies: true
+
+  io-channels:
+    items:
+      - description: battery temperature
+
+  io-channel-names:
+    items:
+      - const: temp
+
   wakeup-source:
     type: boolean
     description: |
@@ -95,3 +109,26 @@  examples:
         wakeup-source;
       };
     };
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      fuel-gauge@36 {
+        compatible = "maxim,max17043";
+        reg = <0x36>;
+
+        interrupt-parent = <&gpio>;
+        interrupts = <144 IRQ_TYPE_EDGE_FALLING>;
+
+        monitored-battery = <&battery>;
+        power-supplies = <&charger>;
+
+        io-channels = <&adc 8>;
+        io-channel-names = "temp";
+
+        maxim,alert-low-soc-level = <10>;
+        wakeup-source;
+      };
+    };