Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Francesco,

On 16.05.22 21:16, Francesco Dolcini wrote:
> On Mon, May 16, 2022 at 09:06:10PM +0200, Ahmad Fatoum wrote:
>> On 16.05.22 21:00, Francesco Dolcini wrote:
>>> Currently the imx thermal driver has a hardcoded critical temperature
>>> value offset of 5 Celsius degrees from the actual SoC maximum
>>> temperature.
>>>
>>> This affects applications and systems designed to be working on this close
>>> to the limit, but yet valid, temperature range.
>>>
>>> Given that there is no single value that will fit all the use cases make
>>> the critical trip point offset from the max temperature configurable
>>> using a newly added trip_offset module parameter, passive trip point is
>>> set to 5 Celsius degrees less than the critical. By default the
>>> system behaves exactly as before.
>>>
>>> Link: https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>>> ---
>>>  drivers/thermal/imx_thermal.c | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
>>> index 16663373b682..42d1f8a3eccb 100644
>>> --- a/drivers/thermal/imx_thermal.c
>>> +++ b/drivers/thermal/imx_thermal.c
>>> @@ -86,6 +86,10 @@ enum imx_thermal_trip {
>>>  #define TEMPMON_IMX6SX			2
>>>  #define TEMPMON_IMX7D			3
>>>  
>>> +static int trip_offset = 5;
>>> +module_param(trip_offset, int, 0444);
>>
>> Is this being r--r--r-- intended?
> 
> Yes, would you expect something more or less strict?
> 
> Daniel was pretty assertive that this is supposed to be a system
> property and not something that should be possible to mess around with
> [0]. Given this and that trip points are a+r in sysfs this was the most
> reasonable solution to me. Said that I do not mind changing
> this to 640 or 0440 or ...

I just found it odd, but with your explanation, it makes sense to have
this a read-only-after-init setting.

Thanks for the clarification,
Ahmad

> 
> [0] https://lore.kernel.org/all/4de41b5e-1fa6-ece4-9d9a-2656d399b452@linaro.org/
> 
> Francesco
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |