Re: [PATCH RFC v2 3/5] thermal: qcom: Add support for MBG thermal monitoring

From: Satya Priya Kakitapalli
Date: Mon Dec 30 2024 - 04:46:19 EST


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@xxxxxxxxxxx>
---
[...]

+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