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

[Guenter Roeck]

On 8/20/26 18:06, 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 ?
>
> # time sensors
> starfive_fan_tach-isa-0000
> Adapter: ISA adapter
> fan1:           FAULT  (min =  300 RPM)
> fan2:           FAULT  (min =  300 RPM)
> fan3:           FAULT  (min =  300 RPM)
> fan4:           FAULT  (min =  300 RPM)
> fan5:           FAULT  (min =  300 RPM)
> fan6:           FAULT  (min =  300 RPM)
> fan7:           FAULT  (min =  300 RPM)
> fan8:           FAULT  (min =  300 RPM)
> fan9:           FAULT  (min =  300 RPM)
> fan10:          FAULT  (min =  300 RPM)
> fan11:          FAULT  (min =  300 RPM)
> fan12:          FAULT  (min =  300 RPM)
> fan13:          FAULT  (min =  300 RPM)
> fan14:          FAULT  (min =  300 RPM)
> fan15:          FAULT  (min =  300 RPM)
> fan16:       1800 RPM  (min =  300 RPM)
>
>
> real    0m4.865s
> user    0m0.023s
> sys     0m0.001s
>
>
> > ...
> > > > > +
> > > > > +	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.
>
> After probe, priv->dev is also used. In the starfive_fan_tach_create_fan() function,
> priv->dev can be used directly as well, so I plan to simply remove struct device *dev.

Are you checking if I pay attention ? starfive_fan_tach_create_fan()
is called from the probe function, not "after" probe.

Guenter