Re: [PATCH v3 3/9] thermal: renesas: rzg3e: make calibration value retrieval per-chip

[Geert Uytterhoeven]

Hi Cosmin,

On Wed, 26 Nov 2025 at 14:05, Cosmin Tanislav
<cosmin-gabriel.tanislav.xa@xxxxxxxxxxx> wrote:
> The Renesas RZ/T2H (R9A09G077) and RZ/N2H (R9A09G087) SoCs expose the
> temperature calibration data via SMC SIP calls.
>
> Prepare for them by moving the syscon usage into a single function, and
> placing it in the chip-specific struct.
>
> Rename the functions to match their functionality, and remove single-use
> variables from the private state.
>
> Also, move the calibration value mask into a macro.
>
> Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@xxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/thermal/renesas/rzg3e_thermal.c
> +++ b/drivers/thermal/renesas/rzg3e_thermal.c
> @@ -70,7 +70,12 @@
>  #define TSU_POLL_DELAY_US      10      /* Polling interval */
>  #define TSU_MIN_CLOCK_RATE     24000000  /* TSU_PCLK minimum 24MHz */
>
> +#define TSU_TEMP_MASK          GENMASK(11, 0)

This is the value same as the existing TSU_CODE_MAX definition.

> +
> +struct rzg3e_thermal_priv;
> +
>  struct rzg3e_thermal_info {
> +       int (*get_trim)(struct rzg3e_thermal_priv *priv);
>         int temp_d_mc;
>         int temp_e_mc;
>  };

> @@ -334,22 +337,8 @@ static const struct thermal_zone_device_ops rzg3e_tz_ops = {
>         .set_trips = rzg3e_thermal_set_trips,
>  };
>
> -static int rzg3e_thermal_get_calibration(struct rzg3e_thermal_priv *priv)
> +static int rzg3e_validate_calibration(struct rzg3e_thermal_priv *priv)
>  {
> -       u32 val;
> -       int ret;
> -
> -       /* Read calibration values from syscon */
> -       ret = regmap_read(priv->syscon, priv->trim_offset, &val);
> -       if (ret)
> -               return ret;
> -       priv->trmval0 = val & GENMASK(11, 0);
> -
> -       ret = regmap_read(priv->syscon, priv->trim_offset + 4, &val);
> -       if (ret)
> -               return ret;
> -       priv->trmval1 = val & GENMASK(11, 0);
> -
>         /* Validate calibration data */

I think this comment can be dropped, as this is clear from the
function name.

>         if (!priv->trmval0 || !priv->trmval1 ||
>             priv->trmval0 == priv->trmval1 ||

> @@ -402,11 +404,16 @@ static int rzg3e_thermal_probe(struct platform_device *pdev)
>         if (IS_ERR(priv->base))
>                 return PTR_ERR(priv->base);
>
> -       /* Parse device tree for trim register info */
> -       ret = rzg3e_thermal_parse_dt(priv);
> +       ret = priv->info->get_trim(priv);
>         if (ret)
>                 return ret;
>
> +       /* Validate calibration data */

Obvious, so please drop this comment.

> +       ret = rzg3e_validate_calibration(priv);
> +       if (ret)
> +               return dev_err_probe(dev, ret,
> +                                    "Failed to get valid calibration data\n");

rzg3e_validate_calibration() already prints its own error message.
As this function became small, perhaps inline it here, and use a single
dev_err_probe() in case of failure?

> +
>         /* Get clock to verify frequency - clock is managed by power domain */
>         clk = devm_clk_get(dev, NULL);
>         if (IS_ERR(clk))

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds