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

[Laurent Pinchart]

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