Re: [PATCH v4 2/4] thermal: qoriq: add i.MX93 tmu support

[Frank Li <Frank.li@nxp.com>]

On Fri, Sep 19, 2025 at 05:51:20PM +0200, Daniel Lezcano wrote:
> On 21/08/2025 08:23, Jacky Bai wrote:
> > For Thermal monitor unit(TMU) used on i.MX93, the HW revision info read
> > from the ID register is the same the one used on some of the QorIQ
> > platform, but the config has some slight differance. Add i.MX93 compatible
> > string and corresponding code for it.
> >
> > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > ---
> >   - v4 changes:
> >    - no
> >
> >   - v3 changes:
> >    - use the drv data struct for match data and refine the code
> >    - update the copyright
> >
> >   - v2 changes:
> >    - use the compatible match data to identify the i.MX93 TMU variant
> > ---
> >   drivers/thermal/qoriq_thermal.c | 18 +++++++++++++++++-
> >   1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
> > index 01b58be0dcc64d14ca5e4bba654eed8f15e827fc..b2e634547271dcf512c714907baa162921d2d527 100644
> > --- a/drivers/thermal/qoriq_thermal.c
> > +++ b/drivers/thermal/qoriq_thermal.c
> > @@ -1,6 +1,7 @@
> >   // SPDX-License-Identifier: GPL-2.0
> >   //
> >   // Copyright 2016 Freescale Semiconductor, Inc.
> > +// Copyright 2025 NXP
> >   #include <linux/clk.h>
> >   #include <linux/err.h>
> > @@ -24,6 +25,7 @@
> >   #define TMTMIR_DEFAULT	0x0000000f
> >   #define TIER_DISABLE	0x0
> >   #define TEUMR0_V2		0x51009c00
> > +#define TEUMR0_V21		0x55000c00
> >   #define TMSARA_V2		0xe
> >   #define TMU_VER1		0x1
> >   #define TMU_VER2		0x2
> > @@ -66,6 +68,8 @@
> >   						   */
> >   #define REGS_V2_TEUMR(n)	(0xf00 + 4 * (n))
> > +#define GET_TEUMR0(drvdata)	(drvdata && drvdata->teumr0 ? drvdata->teumr0 : TEUMR0_V2)
>
> This is not adequate for code which will evolve. Please don't use this
> macro.
>
> >   /*
> >    * Thermal zone data
> >    */
> > @@ -73,12 +77,17 @@ struct qoriq_sensor {
> >   	int				id;
> >   };
> > +struct tmu_drvdata {
> > +	u32 teumr0;
> > +};
> > +
> >   struct qoriq_tmu_data {
> >   	int ver;
> >   	u32 ttrcr[NUM_TTRCR_MAX];
> >   	struct regmap *regmap;
> >   	struct clk *clk;
> >   	struct qoriq_sensor	sensor[SITES_MAX];
> > +	const struct tmu_drvdata *drvdata;
>
> The drvdata pointer is not usually used.
>
> 	u32 model;

I think it is not true.  Copy model here is not extendable if need add
new field in drvdata in future, especially need ops callback for differece
variances.

Just list two examples:

https://elixir.bootlin.com/linux/v6.17-rc6/source/drivers/pci/controller/dwc/pci-imx6.c#L173
https://elixir.bootlin.com/linux/v6.17-rc6/source/drivers/pci/controller/dwc/pcie-qcom.c#L286

Frank

>
> >   };
> >   static struct qoriq_tmu_data *qoriq_sensor_to_data(struct qoriq_sensor *s)
> > @@ -234,7 +243,7 @@ static void qoriq_tmu_init_device(struct qoriq_tmu_data *data)
> >   		regmap_write(data->regmap, REGS_TMTMIR, TMTMIR_DEFAULT);
> >   	} else {
> >   		regmap_write(data->regmap, REGS_V2_TMTMIR, TMTMIR_DEFAULT);
> > -		regmap_write(data->regmap, REGS_V2_TEUMR(0), TEUMR0_V2);
> > +		regmap_write(data->regmap, REGS_V2_TEUMR(0), GET_TEUMR0(data->drvdata));
>
>
> 	regmap_write(data->regmap, REGS_V2_TEUMR(0), data->model);
...
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog