[PATCH 1/3] ACPI: PMIC: constify all struct intel_pmic_opregion_data declarations

[PATCH 1/3] ACPI: PMIC: constify all struct intel_pmic_opregion_data declarations

[Hans de Goede]

The struct intel_pmic_opregion_data declarations never change,
constify them all.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/pmic/intel_pmic.c          | 12 ++++++------
 drivers/acpi/pmic/intel_pmic.h          |  4 +++-
 drivers/acpi/pmic/intel_pmic_bxtwc.c    |  2 +-
 drivers/acpi/pmic/intel_pmic_bytcrc.c   |  2 +-
 drivers/acpi/pmic/intel_pmic_chtcrc.c   |  2 +-
 drivers/acpi/pmic/intel_pmic_chtdc_ti.c |  2 +-
 drivers/acpi/pmic/intel_pmic_chtwc.c    |  2 +-
 drivers/acpi/pmic/intel_pmic_xpower.c   |  2 +-
 8 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
index 9cde299eba88..98bbfd99c709 100644
--- a/drivers/acpi/pmic/intel_pmic.c
+++ b/drivers/acpi/pmic/intel_pmic.c
@@ -25,7 +25,7 @@ struct intel_pmic_opregion {
 	struct mutex lock;
 	struct acpi_lpat_conversion_table *lpat_table;
 	struct regmap *regmap;
-	struct intel_pmic_opregion_data *data;
+	const struct intel_pmic_opregion_data *data;
 	struct intel_pmic_regs_handler_ctx ctx;
 };
 
@@ -53,7 +53,7 @@ static acpi_status intel_pmic_power_handler(u32 function,
 {
 	struct intel_pmic_opregion *opregion = region_context;
 	struct regmap *regmap = opregion->regmap;
-	struct intel_pmic_opregion_data *d = opregion->data;
+	const struct intel_pmic_opregion_data *d = opregion->data;
 	int reg, bit, result;
 
 	if (bits != 32 || !value64)
@@ -135,7 +135,7 @@ static int pmic_thermal_aux(struct intel_pmic_opregion *opregion, int reg,
 static int pmic_thermal_pen(struct intel_pmic_opregion *opregion, int reg,
 			    int bit, u32 function, u64 *value)
 {
-	struct intel_pmic_opregion_data *d = opregion->data;
+	const struct intel_pmic_opregion_data *d = opregion->data;
 	struct regmap *regmap = opregion->regmap;
 
 	if (!d->get_policy || !d->update_policy)
@@ -171,7 +171,7 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
 		void *handler_context, void *region_context)
 {
 	struct intel_pmic_opregion *opregion = region_context;
-	struct intel_pmic_opregion_data *d = opregion->data;
+	const struct intel_pmic_opregion_data *d = opregion->data;
 	int reg, bit, result;
 
 	if (bits != 32 || !value64)
@@ -255,7 +255,7 @@ static acpi_status intel_pmic_regs_handler(u32 function,
 
 int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
 					struct regmap *regmap,
-					struct intel_pmic_opregion_data *d)
+					const struct intel_pmic_opregion_data *d)
 {
 	acpi_status status = AE_OK;
 	struct intel_pmic_opregion *opregion;
@@ -344,7 +344,7 @@ EXPORT_SYMBOL_GPL(intel_pmic_install_opregion_handler);
 int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address,
 					      u32 value, u32 mask)
 {
-	struct intel_pmic_opregion_data *d;
+	const struct intel_pmic_opregion_data *d;
 	int ret;
 
 	if (!intel_pmic_opregion) {
diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h
index 89379476a1df..467a39966dc8 100644
--- a/drivers/acpi/pmic/intel_pmic.h
+++ b/drivers/acpi/pmic/intel_pmic.h
@@ -25,6 +25,8 @@ struct intel_pmic_opregion_data {
 	int pmic_i2c_address;
 };
 
-int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle, struct regmap *regmap, struct intel_pmic_opregion_data *d);
+int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
+					struct regmap *regmap,
+					const struct intel_pmic_opregion_data *d);
 
 #endif
diff --git a/drivers/acpi/pmic/intel_pmic_bxtwc.c b/drivers/acpi/pmic/intel_pmic_bxtwc.c
index bd7621edd60b..6620ce0833f6 100644
--- a/drivers/acpi/pmic/intel_pmic_bxtwc.c
+++ b/drivers/acpi/pmic/intel_pmic_bxtwc.c
@@ -369,7 +369,7 @@ intel_bxtwc_pmic_update_policy(struct regmap *regmap,
 	return regmap_update_bits(regmap, reg, mask, val);
 }
 
-static struct intel_pmic_opregion_data intel_bxtwc_pmic_opregion_data = {
+static const struct intel_pmic_opregion_data intel_bxtwc_pmic_opregion_data = {
 	.get_power      = intel_bxtwc_pmic_get_power,
 	.update_power   = intel_bxtwc_pmic_update_power,
 	.get_raw_temp   = intel_bxtwc_pmic_get_raw_temp,
diff --git a/drivers/acpi/pmic/intel_pmic_bytcrc.c b/drivers/acpi/pmic/intel_pmic_bytcrc.c
index 2a692cc4b7ae..8a1d895ed689 100644
--- a/drivers/acpi/pmic/intel_pmic_bytcrc.c
+++ b/drivers/acpi/pmic/intel_pmic_bytcrc.c
@@ -271,7 +271,7 @@ static int intel_crc_pmic_update_policy(struct regmap *regmap,
 	return 0;
 }
 
-static struct intel_pmic_opregion_data intel_crc_pmic_opregion_data = {
+static const struct intel_pmic_opregion_data intel_crc_pmic_opregion_data = {
 	.get_power	= intel_crc_pmic_get_power,
 	.update_power	= intel_crc_pmic_update_power,
 	.get_raw_temp	= intel_crc_pmic_get_raw_temp,
diff --git a/drivers/acpi/pmic/intel_pmic_chtcrc.c b/drivers/acpi/pmic/intel_pmic_chtcrc.c
index 2900dc3074d2..d8266d22cd8e 100644
--- a/drivers/acpi/pmic/intel_pmic_chtcrc.c
+++ b/drivers/acpi/pmic/intel_pmic_chtcrc.c
@@ -23,7 +23,7 @@
  * intel_soc_pmic_exec_mipi_pmic_seq_element work on devices with a
  * CHT Crystal Cove PMIC.
  */
-static struct intel_pmic_opregion_data intel_chtcrc_pmic_opregion_data = {
+static const struct intel_pmic_opregion_data intel_chtcrc_pmic_opregion_data = {
 	.pmic_i2c_address = 0x6e,
 };
 
diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
index fef7831d0d63..cb444ddec5a0 100644
--- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
+++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
@@ -94,7 +94,7 @@ static int chtdc_ti_pmic_get_raw_temp(struct regmap *regmap, int reg)
 	return ((buf[0] & 0x03) << 8) | buf[1];
 }
 
-static struct intel_pmic_opregion_data chtdc_ti_pmic_opregion_data = {
+static const struct intel_pmic_opregion_data chtdc_ti_pmic_opregion_data = {
 	.get_power = chtdc_ti_pmic_get_power,
 	.update_power = chtdc_ti_pmic_update_power,
 	.get_raw_temp = chtdc_ti_pmic_get_raw_temp,
diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
index 7ffd5624b8e1..59385a9a05e5 100644
--- a/drivers/acpi/pmic/intel_pmic_chtwc.c
+++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
@@ -253,7 +253,7 @@ static int intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap,
  * The thermal table and ops are empty, we do not support the Thermal opregion
  * (DPTF) due to lacking documentation.
  */
-static struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
+static const struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
 	.get_power		= intel_cht_wc_pmic_get_power,
 	.update_power		= intel_cht_wc_pmic_update_power,
 	.exec_mipi_pmic_seq_element = intel_cht_wc_exec_mipi_pmic_seq_element,
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index cbe08e600fa3..b5f4d81c621f 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -293,7 +293,7 @@ static int intel_xpower_exec_mipi_pmic_seq_element(struct regmap *regmap,
 	return ret;
 }
 
-static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
+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,
-- 
2.33.1

[PATCH 2/3] ACPI: PMIC: allow drivers to provide a custom lpat_raw_to_temp() function

[Hans de Goede]

The LPAT tables used in the DSDT for some PMICs require special handling,
allow the PMIC OpRegion drivers to provide an alternative implementation
by adding a lpat_raw_to_temp function pointer to struct pmic_table;
and initialize this to the default acpi_lpat_raw_to_temp function
for all PMICs.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/pmic/intel_pmic.c          | 2 +-
 drivers/acpi/pmic/intel_pmic.h          | 4 ++++
 drivers/acpi/pmic/intel_pmic_bxtwc.c    | 1 +
 drivers/acpi/pmic/intel_pmic_bytcrc.c   | 1 +
 drivers/acpi/pmic/intel_pmic_chtcrc.c   | 1 +
 drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 1 +
 drivers/acpi/pmic/intel_pmic_chtwc.c    | 1 +
 drivers/acpi/pmic/intel_pmic_xpower.c   | 1 +
 8 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
index 98bbfd99c709..f20dbda1a831 100644
--- a/drivers/acpi/pmic/intel_pmic.c
+++ b/drivers/acpi/pmic/intel_pmic.c
@@ -95,7 +95,7 @@ static int pmic_read_temp(struct intel_pmic_opregion *opregion,
 		return 0;
 	}
 
-	temp = acpi_lpat_raw_to_temp(opregion->lpat_table, raw_temp);
+	temp = opregion->data->lpat_raw_to_temp(opregion->lpat_table, raw_temp);
 	if (temp < 0)
 		return temp;
 
diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h
index 467a39966dc8..d956b03a6ca0 100644
--- a/drivers/acpi/pmic/intel_pmic.h
+++ b/drivers/acpi/pmic/intel_pmic.h
@@ -2,6 +2,8 @@
 #ifndef __INTEL_PMIC_H
 #define __INTEL_PMIC_H
 
+#include <acpi/acpi_lpat.h>
+
 struct pmic_table {
 	int address;	/* operation region address */
 	int reg;	/* corresponding thermal register */
@@ -17,6 +19,8 @@ struct intel_pmic_opregion_data {
 	int (*update_policy)(struct regmap *r, int reg, int bit, int enable);
 	int (*exec_mipi_pmic_seq_element)(struct regmap *r, u16 i2c_address,
 					  u32 reg_address, u32 value, u32 mask);
+	int (*lpat_raw_to_temp)(struct acpi_lpat_conversion_table *lpat_table,
+				int raw);
 	struct pmic_table *power_table;
 	int power_table_count;
 	struct pmic_table *thermal_table;
diff --git a/drivers/acpi/pmic/intel_pmic_bxtwc.c b/drivers/acpi/pmic/intel_pmic_bxtwc.c
index 6620ce0833f6..e247615189fa 100644
--- a/drivers/acpi/pmic/intel_pmic_bxtwc.c
+++ b/drivers/acpi/pmic/intel_pmic_bxtwc.c
@@ -376,6 +376,7 @@ static const struct intel_pmic_opregion_data intel_bxtwc_pmic_opregion_data = {
 	.update_aux     = intel_bxtwc_pmic_update_aux,
 	.get_policy     = intel_bxtwc_pmic_get_policy,
 	.update_policy  = intel_bxtwc_pmic_update_policy,
+	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
 	.power_table      = power_table,
 	.power_table_count = ARRAY_SIZE(power_table),
 	.thermal_table     = thermal_table,
diff --git a/drivers/acpi/pmic/intel_pmic_bytcrc.c b/drivers/acpi/pmic/intel_pmic_bytcrc.c
index 8a1d895ed689..9ea79f210965 100644
--- a/drivers/acpi/pmic/intel_pmic_bytcrc.c
+++ b/drivers/acpi/pmic/intel_pmic_bytcrc.c
@@ -278,6 +278,7 @@ static const struct intel_pmic_opregion_data intel_crc_pmic_opregion_data = {
 	.update_aux	= intel_crc_pmic_update_aux,
 	.get_policy	= intel_crc_pmic_get_policy,
 	.update_policy	= intel_crc_pmic_update_policy,
+	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
 	.power_table	= power_table,
 	.power_table_count= ARRAY_SIZE(power_table),
 	.thermal_table	= thermal_table,
diff --git a/drivers/acpi/pmic/intel_pmic_chtcrc.c b/drivers/acpi/pmic/intel_pmic_chtcrc.c
index d8266d22cd8e..f9301c6f098e 100644
--- a/drivers/acpi/pmic/intel_pmic_chtcrc.c
+++ b/drivers/acpi/pmic/intel_pmic_chtcrc.c
@@ -24,6 +24,7 @@
  * CHT Crystal Cove PMIC.
  */
 static const struct intel_pmic_opregion_data intel_chtcrc_pmic_opregion_data = {
+	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
 	.pmic_i2c_address = 0x6e,
 };
 
diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
index cb444ddec5a0..418eec523025 100644
--- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
+++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
@@ -98,6 +98,7 @@ static const struct intel_pmic_opregion_data chtdc_ti_pmic_opregion_data = {
 	.get_power = chtdc_ti_pmic_get_power,
 	.update_power = chtdc_ti_pmic_update_power,
 	.get_raw_temp = chtdc_ti_pmic_get_raw_temp,
+	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
 	.power_table = chtdc_ti_power_table,
 	.power_table_count = ARRAY_SIZE(chtdc_ti_power_table),
 	.thermal_table = chtdc_ti_thermal_table,
diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
index 59385a9a05e5..f2c42f4c79ca 100644
--- a/drivers/acpi/pmic/intel_pmic_chtwc.c
+++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
@@ -257,6 +257,7 @@ static const struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
 	.get_power		= intel_cht_wc_pmic_get_power,
 	.update_power		= intel_cht_wc_pmic_update_power,
 	.exec_mipi_pmic_seq_element = intel_cht_wc_exec_mipi_pmic_seq_element,
+	.lpat_raw_to_temp	= acpi_lpat_raw_to_temp,
 	.power_table		= power_table,
 	.power_table_count	= ARRAY_SIZE(power_table),
 };
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index b5f4d81c621f..e844bc1f3df5 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -298,6 +298,7 @@ static const struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
 	.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,
 	.power_table = power_table,
 	.power_table_count = ARRAY_SIZE(power_table),
 	.thermal_table = thermal_table,
-- 
2.33.1

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

[Hans de Goede]

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(-)

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,
-- 
2.33.1

Re: [PATCH 1/3] ACPI: PMIC: constify all struct intel_pmic_opregion_data declarations

[Andy Shevchenko]

On Fri, Nov 26, 2021 at 04:21:07PM +0100, Hans de Goede wrote:
> The struct intel_pmic_opregion_data declarations never change,
> constify them all.

Makes sense!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/pmic/intel_pmic.c          | 12 ++++++------
>  drivers/acpi/pmic/intel_pmic.h          |  4 +++-
>  drivers/acpi/pmic/intel_pmic_bxtwc.c    |  2 +-
>  drivers/acpi/pmic/intel_pmic_bytcrc.c   |  2 +-
>  drivers/acpi/pmic/intel_pmic_chtcrc.c   |  2 +-
>  drivers/acpi/pmic/intel_pmic_chtdc_ti.c |  2 +-
>  drivers/acpi/pmic/intel_pmic_chtwc.c    |  2 +-
>  drivers/acpi/pmic/intel_pmic_xpower.c   |  2 +-
>  8 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
> index 9cde299eba88..98bbfd99c709 100644
> --- a/drivers/acpi/pmic/intel_pmic.c
> +++ b/drivers/acpi/pmic/intel_pmic.c
> @@ -25,7 +25,7 @@ struct intel_pmic_opregion {
>  	struct mutex lock;
>  	struct acpi_lpat_conversion_table *lpat_table;
>  	struct regmap *regmap;
> -	struct intel_pmic_opregion_data *data;
> +	const struct intel_pmic_opregion_data *data;
>  	struct intel_pmic_regs_handler_ctx ctx;
>  };
>  
> @@ -53,7 +53,7 @@ static acpi_status intel_pmic_power_handler(u32 function,
>  {
>  	struct intel_pmic_opregion *opregion = region_context;
>  	struct regmap *regmap = opregion->regmap;
> -	struct intel_pmic_opregion_data *d = opregion->data;
> +	const struct intel_pmic_opregion_data *d = opregion->data;
>  	int reg, bit, result;
>  
>  	if (bits != 32 || !value64)
> @@ -135,7 +135,7 @@ static int pmic_thermal_aux(struct intel_pmic_opregion *opregion, int reg,
>  static int pmic_thermal_pen(struct intel_pmic_opregion *opregion, int reg,
>  			    int bit, u32 function, u64 *value)
>  {
> -	struct intel_pmic_opregion_data *d = opregion->data;
> +	const struct intel_pmic_opregion_data *d = opregion->data;
>  	struct regmap *regmap = opregion->regmap;
>  
>  	if (!d->get_policy || !d->update_policy)
> @@ -171,7 +171,7 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
>  		void *handler_context, void *region_context)
>  {
>  	struct intel_pmic_opregion *opregion = region_context;
> -	struct intel_pmic_opregion_data *d = opregion->data;
> +	const struct intel_pmic_opregion_data *d = opregion->data;
>  	int reg, bit, result;
>  
>  	if (bits != 32 || !value64)
> @@ -255,7 +255,7 @@ static acpi_status intel_pmic_regs_handler(u32 function,
>  
>  int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
>  					struct regmap *regmap,
> -					struct intel_pmic_opregion_data *d)
> +					const struct intel_pmic_opregion_data *d)
>  {
>  	acpi_status status = AE_OK;
>  	struct intel_pmic_opregion *opregion;
> @@ -344,7 +344,7 @@ EXPORT_SYMBOL_GPL(intel_pmic_install_opregion_handler);
>  int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address,
>  					      u32 value, u32 mask)
>  {
> -	struct intel_pmic_opregion_data *d;
> +	const struct intel_pmic_opregion_data *d;
>  	int ret;
>  
>  	if (!intel_pmic_opregion) {
> diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h
> index 89379476a1df..467a39966dc8 100644
> --- a/drivers/acpi/pmic/intel_pmic.h
> +++ b/drivers/acpi/pmic/intel_pmic.h
> @@ -25,6 +25,8 @@ struct intel_pmic_opregion_data {
>  	int pmic_i2c_address;
>  };
>  
> -int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle, struct regmap *regmap, struct intel_pmic_opregion_data *d);
> +int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
> +					struct regmap *regmap,
> +					const struct intel_pmic_opregion_data *d);
>  
>  #endif
> diff --git a/drivers/acpi/pmic/intel_pmic_bxtwc.c b/drivers/acpi/pmic/intel_pmic_bxtwc.c
> index bd7621edd60b..6620ce0833f6 100644
> --- a/drivers/acpi/pmic/intel_pmic_bxtwc.c
> +++ b/drivers/acpi/pmic/intel_pmic_bxtwc.c
> @@ -369,7 +369,7 @@ intel_bxtwc_pmic_update_policy(struct regmap *regmap,
>  	return regmap_update_bits(regmap, reg, mask, val);
>  }
>  
> -static struct intel_pmic_opregion_data intel_bxtwc_pmic_opregion_data = {
> +static const struct intel_pmic_opregion_data intel_bxtwc_pmic_opregion_data = {
>  	.get_power      = intel_bxtwc_pmic_get_power,
>  	.update_power   = intel_bxtwc_pmic_update_power,
>  	.get_raw_temp   = intel_bxtwc_pmic_get_raw_temp,
> diff --git a/drivers/acpi/pmic/intel_pmic_bytcrc.c b/drivers/acpi/pmic/intel_pmic_bytcrc.c
> index 2a692cc4b7ae..8a1d895ed689 100644
> --- a/drivers/acpi/pmic/intel_pmic_bytcrc.c
> +++ b/drivers/acpi/pmic/intel_pmic_bytcrc.c
> @@ -271,7 +271,7 @@ static int intel_crc_pmic_update_policy(struct regmap *regmap,
>  	return 0;
>  }
>  
> -static struct intel_pmic_opregion_data intel_crc_pmic_opregion_data = {
> +static const struct intel_pmic_opregion_data intel_crc_pmic_opregion_data = {
>  	.get_power	= intel_crc_pmic_get_power,
>  	.update_power	= intel_crc_pmic_update_power,
>  	.get_raw_temp	= intel_crc_pmic_get_raw_temp,
> diff --git a/drivers/acpi/pmic/intel_pmic_chtcrc.c b/drivers/acpi/pmic/intel_pmic_chtcrc.c
> index 2900dc3074d2..d8266d22cd8e 100644
> --- a/drivers/acpi/pmic/intel_pmic_chtcrc.c
> +++ b/drivers/acpi/pmic/intel_pmic_chtcrc.c
> @@ -23,7 +23,7 @@
>   * intel_soc_pmic_exec_mipi_pmic_seq_element work on devices with a
>   * CHT Crystal Cove PMIC.
>   */
> -static struct intel_pmic_opregion_data intel_chtcrc_pmic_opregion_data = {
> +static const struct intel_pmic_opregion_data intel_chtcrc_pmic_opregion_data = {
>  	.pmic_i2c_address = 0x6e,
>  };
>  
> diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> index fef7831d0d63..cb444ddec5a0 100644
> --- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> +++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> @@ -94,7 +94,7 @@ static int chtdc_ti_pmic_get_raw_temp(struct regmap *regmap, int reg)
>  	return ((buf[0] & 0x03) << 8) | buf[1];
>  }
>  
> -static struct intel_pmic_opregion_data chtdc_ti_pmic_opregion_data = {
> +static const struct intel_pmic_opregion_data chtdc_ti_pmic_opregion_data = {
>  	.get_power = chtdc_ti_pmic_get_power,
>  	.update_power = chtdc_ti_pmic_update_power,
>  	.get_raw_temp = chtdc_ti_pmic_get_raw_temp,
> diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
> index 7ffd5624b8e1..59385a9a05e5 100644
> --- a/drivers/acpi/pmic/intel_pmic_chtwc.c
> +++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
> @@ -253,7 +253,7 @@ static int intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap,
>   * The thermal table and ops are empty, we do not support the Thermal opregion
>   * (DPTF) due to lacking documentation.
>   */
> -static struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
> +static const struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
>  	.get_power		= intel_cht_wc_pmic_get_power,
>  	.update_power		= intel_cht_wc_pmic_update_power,
>  	.exec_mipi_pmic_seq_element = intel_cht_wc_exec_mipi_pmic_seq_element,
> diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
> index cbe08e600fa3..b5f4d81c621f 100644
> --- a/drivers/acpi/pmic/intel_pmic_xpower.c
> +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
> @@ -293,7 +293,7 @@ static int intel_xpower_exec_mipi_pmic_seq_element(struct regmap *regmap,
>  	return ret;
>  }
>  
> -static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
> +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,
> -- 
> 2.33.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] ACPI: PMIC: allow drivers to provide a custom lpat_raw_to_temp() function

[Andy Shevchenko]

On Fri, Nov 26, 2021 at 04:21:08PM +0100, Hans de Goede wrote:
> The LPAT tables used in the DSDT for some PMICs require special handling,
> allow the PMIC OpRegion drivers to provide an alternative implementation
> by adding a lpat_raw_to_temp function pointer to struct pmic_table;
> and initialize this to the default acpi_lpat_raw_to_temp function

acpi_lpat_raw_to_temp()

> for all PMICs.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

(see one nit-pick below)

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/pmic/intel_pmic.c          | 2 +-
>  drivers/acpi/pmic/intel_pmic.h          | 4 ++++
>  drivers/acpi/pmic/intel_pmic_bxtwc.c    | 1 +
>  drivers/acpi/pmic/intel_pmic_bytcrc.c   | 1 +
>  drivers/acpi/pmic/intel_pmic_chtcrc.c   | 1 +
>  drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 1 +
>  drivers/acpi/pmic/intel_pmic_chtwc.c    | 1 +
>  drivers/acpi/pmic/intel_pmic_xpower.c   | 1 +
>  8 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
> index 98bbfd99c709..f20dbda1a831 100644
> --- a/drivers/acpi/pmic/intel_pmic.c
> +++ b/drivers/acpi/pmic/intel_pmic.c
> @@ -95,7 +95,7 @@ static int pmic_read_temp(struct intel_pmic_opregion *opregion,
>  		return 0;
>  	}
>  
> -	temp = acpi_lpat_raw_to_temp(opregion->lpat_table, raw_temp);
> +	temp = opregion->data->lpat_raw_to_temp(opregion->lpat_table, raw_temp);
>  	if (temp < 0)
>  		return temp;
>  
> diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h
> index 467a39966dc8..d956b03a6ca0 100644
> --- a/drivers/acpi/pmic/intel_pmic.h
> +++ b/drivers/acpi/pmic/intel_pmic.h
> @@ -2,6 +2,8 @@
>  #ifndef __INTEL_PMIC_H
>  #define __INTEL_PMIC_H
>  
> +#include <acpi/acpi_lpat.h>
> +
>  struct pmic_table {
>  	int address;	/* operation region address */
>  	int reg;	/* corresponding thermal register */
> @@ -17,6 +19,8 @@ struct intel_pmic_opregion_data {
>  	int (*update_policy)(struct regmap *r, int reg, int bit, int enable);
>  	int (*exec_mipi_pmic_seq_element)(struct regmap *r, u16 i2c_address,
>  					  u32 reg_address, u32 value, u32 mask);

> +	int (*lpat_raw_to_temp)(struct acpi_lpat_conversion_table *lpat_table,
> +				int raw);

Can be on one line.

>  	struct pmic_table *power_table;
>  	int power_table_count;
>  	struct pmic_table *thermal_table;
> diff --git a/drivers/acpi/pmic/intel_pmic_bxtwc.c b/drivers/acpi/pmic/intel_pmic_bxtwc.c
> index 6620ce0833f6..e247615189fa 100644
> --- a/drivers/acpi/pmic/intel_pmic_bxtwc.c
> +++ b/drivers/acpi/pmic/intel_pmic_bxtwc.c
> @@ -376,6 +376,7 @@ static const struct intel_pmic_opregion_data intel_bxtwc_pmic_opregion_data = {
>  	.update_aux     = intel_bxtwc_pmic_update_aux,
>  	.get_policy     = intel_bxtwc_pmic_get_policy,
>  	.update_policy  = intel_bxtwc_pmic_update_policy,
> +	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
>  	.power_table      = power_table,
>  	.power_table_count = ARRAY_SIZE(power_table),
>  	.thermal_table     = thermal_table,
> diff --git a/drivers/acpi/pmic/intel_pmic_bytcrc.c b/drivers/acpi/pmic/intel_pmic_bytcrc.c
> index 8a1d895ed689..9ea79f210965 100644
> --- a/drivers/acpi/pmic/intel_pmic_bytcrc.c
> +++ b/drivers/acpi/pmic/intel_pmic_bytcrc.c
> @@ -278,6 +278,7 @@ static const struct intel_pmic_opregion_data intel_crc_pmic_opregion_data = {
>  	.update_aux	= intel_crc_pmic_update_aux,
>  	.get_policy	= intel_crc_pmic_get_policy,
>  	.update_policy	= intel_crc_pmic_update_policy,
> +	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
>  	.power_table	= power_table,
>  	.power_table_count= ARRAY_SIZE(power_table),
>  	.thermal_table	= thermal_table,
> diff --git a/drivers/acpi/pmic/intel_pmic_chtcrc.c b/drivers/acpi/pmic/intel_pmic_chtcrc.c
> index d8266d22cd8e..f9301c6f098e 100644
> --- a/drivers/acpi/pmic/intel_pmic_chtcrc.c
> +++ b/drivers/acpi/pmic/intel_pmic_chtcrc.c
> @@ -24,6 +24,7 @@
>   * CHT Crystal Cove PMIC.
>   */
>  static const struct intel_pmic_opregion_data intel_chtcrc_pmic_opregion_data = {
> +	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
>  	.pmic_i2c_address = 0x6e,
>  };
>  
> diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> index cb444ddec5a0..418eec523025 100644
> --- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> +++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> @@ -98,6 +98,7 @@ static const struct intel_pmic_opregion_data chtdc_ti_pmic_opregion_data = {
>  	.get_power = chtdc_ti_pmic_get_power,
>  	.update_power = chtdc_ti_pmic_update_power,
>  	.get_raw_temp = chtdc_ti_pmic_get_raw_temp,
> +	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
>  	.power_table = chtdc_ti_power_table,
>  	.power_table_count = ARRAY_SIZE(chtdc_ti_power_table),
>  	.thermal_table = chtdc_ti_thermal_table,
> diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
> index 59385a9a05e5..f2c42f4c79ca 100644
> --- a/drivers/acpi/pmic/intel_pmic_chtwc.c
> +++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
> @@ -257,6 +257,7 @@ static const struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
>  	.get_power		= intel_cht_wc_pmic_get_power,
>  	.update_power		= intel_cht_wc_pmic_update_power,
>  	.exec_mipi_pmic_seq_element = intel_cht_wc_exec_mipi_pmic_seq_element,
> +	.lpat_raw_to_temp	= acpi_lpat_raw_to_temp,
>  	.power_table		= power_table,
>  	.power_table_count	= ARRAY_SIZE(power_table),
>  };
> diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
> index b5f4d81c621f..e844bc1f3df5 100644
> --- a/drivers/acpi/pmic/intel_pmic_xpower.c
> +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
> @@ -298,6 +298,7 @@ static const struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
>  	.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,
>  	.power_table = power_table,
>  	.power_table_count = ARRAY_SIZE(power_table),
>  	.thermal_table = thermal_table,
> -- 
> 2.33.1
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] ACPI: PMIC: allow drivers to provide a custom lpat_raw_to_temp() function

[Hans de Goede]

Hi,

On 11/26/21 16:46, Andy Shevchenko wrote:
> On Fri, Nov 26, 2021 at 04:21:08PM +0100, Hans de Goede wrote:
>> The LPAT tables used in the DSDT for some PMICs require special handling,
>> allow the PMIC OpRegion drivers to provide an alternative implementation
>> by adding a lpat_raw_to_temp function pointer to struct pmic_table;
>> and initialize this to the default acpi_lpat_raw_to_temp function
> 
> acpi_lpat_raw_to_temp()
> 
>> for all PMICs.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> 
> (see one nit-pick below)
> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/acpi/pmic/intel_pmic.c          | 2 +-
>>  drivers/acpi/pmic/intel_pmic.h          | 4 ++++
>>  drivers/acpi/pmic/intel_pmic_bxtwc.c    | 1 +
>>  drivers/acpi/pmic/intel_pmic_bytcrc.c   | 1 +
>>  drivers/acpi/pmic/intel_pmic_chtcrc.c   | 1 +
>>  drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 1 +
>>  drivers/acpi/pmic/intel_pmic_chtwc.c    | 1 +
>>  drivers/acpi/pmic/intel_pmic_xpower.c   | 1 +
>>  8 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
>> index 98bbfd99c709..f20dbda1a831 100644
>> --- a/drivers/acpi/pmic/intel_pmic.c
>> +++ b/drivers/acpi/pmic/intel_pmic.c
>> @@ -95,7 +95,7 @@ static int pmic_read_temp(struct intel_pmic_opregion *opregion,
>>  		return 0;
>>  	}
>>  
>> -	temp = acpi_lpat_raw_to_temp(opregion->lpat_table, raw_temp);
>> +	temp = opregion->data->lpat_raw_to_temp(opregion->lpat_table, raw_temp);
>>  	if (temp < 0)
>>  		return temp;
>>  
>> diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h
>> index 467a39966dc8..d956b03a6ca0 100644
>> --- a/drivers/acpi/pmic/intel_pmic.h
>> +++ b/drivers/acpi/pmic/intel_pmic.h
>> @@ -2,6 +2,8 @@
>>  #ifndef __INTEL_PMIC_H
>>  #define __INTEL_PMIC_H
>>  
>> +#include <acpi/acpi_lpat.h>
>> +
>>  struct pmic_table {
>>  	int address;	/* operation region address */
>>  	int reg;	/* corresponding thermal register */
>> @@ -17,6 +19,8 @@ struct intel_pmic_opregion_data {
>>  	int (*update_policy)(struct regmap *r, int reg, int bit, int enable);
>>  	int (*exec_mipi_pmic_seq_element)(struct regmap *r, u16 i2c_address,
>>  					  u32 reg_address, u32 value, u32 mask);
> 
>> +	int (*lpat_raw_to_temp)(struct acpi_lpat_conversion_table *lpat_table,
>> +				int raw);
> 
> Can be on one line.

I tried to stay within (or at least close too) 80 lines here as that is
still the recommended limit. The new 100 is more of a checkpatch only
change, not a change in the codingstyle docs.

With that said I'm fine with changing this. Rafael, feel free to
change this while merging. Or let me know if you want a v2.

Regards,

Hans


> 
>>  	struct pmic_table *power_table;
>>  	int power_table_count;
>>  	struct pmic_table *thermal_table;
>> diff --git a/drivers/acpi/pmic/intel_pmic_bxtwc.c b/drivers/acpi/pmic/intel_pmic_bxtwc.c
>> index 6620ce0833f6..e247615189fa 100644
>> --- a/drivers/acpi/pmic/intel_pmic_bxtwc.c
>> +++ b/drivers/acpi/pmic/intel_pmic_bxtwc.c
>> @@ -376,6 +376,7 @@ static const struct intel_pmic_opregion_data intel_bxtwc_pmic_opregion_data = {
>>  	.update_aux     = intel_bxtwc_pmic_update_aux,
>>  	.get_policy     = intel_bxtwc_pmic_get_policy,
>>  	.update_policy  = intel_bxtwc_pmic_update_policy,
>> +	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
>>  	.power_table      = power_table,
>>  	.power_table_count = ARRAY_SIZE(power_table),
>>  	.thermal_table     = thermal_table,
>> diff --git a/drivers/acpi/pmic/intel_pmic_bytcrc.c b/drivers/acpi/pmic/intel_pmic_bytcrc.c
>> index 8a1d895ed689..9ea79f210965 100644
>> --- a/drivers/acpi/pmic/intel_pmic_bytcrc.c
>> +++ b/drivers/acpi/pmic/intel_pmic_bytcrc.c
>> @@ -278,6 +278,7 @@ static const struct intel_pmic_opregion_data intel_crc_pmic_opregion_data = {
>>  	.update_aux	= intel_crc_pmic_update_aux,
>>  	.get_policy	= intel_crc_pmic_get_policy,
>>  	.update_policy	= intel_crc_pmic_update_policy,
>> +	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
>>  	.power_table	= power_table,
>>  	.power_table_count= ARRAY_SIZE(power_table),
>>  	.thermal_table	= thermal_table,
>> diff --git a/drivers/acpi/pmic/intel_pmic_chtcrc.c b/drivers/acpi/pmic/intel_pmic_chtcrc.c
>> index d8266d22cd8e..f9301c6f098e 100644
>> --- a/drivers/acpi/pmic/intel_pmic_chtcrc.c
>> +++ b/drivers/acpi/pmic/intel_pmic_chtcrc.c
>> @@ -24,6 +24,7 @@
>>   * CHT Crystal Cove PMIC.
>>   */
>>  static const struct intel_pmic_opregion_data intel_chtcrc_pmic_opregion_data = {
>> +	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
>>  	.pmic_i2c_address = 0x6e,
>>  };
>>  
>> diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
>> index cb444ddec5a0..418eec523025 100644
>> --- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
>> +++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
>> @@ -98,6 +98,7 @@ static const struct intel_pmic_opregion_data chtdc_ti_pmic_opregion_data = {
>>  	.get_power = chtdc_ti_pmic_get_power,
>>  	.update_power = chtdc_ti_pmic_update_power,
>>  	.get_raw_temp = chtdc_ti_pmic_get_raw_temp,
>> +	.lpat_raw_to_temp = acpi_lpat_raw_to_temp,
>>  	.power_table = chtdc_ti_power_table,
>>  	.power_table_count = ARRAY_SIZE(chtdc_ti_power_table),
>>  	.thermal_table = chtdc_ti_thermal_table,
>> diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
>> index 59385a9a05e5..f2c42f4c79ca 100644
>> --- a/drivers/acpi/pmic/intel_pmic_chtwc.c
>> +++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
>> @@ -257,6 +257,7 @@ static const struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
>>  	.get_power		= intel_cht_wc_pmic_get_power,
>>  	.update_power		= intel_cht_wc_pmic_update_power,
>>  	.exec_mipi_pmic_seq_element = intel_cht_wc_exec_mipi_pmic_seq_element,
>> +	.lpat_raw_to_temp	= acpi_lpat_raw_to_temp,
>>  	.power_table		= power_table,
>>  	.power_table_count	= ARRAY_SIZE(power_table),
>>  };
>> diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
>> index b5f4d81c621f..e844bc1f3df5 100644
>> --- a/drivers/acpi/pmic/intel_pmic_xpower.c
>> +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
>> @@ -298,6 +298,7 @@ static const struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
>>  	.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,
>>  	.power_table = power_table,
>>  	.power_table_count = ARRAY_SIZE(power_table),
>>  	.thermal_table = thermal_table,
>> -- 
>> 2.33.1
>>
> 

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

[Hans de Goede]

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
>>
> 

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] ACPI: PMIC: allow drivers to provide a custom lpat_raw_to_temp() function

[Andy Shevchenko]

On Sat, Nov 27, 2021 at 11:57 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 11/26/21 16:46, Andy Shevchenko wrote:
> > On Fri, Nov 26, 2021 at 04:21:08PM +0100, Hans de Goede wrote:

...

> >> +    int (*lpat_raw_to_temp)(struct acpi_lpat_conversion_table *lpat_table,
> >> +                            int raw);
> >
> > Can be on one line.
>
> I tried to stay within (or at least close too) 80 lines here as that is
> still the recommended limit. The new 100 is more of a checkpatch only
> change, not a change in the codingstyle docs.

Yes I know that and IIRC in ACPI we have a bit relaxed rules (WRT line
length) even before checkpatch has relaxed them. But as you pointed
out below it's not worth of resending since it can be amended when
applying.

> With that said I'm fine with changing this. Rafael, feel free to
> change this while merging. Or let me know if you want a v2.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] ACPI: PMIC: constify all struct intel_pmic_opregion_data declarations

[Rafael J. Wysocki]

On Fri, Nov 26, 2021 at 4:50 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Fri, Nov 26, 2021 at 04:21:07PM +0100, Hans de Goede wrote:
> > The struct intel_pmic_opregion_data declarations never change,
> > constify them all.
>
> Makes sense!
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

Applied as 5.17 material along with the [2-3/3], thanks!