| Message ID | 20211008162121.6628-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for X86/ACPI camera sensor/PMIC setup with clk and regulator platform data | expand |
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@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). > sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); > if (!sensor) > return -ENOMEM;
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@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, 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; >
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) ? > 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. I'd like to hear what Sakari thinks about this. > >> sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); > >> if (!sensor) > >> return -ENOMEM;
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
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; + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); if (!sensor) return -ENOMEM;
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(+)