Re: [PATCH v3 3/3] thermal: qcom: add support for PMIC5 Gen3 ADC thermal monitoring

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

Hi Andy,

On 7/6/2026 11:29 AM, Andy Shevchenko wrote:
> On Sun, Jul 05, 2026 at 10:23:35PM +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.
> 
> ...
> 
>> +DEFINE_GUARD(adc5_gen3, struct adc_tm5_gen3_chip *, adc5_gen3_mutex_lock(_T->dev),
>> +	     adc5_gen3_mutex_unlock(_T->dev))
> 
> Logical split is slightly better.
> 
> DEFINE_GUARD(adc5_gen3, struct adc_tm5_gen3_chip *,
> 	     adc5_gen3_mutex_lock(_T->dev), adc5_gen3_mutex_unlock(_T->dev))
> 
> 
> ...
> 
>> +static int adc5_gen3_tm_status_check(struct adc_tm5_gen3_chip *adc_tm5,
>> +				     int sdam_index, u8 tm_status[at_least 2])
> 
> This function uses exact size of the submitted tm_status. Instead of creating a
> complications to the compiler with all these new fancy attributes, just pass the
> sizeof in an additional argument.
> 
>> +{
>> +	int ret;
>> +
>> +	ret = adc5_gen3_read(adc_tm5->dev_data, sdam_index, ADC5_GEN3_TM_HIGH_STS,
>> +			     tm_status, 2);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return adc5_gen3_status_clear(adc_tm5->dev_data, sdam_index, ADC5_GEN3_TM_HIGH_STS_CLR,
>> +				     tm_status, 2);
>> +}
>> +
>> +static irqreturn_t adctm5_gen3_isr_thread(int irq, void *dev_id)
>> +{
>> +	struct adc_tm5_gen3_chip *adc_tm5 = dev_id;
>> +	u8 tm_status[2] = { };
> 
> ^^^^ The only case for the above function.

Yes, it might be better to call adc5_gen3_read() and adc5_gen3_status_clear()
directly now than keep them in a separate function. I'll update it this way.

> 
> Also why do you need to nullify it? If it's an error condition, the compiler
> should be clever enough to see this. It's not that complicated code.
> 

I think the null initialization is not needed, I'll remove it.

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

Thanks,
Jishnu


>> +	int sdam_index;
>> +
>> +	sdam_index = get_sdam_from_irq(adc_tm5, irq);
>> +	if (sdam_index < 0)
>> +		return IRQ_NONE;
>> +
>> +	scoped_guard(adc5_gen3, adc_tm5) {
> 
>> +		int ret = adc5_gen3_tm_status_check(adc_tm5, sdam_index,
>> +						    tm_status);
>> +		if (ret)
>> +			return IRQ_NONE;