Re: [PATCH 4/5] thermal: qcom-spmi-temp-alarm: add support for GEN2 rev 2 PMIC peripherals

[Anjelique Melendez <quic_amelende@quicinc.com>]

>>  
>> +/* Configure TEMP_DAC registers based on DT thermal_zone trips */
>> +static int qpnp_tm_gen2_rev2_update_trip_temps(struct qpnp_tm_chip *chip)
>> +{
>> +	struct thermal_trip trip = {0};
>> +	int ret, ntrips, i;
>> +
>> +	ntrips = thermal_zone_get_num_trips(chip->tz_dev);
>> +	/* Keep hardware defaults if no DT trips are defined. */
>> +	if (ntrips <= 0)
>> +		return 0;
>> +
>> +	for (i = 0; i < ntrips; i++) {
>> +		ret = thermal_zone_get_trip(chip->tz_dev, i, &trip);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		ret = qpnp_tm_gen2_rev2_set_temp_thresh(chip, i, trip.temperature);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	/* Verify that trips are strictly increasing. */
> 
> There is no such requirement in the DT bindings. Please don't invent
> artificial restrictions, especially if they are undocumented.
> 

This is not an entirely new restirction. Currently the temp alarm driver
has hardcoded temperature thresholds options which are "strictly increasing"
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/qcom/qcom-spmi-temp-alarm.c?h=v6.11-rc1#n44). 
The threshold values are initially configured based on the stage 2 critical trip
temperature.
For newer PMICs, we have individual temperature registers for stage 1, 2, and 3,
so we instead configure each threshold temperature as defined in DT. In general
since stage 1 = warning, stage 2 = system should shut down, stage 3 = emergency shutdown,
we would expect for temperature thresholds to increase for each stage
(https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/thermal?h=v5.4.281&id=f1599f9e4cd6f1dd0cad202853fb830854f4e944).

I agree that we are missing some documentation but since the trips are defined in the
thermal_zone node what is the best way to mention this requirement? Will adding a
few sentences to qcom,spmi-temp-alarm.yaml description be enough? Do we need
to make changes to thermal_zone.yaml so that dt_binding_check catches this requirement? 

>> +	for (i = 1; i < STAGE_COUNT; i++) {
>> +		if (chip->temp_dac_map[i] <= chip->temp_dac_map[i - 1]) {
>> +			dev_err(chip->dev, "Threshold %d=%ld <= threshold %d=%ld\n",
>> +				i, chip->temp_dac_map[i], i - 1,
>> +				chip->temp_dac_map[i - 1]);
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	return 0;
Thanks,
Anjelique