From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Daniel Scally <djrscally@gmail.com>,
linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-i2c@vger.kernel.org,
linux-media@vger.kernel.org, devel@acpica.org, rjw@rjwysocki.net,
lenb@kernel.org, gregkh@linuxfoundation.org,
mika.westerberg@linux.intel.com, linus.walleij@linaro.org,
bgolaszewski@baylibre.com, wsa@kernel.org, yong.zhi@intel.com,
bingbu.cao@intel.com, tian.shu.qiu@intel.com, mchehab@kernel.org,
robert.moore@intel.com, erik.kaneda@intel.com, pmladek@suse.com,
rostedt@goodmis.org, sergey.senozhatsky@gmail.com,
linux@rasmusvillemoes.dk,
kieran.bingham+renesas@ideasonboard.com,
jacopo+renesas@jmondi.org,
laurent.pinchart+renesas@ideasonboard.com,
jorhand@linux.microsoft.com, kitakar@gmail.com,
heikki.krogerus@linux.intel.com
Subject: Re: [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device
Date: Tue, 1 Dec 2020 20:37:58 +0200 [thread overview]
Message-ID: <20201201183758.GE3085@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20201201155513.GB852@paasikivi.fi.intel.com>
Hi Sakari,
On Tue, Dec 01, 2020 at 05:55:13PM +0200, Sakari Ailus wrote:
> 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:
> > > > 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.
>
> That's what I thought of, too, as one option, but with some more detail.
> This would be indeed the cleanest option.
>
> I think it'd be nice if the CLDB stuff (apart from checking whether it's
> there) would be in a different module to avoid cluttering up the real
> tps68470 driver.
Given the amount of code, and the fact that the driver should be
compiled as a module, I don't think it will make a huge difference in
the memory footprint.
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2020-12-01 18:39 UTC|newest]
Thread overview: 144+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-30 13:31 [PATCH 00/18] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
2020-11-30 13:31 ` [PATCH 01/18] property: Return true in fwnode_device_is_available for node types that do not implement this operation Daniel Scally
2020-11-30 16:05 ` Laurent Pinchart
2020-11-30 17:21 ` Andy Shevchenko
2020-12-01 3:12 ` Bingbu Cao
2020-12-01 8:46 ` Dan Scally
2020-11-30 13:31 ` [PATCH 02/18] property: Add support for calling fwnode_graph_get_endpoint_by_id() for fwnode->secondary Daniel Scally
2020-11-30 16:08 ` Laurent Pinchart
2020-11-30 17:29 ` Andy Shevchenko
2020-11-30 17:28 ` Laurent Pinchart
2020-11-30 17:53 ` Andy Shevchenko
2020-11-30 18:41 ` Laurent Pinchart
2020-11-30 19:21 ` Andy Shevchenko
2020-12-02 10:13 ` Dan Scally
2020-11-30 13:31 ` [PATCH 03/18] software_node: Fix failure to put() and get() references to children in software_node_get_next_child() Daniel Scally
2020-11-30 16:10 ` Laurent Pinchart
2020-11-30 17:30 ` Andy Shevchenko
2020-11-30 13:31 ` [PATCH 04/18] software_node: Enforce parent before child ordering of nodes array for software_node_register_nodes() Daniel Scally
2020-11-30 16:11 ` Laurent Pinchart
2020-11-30 16:12 ` Laurent Pinchart
2020-11-30 23:10 ` Dan Scally
2020-11-30 17:35 ` Andy Shevchenko
2020-11-30 17:37 ` Andy Shevchenko
2020-11-30 13:31 ` [PATCH 05/18] software_node: Alter software_node_unregister_nodes() to unregister the array in reverse order Daniel Scally
2020-11-30 16:14 ` Laurent Pinchart
2020-11-30 17:45 ` Andy Shevchenko
2020-12-01 23:36 ` Dan Scally
2020-11-30 13:31 ` [PATCH 06/18] software_node: amend software_node_unregister_node_group() to perform unregistration of array in reverse order to be consistent with software_node_unregister_nodes() Daniel Scally
2020-11-30 16:17 ` Laurent Pinchart
2020-11-30 17:47 ` Andy Shevchenko
2020-12-02 10:04 ` Dan Scally
2020-11-30 17:19 ` kernel test robot
2020-12-01 18:18 ` Dan Carpenter
2020-11-30 13:31 ` [PATCH 07/18] software_node: Add support for fwnode_graph*() family of functions Daniel Scally
2020-11-30 16:25 ` Laurent Pinchart
2020-11-30 23:34 ` Dan Scally
2020-11-30 13:31 ` [PATCH 08/18] lib/test_printf.c: Use helper function to unwind array of software_nodes Daniel Scally
2020-11-30 15:54 ` Sergey Senozhatsky
2020-11-30 16:26 ` Laurent Pinchart
2020-11-30 13:31 ` [PATCH 09/18] ipu3-cio2: Add T: entry to MAINTAINERS Daniel Scally
2020-11-30 13:31 ` [PATCH 10/18] ipu3-cio2: Rename ipu3-cio2.c to allow module to be built from multiple source files retaining ipu3-cio2 name Daniel Scally
2020-12-01 6:56 ` Bingbu Cao
2020-12-01 7:07 ` Bingbu Cao
2020-11-30 13:31 ` [PATCH 11/18] media: v4l2-core: v4l2-async: Check possible match in match_fwnode based on sd->fwnode->secondary Daniel Scally
2020-11-30 16:27 ` Laurent Pinchart
2020-11-30 17:55 ` Andy Shevchenko
2020-11-30 13:31 ` [PATCH 12/18] acpi: Add acpi_dev_get_next_match_dev() and macro to iterate through acpi_devices matching a given _HID Daniel Scally
2020-11-30 17:58 ` Andy Shevchenko
2020-11-30 13:31 ` [PATCH 13/18] ipu3-cio2: Add functionality allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
2020-11-30 16:45 ` kernel test robot
2020-11-30 17:09 ` Laurent Pinchart
2020-11-30 18:14 ` Andy Shevchenko
2020-12-01 22:08 ` Dan Scally
2020-12-01 22:11 ` Dan Scally
2020-12-01 22:30 ` Laurent Pinchart
2020-12-01 23:15 ` Dan Scally
2020-12-02 10:38 ` Sakari Ailus
2020-12-02 10:53 ` Dan Scally
2020-12-02 12:02 ` Andy Shevchenko
2020-12-02 22:44 ` Dan Scally
2020-12-02 12:01 ` Andy Shevchenko
2020-11-30 20:27 ` kernel test robot
2020-11-30 20:35 ` Sakari Ailus
2020-12-01 8:13 ` Dan Scally
2020-12-01 15:06 ` Andy Shevchenko
2020-12-15 10:28 ` Daniel Scally
2020-12-15 10:32 ` Daniel Scally
2020-12-15 22:02 ` Sakari Ailus
2020-12-17 14:17 ` Daniel Scally
2020-11-30 13:31 ` [PATCH 14/18] acpi: utils: Add function to fetch dependent acpi_devices Daniel Scally
2020-11-30 18:23 ` Andy Shevchenko
2020-11-30 18:40 ` Laurent Pinchart
2020-11-30 23:54 ` Dan Scally
2020-12-01 15:10 ` Andy Shevchenko
2020-12-01 15:23 ` Dan Scally
2020-11-30 13:31 ` [PATCH 15/18] i2c: i2c-core-acpi: Add i2c_acpi_dev_name() Daniel Scally
2020-11-30 17:11 ` Laurent Pinchart
2020-12-02 22:44 ` Dan Scally
2020-11-30 13:31 ` [PATCH 16/18] i2c: i2c-core-base: Use the new i2c_acpi_dev_name() in i2c_set_dev_name() Daniel Scally
2020-11-30 17:12 ` Laurent Pinchart
2020-11-30 19:18 ` Andy Shevchenko
2020-12-02 9:35 ` Andy Shevchenko
2020-12-02 9:49 ` Dan Scally
2020-12-01 23:50 ` Dan Scally
2020-11-30 13:31 ` [PATCH 17/18] gpio: gpiolib-acpi: Export acpi_get_gpiod() Daniel Scally
2020-11-30 17:02 ` kernel test robot
2020-11-30 18:04 ` kernel test robot
2020-11-30 19:20 ` Andy Shevchenko
2020-11-30 13:31 ` [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device Daniel Scally
2020-11-30 16:17 ` Jean-Michel Hautbois
2020-11-30 23:20 ` Dan Scally
2020-12-01 18:31 ` Andy Shevchenko
2020-12-01 18:36 ` Laurent Pinchart
2020-12-01 18:51 ` Andy Shevchenko
2020-11-30 16:29 ` Kieran Bingham
2020-11-30 17:21 ` Laurent Pinchart
2020-11-30 20:07 ` Andy Shevchenko
2020-11-30 23:32 ` Laurent Pinchart
2020-12-01 15:55 ` Sakari Ailus
2020-12-01 18:37 ` Laurent Pinchart [this message]
2020-12-02 11:09 ` Sakari Ailus
2020-12-02 12:42 ` Laurent Pinchart
2020-12-02 15:08 ` Andy Shevchenko
2020-12-03 12:37 ` Dan Scally
2020-12-03 12:57 ` Andy Shevchenko
2020-12-01 18:49 ` Andy Shevchenko
2020-12-01 20:59 ` Dan Scally
2020-12-02 9:39 ` Andy Shevchenko
2020-12-02 12:35 ` Laurent Pinchart
2020-12-02 15:11 ` Andy Shevchenko
2020-12-03 12:25 ` Dan Scally
2020-12-13 22:48 ` Daniel Scally
2020-12-14 15:33 ` Andy Shevchenko
2020-12-01 8:30 ` Dan Scally
2020-12-01 18:54 ` Andy Shevchenko
2020-12-01 18:55 ` Laurent Pinchart
2020-12-01 19:05 ` Andy Shevchenko
2020-12-01 19:06 ` Laurent Pinchart
2020-12-01 19:21 ` Andy Shevchenko
2020-12-01 20:34 ` Hans de Goede
2020-12-01 20:46 ` Andy Shevchenko
2020-12-02 12:48 ` Laurent Pinchart
2020-12-02 15:15 ` Andy Shevchenko
2020-12-01 21:05 ` Dan Scally
2020-12-02 9:42 ` Andy Shevchenko
2021-01-07 23:55 ` Daniel Scally
2021-01-08 12:17 ` Andy Shevchenko
2021-01-08 23:24 ` Daniel Scally
[not found] ` <CAHp75Vcy878xKUUUDH5ory9uS-Vhhx_W1PZc=S6hsSLYJ0i60w@mail.gmail.com>
2021-01-09 9:58 ` Daniel Scally
2021-01-09 0:18 ` Laurent Pinchart
2021-01-09 0:39 ` Daniel Scally
2020-11-30 20:52 ` Sakari Ailus
2020-11-30 23:06 ` Dan Scally
2020-11-30 23:21 ` Laurent Pinchart
2020-12-01 0:05 ` Dan Scally
2020-12-01 6:44 ` Sakari Ailus
2020-12-01 8:08 ` Dan Scally
2020-12-01 8:09 ` Dan Scally
2020-12-01 12:32 ` Sakari Ailus
2020-12-01 12:40 ` Laurent Pinchart
2020-12-01 12:44 ` Sakari Ailus
2020-12-01 12:48 ` Dan Scally
2020-12-01 19:01 ` Andy Shevchenko
2020-12-01 18:55 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201201183758.GE3085@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=bgolaszewski@baylibre.com \
--cc=bingbu.cao@intel.com \
--cc=devel@acpica.org \
--cc=djrscally@gmail.com \
--cc=erik.kaneda@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=jacopo+renesas@jmondi.org \
--cc=jorhand@linux.microsoft.com \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=kitakar@gmail.com \
--cc=laurent.pinchart+renesas@ideasonboard.com \
--cc=lenb@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=mchehab@kernel.org \
--cc=mika.westerberg@linux.intel.com \
--cc=pmladek@suse.com \
--cc=rjw@rjwysocki.net \
--cc=robert.moore@intel.com \
--cc=rostedt@goodmis.org \
--cc=sakari.ailus@linux.intel.com \
--cc=sergey.senozhatsky@gmail.com \
--cc=tian.shu.qiu@intel.com \
--cc=wsa@kernel.org \
--cc=yong.zhi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).