Hi Laurent, On 10/8/21 8:41 PM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > 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, 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. Regards, Hans > >> sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); >> if (!sensor) >> return -ENOMEM; >
