Re: [PATCH 06/13] thermal: mediatek: add pmic thermal support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, May 04, 2026 at 09:24:58PM +0300, Roman Vivchar via B4 Relay wrote:

> Add a new driver to support thermal monitoring on MediaTek PMICs.
> 
> The driver retrieves calibration data from EFUSE, calculates the
> temperature using a linear interpolation, and registers the device with
> the thermal framework.
> 
> Initial support is added for the mt6323 PMIC.

First of all, please take into account the comments given against previous
patches.

...

> +config MTK_PMIC_THERMAL
> +	tristate "AUXADC temperature sensor driver for MediaTek PMICs"
> +	depends on MFD_MT6397
> +	help
> +	  Enable this option if you want to get PMIC temperature
> +	  information for MediaTek platforms.
> +	  This driver configures thermal controllers to collect
> +	  temperature via AUXADC interface.

Don't we want to tell user how the module will be called in case of M choice?

...

> +#include <linux/err.h>
> +#include <linux/iio/consumer.h>

> +#include <linux/kernel.h>

No way your driver uses this header.

> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>

Please, follow IWYU.

> +#include <linux/mfd/mt6323/registers.h>

...

> +#define MT6323_TEMP_MIN			-20000
> +#define MT6323_TEMP_MAX			150000

Don't you want to use units.h multipliers?

...

> +/* Layout of the fuses providing the calibration data */
> +#define CALIB_BUF0_VTS(x)		(((x) >> 8) & 0xff)
> +#define CALIB_BUF0_DEGC_CALI(x)		(((x) >> 2) & 0x3f)
> +#define CALIB_BUF0_ADC_CALI_EN(x)	(((x) >> 1) & 0x1)
> +
> +#define CALIB_BUF1_ID_20(x)		(((x) >> 14) & 0x1)
> +#define CALIB_BUF1_ID_10(x)		(((x) >> 12) & 0x1)
> +#define CALIB_BUF1_O_SLOPE_20(x)	(((((x) >> 11) & 0x7) << 3) + (((x) >> 6) & 0x7))
> +#define CALIB_BUF1_O_SLOPE_10(x)	(((x) >> 6) & 0x3f)
> +#define CALIB_BUF1_O_SLOPE_SIGN(x)	(((x) >> 5) & 0x1)
> +#define CALIB_BUF1_VTS(x)		((((x) >> 0) & 0x1f) << 8)

Why not use BIT() and GENMASK() as appropriate? But better, why not use
bitfield.h APIs?

...

> +struct mtk_thermal_data {
> +	const char *const *sensors;
> +	s32 num_sensors;

> +	const int cali_val;

What exactly does 'const' benefit here with?

> +	int (*extract_efuse)(struct mtk_pmic_thermal *mt, u16 *buf);
> +	void (*precalc)(struct mtk_pmic_thermal *mt, s32 vts, s32 degc_cali,
> +			s32 o_slope, s32 o_slope_sign);
> +};

...

> +struct mtk_pmic_sensor {
> +	struct mtk_pmic_thermal *mt;
> +	int id;
> +	struct iio_channel *adc_channel;
> +	struct thermal_zone_device *tzdev;
> +};

Can you confirm with `pahole` that this is the best layout (taking into account
the use in the below data structure)?

> +struct mtk_pmic_thermal {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct mtk_pmic_sensor sensors[MAX_SENSORS];
> +
> +	s32 t_slope1;
> +	s32 t_slope2;
> +	s32 t_intercept;
> +
> +	const struct mtk_thermal_data *data;
> +};

...

> +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;
> +	}
> +
> +	temp = sensor->mt->t_intercept +
> +	       ((sensor->mt->t_slope1 * raw) / sensor->mt->t_slope2);

Too many parentheses.

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

...

> +static void mtk_pmic_thermal_precalc_mt6323(struct mtk_pmic_thermal *mt,
> +					    s32 vts, s32 degc_cali, s32 o_slope,
> +					    s32 o_slope_sign)
> +{
> +	s32 vbe_t;
> +
> +	mt->t_slope1 = 100 * 1000;

magic1 * magic2...

> +	if (o_slope_sign == 0)
> +		mt->t_slope2 = -(mt->data->cali_val + o_slope);
> +	else
> +		mt->t_slope2 = -(mt->data->cali_val - o_slope);
> +
> +	vbe_t = -1 * (((vts + 9102) * 1800) / 32768) * 1000;

More magics...

> +	if (o_slope_sign == 0)
> +		mt->t_intercept =
> +			(vbe_t * 100) / -(mt->data->cali_val + o_slope);
> +	else
> +		mt->t_intercept =
> +			(vbe_t * 100) / -(mt->data->cali_val - o_slope);
> +
> +	mt->t_intercept += (degc_cali * (1000 / 2));

Too many parentheses.

> +}

This entire function misses a lot of comments and formulas with the references
to the datasheet.

...

> +{
> +	struct nvmem_cell *cell;
> +	void *buf;
> +	size_t len;
> +	int ret;
> +
> +	cell = nvmem_cell_get(dev, "calibration-data");
> +	if (IS_ERR(cell))
> +		return PTR_ERR(cell);
> +
> +	buf = nvmem_cell_read(cell, &len);
> +	nvmem_cell_put(cell);
> +
> +	if (IS_ERR(buf))
> +		return PTR_ERR(buf);
> +
> +	if (len < 2 * sizeof(u16)) {
> +		dev_warn(dev, "invalid calibration data length\n");

And? Is it fatal error or not?

> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = mt->data->extract_efuse(mt, buf);
> +	if (ret) {
> +		dev_info(dev, "device not calibrated, using default values\n");
> +		mt->data->precalc(mt, 3698, 50, 0, 0);
> +		ret = 0;
> +	}

> +out:
> +	kfree(buf);
> +	return ret;

Why not use __free() from cleanup.h?

> +}

...

> +static int mtk_pmic_thermal_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_pmic_thermal *mt;
> +	int i, 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 = of_device_get_match_data(dev);

property.h instead of of.h and use just device_get_match_data().

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

> +	for (i = 0; i < mt->data->num_sensors; i++) {

'i' is not used outside the loop:

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

> +		struct mtk_pmic_sensor *sensor = &mt->sensors[i];
> +
> +		sensor->id = i;
> +		sensor->mt = mt;
> +
> +		sensor->adc_channel =
> +			devm_iio_channel_get(dev, mt->data->sensors[i]);
> +		if (IS_ERR(sensor->adc_channel))
> +			return dev_err_probe(dev, PTR_ERR(sensor->adc_channel),
> +					     "failed to get channel %s\n",
> +					     mt->data->sensors[i]);

> +		sensor->tzdev = devm_thermal_of_zone_register(
> +			dev, i, sensor, &mtk_pmic_thermal_ops);
> +		if (IS_ERR(sensor->tzdev))

> +			return dev_err_probe(
> +				dev, PTR_ERR(sensor->tzdev),

What a broken indentation in the whole stanza...

> +				"failed to register thermal zone %d\n", i);
> +	}

> +	return 0;
> +}

...

> +static const struct of_device_id mtk_pmic_thermal_of_match[] = {
> +	{ .compatible = "mediatek,mt6323-thermal",
> +	  .data = &mt6323_thermal_data },

> +	{ /* sentinel */ },

Nonsense trailing comma.

> +};

-- 
With Best Regards,
Andy Shevchenko