| Message ID | 20230518153214.194976-2-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: v4l: Add v4l2_acpi_parse_sensor_gpios() helper + gc0310 sensor driver | expand |
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.
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 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