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

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Dan,

On Tue, Dec 01, 2020 at 08:08:26AM +0000, Dan Scally wrote:
> 
> On 01/12/2020 06:44, Sakari Ailus wrote:
> > Hi Dan,
> >
> > On Mon, Nov 30, 2020 at 11:06:03PM +0000, Dan Scally wrote:
> >> Hi Sakari
> >>
> >> On 30/11/2020 20:52, Sakari Ailus wrote:
> >>>> +static const struct acpi_device_id int3472_device_id[] = {
> >>>> +	{ "INT3472", 0 },
> >>> The INT3472 _HID is really allocated for the tps68470 PMIC chip. It may not
> >>> be used by other drivers; people will want to build kernels where both of
> >>> these ACPI table layouts are functional.
> >>>
> >>> Instead, I propose, that you add this as an option to the tps68470 driver
> >>> that figures out whether the ACPI device for the tps68470 device actually
> >>> describes something else, in a similar fashion you do with the cio2-bridge
> >>> driver. I think it may need a separate Kconfig option albeit this and
> >>> cio2-bridge cannot be used separately.
> >> It actually occurs to me that that may not work (I know I called that
> >> out as an option we considered, but that was a while ago actually). The
> >> reason I wasn't worried about the existing tps68470 driver binding to
> >> these devices is that it's an i2c driver, and these dummy devices don't
> >> have an I2cSerialBusV2, so no I2C device is created by them the kernel.
> >>
> >>
> >> Won't that mean the tps68470 driver won't ever be probed for these devices?
> > Oops. I missed this indeed was not an I²C driver. So please ignore the
> > comment.
> >
> > So I guess this wouldn't be an actual problem. I'd still like to test this
> > on a system with tps68470, as the rest of the set.
> On my Go2, it .probes() for the actual tps68740 (that machine has both
> types of INT3472 device) but fails with EINVAL when it can't find the
> CLDB buffer that these discrete type devices have. My understanding is
> that means it's free for the actual tps68470 driver to grab the device;
> although that's not happening because I had to blacklist that driver or
> it stops the machine from booting at the moment - haven't gotten round
> to investigating yet.

Oh, then the problem is actually there. If it probes the tps68470 driver on
the systems with Windows ACPI tables, then it should be that driver which
works with the Windows ACPI tables, too.

Checking for random objects such as CLDB in multiple drivers and returning
an error based on them being there or not wouldn't be exactly neat.
Although I'm not sure thare are options that are obviosly pretty here. I
wouldn't two separate drivers checking for e.g. CLDB (tps68470 + this one).

The tps68470 driver is an MFD driver that instantiates a number of platform
devices. Alternatively, if you make this one a platform device, you can, in
case the CLDB (or whatever object) is present, in the tps68470 driver
instantiate a device for this driver instead of the rest.

So I'd think what matters is that both drivers can be selected at the same
time but the user does not need to manually select them. Both ways could
work I guess?

-- 
Kind regards,

Sakari Ailus