Message-ID: <685d5518.2620.19bc511aaf8.Coremail.hehuan1@eswincomputing.com>
Date: Fri, 16 Jan 2026 12:30:28 +0800 (GMT+08:00)
From: "Huan He" <hehuan1@...incomputing.com>
To: "Guenter Roeck" <linux@...ck-us.net>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
	linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, p.zabel@...gutronix.de,
	ningyu@...incomputing.com, linmin@...incomputing.com,
	pinkesh.vaghela@...fochips.com, luyulin@...incomputing.com,
	weishangjuan@...incomputing.com
Subject: Re: Re: [PATCH v1 2/2] hwmon: Add Eswin EIC7700 PVT sensor driver

> > 
> > Add support for ESWIN EIC7700 Process, Voltage and Temperature sensor. The
> > driver supports temperature and voltage monitoring with polynomial
> > conversion, and provides sysfs interface for sensor data access.
> > 
> > The PVT IP contains one temperature sensor and four voltage sensors for
> > process variation monitoring.
> > 
> > Signed-off-by: Yulin Lu <luyulin@...incomputing.com>
> > Signed-off-by: Huan He <hehuan1@...incomputing.com>
> > ---
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ESWIN EIC7700 Process, Voltage, Temperature sensor driver
> > + *
> > + * Copyright 2026, Beijing ESWIN Computing Technology Co., Ltd.
> > + *
> > + * Authors:
> > + *   Yulin Lu <luyulin@...incomputing.com>
> > + *   Huan He <hehuan1@...incomputing.com>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/hwmon-sysfs.h>
> 
> I do not see why this include would be needed.

Thank you for pointing this out. We will remove this #include in v2.

> 
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/polynomial.h>
> > +#include <linux/reset.h>
> > +#include "eic7700-pvt.h"
> > +
> > +/*
> > + * For the sake of the code simplification we created the sensors info table
> > + * with the sensor names, activation modes.
> > + */
> 
> Simplification ? I'd argue that this code is fragile and complicated,
> and doesn't simplify anything.
> 
> > +static int eic7700_pvt_create_sensor_info(struct pvt_hwmon *pvt)
> > +{
> > +	const char *suffixes[PVT_SENSORS_NUM] = {
> 
> PVT_SENSORS_NUM == 6, so this allocates one too many array entries.

The value of PVT_SENSORS_NUM is 5, and the PVT_SENSOR_FIRST and
PVT_SENSOR_LAST macros are redundant. They will be removed.

> 
> > +		" Temperature",
> > +		" Voltage",
> > +		" Low-Vt",
> > +		" UltraLow-Vt",
> > +		" Standard-Vt"
> > +	};
> > +	struct device *dev = pvt->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct pvt_sensor_info *info_array;
> > +	char *labels[PVT_SENSORS_NUM];
> 
> This is a character array with PVT_SENSORS_NUM entries.
> 
> > +	const char *prefix = NULL;
> > +	const char *node_label;
> > +	int i;
> > +
> > +	if (of_property_read_string(np, "label", &node_label)) {
> > +		dev_err(dev, "Missing 'label' property in DTS node\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	struct {
> > +		const char *label;
> > +		const char *prefix;
> > +	} label_map[] = {
> > +		{ "pvt0", "SoC"},
> > +		{ "pvt1", "DDR Core"},
> > +	};
> 
> It seems to me that using a string property such as "label"
> to distinguish SoC from DDR core is just wrong. First of all,
> the assignment is really fixed. The driver instance name could
> be used to identify that. Having each individual sensor named
> "SoC <something>" or "DDR core <something>" seems excessive.
> It would be much simpler and less error prone to name the instances
> instead and use the same labels for both.
> 
> Ah, I see in the probe function that this is already the case.
> So the instances are named "soc_pvt" and "ddr_pvt", and then
> the individual sensors are again named "SoC ..." and "DDR Core ...".
> That seems redundant.

The original design was indeed redundant. In v2, we will:

1.Standardize the naming of all temperature sensors as "Temperature", and
voltage sensors as "Voltage", "Low-Vt", etc.;
2.Keep soc_pvt / ddr_pvt as the hwmon device names, which are sufficient
to differentiate the sources.

> 
> > +
> > +	for (i = 0; i < ARRAY_SIZE(label_map); i++) {
> > +		if (strcmp(node_label, label_map[i].label) == 0) {
> > +			prefix = label_map[i].prefix;
> > +			break;
> > +		}
> > +	}
> > +
> > +	info_array = devm_kzalloc(dev, PVT_SENSORS_NUM * sizeof(*info_array),
> > +				  GFP_KERNEL);
> > +	if (!info_array)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < PVT_SENSORS_NUM; i++) {
> > +		labels[i] = devm_kasprintf(dev, GFP_KERNEL, "%s%s", prefix,
> > +					   suffixes[i]);
> 
> So the property is evaluated twice, in the probe function and here.
> Why ?

To concatenate the SOC/DDR prefix with 'Temperature'.

> 
> Also, labels[i] is a character. Does this code even compile ?

char *labels[PVT_SENSORS_NUM]; is an array of pointers, and devm_kasprintf
returns a char*.

> 
> > +		if (!labels[i])
> > +			return -ENOMEM;
> > +	}
> > +
> > +	info_array[0] = (struct pvt_sensor_info)PVT_SENSOR_INFO(0, labels[0],
> > +								hwmon_temp,
> > +								TEMP);
> > +	info_array[1] = (struct pvt_sensor_info)PVT_SENSOR_INFO(0, labels[1],
> > +								hwmon_in,
> > +								VOLT);
> > +	info_array[2] = (struct pvt_sensor_info)PVT_SENSOR_INFO(1, labels[2],
> > +								hwmon_in,
> > +								LVT);
> > +	info_array[3] = (struct pvt_sensor_info)PVT_SENSOR_INFO(2, labels[3],
> > +								hwmon_in,
> > +								ULVT);
> > +	info_array[4] = (struct pvt_sensor_info)PVT_SENSOR_INFO(3, labels[4],
> > +								hwmon_in,
> > +								SVT);
> > +
> > +	pvt->sensor_info = info_array;
> > +	return 0;
> > +}
> > +
> > +static int eic7700_pvt_read_data(struct pvt_hwmon *pvt,
> > +				 enum pvt_sensor_type type, long *val)
> > +{
> > +	const struct pvt_sensor_info *pvt_info = pvt->sensor_info;
> > +	struct pvt_cache *cache = &pvt->cache[type];
> > +	u32 data;
> > +	int ret;
> > +
> > +	if (!pvt_info) {
> > +		dev_err(pvt->dev, "No matching device data found\n");
> > +		return -EINVAL;
> > +	}
> 
> How would pvt_info ever be NULL ?

It is impossible for this to be NULL at runtime. We will remove this
validation in v2.

> 
> > +	/*
> > +	 * Lock PVT conversion interface until data cache is updated. The data
> > +	 * read procedure is following: set the requested PVT sensor mode,
> > +	 * enable conversion, wait until conversion is finished, then disable
> > +	 * conversion and IRQ, and read the cached data.
> > +	 */
> > +	ret = mutex_lock_interruptible(&pvt->iface_mtx);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pvt->sensor = type;
> > +	eic7700_pvt_set_mode(pvt, pvt_info[type].mode);
> > +
> > +	eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, PVT_ENA_EN);
> > +
> > +	ret = wait_for_completion_interruptible(&cache->conversion);
> > +
> > +	eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, 0);
> > +	eic7700_pvt_update(pvt->regs + PVT_INT, PVT_INT_CLR, PVT_INT_CLR);
> > +
> > +	data = READ_ONCE(cache->data);
> > +
> > +	mutex_unlock(&pvt->iface_mtx);
> > +
> > +	if (ret && (ret != -ERESTARTSYS))
> > +		return ret;
> > +
> > +	if (type == PVT_TEMP)
> > +		*val = polynomial_calc(&poly_N_to_temp, data);
> > +	else if (type == PVT_VOLT)
> > +		*val = polynomial_calc(&poly_N_to_volt, data);
> > +	else
> > +		*val = data;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct hwmon_channel_info *pvt_channel_info[] = {
> > +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> > +	HWMON_CHANNEL_INFO(temp,
> > +			   HWMON_T_INPUT | HWMON_T_TYPE | HWMON_T_LABEL |
> > +			   HWMON_T_OFFSET),
> > +	HWMON_CHANNEL_INFO(in,
> > +			   HWMON_I_INPUT | HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_LABEL),
> > +	NULL
> > +};
> > +
> > +static inline bool
> > +eic7700_pvt_hwmon_channel_is_valid(enum hwmon_sensor_types type, int ch)
> > +{
> > +	switch (type) {
> > +	case hwmon_temp:
> > +		if (ch < 0 || ch >= PVT_TEMP_CHS)
> > +			return false;
> > +		break;
> > +	case hwmon_in:
> > +		if (ch < 0 || ch >= PVT_VOLT_CHS)
> > +			return false;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return true;
> > +}
> 
> This validation is unnecessary.

This validation will be removed.

> 
> > +
> > +static int eic7700_pvt_hwmon_read(struct device *dev,
> > +				  enum hwmon_sensor_types type, u32 attr,
> > +				  int ch, long *val)
> > +{
> > +	struct pvt_hwmon *pvt = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = pm_runtime_get_sync(pvt->dev);
> > +	if (ret < 0) {
> > +		dev_err(pvt->dev, "Failed to resume PVT device: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (!eic7700_pvt_hwmon_channel_is_valid(type, ch)) {
> 
> How would that ever be invalid ? This validation is pointless.

This validation will be removed.

> 
> > +		ret = -EINVAL;
> > +		goto out_put;
> > +	}
> > +
> > +	switch (type) {
> > +	case hwmon_temp:
> > +		switch (attr) {
> > +		case hwmon_temp_input:
> > +			ret = eic7700_pvt_read_data(pvt, ch, val);
> > +			break;
> > +		case hwmon_temp_type:
> > +			*val = 1;
> 
> Drop this attribute.

This attribute will be removed.

> 
> > +			ret = 0;
> > +			break;
> > +		case hwmon_temp_offset:
> > +			ret = eic7700_pvt_read_trim(pvt, val);
> > +			break;
> > +		default:
> > +			ret = -EOPNOTSUPP;
> > +		}
> > +		break;
> > +	case hwmon_in:
> > +		if (attr == hwmon_in_input)
> > +			ret = eic7700_pvt_read_data(pvt, PVT_VOLT + ch, val);
> > +		else
> > +			ret = -EOPNOTSUPP;
> > +		break;
> > +	default:
> > +		ret = -EOPNOTSUPP;
> > +	}
> > +
> > +out_put:
> > +	pm_runtime_mark_last_busy(pvt->dev);
> > +	pm_runtime_put_autosuspend(pvt->dev);
> > +	return ret;
> > +}
> > +
> > +static int eic7700_pvt_hwmon_read_string(struct device *dev,
> > +					 enum hwmon_sensor_types type, u32 attr,
> > +					 int ch, const char **str)
> > +{
> > +	struct pvt_hwmon *pvt = dev_get_drvdata(dev);
> > +	const struct pvt_sensor_info *pvt_info = pvt->sensor_info;
> > +
> > +	if (!eic7700_pvt_hwmon_channel_is_valid(type, ch))
> > +		return -EINVAL;
> > +
> > +	if (!pvt_info) {
> > +		dev_err(pvt->dev, "No matching device data found\n");
> > +		return -EINVAL;
> > +	}
> 
> More unnecessary validations.

This validation will be removed.

> 
> > +
> > +	switch (type) {
> > +	case hwmon_temp:
> > +		if (attr == hwmon_temp_label) {
> > +			*str = pvt_info[ch].label;
> > +			return 0;
> > +		}
> > +		break;
> > +	case hwmon_in:
> > +		if (attr == hwmon_in_label) {
> > +			*str = pvt_info[PVT_VOLT + ch].label;
> > +			return 0;
> > +		}
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static int eic7700_pvt_hwmon_write(struct device *dev,
> > +				   enum hwmon_sensor_types type, u32 attr,
> > +				   int ch, long val)
> > +{
> > +	struct pvt_hwmon *pvt = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = pm_runtime_get_sync(pvt->dev);
> > +	if (ret < 0) {
> > +		dev_err(pvt->dev, "Failed to resume PVT device: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (!eic7700_pvt_hwmon_channel_is_valid(type, ch)) {
> > +		ret = -EINVAL;
> > +		goto out_put;
> > +	}
> 
> And again.

This validation will be removed.

> 
> > +
> > +	if (type == hwmon_temp || attr == hwmon_temp_offset)
> > +		ret = eic7700_pvt_write_trim(pvt, val);
> > +	else
> > +		ret = -EOPNOTSUPP;
> > +
> > +out_put:
> > +	pm_runtime_mark_last_busy(pvt->dev);
> > +	pm_runtime_put_autosuspend(pvt->dev);
> > +	return ret;
> > +}
> > +
> > +static const struct hwmon_ops pvt_hwmon_ops = {
> > +	.is_visible = eic7700_pvt_hwmon_is_visible,
> > +	.read = eic7700_pvt_hwmon_read,
> > +	.read_string = eic7700_pvt_hwmon_read_string,
> > +	.write = eic7700_pvt_hwmon_write
> > +};
> > +
> > +static const struct hwmon_chip_info pvt_hwmon_info = {
> > +	.ops = &pvt_hwmon_ops,
> > +	.info = pvt_channel_info
> > +};
> > +
> > +static void pvt_clear_data(void *data)
> > +{
> > +	struct pvt_hwmon *pvt = data;
> > +	int idx;
> > +
> > +	for (idx = 0; idx < PVT_SENSORS_NUM; ++idx)
> > +		complete_all(&pvt->cache[idx].conversion);
> > +
> > +	mutex_destroy(&pvt->iface_mtx);
> > +}
> > +
> > +static struct pvt_hwmon *eic7700_pvt_create_data(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct pvt_hwmon *pvt;
> > +	int ret, idx;
> > +
> > +	pvt = devm_kzalloc(dev, sizeof(*pvt), GFP_KERNEL);
> > +	if (!pvt)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	ret = devm_add_action(dev, pvt_clear_data, pvt);
> > +	if (ret) {
> > +		dev_err(dev, "Can't add PVT data clear action\n");
> > +		return ERR_PTR(ret);
> > +	}
> 
> Doing that before any data is initialized seems wrong.

We will move the devm_add_action() call to after initialization.

> 
> > +
> > +	pvt->dev = dev;
> > +	pvt->sensor = PVT_SENSOR_FIRST;
> 
> What is this supposed to accomplish ?

This initialization is redundant, we will remove this line to avoid
unnecessary initialization.

> 
> > +	mutex_init(&pvt->iface_mtx);
> 
> The hwmon core already serializes accesses. Why would this driver need
> another mutex ?

Since hwmon serializes access, the iface_mtx mutex is redundant. It will
be removed in v2, along with the related locking operations.

> 
> > +
> > +	for (idx = 0; idx < PVT_SENSORS_NUM; ++idx)
> > +		init_completion(&pvt->cache[idx].conversion);
> 
> Why would more than one completion ever be needed ? All accesses are serialized
> in the hwmon core, so there will only be one outstanding conversion
> at any given time. On top of that, the iface_mtx serializs accesses again.
> 
> Why is more than one cache entry needed anyway, given that all accesses are serialized ?

Due to hwmon's serialized access, we will simplify the code to use a
single data structure and a single completion signal, eliminating the need
for arrays based on sensor types.

> 
> > +
> > +	return pvt;
> > +}
> > +
> > +static void eic7700_pvt_remove(void *data)
> > +{
> > +	int ret;
> > +	struct pvt_hwmon *pvt = data;
> > +
> > +	pm_runtime_disable(pvt->dev);
> > +	pm_runtime_dont_use_autosuspend(pvt->dev);
> > +	pm_runtime_get_sync(pvt->dev);
> > +
> > +	ret = reset_control_assert(pvt->rst);
> > +	if (ret)
> > +		dev_err(pvt->dev, "Failed to assert reset: %d\n", ret);
> > +
> > +	if (pm_runtime_active(pvt->dev))
> > +		clk_disable_unprepare(pvt->clk);
> 
> The probe function calls devm_clk_get_enabled(), which means that the clock
> will be disabled on remove. Why is it necessary to disable it again here ?

The clk_disable_unprepare() call is redundant and will be removed.

> 
> > +
> > +	pm_runtime_put_noidle(pvt->dev);
> > +}
> > +
> > +/*
> > + * enum pvt_sensor_type - ESWIN EIC7700 PVT sensor types (correspond to each PVT
> > + *			  sampling mode)
> > + * @PVT_SENSOR*: helpers to traverse the sensors in loops.
> > + * @PVT_TEMP: PVT Temperature sensor.
> > + * @PVT_VOLT: PVT Voltage sensor.
> > + * @PVT_LVT: PVT Low-Voltage threshold sensor.
> > + * @PVT_HVT: PVT High-Voltage threshold sensor.
> > + * @PVT_SVT: PVT Standard-Voltage threshold sensor.
> > + */
> > +enum pvt_sensor_type {
> > +	PVT_SENSOR_FIRST,
> > +	PVT_TEMP = PVT_SENSOR_FIRST,
> > +	PVT_VOLT,
> > +	PVT_LVT,
> > +	PVT_ULVT,
> > +	PVT_SVT,
> > +	PVT_SENSOR_LAST = PVT_SVT,
> 
> Why PVT_SENSOR_FIRST and PVT_SENSOR_LAST ?

The PVT_SENSOR_FIRST and PVT_SENSOR_LAST macros are redundant and will be
removed.

> 
> > +	PVT_SENSORS_NUM
> > +};
> > +

Thank you very much for taking the time to review the patch and for your
valuable feedback.
All these issues will be addressed in the next patch.

Best regards,
Huan He

Powered by blists - more mailing lists