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

  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
  

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!