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

[Dan Scally <djrscally@xxxxxxxxx>]

Hi Andy, thanks for comments

On 30/11/2020 20:07, Andy Shevchenko wrote:
>> 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.

Ah! That's really useful, thank you. We can handle the clock the same
way as regulators are being handled now, so that's no problem. And
likewise 0x01 for power down can just be mapped to the Sensor device
along with the reset pin and led pins. "Power enable" sounds like a
regulator indeed...it's not present on many (most) of our sensors
actually but that's not a problem for them of course as they'll just be
driven by the dummy regulators.


Thanks for the info


>
> ...
>
>> +	table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev),
>> +							   ares->data.gpio.pin_table[0],
>> +							   func, 0, GPIO_ACTIVE_HIGH);
> You won't need this if you have regular INT3472 platform driver.
> Simple call there _DSM to map resources to the type and use devm_gpiod_get on
> consumer behalf. Thus, previous patch is not needed.
But the resources need to be available to the sensor devices; they're
basically in the wrong place. They should be in _CRS of the sensor,
rather than INT3472, so we need to map them across.
> ...
>
>> +static struct regulator_consumer_supply miix_510_ov2680[] = {
>> +	{ "i2c-OVTI2680:00", "avdd" },
>> +	{ "i2c-OVTI2680:00", "dovdd" },
>> +};
> Can we use acpi_dev_first_match_dev() to get instance name out of their HIDs?
We need the full i2c device name, which afaik isn't available from the
acpi_device
>> +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = {
>> +	{ "GNDF140809R", 2, miix_510_ov2680 },
>> +	{ "YHCU", 2, surface_go2_ov5693 },
>> +	{ "MSHW0070", 2, surface_book_ov5693 },
>> +};
> Hmm... Usual way is to use DMI for that. I'm not sure above will not give us
> false positive matches.
I considered DMI too, no problem to switch to that if it's a better choice.