* Re: [PATCH v1 1/3] drivers/platform: lenovo-t14s-ec: Add hwmon support for temperatures and fan speed
From: Konrad Dybcio @ 2026-06-25 8:03 UTC (permalink / raw)
To: Daniel Lezcano, sre, hansg, ilpo.jarvinen, linux, andersson,
konradybcio, robh, krzk+dt, conor+dt
Cc: bryan.odonoghue, platform-driver-x86, linux-kernel, linux-hwmon,
linux-arm-msm, devicetree
In-Reply-To: <20260624210825.264454-2-daniel.lezcano@oss.qualcomm.com>
On 6/24/26 11:08 PM, Daniel Lezcano wrote:
> Expose the Lenovo ThinkPad T14s EC environmental sensors through
> the hwmon subsystem.
>
> The driver now registers a hwmon device providing access to six EC
> temperature sensors corresponding to the SoC, keyboard area, base
> cover, PMIC/charging circuitry, QTM module and SSD. Sensor labels
> are exported to allow user space to identify each measurement.
>
> Additionally, expose the system fan speed by reading the fan RPM
> registers from the embedded controller.
>
> This allows standard monitoring tools such as lm-sensors to report
> platform temperatures and fan speed.
>
> Signed-off-by: Daniel Lezcano daniel.lezcano@oss.qualcomm.com
> ---
[...]
> + case hwmon_fan:
> + if (attr == hwmon_fan_input) {
> + int lsb, msb;
> + ret = t14s_ec_read(ec, T14S_EC_FAN_RPM_LSB, &lsb);
> + if (ret)
> + return ret;
> +
> + ret = t14s_ec_read(ec, T14S_EC_FAN_RPM_MSB, &msb);
> + if (ret)
> + return ret;
> +
> + *val = 0;
> + *val = lsb + (msb << 8);
'+' looks funny here.. although t14s_ec_read() only reads a
single byte and assigns a u8 value to the u32 that's being passed
to it, so it never *actually* breaks..
[...]
> +static const struct hwmon_channel_info *t14s_ec_hwmon_info[] = {
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL),
> + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
> + NULL
> +};
> +
> +static const struct hwmon_chip_info t14s_ec_chip_info = {
> + .ops = &t14s_ec_hwmon_ops,
> + .info = t14s_ec_hwmon_info,
> +};
> +
> +static int t14s_ec_hwmon_probe(struct t14s_ec *ec)
> +{
> + struct device *dev;
> + struct t14s_ec_hwmon_sys_thermx sys_thermx[] = {
> + { T14S_EC_SYS_THERM0, "soc" },
> + { T14S_EC_SYS_THERM1, "keyboard" },
> + { T14S_EC_SYS_THERM2, "base" },
> + { T14S_EC_SYS_THERM3, "pmbm" },
> + { T14S_EC_SYS_THERM6, "qtm" },
> + { T14S_EC_SYS_THERM7, "ssd" },
Makes one wonder what happened to THERM4/5 - may they be dedicated to
the 5G modem, perhaps?
Konrad
^ permalink raw reply
* Re: [PATCH 1/7] dt-bindings: adm1275: ROHM BD12780 hot-swap controller
From: Krzysztof Kozlowski @ 2026-06-25 7:21 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Matti Vaittinen, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Shuah Khan,
Wensheng Wang, Ashish Yadav, Kim Seer Paller, Cedric Encarnacion,
Chris Packham, Yuxi Wang, Charles Hsu, ChiShih Tsai, linux-hwmon,
devicetree, linux-kernel, linux-doc
In-Reply-To: <bd9419aa-1a21-4ca2-990b-ad1bebf5c9c8@gmail.com>
On 25/06/2026 09:05, Matti Vaittinen wrote:
>>> + - adi,adm1075
>>> + - adi,adm1272
>>> + - adi,adm1273
>>> + - adi,adm1275
>>> + - adi,adm1276
>>> + - adi,adm1278
>>> + - adi,adm1281
>>> + - adi,adm1293
>>> + - adi,adm1294
>>> + - rohm,bd12780
>>> + - silergy,mc09c
>>> +
>>> +# Require BD12780 as a fall-back for BD12780A.
>>
>> No need for the comment, schema is quite explicit.
>
> Eh... I know it is explicit for one who fluently reads yaml. Not all of
> us do that :| (See my reply to the previous comment...) I am not sure
> the comment hurts - while I am sure it helps occasional binding reader
> like me. Can you please reconsider keeping the comment?
This one does not, but if people take the existing code as a starting
point or even as an example in arguments ("he did like that, so I am
allowed as well"), it gets multiplied and we have more bindings with
redundant data.
That's said, if you insist then fine with me, keep it.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/7] dt-bindings: adm1275: ROHM BD12780 hot-swap controller
From: Matti Vaittinen @ 2026-06-25 7:05 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Matti Vaittinen, Matti Vaittinen, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Shuah Khan,
Wensheng Wang, Ashish Yadav, Kim Seer Paller, Cedric Encarnacion,
Chris Packham, Yuxi Wang, Charles Hsu, ChiShih Tsai, linux-hwmon,
devicetree, linux-kernel, linux-doc
In-Reply-To: <20260617-uptight-sexy-hippo-f4bc62@quoll>
I think I (almost) missed this review... Sorry for the belated reply.
On 17/06/2026 13:28, Krzysztof Kozlowski wrote:
> On Tue, Jun 16, 2026 at 09:35:35AM +0300, Matti Vaittinen wrote:
> +
>> + Datasheets:
>> + https://fscdn.rohm.com/en/products/databook/datasheet/ic/power/power_switch/bd12780muv-lb-e.pdf
>> + https://fscdn.rohm.com/en/products/databook/datasheet/ic/power/power_switch/bd12780amuv-lb-e.pdf
>> +
>> properties:
>> compatible:
>> - enum:
>> - - adi,adm1075
>> - - adi,adm1272
>> - - adi,adm1273
>> - - adi,adm1275
>> - - adi,adm1276
>> - - adi,adm1278
>> - - adi,adm1281
>> - - adi,adm1293
>> - - adi,adm1294
>> - - silergy,mc09c
>> + oneOf:
>> + - items:
>> + enum:
>
>
> s/items/enum/, so:
>
> oneOf:
> - enum:
> ....
Thanks Krzysztof. I am always so lost with these bindings. Giving the
concrete suggestion(s) helps a lot!
>
>> + - adi,adm1075
>> + - adi,adm1272
>> + - adi,adm1273
>> + - adi,adm1275
>> + - adi,adm1276
>> + - adi,adm1278
>> + - adi,adm1281
>> + - adi,adm1293
>> + - adi,adm1294
>> + - rohm,bd12780
>> + - silergy,mc09c
>> +
>> +# Require BD12780 as a fall-back for BD12780A.
>
> No need for the comment, schema is quite explicit.
Eh... I know it is explicit for one who fluently reads yaml. Not all of
us do that :| (See my reply to the previous comment...) I am not sure
the comment hurts - while I am sure it helps occasional binding reader
like me. Can you please reconsider keeping the comment?
Although, I am not sure if Guenter suggested me to drop the compatible
for the bd12780a and only use the bd12780 - or if his comment only
applied to the i2c IDs.
https://lore.kernel.org/all/751cd5eb-104f-4445-a6d2-8119ad5d5660@roeck-us.net/
Well, I will keep the bd12780a compatible and drop the I2C ID unless
something else is suggested. Again, the BD12780 and BD12780A do have
different hardware properties (at least in I2C slave address selection
pins), and while it doesn't really matter for the Linux drivers, the DT
bindings should ideally be generic and not Linux specific.
Yours,
-- Matti.
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply
* Re: [PATCH v8 2/2] hwmon: Add Eswin EIC7700 PVT sensor driver
From: sashiko-bot @ 2026-06-25 6:22 UTC (permalink / raw)
To: hehuan1; +Cc: linux-hwmon, devicetree, robh, conor+dt
In-Reply-To: <20260625061214.1648-1-hehuan1@eswincomputing.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing Hardware State Restoration after System Sleep / Runtime PM
--
commit 90e30b4d9c6082a265da7f7091bf9e2cd31aec81
Author: Huan He <hehuan1@eswincomputing.com>
hwmon: Add Eswin EIC7700 PVT sensor driver
This commit introduces support for the ESWIN EIC7700 Voltage and
Temperature (PVT) sensor. The driver implements temperature and voltage
monitoring using polynomial conversion and provides a sysfs interface
for accessing the sensor data.
> diff --git a/drivers/hwmon/eic7700-pvt.c b/drivers/hwmon/eic7700-pvt.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hwmon/eic7700-pvt.c
[ ... ]
> +static int __maybe_unused eic7700_pvt_runtime_resume(struct device *dev)
> +{
> + struct pvt_hwmon *pvt = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_bulk_prepare_enable(PVT_CLK_NUM, pvt->clks);
> + if (ret) {
> + dev_err(dev, "Failed to enable clocks: %d\n", ret);
> + return ret;
> + }
> +
[Severity: Medium]
Does this code need to restore the hardware trim value after resuming?
The driver initializes the sensor mode and trim configuration via
eic7700_pvt_init_iface() during probe.
If the device enters system suspend or runtime suspend and the power domain
is disabled, the hardware configuration could be lost. Since the driver
utilizes pm_runtime_force_resume() for system suspend paths, should this
callback re-apply the trim values to prevent incorrect sensor readings
upon wake?
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260625061049.1614-1-hehuan1@eswincomputing.com?part=2
^ permalink raw reply
* Re: [PATCH v8 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
From: sashiko-bot @ 2026-06-25 6:17 UTC (permalink / raw)
To: hehuan1; +Cc: conor+dt, devicetree, robh, linux-hwmon
In-Reply-To: <20260625061147.1631-1-hehuan1@eswincomputing.com>
> From: Huan He <hehuan1@eswincomputing.com>
>
> Add device tree binding documentation for ESWIN EIC7700 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>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260625061049.1614-1-hehuan1@eswincomputing.com?part=1
^ permalink raw reply
* [PATCH v8 2/2] hwmon: Add Eswin EIC7700 PVT sensor driver
From: hehuan1 @ 2026-06-25 6:12 UTC (permalink / raw)
To: linux, robh, krzk+dt, conor+dt, p.zabel, linux-hwmon, devicetree,
linux-kernel
Cc: ningyu, linmin, pinkesh.vaghela, luyulin, dongxuyang, Huan He
In-Reply-To: <20260625061049.1614-1-hehuan1@eswincomputing.com>
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 one voltage sensor.
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 | 507 ++++++++++++++++++++++++++++++++++++
drivers/hwmon/eic7700-pvt.h | 99 +++++++
4 files changed, 619 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 5c2d3ff5fce8..6375c5f1136e 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2067,6 +2067,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 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 63effc0ab8d1..e49cfdda970c 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -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_EMC1812) += emc1812.o
obj-$(CONFIG_SENSORS_EMC2103) += emc2103.o
diff --git a/drivers/hwmon/eic7700-pvt.c b/drivers/hwmon/eic7700-pvt.c
new file mode 100644
index 000000000000..f694259258a9
--- /dev/null
+++ b/drivers/hwmon/eic7700-pvt.c
@@ -0,0 +1,507 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ESWIN EIC7700 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),
+};
+
+static const char * const pvt_clk_names[PVT_CLK_NUM] = {"enable", "apb"};
+
+/*
+ * 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 stat, val;
+ int active;
+
+ if (IS_ENABLED(CONFIG_PM)) {
+ active = pm_runtime_get_if_active(pvt->dev);
+ if (active <= 0)
+ return IRQ_NONE;
+ }
+
+ stat = readl(pvt->regs + PVT_INT);
+ if (!(stat & PVT_INT_STAT)) {
+ if (IS_ENABLED(CONFIG_PM))
+ pm_runtime_put(pvt->dev);
+ return IRQ_NONE;
+ }
+
+ 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);
+
+ if (IS_ENABLED(CONFIG_PM))
+ pm_runtime_put(pvt->dev);
+
+ 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);
+
+ if (!ret)
+ synchronize_irq(pvt->irq);
+
+ 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_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;
+ }
+ break;
+ case hwmon_in:
+ switch (attr) {
+ case hwmon_in_input:
+ case hwmon_in_label:
+ return 0444;
+ }
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
+static int eic7700_pvt_hwmon_read(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int ch, long *val)
+{
+ struct pvt_hwmon *pvt = dev_get_drvdata(dev);
+ int ret;
+
+ ret = pm_runtime_get_sync(pvt->dev);
+ if (ret < 0) {
+ dev_err(pvt->dev, "Failed to resume PVT device: %d\n", ret);
+ 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;
+ 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 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
+};
+
+static const struct hwmon_chip_info pvt_hwmon_info = {
+ .ops = &pvt_hwmon_ops,
+ .info = pvt_channel_info
+};
+
+static struct pvt_hwmon *eic7700_pvt_create_data(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct pvt_hwmon *pvt;
+
+ pvt = devm_kzalloc(dev, sizeof(*pvt), GFP_KERNEL);
+ if (!pvt)
+ return ERR_PTR(-ENOMEM);
+
+ pvt->dev = dev;
+ init_completion(&pvt->conversion);
+
+ 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_ENA, PVT_ENA_EN, 0);
+ eic7700_pvt_update(pvt->regs + PVT_INT, PVT_INT_CLR, PVT_INT_CLR);
+ 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)
+{
+ pvt->hwmon = devm_hwmon_device_register_with_info(pvt->dev, "pvt",
+ 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);
+
+ if (!pm_runtime_status_suspended(pvt->dev)) {
+ clk_bulk_disable_unprepare(PVT_CLK_NUM, pvt->clks);
+ pm_runtime_set_suspended(pvt->dev);
+ }
+}
+
+static int eic7700_pvt_probe(struct platform_device *pdev)
+{
+ struct reset_control *rst;
+ struct pvt_hwmon *pvt;
+ int i, 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);
+
+ for (i = 0; i < PVT_CLK_NUM; i++)
+ pvt->clks[i].id = pvt_clk_names[i];
+
+ ret = devm_clk_bulk_get(&pdev->dev, PVT_CLK_NUM, pvt->clks);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "Couldn't get clock descriptors\n");
+
+ rst = devm_reset_control_get_exclusive_deasserted(&pdev->dev, NULL);
+ if (IS_ERR(rst))
+ return dev_err_probe(pvt->dev, PTR_ERR(rst),
+ "Couldn't get reset control\n");
+
+ ret = clk_bulk_prepare_enable(PVT_CLK_NUM, pvt->clks);
+ if (ret)
+ return dev_err_probe(pvt->dev, ret,
+ "Failed to enable clocks\n");
+
+ ret = eic7700_pvt_init_iface(pvt);
+ if (ret) {
+ clk_bulk_disable_unprepare(PVT_CLK_NUM, pvt->clks);
+ return ret;
+ }
+
+ if (IS_ENABLED(CONFIG_PM))
+ clk_bulk_disable_unprepare(PVT_CLK_NUM, pvt->clks);
+
+ 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_request_irq(pvt);
+ if (ret)
+ goto err_put_pm_runtime;
+
+ 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_bulk_prepare_enable(PVT_CLK_NUM, pvt->clks);
+ if (ret) {
+ dev_err(dev, "Failed to enable clocks: %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_bulk_disable_unprepare(PVT_CLK_NUM, pvt->clks);
+
+ return 0;
+}
+
+static const struct dev_pm_ops eic7700_pvt_pm_ops = {
+ SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_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..fb10f9e4e93a
--- /dev/null
+++ b/drivers/hwmon/eic7700-pvt.h
@@ -0,0 +1,99 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ESWIN EIC7700 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
+};
+
+#define PVT_CLK_NUM 2
+
+/*
+ * 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.
+ * @clks: PVT clock descriptors.
+ * @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_bulk_data clks[PVT_CLK_NUM];
+ u32 data_cache;
+ struct completion conversion;
+ ktime_t timeout;
+};
+
+#endif /* __HWMON_EIC7700_PVT_H__ */
--
2.34.1
^ permalink raw reply related
* [PATCH v8 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
From: hehuan1 @ 2026-06-25 6:11 UTC (permalink / raw)
To: linux, robh, krzk+dt, conor+dt, p.zabel, linux-hwmon, devicetree,
linux-kernel
Cc: ningyu, linmin, pinkesh.vaghela, luyulin, dongxuyang, Huan He
In-Reply-To: <20260625061049.1614-1-hehuan1@eswincomputing.com>
From: Huan He <hehuan1@eswincomputing.com>
Add device tree binding documentation for ESWIN EIC7700 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 | 72 +++++++++++++++++++
1 file changed, 72 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..58ec8635dce3
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
@@ -0,0 +1,72 @@
+# 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 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:
+ items:
+ - description: PVT enable clock
+ - description: APB bus clock
+
+ clock-names:
+ items:
+ - const: enable
+ - const: apb
+
+ interrupts:
+ maxItems: 1
+
+ resets:
+ maxItems: 1
+
+ '#thermal-sensor-cells':
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - interrupts
+ - resets
+ - '#thermal-sensor-cells'
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ sensor@50b00000 {
+ compatible = "eswin,eic7700-pvt";
+ reg = <0x50b00000 0x10000>;
+ clocks = <&clocks 244>, <&clocks 234>;
+ clock-names = "enable", "apb";
+ interrupts = <349>;
+ interrupt-parent = <&plic>;
+ label = "pvt0";
+ resets = <&reset 111>;
+ #thermal-sensor-cells = <0>;
+ };
+...
--
2.34.1
^ permalink raw reply related
* [PATCH v8 0/2] Add driver support for ESWIN EIC7700 PVT controller
From: hehuan1 @ 2026-06-25 6:10 UTC (permalink / raw)
To: linux, robh, krzk+dt, conor+dt, p.zabel, linux-hwmon, devicetree,
linux-kernel
Cc: ningyu, linmin, pinkesh.vaghela, luyulin, dongxuyang, Huan He
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 v8:
- Update eswin,eic7700-pvt.yaml
- Delete reviewed-by tag of Krzysztof Kozlowski due to functional
changes. Add the APB clock because when the kernel is booted with
CMDLINE option "clk_ignore_unused", the APB clock remains enabled by
default; without this option, the APB clock may be gated and the PVT
driver will not operate correctly
- Update eic7700-pvt.c and eic7700-pvt.h
- Add APB clock support and retrieve clocks using devm_clk_bulk_get()
- Update eic7700_pvt_hard_isr() to verify PVT_INT_STAT before clearing
the interrupt and completing a conversion, preventing spurious
interrupts from returning stale data or completing a conversion early
- Update eic7700_pvt_probe() to register the PM runtime cleanup action
before requesting the IRQ, so the IRQ is torn down before clocks are
disabled during driver removal, preventing a possible
use-after-disable of the hardware clock in the ISR
Changes in v7:
- Remove the unused reset control pointer from struct pvt_hwmon and keep
the reset control handle local to eic7700_pvt_probe()
- Update eic7700_pvt_init_iface() to disable PVT_ENA_EN before clearing
the interrupt status, preventing a possible level-triggered interrupt
storm if the bootloader leaves the conversion engine running
- Update eic7700_pvt_disable_pm_runtime() to explicitly disable runtime
PM and avoid an unbalanced disable_depth
Changes in v6:
- Fix the !CONFIG_PM probe error path by disabling the clock if IRQ
request fails before the PM cleanup action is registered
- Replace pm_runtime_put_noidle() with pm_runtime_put() in the IRQ
handler to avoid a runtime PM reference-count race with the read path
- Remove the unused pvt_clear_data() devres action and its associated
devm_add_action() registration
Changes in v5:
- Update eswin,eic7700-pvt.yaml
- Drop the label enum constraint and remove label from the required
list
- Add '#thermal-sensor-cells' to the required list
- Rename the example node to the generic sensor@... form
- Update the binding description to describe one temperature sensor
and one voltage sensor
- Update eic7700-pvt.c
- Register the hwmon device with the fixed name "pvt"
- Remove label-based instance identification from the driver
- Fix CONFIG_PM=n support by keeping the clock enabled when runtime PM
is unavailable
- Add pm_runtime_force_suspend() in the cleanup path to avoid leaving
the device active during unbind
- Switch system sleep callbacks to pm_runtime_force_suspend() and
pm_runtime_force_resume()
- Guard ISR register accesses with pm_runtime_get_if_active()
- Add synchronize_irq() on the timeout path to avoid stale completion
races
- Remove temp_offset support because the raw trim register does not
match the hwmon ABI
- Align the commit message with the implementation (one temperature
sensor, one voltage sensor)
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 | 72 +++
drivers/hwmon/Kconfig | 12 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/eic7700-pvt.c | 507 ++++++++++++++++++
drivers/hwmon/eic7700-pvt.h | 99 ++++
5 files changed, 691 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.34.1
^ permalink raw reply
* RE: [PATCH 2/3] dt-bindings: hwmon: pmbus: Support for onsemi's FD5121
From: Selvamani Rajagopal @ 2026-06-25 0:35 UTC (permalink / raw)
To: Guenter Roeck, Conor Dooley
Cc: Jonathan Corbet, Shuah Khan, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-hwmon@vger.kernel.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org
In-Reply-To: <eed3e19b-8cc7-4aef-b058-b2242c94c940@roeck-us.net>
> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Subject: Re: [PATCH 2/3] dt-bindings: hwmon: pmbus: Support for onsemi's FD5121
>
>
> One of the problems here is that the chip datasheet is not public,
Agree on both points. No datasheets on our website. And chips not available through distributors. Little
early in the process. I am working with our product team to see how to move forward. So, there will be some
delay in giving next patch. Thanks for your feedback. I have all the information to send a cleaner, hopefully
acceptable patch,
> so we can not verify what this actually is. The only available
> public document appears to be the "onsemi FD512x Ax Digital Controller
> User Manual" which describes the chip as follows.
>
> "The FD512x Digital Controller is a programmable device designed
> for machine vendors to configure their equipment at the factory."
>
> That really does not explain anything at all, and actually looks like
> an AI generated summary with the AI not understanding what it is talking
> about. According to the onsemi web page, the chip does not exist,
> and it appears that it is not available to buy from any distributors
> either.
>
> Guenter
>
^ permalink raw reply
* Re: [PATCH 2/3] dt-bindings: hwmon: pmbus: Support for onsemi's FD5121
From: Guenter Roeck @ 2026-06-24 23:05 UTC (permalink / raw)
To: Selvamani Rajagopal, Conor Dooley
Cc: Jonathan Corbet, Shuah Khan, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-hwmon@vger.kernel.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org
In-Reply-To: <CYYPR02MB9828EECB3F6AFDD2A7BD3E9B83ED2@CYYPR02MB9828.namprd02.prod.outlook.com>
On 6/24/26 15:36, Selvamani Rajagopal wrote:
>> -----Original Message-----
>> From: Conor Dooley <conor@kernel.org>
>> Subject: Re: [PATCH 2/3] dt-bindings: hwmon: pmbus: Support for onsemi's FD5121
>>
>>
>> My point is that what's actually being controlled is missing. Maybe it
>> is obvious to you, but it is not to me. Your nodename in your example is
>
>
> You are right. This chip may not be a "controller" in the traditional sense as it doesn't control anything.
> We can change node naming to sensor or regulator so that it aligns with the convention.
>
One of the problems here is that the chip datasheet is not public,
so we can not verify what this actually is. The only available
public document appears to be the "onsemi FD512x Ax Digital Controller
User Manual" which describes the chip as follows.
"The FD512x Digital Controller is a programmable device designed
for machine vendors to configure their equipment at the factory."
That really does not explain anything at all, and actually looks like
an AI generated summary with the AI not understanding what it is talking
about. According to the onsemi web page, the chip does not exist,
and it appears that it is not available to buy from any distributors
either.
Guenter
>
>>> + fd5121@50 {
>> which doesn't comply with node naming requirements and I wanted to come
>> up with a suggestion for what it should be.
>> I am assuming that its power or voltage that you're controlling so
>> either it should be hwmon@ or regulator@.
>>
>
^ permalink raw reply
* RE: [PATCH 2/3] dt-bindings: hwmon: pmbus: Support for onsemi's FD5121
From: Selvamani Rajagopal @ 2026-06-24 22:36 UTC (permalink / raw)
To: Conor Dooley
Cc: Guenter Roeck, Jonathan Corbet, Shuah Khan, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-hwmon@vger.kernel.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org
In-Reply-To: <20260623-scared-judgingly-7efc1c188670@spud>
> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Subject: Re: [PATCH 2/3] dt-bindings: hwmon: pmbus: Support for onsemi's FD5121
>
>
> My point is that what's actually being controlled is missing. Maybe it
> is obvious to you, but it is not to me. Your nodename in your example is
You are right. This chip may not be a "controller" in the traditional sense as it doesn't control anything.
We can change node naming to sensor or regulator so that it aligns with the convention.
> > + fd5121@50 {
> which doesn't comply with node naming requirements and I wanted to come
> up with a suggestion for what it should be.
> I am assuming that its power or voltage that you're controlling so
> either it should be hwmon@ or regulator@.
>
^ permalink raw reply
* Re: [PATCH v1 1/3] drivers/platform: lenovo-t14s-ec: Add hwmon support for temperatures and fan speed
From: sashiko-bot @ 2026-06-24 21:22 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: robh, conor+dt, devicetree, linux-hwmon
In-Reply-To: <20260624210825.264454-2-daniel.lezcano@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] Direct calls to `t14s_ec_read` bypass the `regmap` mutex, violating the mandatory 10ms hardware recovery delay between EC commands.
- [Medium] Reading the fan RPM requires two separate I2C transfers for the LSB and MSB, allowing the hardware value to update in between and cause torn reads.
- [Low] Passing `int *` arguments to a function expecting `unsigned int *` for I2C register reads.
Pre-existing issues:
- [High] The `t14s_ec_read` function passes buffers allocated on the kernel stack to `__i2c_transfer()`, which violates DMA mapping rules and can cause panics.
--
commit d0870f0c919ef26da98287c26fc38bc327dcd76b
Author: Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>
drivers/platform: lenovo-t14s-ec: Add hwmon support for temperatures and fan speed
Expose the Lenovo ThinkPad T14s EC environmental sensors through
the hwmon subsystem.
> diff --git a/drivers/platform/arm64/lenovo-thinkpad-t14s.c b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> index 5590302a5694f..142464623f0e8 100644
> --- a/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> +++ b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
[ ... ]
> @@ -555,6 +576,128 @@ static irqreturn_t t14s_ec_irq_handler(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> +static umode_t t14s_ec_hwmon_is_visible(const void *drvdata,
[ ... ]
> +static int t14s_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct t14s_ec *ec = dev_get_drvdata(dev);
> + unsigned int value;
> + int ret;
> +
> + switch (type) {
> + case hwmon_temp:
> + if (attr == hwmon_temp_input) {
> + ret = t14s_ec_read(ec, ec->ec_hwmon.sys_thermx[channel].reg, &value);
[Severity: High]
Does calling t14s_ec_read() directly here bypass the regmap mutex?
Because the embedded controller requires a 10ms hardware recovery delay between
commands, regmap serializes accesses and includes the sleep while holding the
lock. Calling t14s_ec_read() directly appears to bypass this locking, potentially
allowing concurrent sysfs reads to interleave with other subsystem I2C
transfers and violate the hardware constraint.
[Severity: High]
This isn't a bug introduced by this patch, but because this exposes new sysfs
attributes that userspace can trigger, does t14s_ec_read() pass stack-allocated
buffers to __i2c_transfer()?
Looking at t14s_ec_read():
static int t14s_ec_read(void *context, unsigned int reg,
unsigned int *val)
{
...
u8 buf[4] = {T14S_EC_CMD_ECRD, reg, 0x00, 0x01};
struct i2c_msg request, response;
u8 result;
...
request.buf = buf;
...
response.buf = &result;
...
ret = __i2c_transfer(client->adapter, &request, 1);
On ARM64 architectures with CONFIG_VMAP_STACK, passing stack memory to I2C
drivers using DMA (like Qualcomm GENI) can trigger a BUG() panic or corrupt
adjacent physical memory. Is it possible to use heap-allocated buffers here
instead?
> + if (ret)
> + return ret;
> + *val = value * 1000;
> +
> + return 0;
> + }
> + break;
> +
> + case hwmon_fan:
> + if (attr == hwmon_fan_input) {
> + int lsb, msb;
> + ret = t14s_ec_read(ec, T14S_EC_FAN_RPM_LSB, &lsb);
[Severity: Low]
Does this pass pointers of type int * to t14s_ec_read() which expects
unsigned int *?
Newer compilers like GCC 14 treat -Wincompatible-pointer-types as an
error by default, which might break the build here.
[Severity: Medium]
Is there a risk of torn reads when reading the lsb and msb sequentially?
Because t14s_ec_read() enforces a 10ms sleep after each I2C transfer, there is
a large race window between reading T14S_EC_FAN_RPM_LSB and
T14S_EC_FAN_RPM_MSB. If the fan RPM rolls over (e.g., from 0x01ff to 0x0200)
during that window, could we combine an old lsb with a new msb and report
a wildly incorrect fan speed?
> + if (ret)
> + return ret;
> +
> + ret = t14s_ec_read(ec, T14S_EC_FAN_RPM_MSB, &msb);
> + if (ret)
> + return ret;
> +
> + *val = 0;
> + *val = lsb + (msb << 8);
> +
> + return 0;
> + }
> + break;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624210825.264454-1-daniel.lezcano@oss.qualcomm.com?part=1
^ permalink raw reply
* Re: [PATCH v1 2/3] platform: arm64: thinkpad-t14s-ec: Wire EC thermal events to hwmon
From: sashiko-bot @ 2026-06-24 21:21 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-hwmon, robh, conor+dt, devicetree
In-Reply-To: <20260624210825.264454-3-daniel.lezcano@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing HWMON_T_ALARM capability prevents sysfs notification from working.
--
commit 1242d377b6327e98b089989a953188982e0ca595
Author: Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>
platform: arm64: thinkpad-t14s-ec: Wire EC thermal events to hwmon
This patch wires up EC thermal zone status change notifications to the hwmon
notification framework. It associates hwmon temperature channels with EC
thermal events and emits hwmon_temp_alarm notifications to userspace.
> diff --git a/drivers/platform/arm64/lenovo-thinkpad-t14s.c b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> index 142464623f0e8..276bb51da33a3 100644
> --- a/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> +++ b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
[ ... ]
> @@ -493,6 +496,20 @@ static int t14s_input_probe(struct t14s_ec *ec)
> return input_register_device(ec->inputdev);
> }
>
> +static void t14s_ec_hwmon_notify_event(struct t14s_ec *ec, u8 event)
> +{
> + for (int i = 0; i < ec->ec_hwmon.num_sys_thermx; i++) {
> + if (ec->ec_hwmon.sys_thermx[i].event != event)
> + continue;
> +
> + hwmon_notify_event(ec->ec_hwmon.dev, hwmon_temp,
> + hwmon_temp_alarm, i);
> +
> + dev_dbg(ec->dev, "Thermal Zone (%s) Status Change Event\n",
> + ec->ec_hwmon.sys_thermx[i].label);
> + }
> +}
> +
[ ... ]
> @@ -658,6 +671,7 @@ static const struct hwmon_ops t14s_ec_hwmon_ops = {
> };
>
> static const struct hwmon_channel_info *t14s_ec_hwmon_info[] = {
> + HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> HWMON_CHANNEL_INFO(temp,
> HWMON_T_INPUT | HWMON_T_LABEL,
> HWMON_T_INPUT | HWMON_T_LABEL,
> HWMON_T_INPUT | HWMON_T_LABEL,
> HWMON_T_INPUT | HWMON_T_LABEL,
> HWMON_T_INPUT | HWMON_T_LABEL,
> HWMON_T_INPUT | HWMON_T_LABEL),
[Severity: Medium]
Will the hwmon_temp_alarm notifications successfully reach userspace?
The channel configuration array does not include the HWMON_T_ALARM capability
for any of the temperature channels. Without this, the hwmon core will not
create the corresponding tempX_alarm sysfs attributes.
When t14s_ec_hwmon_notify_event() calls hwmon_notify_event() for
hwmon_temp_alarm, it appears the sysfs notification will fail silently because
the attribute does not exist. The thermal subsystem still receives the event
via hwmon_thermal_notify(), but userspace monitoring tools will not be able to
read or poll the sysfs attributes.
Does HWMON_T_ALARM need to be added to the channel configurations here to make
the sysfs event notification work as described in the commit message?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624210825.264454-1-daniel.lezcano@oss.qualcomm.com?part=2
^ permalink raw reply
* Re: [PATCH v1 3/3] arm64: dts: qcom: x1e78100-t14s: Add thermal zones for keyboard skin and charging sensors
From: sashiko-bot @ 2026-06-24 21:18 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-hwmon, robh, conor+dt, devicetree
In-Reply-To: <20260624210825.264454-4-daniel.lezcano@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message claims the charging thermal zone uses 'multiple passive trip points' to 'progressively throttle the different CPU clusters', but the implementation provides only a single passive trip point that throttles all CPUs simultaneously.
--
commit 165036bedb34a29493f4bd0a7af23dab152aff0c
Author: Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>
arm64: dts: qcom: x1e78100-t14s: Add thermal zones for keyboard skin and charging sensors
Add thermal zone descriptions for the keyboard skin temperature and battery
charging circuitry sensors exposed by the embedded controller.
> - a charging circuitry temperature zone with multiple passive trip
> points and CPU frequency mitigation levels.
>
> The charging thermal zone progressively throttles the different CPU
> clusters as the charging circuitry temperature rises and triggers a hot
> condition at the highest trip point.
[Severity: Medium]
Does the implementation match this commit message description?
The commit message claims there are multiple passive trip points and
progressive throttling across different CPU clusters. Is a chunk of the patch
missing, or should the commit message be updated to reflect the final design
shown below?
> diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
> --- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
[ ... ]
> + ec-charging-thermal {
> + polling-delay = <5000>;
> + polling-delay-passive = <2000>;
> +
> + thermal-sensors = <&ec 3>;
> +
> + trips {
> + psv0: trip-point0 {
> + temperature = <55000>;
> + hysteresis = <0>;
> + type = "passive";
> + };
> +
> + alert0: trip-point1 {
> + temperature = <63000>;
> + hysteresis = <0>;
> + type = "hot";
> + };
> + };
[Severity: Medium]
Only a single passive trip point is defined here in ec-charging-thermal,
which doesn't match the "multiple passive trip points" mentioned in the
commit message.
> +
> + cooling-maps {
> + map0 {
> + trip = <&psv0>;
> + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu8 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu9 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu10 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu11 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +
> + };
> + };
[Severity: Medium]
Additionally, all 12 CPUs are tied uniformly to this single trip point.
Should this define progressive throttling across different CPU clusters as
described in the commit message?
> +
> + };
> +};
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624210825.264454-1-daniel.lezcano@oss.qualcomm.com?part=3
^ permalink raw reply
* [PATCH v1 3/3] arm64: dts: qcom: x1e78100-t14s: Add thermal zones for keyboard skin and charging sensors
From: Daniel Lezcano @ 2026-06-24 21:08 UTC (permalink / raw)
To: sre, hansg, ilpo.jarvinen, linux, andersson, konradybcio, robh,
krzk+dt, conor+dt
Cc: bryan.odonoghue, platform-driver-x86, linux-kernel, linux-hwmon,
linux-arm-msm, devicetree
In-Reply-To: <20260624210825.264454-1-daniel.lezcano@oss.qualcomm.com>
Add thermal zone descriptions for the keyboard skin temperature and
battery charging circuitry sensors exposed by the embedded controller.
Expose the EC as a thermal sensor provider and define two thermal zones
using the temperatures reported by the EC:
- a keyboard skin temperature zone with passive and hot trip points,
- a charging circuitry temperature zone with multiple passive trip
points and CPU frequency mitigation levels.
The charging thermal zone progressively throttles the different CPU
clusters as the charging circuitry temperature rises and triggers a hot
condition at the highest trip point.
This provides thermal framework integration for the EC temperature
sensors and enables platform thermal management through standard thermal
zone definitions.
Signed-off-by: Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>
---
.../qcom/x1e78100-lenovo-thinkpad-t14s.dtsi | 68 ++++++++++++++++++-
1 file changed, 67 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
index 5d49df41be02..e27533665c8d 100644
--- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
@@ -979,7 +979,7 @@ &i2c6 {
status = "okay";
- embedded-controller@28 {
+ ec: embedded-controller@28 {
compatible = "lenovo,thinkpad-t14s-ec";
reg = <0x28>;
@@ -988,6 +988,8 @@ embedded-controller@28 {
pinctrl-0 = <&ec_int_n_default>;
pinctrl-names = "default";
+ #thermal-sensor-cells = <1>;
+
wakeup-source;
};
};
@@ -1729,3 +1731,67 @@ &usb_mp_qmpphy1 {
status = "okay";
};
+
+&thermal_zones {
+ ec-keyboard-thermal {
+ polling-delay = <5000>;
+ polling-delay-passive = <1000>;
+
+ thermal-sensors = <&ec 1>;
+
+ trips {
+ trip-point0 {
+ temperature = <55000>;
+ hysteresis = <2000>;
+ type = "passive";
+ };
+
+ trip-point1 {
+ temperature = <62000>;
+ hysteresis = <0>;
+ type = "hot";
+ };
+ };
+ };
+
+ ec-charging-thermal {
+ polling-delay = <5000>;
+ polling-delay-passive = <2000>;
+
+ thermal-sensors = <&ec 3>;
+
+ trips {
+ psv0: trip-point0 {
+ temperature = <55000>;
+ hysteresis = <0>;
+ type = "passive";
+ };
+
+ alert0: trip-point1 {
+ temperature = <63000>;
+ hysteresis = <0>;
+ type = "hot";
+ };
+ };
+
+ cooling-maps {
+ map0 {
+ trip = <&psv0>;
+ cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu8 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu9 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu10 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+ <&cpu11 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+
+ };
+ };
+
+ };
+};
--
2.53.0
^ permalink raw reply related
* [PATCH v1 2/3] platform: arm64: thinkpad-t14s-ec: Wire EC thermal events to hwmon
From: Daniel Lezcano @ 2026-06-24 21:08 UTC (permalink / raw)
To: sre, hansg, ilpo.jarvinen, linux, andersson, konradybcio, robh,
krzk+dt, conor+dt
Cc: bryan.odonoghue, platform-driver-x86, linux-kernel, linux-hwmon,
linux-arm-msm, devicetree
In-Reply-To: <20260624210825.264454-1-daniel.lezcano@oss.qualcomm.com>
The EC generates thermal zone status change notifications for a subset
of the exposed temperature sensors. Wire these EC events to the hwmon
notification framework so userspace can be informed when a thermal alarm
state changes.
Associate each hwmon temperature channel with its corresponding EC
thermal event and emit hwmon_temp_alarm notifications through
hwmon_notify_event() when the EC reports a thermal zone status change.
Also register thermal zones in the hwmon chip capabilities and keep a
reference to the hwmon device to allow event propagation from the IRQ
handler.
This allows userspace monitoring tools to receive thermal alarm
updates without polling the sensors and gives the opportuniy to the
kernel to cool them down.
Signed-off-by: Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>
---
drivers/platform/arm64/lenovo-thinkpad-t14s.c | 66 ++++++++++++++-----
1 file changed, 51 insertions(+), 15 deletions(-)
diff --git a/drivers/platform/arm64/lenovo-thinkpad-t14s.c b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
index 142464623f0e..276bb51da33a 100644
--- a/drivers/platform/arm64/lenovo-thinkpad-t14s.c
+++ b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
@@ -107,10 +107,13 @@ struct t14s_ec_led_classdev {
struct t14s_ec_hwmon_sys_thermx {
const char *label;
int reg;
+ u8 event;
};
struct t14s_ec_hwmon {
+ struct device *dev;
struct t14s_ec_hwmon_sys_thermx *sys_thermx;
+ size_t num_sys_thermx;
};
struct t14s_ec {
@@ -493,6 +496,20 @@ static int t14s_input_probe(struct t14s_ec *ec)
return input_register_device(ec->inputdev);
}
+static void t14s_ec_hwmon_notify_event(struct t14s_ec *ec, u8 event)
+{
+ for (int i = 0; i < ec->ec_hwmon.num_sys_thermx; i++) {
+ if (ec->ec_hwmon.sys_thermx[i].event != event)
+ continue;
+
+ hwmon_notify_event(ec->ec_hwmon.dev, hwmon_temp,
+ hwmon_temp_alarm, i);
+
+ dev_dbg(ec->dev, "Thermal Zone (%s) Status Change Event\n",
+ ec->ec_hwmon.sys_thermx[i].label);
+ }
+}
+
static irqreturn_t t14s_ec_irq_handler(int irq, void *data)
{
struct t14s_ec *ec = data;
@@ -542,13 +559,9 @@ static irqreturn_t t14s_ec_irq_handler(int irq, void *data)
dev_dbg(ec->dev, "LID closed\n");
break;
case T14S_EC_EVT_THERMAL_TZ40:
- dev_dbg(ec->dev, "Thermal Zone 40 Status Change Event (CPU/GPU)\n");
- break;
case T14S_EC_EVT_THERMAL_TZ42:
- dev_dbg(ec->dev, "Thermal Zone 42 Status Change Event (Battery)\n");
- break;
case T14S_EC_EVT_THERMAL_TZ39:
- dev_dbg(ec->dev, "Thermal Zone 39 Status Change Event (CPU/GPU)\n");
+ t14s_ec_hwmon_notify_event(ec, val);
break;
case T14S_EC_EVT_KEY_FN_G:
dev_dbg(ec->dev, "FN + G - toggle double-tapping\n");
@@ -658,6 +671,7 @@ static const struct hwmon_ops t14s_ec_hwmon_ops = {
};
static const struct hwmon_channel_info *t14s_ec_hwmon_info[] = {
+ HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
HWMON_CHANNEL_INFO(temp,
HWMON_T_INPUT | HWMON_T_LABEL,
HWMON_T_INPUT | HWMON_T_LABEL,
@@ -676,14 +690,34 @@ static const struct hwmon_chip_info t14s_ec_chip_info = {
static int t14s_ec_hwmon_probe(struct t14s_ec *ec)
{
- struct device *dev;
struct t14s_ec_hwmon_sys_thermx sys_thermx[] = {
- { T14S_EC_SYS_THERM0, "soc" },
- { T14S_EC_SYS_THERM1, "keyboard" },
- { T14S_EC_SYS_THERM2, "base" },
- { T14S_EC_SYS_THERM3, "pmbm" },
- { T14S_EC_SYS_THERM6, "qtm" },
- { T14S_EC_SYS_THERM7, "ssd" },
+ {
+ .label = "soc",
+ .reg = T14S_EC_SYS_THERM0,
+ .event = T14S_EC_EVT_THERMAL_TZ39
+ },
+ {
+ .label = "keyboard",
+ .reg = T14S_EC_SYS_THERM1,
+ .event = T14S_EC_EVT_THERMAL_TZ40
+ },
+ {
+ .label = "base",
+ .reg = T14S_EC_SYS_THERM2,
+ },
+ {
+ .label = "pmbm",
+ .reg = T14S_EC_SYS_THERM3,
+ .event = T14S_EC_EVT_THERMAL_TZ42
+ },
+ {
+ .label = "qtm",
+ .reg = T14S_EC_SYS_THERM6
+ },
+ {
+ .label = "ssd",
+ .reg = T14S_EC_SYS_THERM7
+ },
};
ec->ec_hwmon.sys_thermx = devm_kmemdup_array(ec->dev, sys_thermx,
@@ -692,10 +726,12 @@ static int t14s_ec_hwmon_probe(struct t14s_ec *ec)
if (!ec->ec_hwmon.sys_thermx)
return -ENOMEM;
- dev = devm_hwmon_device_register_with_info(ec->dev, "t14s_ec", ec,
- &t14s_ec_chip_info, NULL);
+ ec->ec_hwmon.num_sys_thermx = ARRAY_SIZE(sys_thermx);
+
+ ec->ec_hwmon.dev = devm_hwmon_device_register_with_info(ec->dev, "t14s_ec", ec,
+ &t14s_ec_chip_info, NULL);
- return PTR_ERR_OR_ZERO(dev);
+ return PTR_ERR_OR_ZERO(ec->ec_hwmon.dev);
}
static int t14s_ec_probe(struct i2c_client *client)
--
2.53.0
^ permalink raw reply related
* [PATCH v1 1/3] drivers/platform: lenovo-t14s-ec: Add hwmon support for temperatures and fan speed
From: Daniel Lezcano @ 2026-06-24 21:08 UTC (permalink / raw)
To: sre, hansg, ilpo.jarvinen, linux, andersson, konradybcio, robh,
krzk+dt, conor+dt
Cc: bryan.odonoghue, platform-driver-x86, linux-kernel, linux-hwmon,
linux-arm-msm, devicetree
In-Reply-To: <20260624210825.264454-1-daniel.lezcano@oss.qualcomm.com>
Expose the Lenovo ThinkPad T14s EC environmental sensors through
the hwmon subsystem.
The driver now registers a hwmon device providing access to six EC
temperature sensors corresponding to the SoC, keyboard area, base
cover, PMIC/charging circuitry, QTM module and SSD. Sensor labels
are exported to allow user space to identify each measurement.
Additionally, expose the system fan speed by reading the fan RPM
registers from the embedded controller.
This allows standard monitoring tools such as lm-sensors to report
platform temperatures and fan speed.
Signed-off-by: Daniel Lezcano daniel.lezcano@oss.qualcomm.com
---
drivers/platform/arm64/lenovo-thinkpad-t14s.c | 147 ++++++++++++++++++
1 file changed, 147 insertions(+)
diff --git a/drivers/platform/arm64/lenovo-thinkpad-t14s.c b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
index 5590302a5694..142464623f0e 100644
--- a/drivers/platform/arm64/lenovo-thinkpad-t14s.c
+++ b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
@@ -11,6 +11,7 @@
#include <linux/delay.h>
#include <linux/dev_printk.h>
#include <linux/err.h>
+#include <linux/hwmon.h>
#include <linux/i2c.h>
#include <linux/input.h>
#include <linux/input/sparse-keymap.h>
@@ -67,6 +68,16 @@
#define T14S_EC_EVT_KEY_FN_F11 0x7a
#define T14S_EC_EVT_KEY_FN_G 0x7e
+#define T14S_EC_SYS_THERM0 0x78 /* SoC (CPU+GPU) */
+#define T14S_EC_SYS_THERM1 0x79 /* Keyboard */
+#define T14S_EC_SYS_THERM2 0x7a /* Back cover */
+#define T14S_EC_SYS_THERM3 0x7b /* Charger / PMIC */
+#define T14S_EC_SYS_THERM6 0x7c /* QTM West */
+#define T14S_EC_SYS_THERM7 0x7d /* SSD */
+
+#define T14S_EC_FAN_RPM_LSB 0x84
+#define T14S_EC_FAN_RPM_MSB 0x85
+
/* Hardware LED blink rate is 1 Hz (500ms off, 500ms on) */
#define T14S_EC_BLINK_RATE_ON_OFF_MS 500
@@ -93,9 +104,19 @@ struct t14s_ec_led_classdev {
struct t14s_ec *ec;
};
+struct t14s_ec_hwmon_sys_thermx {
+ const char *label;
+ int reg;
+};
+
+struct t14s_ec_hwmon {
+ struct t14s_ec_hwmon_sys_thermx *sys_thermx;
+};
+
struct t14s_ec {
struct regmap *regmap;
struct device *dev;
+ struct t14s_ec_hwmon ec_hwmon;
struct t14s_ec_led_classdev led_pwr_btn;
struct t14s_ec_led_classdev led_chrg_orange;
struct t14s_ec_led_classdev led_chrg_white;
@@ -555,6 +576,128 @@ static irqreturn_t t14s_ec_irq_handler(int irq, void *data)
return IRQ_HANDLED;
}
+static umode_t t14s_ec_hwmon_is_visible(const void *drvdata,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ switch (type) {
+ case hwmon_temp:
+ return 0444;
+ case hwmon_fan:
+ return 0444;
+ default:
+ return 0;
+ }
+}
+
+static int t14s_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, const char **str)
+{
+ struct t14s_ec *ec = dev_get_drvdata(dev);
+ switch (type) {
+ case hwmon_temp:
+ if (attr == hwmon_temp_label) {
+ *str = ec->ec_hwmon.sys_thermx[channel].label;
+ return 0;
+ }
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return -EOPNOTSUPP;
+}
+
+static int t14s_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct t14s_ec *ec = dev_get_drvdata(dev);
+ unsigned int value;
+ int ret;
+
+ switch (type) {
+ case hwmon_temp:
+ if (attr == hwmon_temp_input) {
+ ret = t14s_ec_read(ec, ec->ec_hwmon.sys_thermx[channel].reg, &value);
+ if (ret)
+ return ret;
+ *val = value * 1000;
+
+ return 0;
+ }
+ break;
+
+ case hwmon_fan:
+ if (attr == hwmon_fan_input) {
+ int lsb, msb;
+ ret = t14s_ec_read(ec, T14S_EC_FAN_RPM_LSB, &lsb);
+ if (ret)
+ return ret;
+
+ ret = t14s_ec_read(ec, T14S_EC_FAN_RPM_MSB, &msb);
+ if (ret)
+ return ret;
+
+ *val = 0;
+ *val = lsb + (msb << 8);
+
+ return 0;
+ }
+ break;
+ default:
+ break;
+ }
+
+ return -EOPNOTSUPP;
+}
+
+static const struct hwmon_ops t14s_ec_hwmon_ops = {
+ .is_visible = t14s_ec_hwmon_is_visible,
+ .read = t14s_ec_hwmon_read,
+ .read_string = t14s_ec_hwmon_read_string,
+};
+
+static const struct hwmon_channel_info *t14s_ec_hwmon_info[] = {
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL),
+ HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
+ NULL
+};
+
+static const struct hwmon_chip_info t14s_ec_chip_info = {
+ .ops = &t14s_ec_hwmon_ops,
+ .info = t14s_ec_hwmon_info,
+};
+
+static int t14s_ec_hwmon_probe(struct t14s_ec *ec)
+{
+ struct device *dev;
+ struct t14s_ec_hwmon_sys_thermx sys_thermx[] = {
+ { T14S_EC_SYS_THERM0, "soc" },
+ { T14S_EC_SYS_THERM1, "keyboard" },
+ { T14S_EC_SYS_THERM2, "base" },
+ { T14S_EC_SYS_THERM3, "pmbm" },
+ { T14S_EC_SYS_THERM6, "qtm" },
+ { T14S_EC_SYS_THERM7, "ssd" },
+ };
+
+ ec->ec_hwmon.sys_thermx = devm_kmemdup_array(ec->dev, sys_thermx,
+ ARRAY_SIZE(sys_thermx),
+ sizeof(sys_thermx[0]), GFP_KERNEL);
+ if (!ec->ec_hwmon.sys_thermx)
+ return -ENOMEM;
+
+ dev = devm_hwmon_device_register_with_info(ec->dev, "t14s_ec", ec,
+ &t14s_ec_chip_info, NULL);
+
+ return PTR_ERR_OR_ZERO(dev);
+}
+
static int t14s_ec_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
@@ -590,6 +733,10 @@ static int t14s_ec_probe(struct i2c_client *client)
if (ret < 0)
return ret;
+ ret = t14s_ec_hwmon_probe(ec);
+ if (ret < 0)
+ return ret;
+
ret = devm_request_threaded_irq(dev, client->irq, NULL,
t14s_ec_irq_handler,
IRQF_ONESHOT, dev_name(dev), ec);
--
2.53.0
^ permalink raw reply related
* [PATCH v1 0/3] Lenovo ThinkPad T14s EC thermal monitoring and thermal zone integration
From: Daniel Lezcano @ 2026-06-24 21:08 UTC (permalink / raw)
To: sre, hansg, ilpo.jarvinen, linux, andersson, konradybcio, robh,
krzk+dt, conor+dt
Cc: bryan.odonoghue, platform-driver-x86, linux-kernel, linux-hwmon,
linux-arm-msm, devicetree
Hi,
This series extends the Lenovo ThinkPad T14s embedded controller driver
with environmental monitoring capabilities and integrates the exposed
sensors into the Linux thermal framework.
The EC provides access to several platform temperature sensors covering
the SoC, keyboard area, bottom cover, charging circuitry, QTM module and
SSD, as well as fan speed information. These sensors are currently used
by the firmware for thermal management but are not exposed to Linux.
The first patch adds hwmon support for the EC temperature sensors and fan
RPM reporting.
The second patch wires EC thermal notifications into the hwmon event
framework, allowing userspace to receive alarm notifications when the EC
reports thermal zone state changes.
The last patch exposes the EC as a thermal sensor provider in the device
tree and defines thermal zones for the keyboard skin temperature and the
charging circuitry temperature. This allows the generic thermal
framework to react to EC-reported temperatures and apply standard Linux
thermal mitigation policies.
With the latest patch, this series fixes thermal issues happening on
this platform where a kernel compilation leads to a system reboot.
Tested on a Lenovo ThinkPad T14s Gen 6 (Snapdragon X Elite).
Thanks,
Daniel
Daniel Lezcano (3):
drivers/platform: lenovo-t14s-ec: Add hwmon support for temperatures
and fan speed
platform: arm64: thinkpad-t14s-ec: Wire EC thermal events to hwmon
arm64: dts: qcom: x1e78100-t14s: Add thermal zones for keyboard skin
and charging sensors
.../qcom/x1e78100-lenovo-thinkpad-t14s.dtsi | 68 +++++-
drivers/platform/arm64/lenovo-thinkpad-t14s.c | 193 +++++++++++++++++-
2 files changed, 255 insertions(+), 6 deletions(-)
--
2.53.0
^ permalink raw reply
* Re: [PATCH v5 08/12] i3c: dw-i3c-master: Add ACPI core clock frequency quirk
From: Frank Li @ 2026-06-24 18:09 UTC (permalink / raw)
To: Akhil R
Cc: Alexandre Belloni, Frank Li, Miquel Raynal, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Guenter Roeck, Philipp Zabel,
Jon Hunter, Thierry Reding, linux-i3c, devicetree, linux-hwmon,
linux-tegra, linux-kernel
In-Reply-To: <20260624102153.1770072-9-akhilrajeev@nvidia.com>
On Wed, Jun 24, 2026 at 10:21:02AM +0000, Akhil R wrote:
> Some ACPI-enumerated devices like Tegra410 do not expose the controller
> core clock through the clk framework. Unlike device tree, ACPI on Arm does
> not model clock providers. The hardware is expected to have its clocks
> enabled by firmware before the OS takes over.
>
> Make the core clock optional and allow selected ACPI devices to provide the
> core clock rate through the "clock-frequency" _DSD property when the core
> clock is absent.
>
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> ---
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> drivers/i3c/master/dw-i3c-master.c | 27 ++++++++++++++++++++++++---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 29030fd9594a..8e40d178d500 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -242,6 +242,7 @@
> /* List of quirks */
> #define AMD_I3C_OD_PP_TIMING BIT(1)
> #define DW_I3C_DISABLE_RUNTIME_PM_QUIRK BIT(2)
> +#define DW_I3C_ACPI_SKIP_CLK_RST BIT(3)
>
> struct dw_i3c_cmd {
> u32 cmd_lo;
> @@ -556,13 +557,33 @@ static void dw_i3c_master_set_intr_regs(struct dw_i3c_master *master)
> writel(IBI_REQ_REJECT_ALL, master->regs + IBI_MR_REQ_REJECT);
> }
>
> +static unsigned long dw_i3c_master_get_core_rate(struct dw_i3c_master *master)
> +{
> + unsigned int core_rate_prop;
> +
> + if (master->core_clk)
> + return clk_get_rate(master->core_clk);
> +
> + if (!(master->quirks & DW_I3C_ACPI_SKIP_CLK_RST))
> + dev_err(master->dev, "missing core clock\n");
> + return 0;
> + }
> +
> + if (device_property_read_u32(master->dev, "clock-frequency", &core_rate_prop)) {
> + dev_err(master->dev, "missing clock-frequency property\n");
> + return 0;
> + }
> +
> + return core_rate_prop;
> +}
> +
> static int dw_i3c_clk_cfg(struct dw_i3c_master *master)
> {
> unsigned long core_rate, core_period;
> u32 scl_timing;
> u8 hcnt, lcnt;
>
> - core_rate = clk_get_rate(master->core_clk);
> + core_rate = dw_i3c_master_get_core_rate(master);
> if (!core_rate)
> return -EINVAL;
>
> @@ -615,7 +636,7 @@ static int dw_i2c_clk_cfg(struct dw_i3c_master *master)
> u16 hcnt, lcnt;
> u32 scl_timing;
>
> - core_rate = clk_get_rate(master->core_clk);
> + core_rate = dw_i3c_master_get_core_rate(master);
> if (!core_rate)
> return -EINVAL;
>
> @@ -1577,7 +1598,7 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
> if (IS_ERR(master->regs))
> return PTR_ERR(master->regs);
>
> - master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);
> + master->core_clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
> if (IS_ERR(master->core_clk))
> return PTR_ERR(master->core_clk);
>
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH v5 04/12] i3c: master: Add support for devices using SETAASA
From: Frank Li @ 2026-06-24 17:57 UTC (permalink / raw)
To: sashiko-reviews
Cc: Akhil R, linux-hwmon, devicetree, robh, linux-i3c, conor+dt,
Frank.Li
In-Reply-To: <20260624104326.AE6C81F000E9@smtp.kernel.org>
On Wed, Jun 24, 2026 at 10:43:26AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [High] SETAASA broadcast is sent before SETDASA direct assignments, which breaks initialization for devices that natively support SETAASA but are configured for SETDASA.
>
> Pre-existing issues:
> - [High] Missing RSTDAA on error path due to premature address slot clearing leads to bus address collision.
> - [High] Double free and Use-After-Free of `desc->dev` when `device_register()` fails.
> --
>
> commit 08b7cd00b36fcaa4fe6292af27e273df6dc64b5a
> Author: Akhil R <akhilrajeev@nvidia.com>
>
> i3c: master: Add support for devices using SETAASA
>
> Add support for devices using SETAASA attached to DDR5 memory modules that
> do not support ENTDAA. Devices using SETAASA assign their pre-defined static
> addresses as dynamic addresses during DAA.
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 4bba2bad897a7..3deae8cdae525 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
>
> [ ... ]
>
> > @@ -1878,6 +1924,22 @@ static int i3c_master_early_i3c_dev_add(struct i3c_master_controller *master,
> > if (ret)
> > goto err_free_dev;
> >
> > + /*
> > + * For devices using SETAASA instead of ENTDAA, the address is statically
> > + * assigned. Update the dynamic address to the provided static address.
> > + * Reattach the I3C device after updating the dynamic address with the same
> > + * static address. It is not mandatory for such devices to implement CCC
> > + * commands like GETPID, GETDCR etc. Hence, we can return after reattaching.
> > + */
> > + if (i3cdev->boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> > + i3cdev->info.dyn_addr = i3cdev->boardinfo->static_addr;
> > + ret = i3c_master_reattach_i3c_dev_locked(i3cdev, 0);
> > + if (ret)
> > + goto err_rstdaa;
> > +
> > + return 0;
> > + }
> > +
>
> [Severity: High]
> This is a pre-existing issue, but missing RSTDAA on the error path due to
> premature address slot clearing can lead to bus address collisions.
>
> In i3c_master_early_i3c_dev_add(), the driver assigns the dynamic address
> with SETDASA and reattaches the device. If reattaching fails, the error
> path internally calls i3c_master_put_i3c_addrs(), which clears the
> init_dyn_addr slot status back to I3C_ADDR_SLOT_FREE.
>
> When the code jumps to err_rstdaa:
>
> err_rstdaa:
> i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);
>
> It calls i3c_master_rstdaa_locked() to reset the device's dynamic address.
> However, i3c_master_rstdaa_locked() verifies the slot status:
>
> drivers/i3c/master.c:i3c_master_rstdaa_locked() {
> ...
> if (addr != I3C_BROADCAST_ADDR && addrstat != I3C_ADDR_SLOT_I3C_DEV)
> return -EINVAL;
> ...
> }
>
> Because the slot was just freed, the function silently returns without
> sending the RSTDAA CCC command.
>
> Will this allow the hardware device to continue squatting on the dynamic
> address while the master considers the slot free, causing collisions later?
>
> > ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
> > i3cdev->boardinfo->init_dyn_addr);
>
> [ ... ]
>
> > @@ -2232,6 +2294,12 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> > if (ret)
> > goto err_bus_cleanup;
> >
> > + if (master->addr_method & I3C_ADDR_METHOD_SETAASA) {
> > + ret = i3c_master_setaasa_locked(master);
> > + if (ret)
> > + goto err_bus_cleanup;
> > + }
> > +
>
> [Severity: High]
> Is it possible that sending the SETAASA broadcast before direct SETDASA
> assignments breaks initialization for devices that natively support SETAASA
> but are configured for SETDASA?
>
> According to the I3C specification, any device on the bus natively supporting
> SETAASA will respond to this broadcast by adopting its static address as its
> dynamic address.
>
> After this broadcast, the driver iterates through devices and attempts to
> assign custom dynamic addresses via direct SETDASA commands:
>
> drivers/i3c/master.c:i3c_master_early_i3c_dev_add() {
> ...
> ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
> i3cdev->boardinfo->init_dyn_addr);
> ...
> }
>
> Since the target device already adopted its dynamic address during the
> SETAASA broadcast, it is no longer in the unassigned state and will NACK
> the subsequent SETDASA command.
Look like correct, but I am not sure if target will NACK SETDASA. Or should
use SETNEWDA for SETAASA method.
Frank
>
> Could this cause device initialization to abort entirely?
>
> Should SETDASA direct assignments be executed before broadcasting SETAASA?
>
> > /*
> > * Reserve init_dyn_addr first, and then try to pre-assign dynamic
> > * address and retrieve device information if needed.
>
> [ ... ]
>
> [Severity: High]
> This isn't a bug introduced by this patch, but there is a potential double
> free and use-after-free of desc->dev when device_register() fails.
>
> In i3c_master_register_new_i3c_devs(), if device_register() fails,
> put_device() is called:
>
> drivers/i3c/master.c:i3c_master_register_new_i3c_devs() {
> ...
> ret = device_register(&desc->dev->dev);
> if (ret) {
> dev_err(&master->dev, "Failed to add I3C device (err = %d)\n", ret);
> put_device(&desc->dev->dev);
> }
> ...
> }
>
> This drops the reference count to 0 and triggers i3c_device_release(), which
> frees desc->dev via kfree(i3cdev). However, the pointer desc->dev is not set
> to NULL and remains dangling in the master->bus.devs.i3c list.
>
> Later, when the master module is unloaded or unregistered, the cleanup
> iterates over the list:
>
> drivers/i3c/master.c:i3c_master_unregister_i3c_devs() {
> ...
> if (!i3cdev->dev)
> continue;
>
> i3cdev->dev->desc = NULL;
> if (device_is_registered(&i3cdev->dev->dev))
> device_unregister(&i3cdev->dev->dev);
> else
> put_device(&i3cdev->dev->dev);
> i3cdev->dev = NULL;
> ...
> }
>
> Because i3cdev->dev is a dangling pointer, the check passes. This leads to
> writing to i3cdev->dev->desc (use-after-free) and calling put_device() again
> (double free).
>
> Could we set desc->dev to NULL after put_device() in the registration error
> path?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260624102153.1770072-1-akhilrajeev@nvidia.com?part=4
^ permalink raw reply
* Re: [PATCH v5 11/12] hwmon: spd5118: Add I3C support
From: sashiko-bot @ 2026-06-24 10:49 UTC (permalink / raw)
To: Akhil R; +Cc: devicetree, linux-i3c, linux-hwmon, robh, Frank.Li, conor+dt
In-Reply-To: <20260624102153.1770072-12-akhilrajeev@nvidia.com>
> Add a regmap config and a probe function to support I3C-based
> communication with SPD5118 devices.
>
> On an I3C bus, SPD5118 devices are enumerated via SETAASA and always
> require an ACPI or device tree entry. Device matching is hence through
> the OF match tables only and does not need an I3C class match table. The
> device identity is verified in the type registers before proceeding to
> the common probe function.
>
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624102153.1770072-1-akhilrajeev@nvidia.com?part=11
^ permalink raw reply
* Re: [PATCH v5 02/12] i3c: master: Use unified device property interface
From: sashiko-bot @ 2026-06-24 10:48 UTC (permalink / raw)
To: Akhil R; +Cc: devicetree, linux-hwmon, Frank.Li, linux-i3c, conor+dt, robh
In-Reply-To: <20260624102153.1770072-3-akhilrajeev@nvidia.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Fwnode references acquired for I2C and I3C boardinfo entries are permanently leaked.
Pre-existing issues:
- [High] Dangling `desc->dev` pointer causes Use-After-Free and Double Free when device registration fails.
--
commit cf59bb0f706cf267c9ca158815835766a81fc214
Author: Akhil R <akhilrajeev@nvidia.com>
i3c: master: Use unified device property interface
Replace all OF-specific functions with unified device property functions
as a prerequisite to support both ACPI and device tree.
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index f1be38a640ca1..b20f56f7b68e8 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
[ ... ]
> @@ -1928,7 +1930,8 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
> desc->info.pid);
>
> if (desc->boardinfo)
> - desc->dev->dev.of_node = desc->boardinfo->of_node;
> + device_set_node(&desc->dev->dev,
> + fwnode_handle_get(desc->boardinfo->fwnode));
[Severity: High]
Does this take an extra reference to the fwnode that ends up leaking?
The original code simply assigned the pointer, but the new code calls
fwnode_handle_get(), acquiring an additional reference. When the device is
subsequently released via i3c_device_release(), it will drop this second
reference.
Since the original reference stored in desc->boardinfo->fwnode does not appear
to be dropped during master teardown, does this cause the initial reference to
permanently leak on the normal success path?
> ret = device_register(&desc->dev->dev);
> if (ret) {
[Severity: High]
This isn't a bug introduced by this patch, but does this error path leave a
dangling pointer in desc->dev?
If device_register() fails, put_device(&desc->dev->dev) is called, which drops
the reference count to 0 and invokes i3c_device_release(). The release
function frees desc->dev (the i3cdev structure). However, desc->dev is not set
to NULL.
Later, when i3c_master_unregister_i3c_devs() iterates through the bus,
it will dereference the dangling pointer:
drivers/i3c/master.c:i3c_master_unregister_i3c_devs() {
...
i3c_bus_for_each_i3cdev(&master->bus, i3cdev) {
if (!i3cdev->dev)
continue;
i3cdev->dev->desc = NULL;
if (device_is_registered(&i3cdev->dev->dev))
device_unregister(&i3cdev->dev->dev);
else
put_device(&i3cdev->dev->dev);
i3cdev->dev = NULL;
}
...
}
Could this lead to a use-after-free and double-free when it evaluates
i3cdev->dev and attempts to call put_device() again?
[ ... ]
> @@ -2649,14 +2656,14 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
> boardinfo->lvr = reg[2];
>
> list_add_tail(&boardinfo->node, &master->boardinfo.i2c);
> - of_node_get(node);
> + fwnode_handle_get(fwnode);
>
> return 0;
> }
[Severity: High]
Does this code leak the fwnode reference?
The fwnode_handle_get(fwnode) call increments the reference count, but the
resulting reference is neither stored in boardinfo nor dropped before the
function returns. This appears to leak the reference immediately on the
success path.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624102153.1770072-1-akhilrajeev@nvidia.com?part=2
^ permalink raw reply
* Re: [PATCH v5 05/12] i3c: master: Add support for devices without PID
From: sashiko-bot @ 2026-06-24 10:45 UTC (permalink / raw)
To: Akhil R; +Cc: Frank.Li, robh, conor+dt, linux-i3c, devicetree, linux-hwmon
In-Reply-To: <20260624102153.1770072-6-akhilrajeev@nvidia.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [High] Devices returning a PID of 0 during Dynamic Address Assignment (DAA) bypass duplicate detection, leading to sysfs naming collisions that reliably trigger a preexisting Use-After-Free.
- [Medium] The driver incorrectly allows non-SETAASA I3C devices to be defined in the device tree with a PID of 0, permanently breaking device matching upon Hot-Join re-attachment.
- [Low] The newly added static address matching logic in `i3c_master_attach_boardinfo()` will never successfully match any device, rendering the `else` branch dead code.
- [Low] Redundant boolean check for `i3cdev->info.pid` inside the duplicate search loop.
Pre-existing issues:
- [High] Dangling pointer in `desc->dev` leads to a Use-After-Free and Double Free if `device_register()` fails.
- [High] Calling `device_register()` while holding a read lock on the bus rwsem can lead to a deadlock if the driver's probe function attempts an I3C transfer.
--
commit 76e7c85069f65f75e5476a13e6f4ab2ae705589f
Author: Akhil R <akhilrajeev@nvidia.com>
i3c: master: Add support for devices without PID
Devices using SETAASA for address assignment are not required to have
a 48-bit PID according to the I3C specification. Allow such devices to
register and use the static address where PID was required.
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 3deae8cdae525..bd0dc76c7ba1e 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1989,8 +1989,17 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
> desc->dev->dev.type = &i3c_device_type;
> desc->dev->dev.bus = &i3c_bus_type;
> desc->dev->dev.release = i3c_device_release;
> - dev_set_name(&desc->dev->dev, "%d-%llx", master->bus.id,
> - desc->info.pid);
> +
> + /*
> + * For devices without PID (e.g., SETAASA devices), use
> + * static address for naming instead.
> + */
> + if (desc->info.pid)
> + dev_set_name(&desc->dev->dev, "%d-%llx", master->bus.id,
> + desc->info.pid);
> + else
> + dev_set_name(&desc->dev->dev, "%d-%02x", master->bus.id,
> + desc->info.static_addr);
>
> if (desc->boardinfo)
> device_set_node(&desc->dev->dev,
[Severity: High]
This is a pre-existing issue, but does failing to clear desc->dev when
device_register() fails lead to a double free and use-after-free later?
Looking at i3c_master_register_new_i3c_devs():
ret = device_register(&desc->dev->dev);
if (ret) {
dev_err(&master->dev, "Failed to add I3C device (err = %d)\n", ret);
put_device(&desc->dev->dev);
}
If device_register() fails, put_device() is called which frees the i3c_device
structure, but desc->dev is left dangling. Later, when the master is
unregistered in i3c_master_unregister_i3c_devs(), the loop checks:
if (!i3cdev->dev)
continue;
i3cdev->dev->desc = NULL;
if (device_is_registered(&i3cdev->dev->dev))
device_unregister(&i3cdev->dev->dev);
else
put_device(&i3cdev->dev->dev);
Since desc->dev is not NULL, this writes to the freed memory and then calls
put_device() again, resulting in a use-after-free and double free.
[Severity: High]
This is also a pre-existing issue, but can calling device_register() while
holding a read lock on the bus rwsem lead to a deadlock?
In i3c_master_reg_work_fn():
i3c_bus_normaluse_lock(&master->bus);
if (!master->shutting_down)
i3c_master_register_new_i3c_devs(master);
The device_register() call inside i3c_master_register_new_i3c_devs()
synchronously triggers the driver's probe function. If the probe attempts an
I3C transfer, it calls i3c_device_do_priv_xfers(), which recursively requests
the read lock on the bus.
If a concurrent thread, such as a Hot Join, requests a write lock between
these two read lock attempts, the rwsem blocks the second read lock to prevent
writer starvation. The writer blocks waiting for the first read lock to be
released, which won't happen until device_register() returns, resulting in an
ABBA deadlock.
[ ... ]
> @@ -2383,8 +2392,18 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev)
> struct i3c_dev_boardinfo *i3cboardinfo;
>
> list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> - if (i3cdev->info.pid != i3cboardinfo->pid)
> - continue;
> + /*
> + * For devices without PID (e.g., SETAASA devices), match by
> + * static address. For devices with PID, match by PID.
> + */
> + if (i3cboardinfo->pid) {
> + if (i3cdev->info.pid != i3cboardinfo->pid)
> + continue;
> + } else {
> + if (!i3cboardinfo->static_addr ||
> + i3cdev->info.static_addr != i3cboardinfo->static_addr)
> + continue;
> + }
[Severity: Low]
Will this newly added else block ever successfully match a device?
i3c_master_attach_boardinfo() is called from
__i3c_master_add_i3c_dev_locked() to handle devices discovered dynamically via
DAA or Hot-Join. For these devices, the hardware does not provide a static
address during DAA, so i3cdev->info.static_addr is strictly 0.
Because of this, the condition
i3cdev->info.static_addr != i3cboardinfo->static_addr
will evaluate to 0 != non_zero, which is always true. This causes the loop
to unconditionally continue, making this fallback dead code.
>
> i3cdev->boardinfo = i3cboardinfo;
> i3cdev->info.static_addr = i3cboardinfo->static_addr;
> @@ -2398,8 +2417,12 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
> struct i3c_master_controller *master = i3c_dev_get_master(refdev);
> struct i3c_dev_desc *i3cdev;
>
> + if (!refdev->info.pid)
> + return NULL;
> +
[Severity: High]
Does returning NULL when PID is 0 allow devices to bypass duplicate detection
and trigger the pre-existing use-after-free bug?
If a dynamically discovered device reports a PID of 0, this check
unconditionally bypasses duplicate rejection, so the device is repeatedly
added.
Because static_addr is also 0 for DAA-discovered devices, the new naming
fallback in i3c_master_register_new_i3c_devs():
dev_set_name(&desc->dev->dev, "%d-%02x", master->bus.id,
desc->info.static_addr);
will name multiple devices the same (e.g., %d-00). The first instance will
register successfully, but subsequent ones will fail device_register() due to
sysfs name collisions. This perfectly triggers the desc->dev use-after-free
on the error path.
> i3c_bus_for_each_i3cdev(&master->bus, i3cdev) {
> - if (i3cdev != refdev && i3cdev->info.pid == refdev->info.pid)
> + if (i3cdev != refdev && i3cdev->info.pid &&
> + i3cdev->info.pid == refdev->info.pid)
[Severity: Low]
Is the check for i3cdev->info.pid redundant here?
Since refdev->info.pid was explicitly checked for being non-zero right before
this loop, the equality check i3cdev->info.pid == refdev->info.pid already
guarantees that i3cdev->info.pid is non-zero. The boolean check isn't strictly
necessary.
> return i3cdev;
> }
[ ... ]
> @@ -2832,9 +2855,15 @@ i3c_master_add_i3c_boardinfo(struct i3c_master_controller *master,
>
> boardinfo->pid = ((u64)reg[1] << 32) | reg[2];
>
> - if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
> - I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> - return -EINVAL;
> + /* For SETAASA devices, validate the static address instead of PID */
> + if (boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> + if (!boardinfo->static_addr)
> + return -EINVAL;
> + } else {
> + if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
> + I3C_PID_RND_LOWER_32BITS(boardinfo->pid))
> + return -EINVAL;
> + }
[Severity: Medium]
Could this logic incorrectly allow non-SETAASA I3C devices in the device tree
to have a PID of 0, permanently breaking device matching upon Hot-Join?
If a device is configured with mipi-i3c-static-method = SETDASA, it will reach
this else block. If its PID is 0, both boardinfo->pid & GENMASK_ULL(63, 48)
and I3C_PID_RND_LOWER_32BITS(boardinfo->pid) evaluate to 0, which bypasses
validation, and the device registers with PID 0.
If the device later Hot-Joins, DAA will retrieve its real, non-zero PID. When
i3c_master_attach_boardinfo() runs, it sees i3cboardinfo->pid == 0 and falls
back to matching by static_addr. Because static_addr is 0 during DAA, it
fails to match and the device permanently loses its DT properties.
>
> boardinfo->init_dyn_addr = init_dyn_addr;
> boardinfo->fwnode = fwnode_handle_get(fwnode);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624102153.1770072-1-akhilrajeev@nvidia.com?part=5
^ permalink raw reply
* Re: [PATCH v5 08/12] i3c: dw-i3c-master: Add ACPI core clock frequency quirk
From: sashiko-bot @ 2026-06-24 10:45 UTC (permalink / raw)
To: Akhil R; +Cc: robh, Frank.Li, linux-i3c, devicetree, linux-hwmon, conor+dt
In-Reply-To: <20260624102153.1770072-9-akhilrajeev@nvidia.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Changing `core_clk` from mandatory to optional delays missing clock validation, leading to unclocked hardware register accesses that can trigger a synchronous external abort (system crash) on misconfigured platforms.
Pre-existing issues:
- [High] The workqueue `master->wq` is destroyed during unregistration, but the devres-managed hardware IRQ remains active without being masked, allowing a late interrupt to queue work on the destroyed workqueue.
- [High] The driver unconditionally casts the pointer returned by `device_get_match_data()` to an `unsigned long` when binding an ACPI companion, leading to garbage quirk flags if the device is bound via the PRP0001 ACPI-OF fallback.
--
commit d5fbc1910ed540d2be9a4cb57886399f9c4f9b04
Author: Akhil R <akhilrajeev@nvidia.com>
i3c: dw-i3c-master: Add ACPI core clock frequency quirk
Some ACPI-enumerated devices like Tegra410 do not expose the controller
core clock through the clk framework. Unlike device tree, ACPI on Arm does
not model clock providers. The hardware is expected to have its clocks
enabled by firmware before the OS takes over.
Make the core clock optional and allow selected ACPI devices to provide the
core clock rate through the "clock-frequency" _DSD property when the core
clock is absent.
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 29030fd9594aa..8e40d178d500a 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
[ ... ]
> @@ -1577,7 +1598,7 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
> if (IS_ERR(master->regs))
> return PTR_ERR(master->regs);
>
> - master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);
> + master->core_clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
[Severity: High]
If the device tree is missing the core clock, this will now return NULL
instead of an error. Could this lead to unclocked MMIO accesses?
In dw_i3c_common_probe():
master->core_clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
...
writel(INTR_ALL, master->regs + INTR_STATUS);
If the clock is missing, the MMIO access to INTR_STATUS will happen
unconditionally, which can trigger a synchronous external abort on some
systems.
The missing clock is checked later in dw_i3c_master_get_core_rate():
if (!(master->quirks & DW_I3C_ACPI_SKIP_CLK_RST)) {
dev_err(master->dev, "missing core clock\n");
return 0;
}
Is it possible to validate the missing clock earlier to prevent the kernel
panic during the probe sequence?
> if (IS_ERR(master->core_clk))
> return PTR_ERR(master->core_clk);
>
[Severity: High]
This isn't a bug introduced by this patch, but does the removal path
destroy the workqueue while hardware interrupts are still active?
In drivers/i3c/master/dw-i3c-master.c:dw_i3c_common_remove():
i3c_master_unregister(&master->base);
This unregisters the device and eventually triggers:
drivers/i3c/master.c:i3c_masterdev_release():
if (master->wq)
destroy_workqueue(master->wq);
Since INTR_STATUS_EN is never cleared to mask interrupts and the devres
managed IRQ is not yet freed, could a delayed IBI or Hot-Join interrupt
still fire?
If it does, the still-active IRQ handler will attempt to queue work on
the destroyed workqueue:
drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_irq_handle_ibis():
} else if (IBI_TYPE_HJ(reg)) {
i3c_master_queue_hotjoin(&master->base);
}
Can this result in a use-after-free?
[Severity: High]
This is a pre-existing issue, but does this unconditionally cast the match
data pointer to an integer?
In drivers/i3c/master/dw-i3c-master.c:dw_i3c_common_probe():
if (has_acpi_companion(&pdev->dev)) {
quirks = (unsigned long)device_get_match_data(&pdev->dev);
} else if (pdev->dev.of_node) {
drvdata = device_get_match_data(&pdev->dev);
if (drvdata)
quirks = drvdata->flags;
}
If the device uses the PRP0001 ACPI-OF fallback, has_acpi_companion() will
be true, and device_get_match_data() will return a pointer to the OF table
struct dw_i3c_drvdata.
Does this cast the pointer address directly into the quirks bitmask, causing
unpredictable feature toggles?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624102153.1770072-1-akhilrajeev@nvidia.com?part=8
^ permalink raw reply
* Re: [PATCH v5 04/12] i3c: master: Add support for devices using SETAASA
From: sashiko-bot @ 2026-06-24 10:43 UTC (permalink / raw)
To: Akhil R; +Cc: linux-hwmon, devicetree, robh, linux-i3c, conor+dt, Frank.Li
In-Reply-To: <20260624102153.1770072-5-akhilrajeev@nvidia.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] SETAASA broadcast is sent before SETDASA direct assignments, which breaks initialization for devices that natively support SETAASA but are configured for SETDASA.
Pre-existing issues:
- [High] Missing RSTDAA on error path due to premature address slot clearing leads to bus address collision.
- [High] Double free and Use-After-Free of `desc->dev` when `device_register()` fails.
--
commit 08b7cd00b36fcaa4fe6292af27e273df6dc64b5a
Author: Akhil R <akhilrajeev@nvidia.com>
i3c: master: Add support for devices using SETAASA
Add support for devices using SETAASA attached to DDR5 memory modules that
do not support ENTDAA. Devices using SETAASA assign their pre-defined static
addresses as dynamic addresses during DAA.
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 4bba2bad897a7..3deae8cdae525 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
[ ... ]
> @@ -1878,6 +1924,22 @@ static int i3c_master_early_i3c_dev_add(struct i3c_master_controller *master,
> if (ret)
> goto err_free_dev;
>
> + /*
> + * For devices using SETAASA instead of ENTDAA, the address is statically
> + * assigned. Update the dynamic address to the provided static address.
> + * Reattach the I3C device after updating the dynamic address with the same
> + * static address. It is not mandatory for such devices to implement CCC
> + * commands like GETPID, GETDCR etc. Hence, we can return after reattaching.
> + */
> + if (i3cdev->boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> + i3cdev->info.dyn_addr = i3cdev->boardinfo->static_addr;
> + ret = i3c_master_reattach_i3c_dev_locked(i3cdev, 0);
> + if (ret)
> + goto err_rstdaa;
> +
> + return 0;
> + }
> +
[Severity: High]
This is a pre-existing issue, but missing RSTDAA on the error path due to
premature address slot clearing can lead to bus address collisions.
In i3c_master_early_i3c_dev_add(), the driver assigns the dynamic address
with SETDASA and reattaches the device. If reattaching fails, the error
path internally calls i3c_master_put_i3c_addrs(), which clears the
init_dyn_addr slot status back to I3C_ADDR_SLOT_FREE.
When the code jumps to err_rstdaa:
err_rstdaa:
i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);
It calls i3c_master_rstdaa_locked() to reset the device's dynamic address.
However, i3c_master_rstdaa_locked() verifies the slot status:
drivers/i3c/master.c:i3c_master_rstdaa_locked() {
...
if (addr != I3C_BROADCAST_ADDR && addrstat != I3C_ADDR_SLOT_I3C_DEV)
return -EINVAL;
...
}
Because the slot was just freed, the function silently returns without
sending the RSTDAA CCC command.
Will this allow the hardware device to continue squatting on the dynamic
address while the master considers the slot free, causing collisions later?
> ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
> i3cdev->boardinfo->init_dyn_addr);
[ ... ]
> @@ -2232,6 +2294,12 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> if (ret)
> goto err_bus_cleanup;
>
> + if (master->addr_method & I3C_ADDR_METHOD_SETAASA) {
> + ret = i3c_master_setaasa_locked(master);
> + if (ret)
> + goto err_bus_cleanup;
> + }
> +
[Severity: High]
Is it possible that sending the SETAASA broadcast before direct SETDASA
assignments breaks initialization for devices that natively support SETAASA
but are configured for SETDASA?
According to the I3C specification, any device on the bus natively supporting
SETAASA will respond to this broadcast by adopting its static address as its
dynamic address.
After this broadcast, the driver iterates through devices and attempts to
assign custom dynamic addresses via direct SETDASA commands:
drivers/i3c/master.c:i3c_master_early_i3c_dev_add() {
...
ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
i3cdev->boardinfo->init_dyn_addr);
...
}
Since the target device already adopted its dynamic address during the
SETAASA broadcast, it is no longer in the unassigned state and will NACK
the subsequent SETDASA command.
Could this cause device initialization to abort entirely?
Should SETDASA direct assignments be executed before broadcasting SETAASA?
> /*
> * Reserve init_dyn_addr first, and then try to pre-assign dynamic
> * address and retrieve device information if needed.
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but there is a potential double
free and use-after-free of desc->dev when device_register() fails.
In i3c_master_register_new_i3c_devs(), if device_register() fails,
put_device() is called:
drivers/i3c/master.c:i3c_master_register_new_i3c_devs() {
...
ret = device_register(&desc->dev->dev);
if (ret) {
dev_err(&master->dev, "Failed to add I3C device (err = %d)\n", ret);
put_device(&desc->dev->dev);
}
...
}
This drops the reference count to 0 and triggers i3c_device_release(), which
frees desc->dev via kfree(i3cdev). However, the pointer desc->dev is not set
to NULL and remains dangling in the master->bus.devs.i3c list.
Later, when the master module is unloaded or unregistered, the cleanup
iterates over the list:
drivers/i3c/master.c:i3c_master_unregister_i3c_devs() {
...
if (!i3cdev->dev)
continue;
i3cdev->dev->desc = NULL;
if (device_is_registered(&i3cdev->dev->dev))
device_unregister(&i3cdev->dev->dev);
else
put_device(&i3cdev->dev->dev);
i3cdev->dev = NULL;
...
}
Because i3cdev->dev is a dangling pointer, the check passes. This leads to
writing to i3cdev->dev->desc (use-after-free) and calling put_device() again
(double free).
Could we set desc->dev to NULL after put_device() in the registration error
path?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624102153.1770072-1-akhilrajeev@nvidia.com?part=4
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox