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

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 <[email protected]>
> > > > Signed-off-by: William Qiu <[email protected]>
> > > > Signed-off-by: Changhuang Liang
> > > > <[email protected]>
> > > > ---
> ...
> > > > +
> > > > +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.

static int starfive_fan_tach_create_fan(struct device *dev, struct device_node *child,
                                        struct starfive_fan_tach_data *priv)

===》

static int starfive_fan_tach_create_fan(struct device_node *child,
                                        struct starfive_fan_tach_data *priv)

Best Regards,
Changhuang