Re: [RFC PATCH 1/1] ACPI / PMIC: Add i2c address to intel_pmic_bytcrc driver

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

[-- Attachment #1: Type: text/plain, Size: 3061 bytes --]

Hi,

On 10/17/21 18:15, Tsuchiya Yuto wrote:
> On Microsoft Surface 3 (uses Intel's Atom Cherry Trail SoC), executing
> intel_soc_pmic_exec_mipi_pmic_seq_element() results in the following
> error message:
> 
>         [ 7196.356682] intel_soc_pmic_exec_mipi_pmic_seq_element: Not implemented
>         [ 7196.356686] intel_soc_pmic_exec_mipi_pmic_seq_element: i2c-addr: 0x6e reg-addr 0x57 value 0x63 mask 0xff
> 
> Surface 3 uses the PMIC device INT33FD, and the DSDT describes its _HRV
> value is 0x02 [1]:
> 
>         Scope (PCI0.I2C7)
>         {
>             Device (PMIC)
>             {
>                 Name (_ADR, Zero)  // _ADR: Address
>                 Name (_HID, "INT33FD" /* Intel Baytrail Power Management IC */)  // _HID: Hardware ID
>                 Name (_CID, "INT33FD" /* Intel Baytrail Power Management IC */)  // _CID: Compatible ID
>                 Name (_DDN, "CRYSTAL COVE+ AIC")  // _DDN: DOS Device Name
>                 Name (_HRV, 0x02)  // _HRV: Hardware Revision
>                 Name (_UID, One)  // _UID: Unique ID
>                 Name (_DEP, Package (0x01)  // _DEP: Dependencies
>                 {
>                     I2C7
>                 })
>         [...]
> 
> Due to this _HRV value, intel_pmic_bytcrc is used as the PMIC driver.
> However, the i2c address is currently not defined in this driver.
> This commit adds the missing i2c address 0x6e to the intel_pmic_bytcrc
> driver.
> 
> [1] https://github.com/linux-surface/acpidumps/blob/f8db3d150815aa21530635b7e646eee271e3b8fe/surface_3/dsdt.dsl#L10868
> 
> References: cc0594c4b0ef ("ACPI / PMIC: Add i2c address for thermal control")
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>

I believe that it is very unlikely that a device with a Cherry Trail SoC is actually using
the Bay Trail version of the PMIC, I would expect that to not necessarily work all that well.

So as Andy said, the right fix here is something like the:

+	hrv = 0x03;

Workaround from your cover-letter.

As Andy said we could use a DMI quirk for this, but chances are that the Microsoft Surface
DSDT is not the only one with the wrong HRV value. So instead it might be better to
just test for the SoC type as the attached patch does.

Tsuchiya, can you give the attached patch a try.

Andy, what do you think, should we go with the attached patch or would you prefer using
a DMI quirk ?

Regards,

Hans






> ---
>  drivers/acpi/pmic/intel_pmic_bytcrc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/acpi/pmic/intel_pmic_bytcrc.c b/drivers/acpi/pmic/intel_pmic_bytcrc.c
> index 2a692cc4b7ae..a64f50a42c54 100644
> --- a/drivers/acpi/pmic/intel_pmic_bytcrc.c
> +++ b/drivers/acpi/pmic/intel_pmic_bytcrc.c
> @@ -282,6 +282,7 @@ static struct intel_pmic_opregion_data intel_crc_pmic_opregion_data = {
>  	.power_table_count= ARRAY_SIZE(power_table),
>  	.thermal_table	= thermal_table,
>  	.thermal_table_count = ARRAY_SIZE(thermal_table),
> +	.pmic_i2c_address = 0x6e,
>  };
>  
>  static int intel_crc_pmic_opregion_probe(struct platform_device *pdev)
> 

[-- Attachment #2: 0001-mfd-intel_soc_pmic-Use-CPU-id-check-instead-of-_HRV-.patch --]
[-- Type: text/x-patch, Size: 3237 bytes --]

From c656a01ad8d9252eb5747c3f3c1c861534acbcbd Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Mon, 18 Oct 2021 11:11:34 +0200
Subject: [PATCH] mfd: intel_soc_pmic: Use CPU-id check instead of _HRV check
 to differentiate variants

The Intel Crystal Cove PMIC has 2 different variants, one for use with
Bay Trail (BYT) SoCs and one for use with Cherry Trail (CHT) SoCs.

So far we have been using an ACPI _HRV check to differentiate between
the 2, but at least on the Microsoft Surface 3, which is a CHT device,
the wrong _HRV value is reported by ACPI.

So instead switch to a CPU-ID check which avoids us relying on the
possibly wrong ACPI _HRV value.

Reported-by: Tsuchiya Yuto <kitakar@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mfd/Kconfig               |  2 +-
 drivers/mfd/intel_soc_pmic_core.c | 35 +++++++++----------------------
 2 files changed, 11 insertions(+), 26 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ca0edab91aeb..58866c425494 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -587,7 +587,7 @@ config LPC_SCH
 config INTEL_SOC_PMIC
 	bool "Support for Crystal Cove PMIC"
 	depends on ACPI && HAS_IOMEM && I2C=y && GPIOLIB && COMMON_CLK
-	depends on X86 || COMPILE_TEST
+	depends on X86
 	depends on I2C_DESIGNWARE_PLATFORM=y
 	select MFD_CORE
 	select REGMAP_I2C
diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
index ddd64f9e3341..9e1588d4c82e 100644
--- a/drivers/mfd/intel_soc_pmic_core.c
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -17,48 +17,33 @@
 #include <linux/pwm.h>
 #include <linux/regmap.h>
 
-#include "intel_soc_pmic_core.h"
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
 
-/* Crystal Cove PMIC shares same ACPI ID between different platforms */
-#define BYT_CRC_HRV		2
-#define CHT_CRC_HRV		3
+#include "intel_soc_pmic_core.h"
 
 /* PWM consumed by the Intel GFX */
 static struct pwm_lookup crc_pwm_lookup[] = {
 	PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_pmic_backlight", 0, PWM_POLARITY_NORMAL),
 };
 
+static const struct x86_cpu_id byt_cpu_ids[] = {
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
+	{}
+};
+
 static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
 				    const struct i2c_device_id *i2c_id)
 {
 	struct device *dev = &i2c->dev;
 	struct intel_soc_pmic_config *config;
 	struct intel_soc_pmic *pmic;
-	unsigned long long hrv;
-	acpi_status status;
 	int ret;
 
-	/*
-	 * There are 2 different Crystal Cove PMICs a Bay Trail and Cherry
-	 * Trail version, use _HRV to differentiate between the 2.
-	 */
-	status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_HRV", NULL, &hrv);
-	if (ACPI_FAILURE(status)) {
-		dev_err(dev, "Failed to get PMIC hardware revision\n");
-		return -ENODEV;
-	}
-
-	switch (hrv) {
-	case BYT_CRC_HRV:
+	if (x86_match_cpu(byt_cpu_ids))
 		config = &intel_soc_pmic_config_byt_crc;
-		break;
-	case CHT_CRC_HRV:
+	else
 		config = &intel_soc_pmic_config_cht_crc;
-		break;
-	default:
-		dev_warn(dev, "Unknown hardware rev %llu, assuming BYT\n", hrv);
-		config = &intel_soc_pmic_config_byt_crc;
-	}
 
 	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
 	if (!pmic)
-- 
2.31.1