Hi Guenter,
On 2022/11/1, 9:15 PM, "Guenter Roeck" <groeck7@xxxxxxxxx on behalf of linux@xxxxxxxxxxxx> wrote:
On Tue, Nov 01, 2022 at 05:51:56PM +0800, Billy Tsai wrote:
> > +
> > + /* Restart the Tach channel to guarantee the value is fresh */
> > + aspeed_tach_ch_enable(priv, fan_tach_ch, false);
> > + aspeed_tach_ch_enable(priv, fan_tach_ch, true);
> Is that really needed ? Doesn't the controller measure values continuously ?
Yes, the controller will measure values continuously by hardware. I will remove it.
If the user want to get the fresh value, it should be done by the application layer
(e.g. read two times).
> > +
> > + if (ret) {
> > + /* return 0 if we didn't get an answer because of timeout*/
> > + if (ret == -ETIMEDOUT)
> > + return 0;
> > + else
> > + return ret;
> else after return is unnecessary, and why would a timeout be be ignored ?
When the user sets the correct fan information (i.e., min_rpm, max_rpm), the read
poll timeout will only occur if the tach pin does not get any signal (i.e. rpm=0).
> > + }
> > +
> > + raw_data = val & TACH_ASPEED_VALUE_MASK;
> > + /*
> > + * We need the mode to determine if the raw_data is double (from
> > + * counting both edges).
> > + */
> > + if (priv->tach_channel[fan_tach_ch].tach_edge == BOTH_EDGES)
> > + raw_data <<= 1;
> > +
> > + tach_div = raw_data * (priv->tach_channel[fan_tach_ch].divisor) *
> > + (priv->tach_channel[fan_tach_ch].pulse_pr);
> > +
> > + clk_source = clk_get_rate(priv->clk);
> > + dev_dbg(priv->dev, "clk %ld, raw_data %d , tach_div %d\n", clk_source,
> > + raw_data, tach_div);
> > +
> > + if (tach_div == 0)
> > + return -EDOM;
> If the fan is off or not connected, would that return an error ?
> If so, that would be inappropriate; it should return a speed
> of 0 in that case.
It will be handled by the regmap_read_poll_timeout.
Thanks
Best Regards,
Billy Tsai
