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

Hi, Guenter

Thanks for the review.

> 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 <[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.
> >
> 
> Are you checking if I pay attention ? starfive_fan_tach_create_fan() is called
> from the probe function, not "after" probe.
> 

Sorry, I initially misunderstood your point. You're right, I re-examined the code 
and realized that priv->dev is only used in starfive_fan_tach_create_fan() during probe, 
and not used after probe at all. So it's indeed unnecessary.

I'll fix it as you suggested:

Remove struct device *dev from struct starfive_fan_tach_data.

Remove priv->dev = dev; from probe.

Keep the struct device *dev parameter in starfive_fan_tach_create_fan() (passed from probe) 
and use it directly instead of priv->dev.

Thanks for your patience.

Best Regards,
Changhuang