Re: [PATCH v3 3/3] thermal: qcom: add support for PMIC5 Gen3 ADC thermal monitoring

[Jishnu Prakash]

Hi Jonathan,

On 7/6/2026 5:21 AM, Jonathan Cameron wrote:
> On Sun, 05 Jul 2026 22:23:35 +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>
> Hi Jishnu,
> 
> I took a quick look at this. A few really minor suggestions inline.
> 
>> 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..5a82c4d8a37e
>> --- /dev/null
>> +++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
> 
> 
>> +static int adc_tm5_register_tzd(struct adc_tm5_gen3_chip *adc_tm5)
>> +{
>> +	struct thermal_zone_device *tzd;
>> +	unsigned int channel;
>> +	int ret;
>> +
>> +	for (int i = 0; i < adc_tm5->nchannels; i++) {
>> +		channel = ADC5_GEN3_V_CHAN(adc_tm5->chan_props[i].common_props);
>> +		tzd = devm_thermal_of_zone_register(adc_tm5->dev, channel,
>> +						    &adc_tm5->chan_props[i],
>> +						    &adc_tm_ops);
>> +		if (IS_ERR(tzd)) {
>> +			if (PTR_ERR(tzd) == -ENODEV) {
>> +				dev_info(adc_tm5->dev,
>> +					 "thermal sensor on channel %d is not used\n",
> 
> That seems noisy.  Maybe dev_dbg() appropriate if this is an expected
> board dependent condition?
> 
>> +					 channel);
>> +				continue;
>> +			}
>> +			return dev_err_probe(adc_tm5->dev, PTR_ERR(tzd),
>> +					     "Error registering TZ zone:%ld for channel:%d\n",
>> +					     PTR_ERR(tzd), channel);
>> +		}
>> +		adc_tm5->chan_props[i].tzd = tzd;
>> +		ret = devm_thermal_add_hwmon_sysfs(adc_tm5->dev, tzd);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	return 0;
>> +}
> 
> 
>> +static int adc_tm5_probe(struct auxiliary_device *aux_dev,
>> +			 const struct auxiliary_device_id *id)
>> +{
>> +	struct adc_tm5_gen3_chip *adc_tm5;
>> +	struct tm5_aux_dev_wrapper *aux_dev_wrapper;
>> +	struct device *dev = &aux_dev->dev;
>> +	u32 irq_flags;
>> +	int ret;
>> +
>> +	adc_tm5 = devm_kzalloc(dev, sizeof(*adc_tm5), GFP_KERNEL);
>> +	if (!adc_tm5)
>> +		return -ENOMEM;
>> +
>> +	aux_dev_wrapper = container_of(aux_dev, struct tm5_aux_dev_wrapper, aux_dev);
>> +
>> +	adc_tm5->dev = dev;
>> +	adc_tm5->dev_data = aux_dev_wrapper->dev_data;
>> +	adc_tm5->nchannels = aux_dev_wrapper->n_tm_channels;
>> +	adc_tm5->chan_props = devm_kcalloc(dev, aux_dev_wrapper->n_tm_channels,
>> +					   sizeof(*adc_tm5->chan_props), GFP_KERNEL);
>> +	if (!adc_tm5->chan_props)
>> +		return -ENOMEM;
>> +
>> +	for (int i = 0; i < adc_tm5->nchannels; i++) {
>> +		adc_tm5->chan_props[i].common_props = aux_dev_wrapper->tm_props[i];
>> +		adc_tm5->chan_props[i].timer = MEAS_INT_1S;
>> +		adc_tm5->chan_props[i].sdam_index = (i + 1) / 8;
>> +		adc_tm5->chan_props[i].tm_chan_index = (i + 1) % 8;
>> +		adc_tm5->chan_props[i].chip = adc_tm5;
>> +	}
>> +
>> +	/* This is to disable all ADC_TM channels in case of probe failure. */
> 
> Perhaps indicate who turned them on?  On from reset or something hidden
> somewhere else?

In the probe, ADC_TM channels are only enabled in the loop in adc_tm5_register_tzd(),
when they have the set_trips API called during thermal zone registration. I'll
update the comment to mention this.

> 
>> +	ret = devm_add_action(dev, adc5_gen3_disable, adc_tm5);
>> +	if (ret)
>> +		return ret;
> 
> ...
> 
>> +	/*
>> +	 * First SDAM's interrupt is shared between main ADC driver
>> +	 * and auxiliary TM driver, so its flags must include
>> +	 * IRQF_SHARED. This is not needed for other SDAMs as they
>> +	 * will be used only for TM functionality.
> 
> If indent of this doesn't change, rewrap to 80 chars.
> 
>> +	 */
>> +	irq_flags = IRQF_ONESHOT | IRQF_SHARED;
>> +	for (int i = 0; i < adc_tm5->dev_data->num_sdams; i++) {
>> +		ret = devm_request_threaded_irq(dev,
>> +						adc_tm5->dev_data->base[i].irq,
>> +						adctm5_gen3_isr, adctm5_gen3_isr_thread,
>> +						irq_flags, adc_tm5->dev_data->base[i].irq_name,
>> +						adc_tm5);
>> +		if (ret < 0)
>> +			return ret;
>> +		irq_flags = IRQF_ONESHOT;
> Whilst this code works, I'd be tempted to make it less ordering dependent.  E.g.
> 	for (int i = 0; i < adc_tm5->dev_data->num_sdams; i++) {
> 		u32 irq_flags = IRQF_ONESHOT;
> 
> 		/*
> 		 * First SDAM's interrupt is shared between main ADC driver
> 		 * and auxiliary TM driver, so its flags must include
> 		 * IRQF_SHARED. This is not needed for other SDAMs as they
> 		 * will be used only for TM functionality.
> 		 */
> 		if (i == 0)
> 			irq_flags |= IRQF_SHARED;

Sure, I'll update the function this way and also address your other comments.

Thanks,
Jishnu

> 
> 		ret = devm_request_threaded_irq(dev,
>> +						adc_tm5->dev_data->base[i].irq,
>> +						adctm5_gen3_isr, adctm5_gen3_isr_thread,
>> +						irq_flags, adc_tm5->dev_data->base[i].irq_name,
>> +						adc_tm5);
>> +	}
>> +
>> +	return 0;
>> +}
> 
>