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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] thermal: loongson2: invalid thermal trip thresholds written for 2K0300
- [Medium] thermal: loongson2: missing regmap_read error check and inefficient static data polling
- [Medium] thermal: loongson2: transient ADC errors permanently latch old fuse state
--

commit b926fbbb617cddc0a0e12efb6da240071ce01710
Author: Binbin Zhou <zhoubinbin@loongson.cn>

thermal/drivers/loongson2: Add support for Loongson-2K0300

The Loongson-2K0300 thermal sensor uses a 10-bit ADC and requires
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.

> diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongson2_thermal.c
> index fb4793a9bc554..8930a8a75da78 100644
> --- a/drivers/thermal/loongson2_thermal.c
> +++ b/drivers/thermal/loongson2_thermal.c

[ ... ]

> @@ -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);
> +	raw_adc = FIELD_GET(LOONGSON2_THSENS_OUT_11B_MASK,
> +			    readl(tdata->ctrl_reg + LOONGSON2_THSENS_OUT_REG));
> +
> +	if (tdata->flags & LS2K0300_OLD_FUSE_FLAG) {
> +		*temp = raw_adc * 569 - 394700;
> +		return 0;
> +	}
> +
> +	regmap_read(tdata->regmap_cfg, LS2K0300_CHIP_ID0, &chip_id0);
> +	regmap_read(tdata->regmap_cfg, LS2K0300_CHIP_ID1, &chip_id1);

[Severity: Medium]
Should we check the return value of regmap_read() here?

If the read fails, chip_id0 and chip_id1 will remain uninitialized (zero),
which leads to a silent incorrect calibration offset and corrupts the
temperature output.

Also, since this is static calibration data, could we read it just once during
probe() instead of repeatedly polling it in the hot path?

> +
> +	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;
> +
> +	/* 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);
> +		tdata->flags |= LS2K0300_OLD_FUSE_FLAG;
> +		temp_mc = raw_adc * 569 - 394700;
> +	}

[Severity: Medium]
Is it safe to permanently latch the LS2K0300_OLD_FUSE_FLAG here based on a
dynamic reading?

If a single anomalous or transient ADC reading causes temp_mc to fall out of
bounds, the driver will permanently downgrade the sensor to the uncalibrated
fallback formula for all future reads until the system reboots.

Should the old fuse detection be strictly based on static fuse register data
instead?

> +	*temp = temp_mc;
> +
> +	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,
> +};

[Severity: High]
Does loongson2_thermal_set_trips() work correctly for the 2K0300 device?

The 2K1000 ADC outputs directly in Celsius + 100 (8-bit), which is what
loongson2_set_ctrl_regs() expects and writes to the threshold registers.

However, the new 2K0300 ADC outputs an 11-bit raw value mapped via
(raw_adc + offset) * 570 - 394700. By reusing the legacy .set_trips callback,
the 2K0300 writes the 8-bit Celsius + 100 value to hardware, fundamentally
mismatching the 11-bit raw scale the comparator requires.

Could this cause incorrect hardware thermal limits or missed interrupts for
the 2K0300?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789546118.git.zhoubinbin@loongson.cn?part=4