Re: [PATCH v5 4/4] thermal/drivers/loongson2: Add support for Loongson-2K0300

[Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>]

Hi Binbin,


On 9/17/26 13:15, Binbin Zhou wrote:
> The Loongson-2K0300 thermal sensor uses a 10-bit ADC and requires

The commit message says this is a 10-bit ADC, while the code uses bits 
[10:0]. The 2K0300 user manual also describes Thsens_val[10:0], i.e. an 
11-bit value. Should this say 11-bit instead?

> per-chip calibration. The calibration offset is stored in the CHIP ID
> registers, which are accessed via syscon. The driver reads this offset
> and applies it when converting the raw ADC value to millicelsius.
> 
> To handle old fuse versions that cannot be calibrated correctly, the
> driver includes a fallback formula and a validity check. Once invalid
> data is detected, the driver falls back to the old formula for future
> reads and warns the user.
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>   drivers/thermal/loongson2_thermal.c | 94 ++++++++++++++++++++++++++++-
>   1 file changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongson2_thermal.c
> index fb4793a9bc55..8930a8a75da7 100644
> --- a/drivers/thermal/loongson2_thermal.c
> +++ b/drivers/thermal/loongson2_thermal.c
> @@ -2,9 +2,11 @@
>   /*
>    * Author: zhanghongchen <zhanghongchen@loongson.cn>
>    *         Yinbo Zhu <zhuyinbo@loongson.cn>
> + *         Binbin Zhou <zhoubinbin@loongson.cn>
>    * Copyright (C) 2022-2023 Loongson Technology Corporation Limited
>    */
>   
> +#include <linux/bitfield.h>
>   #include <linux/interrupt.h>
>   #include <linux/io.h>
>   #include <linux/minmax.h>
> @@ -13,6 +15,9 @@
>   #include <linux/property.h>
>   #include <linux/thermal.h>
>   #include <linux/units.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/syscore_ops.h>

Is it enabled ?

>   
>   #include "thermal_hwmon.h"
>   
> @@ -22,18 +27,34 @@
>   #define LOONGSON2_THSENS_CTRL_LOW_REG	0x8
>   #define LOONGSON2_THSENS_STATUS_REG	0x10
>   #define LOONGSON2_THSENS_OUT_REG	0x14
> +#define LOONGSON2_THSENS_CFG_REG	0x18
>   
>   #define LOONGSON2_THSENS_INT_LO		BIT(0)
>   #define LOONGSON2_THSENS_INT_HIGH	BIT(1)
>   #define LOONGSON2_THSENS_INT_EN		(LOONGSON2_THSENS_INT_LO | \
>   					 LOONGSON2_THSENS_INT_HIGH)
> -#define LOONGSON2_THSENS_OUT_MASK	0xFF
> +#define LOONGSON2_THSENS_OUT_8B_MASK	0xFF
> +#define LOONGSON2_THSENS_OUT_11B_MASK	GENMASK(10, 0)
> +
> +#define LS2K0300_CHIP_ID0		0x10
> +#define LS2K0300_CHIP_ID1		0x14
> +#define LS2K0300_EXTERN_ID		BIT(4)
> +#define LS2K0300_ID0_VAL_MASK		GENMASK(31, 20)
> +#define LS2K0300_ID1_VAL_MASK		GENMASK(15, 0)
> +
> +#define LS2K0300_COMP_VAL_MASK		GENMASK(14, 0)
> +#define LS2K0300_COMP_SIGN_BIT		BIT(15)
> +
> +#define LS2K0300_LOWEST_VALID_TEMP	(-55000)
> +#define LS2K0300_HIGHEST_VALID_TEMP	(125000)
>   
>   /*
>    * This flag is used to indicate the temperature reading
>    * method of the Loongson-2K2000
>    */
>   #define LS2K2000_THSENS_OUT_FLAG	BIT(0)
> +#define LS2K0300_CHIP_ID_FLAG		BIT(1)
> +#define LS2K0300_OLD_FUSE_FLAG		BIT(2)
>   
>   struct loongson2_thermal_chip_data {
>   	unsigned int thermal_sensor_sel;
> @@ -42,8 +63,11 @@ struct loongson2_thermal_chip_data {
>   };
>   
>   struct loongson2_thermal_data {
> +	struct device *dev;
>   	void __iomem *ctrl_reg;
>   	void __iomem *temp_reg;
> +	struct regmap *regmap_cfg;
> +	u32 flags;
>   	const struct loongson2_thermal_chip_data *chip_data;
>   };
>   
> @@ -71,13 +95,54 @@ static int loongson2_thermal_set(struct loongson2_thermal_data *data,
>   	return 0;
>   }
>   
> +static int loongson2_2k0300_get_temp(struct thermal_zone_device *tz, int *temp)
> +{
> +	struct loongson2_thermal_data *tdata = thermal_zone_device_priv(tz);
> +	int calib_data, calib_offset, temp_mc, raw_adc;
> +	u32 chip_id0 = 0, chip_id1 = 0;
> +
> +	writel(0xff03, tdata->ctrl_reg + LOONGSON2_THSENS_CFG_REG);

Please no litterals or magic values in the code. Add a define with a 
self-explanatory names and a comment if it makes sense (there are other 
places in the code to be checked).

> +	raw_adc = FIELD_GET(LOONGSON2_THSENS_OUT_11B_MASK,
> +			    readl(tdata->ctrl_reg + LOONGSON2_THSENS_OUT_REG));

Why is it done at every read and not at probe time ?

> +
> +	if (tdata->flags & LS2K0300_OLD_FUSE_FLAG) {
> +		*temp = raw_adc * 569 - 394700;

				no litterals ...

and don't repeat the formula, write a function for it

> +		return 0;
> +	}
> +
> +	regmap_read(tdata->regmap_cfg, LS2K0300_CHIP_ID0, &chip_id0);
> +	regmap_read(tdata->regmap_cfg, LS2K0300_CHIP_ID1, &chip_id1);

The return values of regmap_read() are ignored here. If accessing the 
CHIP ID registers fails, the driver will silently use zero or partially 
initialized calibration data and may report a plausible but incorrect 
temperature. Could you propagate the error instead?
> +	if (chip_id0 & LS2K0300_EXTERN_ID) {
> +		calib_data = FIELD_GET(LS2K0300_ID1_VAL_MASK, chip_id1);
> +		calib_offset = FIELD_GET(LS2K0300_COMP_VAL_MASK, calib_data);
> +		if (calib_data & LS2K0300_COMP_SIGN_BIT)
> +			calib_offset = -calib_offset;
> +	} else {
> +		calib_data = FIELD_GET(LS2K0300_ID0_VAL_MASK, chip_id0);
> +		calib_offset = FIELD_GET(LS2K0300_COMP_VAL_MASK, calib_data);
> +	}
> +
> +	temp_mc = (raw_adc + calib_offset) * 570 - 394700;

				no litterals + formula ...

> +
> +	/* For old fuse which can not read right thermal data */
> +	if (temp_mc < LS2K0300_LOWEST_VALID_TEMP || temp_mc > LS2K0300_HIGHEST_VALID_TEMP) {
> +		dev_warn_once(tdata->dev, "It's an old fuse, thermal %d is not right\n", temp_mc);

Improve the message please

> +		tdata->flags |= LS2K0300_OLD_FUSE_FLAG;
> +		temp_mc = raw_adc * 569 - 394700;
> +	}
> +	*temp = temp_mc;
> +
> +	return 0;
> +}
> +
>   static int loongson2_2k1000_get_temp(struct thermal_zone_device *tz, int *temp)
>   {
>   	int val;
>   	struct loongson2_thermal_data *data = thermal_zone_device_priv(tz);
>   
>   	val = readl(data->ctrl_reg + LOONGSON2_THSENS_OUT_REG);
> -	*temp = ((val & LOONGSON2_THSENS_OUT_MASK) - HECTO) * KILO;
> +	*temp = ((val & LOONGSON2_THSENS_OUT_8B_MASK) - HECTO) * KILO;
>   
>   	return 0;
>   }
> @@ -112,6 +177,11 @@ static int loongson2_thermal_set_trips(struct thermal_zone_device *tz, int low,
>   	return loongson2_thermal_set(data, low/MILLI, high/MILLI, true);
>   }
>   
> +static const struct thermal_zone_device_ops loongson2_2k0300_of_thermal_ops = {
> +	.get_temp = loongson2_2k0300_get_temp,
> +	.set_trips = loongson2_thermal_set_trips,
> +};
> +
>   static const struct thermal_zone_device_ops loongson2_2k1000_of_thermal_ops = {
>   	.get_temp = loongson2_2k1000_get_temp,
>   	.set_trips = loongson2_thermal_set_trips,

Can you confirm the set_trips function is the same for 2k1000 and 2k0300 ?

> @@ -134,6 +204,8 @@ static int loongson2_thermal_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	data->chip_data = device_get_match_data(dev);
> +	data->flags = data->chip_data->flags;
> +	data->dev = dev;
>   
>   	data->ctrl_reg = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(data->ctrl_reg))
> @@ -146,6 +218,14 @@ static int loongson2_thermal_probe(struct platform_device *pdev)
>   			return PTR_ERR(data->temp_reg);
>   	}
>   
> +	/* The chip id register is needed for Loongson-2K0300 */
> +	if (data->chip_data->flags & LS2K0300_CHIP_ID_FLAG) {
> +		data->regmap_cfg =
> +			syscon_regmap_lookup_by_phandle(dev->of_node, "loongson,chipid");
> +		if (IS_ERR(data->regmap_cfg))
> +			return PTR_ERR(data->regmap_cfg);
> +	}
> +
>   	irq = platform_get_irq(pdev, 0);
>   	if (irq < 0)
>   		return irq;
> @@ -178,6 +258,12 @@ static int loongson2_thermal_probe(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static const struct loongson2_thermal_chip_data loongson2_thermal_ls2k0300_data = {
> +	.thermal_sensor_sel = 0,
> +	.flags = LS2K0300_CHIP_ID_FLAG,
> +	.thermal_ops = &loongson2_2k0300_of_thermal_ops,
> +};
> +
>   static const struct loongson2_thermal_chip_data loongson2_thermal_ls2k1000_data = {
>   	.thermal_sensor_sel = 0,
>   	.flags = 0,
> @@ -191,6 +277,10 @@ static const struct loongson2_thermal_chip_data loongson2_thermal_ls2k2000_data
>   };
>   
>   static const struct of_device_id of_loongson2_thermal_match[] = {
> +	{
> +		.compatible = "loongson,ls2k0300-thermal",
> +		.data = &loongson2_thermal_ls2k0300_data,
> +	},
>   	{
>   		.compatible = "loongson,ls2k1000-thermal",
>   		.data = &loongson2_thermal_ls2k1000_data,