Re: [PATCH 1/5] thermal/drivers/mediatek/lvts: Disable monitor mode during suspend

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 26/11/24 14:19, Nícolas F. R. A. Prado ha scritto:
> On Tue, Nov 26, 2024 at 10:43:55AM +0100, AngeloGioacchino Del Regno wrote:
>> Il 25/11/24 22:20, Nícolas F. R. A. Prado ha scritto:
>>> When configured in filtered mode, the LVTS thermal controller will
>>> monitor the temperature from the sensors and trigger an interrupt once a
>>> thermal threshold is crossed.
>>>
>>> Currently this is true even during suspend and resume. The problem with
>>> that is that when enabling the internal clock of the LVTS controller in
>>> lvts_ctrl_set_enable() during resume, the temperature reading can glitch
>>> and appear much higher than the real one, resulting in a spurious
>>> interrupt getting generated.
>>>
>>> Disable the temperature monitoring and give some time for the signals to
>>> stabilize during suspend in order to prevent such spurious interrupts.
>>>
>>> Cc: stable@vger.kernel.org
>>> Reported-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
>>> Closes: https://lore.kernel.org/all/20241108-lvts-v1-1-eee339c6ca20@chromium.org/
>>> Fixes: 8137bb90600d ("thermal/drivers/mediatek/lvts_thermal: Add suspend and resume")
>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>>> ---
>>>    drivers/thermal/mediatek/lvts_thermal.c | 36 +++++++++++++++++++++++++++++++--
>>>    1 file changed, 34 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
>>> index 1997e91bb3be94a3059db619238aa5787edc7675..a92ff2325c40704adc537af6995b34f93c3b0650 100644
>>> --- a/drivers/thermal/mediatek/lvts_thermal.c
>>> +++ b/drivers/thermal/mediatek/lvts_thermal.c
>>> @@ -860,6 +860,32 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
>>>    	return 0;
>>>    }
>>> +static void lvts_ctrl_monitor_enable(struct device *dev, struct lvts_ctrl *lvts_ctrl, bool enable)
>>> +{
>>> +	/*
>>> +	 * Bitmaps to enable each sensor on filtered mode in the MONCTL0
>>> +	 * register.
>>> +	 */
>>> +	u32 sensor_filt_bitmap[] = { BIT(0), BIT(1), BIT(2), BIT(3) };
>>> +	u32 sensor_map = 0;
>>> +	int i;
>>> +
>>> +	if (lvts_ctrl->mode != LVTS_MSR_FILTERED_MODE)
>>> +		return;
>>> +
>>
>> That's easier and shorter:
>>
>> static void lvts_ctrl_monitor_enable( .... )
>> {
>> 	/* Bitmap to enable each sensor on filtered mode in the MONCTL0 register */
>> 	const u32 sensor_map = GENMASK(3, 0);
>>
>> 	if (lvts_ctrl->mode != LVTS_MSR_FILTERED_MODE)
>> 		return;
>>
>> 	/* Bits 0-3: Sensing points - Bit 9: Single point access flow */
>> 	if (enable)
>> 		writel(sensor_map | BIT(9), LVTS_MONCTL0(lvts_ctrl->base));
> 
> Wait, no, here you're enabling all the sensors in the controller. We only want
> to enable ones that are valid, otherwise we might get garbage data and irqs from
> sensors that aren't actually there. That's why I use the
> lvts_for_each_valid_sensor() helper in this patch.
> 

Whoa, my brain actually missed the lvts_for_each_valid_sensor()!

Okay no, then you're right - sorry for the bad example! In that case, though, I
still have one more comment.

You can constify sensor_filt_bitmap, and since the values never go higher than
BIT(3), you should also be able to spare some memory by turning that into a u8:

	const u8 sensor_filt_bitmap[] = { BIT(0), BIT(1), BIT(2), BIT(3) };

...and then I assume that there's no way valid sensors could ever read from an
index that is more than 4 (so, I assume that there's no way the loop tries to
read out of the array upper boundary).

In which case - after at least constifying the sensor_filt_bitmap array, for v2
feel free to add my

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

...and sorry again for the initial miss :-)

Cheers,
Angelo