| Message ID | 20230518153214.194976-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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: > > 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. than (not the first time I see the same typo in your commit messages :-) > 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. > > Note v4l2_acpi_parse_sensor_gpios() is basically a copy of > the atomisp2 v4l2_get_acpi_sensor_info() helper from: > drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c > minus logging the sensor module-name using a second _DSM. > v4l2_get_acpi_sensor_info() was already reviewed by Andy, > see the Link tag below. > > (this code duplication is removed in the next patch in this series) I believe the above is not needed to be in the commit message, but rather in the comments below (after the '---' cutter line). ... > +/* Note this always returns 1 to continue looping so that res_count is accurate */ > +static int v4l2_acpi_handle_gpio_res(struct acpi_resource *ares, void *_data) > +{ > + struct v4l2_acpi_gpio_parsing_data *data = _data; > + struct acpi_resource_gpio *agpio; > + const char *name; > + bool active_low; > + unsigned int i; > + u32 settings; > + u8 pin; Should be u16. > + pin = agpio->pin_table[0]; > + for (i = 0; i < data->settings_count; i++) { > + if (INTEL_DSM_PIN(data->settings[i]) == pin) { > + settings = data->settings[i]; > + break; > + } > + } > + > + if (i == data->settings_count) { > + dev_warn(data->dev, "Could not find DSM GPIO settings for pin %d\n", pin); %u > + return 1; > + } > + > + switch (INTEL_DSM_TYPE(settings)) { > + case 0: > + name = "reset-gpios"; > + break; > + case 1: > + name = "powerdown-gpios"; > + break; > + default: > + dev_warn(data->dev, "Unknown GPIO type 0x%02lx for pin %d\n", %u > + INTEL_DSM_TYPE(settings), pin); > + return 1; > + } > + > + /* > + * Both reset and power-down need to be logical false when the sensor > + * is on (sensor should not be in reset and not be powered-down). So > + * when the sensor-on-value (which is the physical pin value) is high, > + * then the signal is active-low. > + */ > + active_low = INTEL_DSM_SENSOR_ON_VAL(settings) ? true : false; Ternary part is redundant. > + i = data->map_count; > + if (i == V4L2_SENSOR_MAX_ACPI_GPIOS) > + return 1; > + > + /* res_count is already incremented */ > + data->map->params[i].crs_entry_index = data->res_count - 1; > + data->map->params[i].active_low = active_low; > + data->map->mapping[i].name = name; > + data->map->mapping[i].data = &data->map->params[i]; > + data->map->mapping[i].size = 1; > + data->map_count++; > + > + dev_info(data->dev, "%s crs %d %s pin %d active-%s\n", name, %u (for pin) > + data->res_count - 1, agpio->resource_source.string_ptr, > + pin, active_low ? "low" : "high"); > + > + return 1; > +} ... > + * Note this code uses the same DSM GUID as the INT3472 discrete.c code > + * and there is some overlap, but there are enough differences that it is > + * difficult to share the code. Can you add the name of the variable in that file, so likely the source code indexing tool might add a link? ... > + struct acpi_device *adev = ACPI_COMPANION(dev); I would split this assignment and... > + struct v4l2_acpi_gpio_parsing_data data = { }; > + LIST_HEAD(resource_list); > + union acpi_object *obj; > + unsigned int i, j; > + int ret; > + ...place it here. > + if (!adev || (!soc_intel_is_byt() && !soc_intel_is_cht())) > + return; ... > + devm_acpi_dev_add_driver_gpios(dev, data.map->mapping); Won't we print a warning here as well in case of error? ... > +#ifdef CONFIG_ACPI > +struct device; This should be outside of previous ifdeffery as it's used in a stub. > +/** > + * v4l2_acpi_parse_sensor_gpios - Parse ACPI info describing sensor GPIOs. > + * Dunno the style of v4l2, but this line is redundant. > + * @dev: Device to parse the ACPI info for > + * > + * On x86/ACPI platforms the GPIO resources do not provide information > + * about which resource maps to which connection-id. > + * > + * Sensor drivers can call this function to use platform specific methods > + * (e.g. the Intel 79234640-9e10-4fea-a5c1-b5aa8b19756f _DSM) to get > + * information about the pins and add GPIO mappings to make standard gpiod_get() > + * calls work. > + * > + * The registered mappings use devm managed memory and are automatically free-ed > + * on remove() and on probe() failure. > + */ Usually the kernel doc is attached to the function implementation. > +void v4l2_acpi_parse_sensor_gpios(struct device *dev); > + > +#else /* ifdef CONFIG_ACPI */ > +static inline void v4l2_acpi_parse_sensor_gpios(struct device *dev) { return 0; } > +#endif /* ifdef CONFIG_ACPI */
Hi, On 5/18/23 18:36, Andy Shevchenko wrote: > On Thu, May 18, 2023 at 6:32 PM Hans de Goede <hdegoede@redhat.com> 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. > > than > > (not the first time I see the same typo in your commit messages :-) > > >> 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. >> >> Note v4l2_acpi_parse_sensor_gpios() is basically a copy of >> the atomisp2 v4l2_get_acpi_sensor_info() helper from: >> drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c >> minus logging the sensor module-name using a second _DSM. > >> v4l2_get_acpi_sensor_info() was already reviewed by Andy, >> see the Link tag below. >> >> (this code duplication is removed in the next patch in this series) > > I believe the above is not needed to be in the commit message, but > rather in the comments below (after the '---' cutter line). > > ... <snip> >> + * Note this code uses the same DSM GUID as the INT3472 discrete.c code >> + * and there is some overlap, but there are enough differences that it is >> + * difficult to share the code. > > Can you add the name of the variable in that file, so likely the > source code indexing tool might add a link? You mean change the comment something like this: * Note this code uses the same DSM GUID as the int3472_gpio_guid in * the INT3472 discrete.c code and there is some overlap, but there are * enough differences that it is difficult to share the code. I guess, where int3472_gpio_guid comes from: drivers/platform/x86/intel/int3472/discrete.c: static const guid_t int3472_gpio_guid = GUID_INIT(0x79234640, 0x9e10, 0x4fea, 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f); ? >> + devm_acpi_dev_add_driver_gpios(dev, data.map->mapping); > > Won't we print a warning here as well in case of error? The only way this can fail is with -ENOMEM (we already know dev has an ACPI companion) and generally speaking the rule is to not log errors for ENOMEM since when we hit that the kernel already complains loudly before returning from the alloc call. > ... > >> +#ifdef CONFIG_ACPI > >> +struct device; > > This should be outside of previous ifdeffery as it's used in a stub. > >> +/** >> + * v4l2_acpi_parse_sensor_gpios - Parse ACPI info describing sensor GPIOs. >> + * > > Dunno the style of v4l2, but this line is redundant. > >> + * @dev: Device to parse the ACPI info for >> + * >> + * On x86/ACPI platforms the GPIO resources do not provide information >> + * about which resource maps to which connection-id. >> + * >> + * Sensor drivers can call this function to use platform specific methods >> + * (e.g. the Intel 79234640-9e10-4fea-a5c1-b5aa8b19756f _DSM) to get >> + * information about the pins and add GPIO mappings to make standard gpiod_get() >> + * calls work. >> + * >> + * The registered mappings use devm managed memory and are automatically free-ed >> + * on remove() and on probe() failure. >> + */ > > Usually the kernel doc is attached to the function implementation Thank you for the review! I agree with all your other points and I'll address them in the next version. The kernel doc being here is modeled after newer (less old) v4l2 APIs like include/media/v4l2-fwnode.h and include/media/v4l2-async.h which all also have this in the header. So unless the v4l2 folks want me to change this I'm going to keep the kernel docs in the header. (more in general I have seen both header and next to the body placements for these kernel docs in various places in the kernel) Regards, Hans >> +void v4l2_acpi_parse_sensor_gpios(struct device *dev); >> + >> +#else /* ifdef CONFIG_ACPI */ >> +static inline void v4l2_acpi_parse_sensor_gpios(struct device *dev) { return 0; } >> +#endif /* ifdef CONFIG_ACPI */ >
Hi Hans,
kernel test robot noticed the following build warnings:
[auto build test WARNING on media-tree/master]
[also build test WARNING on linus/master sailus-media-tree/streams v6.4-rc2 next-20230518]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l-Add-v4l2_acpi_parse_sensor_gpios-helper-function/20230518-233727
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230518153214.194976-2-hdegoede%40redhat.com
patch subject: [PATCH 1/9] media: v4l: Add v4l2_acpi_parse_sensor_gpios() helper function
config: ia64-buildonly-randconfig-r004-20230517
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f25b51366af07f398d93a8ab49d51668cad2e258
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hans-de-Goede/media-v4l-Add-v4l2_acpi_parse_sensor_gpios-helper-function/20230518-233727
git checkout f25b51366af07f398d93a8ab49d51668cad2e258
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/media/v4l2-core/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305190321.6Dq9id9F-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/media/v4l2-core/v4l2-acpi.c:140:6: warning: no previous prototype for 'v4l2_acpi_parse_sensor_gpios' [-Wmissing-prototypes]
140 | void v4l2_acpi_parse_sensor_gpios(struct device *dev)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/v4l2_acpi_parse_sensor_gpios +140 drivers/media/v4l2-core/v4l2-acpi.c
123
124 /*
125 * Helper function to create an ACPI GPIO lookup table for sensor reset and
126 * powerdown signals on Intel Bay Trail (BYT) and Cherry Trail (CHT) devices,
127 * including setting the correct polarity for the GPIO.
128 *
129 * This uses the "79234640-9e10-4fea-a5c1-b5aa8b19756f" DSM method directly
130 * on the sensor device's ACPI node. This is different from later Intel
131 * hardware which has a separate INT3472 with this info. Since there is
132 * no separate firmware-node to which we can bind to register the GPIO lookups
133 * this unfortunately means that all sensor drivers which may be used on
134 * BYT or CHT hw need to call this function.
135 *
136 * Note this code uses the same DSM GUID as the INT3472 discrete.c code
137 * and there is some overlap, but there are enough differences that it is
138 * difficult to share the code.
139 */
> 140 void v4l2_acpi_parse_sensor_gpios(struct device *dev)
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. 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. 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.
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
On Thu, May 18, 2023 at 7:57 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 5/18/23 18:36, Andy Shevchenko wrote: > > On Thu, May 18, 2023 at 6:32 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > >> + * Note this code uses the same DSM GUID as the INT3472 discrete.c code > >> + * and there is some overlap, but there are enough differences that it is > >> + * difficult to share the code. > > > > Can you add the name of the variable in that file, so likely the > > source code indexing tool might add a link? > > You mean change the comment something like this: > > * Note this code uses the same DSM GUID as the int3472_gpio_guid in > * the INT3472 discrete.c code and there is some overlap, but there are > * enough differences that it is difficult to share the code. > > I guess, where int3472_gpio_guid comes from: > > drivers/platform/x86/intel/int3472/discrete.c: > > static const guid_t int3472_gpio_guid = > GUID_INIT(0x79234640, 0x9e10, 0x4fea, > 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f); > > > ? Yes! ... > >> + devm_acpi_dev_add_driver_gpios(dev, data.map->mapping); > > > > Won't we print a warning here as well in case of error? > > The only way this can fail is with -ENOMEM (we already know dev > has an ACPI companion) and generally speaking the rule is to > not log errors for ENOMEM since when we hit that the kernel > already complains loudly before returning from the alloc call. But this is based on the information about internal implementation of the above mentioned API. Strictly speaking it might return, in the future, another error code as well.
On Fri, May 19, 2023 at 11:55 AM Hans de Goede <hdegoede@redhat.com> wrote: > On 5/19/23 09:31, Sakari Ailus wrote: ... > 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. I'm on Hans' side here. And yes, many drivers (mostly in the ASoC area, i.e. audio codecs) nowadays have GPIO ACPI mapping tables due to lack of _DSD in ACPI. It's a problem of firmware which we can't avoid if we really want to support this hardware in Linux.
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.
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
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?
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
Hi Hans, On Fri, May 19, 2023 at 03:08:02PM +0200, Hans de Goede wrote: > 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. In general the number of lanes used is dependent on a particular board, and in many cases the same sensor is used on different systems with a different lane configuration. This might not happen on sensors used with atomisp but it probably already happens on those used by the CIO2 (ov8865). What the drivers should do and in most cases do is that they check the lane configuration is valid for the device. > > 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. On sensors the number of lanes is practically never selected dynamically, it's simply a property of the board (the CSI-2 receiver and how many wires happen to be connected, whichever has fewer lanes). > > 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. The other, somewhat cleaner in my opinion and clearly more future-proof, would be to return -EPROBE_DEFER if there are no endpoints. Which is also what the ov8865 driver does already. In the long run such bridge code could hopefully live in the ACPI framework itself. Then we would have no probe ordering issues anymore. But that's probably not going to happen for quite a while.
Hi, On 5/19/23 16:01, Sakari Ailus wrote: > Hi Hans, > > On Fri, May 19, 2023 at 03:08:02PM +0200, Hans de Goede wrote: >> 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. > > In general the number of lanes used is dependent on a particular board, and > in many cases the same sensor is used on different systems with a different > lane configuration. This might not happen on sensors used with atomisp but > it probably already happens on those used by the CIO2 (ov8865). > > What the drivers should do and in most cases do is that they check the lane > configuration is valid for the device. > >> >> 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. > > On sensors the number of lanes is practically never selected dynamically, > it's simply a property of the board (the CSI-2 receiver and how many wires > happen to be connected, whichever has fewer lanes). Right, I understand / agree what I meant is that the driver does not really need to know the number of connected lanes during probe(). But yes if you want to check the number of lanes is valid in probe() then you do need to know it. >> 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. > > The other, somewhat cleaner in my opinion and clearly more future-proof, > would be to return -EPROBE_DEFER if there are no endpoints. Which is also > what the ov8865 driver does already. Ok that is a good suggestion, which I think should work. I'll give this a try sometime during the coming days. Regards, Hans
Hi Hans, On Fri, May 19, 2023 at 04:39:29PM +0200, Hans de Goede wrote: > Hi, > > On 5/19/23 16:01, Sakari Ailus wrote: > > Hi Hans, > > > > On Fri, May 19, 2023 at 03:08:02PM +0200, Hans de Goede wrote: > >> 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. > > > > In general the number of lanes used is dependent on a particular board, and > > in many cases the same sensor is used on different systems with a different > > lane configuration. This might not happen on sensors used with atomisp but > > it probably already happens on those used by the CIO2 (ov8865). > > > > What the drivers should do and in most cases do is that they check the lane > > configuration is valid for the device. > > > >> > >> 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. > > > > On sensors the number of lanes is practically never selected dynamically, > > it's simply a property of the board (the CSI-2 receiver and how many wires > > happen to be connected, whichever has fewer lanes). > > Right, I understand / agree what I meant is that the driver does not > really need to know the number of connected lanes during probe(). > > But yes if you want to check the number of lanes is valid in probe() > then you do need to know it. > > >> 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. > > > > The other, somewhat cleaner in my opinion and clearly more future-proof, > > would be to return -EPROBE_DEFER if there are no endpoints. Which is also > > what the ov8865 driver does already. > > Ok that is a good suggestion, which I think should work. I'll give this > a try sometime during the coming days. Thanks, looking forward to see v2!
diff --git a/Documentation/driver-api/media/v4l2-acpi.rst b/Documentation/driver-api/media/v4l2-acpi.rst new file mode 100644 index 000000000000..366e85187976 --- /dev/null +++ b/Documentation/driver-api/media/v4l2-acpi.rst @@ -0,0 +1,5 @@ +.. SPDX-License-Identifier: GPL-2.0 + +V4L2 ACPI kAPI +^^^^^^^^^^^^^^ +.. kernel-doc:: include/media/v4l2-acpi.h diff --git a/Documentation/driver-api/media/v4l2-core.rst b/Documentation/driver-api/media/v4l2-core.rst index 1a8c4a5f256b..faef0e918ca3 100644 --- a/Documentation/driver-api/media/v4l2-core.rst +++ b/Documentation/driver-api/media/v4l2-core.rst @@ -26,3 +26,4 @@ Video4Linux devices v4l2-tuner v4l2-common v4l2-tveeprom + v4l2-acpi diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile index 41d91bd10cf2..3ad6c6ab5e74 100644 --- a/drivers/media/v4l2-core/Makefile +++ b/drivers/media/v4l2-core/Makefile @@ -15,6 +15,7 @@ videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \ # Please keep it alphabetically sorted by Kconfig name # (e. g. LC_ALL=C sort Makefile) +videodev-$(CONFIG_ACPI) += v4l2-acpi.o videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o videodev-$(CONFIG_SPI) += v4l2-spi.o diff --git a/drivers/media/v4l2-core/v4l2-acpi.c b/drivers/media/v4l2-core/v4l2-acpi.c new file mode 100644 index 000000000000..95278c619d82 --- /dev/null +++ b/drivers/media/v4l2-core/v4l2-acpi.c @@ -0,0 +1,227 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * V4L2 sensor driver ACPI helpers + * + * Copyright (C) 2023, Hans de Goede <hansg@kernel.org> + */ + +#include <linux/acpi.h> +#include <linux/device.h> +#include <linux/gpio/consumer.h> +#include <linux/list.h> +#include <linux/platform_data/x86/soc.h> +#include <linux/uuid.h> + +/* + * 79234640-9e10-4fea-a5c1-b5aa8b19756f + * This _DSM GUID returns information about the GPIO lines mapped to a sensor. + * Function number 1 returns a count of the GPIO lines that are mapped. + * Subsequent functions return 32 bit ints encoding information about the GPIO. + */ +static const guid_t intel_sensor_gpio_info_guid = + GUID_INIT(0x79234640, 0x9e10, 0x4fea, + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f); + +#define INTEL_DSM_TYPE_SHIFT 0 +#define INTEL_DSM_TYPE_MASK GENMASK(7, 0) +#define INTEL_DSM_PIN_SHIFT 8 +#define INTEL_DSM_PIN_MASK GENMASK(15, 8) +#define INTEL_DSM_SENSOR_ON_VAL_SHIFT 24 +#define INTEL_DSM_SENSOR_ON_VAL_MASK GENMASK(31, 24) + +#define INTEL_DSM_TYPE(x) \ + (((x) & INTEL_DSM_TYPE_MASK) >> INTEL_DSM_TYPE_SHIFT) +#define INTEL_DSM_PIN(x) \ + (((x) & INTEL_DSM_PIN_MASK) >> INTEL_DSM_PIN_SHIFT) +#define INTEL_DSM_SENSOR_ON_VAL(x) \ + (((x) & INTEL_DSM_SENSOR_ON_VAL_MASK) >> INTEL_DSM_SENSOR_ON_VAL_SHIFT) + +#define V4L2_SENSOR_MAX_ACPI_GPIOS 2u + +struct v4l2_acpi_gpio_map { + struct acpi_gpio_params params[V4L2_SENSOR_MAX_ACPI_GPIOS]; + struct acpi_gpio_mapping mapping[V4L2_SENSOR_MAX_ACPI_GPIOS + 1]; +}; + +struct v4l2_acpi_gpio_parsing_data { + struct device *dev; + u32 settings[V4L2_SENSOR_MAX_ACPI_GPIOS]; + unsigned int settings_count; + unsigned int res_count; + unsigned int map_count; + struct v4l2_acpi_gpio_map *map; +}; + +/* Note this always returns 1 to continue looping so that res_count is accurate */ +static int v4l2_acpi_handle_gpio_res(struct acpi_resource *ares, void *_data) +{ + struct v4l2_acpi_gpio_parsing_data *data = _data; + struct acpi_resource_gpio *agpio; + const char *name; + bool active_low; + unsigned int i; + u32 settings; + u8 pin; + + if (!acpi_gpio_get_io_resource(ares, &agpio)) + return 1; /* Not a GPIO, continue the loop */ + + data->res_count++; + + pin = agpio->pin_table[0]; + for (i = 0; i < data->settings_count; i++) { + if (INTEL_DSM_PIN(data->settings[i]) == pin) { + settings = data->settings[i]; + break; + } + } + + if (i == data->settings_count) { + dev_warn(data->dev, "Could not find DSM GPIO settings for pin %d\n", pin); + return 1; + } + + switch (INTEL_DSM_TYPE(settings)) { + case 0: + name = "reset-gpios"; + break; + case 1: + name = "powerdown-gpios"; + break; + default: + dev_warn(data->dev, "Unknown GPIO type 0x%02lx for pin %d\n", + INTEL_DSM_TYPE(settings), pin); + return 1; + } + + /* + * Both reset and power-down need to be logical false when the sensor + * is on (sensor should not be in reset and not be powered-down). So + * when the sensor-on-value (which is the physical pin value) is high, + * then the signal is active-low. + */ + active_low = INTEL_DSM_SENSOR_ON_VAL(settings) ? true : false; + + i = data->map_count; + if (i == V4L2_SENSOR_MAX_ACPI_GPIOS) + return 1; + + /* res_count is already incremented */ + data->map->params[i].crs_entry_index = data->res_count - 1; + data->map->params[i].active_low = active_low; + data->map->mapping[i].name = name; + data->map->mapping[i].data = &data->map->params[i]; + data->map->mapping[i].size = 1; + data->map_count++; + + dev_info(data->dev, "%s crs %d %s pin %d active-%s\n", name, + data->res_count - 1, agpio->resource_source.string_ptr, + pin, active_low ? "low" : "high"); + + return 1; +} + +/* + * Helper function to create an ACPI GPIO lookup table for sensor reset and + * powerdown signals on Intel Bay Trail (BYT) and Cherry Trail (CHT) devices, + * including setting the correct polarity for the GPIO. + * + * This uses the "79234640-9e10-4fea-a5c1-b5aa8b19756f" DSM method directly + * on the sensor device's ACPI node. This is different from later Intel + * hardware which has a separate INT3472 with this info. Since there is + * no separate firmware-node to which we can bind to register the GPIO lookups + * this unfortunately means that all sensor drivers which may be used on + * BYT or CHT hw need to call this function. + * + * Note this code uses the same DSM GUID as the INT3472 discrete.c code + * and there is some overlap, but there are enough differences that it is + * difficult to share the code. + */ +void v4l2_acpi_parse_sensor_gpios(struct device *dev) +{ + struct acpi_device *adev = ACPI_COMPANION(dev); + struct v4l2_acpi_gpio_parsing_data data = { }; + LIST_HEAD(resource_list); + union acpi_object *obj; + unsigned int i, j; + int ret; + + if (!adev || (!soc_intel_is_byt() && !soc_intel_is_cht())) + return; + + /* + * First get the GPIO-settings count and then get count GPIO-settings + * values. Note the order of these may differ from the order in which + * the GPIOs are listed on the ACPI resources! So we first store them all + * and then enumerate the ACPI resources and match them up by pin number. + */ + obj = acpi_evaluate_dsm_typed(adev->handle, + &intel_sensor_gpio_info_guid, 0x00, 1, + NULL, ACPI_TYPE_INTEGER); + if (!obj) { + dev_warn(dev, "No _DSM entry for GPIO pin count\n"); + return; + } + + data.settings_count = obj->integer.value; + ACPI_FREE(obj); + + if (data.settings_count > V4L2_SENSOR_MAX_ACPI_GPIOS) { + dev_err(dev, "_DSM returns too many GPIOs %u > %u\n", + data.settings_count, V4L2_SENSOR_MAX_ACPI_GPIOS); + return; + } + + for (i = 0; i < data.settings_count; i++) { + /* + * i + 2 because the index of this _DSM function is 1-based + * and the first function is just a count. + */ + obj = acpi_evaluate_dsm_typed(adev->handle, + &intel_sensor_gpio_info_guid, + 0x00, i + 2, + NULL, ACPI_TYPE_INTEGER); + if (!obj) { + dev_err(dev, "No _DSM entry for GPIO pin %u\n", i); + return; + } + + data.settings[i] = obj->integer.value; + ACPI_FREE(obj); + } + + /* Since we match up by pin-number the pin-numbers must be unique */ + for (i = 0; i < data.settings_count; i++) { + for (j = i + 1; j < data.settings_count; j++) { + if (INTEL_DSM_PIN(data.settings[i]) != + INTEL_DSM_PIN(data.settings[j])) + continue; + + dev_err(dev, "Duplicate pin number %lu\n", + INTEL_DSM_PIN(data.settings[i])); + return; + } + } + + /* Use devm_kzalloc() for the mappings + params to auto-free them */ + data.map = devm_kzalloc(dev, sizeof(*data.map), GFP_KERNEL); + if (!data.map) + return; + + /* Now parse the ACPI resources and build the lookup table */ + data.dev = dev; + ret = acpi_dev_get_resources(adev, &resource_list, + v4l2_acpi_handle_gpio_res, &data); + if (ret < 0) + return; + + acpi_dev_free_resource_list(&resource_list); + + if (data.map_count != data.settings_count || + data.res_count != data.settings_count) + dev_warn(dev, "ACPI GPIO resources vs DSM GPIO-info count mismatch (dsm: %d res: %d map %d\n", + data.settings_count, data.res_count, data.map_count); + + devm_acpi_dev_add_driver_gpios(dev, data.map->mapping); +} +EXPORT_SYMBOL_GPL(v4l2_acpi_parse_sensor_gpios); diff --git a/include/media/v4l2-acpi.h b/include/media/v4l2-acpi.h new file mode 100644 index 000000000000..0729cbdd3749 --- /dev/null +++ b/include/media/v4l2-acpi.h @@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * V4L2 sensor driver ACPI helpers + * + * Copyright (C) 2023, Hans de Goede <hansg@kernel.org> + */ + +#ifndef V4L2_ACPI_H +#define V4L2_ACPI_H + +#ifdef CONFIG_ACPI + +struct device; + +/** + * v4l2_acpi_parse_sensor_gpios - Parse ACPI info describing sensor GPIOs. + * + * @dev: Device to parse the ACPI info for + * + * On x86/ACPI platforms the GPIO resources do not provide information + * about which resource maps to which connection-id. + * + * Sensor drivers can call this function to use platform specific methods + * (e.g. the Intel 79234640-9e10-4fea-a5c1-b5aa8b19756f _DSM) to get + * information about the pins and add GPIO mappings to make standard gpiod_get() + * calls work. + * + * The registered mappings use devm managed memory and are automatically free-ed + * on remove() and on probe() failure. + */ +void v4l2_acpi_parse_sensor_gpios(struct device *dev); + +#else /* ifdef CONFIG_ACPI */ +static inline void v4l2_acpi_parse_sensor_gpios(struct device *dev) { return 0; } +#endif /* ifdef CONFIG_ACPI */ + +#endif /* ifdef V4L2_ACPI_H */
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. Note v4l2_acpi_parse_sensor_gpios() is basically a copy of the atomisp2 v4l2_get_acpi_sensor_info() helper from: drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c minus logging the sensor module-name using a second _DSM. v4l2_get_acpi_sensor_info() was already reviewed by Andy, see the Link tag below. (this code duplication is removed in the next patch in this series) Link: https://patchwork.kernel.org/project/linux-media/patch/20230401145926.596216-2-hdegoede@redhat.com/ Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Documentation/driver-api/media/v4l2-acpi.rst | 5 + Documentation/driver-api/media/v4l2-core.rst | 1 + drivers/media/v4l2-core/Makefile | 1 + drivers/media/v4l2-core/v4l2-acpi.c | 227 +++++++++++++++++++ include/media/v4l2-acpi.h | 37 +++ 5 files changed, 271 insertions(+) create mode 100644 Documentation/driver-api/media/v4l2-acpi.rst create mode 100644 drivers/media/v4l2-core/v4l2-acpi.c create mode 100644 include/media/v4l2-acpi.h