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

Krzysztof Kozlowski
2026-01-11T10:35:09Z

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