linux-kernel - Re: [PATCH V7 5/5] thermal: qcom: add support for PMIC5 Gen3 ADC thermal monitoring

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <47f7a917-3bb6-4331-8813-49f6646333e5@oss.qualcomm.com>
Date: Thu, 18 Sep 2025 01:17:58 +0530
From: Jishnu Prakash <jishnu.prakash@....qualcomm.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: robh@...nel.org, krzysztof.kozlowski@...aro.org, krzk+dt@...nel.org,
        conor+dt@...nel.org, agross@...nel.org, andersson@...nel.org,
        lumag@...nel.org, dmitry.baryshkov@....qualcomm.com,
        konradybcio@...nel.org, daniel.lezcano@...aro.org, sboyd@...nel.org,
        amitk@...nel.org, thara.gopinath@...il.com, lee@...nel.org,
        rafael@...nel.org, subbaraman.narayanamurthy@....qualcomm.com,
        david.collins@....qualcomm.com, anjelique.melendez@....qualcomm.com,
        kamal.wadhwa@....qualcomm.com, rui.zhang@...el.com,
        lukasz.luba@....com, devicetree@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        cros-qcom-dts-watchers@...omium.org, quic_kotarake@...cinc.com,
        neil.armstrong@...aro.org, stephan.gerhold@...aro.org
Subject: Re: [PATCH V7 5/5] thermal: qcom: add support for PMIC5 Gen3 ADC
 thermal monitoring

Hi Jonathan,

On 8/30/2025 11:28 PM, Jonathan Cameron wrote:
> On Tue, 26 Aug 2025 14:06:57 +0530
> Jishnu Prakash <jishnu.prakash@....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@....qualcomm.com>
> Hi Jishnu,
> 
> A few comment inline from a fresh read
> 
> Jonathan
> 
> 

...

>> +
>> +	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 (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;
>> +	}
>> +
>> +	ret = devm_add_action_or_reset(dev, adc5_gen3_disable, adc_tm5);
> 
> I'd normally expect a pairing of a devm action with whatever it is undoing.
> If not add a comment for why that isn't the case here.

This is meant to disable all ADC_TM channels in case of a probe failure.

But thinking more on it, ADC_TM channels could be enabled only by the thermal
framework calling the .set_trip function and that could happen only after
this line at the end of the probe, for thermal framework registration:

        return adc_tm5_register_tzd(adc_tm5);

So I think it makes more sense to move this call to near the end of the probe
and to make it devm_add_action() instead of devm_add_action_or_reset(). I'll
also add a comment mentioning what it does.

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

Thanks,
Jishnu

> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	INIT_WORK(&adc_tm5->tm_handler_work, tm_handler_work);
> 
>> +}
>> +
>> +static const struct auxiliary_device_id adctm5_auxiliary_id_table[] = {
>> +	{ .name = "qcom_spmi_adc5_gen3.adc5_tm_gen3", },
>> +	{}
> 
> For IIO drivers I'm trying to slowly standardize some formatting choices.
> For these I picked (for no particular reason)
> 	{ }
> 
>> +};