Re: [BUG 5/5] [BUG] media: atomisp: atomisp causes touchscreen to stop working on Microsoft Surface 3

[Tsuchiya Yuto <kitakar@gmail.com>]

Thank you for your comment :-)

First, I need to correct what I said in the previous mail. I later found
that loading only "atomisp" (as well as its dependency,
atomisp_gmin_platform) does not cause this issue.

What causes this issue is rather, loading sensor drivers (as well as its
dependency, atomisp_gmin_platform).

These sensor drivers for surface3 are both not upstream, but I made them
as similar as possible to the upstreamed ones. So, I guess this issue
can still be reproducible on some other devices.

I can't (easily) try touchscreen on mipad2 because it won't boot without
nomodeset and somehow GNOME won't start if it's using nomodeset (on Arch
Linux).

On Mon, 2021-10-18 at 10:30 +0200, Hans de Goede wrote:
> Hi,
> 
> On 10/17/21 18:23, Tsuchiya Yuto wrote:
> > Touchscreen input works fine before loading atomisp driver on Surface 3.
> > 
> > However, after loading atomisp driver, touchscreen works only when
> > capturing images. This sounds like atomisp turns off something needed
> > for touchscreen when atomisp is idle.
> > 
> > There is no useful kernel log. Just the touchscreen stops working
> > with no log.
> > 
> > I'll update if I find something further. First of all, can someone
> > reproduce this issue on the other devices?
> 
> My first bet would be some regulator getting turned off.
> 
> What you can do is:
> 
> 1. ls -l /dev/bus/i2c/devices
> 
> And then write down the number of the i2c-bus to which the
> CRC PMIC is connected, lets say it is number "4". Then:
> 
> 2. Before loading the atomisp drivers run:
>    "sudo i2cdump -y -f 4 0x6e > crc-regs-good"
> 
> 3. After loading the atomisp drivers run:
>    "sudo i2cdump -y -f 4 0x6e > crc-regs-bad
> 
> 4. "diff -u crc-regs-good crc-regs-bad"
> 
> And see what changed.

Here is the diff. The "good" one is before loading sensor driver, the
"bad" one is from after loading sensor driver:

        $ diff -u crc-regs-good crc-regs-bad
        --- crc-regs-good	2021-10-21 18:04:57.853396866 +0900
        +++ crc-regs-bad	2021-10-21 18:06:13.755738054 +0900
        @@ -4,14 +4,14 @@
         20: 00 00 01 c8 68 07 0a 10 10 01 1f 10 10 10 10 10    ..??h???????????
         30: 10 10 00 20 21 20 20 20 20 20 00 2a 1c 1c 14 10    ??. !     .*????
         40: 10 10 10 28 20 20 28 2e 2f 20 20 83 00 00 4c 00    ???(  (./  ?..L.
        -50: 00 01 01 01 00 00 60 00 60 00 00 02 02 03 60 60    .???..`.`..???``
        +50: 00 01 01 01 00 00 60 02 60 00 00 02 02 62 60 60    .???..`?`..??b``
         60: 60 01 03 00 00 00 00 00 00 00 30 04 00 00 00 00    `??.......0?....
        -70: 00 03 00 00 02 7b 02 6c 02 71 02 55 02 7c 02 9d    .?..?{?l?q?U?|??
        +70: 00 03 00 00 02 7b 02 6d 02 73 02 58 02 7f 02 9e    .?..?{?m?s?X????
         80: 00 00 00 00 00 00 00 00 00 00 00 02 08 00 0b 02    ...........??.??
         90: 3f 07 00 00 00 00 4c 00 4e 00 00 4c 00 23 01 b4    ??....L.N..L.#??
         a0: 4c 00 4e 00 00 3d ca 6a f0 00 00 3d ca 6a f0 00    L.N..=?j?..=?j?.
         b0: 00 7e 2a ff 02 04 06 00 00 00 00 00 00 20 00 00    .~*.???...... ..
         c0: 00 00 00 cd 08 00 00 4c 00 00 00 4c 00 00 00 3d    ...??..L...L...=
        -d0: 97 00 00 3d 97 00 00 fe 17 00 ff 02 01 07 94 03    ?..=?..??..?????
        -e0: 9a 00 27 00 00 00 00 00 00 00 00 00 00 00 00 00    ?.'.............
        +d0: 97 00 00 3d 97 00 00 fe 17 00 ff 02 01 07 ec 03    ?..=?..??..?????
        +e0: 96 00 21 00 00 00 00 00 00 00 00 00 00 00 00 00    ?.!.............
         f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................

Note that lines "70:" and "d0:"/"e0:" change over time. So, the actual
change caused by loading sensor driver is line "50:"

        -50: 00 01 01 01 00 00 60 00 60 00 00 02 02 03 60 60    .???..`.`..???``
        +50: 00 01 01 01 00 00 60 02 60 00 00 02 02 62 60 60    .???..`?`..??b``

and in atomisp_gmin_platform.c file,

        /* CRYSTAL COVE PMIC register set */
        #define CRYSTAL_1P8V_REG	0x57
        #define CRYSTAL_2P8V_REG	0x5d
        #define CRYSTAL_ON		0x63
        #define CRYSTAL_OFF		0x62

indeed we're poking at 0x57 and 0x5d. So, maybe this issue is caused by
regulators? I tried what would happen if I kept sensor power on before
in sensor drivers, but there was no effect. But I feel I need to look
into it again further.

> There are 2 possible root causes here:
> 
> 1. Some regulator is shared between the cameras and
> touchscreen
> 
> 2. The crc code in atomisp which you are using is
> poking registers assuming the Bay Trail version of
> the Crystal Cove PMIC (aka CRC PMIC) but your
> Surface 3 has the Cherry Trail version and we know
> that the regulators are add different register
> addresses there, see the comment in:
> drivers/acpi/pmic/intel_pmic_chtcrc.c
> so it is possible that the atomisp code is simply
> poking the wrong register for one of the regulators

According to this Android kernel commit [1], the address for 1P8V and
2P8V are updated to the CRC+ ones (the upstreamed atomisp already has
this change)

[1] https://github.com/MiCode/Xiaomi_Kernel_OpenSource/commit/2f8221ba9a3770aed1ecfad2d04db61b95f30394
    ("update PMIC v1p8/v2p8 address")

> I also wonder if this goes away if you do the
> 
> 	hrv = 0x03;
> 
> Hack inside drivers/mfd/intel_soc_pmic_core.c ?
> 
> Without that we end up using the wrong PMIC
> OpRegion driver which also uses the wrong
> regulator addresses.

I'm now using cht one with your patch, but the situation is the same
as before.

Regards,
Tsuchiya Yuto

> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> Here are the 2 different regulator drivers the
> comment in drivers/acpi/pmic/intel_pmic_chtcrc.c
> refers to:
> 
> https://github.com/Zenfone2-Dev/android_kernel_asus_moorefield-1/blob/stock/drivers/regulator/pmic_crystal_cove.c
> https://github.com/Zenfone2-Dev/android_kernel_asus_moorefield-1/blob/stock/drivers/regulator/pmic_crystal_cove_plus.c
>