[PATCH v1 0/2] Add JHB100 Fan-Tach support

[PATCH v1 0/2] Add JHB100 Fan-Tach support

[Changhuang Liang]

StarFive Fan-Tach controller can support up to 16 fan tach input.
The series has been tested on the EVB1 board.

Changhuang Liang (2):
  dt-bindings: hwmon: Add starfive,jhb100-fan-tach
  hwmon: Add fan tach driver for StarFive

 .../hwmon/starfive,jhb100-fan-tach.yaml       |  98 ++++
 MAINTAINERS                                   |   6 +
 drivers/hwmon/Kconfig                         |   9 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/starfive-fan-tach.c             | 467 ++++++++++++++++++
 5 files changed, 581 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
 create mode 100644 drivers/hwmon/starfive-fan-tach.c

--
2.25.1

[PATCH v1 1/2] dt-bindings: hwmon: Add starfive,jhb100-fan-tach

[Changhuang Liang]

Add compatible "starfive,jhb100-fan-tach" for StarFive JHB100 Fan-Tach
controller.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 .../hwmon/starfive,jhb100-fan-tach.yaml       | 98 +++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
new file mode 100644
index 000000000000..fdc8539ec804
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
@@ -0,0 +1,98 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/starfive,jhb100-fan-tach.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JHB100 Fan-Tach controller
+
+maintainers:
+  - Changhuang Liang <changhuang.liang@starfivetech.com>
+
+description:
+  The StarFive Fan-Tach controller can support up to 16 fan tach input.
+
+properties:
+  compatible:
+    enum:
+      - starfive,jhb100-fan-tach
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  "^fan@[0-9]+$":
+    $ref: fan-common.yaml#
+    unevaluatedProperties: false
+    required:
+      - reg
+      - tach-ch
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    fan-tach-controller@11be0000 {
+        compatible = "starfive,jhb100-fan-tach";
+        reg = <0x11be0000 0x400>;
+        interrupts = <128>;
+        clocks = <&per2crg 33>;
+        resets = <&per2crg 5>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        fan@0 {
+            reg = <0>;
+            tach-ch = /bits/ 8 <0x0>, <0x8>;
+        };
+        fan@1 {
+            reg = <1>;
+            tach-ch = /bits/ 8 <0x1>, <0x9>;
+        };
+        fan@2 {
+            reg = <2>;
+            tach-ch = /bits/ 8 <0x2>, <0xa>;
+        };
+        fan@3 {
+            reg = <3>;
+            tach-ch = /bits/ 8 <0x3>, <0xb>;
+        };
+        fan@4 {
+            reg = <4>;
+            tach-ch = /bits/ 8 <0x4>, <0xc>;
+        };
+        fan@5 {
+            reg = <5>;
+            tach-ch = /bits/ 8 <0x5>, <0xd>;
+        };
+        fan@6 {
+            reg = <6>;
+            tach-ch = /bits/ 8 <0x6>, <0xe>;
+        };
+        fan@7 {
+            reg = <7>;
+            tach-ch = /bits/ 8 <0x7>, <0xf>;
+        };
+    };
-- 
2.25.1

[PATCH v1 2/2] hwmon: Add fan tach driver for StarFive

[Changhuang Liang]

Add fan tach driver for StarFive JHB100 SoC.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 MAINTAINERS                       |   6 +
 drivers/hwmon/Kconfig             |   9 +
 drivers/hwmon/Makefile            |   1 +
 drivers/hwmon/starfive-fan-tach.c | 467 ++++++++++++++++++++++++++++++
 4 files changed, 483 insertions(+)
 create mode 100644 drivers/hwmon/starfive-fan-tach.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2fb1c75afd16..72c1e093b865 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25539,6 +25539,12 @@ S:	Supported
 F:	Documentation/devicetree/bindings/interrupt-controller/starfive,jh8100-intc.yaml
 F:	drivers/irqchip/irq-starfive-jh8100-intc.c
 
+STARFIVE JHB100 FAN-TACH DRIVER
+M:	Changhuang Liang <changhuang.liang@starfivetech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
+F:	drivers/hwmon/starfive-fan-tach.c
+
 STATIC BRANCH/CALL
 M:	Peter Zijlstra <peterz@infradead.org>
 M:	Josh Poimboeuf <jpoimboe@kernel.org>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 14e4cea48acc..85cd34a635fb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2338,6 +2338,15 @@ config SENSORS_SPD5118_DETECT
 
 	  If unsure, say Y.
 
+config SENSORS_STARFIVE_FAN_TACH
+	tristate "StarFive Fan-Tach driver"
+	depends on ARCH_STARFIVE || COMPILE_TEST
+	help
+	  This driver provides support for StarFive Fan-Tach controllers.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called starfive-fan-tach.
+
 config SENSORS_TC74
 	tristate "Microchip TC74"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 4788996aa137..ff0bb789defc 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -220,6 +220,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
 obj-$(CONFIG_SENSORS_SPARX5)	+= sparx5-temp.o
 obj-$(CONFIG_SENSORS_SPD5118)	+= spd5118.o
+obj-$(CONFIG_SENSORS_STARFIVE_FAN_TACH)	+= starfive-fan-tach.o
 obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
 obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o
 obj-$(CONFIG_SENSORS_SURFACE_TEMP)+= surface_temp.o
diff --git a/drivers/hwmon/starfive-fan-tach.c b/drivers/hwmon/starfive-fan-tach.c
new file mode 100644
index 000000000000..394c562d2267
--- /dev/null
+++ b/drivers/hwmon/starfive-fan-tach.c
@@ -0,0 +1,467 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * FAN-TACH controller driver for StarFive JHB100
+ *
+ * Copyright (C) 2018-2024 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/errno.h>
+#include <linux/hwmon.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/sysfs.h>
+
+#define STARFIVE_FAN_TACH_CH			16
+
+/* Fan-tach register offest */
+#define STARFIVE_FAN_TACH_STATUS		0x0c
+
+#define STARFIVE_FAN_TACH_SPEED(ch)		(((ch) * 0x04) + 0x10)
+#define STARFIVE_FAN_TACH_SPEED_VALID		BIT(31)
+#define STARFIVE_FAN_TACH_VALUE_MASK		GENMASK(30, 0)
+
+#define STARFIVE_FAN_TACH_THRESHOLD(ch)		(((ch) * 0x04) + 0x50)
+
+#define STARFIVE_FAN_TACH_INT_EN		0x90
+#define STARFIVE_FAN_TACH_STALL_INT_MASK	GENMASK(15, 0)
+#define STARFIVE_FAN_TACH_SLOW_INT_MASK		GENMASK(31, 16)
+#define STARFIVE_FAN_TACH_STALL_INT(ch)		BIT(ch)
+#define STARFIVE_FAN_TACH_SLOW_INT(ch)		(BIT(ch) << 16)
+
+#define STARFIVE_FAN_TACH_MEASURE_TIME		0x94
+
+#define STARFIVE_FAN_TACH_CH_EN			0x98
+#define STARFIVE_FAN_TACH_EN(ch)		BIT(ch)
+
+#define STARFIVE_FAN_DEFAULT_PULSE_PR		2
+#define STARFIVE_FAN_DEFAULT_MEASURE_RATIO	2
+#define STARFIVE_FAN_DEFAULT_RPM_PAUSE_TIME	(60 * STARFIVE_FAN_DEFAULT_MEASURE_RATIO)
+
+#define STARFIVE_FAN_TACH_TIMEOUT \
+	(USEC_PER_SEC / STARFIVE_FAN_DEFAULT_MEASURE_RATIO)
+
+#define STARFIVE_FAN_TACH_TIMEOUT_JIFFIES \
+	(msecs_to_jiffies(1000) / STARFIVE_FAN_DEFAULT_MEASURE_RATIO)
+
+#define FAN_ATTRIBUTE_SET \
+	(HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_ENABLE | \
+	 HWMON_F_FAULT | HWMON_F_MIN_ALARM)
+
+struct starfive_fan_tach_data {
+	struct device *dev;
+	void __iomem *regs;
+	bool tach_present[STARFIVE_FAN_TACH_CH];
+	u32 clk_rate; /* APB clock frequency */
+	struct completion comp_stall[STARFIVE_FAN_TACH_CH];
+	struct completion comp_slow[STARFIVE_FAN_TACH_CH];
+	u8 fan_stall[STARFIVE_FAN_TACH_CH];
+	u8 fan_slow[STARFIVE_FAN_TACH_CH];
+};
+
+static void starfive_fan_tach_ch_enable(struct starfive_fan_tach_data *priv, u8 tach_ch,
+					bool enable)
+{
+	if (enable) {
+		writel(readl(priv->regs + STARFIVE_FAN_TACH_CH_EN) |
+		       STARFIVE_FAN_TACH_EN(tach_ch),
+		       priv->regs + STARFIVE_FAN_TACH_CH_EN);
+	} else {
+		writel(readl(priv->regs + STARFIVE_FAN_TACH_CH_EN) &
+		       ~(STARFIVE_FAN_TACH_EN(tach_ch)),
+		       priv->regs + STARFIVE_FAN_TACH_CH_EN);
+	}
+}
+
+static void starfive_fan_tach_ch_stall_unmask(struct starfive_fan_tach_data *priv, u8 tach_ch,
+					      bool unmask)
+{
+	if (unmask) {
+		writel(readl(priv->regs + STARFIVE_FAN_TACH_INT_EN) |
+		       STARFIVE_FAN_TACH_STALL_INT(tach_ch),
+		       priv->regs + STARFIVE_FAN_TACH_INT_EN);
+	} else {
+		writel(readl(priv->regs + STARFIVE_FAN_TACH_INT_EN) &
+		       ~STARFIVE_FAN_TACH_STALL_INT(tach_ch),
+		       priv->regs + STARFIVE_FAN_TACH_INT_EN);
+	}
+}
+
+static void starfive_fan_tach_ch_slow_unmask(struct starfive_fan_tach_data *priv, u8 tach_ch,
+					     bool unmask)
+{
+	if (unmask) {
+		writel(readl(priv->regs + STARFIVE_FAN_TACH_INT_EN) |
+		       STARFIVE_FAN_TACH_SLOW_INT(tach_ch),
+		       priv->regs + STARFIVE_FAN_TACH_INT_EN);
+	} else {
+		writel(readl(priv->regs + STARFIVE_FAN_TACH_INT_EN) &
+		       ~STARFIVE_FAN_TACH_SLOW_INT(tach_ch),
+		       priv->regs + STARFIVE_FAN_TACH_INT_EN);
+	}
+}
+
+static int starfive_fan_tach_rpm_to_val(struct starfive_fan_tach_data *priv, u32 rpm)
+{
+	u64 tach_val;
+
+	rpm *= STARFIVE_FAN_DEFAULT_PULSE_PR;
+	tach_val = rpm / STARFIVE_FAN_DEFAULT_RPM_PAUSE_TIME;
+
+	return (int)tach_val;
+}
+
+static int starfive_fan_tach_val_to_rpm(struct starfive_fan_tach_data *priv, u32 tach_val)
+{
+	u64 rpm;
+
+	rpm = tach_val * STARFIVE_FAN_DEFAULT_RPM_PAUSE_TIME;
+
+	do_div(rpm, STARFIVE_FAN_DEFAULT_PULSE_PR);
+
+	return (int)rpm;
+}
+
+static int starfive_fan_tach_get_rpm(struct starfive_fan_tach_data *priv,
+				     u8 fan_tach_ch)
+{
+	u32 val;
+	int ret;
+
+	ret = readl_poll_timeout(priv->regs + STARFIVE_FAN_TACH_SPEED(fan_tach_ch),
+				 val, val & STARFIVE_FAN_TACH_SPEED_VALID,
+				 100, STARFIVE_FAN_TACH_TIMEOUT);
+	if (ret)
+		return ret;
+
+	val = FIELD_GET(STARFIVE_FAN_TACH_VALUE_MASK, val);
+
+	return starfive_fan_tach_val_to_rpm(priv, val);
+}
+
+static int starfive_fan_tach_get_rpm_threshold(struct starfive_fan_tach_data *priv,
+					       u8 fan_tach_ch)
+{
+	u32 val;
+
+	val = readl(priv->regs + STARFIVE_FAN_TACH_THRESHOLD(fan_tach_ch));
+
+	val = FIELD_GET(STARFIVE_FAN_TACH_VALUE_MASK, val);
+
+	return starfive_fan_tach_val_to_rpm(priv, (int)val);
+}
+
+static void starfive_set_tach_rpm_threshold(struct starfive_fan_tach_data *priv,
+					    u8 fan_tach_ch, u32 val)
+{
+	val = clamp_val(starfive_fan_tach_rpm_to_val(priv, val), 1, STARFIVE_FAN_TACH_VALUE_MASK);
+
+	writel(val, priv->regs + STARFIVE_FAN_TACH_THRESHOLD(fan_tach_ch));
+}
+
+static int starfive_fan_tach_get_ch_en(struct starfive_fan_tach_data *priv,
+				       u8 fan_tach_ch)
+{
+	u32 enable;
+
+	enable = readl(priv->regs + STARFIVE_FAN_TACH_CH_EN);
+
+	return (enable & (1 << fan_tach_ch)) ? 1 : 0;
+}
+
+static void starfive_fan_tach_set_ch_en(struct starfive_fan_tach_data *priv,
+					u8 fan_tach_ch, u32 val)
+{
+	bool enable;
+
+	switch (val) {
+	case 0:
+		enable = false;
+		break;
+	case 1:
+		enable = true;
+		break;
+	default:
+		return;
+	}
+
+	starfive_fan_tach_ch_enable(priv, fan_tach_ch, enable);
+}
+
+static int starfive_fan_tach_hwmon_read(struct device *dev,
+					enum hwmon_sensor_types type, u32 attr,
+					int channel, long *val)
+{
+	struct starfive_fan_tach_data *priv = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_fan_fault:
+		starfive_fan_tach_ch_stall_unmask(priv, channel, true);
+		/* clear fan_stall first */
+		priv->fan_stall[channel] = 0;
+		reinit_completion(&priv->comp_stall[channel]);
+		wait_for_completion_timeout(&priv->comp_stall[channel],
+					    2 * STARFIVE_FAN_TACH_TIMEOUT_JIFFIES);
+		*val = priv->fan_stall[channel];
+		starfive_fan_tach_ch_stall_unmask(priv, channel, false);
+		break;
+	case hwmon_fan_input:
+		*val = starfive_fan_tach_get_rpm(priv, channel);
+		break;
+	case hwmon_fan_min:
+		*val = starfive_fan_tach_get_rpm_threshold(priv, channel);
+		break;
+	case hwmon_fan_min_alarm:
+		starfive_fan_tach_ch_slow_unmask(priv, channel, true);
+		/* clear fan_slow first */
+		priv->fan_slow[channel] = 0;
+		reinit_completion(&priv->comp_slow[channel]);
+		wait_for_completion_timeout(&priv->comp_stall[channel],
+					    2 * STARFIVE_FAN_TACH_TIMEOUT_JIFFIES);
+		*val = priv->fan_slow[channel];
+		starfive_fan_tach_ch_slow_unmask(priv, channel, false);
+		break;
+	case hwmon_fan_enable:
+		*val = starfive_fan_tach_get_ch_en(priv, channel);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int starfive_fan_tach_hwmon_write(struct device *dev,
+					 enum hwmon_sensor_types type, u32 attr,
+					 int channel, long val)
+{
+	struct starfive_fan_tach_data *priv = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_fan_min:
+		starfive_set_tach_rpm_threshold(priv, channel, (u32)val);
+		break;
+	case hwmon_fan_enable:
+		starfive_fan_tach_set_ch_en(priv, channel, (u32)val);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static umode_t starfive_fan_tach_dev_is_visible(const void *drvdata,
+						enum hwmon_sensor_types type,
+						u32 attr, int channel)
+{
+	const struct starfive_fan_tach_data *priv = drvdata;
+
+	if (!priv->tach_present[channel])
+		return 0;
+
+	switch (attr) {
+	case hwmon_fan_input:
+	case hwmon_fan_fault:
+	case hwmon_fan_min_alarm:
+		return 0444;
+	case hwmon_fan_min:
+	case hwmon_fan_pulses:
+	case hwmon_fan_enable:
+		return 0644;
+	}
+
+	return 0;
+}
+
+static const struct hwmon_ops starfive_fan_tach_ops = {
+	.is_visible = starfive_fan_tach_dev_is_visible,
+	.read = starfive_fan_tach_hwmon_read,
+	.write = starfive_fan_tach_hwmon_write
+};
+
+static const struct hwmon_channel_info *starfive_fan_tach_info[] = {
+	HWMON_CHANNEL_INFO(fan,
+			   FAN_ATTRIBUTE_SET, FAN_ATTRIBUTE_SET,
+			   FAN_ATTRIBUTE_SET, FAN_ATTRIBUTE_SET,
+			   FAN_ATTRIBUTE_SET, FAN_ATTRIBUTE_SET,
+			   FAN_ATTRIBUTE_SET, FAN_ATTRIBUTE_SET,
+			   FAN_ATTRIBUTE_SET, FAN_ATTRIBUTE_SET,
+			   FAN_ATTRIBUTE_SET, FAN_ATTRIBUTE_SET,
+			   FAN_ATTRIBUTE_SET, FAN_ATTRIBUTE_SET,
+			   FAN_ATTRIBUTE_SET, FAN_ATTRIBUTE_SET),
+	NULL
+};
+
+static const struct hwmon_chip_info starfive_fan_tach_chip_info = {
+	.ops = &starfive_fan_tach_ops,
+	.info = starfive_fan_tach_info,
+};
+
+static void starfive_fan_tach_present(struct starfive_fan_tach_data *priv, u8 *tach_ch,
+				      int count)
+{
+	u8 ch, index;
+
+	for (index = 0; index < count; index++) {
+		ch = tach_ch[index];
+		priv->tach_present[ch] = true;
+
+		starfive_fan_tach_ch_enable(priv, ch, true);
+		init_completion(&priv->comp_stall[ch]);
+		init_completion(&priv->comp_slow[ch]);
+	}
+}
+
+static int starfive_fan_tach_create_fan(struct device *dev, struct device_node *child,
+					struct starfive_fan_tach_data *priv)
+{
+	int ret, count;
+	u8 *tach_ch;
+
+	count = of_property_count_u8_elems(child, "tach-ch");
+	if (count < 1)
+		return -EINVAL;
+
+	tach_ch = devm_kcalloc(dev, count, sizeof(*tach_ch), GFP_KERNEL);
+	if (!tach_ch)
+		return -ENOMEM;
+
+	ret = of_property_read_u8_array(child, "tach-ch", tach_ch, count);
+	if (ret)
+		return ret;
+
+	starfive_fan_tach_present(priv, tach_ch, count);
+
+	return 0;
+}
+
+static irqreturn_t starfive_fan_tach_irq(int irq, void *dev_id)
+{
+	struct starfive_fan_tach_data *priv = dev_id;
+	u32 fan_status;
+	int i;
+
+	fan_status = readl(priv->regs + STARFIVE_FAN_TACH_STATUS);
+
+	for (i = 0; i < STARFIVE_FAN_TACH_CH; i++) {
+		if (fan_status & STARFIVE_FAN_TACH_STALL_INT(i)) {
+			priv->fan_stall[i] = 1;
+			complete(&priv->comp_stall[i]);
+		}
+
+		if (fan_status & STARFIVE_FAN_TACH_SLOW_INT(i)) {
+			priv->fan_slow[i] = 1;
+			complete(&priv->comp_slow[i]);
+		}
+	}
+
+	writel(fan_status, priv->regs + STARFIVE_FAN_TACH_STATUS);
+
+	return IRQ_HANDLED;
+}
+
+static void starfive_fan_tach_reset_control_assert(void *data)
+{
+	reset_control_assert(data);
+}
+
+static int starfive_fan_tach_probe(struct platform_device *pdev)
+{
+	struct device *hwmon_dev, *dev = &pdev->dev;
+	struct starfive_fan_tach_data *priv;
+	struct reset_control *rst;
+	struct clk *clk;
+	int irq;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	priv->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->regs))
+		return dev_err_probe(dev, PTR_ERR(priv->regs),
+				     "Unable to map IO resources\n");
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return dev_err_probe(dev, irq, "Failed to get IRQ\n");
+
+	ret = devm_request_irq(dev, irq, starfive_fan_tach_irq, 0, pdev->name,
+			       (void *)priv);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register interrupt handler\n");
+
+	rst = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(rst))
+		return dev_err_probe(dev, PTR_ERR(rst),
+				     "Unable to get fan tach's reset\n");
+
+	clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "Unable to get & enable fan tach's clock\n");
+
+	priv->clk_rate = clk_get_rate(clk);
+	if (priv->clk_rate <= 0)
+		return dev_err_probe(dev, priv->clk_rate,
+				     "Unable to get clock's rate\n");
+
+	reset_control_deassert(rst);
+	ret = devm_add_action_or_reset(dev, starfive_fan_tach_reset_control_assert, rst);
+	if (ret)
+		return ret;
+
+	writel(STARFIVE_FAN_TACH_STALL_INT_MASK | STARFIVE_FAN_TACH_SLOW_INT_MASK,
+	       priv->regs + STARFIVE_FAN_TACH_STATUS);
+	writel(priv->clk_rate / STARFIVE_FAN_DEFAULT_MEASURE_RATIO,
+	       priv->regs + STARFIVE_FAN_TACH_MEASURE_TIME);
+
+	for_each_child_of_node_scoped(dev->of_node, child) {
+		ret = starfive_fan_tach_create_fan(dev, child, priv);
+		if (ret < 0) {
+			dev_warn(dev, "Failed to create fan %d", ret);
+			return 0;
+		}
+	}
+
+	platform_set_drvdata(pdev, priv);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "starfive_fan_tach",
+							 priv, &starfive_fan_tach_chip_info,
+							 NULL);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct of_device_id starfive_fan_tach_of_match[] = {
+	{ .compatible = "starfive,jhb100-fan-tach", },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver starfive_fan_tach_driver = {
+	.probe = starfive_fan_tach_probe,
+	.driver	= {
+		.name = "starfive-fan-tach",
+		.of_match_table = starfive_fan_tach_of_match,
+	},
+};
+
+module_platform_driver(starfive_fan_tach_driver);
+
+MODULE_AUTHOR("William Qiu <william.qiu@starfivetech.com>");
+MODULE_AUTHOR("Changhuang Liang <changhuang.liang@starfivetech.com>");
+MODULE_DESCRIPTION("StarFive JHB100 Fan Tach device driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v1 1/2] dt-bindings: hwmon: Add starfive,jhb100-fan-tach

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The `tach-ch` property in the device tree example uses incorrect comma-separated syntax that evaluates to a 5-byte array rather than the intended 2-byte array.
--

commit 4bf196984d444742e611042f289ee38d6b5ced8c
Author: Changhuang Liang <changhuang.liang@starfivetech.com>

dt-bindings: hwmon: Add starfive,jhb100-fan-tach

This commit adds the compatible string "starfive,jhb100-fan-tach" and the
corresponding binding schema for the StarFive JHB100 Fan-Tach controller.
The controller supports up to 16 fan tach inputs.

> diff --git a/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
[ ... ]
> +examples:
> +  - |
> +    fan-tach-controller@11be0000 {
> +        compatible = "starfive,jhb100-fan-tach";
> +        reg = <0x11be0000 0x400>;
> +        interrupts = <128>;
> +        clocks = <&per2crg 33>;
> +        resets = <&per2crg 5>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        fan@0 {
> +            reg = <0>;
> +            tach-ch = /bits/ 8 <0x0>, <0x8>;

Is the comma-separated syntax in tach-ch intentional? In Device Tree Source
syntax, the /bits/ 8 directive applies only to the immediately following
angle-bracket block.

Because tach-ch = /bits/ 8 <0x0>, <0x8>; separates the values with a comma,
the second block reverts to the default 32-bit cell size. This produces a
5-byte array [0x00, 0x00, 0x00, 0x00, 0x08] rather than the intended 2-byte
array.

If a developer copies this example into a real device tree, will the driver
incorrectly interpret it as 5 separate tachometer channels (0, 0, 0, 0, 8)
instead of the intended 2 channels (0, 8)?

Should this be written as /bits/ 8 <0x0 0x8>; or using byte array syntax
like [00 08] instead?

> +        };
> +        fan@1 {
> +            reg = <1>;
> +            tach-ch = /bits/ 8 <0x1>, <0x9>;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428082337.743546-1-changhuang.liang@starfivetech.com?part=1

Re: [PATCH v1 2/2] hwmon: Add fan tach driver for StarFive

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Sysfs read operations for fault and alarm attributes block for 1 second during normal operation.
- [High] Interrupt handler is registered before the device clock is enabled, risking fatal bus faults.
- [High] Probe returns success (0) on failure paths, bypassing hwmon registration but leaving resources active.
- [High] Copy-paste error in `hwmon_fan_min_alarm` waits on the wrong completion structure, guaranteeing a timeout.
- [High] Race condition when enabling interrupts causes missed fault/alarm events.
- [High] IRQ handler processes uninitialized completions for unused channels, leading to a kernel panic.
- [High] Unvalidated device tree properties lead to out-of-bounds array access.
- [Medium] Hardware read errors are leaked to user space as valid sensor values.
- [Medium] Integer overflow in RPM conversion calculations yields incorrect readings.
--

commit 48298341a9b8797b12cfca7ded254542d53095fd
Author: Changhuang Liang <changhuang.liang@starfivetech.com>
Subject: hwmon: Add fan tach driver for StarFive

This commit adds the fan tach driver for the StarFive JHB100 SoC.

> diff --git a/drivers/hwmon/starfive-fan-tach.c b/drivers/hwmon/starfive-fan-tach.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hwmon/starfive-fan-tach.c

[ ... ]

> +static int starfive_fan_tach_rpm_to_val(struct starfive_fan_tach_data *priv, u32 rpm)
> +{
> +	u64 tach_val;
> +
> +	rpm *= STARFIVE_FAN_DEFAULT_PULSE_PR;

Does this multiplication overflow a 32-bit integer before assignment? Casting
rpm to u64 before multiplying would prevent truncation.

> +	tach_val = rpm / STARFIVE_FAN_DEFAULT_RPM_PAUSE_TIME;
> +
> +	return (int)tach_val;
> +}
> +
> +static int starfive_fan_tach_val_to_rpm(struct starfive_fan_tach_data *priv, u32 tach_val)
> +{
> +	u64 rpm;
> +
> +	rpm = tach_val * STARFIVE_FAN_DEFAULT_RPM_PAUSE_TIME;

Can this code overflow the 32-bit integer when multiplying tach_val by 120?
Since tach_val can be up to 31 bits, casting to u64 beforehand would
ensure correct calculations.

> +
> +	do_div(rpm, STARFIVE_FAN_DEFAULT_PULSE_PR);
> +
> +	return (int)rpm;
> +}

[ ... ]

> +static int starfive_fan_tach_hwmon_read(struct device *dev,
> +					enum hwmon_sensor_types type, u32 attr,
> +					int channel, long *val)
> +{
> +	struct starfive_fan_tach_data *priv = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_fan_fault:
> +		starfive_fan_tach_ch_stall_unmask(priv, channel, true);
> +		/* clear fan_stall first */
> +		priv->fan_stall[channel] = 0;
> +		reinit_completion(&priv->comp_stall[channel]);

Is there a race condition here that can cause a missed fault event? Unmasking
the hardware interrupt before clearing priv->fan_stall[] and calling
reinit_completion() might allow the interrupt to fire instantly. 

If the ISR sets the flag and completes the completion, the read thread will 
subsequently overwrite the flag and reinitialize the completion, erasing the
event and causing a timeout.

> +		wait_for_completion_timeout(&priv->comp_stall[channel],
> +					    2 * STARFIVE_FAN_TACH_TIMEOUT_JIFFIES);

If the fan is operating normally, will this read operation unconditionally block
for the entire 1-second timeout duration? Since the fault interrupt will never
fire during normal operation, and the hwmon core serializes sysfs accesses
with a device-wide lock, this might cause user-space tools to hang.

> +		*val = priv->fan_stall[channel];
> +		starfive_fan_tach_ch_stall_unmask(priv, channel, false);
> +		break;
> +	case hwmon_fan_input:
> +		*val = starfive_fan_tach_get_rpm(priv, channel);

Does this code leak hardware read errors to user space as valid sensor values?
If starfive_fan_tach_get_rpm() returns a negative error code like -ETIMEDOUT,
it is directly assigned to *val while returning 0 for success.

> +		break;
> +	case hwmon_fan_min:
> +		*val = starfive_fan_tach_get_rpm_threshold(priv, channel);
> +		break;
> +	case hwmon_fan_min_alarm:
> +		starfive_fan_tach_ch_slow_unmask(priv, channel, true);
> +		/* clear fan_slow first */
> +		priv->fan_slow[channel] = 0;
> +		reinit_completion(&priv->comp_slow[channel]);
> +		wait_for_completion_timeout(&priv->comp_stall[channel],
> +					    2 * STARFIVE_FAN_TACH_TIMEOUT_JIFFIES);

Should this wait on priv->comp_slow[channel] instead? Waiting on comp_stall
after reinitializing comp_slow guarantees a timeout, as a valid slow alarm
interrupt will never wake up the stall completion.

> +		*val = priv->fan_slow[channel];
> +		starfive_fan_tach_ch_slow_unmask(priv, channel, false);
> +		break;

[ ... ]

> +static void starfive_fan_tach_present(struct starfive_fan_tach_data *priv, u8 *tach_ch,
> +				      int count)
> +{
> +	u8 ch, index;
> +
> +	for (index = 0; index < count; index++) {
> +		ch = tach_ch[index];
> +		priv->tach_present[ch] = true;

Are we checking for out-of-bounds array access here? Since the arrays
have a fixed size of 16 (STARFIVE_FAN_TACH_CH), a device tree providing
a channel number >= 16 will write outside the bounds of tach_present,
comp_stall, and comp_slow.

[ ... ]

> +static irqreturn_t starfive_fan_tach_irq(int irq, void *dev_id)
> +{
> +	struct starfive_fan_tach_data *priv = dev_id;
> +	u32 fan_status;
> +	int i;
> +
> +	fan_status = readl(priv->regs + STARFIVE_FAN_TACH_STATUS);
> +
> +	for (i = 0; i < STARFIVE_FAN_TACH_CH; i++) {
> +		if (fan_status & STARFIVE_FAN_TACH_STALL_INT(i)) {
> +			priv->fan_stall[i] = 1;
> +			complete(&priv->comp_stall[i]);

Can this code process uninitialized completions for unused channels? If the
hardware asserts a bit for an unconfigured channel, calling complete() on an
uninitialized waitqueue lock can result in a kernel panic. Verifying
priv->tach_present[i] before processing would prevent this.

> +		}
> +
> +		if (fan_status & STARFIVE_FAN_TACH_SLOW_INT(i)) {
> +			priv->fan_slow[i] = 1;
> +			complete(&priv->comp_slow[i]);
> +		}
> +	}

[ ... ]

> +static int starfive_fan_tach_probe(struct platform_device *pdev)
> +{
> +	struct device *hwmon_dev, *dev = &pdev->dev;
> +	struct starfive_fan_tach_data *priv;
> +	struct reset_control *rst;
> +	struct clk *clk;
> +	int irq;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +
> +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->regs))
> +		return dev_err_probe(dev, PTR_ERR(priv->regs),
> +				     "Unable to map IO resources\n");
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(dev, irq, "Failed to get IRQ\n");
> +
> +	ret = devm_request_irq(dev, irq, starfive_fan_tach_irq, 0, pdev->name,
> +			       (void *)priv);

Does requesting the IRQ here before the clock is enabled and the reset is
deasserted risk fatal bus faults? If an interrupt is pending or fires
spuriously, starfive_fan_tach_irq() will execute and attempt to read the
peripheral's status register without an enabled clock.

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register interrupt handler\n");
> +
> +	rst = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(rst))
> +		return dev_err_probe(dev, PTR_ERR(rst),
> +				     "Unable to get fan tach's reset\n");
> +
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk),
> +				     "Unable to get & enable fan tach's clock\n");
> +
> +	priv->clk_rate = clk_get_rate(clk);
> +	if (priv->clk_rate <= 0)
> +		return dev_err_probe(dev, priv->clk_rate,
> +				     "Unable to get clock's rate\n");

Is it intentional to return 0 on this error path? Passing 0 to dev_err_probe()
and returning 0 bypasses hwmon registration while leaving the device entirely
non-functional and leaking the enabled clock and active IRQ.

> +
> +	reset_control_deassert(rst);
> +	ret = devm_add_action_or_reset(dev, starfive_fan_tach_reset_control_assert, rst);
> +	if (ret)
> +		return ret;
> +
> +	writel(STARFIVE_FAN_TACH_STALL_INT_MASK | STARFIVE_FAN_TACH_SLOW_INT_MASK,
> +	       priv->regs + STARFIVE_FAN_TACH_STATUS);
> +	writel(priv->clk_rate / STARFIVE_FAN_DEFAULT_MEASURE_RATIO,
> +	       priv->regs + STARFIVE_FAN_TACH_MEASURE_TIME);
> +
> +	for_each_child_of_node_scoped(dev->of_node, child) {
> +		ret = starfive_fan_tach_create_fan(dev, child, priv);
> +		if (ret < 0) {
> +			dev_warn(dev, "Failed to create fan %d", ret);
> +			return 0;

Could returning 0 here mistakenly signal success during a probe failure? This
would also skip devm_hwmon_device_register_with_info() and leave resources
active but non-functional.

> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428082337.743546-1-changhuang.liang@starfivetech.com?part=2

Re: [PATCH v1 1/2] dt-bindings: hwmon: Add starfive,jhb100-fan-tach

[Rob Herring]

On Tue, Apr 28, 2026 at 07:39:52PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] The `tach-ch` property in the device tree example uses incorrect comma-separated syntax that evaluates to a 5-byte array rather than the intended 2-byte array.
> --
> 
> commit 4bf196984d444742e611042f289ee38d6b5ced8c
> Author: Changhuang Liang <changhuang.liang@starfivetech.com>
> 
> dt-bindings: hwmon: Add starfive,jhb100-fan-tach
> 
> This commit adds the compatible string "starfive,jhb100-fan-tach" and the
> corresponding binding schema for the StarFive JHB100 Fan-Tach controller.
> The controller supports up to 16 fan tach inputs.
> 
> > diff --git a/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
> [ ... ]
> > +examples:
> > +  - |
> > +    fan-tach-controller@11be0000 {
> > +        compatible = "starfive,jhb100-fan-tach";
> > +        reg = <0x11be0000 0x400>;
> > +        interrupts = <128>;
> > +        clocks = <&per2crg 33>;
> > +        resets = <&per2crg 5>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        fan@0 {
> > +            reg = <0>;
> > +            tach-ch = /bits/ 8 <0x0>, <0x8>;
> 
> Is the comma-separated syntax in tach-ch intentional? In Device Tree Source
> syntax, the /bits/ 8 directive applies only to the immediately following
> angle-bracket block.
> 
> Because tach-ch = /bits/ 8 <0x0>, <0x8>; separates the values with a comma,
> the second block reverts to the default 32-bit cell size. This produces a
> 5-byte array [0x00, 0x00, 0x00, 0x00, 0x08] rather than the intended 2-byte
> array.
> 
> If a developer copies this example into a real device tree, will the driver
> incorrectly interpret it as 5 separate tachometer channels (0, 0, 0, 0, 8)
> instead of the intended 2 channels (0, 8)?
> 
> Should this be written as /bits/ 8 <0x0 0x8>; or using byte array syntax
> like [00 08] instead?

Yes. The former style though.

Rob

Re: [PATCH v1 1/2] dt-bindings: hwmon: Add starfive,jhb100-fan-tach

[Rob Herring]

On Tue, Apr 28, 2026 at 01:23:36AM -0700, Changhuang Liang wrote:
> Add compatible "starfive,jhb100-fan-tach" for StarFive JHB100 Fan-Tach
> controller.
> 
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  .../hwmon/starfive,jhb100-fan-tach.yaml       | 98 +++++++++++++++++++
>  1 file changed, 98 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
> new file mode 100644
> index 000000000000..fdc8539ec804
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/starfive,jhb100-fan-tach.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JHB100 Fan-Tach controller
> +
> +maintainers:
> +  - Changhuang Liang <changhuang.liang@starfivetech.com>
> +
> +description:
> +  The StarFive Fan-Tach controller can support up to 16 fan tach input.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - starfive,jhb100-fan-tach
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  "^fan@[0-9]+$":

Unit-addresses are hex.

> +    $ref: fan-common.yaml#
> +    unevaluatedProperties: false
> +    required:
> +      - reg
> +      - tach-ch

You need constraints on the 'reg' values. Also, what does 'reg' 
represent in this case?

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    fan-tach-controller@11be0000 {
> +        compatible = "starfive,jhb100-fan-tach";
> +        reg = <0x11be0000 0x400>;
> +        interrupts = <128>;
> +        clocks = <&per2crg 33>;
> +        resets = <&per2crg 5>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        fan@0 {
> +            reg = <0>;
> +            tach-ch = /bits/ 8 <0x0>, <0x8>;

How do you have 1 fan with 2 tach signals?

Rob

Re: [PATCH v1 1/2] dt-bindings: hwmon: Add starfive,jhb100-fan-tach

[Changhuang Liang]

Hi, Rob

Thanks for the review.

> On Tue, Apr 28, 2026 at 01:23:36AM -0700, Changhuang Liang wrote:
> > Add compatible "starfive,jhb100-fan-tach" for StarFive JHB100 Fan-Tach
> > controller.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > ---
> >  .../hwmon/starfive,jhb100-fan-tach.yaml       | 98
> +++++++++++++++++++
> >  1 file changed, 98 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
> >
> > diff --git
> >
> a/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yam
> > l
> >
> b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yam
> > l
> > new file mode 100644
> > index 000000000000..fdc8539ec804
> > --- /dev/null
> > +++
> b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach
> > +++ .yaml
> > @@ -0,0 +1,98 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2
> > +---
> > +$id:
> > +http://devicetree.org/schemas/hwmon/starfive,jhb100-fan-tach.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: StarFive JHB100 Fan-Tach controller
> > +
> > +maintainers:
> > +  - Changhuang Liang <changhuang.liang@starfivetech.com>
> > +
> > +description:
> > +  The StarFive Fan-Tach controller can support up to 16 fan tach input.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - starfive,jhb100-fan-tach
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^fan@[0-9]+$":
> 
> Unit-addresses are hex.
> 
> > +    $ref: fan-common.yaml#
> > +    unevaluatedProperties: false
> > +    required:
> > +      - reg
> > +      - tach-ch
> 
> You need constraints on the 'reg' values. Also, what does 'reg'
> represent in this case?

"reg" is only used to distinguish between different fan instances.

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - resets
> > +  - interrupts
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    fan-tach-controller@11be0000 {
> > +        compatible = "starfive,jhb100-fan-tach";
> > +        reg = <0x11be0000 0x400>;
> > +        interrupts = <128>;
> > +        clocks = <&per2crg 33>;
> > +        resets = <&per2crg 5>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        fan@0 {
> > +            reg = <0>;
> > +            tach-ch = /bits/ 8 <0x0>, <0x8>;
> 
> How do you have 1 fan with 2 tach signals?

On our platform, two fans share one PWM channel. It seems there was a misunderstanding earlier.
I will change it to the form of one tach per fan.

Best Regards,
Changhuang

Re: [PATCH v1 1/2] dt-bindings: hwmon: Add starfive,jhb100-fan-tach

[Rob Herring]

On Wed, May 6, 2026 at 8:36 PM Changhuang Liang
<changhuang.liang@starfivetech.com> wrote:
>
> Hi, Rob
>
> Thanks for the review.
>
> > On Tue, Apr 28, 2026 at 01:23:36AM -0700, Changhuang Liang wrote:
> > > Add compatible "starfive,jhb100-fan-tach" for StarFive JHB100 Fan-Tach
> > > controller.
> > >
> > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > ---
> > >  .../hwmon/starfive,jhb100-fan-tach.yaml       | 98
> > +++++++++++++++++++
> > >  1 file changed, 98 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
> > >
> > > diff --git
> > >
> > a/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yam
> > > l
> > >
> > b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yam
> > > l
> > > new file mode 100644
> > > index 000000000000..fdc8539ec804
> > > --- /dev/null
> > > +++
> > b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach
> > > +++ .yaml
> > > @@ -0,0 +1,98 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2
> > > +---
> > > +$id:
> > > +http://devicetree.org/schemas/hwmon/starfive,jhb100-fan-tach.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: StarFive JHB100 Fan-Tach controller
> > > +
> > > +maintainers:
> > > +  - Changhuang Liang <changhuang.liang@starfivetech.com>
> > > +
> > > +description:
> > > +  The StarFive Fan-Tach controller can support up to 16 fan tach input.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - starfive,jhb100-fan-tach
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  resets:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  '#address-cells':
> > > +    const: 1
> > > +
> > > +  '#size-cells':
> > > +    const: 0
> > > +
> > > +patternProperties:
> > > +  "^fan@[0-9]+$":
> >
> > Unit-addresses are hex.
> >
> > > +    $ref: fan-common.yaml#
> > > +    unevaluatedProperties: false
> > > +    required:
> > > +      - reg
> > > +      - tach-ch
> >
> > You need constraints on the 'reg' values. Also, what does 'reg'
> > represent in this case?
>
> "reg" is only used to distinguish between different fan instances.
>
> >
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - resets
> > > +  - interrupts
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    fan-tach-controller@11be0000 {
> > > +        compatible = "starfive,jhb100-fan-tach";
> > > +        reg = <0x11be0000 0x400>;
> > > +        interrupts = <128>;
> > > +        clocks = <&per2crg 33>;
> > > +        resets = <&per2crg 5>;
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        fan@0 {
> > > +            reg = <0>;
> > > +            tach-ch = /bits/ 8 <0x0>, <0x8>;
> >
> > How do you have 1 fan with 2 tach signals?
>
> On our platform, two fans share one PWM channel. It seems there was a misunderstanding earlier.
> I will change it to the form of one tach per fan.

I think you have to leave it like this as you can't have 2 fan nodes
at the same address. Just need to fix the size.

Rob