[PATCH v4 0/2] Add driver support for ESWIN EIC7700 PVT controller

[PATCH v4 0/2] Add driver support for ESWIN EIC7700 PVT controller

[hehuan1]

From: Huan He <hehuan1@eswincomputing.com>

Add support for the ESWIN EIC7700 PVT (Voltage, Temperature) sensor

Features:
The driver supports monitoring of voltage and temperature parameters
through the hardware monitoring subsystem. It provides an access to the
sampled Temperature and Voltage.

Test:
Tested this patch on the SiFive HiFive Premier P550 (which uses the ESWIN
EIC7700 SoC).

Updates:

  Changes in v4:
  - Update eswin,eic7700-pvt.yaml
    - Delete reviewed-by tag of Conor Dooley, because the label enum
      constraint is introduced
  - Update eic7700-pvt.c and eic7700-pvt.h
    - Remove the unused LVT/ULVT/SVT process-monitoring channels
    - Remove the probe-time power check since the PVT block is always
      powered on EIC7700 and the extra verification is unnecessary
    - Stop requesting the interrupt as shared and use the dedicated PVT IRQ
      only
    - Reorder probe initialization so the interface is initialized before
      the clock is disabled, avoiding register accesses with the clock gated
    - Fix runtime PM reference handling on error paths by balancing
      pm_runtime_get_noresume() with pm_runtime_put_noidle()
    - Add pm_runtime_put_noidle() handling for failed pm_runtime_get_sync()
      calls in hwmon read/write paths
    - Switch the PM callback registration from pm_sleep_ptr() to pm_ptr()

  Changes in v3:
  - Update eswin,eic7700-pvt.yaml
    - Remove redundant label property description and use 'label: true' to
      reference the definition in hwmon-common.yaml
    - Replace 'additionalProperties: false' with
      'unevaluatedProperties: false'
    - Remove the description for '#thermal-sensor-cells'
  - Update eic7700-pvt.c and eic7700-pvt.h
    - Fix clock reference count imbalance with Runtime PM:
      Replace devm_clk_get_enabled() with devm_clk_get() and manually
      manage clock enable/disable to avoid double-disable in remove() when
      Runtime PM is active. Clock is now enabled only during probe for
      eic7700_pvt_check_pwr(), then disabled before enabling Runtime PM,
      which takes full control of the clock thereafter
    - Add detailed comment explaining the spurious interrupt risk in
      eic7700_pvt_check_pwr()
    - Replace wait_for_completion_interruptible() with
      wait_for_completion_timeout() to prevent infinite wait

  Changes in v2:
  - Update eswin,eic7700-pvt.yaml
    - Reference the hwmon-common.yaml file
    - Remove the clock-names and reset-names properties
    - Move additionalProperties: false after the required block
    - Remove one example node to avoid redundancy
  - Update eic7700-pvt.c and eic7700-pvt.h
    - Remove unused sensor macros (PVT_SENSOR_FIRST, PVT_SENSOR_LAST,
      PVT_SENSORS_NUM)
    - Drop the unnecessary hwmon-sysfs.h header
    - Replace dynamic sensor info allocation with a static array and unify
      sensor labels
    - Remove unused hwmon_temp_type attribute
    - Eliminate redundant validation checks
    - Remove mutex and related locking, relying on hwmon core
      serialization
    - Replace per-sensor caches and completions with a single data cache
      and completion object
    - Remove pvt->sensor tracking. ISR no longer depends on the currently
      selected sensor
    - Move devm_add_action() registration after init_completion() for
      safer cleanup, and update cleanup function (pvt_clear_data)
    - Replace devm_reset_control_get_optional_exclusive() with
      devm_reset_control_get_exclusive_deasserted()
    - Replace eic7700_pvt_remove() with eic7700_pvt_disable_pm_runtime()
      and move it after PM runtime enable to avoid resource leaks on probe
      failure and remove clock disable and reset assert from
      eic7700_pvt_disable_pm_runtime() as it is already handled by devm_*
      framework
    - Remove redundant clock presence check in runtime_resume

  - Link to v1: https://lore.kernel.org/all/20260109090718.442-1-hehuan1@eswincomputing.com/

Huan He (2):
  dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
  hwmon: Add Eswin EIC7700 PVT sensor driver

 .../bindings/hwmon/eswin,eic7700-pvt.yaml     |  68 ++
 drivers/hwmon/Kconfig                         |  12 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/eic7700-pvt.c                   | 591 ++++++++++++++++++
 drivers/hwmon/eic7700-pvt.h                   |  99 +++
 5 files changed, 771 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
 create mode 100644 drivers/hwmon/eic7700-pvt.c
 create mode 100644 drivers/hwmon/eic7700-pvt.h

-- 
2.25.1

[PATCH v4 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor

[hehuan1]

From: Huan He <hehuan1@eswincomputing.com>

Add device tree binding documentation for ESWIN EIC7700 Process, Voltage
and Temperature sensor.

The EIC7700 SoC integrates two PVT instances for monitoring SoC and DDR
power domains respectively.

Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
Signed-off-by: Huan He <hehuan1@eswincomputing.com>
---
 .../bindings/hwmon/eswin,eic7700-pvt.yaml     | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml b/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
new file mode 100644
index 000000000000..ba89b829ee94
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/eswin,eic7700-pvt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ESWIN EIC7700 PVT Sensor
+
+maintainers:
+  - Yulin Lu <luyulin@eswincomputing.com>
+  - Huan He <hehuan1@eswincomputing.com>
+
+description:
+  ESWIN EIC7700 SoC integrates embedded process, voltage and temperature
+  sensors to monitor the internal SoC environment. The system includes two
+  PVT sensor instances. The PVT0 monitors the main SoC power domain. The
+  PVT1 sensor monitors the DDR core power domain.
+
+allOf:
+  - $ref: /schemas/hwmon/hwmon-common.yaml#
+
+properties:
+  compatible:
+    const: eswin,eic7700-pvt
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  label:
+    enum:
+      - pvt0
+      - pvt1
+
+  resets:
+    maxItems: 1
+
+  '#thermal-sensor-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - interrupts
+  - label
+  - resets
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    pvt@50b00000 {
+      compatible = "eswin,eic7700-pvt";
+      reg = <0x50b00000 0x10000>;
+      clocks = <&clocks 244>;
+      interrupts = <349>;
+      interrupt-parent = <&plic>;
+      label = "pvt0";
+      resets = <&reset 111>;
+      #thermal-sensor-cells = <0>;
+    };
+...
-- 
2.25.1

Re: [PATCH v4 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The 'label' property is required and restricted to an enum, misusing it for hardware instance identification.
- [Medium] The '#thermal-sensor-cells' property is defined but omitted from the 'required' list.
--

commit 5a5ed34d17ea4e88cd80b5a8e96e21dcc65e9f11
Author: Huan He <hehuan1@eswincomputing.com>

dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor

This commit adds the device tree binding documentation for the Eswin
EIC7700 Process, Voltage and Temperature sensor. The SoC integrates two
PVT instances for monitoring the main SoC and DDR power domains.

> diff --git a/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml b/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
> new file mode 100644
> index 0000000000000..ba89b829ee947
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml

[ ... ]

> +  interrupts:
> +    maxItems: 1
> +
> +  label:
> +    enum:
> +      - pvt0
> +      - pvt1

Is it appropriate to restrict the standard label property to a strict enum
and require it? 

The devicetree specification generally defines label as an optional,
human-readable string. Should the hardware instances be differentiated by
their reg addresses, aliases, or dedicated properties instead of imposing
a strict enum on a standard free-form string?

> +
> +  resets:
> +    maxItems: 1
> +
> +  '#thermal-sensor-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +  - label
> +  - resets

Should #thermal-sensor-cells be included in this required list?

Since the device acts as a thermal sensor provider, omitting its cell-count
property allows incomplete device tree nodes to silently pass schema
validation. This might lead to runtime failures or misconfigurations when
dependent thermal zones attempt to reference the sensor.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260430064107.1598-1-hehuan1@eswincomputing.com?part=1

Re: Re: [PATCH v4 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor

[Huan He]

Hi Sashiko,

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

> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The 'label' property is required and restricted to an enum, misusing it for hardware instance identification.
> - [Medium] The '#thermal-sensor-cells' property is defined but omitted from the 'required' list.
> --
> 
> commit 5a5ed34d17ea4e88cd80b5a8e96e21dcc65e9f11
> Author: Huan He <hehuan1@eswincomputing.com>
> 
> dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
> 
> This commit adds the device tree binding documentation for the Eswin
> EIC7700 Process, Voltage and Temperature sensor. The SoC integrates two
> PVT instances for monitoring the main SoC and DDR power domains.
> 
> > diff --git a/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml b/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
> > new file mode 100644
> > index 0000000000000..ba89b829ee947
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
> 
> [ ... ]
> 
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  label:
> > +    enum:
> > +      - pvt0
> > +      - pvt1
> 
> Is it appropriate to restrict the standard label property to a strict enum
> and require it? 
> 
> The devicetree specification generally defines label as an optional,
> human-readable string. Should the hardware instances be differentiated by
> their reg addresses, aliases, or dedicated properties instead of imposing
> a strict enum on a standard free-form string?

I am planning to update the next revision as follows. Would this be
acceptable?

YAML:
-  label:
-    enum:
-      - pvt0
-      - pvt1
+  label: true

required:
  - compatible
  - reg
  - clocks
  - interrupts
- - label

Driver:
 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;
-   }
+   const char *name = "pvt";
+
+   of_property_read_string(pvt->dev->of_node, "label", &name);
 
-   pvt->hwmon = devm_hwmon_device_register_with_info(pvt->dev, names[type],
+   pvt->hwmon = devm_hwmon_device_register_with_info(pvt->dev, name,
                              pvt, &pvt_hwmon_info,
                              NULL);

> 
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  '#thermal-sensor-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - interrupts
> > +  - label
> > +  - resets
> 
> Should #thermal-sensor-cells be included in this required list?
> 
> Since the device acts as a thermal sensor provider, omitting its cell-count
> property allows incomplete device tree nodes to silently pass schema
> validation. This might lead to runtime failures or misconfigurations when
> dependent thermal zones attempt to reference the sensor.

I will add "#thermal-sensor-cells" to the required list in the next
revision.

Best regards,
Huan He

Re: [PATCH v4 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor

[Krzysztof Kozlowski]

On Thu, Apr 30, 2026 at 02:44:44PM +0800, hehuan1@eswincomputing.com wrote:
> +
> +  label:
> +    enum:
> +      - pvt0
> +      - pvt1

No, label is user-visible name. Can be whatever user decides.

Please read writing bindings - instance IDs are not allowed.

> +
> +  resets:
> +    maxItems: 1
> +
> +  '#thermal-sensor-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +  - label
> +  - resets
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    pvt@50b00000 {


Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
If you cannot find a name matching your device, please check in kernel
sources for similar cases or you can grow the spec (via pull request to
DT spec repo).

Best regards,
Krzysztof

Re: Re: [PATCH v4 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor

[Huan He]

Hi Krzysztof,

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

> On Thu, Apr 30, 2026 at 02:44:44PM +0800, hehuan1@eswincomputing.com wrote:
> > +
> > +  label:
> > +    enum:
> > +      - pvt0
> > +      - pvt1
> 
> No, label is user-visible name. Can be whatever user decides.
> 
> Please read writing bindings - instance IDs are not allowed.

Thanks for the clarification.
I am planning to update the next revision as follows. Would this be
acceptable?

YAML:
-  label:
-    enum:
-      - pvt0
-      - pvt1
+  label: true

required:
  - compatible
  - reg
  - clocks
  - interrupts
- - label

Driver:
 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;
-   }
+   const char *name = "pvt";
+
+   of_property_read_string(pvt->dev->of_node, "label", &name);
 
-   pvt->hwmon = devm_hwmon_device_register_with_info(pvt->dev, names[type],
+   pvt->hwmon = devm_hwmon_device_register_with_info(pvt->dev, name,
                              pvt, &pvt_hwmon_info,
                              NULL);

> 
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  '#thermal-sensor-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - interrupts
> > +  - label
> > +  - resets
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    pvt@50b00000 {
> 
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> If you cannot find a name matching your device, please check in kernel
> sources for similar cases or you can grow the spec (via pull request to
> DT spec repo).

I will update the example node name from "pvt@..." to the generic
"sensor@...". Is this acceptable?

Best regards,
Huan He

Re: [PATCH v4 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor

[Guenter Roeck]

On 5/12/26 02:16, Huan He wrote:
> Hi Krzysztof,
> 
> Thank you very much for your detailed review. We appreciate the feedback.
> 
>> On Thu, Apr 30, 2026 at 02:44:44PM +0800, hehuan1@eswincomputing.com wrote:
>>> +
>>> +  label:
>>> +    enum:
>>> +      - pvt0
>>> +      - pvt1
>>
>> No, label is user-visible name. Can be whatever user decides.
>>
>> Please read writing bindings - instance IDs are not allowed.
> 
> Thanks for the clarification.
> I am planning to update the next revision as follows. Would this be
> acceptable?
> 
> YAML:
> -  label:
> -    enum:
> -      - pvt0
> -      - pvt1
> +  label: true
> 
> required:
>    - compatible
>    - reg
>    - clocks
>    - interrupts
> - - label
> 
> Driver:
>   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;
> -   }
> +   const char *name = "pvt";
> +
> +   of_property_read_string(pvt->dev->of_node, "label", &name);
>   
> -   pvt->hwmon = devm_hwmon_device_register_with_info(pvt->dev, names[type],
> +   pvt->hwmon = devm_hwmon_device_register_with_info(pvt->dev, name,
>                                pvt, &pvt_hwmon_info,
>                                NULL);
> 

This would try to register a free-text name for the hwmon device,
which would be unacceptable.

There are lots of multi-channel devices out there. None of them
have those problems. Why do you insist in free-text names instead of
using, say, "reg" to distinguish the channels ?

Guenter

>>
>>> +
>>> +  resets:
>>> +    maxItems: 1
>>> +
>>> +  '#thermal-sensor-cells':
>>> +    const: 0
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - clocks
>>> +  - interrupts
>>> +  - label
>>> +  - resets
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    pvt@50b00000 {
>>
>>
>> Node names should be generic. See also an explanation and list of
>> examples (not exhaustive) in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>> If you cannot find a name matching your device, please check in kernel
>> sources for similar cases or you can grow the spec (via pull request to
>> DT spec repo).
> 
> I will update the example node name from "pvt@..." to the generic
> "sensor@...". Is this acceptable?
> 
> Best regards,
> Huan He

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

[hehuan1]

From: Huan He <hehuan1@eswincomputing.com>

Add support for ESWIN EIC7700 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>
---
 drivers/hwmon/Kconfig       |  12 +
 drivers/hwmon/Makefile      |   1 +
 drivers/hwmon/eic7700-pvt.c | 591 ++++++++++++++++++++++++++++++++++++
 drivers/hwmon/eic7700-pvt.h |  99 ++++++
 4 files changed, 703 insertions(+)
 create mode 100644 drivers/hwmon/eic7700-pvt.c
 create mode 100644 drivers/hwmon/eic7700-pvt.h

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 14e4cea48acc..1a69dfe7bdc1 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2033,6 +2033,18 @@ config SENSORS_DME1737
 	  This driver can also be built as a module. If so, the module
 	  will be called dme1737.
 
+config SENSORS_EIC7700_PVT
+	tristate "Eswin EIC7700 Process, Voltage, Temperature sensor driver"
+	depends on ARCH_ESWIN || COMPILE_TEST
+	depends on HWMON
+	select POLYNOMIAL
+	help
+	  If you say yes here you get support for Eswin EIC7700 PVT sensor
+	  embedded into the SoC.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called eic7700-pvt.
+
 config SENSORS_EMC1403
 	tristate "SMSC EMC1403/23 thermal sensor"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 4788996aa137..3f8c347e3758 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -71,6 +71,7 @@ obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
 obj-$(CONFIG_SENSORS_DRIVETEMP)	+= drivetemp.o
 obj-$(CONFIG_SENSORS_DS620)	+= ds620.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
+obj-$(CONFIG_SENSORS_EIC7700_PVT) += eic7700-pvt.o
 obj-$(CONFIG_SENSORS_EMC1403)	+= emc1403.o
 obj-$(CONFIG_SENSORS_EMC2103)	+= emc2103.o
 obj-$(CONFIG_SENSORS_EMC2305)	+= emc2305.o
diff --git a/drivers/hwmon/eic7700-pvt.c b/drivers/hwmon/eic7700-pvt.c
new file mode 100644
index 000000000000..d9d6a035defe
--- /dev/null
+++ b/drivers/hwmon/eic7700-pvt.c
@@ -0,0 +1,591 @@
+// 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@eswincomputing.com>
+ *   Huan He <hehuan1@eswincomputing.com>
+ */
+
+#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),
+};
+
+/*
+ * 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);
+	/*
+	 * 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
+		*val = polynomial_calc(&poly_N_to_volt, 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_LABEL | HWMON_T_OFFSET),
+	HWMON_CHANNEL_INFO(in, 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_read_trim(struct pvt_hwmon *pvt, long *val)
+{
+	u32 data;
+
+	data = readl(pvt->regs + PVT_TRIM);
+	*val = data;
+
+	return 0;
+}
+
+static int eic7700_pvt_write_trim(struct pvt_hwmon *pvt, long val)
+{
+	/*
+	 * Update PVT trim register safely while the controller is temporarily
+	 * disabled.
+	 */
+	eic7700_pvt_set_trim(pvt, val);
+
+	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);
+		pm_runtime_put_noidle(pvt->dev);
+		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);
+		pm_runtime_put_noidle(pvt->dev);
+		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_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_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");
+
+	ret = clk_prepare_enable(pvt->clk);
+	if (ret)
+		return dev_err_probe(pvt->dev, ret, "Failed to enable clock\n");
+
+	ret = eic7700_pvt_init_iface(pvt);
+	if (ret) {
+		clk_disable_unprepare(pvt->clk);
+		return ret;
+	}
+
+	clk_disable_unprepare(pvt->clk);
+
+	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) {
+		pm_runtime_put_noidle(&pdev->dev);
+		return dev_err_probe(&pdev->dev, ret,
+				     "Can't register PM cleanup\n");
+	}
+
+	ret = eic7700_pvt_create_hwmon(pvt);
+	if (ret)
+		goto err_put_pm_runtime;
+
+	pm_runtime_put_autosuspend(&pdev->dev);
+
+	return 0;
+
+err_put_pm_runtime:
+	pm_runtime_put_noidle(&pdev->dev);
+	return ret;
+}
+
+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);
+
+	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_ptr(&eic7700_pvt_pm_ops),
+	},
+};
+module_platform_driver(pvt_driver);
+
+MODULE_AUTHOR("Yulin Lu <luyulin@eswincomputing.com>");
+MODULE_AUTHOR("Huan He <hehuan1@eswincomputing.com>");
+MODULE_DESCRIPTION("Eswin eic7700 PVT driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/eic7700-pvt.h b/drivers/hwmon/eic7700-pvt.h
new file mode 100644
index 000000000000..4b46cdeb2d58
--- /dev/null
+++ b/drivers/hwmon/eic7700-pvt.h
@@ -0,0 +1,99 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ESWIN EIC7700 Process, Voltage, Temperature sensor driver
+ *
+ * Copyright 2026, Beijing ESWIN Computing Technology Co., Ltd.
+ */
+#ifndef __HWMON_EIC7700_PVT_H__
+#define __HWMON_EIC7700_PVT_H__
+
+#include <linux/completion.h>
+#include <linux/hwmon.h>
+#include <linux/kernel.h>
+#include <linux/time.h>
+
+/* ESWIN EIC7700 PVT registers and their bitfields */
+#define PVT_TRIM		0x04
+#define PVT_MODE		0x08
+#define PVT_MODE_MASK		GENMASK(2, 0)
+#define PVT_CTRL_MODE_TEMP	0x0
+#define PVT_CTRL_MODE_VOLT	0x4
+#define PVT_ENA			0x0c
+#define PVT_ENA_EN		BIT(0)
+#define PVT_INT			0x10
+#define PVT_INT_STAT		BIT(0)
+#define PVT_INT_CLR		BIT(1)
+#define PVT_DATA		0x14
+#define PVT_DATA_OUT		GENMASK(9, 0)
+
+/*
+ * PVT sensors-related limits and default values
+ * @PVT_TEMP_CHS: Number of temperature hwmon channels.
+ * @PVT_VOLT_CHS: Number of voltage hwmon channels.
+ * @PVT_TRIM_DEF: Default temperature sensor trim value (set a proper value
+ *		  when one is determined for ESWIN EIC7700 SoC).
+ * @PVT_TOUT_MIN: Minimal timeout between samples in nanoseconds.
+ */
+#define PVT_TEMP_CHS		1
+#define PVT_VOLT_CHS		1
+#define PVT_TRIM_DEF		0
+#define PVT_TOUT_MIN		(NSEC_PER_SEC / 3000)
+
+/*
+ * enum pvt_sensor_type - ESWIN EIC7700 PVT sensor types (correspond to each PVT
+ *			  sampling mode)
+ * @PVT_TEMP: PVT Temperature sensor.
+ * @PVT_VOLT: PVT Voltage sensor.
+ */
+enum pvt_sensor_type {
+	PVT_TEMP = 0,
+	PVT_VOLT
+};
+
+/*
+ * struct pvt_sensor_info - ESWIN EIC7700 PVT sensor informational structure
+ * @channel: Sensor channel ID.
+ * @label: hwmon sensor label.
+ * @mode: PVT mode corresponding to the channel.
+ * @type: Sensor type.
+ */
+struct pvt_sensor_info {
+	int channel;
+	const char *label;
+	u32 mode;
+	enum hwmon_sensor_types type;
+};
+
+#define PVT_SENSOR_INFO(_ch, _label, _type, _mode)	\
+	{						\
+		.channel = _ch,				\
+		.label = _label,			\
+		.mode = PVT_CTRL_MODE_ ##_mode,		\
+		.type = _type,				\
+	}
+
+/*
+ * struct pvt_hwmon - Eswin EIC7700 PVT private data
+ * @dev: device structure of the PVT platform device.
+ * @hwmon: hwmon device structure.
+ * @regs: pointer to the Eswin EIC7700 PVT registers region.
+ * @irq: PVT events IRQ number.
+ * @clk: PVT core clock (1.2MHz).
+ * @rst: pointer to the reset descriptor.
+ * @data_cache: data cache in raw format.
+ * @conversion: data conversion completion.
+ * @timeout: conversion timeout.
+ */
+struct pvt_hwmon {
+	struct device *dev;
+	struct device *hwmon;
+	void __iomem *regs;
+	int irq;
+	struct clk *clk;
+	struct reset_control *rst;
+	u32 data_cache;
+	struct completion conversion;
+	ktime_t timeout;
+};
+
+#endif /* __HWMON_EIC7700_PVT_H__ */
-- 
2.25.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Hardware access with disabled clock when CONFIG_PM=n
- [High] Resource leak and hardware powered on during driver unbind
- [High] System suspend state desynchronization causes potential crashes
- [High] Unclocked hardware access in the interrupt handler
- [Medium] Missing synchronization on timeout path causes race condition and stale data
- [Medium] Unvalidated sysfs input and hwmon ABI violation for temp_offset
- [Low] Mismatch between commit message and implemented voltage channels
--

commit 226d2b003d7fbabad9b8abaf9c71cf6fdf386a91
Author: Huan He <hehuan1@eswincomputing.com>

hwmon: Add Eswin EIC7700 PVT sensor driver
    
Add support for ESWIN EIC7700 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.

> The PVT IP contains one temperature sensor and four voltage sensors for
> process variation monitoring.

This isn't a bug, but the commit message states there are four voltage
sensors, while the driver implementation only exposes a single voltage channel.
Could the commit message be updated to match the implementation?

> diff --git a/drivers/hwmon/eic7700-pvt.c b/drivers/hwmon/eic7700-pvt.c
> new file mode 100644
> index 0000000000000..d9d6a035defeb
> --- /dev/null
> +++ b/drivers/hwmon/eic7700-pvt.c
[ ... ]
> +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);
> +	/*
> +	 * Read the data, update the cache and notify a waiter of this event.
> +	 */
> +	val = readl(pvt->regs + PVT_DATA);

Could this cause a bus fault if a spurious or delayed interrupt fires while
the device is in the RPM_SUSPENDED state?

When the device is suspended, the hardware clock is gated. Accessing registers
without checking the PM runtime state could lead to a crash. Should this use
pm_runtime_get_if_active() to protect against accessing unclocked hardware?

[ ... ]
> +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 the timeout expires, could an interrupt still be in flight?

If so, a subsequent read request would call reinit_completion(), and the
delayed interrupt handler could execute, complete the new request prematurely,
and cause the read to return stale data.

Does the timeout path need to call synchronize_irq() to ensure no interrupt
handler is running before returning?

[ ... ]
> +static int eic7700_pvt_write_trim(struct pvt_hwmon *pvt, long val)
> +{
> +	/*
> +	 * Update PVT trim register safely while the controller is temporarily
> +	 * disabled.
> +	 */
> +	eic7700_pvt_set_trim(pvt, val);
> +
> +	return 0;
> +}

Does this code validate the user input before writing it to the hardware?

Writing a negative value could truncate to 0xFFFFFFFF and overwrite reserved
bits in the register. Additionally, the hwmon ABI specifies that temp_offset
should be in millidegrees Celsius, but this writes the raw value directly to
the register without scaling.

[ ... ]
> +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);
> +}

Can this leak the clock reference and leave the hardware powered on if the
device is unbound?

Because pm_runtime_put_autosuspend() is used after reads, the device remains
active for a 3-second window. If the module is removed during this window,
pm_runtime_disable() will cancel the pending suspend but it does not suspend
a device that is currently RPM_ACTIVE.

Should this cleanup action explicitly suspend the device if it is active?

[ ... ]
> +static int eic7700_pvt_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = eic7700_pvt_init_iface(pvt);
> +	if (ret) {
> +		clk_disable_unprepare(pvt->clk);
> +		return ret;
> +	}
> +
> +	clk_disable_unprepare(pvt->clk);

Is this driver intended to be built when CONFIG_PM is disabled?

If CONFIG_PM is disabled, pm_runtime_get_sync() in the hwmon read function is
a no-op that returns 1. Because the clock is explicitly disabled at the end of
probe() and relies entirely on PM runtime to re-enable it, a read operation
will access the hardware registers while the clock is gated, leading to an
external abort or system hang.

Also, eic7700_pvt_suspend() checks !pm_runtime_status_suspended(dev), which
is always false when CONFIG_PM=n, causing it to erroneously call
clk_disable_unprepare() on an already disabled clock.

[ ... ]
> +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;
> +}

Does calling eic7700_pvt_runtime_suspend() directly leave the PM runtime
state desynchronized?

This manually gates the clock if the device is active, but it fails to update
the device's PM runtime status. The PM core still considers the device to be
RPM_ACTIVE.

If a kernel thread calls pm_runtime_get_sync() before late suspend, it will
see the state as active, return immediately without re-enabling the clock,
and crash when accessing registers.

Should this use pm_runtime_force_suspend() and pm_runtime_force_resume() to
safely maintain state consistency?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260430064107.1598-1-hehuan1@eswincomputing.com?part=2

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

[Huan He]

Hi Sashiko, Guenter,

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

> 
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [High] Hardware access with disabled clock when CONFIG_PM=n
> - [High] Resource leak and hardware powered on during driver unbind
> - [High] System suspend state desynchronization causes potential crashes
> - [High] Unclocked hardware access in the interrupt handler
> - [Medium] Missing synchronization on timeout path causes race condition and stale data
> - [Medium] Unvalidated sysfs input and hwmon ABI violation for temp_offset
> - [Low] Mismatch between commit message and implemented voltage channels
> --
> 
> commit 226d2b003d7fbabad9b8abaf9c71cf6fdf386a91
> Author: Huan He <hehuan1@eswincomputing.com>
> 
> hwmon: Add Eswin EIC7700 PVT sensor driver
>     
> Add support for ESWIN EIC7700 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.
> 
> > The PVT IP contains one temperature sensor and four voltage sensors for
> > process variation monitoring.
> 
> This isn't a bug, but the commit message states there are four voltage
> sensors, while the driver implementation only exposes a single voltage channel.
> Could the commit message be updated to match the implementation?

I will update the commit message accordingly.

The current driver only exposes one temperature channel and one voltage
channel, so the commit text will be changed to match the implementation.

> 
> > diff --git a/drivers/hwmon/eic7700-pvt.c b/drivers/hwmon/eic7700-pvt.c
> > new file mode 100644
> > index 0000000000000..d9d6a035defeb
> > --- /dev/null
> > +++ b/drivers/hwmon/eic7700-pvt.c
> [ ... ]
> > +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);
> > +	/*
> > +	 * Read the data, update the cache and notify a waiter of this event.
> > +	 */
> > +	val = readl(pvt->regs + PVT_DATA);
> 
> Could this cause a bus fault if a spurious or delayed interrupt fires while
> the device is in the RPM_SUSPENDED state?
> 
> When the device is suspended, the hardware clock is gated. Accessing registers
> without checking the PM runtime state could lead to a crash. Should this use
> pm_runtime_get_if_active() to protect against accessing unclocked hardware?

Yes, this is a valid concern.

If a delayed or spurious interrupt fires while the device is
runtime-suspended, the handler may otherwise touch registers after the
clock has been gated.

I will update the ISR to guard register accesses with
pm_runtime_get_if_active() and only access the hardware when the device is
runtime active. The temporary PM reference will be dropped with
pm_runtime_put_noidle() on the handled path.

The ISR will be changed along these lines:
+   int active;
+   if (IS_ENABLED(CONFIG_PM)) {
+       active = pm_runtime_get_if_active(pvt->dev);
+       if (active <= 0)
+           return IRQ_NONE;
+   }

    ... access registers ...

+   if (IS_ENABLED(CONFIG_PM))
+       pm_runtime_put_noidle(pvt->dev);

> 
> [ ... ]
> > +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 the timeout expires, could an interrupt still be in flight?
> 
> If so, a subsequent read request would call reinit_completion(), and the
> delayed interrupt handler could execute, complete the new request prematurely,
> and cause the read to return stale data.
> 
> Does the timeout path need to call synchronize_irq() to ensure no interrupt
> handler is running before returning?

If a timeout occurs, a delayed interrupt may still be in flight and could
complete the next conversion request after reinit_completion().

I will add synchronize_irq() on the timeout path so that any in-flight
handler has completed before returning.

The eic7700_pvt_read_data() function will be changed along these lines:

    eic7700_pvt_update(pvt->regs + PVT_INT, PVT_INT_CLR, PVT_INT_CLR);

+   if (!ret)
+       synchronize_irq(pvt->irq);
+
    data = READ_ONCE(pvt->data_cache);

> 
> [ ... ]
> > +static int eic7700_pvt_write_trim(struct pvt_hwmon *pvt, long val)
> > +{
> > +	/*
> > +	 * Update PVT trim register safely while the controller is temporarily
> > +	 * disabled.
> > +	 */
> > +	eic7700_pvt_set_trim(pvt, val);
> > +
> > +	return 0;
> > +}
> 
> Does this code validate the user input before writing it to the hardware?
> 
> Writing a negative value could truncate to 0xFFFFFFFF and overwrite reserved
> bits in the register. Additionally, the hwmon ABI specifies that temp_offset
> should be in millidegrees Celsius, but this writes the raw value directly to
> the register without scaling.

Agreed.

The current temp_offset implementation exposes the raw PVT trim register
directly, which does not match the hwmon ABI requirement for millidegree
Celsius units.

I will remove the temp_offset attribute in the next revision instead of
exposing an ABI-incompatible raw hardware value.

> 
> [ ... ]
> > +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);
> > +}
> 
> Can this leak the clock reference and leave the hardware powered on if the
> device is unbound?
> 
> Because pm_runtime_put_autosuspend() is used after reads, the device remains
> active for a 3-second window. If the module is removed during this window,
> pm_runtime_disable() will cancel the pending suspend but it does not suspend
> a device that is currently RPM_ACTIVE.
> 
> Should this cleanup action explicitly suspend the device if it is active?

pm_runtime_disable() alone does not guarantee that an RPM_ACTIVE device
is runtime-suspended during driver unbind, so the clock may remain enabled
if the driver is removed inside the autosuspend window.

I will fix this by forcing the device into runtime suspend before
disabling runtime PM in the cleanup path.

The eic7700_pvt_disable_pm_runtime() function will be changed along these
lines:

    pm_runtime_dont_use_autosuspend(pvt->dev);
+   pm_runtime_force_suspend(pvt->dev);
    pm_runtime_disable(pvt->dev);

> 
> [ ... ]
> > +static int eic7700_pvt_probe(struct platform_device *pdev)
> > +{
> [ ... ]
> > +	ret = eic7700_pvt_init_iface(pvt);
> > +	if (ret) {
> > +		clk_disable_unprepare(pvt->clk);
> > +		return ret;
> > +	}
> > +
> > +	clk_disable_unprepare(pvt->clk);
> 
> Is this driver intended to be built when CONFIG_PM is disabled?
> 
> If CONFIG_PM is disabled, pm_runtime_get_sync() in the hwmon read function is
> a no-op that returns 1. Because the clock is explicitly disabled at the end of
> probe() and relies entirely on PM runtime to re-enable it, a read operation
> will access the hardware registers while the clock is gated, leading to an
> external abort or system hang.

With CONFIG_PM disabled, pm_runtime_get_sync() becomes a stub and the
runtime PM callbacks are never invoked, while probe() currently gates the
clock at the end. As a result, later hwmon register accesses may touch the
hardware with the clock disabled.

I will fix this by keeping the clock enabled permanently when CONFIG_PM=n
and gating it only during driver cleanup.

The eic7700_pvt_probe() function will be changed along these lines:

ret = devm_add_action_or_reset(dev, eic7700_pvt_disable_pm_runtime, pvt);
if (ret)
    return ret;

pm_runtime_put_autosuspend(dev);

+   if (!IS_ENABLED(CONFIG_PM)) {
+       ret = clk_prepare_enable(pvt->clk);
+       if (ret)
+           return dev_err_probe(&pdev->dev, ret,
+                        "Failed to enable clock\n");
+   }

In eic7700_pvt_disable_pm_runtime:

    pm_runtime_disable(pvt->dev);
+   if (!IS_ENABLED(CONFIG_PM))
+       clk_disable_unprepare(pvt->clk);

> 
> Also, eic7700_pvt_suspend() checks !pm_runtime_status_suspended(dev), which
> is always false when CONFIG_PM=n, causing it to erroneously call
> clk_disable_unprepare() on an already disabled clock.

This part is not applicable with CONFIG_PM=n.

The driver uses pm_ptr(&eic7700_pvt_pm_ops), so with CONFIG_PM disabled
the PM callback pointer is NULL and eic7700_pvt_suspend() is not executed
by the PM core. Therefore there is no real double clk_disable_unprepare()
from that path.

> 
> [ ... ]
> > +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;
> > +}
> 
> Does calling eic7700_pvt_runtime_suspend() directly leave the PM runtime
> state desynchronized?
> 
> This manually gates the clock if the device is active, but it fails to update
> the device's PM runtime status. The PM core still considers the device to be
> RPM_ACTIVE.
> 
> If a kernel thread calls pm_runtime_get_sync() before late suspend, it will
> see the state as active, return immediately without re-enabling the clock,
> and crash when accessing registers.
> 
> Should this use pm_runtime_force_suspend() and pm_runtime_force_resume() to
> safely maintain state consistency?

Calling the driver's runtime suspend/resume callbacks directly from the
system sleep callbacks does not update the PM core state, so the runtime
PM status can become inconsistent with the actual clock state.

I will switch the system sleep callbacks to pm_runtime_force_suspend() and
pm_runtime_force_resume() in the next revision.

eic7700_pvt_pm_ops will be changed along these lines:

-   SYSTEM_SLEEP_PM_OPS(eic7700_pvt_suspend, eic7700_pvt_resume)
+   SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)

Best regards,
Huan He


With CONFIG_PM disabled, pm_runtime_get_sync() becomes a stub and the
runtime PM callbacks are never invoked, while probe() currently gates the
clock at the end. As a result, later hwmon register accesses may touch the
hardware with the clock disabled.

I will fix this by keeping the clock enabled permanently when CONFIG_PM=n
and gating it only during driver cleanup.

ret = devm_add_action_or_reset(dev, eic7700_pvt_disable_pm_runtime, pvt);
if (ret)
    return ret;

pm_runtime_put_autosuspend(dev);

+   if (!IS_ENABLED(CONFIG_PM)) {
+       ret = clk_prepare_enable(pvt->clk);
+       if (ret)
+           return dev_err_probe(&pdev->dev, ret,
+                        "Failed to enable clock\n");
+   }
In eic7700_pvt_disable_pm_runtime:

    pm_runtime_disable(pvt->dev);
+   if (!IS_ENABLED(CONFIG_PM))
+       clk_disable_unprepare(pvt->clk);

With CONFIG_PM disabled, pm_runtime_get_sync() becomes a stub and the
runtime PM callbacks are never invoked, while probe() currently gates the
clock at the end. As a result, later hwmon register accesses may touch the
hardware with the clock disabled.

I will fix this by keeping the clock enabled permanently when CONFIG_PM=n
and gating it only during driver cleanup.

ret = devm_add_action_or_reset(dev, eic7700_pvt_disable_pm_runtime, pvt);
if (ret)
    return ret;

pm_runtime_put_autosuspend(dev);

+   if (!IS_ENABLED(CONFIG_PM)) {
+       ret = clk_prepare_enable(pvt->clk);
+       if (ret)
+           return dev_err_probe(&pdev->dev, ret,
+                        "Failed to enable clock\n");
+   }
In eic7700_pvt_disable_pm_runtime:

    pm_runtime_disable(pvt->dev);
+   if (!IS_ENABLED(CONFIG_PM))
+       clk_disable_unprepare(pvt->clk);

With CONFIG_PM disabled, pm_runtime_get_sync() becomes a stub and the
runtime PM callbacks are never invoked, while probe() currently gates the
clock at the end. As a result, later hwmon register accesses may touch the
hardware with the clock disabled.

I will fix this by keeping the clock enabled permanently when CONFIG_PM=n
and gating it only during driver cleanup.

ret = devm_add_action_or_reset(dev, eic7700_pvt_disable_pm_runtime, pvt);
if (ret)
    return ret;

pm_runtime_put_autosuspend(dev);

+   if (!IS_ENABLED(CONFIG_PM)) {
+       ret = clk_prepare_enable(pvt->clk);
+       if (ret)
+           return dev_err_probe(&pdev->dev, ret,
+                        "Failed to enable clock\n");
+   }
In eic7700_pvt_disable_pm_runtime:

    pm_runtime_disable(pvt->dev);
+   if (!IS_ENABLED(CONFIG_PM))
+       clk_disable_unprepare(pvt->clk);

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

[Huan He]

Hi Sashiko, Guenter,

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

> 
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [High] Hardware access with disabled clock when CONFIG_PM=n
> - [High] Resource leak and hardware powered on during driver unbind
> - [High] System suspend state desynchronization causes potential crashes
> - [High] Unclocked hardware access in the interrupt handler
> - [Medium] Missing synchronization on timeout path causes race condition and stale data
> - [Medium] Unvalidated sysfs input and hwmon ABI violation for temp_offset
> - [Low] Mismatch between commit message and implemented voltage channels
> --
> 
> commit 226d2b003d7fbabad9b8abaf9c71cf6fdf386a91
> Author: Huan He <hehuan1@eswincomputing.com>
> 
> hwmon: Add Eswin EIC7700 PVT sensor driver
>     
> Add support for ESWIN EIC7700 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.
> 
> > The PVT IP contains one temperature sensor and four voltage sensors for
> > process variation monitoring.
> 
> This isn't a bug, but the commit message states there are four voltage
> sensors, while the driver implementation only exposes a single voltage channel.
> Could the commit message be updated to match the implementation?

I will update the commit message accordingly.

The current driver only exposes one temperature channel and one voltage
channel, so the commit text will be changed to match the implementation.

> 
> > diff --git a/drivers/hwmon/eic7700-pvt.c b/drivers/hwmon/eic7700-pvt.c
> > new file mode 100644
> > index 0000000000000..d9d6a035defeb
> > --- /dev/null
> > +++ b/drivers/hwmon/eic7700-pvt.c
> [ ... ]
> > +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);
> > +	/*
> > +	 * Read the data, update the cache and notify a waiter of this event.
> > +	 */
> > +	val = readl(pvt->regs + PVT_DATA);
> 
> Could this cause a bus fault if a spurious or delayed interrupt fires while
> the device is in the RPM_SUSPENDED state?
> 
> When the device is suspended, the hardware clock is gated. Accessing registers
> without checking the PM runtime state could lead to a crash. Should this use
> pm_runtime_get_if_active() to protect against accessing unclocked hardware?

Yes, this is a valid concern.

If a delayed or spurious interrupt fires while the device is
runtime-suspended, the handler may otherwise touch registers after the
clock has been gated.

I will update the ISR to guard register accesses with
pm_runtime_get_if_active() and only access the hardware when the device is
runtime active. The temporary PM reference will be dropped with
pm_runtime_put_noidle() on the handled path.

The ISR will be changed along these lines:
+   int active;
+   if (IS_ENABLED(CONFIG_PM)) {
+       active = pm_runtime_get_if_active(pvt->dev);
+       if (active <= 0)
+           return IRQ_NONE;
+   }

    ... access registers ...

+   if (IS_ENABLED(CONFIG_PM))
+       pm_runtime_put_noidle(pvt->dev);

> 
> [ ... ]
> > +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 the timeout expires, could an interrupt still be in flight?
> 
> If so, a subsequent read request would call reinit_completion(), and the
> delayed interrupt handler could execute, complete the new request prematurely,
> and cause the read to return stale data.
> 
> Does the timeout path need to call synchronize_irq() to ensure no interrupt
> handler is running before returning?

If a timeout occurs, a delayed interrupt may still be in flight and could
complete the next conversion request after reinit_completion().

I will add synchronize_irq() on the timeout path so that any in-flight
handler has completed before returning.

The eic7700_pvt_read_data() function will be changed along these lines:

    eic7700_pvt_update(pvt->regs + PVT_INT, PVT_INT_CLR, PVT_INT_CLR);

+   if (!ret)
+       synchronize_irq(pvt->irq);
+
    data = READ_ONCE(pvt->data_cache);

> 
> [ ... ]
> > +static int eic7700_pvt_write_trim(struct pvt_hwmon *pvt, long val)
> > +{
> > +	/*
> > +	 * Update PVT trim register safely while the controller is temporarily
> > +	 * disabled.
> > +	 */
> > +	eic7700_pvt_set_trim(pvt, val);
> > +
> > +	return 0;
> > +}
> 
> Does this code validate the user input before writing it to the hardware?
> 
> Writing a negative value could truncate to 0xFFFFFFFF and overwrite reserved
> bits in the register. Additionally, the hwmon ABI specifies that temp_offset
> should be in millidegrees Celsius, but this writes the raw value directly to
> the register without scaling.

Agreed.

The current temp_offset implementation exposes the raw PVT trim register
directly, which does not match the hwmon ABI requirement for millidegree
Celsius units.

I will remove the temp_offset attribute in the next revision instead of
exposing an ABI-incompatible raw hardware value.

> 
> [ ... ]
> > +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);
> > +}
> 
> Can this leak the clock reference and leave the hardware powered on if the
> device is unbound?
> 
> Because pm_runtime_put_autosuspend() is used after reads, the device remains
> active for a 3-second window. If the module is removed during this window,
> pm_runtime_disable() will cancel the pending suspend but it does not suspend
> a device that is currently RPM_ACTIVE.
> 
> Should this cleanup action explicitly suspend the device if it is active?

pm_runtime_disable() alone does not guarantee that an RPM_ACTIVE device
is runtime-suspended during driver unbind, so the clock may remain enabled
if the driver is removed inside the autosuspend window.

I will fix this by forcing the device into runtime suspend before
disabling runtime PM in the cleanup path.

The eic7700_pvt_disable_pm_runtime() function will be changed along these
lines:

    pm_runtime_dont_use_autosuspend(pvt->dev);
+   pm_runtime_force_suspend(pvt->dev);
    pm_runtime_disable(pvt->dev);

> 
> [ ... ]
> > +static int eic7700_pvt_probe(struct platform_device *pdev)
> > +{
> [ ... ]
> > +	ret = eic7700_pvt_init_iface(pvt);
> > +	if (ret) {
> > +		clk_disable_unprepare(pvt->clk);
> > +		return ret;
> > +	}
> > +
> > +	clk_disable_unprepare(pvt->clk);
> 
> Is this driver intended to be built when CONFIG_PM is disabled?
> 
> If CONFIG_PM is disabled, pm_runtime_get_sync() in the hwmon read function is
> a no-op that returns 1. Because the clock is explicitly disabled at the end of
> probe() and relies entirely on PM runtime to re-enable it, a read operation
> will access the hardware registers while the clock is gated, leading to an
> external abort or system hang.

With CONFIG_PM disabled, pm_runtime_get_sync() becomes a stub and the
runtime PM callbacks are never invoked, while probe() currently gates the
clock at the end. As a result, later hwmon register accesses may touch the
hardware with the clock disabled.

I will fix this by keeping the clock enabled permanently when CONFIG_PM=n
and gating it only during driver cleanup.

The eic7700_pvt_probe() function will be changed along these lines:

ret = devm_add_action_or_reset(dev, eic7700_pvt_disable_pm_runtime, pvt);
if (ret)
    return ret;

pm_runtime_put_autosuspend(dev);

+   if (!IS_ENABLED(CONFIG_PM)) {
+       ret = clk_prepare_enable(pvt->clk);
+       if (ret)
+           return dev_err_probe(&pdev->dev, ret,
+                        "Failed to enable clock\n");
+   }

In eic7700_pvt_disable_pm_runtime:

    pm_runtime_disable(pvt->dev);
+   if (!IS_ENABLED(CONFIG_PM))
+       clk_disable_unprepare(pvt->clk);

> 
> Also, eic7700_pvt_suspend() checks !pm_runtime_status_suspended(dev), which
> is always false when CONFIG_PM=n, causing it to erroneously call
> clk_disable_unprepare() on an already disabled clock.

This part is not applicable with CONFIG_PM=n.

The driver uses pm_ptr(&eic7700_pvt_pm_ops), so with CONFIG_PM disabled
the PM callback pointer is NULL and eic7700_pvt_suspend() is not executed
by the PM core. Therefore there is no real double clk_disable_unprepare()
from that path.

> 
> [ ... ]
> > +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;
> > +}
> 
> Does calling eic7700_pvt_runtime_suspend() directly leave the PM runtime
> state desynchronized?
> 
> This manually gates the clock if the device is active, but it fails to update
> the device's PM runtime status. The PM core still considers the device to be
> RPM_ACTIVE.
> 
> If a kernel thread calls pm_runtime_get_sync() before late suspend, it will
> see the state as active, return immediately without re-enabling the clock,
> and crash when accessing registers.
> 
> Should this use pm_runtime_force_suspend() and pm_runtime_force_resume() to
> safely maintain state consistency?

Calling the driver's runtime suspend/resume callbacks directly from the
system sleep callbacks does not update the PM core state, so the runtime
PM status can become inconsistent with the actual clock state.

I will switch the system sleep callbacks to pm_runtime_force_suspend() and
pm_runtime_force_resume() in the next revision.

eic7700_pvt_pm_ops will be changed along these lines:

-   SYSTEM_SLEEP_PM_OPS(eic7700_pvt_suspend, eic7700_pvt_resume)
+   SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)

Best regards,
Huan He

Re: [PATCH v4 0/2] Add driver support for ESWIN EIC7700 PVT controller

[Guenter Roeck]

On 4/29/26 23:41, hehuan1@eswincomputing.com wrote:
> From: Huan He <hehuan1@eswincomputing.com>
> 
> Add support for the ESWIN EIC7700 PVT (Voltage, Temperature) sensor
> 
> Features:
> The driver supports monitoring of voltage and temperature parameters
> through the hardware monitoring subsystem. It provides an access to the
> sampled Temperature and Voltage.
> 

Please either address the issues reported by Sashiko, or explain why they are invalid.

Thanks,
Guenter

> Test:
> Tested this patch on the SiFive HiFive Premier P550 (which uses the ESWIN
> EIC7700 SoC).
> 
> Updates:
> 
>    Changes in v4:
>    - Update eswin,eic7700-pvt.yaml
>      - Delete reviewed-by tag of Conor Dooley, because the label enum
>        constraint is introduced
>    - Update eic7700-pvt.c and eic7700-pvt.h
>      - Remove the unused LVT/ULVT/SVT process-monitoring channels
>      - Remove the probe-time power check since the PVT block is always
>        powered on EIC7700 and the extra verification is unnecessary
>      - Stop requesting the interrupt as shared and use the dedicated PVT IRQ
>        only
>      - Reorder probe initialization so the interface is initialized before
>        the clock is disabled, avoiding register accesses with the clock gated
>      - Fix runtime PM reference handling on error paths by balancing
>        pm_runtime_get_noresume() with pm_runtime_put_noidle()
>      - Add pm_runtime_put_noidle() handling for failed pm_runtime_get_sync()
>        calls in hwmon read/write paths
>      - Switch the PM callback registration from pm_sleep_ptr() to pm_ptr()
> 
>    Changes in v3:
>    - Update eswin,eic7700-pvt.yaml
>      - Remove redundant label property description and use 'label: true' to
>        reference the definition in hwmon-common.yaml
>      - Replace 'additionalProperties: false' with
>        'unevaluatedProperties: false'
>      - Remove the description for '#thermal-sensor-cells'
>    - Update eic7700-pvt.c and eic7700-pvt.h
>      - Fix clock reference count imbalance with Runtime PM:
>        Replace devm_clk_get_enabled() with devm_clk_get() and manually
>        manage clock enable/disable to avoid double-disable in remove() when
>        Runtime PM is active. Clock is now enabled only during probe for
>        eic7700_pvt_check_pwr(), then disabled before enabling Runtime PM,
>        which takes full control of the clock thereafter
>      - Add detailed comment explaining the spurious interrupt risk in
>        eic7700_pvt_check_pwr()
>      - Replace wait_for_completion_interruptible() with
>        wait_for_completion_timeout() to prevent infinite wait
> 
>    Changes in v2:
>    - Update eswin,eic7700-pvt.yaml
>      - Reference the hwmon-common.yaml file
>      - Remove the clock-names and reset-names properties
>      - Move additionalProperties: false after the required block
>      - Remove one example node to avoid redundancy
>    - Update eic7700-pvt.c and eic7700-pvt.h
>      - Remove unused sensor macros (PVT_SENSOR_FIRST, PVT_SENSOR_LAST,
>        PVT_SENSORS_NUM)
>      - Drop the unnecessary hwmon-sysfs.h header
>      - Replace dynamic sensor info allocation with a static array and unify
>        sensor labels
>      - Remove unused hwmon_temp_type attribute
>      - Eliminate redundant validation checks
>      - Remove mutex and related locking, relying on hwmon core
>        serialization
>      - Replace per-sensor caches and completions with a single data cache
>        and completion object
>      - Remove pvt->sensor tracking. ISR no longer depends on the currently
>        selected sensor
>      - Move devm_add_action() registration after init_completion() for
>        safer cleanup, and update cleanup function (pvt_clear_data)
>      - Replace devm_reset_control_get_optional_exclusive() with
>        devm_reset_control_get_exclusive_deasserted()
>      - Replace eic7700_pvt_remove() with eic7700_pvt_disable_pm_runtime()
>        and move it after PM runtime enable to avoid resource leaks on probe
>        failure and remove clock disable and reset assert from
>        eic7700_pvt_disable_pm_runtime() as it is already handled by devm_*
>        framework
>      - Remove redundant clock presence check in runtime_resume
> 
>    - Link to v1: https://lore.kernel.org/all/20260109090718.442-1-hehuan1@eswincomputing.com/
> 
> Huan He (2):
>    dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
>    hwmon: Add Eswin EIC7700 PVT sensor driver
> 
>   .../bindings/hwmon/eswin,eic7700-pvt.yaml     |  68 ++
>   drivers/hwmon/Kconfig                         |  12 +
>   drivers/hwmon/Makefile                        |   1 +
>   drivers/hwmon/eic7700-pvt.c                   | 591 ++++++++++++++++++
>   drivers/hwmon/eic7700-pvt.h                   |  99 +++
>   5 files changed, 771 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
>   create mode 100644 drivers/hwmon/eic7700-pvt.c
>   create mode 100644 drivers/hwmon/eic7700-pvt.h
>