| 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: <e1ab8bf0-f3ea-496f-bc83-6496e60adcdb@quicinc.com>
Date: Tue, 19 Nov 2024 13:30:42 +0530
From: Satya Priya Kakitapalli <quic_skakitap@...cinc.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Daniel Lezcano
<daniel.lezcano@...aro.org>,
Zhang Rui <rui.zhang@...el.com>, Lukasz Luba
<lukasz.luba@....com>,
Amit Kucheria <amitk@...nel.org>,
Thara Gopinath
<thara.gopinath@...il.com>,
Bjorn Andersson <andersson@...nel.org>,
"Konrad
Dybcio" <konrad.dybcio@...aro.org>,
Kamal Wadhwa <quic_kamalw@...cinc.com>,
Taniya Das <quic_tdas@...cinc.com>,
Jishnu Prakash
<quic_jprakash@...cinc.com>,
<linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-pm@...r.kernel.org>, Ajit Pandey
<quic_ajipan@...cinc.com>,
Imran Shaik <quic_imrashai@...cinc.com>,
"Jagadeesh Kona" <quic_jkona@...cinc.com>
Subject: Re: [PATCH 3/5] thermal: qcom: Add support for MBG Temp monitor
On 7/13/2024 5:29 PM, Jonathan Cameron wrote:
> On Fri, 12 Jul 2024 18:13:30 +0530
> Satya Priya Kakitapalli <quic_skakitap@...cinc.com> wrote:
>
>> Add driver for the MBG thermal monitoring device. It monitors
>> the die temperature, and when there is a level 1 upper threshold
>> violation, it receives an interrupt over spmi. The driver reads
>> the fault status register and notifies thermal accordingly.
>>
>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@...cinc.com>
> Hi Satya,
>
> Some comments inline.
Thanks for review, I'll address the comments in my V2 patches.
>
> Jonathan
>
>> diff --git a/drivers/thermal/qcom/qcom-spmi-mbg-tm.c b/drivers/thermal/qcom/qcom-spmi-mbg-tm.c
>> new file mode 100644
>> index 000000000000..70964ea5a48d
>> --- /dev/null
>> +++ b/drivers/thermal/qcom/qcom-spmi-mbg-tm.c
>> @@ -0,0 +1,269 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +//Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/thermal.h>
>> +#include <linux/iio/consumer.h>
>> +
>> +#include "../thermal_core.h"
>> +
>> +#define MBG_TEMP_MON_MM_MON2_FAULT_STATUS 0x50
>> +
>> +#define MON_FAULT_STATUS_MASK GENMASK(7, 6)
>> +#define MON_FAULT_STATUS_SHIFT 6
> Use FIELD_GET() and FIELD_PREP() then no need to have shift defined.
>
>> +#define MON2_LVL1_ERR 0x1
>> +
>> +#define MON2_LVL1_UP_THRESH 0x59
>> +
>> +#define MBG_TEMP_MON_MM_MON2_MISC_CFG 0x5f
>> +#define UP_THRESH_EN BIT(1)
>> +
>> +#define STEP_MV 8
>> +#define MBG_DEFAULT_TEMP_MV 600
>> +#define MBG_TEMP_CONSTANT 1000
>> +#define MIN_TRIP_TEMP 25000
>> +#define MAX_SUPPORTED_TEMP 160000
> these are all device specific but don't sound it. I'd prefix them with MBG_TEMP
>
>
>> +
>> +static int mbg_tm_set_trip_temp(struct thermal_zone_device *tz, int low_temp,
>> + int temp)
>> +{
>> + struct mbg_tm_chip *chip = thermal_zone_device_priv(tz);
>> + int ret = 0, vtemp = 0;
>> +
>> + mutex_lock(&chip->lock);
> guard(mutex)(&chip->lock);
>
>
> As then you don't need to carefully unlock as it will be done at exit of scope.
>
>> +
>> + /* The HW has a limitation that the trip set must be above 25C */
>> + if (temp > MIN_TRIP_TEMP && temp < INT_MAX) {
> int vtemp;
>
> reduce the scope and it becomes clear no need to init.
>
>> + mbg_tm_reg_update(chip, MBG_TEMP_MON_MM_MON2_MISC_CFG,
>> + UP_THRESH_EN, UP_THRESH_EN);
>> + vtemp = temp_to_vtemp(temp);
>> + ret = mbg_tm_write(chip, MON2_LVL1_UP_THRESH, vtemp);
>> + if (ret < 0) {
>> + mutex_unlock(&chip->lock);
>> + return ret;
>> + }
>> + } else {
>> + dev_dbg(chip->dev, "Setting %d failed, set trip between 25C and INT_MAX\n", temp);
>> + mbg_tm_reg_update(chip, MBG_TEMP_MON_MM_MON2_MISC_CFG,
>> + UP_THRESH_EN, 0);
>> + }
>> +
>> + mutex_unlock(&chip->lock);
>> +
>> + /*
>> + * Configure the last_temp one degree higher, to ensure the
>> + * violated temp is returned to thermal framework when it reads
>> + * temperature for the first time after the violation happens.
>> + * This is needed to account for the inaccuracy in the conversion
>> + * formula used which leads to the thermal framework setting back
>> + * the same thresholds in case the temperature it reads does not
>> + * show violation.
>> + */
>> + chip->last_temp = temp + MBG_TEMP_CONSTANT;
>> +
>> + return ret;
>> +}
>> +
>> +static const struct thermal_zone_device_ops mbg_tm_ops = {
>> + .get_temp = mbg_tm_get_temp,
>> + .set_trips = mbg_tm_set_trip_temp,
>> +};
>> +
>> +static irqreturn_t mbg_tm_isr(int irq, void *data)
>> +{
>> + struct mbg_tm_chip *chip = data;
>> + int ret;
>> + int val = 0;
>> +
>> + mutex_lock(&chip->lock);
>> +
>> + ret = mbg_tm_read(chip, MBG_TEMP_MON_MM_MON2_FAULT_STATUS, &val);
>> +
>> + mutex_unlock(&chip->lock);
> scoped_guard(mutex, &chip->lock) {
> ret = mbg...
> if (ret < 0)
> return IRQ_HANDLED;
> }
> avoids need to unlock before return and to me at least gives slightly more
> readable code.
>
>> +
>> + if (ret < 0)
>> + return IRQ_HANDLED;
>> +
>> + val &= MON_FAULT_STATUS_MASK;
>> + if ((val >> MON_FAULT_STATUS_SHIFT) & MON2_LVL1_ERR) {
> as above use FIELD_GET() with the mask - it is more readable and
> does the mask and shift in one call.
>
>
>
>> + chip->last_temp_set = true;
>> + thermal_zone_device_update(chip->tz_dev,
>> + THERMAL_TRIP_VIOLATED);
> I think the above is 80 chars on one line so no need to wrap.
>
>> + dev_dbg(chip->dev, "Notifying Thermal, fault status=%d\n", val);
>> + } else {
>> + dev_dbg(chip->dev, "Lvl 1 upper threshold not violated, ignoring interrupt\n");
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int mbg_tm_probe(struct platform_device *pdev)
>> +{
>> + struct mbg_tm_chip *chip;
>> + struct device_node *node = pdev->dev.of_node;
>> + u32 res;
>> + int ret = 0;
> It's always set, so don't intiialize here.
>
>> +
>> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>> + if (!chip)
>> + return -ENOMEM;
>> +
>> + chip->dev = &pdev->dev;
>> +
>> + mutex_init(&chip->lock);
>> +
>> + chip->map = dev_get_regmap(pdev->dev.parent, NULL);
>> + if (!chip->map)
>> + return -ENXIO;
>> +
>> + ret = of_property_read_u32(node, "reg", &res);
> I'm not sure on thermal subsystem opinion on this but I'd use
> the property.h generic firmware property reading stuff rather than of
> specific.
> device_property_read_u32()
>
>> + if (ret < 0)
>> + return ret;
>> +
>> + chip->base = res;
>> +
>> + chip->irq = platform_get_irq(pdev, 0);
>> + if (chip->irq < 0)
>> + return chip->irq;
>> +
>> + chip->adc = devm_iio_channel_get(&pdev->dev, "thermal");
>> + if (IS_ERR(chip->adc))
>> + return PTR_ERR(chip->adc);
>> +
>> + chip->tz_dev = devm_thermal_of_zone_register(&pdev->dev,
>> + 0, chip, &mbg_tm_ops);
>> + if (IS_ERR(chip->tz_dev)) {
>> + dev_err(&pdev->dev, "failed to register sensor\n");
>> + return PTR_ERR(chip->tz_dev);
> return dev_err_probe(...)
>
>> + }
>> +
>> + ret = devm_request_threaded_irq(&pdev->dev, chip->irq, NULL,
>> + mbg_tm_isr, IRQF_ONESHOT, node->name, chip);
>> +
>> + return ret;
> return devm_request...
>
>> +}
Powered by blists - more mailing lists