[PATCH v3 0/1] Update the max31790 driver

[PATCH v3 0/1] Update the max31790 driver

[Chanh Nguyen]

Add device tree binding for the max31790 device.

Changes in v2:
 - Drop "driver" in the patch 1/3 commit title                           [Krzysztof]
 - Update filename of the maxim,max31790.yaml                            [Krzysztof]
 - Add the common fan schema to $ref                                     [Krzysztof]
 - Update the node name to "fan-controller" in maxim,max31790.yaml       [Krzysztof]
 - Update the vendor property name to "maxim,pwmout-pin-as-tach-input"   [Rob]
Changes in v3:
 - Drop redundant "bindings" in commit message                           [Krzysztof]
 - Add the clocks and resets property to example                         [Krzysztof]
 - Add child node refer to fan-common.yaml                               [Krzysztof, Conor]
 - Drop "Support config PWM output become TACH" patch                    [Chanh]
 - Drop "Add maxim,pwmout-pin-as-tach-input property" patch              [Chanh]

Chanh Nguyen (1):
  dt-bindings: hwmon: Add maxim max31790

 .../bindings/hwmon/maxim,max31790.yaml        | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml

-- 
2.43.0

[PATCH v3 1/1] dt-bindings: hwmon: Add maxim max31790

[Chanh Nguyen]

Add device tree bindings and an example for max31790 device.

Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com>
---
Changes in v2:
 - Update filename of the maxim,max31790.yaml                        [Krzysztof]
 - Add the common fan schema to $ref                                 [Krzysztof]
 - Update the node name to "fan-controller" in maxim,max31790.yaml   [Krzysztof]
 - Drop "driver" in commit title                                     [Krzysztof]
Changes in v3:
 - Drop redundant "bindings" in commit title                         [Krzysztof]
 - Add the clocks and resets property in example                     [Krzysztof]
 - Add child node refer to fan-common.yaml                           [Krzysztof, Conor]
---
 .../bindings/hwmon/maxim,max31790.yaml        | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml b/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
new file mode 100644
index 000000000000..d28a6373edd3
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/maxim,max31790.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: The Maxim MAX31790 Fan Controller
+
+maintainers:
+  - Guenter Roeck <linux@roeck-us.net>
+
+description: >
+  The MAX31790 controls the speeds of up to six fans using six
+  independent PWM outputs. The desired fan speeds (or PWM duty cycles)
+  are written through the I2C interface.
+
+  Datasheets:
+    https://datasheets.maximintegrated.com/en/ds/MAX31790.pdf
+
+properties:
+  compatible:
+    const: maxim,max31790
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 1
+
+patternProperties:
+  "^fan-[0-9]+$":
+    $ref: fan-common.yaml#
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      fan-controller@21 {
+        compatible = "maxim,max31790";
+        reg = <0x21>;
+        clocks = <&sys_clk>;
+        resets = <&reset 0>;
+      };
+    };
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      pwm_provider: fan-controller@20 {
+        compatible = "maxim,max31790";
+        reg = <0x20>;
+        clocks = <&sys_clk>;
+        resets = <&reset 0>;
+        #pwm-cells = <1>;
+
+        fan-0 {
+          pwms = <&pwm_provider 1>;
+        };
+
+        fan-1 {
+          pwms = <&pwm_provider 2>;
+        };
+      };
+    };
+
-- 
2.43.0

Re: [PATCH v3 1/1] dt-bindings: hwmon: Add maxim max31790

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3477 bytes --]

On Tue, Aug 13, 2024 at 08:41:52AM +0000, Chanh Nguyen wrote:
> Add device tree bindings and an example for max31790 device.
> 
> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com>
> ---
> Changes in v2:
>  - Update filename of the maxim,max31790.yaml                        [Krzysztof]
>  - Add the common fan schema to $ref                                 [Krzysztof]
>  - Update the node name to "fan-controller" in maxim,max31790.yaml   [Krzysztof]
>  - Drop "driver" in commit title                                     [Krzysztof]
> Changes in v3:
>  - Drop redundant "bindings" in commit title                         [Krzysztof]
>  - Add the clocks and resets property in example                     [Krzysztof]
>  - Add child node refer to fan-common.yaml                           [Krzysztof, Conor]
> ---
>  .../bindings/hwmon/maxim,max31790.yaml        | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml b/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
> new file mode 100644
> index 000000000000..d28a6373edd3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/maxim,max31790.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: The Maxim MAX31790 Fan Controller
> +
> +maintainers:
> +  - Guenter Roeck <linux@roeck-us.net>

Why Guenter and not you?

> +
> +description: >
> +  The MAX31790 controls the speeds of up to six fans using six
> +  independent PWM outputs. The desired fan speeds (or PWM duty cycles)
> +  are written through the I2C interface.
> +
> +  Datasheets:
> +    https://datasheets.maximintegrated.com/en/ds/MAX31790.pdf
> +
> +properties:
> +  compatible:
> +    const: maxim,max31790
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  "#pwm-cells":
> +    const: 1
> +
> +patternProperties:
> +  "^fan-[0-9]+$":
> +    $ref: fan-common.yaml#
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      fan-controller@21 {
> +        compatible = "maxim,max31790";
> +        reg = <0x21>;
> +        clocks = <&sys_clk>;
> +        resets = <&reset 0>;
> +      };
> +    };

What does this example demonstrate? The one below seems useful, this one
I don't quite understand - what's the point of a fan controller with no
fans connected to it? What am I missing?

Otherwise, this looks pretty good.

Cheers,
Conor.

> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      pwm_provider: fan-controller@20 {
> +        compatible = "maxim,max31790";
> +        reg = <0x20>;
> +        clocks = <&sys_clk>;
> +        resets = <&reset 0>;
> +        #pwm-cells = <1>;
> +
> +        fan-0 {
> +          pwms = <&pwm_provider 1>;
> +        };
> +
> +        fan-1 {
> +          pwms = <&pwm_provider 2>;
> +        };
> +      };
> +    };
> +
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/1] dt-bindings: hwmon: Add maxim max31790

[Guenter Roeck]

On 8/13/24 08:33, Conor Dooley wrote:
> On Tue, Aug 13, 2024 at 08:41:52AM +0000, Chanh Nguyen wrote:
>> Add device tree bindings and an example for max31790 device.
>>
>> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com>
>> ---
>> Changes in v2:
>>   - Update filename of the maxim,max31790.yaml                        [Krzysztof]
>>   - Add the common fan schema to $ref                                 [Krzysztof]
>>   - Update the node name to "fan-controller" in maxim,max31790.yaml   [Krzysztof]
>>   - Drop "driver" in commit title                                     [Krzysztof]
>> Changes in v3:
>>   - Drop redundant "bindings" in commit title                         [Krzysztof]
>>   - Add the clocks and resets property in example                     [Krzysztof]
>>   - Add child node refer to fan-common.yaml                           [Krzysztof, Conor]
>> ---
>>   .../bindings/hwmon/maxim,max31790.yaml        | 81 +++++++++++++++++++
>>   1 file changed, 81 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml b/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
>> new file mode 100644
>> index 000000000000..d28a6373edd3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
>> @@ -0,0 +1,81 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwmon/maxim,max31790.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: The Maxim MAX31790 Fan Controller
>> +
>> +maintainers:
>> +  - Guenter Roeck <linux@roeck-us.net>
> 
> Why Guenter and not you?
> 

Fine with me, actually. We don't expect individual driver maintainers
in the hardware monitoring subsystem, and this chip doesn't have an
explicit maintainer. Forcing people to act as maintainer for .yaml
files they submit can only result in fewer submissions. I prefer to be
listed as maintainer over having no devicetree bindings.

>> +
>> +description: >
>> +  The MAX31790 controls the speeds of up to six fans using six
>> +  independent PWM outputs. The desired fan speeds (or PWM duty cycles)
>> +  are written through the I2C interface.
>> +
>> +  Datasheets:
>> +    https://datasheets.maximintegrated.com/en/ds/MAX31790.pdf
>> +
>> +properties:
>> +  compatible:
>> +    const: maxim,max31790
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  "#pwm-cells":
>> +    const: 1
>> +
>> +patternProperties:
>> +  "^fan-[0-9]+$":
>> +    $ref: fan-common.yaml#
>> +    unevaluatedProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      fan-controller@21 {
>> +        compatible = "maxim,max31790";
>> +        reg = <0x21>;
>> +        clocks = <&sys_clk>;
>> +        resets = <&reset 0>;
>> +      };
>> +    };
> 
> What does this example demonstrate? The one below seems useful, this one
> I don't quite understand - what's the point of a fan controller with no
> fans connected to it? What am I missing?
> 

Just guessing, but maybe this is supposed to reflect a system which only monitors fan
speeds but does not implement fan control.

Guenter

> Otherwise, this looks pretty good.
> 
> Cheers,
> Conor.
> 
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      pwm_provider: fan-controller@20 {
>> +        compatible = "maxim,max31790";
>> +        reg = <0x20>;
>> +        clocks = <&sys_clk>;
>> +        resets = <&reset 0>;
>> +        #pwm-cells = <1>;
>> +
>> +        fan-0 {
>> +          pwms = <&pwm_provider 1>;
>> +        };
>> +
>> +        fan-1 {
>> +          pwms = <&pwm_provider 2>;
>> +        };
>> +      };
>> +    };
>> +
>> -- 
>> 2.43.0
>>

Re: [PATCH v3 1/1] dt-bindings: hwmon: Add maxim max31790

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4856 bytes --]

On Tue, Aug 13, 2024 at 08:52:22AM -0700, Guenter Roeck wrote:
> On 8/13/24 08:33, Conor Dooley wrote:
> > On Tue, Aug 13, 2024 at 08:41:52AM +0000, Chanh Nguyen wrote:
> > > Add device tree bindings and an example for max31790 device.
> > > 
> > > Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com>
> > > ---
> > > Changes in v2:
> > >   - Update filename of the maxim,max31790.yaml                        [Krzysztof]
> > >   - Add the common fan schema to $ref                                 [Krzysztof]
> > >   - Update the node name to "fan-controller" in maxim,max31790.yaml   [Krzysztof]
> > >   - Drop "driver" in commit title                                     [Krzysztof]
> > > Changes in v3:
> > >   - Drop redundant "bindings" in commit title                         [Krzysztof]
> > >   - Add the clocks and resets property in example                     [Krzysztof]
> > >   - Add child node refer to fan-common.yaml                           [Krzysztof, Conor]
> > > ---
> > >   .../bindings/hwmon/maxim,max31790.yaml        | 81 +++++++++++++++++++
> > >   1 file changed, 81 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml b/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
> > > new file mode 100644
> > > index 000000000000..d28a6373edd3
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
> > > @@ -0,0 +1,81 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/hwmon/maxim,max31790.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: The Maxim MAX31790 Fan Controller
> > > +
> > > +maintainers:
> > > +  - Guenter Roeck <linux@roeck-us.net>
> > 
> > Why Guenter and not you?
> > 
> 
> Fine with me, actually. We don't expect individual driver maintainers
> in the hardware monitoring subsystem, and this chip doesn't have an
> explicit maintainer. Forcing people to act as maintainer for .yaml
> files they submit can only result in fewer submissions. I prefer to be
> listed as maintainer over having no devicetree bindings.

Sure, if you're happy with it. Having someone that knows about how the
particular model works is usually preferred however!

> 
> > > +
> > > +description: >
> > > +  The MAX31790 controls the speeds of up to six fans using six
> > > +  independent PWM outputs. The desired fan speeds (or PWM duty cycles)
> > > +  are written through the I2C interface.
> > > +
> > > +  Datasheets:
> > > +    https://datasheets.maximintegrated.com/en/ds/MAX31790.pdf
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: maxim,max31790
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  resets:
> > > +    maxItems: 1
> > > +
> > > +  "#pwm-cells":
> > > +    const: 1
> > > +
> > > +patternProperties:
> > > +  "^fan-[0-9]+$":
> > > +    $ref: fan-common.yaml#
> > > +    unevaluatedProperties: false
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    i2c {
> > > +      #address-cells = <1>;
> > > +      #size-cells = <0>;
> > > +
> > > +      fan-controller@21 {
> > > +        compatible = "maxim,max31790";
> > > +        reg = <0x21>;
> > > +        clocks = <&sys_clk>;
> > > +        resets = <&reset 0>;
> > > +      };
> > > +    };
> > 
> > What does this example demonstrate? The one below seems useful, this one
> > I don't quite understand - what's the point of a fan controller with no
> > fans connected to it? What am I missing?
> > 
> 
> Just guessing, but maybe this is supposed to reflect a system which only monitors fan
> speeds but does not implement fan control.

Even without any control, I would expect to see fan-# child nodes, just
no pwms property in them. Without the child nodes, how does software
determine which fan is being monitored by which channel?

Cheers,
Conor.

> > > +  - |
> > > +    i2c {
> > > +      #address-cells = <1>;
> > > +      #size-cells = <0>;
> > > +
> > > +      pwm_provider: fan-controller@20 {
> > > +        compatible = "maxim,max31790";
> > > +        reg = <0x20>;
> > > +        clocks = <&sys_clk>;
> > > +        resets = <&reset 0>;
> > > +        #pwm-cells = <1>;
> > > +
> > > +        fan-0 {
> > > +          pwms = <&pwm_provider 1>;
> > > +        };
> > > +
> > > +        fan-1 {
> > > +          pwms = <&pwm_provider 2>;
> > > +        };
> > > +      };
> > > +    };
> > > +
> > > -- 
> > > 2.43.0
> > > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/1] dt-bindings: hwmon: Add maxim max31790

[Krzysztof Kozlowski]

On 13/08/2024 18:16, Conor Dooley wrote:
>>>> +examples:
>>>> +  - |
>>>> +    i2c {
>>>> +      #address-cells = <1>;
>>>> +      #size-cells = <0>;
>>>> +
>>>> +      fan-controller@21 {
>>>> +        compatible = "maxim,max31790";
>>>> +        reg = <0x21>;
>>>> +        clocks = <&sys_clk>;
>>>> +        resets = <&reset 0>;
>>>> +      };
>>>> +    };
>>>
>>> What does this example demonstrate? The one below seems useful, this one
>>> I don't quite understand - what's the point of a fan controller with no
>>> fans connected to it? What am I missing?
>>>
>>
>> Just guessing, but maybe this is supposed to reflect a system which only monitors fan
>> speeds but does not implement fan control.
> 
> Even without any control, I would expect to see fan-# child nodes, just
> no pwms property in them. Without the child nodes, how does software
> determine which fan is being monitored by which channel?

Yeah, to me this example is confusing. If device's purpose is to also
monitor, then hardware description in "description:" field should be a
bit extended.

Best regards,
Krzysztof

Re: [PATCH v3 1/1] dt-bindings: hwmon: Add maxim max31790

[Chanh Nguyen]

On 13/08/2024 23:21, Krzysztof Kozlowski wrote:
> On 13/08/2024 18:16, Conor Dooley wrote:
>>>>> +examples:
>>>>> +  - |
>>>>> +    i2c {
>>>>> +      #address-cells = <1>;
>>>>> +      #size-cells = <0>;
>>>>> +
>>>>> +      fan-controller@21 {
>>>>> +        compatible = "maxim,max31790";
>>>>> +        reg = <0x21>;
>>>>> +        clocks = <&sys_clk>;
>>>>> +        resets = <&reset 0>;
>>>>> +      };
>>>>> +    };
>>>>
>>>> What does this example demonstrate? The one below seems useful, this one
>>>> I don't quite understand - what's the point of a fan controller with no
>>>> fans connected to it? What am I missing?
>>>>
>>>
>>> Just guessing, but maybe this is supposed to reflect a system which only monitors fan
>>> speeds but does not implement fan control.
>>
>> Even without any control, I would expect to see fan-# child nodes, just
>> no pwms property in them. Without the child nodes, how does software
>> determine which fan is being monitored by which channel?
> 
> Yeah, to me this example is confusing. If device's purpose is to also
> monitor, then hardware description in "description:" field should be a
> bit extended.
> 
> Best regards,
> Krzysztof
> 

Thank all for your comments!

I'll keep only complete example. I'm going to push patch v4 in the 
coming days.

Regards!
Chanh

Re: [PATCH v3 1/1] dt-bindings: hwmon: Add maxim max31790

[Chanh Nguyen]

On 13/08/2024 23:16, Conor Dooley wrote:
> On Tue, Aug 13, 2024 at 08:52:22AM -0700, Guenter Roeck wrote:
>> On 8/13/24 08:33, Conor Dooley wrote:
>>> On Tue, Aug 13, 2024 at 08:41:52AM +0000, Chanh Nguyen wrote:
>>>> Add device tree bindings and an example for max31790 device.
>>>>
>>>> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com>
>>>> ---
>>>> Changes in v2:
>>>>    - Update filename of the maxim,max31790.yaml                        [Krzysztof]
>>>>    - Add the common fan schema to $ref                                 [Krzysztof]
>>>>    - Update the node name to "fan-controller" in maxim,max31790.yaml   [Krzysztof]
>>>>    - Drop "driver" in commit title                                     [Krzysztof]
>>>> Changes in v3:
>>>>    - Drop redundant "bindings" in commit title                         [Krzysztof]
>>>>    - Add the clocks and resets property in example                     [Krzysztof]
>>>>    - Add child node refer to fan-common.yaml                           [Krzysztof, Conor]
>>>> ---
>>>>    .../bindings/hwmon/maxim,max31790.yaml        | 81 +++++++++++++++++++
>>>>    1 file changed, 81 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml b/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
>>>> new file mode 100644
>>>> index 000000000000..d28a6373edd3
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
>>>> @@ -0,0 +1,81 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/hwmon/maxim,max31790.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: The Maxim MAX31790 Fan Controller
>>>> +
>>>> +maintainers:
>>>> +  - Guenter Roeck <linux@roeck-us.net>
>>>
>>> Why Guenter and not you?
>>>
>>
>> Fine with me, actually. We don't expect individual driver maintainers
>> in the hardware monitoring subsystem, and this chip doesn't have an
>> explicit maintainer. Forcing people to act as maintainer for .yaml
>> files they submit can only result in fewer submissions. I prefer to be
>> listed as maintainer over having no devicetree bindings.
> 
> Sure, if you're happy with it. Having someone that knows about how the
> particular model works is usually preferred however!
> 

Thank Guenter and Conor for your comments!

I will add me to maintainers list. I'm going to push the patch v4 with 
this update soon. It will be as below:

  maintainers:
    - Guenter Roeck <linux@roeck-us.net>
    - Chanh Nguyen <chanh@os.amperecomputing.com>

I think I can support to reviewing any max31790 binding update later.

Thanks,
Chanh

>>
>>>> +
>>>> +description: >
>>>> +  The MAX31790 controls the speeds of up to six fans using six
>>>> +  independent PWM outputs. The desired fan speeds (or PWM duty cycles)
>>>> +  are written through the I2C interface.
>>>> +
>>>> +  Datasheets:
>>>> +    https://datasheets.maximintegrated.com/en/ds/MAX31790.pdf
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: maxim,max31790
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 1
>>>> +
>>>> +  resets:
>>>> +    maxItems: 1
>>>> +
>>>> +  "#pwm-cells":
>>>> +    const: 1
>>>> +
>>>> +patternProperties:
>>>> +  "^fan-[0-9]+$":
>>>> +    $ref: fan-common.yaml#
>>>> +    unevaluatedProperties: false
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    i2c {
>>>> +      #address-cells = <1>;
>>>> +      #size-cells = <0>;
>>>> +
>>>> +      fan-controller@21 {
>>>> +        compatible = "maxim,max31790";
>>>> +        reg = <0x21>;
>>>> +        clocks = <&sys_clk>;
>>>> +        resets = <&reset 0>;
>>>> +      };
>>>> +    };
>>>
>>> What does this example demonstrate? The one below seems useful, this one
>>> I don't quite understand - what's the point of a fan controller with no
>>> fans connected to it? What am I missing?
>>>
>>
>> Just guessing, but maybe this is supposed to reflect a system which only monitors fan
>> speeds but does not implement fan control.
> 
> Even without any control, I would expect to see fan-# child nodes, just
> no pwms property in them. Without the child nodes, how does software
> determine which fan is being monitored by which channel?
> 
> Cheers,
> Conor.
> 
>>>> +  - |
>>>> +    i2c {
>>>> +      #address-cells = <1>;
>>>> +      #size-cells = <0>;
>>>> +
>>>> +      pwm_provider: fan-controller@20 {
>>>> +        compatible = "maxim,max31790";
>>>> +        reg = <0x20>;
>>>> +        clocks = <&sys_clk>;
>>>> +        resets = <&reset 0>;
>>>> +        #pwm-cells = <1>;
>>>> +
>>>> +        fan-0 {
>>>> +          pwms = <&pwm_provider 1>;
>>>> +        };
>>>> +
>>>> +        fan-1 {
>>>> +          pwms = <&pwm_provider 2>;
>>>> +        };
>>>> +      };
>>>> +    };
>>>> +
>>>> -- 
>>>> 2.43.0
>>>>
>>

Re: [PATCH v3 1/1] dt-bindings: hwmon: Add maxim max31790

[Krzysztof Kozlowski]

On 13/08/2024 10:41, Chanh Nguyen wrote:
> Add device tree bindings and an example for max31790 device.
> 
> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com>


> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      fan-controller@21 {
> +        compatible = "maxim,max31790";
> +        reg = <0x21>;
> +        clocks = <&sys_clk>;
> +        resets = <&reset 0>;

This node is incomplete. I asked to make the example complete, not by
adding two incomplete examples or other ways... The binding description
says this device controls fan. If so, where is the fan here?

IOW, keep only one, complete example.

Rest looks good. With this addressed (and optionally with maintainer
change, which Conor asked):

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 1/1] dt-bindings: hwmon: Add maxim max31790

[Chanh Nguyen]

On 13/08/2024 23:20, Krzysztof Kozlowski wrote:
> On 13/08/2024 10:41, Chanh Nguyen wrote:
>> Add device tree bindings and an example for max31790 device.
>>
>> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com>
> 
> 
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      fan-controller@21 {
>> +        compatible = "maxim,max31790";
>> +        reg = <0x21>;
>> +        clocks = <&sys_clk>;
>> +        resets = <&reset 0>;
> 
> This node is incomplete. I asked to make the example complete, not by
> adding two incomplete examples or other ways... The binding description
> says this device controls fan. If so, where is the fan here?
> 
> IOW, keep only one, complete example.
> 
> Rest looks good. With this addressed (and optionally with maintainer
> change, which Conor asked):

Thank Krzysztof for your review!

I'll keep only complete example. I'm going to push patch v4 in the 
coming days.

Thanks,
Chanh

> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 0/1] Update the max31790 driver

[Chanh Nguyen]

Hi all,

I dropped the "Support config PWM output becomes TACH" patch. I'm 
implementing and testing the "Support config PWM output become TACH" 
support; I'll push this feature later. Now, I'm only adding the device 
tree binding for the max31790 device; it is necessary for some device 
tree upstreams that use the "maxim,max31790" compatible.

Best Regards,
Chanh

On 13/08/2024 15:41, Chanh Nguyen wrote:
> Add device tree binding for the max31790 device.
> 
> Changes in v2:
>   - Drop "driver" in the patch 1/3 commit title                           [Krzysztof]
>   - Update filename of the maxim,max31790.yaml                            [Krzysztof]
>   - Add the common fan schema to $ref                                     [Krzysztof]
>   - Update the node name to "fan-controller" in maxim,max31790.yaml       [Krzysztof]
>   - Update the vendor property name to "maxim,pwmout-pin-as-tach-input"   [Rob]
> Changes in v3:
>   - Drop redundant "bindings" in commit message                           [Krzysztof]
>   - Add the clocks and resets property to example                         [Krzysztof]
>   - Add child node refer to fan-common.yaml                               [Krzysztof, Conor]
>   - Drop "Support config PWM output become TACH" patch                    [Chanh]
>   - Drop "Add maxim,pwmout-pin-as-tach-input property" patch              [Chanh]
> 
> Chanh Nguyen (1):
>    dt-bindings: hwmon: Add maxim max31790
> 
>   .../bindings/hwmon/maxim,max31790.yaml        | 81 +++++++++++++++++++
>   1 file changed, 81 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max31790.yaml
>