Re: [PATCH 2/2] thermal: qcom: Add support for Qualcomm MBG thermal monitoring

[Sachin Gupta <sachin.gupta@oss.qualcomm.com>]

On 6/16/2026 3:40 PM, Konrad Dybcio wrote:
> On 6/1/26 1:01 PM, Sachin Gupta wrote:
>> From: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>>
>> Add driver for the Qualcomm 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>
>> Co-developed-by: Sachin Gupta <sachin.gupta@oss.qualcomm.com>
>> Signed-off-by: Sachin Gupta <sachin.gupta@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> +static const struct mbg_map_table map_table[] = {
>> +	/* minT	vtemp0	tc */
> 
> The struct is defined 2 lines above, the reader can tell the names
> of the fields

I will remove this in the next patch series.

>> +	{ -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 },
>> +};
> 
> Please add a comment stating this map is not PMIC-specific
> 

This table is PMIC-specific and applies only to the PM8775 MBG block.
MBG is used only on PM8775 PMIC.

> [...]
> 
>> +	/* The HW has a limitation that the trip set must be above 25C */
>> +	if (temp > MBG_MIN_TRIP_TEMP && temp < MBG_MAX_SUPPORTED_TEMP) {
>> +		ret = regmap_set_bits(chip->map, chip->base + MBG_TEMP_MON2_MISC_CFG,
>> +				      MON2_UP_THRESH_EN);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		ret = regmap_write(chip->map, chip->base + MON2_LVL1_UP_THRESH,
>> +				   temp_to_vtemp_mv(temp));
>> +		if (ret < 0)
>> +			return ret;
>> +	} else {
>> +		dev_dbg(chip->dev, "Set trip b/w 25C and 160C\n");
> 
> Should this be an error print, returning an error condition?
> 

Yes, this should be treated as an error path. For out-of-range trip 
requests, I will return -ERANGE and update the log to an error-level 
message in the next patch series.

>> +		ret = regmap_clear_bits(chip->map, chip->base + MBG_TEMP_MON2_MISC_CFG,
>> +					MON2_UP_THRESH_EN);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * 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;
> 
> Will this work fine if the user tries to set the max temp supported
> by the hardware (i.e. is there headroom for max+1)?
> 

In the current implementation, temp == MBG_MAX_SUPPORTED_TEMP is not 
accepted (temp < MBG_MAX_SUPPORTED_TEMP), so the last_temp = temp + 
MBG_TEMP_CONSTANT path is never taken at absolute max. For accepted 
trips (strictly below max), there is headroom for the +1C adjustment.

>> +
>> +	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, val;
>> +
>> +	scoped_guard(mutex, &chip->lock) {
>> +		ret = regmap_read(chip->map, chip->base + MBG_TEMP_MON2_FAULT_STATUS, &val);
>> +		if (ret < 0)
>> +			return IRQ_HANDLED;
>> +	}
>> +
>> +	if (FIELD_GET(MON_FAULT_STATUS_MASK, val) & MON_FAULT_LVL1_UPR) {
>> +		chip->last_thres_crossed = true;
>> +		dev_dbg(chip->dev, "Notifying Thermal, fault status=%d\n", val);
>> +		thermal_zone_device_update(chip->tz_dev, THERMAL_TRIP_VIOLATED);
> 
> Should the assignment and this call also be guarded by the mutex?
> 
> Konrad

Yes, agreed. I will update this locking in the next patch series.

Thanks,
Sachin