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

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

Am Mon, May 12, 2025 at 06:53:21AM -0700 schrieb Guenter Roeck:
> On 5/12/25 06:31, Dimitri Fedrau wrote:
> > 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 ?
> > 
> 
> Hardware monitoring is _expected_ to report the last latched status and clear
> it afterwards, to ensure that historic alarms are reported at least once.
> This isn't about "false positive", it is about "report at least once if
> possible".
>
Didn't know that, thanks for the explanation.

> Given that, the second read is unnecessary from hwmon ABI perspective. If you
> don't want to do that, you should explicitly document that latched (historic)
> over-temperature alarms are not reported.
> 
I would stick to hwmon ABI, just didn't know better.

Best regards,
Dimitri Fedrau