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

[Daniel Scally <djrscally@xxxxxxxxx>]

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.