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

[Ivan Vecera <ivecera@redhat.com>]

Hi Paolo,

On 3/24/26 11:49 AM, Paolo Abeni wrote:
> 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@redhat.com>
>>>>>
>>>>> "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.

I'd rather avoid debugfs... My proposal is to expose absolute measured
frequency attribute of dpll-pin and follow phase-offset-monitor
functionality:

So:
* add real-frequency attribute to dpll pin
* add real-frequency-monitor attribute dpll device
* user will be able to enable/disable monitoring by enabling/disabling
   real-frequency-monitor feature (similarly to phase-offset-monitor)

Thoughts?

Thanks,
Ivan