Re: [PATCH v2 2/2] hwmon: (starfive-fan-tach) Add fan tach driver for StarFive JHB100

[Changhuang Liang]

Hi, Guenter

Thanks for the review.

> On 8/19/26 02:16, Changhuang Liang wrote:
> > Add fan tach driver for StarFive JHB100 SoC.
> >
> > The controller supports up to 16 independent fan tachometer inputs and
> > reports fan speed in RPM through the hwmon sysfs interface, along with
> > stall and low-speed alarms.
> >
> > For this controller, the special clock and reset operation sequence is:
> >    probe: clk_prepare_enable() than reset_control_deassert()
> >    remove: clk_disable_unprepare() than reset_control_assert()
> 
> s/than/then/
> 
> >
> > Co-developed-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> > Signed-off-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > ---
> >   Documentation/hwmon/index.rst             |   1 +
> >   Documentation/hwmon/starfive-fan-tach.rst |  34 ++
> >   MAINTAINERS                               |   8 +
> >   drivers/hwmon/Kconfig                     |  14 +
> >   drivers/hwmon/Makefile                    |   1 +
> >   drivers/hwmon/starfive-fan-tach.c         | 561
> ++++++++++++++++++++++
> >   6 files changed, 619 insertions(+)
> >   create mode 100644 Documentation/hwmon/starfive-fan-tach.rst
> >   create mode 100644 drivers/hwmon/starfive-fan-tach.c
> >
> > diff --git a/Documentation/hwmon/index.rst
> > b/Documentation/hwmon/index.rst index 29130df44d12..0d64e4cf1051
> > 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -253,6 +253,7 @@ Hardware Monitoring Kernel Drivers
> >      smsc47m1
> >      sparx5-temp
> >      spd5118
> > +   starfive-fan-tach
> >      stpddc60
> >      surface_fan
> >      sy7636a-hwmon
> > diff --git a/Documentation/hwmon/starfive-fan-tach.rst
> > b/Documentation/hwmon/starfive-fan-tach.rst
> > new file mode 100644
> > index 000000000000..5ca03b52ad58
> > --- /dev/null
> > +++ b/Documentation/hwmon/starfive-fan-tach.rst
> > @@ -0,0 +1,34 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +Kernel driver starfive-fan-tach
> > +===============================
> > +
> > +Supported chips:
> > +
> > +  * StarFive JHB100
> > +
> > +    Prefix: 'starfive_fan_tach'
> > +
> > +    Addresses scanned: -
> > +
> > +Authors:
> > +      - Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > +
> > +Description
> > +-----------
> > +
> > +This driver implements support for the fan tachometer controller
> > +found on the StarFive JHB100 SoC. The controller supports up to 16
> > +independent fan tachometer inputs. Each tachometer channel measures
> > +the number of pulses within a fixed 100 ms window.
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +====================
> =======================================================
> > +fan[1-16]_input      Fan speed in RPM (read-only)
> > +fan[1-16]_min        Lower fan speed limit in RPM (read/write)
> > +fan[1-16]_enable     Enable/disable the tachometer channel
> (read/write)
> > +fan[1-16]_fault      Fan stall indication (read-only)
> > +fan[1-16]_min_alarm  Fan speed below fan[1-16]_min (read-only)
> > +====================
> > +=======================================================
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > c545aa121d51..83318d2e3e2e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -25897,6 +25897,14 @@ S:	Supported
> >   F:
> 	Documentation/devicetree/bindings/interrupt-controller/starfive,jhb100
> -intc.yaml
> >   F:	drivers/irqchip/irq-starfive-jhb100-intc.c
> >
> > +STARFIVE JHB100 FAN-TACH DRIVER
> > +M:	Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > +L:	linux-hwmon@xxxxxxxxxxxxxxx
> > +S:	Supported
> > +F:
> 	Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.ya
> ml
> > +F:	Documentation/hwmon/starfive-fan-tach.rst
> > +F:	drivers/hwmon/starfive-fan-tach.c
> > +
> >   STATIC BRANCH/CALL
> >   M:	Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> >   M:	Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index
> > 5c2d3ff5fce8..2c0000f8e777 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -2383,6 +2383,20 @@ config SENSORS_SPD5118_DETECT
> >
> >   	  If unsure, say Y.
> >
> > +config SENSORS_STARFIVE_FAN_TACH
> > +	tristate "StarFive Fan-Tach driver"
> > +	depends on ARCH_STARFIVE || COMPILE_TEST
> > +	depends on HAS_IOMEM
> > +	help
> > +	  This driver provides support for the fan tachometer controller
> > +	  found on StarFive SoCs such as the JHB100. The controller supports
> > +	  up to 16 independent fan tachometer inputs and reports fan speed
> > +	  in RPM through the hwmon sysfs interface, along with stall and
> > +	  low-speed alarms.
> > +
> > +	  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
> > 63effc0ab8d1..80b2704b94e5 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -223,6 +223,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..1e6399db424d
> > --- /dev/null
> > +++ b/drivers/hwmon/starfive-fan-tach.c
> > @@ -0,0 +1,561 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * FAN-TACH controller driver for StarFive JHB100
> > + *
> > + * Copyright (C) 2018-2026 StarFive Technology Co., Ltd.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/kernel.h>
> > +#include <linux/math64.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +#include <linux/spinlock.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	10
> > +#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;
> > +	/*
> > +	 * Serialises the read-modify-write sequences on CH_EN and INT_EN,
> > +	 * and the arm/disarm window of the fault/min_alarm read paths.
> > +	 */
> > +	struct mutex lock;
> 
> I don't see why this would be needed on top of the hwmon subsystem lock.

Will drop it.

> 
> > +	void __iomem *regs;
> > +	struct reset_control *rst;
> > +	struct clk *clk;
> > +	bool tach_present[STARFIVE_FAN_TACH_CH];
> > +	unsigned long clk_rate; /* APB clock frequency */
> > +	u8 pulses_per_rev[STARFIVE_FAN_TACH_CH];
> > +	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];
> > +	u32 armed_stall;
> > +	u32 armed_slow;
> > +	/* protects fan_stall[]/fan_slow[]/armed_* and the STATUS register */
> > +	spinlock_t irq_lock;
> > +};
> > +
> > +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 u32 starfive_fan_tach_rpm_to_val(struct starfive_fan_tach_data
> > +*priv, u8 tach_ch, u32 rpm) {
> > +	u64 tach_val;
> > +	u8 ppr;
> > +
> > +	ppr = priv->pulses_per_rev[tach_ch];
> > +
> > +	tach_val = (u64)rpm * ppr;
> > +	tach_val = div_u64(tach_val,
> STARFIVE_FAN_DEFAULT_RPM_PAUSE_TIME);
> > +
> > +	return min_t(u64, tach_val, STARFIVE_FAN_TACH_VALUE_MASK); }
> > +
> > +static long starfive_fan_tach_val_to_rpm(struct starfive_fan_tach_data
> *priv, u8 tach_ch,
> > +					 u32 tach_val)
> > +{
> > +	u64 rpm;
> > +	u8 ppr;
> > +
> > +	ppr = priv->pulses_per_rev[tach_ch];
> > +
> > +	rpm = (u64)tach_val * STARFIVE_FAN_DEFAULT_RPM_PAUSE_TIME;
> > +
> > +	return div_u64(rpm, ppr);
> > +}
> > +
> > +static int starfive_fan_tach_get_rpm(struct starfive_fan_tach_data *priv,
> > +				     u8 tach_ch, long *rpm)
> > +{
> > +	u32 val;
> > +	int ret;
> > +
> > +	ret = readl_poll_timeout(priv->regs +
> STARFIVE_FAN_TACH_SPEED(tach_ch),
> > +				 val, val & STARFIVE_FAN_TACH_SPEED_VALID,
> > +				 100, STARFIVE_FAN_TACH_TIMEOUT);
> > +	if (ret)
> > +		return -ENODATA;
> > +
> > +	val = FIELD_GET(STARFIVE_FAN_TACH_VALUE_MASK, val);
> > +	if (!val) {
> > +		/* Sampling = 0 may mean no valid capture; wait 1.5 more cycles for
> valid read. */
> > +		fsleep(STARFIVE_FAN_TACH_TIMEOUT +
> STARFIVE_FAN_TACH_TIMEOUT / 2);
> > +		ret = readl_poll_timeout(priv->regs +
> STARFIVE_FAN_TACH_SPEED(tach_ch),
> > +					 val, val & STARFIVE_FAN_TACH_SPEED_VALID,
> > +					 100, STARFIVE_FAN_TACH_TIMEOUT);
> > +		if (ret)
> > +			return -ENODATA;
> > +
> > +		val = FIELD_GET(STARFIVE_FAN_TACH_VALUE_MASK, val);
> > +	}
> > +
> > +	*rpm = starfive_fan_tach_val_to_rpm(priv, tach_ch, val);
> > +
> > +	return 0;
> > +}
> > +
> > +static long starfive_fan_tach_get_rpm_threshold(struct
> starfive_fan_tach_data *priv,
> > +						u8 tach_ch)
> > +{
> > +	u32 val;
> > +
> > +	val = readl(priv->regs + STARFIVE_FAN_TACH_THRESHOLD(tach_ch));
> > +
> > +	val = FIELD_GET(STARFIVE_FAN_TACH_VALUE_MASK, val);
> > +
> > +	return starfive_fan_tach_val_to_rpm(priv, tach_ch, val); }
> > +
> > +static void starfive_set_tach_rpm_threshold(struct starfive_fan_tach_data
> *priv,
> > +					    u8 tach_ch, u32 val)
> > +{
> > +	u32 reg_val;
> > +
> > +	reg_val = starfive_fan_tach_rpm_to_val(priv, tach_ch, val);
> > +	reg_val = clamp_val(reg_val, 1, STARFIVE_FAN_TACH_VALUE_MASK);
> > +
> > +	writel(reg_val, priv->regs + STARFIVE_FAN_TACH_THRESHOLD(tach_ch));
> > +}
> > +
> > +static int starfive_fan_tach_get_ch_en(struct starfive_fan_tach_data
> *priv,
> > +				       u8 tach_ch)
> 
> Why not bool ?
> 
> > +{
> > +	u32 enable;
> > +
> > +	enable = readl(priv->regs + STARFIVE_FAN_TACH_CH_EN);
> > +
> > +	return (enable & (1 << tach_ch)) ? 1 : 0;
> 
> 	return !!(enable & STARFIVE_FAN_TACH_EN(tach_ch));
> 
> > +}
> > +
> > +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);
> > +	int ret = 0;
> > +
> > +	switch (attr) {
> > +	case hwmon_fan_fault:
> > +		scoped_guard(mutex, &priv->lock) {
> > +			scoped_guard(spinlock_irqsave, &priv->irq_lock) {
> > +				writel(STARFIVE_FAN_TACH_STALL_INT(channel),
> > +				       priv->regs + STARFIVE_FAN_TACH_STATUS);
> > +				/* clear fan_stall first */
> > +				priv->fan_stall[channel] = 0;
> > +				reinit_completion(&priv->comp_stall[channel]);
> > +				priv->armed_stall |= BIT(channel);
> > +			}
> > +
> > +			starfive_fan_tach_ch_stall_unmask(priv, channel, true);
> > +
> > +			/* Waiting for hardware to measure */
> > +			wait_for_completion_timeout(&priv->comp_stall[channel],
> > +						    2 *
> STARFIVE_FAN_TACH_TIMEOUT_JIFFIES);
> > +
> > +			starfive_fan_tach_ch_stall_unmask(priv, channel, false);
> > +
> > +			scoped_guard(spinlock_irqsave, &priv->irq_lock) {
> > +				priv->armed_stall &= ~BIT(channel);
> > +				*val = priv->fan_stall[channel];
> > +			}
> > +		}
> > +
> 
> I'd really be interested to see how long it takes to read the alarm and fault
> status of all fans. And, yes, Sashiko has a point: the attributes are supported to
> report a sticky status, not something that is calculated on the fly. Is this based
> on some heuristics ? I don't claim to understand how fault and min_alarm are
> calculated. A comment in the code describing how this works would be
> helpful.

There is no real calculation for fault and min_alarm, the hardware simply counts the 
corresponding pulses within a specified time period and reports the counts via interrupt 
when the counting period expires. I will try to modify it so that the previous statistical 
result is recorded, and when reading, the previous result is returned,  thus eliminating 
the need to wait for the measurement.

> 
> > +		break;
> > +	case hwmon_fan_input:
> > +		ret = starfive_fan_tach_get_ch_en(priv, channel);
> > +		if (!ret)
> > +			return -ENODATA;
> 
> I don't see why the assignment to ret would add value here.
> 
> 		if (!starfive_fan_tach_get_ch_en(priv, channel))
> 			return -ENODATA;
> 
> > +
> > +		ret = starfive_fan_tach_get_rpm(priv, channel, val);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		break;
> > +	case hwmon_fan_min:
> > +		*val = starfive_fan_tach_get_rpm_threshold(priv, channel);
> > +
> > +		break;
> > +	case hwmon_fan_min_alarm:
> > +		scoped_guard(mutex, &priv->lock) {
> > +			scoped_guard(spinlock_irqsave, &priv->irq_lock) {
> > +				writel(STARFIVE_FAN_TACH_SLOW_INT(channel),
> > +				       priv->regs + STARFIVE_FAN_TACH_STATUS);
> > +				/* clear fan_slow first */
> > +				priv->fan_slow[channel] = 0;
> > +				reinit_completion(&priv->comp_slow[channel]);
> > +				priv->armed_slow |= BIT(channel);
> > +			}
> > +
> > +			starfive_fan_tach_ch_slow_unmask(priv, channel, true);
> > +
> > +			/* Waiting for hardware to measure */
> > +			wait_for_completion_timeout(&priv->comp_slow[channel],
> > +						    2 *
> STARFIVE_FAN_TACH_TIMEOUT_JIFFIES);
> > +
> > +			starfive_fan_tach_ch_slow_unmask(priv, channel, false);
> > +
> > +			scoped_guard(spinlock_irqsave, &priv->irq_lock) {
> > +				priv->armed_slow &= ~BIT(channel);
> > +				*val = priv->fan_slow[channel];
> > +			}
> > +		}
> > +
> > +		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);
> > +
> > +	guard(mutex)(&priv->lock);
> > +
> > +	switch (attr) {
> > +	case hwmon_fan_min:
> > +		if (val < 0 || val > U32_MAX)
> > +			return -EINVAL;
> > +
> > +		starfive_set_tach_rpm_threshold(priv, channel, val);
> > +		break;
> > +	case hwmon_fan_enable:
> > +		if (val != 0 && val != 1)
> > +			return -EINVAL;
> > +
> > +		starfive_fan_tach_ch_enable(priv, channel, 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_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 int starfive_fan_tach_create_fan(struct device *dev, struct
> device_node *child,
> > +					struct starfive_fan_tach_data *priv) {
> > +	u8 tach_ch[STARFIVE_FAN_TACH_CH];
> > +	int ret, count;
> > +	u32 ppr, index;
> > +
> > +	count = of_property_count_u8_elems(child, "tach-ch");
> > +	if (count < 1 || count > STARFIVE_FAN_TACH_CH)
> > +		return -EINVAL;
> > +
> > +	ret = of_property_read_u8_array(child, "tach-ch", tach_ch, count);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Parse pulses-per-revolution, default to 2 if not specified */
> > +	ppr = STARFIVE_FAN_DEFAULT_PULSE_PR;
> > +	of_property_read_u32(child, "pulses-per-revolution", &ppr);
> > +
> > +	if (!ppr || ppr > 4) {
> > +		dev_err(dev, "Invalid pulses-per-revolution %u, must be 1-4\n",
> ppr);
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (index = 0; index < count; index++) {
> > +		u8 ch = tach_ch[index];
> > +
> > +		if (ch >= STARFIVE_FAN_TACH_CH) {
> > +			dev_warn(priv->dev, "Invalid tach-ch %d, skipping\n", ch);
> > +			continue;
> 
> This should also return an error. Also, this is the only use of priv->dev, and
> priv->dev == dev. priv->dev is therefore unnecessary.
> 

Thank you for pointing that out. I noticed that the `struct device *dev` passed in the function 
starfive_fan_tach_create_fan() is somewhat redundant, so I plan to remove it directly and 
keep `priv->dev` instead.

> > +		}
> > +
> > +		priv->pulses_per_rev[ch] = ppr;
> 
> What is the point of storing the same ppr value separately for each channel ?
> 
> > +		priv->tach_present[ch] = true;
> > +		starfive_fan_tach_ch_enable(priv, ch, true);
> > +	}
> > +
> > +	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;
> > +
> > +	guard(spinlock)(&priv->irq_lock);
> > +
> > +	fan_status = readl(priv->regs + STARFIVE_FAN_TACH_STATUS);
> > +	if (!fan_status)
> > +		return IRQ_NONE;
> > +
> > +	writel(fan_status, priv->regs + STARFIVE_FAN_TACH_STATUS);
> > +
> > +	for (i = 0; i < STARFIVE_FAN_TACH_CH; i++) {
> > +		if (!priv->tach_present[i])
> > +			continue;
> > +
> > +		if ((fan_status & STARFIVE_FAN_TACH_STALL_INT(i)) &&
> > +		    (priv->armed_stall & BIT(i))) {
> > +			priv->fan_stall[i] = 1;
> > +			complete(&priv->comp_stall[i]);
> > +		}
> > +
> > +		if ((fan_status & STARFIVE_FAN_TACH_SLOW_INT(i)) &&
> > +		    (priv->armed_slow & BIT(i))) {
> > +			priv->fan_slow[i] = 1;
> > +			complete(&priv->comp_slow[i]);
> > +		}
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void starfive_fan_tach_disable(void *data) {
> > +	struct starfive_fan_tach_data *priv = data;
> > +
> > +	clk_disable_unprepare(priv->clk);
> > +	reset_control_assert(priv->rst);
> > +}
> > +
> > +static int starfive_fan_tach_probe(struct platform_device *pdev) {
> > +	struct device *hwmon_dev, *dev = &pdev->dev;
> > +	struct starfive_fan_tach_data *priv;
> > +	int irq;
> > +	int ret;
> > +	u32 i;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->dev = dev;
> > +	spin_lock_init(&priv->irq_lock);
> > +
> > +	for (i = 0; i < STARFIVE_FAN_TACH_CH; i++) {
> > +		init_completion(&priv->comp_stall[i]);
> > +		init_completion(&priv->comp_slow[i]);
> > +	}
> > +
> > +	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");
> > +
> > +	priv->clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(priv->clk))
> > +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> > +				     "Unable to get fan tach's clock\n");
> > +
> > +	priv->rst = devm_reset_control_get_exclusive(dev, NULL);
> > +	if (IS_ERR(priv->rst))
> > +		return dev_err_probe(dev, PTR_ERR(priv->rst),
> > +				     "Unable to get fan tach's reset\n");
> > +
> > +	ret = clk_prepare_enable(priv->clk);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to enable clock\n");
> > +
> > +	priv->clk_rate = clk_get_rate(priv->clk);
> > +	if (!priv->clk_rate) {
> > +		clk_disable_unprepare(priv->clk);
> > +		return dev_err_probe(dev, -EINVAL, "Unable to get clock's rate\n");
> > +	}
> > +
> > +	ret = reset_control_deassert(priv->rst);
> > +	if (ret) {
> > +		clk_disable_unprepare(priv->clk);
> > +		return dev_err_probe(dev, ret, "Failed to deassert reset\n");
> > +	}
> > +
> > +	ret = devm_add_action_or_reset(dev, starfive_fan_tach_disable, priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_mutex_init(dev, &priv->lock);
> > +	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)
> > +			return dev_err_probe(dev, ret, "Failed to create fan %pOF\n",
> child);
> > +	}
> > +
> > +	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,
> priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	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 */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, starfive_fan_tach_of_match);
> > +
> > +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@xxxxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Changhuang Liang
> > +<changhuang.liang@xxxxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("StarFive JHB100 Fan Tach device driver");
> > +MODULE_LICENSE("GPL");

Best Regards,
Changhuang