Re: [PATCH net-next 3/3] dpll: zl3073x: add hwmon support for input reference frequencies

[Paolo Abeni]

On 3/24/26 6:16 AM, Guenter Roeck wrote:
> On 3/23/26 15:48, Jakub Kicinski wrote:
>> On Fri, 20 Mar 2026 14:48:01 +0100 Ivan Vecera wrote:
>>> On 3/20/26 1:21 PM, Guenter Roeck wrote:
>>>> On 3/20/26 03:59, Ivan Vecera wrote:
>>>>> Expose measured input reference frequencies via the hwmon interface
>>>>> using custom sysfs attributes (freqN_input and freqN_label) since
>>>>> hwmon has no native frequency sensor type. The frequency values are
>>>>> read from the cached measurements updated by the periodic work thread.
>>>>>
>>>>> Cache the device ready state in struct zl3073x_dev so that
>>>>> freq_input_show() can return -ENODATA without an I2C access when
>>>>> the device firmware is not configured.
>>>>>
>>>>> Signed-off-by: Ivan Vecera <ivecera@xxxxxxxxxx>
>>>>
>>>> "frequency" is not a hardware monitoring attribute. I understand that it is
>>>> convenient to report it as one, and that other drivers implement it as
>>>> well,
>>>> but that doesn't change that.
>>>>
>>>> I understand that the code lives outside the hardware monitoring
>>>> subsystem and is
>>>> thus not in control of its maintainers, so you can essentially do
>>>> whatever you want,
>>>> even if it is wrong. That doesn't change the fact that it is wrong.
>>>>
>>>> However, do _not_ try to add it into the official list of hardware
>>>> monitoring
>>>> attributes. I would NACK that.
>>>
>>> Understood. I recognize that frequency falls outside the strict scope of
>>> hardware monitoring and does not belong in the official hwmon ABI.
>>>
>>> I'm using it here as a convenient way to expose these specific driver
>>> metrics, but I hear you loud and clear. I will absolutely not propose
>>> adding frequency to the official list of hwmon attributes or
>>> documentation.
>>>
>>> Thank you for your time and for reviewing the patch.
>>
>> Guenter, should this be a debugfs interface, then?
>>
> 
> There is nothing that prevents the actual (generated ?) frequency to
> be reported as sysfs attribute attached to the chip (spi) driver, if
> it is indeed of interest. If it is of interest for all dpll drivers,
> it could be attached to the dpll device, the chip driver could make it
> available via dpll_device_ops to the dpll subsystem, and the dpll
> subsystem could provide a common API function (such as the existing
> temp_get) and generate a common set of sysfs attributes for all dpll
> devices.
> 
>> Also an hwmon noob question - isn't it better for the monitoring
>> interface to report frequency error / instability in this case
>> instead of absolute value? Or do you not know the expected freq?
>>
> In the hardware monitoring world one would have min/max attributes and
> one or more alarm attributes. I never heard about the idea of reporting
> deviations, and for typical hardware monitoring attributes it does not
> really make sense. What would be a "deviation" of a temperature/
> voltage/current/power/humidity sensor ? Such attributes typically have
> an operational range, and they are allowed and even expected to
> fluctuate within that range.

Ivan, my take on all the above is that using the HWmon interface here is
stretching it too much. I think it would be better to move debugfs
and/or netlink events.

/P