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

[Guenter Roeck]

On Thu, Aug 20, 2026 at 10:15:52AM +0000, Changhuang Liang wrote:
> 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>
> > > ---
...
> > > +
> > > +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.
> 

So how long does it take to execute the "sensors" command with 16 active fans ?

...
> > > +
> > > +	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.

Just for use in this function ? Why ? It is not used after probe.

Guenter