[RFC PATCH 0/1] add ccove PMIC i2c address for Microsoft Surface 3

[RFC PATCH 0/1] add ccove PMIC i2c address for Microsoft Surface 3

[Tsuchiya Yuto]

Hi all,

Firstly, I'm still not used to Linux patch sending flow. Sorry in advance
if there is some weirdness :-) but I did my best.

I need to use the function intel_soc_pmic_exec_mipi_pmic_seq_element()
with atomisp Image Signal Processing driver on Microsoft Surface 3
(Cherry Trail).

However, it currently fails with the message I added to the commit
message below. I wondered why. The driver intel_pmic_chtcrc does define
the i2c address.

It later turned out that the intel_pmic_bytcrc driver is used on surface3
instead, where the i2c address is not defined. So, I added the address
with the patch I'm sending as RFC in this mail. It's working well.

The question is that, should Surface 3 (Cherry Trail) really use the
intel_pmic_bytcrc driver?

As I wrote in the commit message, the _HRV value of the PMIC is 0x02,
although the _DDN entry describes it as "CRYSTAL COVE+ AIC". So, maybe,
it should rather use intel_pmic_chtcrc? Does anyone know the other
instances where the _HRV value is 0x02 although it's based on Cherry
Trail SoC ?

So, I also tried using the intel_pmic_chtcrc driver instead, with the
following (temporary) change [drivers/mfd/intel_soc_pmic_core.c]:

+	hrv = 0x03;
+
	switch (hrv) {
	case BYT_CRC_HRV:
		config = &intel_soc_pmic_config_byt_crc;
		break;
	case CHT_CRC_HRV:
		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;
	}

and the function intel_soc_pmic_exec_mipi_pmic_seq_element() worked well
just like with the intel_pmic_bytcrc driver.

I don't mind which driver is used on surface3 for now, considering that
the atomisp driver is working with both PMIC drivers. But I'd like to
hear from maintainers which is better :)

Tested on surface3 with v5.15-rc5.

Regards,
Tsuchiya Yuto

Tsuchiya Yuto (1):
  ACPI / PMIC: Add i2c address to intel_pmic_bytcrc driver

 drivers/acpi/pmic/intel_pmic_bytcrc.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.33.1

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

[Tsuchiya Yuto]

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

Re: [RFC PATCH 0/1] add ccove PMIC i2c address for Microsoft Surface 3

[Andy Shevchenko]

On Sun, Oct 17, 2021 at 7:16 PM Tsuchiya Yuto <kitakar@gmail.com> wrote:
>
> Hi all,
>
> Firstly, I'm still not used to Linux patch sending flow. Sorry in advance
> if there is some weirdness :-) but I did my best.
>
> I need to use the function intel_soc_pmic_exec_mipi_pmic_seq_element()
> with atomisp Image Signal Processing driver on Microsoft Surface 3
> (Cherry Trail).
>
> However, it currently fails with the message I added to the commit
> message below. I wondered why. The driver intel_pmic_chtcrc does define
> the i2c address.
>
> It later turned out that the intel_pmic_bytcrc driver is used on surface3
> instead, where the i2c address is not defined. So, I added the address
> with the patch I'm sending as RFC in this mail. It's working well.
>
> The question is that, should Surface 3 (Cherry Trail) really use the
> intel_pmic_bytcrc driver?

I believe Cherry Trail should use the chtcrc driver.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Oct 17, 2021 at 7:16 PM Tsuchiya Yuto <kitakar@gmail.com> 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")

Not sure what this tag means.

After reading a bit of code I think the best approach is to quirk the
drivers/mfd/intel_soc_pmic_core.c with DMI to supply HRV. Setting an
address to all BYT devices may not be the best since I have no idea if
there are different addresses in use. It may be the case, but the
problem is that we have no proof. Also BYT driver uses hardcoded power
and thermal tables, I'm completely unsure that this would not damage
hardware in some circumstances.

Hans, what's your opinion on this?

P.S. I dunno if _UID correlates to the type of the chipset (BYT/CHT).

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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

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

[Andy Shevchenko]

On Mon, Oct 18, 2021 at 12:16 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 10/17/21 18:15, Tsuchiya Yuto wrote:
> > On Microsoft Surface 3 (uses Intel's Atom Cherry Trail SoC), executing

...

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

TBH I have no strong opinion. Only one remark on your patch, I am not
a fan of removing COMPILE_TEST but at the same time I'm not a fan of
ifdeffery. All on all I think having COMPILE_TEST is preferable even
if we have ifdeffery. Btw, IIRC similar code (i.e. BYT vs CHT by CPU
ID) is being used elsewhere. Perhaps we might have some common
(library) under arc/x86, PDx86 or so (headers?)?

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 10/18/21 12:31, Andy Shevchenko wrote:
> On Mon, Oct 18, 2021 at 12:16 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 10/17/21 18:15, Tsuchiya Yuto wrote:
>>> On Microsoft Surface 3 (uses Intel's Atom Cherry Trail SoC), executing
> 
> ...
> 
>> 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 ?
> 
> TBH I have no strong opinion. Only one remark on your patch, I am not
> a fan of removing COMPILE_TEST but at the same time I'm not a fan of
> ifdeffery. All on all I think having COMPILE_TEST is preferable even
> if we have ifdeffery. Btw, IIRC similar code (i.e. BYT vs CHT by CPU
> ID) is being used elsewhere. Perhaps we might have some common
> (library) under arc/x86, PDx86 or so (headers?)?

We already have helpers for this defined in:

sound/soc/intel/common/soc-intel-quirks.h

We could move those to some header under include, maybe:

include/linux/platform_data/x86/atom.h

And add #ifdef-ery there so that things will also build on
non x86 ?

Then we could do a 2 patch series adding the
include/linux/platform_data/x86/atom.h
file + the drivers/mfd/intel_soc_pmic_core.c
change and Lee can merge both through the MFD tree.

And then we can do further clean-ups of e.g. sound/soc
on top (we can ask Lee to provide an immutable branch).

How does that sound ?

Regards,

Hans

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

[Andy Shevchenko]

On Mon, Oct 18, 2021 at 12:38:51PM +0200, Hans de Goede wrote:
> On 10/18/21 12:31, Andy Shevchenko wrote:
> > On Mon, Oct 18, 2021 at 12:16 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > Btw, IIRC similar code (i.e. BYT vs CHT by CPU
> > ID) is being used elsewhere. Perhaps we might have some common
> > (library) under arc/x86, PDx86 or so (headers?)?
> 
> We already have helpers for this defined in:
> 
> sound/soc/intel/common/soc-intel-quirks.h
> 
> We could move those to some header under include, maybe:
> 
> include/linux/platform_data/x86/atom.h
> 
> And add #ifdef-ery there so that things will also build on
> non x86 ?
> 
> Then we could do a 2 patch series adding the
> include/linux/platform_data/x86/atom.h
> file + the drivers/mfd/intel_soc_pmic_core.c
> change and Lee can merge both through the MFD tree.
> 
> And then we can do further clean-ups of e.g. sound/soc
> on top (we can ask Lee to provide an immutable branch).
> 
> How does that sound ?

Sounds like a good plan to me!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Oct 18, 2021 at 12:38:51PM +0200, Hans de Goede wrote:
> On 10/18/21 12:31, Andy Shevchenko wrote:

...

> We already have helpers for this defined in:
> 
> sound/soc/intel/common/soc-intel-quirks.h
> 
> We could move those to some header under include, maybe:
> 
> include/linux/platform_data/x86/atom.h
> 
> And add #ifdef-ery there so that things will also build on
> non x86 ?

But it seems the above mentioned header has the stubs!


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 10/18/21 12:46, Andy Shevchenko wrote:
> On Mon, Oct 18, 2021 at 12:38:51PM +0200, Hans de Goede wrote:
>> On 10/18/21 12:31, Andy Shevchenko wrote:
>>> On Mon, Oct 18, 2021 at 12:16 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>> Btw, IIRC similar code (i.e. BYT vs CHT by CPU
>>> ID) is being used elsewhere. Perhaps we might have some common
>>> (library) under arc/x86, PDx86 or so (headers?)?
>>
>> We already have helpers for this defined in:
>>
>> sound/soc/intel/common/soc-intel-quirks.h
>>
>> We could move those to some header under include, maybe:
>>
>> include/linux/platform_data/x86/atom.h
>>
>> And add #ifdef-ery there so that things will also build on
>> non x86 ?
>>
>> Then we could do a 2 patch series adding the
>> include/linux/platform_data/x86/atom.h
>> file + the drivers/mfd/intel_soc_pmic_core.c
>> change and Lee can merge both through the MFD tree.
>>
>> And then we can do further clean-ups of e.g. sound/soc
>> on top (we can ask Lee to provide an immutable branch).
>>
>> How does that sound ?
> 
> Sounds like a good plan to me!

So I've been thinking about this a bit more.

Since sound/soc/intel/common/soc-intel-quirks.h already
has stubs for non X86 too, I think it is best to just
move that to include/linux/platform_data/x86/soc.h .

Since the drivers/mfd/intel_soc_pmic_core.c thing is
a bugfix of sorts, it is probably best to open-code
the check there and then replace it with the helper
from include/linux/platform_data/x86/soc.h later.

I'll start prepping a patch series doing things
this way now.

Regards,

Hans

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

[Tsuchiya Yuto]

On Sun, 2021-10-17 at 22:08 +0300, Andy Shevchenko wrote:
> On Sun, Oct 17, 2021 at 7:16 PM Tsuchiya Yuto <kitakar@gmail.com> 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")
> 
> Not sure what this tag means.

Thanks!

I thought I could use this tag to mean "I made this patch with this
commit as a reference". If I used this tag incorrectly, I'll avoid using
it in the future and instead write with my words.

Regards,
Tsuchiya Yuto

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

[Tsuchiya Yuto]

On Mon, 2021-10-18 at 11:16 +0200, Hans de Goede wrote:
> 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.

Thanks!

I tried your attached patch, and I can confirm that it's working as
expected.

Now it's using cht one:

        $ ls /sys/devices/pci0000:00/808622C1:05/i2c-5/i2c-INT33FD:00
        cht_crystal_cove_pmic  crystal_cove_gpio  crystal_cove_pwm  driver  firmware_node  modalias  name  power  subsystem  uevent

and the function intel_soc_pmic_exec_mipi_pmic_seq_element() is also
working with atomisp driver.

Regards,
Tsuchiya Yuto

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

[Andy Shevchenko]

On Tue, Oct 19, 2021 at 08:56:04PM +0900, Tsuchiya Yuto wrote:
> On Mon, 2021-10-18 at 11:16 +0200, Hans de Goede wrote:
> > On 10/17/21 18:15, Tsuchiya Yuto wrote:

...

> > Tsuchiya, can you give the attached patch a try.
> 
> Thanks!
> 
> I tried your attached patch, and I can confirm that it's working as
> expected.
> 
> Now it's using cht one:
> 
>         $ ls /sys/devices/pci0000:00/808622C1:05/i2c-5/i2c-INT33FD:00
>         cht_crystal_cove_pmic  crystal_cove_gpio  crystal_cove_pwm  driver  firmware_node  modalias  name  power  subsystem  uevent
> 
> and the function intel_soc_pmic_exec_mipi_pmic_seq_element() is also
> working with atomisp driver.

To be formal you may give a dedicated tag here, i.e. Tested-by:.
It will be easier for tools, such as `b4`, to catch it up
and not forget.

-- 
With Best Regards,
Andy Shevchenko

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

[Tsuchiya Yuto]

On Tue, 2021-10-19 at 15:05 +0300, Andy Shevchenko wrote:
> On Tue, Oct 19, 2021 at 08:56:04PM +0900, Tsuchiya Yuto wrote:
> > On Mon, 2021-10-18 at 11:16 +0200, Hans de Goede wrote:
> > > On 10/17/21 18:15, Tsuchiya Yuto wrote:
> 
> ...
> 
> > > Tsuchiya, can you give the attached patch a try.
> > 
> > Thanks!
> > 
> > I tried your attached patch, and I can confirm that it's working as
> > expected.
> > 
> > Now it's using cht one:
> > 
> >         $ ls /sys/devices/pci0000:00/808622C1:05/i2c-5/i2c-INT33FD:00
> >         cht_crystal_cove_pmic  crystal_cove_gpio  crystal_cove_pwm  driver  firmware_node  modalias  name  power  subsystem  uevent
> > 
> > and the function intel_soc_pmic_exec_mipi_pmic_seq_element() is also
> > working with atomisp driver.
> 
> To be formal you may give a dedicated tag here, i.e. Tested-by:.
> It will be easier for tools, such as `b4`, to catch it up
> and not forget.

Thanks!

Hm, then, not sure if mine is helpful but here is
Tested-by: Tsuchiya Yuto <kitakar@gmail.com>

Regards,
Tsuchiya Yuto