| 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
| ||
|
Message-ID: <bb1035cf-f025-4ac8-b22f-ad5a08ac1bfb@oss.qualcomm.com>
Date: Fri, 19 Dec 2025 18:46:19 +0530
From: Jishnu Prakash <jishnu.prakash@....qualcomm.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: robh@...nel.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 V8 4/4] thermal: qcom: add support for PMIC5 Gen3 ADC
thermal monitoring
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@....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
>
> 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;
>> +}
Powered by blists - more mailing lists