Hi Hans, On Fri, May 19, 2023 at 10:55:42AM +0200, Hans de Goede wrote: > 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. I'm not referring referring to differences in GPIOs between ACPI and Linux, but the rest, and why drivers need to call the newly added function. > > > 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 initialised. Could this code live in the atomisp bridge instead? > > 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. -- Kind regards, Sakari Ailus
