Re: [PATCH 02/12] media: i2c: ov8865: Add an has_unmet_acpi_deps() check

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 10/8/21 8:58 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Fri, Oct 08, 2021 at 08:48:18PM +0200, Hans de Goede wrote:
>> On 10/8/21 8:41 PM, Laurent Pinchart wrote:
>>> On Fri, Oct 08, 2021 at 06:21:11PM +0200, Hans de Goede wrote:
>>>> The clk and regulator frameworks expect clk/regulator consumer-devices
>>>> to have info about the consumed clks/regulators described in the device's
>>>> fw_node.
>>>>
>>>> To work around cases where this info is not present in the firmware tables,
>>>> which is often the case on x86/ACPI devices, both frameworks allow the
>>>> provider-driver to attach info about consumers to the clks/regulators
>>>> when registering these.
>>>>
>>>> This causes problems with the probe ordering of the ov8865 driver vs the
>>>> drivers for these clks/regulators. Since the lookups are only registered
>>>> when the provider-driver binds, trying to get these clks/regulators before
>>>> then results in a -ENOENT error for clks and a dummy regulator for regs.
>>>>
>>>> On ACPI/x86 where this is a problem, the ov8865 ACPI fw-nodes have a _DEP
>>>> dependency on the INT3472 ACPI fw-node which describes the hardware which
>>>> provides the clks/regulators.
>>>>
>>>> The drivers/platform/x86/intel/int3472/ code dealing with these ACPI
>>>> fw-nodes will call acpi_dev_clear_dependencies() to indicate that this
>>>> _DEP has been "met" when all the clks/regulators have been setup.
>>>>
>>>> Call the has_unmet_acpi_deps() helper to check for unmet _DEPs
>>>> and return -EPROBE_DEFER if this returns true, so that we wait for
>>>> the clk/regulator setup to be done before continuing with probing.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/media/i2c/ov8865.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
>>>> index ce4e0ae2c4d3..fd18d1256f78 100644
>>>> --- a/drivers/media/i2c/ov8865.c
>>>> +++ b/drivers/media/i2c/ov8865.c
>>>> @@ -2978,6 +2978,9 @@ static int ov8865_probe(struct i2c_client *client)
>>>>  	unsigned int i;
>>>>  	int ret;
>>>>  
>>>> +	if (has_unmet_acpi_deps(dev))
>>>> +		return -EPROBE_DEFER;
>>>> +
>>>
>>> We've worked hard to avoid adding ACPI-specific code such as this in
>>> sensor drivers, as it would then spread like crazy, and also open the
>>> door to more ACPI-specific support. I don't want to open this pandora's
>>> box, I'd like to see this handled in another layer (the I2C core could
>>> be a condidate for instance, but bonus points if it can be handled in
>>> the ACPI subsystem itself).
>>
>> The problem is that we do NOT want this check for all i2c devices,
> 
> Any of these sensors can be used on non-ACPI-based platforms, or on
> ACPI-based platforms where integration has been done right. If it causes
> an issue to call this function on those platforms, then this driver
> won't work. If it causes no issue, why can't we call it in the I2C core
> (or somewhere else) ?

This call checks the ACPI-core's count which keep track of all 
dependencies listed in the _DEP method have been marked as
"ready/available" by the driver for the Linux-device for those
dependencies having called acpi_dev_clear_dependencies().

The ACPI core code could delay instantiating devices for ACPI described
devices based on this itself, but it is deliberately not doing this.

This is deliberately not done because the _DEP method
may point to pretty much any random ACPI object and Linux does
not necessarily have a driver for all ACPI objects the driver
points too, which would lead to the devices never getting instantiated.

>> so doing
>> it in any place other then the drivers means having some list of APCI-ids
>> to which to apply this someplace else. And then for every sensor driver
>> which needs this we need to update this list.
>>
>> This is wht I've chosen to just put this check directly in the sensor
>> drivers. It is only 2 lines, it is a no-op on kernels where ACPI
>> is not enabled (without need a #ifdef) and it is a no-op if the
>> sensor i2c-client is not instantiated through APCI even when ACPI
>> support is enabled in the kernel.
>>
>> I understand that you don't want a lot of ACPI specific code inside
>> the drivers, which is why I've come up with this fix which consists
>> of only 2 lines.  My previous attempts (which I never posted)
>> where much worse then this.
> 
> So we only need to take one more step to remove just two lines :-)
> 
> This is all caused by Intel messing up their ACPI design badly. It's too
> late to point and shame, it won't fix the problem, but I don't want this
> to spread through drivers, neither for just those two lines (there are
> dozens of sensors that would need the same treatment), nor for what the
> next steps would be when someone else will want to add ACPI-specific
> code and use this as a precedent. That's why we tried hard with Dan
> Scally to isolate all the necessary quirks in a single place instead of
> spreading them through drivers, which would have been easier to
> implement.

Ok, so I've come up with a way to deal with this in the ACPI code
which does not require sensor-driver code modifications.

What I've done is make the ACPI core honor _DEP dependencies for
a device (instead of mostly ignore them) when one of those _DEPs
is for a device with a HID of INT3472 (the camera PMIC / discrete
clk/regulator ACPI device/node). This way the ACPI core can make
this decision without it having to keep an ever growing list of
sensor HIDs for which it should honor _DEP-s.

I'm about to send out a v2 of this series with this change,
see patch 1 + 2 of the v2 series.

I hope that Rafael is ok with the ACPI changes in there,
we will see...

Regards,

Hans