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

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

Hi Jonathan,

On 12/7/2025 10:34 PM, Jonathan Cameron wrote:
> 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.

OK, I'll just keep the following lines here:

    struct adc_tm5_gen3_chip *adc_tm5 = container_of(work, struct adc_tm5_gen3_chip, tm_handler_work);
    int sdam_index = -1;

and declare the remaining variables in the loop.


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

The mutex is meant to guard register writes, so it might be sufficient to
have it around adc5_gen3_tm_status_check() alone. I'll make this change.

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

Yes, that check should be (low_temp == -INT_MAX), I'll fix it.

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

I'll add this and address your other comments.

Thanks,
Jishnu


>> +
>> +	return ret;
>> +}