Re: [PATCH] thermal/drivers/mediatek/lvts_thermal: Make reset optional for MT8196

On 7/21/26 09:53, Philipp Zabel wrote:
> On Mo, 2026-07-20 at 16:42 +0200, AngeloGioacchino Del Regno wrote:
>> Depending on the SoC+Firmware combination, the LVTS hardware may be
>> may be actively used by one or even multiple concurrent MCUs!
>> In this case, resetting it may produce either a severe slowdown of
>> the entire system, or even a thermal protection AP reset, as some
>> MCU(s) may be reading a very high or very low temperature while the
>> LVTS is being reset.
>>
>> On those, don't fail if no reset is found as that may be omitted on
>> purpose, but still check if there's one, because some board(s) may
>> be running on a different bootchain with reduced firmwares or using
>> firmwares with reduced functionality.
>>
>> Add a new "optional_reset" member to lvts_data and use it to check
>> whether the resets should be mandatory or not.
>>
>> Also, while at it, since devm_reset_control_get_by_index() is now
>> deprecated, change the probe function to instead call function
>> devm_reset_control_get_exclusive_by_index(), which does the same.
> 
> Why is this using _by_index() at all? mediatek,lvts-thermal.yaml
> specifies a single reset, so the driver should just
> devm_reset_control_get_exclusive(dev, NULL).
> 
>>
>> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
>> ---
>>   drivers/thermal/mediatek/lvts_thermal.c | 29 ++++++++++++++++++++++---
>>   1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
>> index 92711896ce24..6670b5458f2c 100644
>> --- a/drivers/thermal/mediatek/lvts_thermal.c
>> +++ b/drivers/thermal/mediatek/lvts_thermal.c
>> @@ -160,6 +160,7 @@ struct lvts_data {
>>   	int gt_calib_bit_offset;
>>   	unsigned int def_calibration;
>>   	u16 msr_offset;
>> +	bool optional_reset;
> 
> No need to complicate things. It's not the driver's business to check
> device tree correctness. Just make the reset optional, as the commit
> message says.
> 
>>   };
>>   
>>   struct lvts_sensor {
>> @@ -1473,9 +1474,29 @@ static int lvts_probe(struct platform_device *pdev)
>>   	if (IS_ERR(lvts_td->base))
>>   		return dev_err_probe(dev, PTR_ERR(lvts_td->base), "Failed to map io resource\n");
>>   
>> -	lvts_td->reset = devm_reset_control_get_by_index(dev, 0);
>> -	if (IS_ERR(lvts_td->reset))
>> -		return dev_err_probe(dev, PTR_ERR(lvts_td->reset), "Failed to get reset control\n");
>> +	/*
>> +	 * Depending on the SoC+Firmware combination, the LVTS hardware may be
>> +	 * may be actively used by one or even multiple concurrent MCUs!
>> +	 * In this case, resetting it may produce either a severe slowdown of
>> +	 * the entire system, or even a thermal protection AP reset, as some
>> +	 * MCU(s) may be reading a very high or very low temperature while the
>> +	 * LVTS is being reset.
>> +	 *
>> +	 * On those, don't fail if no reset is found as that may be omitted on
>> +	 * purpose, but still check if there's one, because some board(s) may
>> +	 * be running on a different bootchain with reduced firmwares or using
>> +	 * firmwares with reduced functionality.
>> +	 */
>> +	lvts_td->reset = devm_reset_control_get_exclusive_by_index(dev, 0);
> 
> So this should be:
> 
> 	lvts_td->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
> 
>> +	if (IS_ERR(lvts_td->reset)) {
>> +		if (lvts_data->optional_reset) {
>> +			dev_dbg(dev, "No reset found. LVTS may be used by firmware.\n");
>> +			lvts_td->reset = NULL;
> 
> This is not necessary then.
> 
>> +		} else {
>> +			return dev_err_probe(dev, PTR_ERR(lvts_td->reset),
>> +					     "Failed to get reset control\n");
>> +		}
> 
> And this part could be kept as before.
> 
>> +	}
>>   
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq < 0)
>> @@ -2163,6 +2184,7 @@ static const struct lvts_data mt8196_lvts_mcu_data = {
>>   	.num_cal_offsets = LVTS_NUM_CAL_OFFSETS_MT8196,
>>   	.msr_offset = LVTS_MSR_OFFSET_MT8196,
>>   	.ops = &lvts_platform_ops_mt8196,
>> +	.optional_reset = true,
> 
> But mediatek,lvts-thermal.yaml still specifies the reset as required
> for mediatek,mt8196-lvts-ap. That should be changed instead.
> 
>>   };
>>   
>>   static const struct lvts_data mt8196_lvts_ap_data = {
>> @@ -2175,6 +2197,7 @@ static const struct lvts_data mt8196_lvts_ap_data = {
>>   	.num_cal_offsets = LVTS_NUM_CAL_OFFSETS_MT8196,
>>   	.msr_offset = LVTS_MSR_OFFSET_MT8196,
>>   	.ops = &lvts_platform_ops_mt8196,
>> +	.optional_reset = true,
> 
> Same as above, but for mediatek,mt8196-lvts-mcu.
> 

Ack. Will send a v2 shortly.

Thanks,
Angelo