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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 30 Jan 2026 17:24:21 +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.

Some minor editorial style stuff below if you are spinning again.
Otherwise this looks good to me

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Given I expect this patch will go through the thermal tree and not IIO.
As mentioned in previous patch review, we've missed this cycle for IIO where
I'd expect to spin an immutable branch for 1-3 so we can do this early
next cycle.

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..882355d6606d
> --- /dev/null
> +++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
> @@ -0,0 +1,512 @@
> +// 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.h>

Similar comment to previous.  It's rare we need device.h
and if a forwards definition of struct device is enough it
is better to just do that.

> +#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>
> +#include <linux/module.h>
> +#include <linux/thermal.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +#include <linux/unaligned.h>
> +
> +#include "../thermal_hwmon.h"

> +
> +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);
> +	int sdam_index = -1;
> +	u8 tm_status[2] = { };
> +	u8 buf[16] = { };
> +
> +	for (int i = 0; i < adc_tm5->nchannels; i++) {

Not that important but you've been a bit inconsistent on this style
of putting the loop iterator declaration in the for loop or not.
You could definitely do it in a few more places.  I didn't comment on those
because it's a style choice, but consistency is a good idea - hence I'm
commenting here.

> +		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, temp;
> +		u16 code;
> +
> +		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);
> +				if (ret)
> +					return;
> +			}
> +
> +			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;
> +
> +		code = get_unaligned_le16(&buf[2 * offset]);
> +		dev_dbg(adc_tm5->dev, "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_register_tzd(struct adc_tm5_gen3_chip *adc_tm5)
> +{
> +	unsigned int i, channel;
> +	struct thermal_zone_device *tzd;
> +	int ret;
> +
> +	for (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);
> +
No blank line here.  Keep the function and the check on it's error tightly
coupled by not having one.  Slightly improves readability.

> +		if (IS_ERR(tzd)) {
> +			if (PTR_ERR(tzd) == -ENODEV) {
> +				dev_warn(adc_tm5->dev,
> +					 "thermal sensor on channel %d is not used\n",
> +					 channel);

Why is it a warning?  Seems like maybe they'd sometimes not be used. In which case
maybe dev_dbg() or dev_info() is more appropriate.

> +				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 void adc5_gen3_clear_work(void *data)
> +{
> +	struct adc_tm5_gen3_chip *adc_tm5 = data;
> +
> +	cancel_work_sync(&adc_tm5->tm_handler_work);
> +}

> +
> +static int adc_tm5_probe(struct auxiliary_device *aux_dev,
> +			 const struct auxiliary_device_id *id)
> +{

> +	adc5_gen3_register_tm_event_notifier(dev, adctm_event_handler);
> +
> +	/*
> +	 * This is to cancel any instances of tm_handler_work scheduled by
> +	 * TM interrupt, at the time of module removal.
> +	 */
> +

Drop this blank line to keep the association between the comment and the call
it is talking about.

> +	ret = devm_add_action(dev, adc5_gen3_clear_work, adc_tm5);
> +	if (ret)
> +		return ret;
> +
> +	ret = adc_tm5_register_tzd(adc_tm5);
> +	if (ret)
> +		return ret;
> +
> +	/* This is to disable all ADC_TM channels in case of probe failure. */
> +
> +	return devm_add_action(dev, adc5_gen3_disable, adc_tm5);
> +}