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

[Hans de Goede <hdegoede@redhat.com>]

Hi Sakari,

On 5/19/23 14:16, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, May 19, 2023 at 01:55:04PM +0200, Hans de Goede wrote:
>> 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).
> 
> A simple question: how do you solve the probe ordering issue when it comes
> to the atomisp bridge creating the graph endpoints needed by sensor
> drivers?
> 
> Or do you assume the sensor drivers will always use some static
> configuration?

This is all modeled after the IPU3 work done by Dan Scally and
ATM the involved sensor drivers assume a static configuration
wrt number of lanes + link frequency.

If / when we want to start supporting say both single + dual
lane modes (with e.g. reduced framerate for high res in single lane)
then AFAICT this will only influence things like e.g. subdev calls
to get supported framerates and of course turning on streaming,
all of which only ever happen after the async subdev registration
of all involved subdevs is complete.

So (again AFAICT) unlike the GPIOs there is no need to need
to know the endpoint configuration at probe() time. But since
we do want to turn the sensor on to check it is actually there
and if it is the type of sensor we expect during probe() we
do need the GPIOs beforehand.

Regards,

Hans