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

[Andy Shevchenko]

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

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

> +};

...

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

> +	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 = ...
	}

?

> +	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);

-- 
With Best Regards,
Andy Shevchenko