| Message ID | 20250219133221.2641041-4-florin.leotescu@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | emc2305 driver updates | expand |
On Wed, Feb 19, 2025 at 10:26 AM <florin.leotescu@oss.nxp.com> wrote: > +description: | > + The driver implements support for Microchip EMC2301/2/3/5 PWM Fan Controller. > + The EMC2301 Fan Controller supports only one controlled PWM fan channel. > + The EMC2305 Fan Controller supports up to 5 independently > + controlled PWM fan drives. > + > +properties: > + compatible: > + enum: > + - hwmon,emc2301 As Microchip is the manufacturer, this should be: microchip,emc2301
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
On Wed, 19 Feb 2025 15:32:21 +0200, florin.leotescu@oss.nxp.com wrote: > From: Florin Leotescu <florin.leotescu@nxp.com> > > 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 +++++++++++++++++++ > 1 file changed, 95 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/emc2305.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/hwmon/emc2305.yaml:30:18: [error] string value is redundantly quoted with any quotes (quoted-strings) ./Documentation/devicetree/bindings/hwmon/emc2305.yaml:34:18: [error] string value is redundantly quoted with any quotes (quoted-strings) ./Documentation/devicetree/bindings/hwmon/emc2305.yaml:38:18: [error] string value is redundantly quoted with any quotes (quoted-strings) dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/emc2305.yaml: pwm_output: missing type definition Error: Documentation/devicetree/bindings/hwmon/emc2305.example.dts:59.9-17 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/hwmon/emc2305.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1511: dt_binding_check] Error 2 make: *** [Makefile:251: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250219133221.2641041-4-florin.leotescu@oss.nxp.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 2/19/25 06:01, Krzysztof Kozlowski wrote: [ ... ] >> +properties: >> + compatible: >> + enum: >> + - hwmon,emc2301 >> + - hwmon,emc2302 >> + - hwmon,emc2303 >> + - hwmon,emc2305 > > Nope. > > Was it ever internally reviewed? > No. I intentionally do not review bindings because I notoriously get it wrong, and instead rely on DT maintainers. I agree though that this one is really bad :-(. Guenter
On 19/02/2025 16:52, Guenter Roeck wrote: > On 2/19/25 06:01, Krzysztof Kozlowski wrote: > [ ... ] > >>> +properties: >>> + compatible: >>> + enum: >>> + - hwmon,emc2301 >>> + - hwmon,emc2302 >>> + - hwmon,emc2303 >>> + - hwmon,emc2305 >> >> Nope. >> >> Was it ever internally reviewed? >> > No. I intentionally do not review bindings because I notoriously get it wrong, > and instead rely on DT maintainers. > > I agree though that this one is really bad :-( Wait, my comment was not towards you, but towards NXP and their internal review. NXP is a big company, not individual contributor, so they should use internal review to catch obvious issues instead of using community resources for such trivial tasks. Best regards, Krzysztof
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. + The EMC2301 Fan Controller supports only one controlled PWM fan channel. + The EMC2305 Fan Controller supports up to 5 independently + controlled PWM fan drives. + +properties: + compatible: + enum: + - hwmon,emc2301 + - hwmon,emc2302 + - hwmon,emc2303 + - hwmon,emc2305 + + reg: + description: I2C address of the emc2305 device + + pwm_output: + description: "PWM output type Push-Pull/ Open Drain" + maxItems: 1 + + pwm_polarity: + description: "PWM polarity" + maxItems: 1 + + '#cooling-cells': + description: "cooling state range" + +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>; + }; + }; + }; + + } + emc2301: pwm@2f { + compatible = "hwmon,emc2301"; + reg = <0x2f>; + #cooling-cells = <2>; + pwm_output = <0x1>; + pwm_polarity = <0x1>; + };