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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 27 Nov 2025 19:10:36 +0530
Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> 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@oss.qualcomm.com>
> ---
Hi Jishnu

Fresh read threw up a few more comments from me.

See inline

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..c6cc8ef76f7e
> --- /dev/null
> +++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c

> +
> +static void tm_handler_work(struct work_struct *work)
> +{
> +	struct adc_tm5_gen3_chip *adc_tm5 = container_of(work, struct adc_tm5_gen3_chip,
> +							 tm_handler_work);
> +	struct adc_tm5_gen3_channel_props *chan_prop;

Why not declare in the reduced scope below?  Then could probably combine
declaration and assignment for this, and offset.


> +	u8 tm_status[2] = { };
> +	u8 buf[16] = { };
> +	int sdam_index = -1;
> +	int i, ret;
> +
> +	for (i = 0; i < adc_tm5->nchannels; i++) {
It's considered fine in new kernel code to declare the loop variable
as
	for (int i = 0;

> +		bool upper_set, lower_set;
> +		int temp, offset;
> +		u16 code = 0;
> +
> +		chan_prop = &adc_tm5->chan_props[i];
> +		offset = chan_prop->tm_chan_index;
> +
> +		adc5_gen3_mutex_lock(adc_tm5->dev);
> +		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);
> +			if (ret) {
> +				adc5_gen3_mutex_unlock(adc_tm5->dev);

If you had the guard() below, could perhaps use scoped_guard() here
to avoid need for unlocking in error paths.
That would be at the cost of increased indent however, so may not be worth it
or that may suggest factoring out some of this code as a helper.

> +				break;
> +			}
> +		}
> +
> +		upper_set = ((tm_status[0] & BIT(offset)) && chan_prop->high_thr_en);
> +		lower_set = ((tm_status[1] & BIT(offset)) && chan_prop->low_thr_en);
> +		adc5_gen3_mutex_unlock(adc_tm5->dev);
> +
> +		if (!(upper_set || lower_set))
> +			continue;
> +
> +		code = get_unaligned_le16(&buf[2 * offset]);
> +		pr_debug("ADC_TM threshold code:%#x\n", code);
> +
> +		ret = adc5_gen3_therm_code_to_temp(adc_tm5->dev,
> +						   &chan_prop->common_props,
> +						   code, &temp);
> +		if (ret) {
> +			dev_err(adc_tm5->dev,
> +				"Invalid temperature reading, ret = %d, code=%#x\n",
> +				ret, code);
> +			continue;
> +		}
> +
> +		chan_prop->last_temp = temp;
> +		chan_prop->last_temp_set = true;
> +		thermal_zone_device_update(chan_prop->tzd, THERMAL_TRIP_VIOLATED);
> +	}
> +}

> +static int adc_tm5_gen3_set_trip_temp(struct thermal_zone_device *tz,
> +				      int low_temp, int high_temp)
> +{
> +	struct adc_tm5_gen3_channel_props *prop = thermal_zone_device_priv(tz);
> +	struct adc_tm5_gen3_chip *adc_tm5;
> +	int ret;
> +
> +	if (!prop || !prop->chip)
> +		return -EINVAL;
> +
> +	adc_tm5 = prop->chip;
> +
> +	dev_dbg(adc_tm5->dev, "channel:%s, low_temp(mdegC):%d, high_temp(mdegC):%d\n",
> +		prop->common_props.label, low_temp, high_temp);
> +
> +	adc5_gen3_mutex_lock(adc_tm5->dev);
> +	if (high_temp == INT_MAX && low_temp <= -INT_MAX)

How is low temp lower than the min value that fits in an integer?

> +		ret = adc_tm5_gen3_disable_channel(prop);
> +	else
> +		ret = adc_tm5_gen3_configure(prop, low_temp, high_temp);
> +	adc5_gen3_mutex_unlock(adc_tm5->dev);
Might be worth a DEFINE_GUARD() so you can do
	guard(adc5_gen3)(adc_tm5->dev);
	if (high_temp = INT_MAX && low_temp <= -INT_MAX)
		return adc_tm5_gen3_disable_channel(prop);
	
	return adc_tm5...

I haven't looked to see if this is useful elsewhere in these drivers.

> +
> +	return ret;
> +}