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

On 8/19/26 21:57, Guenter Roeck wrote:
> 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_create_fan(struct device *dev, struct device_node *child,
>> +                    struct starfive_fan_tach_data *priv)
>> +{
>> +    u8 tach_ch[STARFIVE_FAN_TACH_CH];
>> +    int ret, count;
>> +    u32 ppr, index;
>> +
>> +    count = of_property_count_u8_elems(child, "tach-ch");
>> +    if (count < 1 || count > STARFIVE_FAN_TACH_CH)
>> +        return -EINVAL;
>> +
>> +    ret = of_property_read_u8_array(child, "tach-ch", tach_ch, count);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* Parse pulses-per-revolution, default to 2 if not specified */
>> +    ppr = STARFIVE_FAN_DEFAULT_PULSE_PR;
>> +    of_property_read_u32(child, "pulses-per-revolution", &ppr);
>> +
>> +    if (!ppr || ppr > 4) {
>> +        dev_err(dev, "Invalid pulses-per-revolution %u, must be 1-4\n", ppr);
>> +        return -EINVAL;
>> +    }
>> +
>> +    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.
> 
>> +        }
>> +
>> +        priv->pulses_per_rev[ch] = ppr;
> 
> What is the point of storing the same ppr value separately for each channel ?
> 

Sorry, not reading the code correctly. Ignore the noise.

Guenter