Re: [PATCH 6.12.y-cip 3/6] thermal/drivers/renesas/rzg3s: Add thermal driver for the Renesas RZ/G3S SoC

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Pavel,

Sorry for the late reply, I was off for a while.

On 12/29/25 22:21, Pavel Machek wrote:
> Hi!
> 
> I have some comments below.
> 
>> index 000000000000..d184dccc882f
>> --- /dev/null
>> +++ b/drivers/thermal/renesas/rzg3s_thermal.c
>> +static int rzg3s_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
>> +{
>> +	struct rzg3s_thermal_priv *priv = thermal_zone_device_priv(tz);
>> +	int ts_code_ave = 0;
>> +
>> +	if (priv->mode != THERMAL_DEVICE_ENABLED)
>> +		return -EAGAIN;
> 
> I'm pretty sure -EAGAIN is wrong error code here. -EBUSY or something?

-EAGAIN is something that the thermal core expects here in case the 
temperature reading is failing:

https://elixir.bootlin.com/linux/v6.18.3/source/drivers/thermal/thermal_core.c#L319

https://elixir.bootlin.com/linux/v6.18.3/source/drivers/thermal/thermal_sysfs.c#L46


> 
>> +	/*
>> +	 * According to the HW manual (Rev.1.10, section 40.4.4 Procedure for Measuring the
>> +	 * Temperature) the computation formula is as follows:
>> +	 *
>> +	 * Tj = (ts_code_ave - priv->calib1) * 165 / (priv->calib0 - priv->calib1) - 40
>> +	 *
>> +	 * Convert everything to milli Celsius before applying the formula to avoid
>> +	 * losing precision.
>> +	 */
>> +
>> +	*temp = div_s64((s64)(ts_code_ave - MCELSIUS(priv->calib1)) * MCELSIUS(165),
>> +			MCELSIUS(priv->calib0 - priv->calib1)) - MCELSIUS(40);
>> +
>> +	/* Report it in milli degrees Celsius and round it up to 0.5 degrees Celsius. */
>> +	*temp = roundup(*temp, 500);
> 
> If we did the computation in high precision, and interface wants
> millicelsius, is rounding good idea?

If I remember correctly, not adjusting the above code for precision lead 
to temperature variation at degree level not be detected. I chose to 
round it to 0.5 milli degrees to follow similar Renesas thermal drivers.

> 
>> +static void rzg3s_thermal_set_mode(struct rzg3s_thermal_priv *priv,
>> +				   enum thermal_device_mode mode)
>> +{
>> +	struct device *dev = priv->dev;
>> +	int ret;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret)
>> +		return;
> 
> Now caller does not know if we failed or not. Should this return errno?

Yes, this one could be improved to return the error code.

Thank you,
Claudiu