Hi Danoel,
> -----Original Message-----
> From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> Sent: Monday, August 4, 2025 6:08 PM
> To: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH v6 3/5] thermal: renesas: rzg3e: Add thermal driver for
> the Renesas RZ/G3E SoC
>
> On 31/07/2025 19:19, John Madieu wrote:
> > Hi Daniel,
> >
> > Thanks for your review.
> >
> >> -----Original Message-----
> >> From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> >> Sent: Wednesday, July 16, 2025 11:11 PM
> >> To: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> >> Subject: Re: [PATCH v6 3/5] thermal: renesas: rzg3e: Add thermal
> >> driver for the Renesas RZ/G3E SoC
> >>
> >> On Thu, May 22, 2025 at 08:22:46PM +0200, John Madieu wrote:
> >>> The RZ/G3E SoC integrates a Temperature Sensor Unit (TSU) block
> >>> designed to monitor the chip's junction temperature. This sensor is
> >>> connected to channel 1 of the APB port clock/reset and provides
> >> temperature measurements.
> >>>
> >>> It also requires calibration values stored in the system controller
> >>> registers for accurate temperature measurement. Add a driver for the
> >> Renesas RZ/G3E TSU.
> >>>
> >>> Signed-off-by: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> >>> ---
>
> [ ... ]
>
> >>> +static int rzg3e_thermal_get_temp(struct thermal_zone_device *zone,
> >>> +int *temp) {
> >>> + struct rzg3e_thermal_priv *priv = thermal_zone_device_priv(zone);
> >>> + u32 val;
> >>> + int ret;
> >>> +
> >>> + if (priv->mode == THERMAL_DEVICE_DISABLED)
> >>> + return -EBUSY;
>
> [ ... ]
>
> >>> + reinit_completion(&priv->conv_complete);
> >>> +
> >>> + /* Enable ADC interrupt */
> >>> + writel(TSU_SIER_ADIE, priv->base + TSU_SIER);
> >>
> >> Why enable irq here ?
> >>
> >
> > I did it this way because, in 'set_trips' callback, the driver does
> > trigger conversion to check whether the current temperature is part of
> > the window or not, and triggers the comparison interrupt accordingly.
> > Because of that, I did not want the conversion-complete interrupt to
> > also be triggered.
> >
> > That's the reason why I enable conversion-complete interrupt in
> > 'get_temp', to make sure its interrupt is being triggered only when
> > the thermal core calls it.
> >
> > Should I do it another way ?
>
> I don't ATM, the approach is very unusual so I'm still trying to figure out
> what is this completion approach and readl_poll_timeout_atomic. At the
> first glance I would say it is wrong.
>
>
> >>> + /* Verify no ongoing conversion */
> >>> + ret = readl_poll_timeout_atomic(priv->base + TSU_SSR, val,
> >>> + !(val & TSU_SSR_CONV_RUNNING),
> >>> + TSU_POLL_DELAY_US, TSU_TIMEOUT_US);
> >>> + if (ret) {
> >>> + dev_err(priv->dev, "ADC conversion timed out\n");
> >>> + return ret;
> >>> + }
> >>> +
> >>> + /* Start conversion */
> >>> + writel(TSU_STRGR_ADST, priv->base + TSU_STRGR);
> >>> +
> >>> + if (!wait_for_completion_timeout(&priv->conv_complete,
> >>> + msecs_to_jiffies(100))) {
> >>> + dev_err(priv->dev, "ADC conversion completion timeout\n");
> >>> + return -ETIMEDOUT;
> >>> + }
> >>
> >> Can you explain what is happening here ?
> >>
> >
> > I might not get what you are asking, but since I compute the
> > temperature in the hard IRQ handler, I just wait for it to complete
> > and notify the completion so I can grab the processed value to notify
> > the thermal core.
> >
> > Please let me know if this does not answer your question.
>
> Can you describe how the sensor works ? And perhaps if you have a pointer
> to some documentation ?
Here is the documentation [1]. The thermal device is referred to as TSU.
[1] https://www.renesas.com/en/document/mah/rzg3e-group-users-manual-hardware?r=25574493
> [ ... ]
Regards,
John
