Re: [PATCH v2 2/2] thermal: qcom: add support for PMIC5 Gen3 ADC thermal monitoring

[Jishnu Prakash]

On 5/27/2026 5:12 PM, Jonathan Cameron wrote:
> On Tue, 26 May 2026 16:26:10 +0530
> Jishnu Prakash <jishnu.prakash@xxxxxxxxxxxxxxxx> wrote:
> 
>> Add support for ADC_TM part of PMIC5 Gen3.
>>
>> This is an auxiliary driver under the Gen3 ADC driver, which implements the
>> threshold setting and interrupt generating functionalities of QCOM ADC_TM
>> drivers, used to support thermal trip points.
>>
>> Signed-off-by: Jishnu Prakash <jishnu.prakash@xxxxxxxxxxxxxxxx>
> A couple of minor comments from me.
> 
> Thanks,
> 
> Jonathan
> 
>> diff --git a/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
>> new file mode 100644
>> index 000000000000..633008f173a8
>> --- /dev/null
>> +++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
>> @@ -0,0 +1,437 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
>> +
>> +#include <linux/auxiliary_bus.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/cleanup.h>
>> +#include <linux/container_of.h>
>> +#include <linux/device/devres.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/err.h>
>> +#include <linux/iio/adc/qcom-adc5-gen3-common.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
> 
> Do you need kernel.h?  It's odd to see a driver (correctly)
> use the subheaders of device.h but still include the mega
> header kernel.h rather than more focused ones.
> 

Hi Jonathan,

I think this is not needed with the other headers included, I'll
remove it.


>> +#include <linux/module.h>
>> +#include <linux/thermal.h>
>> +#include <linux/types.h>
>> +#include <linux/unaligned.h>
> 
>> +
>> +static irqreturn_t adctm5_gen3_isr(int irq, void *dev_id)
>> +{
>> +	struct adc_tm5_gen3_chip *adc_tm5 = dev_id;
>> +	int ret, sdam_num;
>> +	u8 tm_status[2];
>> +	u8 status, val;
>> +
>> +	sdam_num = get_sdam_from_irq(adc_tm5, irq);
>> +	if (sdam_num < 0)
>> +		return IRQ_NONE;
>> +
>> +	ret = adc5_gen3_read(adc_tm5->dev_data, sdam_num, ADC5_GEN3_STATUS1,
>> +			     &status, sizeof(status));
>> +	if (ret)
>> +		return IRQ_NONE;
>> +
>> +	if (status & ADC5_GEN3_STATUS1_CONV_FAULT) {
>> +		val = ADC5_GEN3_CONV_ERR_CLR_REQ;
>> +		adc5_gen3_status_clear(adc_tm5->dev_data, sdam_num,
>> +				       ADC5_GEN3_CONV_ERR_CLR, &val, 1);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	ret = adc5_gen3_read(adc_tm5->dev_data, sdam_num, ADC5_GEN3_TM_HIGH_STS,
>> +			     tm_status, sizeof(tm_status));
>> +	if (ret)
>> +		return IRQ_NONE;
>> +
>> +	if (tm_status[0] || tm_status[1])
>> +		return IRQ_WAKE_THREAD;
>> +
>> +	return IRQ_NONE;
>> +}
>> +
>> +static int adc5_gen3_tm_status_check(struct adc_tm5_gen3_chip *adc_tm5,
>> +				     int sdam_index, u8 *tm_status, u8 *buf)
> 
> Might be worth an at_least marking for buf and maybe for tm_status as well
> so it is clear they are big enough for how they are used in here.
> Sooner or later compilers will check those.
> 

Sure, hope it's sufficient if I update it like this:

static int adc5_gen3_tm_status_check(struct adc_tm5_gen3_chip *adc_tm5,
				int sdam_index, u8 tm_status[at_least 2],
				u8 buf[at_least 16])

> 
>> +{
>> +	int ret;
>> +
>> +	ret = adc5_gen3_read(adc_tm5->dev_data, sdam_index, ADC5_GEN3_TM_HIGH_STS,
>> +			     tm_status, 2);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = adc5_gen3_status_clear(adc_tm5->dev_data, sdam_index, ADC5_GEN3_TM_HIGH_STS_CLR,
>> +				     tm_status, 2);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = adc5_gen3_read(adc_tm5->dev_data, sdam_index, ADC5_GEN3_CH_DATA0(0),
>> +			     buf, 16);
>> +	return ret;
> 
> 	return adc5...
> 
>> +}
>> +
>> +static irqreturn_t adctm5_gen3_isr_thread(int irq, void *dev_id)
>> +{
>> +	struct adc_tm5_gen3_chip *adc_tm5 = dev_id;
>> +	int sdam_index = -1;
>> +	u8 tm_status[2] = { };
>> +	u8 buf[16] = { };
>> +
>> +	for (int i = 0; i < adc_tm5->nchannels; i++) {
>> +		struct adc_tm5_gen3_channel_props *chan_prop = &adc_tm5->chan_props[i];
>> +		int offset = chan_prop->tm_chan_index;
>> +		bool upper_set, lower_set;
>> +		int ret;
>> +
>> +		scoped_guard(adc5_gen3, adc_tm5) {
>> +			if (chan_prop->sdam_index != sdam_index) {
>> +				sdam_index = chan_prop->sdam_index;
>> +				ret = adc5_gen3_tm_status_check(adc_tm5, sdam_index,
>> +								tm_status, buf);
> 
> I think the clear of other sdam interrupt status that sashiko was pointing out
> is here as somewhat unexpectedly a function called status_check clears as well.
> 

This is the full comment from Sashiko at this point:

> "Does the threaded handler clear statuses across all SDAMs indiscriminately?

> Since this thread loops over all channels and clears the high status on any
> SDAM with an active event, could it clear a pending event on a different SDAM
> than the one that triggered the IRQ?

> Because each SDAM has its own independent IRQ line, if the thread clears a
> pending event on SDAM 1 while servicing SDAM 0, couldn't SDAM 1's subsequent
> hardirq read a status of 0 and return IRQ_NONE? Could repeated IRQ_NONE
> returns cause the IRQ subsystem to shut down SDAM 1's interrupt line as a
> spurious interrupt storm?"

This sequence of events can happen, but it should not be an issue.

It is possible that the threaded handler is called for servicing an
interrupt on SDAM0, and in the loop there is a violation detected on
a TM channel on SDAM1, and the SDAM1 TM status is cleared. But in this
case, this violation would also be handled after we notify the thermal
framework at the end of the loop, by some threshold update or disablement.

Even if the subsequent hardirq fires for SDAM1 and it returns IRQ_NONE
as the TM status was cleared, the violation would have been handled
by some threshold update, so the interrupt would not keep getting
triggered afterwards for the same threshold's violation.


I also checked the conditions from note_interrupt() in kernel/irq/spurious.c,
for enough repeated IRQ_NONE returns to happen to cause a spurious interrupt
disablement. 
It looks like there needs to be more than one interrupts returning IRQ_NONE
within 0.1 second to increment the irqs_unhandled counter once, but there can be
at most one TM interrupt in one second since we set the time period of
recurring TM measurement as one second here in the probe:

		adc_tm5->chan_props[i].timer = MEAS_INT_1S;

So a spurious interrupt storm is not possible here.


> 
>> +				if (ret)
>> +					return IRQ_NONE;
>> +			}
>> +
>> +			upper_set = ((tm_status[0] & BIT(offset)) && chan_prop->high_thr_en);
>> +			lower_set = ((tm_status[1] & BIT(offset)) && chan_prop->low_thr_en);
>> +		}
>> +
>> +		if (!(upper_set || lower_set))
>> +			continue;
>> +
>> +		thermal_zone_device_update(chan_prop->tzd, THERMAL_TRIP_VIOLATED);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
>> +static int adc_tm5_gen3_configure(struct adc_tm5_gen3_channel_props *prop,
>> +				  int low_temp, int high_temp)
>> +{
>> +	struct adc_tm5_gen3_chip *adc_tm5 = prop->chip;
>> +	u8 buf[ADC_TM5_GEN3_CONFIG_REGS];
>> +	u8 conv_req;
>> +	u16 adc_code;
>> +	int ret;
>> +
>> +	ret = adc5_gen3_poll_wait_hs(adc_tm5->dev_data, prop->sdam_index);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = adc5_gen3_read(adc_tm5->dev_data, prop->sdam_index,
>> +			     ADC5_GEN3_SID, buf, sizeof(buf));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Write SID */
>> +	buf[0] = FIELD_PREP(ADC5_GEN3_SID_MASK, prop->common_props.sid);
>> +
>> +	/* Select TM channel and indicate there is an actual conversion request */
>> +	buf[1] = ADC5_GEN3_CHAN_CONV_REQ | prop->tm_chan_index;
>> +
>> +	buf[2] = prop->timer;
>> +
>> +	/* Digital param selection */
>> +	adc5_gen3_update_dig_param(&prop->common_props, &buf[3]);
>> +
>> +	/* Update fast average sample value */
>> +	buf[4] &= ~ADC5_GEN3_FAST_AVG_CTL_SAMPLES_MASK;
> 
> Maybe use FIELD_MODIFY() for this as makes it obvious where the update is.
> 
>> +	buf[4] |= prop->common_props.avg_samples | ADC5_GEN3_FAST_AVG_CTL_EN;
> 
> Looking at the field defines is this writing them all?  For other fields you don't
> seem to have been careful to preserve reserved values so why this one?

I think this was just overlooked earlier, preserving the reserved values
should not matter here either. I'll update it using FIELD_PREP() like
how it's done in the main driver.

I'll also address your remaining comments in the next patch series.

Thanks,
Jishnu

> 
>> +
>> +	/* Select ADC channel */
>> +	buf[5] = prop->common_props.channel;
>> +
>> +	/* Select HW settle delay for channel */
>> +	buf[6] = FIELD_PREP(ADC5_GEN3_HW_SETTLE_DELAY_MASK,
>> +			    prop->common_props.hw_settle_time_us);
>> +
>> +	/* High temperature corresponds to low voltage threshold */