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

[Daniel Lezcano]

Hi Jishnu,

On 4/25/26 12:37, Jishnu Prakash wrote:
> Hi Daniel,

[ ... ]


> > Actually, if get_sdam_from_irq() or adc5_gen3_read() fail, they will return without clearing the interrupt flag, so we should potentially end up in an infinite loop.
>
> I went over this discussion and the code again and I need to explain a few things.
>
> First, please note that individual PMIC drivers do not handle interrupt clearing by
> themselves - they are under the SPMI PMIC arbiter (drivers/spmi/spmi-pmic-arb.c),
> which is an interrupt controller and the arbiter driver handles IRQ clearing internally
> for all the PMIC drivers under it. So we won't be clearing the interrupt explicitly here.
>
> > So the status should be cleared at the end with IRQ_HANDLED. IRQ_NONE returned if it is for another subsystem.
>
> Maybe my explanation above wasn't sufficiently detailed - the "status" I mentioned
> above is not related to the interrupt by itself, it is just the ADC's status register
> reading. If it indicates an ADC conversion fault, the fault has to be cleared by
> calling the function adc5_gen3_status_clear() before we can make subsequent conversion
> requests (like for .set_trips) on the ADC.
>
>
> There are 3 main reasons for which the Gen3 ADC interrupt may fire:
>
> 1. End of conversion for immediate conversion request (handled in main ADC driver)
> 2. Threshold violation for TM conversions (handled in this auxiliary driver)
> 3. Conversion fault (handled in both drivers)
>
>
> I think it would make the most sense if I make changes so that the ISRs
> (of both main and aux driver) return IRQ_HANDLED only when any of the above
> 3 cases are detected and they return IRQ_NONE in case of errors or when the
> interrupt is confirmed to be for the other driver.
>
> Does this sound fine?

Thanks for clarifying. It should be fine.

> > If you think there can be a significant number of errors in the handler may be you should add statistics but later in an additional series if it makes sense.

[ ... ]

> > [ ... ]
> >
> > > > > +    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.

> 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 ?

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

> > [ ... ]
> >
> > > > > +    /*
> > > > > +     * 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