| 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: <c5079172-e127-4dfc-826a-b32489d852f8@quicinc.com>
Date: Mon, 30 Dec 2024 15:15:32 +0530
From: Satya Priya Kakitapalli <quic_skakitap@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
"Rafael J. Wysocki"
<rafael@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Zhang Rui
<rui.zhang@...el.com>, Lukasz Luba <lukasz.luba@....com>,
Rob Herring
<robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>,
Jonathan Cameron <jic23@...nel.org>,
"Lars-Peter
Clausen" <lars@...afoo.de>, Lee Jones <lee@...nel.org>,
Stephen Boyd
<sboyd@...nel.org>, Amit Kucheria <amitk@...nel.org>,
Thara Gopinath
<thara.gopinath@...il.com>,
Bjorn Andersson <andersson@...nel.org>,
"Konrad
Dybcio" <konradybcio@...nel.org>
CC: Ajit Pandey <quic_ajipan@...cinc.com>,
Imran Shaik
<quic_imrashai@...cinc.com>,
Taniya Das <quic_tdas@...cinc.com>,
"Jagadeesh
Kona" <quic_jkona@...cinc.com>, <quic_kamalw@...cinc.com>,
<quic_jprakash@...cinc.com>, <linux-arm-msm@...r.kernel.org>,
<linux-pm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>
Subject: Re: [PATCH RFC v2 3/5] thermal: qcom: Add support for MBG thermal
monitoring
On 12/13/2024 9:18 PM, Konrad Dybcio wrote:
> On 12.12.2024 5:11 PM, Satya Priya Kakitapalli 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>
>> ---
> [...]
>
>> +static const struct mbg_map_table map_table[] = {
> Is this peripheral/pmic-specific?
Yes, peripheral specific.
>> + /* minT vtemp0 tc */
>> + { -60000, 4337, 1967 },
>> + { -40000, 4731, 1964 },
>> + { -20000, 5124, 1957 },
>> + { 0, 5515, 1949 },
>> + { 20000, 5905, 1940 },
>> + { 40000, 6293, 1930 },
>> + { 60000, 6679, 1921 },
>> + { 80000, 7064, 1910 },
>> + { 100000, 7446, 1896 },
>> + { 120000, 7825, 1878 },
>> + { 140000, 8201, 1859 },
>> +};
>> +
>> +static int mbg_tm_get_temp(struct thermal_zone_device *tz, int *temp)
>> +{
>> + struct mbg_tm_chip *chip = thermal_zone_device_priv(tz);
>> + int ret, milli_celsius;
>> +
>> + if (!temp)
>> + return -EINVAL;
>> +
>> + if (chip->last_thres_crossed) {
>> + pr_debug("last_temp: %d\n", chip->last_temp);
> Use dev_dbg for consistency with the other debug prints
Okay.
>> + chip->last_thres_crossed = false;
>> + *temp = chip->last_temp;
>> + return 0;
>> + }
>> +
>> + ret = iio_read_channel_processed(chip->adc, &milli_celsius);
>> + if (ret < 0) {
>> + dev_err(chip->dev, "failed to read iio channel %d\n", ret);
>> + return ret;
>> + }
>> +
>> + *temp = milli_celsius;
>> +
>> + return 0;
>> +}
>> +
>> +static int temp_to_vtemp(int temp)
>> +{
>> +
>> + int idx, vtemp, tc = 0, t0 = 0, vtemp0 = 0;
>> +
>> + for (idx = 0; idx < ARRAY_SIZE(map_table); idx++)
>> + if (temp >= map_table[idx].min_temp &&
>> + temp < (map_table[idx].min_temp + 20000)) {
> Please align the two lines, tab width is 8 for kernel code
Okay.
>> + tc = map_table[idx].tc;
>> + t0 = map_table[idx].min_temp;
>> + vtemp0 = map_table[idx].vtemp0;
>> + break;
>> + }
>> +
>> + /*
>> + * Formula to calculate vtemp(mV) from a given temp
>> + * vtemp = (temp - minT) * tc + vtemp0
>> + * tc, t0 and vtemp0 values are mentioned in the map_table array.
>> + */
>> + vtemp = ((temp - t0) * tc + vtemp0 * 100000) / 1000000;
> So you say vtemp = ... and the func is called temp_to_vtemp
>
>> + return abs(vtemp - MBG_TEMP_DEFAULT_TEMP_MV) / MBG_TEMP_STEP_MV;
> But you end up returning a scaled version of it..
> Please clarify that in the code
Sure, I'll update the function name to temp_to_vtemp_mv and probably add
a comment in the code.
>
>> +}
>> +
>> +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;
>> +
>> + guard(mutex)(&chip->lock);
>> +
>> + /* The HW has a limitation that the trip set must be above 25C */
>> + if (temp > MBG_MIN_TRIP_TEMP && temp < MBG_MAX_SUPPORTED_TEMP) {
>> + regmap_set_bits(chip->map,
>> + chip->base + MBG_TEMP_MON2_MISC_CFG, MON2_UP_THRESH_EN);
>> + ret = regmap_write(chip->map, chip->base + MON2_LVL1_UP_THRESH,
>> + temp_to_vtemp(temp));
>> + if (ret < 0)
>> + return ret;
>> + } else {
>> + dev_dbg(chip->dev, "Set trip b/w 25C and 160C\n");
>> + regmap_clear_bits(chip->map,
>> + chip->base + MBG_TEMP_MON2_MISC_CFG, MON2_UP_THRESH_EN);
>> + }
>> +
>> + /*
>> + * 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, val;
>> +
>> + scoped_guard(mutex, &chip->lock) {
>> + ret = regmap_read(chip->map,
>> + chip->base + MBG_TEMP_MON2_FAULT_STATUS, &val);
>> + if (ret < 0)
>> + return IRQ_HANDLED;
>> + }
>> +
>> + if ((val & MON_FAULT_STATUS_MASK) & MON_FAULT_STATUS_LVL1) {
>> + if ((val & MON_POLARITY_STATUS_MASK) & MON_POLARITY_STATUS_UPR) {
> Just checking the last argument to AND in both lines is enough, as
> they're both parts of the bitfield
Both the bits of each mask need to be checked in order to proceed
accordingly, I will update with proper logic in next version.
>
> [...]
>
>> + ret = device_property_read_u32(chip->dev, "reg", &res);
>> + if (ret < 0)
>> + return ret;
> return dev_err_probe(dev, ret, "Couldn't read reg property"\n);
>
>> +
>> + chip->base = res;
>> +
>> + chip->irq = platform_get_irq(pdev, 0);
>> + if (chip->irq < 0)
>> + return chip->irq;
> Similarly here
>
>> +
>> + chip->adc = devm_iio_channel_get(&pdev->dev, "thermal");
>> + if (IS_ERR(chip->adc))
>> + return dev_err_probe(&pdev->dev, PTR_ERR(chip->adc),
>> + "failed to get adc channel\n");
>> +
>> + chip->tz_dev = devm_thermal_of_zone_register(&pdev->dev, 0,
>> + chip, &mbg_tm_ops);
>> + if (IS_ERR(chip->tz_dev))
>> + return dev_err_probe(&pdev->dev, PTR_ERR(chip->tz_dev),
>> + "failed to register sensor\n");
> Please also make the error messages start with an uppercase letter
>
>> +
>> + return devm_request_threaded_irq(&pdev->dev, chip->irq, NULL,
>> + mbg_tm_isr, IRQF_ONESHOT, node->name, chip);
>> +}
>> +
>> +static const struct of_device_id mbg_tm_match_table[] = {
>> + { .compatible = "qcom,spmi-pm8775-mbg-tm" },
> I don't think the 'spmi' bit belongs here
Okay, will update it.
> Konrad
Powered by blists - more mailing lists