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@quicinc.com>
---
 drivers/thermal/qcom/Kconfig            |  11 ++
 drivers/thermal/qcom/Makefile           |   1 +
 drivers/thermal/qcom/qcom-spmi-mbg-tm.c | 269 ++++++++++++++++++++++++++++++++
 3 files changed, 281 insertions(+)

diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
index 2c7f3f9a26eb..46045094020c 100644
--- a/drivers/thermal/qcom/Kconfig
+++ b/drivers/thermal/qcom/Kconfig
@@ -21,6 +21,17 @@ config QCOM_SPMI_ADC_TM5
 	  Thermal client sets threshold temperature for both warm and cool and
 	  gets updated when a threshold is reached.
 
+config QCOM_SPMI_MBG_TM
+	tristate "Qualcomm Technologies, Inc. SPMI PMIC MBG Temperature monitor"
+	depends on OF && SPMI && IIO
+	select REGMAP_SPMI
+	help
+	 This enables a thermal driver for the MBG thermal monitoring device.
+	 It shows up in sysfs as a thermal sensor with two trip points.
+	 It notifies the thermal framework when level 1 high threshold is
+	 violated. The temperature reported by the thermal sensor reflects
+	 the real time die temperature through ADC channel.
+
 config QCOM_SPMI_TEMP_ALARM
 	tristate "Qualcomm SPMI PMIC Temperature Alarm"
 	depends on OF && SPMI && IIO
diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
index 0fa2512042e7..bc18e08ee3e2 100644
--- a/drivers/thermal/qcom/Makefile
+++ b/drivers/thermal/qcom/Makefile
@@ -4,5 +4,6 @@ obj-$(CONFIG_QCOM_TSENS)	+= qcom_tsens.o
 qcom_tsens-y			+= tsens.o tsens-v2.o tsens-v1.o tsens-v0_1.o \
 				   tsens-8960.o
 obj-$(CONFIG_QCOM_SPMI_ADC_TM5)	+= qcom-spmi-adc-tm5.o
+obj-$(CONFIG_QCOM_SPMI_MBG_TM)	+= qcom-spmi-mbg-tm.o
 obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
 obj-$(CONFIG_QCOM_LMH)		+= lmh.o
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
+#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
+
+struct mbg_tm_chip {
+	struct regmap			*map;
+	struct device			*dev;
+	struct thermal_zone_device	*tz_dev;
+	struct mutex                    lock;
+	unsigned int			base;
+	int				irq;
+	int				last_temp;
+	bool				last_temp_set;
+	struct iio_channel		*adc;
+};
+
+struct mbg_map_table {
+	int min_temp;
+	int vtemp0;
+	int tc;
+};
+
+static const struct mbg_map_table map_table[] = {
+	/* 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_read(struct mbg_tm_chip *chip, u16 addr, int *data)
+{
+	return regmap_read(chip->map, chip->base + addr, data);
+}
+
+static int mbg_tm_write(struct mbg_tm_chip *chip, u16 addr, int data)
+{
+	return regmap_write(chip->map, chip->base + addr, data);
+}
+
+static int mbg_tm_reg_update(struct mbg_tm_chip *chip, u16 addr, u8 mask, u8 val)
+{
+	return regmap_write_bits(chip->map, chip->base + addr, mask, val);
+}
+
+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_temp_set) {
+		pr_debug("last_temp: %d\n", chip->last_temp);
+		chip->last_temp_set = 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;
+
+	if (temp > MAX_SUPPORTED_TEMP)
+		temp = MAX_SUPPORTED_TEMP - MBG_TEMP_CONSTANT;
+
+	for (idx = 0; idx < ARRAY_SIZE(map_table); idx++)
+		if (temp >= map_table[idx].min_temp &&
+				temp < (map_table[idx].min_temp + 20000)) {
+			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;
+
+	return abs(vtemp - MBG_DEFAULT_TEMP_MV) / STEP_MV;
+}
+
+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);
+
+	/* The HW has a limitation that the trip set must be above 25C */
+	if (temp > MIN_TRIP_TEMP && temp < INT_MAX) {
+		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);
+
+	if (ret < 0)
+		return IRQ_HANDLED;
+
+	val &= MON_FAULT_STATUS_MASK;
+	if ((val >> MON_FAULT_STATUS_SHIFT) & MON2_LVL1_ERR) {
+		chip->last_temp_set = true;
+		thermal_zone_device_update(chip->tz_dev,
+					THERMAL_TRIP_VIOLATED);
+		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;
+
+	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);
+	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);
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, chip->irq, NULL,
+			mbg_tm_isr, IRQF_ONESHOT, node->name, chip);
+
+	return ret;
+}
+
+static const struct of_device_id mbg_tm_match_table[] = {
+	{ .compatible = "qcom,spmi-mbg-tm" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mbg_tm_match_table);
+
+static struct platform_driver mbg_tm_driver = {
+	.driver = {
+		.name = "qcom-spmi-mbg-tm",
+		.of_match_table = mbg_tm_match_table,
+	},
+	.probe  = mbg_tm_probe,
+};
+module_platform_driver(mbg_tm_driver);
+
+MODULE_DESCRIPTION("PMIC MBG Temperature monitor driver");
+MODULE_LICENSE("GPL");

-- 
2.25.1

On 12.07.2024 2:43 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@quicinc.com>
> ---

[...]

>  drivers/thermal/qcom/Kconfig            |  11 ++
>  drivers/thermal/qcom/Makefile           |   1 +
>  drivers/thermal/qcom/qcom-spmi-mbg-tm.c | 269 ++++++++++++++++++++++++++++++++
>  3 files changed, 281 insertions(+)
> 
> diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
> index 2c7f3f9a26eb..46045094020c 100644
> --- a/drivers/thermal/qcom/Kconfig
> +++ b/drivers/thermal/qcom/Kconfig
> @@ -21,6 +21,17 @@ config QCOM_SPMI_ADC_TM5
>  	  Thermal client sets threshold temperature for both warm and cool and
>  	  gets updated when a threshold is reached.
>  
> +config QCOM_SPMI_MBG_TM
> +	tristate "Qualcomm Technologies, Inc. SPMI PMIC MBG Temperature monitor"
> +	depends on OF && SPMI && IIO
> +	select REGMAP_SPMI
> +	help
> +	 This enables a thermal driver for the MBG thermal monitoring device.
> +	 It shows up in sysfs as a thermal sensor with two trip points.
> +	 It notifies the thermal framework when level 1 high threshold is
> +	 violated. The temperature reported by the thermal sensor reflects
> +	 the real time die temperature through ADC channel.
> +
>  config QCOM_SPMI_TEMP_ALARM
>  	tristate "Qualcomm SPMI PMIC Temperature Alarm"
>  	depends on OF && SPMI && IIO
> diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
> index 0fa2512042e7..bc18e08ee3e2 100644
> --- a/drivers/thermal/qcom/Makefile
> +++ b/drivers/thermal/qcom/Makefile
> @@ -4,5 +4,6 @@ obj-$(CONFIG_QCOM_TSENS)	+= qcom_tsens.o
>  qcom_tsens-y			+= tsens.o tsens-v2.o tsens-v1.o tsens-v0_1.o \
>  				   tsens-8960.o
>  obj-$(CONFIG_QCOM_SPMI_ADC_TM5)	+= qcom-spmi-adc-tm5.o
> +obj-$(CONFIG_QCOM_SPMI_MBG_TM)	+= qcom-spmi-mbg-tm.o
>  obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
>  obj-$(CONFIG_QCOM_LMH)		+= lmh.o
> 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
> +#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
> +
> +struct mbg_tm_chip {
> +	struct regmap			*map;
> +	struct device			*dev;
> +	struct thermal_zone_device	*tz_dev;
> +	struct mutex                    lock;
> +	unsigned int			base;
> +	int				irq;
> +	int				last_temp;
> +	bool				last_temp_set;
> +	struct iio_channel		*adc;
> +};
> +
> +struct mbg_map_table {
> +	int min_temp;
> +	int vtemp0;
> +	int tc;
> +};
> +
> +static const struct mbg_map_table map_table[] = {
> +	/* 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_read(struct mbg_tm_chip *chip, u16 addr, int *data)
> +{
> +	return regmap_read(chip->map, chip->base + addr, data);
> +}
> +
> +static int mbg_tm_write(struct mbg_tm_chip *chip, u16 addr, int data)
> +{
> +	return regmap_write(chip->map, chip->base + addr, data);
> +}
> +
> +static int mbg_tm_reg_update(struct mbg_tm_chip *chip, u16 addr, u8 mask, u8 val)
> +{
> +	return regmap_write_bits(chip->map, chip->base + addr, mask, val);
> +}

You're not saving much on code amount, or readability by adding these
accessors, r/w are used once and update is used twice.

> +
> +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_temp_set) {
> +		pr_debug("last_temp: %d\n", chip->last_temp);
> +		chip->last_temp_set = false;

last_temp_set -> last_thres_crossed?

> +		*temp = chip->last_temp;
> +		return 0;
> +	}

[...]

> +
> +	/* The HW has a limitation that the trip set must be above 25C */
> +	if (temp > MIN_TRIP_TEMP && temp < INT_MAX) {

INT_MAX -> MAX_SUPPORTED_TEMP?

> +		mbg_tm_reg_update(chip, MBG_TEMP_MON_MM_MON2_MISC_CFG,
> +					 UP_THRESH_EN, UP_THRESH_EN);

regmap_set_bits

> +		vtemp = temp_to_vtemp(temp);
> +		ret = mbg_tm_write(chip, MON2_LVL1_UP_THRESH, vtemp);

regmap_write(...., temp_to_vtemp(temp))

> +		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);

regmap_clear_bits

[...]

> +	val &= MON_FAULT_STATUS_MASK;
> +	if ((val >> MON_FAULT_STATUS_SHIFT) & MON2_LVL1_ERR) {

FIELD_GET

> +		chip->last_temp_set = true;
> +		thermal_zone_device_update(chip->tz_dev,
> +					THERMAL_TRIP_VIOLATED);
> +		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");

Would such spurious IRQs carry any meaning at all?

[...]

> +static struct platform_driver mbg_tm_driver = {
> +	.driver = {
> +		.name = "qcom-spmi-mbg-tm",
> +		.of_match_table = mbg_tm_match_table,
> +	},
> +	.probe  = mbg_tm_probe,

Double space

Konrad

On Fri, 12 Jul 2024 18:13:30 +0530
Satya Priya Kakitapalli <quic_skakitap@quicinc.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@quicinc.com>

Hi Satya,

Some comments inline.

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

> +}

On 7/13/2024 5:29 PM, Jonathan Cameron wrote:
> On Fri, 12 Jul 2024 18:13:30 +0530
> Satya Priya Kakitapalli <quic_skakitap@quicinc.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@quicinc.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...
>
>> +}