Date:   Sat, 9 Jan 2021 00:39:45 +0000
From:   Daniel Scally <djrscally@...il.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        devel@...ica.org, "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Wolfram Sang <wsa@...nel.org>, Yong Zhi <yong.zhi@...el.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Bingbu Cao <bingbu.cao@...el.com>,
        Tian Shu Qiu <tian.shu.qiu@...el.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Robert Moore <robert.moore@...el.com>,
        Erik Kaneda <erik.kaneda@...el.com>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        kieran.bingham+renesas@...asonboard.com,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Jordan Hand <jorhand@...ux.microsoft.com>,
        Tsuchiya Yuto <kitakar@...il.com>,
        "Krogerus, Heikki" <heikki.krogerus@...ux.intel.com>
Subject: Re: [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device

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.

Powered by blists - more mailing lists