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

[Conor Dooley <conor@kernel.org>]

[-- 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 --]