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 09:31, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, May 18, 2023 at 05:32:06PM +0200, Hans de Goede wrote:
>> On x86/ACPI platforms the GPIO resources do not provide information
>> about which GPIO resource maps to which connection-id. So e.g.
>> gpiod_get(devg, "reset") does not work.
>>
>> On devices with an Intel IPU3 or newer ISP there is a special ACPI
>> INT3472 device describing the GPIOs and instantiating of the i2c_client
>> for a sensor is deferred until the INT3472 driver has been bound based
>> on the sensor ACPI device having a _DEP on the INT3472 ACPI device.
>>
>> This allows the INT3472 driver to add the necessary GPIO lookups
>> without needing any special ACPI handling in the sensor driver.
>>
>> Unfortunately this does not work on devices with an atomisp2 ISP,
>> there the _DSM describing the GPIOs is part of the sensor ACPI device
>> itself, rather then being part of a separate ACPI device.
>>
>> IOW there is no separate firmware-node to which we can bind to register
>> the GPIO lookups (and also no way to defer creating the sensor i2c_client).
>>
>> This unfortunately means that all sensor drivers which may be used on
>> BYT or CHT hw need some code to deal with ACPI integration.
>>
>> This patch adds a new v4l2_acpi_parse_sensor_gpios() helper function
>> for this, which does all the necessary work. This minimizes the
>> (unavoidable) change to sensor drivers for ACPI integration to just
>> adding a single line calling this void function to probe().
> 
> I'd rather avoid making changes to sensor drivers due to this hack. At the
> very least it must be labelled so: this has no more to do with ACPI
> standard than that this information happens to be located in an ACPI table.

IMHO this is definitely a problem with a mismatch between how the ACPI spec.
describes GPIOs vs the linux GPIO APIs which are based on the DT model

Almost all drivers which deal with ACPI enumerated devices also have to
deal with this mismatch. Most drivers come with their own acpi_gpio_mapping
table and call devm_acpi_dev_add_driver_gpios() before doing any gpiod_get()
calls because of this. This is in no way unique to sensor drivers.

The only way around this is embedding DT bits inside ACPI and if anything
the embedding DT bits inside ACPI is the hack here.

Anyways whether this is a hack or not is bikeshedding. But your calling
it a hack seems to come from a somewhat devicetree centric view; at least
doing the DT embedding thing is certainly the exception and not the rule
in the ACPI world since most ACPI tables are written for Windows which
does not use the embedded DT bits.

The Intel ISP/IPU sensor GPIO handling is a bit special because instead
of having a simple index -> connection-id mapping it involves a _DSM,
but that part is all abstracted away inside the new helper and actually
avoids the need to have an acpi_gpio_mapping per sensor-driver, which would
be the normal way to deal with this and which would actually mean a lot
more code per driver.

> 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.

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.

Regards,

Hans