Re: [PATCH v2 3/3] dt-bindings: hwmon: emc2305: Add YAML binding documentation for emc2305 driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/02/2025 14:32, florin.leotescu@oss.nxp.com wrote:
> From: Florin Leotescu <florin.leotescu@nxp.com>
> 

A nit, subject: drop second/last, redundant "YAML binding
documentation". Three useless/redundant terms. The "dt-bindings" prefix
is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

And drop all driver references - you put it everywhere.


> Add yaml-based Device Tree bindings documentation for emc2305 driver
> The file provides the necessary structure, configuration
> and other parameters for Device Tree definition.
> 
> Signed-off-by: Florin Leotescu <florin.leotescu@nxp.com>
> ---
>  .../devicetree/bindings/hwmon/emc2305.yaml    | 95 +++++++++++++++++++

Filename matching compatible.

>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/emc2305.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/emc2305.yaml b/Documentation/devicetree/bindings/hwmon/emc2305.yaml
> new file mode 100644
> index 000000000000..51e2a82d8f25
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/emc2305.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/emc2305.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: EMC2305 i2c pwm fan controller
> +
> +maintainers:
> +  - Michael Shych <michaelsh@nvidia.com>
> +
> +description: |
> +  The driver implements support for Microchip EMC2301/2/3/5 PWM Fan Controller.

This is a binding so describe hardware, not your implementation.


> +  The EMC2301 Fan Controller supports only one controlled PWM fan channel.
> +  The EMC2305 Fan Controller supports up to 5 independently
> +  controlled PWM fan drives.
> +

Missing fan-common reference.

> +properties:
> +  compatible:
> +    enum:
> +      - hwmon,emc2301
> +      - hwmon,emc2302
> +      - hwmon,emc2303
> +      - hwmon,emc2305

Nope.

Was it ever internally reviewed?


> +
> +  reg:
> +    description: I2C address of the emc2305 device

Look how other bindings do it.

> +
> +  pwm_output:

NAK.

There are so many issues with this code, from trivial incorrect quotes
and not following DTS coding style, to actual duplicating other schemas
or common part.

Sorry, get it first internally reviewed.

> +    description: "PWM output type Push-Pull/ Open Drain"
> +    maxItems: 1
> +
> +  pwm_polarity:
> +    description: "PWM polarity"
> +    maxItems: 1
> +
> +  '#cooling-cells':
> +    description: "cooling state range"

Do you see any binding like that? Any?

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    thermal_zones {
> +        a55-thermal {
> +                trips {
> +                        atrip0: trip0 {
> +                                temperature = <55000>;
> +                                hysteresis = <2000>;
> +                                type = "active";
> +                        };
> +
> +                        atrip1: trip1 {
> +                                temperature = <65000>;
> +                                hysteresis = <2000>;
> +                                type = "active";
> +                        };
> +
> +                        atrip2: trip2 {
> +                                temperature = <75000>;
> +                                hysteresis = <2000>;
> +                                type = "active";
> +                        };
> +                };
> +
> +                cooling-maps {
> +                        map1 {
> +                                trip = <&atrip0>;
> +                                cooling-device = <&emc2301 4 6>;
> +                        };
> +
> +                        map2 {
> +                                trip = <&atrip1>;
> +                                cooling-device = <&emc2301 6 8>;
> +                        };
> +
> +                        map3 {
> +                                trip = <&atrip2>;
> +                                cooling-device = <&emc2301 8 10>;
> +                        };
> +                };
> +        };
> +
> +    }

This DTS is also in very poor shape - even your indentation does not
match. Drop redundant parts - entire thermal.

> +    emc2301: pwm@2f {
> +       compatible = "hwmon,emc2301";
> +       reg = <0x2f>;
> +       #cooling-cells = <2>;
> +       pwm_output = <0x1>;
> +       pwm_polarity = <0x1>;
> +    };


Best regards,
Krzysztof