Message-ID: <20260111-poetic-kittiwake-of-certainty-f60f4d@quoll>
Date: Sun, 11 Jan 2026 11:35:06 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: hehuan1@...incomputing.com
Cc: linux@...ck-us.net, 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: [PATCH v1 2/2] hwmon: Add Eswin EIC7700 PVT sensor driver

On Fri, Jan 09, 2026 at 05:09:29PM +0800, hehuan1@...incomputing.com wrote:
> +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);

Why do you set up cleaning - mutex destroy, complete - before even
initializing them?

This is very messy or even buggy coding style.

> +	}
> +
> +	pvt->dev = dev;
> +	pvt->sensor = PVT_SENSOR_FIRST;
> +	mutex_init(&pvt->iface_mtx);
> +
> +	for (idx = 0; idx < PVT_SENSORS_NUM; ++idx)
> +		init_completion(&pvt->cache[idx].conversion);
> +
> +	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);
> +
> +	pm_runtime_put_noidle(pvt->dev);
> +}
> +
> +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);
> +	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);
> +
> +	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)
> +{
> +	unsigned long rate;
> +	const struct pvt_sensor_info *pvt_info = pvt->sensor_info;
> +
> +	rate = clk_get_rate(pvt->clk);
> +	if (!rate) {
> +		dev_err(pvt->dev, "Invalid reference clock rate\n");
> +		return -ENODEV;
> +	}
> +	if (!pvt_info) {
> +		dev_err(pvt->dev, "No matching device data found\n");
> +		return -EINVAL;
> +	}
> +	/*
> +	 * 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->sensor].mode);
> +
> +	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 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);
> +
> +	ret = eic7700_pvt_create_sensor_info(pvt);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to create sensor info\n");
> +		return ret;
> +	}
> +
> +	pvt->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pvt->regs))
> +		return PTR_ERR(pvt->regs);
> +
> +	pvt->clk = devm_clk_get_enabled(&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_optional_exclusive(&pdev->dev, NULL);
> +	if (IS_ERR(pvt->rst))
> +		return dev_err_probe(pvt->dev, PTR_ERR(pvt->rst),
> +				     "Couldn't to get reset control\n");
> +
> +	if (pvt->rst) {
> +		ret = reset_control_reset(pvt->rst);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "Couldn't to trigger reset\n");
> +	}
> +
> +	ret = eic7700_pvt_check_pwr(pvt);
> +	if (ret)
> +		return ret;
> +
> +	ret = eic7700_pvt_init_iface(pvt);
> +	if (ret)
> +		return ret;
> +
> +	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 = eic7700_pvt_create_hwmon(pvt);
> +	if (ret)

You leak here - missing PM runtime cleanup.

> +		return ret;
> +
> +	ret = devm_add_action_or_reset(pvt->dev, eic7700_pvt_remove, pvt);
> +	if (ret) {
> +		dev_err(pvt->dev, "Can't add PVT clocks disable action\n");

Why PM runtime cleanup should happen only from this moment/error path?

Way you organized your code and stuffed multiple cleanups at once is
very messy. This is unreadable at best or even buggy code.

> +		return ret;
> +	}
> +
> +	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;
> +
> +	if (!pvt->clk) {
> +		dev_err(dev, "clk not initialized\n");

What? How?

> +		return -ENODEV;
> +	}

Best regards,
Krzysztof

Powered by blists - more mailing lists