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>
---
 drivers/dpll/zl3073x/core.c  |  4 +-
 drivers/dpll/zl3073x/core.h  |  2 +
 drivers/dpll/zl3073x/hwmon.c | 86 +++++++++++++++++++++++++++++++++++-
 3 files changed, 90 insertions(+), 2 deletions(-)

diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
index 67e65f8e7e7d4..5805f87167c20 100644
--- a/drivers/dpll/zl3073x/core.c
+++ b/drivers/dpll/zl3073x/core.c
@@ -874,7 +874,9 @@ int zl3073x_dev_start(struct zl3073x_dev *zldev, bool full)
 		return rc;
 	}
 
-	if (!FIELD_GET(ZL_INFO_READY, info)) {
+	zldev->ready = !!FIELD_GET(ZL_INFO_READY, info);
+
+	if (!zldev->ready) {
 		/* The ready bit indicates that the firmware was successfully
 		 * configured and is ready for normal operation. If it is
 		 * cleared then the configuration stored in flash is wrong
diff --git a/drivers/dpll/zl3073x/core.h b/drivers/dpll/zl3073x/core.h
index 99440620407da..a416b8a65f41b 100644
--- a/drivers/dpll/zl3073x/core.h
+++ b/drivers/dpll/zl3073x/core.h
@@ -48,6 +48,7 @@ struct zl3073x_chip_info {
  * @regmap: regmap to access device registers
  * @info: detected chip info
  * @multiop_lock: to serialize multiple register operations
+ * @ready: true if device firmware is configured and ready for normal operation
  * @ref: array of input references' invariants
  * @out: array of outs' invariants
  * @synth: array of synths' invariants
@@ -63,6 +64,7 @@ struct zl3073x_dev {
 	struct regmap			*regmap;
 	const struct zl3073x_chip_info	*info;
 	struct mutex			multiop_lock;
+	bool				ready;
 
 	/* Invariants */
 	struct zl3073x_ref	ref[ZL3073X_NUM_REFS];
diff --git a/drivers/dpll/zl3073x/hwmon.c b/drivers/dpll/zl3073x/hwmon.c
index 4b44df4def820..96879609ce100 100644
--- a/drivers/dpll/zl3073x/hwmon.c
+++ b/drivers/dpll/zl3073x/hwmon.c
@@ -2,9 +2,11 @@
 
 #include <linux/device.h>
 #include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
 
 #include "core.h"
 #include "hwmon.h"
+#include "ref.h"
 #include "regs.h"
 
 static int zl3073x_hwmon_read(struct device *dev,
@@ -55,6 +57,88 @@ static const struct hwmon_chip_info zl3073x_hwmon_chip_info = {
 	.info = zl3073x_hwmon_info,
 };
 
+static ssize_t freq_input_show(struct device *dev,
+			       struct device_attribute *devattr, char *buf)
+{
+	struct zl3073x_dev *zldev = dev_get_drvdata(dev);
+	int index = to_sensor_dev_attr(devattr)->index;
+	const struct zl3073x_ref *ref;
+
+	if (!zldev->ready)
+		return -ENODATA;
+
+	ref = zl3073x_ref_state_get(zldev, index);
+
+	return sysfs_emit(buf, "%u\n", zl3073x_ref_meas_freq_get(ref));
+}
+
+static ssize_t freq_label_show(struct device *dev,
+			       struct device_attribute *devattr, char *buf)
+{
+	static const char * const labels[] = {
+		"REF0P", "REF0N", "REF1P", "REF1N", "REF2P",
+		"REF2N", "REF3P", "REF3N", "REF4P", "REF4N",
+	};
+	int index = to_sensor_dev_attr(devattr)->index;
+
+	return sysfs_emit(buf, "%s\n", labels[index]);
+}
+
+static SENSOR_DEVICE_ATTR_RO(freq0_input, freq_input, 0);
+static SENSOR_DEVICE_ATTR_RO(freq1_input, freq_input, 1);
+static SENSOR_DEVICE_ATTR_RO(freq2_input, freq_input, 2);
+static SENSOR_DEVICE_ATTR_RO(freq3_input, freq_input, 3);
+static SENSOR_DEVICE_ATTR_RO(freq4_input, freq_input, 4);
+static SENSOR_DEVICE_ATTR_RO(freq5_input, freq_input, 5);
+static SENSOR_DEVICE_ATTR_RO(freq6_input, freq_input, 6);
+static SENSOR_DEVICE_ATTR_RO(freq7_input, freq_input, 7);
+static SENSOR_DEVICE_ATTR_RO(freq8_input, freq_input, 8);
+static SENSOR_DEVICE_ATTR_RO(freq9_input, freq_input, 9);
+
+static SENSOR_DEVICE_ATTR_RO(freq0_label, freq_label, 0);
+static SENSOR_DEVICE_ATTR_RO(freq1_label, freq_label, 1);
+static SENSOR_DEVICE_ATTR_RO(freq2_label, freq_label, 2);
+static SENSOR_DEVICE_ATTR_RO(freq3_label, freq_label, 3);
+static SENSOR_DEVICE_ATTR_RO(freq4_label, freq_label, 4);
+static SENSOR_DEVICE_ATTR_RO(freq5_label, freq_label, 5);
+static SENSOR_DEVICE_ATTR_RO(freq6_label, freq_label, 6);
+static SENSOR_DEVICE_ATTR_RO(freq7_label, freq_label, 7);
+static SENSOR_DEVICE_ATTR_RO(freq8_label, freq_label, 8);
+static SENSOR_DEVICE_ATTR_RO(freq9_label, freq_label, 9);
+
+static struct attribute *zl3073x_freq_attrs[] = {
+	&sensor_dev_attr_freq0_input.dev_attr.attr,
+	&sensor_dev_attr_freq0_label.dev_attr.attr,
+	&sensor_dev_attr_freq1_input.dev_attr.attr,
+	&sensor_dev_attr_freq1_label.dev_attr.attr,
+	&sensor_dev_attr_freq2_input.dev_attr.attr,
+	&sensor_dev_attr_freq2_label.dev_attr.attr,
+	&sensor_dev_attr_freq3_input.dev_attr.attr,
+	&sensor_dev_attr_freq3_label.dev_attr.attr,
+	&sensor_dev_attr_freq4_input.dev_attr.attr,
+	&sensor_dev_attr_freq4_label.dev_attr.attr,
+	&sensor_dev_attr_freq5_input.dev_attr.attr,
+	&sensor_dev_attr_freq5_label.dev_attr.attr,
+	&sensor_dev_attr_freq6_input.dev_attr.attr,
+	&sensor_dev_attr_freq6_label.dev_attr.attr,
+	&sensor_dev_attr_freq7_input.dev_attr.attr,
+	&sensor_dev_attr_freq7_label.dev_attr.attr,
+	&sensor_dev_attr_freq8_input.dev_attr.attr,
+	&sensor_dev_attr_freq8_label.dev_attr.attr,
+	&sensor_dev_attr_freq9_input.dev_attr.attr,
+	&sensor_dev_attr_freq9_label.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group zl3073x_freq_group = {
+	.attrs = zl3073x_freq_attrs,
+};
+
+static const struct attribute_group *zl3073x_hwmon_groups[] = {
+	&zl3073x_freq_group,
+	NULL,
+};
+
 int zl3073x_hwmon_init(struct zl3073x_dev *zldev)
 {
 	struct device *hwmon;
@@ -62,6 +146,6 @@ int zl3073x_hwmon_init(struct zl3073x_dev *zldev)
 	hwmon = devm_hwmon_device_register_with_info(zldev->dev, "zl3073x",
 						     zldev,
 						     &zl3073x_hwmon_chip_info,
-						     NULL);
+						     zl3073x_hwmon_groups);
 	return PTR_ERR_OR_ZERO(hwmon);
 }
-- 
2.52.0

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.

Guenter

> ---
>   drivers/dpll/zl3073x/core.c  |  4 +-
>   drivers/dpll/zl3073x/core.h  |  2 +
>   drivers/dpll/zl3073x/hwmon.c | 86 +++++++++++++++++++++++++++++++++++-
>   3 files changed, 90 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
> index 67e65f8e7e7d4..5805f87167c20 100644
> --- a/drivers/dpll/zl3073x/core.c
> +++ b/drivers/dpll/zl3073x/core.c
> @@ -874,7 +874,9 @@ int zl3073x_dev_start(struct zl3073x_dev *zldev, bool full)
>   		return rc;
>   	}
>   
> -	if (!FIELD_GET(ZL_INFO_READY, info)) {
> +	zldev->ready = !!FIELD_GET(ZL_INFO_READY, info);
> +
> +	if (!zldev->ready) {
>   		/* The ready bit indicates that the firmware was successfully
>   		 * configured and is ready for normal operation. If it is
>   		 * cleared then the configuration stored in flash is wrong
> diff --git a/drivers/dpll/zl3073x/core.h b/drivers/dpll/zl3073x/core.h
> index 99440620407da..a416b8a65f41b 100644
> --- a/drivers/dpll/zl3073x/core.h
> +++ b/drivers/dpll/zl3073x/core.h
> @@ -48,6 +48,7 @@ struct zl3073x_chip_info {
>    * @regmap: regmap to access device registers
>    * @info: detected chip info
>    * @multiop_lock: to serialize multiple register operations
> + * @ready: true if device firmware is configured and ready for normal operation
>    * @ref: array of input references' invariants
>    * @out: array of outs' invariants
>    * @synth: array of synths' invariants
> @@ -63,6 +64,7 @@ struct zl3073x_dev {
>   	struct regmap			*regmap;
>   	const struct zl3073x_chip_info	*info;
>   	struct mutex			multiop_lock;
> +	bool				ready;
>   
>   	/* Invariants */
>   	struct zl3073x_ref	ref[ZL3073X_NUM_REFS];
> diff --git a/drivers/dpll/zl3073x/hwmon.c b/drivers/dpll/zl3073x/hwmon.c
> index 4b44df4def820..96879609ce100 100644
> --- a/drivers/dpll/zl3073x/hwmon.c
> +++ b/drivers/dpll/zl3073x/hwmon.c
> @@ -2,9 +2,11 @@
>   
>   #include <linux/device.h>
>   #include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
>   
>   #include "core.h"
>   #include "hwmon.h"
> +#include "ref.h"
>   #include "regs.h"
>   
>   static int zl3073x_hwmon_read(struct device *dev,
> @@ -55,6 +57,88 @@ static const struct hwmon_chip_info zl3073x_hwmon_chip_info = {
>   	.info = zl3073x_hwmon_info,
>   };
>   
> +static ssize_t freq_input_show(struct device *dev,
> +			       struct device_attribute *devattr, char *buf)
> +{
> +	struct zl3073x_dev *zldev = dev_get_drvdata(dev);
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	const struct zl3073x_ref *ref;
> +
> +	if (!zldev->ready)
> +		return -ENODATA;
> +
> +	ref = zl3073x_ref_state_get(zldev, index);
> +
> +	return sysfs_emit(buf, "%u\n", zl3073x_ref_meas_freq_get(ref));
> +}
> +
> +static ssize_t freq_label_show(struct device *dev,
> +			       struct device_attribute *devattr, char *buf)
> +{
> +	static const char * const labels[] = {
> +		"REF0P", "REF0N", "REF1P", "REF1N", "REF2P",
> +		"REF2N", "REF3P", "REF3N", "REF4P", "REF4N",
> +	};
> +	int index = to_sensor_dev_attr(devattr)->index;
> +
> +	return sysfs_emit(buf, "%s\n", labels[index]);
> +}
> +
> +static SENSOR_DEVICE_ATTR_RO(freq0_input, freq_input, 0);
> +static SENSOR_DEVICE_ATTR_RO(freq1_input, freq_input, 1);
> +static SENSOR_DEVICE_ATTR_RO(freq2_input, freq_input, 2);
> +static SENSOR_DEVICE_ATTR_RO(freq3_input, freq_input, 3);
> +static SENSOR_DEVICE_ATTR_RO(freq4_input, freq_input, 4);
> +static SENSOR_DEVICE_ATTR_RO(freq5_input, freq_input, 5);
> +static SENSOR_DEVICE_ATTR_RO(freq6_input, freq_input, 6);
> +static SENSOR_DEVICE_ATTR_RO(freq7_input, freq_input, 7);
> +static SENSOR_DEVICE_ATTR_RO(freq8_input, freq_input, 8);
> +static SENSOR_DEVICE_ATTR_RO(freq9_input, freq_input, 9);
> +
> +static SENSOR_DEVICE_ATTR_RO(freq0_label, freq_label, 0);
> +static SENSOR_DEVICE_ATTR_RO(freq1_label, freq_label, 1);
> +static SENSOR_DEVICE_ATTR_RO(freq2_label, freq_label, 2);
> +static SENSOR_DEVICE_ATTR_RO(freq3_label, freq_label, 3);
> +static SENSOR_DEVICE_ATTR_RO(freq4_label, freq_label, 4);
> +static SENSOR_DEVICE_ATTR_RO(freq5_label, freq_label, 5);
> +static SENSOR_DEVICE_ATTR_RO(freq6_label, freq_label, 6);
> +static SENSOR_DEVICE_ATTR_RO(freq7_label, freq_label, 7);
> +static SENSOR_DEVICE_ATTR_RO(freq8_label, freq_label, 8);
> +static SENSOR_DEVICE_ATTR_RO(freq9_label, freq_label, 9);
> +
> +static struct attribute *zl3073x_freq_attrs[] = {
> +	&sensor_dev_attr_freq0_input.dev_attr.attr,
> +	&sensor_dev_attr_freq0_label.dev_attr.attr,
> +	&sensor_dev_attr_freq1_input.dev_attr.attr,
> +	&sensor_dev_attr_freq1_label.dev_attr.attr,
> +	&sensor_dev_attr_freq2_input.dev_attr.attr,
> +	&sensor_dev_attr_freq2_label.dev_attr.attr,
> +	&sensor_dev_attr_freq3_input.dev_attr.attr,
> +	&sensor_dev_attr_freq3_label.dev_attr.attr,
> +	&sensor_dev_attr_freq4_input.dev_attr.attr,
> +	&sensor_dev_attr_freq4_label.dev_attr.attr,
> +	&sensor_dev_attr_freq5_input.dev_attr.attr,
> +	&sensor_dev_attr_freq5_label.dev_attr.attr,
> +	&sensor_dev_attr_freq6_input.dev_attr.attr,
> +	&sensor_dev_attr_freq6_label.dev_attr.attr,
> +	&sensor_dev_attr_freq7_input.dev_attr.attr,
> +	&sensor_dev_attr_freq7_label.dev_attr.attr,
> +	&sensor_dev_attr_freq8_input.dev_attr.attr,
> +	&sensor_dev_attr_freq8_label.dev_attr.attr,
> +	&sensor_dev_attr_freq9_input.dev_attr.attr,
> +	&sensor_dev_attr_freq9_label.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group zl3073x_freq_group = {
> +	.attrs = zl3073x_freq_attrs,
> +};
> +
> +static const struct attribute_group *zl3073x_hwmon_groups[] = {
> +	&zl3073x_freq_group,
> +	NULL,
> +};
> +
>   int zl3073x_hwmon_init(struct zl3073x_dev *zldev)
>   {
>   	struct device *hwmon;
> @@ -62,6 +146,6 @@ int zl3073x_hwmon_init(struct zl3073x_dev *zldev)
>   	hwmon = devm_hwmon_device_register_with_info(zldev->dev, "zl3073x",
>   						     zldev,
>   						     &zl3073x_hwmon_chip_info,
> -						     NULL);
> +						     zl3073x_hwmon_groups);
>   	return PTR_ERR_OR_ZERO(hwmon);
>   }

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.
> 
> Guenter

Hi Guenter,

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.

Regards,
Ivan

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?

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?

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.

Thanks,
Guenter

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.

/P

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

On Tue, 24 Mar 2026 13:59:19 +0100 Ivan Vecera wrote:
> >> 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?

I don't have a strong opinion. IDK where to draw the line between DPLL
and "random functionality some devices may have". We have 3 DPLL
maintainers, let's see what their majority opinion is..

On 3/24/26 10:36 PM, Jakub Kicinski wrote:
> On Tue, 24 Mar 2026 13:59:19 +0100 Ivan Vecera wrote:
>>>> 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?
> 
> I don't have a strong opinion. IDK where to draw the line between DPLL
> and "random functionality some devices may have". We have 3 DPLL
> maintainers, let's see what their majority opinion is..

I think that line has already been crossed by temperature reporting,
which has been there since the beginning. Actual frequency measurement
is, in my opinion, much closer to the DPLL world than temperature 
reporting. :-)

Anyway, I have the relevant patch-set already prepared, so I can submit
it if you want to see it.

Thanks,
Ivan

On 25/03/2026 11:19, Ivan Vecera wrote:
> 
> 
> On 3/24/26 10:36 PM, Jakub Kicinski wrote:
>> On Tue, 24 Mar 2026 13:59:19 +0100 Ivan Vecera wrote:
>>>>> 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?
>>
>> I don't have a strong opinion. IDK where to draw the line between DPLL
>> and "random functionality some devices may have". We have 3 DPLL
>> maintainers, let's see what their majority opinion is..
> 
> I think that line has already been crossed by temperature reporting,
> which has been there since the beginning. Actual frequency measurement
> is, in my opinion, much closer to the DPLL world than temperature 
> reporting. :-)

Frequency measurements are DPLL-world properties indeed. Temperature
reporting was added as it has some influence on the stability of DPLL 
devices as well as on any oscillators.

> Anyway, I have the relevant patch-set already prepared, so I can submit
> it if you want to see it.
Feel free to submit, I'll be happy to review it.

On 3/25/26 4:56 PM, Vadim Fedorenko wrote:
> On 25/03/2026 11:19, Ivan Vecera wrote:
>>
>>
>> On 3/24/26 10:36 PM, Jakub Kicinski wrote:
>>> On Tue, 24 Mar 2026 13:59:19 +0100 Ivan Vecera wrote:
>>>>>> 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?
>>>
>>> I don't have a strong opinion. IDK where to draw the line between DPLL
>>> and "random functionality some devices may have". We have 3 DPLL
>>> maintainers, let's see what their majority opinion is..
>>
>> I think that line has already been crossed by temperature reporting,
>> which has been there since the beginning. Actual frequency measurement
>> is, in my opinion, much closer to the DPLL world than temperature 
>> reporting. :-)
> 
> Frequency measurements are DPLL-world properties indeed. Temperature
> reporting was added as it has some influence on the stability of DPLL 
> devices as well as on any oscillators.
> 
>> Anyway, I have the relevant patch-set already prepared, so I can submit
>> it if you want to see it.
> Feel free to submit, I'll be happy to review it.
> 
Thanks, will do..

Ivan