Re: [PATCH V10 4/4] thermal: qcom: add support for PMIC5 Gen3 ADC thermal monitoring

[Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>]

Hi Daniel,

On 4/27/2026 1:48 PM, Daniel Lezcano wrote:
> 
> Hi Jishnu,
> 
> On 4/25/26 12:37, Jishnu Prakash wrote:
>> Hi Daniel,
> 
> [ ... ]
> 
> 

...

> 
> [ ... ]
> 
>>> [ ... ]
>>>
>>>>>> +    dev_dbg(adc_tm5->dev, "channel:%s, low_temp(mdegC):%d, high_temp(mdegC):%d\n",
>>>>>> +        prop->common_props.label, low_temp, high_temp);
>>>>>> +
>>>>>> +    guard(adc5_gen3)(adc_tm5);
>>>>>> +    if (high_temp == INT_MAX && low_temp == -INT_MAX)
>>>>>> +        return adc_tm5_gen3_disable_channel(prop);
>>>>>
>>>>> Why disable the channel instead of returning an errno ?
>>>>>
>>>>
>>>> This is the convention we follow in our existing ADC_TM driver at
>>>> drivers/thermal/qcom/qcom-spmi-adc-tm5.c. If both upper and lower
>>>> thresholds are meant to be disabled, we disable the channel fully
>>>> in HW to save some power and it can be enabled later if this API
>>>> is called for it with valid thresholds.
>>>>
>>>> Is it considered invalid in the thermal framework to try to disable
>>>> both thresholds? Should I both disable the channel and return some
>>>> error from here?
>>>
>>> Well, if the channel is disabled, then the temperature sensor of the thermal zone is disabled, consequently the thermal zone is disabled from a HW POV but enabled from the kernel POV.
>>>
>>> Why not add the 'change_mode' ops and then disable the thermal zone (+ pm_runtime) ?
>>>
>>
>> I have a doubt about this part - if the thermal framework sends threshold values
>> of (-INT_MAX, INT_MAX) in the .set_trips callback, doesn't it mean that the
>> framework is already trying to disable the thermal sensor?
> 
> No, let me clarify this:
> 
>  * the thermal thresholds are set by the userspace to get notification when a temperature is crossing the way up or/and down a specified limit. Thresholds can be deleted, added, flushed, etc ...
> 
>  * the thermal trips are specified by the firmware and should not be changed even if I agree there are the writable trip points which IMO we can consider for debug purpose
> 
> If you specify a trip/threshold point with min=-INT_MAX and max=INT_MAX, semantically speaking it is correct but in practice these temperatures won't be reached so it is like the trip point is disabled but it is a side effect.
> 
> A trip point is a property of a thermal zone.
> 
> A thermal sensor is represented by a thermal zone.
> 
> A thermal zone can be enabled without trip points or thresholds. The userspace is still able to read a temperature through netlink or sysfs.
> 

Thanks for your explanation, I understand the convention now.

>> Or does it just mean threshold monitoring functionality can be disabled for now,
>> but other functionality like temperature reading is still expected to work?
>>
>> Please note that adc_tm5_gen3_disable_channel() only disables the threshold
>> monitoring and interrupt generation functionality - the get_temp() call to read
>> temperature will still work.
> 
> Above you stated we receive a notification when the conversion is finished. So how do you read the temperature if there is no interrupt telling to read the converted value ?
> 

I think you are talking about the immediate conversion here, which is called in get_temp() -
let me try explaining with an example, to avoid missing anything again.

On an ADC SDAM, we have 8 channels which can be used for either immediate read or
ADC_TM functionality. So by convention, we reserve the first channel of the first
SDAM for *all* immediate reads and use all the remaining channels for ADC_TM.

So if we have SDAM 0 and SDAM 1 used for ADC, channel 0 of SDAM 0 is used for all
immediate conversions and channels 1-7 of SDAM 0 and channels 0-7 of SDAM 1 can
all be used for ADC_TM functionality.

Now if channel 5 of SDAM 0 is used to monitor the xo-therm ADC thermistor and we call
adc_tm5_gen3_disable_channel() on it, threshold monitoring and interrupt generation
from channel 5 alone would be stopped, but it would not change for other channels.

So if we wanted to read the xo-therm thermistor's temperature through the get_temp()
callback, it would still work, as calling get_temp() results in an immediate
conversion request done through channel 0 of SDAM 0 to read this thermistor's temperature.


>> Perhaps I should not have used the wording
>> "disable the channel" above, as calling adc_tm5_gen3_disable_channel() is not
>> exactly the same as fully disabling the thermal zone.
>>
>> Please let me know if any change is needed there - should I return any error
>> after calling adc_tm5_gen3_disable_channel() ?
> 
> I understand, your point. Perhaps just put it apart and address it later as an optimization when the driver is merged and everything is clarified.
> 

OK, I'll do as you suggest. For now, I'll just drop this special case check:

        if (high_temp == INT_MAX && low_temp == -INT_MAX)

and try adding it in a separate change later.

Thanks,
Jishnu


>>> [ ... ]
>>>
>>>>>> +    /*
>>>>>> +     * Skipping first SDAM IRQ as it is requested in parent driver.
>>>>>> +     * If there is a TM violation on that IRQ, the parent driver calls
>>>>>> +     * the notifier (adctm_event_handler) exposed from this driver to handle it.
>>>>>> +     */
>>>>>> +    for (i = 1; i < adc_tm5->dev_data->num_sdams; i++) {
>>>>>> +        ret = devm_request_threaded_irq(dev,
>>>>>> +                        adc_tm5->dev_data->base[i].irq,
>>>>>> +                        NULL, adctm5_gen3_isr, IRQF_ONESHOT,
>>>>>> +                        adc_tm5->dev_data->base[i].irq_name,
>>>>>> +                        adc_tm5);
>>>>>
>>>>> The threaded interrupts set the isr in a thread and from the thread
>>>>> handling the event, there is a work queue scheduled. Why not use the
>>>>> top and bottom halves of the threaded interrupt ? Hopefully you should
>>>>> be able to remove the lock.
>>>>
>>>> Yes, I can use the top and bottom halves of the threaded interrupt as you
>>>> suggested. But what exactly do you mean by removing the lock?
>>>>
>>>> If you meant the mutex lock used in this driver, we cannot remove that.
>>>> This is because the ADC_TM driver needs to write into several registers
>>>> shared with the main ADC driver for setting new thresholds, so we
>>>> have to share a mutex between the drivers to prevent concurrency issues.
>>>
>>> When using a workqueue tampering with registers while an interrupt handler is doing the same, the lock is needed.
>>>
>>> But if the workqueue is replaced by threaded interrupt, the lock *may* not be needed because the design may prevent race conditions.
>>>
>>> That may be not true in this case, I did not investigate deeper in the code to figure it out. Let's see the next version
>>>
>>
>> I think a lock will not be needed with the change I planned, but you
>> can judge it in the next version.
> 
> Sure, thanks
> 
>   -- Daniel