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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <35402164-dce0-b972-bf98-c025fe5620f0@quicinc.com>
Date: Tue, 30 Jul 2024 15:44:06 -0700
From: Anjelique Melendez <quic_amelende@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
        <amitk@...nel.org>, <thara.gopinath@...il.com>, <andersson@...nel.org>,
        <quic_collinsd@...cinc.com>, <rafael@...nel.org>,
        <daniel.lezcano@...aro.org>, <rui.zhang@...el.com>,
        <lukasz.luba@....com>, <linux-arm-msm@...r.kernel.org>,
        <linux-pm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/5] thermal: qcom-spmi-temp-alarm: add support for GEN2
 rev 2 PMIC peripherals

>>  
>> +/* 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