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

[Guenter Roeck <linux@roeck-us.net>]

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".

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.

Guenter