[PATCH v2 0/3] emc2305 driver updates

[PATCH v2 0/3] emc2305 driver updates

[florin.leotescu]

From: Florin Leotescu <florin.leotescu@nxp.com>

This patch series enhance the emc2305 driver by introducing
device tree support for configuring PWM output config (open drain /
push pull) and PWM polarity config (default/inverted).
Also updates the thermal management, to be configured based on 
device tree definition.

Changes since v1:
- Add dt-binding documentation, as recommended by Guenter Roeck.
- Remove emc2305_unset_tz due to 
  devm_thermal_of_cooling_device_register use.

Florin Leotescu (3):
  hwmon: emc2305: Update cooling device registration to include device
    node
  hwmon: emc2305: Add device tree support for polarity and pwm output
  dt-bindings: hwmon: emc2305: Add YAML binding documentation for
    emc2305 driver

 .../devicetree/bindings/hwmon/emc2305.yaml    | 95 +++++++++++++++++++
 drivers/hwmon/emc2305.c                       | 55 ++++++-----
 2 files changed, 125 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/emc2305.yaml

-- 
2.34.1

[PATCH v2 1/3] hwmon: emc2305: Update cooling device registration to include device node

[florin.leotescu]

From: Florin Leotescu <florin.leotescu@nxp.com>

This patch updates the EMC2305 hwmon driver to register the thermal
cooling device with Device Tree (DTS) node. This change allows
cooling device to be configured based on the properties defined
in Device Tree. Use devm_thermal_of_cooling_device_register for
automatic resource management and to simplify resource management.

Signed-off-by: Florin Leotescu <florin.leotescu@nxp.com>
---
 drivers/hwmon/emc2305.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
index 4d39fbd83769..5d8fb7526e23 100644
--- a/drivers/hwmon/emc2305.c
+++ b/drivers/hwmon/emc2305.c
@@ -112,8 +112,6 @@ static char *emc2305_fan_name[] = {
 	"emc2305_fan5",
 };
 
-static void emc2305_unset_tz(struct device *dev);
-
 static int emc2305_get_max_channel(const struct emc2305_data *data)
 {
 	return data->pwm_num;
@@ -293,8 +291,9 @@ static int emc2305_set_single_tz(struct device *dev, int idx)
 	pwm = data->pwm_min[cdev_idx];
 
 	data->cdev_data[cdev_idx].cdev =
-		thermal_cooling_device_register(emc2305_fan_name[idx], data,
-						&emc2305_cooling_ops);
+		devm_thermal_of_cooling_device_register(dev, dev->of_node,
+							emc2305_fan_name[idx], data,
+							&emc2305_cooling_ops);
 
 	if (IS_ERR(data->cdev_data[cdev_idx].cdev)) {
 		dev_err(dev, "Failed to register cooling device %s\n", emc2305_fan_name[idx]);
@@ -337,21 +336,9 @@ static int emc2305_set_tz(struct device *dev)
 	return 0;
 
 thermal_cooling_device_register_fail:
-	emc2305_unset_tz(dev);
 	return ret;
 }
 
-static void emc2305_unset_tz(struct device *dev)
-{
-	struct emc2305_data *data = dev_get_drvdata(dev);
-	int i;
-
-	/* Unregister cooling device. */
-	for (i = 0; i < EMC2305_PWM_MAX; i++)
-		if (data->cdev_data[i].cdev)
-			thermal_cooling_device_unregister(data->cdev_data[i].cdev);
-}
-
 static umode_t
 emc2305_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr, int channel)
 {
@@ -599,20 +586,11 @@ static int emc2305_probe(struct i2c_client *client)
 	return 0;
 }
 
-static void emc2305_remove(struct i2c_client *client)
-{
-	struct device *dev = &client->dev;
-
-	if (IS_REACHABLE(CONFIG_THERMAL))
-		emc2305_unset_tz(dev);
-}
-
 static struct i2c_driver emc2305_driver = {
 	.driver = {
 		.name = "emc2305",
 	},
 	.probe = emc2305_probe,
-	.remove	  = emc2305_remove,
 	.id_table = emc2305_ids,
 };
 
-- 
2.34.1

[PATCH v2 2/3] hwmon: emc2305: Add device tree support for polarity and pwm output

[florin.leotescu]

From: Florin Leotescu <florin.leotescu@nxp.com>

The patch enhances emc2305 driver by adding support for configuring
pwm output and polarity via Device Tree properties.

Signed-off-by: Florin Leotescu <florin.leotescu@nxp.com>
---
 drivers/hwmon/emc2305.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
index 5d8fb7526e23..e646221ab65d 100644
--- a/drivers/hwmon/emc2305.c
+++ b/drivers/hwmon/emc2305.c
@@ -23,6 +23,8 @@
 #define EMC2305_TACH_REGS_UNUSE_BITS	3
 #define EMC2305_TACH_CNT_MULTIPLIER	0x02
 #define EMC2305_TACH_RANGE_MIN		480
+#define EMC2305_REG_DRIVE_PWM_OUT_CONFIG 0x2b
+#define EMC2305_REG_POLARITY 0x2a
 
 #define EMC2305_PWM_DUTY2STATE(duty, max_state, pwm_max) \
 	DIV_ROUND_CLOSEST((duty) * (max_state), (pwm_max))
@@ -523,6 +525,8 @@ static int emc2305_probe(struct i2c_client *client)
 	int vendor;
 	int ret;
 	int i;
+	int pwm_polarity;
+	int pwm_output;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
 		return -ENODEV;
@@ -576,6 +580,29 @@ static int emc2305_probe(struct i2c_client *client)
 			return ret;
 	}
 
+	if (!of_property_read_u32(dev->of_node, "pwm_output", &pwm_output)) {
+		dev_dbg(dev, "Configuring pwm output\n");
+		if (pwm_output >= 0 && pwm_output <= ((1 << data->pwm_num) - 1)) {
+			ret = i2c_smbus_write_byte_data(client, EMC2305_REG_DRIVE_PWM_OUT_CONFIG,
+							 pwm_output);
+			if (ret < 0)
+				dev_err(dev, "Failed to configure pwm output, using default\n");
+		} else {
+			dev_err(dev, "Wrong PWM output config provided: %u\n", pwm_output);
+		}
+	}
+
+	if (!of_property_read_u32(dev->of_node, "pwm_polarity", &pwm_polarity)) {
+		dev_dbg(dev, "Configuring pwm polarity\n");
+		if (pwm_polarity >= 0 && pwm_polarity  <= ((1 << data->pwm_num) - 1)) {
+			ret = i2c_smbus_write_byte_data(client, EMC2305_REG_POLARITY, pwm_polarity);
+			if (ret < 0)
+				dev_err(dev, "Failed to configure pwm polarity, using default\n");
+		} else {
+			dev_err(dev, "Wrong PWM polarity config provided: %u\n", pwm_polarity);
+		}
+	}
+
 	for (i = 0; i < data->pwm_num; i++) {
 		ret = i2c_smbus_write_byte_data(client, EMC2305_REG_FAN_MIN_DRIVE(i),
 						data->pwm_min[i]);
-- 
2.34.1

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

[florin.leotescu]

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

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>;
+    };
-- 
2.34.1

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

[Fabio Estevam]

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

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

[Krzysztof Kozlowski]

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

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

[Rob Herring (Arm)]

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.

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

[Guenter Roeck]

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

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

[Krzysztof Kozlowski]

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