| Message ID | 20211126152109.230986-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [1/3] ACPI: PMIC: constify all struct intel_pmic_opregion_data declarations | expand |
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. > + raw = clamp(raw, first.raw, last.raw); > + else > + raw = clamp(raw, last.raw, first.raw); clamp_value() slightly better due to type checking. > + > + 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 >
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 >> >
On Sat, Nov 27, 2021 at 11:59 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/26/21 16:56, Andy Shevchenko wrote: > > On Fri, Nov 26, 2021 at 04:21:09PM +0100, Hans de Goede wrote: ... > >> + /* > >> + * 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. Thanks for explanation. Never thought that somebody can put tables in reversed order. ... > >> + 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() Pardon me, I was confused myself, it's actually other way around, so clamp() does require strict types, while clamp_val() for the cases when range is not of the same type as value.
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) + raw = clamp(raw, first.raw, last.raw); + else + raw = clamp(raw, last.raw, first.raw); + + 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,
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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/pmic/intel_pmic_xpower.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)