Re: [PATCH 1/9] media: v4l: Add v4l2_acpi_parse_sensor_gpios() helper function

Hans de Goede
2023-05-19T11:55:09Z

Hi Sakari,

On 5/19/23 12:58, Sakari Ailus wrote:
> Hi Hans,
>
> On Fri, May 19, 2023 at 10:55:42AM +0200, Hans de Goede wrote:

<snip>

>>> Although if the number of those drivers would be small, this could be just
>>> undesirable but still somehow acceptable. And I wouldn't expect new sensors
>>> to be paired with the IPU2 anymore. How many drivers there would be
>>> roughly? I think I've seen ten-ish sensor drivers with the atomisp driver.
>>
>> About ten-ish drivers sounds right.
>>
>>> Isn't it possible to create a device for this purpose and use software
>>> nodes for the GPIOs? I guess that would be a hack as well and you'd somehow
>>> have to initialise this via other route than driver probe.
>>
>> This creates unsolvable probe-ordering problems. At a minimum we would
>> need some check inside sensor drivers for them to return -EPROBE_DEFER
>> until the GPIO mappings are added through some other means. Note that
>> without the mappings gpiod_get will return -ENOENT, so we cannot just
>> use its return value.
>
> They probably will already need this in order to make sure the atomisp
> bridge has been initialized.

The instantiation of the sensor i2c_clients and of the atomisp PCI device
is independent of each other. In my other patch series I'm moving sensor
registration for atomisp over to the v4l2-async framework like all other
bridge/ISP drivers use so that atomisp no longer needs special sensor
drivers.

And AFAIK one of the reasons for having the v4l2-async framework is
to avoid needing to have a specific probe order between bridge vs
sensor drivers.

> Could this code live in the atomisp bridge instead?

That does not solve the probe ordering problem the sensor drivers
need the GPIOs to enable the sensor and they all enable the sensor
in their probe() to check the hw-id. The sensor's probe() function
runs without any ordering guarantees vs the ISP/brige's probe()
function. This is not an issue because at least during probe()
the sensor drivers don't have any interactions with the bridge
and any further access to the sensor-drivers is delayed until
the v4l2-async notifier completion callback has run.

The only way to work around the probe-ordering problem would
be to delay checking the hw-id until the sensor gets linked
to the bridge. Which would mean registering an async notifier
from the sensors to catch binding from the sensor drivers
and allowing the binding to fail.

Delaying the hw-id check like this would be much more involved
then the currently proposed solution and will likely cause
other issues like the wrong driver binding when hw-vendors
get the ACPI hw-id wrong (this is a known issue with audio-codecs,
so chances are we will see this with sensors too).

>> And if we need some changes anyways just adding the single line call
>> to the new helper seems both the least invasive and the easiest.
>
> Simplest given the current patches, surely. But nothing to do with the
> sensor drivers. I'd at least like to see relatively trivial ways to avoid
> this researched first.

There are no trivial ways to avoid this. Period, full stop. I have
been looking into this for quite some time now and this really is
the easiest solution.

I cannot help but feel that you are blocking progress here purely
because you have a very dt centric view. I've done my best to make
the changes as non-invasive as possible and I must say I'm quite
unhappy about the in my eyes unnecessary and unproductive push back
here. I say unproductive because you have not offered any workable
alternatives yet.

Please keep in mind that the atomisp work I do is a volunteer /
side project, so as such I have limited time to invest in this and
I must say that your in my eyes unproductive unnecessary push back
is not helping with my motivation for this.

Also I'm basically fixing a mess here which Intel has left behind by
never properly doing hw-enablement for this...

Regards,

Hans