Re: [PATCH v1 4/4] hwmon: add driver for the Microchip LAN966x SoC

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

On 3/27/22 07:18, Michael Walle wrote:
> Am 2022-03-27 03:34, schrieb Guenter Roeck:
> 
>>> +    /*
>>> +     * Data is given in pulses per second. According to the hwmon ABI we
>>> +     * have to assume two pulses per revolution.
>>
>> The hwmon ABI doesn't make any such assumptions. It wants to see RPM,
>> that is all. Pulses per revolution is a fan property.
> 
> There is fanY_pulses according to Documentation/ABI/testing/sysfs-class-hwmon:
> 
>    Should only be created if the chip has a register to configure
>    the number of pulses. In the absence of such a register (and
>    thus attribute) the value assumed by all devices is 2 pulses
>    per fan revolution.
> 
> The hardware returns just the pulses per second. Doesn't that
> mean I have to divide that value by two?
> 

The above refers to hardware which reports RPM.

It is up to the driver to calculate and return RPM. How you do it is your
decision. Drivers should report the most likely correct RPM value to
userspace, one that rarely needs manual adjustment. Almost all fans
report two pulses per revolution, so normally that assumption is used
to convert PPM to RPM. That isn't mandated (or supposed to be mandated)
by the ABI. I would call it common sense.

I'll be happy to accept a patch clarifying this.

>>> +     */
>>> +    *val = FIELD_GET(FAN_CNT_DATA, data) * 60 / 2;
> 
> .. otherwise this should then be
> *val = FIELD_GET(FAN_CNT_DATA, data) * 60;
> 

If you really want to do this, make sure it is well documented that users
will need to adjust the fan speed via sensors3.conf to get the real fan speed.

> 
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int lan966x_hwmon_read_pwm(struct device *dev, long *val)
>>> +{
>>> +    struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
>>> +    unsigned int data;
>>> +    int ret;
>>> +
>>> +    ret = regmap_read(hwmon->regmap_fan, FAN_CFG, &data);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    *val = FIELD_GET(FAN_CFG_DUTY_CYCLE, data);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int lan966x_hwmon_read_pwm_freq(struct device *dev, long *val)
>>> +{
>>> +    struct lan966x_hwmon *hwmon = dev_get_drvdata(dev);
>>> +    unsigned long rate = clk_get_rate(hwmon->clk);
>>
>> Is that a dynamic frequency ? If not, it would be better to read it once
>> and store it in struct lan966x_hwmon.
> 
> yes it is configurable, actually. See lan966x_hwmon_write_pwm_freq().
> 

That is the pwm frequency, not the clock frequency. I don't see any
code which updates the clock frequency reported by clk_get_rate(hwmon->clk),
ie I don't see a call to clk_set_rate().

Thanks,
Guenter