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

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

Hi Andy,

On 6/3/2026 5:14 AM, Andy Shevchenko wrote:
> On Tue, May 26, 2026 at 04:26:10PM +0530, Jishnu Prakash 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.
> 
> ...
> 
>> +#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>
> 
> Why?!

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

> 
>> +#include <linux/module.h>
>> +#include <linux/thermal.h>
>> +#include <linux/types.h>
>> +#include <linux/unaligned.h>
> 
> ...
> 
>> +/**
>> + * struct adc_tm5_gen3_channel_props - ADC_TM channel structure
>> + * @timer: time period of recurring TM measurement.
>> + * @tm_chan_index: TM channel number used (ranging from 1-7).
>> + * @sdam_index: SDAM on which this TM channel lies.
>> + * @common_props: structure with common  ADC channel properties.
>> + * @high_thr_en: TM high threshold crossing detection enabled.
>> + * @low_thr_en: TM low threshold crossing detection enabled.
>> + * @chip: ADC TM device.
>> + * @tzd: pointer to thermal device corresponding to TM channel.
>> + */
>> +struct adc_tm5_gen3_channel_props {
>> +	unsigned int timer;
>> +	unsigned int tm_chan_index;
>> +	unsigned int sdam_index;
>> +	struct adc5_channel_common_prop common_props;
>> +	bool high_thr_en;
>> +	bool low_thr_en;
>> +	struct adc_tm5_gen3_chip *chip;
>> +	struct thermal_zone_device *tzd;
> 
> Wouldn't `pahole` suggest better layout?
> 
>> +};
> 
> ...
> 
>> +struct adc_tm5_gen3_chip {
>> +	struct adc5_device_data *dev_data;
>> +	struct adc_tm5_gen3_channel_props *chan_props;
>> +	unsigned int nchannels;
>> +	struct device *dev;
> 
> Ditto. I would expect the nchannels to be the last. Also play with the position
> of dev to see if bloat-o-meter will show the difference.
> 

Thanks for your suggestions, I used pahole to check and I'll update the above two
structs to remove existing holes.

And with bloat-o-meter, I tried moving dev around in adc_tm5_gen3_chip and
found the below arrangement was the most optimal:

struct adc_tm5_gen3_chip {
	struct adc5_device_data *dev_data;
	struct adc_tm5_gen3_channel_props *chan_props;
	struct device *dev;
	unsigned int nchannels;
};

possibly because dev_data and chan_props are accessed the most frequently
in this file. I'll make these changes in the next patch series.



>> +};
> 
> ...
> 
>> +	return adc5_gen3_get_scaled_reading(adc_tm5->dev, &prop->common_props,
>> +					    temp);
> 
> Make it a single line.
> 
> ...
> 
>> +	/* Low temperature corresponds to high voltage threshold */
>> +	prop->high_thr_en = (low_temp != -INT_MAX);
> 
> Can low_temp be INT_MIN at some point?

From what I see, that would not happen. It looks like the convention
in the thermal framework is to use -INT_MAX as the limit for the lower
threshold in the .set_trips callback.

> 
>> +	if (prop->high_thr_en) {
>> +		adc_code = qcom_adc_tm5_gen2_temp_res_scale(low_temp);
>> +		put_unaligned_le16(adc_code, &buf[10]);
>> +	}
> 
> ...
> 
>> +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;
>> +	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);
> 
> One line is easier to read.
> 
>> +	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. */
>> +	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.
>> +	 */
> 
>> +	ret = devm_request_threaded_irq(dev,
>> +					adc_tm5->dev_data->base[0].irq,
>> +					adctm5_gen3_isr, adctm5_gen3_isr_thread,
>> +					IRQF_ONESHOT | IRQF_SHARED,
>> +					adc_tm5->dev_data->base[0].irq_name,
>> +					adc_tm5);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	for (int i = 1; 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,
>> +						IRQF_ONESHOT, adc_tm5->dev_data->base[i].irq_name,
>> +						adc_tm5);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
> 
> Can't it be combined by using temporary irq_flags variable
> 
> 	/* ...the fat comment... */
> 	irq_flags = ...
> 	for (int i = 0; ...) {
> 		...
> 		irq_flags = ...
> 	}
> 
> ?

Thanks for your suggestion, I'll update it this way.

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

Thanks,
Jishnu


> 
>> +	return adc_tm5_register_tzd(adc_tm5);
>> +}
> 
> ...
> 
>> +static const struct auxiliary_device_id adctm5_auxiliary_id_table[] = {
>> +	{ .name = "qcom_spmi_adc5_gen3.adc5_tm_gen3", },
> 
> Inner comma is redundant.
> 
>> +	{ }
>> +};
> 
>> +
> 
> Unneeded blank line.
> 
>> +MODULE_DEVICE_TABLE(auxiliary, adctm5_auxiliary_id_table);
>> +
>> +static struct auxiliary_driver adctm5gen3_auxiliary_driver = {
>> +	.id_table = adctm5_auxiliary_id_table,
>> +	.probe = adc_tm5_probe,
>> +};
> 
>> +
> 
> Ditto.
> 
>> +module_auxiliary_driver(adctm5gen3_auxiliary_driver);
>