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

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

[Naresh Solanki]

These patches adds devicetree support for MAX6639.

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 (2):
  dt-bindings: hwmon: Add binding for max6639
  hwmon: (max6639) Change from pdata to dt configuration

Naresh Solanki (1):
  dt-bindings: hwmon: fan: Add fan binding to schema

 .../devicetree/bindings/hwmon/fan-common.yaml |  80 ++++++
 .../bindings/hwmon/maxim,max6639.yaml         | 116 ++++++++
 drivers/hwmon/max6639.c                       | 255 +++++++++++++-----
 include/linux/platform_data/max6639.h         |  15 --
 4 files changed, 388 insertions(+), 78 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/fan-common.yaml
 create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
 delete mode 100644 include/linux/platform_data/max6639.h


base-commit: 0cf46a653bdae56683fece68dc50340f7520e6c4
-- 
2.37.3

[PATCH v2 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
      allOf:
        - $ref: fan-common.yaml#

Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 .../devicetree/bindings/hwmon/fan-common.yaml | 80 +++++++++++++++++++
 1 file changed, 80 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..abc8375da646
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%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
+
+  pulse-per-revolution:
+    description:
+      The number of pulse from fan sensor per revolution.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  target-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.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  pwm-polarity-inverse:
+    description:
+      PWM polarity for fan.
+    type: boolean
+
+  label:
+    description:
+      Optional fan label
+    $ref: /schemas/types.yaml#/definitions/string
+
+additionalProperties: true
+
+examples:
+  - |
+
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        fan-controller@30 {
+            compatible = "maxim,max6639";
+            reg = <0x30>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            fan@0 {
+                reg = <0>;
+                label = "CPU0_Fan";
+                max-rpm = <32000>;
+                pulse-per-revolution = <2>;
+                target-rpm = <2000>;
+                pwm-frequency = <25000>;
+            };
+
+            fan@1 {
+                reg = <1>;
+                label = "PCIe0_Fan";
+                max-rpm = <32000>;
+                pulse-per-revolution = <2>;
+                target-rpm = <2000>;
+                pwm-frequency = <25000>;
+            };
+
+        };
+    };
+
+...

base-commit: 0cf46a653bdae56683fece68dc50340f7520e6c4
-- 
2.37.3

[PATCH v2 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         | 116 ++++++++++++++++++
 1 file changed, 116 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..bbefb0a57ab3
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
@@ -0,0 +1,116 @@
+# 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:
+  "^ot[0-1]_indication$":
+    type: boolean
+    default: false
+    description:
+      If true then enable OT pin indication.
+
+  "^therm[0-1]_indication$":
+    type: boolean
+    default: false
+    description:
+      If true then enable THERM pin indication.
+
+  "^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
+
+      maxim,fan-spin-up:
+        type: boolean
+        description:
+          If true then whnever the fan starts up from zero drive, it
+          is driven with 100% duty cycle for 2s to ensure that it
+          starts.
+
+      maxim,full-speed-on-therm:
+        type: boolean
+        description:
+          If true then force fan to full speed if THERM pin goes low.
+
+      maxim,fanfail_indication:
+        type: boolean
+        description:
+          If true then enable fanfail pin indication.
+    required:
+      - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      max6639@10 {
+        compatible = "maxim,max6639";
+        reg = <0x10>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        fan@0 {
+          reg = <0x0>;
+          pulses-per-revolution = <2>;
+          max-rpm = <4000>;
+          pwm-frequency = <25000>;
+        };
+
+        fan@1 {
+          reg = <0x1>;
+          pulses-per-revolution = <2>;
+          max-rpm = <32000>;
+          pwm-frequency = <25000>;
+        };
+      };
+    };
+...
-- 
2.37.3

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

[Naresh Solanki]

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

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

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

Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/max6639.c               | 260 +++++++++++++++++++-------
 include/linux/platform_data/max6639.h |  15 --
 2 files changed, 197 insertions(+), 78 deletions(-)
 delete mode 100644 include/linux/platform_data/max6639.h

diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
index 9b895402c80d..75eb4522fc9b 100644
--- a/drivers/hwmon/max6639.c
+++ b/drivers/hwmon/max6639.c
@@ -19,48 +19,53 @@
 #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 };
 
 /* The MAX6639 registers, valid channel numbers: 0, 1 */
 #define MAX6639_REG_TEMP(ch)			(0x00 + (ch))
-#define MAX6639_REG_STATUS			0x02
+#define MAX6639_REG_STATUS				0x02
 #define MAX6639_REG_OUTPUT_MASK			0x03
-#define MAX6639_REG_GCONFIG			0x04
+#define MAX6639_REG_GCONFIG				0x04
 #define MAX6639_REG_TEMP_EXT(ch)		(0x05 + (ch))
 #define MAX6639_REG_ALERT_LIMIT(ch)		(0x08 + (ch))
 #define MAX6639_REG_OT_LIMIT(ch)		(0x0A + (ch))
 #define MAX6639_REG_THERM_LIMIT(ch)		(0x0C + (ch))
 #define MAX6639_REG_FAN_CONFIG1(ch)		(0x10 + (ch) * 4)
-#define MAX6639_REG_FAN_CONFIG2a(ch)		(0x11 + (ch) * 4)
-#define MAX6639_REG_FAN_CONFIG2b(ch)		(0x12 + (ch) * 4)
+#define MAX6639_REG_FAN_CONFIG2a(ch)	(0x11 + (ch) * 4)
+#define MAX6639_REG_FAN_CONFIG2b(ch)	(0x12 + (ch) * 4)
 #define MAX6639_REG_FAN_CONFIG3(ch)		(0x13 + (ch) * 4)
 #define MAX6639_REG_FAN_CNT(ch)			(0x20 + (ch))
 #define MAX6639_REG_TARGET_CNT(ch)		(0x22 + (ch))
 #define MAX6639_REG_FAN_PPR(ch)			(0x24 + (ch))
 #define MAX6639_REG_TARGTDUTY(ch)		(0x26 + (ch))
-#define MAX6639_REG_FAN_START_TEMP(ch)		(0x28 + (ch))
-#define MAX6639_REG_DEVID			0x3D
-#define MAX6639_REG_MANUID			0x3E
-#define MAX6639_REG_DEVREV			0x3F
+#define MAX6639_REG_FAN_START_TEMP(ch)	(0x28 + (ch))
+#define MAX6639_REG_DEVID				0x3D
+#define MAX6639_REG_MANUID				0x3E
+#define MAX6639_REG_DEVREV				0x3F
 
 /* Register bits */
+#define MAX6639_REG_OUTPUT_MASK_OT(x, ch)		(x << (5 - ch))
+#define MAX6639_REG_OUTPUT_MASK_THERM(x, ch)	(x << (3 - ch))
+#define MAX6639_REG_OUTPUT_MASK_FANFAIL(x, ch)	(x << (1 - ch))
 #define MAX6639_GCONFIG_STANDBY			0x80
-#define MAX6639_GCONFIG_POR			0x40
-#define MAX6639_GCONFIG_DISABLE_TIMEOUT		0x20
+#define MAX6639_GCONFIG_POR				0x40
+#define MAX6639_GCONFIG_DISABLE_TIMEOUT	0x20
 #define MAX6639_GCONFIG_CH2_LOCAL		0x10
 #define MAX6639_GCONFIG_PWM_FREQ_HI		0x08
 
 #define MAX6639_FAN_CONFIG1_PWM			0x80
 
+#define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED	0x40
+#define MAX6639_FAN_CONFIG3_SPIN_UP_DISABLE		0x80
 #define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED	0x40
 
 static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
 
 #define FAN_FROM_REG(val, rpm_range)	((val) == 0 || (val) == 255 ? \
 				0 : (rpm_ranges[rpm_range] * 30) / (val))
+
 #define TEMP_LIMIT_TO_REG(val)	clamp_val((val) / 1000, 0, 255)
 
 /*
@@ -69,7 +74,7 @@ static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
 struct max6639_data {
 	struct i2c_client *client;
 	struct mutex update_lock;
-	bool valid;		/* true if following fields are valid */
+	char valid;		/* !=0 if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
 
 	/* Register values sampled regularly */
@@ -85,9 +90,14 @@ 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 */
+	bool full_speed_on_therm[2];	/* disable THERM full speed assertion */
+	bool spin_up_enable[2];		/* Enable fan spin-up if fan is not spinning */
+	bool ot_indication[2];		/* Enable OT pin indication */
+	bool therm_indication[2];	/* Enable THERM pin indication */
+	bool fan_fail_indication[2];	/* Enable FANFAIL pin indication */
 	/* Optional regulator for FAN supply */
 	struct regulator *reg;
 };
@@ -144,7 +154,7 @@ static struct max6639_data *max6639_update_device(struct device *dev)
 		}
 
 		data->last_updated = jiffies;
-		data->valid = true;
+		data->valid = 1;
 	}
 abort:
 	mutex_unlock(&data->update_lock);
@@ -319,7 +329,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,28 +396,34 @@ 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(u8 *index, int rpm)
 {
 	int i;
 
+	if (rpm <= 0)
+		return -EINVAL;
+
+	/* If provided RPM is more than 16000 RPM then select 16000
+	 * RPM range.
+	 */
+	*index = ARRAY_SIZE(rpm_ranges) - 1;
+
 	for (i = 0; i < ARRAY_SIZE(rpm_ranges); i++) {
-		if (rpm_ranges[i] == range)
-			return i;
+		if (rpm <= rpm_ranges[i]) {
+			*index = i;
+			break;
+		}
 	}
 
-	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 i, val;
 	int err;
 
 	/* Reset chip to default values, see below for GCONFIG setup */
@@ -416,58 +432,40 @@ 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,
+		 * Full speed on therm, spin-up at zero rpm.
 		 * 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);
+			(data->full_speed_on_therm[i] ?
+			 MAX6639_FAN_CONFIG3_THERM_FULL_SPEED : 0) |
+			(data->spin_up_enable[i] ?
+			 0 : MAX6639_FAN_CONFIG3_SPIN_UP_DISABLE) | 0x03);
+
 		if (err)
 			goto exit;
 
-		/* Max. temp. 80C/90C/100C */
-		data->temp_therm[i] = 80;
-		data->temp_alert[i] = 90;
-		data->temp_ot[i] = 100;
 		err = i2c_smbus_write_byte_data(client,
 				MAX6639_REG_THERM_LIMIT(i),
 				data->temp_therm[i]);
@@ -483,13 +481,24 @@ 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)
 			goto exit;
 	}
+
+	val = 0;
+	for (i = 0; i < 2; i++) {
+		val |= MAX6639_REG_OUTPUT_MASK_OT(
+				data->ot_indication[i], i);
+		val |= MAX6639_REG_OUTPUT_MASK_THERM(
+				data->therm_indication[i], i);
+		val |= MAX6639_REG_OUTPUT_MASK_FANFAIL(
+				data->fan_fail_indication[i], i);
+	}
+	/* Configure output mask register */
+	err = i2c_smbus_write_byte_data(client, MAX6639_REG_OUTPUT_MASK, val);
+
 	/* Start monitoring */
 	err = i2c_smbus_write_byte_data(client, MAX6639_REG_GCONFIG,
 		MAX6639_GCONFIG_DISABLE_TIMEOUT | MAX6639_GCONFIG_CH2_LOCAL |
@@ -524,12 +533,105 @@ 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(&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)
+		data->pwm[i] =
+			(u8)(val * 255 / maxrpm);
+
+	data->pwm_polarity[i] =  of_property_read_bool(child, "pwm-polarity-inverse");
+
+	data->spin_up_enable[i] = of_property_read_bool(child, "maxim,fan-spin-up");
+
+	data->full_speed_on_therm[i] = of_property_read_bool(child,
+		"maxim,full-speed-on-therm");
+
+	data->fan_fail_indication[i] = of_property_read_bool(child, "maxim,fanfail_indication");
+
+	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;
+		}
+	}
+
+	data->ot_indication[0] = !of_property_read_bool(np, "ot0_indication");
+	data->ot_indication[1] = !of_property_read_bool(np, "ot1_indication");
+	data->therm_indication[0] = !of_property_read_bool(np, "therm0_indication");
+	data->therm_indication[1] = !of_property_read_bool(np, "therm1_indication");
+
+	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 +641,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 +664,25 @@ static int max6639_probe(struct i2c_client *client)
 
 	mutex_init(&data->update_lock);
 
+	/* default values */
+	for (i = 0; i < 2; i++) {
+		/* 4000 RPM */
+		data->rpm_range[i] = 1;
+		data->ppr[i] = 2;
+		data->pwm_polarity[i] = 1;
+		/* 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;
+		data->full_speed_on_therm[i] = false;
+	}
+
+	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 +694,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 +732,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 +741,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,
diff --git a/include/linux/platform_data/max6639.h b/include/linux/platform_data/max6639.h
deleted file mode 100644
index 65bfdb4fdc15..000000000000
--- a/include/linux/platform_data/max6639.h
+++ /dev/null
@@ -1,15 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _LINUX_MAX6639_H
-#define _LINUX_MAX6639_H
-
-#include <linux/types.h>
-
-/* platform data for the MAX6639 temperature sensor and fan control */
-
-struct max6639_platform_data {
-	bool pwm_polarity;	/* Polarity low (0) or high (1, default) */
-	int ppr;		/* Pulses per rotation 1..4 (default == 2) */
-	int rpm_range;		/* 2000, 4000 (default), 8000 or 16000 */
-};
-
-#endif /* _LINUX_MAX6639_H */
-- 
2.37.3

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

[Guenter Roeck]

On 10/11/22 03:47, 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
>        allOf:
>          - $ref: fan-common.yaml#
> 
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
>   .../devicetree/bindings/hwmon/fan-common.yaml | 80 +++++++++++++++++++
>   1 file changed, 80 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..abc8375da646
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%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
> +
> +  pulse-per-revolution:
> +    description:
> +      The number of pulse from fan sensor per revolution.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  target-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.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  pwm-polarity-inverse:
> +    description:
> +      PWM polarity for fan.
> +    type: boolean
> +
> +  label:
> +    description:
> +      Optional fan label
> +    $ref: /schemas/types.yaml#/definitions/string
> +

Same question as before:

How are additional common bindings, such as min-rpm or fan-divider
(also sometimes called fan-prescale) supposed to be handled ?
As additions to this schema, or individually in each driver needing/
using them ?

Thanks,
Guenter

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

[Naresh Solanki]

Hi Guenter,

fan-common is intended for fan properties. i.e., those derived from
fan datasheets.
For min-rpm, some fans have minimum non zero rpm like 1900rpm below which
the fan cannot run.

But not sure what the best approach is but for chip specific setting
it should be in
chip specific DT schema. Suggestion?

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 Tue, 11 Oct 2022 at 20:30, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/11/22 03:47, 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
> >        allOf:
> >          - $ref: fan-common.yaml#
> >
> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > ---
> >   .../devicetree/bindings/hwmon/fan-common.yaml | 80 +++++++++++++++++++
> >   1 file changed, 80 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..abc8375da646
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%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
> > +
> > +  pulse-per-revolution:
> > +    description:
> > +      The number of pulse from fan sensor per revolution.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  target-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.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  pwm-polarity-inverse:
> > +    description:
> > +      PWM polarity for fan.
> > +    type: boolean
> > +
> > +  label:
> > +    description:
> > +      Optional fan label
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +
>
> Same question as before:
>
> How are additional common bindings, such as min-rpm or fan-divider
> (also sometimes called fan-prescale) supposed to be handled ?
> As additions to this schema, or individually in each driver needing/
> using them ?
>
> Thanks,
> Guenter

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

[Krzysztof Kozlowski]

On 11/10/2022 06:47, 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
>       allOf:
>         - $ref: fan-common.yaml#
> 
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
>  .../devicetree/bindings/hwmon/fan-common.yaml | 80 +++++++++++++++++++
>  1 file changed, 80 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..abc8375da646
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/fan-common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common fan properties

Is a fan a hardware monitoring device? Maybe this should not be called a
fan?

> +
> +maintainers:
> +  - Naresh Solanki <naresh.solanki@9elements.com>
> +
> +properties:
> +  max-rpm:
> +    description:
> +      Max RPM supported by fan
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  pulse-per-revolution:
> +    description:
> +      The number of pulse from fan sensor per revolution.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  target-rpm:
> +    description:
> +      Target RPM the fan should be configured during driver probe.

I think target depends on conditions, e.g. it is rarely one target.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  pwm-frequency:
> +    description:
> +      PWM frequency for fan.
> +    $ref: /schemas/types.yaml#/definitions/uint32

Use common units, so -hz

However I wonder if frequency is appropriate here - I thought PWMs are
rather configured via duty cycles.

> +
> +  pwm-polarity-inverse:
> +    description:
> +      PWM polarity for fan.

Rather: Inversed PWM polarity for the fan.

> +    type: boolean
> +
> +  label:
> +    description:
> +      Optional fan label
> +    $ref: /schemas/types.yaml#/definitions/string

Ref is not needed, core brings it.

> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +
> +

Drop unneeded empty lines.

> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        fan-controller@30 {
> +            compatible = "maxim,max6639";
> +            reg = <0x30>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            fan@0 {
> +                reg = <0>;
> +                label = "CPU0_Fan";
> +                max-rpm = <32000>;
> +                pulse-per-revolution = <2>;
> +                target-rpm = <2000>;
> +                pwm-frequency = <25000>;
> +            };
> +
> +            fan@1 {
> +                reg = <1>;
> +                label = "PCIe0_Fan";
> +                max-rpm = <32000>;
> +                pulse-per-revolution = <2>;
> +                target-rpm = <2000>;
> +                pwm-frequency = <25000>;
> +            };
> +

Drop unneeded empty lines.

> +        };
> +    };
> +
> +...
> 
> base-commit: 0cf46a653bdae56683fece68dc50340f7520e6c4

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 11/10/2022 06:47, Naresh Solanki wrote:
> 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         | 116 ++++++++++++++++++
>  1 file changed, 116 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..bbefb0a57ab3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
> @@ -0,0 +1,116 @@
> +# 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:
> +  "^ot[0-1]_indication$":

No underscores in names.

> +    type: boolean
> +    default: false
> +    description:
> +      If true then enable OT pin indication.

Description copies the name of property. Not really useful. Describe
that it does.

Why this has 0 and 1 numbers? Isn't it connected with fan?

> +
> +  "^therm[0-1]_indication$":
> +    type: boolean
> +    default: false
> +    description:
> +      If true then enable THERM pin indication.

Ditto

> +
> +  "^fan@[0-1]$":

[01]
The same in other cases.

> +    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
> +
> +      maxim,fan-spin-up:
> +        type: boolean
> +        description:
> +          If true then whnever the fan starts up from zero drive, it

whenever
run spell-check

> +          is driven with 100% duty cycle for 2s to ensure that it
> +          starts.
> +
> +      maxim,full-speed-on-therm:
> +        type: boolean
> +        description:
> +          If true then force fan to full speed if THERM pin goes low.
> +
> +      maxim,fanfail_indication:

No underscores

> +        type: boolean
> +        description:
> +          If true then enable fanfail pin indication.

Missing blank line

> +    required:
> +      - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      max6639@10 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +        compatible = "maxim,max6639";
> +        reg = <0x10>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        fan@0 {
> +          reg = <0x0>;
> +          pulses-per-revolution = <2>;
> +          max-rpm = <4000>;
> +          pwm-frequency = <25000>;
> +        };
> +
> +        fan@1 {
> +          reg = <0x1>;
> +          pulses-per-revolution = <2>;
> +          max-rpm = <32000>;
> +          pwm-frequency = <25000>;
> +        };
> +      };
> +    };
> +...

Best regards,
Krzysztof

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

[Guenter Roeck]

On 10/11/22 09:12, Naresh Solanki wrote:
> Hi Guenter,
> 
> fan-common is intended for fan properties. i.e., those derived from
> fan datasheets.
> For min-rpm, some fans have minimum non zero rpm like 1900rpm below which
> the fan cannot run.
> 

I would argue the properties are for fan controllers, not for fans.
Fans don't have or depend on specific pwm frequencies. Fan controllers
do. Fans don't have a configurable pwm polarity. Fan controllers do,
to match the hardware on a board. Fans don't have or rely on
a target rpm. That is a system property, configured into the
fan controller. And so on.

If this is for fans, we'll need another set of properties for
fan controllers. The driver for max6639, being a fan controller,
would need to implement those properties.

Also, as implemented in the MAX6639, max-rpm is the fan's
rpm range, not the actual rpm. Your implementation is just
confusing, including the example in the bindings. Valid values
should be what the chip accepts, not some random value.

Really, I don't understand where you are going with this.

Guenter

> But not sure what the best approach is but for chip specific setting
> it should be in
> chip specific DT schema. Suggestion?
> 
> 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 Tue, 11 Oct 2022 at 20:30, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 10/11/22 03:47, 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
>>>         allOf:
>>>           - $ref: fan-common.yaml#
>>>
>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>>> ---
>>>    .../devicetree/bindings/hwmon/fan-common.yaml | 80 +++++++++++++++++++
>>>    1 file changed, 80 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..abc8375da646
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
>>> @@ -0,0 +1,80 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +%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
>>> +
>>> +  pulse-per-revolution:
>>> +    description:
>>> +      The number of pulse from fan sensor per revolution.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +
>>> +  target-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.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +
>>> +  pwm-polarity-inverse:
>>> +    description:
>>> +      PWM polarity for fan.
>>> +    type: boolean
>>> +
>>> +  label:
>>> +    description:
>>> +      Optional fan label
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +
>>
>> Same question as before:
>>
>> How are additional common bindings, such as min-rpm or fan-divider
>> (also sometimes called fan-prescale) supposed to be handled ?
>> As additions to this schema, or individually in each driver needing/
>> using them ?
>>
>> Thanks,
>> Guenter

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

[Guenter Roeck]

On 10/11/22 09:28, Krzysztof Kozlowski wrote:
> On 11/10/2022 06:47, Naresh Solanki wrote:
>> 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         | 116 ++++++++++++++++++
>>   1 file changed, 116 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..bbefb0a57ab3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
>> @@ -0,0 +1,116 @@
>> +# 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:
>> +  "^ot[0-1]_indication$":
> 
> No underscores in names.
> 
>> +    type: boolean
>> +    default: false
>> +    description:
>> +      If true then enable OT pin indication.
> 
> Description copies the name of property. Not really useful. Describe
> that it does.
> 
> Why this has 0 and 1 numbers? Isn't it connected with fan?
> 

I had to look up the suggested code to understand what it means.
All the _indication properties actually configure the chip
to enable or disable various alarm output pins (ALERT, OT,
THERM, and FANFAIL). I for my part find the therm "indication"
quite confusing.

Guenter

>> +
>> +  "^therm[0-1]_indication$":
>> +    type: boolean
>> +    default: false
>> +    description:
>> +      If true then enable THERM pin indication.
> 
> Ditto
> 
>> +
>> +  "^fan@[0-1]$":
> 
> [01]
> The same in other cases.
> 
>> +    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
>> +
>> +      maxim,fan-spin-up:
>> +        type: boolean
>> +        description:
>> +          If true then whnever the fan starts up from zero drive, it
> 
> whenever
> run spell-check
> 
>> +          is driven with 100% duty cycle for 2s to ensure that it
>> +          starts.
>> +
>> +      maxim,full-speed-on-therm:
>> +        type: boolean
>> +        description:
>> +          If true then force fan to full speed if THERM pin goes low.
>> +
>> +      maxim,fanfail_indication:
> 
> No underscores
> 
>> +        type: boolean
>> +        description:
>> +          If true then enable fanfail pin indication.
> 
> Missing blank line
> 
>> +    required:
>> +      - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      max6639@10 {
> 
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
>> +        compatible = "maxim,max6639";
>> +        reg = <0x10>;
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        fan@0 {
>> +          reg = <0x0>;
>> +          pulses-per-revolution = <2>;
>> +          max-rpm = <4000>;
>> +          pwm-frequency = <25000>;
>> +        };
>> +
>> +        fan@1 {
>> +          reg = <0x1>;
>> +          pulses-per-revolution = <2>;
>> +          max-rpm = <32000>;
>> +          pwm-frequency = <25000>;
>> +        };
>> +      };
>> +    };
>> +...
> 
> Best regards,
> Krzysztof
> 

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

[Guenter Roeck]

On 10/11/22 03:47, Naresh Solanki wrote:
> From: Marcello Sylvester Bauer <sylv@sylv.io>
> 
> max6639_platform_data is not used by any in-kernel driver and does not
> address the MAX6639 channels separately. Move to device tree
> configuration with explicit properties to configure each channel.
> 

This patch does way more than that. It introduces several additional
configuration options. The patch will have to be split into several patches,
each with a single logical change.

> Non-DT platform can still use this module with its default
> configuration.
> 
> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
>   drivers/hwmon/max6639.c               | 260 +++++++++++++++++++-------
>   include/linux/platform_data/max6639.h |  15 --
>   2 files changed, 197 insertions(+), 78 deletions(-)
>   delete mode 100644 include/linux/platform_data/max6639.h
> 
> diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
> index 9b895402c80d..75eb4522fc9b 100644
> --- a/drivers/hwmon/max6639.c
> +++ b/drivers/hwmon/max6639.c
> @@ -19,48 +19,53 @@
>   #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 };
>   
>   /* The MAX6639 registers, valid channel numbers: 0, 1 */
>   #define MAX6639_REG_TEMP(ch)			(0x00 + (ch))
> -#define MAX6639_REG_STATUS			0x02
> +#define MAX6639_REG_STATUS				0x02
>   #define MAX6639_REG_OUTPUT_MASK			0x03
> -#define MAX6639_REG_GCONFIG			0x04
> +#define MAX6639_REG_GCONFIG				0x04
>   #define MAX6639_REG_TEMP_EXT(ch)		(0x05 + (ch))
>   #define MAX6639_REG_ALERT_LIMIT(ch)		(0x08 + (ch))
>   #define MAX6639_REG_OT_LIMIT(ch)		(0x0A + (ch))
>   #define MAX6639_REG_THERM_LIMIT(ch)		(0x0C + (ch))
>   #define MAX6639_REG_FAN_CONFIG1(ch)		(0x10 + (ch) * 4)
> -#define MAX6639_REG_FAN_CONFIG2a(ch)		(0x11 + (ch) * 4)
> -#define MAX6639_REG_FAN_CONFIG2b(ch)		(0x12 + (ch) * 4)
> +#define MAX6639_REG_FAN_CONFIG2a(ch)	(0x11 + (ch) * 4)
> +#define MAX6639_REG_FAN_CONFIG2b(ch)	(0x12 + (ch) * 4)
>   #define MAX6639_REG_FAN_CONFIG3(ch)		(0x13 + (ch) * 4)
>   #define MAX6639_REG_FAN_CNT(ch)			(0x20 + (ch))
>   #define MAX6639_REG_TARGET_CNT(ch)		(0x22 + (ch))
>   #define MAX6639_REG_FAN_PPR(ch)			(0x24 + (ch))
>   #define MAX6639_REG_TARGTDUTY(ch)		(0x26 + (ch))
> -#define MAX6639_REG_FAN_START_TEMP(ch)		(0x28 + (ch))
> -#define MAX6639_REG_DEVID			0x3D
> -#define MAX6639_REG_MANUID			0x3E
> -#define MAX6639_REG_DEVREV			0x3F
> +#define MAX6639_REG_FAN_START_TEMP(ch)	(0x28 + (ch))
> +#define MAX6639_REG_DEVID				0x3D
> +#define MAX6639_REG_MANUID				0x3E
> +#define MAX6639_REG_DEVREV				0x3F
>   
>   /* Register bits */
> +#define MAX6639_REG_OUTPUT_MASK_OT(x, ch)		(x << (5 - ch))
> +#define MAX6639_REG_OUTPUT_MASK_THERM(x, ch)	(x << (3 - ch))
> +#define MAX6639_REG_OUTPUT_MASK_FANFAIL(x, ch)	(x << (1 - ch))
>   #define MAX6639_GCONFIG_STANDBY			0x80
> -#define MAX6639_GCONFIG_POR			0x40
> -#define MAX6639_GCONFIG_DISABLE_TIMEOUT		0x20
> +#define MAX6639_GCONFIG_POR				0x40
> +#define MAX6639_GCONFIG_DISABLE_TIMEOUT	0x20
>   #define MAX6639_GCONFIG_CH2_LOCAL		0x10
>   #define MAX6639_GCONFIG_PWM_FREQ_HI		0x08
>   
>   #define MAX6639_FAN_CONFIG1_PWM			0x80
>   
> +#define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED	0x40
> +#define MAX6639_FAN_CONFIG3_SPIN_UP_DISABLE		0x80
>   #define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED	0x40
>   
>   static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
>   
>   #define FAN_FROM_REG(val, rpm_range)	((val) == 0 || (val) == 255 ? \
>   				0 : (rpm_ranges[rpm_range] * 30) / (val))
> +
>   #define TEMP_LIMIT_TO_REG(val)	clamp_val((val) / 1000, 0, 255)
>   
>   /*
> @@ -69,7 +74,7 @@ static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
>   struct max6639_data {
>   	struct i2c_client *client;
>   	struct mutex update_lock;
> -	bool valid;		/* true if following fields are valid */
> +	char valid;		/* !=0 if following fields are valid */
>   	unsigned long last_updated;	/* In jiffies */
>   
>   	/* Register values sampled regularly */
> @@ -85,9 +90,14 @@ 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 */
> +	bool full_speed_on_therm[2];	/* disable THERM full speed assertion */
> +	bool spin_up_enable[2];		/* Enable fan spin-up if fan is not spinning */
> +	bool ot_indication[2];		/* Enable OT pin indication */
> +	bool therm_indication[2];	/* Enable THERM pin indication */
> +	bool fan_fail_indication[2];	/* Enable FANFAIL pin indication */
>   	/* Optional regulator for FAN supply */
>   	struct regulator *reg;
>   };
> @@ -144,7 +154,7 @@ static struct max6639_data *max6639_update_device(struct device *dev)
>   		}
>   
>   		data->last_updated = jiffies;
> -		data->valid = true;
> +		data->valid = 1;
>   	}
>   abort:
>   	mutex_unlock(&data->update_lock);
> @@ -319,7 +329,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,28 +396,34 @@ 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(u8 *index, int rpm)
>   {
>   	int i;
>   
> +	if (rpm <= 0)
> +		return -EINVAL;
> +
> +	/* If provided RPM is more than 16000 RPM then select 16000
> +	 * RPM range.
> +	 */
> +	*index = ARRAY_SIZE(rpm_ranges) - 1;
> +
>   	for (i = 0; i < ARRAY_SIZE(rpm_ranges); i++) {
> -		if (rpm_ranges[i] == range)
> -			return i;
> +		if (rpm <= rpm_ranges[i]) {
> +			*index = i;
> +			break;
> +		}
>   	}
>   
> -	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 i, val;
>   	int err;
>   
>   	/* Reset chip to default values, see below for GCONFIG setup */
> @@ -416,58 +432,40 @@ 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,
> +		 * Full speed on therm, spin-up at zero rpm.
>   		 * 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);
> +			(data->full_speed_on_therm[i] ?
> +			 MAX6639_FAN_CONFIG3_THERM_FULL_SPEED : 0) |
> +			(data->spin_up_enable[i] ?
> +			 0 : MAX6639_FAN_CONFIG3_SPIN_UP_DISABLE) | 0x03);
> +
>   		if (err)
>   			goto exit;
>   
> -		/* Max. temp. 80C/90C/100C */
> -		data->temp_therm[i] = 80;
> -		data->temp_alert[i] = 90;
> -		data->temp_ot[i] = 100;
>   		err = i2c_smbus_write_byte_data(client,
>   				MAX6639_REG_THERM_LIMIT(i),
>   				data->temp_therm[i]);
> @@ -483,13 +481,24 @@ 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)
>   			goto exit;
>   	}
> +
> +	val = 0;
> +	for (i = 0; i < 2; i++) {
> +		val |= MAX6639_REG_OUTPUT_MASK_OT(
> +				data->ot_indication[i], i);
> +		val |= MAX6639_REG_OUTPUT_MASK_THERM(
> +				data->therm_indication[i], i);
> +		val |= MAX6639_REG_OUTPUT_MASK_FANFAIL(
> +				data->fan_fail_indication[i], i);
> +	}
> +	/* Configure output mask register */
> +	err = i2c_smbus_write_byte_data(client, MAX6639_REG_OUTPUT_MASK, val);
> +
>   	/* Start monitoring */
>   	err = i2c_smbus_write_byte_data(client, MAX6639_REG_GCONFIG,
>   		MAX6639_GCONFIG_DISABLE_TIMEOUT | MAX6639_GCONFIG_CH2_LOCAL |
> @@ -524,12 +533,105 @@ 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(&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)
> +		data->pwm[i] =
> +			(u8)(val * 255 / maxrpm);
> +
> +	data->pwm_polarity[i] =  of_property_read_bool(child, "pwm-polarity-inverse");
> +
> +	data->spin_up_enable[i] = of_property_read_bool(child, "maxim,fan-spin-up");
> +
> +	data->full_speed_on_therm[i] = of_property_read_bool(child,
> +		"maxim,full-speed-on-therm");
> +
> +	data->fan_fail_indication[i] = of_property_read_bool(child, "maxim,fanfail_indication");
> +
> +	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;
> +		}
> +	}
> +
> +	data->ot_indication[0] = !of_property_read_bool(np, "ot0_indication");
> +	data->ot_indication[1] = !of_property_read_bool(np, "ot1_indication");
> +	data->therm_indication[0] = !of_property_read_bool(np, "therm0_indication");
> +	data->therm_indication[1] = !of_property_read_bool(np, "therm1_indication");
> +
> +	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 +641,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 +664,25 @@ static int max6639_probe(struct i2c_client *client)
>   
>   	mutex_init(&data->update_lock);
>   
> +	/* default values */
> +	for (i = 0; i < 2; i++) {
> +		/* 4000 RPM */
> +		data->rpm_range[i] = 1;
> +		data->ppr[i] = 2;
> +		data->pwm_polarity[i] = 1;
> +		/* 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;
> +		data->full_speed_on_therm[i] = false;
> +	}

This is an absolute no-go. Do not make up random default values.
Default is whatever is configured into the chip (it may have been
configured by the ROMMON / BIOS).

Guenter

> +
> +	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 +694,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 +732,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 +741,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,
> diff --git a/include/linux/platform_data/max6639.h b/include/linux/platform_data/max6639.h
> deleted file mode 100644
> index 65bfdb4fdc15..000000000000
> --- a/include/linux/platform_data/max6639.h
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _LINUX_MAX6639_H
> -#define _LINUX_MAX6639_H
> -
> -#include <linux/types.h>
> -
> -/* platform data for the MAX6639 temperature sensor and fan control */
> -
> -struct max6639_platform_data {
> -	bool pwm_polarity;	/* Polarity low (0) or high (1, default) */
> -	int ppr;		/* Pulses per rotation 1..4 (default == 2) */
> -	int rpm_range;		/* 2000, 4000 (default), 8000 or 16000 */
> -};
> -
> -#endif /* _LINUX_MAX6639_H */

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

[Rob Herring]

On Tue, Oct 11, 2022 at 5:47 AM Naresh Solanki
<naresh.solanki@9elements.com> 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
>       allOf:

Don't allOf here.

>         - $ref: fan-common.yaml#
>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
>  .../devicetree/bindings/hwmon/fan-common.yaml | 80 +++++++++++++++++++
>  1 file changed, 80 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..abc8375da646
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: GPL-2.0-only

Dual license with 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
> +
> +  pulse-per-revolution:

The already in use property is 'pulses-per-revolution'.

> +    description:
> +      The number of pulse from fan sensor per revolution.
> +    $ref: /schemas/types.yaml#/definitions/uint32

I assume there's a known set of values various fans have?

> +
> +  target-rpm:
> +    description:
> +      Target RPM the fan should be configured during driver probe.

Which driver? I think 'default-rpm' would be a better name.

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

Both of these properties are handled by the PWM binding already. I
think this should use it even though the PWMs are just connected to
the child nodes. There's always the possibility that someone hooks up
a fan controller PWM to something else besides a fan.

> +
> +  label:
> +    description:
> +      Optional fan label
> +    $ref: /schemas/types.yaml#/definitions/string

Doesn't a fan need power? 'fan-supply' is already in use, so that could be used.

> +
> +additionalProperties: true
> +
> +examples:
> +  - |

Drop the example here as you have it in the max6639 schema.

> +
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        fan-controller@30 {
> +            compatible = "maxim,max6639";
> +            reg = <0x30>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            fan@0 {
> +                reg = <0>;
> +                label = "CPU0_Fan";
> +                max-rpm = <32000>;
> +                pulse-per-revolution = <2>;
> +                target-rpm = <2000>;
> +                pwm-frequency = <25000>;
> +            };
> +
> +            fan@1 {
> +                reg = <1>;
> +                label = "PCIe0_Fan";
> +                max-rpm = <32000>;
> +                pulse-per-revolution = <2>;
> +                target-rpm = <2000>;
> +                pwm-frequency = <25000>;
> +            };
> +
> +        };
> +    };
> +
> +...
>
> base-commit: 0cf46a653bdae56683fece68dc50340f7520e6c4
> --
> 2.37.3
>

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

[Rob Herring]

On Tue, Oct 11, 2022 at 09:46:08AM -0700, Guenter Roeck wrote:
> On 10/11/22 09:12, Naresh Solanki wrote:
> > Hi Guenter,
> > 
> > fan-common is intended for fan properties. i.e., those derived from
> > fan datasheets.
> > For min-rpm, some fans have minimum non zero rpm like 1900rpm below which
> > the fan cannot run.
> > 
> 
> I would argue the properties are for fan controllers, not for fans.
> Fans don't have or depend on specific pwm frequencies. Fan controllers
> do.

Presumably fan controllers can produce a range of frequencies. If they 
need a specific frequency, then why are they programmable? Something 
outside the fan controller must have the constraint.

> Fans don't have a configurable pwm polarity. Fan controllers do,
> to match the hardware on a board.

We don't model an inverter, so it's got to go somewhere.

> Fans don't have or rely on
> a target rpm. That is a system property, configured into the
> fan controller. And so on.

Yes, but it is per fan. per fan properties/settings should go in the fan 
node IMO.

> If this is for fans, we'll need another set of properties for
> fan controllers. The driver for max6639, being a fan controller,
> would need to implement those properties.
> 
> Also, as implemented in the MAX6639, max-rpm is the fan's
> rpm range, not the actual rpm. Your implementation is just
> confusing, including the example in the bindings. Valid values
> should be what the chip accepts, not some random value.

A fan would have some design maximum RPM depending on its mechanical 
design and lifetime requirements. A controller would have some maximum 
in terms of electrical pulse frequency or register bit sizes (depending 
how RPM or pulse counts are exposed to s/w. That should all be implied 
from the controller part and not in DT. It's the mechanical limits of 
the fan we should be defining here.

> Really, I don't understand where you are going with this.

Certainly it needs more thought for different cases.

Rob

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

[Naresh Solanki]

Hi Rob, Guenter, Krzysztof,

I want to align with the implementation for the fan dt schema.

Current implementation is intending to use fan-common.yaml only for the purpose
of defining fan property as I felt this is the best way. This is how
other drivers have approached(eg: leds)

With this fan-controller driver will configure the chip  based on fan
characteristics accordingly.
target-rpm/default-rpm is included in it to enable driver configure
fan controllers during driver
probe.

Fan datasheets do specify the pwm frequency used to evaluate its
characteristic. That is the reason I've
included pwm-frequency here which fan-controller drivers can use &
initialize pwm frequency accordingly.

I'm ok with other approaches so do provide your perspective.

Regards,
Naresh Solanki

On Wed, 12 Oct 2022 at 00:35, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Oct 11, 2022 at 5:47 AM Naresh Solanki
> <naresh.solanki@9elements.com> 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
> >       allOf:
>
> Don't allOf here.
>
> >         - $ref: fan-common.yaml#
> >
> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > ---
> >  .../devicetree/bindings/hwmon/fan-common.yaml | 80 +++++++++++++++++++
> >  1 file changed, 80 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..abc8375da646
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
>
> Dual license with 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
> > +
> > +  pulse-per-revolution:
>
> The already in use property is 'pulses-per-revolution'.
>
> > +    description:
> > +      The number of pulse from fan sensor per revolution.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
>
> I assume there's a known set of values various fans have?
>
> > +
> > +  target-rpm:
> > +    description:
> > +      Target RPM the fan should be configured during driver probe.
>
> Which driver? I think 'default-rpm' would be a better name.
>
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  pwm-frequency:
> > +    description:
> > +      PWM frequency for fan.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  pwm-polarity-inverse:
> > +    description:
> > +      PWM polarity for fan.
> > +    type: boolean
>
> Both of these properties are handled by the PWM binding already. I
> think this should use it even though the PWMs are just connected to
> the child nodes. There's always the possibility that someone hooks up
> a fan controller PWM to something else besides a fan.
>
> > +
> > +  label:
> > +    description:
> > +      Optional fan label
> > +    $ref: /schemas/types.yaml#/definitions/string
>
> Doesn't a fan need power? 'fan-supply' is already in use, so that could be used.
>
> > +
> > +additionalProperties: true
> > +
> > +examples:
> > +  - |
>
> Drop the example here as you have it in the max6639 schema.
>
> > +
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        fan-controller@30 {
> > +            compatible = "maxim,max6639";
> > +            reg = <0x30>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            fan@0 {
> > +                reg = <0>;
> > +                label = "CPU0_Fan";
> > +                max-rpm = <32000>;
> > +                pulse-per-revolution = <2>;
> > +                target-rpm = <2000>;
> > +                pwm-frequency = <25000>;
> > +            };
> > +
> > +            fan@1 {
> > +                reg = <1>;
> > +                label = "PCIe0_Fan";
> > +                max-rpm = <32000>;
> > +                pulse-per-revolution = <2>;
> > +                target-rpm = <2000>;
> > +                pwm-frequency = <25000>;
> > +            };
> > +
> > +        };
> > +    };
> > +
> > +...
> >
> > base-commit: 0cf46a653bdae56683fece68dc50340f7520e6c4
> > --
> > 2.37.3
> >