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

Andy Shevchenko
2026-05-05T08:16:24Z

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