Re: [PATCH v2 07/16] thermal: mediatek: add PMIC thermal support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Incorrect error handling logic prevents the use of default calibration values and masks fatal hardware errors.
- [High] MT6323_TEMP_MAX is capped at 50°C, causing valid high temperatures to be discarded and breaking thermal mitigation.
- [High] Signed integer overflow in temperature calculation results in wildly negative temperatures under load.
- [High] Missing NULL check for the result of `device_get_match_data()` leads to a potential kernel panic on probe.
- [High] Missing Kconfig dependencies on the IIO subsystem.
- [Medium] NVMEM byte stream is incorrectly cast directly to a `u16` array, causing Big Endian read failures.
--

commit 7a6cbdb919c201e8e0890350ae5a7b867cecd588
Author: Roman Vivchar <rva333@protonmail.com>

thermal: mediatek: add PMIC thermal support

Add a new driver to support thermal monitoring on MediaTek PMICs, which
retrieves calibration data from EFUSE and registers with the thermal framework.

> diff --git a/drivers/thermal/mediatek/Kconfig b/drivers/thermal/mediatek/Kconfig
> index d82c86d9be563..8320d109fde68 100644
> --- a/drivers/thermal/mediatek/Kconfig
> +++ b/drivers/thermal/mediatek/Kconfig
> @@ -34,4 +34,16 @@ config MTK_LVTS_THERMAL_DEBUGFS
>         help
>           Enable this option to debug the internals of the device driver.
>  
> +config MTK_PMIC_THERMAL
> +	tristate "AUXADC temperature sensor driver for MediaTek PMICs"
> +	depends on MFD_MT6397

Should this also depend on IIO?

If the thermal driver is compiled as built-in (CONFIG_MTK_PMIC_THERMAL=y)
while IIO is compiled as a module (CONFIG_IIO=m), could the kernel build
fail during linking with unresolved symbols?

> +	help
> +	  Enable this option if you want to get PMIC temperature

[ ... ]

> diff --git a/drivers/thermal/mediatek/mtk_pmic_thermal.c b/drivers/thermal/mediatek/mtk_pmic_thermal.c
> new file mode 100644
> index 0000000000000..f644dad15fc2f
> --- /dev/null
> +++ b/drivers/thermal/mediatek/mtk_pmic_thermal.c
> @@ -0,0 +1,316 @@

[ ... ]

> +#define MAX_SENSORS			1
> +
> +#define MT6323_TEMP_MIN			(-20 * MILLIDEGREE_PER_DEGREE)
> +#define MT6323_TEMP_MAX			(50 * MILLIDEGREE_PER_DEGREE)

Is 50 degrees Celsius the intended maximum limit for MT6323_TEMP_MAX?

PMICs routinely operate well above 50 degrees under normal load, and thermal
mitigation trip points are typically set above 105 degrees.

Could this cap cause the driver to return -EINVAL for valid high temperatures,
effectively hiding overheating from the thermal subsystem?

[ ... ]

> +static int mtk_pmic_read_temp(struct thermal_zone_device *tz, int *temperature)
> +{
> +	struct mtk_pmic_sensor *sensor = thermal_zone_device_priv(tz);
> +	int ret, raw, temp;
> +
> +	ret = iio_read_channel_processed(sensor->adc_channel, &raw);
> +	if (ret < 0) {
> +		dev_err(sensor->mt->dev, "failed to read iio channel: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 *                 slope1 * V
> +	 * t = Intercept + ----------
> +	 *                   slope2
> +	 */
> +	temp = sensor->mt->t_intercept +
> +	       (sensor->mt->t_slope1 * raw) / sensor->mt->t_slope2;

Can the multiplication of mt->t_slope1 and raw overflow the signed 32-bit
integer here?

mt->t_slope1 is initialized to 100,000, and raw comes from an AUXADC
with a resolution up to 32,767. Any raw value exceeding 21,474 will
cause the multiplication to exceed 2,147,483,647.

Would it be better to use a 64-bit cast, such as div_s64(), to avoid
returning wrapped negative temperatures when the device is under load?

> +
> +	if (!mtk_pmic_thermal_temp_is_valid(temp))
> +		return -EINVAL;
> +
> +	*temperature = temp;
> +	return 0;
> +}

[ ... ]

> +static int mtk_pmic_thermal_extract_efuse_mt6323(struct mtk_pmic_thermal *mt,
> +						 u16 *buf)
> +{
> +	u32 reg;
> +	s32 vts, degc_cali, o_slope, o_slope_sign, id;
> +	int ret;
> +
> +	if (!FIELD_GET(CALIB_BUF0_ADC_CALI_EN_MASK, buf[0]))

Does buf need to be parsed with endian-safe helpers?

Since nvmem_cell_read() returns an opaque byte stream buffer, treating it
directly as a u16 array relies on the CPU byte order. On a big-endian kernel,
could this swap the bytes within each 16-bit word and corrupt the
calibration data?

> +		return -EINVAL;
> +
> +	/* Voltage offset */

[ ... ]

> +static int mtk_pmic_thermal_get_calib_data(struct device *dev,
> +					   struct mtk_pmic_thermal *mt)
> +{
> +	void *buf __free(kfree) = NULL;
> +	struct nvmem_cell *cell;
> +	size_t len;
> +	int ret;
> +
> +	cell = nvmem_cell_get(dev, NULL);
> +	if (IS_ERR(cell))
> +		return PTR_ERR(cell);

Does returning here bypass the fallback logic for uncalibrated devices?

If nvmem_cell_get() returns -ENOENT because the nvmem-cells property is
legitimately missing from the device tree, the probe is aborted before
applying the default calibration values.

> +
> +	buf = nvmem_cell_read(cell, &len);
> +	nvmem_cell_put(cell);
> +
> +	if (IS_ERR(buf)) {
> +		ret = PTR_ERR(buf);
> +		buf = NULL;
> +		return ret;
> +	}
> +
> +	if (len < 2 * sizeof(u16)) {
> +		dev_err(dev, "invalid calibration data length\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = mt->data->extract_efuse(mt, buf);
> +	if (ret) {
> +		dev_info(dev, "device not calibrated, using default values\n");

Conversely, if extract_efuse() encounters a hardware communication error
or an invalid chip ID, will this incorrectly mask the hardware error as
"device not calibrated"?

This might cause the driver to register the thermal zone using default values
on unresponsive hardware.

> +		mt->data->precalc(mt, MT6323_DEFAULT_VTS,
> +				  MT6323_DEFAULT_DEGC_CALI,
> +				  MT6323_DEFAULT_SLOPE,
> +				  MT6323_DEFAULT_SLOPE_SIGN);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int mtk_pmic_thermal_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_pmic_thermal *mt;
> +	int ret;
> +
> +	mt = devm_kzalloc(dev, sizeof(*mt), GFP_KERNEL);
> +	if (!mt)
> +		return -ENOMEM;
> +
> +	mt->regmap = dev_get_regmap(dev->parent->parent, NULL);
> +	if (!mt->regmap)
> +		return dev_err_probe(dev, -ENODEV, "failed to get regmap");
> +
> +	mt->dev = dev;
> +	mt->data = device_get_match_data(dev);

Should mt->data be checked for NULL?

If the driver is bound to the MFD cell purely by platform name (e.g., if
the MFD core does not pass an of_node to the cell), device_get_match_data()
will return NULL.

> +
> +	ret = mtk_pmic_thermal_get_calib_data(dev, mt);

If mt->data is NULL, calling mtk_pmic_thermal_get_calib_data() will
dereference a NULL pointer when it attempts to call
mt->data->extract_efuse(mt, buf).

> +	if (ret)
> +		return ret;
> +
> +	for (int i = 0; i < mt->data->num_sensors; i++) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-mt6323-v2-0-3efcba579e88@protonmail.com?part=7