Messages in this thread |  | | Date | Tue, 5 May 2026 11:16:16 +0300 | | From | Andy Shevchenko <> | | Subject | Re: [PATCH 06/13] thermal: mediatek: add pmic thermal support |
| |
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
|  |