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

[Guenter Roeck <linux@roeck-us.net>]

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@starfivetech.com>
>>>>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>>>>> Signed-off-by: Changhuang Liang
>>>>> <changhuang.liang@starfivetech.com>
>>>>> ---
>> ...
>>>>> +
>>>>> +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