Re: [PATCH v3 1/2] dt-bindings: hwmon: amc6821: add fan and PWM output

[Guenter Roeck]

On 4/1/25 00:43, Francesco Dolcini wrote:
> On Tue, Apr 01, 2025 at 08:13:14AM +0200, Krzysztof Kozlowski wrote:
> > On Mon, Mar 31, 2025 at 05:52:28PM +0200, Francesco Dolcini wrote:
> > > From: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx>
> > >
> > > Add properties to describe the fan and the PWM controller output.
> > >
> > > Link: https://www.ti.com/lit/gpn/amc6821
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx>
> > > ---
> > > v3:
> > >   - explicitly describe the fan, use standard PWM and FAN bindings
> > >   - pwm.yaml cannot be referenced, because of the $nodename pattern that is
> > >     enforced there
> > > v2: https://lore.kernel.org/all/20250224180801.128685-2-francesco@xxxxxxxxxx/
> > >   - no changes
> > > v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@xxxxxxxxxx/
> > > ---
> > >   .../devicetree/bindings/hwmon/ti,amc6821.yaml      | 14 +++++++++++++-
> > >   1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> > > index 5d33f1a23d03..94aca9c378e6 100644
> > > --- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> > > +++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> > > @@ -28,6 +28,13 @@ properties:
> > >     i2c-mux:
> > >       type: object
> > >   
> > > +  fan:
> > > +    $ref: fan-common.yaml#
> > > +    unevaluatedProperties: false
> >
> > Why do you need the child, instead of referencing fan-common in the top
> > level?
>
> Two small reasons.
>
> First is that the amc6821 is a fan controller, and the fan is just
> connected to it. So having the fan as a child seemed the right way to
> describe it, and this is done like that in other hwmon binding.
>
> .. but now that you asked I tried to move the fan-common to the top
> level and it's not working.
>
> I added
>
>    allOf:
>      - $ref: fan-common.yaml#
>
> at top level, removed the fan child, and moved the pwms up one level in
> the example
>
>      i2c {
>          #address-cells = <1>;
>          #size-cells = <0>;
>
>          fan_controller: fan@18 {
>              compatible = "ti,amc6821";
>              reg = <0x18>;
>              #pwm-cells = <2>;
>              pwms = <&fan_controller 40000 0>;

devicetree wants to see #pwm-cells one level above pwms. Obviously you can't
put it under the i2c node, so you need the sub-level. Maybe there is a way
around it, but if so I don't know what it might be.

Guenter