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

[Guenter Roeck]

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