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

[Dan Scally]

On 01/12/2020 08:08, 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.
Though having said that, it looks like a separate driver like this is
the least favoured option anyway, so probably it's going to change anyway.