[PATCH v4 0/3] Add devicetree support for max6639

[PATCH v4 0/3] Add devicetree support for max6639

[Naresh Solanki]

These patches adds devicetree support for MAX6639.

Changes V4:
- Fix dt error
- update comment
Changes V3:
- correct fan dt property name
- remove unrelevent changes
Changes V2:
- Fix dt schema error.
Changes:
- Add fan-common dt schema.
- add dt-binding support for max6639
- add max6639 specific property

Marcello Sylvester Bauer (1):
  dt-bindings: hwmon: Add binding for max6639

Naresh Solanki (2):
  dt-bindings: hwmon: fan: Add fan binding to schema
  hwmon: (max6639) Change from pdata to dt configuration

 .../devicetree/bindings/hwmon/fan-common.yaml |  48 ++++
 .../bindings/hwmon/maxim,max6639.yaml         |  86 ++++++++
 drivers/hwmon/max6639.c                       | 206 +++++++++++++-----
 3 files changed, 287 insertions(+), 53 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/fan-common.yaml
 create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml


base-commit: 0cf46a653bdae56683fece68dc50340f7520e6c4
-- 
2.37.3

[PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Naresh Solanki]

Add common fan properties bindings to a schema.

Bindings for fan controllers can reference the common schema for the
fan

child nodes:

  patternProperties:
    "^fan@[0-2]":
      type: object
      $ref: fan-common.yaml#

Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 .../devicetree/bindings/hwmon/fan-common.yaml | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/fan-common.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/fan-common.yaml b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
new file mode 100644
index 000000000000..224f5013c93f
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: GPL-2.0-or-later OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/fan-common.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common fan properties
+
+maintainers:
+  - Naresh Solanki <naresh.solanki@9elements.com>
+
+properties:
+  max-rpm:
+    description:
+      Max RPM supported by fan.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  pulses-per-revolution:
+    description:
+      The number of pulse from fan sensor per revolution.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  default-rpm:
+    description:
+      Target RPM the fan should be configured during driver probe.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  pwm-frequency:
+    description:
+      PWM frequency for fan in Hertz(Hz).
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  pwm-polarity-inverse:
+    description:
+      Inverse PWM polarity for fan.
+    type: boolean
+
+  label:
+    description:
+      Optional fan label
+
+  fan-supply:
+    description:
+      Power supply for fan.
+
+additionalProperties: true
+
+...

base-commit: 0cf46a653bdae56683fece68dc50340f7520e6c4
-- 
2.37.3

[PATCH v4 2/3] dt-bindings: hwmon: Add binding for max6639

[Naresh Solanki]

From: Marcello Sylvester Bauer <sylv@sylv.io>

Add Devicetree binding documentation for Maxim MAX6639 temperature
monitor with PWM fan-speed controller.

Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 .../bindings/hwmon/maxim,max6639.yaml         | 86 +++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
new file mode 100644
index 000000000000..22065ea56880
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/maxim,max6639.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim max6639
+
+maintainers:
+  - Roland Stigge <stigge@antcom.de>
+
+description: |
+  The MAX6639 is a 2-channel temperature monitor with dual, automatic, PWM
+  fan-speed controller.  It monitors its own temperature and one external
+  diode-connected transistor or the temperatures of two external diode-connected
+  transistors, typically available in CPUs, FPGAs, or GPUs.
+
+  Datasheets:
+    https://datasheets.maximintegrated.com/en/ds/MAX6639-MAX6639F.pdf
+
+properties:
+  compatible:
+    enum:
+      - maxim,max6639
+
+  reg:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+
+patternProperties:
+  "^fan@[0-1]$":
+    type: object
+    description: |
+      Represents the two fans and their specific configuration.
+
+    $ref: fan-common.yaml#
+
+    properties:
+      reg:
+        description: |
+          The fan number.
+        items:
+          minimum: 0
+          maximum: 1
+
+    required:
+      - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      fan-controller@10 {
+        compatible = "maxim,max6639";
+        reg = <0x10>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        fan@0 {
+          reg = <0x0>;
+          pulses-per-revolution = <2>;
+          max-rpm = <4000>;
+        };
+
+        fan@1 {
+          reg = <0x1>;
+          pulses-per-revolution = <2>;
+          max-rpm = <8000>;
+        };
+      };
+    };
+...
-- 
2.37.3

[PATCH v4 3/3] hwmon: (max6639) Change from pdata to dt configuration

[Naresh Solanki]

max6639_platform_data is not used by any in-kernel driver and does not
address the MAX6639 fans separately.
Move to device tree configuration with explicit properties to configure
each fan.

Non-DT platform can still use this module with its default
configuration.

Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/max6639.c | 206 +++++++++++++++++++++++++++++-----------
 1 file changed, 153 insertions(+), 53 deletions(-)

diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
index 9b895402c80d..bb6bcd61f34d 100644
--- a/drivers/hwmon/max6639.c
+++ b/drivers/hwmon/max6639.c
@@ -19,7 +19,6 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
-#include <linux/platform_data/max6639.h>
 
 /* Addresses to scan */
 static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
@@ -85,9 +84,9 @@ struct max6639_data {
 	u8 temp_ot[2];		/* OT Temperature, 0..255 C (->_emergency) */
 
 	/* Register values initialized only once */
-	u8 ppr;			/* Pulses per rotation 0..3 for 1..4 ppr */
-	u8 rpm_range;		/* Index in above rpm_ranges table */
-
+	u8 ppr[2];		/* Pulses per rotation 0..3 for 1..4 ppr */
+	u8 rpm_range[2];	/* Index in above rpm_ranges table */
+	u8 pwm_polarity[2];	/* Fans PWM polarity, 0..1 */
 	/* Optional regulator for FAN supply */
 	struct regulator *reg;
 };
@@ -319,7 +318,7 @@ static ssize_t fan_input_show(struct device *dev,
 		return PTR_ERR(data);
 
 	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
-		       data->rpm_range));
+		       data->rpm_range[attr->index]));
 }
 
 static ssize_t alarm_show(struct device *dev,
@@ -386,29 +385,40 @@ static struct attribute *max6639_attrs[] = {
 ATTRIBUTE_GROUPS(max6639);
 
 /*
- *  returns respective index in rpm_ranges table
- *  1 by default on invalid range
+ *  Get respective index in rpm_ranges table
  */
-static int rpm_range_to_reg(int range)
+static int rpm_range_to_index(struct device *dev, u8 *index, int rpm)
 {
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(rpm_ranges); i++) {
-		if (rpm_ranges[i] == range)
-			return i;
+	if (rpm < 0)
+		return -EINVAL;
+
+	/* Set index based on chip support */
+	switch (rpm) {
+	case 0 ... 2000:
+		*index = 0;
+		break;
+	case 2001 ... 4000:
+		*index = 1;
+		break;
+	case 4001 ... 8000:
+		*index = 2;
+		break;
+	case 8001 ... 16000:
+		*index = 3;
+		break;
+	default:
+		/* Use max range for higher RPM */
+		dev_warn(dev,
+		    "RPM higher than supported range. Default to 16000 RPM");
+		*index = 3;
 	}
-
-	return 1; /* default: 4000 RPM */
+	return 0;
 }
 
 static int max6639_init_client(struct i2c_client *client,
 			       struct max6639_data *data)
 {
-	struct max6639_platform_data *max6639_info =
-		dev_get_platdata(&client->dev);
-	int i;
-	int rpm_range = 1; /* default: 4000 RPM */
-	int err;
+	int i, err;
 
 	/* Reset chip to default values, see below for GCONFIG setup */
 	err = i2c_smbus_write_byte_data(client, MAX6639_REG_GCONFIG,
@@ -416,51 +426,32 @@ static int max6639_init_client(struct i2c_client *client,
 	if (err)
 		goto exit;
 
-	/* Fans pulse per revolution is 2 by default */
-	if (max6639_info && max6639_info->ppr > 0 &&
-			max6639_info->ppr < 5)
-		data->ppr = max6639_info->ppr;
-	else
-		data->ppr = 2;
-	data->ppr -= 1;
-
-	if (max6639_info)
-		rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
-	data->rpm_range = rpm_range;
-
 	for (i = 0; i < 2; i++) {
 
 		/* Set Fan pulse per revolution */
 		err = i2c_smbus_write_byte_data(client,
 				MAX6639_REG_FAN_PPR(i),
-				data->ppr << 6);
+				data->ppr[i] << 6);
 		if (err)
 			goto exit;
 
 		/* Fans config PWM, RPM */
 		err = i2c_smbus_write_byte_data(client,
 			MAX6639_REG_FAN_CONFIG1(i),
-			MAX6639_FAN_CONFIG1_PWM | rpm_range);
+			MAX6639_FAN_CONFIG1_PWM | data->rpm_range[i]);
 		if (err)
 			goto exit;
 
-		/* Fans PWM polarity high by default */
-		if (max6639_info && max6639_info->pwm_polarity == 0)
-			err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_FAN_CONFIG2a(i), 0x00);
-		else
-			err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_FAN_CONFIG2a(i), 0x02);
-		if (err)
-			goto exit;
+		/* Fans PWM polarity */
+		err = i2c_smbus_write_byte_data(client,
+			MAX6639_REG_FAN_CONFIG2a(i), data->pwm_polarity[i] ? 0x02 : 0x00);
 
 		/*
-		 * /THERM full speed enable,
+		 * /THERM full speed disable,
 		 * PWM frequency 25kHz, see also GCONFIG below
 		 */
 		err = i2c_smbus_write_byte_data(client,
-			MAX6639_REG_FAN_CONFIG3(i),
-			MAX6639_FAN_CONFIG3_THERM_FULL_SPEED | 0x03);
+			MAX6639_REG_FAN_CONFIG3(i), 0x03);
 		if (err)
 			goto exit;
 
@@ -483,8 +474,6 @@ static int max6639_init_client(struct i2c_client *client,
 		if (err)
 			goto exit;
 
-		/* PWM 120/120 (i.e. 100%) */
-		data->pwm[i] = 120;
 		err = i2c_smbus_write_byte_data(client,
 				MAX6639_REG_TARGTDUTY(i), data->pwm[i]);
 		if (err)
@@ -524,12 +513,91 @@ static void max6639_regulator_disable(void *data)
 	regulator_disable(data);
 }
 
+static int max6639_probe_child_from_dt(struct i2c_client *client,
+				      struct device_node *child,
+				      struct max6639_data *data)
+
+{
+	struct device *dev = &client->dev;
+	u32 i, val, maxrpm;
+	int err;
+
+	err = of_property_read_u32(child, "reg", &i);
+	if (err) {
+		dev_err(dev, "missing reg property of %pOFn\n", child);
+		return err;
+	}
+
+	if (i >= 2) {
+		dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
+		return -EINVAL;
+	}
+
+	err = of_property_read_u32(child, "pulses-per-revolution", &val);
+	if (err) {
+		dev_err(dev, "missing pulses-per-revolution property of %pOFn\n", child);
+		return err;
+	}
+
+	if (val < 0 || val > 5) {
+		dev_err(dev, "invalid pulses-per-revolution %d of %pOFn\n", val, child);
+		return -EINVAL;
+	}
+	data->ppr[i] = val;
+
+	err = of_property_read_u32(child, "max-rpm", &maxrpm);
+	if (err) {
+		dev_err(dev, "missing max-rpm property of %pOFn\n", child);
+		return err;
+	}
+
+	err = rpm_range_to_index(dev, &data->rpm_range[i], maxrpm);
+	if (err) {
+		dev_err(dev, "invalid max-rpm %d of %pOFn\n", maxrpm, child);
+		return err;
+	}
+
+	err = of_property_read_u32(child, "target-rpm", &val);
+	/* Convert to PWM from provided target RPM */
+	if (!err && val != 0 && maxrpm > 0)
+		data->pwm[i] =
+			(u8)(val * 120 / maxrpm);
+
+	data->pwm_polarity[i] =  of_property_read_bool(child, "pwm-polarity-inverse");
+
+	return 0;
+}
+static int max6639_probe_from_dt(struct i2c_client *client, struct max6639_data *data)
+{
+	struct device *dev = &client->dev;
+	const struct device_node *np = dev->of_node;
+	struct device_node *child;
+	int err;
+
+	/* Compatible with non-DT platforms */
+	if (!np)
+		return 0;
+
+	for_each_child_of_node(np, child) {
+		if (strcmp(child->name, "fan"))
+			continue;
+
+		err = max6639_probe_child_from_dt(client, child, data);
+		if (err) {
+			of_node_put(child);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int max6639_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct max6639_data *data;
 	struct device *hwmon_dev;
-	int err;
+	int err, i;
 
 	data = devm_kzalloc(dev, sizeof(struct max6639_data), GFP_KERNEL);
 	if (!data)
@@ -539,9 +607,11 @@ static int max6639_probe(struct i2c_client *client)
 
 	data->reg = devm_regulator_get_optional(dev, "fan");
 	if (IS_ERR(data->reg)) {
-		if (PTR_ERR(data->reg) != -ENODEV)
-			return PTR_ERR(data->reg);
-
+		if (PTR_ERR(data->reg) != -ENODEV) {
+			err = (int)PTR_ERR(data->reg);
+			dev_warn(dev, "Failed looking up fan supply: %d\n", err);
+			return err;
+		}
 		data->reg = NULL;
 	} else {
 		/* Spin up fans */
@@ -560,6 +630,25 @@ static int max6639_probe(struct i2c_client *client)
 
 	mutex_init(&data->update_lock);
 
+	/* Below are defaults leter overridden by DT properties */
+	for (i = 0; i < 2; i++) {
+		/* 4000 RPM */
+		data->rpm_range[i] = 1;
+		data->ppr[i] = 2;
+		data->pwm_polarity[i] = 0;
+		/* Max. temp. 80C/90C/100C */
+		data->temp_therm[i] = 80;
+		data->temp_alert[i] = 90;
+		data->temp_ot[i] = 100;
+		/* PWM 120/120 (i.e. 100%) */
+		data->pwm[i] = 120;
+	}
+
+	/* Probe from DT to get configuration */
+	err = max6639_probe_from_dt(client, data);
+	if (err)
+		return err;
+
 	/* Initialize the max6639 chip */
 	err = max6639_init_client(client, data);
 	if (err < 0)
@@ -571,6 +660,7 @@ static int max6639_probe(struct i2c_client *client)
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
+#if IS_ENABLED(CONFIG_PM_SLEEP)
 static int max6639_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -608,6 +698,7 @@ static int max6639_resume(struct device *dev)
 	return i2c_smbus_write_byte_data(client,
 			MAX6639_REG_GCONFIG, ret & ~MAX6639_GCONFIG_STANDBY);
 }
+#endif
 
 static const struct i2c_device_id max6639_id[] = {
 	{"max6639", 0},
@@ -616,13 +707,22 @@ static const struct i2c_device_id max6639_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, max6639_id);
 
-static DEFINE_SIMPLE_DEV_PM_OPS(max6639_pm_ops, max6639_suspend, max6639_resume);
+#ifdef CONFIG_OF
+static const struct of_device_id maxim_of_platform_match[] = {
+	{.compatible = "maxim,max6639"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, maxim_of_platform_match);
+#endif
+
+static SIMPLE_DEV_PM_OPS(max6639_pm_ops, max6639_suspend, max6639_resume);
 
 static struct i2c_driver max6639_driver = {
 	.class = I2C_CLASS_HWMON,
 	.driver = {
 		   .name = "max6639",
 		   .pm = pm_sleep_ptr(&max6639_pm_ops),
+		   .of_match_table = of_match_ptr(maxim_of_platform_match),
 		   },
 	.probe_new = max6639_probe,
 	.id_table = max6639_id,
-- 
2.37.3

Re: [PATCH v4 0/3] Add devicetree support for max6639

[Naresh Solanki]

In this latest patch series V4 , I've addressed the comments of V3.
Since this is merely an upgrade of driver from platform driver to dt
support, can we get this in.

Thanks,
Naresh

Regards,
Naresh Solanki



9elements GmbH, Kortumstraße 19-21, 44787 Bochum, Germany
Email:  naresh.solanki@9elements.com
Mobile:  +91 9538631477

Sitz der Gesellschaft: Bochum
Handelsregister: Amtsgericht Bochum, HRB 17519
Geschäftsführung: Sebastian Deutsch, Eray Basar

Datenschutzhinweise nach Art. 13 DSGVO


On Thu, 13 Oct 2022 at 15:18, Naresh Solanki
<naresh.solanki@9elements.com> wrote:
>
> These patches adds devicetree support for MAX6639.
>
> Changes V4:
> - Fix dt error
> - update comment
> Changes V3:
> - correct fan dt property name
> - remove unrelevent changes
> Changes V2:
> - Fix dt schema error.
> Changes:
> - Add fan-common dt schema.
> - add dt-binding support for max6639
> - add max6639 specific property
>
> Marcello Sylvester Bauer (1):
>   dt-bindings: hwmon: Add binding for max6639
>
> Naresh Solanki (2):
>   dt-bindings: hwmon: fan: Add fan binding to schema
>   hwmon: (max6639) Change from pdata to dt configuration
>
>  .../devicetree/bindings/hwmon/fan-common.yaml |  48 ++++
>  .../bindings/hwmon/maxim,max6639.yaml         |  86 ++++++++
>  drivers/hwmon/max6639.c                       | 206 +++++++++++++-----
>  3 files changed, 287 insertions(+), 53 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/fan-common.yaml
>  create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
>
>
> base-commit: 0cf46a653bdae56683fece68dc50340f7520e6c4
> --
> 2.37.3
>

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Rob Herring]

On Thu, Oct 13, 2022 at 11:48:36AM +0200, Naresh Solanki wrote:
> Add common fan properties bindings to a schema.
> 
> Bindings for fan controllers can reference the common schema for the
> fan
> 
> child nodes:
> 
>   patternProperties:
>     "^fan@[0-2]":
>       type: object
>       $ref: fan-common.yaml#
> 
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
>  .../devicetree/bindings/hwmon/fan-common.yaml | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/fan-common.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/fan-common.yaml b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
> new file mode 100644
> index 000000000000..224f5013c93f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/fan-common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common fan properties
> +
> +maintainers:
> +  - Naresh Solanki <naresh.solanki@9elements.com>
> +
> +properties:
> +  max-rpm:
> +    description:
> +      Max RPM supported by fan.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  pulses-per-revolution:
> +    description:
> +      The number of pulse from fan sensor per revolution.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  default-rpm:
> +    description:
> +      Target RPM the fan should be configured during driver probe.

So if we unload and reload the driver module, it should go back to the 
default? 

I think it is really, 'target RPM if not already configured' which could 
be keep the setting from a register (e.g. what the bootloader set) or 
perhaps you already have temperature information to use...

> +    $ref: /schemas/types.yaml#/definitions/uint32

> +  pwm-frequency:
> +    description:
> +      PWM frequency for fan in Hertz(Hz).
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  pwm-polarity-inverse:
> +    description:
> +      Inverse PWM polarity for fan.
> +    type: boolean

As I said before, the PWM binding handles these 2 settings. Use it. Yes, 
it's a bit of an overkill when the child is the consumer of the parent. 
Until some 'clever' h/w engineer decides to use one of the PWMs for 
something else like a backlight.

Rob

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Naresh Solanki]

On 24-10-2022 09:48 pm, Rob Herring wrote:
> So if we unload and reload the driver module, it should go back to the
> default?
This is RPM to be set during probe if desired.
> 
> I think it is really, 'target RPM if not already configured' which could
> be keep the setting from a register (e.g. what the bootloader set) or
> perhaps you already have temperature information to use...
Yes. missed it. It should be target-rpm will correct this. in next version.
> 
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +  pwm-frequency:
>> +    description:
>> +      PWM frequency for fan in Hertz(Hz).
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  pwm-polarity-inverse:
>> +    description:
>> +      Inverse PWM polarity for fan.
>> +    type: boolean
> As I said before, the PWM binding handles these 2 settings. Use it. Yes,
> it's a bit of an overkill when the child is the consumer of the parent.
> Until some 'clever' h/w engineer decides to use one of the PWMs for
> something else like a backlight.
I would like you to consider this as something recommended by fan 
datasheet for the given fan instance.
This info can be used by fan controller driver to configure PWM 
source/provider accordingly.

If you still feel that may not make sense then I'll remove this property.


Thanks,
Naresh

Re: [PATCH v4 0/3] Add devicetree support for max6639

[Guenter Roeck]

On Sun, Oct 23, 2022 at 11:23:42AM +0530, Naresh Solanki wrote:
> In this latest patch series V4 , I've addressed the comments of V3.
> Since this is merely an upgrade of driver from platform driver to dt
> support, can we get this in.
> 

This is not correct. You did not address many of my comments.

Guenter

Re: [PATCH v4 0/3] Add devicetree support for max6639

[Naresh Solanki]

Hi Guenter,

On 25-10-2022 11:31 pm, Guenter Roeck wrote:
> You did not address many of my comments.
I've update comment to explain defaults. Thus retaining previous behaviour.
This patch series is about incremental change just to make it DT compatible.

Thanks,
Naresh

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Rob Herring]

On Tue, Oct 25, 2022 at 4:16 AM Naresh Solanki
<naresh.solanki@9elements.com> wrote:
>
>
>
> On 24-10-2022 09:48 pm, Rob Herring wrote:
> > So if we unload and reload the driver module, it should go back to the
> > default?
> This is RPM to be set during probe if desired.
> >
> > I think it is really, 'target RPM if not already configured' which could
> > be keep the setting from a register (e.g. what the bootloader set) or
> > perhaps you already have temperature information to use...
> Yes. missed it. It should be target-rpm will correct this. in next version.
> >
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +  pwm-frequency:
> >> +    description:
> >> +      PWM frequency for fan in Hertz(Hz).
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +
> >> +  pwm-polarity-inverse:
> >> +    description:
> >> +      Inverse PWM polarity for fan.
> >> +    type: boolean
> > As I said before, the PWM binding handles these 2 settings. Use it. Yes,
> > it's a bit of an overkill when the child is the consumer of the parent.
> > Until some 'clever' h/w engineer decides to use one of the PWMs for
> > something else like a backlight.
> I would like you to consider this as something recommended by fan
> datasheet for the given fan instance.
> This info can be used by fan controller driver to configure PWM
> source/provider accordingly.
>
> If you still feel that may not make sense then I'll remove this property.

You evidently don't understand my comments. My suggestion is to do this:

fanc: fan-controller {
  #pwm-cells = <3>;
  ...

  fan {
    pwms = <&fanc 0 500000  PWM_POLARITY_INVERTED>;
    ...
  };
};

0 is PWM number and 500000 is the PWM frequency. The 3rd cell are per
consumer flags. See pwm.txt for more details.

Rob

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Naresh Solanki]

Hi Rob,

On 26-10-2022 07:07 pm, Rob Herring wrote:
> fanc: fan-controller {
>    #pwm-cells = <3>;
>    ...
> 
>    fan {
>      pwms = <&fanc 0 500000  PWM_POLARITY_INVERTED>;
>      ...
>    };
> };
> 
> 0 is PWM number and 500000 is the PWM frequency. The 3rd cell are per
> consumer flags. See pwm.txt for more details.

Did the implementation & while testing getting the below err:
[63.626505] max6639 166-002e: failed to create device link to 166-002e

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Rob Herring]

On Mon, Oct 31, 2022 at 01:35:09PM +0530, Naresh Solanki wrote:
> Hi Rob,
> 
> On 26-10-2022 07:07 pm, Rob Herring wrote:
> > fanc: fan-controller {
> >    #pwm-cells = <3>;
> >    ...
> > 
> >    fan {
> >      pwms = <&fanc 0 500000  PWM_POLARITY_INVERTED>;
> >      ...
> >    };
> > };
> > 
> > 0 is PWM number and 500000 is the PWM frequency. The 3rd cell are per
> > consumer flags. See pwm.txt for more details.
> 
> Did the implementation & while testing getting the below err:
> [63.626505] max6639 166-002e: failed to create device link to 166-002e

Does turning off fw_devlink help (fw_devlink=off)?

Rob

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Naresh Solanki]

Hi Rob,

On 02-11-2022 12:14 am, Rob Herring wrote:
> Does turning off fw_devlink help (fw_devlink=off)?
This didn't bring any difference for the error.
Failing due to same consumer & supplier.
Returning from here:
https://github.com/torvalds/linux/blob/master/drivers/base/core.c#L702
Also this can cause return:
https://github.com/torvalds/linux/blob/master/drivers/base/core.c#L732

Regards,
Naresh

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Naresh Solanki]

Hi Rob,

On 02-11-2022 12:14 am, Rob Herring wrote:
> On Mon, Oct 31, 2022 at 01:35:09PM +0530, Naresh Solanki wrote:
>> Hi Rob,
>>
>> On 26-10-2022 07:07 pm, Rob Herring wrote:
>>> fanc: fan-controller {
>>>     #pwm-cells = <3>;
>>>     ...
>>>
>>>     fan {
>>>       pwms = <&fanc 0 500000  PWM_POLARITY_INVERTED>;
>>>       ...
>>>     };
>>> };
>>>
>>> 0 is PWM number and 500000 is the PWM frequency. The 3rd cell are per
>>> consumer flags. See pwm.txt for more details.
>>
>> Did the implementation & while testing getting the below err:
>> [63.626505] max6639 166-002e: failed to create device link to 166-002e
> 
> Does turning off fw_devlink help (fw_devlink=off)?

Will supplier == consumer, device link creation fails.
Not sure what is best approach but not creating device link in this 
scenario help & for that below additional changes needed in pwm core.

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 4527f09a5c50..afea51c49138 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -730,6 +730,12 @@ static struct device_link 
*pwm_device_link_add(struct device *dev,
  		return NULL;
  	}

+	/*
+	 * Do not attempt to create link if consumer itself is supplier.
+	 */
+	if (dev == pwm->chip->dev)
+		return 0;
+
  	dl = device_link_add(dev, pwm->chip->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
  	if (!dl) {
  		dev_err(dev, "failed to create device link to %s\n",



Regards,
Naresh