Re: [PATCH 3/3] ACPI: PMIC: xpower: Fix _TMP ACPI errors

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/26/21 16:56, Andy Shevchenko wrote:
> On Fri, Nov 26, 2021 at 04:21:09PM +0100, Hans de Goede wrote:
>> On some devices with a X-Powers AXP288 PMIC the LPAT tables in the ACPI
>> node for the AXP288 PMIC for some reason only describe a small temperature
>> range, e.g. 27° - 37° Celcius (assuming the entries are in millidegrees).
>>
>> When the tablet is idle in a room at 21° degrees this is causing values
>> outside the LPAT table to be read, causing e.g. the following 2 errors
>> to get spammed to the logs every 4 seconds! :
>>
>> [ 7512.791316] ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion] (20210930/evregion-281)
>> [ 7512.791611] ACPI Error: Aborting method \_SB.SXP1._TMP due to previous error (AE_ERROR) (20210930/psparse-529)
>>
>> Fix this by clamping the raw value to the LPAT table range before
>> passing it to acpi_lpat_raw_to_temp().
>>
>> Note clamping has been chosen rather then extrapolating because it is
>> unknown how other parts of the ACPI tables will respond to temperature
>> values outside of the LPAT range.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> 
> One nit-pick below.
> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/acpi/pmic/intel_pmic_xpower.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
>> index e844bc1f3df5..61bbe4c24d87 100644
>> --- a/drivers/acpi/pmic/intel_pmic_xpower.c
>> +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
>> @@ -293,12 +293,33 @@ static int intel_xpower_exec_mipi_pmic_seq_element(struct regmap *regmap,
>>  	return ret;
>>  }
>>  
>> +static int intel_xpower_lpat_raw_to_temp(struct acpi_lpat_conversion_table *lpat_table,
>> +					 int raw)
>> +{
>> +	struct acpi_lpat first = lpat_table->lpat[0];
>> +	struct acpi_lpat last = lpat_table->lpat[lpat_table->lpat_count - 1];
>> +
>> +	/*
>> +	 * Some LPAT tables in the ACPI Device for the AXP288 PMIC for some
>> +	 * reason only describe a small temperature range, e.g. 27° - 37°
>> +	 * Celcius. Resulting in errors when the tablet is idle in a cool room.
>> +	 *
>> +	 * To avoid these errors clamp the raw value to be inside the LPAT.
>> +	 */
> 
>> +	if (first.raw < last.raw)
> 
> Wondering what that would mean if this condition is false.

That the tables is in descending raw value order, rather then
in ascending one. Which quite a few LPAT tables actually are.

The existing acpi_lpat_raw_to_temp() has been carefully written
to be able to handle both cases too.

> 
>> +		raw = clamp(raw, first.raw, last.raw);
>> +	else
>> +		raw = clamp(raw, last.raw, first.raw);
> 
> clamp_value() slightly better due to type checking.

Quoting from include/linux/minmax.h :

 * This macro does strict typechecking of @lo/@hi to make sure they are of the
 * same type as @val.  See the unnecessary pointer comparisons.
 */
#define clamp(val, lo, hi) min((typeof(val))max(val, lo), hi)

So we already get strict type-checking from plain clamp()

Regards,

Hans



> 
>> +
>> +	return acpi_lpat_raw_to_temp(lpat_table, raw);
>> +}
>> +
>>  static const struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
>>  	.get_power = intel_xpower_pmic_get_power,
>>  	.update_power = intel_xpower_pmic_update_power,
>>  	.get_raw_temp = intel_xpower_pmic_get_raw_temp,
>>  	.exec_mipi_pmic_seq_element = intel_xpower_exec_mipi_pmic_seq_element,
>> -	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
>> +	.lpat_raw_to_temp = intel_xpower_lpat_raw_to_temp,
>>  	.power_table = power_table,
>>  	.power_table_count = ARRAY_SIZE(power_table),
>>  	.thermal_table = thermal_table,
>> -- 
>> 2.33.1
>>
>