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@xxxxxxxxxx> >>>> --- >>>> 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
