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.
> After much internal debate I decided to write this as a standalone
> acpi_driver. Alternative options we considered:
>
> 1. Squash all this into the cio2-bridge code, which I did originally write
> but decided I didn't like.
> 2. Extend the existing tps68470 mfd driver...they share an ACPI ID so this
> kinda makes sense, but ultimately given there is no actual physical
> tps68470 in the scenario this patch handles I decided I didn't like this
> either.
Looking to this I think the best is to create a module that can be consumed by tps68470 and separately.
So, something near to it rather than under ipu3 hood.
You may use same ID's in both drivers (in PMIC less case it can be simple
platform and thus they won't conflict), but both of them should provide GPIO
resources for consumption.
So, something like
tps68470.h with API to consume
split tps68470 to -core, -i2c parts
add int3472, which will serve for above and be standalone platform driver
update cio2-bridge accordingly
Would it be feasible?
...
> + 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.
...
> + case 0x01: /* Power regulators (we think) */
> + case 0x0c:
> + case 0x0b: /* Power regulators, but to a device separate to sensor */
> + case 0x0d: /* Indicator LEDs */
Give names to those constants.
#define INT3472_GPIO_TYPE_RESET 0x00
...
> +static struct acpi_driver int3472_driver = {
No acpi_driver! Use platform_driver instead with plenty of examples all over
the kernel.
> + .name = "int3472",
> + .ids = int3472_device_id,
> + .ops = {
> + .add = int3472_add,
> + .remove = int3472_remove,
> + },
> + .owner = THIS_MODULE,
No need
> +};
...
> +const guid_t int3472_gpio_guid = GUID_INIT(0x79234640, 0x9e10, 0x4fea,
> + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b,
> + 0x19, 0x75, 0x6f);
> +
> +const guid_t cio2_sensor_module_guid = GUID_INIT(0x822ace8f, 0x2814, 0x4174,
> + 0xa5, 0x6b, 0x5f, 0x02, 0x9f,
> + 0xe0, 0x79, 0xee);
Use more or less standard pattern for these, like
/* 79234640-9e10-4fea-a5c1b5aa8b19756f */
const guid_t int3472_gpio_guid =
GUID_INIT(0x79234640, 0x9e10, 0x4fea,
0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f);
...
> +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?
> +static struct regulator_consumer_supply surface_go2_ov5693[] = {
> + { "i2c-INT33BE:00", "avdd" },
> + { "i2c-INT33BE:00", "dovdd" },
> +};
> +
> +static struct regulator_consumer_supply surface_book_ov5693[] = {
> + { "i2c-INT33BE:00", "avdd" },
> + { "i2c-INT33BE:00", "dovdd" },
> +};
Ditto.
...
> +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.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Mon, Nov 30, 2020 at 10:07:19PM +0200, 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)
That's very useful information ! Thank you.
> The above text perhaps should go somewhere under Documentation.
Or in the driver source code, but definitely somewhere else than in the
commit message.
> > After much internal debate I decided to write this as a standalone
> > acpi_driver. Alternative options we considered:
> >
> > 1. Squash all this into the cio2-bridge code, which I did originally write
> > but decided I didn't like.
> > 2. Extend the existing tps68470 mfd driver...they share an ACPI ID so this
> > kinda makes sense, but ultimately given there is no actual physical
> > tps68470 in the scenario this patch handles I decided I didn't like this
> > either.
>
> Looking to this I think the best is to create a module that can be consumed by tps68470 and separately.
> So, something near to it rather than under ipu3 hood.
>
> You may use same ID's in both drivers (in PMIC less case it can be simple
> platform and thus they won't conflict), but both of them should provide GPIO
> resources for consumption.
>
> So, something like
>
> tps68470.h with API to consume
> split tps68470 to -core, -i2c parts
> add int3472, which will serve for above and be standalone platform driver
> update cio2-bridge accordingly
>
> Would it be feasible?
Given that INT3472 means Intel camera power management device (that's
more or less the wording in Windows, I can double-check), would the
following make sense ?
A top-level module named intel-camera-pmic (or int3472, or ...) would
register two drivers, a platform driver and an I2C driver, to
accommodate for both cases ("discrete PMIC" that doesn't have an
I2cSerialBusV2, and TPS64870 or uP6641Q that are I2C devices). The probe
function would perform the following:
- If there's no CLDB, then the device uses the Chrome OS "ACPI
bindings", and refers to a TPS64870. The code that exists in the
kernel today (registering GPIOs, and registering an OpRegion to
communicate with the power management code in the DSDT) would be
activated.
- If there's a CLDB, then the device type would be retrieved from it:
- If the device is a "discrete PMIC", the driver would register clocks
and regulators controlled by GPIOs, and create clock, regulator and
GPIO lookup entries for the sensor device that references the PMIC.
- If the device is a TPS64870, the code that exists in the kernel
today to register GPIOs would be activated, and new code would need
to be written to register regulators and clocks.
- If the device is a uP6641Q, a new driver will need to be written (I
don't know on which devices this PMIC is used, so this can probably
be deferred).
We can split this in multiple files and/or modules.
> ...
>
> > + 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.
How does the consumer (the camera sensor) retrieve the GPIO though ? The
_DSM is in the PMIC device object, while the real consumer is the camera
sensor.
> ...
>
> > + case 0x01: /* Power regulators (we think) */
> > + case 0x0c:
> > + case 0x0b: /* Power regulators, but to a device separate to sensor */
> > + case 0x0d: /* Indicator LEDs */
>
>
> Give names to those constants.
>
> #define INT3472_GPIO_TYPE_RESET 0x00
> ...
>
>
> > +static struct acpi_driver int3472_driver = {
>
> No acpi_driver! Use platform_driver instead with plenty of examples all over
> the kernel.
>
> > + .name = "int3472",
> > + .ids = int3472_device_id,
> > + .ops = {
> > + .add = int3472_add,
> > + .remove = int3472_remove,
> > + },
>
> > + .owner = THIS_MODULE,
>
> No need
>
> > +};
>
> ...
>
> > +const guid_t int3472_gpio_guid = GUID_INIT(0x79234640, 0x9e10, 0x4fea,
> > + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b,
> > + 0x19, 0x75, 0x6f);
> > +
> > +const guid_t cio2_sensor_module_guid = GUID_INIT(0x822ace8f, 0x2814, 0x4174,
> > + 0xa5, 0x6b, 0x5f, 0x02, 0x9f,
> > + 0xe0, 0x79, 0xee);
>
> Use more or less standard pattern for these, like
>
> /* 79234640-9e10-4fea-a5c1b5aa8b19756f */
> const guid_t int3472_gpio_guid =
> GUID_INIT(0x79234640, 0x9e10, 0x4fea,
> 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f);
>
> ...
>
> > +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?
>
> > +static struct regulator_consumer_supply surface_go2_ov5693[] = {
> > + { "i2c-INT33BE:00", "avdd" },
> > + { "i2c-INT33BE:00", "dovdd" },
> > +};
> > +
> > +static struct regulator_consumer_supply surface_book_ov5693[] = {
> > + { "i2c-INT33BE:00", "avdd" },
> > + { "i2c-INT33BE:00", "dovdd" },
> > +};
>
> Ditto.
>
> ...
>
> > +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.
--
Regards,
Laurent Pinchart
On Tue, Dec 01, 2020 at 01:32:32AM +0200, Laurent Pinchart wrote:
> On Mon, Nov 30, 2020 at 10:07:19PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote:
...
> > So, something like
> >
> > tps68470.h with API to consume
> > split tps68470 to -core, -i2c parts
> > add int3472, which will serve for above and be standalone platform driver
> > update cio2-bridge accordingly
> >
> > Would it be feasible?
>
> Given that INT3472 means Intel camera power management device (that's
> more or less the wording in Windows, I can double-check), would the
> following make sense ?
>
> A top-level module named intel-camera-pmic (or int3472, or ...) would
> register two drivers, a platform driver and an I2C driver, to
> accommodate for both cases ("discrete PMIC" that doesn't have an
> I2cSerialBusV2, and TPS64870 or uP6641Q that are I2C devices). The probe
> function would perform the following:
>
> - If there's no CLDB, then the device uses the Chrome OS "ACPI
> bindings", and refers to a TPS64870. The code that exists in the
> kernel today (registering GPIOs, and registering an OpRegion to
> communicate with the power management code in the DSDT) would be
> activated.
>
> - If there's a CLDB, then the device type would be retrieved from it:
>
> - If the device is a "discrete PMIC", the driver would register clocks
> and regulators controlled by GPIOs, and create clock, regulator and
> GPIO lookup entries for the sensor device that references the PMIC.
>
> - If the device is a TPS64870, the code that exists in the kernel
> today to register GPIOs would be activated, and new code would need
> to be written to register regulators and clocks.
>
> - If the device is a uP6641Q, a new driver will need to be written (I
> don't know on which devices this PMIC is used, so this can probably
> be deferred).
>
> We can split this in multiple files and/or modules.
Seems we can do this, by locating intel_int3472.c under PDx86 hood and dropping
ACPI ID table from TPS68470 MFD driver. The PMIC can be instantiated via
i2c_acpi_new_device() (IIRC the API name).
And actually it makes more sense since it's not and MFD and should not be there.
(Dan, patch wise the one creates intel_int3472.c followed by another one that
moves ACPI ID from PMIC and introduces its instantiation via I?C board info
structure)
...
> > > + 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.
>
> How does the consumer (the camera sensor) retrieve the GPIO though ? The
> _DSM is in the PMIC device object, while the real consumer is the camera
> sensor.
1. A GPIO proxy
2. A custom GPIO lookup tables
3. An fwnode passing to the sensor (via swnodes graph)
First may issue deferred probe, while second needs some ordering tricks I guess.
Third one should also provide an ACPI GPIO mapping table or so to make the
consumer rely on names rather than custom numbers.
Perhaps someone may propose other solutions.
--
With Best Regards,
Andy Shevchenko
On 01/12/2020 18:49, Andy Shevchenko wrote:
>>>> + 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.
>>
>> How does the consumer (the camera sensor) retrieve the GPIO though ? The
>> _DSM is in the PMIC device object, while the real consumer is the camera
>> sensor.
>
> 1. A GPIO proxy
> 2. A custom GPIO lookup tables
> 3. An fwnode passing to the sensor (via swnodes graph)
>
> First may issue deferred probe, while second needs some ordering tricks I guess.
> Third one should also provide an ACPI GPIO mapping table or so to make the
> consumer rely on names rather than custom numbers.
>
> Perhaps someone may propose other solutions.
Hi Andy
Sorry; some more clarification here if you have time please:
1. Do you mean here, register a new gpio_chip providing GPIOs to the
sensors, and just have the .set() callback for that function set the
corresponding line against the INT3472 device?
2. I thought custom GPIO lookup tables was what I was doing, are you
referring to something else?
3. I guess you mean something like of_find_gpio() and acpi_find_gpio()
here? As far as I can see there isn't currently a swnodes
equivalent...we could just pass it via reference of course but it would
mean the sensor drivers would all need to account for that.
On Sun, Dec 13, 2020 at 10:48:39PM +0000, Daniel Scally wrote:
> On 01/12/2020 18:49, Andy Shevchenko wrote:
> >>>> + 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.
> >>
> >> How does the consumer (the camera sensor) retrieve the GPIO though ? The
> >> _DSM is in the PMIC device object, while the real consumer is the camera
> >> sensor.
> >
> > 1. A GPIO proxy
> > 2. A custom GPIO lookup tables
> > 3. An fwnode passing to the sensor (via swnodes graph)
> >
> > First may issue deferred probe, while second needs some ordering tricks I guess.
> > Third one should also provide an ACPI GPIO mapping table or so to make the
> > consumer rely on names rather than custom numbers.
> >
> > Perhaps someone may propose other solutions.
>
> Hi Andy
>
> Sorry; some more clarification here if you have time please:
No problem, thanks for discussing this.
> 1. Do you mean here, register a new gpio_chip providing GPIOs to the
> sensors, and just have the .set() callback for that function set the
> corresponding line against the INT3472 device?
Yes. On one hand it should be a consumer (*gpiod_get*() family of APIs),
on the other it should be provider of known (artificial) GPIO chip.
> 2. I thought custom GPIO lookup tables was what I was doing, are you
> referring to something else?
I think so, i.e. nothing else from high point of view.
> 3. I guess you mean something like of_find_gpio() and acpi_find_gpio()
> here? As far as I can see there isn't currently a swnodes
> equivalent...we could just pass it via reference of course but it would
> mean the sensor drivers would all need to account for that.
Theoretically we may provide GPIOs as swnodes. In that case the consumer will
get them as usual But I think it may be too complicated / over engineered.
--
With Best Regards,
Andy Shevchenko
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.
On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally <[email protected]> wrote:
> On 30/11/2020 20:07, Andy Shevchenko wrote:
> > On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote:
...
> > 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.
All of them are GPIOs, the question here is how they are actually
connected on PCB level and I have no answer to that. You have to find
schematics somewhere.
> 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?
The GPIOs can go directly to the sensors or be a control pin for
separate discrete power gates.
So, we can do one of the following:
a) present PD GPIO as fixed regulator;
b) present PD & Reset GPIOs as regulator;
c) provide them as is to the sensor and sensor driver must do what it
considers right.
Since we don't have schematics (yet?) and we have plenty of variations
of sensors, I would go to c) and update the driver of the affected
sensor as needed. Because even if you have separate discrete PD for
one sensor on one platform there is no guarantee that it will be the
same on another. Providing a "virtual" PD in a sensor that doesn't
support it is the best choice I think. Let's hear what Sakari and
other experienced camera sensor developers say.
My vision is purely based on electrical engineering background,
experience with existing (not exactly camera) sensor drivers and
generic cases.
> 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.
--
With Best Regards,
Andy Shevchenko
Hi Andy
On 08/01/2021 12:17, Andy Shevchenko wrote:
> On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally <[email protected]> wrote:
>> On 30/11/2020 20:07, Andy Shevchenko wrote:
>>> On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote:
> ...
>
>>> 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.
> All of them are GPIOs, the question here is how they are actually
> connected on PCB level and I have no answer to that. You have to find
> schematics somewhere.
Yeah; I've been trying to get those but so far, no dice.
>
>> 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?
> The GPIOs can go directly to the sensors or be a control pin for
> separate discrete power gates.
> So, we can do one of the following:
> a) present PD GPIO as fixed regulator;
> b) present PD & Reset GPIOs as regulator;
> c) provide them as is to the sensor and sensor driver must do what it
> considers right.
>
> Since we don't have schematics (yet?) and we have plenty of variations
> of sensors, I would go to c) and update the driver of the affected
> sensor as needed. Because even if you have separate discrete PD for
> one sensor on one platform there is no guarantee that it will be the
> same on another. Providing a "virtual" PD in a sensor that doesn't
> support it is the best choice I think. Let's hear what Sakari and
> other experienced camera sensor developers say.
>
> My vision is purely based on electrical engineering background,
> experience with existing (not exactly camera) sensor drivers and
> generic cases.
Alright; thanks. I'm happy with C being the answer, so unless someone
thinks differently I'll work on that basis.
>> 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.
>
H Andy and Daniel,
On Fri, Jan 08, 2021 at 02:17:49PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally wrote:
> > On 30/11/2020 20:07, Andy Shevchenko wrote:
> > > On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote:
>
> ...
>
> > > 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.
First question, do we have a confirmation that the OV2680 sensor on that
platform requires GPIO 0x01 to be toggled to work properly ? I'd like to
rule out the option of the GPIO being simply not connected (that would
be best for us, although my experience so far with this terrible ACPI
design doesn't of course give me much hope).
Where did you find the OV2680 datasheet by the way, can you share a link
to a leaked version ?
> All of them are GPIOs, the question here is how they are actually
> connected on PCB level and I have no answer to that. You have to find
> schematics somewhere.
>
> > 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?
>
> The GPIOs can go directly to the sensors or be a control pin for
> separate discrete power gates.
GPIO functions 0x00 and 0x01 are meant to control sensor signals, while
GPIO function 0x0b is meant to control a power gate. Of course board
designers may have thought clever to use function 0x01 to control a
second power gate, this can't be ruled out without the schematics (or
reverse engineering of the hardware).
> So, we can do one of the following:
> a) present PD GPIO as fixed regulator;
> b) present PD & Reset GPIOs as regulator;
> c) provide them as is to the sensor and sensor driver must do what it
> considers right.
>
> Since we don't have schematics (yet?) and we have plenty of variations
> of sensors, I would go to c) and update the driver of the affected
> sensor as needed. Because even if you have separate discrete PD for
> one sensor on one platform there is no guarantee that it will be the
> same on another. Providing a "virtual" PD in a sensor that doesn't
> support it is the best choice I think. Let's hear what Sakari and
> other experienced camera sensor developers say.
>
> My vision is purely based on electrical engineering background,
> experience with existing (not exactly camera) sensor drivers and
> generic cases.
If the OV2680 has indeed no power down pin, that won't work. Adding
support for a non-existent powerdown pin to the corresponding driver
won't be accepted. Workarounds and hacks to support IPU3-based devices
need to be kept out of camera sensor drivers.
If we need to map GPIO function 0x01 to a sensor GPIO on some platform,
and to a regulator on other platforms, then we will need per-platform
data in the INT3472 driver. For this particular platform, the reset
(0x00) GPIO should be passed to the sensor, and the powerdown (0x01)
GPIO should control a regulator (again assuming that our assumption that
the GPIO is wired to a power gate is correct).
> > 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.
--
Regards,
Laurent Pinchart
Hi Laurent
On 09/01/2021 00:18, Laurent Pinchart wrote:
> H Andy and Daniel,
>
> On Fri, Jan 08, 2021 at 02:17:49PM +0200, Andy Shevchenko wrote:
>> On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally wrote:
>>> On 30/11/2020 20:07, Andy Shevchenko wrote:
>>>> On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote:
>> ...
>>
>>>> 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.
> First question, do we have a confirmation that the OV2680 sensor on that
> platform requires GPIO 0x01 to be toggled to work properly ?
Yes; without that toggled not even the i2c interface is available.
> I'd like to
> rule out the option of the GPIO being simply not connected (that would
> be best for us, although my experience so far with this terrible ACPI
> design doesn't of course give me much hope).
Sorry to dash what little hope was left.
> Where did you find the OV2680 datasheet by the way, can you share a link
> to a leaked version ?
Sure. I left the PC already, but I'll do that tomorrow.
>> All of them are GPIOs, the question here is how they are actually
>> connected on PCB level and I have no answer to that. You have to find
>> schematics somewhere.
>>
>>> 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?
>> The GPIOs can go directly to the sensors or be a control pin for
>> separate discrete power gates.
> GPIO functions 0x00 and 0x01 are meant to control sensor signals, while
> GPIO function 0x0b is meant to control a power gate. Of course board
> designers may have thought clever to use function 0x01 to control a
> second power gate, this can't be ruled out without the schematics (or
> reverse engineering of the hardware).
>
>> So, we can do one of the following:
>> a) present PD GPIO as fixed regulator;
>> b) present PD & Reset GPIOs as regulator;
>> c) provide them as is to the sensor and sensor driver must do what it
>> considers right.
>>
>> Since we don't have schematics (yet?) and we have plenty of variations
>> of sensors, I would go to c) and update the driver of the affected
>> sensor as needed. Because even if you have separate discrete PD for
>> one sensor on one platform there is no guarantee that it will be the
>> same on another. Providing a "virtual" PD in a sensor that doesn't
>> support it is the best choice I think. Let's hear what Sakari and
>> other experienced camera sensor developers say.
>>
>> My vision is purely based on electrical engineering background,
>> experience with existing (not exactly camera) sensor drivers and
>> generic cases.
> If the OV2680 has indeed no power down pin, that won't work. Adding
> support for a non-existent powerdown pin to the corresponding driver
> won't be accepted. Workarounds and hacks to support IPU3-based devices
> need to be kept out of camera sensor drivers.
>
> If we need to map GPIO function 0x01 to a sensor GPIO on some platform,
> and to a regulator on other platforms, then we will need per-platform
> data in the INT3472 driver. For this particular platform, the reset
> (0x00) GPIO should be passed to the sensor, and the powerdown (0x01)
> GPIO should control a regulator (again assuming that our assumption that
> the GPIO is wired to a power gate is correct).
Let me think of a neat way to do this then.
>
>>> 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.