Hi,
On 9/29/23 14:37, Sakari Ailus wrote:
> Hi Laurent,
>
> On Fri, Sep 15, 2023 at 12:50:27PM +0300, Laurent Pinchart wrote:
>> Hi Sakari,
>>
>> Thank you for the patch.
>>
>> On Fri, Sep 15, 2023 at 10:28:09AM +0300, Sakari Ailus wrote:
>>> With ipu bridge, endpoints may only be created when ipu bridge has
>>> initialised. This may happen after the sensor driver has first probed.
>>
>> That's hard to understand for someone not familiar with the ipu-bridge
>> driver. Could you please expand the commit message ?
>>
>> Also, is there a way to avoid the ov2740 probing before the required
>> initialization is complete ?
>
> One possibility would be to move the ipu bridge functionality to the ACPI
> framework itself. Then it'd be independent of probing any drivers. It
> hasn't been discussed and I'm not sure what the result might be. In any
> case I'd like to have DisCo for Imaging support there first.
The problem is not the IPU bridge functionality per se. We already delay
sensor i2c-client instantiation on ipu3 and ipu6 till after the INT3472
driver has loaded since that does things like register gpio, clk and
regulator lookup tables to glue the ACPI approach into standard kernel
subsytems and probing before the lookups are there leads to dummy-regulators
and optional GPIOs and clks not working.
So one might argue that this bit of code should be moved to
the INT3472 code, making it delay i2c-client instantiation until
the fwnode-graph has been populated by the ipu-bridge code:
static int ov2740_check_hwcfg(struct device *dev)
{
struct fwnode_handle *ep;
struct fwnode_handle *fwnode = dev_fwnode(dev);
...
ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
if (!ep)
return -EPROBE_DEFER;
But notice the `struct device *dev` parameter, that *is*
the i2c_client; and as soon as the INT3472 code has told
the ACPI core that it is ok to move forward with instantiating
the i2c_client by calling acpi_dev_clear_dependencies(INT3472-adev)
then it is too late to do this check inside INT3472 and before the
acpi_dev_clear_dependencies(INT3472-adev) call the i2c-client does
not exist.
So fixing this requires hacks in either the ACPI core and/or
in the i2c-core (to mark a device as initially unprobeable).
And then we are not talking about the atomisp2 yet where
the sensor ACPI device-nodes do not have a _DEP (dependency)
like INT3472 which we can use to delay instantiating
the i2c-clients. With the INT3472 device we can use
the INT3472 hardware-id to trigger deferring instantiation
on that (by default the ACPI core does not honor _DEP for
various reasons). So delaying instantiation on atomisp2
hardware would mean maintaining a list of hardcoded sensor
hardware-ids for which we want to delay instantiation +
some other ACPI core hack to say: it is ok to move forward
with instantiation now.
Last time Sakari and I discussed this we came to
the conclusion that, e.g. (from ov2680.c):
/*
* Sometimes the fwnode graph is initialized by the bridge driver.
* Bridge drivers doing this may also add GPIO mappings, wait for this.
*/
ep_fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
if (!ep_fwnode)
return dev_err_probe(dev, -EPROBE_DEFER,
"waiting for fwnode graph endpoint\n");
Is the least ugly way to deal with this, we have gone through
a lot of trouble to avoid adding ACPI-isms to the driver but this
one seems unavoidable.
Note though that the ov2680 code has both a comment and a
return dev_err_probe(dev, -EPROBE_DEFER, ...)
This last one will make the kernel print messages about devices waiting
for deferred probe + the passed in reason 30 seconds after the last
successful probe() (any successful probe anywhere in the kernel
resets the 30 seconds counter).
This helps a lot to figure out why the driver is not binding if
it is not binding because of this code-path.
I believe it is the best to do the same thing as ov2680.c here too.
Regards,
Hans
>
> Cc Hans.
>
>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>>> ---
>>> drivers/media/i2c/ov2740.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
>>> index de39a66b1b81..a132e8a283b4 100644
>>> --- a/drivers/media/i2c/ov2740.c
>>> +++ b/drivers/media/i2c/ov2740.c
>>> @@ -976,7 +976,7 @@ static int ov2740_check_hwcfg(struct device *dev)
>>>
>>> ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>>> if (!ep)
>>> - return -ENXIO;
>>> + return -EPROBE_DEFER;
>>>
>>> ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
>>> fwnode_handle_put(ep);
>>
>
