| Message ID | 20230518153214.194976-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: v4l: Add v4l2_acpi_parse_sensor_gpios() helper + gc0310 sensor driver | expand |
On Thu, May 18, 2023 at 6:32 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi All, > > Now that the atomisp driver supports v4l2-async sensor driver registration > (I'll post this as a separate series), there is no longer a need to have > atomisp specific sensor drivers and after cleanup atomisp sensor drivers > can now be moved to drivers/media/i2c as regular v4l2 sensor drivers! Cool! But since the patch 9/9 in that series is WIP (as far as I can see), I think we first need to concentrate to review that one. OTOH I'm not sure I got this right that I'm thinking that this series depends on that one one way or the other.
Hi, On 5/18/23 18:02, Andy Shevchenko wrote: > On Thu, May 18, 2023 at 6:32 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi All, >> >> Now that the atomisp driver supports v4l2-async sensor driver registration >> (I'll post this as a separate series), there is no longer a need to have >> atomisp specific sensor drivers and after cleanup atomisp sensor drivers >> can now be moved to drivers/media/i2c as regular v4l2 sensor drivers! > > Cool! > > But since the patch 9/9 in that series is WIP (as far as I can see), I > think we first need to concentrate to review that one. OTOH I'm not > sure I got this right that I'm thinking that this series depends on > that one one way or the other. Only patches 3/9 and 9/9 depend on the other series in the sense that they would break gc0310 support in atomisp without the other series. 3/9 can be moved to the end so the other 7 patches can be merged. Most important in this series and the reason for posting it separately is getting Sakari to ack patch 1/9 . Sakari, I know you don't want platform dependent code in the sensor drivers and I understand why. But in this case it is really unavoidable. To make this as painless as possible the helper function has been made so that all sensors which may be used on affected platforms only need this one extra line: v4l2_acpi_parse_sensor_gpios(&client->dev); And on none ACPI platforms or when client was not instantiated through ACPI this will be a stub / no-op. So I hope that you are ok with this as a solution for how the ACPI tables on these devices work ? Regards, Hans
On Thu, May 18, 2023 at 6:32 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi All, > > Now that the atomisp driver supports v4l2-async sensor driver registration > (I'll post this as a separate series), there is no longer a need to have > atomisp specific sensor drivers and after cleanup atomisp sensor drivers > can now be moved to drivers/media/i2c as regular v4l2 sensor drivers! > > There is one small bit of ugliness involved though. On atomisp hw > the sensor drivers need to do some work to get GPIO mappings from ACPI, > requiring 1 line per sensor driver to call an ACPI specific sensor driver > helper. The commit msg of patch 1/9 explains why: > > """ > 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(). > > There also is a no-op stub provided for non ACPI platforms so that > no #ifdef-s are necessary in the driver. > """ > > This series adds the v4l2_acpi_parse_sensor_gpios() helper and > then after some small cleanups moves the atomisp-gc0310 sensor > driver (which now is a generic v4l2 sensor driver) to > drivers/media/i2c. > > Since this touches drivers/media the intention is for this > series to go upstream through the normal drivers/media process > rather then through my media-atomisp branch. > > Note to v4l2-core maintainers: if you are ok with this series > and would prefer for me to send it upstream through my > media-atomisp branch, then I can do that too. For the non-commented or in case you address comments my way Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Regards, > > Hans > > > Hans de Goede (9): > media: v4l: Add v4l2_acpi_parse_sensor_gpios() helper function > media: atomisp: Switch to v4l2_acpi_parse_sensor_gpios() > media: atomisp: gc0310: Turn into standard v4l2 sensor driver > media: atomisp: gc0310: Drop XXGC0310 ACPI hardware-id > media: atomisp: gc0310: Fix double free in gc0310_remove() > media: atomisp: gc0310: Cleanup includes > media: atomisp: gc0310: Remove gc0310_s_config() function > media: atomisp: gc0310: Remove gc0310.h > media: Move gc0310 sensor drivers to drivers/media/i2c/ > > Documentation/driver-api/media/v4l2-acpi.rst | 5 + > Documentation/driver-api/media/v4l2-core.rst | 1 + > drivers/media/i2c/Kconfig | 10 + > drivers/media/i2c/Makefile | 1 + > .../atomisp-gc0310.c => media/i2c/gc0310.c} | 296 ++++++++++++++--- > drivers/media/v4l2-core/Makefile | 1 + > drivers/media/v4l2-core/v4l2-acpi.c | 227 +++++++++++++ > drivers/staging/media/atomisp/i2c/Kconfig | 8 - > drivers/staging/media/atomisp/i2c/Makefile | 1 - > .../media/atomisp/i2c/atomisp-ov2680.c | 5 +- > drivers/staging/media/atomisp/i2c/gc0310.h | 309 ------------------ > .../media/atomisp/pci/atomisp_gmin_platform.c | 240 -------------- > include/media/v4l2-acpi.h | 37 +++ > 13 files changed, 540 insertions(+), 601 deletions(-) > create mode 100644 Documentation/driver-api/media/v4l2-acpi.rst > rename drivers/{staging/media/atomisp/i2c/atomisp-gc0310.c => media/i2c/gc0310.c} (64%) > create mode 100644 drivers/media/v4l2-core/v4l2-acpi.c > delete mode 100644 drivers/staging/media/atomisp/i2c/gc0310.h > create mode 100644 include/media/v4l2-acpi.h > > -- > 2.40.1 >
Hi All, Now that the atomisp driver supports v4l2-async sensor driver registration (I'll post this as a separate series), there is no longer a need to have atomisp specific sensor drivers and after cleanup atomisp sensor drivers can now be moved to drivers/media/i2c as regular v4l2 sensor drivers! There is one small bit of ugliness involved though. On atomisp hw the sensor drivers need to do some work to get GPIO mappings from ACPI, requiring 1 line per sensor driver to call an ACPI specific sensor driver helper. The commit msg of patch 1/9 explains why: """ 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(). There also is a no-op stub provided for non ACPI platforms so that no #ifdef-s are necessary in the driver. """ This series adds the v4l2_acpi_parse_sensor_gpios() helper and then after some small cleanups moves the atomisp-gc0310 sensor driver (which now is a generic v4l2 sensor driver) to drivers/media/i2c. Since this touches drivers/media the intention is for this series to go upstream through the normal drivers/media process rather then through my media-atomisp branch. Note to v4l2-core maintainers: if you are ok with this series and would prefer for me to send it upstream through my media-atomisp branch, then I can do that too. Regards, Hans Hans de Goede (9): media: v4l: Add v4l2_acpi_parse_sensor_gpios() helper function media: atomisp: Switch to v4l2_acpi_parse_sensor_gpios() media: atomisp: gc0310: Turn into standard v4l2 sensor driver media: atomisp: gc0310: Drop XXGC0310 ACPI hardware-id media: atomisp: gc0310: Fix double free in gc0310_remove() media: atomisp: gc0310: Cleanup includes media: atomisp: gc0310: Remove gc0310_s_config() function media: atomisp: gc0310: Remove gc0310.h media: Move gc0310 sensor drivers to drivers/media/i2c/ Documentation/driver-api/media/v4l2-acpi.rst | 5 + Documentation/driver-api/media/v4l2-core.rst | 1 + drivers/media/i2c/Kconfig | 10 + drivers/media/i2c/Makefile | 1 + .../atomisp-gc0310.c => media/i2c/gc0310.c} | 296 ++++++++++++++--- drivers/media/v4l2-core/Makefile | 1 + drivers/media/v4l2-core/v4l2-acpi.c | 227 +++++++++++++ drivers/staging/media/atomisp/i2c/Kconfig | 8 - drivers/staging/media/atomisp/i2c/Makefile | 1 - .../media/atomisp/i2c/atomisp-ov2680.c | 5 +- drivers/staging/media/atomisp/i2c/gc0310.h | 309 ------------------ .../media/atomisp/pci/atomisp_gmin_platform.c | 240 -------------- include/media/v4l2-acpi.h | 37 +++ 13 files changed, 540 insertions(+), 601 deletions(-) create mode 100644 Documentation/driver-api/media/v4l2-acpi.rst rename drivers/{staging/media/atomisp/i2c/atomisp-gc0310.c => media/i2c/gc0310.c} (64%) create mode 100644 drivers/media/v4l2-core/v4l2-acpi.c delete mode 100644 drivers/staging/media/atomisp/i2c/gc0310.h create mode 100644 include/media/v4l2-acpi.h