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

[Guenter Roeck <linux@roeck-us.net>]

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@toradex.com>
>>>
>>> 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@toradex.com>
>>> ---
>>> 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@dolcini.it/
>>>   - no changes
>>> v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@dolcini.it/
>>> ---
>>>   .../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