On Mon, 2021-10-18 at 11:16 +0200, Hans de Goede wrote:

Hi,

On 10/17/21 18:15, Tsuchiya Yuto wrote:

quoted

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

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