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...
+}
