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

[Pavel Machek <pavel@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

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?

> +	/*
> +	 * 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?

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

Best regards,
								Pavel
-- 
In cooperation with DENX Software Engineering GmbH, HRB 165235 Munich,
Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]