[lkml]   [2020]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device
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


> > > + 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

 \ /
  Last update: 2020-12-01 19:51    [W:0.337 / U:3.476 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site