Add driver support for ESWIN EIC7700 PVT controller
Subsystems (2)
v2 cover Add driver support for ESWIN EIC7700 PVT controller
From: Huan He <[email protected]>
Add support for the ESWIN EIC7700 PVT (Process, Voltage, Temperature) sensor
Features:
The driver supports monitoring of process, 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 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/[email protected]/
Huan He (2):
dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
hwmon: Add Eswin EIC7700 PVT sensor driver
.../bindings/hwmon/eswin,eic7700-pvt.yaml | 70 ++ drivers/hwmon/Kconfig | 12 + drivers/hwmon/Makefile | 1 + drivers/hwmon/eic7700-pvt.c | 612 ++++++++++++++++++ drivers/hwmon/eic7700-pvt.h | 106 +++ 5 files changed, 801 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
Signature · 1 line
2.25.1
PATCH v2 1/2 dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
From: Huan He <[email protected]>
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.
--- .../bindings/hwmon/eswin,eic7700-pvt.yaml | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +70 -0
@@ -0,0 +1,70 @@ +# 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 <[email protected]> + - Huan He <[email protected]> + +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: + description: + Human readable identifier used to distinguish between different PVT + instances. Typically "pvt0" for SoC PVT sensor and "pvt1" for DDR + core PVT sensor. + + resets: + maxItems: 1 + + '#thermal-sensor-cells': + description: Thermal sensor cells if used for thermal sensoring. + const: 0 + +required: + - compatible + - reg + - clocks + - interrupts + - label + - resets + +additionalProperties: 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>; + }; +...
Signature · 1 line
2.25.1
Re: [PATCH v2 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
Quoted 31 lines · On Wed, Jan 28, 2026 at 06:16:36PM +0800, [email protected] wrote:
From: Huan He <[email protected]>
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 <[email protected]>Signed-off-by Huan He <[email protected]>--- .../bindings/hwmon/eswin,eic7700-pvt.yaml | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yamlDocumentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +10
-0@@ -0,0 +1,70 @@ +# 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 <[email protected]>
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml line 20
Quoted 10 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +10
-0+ - Huan He <[email protected]> + +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#
FYI, including this is kinda pointless because you have the label property defined below and your "additionalProperties: false" blocks shunt-resistor-micro-ohms from being used.
Quoted 16 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +16
-0+ +properties: + compatible: + const: eswin,eic7700-pvt + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + interrupts: + maxItems: 1 + + label: + description:
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml line 45
Quoted 10 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +9
-0+ Human readable identifier used to distinguish between different PVT + instances. Typically "pvt0" for SoC PVT sensor and "pvt1" for DDR + core PVT sensor. + + resets: + maxItems: 1 + + '#thermal-sensor-cells': + description: Thermal sensor cells if used for thermal sensoring.
You can drop this description if there's a resend, common properties used in the obvious way don't need any more info.
pw-bot: not-applicable
Cheers,
Conor.
Quoted 28 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +25
-0+ const: 0 + +required: + - compatible + - reg + - clocks + - interrupts + - label + - resets + +additionalProperties: 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>; + }; +...Signature · 1 line
2.25.1
Re: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
Quoted 36 lines
Quoted 36 lines
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 <[email protected]>Signed-off-by Huan He <[email protected]>--- .../bindings/hwmon/eswin,eic7700-pvt.yaml | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yamlDocumentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +16
-0@@ -0,0 +1,70 @@ +# 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 <[email protected]> + - Huan He <[email protected]> + +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
Quoted 8 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml line 20
Quoted 4 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +4
-0+ PVT1 sensor monitors the DDR core power domain. + +allOf: + - $ref: /schemas/hwmon/hwmon-common.yaml#FYI, including this is kinda pointless because you have the label property defined below and your "additionalProperties: false" blocks shunt-resistor-micro-ohms from being used.
I plan to keep the $ref: /schemas/hwmon/hwmon-common.yaml in use, change the original additionalProperties: false to unevaluatedProperties: false, and remove the label definition from our schema since it is already provided by hwmon-common.yaml. Could you please confirm if this modification conforms to the community standards?
Quoted 26 lines
Quoted 25 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +24
-0+ +properties: + compatible: + const: eswin,eic7700-pvt + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + interrupts: + maxItems: 1 + + label: + description: + Human readable identifier used to distinguish between different PVT + instances. Typically "pvt0" for SoC PVT sensor and "pvt1" for DDR + core PVT sensor. + + resets: + maxItems: 1 + + '#thermal-sensor-cells':
Quoted 4 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml line 45
Quoted 1 line
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +1
-0+ description: Thermal sensor cells if used for thermal sensoring.You can drop this description if there's a resend, common properties used in the obvious way don't need any more info.
We will remove this in the next patch.
Thank you very much for taking the time to review the patch and for your valuable feedback.
Best regards,
Huan He
Quoted 35 lines
Reviewed-by Conor Dooley <[email protected]>pw-bot: not-applicable
Cheers,
Conor.Quoted 28 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +25
-0+ const: 0 + +required: + - compatible + - reg + - clocks + - interrupts + - label + - resets + +additionalProperties: 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>; + }; +...Signature · 1 line
2.25.1
Re: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
Quoted 45 lines · Huan He
Quoted 36 lines
Quoted 36 lines
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 <[email protected]>Signed-off-by Huan He <[email protected]>--- .../bindings/hwmon/eswin,eic7700-pvt.yaml | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yamlDocumentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +16
-0@@ -0,0 +1,70 @@ +# 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 <[email protected]> + - Huan He <[email protected]> + +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. TheQuoted 8 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml line 20
Quoted 4 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +4
-0+ PVT1 sensor monitors the DDR core power domain. + +allOf: + - $ref: /schemas/hwmon/hwmon-common.yaml#FYI, including this is kinda pointless because you have the label property defined below and your "additionalProperties: false" blocks shunt-resistor-micro-ohms from being used.
Quoted 5 lines
I plan to keep the $ref: /schemas/hwmon/hwmon-common.yaml in use, change the original additionalProperties: false to unevaluatedProperties: false, and remove the label definition from our schema since it is already provided by
hwmon-common.yaml. Could you please confirm if this modification conforms to the community standards?
That's fine. Does the shunt resistor property apply on your platform?
Re: Re: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
Quoted 45 lines
Quoted 45 lines
Quoted 44 lines
Quoted 30 lines
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 <[email protected]>Signed-off-by Huan He <[email protected]>--- .../bindings/hwmon/eswin,eic7700-pvt.yaml | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yamlDocumentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +10
-0@@ -0,0 +1,70 @@ +# 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 <[email protected]>Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml line 20
Quoted 10 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +10
-0+ - Huan He <[email protected]> + +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#FYI, including this is kinda pointless because you have the label property defined below and your "additionalProperties: false" blocks shunt-resistor-micro-ohms from being used.
Quoted 7 lines
Quoted 5 lines
I plan to keep the $ref: /schemas/hwmon/hwmon-common.yaml in use, change the original additionalProperties: false to unevaluatedProperties: false, and remove the label definition from our schema since it is already provided by
hwmon-common.yaml. Could you please confirm if this modification conforms to the community standards?That's fine. Does the shunt resistor property apply on your platform?
The shunt-resistor-micro-ohms property does not apply to our platform. We only use the label property from hwmon-common.yaml.
Best regards,
Huan He
Re: Re: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
Quoted 51 lines · Huan He
Quoted 51 lines
Quoted 50 lines
Quoted 36 lines
Quoted 36 lines
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 <[email protected]>Signed-off-by Huan He <[email protected]>--- .../bindings/hwmon/eswin,eic7700-pvt.yaml | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yamlDocumentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +16
-0@@ -0,0 +1,70 @@ +# 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 <[email protected]> + - Huan He <[email protected]> + +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. TheQuoted 8 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml line 20
Quoted 4 lines
Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml new file +4
-0+ PVT1 sensor monitors the DDR core power domain. + +allOf: + - $ref: /schemas/hwmon/hwmon-common.yaml#FYI, including this is kinda pointless because you have the label property defined below and your "additionalProperties: false" blocks shunt-resistor-micro-ohms from being used.
I plan to keep the $ref: /schemas/hwmon/hwmon-common.yaml in use, change the original additionalProperties: false to unevaluatedProperties: false, and remove the label definition from our schema since it is already provided by
hwmon-common.yaml. Could you please confirm if this modification conforms to the community standards?
Quoted 4 lines
Quoted 1 line
That's fine. Does the shunt resistor property apply on your platform?
The shunt-resistor-micro-ohms property does not apply to our platform. We only use the label property from
hwmon-common.yaml.
You could just remove
label:
description:
Human readable identifier used to distinguish between different PVT instances. Typically "pvt0" for SoC PVT sensor and "pvt1" for DDR core PVT sensor.
and do
label: true
or leave it as-is.
I don't mind.
PATCH v2 2/2 hwmon: Add Eswin EIC7700 PVT sensor driver
In reply to [email protected]From: Huan He <[email protected]>
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.
--- drivers/hwmon/Kconfig | 12 + drivers/hwmon/Makefile | 1 + drivers/hwmon/eic7700-pvt.c | 612 ++++++++++++++++++++++++++++++++++++ drivers/hwmon/eic7700-pvt.h | 106 +++++++ 4 files changed, 731 insertions(+) create mode 100644 drivers/hwmon/eic7700-pvt.c create mode 100644 drivers/hwmon/eic7700-pvt.h
drivers/hwmon/Kconfig +12 -0
@@ -2043,6 +2043,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
drivers/hwmon/Makefile +1 -0
@@ -72,6 +72,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
drivers/hwmon/eic7700-pvt.c new file +612 -0
612 lines not on this page · raw message
drivers/hwmon/eic7700-pvt.h new file +106 -0
106 lines not on this page · raw message
Signature · 1 line
2.25.1
Quoted 37 lines
Add support for the ESWIN EIC7700 PVT (Process, Voltage, Temperature) sensor
Features:
The driver supports monitoring of process, 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 v2:
- Update eswin,eic7700-pvt.yaml
- Reference thehwmon-common.yamlfile
- Remove the clock-names and reset-names properties
- Move additionalProperties: false after the required block
- Remove one example node to avoid redundancy
- Updateeic7700-pvt.candeic7700-pvt.h
- Remove unused sensor macros (PVT_SENSOR_FIRST,PVT_SENSOR_LAST,PVT_SENSORS_NUM)
- Drop the unnecessaryhwmon-sysfs.hheader
- Replace dynamic sensor info allocation with a static array and unify sensor labels
- Remove unusedhwmon_temp_typeattribute
- 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
- Removepvt->sensortracking. ISR no longer depends on the currently selected sensor
- Movedevm_add_action()registration afterinit_completion()for safer cleanup, and update cleanup function (pvt_clear_data)
Quoted 9 lines
- Replace
devm_reset_control_get_optional_exclusive()withdevm_reset_control_get_exclusive_deasserted()
- Replaceeic7700_pvt_remove()witheic7700_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 fromeic7700_pvt_disable_pm_runtime()as it is already handled by devm_* framework
- Remove redundant clock presence check inruntime_resume
Hi Krzysztof, Guenter, all,
The v1 patch received reviews and comments from Krzysztof and Guenter. The v2 patch incorporates changes based on those comments and has received feedback from Conor.
I'm not sure if there has been time to review the updated v2 yet. Should I wait for further feedback, or is any action needed from my side?
Best regards,
Huan He
Quoted 2 lines
- Link to v1: https://lore.kernel.org/all/[email protected]/
Quoted 11 lines · Huan He
Quoted 2 lines
- Remove redundant clock presence check in
runtime_resumeHi Krzysztof, Guenter, all,
The v1 patch received reviews and comments from Krzysztof and Guenter. The v2 patch incorporates changes based on those comments and has received feedback from Conor.
I'm not sure if there has been time to review the updated v2 yet. Should I wait for further feedback, or is any action needed from my side?
Huh? You received review on v2 thus such ping suggests that you don't consider that given review as an important one. That would be very sad impression to make...
Best regards,
Krzysztof
Quoted 6 lines
Quoted 6 lines
Hi Krzysztof, Guenter, all,
The v1 patch received reviews and comments from Krzysztof and Guenter. The v2 patch incorporates changes based on those comments and has received feedback from Conor.
Quoted 8 lines
Quoted 2 lines
I'm not sure if there has been time to review the updated v2 yet. Should I wait for further feedback, or is any action needed from my side?
Huh? You received review on v2 thus such ping suggests that you don't consider that given review as an important one. That would be very sad impression to make...
Hi Krzysztof, Guenter, Conor,
Sorry for the confusion - I did not mean to disregard the review.
In v1, I received comments on both the YAML binding and the driver code from you and Guenter, and updated both parts in v2 accordingly.
For v2, Conor provided further feedback on the binding part, which I will address in v3.
My question was mainly about the driver changes in v2. I have not received further comments on that part since posting v2, so I just wanted to check whether there are any remaining concerns regarding the driver code.
Best regards,
Huan He
Guenter Roeck
maintainerQuoted 22 lines · Huan He
Quoted 6 lines
Quoted 6 lines
Hi Krzysztof, Guenter, all,
The v1 patch received reviews and comments from Krzysztof and Guenter. The v2 patch incorporates changes based on those comments and has received feedback from Conor.
Quoted 8 lines
Quoted 2 lines
I'm not sure if there has been time to review the updated v2 yet. Should I wait for further feedback, or is any action needed from my side?
Huh? You received review on v2 thus such ping suggests that you don't consider that given review as an important one. That would be very sad impression to make...
Hi Krzysztof, Guenter, Conor,
Sorry for the confusion - I did not mean to disregard the review.
In v1, I received comments on both the YAML binding and the driver code from you and Guenter, and updated both parts in v2 accordingly.
Quoted 7 lines
For v2, Conor provided further feedback on the binding part, which I will address in v3.
My question was mainly about the driver changes in v2. I have not received further comments on that part since posting v2, so I just wanted to check whether there are any remaining concerns regarding the driver code.
See below.
Guenter
---
# Task
Date: 2026-02-11 21:54:07
Model: gemini-3-pro-preview
Prompts SHA: HEAD
Commits to review:
- 2b148b14e234 ("dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor")
- 6f4d5698f334 ("hwmon: Add Eswin EIC7700 PVT sensor driver")
# Commit 2b148b14e234 ("dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor")
Ref: 2b148b14e2344fc9403c02cc27ff0bec77d1568c
# Analysis of commit 2b148b14e2344fc9403c02cc27ff0bec77d1568c
## Context
This commit adds a new DeviceTree binding documentation for the Eswin EIC7700 PVT sensor. The binding is located at Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml.
## Changes
- Adds Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
## Analysis
The new binding file was checked against standard DeviceTree binding conventions and the hwmon subsystem requirements.
### Correctness Checks
- **Schema Validation**: The YAML syntax is valid. $id and $schema are correct.
- **Properties**:
- compatible: eswin,eic7700-pvt uses a documented vendor prefix (eswin).
- reg, clocks, resets, interrupts: All defined with maxItems: 1 or standard constraints.
- label: Defined as a required property to distinguish between two instances (SoC vs DDR). This aligns with hwmon-common.yaml and is justified by the hardware description.
- #thermal-sensor-cells: Defined as const: 0, appropriate for a sensor provider.
- **Example**: The example node pvt@50b00000 is syntactically correct and matches the schema. The node name pvt is consistent with other similar bindings (e.g., baikal,bt1-pvt.yaml).
- **Maintainers**: Listed maintainers match the Signed-off-by lines in the commit.
### Regressions
No regressions were found. The binding file is self-contained and follows established patterns.
## False Positives Eliminated
- **Required label property**: Initially flagged as potentially non-standard for a required property, but verification with hwmon-common.yaml and the specific need to distinguish identical hardware blocks for different domains (SoC vs DDR) confirmed it is valid usage.
- **Node name pvt**: Verified against baikal,bt1-pvt.yaml to confirm pvt is an accepted generic node name for PVT sensors.
FINAL REGRESSIONS FOUND: 0
# Commit 6f4d5698f334 ("hwmon: Add Eswin EIC7700 PVT sensor driver")
1. eic7700-pvt.c:487: ERROR: Unbalanced clock refcount with Runtime PM
Review
Quoted 1 line
(quoted diff) +1
-0+ pvt->clk = devm_clk_get_enabled(&pdev->dev, NULL);
Using devm_clk_get_enabled() enables the clock and registers a devm action to disable it on removal. However, the driver also uses Runtime PM to manage the same clock:
> +static int __maybe_unused eic7700_pvt_runtime_suspend(struct device *dev) > +{
> + struct pvt_hwmon *pvt = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(pvt->clk);
If the device is runtime suspended when remove() is called:
1. probe(): clk_prepare_enable() (Ref: 1)
2. runtime_suspend(): clk_disable_unprepare() (Ref: 0)
3. remove() (via devm): clk_disable_unprepare() (Ref: -1)
This leads to a refcount underflow and warning.
Fix: Use devm_clk_get() and manually call clk_prepare_enable() in probe. Ensure that the manual enable is balanced correctly, or rely on Runtime PM (and pm_runtime_get_sync in probe) to handle the clock, ensuring pm_runtime_put balances it. Since check_pwr needs the clock before Runtime PM is enabled, you should enable it manually and then possibly disable it before enabling Runtime PM, or keep it enabled and let Runtime PM take over (but ensure remove doesn't double disable).
A common pattern:
```c
pvt->clk = devm_clk_get(dev, NULL);
...
clk_prepare_enable(pvt->clk);
...
pm_runtime_enable(dev);
pm_runtime_get_noresume(dev);
...
pm_runtime_put(dev);
```
And ensure remove (or devm action) disables it ONLY if not suspended? Actually, simpler is:
Don't use devm_clk_get_enabled if you use runtime_suspend to disable it. Use devm_clk_get.
In probe: clk_prepare_enable.
Register a devm_add_action that calls clk_disable_unprepare *only if* the driver is not using Runtime PM to control it? No, that's messy.
Better fix:
Use devm_clk_get.
In probe: clk_prepare_enable.
In remove (devm action?): clk_disable_unprepare (but this still has the issue).
Correct Fix:
Do not use clk_disable_unprepare in runtime_suspend if you used devm_clk_get_enabled.
OR
Don't use devm_clk_get_enabled. Use devm_clk_get. Manage the clock entirely via Runtime PM.
In probe:
clk_prepare_enable(pvt->clk); (Temporary for check_pwr) check_pwr...
clk_disable_unprepare(pvt->clk); pm_runtime_enable(dev);
...
2. eic7700-pvt.c:154: ERROR: Swallowing signal interruption
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (ret && (ret != -ERESTARTSYS)) + return ret;
If `wait_for_completion_interruptible()` returns `-ERESTARTSYS` (which is
non-zero), the condition `(ret != -ERESTARTSYS)` is false, so the if-statement
is skipped. The function then proceeds to return 0 (success) with potentially
uninitialized/stale data.
Fix:
```c
if (ret)
return ret;
```3. eic7700-pvt.c:368: WARN: Spurious interrupts on shared IRQ line
Review
Quoted 1 line
(quoted diff) +1
-0+ eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, PVT_ENA_EN);
check_pwr enables the device (and thus potential interrupts) before request_irq is called. If the IRQ line is shared and the device asserts an interrupt immediately, the interrupt will be unhandled (spurious) because no handler is registered yet.
Fix: Request the IRQ before enabling the device, or ensure interrupts are masked at the controller level (if possible) before enabling the block. Since check_pwr relies on polling and ISR clears the status, moving request_irq is tricky. Verify if PVT_ENA has a separate interrupt enable bit or if PVT_INT has a mask. If not, this is a hardware/driver design risk.
4. eic7700-pvt.c:147: NOTE: Infinite wait possibility
Review
Quoted 1 line
(quoted diff) +1
-0+ ret = wait_for_completion_interruptible(&pvt->conversion);
If the hardware fails to generate an interrupt (e.g., hangs), this will wait indefinitely (unless a signal is sent). It is generally safer to use wait_for_completion_timeout in hardware drivers.
# Summary
| Commit | Regressions |
| :---------------------------------------------------------------- | :---------- |
| 2b148b14e234 ("dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor") | 0 |
| 6f4d5698f334 ("hwmon: Add Eswin EIC7700 PVT sensor driver") | 4 |Hi Guenter,
Thank you very much for taking the time to review this patch and for providing such detailed feedback.
Quoted 54 lines
# Commit
6f4d5698f334("hwmon: Add Eswin EIC7700 PVT sensor driver")
1. eic7700-pvt.c:487: ERROR: Unbalanced clock refcount with Runtime PMReview
Quoted 1 line
(quoted diff) +1
-0+ pvt->clk = devm_clk_get_enabled(&pdev->dev, NULL);Using
devm_clk_get_enabled()enables the clock and registers a devm action to disable it on removal. However, the driver also uses Runtime PM to manage the same clock:> +static int
__maybe_unusedeic7700_pvt_runtime_suspend(struct device *dev)> +{
> + structpvt_hwmon*pvt =dev_get_drvdata(dev);
> +
> +clk_disable_unprepare(pvt->clk);If the device is runtime suspended when
remove()is called:
1.probe():clk_prepare_enable()(Ref: 1)
2.runtime_suspend():clk_disable_unprepare()(Ref: 0)
3.remove()(via devm):clk_disable_unprepare()(Ref: -1)This leads to a refcount underflow and warning.
Fix: Use
devm_clk_get()and manually callclk_prepare_enable()in probe. Ensure that the manual enable is balanced correctly, or rely on Runtime PM (andpm_runtime_get_syncin probe) to handle the clock, ensuringpm_runtime_putbalances it. Sincecheck_pwrneeds the clock before Runtime PM is enabled, you should enable it manually and then possibly disable it before enabling Runtime PM, or keep it enabled and let Runtime PM take over (but ensureremovedoesn't double disable).A common pattern:
```c
pvt->clk=devm_clk_get(dev, NULL);
...
clk_prepare_enable(pvt->clk);
...
pm_runtime_enable(dev);
pm_runtime_get_noresume(dev);
...
pm_runtime_put(dev);
```
And ensureremove(or devm action) disables it ONLY if not suspended? Actually, simpler is:
Don't usedevm_clk_get_enabledif you useruntime_suspendto disable it. Usedevm_clk_get.
In probe:clk_prepare_enable.
Register adevm_add_actionthat callsclk_disable_unprepare*only if* the driver is not using Runtime PM to control it? No, that's messy.Better fix:
Usedevm_clk_get.
In probe:clk_prepare_enable.
In remove (devm action?):clk_disable_unprepare(but this still has the issue).
Quoted 12 lines
Correct Fix:
Do not useclk_disable_unprepareinruntime_suspendif you useddevm_clk_get_enabled.
OR
Don't usedevm_clk_get_enabled. Usedevm_clk_get. Manage the clock entirely via Runtime PM.
In probe:
clk_prepare_enable(pvt->clk);(Temporary forcheck_pwr)check_pwr...
clk_disable_unprepare(pvt->clk);pm_runtime_enable(dev);
...
We will reproduce and further analyze the clock refcount imbalance scenario.
Quoted 15 lines
2. eic7700-pvt.c:154: ERROR: Swallowing signal interruption
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (ret && (ret != -ERESTARTSYS)) + return ret;If `wait_for_completion_interruptible()` returns `-ERESTARTSYS` (which is non-zero), the condition `(ret != -ERESTARTSYS)` is false, so the if-statement is skipped. The function then proceeds to return 0 (success) with potentially uninitialized/stale data. Fix: ```c if (ret) return ret; ```
In v3, we will return all non-zero error codes directly, without special-casing -ERESTARTSYS.
Quoted 9 lines
3. eic7700-pvt.c:368: WARN: Spurious interrupts on shared IRQ line
Review
Quoted 1 line
(quoted diff) +1
-0+ eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, PVT_ENA_EN);
check_pwrenables the device (and thus potential interrupts) beforerequest_irqis called. If the IRQ line is shared and the device asserts an interrupt immediately, the interrupt will be unhandled (spurious) because no handler is registered yet.
Quoted 6 lines
Fix: Request the IRQ before enabling the device, or ensure interrupts are masked at the controller level (if possible) before enabling the block. Since
check_pwrrelies on polling and ISR clears the status, movingrequest_irqis tricky. Verify ifPVT_ENAhas a separate interrupt enable bit or ifPVT_INThas a mask. If not, this is a hardware/driver design risk.
Confirmed with the hardware team, the PVT_ENA register has no independent interrupt enable, and PVT_INT does not support masking. Enabling the device before request_irq may generate interrupts, but the driver disables the PVT module (PVT_ENA_EN = 0) and clears interrupts by writing PVT_INT_CLR. In practice, no issues have been observed.
Quoted 7 lines
4. eic7700-pvt.c:147: NOTE: Infinite wait possibility
Review
Quoted 1 line
(quoted diff) +1
-0+ ret = wait_for_completion_interruptible(&pvt->conversion);If the hardware fails to generate an interrupt (e.g., hangs), this will wait indefinitely (unless a signal is sent). It is generally safer to use
wait_for_completion_timeoutin hardware drivers.
In v3, we will replace wait_for_completion_interruptible() with wait_for_completion_timeout().
Best regards,
Huan He
Hi Guenter,
Thank you very much for your detailed review and valuable feedback. I apologize for the delayed response.
Quoted 67 lines
Quoted 54 lines
# Commit
6f4d5698f334("hwmon: Add Eswin EIC7700 PVT sensor driver")
1. eic7700-pvt.c:487: ERROR: Unbalanced clock refcount with Runtime PMReview
Quoted 1 line
(quoted diff) +1
-0+ pvt->clk = devm_clk_get_enabled(&pdev->dev, NULL);Using
devm_clk_get_enabled()enables the clock and registers a devm action to disable it on removal. However, the driver also uses Runtime PM to manage the same clock:> +static int
__maybe_unusedeic7700_pvt_runtime_suspend(struct device *dev)> +{
> + structpvt_hwmon*pvt =dev_get_drvdata(dev);
> +
> +clk_disable_unprepare(pvt->clk);If the device is runtime suspended when
remove()is called:
1.probe():clk_prepare_enable()(Ref: 1)
2.runtime_suspend():clk_disable_unprepare()(Ref: 0)
3.remove()(via devm):clk_disable_unprepare()(Ref: -1)This leads to a refcount underflow and warning.
Fix: Use
devm_clk_get()and manually callclk_prepare_enable()in probe. Ensure that the manual enable is balanced correctly, or rely on Runtime PM (andpm_runtime_get_syncin probe) to handle the clock, ensuringpm_runtime_putbalances it. Sincecheck_pwrneeds the clock before Runtime PM is enabled, you should enable it manually and then possibly disable it before enabling Runtime PM, or keep it enabled and let Runtime PM take over (but ensureremovedoesn't double disable).A common pattern:
```c
pvt->clk=devm_clk_get(dev, NULL);
...
clk_prepare_enable(pvt->clk);
...
pm_runtime_enable(dev);
pm_runtime_get_noresume(dev);
...
pm_runtime_put(dev);
```
And ensureremove(or devm action) disables it ONLY if not suspended? Actually, simpler is:
Don't usedevm_clk_get_enabledif you useruntime_suspendto disable it. Usedevm_clk_get.
In probe:clk_prepare_enable.
Register adevm_add_actionthat callsclk_disable_unprepare*only if* the driver is not using Runtime PM to control it? No, that's messy.Better fix:
Usedevm_clk_get.
In probe:clk_prepare_enable.
In remove (devm action?):clk_disable_unprepare(but this still has the issue).Quoted 12 lines
Correct Fix:
Do not useclk_disable_unprepareinruntime_suspendif you useddevm_clk_get_enabled.
OR
Don't usedevm_clk_get_enabled. Usedevm_clk_get. Manage the clock entirely via Runtime PM.
In probe:
clk_prepare_enable(pvt->clk);(Temporary forcheck_pwr)check_pwr...
clk_disable_unprepare(pvt->clk);pm_runtime_enable(dev);
...
Quoted 2 lines
We will reproduce and further analyze the clock refcount imbalance scenario.
Regarding the Runtime PM issue potentially causing clock refcount imbalance, we have investigated it and will address this in the v3 patch.
Quoted 17 lines
Quoted 10 lines
3. eic7700-pvt.c:368: WARN: Spurious interrupts on shared IRQ line
Review
Quoted 1 line
(quoted diff) +1
-0+ eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, PVT_ENA_EN);
check_pwrenables the device (and thus potential interrupts) beforerequest_irqis called. If the IRQ line is shared and the device asserts an interrupt immediately, the interrupt will be unhandled (spurious) because no handler is registered yet.Quoted 6 lines
Fix: Request the IRQ before enabling the device, or ensure interrupts are masked at the controller level (if possible) before enabling the block. Since
check_pwrrelies on polling and ISR clears the status, movingrequest_irqis tricky. Verify ifPVT_ENAhas a separate interrupt enable bit or ifPVT_INThas a mask. If not, this is a hardware/driver design risk.
Quoted 6 lines
Confirmed with the hardware team, the
PVT_ENAregister has no independent interrupt enable, andPVT_INTdoes not support masking. Enabling the device beforerequest_irqmay generate interrupts, but the driver disables the PVT module (PVT_ENA_EN= 0) and clears interrupts by writingPVT_INT_CLR. In practice, no issues have been observed.
For the spurious interrupt concern: after confirming with the hardware team, the PVT_ENA register has no independent interrupt enable, and PVT_INT does not support masking. In the current implementation, enabling the device during check_pwr may generate an interrupt, but the driver subsequently disables the PVT module (PVT_ENA_EN = 0) and clears the interrupt status by writing PVT_INT_CLR. In practice, no issues have been observed.
Could you please confirm whether it is acceptable to keep the current implementation under these conditions?
Best regards,
Huan He
Guenter Roeck
maintainerQuoted 110 lines · Huan He
Hi Guenter,
Thank you very much for your detailed review and valuable feedback. I apologize for the delayed response.
Quoted 67 lines
Quoted 54 lines
# Commit
6f4d5698f334("hwmon: Add Eswin EIC7700 PVT sensor driver")
1. eic7700-pvt.c:487: ERROR: Unbalanced clock refcount with Runtime PMReview
Quoted 1 line
(quoted diff) +1
-0+ pvt->clk = devm_clk_get_enabled(&pdev->dev, NULL);Using
devm_clk_get_enabled()enables the clock and registers a devm action to disable it on removal. However, the driver also uses Runtime PM to manage the same clock:> +static int
__maybe_unusedeic7700_pvt_runtime_suspend(struct device *dev)> +{
> + structpvt_hwmon*pvt =dev_get_drvdata(dev);
> +
> +clk_disable_unprepare(pvt->clk);If the device is runtime suspended when
remove()is called:
1.probe():clk_prepare_enable()(Ref: 1)
2.runtime_suspend():clk_disable_unprepare()(Ref: 0)
3.remove()(via devm):clk_disable_unprepare()(Ref: -1)This leads to a refcount underflow and warning.
Fix: Use
devm_clk_get()and manually callclk_prepare_enable()in probe. Ensure that the manual enable is balanced correctly, or rely on Runtime PM (andpm_runtime_get_syncin probe) to handle the clock, ensuringpm_runtime_putbalances it. Sincecheck_pwrneeds the clock before Runtime PM is enabled, you should enable it manually and then possibly disable it before enabling Runtime PM, or keep it enabled and let Runtime PM take over (but ensureremovedoesn't double disable).A common pattern:
```c
pvt->clk=devm_clk_get(dev, NULL);
...
clk_prepare_enable(pvt->clk);
...
pm_runtime_enable(dev);
pm_runtime_get_noresume(dev);
...
pm_runtime_put(dev);
```
And ensureremove(or devm action) disables it ONLY if not suspended? Actually, simpler is:
Don't usedevm_clk_get_enabledif you useruntime_suspendto disable it. Usedevm_clk_get.
In probe:clk_prepare_enable.
Register adevm_add_actionthat callsclk_disable_unprepare*only if* the driver is not using Runtime PM to control it? No, that's messy.Better fix:
Usedevm_clk_get.
In probe:clk_prepare_enable.
In remove (devm action?):clk_disable_unprepare(but this still has the issue).Quoted 12 lines
Correct Fix:
Do not useclk_disable_unprepareinruntime_suspendif you useddevm_clk_get_enabled.
OR
Don't usedevm_clk_get_enabled. Usedevm_clk_get. Manage the clock entirely via Runtime PM.
In probe:
clk_prepare_enable(pvt->clk);(Temporary forcheck_pwr)check_pwr...
clk_disable_unprepare(pvt->clk);pm_runtime_enable(dev);
...Quoted 2 lines
We will reproduce and further analyze the clock refcount imbalance scenario.
Regarding the Runtime PM issue potentially causing clock refcount imbalance, we have investigated it and will address this in the v3 patch.
Quoted 17 lines
Quoted 10 lines
3. eic7700-pvt.c:368: WARN: Spurious interrupts on shared IRQ line
Review
Quoted 1 line
(quoted diff) +1
-0+ eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, PVT_ENA_EN);
check_pwrenables the device (and thus potential interrupts) beforerequest_irqis called. If the IRQ line is shared and the device asserts an interrupt immediately, the interrupt will be unhandled (spurious) because no handler is registered yet.Quoted 6 lines
Fix: Request the IRQ before enabling the device, or ensure interrupts are masked at the controller level (if possible) before enabling the block. Since
check_pwrrelies on polling and ISR clears the status, movingrequest_irqis tricky. Verify ifPVT_ENAhas a separate interrupt enable bit or ifPVT_INThas a mask. If not, this is a hardware/driver design risk.Quoted 6 lines
Confirmed with the hardware team, the
PVT_ENAregister has no independent interrupt enable, andPVT_INTdoes not support masking. Enabling the device beforerequest_irqmay generate interrupts, but the driver disables the PVT module (PVT_ENA_EN= 0) and clears interrupts by writingPVT_INT_CLR. In practice, no issues have been observed.For the spurious interrupt concern: after confirming with the hardware team, the
PVT_ENAregister has no independent interrupt enable, andPVT_INTdoes not support masking. In the current implementation, enabling the device duringcheck_pwrmay generate an interrupt, but the driver subsequently disables the PVT module (PVT_ENA_EN= 0) and clears the interrupt status by writingPVT_INT_CLR. In practice, no issues have been observed.
Quoted 2 lines
Could you please confirm whether it is acceptable to keep the current implementation under these conditions?
Yes, but please explain in the code.
Thanks,
Guenter