Re: [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device

[Daniel Scally <djrscally@gmail.com>]

Hi Andy, all

On 30/11/2020 20:07, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote:
>> On platforms where ACPI is designed for use with Windows, resources
>> that are intended to be consumed by sensor devices are sometimes in
>> the _CRS of a dummy INT3472 device upon which the sensor depends. This
>> driver binds to the dummy acpi device (which does not represent a
> acpi device -> acpi_device
>
>> physical PMIC) and maps them into GPIO lines and regulators for use by
>> the sensor device instead.
> ...
>
>> This patch contains the bits of this process that we're least sure about.
>> The sensors in scope for this work are called out as dependent (in their
>> DSDT entry's _DEP) on a device with _HID INT3472. These come in at least
>> 2 kinds; those with an I2cSerialBusV2 entry (which we presume therefore
>> are legitimate tps68470 PMICs that need handling by those drivers - work
>> on that in the future). And those without an I2C device. For those without
>> an I2C device they instead have an array of GPIO pins defined in _CRS. So
>> for example, my Lenovo Miix 510's OVTI2680 sensor is dependent on one of
>> the _latter_ kind of INT3472 devices, with this _CRS:
>>
>> Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>> {
>>     Name (SBUF, ResourceTemplate ()
>>     {
>>         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
>> 	    IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0",
>> 	    0x00, ResourceConsumer, ,
>>             )
>>             {   // Pin list
>>                 0x0079
>>             }
>>         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
>> 	    IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0",
>> 	    0x00, ResourceConsumer, ,
>>             )
>>             {   // Pin list
>>                 0x007A
>>             }
>>         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000,
>> 	    IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0",
>> 	    0x00, ResourceConsumer, ,
>>             )
>>             {   // Pin list
>>                 0x008F
>>             }
>>     })
>>     Return (SBUF) /* \_SB_.PCI0.PMI1._CRS.SBUF */
>> }
>>
>> and the same device has a _DSM Method, which returns 32-bit ints where
>> the second lowest byte we noticed to match the pin numbers of the GPIO
>> lines:
>>
>> Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
>> {
>>     If ((Arg0 == ToUUID ("79234640-9e10-4fea-a5c1-b5aa8b19756f")))
>>     {
>>         If ((Arg2 == One))
>>         {
>>             Return (0x03)
>>         }
>>
>>         If ((Arg2 == 0x02))
>>         {
>>             Return (0x01007900)
>>         }
>>
>>         If ((Arg2 == 0x03))
>>         {
>>             Return (0x01007A0C)
>>         }
>>
>>         If ((Arg2 == 0x04))
>>         {
>>             Return (0x01008F01)
>>         }
>>     }
>>
>>     Return (Zero)
>> }
>>
>> We know that at least some of those pins have to be toggled active for the
>> sensor devices to be available in i2c, so the conclusion we came to was
>> that those GPIO entries assigned to the INT3472 device actually represent
>> GPIOs and regulators to be consumed by the sensors themselves. Tsuchiya
>> noticed that the lowest byte in the return values of the _DSM method
>> seemed to represent the type or function of the GPIO line, and we
>> confirmed that by testing on each surface device that GPIO lines where the
>> low byte in the _DSM entry for that pin was 0x0d controlled the privacy
>> LED of the cameras.
>>
>> We're guessing as to the exact meaning of the function byte, but I
>> conclude they're something like this:
>>
>> 0x00 - probably a reset GPIO
>> 0x01 - regulator for the sensor
>> 0x0c - regulator for the sensor
>> 0x0b - regulator again, but for a VCM or EEPROM
>> 0x0d - privacy led (only one we're totally confident of since we can see
>>        it happen!)
> It's solely Windows driver design...
> Luckily I found some information and can clarify above table:
>
> 0x00 Reset
> 0x01 Power down
> 0x0b Power enable
> 0x0c Clock enable
> 0x0d LED (active high)
>
> The above text perhaps should go somewhere under Documentation.

Coming back to this; there's a bit of an anomaly with the 0x01 Power
Down pin for at least one platform.  As listed above, the OV2680 on one
of my platforms has 3 GPIOs defined, and the table above gives them as
type Reset, Power down and Clock enable. I'd assumed from this table
that "power down" meant a powerdown GPIO (I.E. the one usually called
PWDNB in Omnivision datasheets and "powerdown" in drivers), but the
datasheet for the OV2680 doesn't list a separate reset and powerdown
pin, but rather a single pin that performs both functions.


Am I wrong to treat that as something that ought to be mapped as a
powerdown GPIO to the sensors? Or do you know of any other way to
reconcile that discrepancy?


Failing that; the only way I can think to handle this is to register
proxy GPIO pins assigned to the sensors as you suggested previously, and
have them toggle the GPIO's assigned to the INT3472 based on platform
specific mapping data (I.E. we register a pin called "reset", which on
most platforms just toggles the 0x00 pin, but on this specific platform
would drive both 0x00 and 0x01 together. We're already heading that way
for the regulator consumer supplies so it's sort of nothing new, but I'd
still rather not if it can be avoided.