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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 19/09/2025 18:25, Frank Li wrote:
> 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

Yeah may be I'm doing a confusion with a previous comment I had related 
to this approach




-- 
<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