Re: [PATCH 3/5] thermal: qcom: Add support for MBG Temp monitor

[Konrad Dybcio <konrad.dybcio@linaro.org>]

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