Re: [PATCH] pwm: mc33xs2410: add support for temperature sensors

[Dimitri Fedrau <dima.fedrau@gmail.com>]

Hi Guenter,

Am Mon, May 12, 2025 at 06:04:33AM -0700 schrieb Guenter Roeck:
> On 5/12/25 04:26, Dimitri Fedrau via B4 Relay wrote:
> > From: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
> > 
> > The MC33XS2410 provides temperature sensors for the central die temperature
> > and the four outputs. Additionally a common temperature warning threshold
> > can be configured for the outputs. Add hwmon support for the sensors.
> > 
> > Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
> > ---
> 
> > +
> > +static int mc33xs2410_hwmon_read_out_status(struct spi_device *spi,
> > +					    int channel, u16 *val)
> > +{
> > +	int ret;
> > +
> > +	ret = mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Bits latches high */
> > +	return mc33xs2410_read_reg_diag(spi, MC33XS2410_OUT_STA(channel), val);
> 
> Is that double read of the same register needed ? If so, you'll probably
> need a lock to prevent it from being executed from multiple threads at the
> same time.
> 
> The comment "Bit latches high" doesn't really mean anything to me and doesn't
> explain why the register needs to be read twice.
> 
>

All bits of the output status registers are latched high. In case there
was overtemperature detected, the bit stays set until read once and cleared
afterwards. So I need a second read to get the "realtime" status.
Otherwise I might end up returning an false positive overtemperature
warning. I don't think a lock is really necessary, since I'm only
interested in the "realtime" status but not if there was a warning in
the past. What do you think ?

Will improve the comment.

Best regards
Dimitri Fedrau