Re: Re: [PATCH v3 2/2] hwmon: Add Eswin EIC7700 PVT sensor driver

["Huan He" <hehuan1@eswincomputing.com>]

Hi Guenter,

Thank you very much for your detailed review. We appreciate the feedback.

> > 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@eswincomputing.com>
> > Signed-off-by: Huan He <hehuan1@eswincomputing.com>
> 
> Feedback from AI review inline.
> 
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#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"
> > +
> > +static const struct pvt_sensor_info pvt_info[] = {
> > +	PVT_SENSOR_INFO(0, "Temperature", hwmon_temp, TEMP),
> > +	PVT_SENSOR_INFO(0, "Voltage", hwmon_in, VOLT),
> > +	PVT_SENSOR_INFO(1, "Low-Vt", hwmon_in, LVT),
> > +	PVT_SENSOR_INFO(2, "UltraLow-Vt", hwmon_in, ULVT),
> > +	PVT_SENSOR_INFO(3, "Standard-Vt", hwmon_in, SVT),
> 
> [not from AI]
> 
> Are those limits reported as voltages ? If so, that would violate the ABI.

According to the PVT datasheet, in process mode, the DATA_OUT values for
LVT, ULVT, and SVT are not voltages. They represent a combined effect of
process, voltage, temperature, and device type, and must be interpreted
using the corresponding lookup tables in the datasheet.
Exposing them as hwmon_in (voltage channels) violates the hwmon ABI. These
three channels are planned to be removed in the v4 release.

> 
> > +};
> > +
> > +/*
> > + * The original translation formulae of the temperature (in degrees of Celsius)
> > + * to PVT data and vice-versa are following:
> > + * N = 6.0818e-8*(T^4) +1.2873e-5*(T^3) + 7.2244e-3*(T^2) + 3.6484*(T^1) +
> > + *     1.6198e2,
> > + * T = -1.8439e-11*(N^4) + 8.0705e-8*(N^3) + -1.8501e-4*(N^2) +
> > + *     3.2843e-1*(N^1) - 4.8690e1,
> > + * where T = [-40, 125]C and N = [27, 771].
> > + * They must be accordingly altered to be suitable for the integer arithmetics.
> > + * The technique is called 'factor redistribution', which just makes sure the
> > + * multiplications and divisions are made so to have a result of the operations
> > + * within the integer numbers limit. In addition we need to translate the
> > + * formulae to accept millidegrees of Celsius. Here what they look like after
> > + * the alterations:
> > + * N = (60818e-20*(T^4) + 12873e-14*(T^3) + 72244e-9*(T^2) + 36484e-3*T +
> > + *     16198e2) / 1e4,
> > + * T = -18439e-12*(N^4) + 80705e-9*(N^3) - 185010e-6*(N^2) + 328430e-3*N -
> > + *     48690,
> > + * where T = [-40000, 125000] mC and N = [27, 771].
> > + */
> > +static const struct polynomial poly_N_to_temp = {
> > +	.total_divider = 1,
> > +	.terms = {
> > +		{4, -18439, 1000, 1},
> > +		{3, 80705, 1000, 1},
> > +		{2, -185010, 1000, 1},
> > +		{1, 328430, 1000, 1},
> > +		{0, -48690, 1, 1}
> > +	}
> > +};
> > +
> > +/*
> > + * Similar alterations are performed for the voltage conversion equations.
> > + * The original formulae are:
> > + * N = 1.3905e3*V - 5.7685e2,
> > + * V = (N + 5.7685e2) / 1.3905e3,
> > + * where V = [0.72, 0.88] V and N = [424, 646].
> > + * After the optimization they looks as follows:
> > + * N = (13905e-3*V - 5768.5) / 10,
> > + * V = (N * 10^5 / 13905 + 57685 * 10^3 / 13905) / 10.
> > + * where V = [720, 880] mV and N = [424, 646].
> > + */
> > +static const struct polynomial poly_N_to_volt = {
> > +	.total_divider = 10,
> > +	.terms = {
> > +		{1, 100000, 13905, 1},
> > +		{0, 57685000, 1, 13905}
> > +	}
> > +};
> > +
> > +static inline u32 eic7700_pvt_update(void __iomem *reg, u32 mask, u32 data)
> > +{
> > +	u32 old;
> > +
> > +	old = readl_relaxed(reg);
> > +	writel((old & ~mask) | (data & mask), reg);
> > +
> > +	return old & mask;
> > +}
> > +
> > +static inline void eic7700_pvt_set_mode(struct pvt_hwmon *pvt, u32 mode)
> > +{
> > +	u32 old;
> > +
> > +	mode = FIELD_PREP(PVT_MODE_MASK, mode);
> > +
> > +	old = eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, 0);
> > +	eic7700_pvt_update(pvt->regs + PVT_MODE, PVT_MODE_MASK, mode);
> > +	eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, old);
> > +}
> > +
> > +static inline void eic7700_pvt_set_trim(struct pvt_hwmon *pvt, u32 val)
> > +{
> > +	u32 old;
> > +
> > +	old = eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, 0);
> > +	writel(val, pvt->regs + PVT_TRIM);
> > +	eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, old);
> > +}
> > +
> > +static irqreturn_t eic7700_pvt_hard_isr(int irq, void *data)
> > +{
> > +	struct pvt_hwmon *pvt = data;
> > +	u32 val;
> > +
> > +	eic7700_pvt_update(pvt->regs + PVT_INT, PVT_INT_CLR, PVT_INT_CLR);
> 
> Since the IRQ is registered with IRQF_SHARED, it can be triggered by other
> devices while this PVT sensor is runtime suspended and its clock is disabled.
> Will accessing pvt->regs here cause a bus fault?
> 
> Should the handler use pm_runtime_get_if_active() to verify the device is
> powered before touching its registers?
> 
> Additionally, the handler does not verify if the interrupt originated from
> this device before clearing it. Should it check PVT_INT_STAT and return
> IRQ_NONE if the interrupt is not ours?

It has been confirmed that the PVT interrupt is not shared. In the v4
patch, the driver will remove the use of IRQF_SHARED.
Since the interrupt is dedicated to the PVT sensor, there is no need to
use pm_runtime_get_if_active() to check whether the device is powered, nor
to verify PVT_INT_STAT to determine if the interrupt originated from this
device.

> 
> > +	/*
> > +	 * Read the data, update the cache and notify a waiter of this event.
> > +	 */
> > +	val = readl(pvt->regs + PVT_DATA);
> > +	WRITE_ONCE(pvt->data_cache, FIELD_GET(PVT_DATA_OUT, val));
> > +	complete(&pvt->conversion);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int eic7700_pvt_read_data(struct pvt_hwmon *pvt,
> > +				 enum pvt_sensor_type type, long *val)
> > +{
> > +	unsigned long timeout;
> > +	u32 data;
> > +	int ret;
> > +
> > +	/*
> > +	 * Wait for PVT conversion to complete and update the data cache. 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.
> > +	 */
> > +	reinit_completion(&pvt->conversion);
> > +
> > +	eic7700_pvt_set_mode(pvt, pvt_info[type].mode);
> > +	eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, PVT_ENA_EN);
> > +
> > +	/*
> > +	 * Wait with timeout since in case if the sensor is suddenly powered
> > +	 * down the request won't be completed and the caller will hang up on
> > +	 * this procedure until the power is back up again. Multiply the
> > +	 * timeout by the factor of two to prevent a false timeout.
> > +	 */
> > +	timeout = 2 * usecs_to_jiffies(ktime_to_us(pvt->timeout));
> > +	ret = wait_for_completion_timeout(&pvt->conversion, timeout);
> > +
> > +	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(pvt->data_cache);
> > +
> > +	if (!ret)
> > +		return -ETIMEDOUT;
> > +
> > +	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;
> 
> For PVT_LVT, PVT_ULVT, and PVT_SVT (which are exported as hwmon_in channels),
> this returns the raw data. Does this violate the hwmon ABI, which requires
> voltage readings to be in millivolts? Should poly_N_to_volt be applied to all
> voltage channels?

Exposing them as hwmon_in (voltage channels) violates the hwmon ABI. These
three channels are planned to be removed in the v4 release.

> 
> > +
> > +	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_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 umode_t eic7700_pvt_hwmon_is_visible(const void *data,
> > +					    enum hwmon_sensor_types type,
> > +					    u32 attr, int ch)
> > +{
> > +	switch (type) {
> > +	case hwmon_temp:
> > +		switch (attr) {
> > +		case hwmon_temp_input:
> > +		case hwmon_temp_label:
> > +			return 0444;
> > +		case hwmon_temp_offset:
> > +			return 0644;
> > +		}
> > +		break;
> > +	case hwmon_in:
> > +		switch (attr) {
> > +		case hwmon_in_input:
> > +		case hwmon_in_label:
> > +			return 0444;
> > +		}
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +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;
> > +	}
> > +
> > +	switch (type) {
> > +	case hwmon_temp:
> > +		switch (attr) {
> > +		case hwmon_temp_input:
> > +			ret = eic7700_pvt_read_data(pvt, ch, val);
> > +			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;
> > +	}
> > +
> > +	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)
> > +{
> > +	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 (type == hwmon_temp && attr == hwmon_temp_offset)
> > +		ret = eic7700_pvt_write_trim(pvt, val);
> > +	else
> > +		ret = -EOPNOTSUPP;
> > +
> > +	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;
> > +
> > +	complete_all(&pvt->conversion);
> > +}
> > +
> > +static struct pvt_hwmon *eic7700_pvt_create_data(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct pvt_hwmon *pvt;
> > +	int ret;
> > +
> > +	pvt = devm_kzalloc(dev, sizeof(*pvt), GFP_KERNEL);
> > +	if (!pvt)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	pvt->dev = dev;
> > +	init_completion(&pvt->conversion);
> > +
> > +	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);
> > +	}
> > +
> > +	return pvt;
> > +}
> > +
> > +static int eic7700_pvt_check_pwr(struct pvt_hwmon *pvt)
> > +{
> > +	unsigned long tout;
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * Test out the sensor conversion functionality. If it is not done on
> > +	 * time then the domain must have been unpowered and we won't be able
> > +	 * to use the device later in this driver.
> > +	 */
> > +	eic7700_pvt_update(pvt->regs + PVT_INT, PVT_INT_CLR, PVT_INT_CLR);
> > +	/*
> > +	 * Enable the PVT block to test if the sensor domain is powered.
> > +	 *
> > +	 * This happens before request_irq(). Enabling the block may generate
> > +	 * an interrupt on shared IRQ lines before a handler is registered.
> > +	 *
> > +	 * The hardware does not provide a dedicated interrupt enable bit in
> > +	 * PVT_ENA and PVT_INT does not support interrupt masking. After the
> > +	 * test, the driver disables the PVT block and clears the interrupt
> > +	 * status via PVT_INT_CLR, preventing stale interrupts.
> > +	 */
> > +	eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, PVT_ENA_EN);
> > +	readl(pvt->regs + PVT_DATA);
> > +
> > +	tout = PVT_TOUT_MIN / NSEC_PER_USEC;
> > +	usleep_range(tout, 2 * tout);
> 
> Since this IRQ is shared and level-triggered, won't generating an interrupt
> before the handler is registered cause an IRQ storm? The interrupt controller
> will fire continuously during the usleep_range(), which could cause the
> kernel's spurious IRQ detector to permanently disable the shared IRQ line.

The PVT IP is fully integrated and always powered, so performing the
eic7700_pvt_check_pwr() verification is not required. This check will be
removed in the v4 patch.

> 
> > +
> > +	readl(pvt->regs + PVT_DATA);
> > +	if (!(readl(pvt->regs + PVT_INT) & PVT_INT_STAT)) {
> > +		ret = -ENODEV;
> > +		dev_err(pvt->dev,
> > +			"Sensor is powered down - no interrupt generated\n");
> > +	}
> > +
> > +	eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, 0);
> > +	eic7700_pvt_update(pvt->regs + PVT_INT, PVT_INT_CLR, PVT_INT_CLR);
> > +
> > +	return ret;
> > +}
> > +
> > +static int eic7700_pvt_init_iface(struct pvt_hwmon *pvt)
> > +{
> > +	/*
> > +	 * Make sure controller are disabled so not to accidentally have ISR
> > +	 * executed before the driver data is fully initialized. Clear the IRQ
> > +	 * status as well.
> > +	 */
> > +	eic7700_pvt_update(pvt->regs + PVT_INT, PVT_INT_CLR, PVT_INT_CLR);
> > +	eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, 0);
> > +	readl(pvt->regs + PVT_INT);
> > +	readl(pvt->regs + PVT_DATA);
> > +
> > +	/* Setup default sensor mode and temperature trim. */
> > +	eic7700_pvt_set_mode(pvt, pvt_info[PVT_TEMP].mode);
> > +
> > +	/*
> > +	 * Max conversion latency (~333 µs) derived from PVT spec:
> > +	 * maximum sampling rate = 3000 samples/sec.
> > +	 */
> > +	pvt->timeout = ns_to_ktime(PVT_TOUT_MIN);
> > +
> > +	eic7700_pvt_set_trim(pvt, PVT_TRIM_DEF);
> > +
> > +	return 0;
> > +}
> > +
> > +static int eic7700_pvt_request_irq(struct pvt_hwmon *pvt)
> > +{
> > +	struct platform_device *pdev = to_platform_device(pvt->dev);
> > +	int ret;
> > +
> > +	pvt->irq = platform_get_irq(pdev, 0);
> > +	if (pvt->irq < 0)
> > +		return pvt->irq;
> > +
> > +	ret = devm_request_threaded_irq(pvt->dev, pvt->irq,
> > +					eic7700_pvt_hard_isr, NULL,
> > +					IRQF_SHARED | IRQF_TRIGGER_HIGH, "pvt",
> > +					pvt);
> > +	if (ret) {
> > +		dev_err(pvt->dev, "Couldn't request PVT IRQ\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int eic7700_pvt_create_hwmon(struct pvt_hwmon *pvt)
> > +{
> > +	struct device *dev = pvt->dev;
> > +	struct device_node *np = dev->of_node;
> > +	const char *node_label;
> > +	int type;
> > +	const char *names[2] = {"soc_pvt", "ddr_pvt"};
> > +
> > +	if (of_property_read_string(np, "label", &node_label)) {
> > +		dev_err(dev, "Missing 'label' property in DTS node\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (strcmp(node_label, "pvt0") == 0) {
> > +		type = 0;
> > +	} else if (strcmp(node_label, "pvt1") == 0) {
> > +		type = 1;
> > +	} else {
> > +		dev_err(pvt->dev, "Unsupported label: %s\n", node_label);
> > +		return -EINVAL;
> > +	}
> > +
> > +	pvt->hwmon = devm_hwmon_device_register_with_info(pvt->dev, names[type],
> > +							  pvt, &pvt_hwmon_info,
> > +							  NULL);
> > +	if (IS_ERR(pvt->hwmon)) {
> > +		dev_err(pvt->dev, "Couldn't create hwmon device\n");
> > +		return PTR_ERR(pvt->hwmon);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void eic7700_pvt_disable_pm_runtime(void *data)
> > +{
> > +	struct pvt_hwmon *pvt = data;
> > +
> > +	pm_runtime_dont_use_autosuspend(pvt->dev);
> > +	pm_runtime_disable(pvt->dev);
> > +}
> > +
> > +static int eic7700_pvt_probe(struct platform_device *pdev)
> > +{
> > +	struct pvt_hwmon *pvt;
> > +	int ret;
> > +
> > +	pvt = eic7700_pvt_create_data(pdev);
> > +	if (IS_ERR(pvt))
> > +		return PTR_ERR(pvt);
> > +
> > +	platform_set_drvdata(pdev, pvt);
> > +
> > +	pvt->regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(pvt->regs))
> > +		return PTR_ERR(pvt->regs);
> > +
> > +	pvt->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(pvt->clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(pvt->clk),
> > +				     "Couldn't get clock\n");
> > +
> > +	pvt->rst = devm_reset_control_get_exclusive_deasserted(&pdev->dev,
> > +							       NULL);
> > +	if (IS_ERR(pvt->rst))
> > +		return dev_err_probe(pvt->dev, PTR_ERR(pvt->rst),
> > +				     "Couldn't get reset control\n");
> > +
> > +	clk_prepare_enable(pvt->clk);
> > +
> > +	ret = eic7700_pvt_check_pwr(pvt);
> > +	if (ret)
> > +		return ret;
> > +
> > +	clk_disable_unprepare(pvt->clk);
> > +
> > +	ret = eic7700_pvt_init_iface(pvt);
> > +	if (ret)
> > +		return ret;
> 
> In eic7700_pvt_init_iface(pvt), the code accesses the device's registers.
> Since clk_disable_unprepare(pvt->clk) is called right before it, won't
> accessing the hardware registers here cause a bus fault? Should the clock be
> disabled after initialization instead?

We will move clk_disable_unprepare() to after eic7700_pvt_init_iface() in v4.

> 
> > +
> > +	ret = eic7700_pvt_request_irq(pvt);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_set_autosuspend_delay(&pdev->dev, 3000);
> > +	pm_runtime_use_autosuspend(&pdev->dev);
> > +	pm_runtime_get_noresume(&pdev->dev);
> > +
> > +	ret = devm_add_action_or_reset(pvt->dev, eic7700_pvt_disable_pm_runtime,
> > +				       pvt);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "Can't register PM cleanup\n");
> > +
> > +	ret = eic7700_pvt_create_hwmon(pvt);
> > +	if (ret)
> > +		return ret;
> 
> If eic7700_pvt_create_hwmon() fails, the function returns early. The devres
> action disables runtime PM, but it doesn't balance the
> pm_runtime_get_noresume(). Does this leak the usage counter on probe failure?

We will add proper error handling to ensure pm_runtime_put_noidle() is
called on probe failure paths to balance the pm_runtime_get_noresume()
reference.

> 
> > +
> > +	pm_runtime_put_autosuspend(&pdev->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused eic7700_pvt_runtime_resume(struct device *dev)
> > +{
> > +	struct pvt_hwmon *pvt = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(pvt->clk);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable clock: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused eic7700_pvt_runtime_suspend(struct device *dev)
> > +{
> > +	struct pvt_hwmon *pvt = dev_get_drvdata(dev);
> > +
> > +	clk_disable_unprepare(pvt->clk);
> > +
> Does this need a synchronize_irq(pvt->irq) before disabling the clock? Since
> the IRQ is shared, a handler might be executing mid-flight on another CPU
> when the clock is disabled, which could cause a bus fault.

It has been confirmed that the PVT interrupt is not shared. Therefore,
synchronize_irq() is not required.

> 
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused eic7700_pvt_suspend(struct device *dev)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!pm_runtime_status_suspended(dev)) {
> > +		ret = eic7700_pvt_runtime_suspend(dev);
> > +		if (ret) {
> > +			dev_err(dev, "Failed to suspend: %d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused eic7700_pvt_resume(struct device *dev)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!pm_runtime_status_suspended(dev)) {
> > +		ret = eic7700_pvt_runtime_resume(dev);
> > +		if (ret) {
> > +			dev_err(dev, "Failed to resume: %d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops eic7700_pvt_pm_ops = {
> > +	SYSTEM_SLEEP_PM_OPS(eic7700_pvt_suspend, eic7700_pvt_resume)
> > +	RUNTIME_PM_OPS(eic7700_pvt_runtime_suspend, eic7700_pvt_runtime_resume,
> > +		       NULL)
> > +};
> > +
> > +static const struct of_device_id pvt_of_match[] = {
> > +	{ .compatible = "eswin,eic7700-pvt"},
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, pvt_of_match);
> > +
> > +static struct platform_driver pvt_driver = {
> > +	.probe = eic7700_pvt_probe,
> > +	.driver = {
> > +		.name = "eic7700-pvt",
> > +		.of_match_table = pvt_of_match,
> > +		.pm = pm_sleep_ptr(&eic7700_pvt_pm_ops),
> 
> Since eic7700_pvt_pm_ops contains runtime PM callbacks, using pm_sleep_ptr()
> will drop the entire structure if CONFIG_PM_SLEEP is disabled but CONFIG_PM
> is enabled. Should this use pm_ptr() instead?

We will use pm_ptr() in the v4 patch.

Best regards,
Huan He