[PATCH 0/8] media: uvcvideo: Add support for V4L2_CID_CAMERA_SENSOR_ORIENTATION

[PATCH 0/8] media: uvcvideo: Add support for V4L2_CID_CAMERA_SENSOR_ORIENTATION

[Ricardo Ribalda]

The ACPI has ways to annotate the location of a USB device. Wire that
annotation to a v4l2 control.

To support all possible devices, add a way to annotate USB devices on DT
as well. The original binding discussion happened here:
https://lore.kernel.org/linux-devicetree/20241212-usb-orientation-v1-1-0b69adf05f37@chromium.org/

This set includes a couple of patches that are "under review" but
conflict.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (8):
      media: uvcvideo: Fix deferred probing error
      media: uvcvideo: Use dev_err_probe for devm_gpiod_get_optional
      media: v4l: fwnode: Support acpi devices for v4l2_fwnode_device_parse
      media: ipu-bridge: Use v4l2_fwnode_device_parse helper
      dt-bindings: usb: usb-device: Add orientation
      media: uvcvideo: Factor out gpio functions to its own file
      media: uvcvideo: Add support for V4L2_CID_CAMERA_ORIENTATION
      media: uvcvideo: Do not create MC entities for virtual entities

 .../devicetree/bindings/usb/usb-device.yaml        |   5 +
 drivers/media/pci/intel/ipu-bridge.c               |  32 +----
 drivers/media/usb/uvc/Makefile                     |   3 +-
 drivers/media/usb/uvc/uvc_ctrl.c                   |  21 +++
 drivers/media/usb/uvc/uvc_driver.c                 | 159 +++++----------------
 drivers/media/usb/uvc/uvc_entity.c                 |  11 ++
 drivers/media/usb/uvc/uvc_fwnode.c                 |  73 ++++++++++
 drivers/media/usb/uvc/uvc_gpio.c                   | 123 ++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h                   |  21 +++
 drivers/media/v4l2-core/v4l2-fwnode.c              |  58 +++++++-
 include/linux/usb/uvc.h                            |   3 +
 11 files changed, 349 insertions(+), 160 deletions(-)
---
base-commit: 4e82c87058f45e79eeaa4d5bcc3b38dd3dce7209
change-id: 20250403-uvc-orientation-5f7f19da5adb

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH 1/8] media: uvcvideo: Fix deferred probing error

[Ricardo Ribalda]

uvc_gpio_parse() can return -EPROBE_DEFER when the GPIOs it depends on
have not yet been probed. This return code should be propagated to the
caller of uvc_probe() to ensure that probing is retried when the required
GPIOs become available.

Currently, this error code is incorrectly converted to -ENODEV,
causing some internal cameras to be ignored.

This commit fixes this issue by propagating the -EPROBE_DEFER error.

Cc: stable@vger.kernel.org
Fixes: 2886477ff987 ("media: uvcvideo: Implement UVC_EXT_GPIO_UNIT")
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 107e0fafd80f54ec98c9657e5d58d17a6ed8c02f..25e9aea81196e0eddba6de74951a46a97ae0bdb8 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2232,13 +2232,16 @@ static int uvc_probe(struct usb_interface *intf,
 #endif
 
 	/* Parse the Video Class control descriptor. */
-	if (uvc_parse_control(dev) < 0) {
+	ret = uvc_parse_control(dev);
+	if (ret < 0) {
+		ret = -ENODEV;
 		uvc_dbg(dev, PROBE, "Unable to parse UVC descriptors\n");
 		goto error;
 	}
 
 	/* Parse the associated GPIOs. */
-	if (uvc_gpio_parse(dev) < 0) {
+	ret = uvc_gpio_parse(dev);
+	if (ret < 0) {
 		uvc_dbg(dev, PROBE, "Unable to parse UVC GPIOs\n");
 		goto error;
 	}
@@ -2264,24 +2267,32 @@ static int uvc_probe(struct usb_interface *intf,
 	}
 
 	/* Register the V4L2 device. */
-	if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
+	ret = v4l2_device_register(&intf->dev, &dev->vdev);
+	if (ret < 0)
 		goto error;
 
 	/* Scan the device for video chains. */
-	if (uvc_scan_device(dev) < 0)
+	if (uvc_scan_device(dev) < 0) {
+		ret = -ENODEV;
 		goto error;
+	}
 
 	/* Initialize controls. */
-	if (uvc_ctrl_init_device(dev) < 0)
+	if (uvc_ctrl_init_device(dev) < 0) {
+		ret = -ENODEV;
 		goto error;
+	}
 
 	/* Register video device nodes. */
-	if (uvc_register_chains(dev) < 0)
+	if (uvc_register_chains(dev) < 0) {
+		ret = -ENODEV;
 		goto error;
+	}
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 	/* Register the media device node */
-	if (media_device_register(&dev->mdev) < 0)
+	ret = media_device_register(&dev->mdev);
+	if (ret < 0)
 		goto error;
 #endif
 	/* Save our data pointer in the interface data. */
@@ -2315,7 +2326,7 @@ static int uvc_probe(struct usb_interface *intf,
 error:
 	uvc_unregister_video(dev);
 	kref_put(&dev->ref, uvc_delete);
-	return -ENODEV;
+	return ret;
 }
 
 static void uvc_disconnect(struct usb_interface *intf)

-- 
2.49.0.504.g3bcea36a83-goog

[PATCH 2/8] media: uvcvideo: Use dev_err_probe for devm_gpiod_get_optional

[Ricardo Ribalda]

Use the dev_err_probe() helper for devm_gpiod_get_optional(), like we do
with gpiod_to_irq()

That eventually calls device_set_deferred_probe_reason() which can be
helpful for tracking down problems.

Now that all the error paths in uvc_gpio_parse have dev_err_probe, we
can remove the error message in uvc_probe.

Suggested-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 25e9aea81196e0eddba6de74951a46a97ae0bdb8..da24a655ab68cc0957762f2b67387677c22224d1 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1299,8 +1299,13 @@ static int uvc_gpio_parse(struct uvc_device *dev)
 
 	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
 					       GPIOD_IN);
-	if (IS_ERR_OR_NULL(gpio_privacy))
-		return PTR_ERR_OR_ZERO(gpio_privacy);
+	if (!gpio_privacy)
+		return 0;
+
+	if (IS_ERR(gpio_privacy))
+		return dev_err_probe(&dev->intf->dev,
+				     PTR_ERR(gpio_privacy),
+				     "Can't get privacy GPIO\n");
 
 	irq = gpiod_to_irq(gpio_privacy);
 	if (irq < 0)
@@ -2241,10 +2246,8 @@ static int uvc_probe(struct usb_interface *intf,
 
 	/* Parse the associated GPIOs. */
 	ret = uvc_gpio_parse(dev);
-	if (ret < 0) {
-		uvc_dbg(dev, PROBE, "Unable to parse UVC GPIOs\n");
+	if (ret < 0)
 		goto error;
-	}
 
 	dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
 		 dev->uvc_version >> 8, dev->uvc_version & 0xff,

-- 
2.49.0.504.g3bcea36a83-goog

[PATCH 3/8] media: v4l: fwnode: Support acpi devices for v4l2_fwnode_device_parse

[Ricardo Ribalda]

This patch modifies v4l2_fwnode_device_parse() to support ACPI devices.

We initially add support only for orientation via the ACPI _PLD method.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 58 +++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..81563c36b6436bb61e1c96f2a5ede3fa9d64dab3 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -15,6 +15,7 @@
  * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
  */
 #include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/module.h>
@@ -807,16 +808,47 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link);
 
-int v4l2_fwnode_device_parse(struct device *dev,
-			     struct v4l2_fwnode_device_properties *props)
+static int v4l2_fwnode_device_parse_acpi(struct device *dev,
+					 struct v4l2_fwnode_device_properties *props)
+{
+	struct acpi_pld_info *pld;
+	int ret = 0;
+
+	if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) {
+		dev_dbg(dev, "acpi _PLD call failed\n");
+		return 0;
+	}
+
+	switch (pld->panel) {
+	case ACPI_PLD_PANEL_FRONT:
+		props->orientation = V4L2_FWNODE_ORIENTATION_FRONT;
+		break;
+	case ACPI_PLD_PANEL_BACK:
+		props->orientation = V4L2_FWNODE_ORIENTATION_BACK;
+		break;
+	case ACPI_PLD_PANEL_TOP:
+	case ACPI_PLD_PANEL_LEFT:
+	case ACPI_PLD_PANEL_RIGHT:
+	case ACPI_PLD_PANEL_UNKNOWN:
+		props->orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
+		break;
+	default:
+		dev_dbg(dev, "Unknown _PLD panel val %d\n", pld->panel);
+		ret = -EINVAL;
+		break;
+	}
+
+	ACPI_FREE(pld);
+	return ret;
+}
+
+static int v4l2_fwnode_device_parse_dt(struct device *dev,
+				       struct v4l2_fwnode_device_properties *props)
 {
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	u32 val;
 	int ret;
 
-	memset(props, 0, sizeof(*props));
-
-	props->orientation = V4L2_FWNODE_PROPERTY_UNSET;
 	ret = fwnode_property_read_u32(fwnode, "orientation", &val);
 	if (!ret) {
 		switch (val) {
@@ -833,7 +865,6 @@ int v4l2_fwnode_device_parse(struct device *dev,
 		dev_dbg(dev, "device orientation: %u\n", val);
 	}
 
-	props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
 	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
 	if (!ret) {
 		if (val >= 360) {
@@ -847,6 +878,21 @@ int v4l2_fwnode_device_parse(struct device *dev,
 
 	return 0;
 }
+
+int v4l2_fwnode_device_parse(struct device *dev,
+			     struct v4l2_fwnode_device_properties *props)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+
+	memset(props, 0, sizeof(*props));
+
+	props->orientation = V4L2_FWNODE_PROPERTY_UNSET;
+	props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
+
+	if (is_acpi_device_node(fwnode))
+		return v4l2_fwnode_device_parse_acpi(dev, props);
+	return v4l2_fwnode_device_parse_dt(dev, props);
+}
 EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse);
 
 /*

-- 
2.49.0.504.g3bcea36a83-goog

[PATCH 4/8] media: ipu-bridge: Use v4l2_fwnode_device_parse helper

[Ricardo Ribalda]

v4l2_fwnode_device_parse now supports acpi devices as well. Use the
helper instead of re-implement the logic.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/pci/intel/ipu-bridge.c | 32 ++++++--------------------------
 1 file changed, 6 insertions(+), 26 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 1cb7458556004d2073688bb0d11defd01f2f61b7..79e6998d37dfde50bd4b44584c8864178527d951 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -251,36 +251,16 @@ static u32 ipu_bridge_parse_rotation(struct acpi_device *adev,
 
 static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_device *adev)
 {
-	enum v4l2_fwnode_orientation orientation;
-	struct acpi_pld_info *pld = NULL;
+	struct v4l2_fwnode_device_properties props;
+	int ret;
 
-	if (!acpi_get_physical_device_location(ACPI_PTR(adev->handle), &pld)) {
-		dev_warn(ADEV_DEV(adev), "_PLD call failed, using default orientation\n");
+	ret = v4l2_fwnode_device_parse(ADEV_DEV(adev), &props);
+	if (!ret || props.rotation == V4L2_FWNODE_PROPERTY_UNSET) {
+		dev_warn(ADEV_DEV(adev), "Using default orientation\n");
 		return V4L2_FWNODE_ORIENTATION_EXTERNAL;
 	}
 
-	switch (pld->panel) {
-	case ACPI_PLD_PANEL_FRONT:
-		orientation = V4L2_FWNODE_ORIENTATION_FRONT;
-		break;
-	case ACPI_PLD_PANEL_BACK:
-		orientation = V4L2_FWNODE_ORIENTATION_BACK;
-		break;
-	case ACPI_PLD_PANEL_TOP:
-	case ACPI_PLD_PANEL_LEFT:
-	case ACPI_PLD_PANEL_RIGHT:
-	case ACPI_PLD_PANEL_UNKNOWN:
-		orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
-		break;
-	default:
-		dev_warn(ADEV_DEV(adev), "Unknown _PLD panel val %d\n",
-			 pld->panel);
-		orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
-		break;
-	}
-
-	ACPI_FREE(pld);
-	return orientation;
+	return props.rotation;
 }
 
 int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)

-- 
2.49.0.504.g3bcea36a83-goog

[PATCH 5/8] dt-bindings: usb: usb-device: Add orientation

[Ricardo Ribalda]

For some devices, such as cameras, the OS needs to know where they are
mounted.

ACPI has a property for this purpose, which is parsed by
acpi_get_physical_device_location():
https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#pld-physical-location-of-device

In DT we have similar property for video-interface-devices called
orientation:
Documentation/devicetree/bindings/media/video-interface-devices.yaml

Add a new property orientation for usb-devices that matches the already
existing orientation property of video-interface-devices.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 Documentation/devicetree/bindings/usb/usb-device.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-device.yaml b/Documentation/devicetree/bindings/usb/usb-device.yaml
index da890ee60ce6e71a11910c565b6f805470782e4f..bbcda28ec7d5695307efa797f57180044afda77f 100644
--- a/Documentation/devicetree/bindings/usb/usb-device.yaml
+++ b/Documentation/devicetree/bindings/usb/usb-device.yaml
@@ -42,6 +42,10 @@ properties:
       port to which this device is attached. The range is 1-255.
     maxItems: 1
 
+  orientation:
+    description: If present, specifies the orientation of the usb device.
+    $ref: /schemas/media/video-interface-devices.yaml#/properties/orientation
+
   "#address-cells":
     description: should be 1 for hub nodes with device nodes,
       should be 2 for device nodes with interface nodes.
@@ -101,6 +105,7 @@ examples:
         device@2 {
             compatible = "usb123,4567";
             reg = <2>;
+            orientation = <0>;
         };
 
         device@3 {

-- 
2.49.0.504.g3bcea36a83-goog

[PATCH 6/8] media: uvcvideo: Factor out gpio functions to its own file

[Ricardo Ribalda]

This is just a refactor patch, no new functionality is added.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/Makefile     |   3 +-
 drivers/media/usb/uvc/uvc_driver.c | 121 +-----------------------------------
 drivers/media/usb/uvc/uvc_gpio.c   | 123 +++++++++++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |   6 ++
 4 files changed, 133 insertions(+), 120 deletions(-)

diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
index 4f9eee4f81ab6436a8b90324a688a149b2c3bcd1..85514b6e538fbb8284e574ca14700f2d749e1a2e 100644
--- a/drivers/media/usb/uvc/Makefile
+++ b/drivers/media/usb/uvc/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
-		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
+		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
+		  uvc_gpio.o
 ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
 uvcvideo-objs  += uvc_entity.o
 endif
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index da24a655ab68cc0957762f2b67387677c22224d1..b52e1ff401e24f69b867b5e975cda4260463e760 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -8,7 +8,6 @@
 
 #include <linux/atomic.h>
 #include <linux/bits.h>
-#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
@@ -792,8 +791,8 @@ static const u8 uvc_media_transport_input_guid[16] =
 	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
 static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
 
-static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
-		unsigned int num_pads, unsigned int extra_size)
+struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
+				    unsigned int extra_size)
 {
 	struct uvc_entity *entity;
 	unsigned int num_inputs;
@@ -1242,122 +1241,6 @@ static int uvc_parse_control(struct uvc_device *dev)
 	return 0;
 }
 
-/* -----------------------------------------------------------------------------
- * Privacy GPIO
- */
-
-static void uvc_gpio_event(struct uvc_device *dev)
-{
-	struct uvc_entity *unit = dev->gpio_unit;
-	struct uvc_video_chain *chain;
-	u8 new_val;
-
-	if (!unit)
-		return;
-
-	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
-
-	/* GPIO entities are always on the first chain. */
-	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
-	uvc_ctrl_status_event(chain, unit->controls, &new_val);
-}
-
-static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
-			    u8 cs, void *data, u16 size)
-{
-	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
-		return -EINVAL;
-
-	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
-
-	return 0;
-}
-
-static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
-			     u8 cs, u8 *caps)
-{
-	if (cs != UVC_CT_PRIVACY_CONTROL)
-		return -EINVAL;
-
-	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
-	return 0;
-}
-
-static irqreturn_t uvc_gpio_irq(int irq, void *data)
-{
-	struct uvc_device *dev = data;
-
-	uvc_gpio_event(dev);
-	return IRQ_HANDLED;
-}
-
-static int uvc_gpio_parse(struct uvc_device *dev)
-{
-	struct uvc_entity *unit;
-	struct gpio_desc *gpio_privacy;
-	int irq;
-
-	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
-					       GPIOD_IN);
-	if (!gpio_privacy)
-		return 0;
-
-	if (IS_ERR(gpio_privacy))
-		return dev_err_probe(&dev->intf->dev,
-				     PTR_ERR(gpio_privacy),
-				     "Can't get privacy GPIO\n");
-
-	irq = gpiod_to_irq(gpio_privacy);
-	if (irq < 0)
-		return dev_err_probe(&dev->intf->dev, irq,
-				     "No IRQ for privacy GPIO\n");
-
-	unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
-	if (!unit)
-		return -ENOMEM;
-
-	unit->gpio.gpio_privacy = gpio_privacy;
-	unit->gpio.irq = irq;
-	unit->gpio.bControlSize = 1;
-	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
-	unit->gpio.bmControls[0] = 1;
-	unit->get_cur = uvc_gpio_get_cur;
-	unit->get_info = uvc_gpio_get_info;
-	strscpy(unit->name, "GPIO", sizeof(unit->name));
-
-	list_add_tail(&unit->list, &dev->entities);
-
-	dev->gpio_unit = unit;
-
-	return 0;
-}
-
-static int uvc_gpio_init_irq(struct uvc_device *dev)
-{
-	struct uvc_entity *unit = dev->gpio_unit;
-	int ret;
-
-	if (!unit || unit->gpio.irq < 0)
-		return 0;
-
-	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
-				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
-				   IRQF_TRIGGER_RISING,
-				   "uvc_privacy_gpio", dev);
-
-	unit->gpio.initialized = !ret;
-
-	return ret;
-}
-
-static void uvc_gpio_deinit(struct uvc_device *dev)
-{
-	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
-		return;
-
-	free_irq(dev->gpio_unit->gpio.irq, dev);
-}
-
 /* ------------------------------------------------------------------------
  * UVC device scan
  */
diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
new file mode 100644
index 0000000000000000000000000000000000000000..30e3e6dd22cbc9cfee420dde7f7f64dbdce499b9
--- /dev/null
+++ b/drivers/media/usb/uvc/uvc_gpio.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *      uvc_gpio.c  --  USB Video Class driver
+ *
+ *      Copyright 2025 Google LLC
+ */
+
+#include <linux/kernel.h>
+#include <linux/gpio/consumer.h>
+#include "uvcvideo.h"
+
+static void uvc_gpio_event(struct uvc_device *dev)
+{
+	struct uvc_entity *unit = dev->gpio_unit;
+	struct uvc_video_chain *chain;
+	u8 new_val;
+
+	if (!unit)
+		return;
+
+	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
+
+	/* GPIO entities are always on the first chain. */
+	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
+	uvc_ctrl_status_event(chain, unit->controls, &new_val);
+}
+
+static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
+			    u8 cs, void *data, u16 size)
+{
+	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
+		return -EINVAL;
+
+	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
+
+	return 0;
+}
+
+static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
+			     u8 cs, u8 *caps)
+{
+	if (cs != UVC_CT_PRIVACY_CONTROL)
+		return -EINVAL;
+
+	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
+	return 0;
+}
+
+static irqreturn_t uvc_gpio_irq(int irq, void *data)
+{
+	struct uvc_device *dev = data;
+
+	uvc_gpio_event(dev);
+	return IRQ_HANDLED;
+}
+
+int uvc_gpio_parse(struct uvc_device *dev)
+{
+	struct uvc_entity *unit;
+	struct gpio_desc *gpio_privacy;
+	int irq;
+
+	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
+					       GPIOD_IN);
+	if (!gpio_privacy)
+		return 0;
+
+	if (IS_ERR(gpio_privacy))
+		return dev_err_probe(&dev->intf->dev,
+				     PTR_ERR(gpio_privacy),
+				     "Can't get privacy GPIO\n");
+
+	irq = gpiod_to_irq(gpio_privacy);
+	if (irq < 0)
+		return dev_err_probe(&dev->intf->dev, irq,
+				     "No IRQ for privacy GPIO\n");
+
+	unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
+	if (!unit)
+		return -ENOMEM;
+
+	unit->gpio.gpio_privacy = gpio_privacy;
+	unit->gpio.irq = irq;
+	unit->gpio.bControlSize = 1;
+	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
+	unit->gpio.bmControls[0] = 1;
+	unit->get_cur = uvc_gpio_get_cur;
+	unit->get_info = uvc_gpio_get_info;
+	strscpy(unit->name, "GPIO", sizeof(unit->name));
+
+	list_add_tail(&unit->list, &dev->entities);
+
+	dev->gpio_unit = unit;
+
+	return 0;
+}
+
+int uvc_gpio_init_irq(struct uvc_device *dev)
+{
+	struct uvc_entity *unit = dev->gpio_unit;
+	int ret;
+
+	if (!unit || unit->gpio.irq < 0)
+		return 0;
+
+	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
+				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
+				   IRQF_TRIGGER_RISING,
+				   "uvc_privacy_gpio", dev);
+
+	unit->gpio.initialized = !ret;
+
+	return ret;
+}
+
+void uvc_gpio_deinit(struct uvc_device *dev)
+{
+	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
+		return;
+
+	free_irq(dev->gpio_unit->gpio.irq, dev);
+}
+
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..aef96b96499ce09ffa286c51793482afd9832097 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -683,6 +683,8 @@ do {									\
  */
 
 struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
+struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
+				    unsigned int extra_size);
 
 /* Video buffers queue management. */
 int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type);
@@ -829,4 +831,8 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
 size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
 			    size_t size);
 
+/* gpio */
+int uvc_gpio_parse(struct uvc_device *dev);
+int uvc_gpio_init_irq(struct uvc_device *dev);
+void uvc_gpio_deinit(struct uvc_device *dev);
 #endif

-- 
2.49.0.504.g3bcea36a83-goog

[PATCH 7/8] media: uvcvideo: Add support for V4L2_CID_CAMERA_ORIENTATION

[Ricardo Ribalda]

Fetch the orientation from the fwnode and map it into a control.

The uvc driver does not use the media controller, so we need to create a
virtual entity, like we previously did with the external gpio.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/Makefile     |  2 +-
 drivers/media/usb/uvc/uvc_ctrl.c   | 21 +++++++++++
 drivers/media/usb/uvc/uvc_driver.c | 14 ++++++--
 drivers/media/usb/uvc/uvc_entity.c |  1 +
 drivers/media/usb/uvc/uvc_fwnode.c | 73 ++++++++++++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   | 15 ++++++++
 include/linux/usb/uvc.h            |  3 ++
 7 files changed, 125 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
index 85514b6e538fbb8284e574ca14700f2d749e1a2e..2a5b76aab756c21011d966f3ce0410ff6b8e5b19 100644
--- a/drivers/media/usb/uvc/Makefile
+++ b/drivers/media/usb/uvc/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
 		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
-		  uvc_gpio.o
+		  uvc_gpio.o uvc_fwnode.o
 ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
 uvcvideo-objs  += uvc_entity.o
 endif
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index cbf19aa1d82374a08cf79b6a6787fa348b83523a..df84bf292dcab6b833fbd3c70eccbe9e567ee567 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -376,6 +376,13 @@ static const struct uvc_control_info uvc_ctrls[] = {
 				| UVC_CTRL_FLAG_GET_DEF
 				| UVC_CTRL_FLAG_AUTO_UPDATE,
 	},
+	{
+		.entity		= UVC_GUID_FWNODE,
+		.selector	= 0,
+		.index		= 0,
+		.size		= 1,
+		.flags		= UVC_CTRL_FLAG_GET_CUR,
+	},
 };
 
 static const u32 uvc_control_classes[] = {
@@ -975,6 +982,17 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 		.data_type	= UVC_CTRL_DATA_TYPE_BITMASK,
 		.name		= "Region of Interest Auto Ctrls",
 	},
+	{
+		.id		= V4L2_CID_CAMERA_ORIENTATION,
+		.entity		= UVC_GUID_FWNODE,
+		.selector	= 0,
+		.size		= 8,
+		.offset		= 0,
+		.v4l2_type	= V4L2_CTRL_TYPE_MENU,
+		.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
+		.menu_mask	= GENMASK(V4L2_CAMERA_ORIENTATION_EXTERNAL,
+					  V4L2_CAMERA_ORIENTATION_FRONT),
+	},
 };
 
 /* ------------------------------------------------------------------------
@@ -3170,6 +3188,9 @@ static int uvc_ctrl_init_chain(struct uvc_video_chain *chain)
 		} else if (UVC_ENTITY_TYPE(entity) == UVC_EXT_GPIO_UNIT) {
 			bmControls = entity->gpio.bmControls;
 			bControlSize = entity->gpio.bControlSize;
+		} else if (UVC_ENTITY_TYPE(entity) == UVC_FWNODE_UNIT) {
+			bmControls = entity->fwnode.bmControls;
+			bControlSize = entity->fwnode.bControlSize;
 		}
 
 		/* Remove bogus/blacklisted controls */
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index b52e1ff401e24f69b867b5e975cda4260463e760..9a8e5d94b3eba09e1ee1be20bad5b016b6def915 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1752,11 +1752,15 @@ static int uvc_scan_device(struct uvc_device *dev)
 		return -1;
 	}
 
-	/* Add GPIO entity to the first chain. */
-	if (dev->gpio_unit) {
+	/* Add virtual entities to the first chain. */
+	if (dev->gpio_unit || dev->fwnode_unit) {
 		chain = list_first_entry(&dev->chains,
 					 struct uvc_video_chain, list);
-		list_add_tail(&dev->gpio_unit->chain, &chain->entities);
+		if (dev->gpio_unit)
+			list_add_tail(&dev->gpio_unit->chain, &chain->entities);
+		if (dev->fwnode_unit)
+			list_add_tail(&dev->fwnode_unit->chain,
+				      &chain->entities);
 	}
 
 	return 0;
@@ -2132,6 +2136,10 @@ static int uvc_probe(struct usb_interface *intf,
 	if (ret < 0)
 		goto error;
 
+	ret = uvc_fwnode_parse(dev);
+	if (ret < 0)
+		goto error;
+
 	dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
 		 dev->uvc_version >> 8, dev->uvc_version & 0xff,
 		 udev->product ? udev->product : "<unnamed>",
diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
index cc68dd24eb42dce5b2846ca52a8dfa499c8aed96..01eeba2f049e9ebb25e091340a133656acbf28ca 100644
--- a/drivers/media/usb/uvc/uvc_entity.c
+++ b/drivers/media/usb/uvc/uvc_entity.c
@@ -106,6 +106,7 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain,
 		case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:
 		case UVC_EXTERNAL_VENDOR_SPECIFIC:
 		case UVC_EXT_GPIO_UNIT:
+		case UVC_FWNODE_UNIT:
 		default:
 			function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
 			break;
diff --git a/drivers/media/usb/uvc/uvc_fwnode.c b/drivers/media/usb/uvc/uvc_fwnode.c
new file mode 100644
index 0000000000000000000000000000000000000000..20f7b8847551acfd44cbf09bcbf653d89985561f
--- /dev/null
+++ b/drivers/media/usb/uvc/uvc_fwnode.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *      uvc_fwnode.c  --  USB Video Class driver
+ *
+ *      Copyright 2025 Google LLC
+ */
+
+#include <linux/kernel.h>
+#include <linux/usb/uvc.h>
+#include <media/v4l2-fwnode.h>
+#include "uvcvideo.h"
+
+static int uvc_fwnode_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
+			      u8 cs, void *data, u16 size)
+{
+	if (size < 1)
+		return -EINVAL;
+
+	switch (entity->fwnode.props.orientation) {
+	case V4L2_FWNODE_ORIENTATION_FRONT:
+		*(u8 *)data = V4L2_CAMERA_ORIENTATION_FRONT;
+		break;
+	case V4L2_FWNODE_ORIENTATION_BACK:
+		*(u8 *)data = V4L2_CAMERA_ORIENTATION_BACK;
+		break;
+	default:
+		*(u8 *)data = V4L2_CAMERA_ORIENTATION_EXTERNAL;
+	}
+
+	return 0;
+}
+
+static int uvc_fwnode_get_info(struct uvc_device *dev,
+			       struct uvc_entity *entity, u8 cs, u8 *caps)
+{
+	*caps = UVC_CONTROL_CAP_GET;
+	return 0;
+}
+
+int uvc_fwnode_parse(struct uvc_device *dev)
+{
+	static const u8 uvc_fwnode_guid[] = UVC_GUID_FWNODE;
+	struct v4l2_fwnode_device_properties props;
+	struct uvc_entity *unit;
+	int ret;
+
+	ret = v4l2_fwnode_device_parse(&dev->udev->dev, &props);
+	if (ret)
+		return dev_err_probe(&dev->intf->dev, ret,
+				     "Can't parse fwnode\n");
+
+	if (props.orientation == V4L2_FWNODE_PROPERTY_UNSET)
+		return 0;
+
+	unit = uvc_alloc_entity(UVC_FWNODE_UNIT, UVC_FWNODE_UNIT_ID, 0, 1);
+	if (!unit)
+		return -ENOMEM;
+
+	memcpy(unit->guid, uvc_fwnode_guid, sizeof(unit->guid));
+	unit->fwnode.props = props;
+	unit->fwnode.bControlSize = 1;
+	unit->fwnode.bmControls = (u8 *)unit + sizeof(*unit);
+	unit->fwnode.bmControls[0] = 1;
+	unit->get_cur = uvc_fwnode_get_cur;
+	unit->get_info = uvc_fwnode_get_info;
+	strscpy(unit->name, "FWNODE", sizeof(unit->name));
+
+	list_add_tail(&unit->list, &dev->entities);
+
+	dev->fwnode_unit = unit;
+
+	return 0;
+}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index aef96b96499ce09ffa286c51793482afd9832097..c52ab99bf8d21ab37270d27ffc040fd115b3f5ba 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -19,6 +19,7 @@
 #include <media/v4l2-event.h>
 #include <media/v4l2-fh.h>
 #include <media/videobuf2-v4l2.h>
+#include <media/v4l2-fwnode.h>
 
 /* --------------------------------------------------------------------------
  * UVC constants
@@ -41,6 +42,9 @@
 #define UVC_EXT_GPIO_UNIT		0x7ffe
 #define UVC_EXT_GPIO_UNIT_ID		0x100
 
+#define UVC_FWNODE_UNIT			0x7ffd
+#define UVC_FWNODE_UNIT_ID		0x101
+
 /* ------------------------------------------------------------------------
  * Driver specific constants.
  */
@@ -242,6 +246,12 @@ struct uvc_entity {
 			int irq;
 			bool initialized;
 		} gpio;
+
+		struct {
+			u8  bControlSize;
+			u8  *bmControls;
+			struct v4l2_fwnode_device_properties props;
+		} fwnode;
 	};
 
 	u8 bNrInPins;
@@ -617,6 +627,7 @@ struct uvc_device {
 	} async_ctrl;
 
 	struct uvc_entity *gpio_unit;
+	struct uvc_entity *fwnode_unit;
 };
 
 enum uvc_handle_state {
@@ -835,4 +846,8 @@ size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
 int uvc_gpio_parse(struct uvc_device *dev);
 int uvc_gpio_init_irq(struct uvc_device *dev);
 void uvc_gpio_deinit(struct uvc_device *dev);
+
+/* fwnode */
+int uvc_fwnode_parse(struct uvc_device *dev);
+
 #endif
diff --git a/include/linux/usb/uvc.h b/include/linux/usb/uvc.h
index bce95153e5a65613a710d7316fc17cf5462b5bce..d818839b0442988d94ab44935e1ce855b0adf4a1 100644
--- a/include/linux/usb/uvc.h
+++ b/include/linux/usb/uvc.h
@@ -29,6 +29,9 @@
 #define UVC_GUID_EXT_GPIO_CONTROLLER \
 	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
 	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x03}
+#define UVC_GUID_FWNODE \
+	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
+	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x04}
 
 #define UVC_GUID_FORMAT_MJPEG \
 	{ 'M',  'J',  'P',  'G', 0x00, 0x00, 0x10, 0x00, \

-- 
2.49.0.504.g3bcea36a83-goog

[PATCH 8/8] media: uvcvideo: Do not create MC entities for virtual entities

[Ricardo Ribalda]

Neither the GPIO nor the FWNODE entities form part of the device
pipeline. We just create them to hold emulated uvc controls.

When the device initializes, a warning is thrown by the v4l2 core:
uvcvideo 1-1:1.0: Entity type for entity FWNODE was not initialized!

There are no entity function that matches what we are doing here, and
it does not make to much sense to create a function for entities that
do not really exist.

Do not create MC entities for them and pretend nothing happened here.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_entity.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
index 01eeba2f049e9ebb25e091340a133656acbf28ca..4a0a083b4f58f041023710207a73e8fede8526e0 100644
--- a/drivers/media/usb/uvc/uvc_entity.c
+++ b/drivers/media/usb/uvc/uvc_entity.c
@@ -72,6 +72,16 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain,
 {
 	int ret;
 
+	/*
+	 * Do not initialize virtual entities, they do not really exist
+	 * and are not connected to any other entities.
+	 */
+	switch (UVC_ENTITY_TYPE(entity)) {
+	case UVC_EXT_GPIO_UNIT:
+	case UVC_FWNODE_UNIT:
+		return 0;
+	}
+
 	if (UVC_ENTITY_TYPE(entity) != UVC_TT_STREAMING) {
 		u32 function;
 

-- 
2.49.0.504.g3bcea36a83-goog

Re: [PATCH 5/8] dt-bindings: usb: usb-device: Add orientation

[Rob Herring]

On Thu, Apr 03, 2025 at 07:16:16PM +0000, Ricardo Ribalda wrote:
> For some devices, such as cameras, the OS needs to know where they are
> mounted.

Do you have a usecase that's not a camera?

> 
> ACPI has a property for this purpose, which is parsed by
> acpi_get_physical_device_location():
> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#pld-physical-location-of-device
> 
> In DT we have similar property for video-interface-devices called
> orientation:
> Documentation/devicetree/bindings/media/video-interface-devices.yaml
> 
> Add a new property orientation for usb-devices that matches the already
> existing orientation property of video-interface-devices.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  Documentation/devicetree/bindings/usb/usb-device.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-device.yaml b/Documentation/devicetree/bindings/usb/usb-device.yaml
> index da890ee60ce6e71a11910c565b6f805470782e4f..bbcda28ec7d5695307efa797f57180044afda77f 100644
> --- a/Documentation/devicetree/bindings/usb/usb-device.yaml
> +++ b/Documentation/devicetree/bindings/usb/usb-device.yaml

This is a binding for *all* USB devices. This property should only be 
added for devices where it makes sense.

> @@ -42,6 +42,10 @@ properties:
>        port to which this device is attached. The range is 1-255.
>      maxItems: 1
>  
> +  orientation:
> +    description: If present, specifies the orientation of the usb device.
> +    $ref: /schemas/media/video-interface-devices.yaml#/properties/orientation

Reference the schema from the top level and drop 
'/properties/orientation'.

What about 'rotation'? Seems like you'd want that too.

> +
>    "#address-cells":
>      description: should be 1 for hub nodes with device nodes,
>        should be 2 for device nodes with interface nodes.
> @@ -101,6 +105,7 @@ examples:
>          device@2 {
>              compatible = "usb123,4567";
>              reg = <2>;
> +            orientation = <0>;
>          };
>  
>          device@3 {
> 
> -- 
> 2.49.0.504.g3bcea36a83-goog
> 

Re: [PATCH 5/8] dt-bindings: usb: usb-device: Add orientation

[Ricardo Ribalda]

Hi Rob

On Fri, 4 Apr 2025 at 21:36, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Apr 03, 2025 at 07:16:16PM +0000, Ricardo Ribalda wrote:
> > For some devices, such as cameras, the OS needs to know where they are
> > mounted.
>
> Do you have a usecase that's not a camera?

I personally do not have other use cases, but I suspect that it could
be useful for more people.

The original proposal was more generic and "inspired" in _PLD:
https://lore.kernel.org/linux-devicetree/20241212-usb-orientation-v1-1-0b69adf05f37@chromium.org/

You suggested using the camera's orientation.


>
> >
> > ACPI has a property for this purpose, which is parsed by
> > acpi_get_physical_device_location():
> > https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#pld-physical-location-of-device
> >
> > In DT we have similar property for video-interface-devices called
> > orientation:
> > Documentation/devicetree/bindings/media/video-interface-devices.yaml
> >
> > Add a new property orientation for usb-devices that matches the already
> > existing orientation property of video-interface-devices.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  Documentation/devicetree/bindings/usb/usb-device.yaml | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/usb-device.yaml b/Documentation/devicetree/bindings/usb/usb-device.yaml
> > index da890ee60ce6e71a11910c565b6f805470782e4f..bbcda28ec7d5695307efa797f57180044afda77f 100644
> > --- a/Documentation/devicetree/bindings/usb/usb-device.yaml
> > +++ b/Documentation/devicetree/bindings/usb/usb-device.yaml
>
> This is a binding for *all* USB devices. This property should only be
> added for devices where it makes sense.

Can you provide some examples of how I can do this?


>
> > @@ -42,6 +42,10 @@ properties:
> >        port to which this device is attached. The range is 1-255.
> >      maxItems: 1
> >
> > +  orientation:
> > +    description: If present, specifies the orientation of the usb device.
> > +    $ref: /schemas/media/video-interface-devices.yaml#/properties/orientation
>
> Reference the schema from the top level and drop
> '/properties/orientation'.
>
> What about 'rotation'? Seems like you'd want that too.

At this moment I do not have a usecase for that. But sure, once I need
it I will add it the same way.

In the last thread I proposed Sakari to use:

+      image-sensor:
+        description: Video interface properties associated to USB cameras,
+          typically UVC compliant.
+        allOf:
+          - $ref: /schemas/media/video-interface-devices.yaml#
+

But he preferred to add orientation instead. Either ways work for me.


>
> > +
> >    "#address-cells":
> >      description: should be 1 for hub nodes with device nodes,
> >        should be 2 for device nodes with interface nodes.
> > @@ -101,6 +105,7 @@ examples:
> >          device@2 {
> >              compatible = "usb123,4567";
> >              reg = <2>;
> > +            orientation = <0>;
> >          };
> >
> >          device@3 {
> >
> > --
> > 2.49.0.504.g3bcea36a83-goog
> >



-- 
Ricardo Ribalda

Re: [PATCH 3/8] media: v4l: fwnode: Support acpi devices for v4l2_fwnode_device_parse

[Sakari Ailus]

Hi Ricardo,

Thanks for the patch.

On Thu, Apr 03, 2025 at 07:16:14PM +0000, Ricardo Ribalda wrote:
> This patch modifies v4l2_fwnode_device_parse() to support ACPI devices.
> 
> We initially add support only for orientation via the ACPI _PLD method.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 58 +++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..81563c36b6436bb61e1c96f2a5ede3fa9d64dab3 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -15,6 +15,7 @@
>   * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>   */
>  #include <linux/acpi.h>
> +#include <acpi/acpi_bus.h>
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
> @@ -807,16 +808,47 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link);
>  
> -int v4l2_fwnode_device_parse(struct device *dev,
> -			     struct v4l2_fwnode_device_properties *props)
> +static int v4l2_fwnode_device_parse_acpi(struct device *dev,
> +					 struct v4l2_fwnode_device_properties *props)
> +{
> +	struct acpi_pld_info *pld;
> +	int ret = 0;
> +
> +	if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) {
> +		dev_dbg(dev, "acpi _PLD call failed\n");
> +		return 0;
> +	}

You could have software nodes in an ACPI system as well as DT-aligned
properties. They're not the primary means to convey this information still.

How about returning e.g. -ENODATA here if _PLD doesn't exist for the device
and then proceeding to parse properties as in DT?

> +
> +	switch (pld->panel) {
> +	case ACPI_PLD_PANEL_FRONT:
> +		props->orientation = V4L2_FWNODE_ORIENTATION_FRONT;
> +		break;
> +	case ACPI_PLD_PANEL_BACK:
> +		props->orientation = V4L2_FWNODE_ORIENTATION_BACK;
> +		break;
> +	case ACPI_PLD_PANEL_TOP:
> +	case ACPI_PLD_PANEL_LEFT:
> +	case ACPI_PLD_PANEL_RIGHT:
> +	case ACPI_PLD_PANEL_UNKNOWN:
> +		props->orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
> +		break;

How about the rotation in _PLD?

> +	default:
> +		dev_dbg(dev, "Unknown _PLD panel val %d\n", pld->panel);
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	ACPI_FREE(pld);
> +	return ret;
> +}
> +
> +static int v4l2_fwnode_device_parse_dt(struct device *dev,
> +				       struct v4l2_fwnode_device_properties *props)
>  {
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	u32 val;
>  	int ret;
>  
> -	memset(props, 0, sizeof(*props));
> -
> -	props->orientation = V4L2_FWNODE_PROPERTY_UNSET;
>  	ret = fwnode_property_read_u32(fwnode, "orientation", &val);
>  	if (!ret) {
>  		switch (val) {
> @@ -833,7 +865,6 @@ int v4l2_fwnode_device_parse(struct device *dev,
>  		dev_dbg(dev, "device orientation: %u\n", val);
>  	}
>  
> -	props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
>  	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
>  	if (!ret) {
>  		if (val >= 360) {
> @@ -847,6 +878,21 @@ int v4l2_fwnode_device_parse(struct device *dev,
>  
>  	return 0;
>  }
> +
> +int v4l2_fwnode_device_parse(struct device *dev,
> +			     struct v4l2_fwnode_device_properties *props)
> +{
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +
> +	memset(props, 0, sizeof(*props));
> +
> +	props->orientation = V4L2_FWNODE_PROPERTY_UNSET;
> +	props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
> +
> +	if (is_acpi_device_node(fwnode))
> +		return v4l2_fwnode_device_parse_acpi(dev, props);
> +	return v4l2_fwnode_device_parse_dt(dev, props);
> +}
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse);
>  
>  /*
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 3/8] media: v4l: fwnode: Support acpi devices for v4l2_fwnode_device_parse

[Ricardo Ribalda]

Hi Sakari

On Sun, 13 Apr 2025 at 17:50, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Ricardo,
>
> Thanks for the patch.
>
> On Thu, Apr 03, 2025 at 07:16:14PM +0000, Ricardo Ribalda wrote:
> > This patch modifies v4l2_fwnode_device_parse() to support ACPI devices.
> >
> > We initially add support only for orientation via the ACPI _PLD method.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 58 +++++++++++++++++++++++++++++++----
> >  1 file changed, 52 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..81563c36b6436bb61e1c96f2a5ede3fa9d64dab3 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -15,6 +15,7 @@
> >   * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >   */
> >  #include <linux/acpi.h>
> > +#include <acpi/acpi_bus.h>
> >  #include <linux/kernel.h>
> >  #include <linux/mm.h>
> >  #include <linux/module.h>
> > @@ -807,16 +808,47 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode,
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link);
> >
> > -int v4l2_fwnode_device_parse(struct device *dev,
> > -                          struct v4l2_fwnode_device_properties *props)
> > +static int v4l2_fwnode_device_parse_acpi(struct device *dev,
> > +                                      struct v4l2_fwnode_device_properties *props)
> > +{
> > +     struct acpi_pld_info *pld;
> > +     int ret = 0;
> > +
> > +     if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) {
> > +             dev_dbg(dev, "acpi _PLD call failed\n");
> > +             return 0;
> > +     }
>
> You could have software nodes in an ACPI system as well as DT-aligned
> properties. They're not the primary means to convey this information still.
>
> How about returning e.g. -ENODATA here if _PLD doesn't exist for the device
> and then proceeding to parse properties as in DT?

Do you mean that there can be devices with ACPI handles that can also
have DT properties?

Wow that is new to me :).

What shall we do if _PLD contradicts the DT property? What takes precedence?

>
> > +
> > +     switch (pld->panel) {
> > +     case ACPI_PLD_PANEL_FRONT:
> > +             props->orientation = V4L2_FWNODE_ORIENTATION_FRONT;
> > +             break;
> > +     case ACPI_PLD_PANEL_BACK:
> > +             props->orientation = V4L2_FWNODE_ORIENTATION_BACK;
> > +             break;
> > +     case ACPI_PLD_PANEL_TOP:
> > +     case ACPI_PLD_PANEL_LEFT:
> > +     case ACPI_PLD_PANEL_RIGHT:
> > +     case ACPI_PLD_PANEL_UNKNOWN:
> > +             props->orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
> > +             break;
>
> How about the rotation in _PLD?

If we agree on the orientation part I will extend it to support
rotation. It should not be a complicated change.

>
> > +     default:
> > +             dev_dbg(dev, "Unknown _PLD panel val %d\n", pld->panel);
> > +             ret = -EINVAL;
> > +             break;
> > +     }
> > +
> > +     ACPI_FREE(pld);
> > +     return ret;
> > +}
> > +
> > +static int v4l2_fwnode_device_parse_dt(struct device *dev,
> > +                                    struct v4l2_fwnode_device_properties *props)
> >  {
> >       struct fwnode_handle *fwnode = dev_fwnode(dev);
> >       u32 val;
> >       int ret;
> >
> > -     memset(props, 0, sizeof(*props));
> > -
> > -     props->orientation = V4L2_FWNODE_PROPERTY_UNSET;
> >       ret = fwnode_property_read_u32(fwnode, "orientation", &val);
> >       if (!ret) {
> >               switch (val) {
> > @@ -833,7 +865,6 @@ int v4l2_fwnode_device_parse(struct device *dev,
> >               dev_dbg(dev, "device orientation: %u\n", val);
> >       }
> >
> > -     props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
> >       ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> >       if (!ret) {
> >               if (val >= 360) {
> > @@ -847,6 +878,21 @@ int v4l2_fwnode_device_parse(struct device *dev,
> >
> >       return 0;
> >  }
> > +
> > +int v4l2_fwnode_device_parse(struct device *dev,
> > +                          struct v4l2_fwnode_device_properties *props)
> > +{
> > +     struct fwnode_handle *fwnode = dev_fwnode(dev);
> > +
> > +     memset(props, 0, sizeof(*props));
> > +
> > +     props->orientation = V4L2_FWNODE_PROPERTY_UNSET;
> > +     props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
> > +
> > +     if (is_acpi_device_node(fwnode))
> > +             return v4l2_fwnode_device_parse_acpi(dev, props);
> > +     return v4l2_fwnode_device_parse_dt(dev, props);
> > +}
> >  EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse);
> >
> >  /*
> >
>
> --
> Kind regards,
>
> Sakari Ailus



-- 
Ricardo Ribalda

Re: [PATCH 3/8] media: v4l: fwnode: Support acpi devices for v4l2_fwnode_device_parse

[Hans de Goede]

Hi Ricardo,

On 22-Apr-25 2:23 AM, Ricardo Ribalda wrote:
> Hi Sakari
> 
> On Sun, 13 Apr 2025 at 17:50, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>>
>> Hi Ricardo,
>>
>> Thanks for the patch.
>>
>> On Thu, Apr 03, 2025 at 07:16:14PM +0000, Ricardo Ribalda wrote:
>>> This patch modifies v4l2_fwnode_device_parse() to support ACPI devices.
>>>
>>> We initially add support only for orientation via the ACPI _PLD method.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-fwnode.c | 58 +++++++++++++++++++++++++++++++----
>>>  1 file changed, 52 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>>> index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..81563c36b6436bb61e1c96f2a5ede3fa9d64dab3 100644
>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>>> @@ -15,6 +15,7 @@
>>>   * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>>   */
>>>  #include <linux/acpi.h>
>>> +#include <acpi/acpi_bus.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/mm.h>
>>>  #include <linux/module.h>
>>> @@ -807,16 +808,47 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode,
>>>  }
>>>  EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link);
>>>
>>> -int v4l2_fwnode_device_parse(struct device *dev,
>>> -                          struct v4l2_fwnode_device_properties *props)
>>> +static int v4l2_fwnode_device_parse_acpi(struct device *dev,
>>> +                                      struct v4l2_fwnode_device_properties *props)
>>> +{
>>> +     struct acpi_pld_info *pld;
>>> +     int ret = 0;
>>> +
>>> +     if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) {
>>> +             dev_dbg(dev, "acpi _PLD call failed\n");
>>> +             return 0;
>>> +     }
>>
>> You could have software nodes in an ACPI system as well as DT-aligned
>> properties. They're not the primary means to convey this information still.
>>
>> How about returning e.g. -ENODATA here if _PLD doesn't exist for the device
>> and then proceeding to parse properties as in DT?
> 
> Do you mean that there can be devices with ACPI handles that can also
> have DT properties?

Yes it is possible to embed DT properties in ACPI, but I don't
think that is really applicable here.

But we also have secondary software-fwnodes which are used
extensively on x86 to set device-properties on devices by
platform code to deal with ACPI tables sometimes having
incomplete information.

For example atm _PLD is already being parsed in:

drivers/media/pci/intel/ipu-bridge.c and that is then used to add
a standard "orientation" device-property on the sensor device.

This is actually something which I guess we can drop once your
patches are in, since those should then do the same in a more
generic manner.

> What shall we do if _PLD contradicts the DT property? What takes precedence?

As for priorities, at east for rotation it seems that we are going
to need some quirks, I already have a few Dell laptops where it seems
that the sensor is upside down and parsing the rotation field in
the IPU6 specific SSDB ACPI package does not yield a 180° rotation,
so we are going to need some quirks.

I expect these quirks to live in the bridge code, while your helper
will be called from sensor drivers, so in order to allow quirks to
override things, I think that first the "orientation" device-property
should be checked (which the ACPI glue code we have can set before
the sensor driver binds) and only then should _PLD be checked.

IOW _PLD should be seen as the fallback, because ACPI tables are
often a copy and paste job so it can very well contain wrong info
copy-pasted from some example ACPI code or from another hw model.

Regards,

Hans

Re: [PATCH 3/8] media: v4l: fwnode: Support acpi devices for v4l2_fwnode_device_parse

[Sakari Ailus]

Hi Hans, Ricardo,

On Tue, Apr 22, 2025 at 10:44:41AM +0200, Hans de Goede wrote:
> Hi Ricardo,
> 
> On 22-Apr-25 2:23 AM, Ricardo Ribalda wrote:
> > Hi Sakari
> > 
> > On Sun, 13 Apr 2025 at 17:50, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >>
> >> Hi Ricardo,
> >>
> >> Thanks for the patch.
> >>
> >> On Thu, Apr 03, 2025 at 07:16:14PM +0000, Ricardo Ribalda wrote:
> >>> This patch modifies v4l2_fwnode_device_parse() to support ACPI devices.
> >>>
> >>> We initially add support only for orientation via the ACPI _PLD method.
> >>>
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>> ---
> >>>  drivers/media/v4l2-core/v4l2-fwnode.c | 58 +++++++++++++++++++++++++++++++----
> >>>  1 file changed, 52 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..81563c36b6436bb61e1c96f2a5ede3fa9d64dab3 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> @@ -15,6 +15,7 @@
> >>>   * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >>>   */
> >>>  #include <linux/acpi.h>
> >>> +#include <acpi/acpi_bus.h>
> >>>  #include <linux/kernel.h>
> >>>  #include <linux/mm.h>
> >>>  #include <linux/module.h>
> >>> @@ -807,16 +808,47 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode,
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link);
> >>>
> >>> -int v4l2_fwnode_device_parse(struct device *dev,
> >>> -                          struct v4l2_fwnode_device_properties *props)
> >>> +static int v4l2_fwnode_device_parse_acpi(struct device *dev,
> >>> +                                      struct v4l2_fwnode_device_properties *props)
> >>> +{
> >>> +     struct acpi_pld_info *pld;
> >>> +     int ret = 0;
> >>> +
> >>> +     if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) {
> >>> +             dev_dbg(dev, "acpi _PLD call failed\n");
> >>> +             return 0;
> >>> +     }
> >>
> >> You could have software nodes in an ACPI system as well as DT-aligned
> >> properties. They're not the primary means to convey this information still.
> >>
> >> How about returning e.g. -ENODATA here if _PLD doesn't exist for the device
> >> and then proceeding to parse properties as in DT?
> > 
> > Do you mean that there can be devices with ACPI handles that can also
> > have DT properties?
> 
> Yes it is possible to embed DT properties in ACPI, but I don't
> think that is really applicable here.

This is determined by
Documentation/firmware-guide/acpi/DSD-properties-rules.rst . So rotation
and orientation shouldn't come from _DSD properties on ACPI systems.

> 
> But we also have secondary software-fwnodes which are used
> extensively on x86 to set device-properties on devices by
> platform code to deal with ACPI tables sometimes having
> incomplete information.
> 
> For example atm _PLD is already being parsed in:
> 
> drivers/media/pci/intel/ipu-bridge.c and that is then used to add
> a standard "orientation" device-property on the sensor device.
> 
> This is actually something which I guess we can drop once your
> patches are in, since those should then do the same in a more
> generic manner.

DisCo for Imaging support currently also digs this information from _PDL
(see init_crs_csi2_swnodes() in drivers/acpi/mipi-disco-img.c), but this
is only done if a _CRS CSI-2 descriptor is present. It could also be done
for devices with the IPU Windows specific ACPI objects and it would be a
natural location for handing quirks -- there are some
unrelated Dell DSDT quirks already.

> 
> > What shall we do if _PLD contradicts the DT property? What takes precedence?
> 
> As for priorities, at east for rotation it seems that we are going
> to need some quirks, I already have a few Dell laptops where it seems
> that the sensor is upside down and parsing the rotation field in
> the IPU6 specific SSDB ACPI package does not yield a 180° rotation,
> so we are going to need some quirks.
> 
> I expect these quirks to live in the bridge code, while your helper
> will be called from sensor drivers, so in order to allow quirks to
> override things, I think that first the "orientation" device-property
> should be checked (which the ACPI glue code we have can set before
> the sensor driver binds) and only then should _PLD be checked.
> 
> IOW _PLD should be seen as the fallback, because ACPI tables are
> often a copy and paste job so it can very well contain wrong info
> copy-pasted from some example ACPI code or from another hw model.

Unfortunately that does happen. :-(

-- 
Regards,

Sakari Ailus

Re: [PATCH 6/8] media: uvcvideo: Factor out gpio functions to its own file

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

Hans raised an issue on "[PATCH v3 2/8] media: uvcvideo: Factor out gpio
functions to its own file" (part of "[PATCH v3 0/8] media: uvcvideo:
Implement the Privacy GPIO as a evdev"), asking if GPIO handling should
still use a uvc_entity if it moves to a evdev. There are implications on
this series too. Unless I'm mistaken, I haven't seen a reply from you to
my last e-mail. Can we please first finish that discussion ?

On Thu, Apr 03, 2025 at 07:16:17PM +0000, Ricardo Ribalda wrote:
> This is just a refactor patch, no new functionality is added.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/Makefile     |   3 +-
>  drivers/media/usb/uvc/uvc_driver.c | 121 +-----------------------------------
>  drivers/media/usb/uvc/uvc_gpio.c   | 123 +++++++++++++++++++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   |   6 ++
>  4 files changed, 133 insertions(+), 120 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> index 4f9eee4f81ab6436a8b90324a688a149b2c3bcd1..85514b6e538fbb8284e574ca14700f2d749e1a2e 100644
> --- a/drivers/media/usb/uvc/Makefile
> +++ b/drivers/media/usb/uvc/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
> -		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
> +		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
> +		  uvc_gpio.o
>  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
>  uvcvideo-objs  += uvc_entity.o
>  endif
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index da24a655ab68cc0957762f2b67387677c22224d1..b52e1ff401e24f69b867b5e975cda4260463e760 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -8,7 +8,6 @@
>  
>  #include <linux/atomic.h>
>  #include <linux/bits.h>
> -#include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -792,8 +791,8 @@ static const u8 uvc_media_transport_input_guid[16] =
>  	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
>  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
>  
> -static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
> -		unsigned int num_pads, unsigned int extra_size)
> +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
> +				    unsigned int extra_size)
>  {
>  	struct uvc_entity *entity;
>  	unsigned int num_inputs;
> @@ -1242,122 +1241,6 @@ static int uvc_parse_control(struct uvc_device *dev)
>  	return 0;
>  }
>  
> -/* -----------------------------------------------------------------------------
> - * Privacy GPIO
> - */
> -
> -static void uvc_gpio_event(struct uvc_device *dev)
> -{
> -	struct uvc_entity *unit = dev->gpio_unit;
> -	struct uvc_video_chain *chain;
> -	u8 new_val;
> -
> -	if (!unit)
> -		return;
> -
> -	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> -
> -	/* GPIO entities are always on the first chain. */
> -	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> -	uvc_ctrl_status_event(chain, unit->controls, &new_val);
> -}
> -
> -static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> -			    u8 cs, void *data, u16 size)
> -{
> -	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> -		return -EINVAL;
> -
> -	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> -
> -	return 0;
> -}
> -
> -static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> -			     u8 cs, u8 *caps)
> -{
> -	if (cs != UVC_CT_PRIVACY_CONTROL)
> -		return -EINVAL;
> -
> -	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> -	return 0;
> -}
> -
> -static irqreturn_t uvc_gpio_irq(int irq, void *data)
> -{
> -	struct uvc_device *dev = data;
> -
> -	uvc_gpio_event(dev);
> -	return IRQ_HANDLED;
> -}
> -
> -static int uvc_gpio_parse(struct uvc_device *dev)
> -{
> -	struct uvc_entity *unit;
> -	struct gpio_desc *gpio_privacy;
> -	int irq;
> -
> -	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> -					       GPIOD_IN);
> -	if (!gpio_privacy)
> -		return 0;
> -
> -	if (IS_ERR(gpio_privacy))
> -		return dev_err_probe(&dev->intf->dev,
> -				     PTR_ERR(gpio_privacy),
> -				     "Can't get privacy GPIO\n");
> -
> -	irq = gpiod_to_irq(gpio_privacy);
> -	if (irq < 0)
> -		return dev_err_probe(&dev->intf->dev, irq,
> -				     "No IRQ for privacy GPIO\n");
> -
> -	unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> -	if (!unit)
> -		return -ENOMEM;
> -
> -	unit->gpio.gpio_privacy = gpio_privacy;
> -	unit->gpio.irq = irq;
> -	unit->gpio.bControlSize = 1;
> -	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> -	unit->gpio.bmControls[0] = 1;
> -	unit->get_cur = uvc_gpio_get_cur;
> -	unit->get_info = uvc_gpio_get_info;
> -	strscpy(unit->name, "GPIO", sizeof(unit->name));
> -
> -	list_add_tail(&unit->list, &dev->entities);
> -
> -	dev->gpio_unit = unit;
> -
> -	return 0;
> -}
> -
> -static int uvc_gpio_init_irq(struct uvc_device *dev)
> -{
> -	struct uvc_entity *unit = dev->gpio_unit;
> -	int ret;
> -
> -	if (!unit || unit->gpio.irq < 0)
> -		return 0;
> -
> -	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> -				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> -				   IRQF_TRIGGER_RISING,
> -				   "uvc_privacy_gpio", dev);
> -
> -	unit->gpio.initialized = !ret;
> -
> -	return ret;
> -}
> -
> -static void uvc_gpio_deinit(struct uvc_device *dev)
> -{
> -	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> -		return;
> -
> -	free_irq(dev->gpio_unit->gpio.irq, dev);
> -}
> -
>  /* ------------------------------------------------------------------------
>   * UVC device scan
>   */
> diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..30e3e6dd22cbc9cfee420dde7f7f64dbdce499b9
> --- /dev/null
> +++ b/drivers/media/usb/uvc/uvc_gpio.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *      uvc_gpio.c  --  USB Video Class driver
> + *
> + *      Copyright 2025 Google LLC
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/gpio/consumer.h>
> +#include "uvcvideo.h"
> +
> +static void uvc_gpio_event(struct uvc_device *dev)
> +{
> +	struct uvc_entity *unit = dev->gpio_unit;
> +	struct uvc_video_chain *chain;
> +	u8 new_val;
> +
> +	if (!unit)
> +		return;
> +
> +	new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> +
> +	/* GPIO entities are always on the first chain. */
> +	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> +	uvc_ctrl_status_event(chain, unit->controls, &new_val);
> +}
> +
> +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> +			    u8 cs, void *data, u16 size)
> +{
> +	if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> +		return -EINVAL;
> +
> +	*(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> +
> +	return 0;
> +}
> +
> +static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> +			     u8 cs, u8 *caps)
> +{
> +	if (cs != UVC_CT_PRIVACY_CONTROL)
> +		return -EINVAL;
> +
> +	*caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> +	return 0;
> +}
> +
> +static irqreturn_t uvc_gpio_irq(int irq, void *data)
> +{
> +	struct uvc_device *dev = data;
> +
> +	uvc_gpio_event(dev);
> +	return IRQ_HANDLED;
> +}
> +
> +int uvc_gpio_parse(struct uvc_device *dev)
> +{
> +	struct uvc_entity *unit;
> +	struct gpio_desc *gpio_privacy;
> +	int irq;
> +
> +	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> +					       GPIOD_IN);
> +	if (!gpio_privacy)
> +		return 0;
> +
> +	if (IS_ERR(gpio_privacy))
> +		return dev_err_probe(&dev->intf->dev,
> +				     PTR_ERR(gpio_privacy),
> +				     "Can't get privacy GPIO\n");
> +
> +	irq = gpiod_to_irq(gpio_privacy);
> +	if (irq < 0)
> +		return dev_err_probe(&dev->intf->dev, irq,
> +				     "No IRQ for privacy GPIO\n");
> +
> +	unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> +	if (!unit)
> +		return -ENOMEM;
> +
> +	unit->gpio.gpio_privacy = gpio_privacy;
> +	unit->gpio.irq = irq;
> +	unit->gpio.bControlSize = 1;
> +	unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> +	unit->gpio.bmControls[0] = 1;
> +	unit->get_cur = uvc_gpio_get_cur;
> +	unit->get_info = uvc_gpio_get_info;
> +	strscpy(unit->name, "GPIO", sizeof(unit->name));
> +
> +	list_add_tail(&unit->list, &dev->entities);
> +
> +	dev->gpio_unit = unit;
> +
> +	return 0;
> +}
> +
> +int uvc_gpio_init_irq(struct uvc_device *dev)
> +{
> +	struct uvc_entity *unit = dev->gpio_unit;
> +	int ret;
> +
> +	if (!unit || unit->gpio.irq < 0)
> +		return 0;
> +
> +	ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> +				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> +				   IRQF_TRIGGER_RISING,
> +				   "uvc_privacy_gpio", dev);
> +
> +	unit->gpio.initialized = !ret;
> +
> +	return ret;
> +}
> +
> +void uvc_gpio_deinit(struct uvc_device *dev)
> +{
> +	if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> +		return;
> +
> +	free_irq(dev->gpio_unit->gpio.irq, dev);
> +}
> +
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..aef96b96499ce09ffa286c51793482afd9832097 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -683,6 +683,8 @@ do {									\
>   */
>  
>  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
> +				    unsigned int extra_size);
>  
>  /* Video buffers queue management. */
>  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type);
> @@ -829,4 +831,8 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
>  size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
>  			    size_t size);
>  
> +/* gpio */
> +int uvc_gpio_parse(struct uvc_device *dev);
> +int uvc_gpio_init_irq(struct uvc_device *dev);
> +void uvc_gpio_deinit(struct uvc_device *dev);
>  #endif
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/8] media: uvcvideo: Add support for V4L2_CID_CAMERA_ORIENTATION

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Apr 03, 2025 at 07:16:18PM +0000, Ricardo Ribalda wrote:
> Fetch the orientation from the fwnode and map it into a control.
> 
> The uvc driver does not use the media controller, so we need to create a
> virtual entity, like we previously did with the external gpio.

I don't want to create a new entity every time we need to expose a new
"software" control. If we keep handling GPIO as an entity, can we use it
to expose all non-UVC controls ? Otherwise, if we have to create a new
entity here, let's make sure we can reuse it in the future for more
controls, and name it appropriately.

This being said, functionally speaking, wouldn't the
V4L2_CID_CAMERA_ORIENTATION control make more sense on the camera input
terminal ? Have you considered adding it there ? Or would it be too
intrusive ?

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/Makefile     |  2 +-
>  drivers/media/usb/uvc/uvc_ctrl.c   | 21 +++++++++++
>  drivers/media/usb/uvc/uvc_driver.c | 14 ++++++--
>  drivers/media/usb/uvc/uvc_entity.c |  1 +
>  drivers/media/usb/uvc/uvc_fwnode.c | 73 ++++++++++++++++++++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   | 15 ++++++++
>  include/linux/usb/uvc.h            |  3 ++
>  7 files changed, 125 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> index 85514b6e538fbb8284e574ca14700f2d749e1a2e..2a5b76aab756c21011d966f3ce0410ff6b8e5b19 100644
> --- a/drivers/media/usb/uvc/Makefile
> +++ b/drivers/media/usb/uvc/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
>  		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
> -		  uvc_gpio.o
> +		  uvc_gpio.o uvc_fwnode.o
>  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
>  uvcvideo-objs  += uvc_entity.o
>  endif
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index cbf19aa1d82374a08cf79b6a6787fa348b83523a..df84bf292dcab6b833fbd3c70eccbe9e567ee567 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -376,6 +376,13 @@ static const struct uvc_control_info uvc_ctrls[] = {
>  				| UVC_CTRL_FLAG_GET_DEF
>  				| UVC_CTRL_FLAG_AUTO_UPDATE,
>  	},
> +	{
> +		.entity		= UVC_GUID_FWNODE,
> +		.selector	= 0,
> +		.index		= 0,
> +		.size		= 1,
> +		.flags		= UVC_CTRL_FLAG_GET_CUR,
> +	},
>  };
>  
>  static const u32 uvc_control_classes[] = {
> @@ -975,6 +982,17 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  		.data_type	= UVC_CTRL_DATA_TYPE_BITMASK,
>  		.name		= "Region of Interest Auto Ctrls",
>  	},
> +	{
> +		.id		= V4L2_CID_CAMERA_ORIENTATION,
> +		.entity		= UVC_GUID_FWNODE,
> +		.selector	= 0,
> +		.size		= 8,
> +		.offset		= 0,
> +		.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> +		.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> +		.menu_mask	= GENMASK(V4L2_CAMERA_ORIENTATION_EXTERNAL,
> +					  V4L2_CAMERA_ORIENTATION_FRONT),
> +	},
>  };
>  
>  /* ------------------------------------------------------------------------
> @@ -3170,6 +3188,9 @@ static int uvc_ctrl_init_chain(struct uvc_video_chain *chain)
>  		} else if (UVC_ENTITY_TYPE(entity) == UVC_EXT_GPIO_UNIT) {
>  			bmControls = entity->gpio.bmControls;
>  			bControlSize = entity->gpio.bControlSize;
> +		} else if (UVC_ENTITY_TYPE(entity) == UVC_FWNODE_UNIT) {
> +			bmControls = entity->fwnode.bmControls;
> +			bControlSize = entity->fwnode.bControlSize;
>  		}
>  
>  		/* Remove bogus/blacklisted controls */
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index b52e1ff401e24f69b867b5e975cda4260463e760..9a8e5d94b3eba09e1ee1be20bad5b016b6def915 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1752,11 +1752,15 @@ static int uvc_scan_device(struct uvc_device *dev)
>  		return -1;
>  	}
>  
> -	/* Add GPIO entity to the first chain. */
> -	if (dev->gpio_unit) {
> +	/* Add virtual entities to the first chain. */
> +	if (dev->gpio_unit || dev->fwnode_unit) {
>  		chain = list_first_entry(&dev->chains,
>  					 struct uvc_video_chain, list);
> -		list_add_tail(&dev->gpio_unit->chain, &chain->entities);
> +		if (dev->gpio_unit)
> +			list_add_tail(&dev->gpio_unit->chain, &chain->entities);
> +		if (dev->fwnode_unit)
> +			list_add_tail(&dev->fwnode_unit->chain,
> +				      &chain->entities);
>  	}
>  
>  	return 0;
> @@ -2132,6 +2136,10 @@ static int uvc_probe(struct usb_interface *intf,
>  	if (ret < 0)
>  		goto error;
>  
> +	ret = uvc_fwnode_parse(dev);
> +	if (ret < 0)
> +		goto error;
> +
>  	dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
>  		 dev->uvc_version >> 8, dev->uvc_version & 0xff,
>  		 udev->product ? udev->product : "<unnamed>",
> diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
> index cc68dd24eb42dce5b2846ca52a8dfa499c8aed96..01eeba2f049e9ebb25e091340a133656acbf28ca 100644
> --- a/drivers/media/usb/uvc/uvc_entity.c
> +++ b/drivers/media/usb/uvc/uvc_entity.c
> @@ -106,6 +106,7 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain,
>  		case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:
>  		case UVC_EXTERNAL_VENDOR_SPECIFIC:
>  		case UVC_EXT_GPIO_UNIT:
> +		case UVC_FWNODE_UNIT:
>  		default:
>  			function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>  			break;
> diff --git a/drivers/media/usb/uvc/uvc_fwnode.c b/drivers/media/usb/uvc/uvc_fwnode.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..20f7b8847551acfd44cbf09bcbf653d89985561f
> --- /dev/null
> +++ b/drivers/media/usb/uvc/uvc_fwnode.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *      uvc_fwnode.c  --  USB Video Class driver
> + *
> + *      Copyright 2025 Google LLC
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/usb/uvc.h>
> +#include <media/v4l2-fwnode.h>
> +#include "uvcvideo.h"
> +
> +static int uvc_fwnode_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> +			      u8 cs, void *data, u16 size)
> +{
> +	if (size < 1)
> +		return -EINVAL;
> +
> +	switch (entity->fwnode.props.orientation) {
> +	case V4L2_FWNODE_ORIENTATION_FRONT:
> +		*(u8 *)data = V4L2_CAMERA_ORIENTATION_FRONT;
> +		break;
> +	case V4L2_FWNODE_ORIENTATION_BACK:
> +		*(u8 *)data = V4L2_CAMERA_ORIENTATION_BACK;
> +		break;
> +	default:
> +		*(u8 *)data = V4L2_CAMERA_ORIENTATION_EXTERNAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int uvc_fwnode_get_info(struct uvc_device *dev,
> +			       struct uvc_entity *entity, u8 cs, u8 *caps)
> +{
> +	*caps = UVC_CONTROL_CAP_GET;
> +	return 0;
> +}
> +
> +int uvc_fwnode_parse(struct uvc_device *dev)
> +{
> +	static const u8 uvc_fwnode_guid[] = UVC_GUID_FWNODE;
> +	struct v4l2_fwnode_device_properties props;
> +	struct uvc_entity *unit;
> +	int ret;
> +
> +	ret = v4l2_fwnode_device_parse(&dev->udev->dev, &props);
> +	if (ret)
> +		return dev_err_probe(&dev->intf->dev, ret,
> +				     "Can't parse fwnode\n");
> +
> +	if (props.orientation == V4L2_FWNODE_PROPERTY_UNSET)
> +		return 0;
> +
> +	unit = uvc_alloc_entity(UVC_FWNODE_UNIT, UVC_FWNODE_UNIT_ID, 0, 1);
> +	if (!unit)
> +		return -ENOMEM;
> +
> +	memcpy(unit->guid, uvc_fwnode_guid, sizeof(unit->guid));
> +	unit->fwnode.props = props;
> +	unit->fwnode.bControlSize = 1;
> +	unit->fwnode.bmControls = (u8 *)unit + sizeof(*unit);
> +	unit->fwnode.bmControls[0] = 1;
> +	unit->get_cur = uvc_fwnode_get_cur;
> +	unit->get_info = uvc_fwnode_get_info;
> +	strscpy(unit->name, "FWNODE", sizeof(unit->name));
> +
> +	list_add_tail(&unit->list, &dev->entities);
> +
> +	dev->fwnode_unit = unit;
> +
> +	return 0;
> +}
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index aef96b96499ce09ffa286c51793482afd9832097..c52ab99bf8d21ab37270d27ffc040fd115b3f5ba 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -19,6 +19,7 @@
>  #include <media/v4l2-event.h>
>  #include <media/v4l2-fh.h>
>  #include <media/videobuf2-v4l2.h>
> +#include <media/v4l2-fwnode.h>
>  
>  /* --------------------------------------------------------------------------
>   * UVC constants
> @@ -41,6 +42,9 @@
>  #define UVC_EXT_GPIO_UNIT		0x7ffe
>  #define UVC_EXT_GPIO_UNIT_ID		0x100
>  
> +#define UVC_FWNODE_UNIT			0x7ffd
> +#define UVC_FWNODE_UNIT_ID		0x101
> +
>  /* ------------------------------------------------------------------------
>   * Driver specific constants.
>   */
> @@ -242,6 +246,12 @@ struct uvc_entity {
>  			int irq;
>  			bool initialized;
>  		} gpio;
> +
> +		struct {
> +			u8  bControlSize;
> +			u8  *bmControls;
> +			struct v4l2_fwnode_device_properties props;
> +		} fwnode;
>  	};
>  
>  	u8 bNrInPins;
> @@ -617,6 +627,7 @@ struct uvc_device {
>  	} async_ctrl;
>  
>  	struct uvc_entity *gpio_unit;
> +	struct uvc_entity *fwnode_unit;
>  };
>  
>  enum uvc_handle_state {
> @@ -835,4 +846,8 @@ size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
>  int uvc_gpio_parse(struct uvc_device *dev);
>  int uvc_gpio_init_irq(struct uvc_device *dev);
>  void uvc_gpio_deinit(struct uvc_device *dev);
> +
> +/* fwnode */
> +int uvc_fwnode_parse(struct uvc_device *dev);
> +
>  #endif
> diff --git a/include/linux/usb/uvc.h b/include/linux/usb/uvc.h
> index bce95153e5a65613a710d7316fc17cf5462b5bce..d818839b0442988d94ab44935e1ce855b0adf4a1 100644
> --- a/include/linux/usb/uvc.h
> +++ b/include/linux/usb/uvc.h
> @@ -29,6 +29,9 @@
>  #define UVC_GUID_EXT_GPIO_CONTROLLER \
>  	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
>  	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x03}
> +#define UVC_GUID_FWNODE \
> +	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
> +	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x04}
>  
>  #define UVC_GUID_FORMAT_MJPEG \
>  	{ 'M',  'J',  'P',  'G', 0x00, 0x00, 0x10, 0x00, \
> 
> -- 
> 2.49.0.504.g3bcea36a83-goog
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/8] media: uvcvideo: Add support for V4L2_CID_CAMERA_ORIENTATION

[Laurent Pinchart]

On Wed, Apr 23, 2025 at 12:32:37AM +0300, Laurent Pinchart wrote:
> Hi Ricardo,
> 
> Thank you for the patch.
> 
> On Thu, Apr 03, 2025 at 07:16:18PM +0000, Ricardo Ribalda wrote:
> > Fetch the orientation from the fwnode and map it into a control.
> > 
> > The uvc driver does not use the media controller, so we need to create a
> > virtual entity, like we previously did with the external gpio.
> 
> I don't want to create a new entity every time we need to expose a new
> "software" control. If we keep handling GPIO as an entity, can we use it
> to expose all non-UVC controls ? Otherwise, if we have to create a new
> entity here, let's make sure we can reuse it in the future for more
> controls, and name it appropriately.
> 
> This being said, functionally speaking, wouldn't the
> V4L2_CID_CAMERA_ORIENTATION control make more sense on the camera input
> terminal ? Have you considered adding it there ? Or would it be too
> intrusive ?

Scratch the last comment, I was thinking about exposing controls on
subdevs, which we obviously don't do.

> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/Makefile     |  2 +-
> >  drivers/media/usb/uvc/uvc_ctrl.c   | 21 +++++++++++
> >  drivers/media/usb/uvc/uvc_driver.c | 14 ++++++--
> >  drivers/media/usb/uvc/uvc_entity.c |  1 +
> >  drivers/media/usb/uvc/uvc_fwnode.c | 73 ++++++++++++++++++++++++++++++++++++++
> >  drivers/media/usb/uvc/uvcvideo.h   | 15 ++++++++
> >  include/linux/usb/uvc.h            |  3 ++
> >  7 files changed, 125 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> > index 85514b6e538fbb8284e574ca14700f2d749e1a2e..2a5b76aab756c21011d966f3ce0410ff6b8e5b19 100644
> > --- a/drivers/media/usb/uvc/Makefile
> > +++ b/drivers/media/usb/uvc/Makefile
> > @@ -1,7 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
> >  		  uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
> > -		  uvc_gpio.o
> > +		  uvc_gpio.o uvc_fwnode.o
> >  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
> >  uvcvideo-objs  += uvc_entity.o
> >  endif
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index cbf19aa1d82374a08cf79b6a6787fa348b83523a..df84bf292dcab6b833fbd3c70eccbe9e567ee567 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -376,6 +376,13 @@ static const struct uvc_control_info uvc_ctrls[] = {
> >  				| UVC_CTRL_FLAG_GET_DEF
> >  				| UVC_CTRL_FLAG_AUTO_UPDATE,
> >  	},
> > +	{
> > +		.entity		= UVC_GUID_FWNODE,
> > +		.selector	= 0,
> > +		.index		= 0,
> > +		.size		= 1,
> > +		.flags		= UVC_CTRL_FLAG_GET_CUR,
> > +	},
> >  };
> >  
> >  static const u32 uvc_control_classes[] = {
> > @@ -975,6 +982,17 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
> >  		.data_type	= UVC_CTRL_DATA_TYPE_BITMASK,
> >  		.name		= "Region of Interest Auto Ctrls",
> >  	},
> > +	{
> > +		.id		= V4L2_CID_CAMERA_ORIENTATION,
> > +		.entity		= UVC_GUID_FWNODE,
> > +		.selector	= 0,
> > +		.size		= 8,
> > +		.offset		= 0,
> > +		.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> > +		.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> > +		.menu_mask	= GENMASK(V4L2_CAMERA_ORIENTATION_EXTERNAL,
> > +					  V4L2_CAMERA_ORIENTATION_FRONT),
> > +	},
> >  };
> >  
> >  /* ------------------------------------------------------------------------
> > @@ -3170,6 +3188,9 @@ static int uvc_ctrl_init_chain(struct uvc_video_chain *chain)
> >  		} else if (UVC_ENTITY_TYPE(entity) == UVC_EXT_GPIO_UNIT) {
> >  			bmControls = entity->gpio.bmControls;
> >  			bControlSize = entity->gpio.bControlSize;
> > +		} else if (UVC_ENTITY_TYPE(entity) == UVC_FWNODE_UNIT) {
> > +			bmControls = entity->fwnode.bmControls;
> > +			bControlSize = entity->fwnode.bControlSize;
> >  		}
> >  
> >  		/* Remove bogus/blacklisted controls */
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index b52e1ff401e24f69b867b5e975cda4260463e760..9a8e5d94b3eba09e1ee1be20bad5b016b6def915 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1752,11 +1752,15 @@ static int uvc_scan_device(struct uvc_device *dev)
> >  		return -1;
> >  	}
> >  
> > -	/* Add GPIO entity to the first chain. */
> > -	if (dev->gpio_unit) {
> > +	/* Add virtual entities to the first chain. */
> > +	if (dev->gpio_unit || dev->fwnode_unit) {
> >  		chain = list_first_entry(&dev->chains,
> >  					 struct uvc_video_chain, list);
> > -		list_add_tail(&dev->gpio_unit->chain, &chain->entities);
> > +		if (dev->gpio_unit)
> > +			list_add_tail(&dev->gpio_unit->chain, &chain->entities);
> > +		if (dev->fwnode_unit)
> > +			list_add_tail(&dev->fwnode_unit->chain,
> > +				      &chain->entities);
> >  	}
> >  
> >  	return 0;
> > @@ -2132,6 +2136,10 @@ static int uvc_probe(struct usb_interface *intf,
> >  	if (ret < 0)
> >  		goto error;
> >  
> > +	ret = uvc_fwnode_parse(dev);
> > +	if (ret < 0)
> > +		goto error;
> > +
> >  	dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
> >  		 dev->uvc_version >> 8, dev->uvc_version & 0xff,
> >  		 udev->product ? udev->product : "<unnamed>",
> > diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
> > index cc68dd24eb42dce5b2846ca52a8dfa499c8aed96..01eeba2f049e9ebb25e091340a133656acbf28ca 100644
> > --- a/drivers/media/usb/uvc/uvc_entity.c
> > +++ b/drivers/media/usb/uvc/uvc_entity.c
> > @@ -106,6 +106,7 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain,
> >  		case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:
> >  		case UVC_EXTERNAL_VENDOR_SPECIFIC:
> >  		case UVC_EXT_GPIO_UNIT:
> > +		case UVC_FWNODE_UNIT:
> >  		default:
> >  			function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> >  			break;
> > diff --git a/drivers/media/usb/uvc/uvc_fwnode.c b/drivers/media/usb/uvc/uvc_fwnode.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..20f7b8847551acfd44cbf09bcbf653d89985561f
> > --- /dev/null
> > +++ b/drivers/media/usb/uvc/uvc_fwnode.c
> > @@ -0,0 +1,73 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *      uvc_fwnode.c  --  USB Video Class driver
> > + *
> > + *      Copyright 2025 Google LLC
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/usb/uvc.h>
> > +#include <media/v4l2-fwnode.h>
> > +#include "uvcvideo.h"
> > +
> > +static int uvc_fwnode_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > +			      u8 cs, void *data, u16 size)
> > +{
> > +	if (size < 1)
> > +		return -EINVAL;
> > +
> > +	switch (entity->fwnode.props.orientation) {
> > +	case V4L2_FWNODE_ORIENTATION_FRONT:
> > +		*(u8 *)data = V4L2_CAMERA_ORIENTATION_FRONT;
> > +		break;
> > +	case V4L2_FWNODE_ORIENTATION_BACK:
> > +		*(u8 *)data = V4L2_CAMERA_ORIENTATION_BACK;
> > +		break;
> > +	default:
> > +		*(u8 *)data = V4L2_CAMERA_ORIENTATION_EXTERNAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int uvc_fwnode_get_info(struct uvc_device *dev,
> > +			       struct uvc_entity *entity, u8 cs, u8 *caps)
> > +{
> > +	*caps = UVC_CONTROL_CAP_GET;
> > +	return 0;
> > +}
> > +
> > +int uvc_fwnode_parse(struct uvc_device *dev)
> > +{
> > +	static const u8 uvc_fwnode_guid[] = UVC_GUID_FWNODE;
> > +	struct v4l2_fwnode_device_properties props;
> > +	struct uvc_entity *unit;
> > +	int ret;
> > +
> > +	ret = v4l2_fwnode_device_parse(&dev->udev->dev, &props);
> > +	if (ret)
> > +		return dev_err_probe(&dev->intf->dev, ret,
> > +				     "Can't parse fwnode\n");
> > +
> > +	if (props.orientation == V4L2_FWNODE_PROPERTY_UNSET)
> > +		return 0;
> > +
> > +	unit = uvc_alloc_entity(UVC_FWNODE_UNIT, UVC_FWNODE_UNIT_ID, 0, 1);
> > +	if (!unit)
> > +		return -ENOMEM;
> > +
> > +	memcpy(unit->guid, uvc_fwnode_guid, sizeof(unit->guid));
> > +	unit->fwnode.props = props;
> > +	unit->fwnode.bControlSize = 1;
> > +	unit->fwnode.bmControls = (u8 *)unit + sizeof(*unit);
> > +	unit->fwnode.bmControls[0] = 1;
> > +	unit->get_cur = uvc_fwnode_get_cur;
> > +	unit->get_info = uvc_fwnode_get_info;
> > +	strscpy(unit->name, "FWNODE", sizeof(unit->name));
> > +
> > +	list_add_tail(&unit->list, &dev->entities);
> > +
> > +	dev->fwnode_unit = unit;
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index aef96b96499ce09ffa286c51793482afd9832097..c52ab99bf8d21ab37270d27ffc040fd115b3f5ba 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -19,6 +19,7 @@
> >  #include <media/v4l2-event.h>
> >  #include <media/v4l2-fh.h>
> >  #include <media/videobuf2-v4l2.h>
> > +#include <media/v4l2-fwnode.h>
> >  
> >  /* --------------------------------------------------------------------------
> >   * UVC constants
> > @@ -41,6 +42,9 @@
> >  #define UVC_EXT_GPIO_UNIT		0x7ffe
> >  #define UVC_EXT_GPIO_UNIT_ID		0x100
> >  
> > +#define UVC_FWNODE_UNIT			0x7ffd
> > +#define UVC_FWNODE_UNIT_ID		0x101
> > +
> >  /* ------------------------------------------------------------------------
> >   * Driver specific constants.
> >   */
> > @@ -242,6 +246,12 @@ struct uvc_entity {
> >  			int irq;
> >  			bool initialized;
> >  		} gpio;
> > +
> > +		struct {
> > +			u8  bControlSize;
> > +			u8  *bmControls;
> > +			struct v4l2_fwnode_device_properties props;
> > +		} fwnode;
> >  	};
> >  
> >  	u8 bNrInPins;
> > @@ -617,6 +627,7 @@ struct uvc_device {
> >  	} async_ctrl;
> >  
> >  	struct uvc_entity *gpio_unit;
> > +	struct uvc_entity *fwnode_unit;
> >  };
> >  
> >  enum uvc_handle_state {
> > @@ -835,4 +846,8 @@ size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
> >  int uvc_gpio_parse(struct uvc_device *dev);
> >  int uvc_gpio_init_irq(struct uvc_device *dev);
> >  void uvc_gpio_deinit(struct uvc_device *dev);
> > +
> > +/* fwnode */
> > +int uvc_fwnode_parse(struct uvc_device *dev);
> > +
> >  #endif
> > diff --git a/include/linux/usb/uvc.h b/include/linux/usb/uvc.h
> > index bce95153e5a65613a710d7316fc17cf5462b5bce..d818839b0442988d94ab44935e1ce855b0adf4a1 100644
> > --- a/include/linux/usb/uvc.h
> > +++ b/include/linux/usb/uvc.h
> > @@ -29,6 +29,9 @@
> >  #define UVC_GUID_EXT_GPIO_CONTROLLER \
> >  	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
> >  	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x03}
> > +#define UVC_GUID_FWNODE \
> > +	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
> > +	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x04}
> >  
> >  #define UVC_GUID_FORMAT_MJPEG \
> >  	{ 'M',  'J',  'P',  'G', 0x00, 0x00, 0x10, 0x00, \
> > 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] media: uvcvideo: Factor out gpio functions to its own file

[Ricardo Ribalda]

Hi Laurent

On Wed, 23 Apr 2025 at 05:28, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> Hans raised an issue on "[PATCH v3 2/8] media: uvcvideo: Factor out gpio
> functions to its own file" (part of "[PATCH v3 0/8] media: uvcvideo:
> Implement the Privacy GPIO as a evdev"), asking if GPIO handling should
> still use a uvc_entity if it moves to a evdev. There are implications on
> this series too. Unless I'm mistaken, I haven't seen a reply from you to
> my last e-mail. Can we please first finish that discussion ?

Are you referring to:
https://lore.kernel.org/all/0dfb780b-f2dc-43ed-a67d-afd5f50bb88f@redhat.com/
?

Is it an open discussion? I believe we agreed to remove the uvc_entity.

I posted v4 back in november. In that version I remove completely the
uvc_entity:
https://patchwork.linuxtv.org/project/linux-media/patch/20241125-uvc-subdev-v4-6-51e040599f1a@chromium.org/



Regardss
>
> On Thu, Apr 03, 2025 at 07:16:17PM +0000, Ricardo Ribalda wrote:
> > This is just a refactor patch, no new functionality is added.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/Makefile     |   3 +-
> >  drivers/media/usb/uvc/uvc_driver.c | 121 +-----------------------------------
> >  drivers/media/usb/uvc/uvc_gpio.c   | 123 +++++++++++++++++++++++++++++++++++++
> >  drivers/media/usb/uvc/uvcvideo.h   |   6 ++
> >  4 files changed, 133 insertions(+), 120 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> > index 4f9eee4f81ab6436a8b90324a688a149b2c3bcd1..85514b6e538fbb8284e574ca14700f2d749e1a2e 100644
> > --- a/drivers/media/usb/uvc/Makefile
> > +++ b/drivers/media/usb/uvc/Makefile
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
> > -               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
> > +               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
> > +               uvc_gpio.o
> >  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
> >  uvcvideo-objs  += uvc_entity.o
> >  endif
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index da24a655ab68cc0957762f2b67387677c22224d1..b52e1ff401e24f69b867b5e975cda4260463e760 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -8,7 +8,6 @@
> >
> >  #include <linux/atomic.h>
> >  #include <linux/bits.h>
> > -#include <linux/gpio/consumer.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> >  #include <linux/module.h>
> > @@ -792,8 +791,8 @@ static const u8 uvc_media_transport_input_guid[16] =
> >       UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> >  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> >
> > -static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
> > -             unsigned int num_pads, unsigned int extra_size)
> > +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
> > +                                 unsigned int extra_size)
> >  {
> >       struct uvc_entity *entity;
> >       unsigned int num_inputs;
> > @@ -1242,122 +1241,6 @@ static int uvc_parse_control(struct uvc_device *dev)
> >       return 0;
> >  }
> >
> > -/* -----------------------------------------------------------------------------
> > - * Privacy GPIO
> > - */
> > -
> > -static void uvc_gpio_event(struct uvc_device *dev)
> > -{
> > -     struct uvc_entity *unit = dev->gpio_unit;
> > -     struct uvc_video_chain *chain;
> > -     u8 new_val;
> > -
> > -     if (!unit)
> > -             return;
> > -
> > -     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > -
> > -     /* GPIO entities are always on the first chain. */
> > -     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > -     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > -}
> > -
> > -static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > -                         u8 cs, void *data, u16 size)
> > -{
> > -     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > -             return -EINVAL;
> > -
> > -     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > -
> > -     return 0;
> > -}
> > -
> > -static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > -                          u8 cs, u8 *caps)
> > -{
> > -     if (cs != UVC_CT_PRIVACY_CONTROL)
> > -             return -EINVAL;
> > -
> > -     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > -     return 0;
> > -}
> > -
> > -static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > -{
> > -     struct uvc_device *dev = data;
> > -
> > -     uvc_gpio_event(dev);
> > -     return IRQ_HANDLED;
> > -}
> > -
> > -static int uvc_gpio_parse(struct uvc_device *dev)
> > -{
> > -     struct uvc_entity *unit;
> > -     struct gpio_desc *gpio_privacy;
> > -     int irq;
> > -
> > -     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > -                                            GPIOD_IN);
> > -     if (!gpio_privacy)
> > -             return 0;
> > -
> > -     if (IS_ERR(gpio_privacy))
> > -             return dev_err_probe(&dev->intf->dev,
> > -                                  PTR_ERR(gpio_privacy),
> > -                                  "Can't get privacy GPIO\n");
> > -
> > -     irq = gpiod_to_irq(gpio_privacy);
> > -     if (irq < 0)
> > -             return dev_err_probe(&dev->intf->dev, irq,
> > -                                  "No IRQ for privacy GPIO\n");
> > -
> > -     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > -     if (!unit)
> > -             return -ENOMEM;
> > -
> > -     unit->gpio.gpio_privacy = gpio_privacy;
> > -     unit->gpio.irq = irq;
> > -     unit->gpio.bControlSize = 1;
> > -     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > -     unit->gpio.bmControls[0] = 1;
> > -     unit->get_cur = uvc_gpio_get_cur;
> > -     unit->get_info = uvc_gpio_get_info;
> > -     strscpy(unit->name, "GPIO", sizeof(unit->name));
> > -
> > -     list_add_tail(&unit->list, &dev->entities);
> > -
> > -     dev->gpio_unit = unit;
> > -
> > -     return 0;
> > -}
> > -
> > -static int uvc_gpio_init_irq(struct uvc_device *dev)
> > -{
> > -     struct uvc_entity *unit = dev->gpio_unit;
> > -     int ret;
> > -
> > -     if (!unit || unit->gpio.irq < 0)
> > -             return 0;
> > -
> > -     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > -                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > -                                IRQF_TRIGGER_RISING,
> > -                                "uvc_privacy_gpio", dev);
> > -
> > -     unit->gpio.initialized = !ret;
> > -
> > -     return ret;
> > -}
> > -
> > -static void uvc_gpio_deinit(struct uvc_device *dev)
> > -{
> > -     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > -             return;
> > -
> > -     free_irq(dev->gpio_unit->gpio.irq, dev);
> > -}
> > -
> >  /* ------------------------------------------------------------------------
> >   * UVC device scan
> >   */
> > diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..30e3e6dd22cbc9cfee420dde7f7f64dbdce499b9
> > --- /dev/null
> > +++ b/drivers/media/usb/uvc/uvc_gpio.c
> > @@ -0,0 +1,123 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *      uvc_gpio.c  --  USB Video Class driver
> > + *
> > + *      Copyright 2025 Google LLC
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/gpio/consumer.h>
> > +#include "uvcvideo.h"
> > +
> > +static void uvc_gpio_event(struct uvc_device *dev)
> > +{
> > +     struct uvc_entity *unit = dev->gpio_unit;
> > +     struct uvc_video_chain *chain;
> > +     u8 new_val;
> > +
> > +     if (!unit)
> > +             return;
> > +
> > +     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > +
> > +     /* GPIO entities are always on the first chain. */
> > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > +     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > +}
> > +
> > +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > +                         u8 cs, void *data, u16 size)
> > +{
> > +     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > +             return -EINVAL;
> > +
> > +     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > +
> > +     return 0;
> > +}
> > +
> > +static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > +                          u8 cs, u8 *caps)
> > +{
> > +     if (cs != UVC_CT_PRIVACY_CONTROL)
> > +             return -EINVAL;
> > +
> > +     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > +     return 0;
> > +}
> > +
> > +static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > +{
> > +     struct uvc_device *dev = data;
> > +
> > +     uvc_gpio_event(dev);
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +int uvc_gpio_parse(struct uvc_device *dev)
> > +{
> > +     struct uvc_entity *unit;
> > +     struct gpio_desc *gpio_privacy;
> > +     int irq;
> > +
> > +     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > +                                            GPIOD_IN);
> > +     if (!gpio_privacy)
> > +             return 0;
> > +
> > +     if (IS_ERR(gpio_privacy))
> > +             return dev_err_probe(&dev->intf->dev,
> > +                                  PTR_ERR(gpio_privacy),
> > +                                  "Can't get privacy GPIO\n");
> > +
> > +     irq = gpiod_to_irq(gpio_privacy);
> > +     if (irq < 0)
> > +             return dev_err_probe(&dev->intf->dev, irq,
> > +                                  "No IRQ for privacy GPIO\n");
> > +
> > +     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > +     if (!unit)
> > +             return -ENOMEM;
> > +
> > +     unit->gpio.gpio_privacy = gpio_privacy;
> > +     unit->gpio.irq = irq;
> > +     unit->gpio.bControlSize = 1;
> > +     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > +     unit->gpio.bmControls[0] = 1;
> > +     unit->get_cur = uvc_gpio_get_cur;
> > +     unit->get_info = uvc_gpio_get_info;
> > +     strscpy(unit->name, "GPIO", sizeof(unit->name));
> > +
> > +     list_add_tail(&unit->list, &dev->entities);
> > +
> > +     dev->gpio_unit = unit;
> > +
> > +     return 0;
> > +}
> > +
> > +int uvc_gpio_init_irq(struct uvc_device *dev)
> > +{
> > +     struct uvc_entity *unit = dev->gpio_unit;
> > +     int ret;
> > +
> > +     if (!unit || unit->gpio.irq < 0)
> > +             return 0;
> > +
> > +     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > +                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > +                                IRQF_TRIGGER_RISING,
> > +                                "uvc_privacy_gpio", dev);
> > +
> > +     unit->gpio.initialized = !ret;
> > +
> > +     return ret;
> > +}
> > +
> > +void uvc_gpio_deinit(struct uvc_device *dev)
> > +{
> > +     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > +             return;
> > +
> > +     free_irq(dev->gpio_unit->gpio.irq, dev);
> > +}
> > +
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..aef96b96499ce09ffa286c51793482afd9832097 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -683,6 +683,8 @@ do {                                                                      \
> >   */
> >
> >  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> > +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
> > +                                 unsigned int extra_size);
> >
> >  /* Video buffers queue management. */
> >  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type);
> > @@ -829,4 +831,8 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
> >  size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
> >                           size_t size);
> >
> > +/* gpio */
> > +int uvc_gpio_parse(struct uvc_device *dev);
> > +int uvc_gpio_init_irq(struct uvc_device *dev);
> > +void uvc_gpio_deinit(struct uvc_device *dev);
> >  #endif
> >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH 6/8] media: uvcvideo: Factor out gpio functions to its own file

[Laurent Pinchart]

On Wed, Apr 23, 2025 at 06:20:09AM +0800, Ricardo Ribalda wrote:
> Hi Laurent
> 
> On Wed, 23 Apr 2025 at 05:28, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Ricardo,
> >
> > Thank you for the patch.
> >
> > Hans raised an issue on "[PATCH v3 2/8] media: uvcvideo: Factor out gpio
> > functions to its own file" (part of "[PATCH v3 0/8] media: uvcvideo:
> > Implement the Privacy GPIO as a evdev"), asking if GPIO handling should
> > still use a uvc_entity if it moves to a evdev. There are implications on
> > this series too. Unless I'm mistaken, I haven't seen a reply from you to
> > my last e-mail. Can we please first finish that discussion ?
> 
> Are you referring to:
> https://lore.kernel.org/all/0dfb780b-f2dc-43ed-a67d-afd5f50bb88f@redhat.com/
> ?

I was referring to https://lore.kernel.org/all/20241125214523.GW19381@pendragon.ideasonboard.com/

> Is it an open discussion? I believe we agreed to remove the uvc_entity.
> 
> I posted v4 back in november. In that version I remove completely the
> uvc_entity:
> https://patchwork.linuxtv.org/project/linux-media/patch/20241125-uvc-subdev-v4-6-51e040599f1a@chromium.org/

Should I review that series first ?

> > On Thu, Apr 03, 2025 at 07:16:17PM +0000, Ricardo Ribalda wrote:
> > > This is just a refactor patch, no new functionality is added.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/Makefile     |   3 +-
> > >  drivers/media/usb/uvc/uvc_driver.c | 121 +-----------------------------------
> > >  drivers/media/usb/uvc/uvc_gpio.c   | 123 +++++++++++++++++++++++++++++++++++++
> > >  drivers/media/usb/uvc/uvcvideo.h   |   6 ++
> > >  4 files changed, 133 insertions(+), 120 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> > > index 4f9eee4f81ab6436a8b90324a688a149b2c3bcd1..85514b6e538fbb8284e574ca14700f2d749e1a2e 100644
> > > --- a/drivers/media/usb/uvc/Makefile
> > > +++ b/drivers/media/usb/uvc/Makefile
> > > @@ -1,6 +1,7 @@
> > >  # SPDX-License-Identifier: GPL-2.0
> > >  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
> > > -               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
> > > +               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
> > > +               uvc_gpio.o
> > >  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
> > >  uvcvideo-objs  += uvc_entity.o
> > >  endif
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index da24a655ab68cc0957762f2b67387677c22224d1..b52e1ff401e24f69b867b5e975cda4260463e760 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -8,7 +8,6 @@
> > >
> > >  #include <linux/atomic.h>
> > >  #include <linux/bits.h>
> > > -#include <linux/gpio/consumer.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/list.h>
> > >  #include <linux/module.h>
> > > @@ -792,8 +791,8 @@ static const u8 uvc_media_transport_input_guid[16] =
> > >       UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> > >  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> > >
> > > -static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
> > > -             unsigned int num_pads, unsigned int extra_size)
> > > +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
> > > +                                 unsigned int extra_size)
> > >  {
> > >       struct uvc_entity *entity;
> > >       unsigned int num_inputs;
> > > @@ -1242,122 +1241,6 @@ static int uvc_parse_control(struct uvc_device *dev)
> > >       return 0;
> > >  }
> > >
> > > -/* -----------------------------------------------------------------------------
> > > - * Privacy GPIO
> > > - */
> > > -
> > > -static void uvc_gpio_event(struct uvc_device *dev)
> > > -{
> > > -     struct uvc_entity *unit = dev->gpio_unit;
> > > -     struct uvc_video_chain *chain;
> > > -     u8 new_val;
> > > -
> > > -     if (!unit)
> > > -             return;
> > > -
> > > -     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > > -
> > > -     /* GPIO entities are always on the first chain. */
> > > -     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > -     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > > -}
> > > -
> > > -static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > > -                         u8 cs, void *data, u16 size)
> > > -{
> > > -     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > > -             return -EINVAL;
> > > -
> > > -     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > > -
> > > -     return 0;
> > > -}
> > > -
> > > -static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > > -                          u8 cs, u8 *caps)
> > > -{
> > > -     if (cs != UVC_CT_PRIVACY_CONTROL)
> > > -             return -EINVAL;
> > > -
> > > -     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > > -     return 0;
> > > -}
> > > -
> > > -static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > > -{
> > > -     struct uvc_device *dev = data;
> > > -
> > > -     uvc_gpio_event(dev);
> > > -     return IRQ_HANDLED;
> > > -}
> > > -
> > > -static int uvc_gpio_parse(struct uvc_device *dev)
> > > -{
> > > -     struct uvc_entity *unit;
> > > -     struct gpio_desc *gpio_privacy;
> > > -     int irq;
> > > -
> > > -     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > > -                                            GPIOD_IN);
> > > -     if (!gpio_privacy)
> > > -             return 0;
> > > -
> > > -     if (IS_ERR(gpio_privacy))
> > > -             return dev_err_probe(&dev->intf->dev,
> > > -                                  PTR_ERR(gpio_privacy),
> > > -                                  "Can't get privacy GPIO\n");
> > > -
> > > -     irq = gpiod_to_irq(gpio_privacy);
> > > -     if (irq < 0)
> > > -             return dev_err_probe(&dev->intf->dev, irq,
> > > -                                  "No IRQ for privacy GPIO\n");
> > > -
> > > -     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > > -     if (!unit)
> > > -             return -ENOMEM;
> > > -
> > > -     unit->gpio.gpio_privacy = gpio_privacy;
> > > -     unit->gpio.irq = irq;
> > > -     unit->gpio.bControlSize = 1;
> > > -     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > > -     unit->gpio.bmControls[0] = 1;
> > > -     unit->get_cur = uvc_gpio_get_cur;
> > > -     unit->get_info = uvc_gpio_get_info;
> > > -     strscpy(unit->name, "GPIO", sizeof(unit->name));
> > > -
> > > -     list_add_tail(&unit->list, &dev->entities);
> > > -
> > > -     dev->gpio_unit = unit;
> > > -
> > > -     return 0;
> > > -}
> > > -
> > > -static int uvc_gpio_init_irq(struct uvc_device *dev)
> > > -{
> > > -     struct uvc_entity *unit = dev->gpio_unit;
> > > -     int ret;
> > > -
> > > -     if (!unit || unit->gpio.irq < 0)
> > > -             return 0;
> > > -
> > > -     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > > -                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > > -                                IRQF_TRIGGER_RISING,
> > > -                                "uvc_privacy_gpio", dev);
> > > -
> > > -     unit->gpio.initialized = !ret;
> > > -
> > > -     return ret;
> > > -}
> > > -
> > > -static void uvc_gpio_deinit(struct uvc_device *dev)
> > > -{
> > > -     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > > -             return;
> > > -
> > > -     free_irq(dev->gpio_unit->gpio.irq, dev);
> > > -}
> > > -
> > >  /* ------------------------------------------------------------------------
> > >   * UVC device scan
> > >   */
> > > diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..30e3e6dd22cbc9cfee420dde7f7f64dbdce499b9
> > > --- /dev/null
> > > +++ b/drivers/media/usb/uvc/uvc_gpio.c
> > > @@ -0,0 +1,123 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + *      uvc_gpio.c  --  USB Video Class driver
> > > + *
> > > + *      Copyright 2025 Google LLC
> > > + */
> > > +
> > > +#include <linux/kernel.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include "uvcvideo.h"
> > > +
> > > +static void uvc_gpio_event(struct uvc_device *dev)
> > > +{
> > > +     struct uvc_entity *unit = dev->gpio_unit;
> > > +     struct uvc_video_chain *chain;
> > > +     u8 new_val;
> > > +
> > > +     if (!unit)
> > > +             return;
> > > +
> > > +     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > > +
> > > +     /* GPIO entities are always on the first chain. */
> > > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > +     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > > +}
> > > +
> > > +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > > +                         u8 cs, void *data, u16 size)
> > > +{
> > > +     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > > +             return -EINVAL;
> > > +
> > > +     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > > +                          u8 cs, u8 *caps)
> > > +{
> > > +     if (cs != UVC_CT_PRIVACY_CONTROL)
> > > +             return -EINVAL;
> > > +
> > > +     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > > +     return 0;
> > > +}
> > > +
> > > +static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > > +{
> > > +     struct uvc_device *dev = data;
> > > +
> > > +     uvc_gpio_event(dev);
> > > +     return IRQ_HANDLED;
> > > +}
> > > +
> > > +int uvc_gpio_parse(struct uvc_device *dev)
> > > +{
> > > +     struct uvc_entity *unit;
> > > +     struct gpio_desc *gpio_privacy;
> > > +     int irq;
> > > +
> > > +     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > > +                                            GPIOD_IN);
> > > +     if (!gpio_privacy)
> > > +             return 0;
> > > +
> > > +     if (IS_ERR(gpio_privacy))
> > > +             return dev_err_probe(&dev->intf->dev,
> > > +                                  PTR_ERR(gpio_privacy),
> > > +                                  "Can't get privacy GPIO\n");
> > > +
> > > +     irq = gpiod_to_irq(gpio_privacy);
> > > +     if (irq < 0)
> > > +             return dev_err_probe(&dev->intf->dev, irq,
> > > +                                  "No IRQ for privacy GPIO\n");
> > > +
> > > +     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > > +     if (!unit)
> > > +             return -ENOMEM;
> > > +
> > > +     unit->gpio.gpio_privacy = gpio_privacy;
> > > +     unit->gpio.irq = irq;
> > > +     unit->gpio.bControlSize = 1;
> > > +     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > > +     unit->gpio.bmControls[0] = 1;
> > > +     unit->get_cur = uvc_gpio_get_cur;
> > > +     unit->get_info = uvc_gpio_get_info;
> > > +     strscpy(unit->name, "GPIO", sizeof(unit->name));
> > > +
> > > +     list_add_tail(&unit->list, &dev->entities);
> > > +
> > > +     dev->gpio_unit = unit;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +int uvc_gpio_init_irq(struct uvc_device *dev)
> > > +{
> > > +     struct uvc_entity *unit = dev->gpio_unit;
> > > +     int ret;
> > > +
> > > +     if (!unit || unit->gpio.irq < 0)
> > > +             return 0;
> > > +
> > > +     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > > +                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > > +                                IRQF_TRIGGER_RISING,
> > > +                                "uvc_privacy_gpio", dev);
> > > +
> > > +     unit->gpio.initialized = !ret;
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +void uvc_gpio_deinit(struct uvc_device *dev)
> > > +{
> > > +     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > > +             return;
> > > +
> > > +     free_irq(dev->gpio_unit->gpio.irq, dev);
> > > +}
> > > +
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..aef96b96499ce09ffa286c51793482afd9832097 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -683,6 +683,8 @@ do {                                                                      \
> > >   */
> > >
> > >  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> > > +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
> > > +                                 unsigned int extra_size);
> > >
> > >  /* Video buffers queue management. */
> > >  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type);
> > > @@ -829,4 +831,8 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
> > >  size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
> > >                           size_t size);
> > >
> > > +/* gpio */
> > > +int uvc_gpio_parse(struct uvc_device *dev);
> > > +int uvc_gpio_init_irq(struct uvc_device *dev);
> > > +void uvc_gpio_deinit(struct uvc_device *dev);
> > >  #endif
> > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] media: uvcvideo: Factor out gpio functions to its own file

[Ricardo Ribalda]

On Wed, 23 Apr 2025 at 06:25, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Apr 23, 2025 at 06:20:09AM +0800, Ricardo Ribalda wrote:
> > Hi Laurent
> >
> > On Wed, 23 Apr 2025 at 05:28, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > Thank you for the patch.
> > >
> > > Hans raised an issue on "[PATCH v3 2/8] media: uvcvideo: Factor out gpio
> > > functions to its own file" (part of "[PATCH v3 0/8] media: uvcvideo:
> > > Implement the Privacy GPIO as a evdev"), asking if GPIO handling should
> > > still use a uvc_entity if it moves to a evdev. There are implications on
> > > this series too. Unless I'm mistaken, I haven't seen a reply from you to
> > > my last e-mail. Can we please first finish that discussion ?
> >
> > Are you referring to:
> > https://lore.kernel.org/all/0dfb780b-f2dc-43ed-a67d-afd5f50bb88f@redhat.com/
> > ?
>
> I was referring to https://lore.kernel.org/all/20241125214523.GW19381@pendragon.ideasonboard.com/

I believe the three of us agreed to remove the entity. Am I missing something?

>
> > Is it an open discussion? I believe we agreed to remove the uvc_entity.
> >
> > I posted v4 back in november. In that version I remove completely the
> > uvc_entity:
> > https://patchwork.linuxtv.org/project/linux-media/patch/20241125-uvc-subdev-v4-6-51e040599f1a@chromium.org/
>
> Should I review that series first ?

It would be nicer if you follow the order of what Hans already merged
in the uvc tree. And if we *need* a change, post it on top of it
(unless it is a bug).

HdG and I have gone through multiple revisions, tested it, solved
conflicts, and reviewed merges.  CodeStyles and nice-to-have are
*very* painful to handle this late in the review cycle.

Regards!

>
> > > On Thu, Apr 03, 2025 at 07:16:17PM +0000, Ricardo Ribalda wrote:
> > > > This is just a refactor patch, no new functionality is added.
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/Makefile     |   3 +-
> > > >  drivers/media/usb/uvc/uvc_driver.c | 121 +-----------------------------------
> > > >  drivers/media/usb/uvc/uvc_gpio.c   | 123 +++++++++++++++++++++++++++++++++++++
> > > >  drivers/media/usb/uvc/uvcvideo.h   |   6 ++
> > > >  4 files changed, 133 insertions(+), 120 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> > > > index 4f9eee4f81ab6436a8b90324a688a149b2c3bcd1..85514b6e538fbb8284e574ca14700f2d749e1a2e 100644
> > > > --- a/drivers/media/usb/uvc/Makefile
> > > > +++ b/drivers/media/usb/uvc/Makefile
> > > > @@ -1,6 +1,7 @@
> > > >  # SPDX-License-Identifier: GPL-2.0
> > > >  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
> > > > -               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
> > > > +               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
> > > > +               uvc_gpio.o
> > > >  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
> > > >  uvcvideo-objs  += uvc_entity.o
> > > >  endif
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index da24a655ab68cc0957762f2b67387677c22224d1..b52e1ff401e24f69b867b5e975cda4260463e760 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -8,7 +8,6 @@
> > > >
> > > >  #include <linux/atomic.h>
> > > >  #include <linux/bits.h>
> > > > -#include <linux/gpio/consumer.h>
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/list.h>
> > > >  #include <linux/module.h>
> > > > @@ -792,8 +791,8 @@ static const u8 uvc_media_transport_input_guid[16] =
> > > >       UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> > > >  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> > > >
> > > > -static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
> > > > -             unsigned int num_pads, unsigned int extra_size)
> > > > +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
> > > > +                                 unsigned int extra_size)
> > > >  {
> > > >       struct uvc_entity *entity;
> > > >       unsigned int num_inputs;
> > > > @@ -1242,122 +1241,6 @@ static int uvc_parse_control(struct uvc_device *dev)
> > > >       return 0;
> > > >  }
> > > >
> > > > -/* -----------------------------------------------------------------------------
> > > > - * Privacy GPIO
> > > > - */
> > > > -
> > > > -static void uvc_gpio_event(struct uvc_device *dev)
> > > > -{
> > > > -     struct uvc_entity *unit = dev->gpio_unit;
> > > > -     struct uvc_video_chain *chain;
> > > > -     u8 new_val;
> > > > -
> > > > -     if (!unit)
> > > > -             return;
> > > > -
> > > > -     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > > > -
> > > > -     /* GPIO entities are always on the first chain. */
> > > > -     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > > -     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > > > -}
> > > > -
> > > > -static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > > > -                         u8 cs, void *data, u16 size)
> > > > -{
> > > > -     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > > > -             return -EINVAL;
> > > > -
> > > > -     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > > > -
> > > > -     return 0;
> > > > -}
> > > > -
> > > > -static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > > > -                          u8 cs, u8 *caps)
> > > > -{
> > > > -     if (cs != UVC_CT_PRIVACY_CONTROL)
> > > > -             return -EINVAL;
> > > > -
> > > > -     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > > > -     return 0;
> > > > -}
> > > > -
> > > > -static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > > > -{
> > > > -     struct uvc_device *dev = data;
> > > > -
> > > > -     uvc_gpio_event(dev);
> > > > -     return IRQ_HANDLED;
> > > > -}
> > > > -
> > > > -static int uvc_gpio_parse(struct uvc_device *dev)
> > > > -{
> > > > -     struct uvc_entity *unit;
> > > > -     struct gpio_desc *gpio_privacy;
> > > > -     int irq;
> > > > -
> > > > -     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > > > -                                            GPIOD_IN);
> > > > -     if (!gpio_privacy)
> > > > -             return 0;
> > > > -
> > > > -     if (IS_ERR(gpio_privacy))
> > > > -             return dev_err_probe(&dev->intf->dev,
> > > > -                                  PTR_ERR(gpio_privacy),
> > > > -                                  "Can't get privacy GPIO\n");
> > > > -
> > > > -     irq = gpiod_to_irq(gpio_privacy);
> > > > -     if (irq < 0)
> > > > -             return dev_err_probe(&dev->intf->dev, irq,
> > > > -                                  "No IRQ for privacy GPIO\n");
> > > > -
> > > > -     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > > > -     if (!unit)
> > > > -             return -ENOMEM;
> > > > -
> > > > -     unit->gpio.gpio_privacy = gpio_privacy;
> > > > -     unit->gpio.irq = irq;
> > > > -     unit->gpio.bControlSize = 1;
> > > > -     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > > > -     unit->gpio.bmControls[0] = 1;
> > > > -     unit->get_cur = uvc_gpio_get_cur;
> > > > -     unit->get_info = uvc_gpio_get_info;
> > > > -     strscpy(unit->name, "GPIO", sizeof(unit->name));
> > > > -
> > > > -     list_add_tail(&unit->list, &dev->entities);
> > > > -
> > > > -     dev->gpio_unit = unit;
> > > > -
> > > > -     return 0;
> > > > -}
> > > > -
> > > > -static int uvc_gpio_init_irq(struct uvc_device *dev)
> > > > -{
> > > > -     struct uvc_entity *unit = dev->gpio_unit;
> > > > -     int ret;
> > > > -
> > > > -     if (!unit || unit->gpio.irq < 0)
> > > > -             return 0;
> > > > -
> > > > -     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > > > -                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > > > -                                IRQF_TRIGGER_RISING,
> > > > -                                "uvc_privacy_gpio", dev);
> > > > -
> > > > -     unit->gpio.initialized = !ret;
> > > > -
> > > > -     return ret;
> > > > -}
> > > > -
> > > > -static void uvc_gpio_deinit(struct uvc_device *dev)
> > > > -{
> > > > -     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > > > -             return;
> > > > -
> > > > -     free_irq(dev->gpio_unit->gpio.irq, dev);
> > > > -}
> > > > -
> > > >  /* ------------------------------------------------------------------------
> > > >   * UVC device scan
> > > >   */
> > > > diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..30e3e6dd22cbc9cfee420dde7f7f64dbdce499b9
> > > > --- /dev/null
> > > > +++ b/drivers/media/usb/uvc/uvc_gpio.c
> > > > @@ -0,0 +1,123 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/*
> > > > + *      uvc_gpio.c  --  USB Video Class driver
> > > > + *
> > > > + *      Copyright 2025 Google LLC
> > > > + */
> > > > +
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include "uvcvideo.h"
> > > > +
> > > > +static void uvc_gpio_event(struct uvc_device *dev)
> > > > +{
> > > > +     struct uvc_entity *unit = dev->gpio_unit;
> > > > +     struct uvc_video_chain *chain;
> > > > +     u8 new_val;
> > > > +
> > > > +     if (!unit)
> > > > +             return;
> > > > +
> > > > +     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > > > +
> > > > +     /* GPIO entities are always on the first chain. */
> > > > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > > +     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > > > +}
> > > > +
> > > > +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > > > +                         u8 cs, void *data, u16 size)
> > > > +{
> > > > +     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > > > +             return -EINVAL;
> > > > +
> > > > +     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > > > +                          u8 cs, u8 *caps)
> > > > +{
> > > > +     if (cs != UVC_CT_PRIVACY_CONTROL)
> > > > +             return -EINVAL;
> > > > +
> > > > +     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > > > +{
> > > > +     struct uvc_device *dev = data;
> > > > +
> > > > +     uvc_gpio_event(dev);
> > > > +     return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +int uvc_gpio_parse(struct uvc_device *dev)
> > > > +{
> > > > +     struct uvc_entity *unit;
> > > > +     struct gpio_desc *gpio_privacy;
> > > > +     int irq;
> > > > +
> > > > +     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > > > +                                            GPIOD_IN);
> > > > +     if (!gpio_privacy)
> > > > +             return 0;
> > > > +
> > > > +     if (IS_ERR(gpio_privacy))
> > > > +             return dev_err_probe(&dev->intf->dev,
> > > > +                                  PTR_ERR(gpio_privacy),
> > > > +                                  "Can't get privacy GPIO\n");
> > > > +
> > > > +     irq = gpiod_to_irq(gpio_privacy);
> > > > +     if (irq < 0)
> > > > +             return dev_err_probe(&dev->intf->dev, irq,
> > > > +                                  "No IRQ for privacy GPIO\n");
> > > > +
> > > > +     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > > > +     if (!unit)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     unit->gpio.gpio_privacy = gpio_privacy;
> > > > +     unit->gpio.irq = irq;
> > > > +     unit->gpio.bControlSize = 1;
> > > > +     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > > > +     unit->gpio.bmControls[0] = 1;
> > > > +     unit->get_cur = uvc_gpio_get_cur;
> > > > +     unit->get_info = uvc_gpio_get_info;
> > > > +     strscpy(unit->name, "GPIO", sizeof(unit->name));
> > > > +
> > > > +     list_add_tail(&unit->list, &dev->entities);
> > > > +
> > > > +     dev->gpio_unit = unit;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +int uvc_gpio_init_irq(struct uvc_device *dev)
> > > > +{
> > > > +     struct uvc_entity *unit = dev->gpio_unit;
> > > > +     int ret;
> > > > +
> > > > +     if (!unit || unit->gpio.irq < 0)
> > > > +             return 0;
> > > > +
> > > > +     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > > > +                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > > > +                                IRQF_TRIGGER_RISING,
> > > > +                                "uvc_privacy_gpio", dev);
> > > > +
> > > > +     unit->gpio.initialized = !ret;
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +void uvc_gpio_deinit(struct uvc_device *dev)
> > > > +{
> > > > +     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > > > +             return;
> > > > +
> > > > +     free_irq(dev->gpio_unit->gpio.irq, dev);
> > > > +}
> > > > +
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..aef96b96499ce09ffa286c51793482afd9832097 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -683,6 +683,8 @@ do {                                                                      \
> > > >   */
> > > >
> > > >  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> > > > +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
> > > > +                                 unsigned int extra_size);
> > > >
> > > >  /* Video buffers queue management. */
> > > >  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type);
> > > > @@ -829,4 +831,8 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
> > > >  size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
> > > >                           size_t size);
> > > >
> > > > +/* gpio */
> > > > +int uvc_gpio_parse(struct uvc_device *dev);
> > > > +int uvc_gpio_init_irq(struct uvc_device *dev);
> > > > +void uvc_gpio_deinit(struct uvc_device *dev);
> > > >  #endif
> > > >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH 6/8] media: uvcvideo: Factor out gpio functions to its own file

[Laurent Pinchart]

On Wed, Apr 23, 2025 at 06:35:08AM +0800, Ricardo Ribalda wrote:
> On Wed, 23 Apr 2025 at 06:25, Laurent Pinchart wrote:
> > On Wed, Apr 23, 2025 at 06:20:09AM +0800, Ricardo Ribalda wrote:
> > > On Wed, 23 Apr 2025 at 05:28, Laurent Pinchart wrote:
> > > >
> > > > Hi Ricardo,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > Hans raised an issue on "[PATCH v3 2/8] media: uvcvideo: Factor out gpio
> > > > functions to its own file" (part of "[PATCH v3 0/8] media: uvcvideo:
> > > > Implement the Privacy GPIO as a evdev"), asking if GPIO handling should
> > > > still use a uvc_entity if it moves to a evdev. There are implications on
> > > > this series too. Unless I'm mistaken, I haven't seen a reply from you to
> > > > my last e-mail. Can we please first finish that discussion ?
> > >
> > > Are you referring to:
> > > https://lore.kernel.org/all/0dfb780b-f2dc-43ed-a67d-afd5f50bb88f@redhat.com/
> > > ?
> >
> > I was referring to https://lore.kernel.org/all/20241125214523.GW19381@pendragon.ideasonboard.com/
> 
> I believe the three of us agreed to remove the entity. Am I missing something?

I'll re-read the mail thread.

> > > Is it an open discussion? I believe we agreed to remove the uvc_entity.
> > >
> > > I posted v4 back in november. In that version I remove completely the
> > > uvc_entity:
> > > https://patchwork.linuxtv.org/project/linux-media/patch/20241125-uvc-subdev-v4-6-51e040599f1a@chromium.org/
> >
> > Should I review that series first ?
> 
> It would be nicer if you follow the order of what Hans already merged
> in the uvc tree. And if we *need* a change, post it on top of it
> (unless it is a bug).
> 
> HdG and I have gone through multiple revisions, tested it, solved
> conflicts, and reviewed merges.  CodeStyles and nice-to-have are
> *very* painful to handle this late in the review cycle.

Could you please reply to the review comments ? As I wrote in a separate
e-mail, we can minimize the need for respins, possibly with patches on
top for some of the issues, but I'd like to know what problems need to
be fixed.

> > > > On Thu, Apr 03, 2025 at 07:16:17PM +0000, Ricardo Ribalda wrote:
> > > > > This is just a refactor patch, no new functionality is added.
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > >  drivers/media/usb/uvc/Makefile     |   3 +-
> > > > >  drivers/media/usb/uvc/uvc_driver.c | 121 +-----------------------------------
> > > > >  drivers/media/usb/uvc/uvc_gpio.c   | 123 +++++++++++++++++++++++++++++++++++++
> > > > >  drivers/media/usb/uvc/uvcvideo.h   |   6 ++
> > > > >  4 files changed, 133 insertions(+), 120 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> > > > > index 4f9eee4f81ab6436a8b90324a688a149b2c3bcd1..85514b6e538fbb8284e574ca14700f2d749e1a2e 100644
> > > > > --- a/drivers/media/usb/uvc/Makefile
> > > > > +++ b/drivers/media/usb/uvc/Makefile
> > > > > @@ -1,6 +1,7 @@
> > > > >  # SPDX-License-Identifier: GPL-2.0
> > > > >  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
> > > > > -               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
> > > > > +               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
> > > > > +               uvc_gpio.o
> > > > >  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
> > > > >  uvcvideo-objs  += uvc_entity.o
> > > > >  endif
> > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > index da24a655ab68cc0957762f2b67387677c22224d1..b52e1ff401e24f69b867b5e975cda4260463e760 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > @@ -8,7 +8,6 @@
> > > > >
> > > > >  #include <linux/atomic.h>
> > > > >  #include <linux/bits.h>
> > > > > -#include <linux/gpio/consumer.h>
> > > > >  #include <linux/kernel.h>
> > > > >  #include <linux/list.h>
> > > > >  #include <linux/module.h>
> > > > > @@ -792,8 +791,8 @@ static const u8 uvc_media_transport_input_guid[16] =
> > > > >       UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> > > > >  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> > > > >
> > > > > -static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
> > > > > -             unsigned int num_pads, unsigned int extra_size)
> > > > > +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
> > > > > +                                 unsigned int extra_size)
> > > > >  {
> > > > >       struct uvc_entity *entity;
> > > > >       unsigned int num_inputs;
> > > > > @@ -1242,122 +1241,6 @@ static int uvc_parse_control(struct uvc_device *dev)
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > -/* -----------------------------------------------------------------------------
> > > > > - * Privacy GPIO
> > > > > - */
> > > > > -
> > > > > -static void uvc_gpio_event(struct uvc_device *dev)
> > > > > -{
> > > > > -     struct uvc_entity *unit = dev->gpio_unit;
> > > > > -     struct uvc_video_chain *chain;
> > > > > -     u8 new_val;
> > > > > -
> > > > > -     if (!unit)
> > > > > -             return;
> > > > > -
> > > > > -     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > > > > -
> > > > > -     /* GPIO entities are always on the first chain. */
> > > > > -     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > > > -     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > > > > -}
> > > > > -
> > > > > -static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > > > > -                         u8 cs, void *data, u16 size)
> > > > > -{
> > > > > -     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > > > > -             return -EINVAL;
> > > > > -
> > > > > -     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > > > > -
> > > > > -     return 0;
> > > > > -}
> > > > > -
> > > > > -static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > > > > -                          u8 cs, u8 *caps)
> > > > > -{
> > > > > -     if (cs != UVC_CT_PRIVACY_CONTROL)
> > > > > -             return -EINVAL;
> > > > > -
> > > > > -     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > > > > -     return 0;
> > > > > -}
> > > > > -
> > > > > -static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > > > > -{
> > > > > -     struct uvc_device *dev = data;
> > > > > -
> > > > > -     uvc_gpio_event(dev);
> > > > > -     return IRQ_HANDLED;
> > > > > -}
> > > > > -
> > > > > -static int uvc_gpio_parse(struct uvc_device *dev)
> > > > > -{
> > > > > -     struct uvc_entity *unit;
> > > > > -     struct gpio_desc *gpio_privacy;
> > > > > -     int irq;
> > > > > -
> > > > > -     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > > > > -                                            GPIOD_IN);
> > > > > -     if (!gpio_privacy)
> > > > > -             return 0;
> > > > > -
> > > > > -     if (IS_ERR(gpio_privacy))
> > > > > -             return dev_err_probe(&dev->intf->dev,
> > > > > -                                  PTR_ERR(gpio_privacy),
> > > > > -                                  "Can't get privacy GPIO\n");
> > > > > -
> > > > > -     irq = gpiod_to_irq(gpio_privacy);
> > > > > -     if (irq < 0)
> > > > > -             return dev_err_probe(&dev->intf->dev, irq,
> > > > > -                                  "No IRQ for privacy GPIO\n");
> > > > > -
> > > > > -     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > > > > -     if (!unit)
> > > > > -             return -ENOMEM;
> > > > > -
> > > > > -     unit->gpio.gpio_privacy = gpio_privacy;
> > > > > -     unit->gpio.irq = irq;
> > > > > -     unit->gpio.bControlSize = 1;
> > > > > -     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > > > > -     unit->gpio.bmControls[0] = 1;
> > > > > -     unit->get_cur = uvc_gpio_get_cur;
> > > > > -     unit->get_info = uvc_gpio_get_info;
> > > > > -     strscpy(unit->name, "GPIO", sizeof(unit->name));
> > > > > -
> > > > > -     list_add_tail(&unit->list, &dev->entities);
> > > > > -
> > > > > -     dev->gpio_unit = unit;
> > > > > -
> > > > > -     return 0;
> > > > > -}
> > > > > -
> > > > > -static int uvc_gpio_init_irq(struct uvc_device *dev)
> > > > > -{
> > > > > -     struct uvc_entity *unit = dev->gpio_unit;
> > > > > -     int ret;
> > > > > -
> > > > > -     if (!unit || unit->gpio.irq < 0)
> > > > > -             return 0;
> > > > > -
> > > > > -     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > > > > -                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > > > > -                                IRQF_TRIGGER_RISING,
> > > > > -                                "uvc_privacy_gpio", dev);
> > > > > -
> > > > > -     unit->gpio.initialized = !ret;
> > > > > -
> > > > > -     return ret;
> > > > > -}
> > > > > -
> > > > > -static void uvc_gpio_deinit(struct uvc_device *dev)
> > > > > -{
> > > > > -     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > > > > -             return;
> > > > > -
> > > > > -     free_irq(dev->gpio_unit->gpio.irq, dev);
> > > > > -}
> > > > > -
> > > > >  /* ------------------------------------------------------------------------
> > > > >   * UVC device scan
> > > > >   */
> > > > > diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
> > > > > new file mode 100644
> > > > > index 0000000000000000000000000000000000000000..30e3e6dd22cbc9cfee420dde7f7f64dbdce499b9
> > > > > --- /dev/null
> > > > > +++ b/drivers/media/usb/uvc/uvc_gpio.c
> > > > > @@ -0,0 +1,123 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > +/*
> > > > > + *      uvc_gpio.c  --  USB Video Class driver
> > > > > + *
> > > > > + *      Copyright 2025 Google LLC
> > > > > + */
> > > > > +
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > > +#include "uvcvideo.h"
> > > > > +
> > > > > +static void uvc_gpio_event(struct uvc_device *dev)
> > > > > +{
> > > > > +     struct uvc_entity *unit = dev->gpio_unit;
> > > > > +     struct uvc_video_chain *chain;
> > > > > +     u8 new_val;
> > > > > +
> > > > > +     if (!unit)
> > > > > +             return;
> > > > > +
> > > > > +     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> > > > > +
> > > > > +     /* GPIO entities are always on the first chain. */
> > > > > +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> > > > > +     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> > > > > +}
> > > > > +
> > > > > +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> > > > > +                         u8 cs, void *data, u16 size)
> > > > > +{
> > > > > +     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> > > > > +                          u8 cs, u8 *caps)
> > > > > +{
> > > > > +     if (cs != UVC_CT_PRIVACY_CONTROL)
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static irqreturn_t uvc_gpio_irq(int irq, void *data)
> > > > > +{
> > > > > +     struct uvc_device *dev = data;
> > > > > +
> > > > > +     uvc_gpio_event(dev);
> > > > > +     return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > > > > +int uvc_gpio_parse(struct uvc_device *dev)
> > > > > +{
> > > > > +     struct uvc_entity *unit;
> > > > > +     struct gpio_desc *gpio_privacy;
> > > > > +     int irq;
> > > > > +
> > > > > +     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> > > > > +                                            GPIOD_IN);
> > > > > +     if (!gpio_privacy)
> > > > > +             return 0;
> > > > > +
> > > > > +     if (IS_ERR(gpio_privacy))
> > > > > +             return dev_err_probe(&dev->intf->dev,
> > > > > +                                  PTR_ERR(gpio_privacy),
> > > > > +                                  "Can't get privacy GPIO\n");
> > > > > +
> > > > > +     irq = gpiod_to_irq(gpio_privacy);
> > > > > +     if (irq < 0)
> > > > > +             return dev_err_probe(&dev->intf->dev, irq,
> > > > > +                                  "No IRQ for privacy GPIO\n");
> > > > > +
> > > > > +     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > > > > +     if (!unit)
> > > > > +             return -ENOMEM;
> > > > > +
> > > > > +     unit->gpio.gpio_privacy = gpio_privacy;
> > > > > +     unit->gpio.irq = irq;
> > > > > +     unit->gpio.bControlSize = 1;
> > > > > +     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> > > > > +     unit->gpio.bmControls[0] = 1;
> > > > > +     unit->get_cur = uvc_gpio_get_cur;
> > > > > +     unit->get_info = uvc_gpio_get_info;
> > > > > +     strscpy(unit->name, "GPIO", sizeof(unit->name));
> > > > > +
> > > > > +     list_add_tail(&unit->list, &dev->entities);
> > > > > +
> > > > > +     dev->gpio_unit = unit;
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +int uvc_gpio_init_irq(struct uvc_device *dev)
> > > > > +{
> > > > > +     struct uvc_entity *unit = dev->gpio_unit;
> > > > > +     int ret;
> > > > > +
> > > > > +     if (!unit || unit->gpio.irq < 0)
> > > > > +             return 0;
> > > > > +
> > > > > +     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> > > > > +                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> > > > > +                                IRQF_TRIGGER_RISING,
> > > > > +                                "uvc_privacy_gpio", dev);
> > > > > +
> > > > > +     unit->gpio.initialized = !ret;
> > > > > +
> > > > > +     return ret;
> > > > > +}
> > > > > +
> > > > > +void uvc_gpio_deinit(struct uvc_device *dev)
> > > > > +{
> > > > > +     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> > > > > +             return;
> > > > > +
> > > > > +     free_irq(dev->gpio_unit->gpio.irq, dev);
> > > > > +}
> > > > > +
> > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > > index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..aef96b96499ce09ffa286c51793482afd9832097 100644
> > > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > > @@ -683,6 +683,8 @@ do {                                                                      \
> > > > >   */
> > > > >
> > > > >  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> > > > > +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
> > > > > +                                 unsigned int extra_size);
> > > > >
> > > > >  /* Video buffers queue management. */
> > > > >  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type);
> > > > > @@ -829,4 +831,8 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
> > > > >  size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
> > > > >                           size_t size);
> > > > >
> > > > > +/* gpio */
> > > > > +int uvc_gpio_parse(struct uvc_device *dev);
> > > > > +int uvc_gpio_init_irq(struct uvc_device *dev);
> > > > > +void uvc_gpio_deinit(struct uvc_device *dev);
> > > > >  #endif
> > > > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] media: uvcvideo: Factor out gpio functions to its own file

[Hans de Goede]

Hi,

On 23-Apr-25 00:35, Ricardo Ribalda wrote:
> On Wed, 23 Apr 2025 at 06:25, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> On Wed, Apr 23, 2025 at 06:20:09AM +0800, Ricardo Ribalda wrote:
>>> Hi Laurent
>>>
>>> On Wed, 23 Apr 2025 at 05:28, Laurent Pinchart
>>> <laurent.pinchart@ideasonboard.com> wrote:
>>>>
>>>> Hi Ricardo,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> Hans raised an issue on "[PATCH v3 2/8] media: uvcvideo: Factor out gpio
>>>> functions to its own file" (part of "[PATCH v3 0/8] media: uvcvideo:
>>>> Implement the Privacy GPIO as a evdev"), asking if GPIO handling should
>>>> still use a uvc_entity if it moves to a evdev. There are implications on
>>>> this series too. Unless I'm mistaken, I haven't seen a reply from you to
>>>> my last e-mail. Can we please first finish that discussion ?
>>>
>>> Are you referring to:
>>> https://lore.kernel.org/all/0dfb780b-f2dc-43ed-a67d-afd5f50bb88f@redhat.com/
>>> ?
>>
>> I was referring to https://lore.kernel.org/all/20241125214523.GW19381@pendragon.ideasonboard.com/
> 
> I believe the three of us agreed to remove the entity. Am I missing something?

That is what I remember too.

2 other remarks:

1. About this patch, what is this patch doing in *this* series, outside of exporting
uvc_alloc_entity(), I don't think we need this here. So for v2 I would prefer to
have this replaced with a patch just making uvc_alloc_entity() non static.

That avoids unnecessary dependencies between this series and the GPIO privacy control
use evdev series. Any conflicts from exporting uvc_alloc_entity() in this series should
be trivial to fix.

2. About the series making the GPIO privacy control use evdev, if I've understood
things correctly the main motivation for that was power-consumption reasons and with
the granular power management series sitting in uvc/next those reasons are gone ?

It would still be good to move to evdev to unify the userspace API with various
x86 laptop EC/ACPI drivers, but AFAIK this is a somewhat lower priority series to
get merged now because the power-consumption issues are resolved now, right ?

Regards,

Hans



>>>> On Thu, Apr 03, 2025 at 07:16:17PM +0000, Ricardo Ribalda wrote:
>>>>> This is just a refactor patch, no new functionality is added.
>>>>>
>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>> ---
>>>>>  drivers/media/usb/uvc/Makefile     |   3 +-
>>>>>  drivers/media/usb/uvc/uvc_driver.c | 121 +-----------------------------------
>>>>>  drivers/media/usb/uvc/uvc_gpio.c   | 123 +++++++++++++++++++++++++++++++++++++
>>>>>  drivers/media/usb/uvc/uvcvideo.h   |   6 ++
>>>>>  4 files changed, 133 insertions(+), 120 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
>>>>> index 4f9eee4f81ab6436a8b90324a688a149b2c3bcd1..85514b6e538fbb8284e574ca14700f2d749e1a2e 100644
>>>>> --- a/drivers/media/usb/uvc/Makefile
>>>>> +++ b/drivers/media/usb/uvc/Makefile
>>>>> @@ -1,6 +1,7 @@
>>>>>  # SPDX-License-Identifier: GPL-2.0
>>>>>  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
>>>>> -               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
>>>>> +               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
>>>>> +               uvc_gpio.o
>>>>>  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
>>>>>  uvcvideo-objs  += uvc_entity.o
>>>>>  endif
>>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>>>> index da24a655ab68cc0957762f2b67387677c22224d1..b52e1ff401e24f69b867b5e975cda4260463e760 100644
>>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>>>> @@ -8,7 +8,6 @@
>>>>>
>>>>>  #include <linux/atomic.h>
>>>>>  #include <linux/bits.h>
>>>>> -#include <linux/gpio/consumer.h>
>>>>>  #include <linux/kernel.h>
>>>>>  #include <linux/list.h>
>>>>>  #include <linux/module.h>
>>>>> @@ -792,8 +791,8 @@ static const u8 uvc_media_transport_input_guid[16] =
>>>>>       UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
>>>>>  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
>>>>>
>>>>> -static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
>>>>> -             unsigned int num_pads, unsigned int extra_size)
>>>>> +struct uvc_entity *u16 type, u16 id, unsigned int num_pads,
>>>>> +                                 unsigned int extra_size)
>>>>>  {
>>>>>       struct uvc_entity *entity;
>>>>>       unsigned int num_inputs;
>>>>> @@ -1242,122 +1241,6 @@ static int uvc_parse_control(struct uvc_device *dev)
>>>>>       return 0;
>>>>>  }
>>>>>
>>>>> -/* -----------------------------------------------------------------------------
>>>>> - * Privacy GPIO
>>>>> - */
>>>>> -
>>>>> -static void uvc_gpio_event(struct uvc_device *dev)
>>>>> -{
>>>>> -     struct uvc_entity *unit = dev->gpio_unit;
>>>>> -     struct uvc_video_chain *chain;
>>>>> -     u8 new_val;
>>>>> -
>>>>> -     if (!unit)
>>>>> -             return;
>>>>> -
>>>>> -     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
>>>>> -
>>>>> -     /* GPIO entities are always on the first chain. */
>>>>> -     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
>>>>> -     uvc_ctrl_status_event(chain, unit->controls, &new_val);
>>>>> -}
>>>>> -
>>>>> -static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
>>>>> -                         u8 cs, void *data, u16 size)
>>>>> -{
>>>>> -     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
>>>>> -             return -EINVAL;
>>>>> -
>>>>> -     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
>>>>> -
>>>>> -     return 0;
>>>>> -}
>>>>> -
>>>>> -static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
>>>>> -                          u8 cs, u8 *caps)
>>>>> -{
>>>>> -     if (cs != UVC_CT_PRIVACY_CONTROL)
>>>>> -             return -EINVAL;
>>>>> -
>>>>> -     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
>>>>> -     return 0;
>>>>> -}
>>>>> -
>>>>> -static irqreturn_t uvc_gpio_irq(int irq, void *data)
>>>>> -{
>>>>> -     struct uvc_device *dev = data;
>>>>> -
>>>>> -     uvc_gpio_event(dev);
>>>>> -     return IRQ_HANDLED;
>>>>> -}
>>>>> -
>>>>> -static int uvc_gpio_parse(struct uvc_device *dev)
>>>>> -{
>>>>> -     struct uvc_entity *unit;
>>>>> -     struct gpio_desc *gpio_privacy;
>>>>> -     int irq;
>>>>> -
>>>>> -     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
>>>>> -                                            GPIOD_IN);
>>>>> -     if (!gpio_privacy)
>>>>> -             return 0;
>>>>> -
>>>>> -     if (IS_ERR(gpio_privacy))
>>>>> -             return dev_err_probe(&dev->intf->dev,
>>>>> -                                  PTR_ERR(gpio_privacy),
>>>>> -                                  "Can't get privacy GPIO\n");
>>>>> -
>>>>> -     irq = gpiod_to_irq(gpio_privacy);
>>>>> -     if (irq < 0)
>>>>> -             return dev_err_probe(&dev->intf->dev, irq,
>>>>> -                                  "No IRQ for privacy GPIO\n");
>>>>> -
>>>>> -     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
>>>>> -     if (!unit)
>>>>> -             return -ENOMEM;
>>>>> -
>>>>> -     unit->gpio.gpio_privacy = gpio_privacy;
>>>>> -     unit->gpio.irq = irq;
>>>>> -     unit->gpio.bControlSize = 1;
>>>>> -     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
>>>>> -     unit->gpio.bmControls[0] = 1;
>>>>> -     unit->get_cur = uvc_gpio_get_cur;
>>>>> -     unit->get_info = uvc_gpio_get_info;
>>>>> -     strscpy(unit->name, "GPIO", sizeof(unit->name));
>>>>> -
>>>>> -     list_add_tail(&unit->list, &dev->entities);
>>>>> -
>>>>> -     dev->gpio_unit = unit;
>>>>> -
>>>>> -     return 0;
>>>>> -}
>>>>> -
>>>>> -static int uvc_gpio_init_irq(struct uvc_device *dev)
>>>>> -{
>>>>> -     struct uvc_entity *unit = dev->gpio_unit;
>>>>> -     int ret;
>>>>> -
>>>>> -     if (!unit || unit->gpio.irq < 0)
>>>>> -             return 0;
>>>>> -
>>>>> -     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
>>>>> -                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
>>>>> -                                IRQF_TRIGGER_RISING,
>>>>> -                                "uvc_privacy_gpio", dev);
>>>>> -
>>>>> -     unit->gpio.initialized = !ret;
>>>>> -
>>>>> -     return ret;
>>>>> -}
>>>>> -
>>>>> -static void uvc_gpio_deinit(struct uvc_device *dev)
>>>>> -{
>>>>> -     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
>>>>> -             return;
>>>>> -
>>>>> -     free_irq(dev->gpio_unit->gpio.irq, dev);
>>>>> -}
>>>>> -
>>>>>  /* ------------------------------------------------------------------------
>>>>>   * UVC device scan
>>>>>   */
>>>>> diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
>>>>> new file mode 100644
>>>>> index 0000000000000000000000000000000000000000..30e3e6dd22cbc9cfee420dde7f7f64dbdce499b9
>>>>> --- /dev/null
>>>>> +++ b/drivers/media/usb/uvc/uvc_gpio.c
>>>>> @@ -0,0 +1,123 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>>> +/*
>>>>> + *      uvc_gpio.c  --  USB Video Class driver
>>>>> + *
>>>>> + *      Copyright 2025 Google LLC
>>>>> + */
>>>>> +
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/gpio/consumer.h>
>>>>> +#include "uvcvideo.h"
>>>>> +
>>>>> +static void uvc_gpio_event(struct uvc_device *dev)
>>>>> +{
>>>>> +     struct uvc_entity *unit = dev->gpio_unit;
>>>>> +     struct uvc_video_chain *chain;
>>>>> +     u8 new_val;
>>>>> +
>>>>> +     if (!unit)
>>>>> +             return;
>>>>> +
>>>>> +     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
>>>>> +
>>>>> +     /* GPIO entities are always on the first chain. */
>>>>> +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
>>>>> +     uvc_ctrl_status_event(chain, unit->controls, &new_val);
>>>>> +}
>>>>> +
>>>>> +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
>>>>> +                         u8 cs, void *data, u16 size)
>>>>> +{
>>>>> +     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
>>>>> +                          u8 cs, u8 *caps)
>>>>> +{
>>>>> +     if (cs != UVC_CT_PRIVACY_CONTROL)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static irqreturn_t uvc_gpio_irq(int irq, void *data)
>>>>> +{
>>>>> +     struct uvc_device *dev = data;
>>>>> +
>>>>> +     uvc_gpio_event(dev);
>>>>> +     return IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +int uvc_gpio_parse(struct uvc_device *dev)
>>>>> +{
>>>>> +     struct uvc_entity *unit;
>>>>> +     struct gpio_desc *gpio_privacy;
>>>>> +     int irq;
>>>>> +
>>>>> +     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
>>>>> +                                            GPIOD_IN);
>>>>> +     if (!gpio_privacy)
>>>>> +             return 0;
>>>>> +
>>>>> +     if (IS_ERR(gpio_privacy))
>>>>> +             return dev_err_probe(&dev->intf->dev,
>>>>> +                                  PTR_ERR(gpio_privacy),
>>>>> +                                  "Can't get privacy GPIO\n");
>>>>> +
>>>>> +     irq = gpiod_to_irq(gpio_privacy);
>>>>> +     if (irq < 0)
>>>>> +             return dev_err_probe(&dev->intf->dev, irq,
>>>>> +                                  "No IRQ for privacy GPIO\n");
>>>>> +
>>>>> +     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
>>>>> +     if (!unit)
>>>>> +             return -ENOMEM;
>>>>> +
>>>>> +     unit->gpio.gpio_privacy = gpio_privacy;
>>>>> +     unit->gpio.irq = irq;
>>>>> +     unit->gpio.bControlSize = 1;
>>>>> +     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
>>>>> +     unit->gpio.bmControls[0] = 1;
>>>>> +     unit->get_cur = uvc_gpio_get_cur;
>>>>> +     unit->get_info = uvc_gpio_get_info;
>>>>> +     strscpy(unit->name, "GPIO", sizeof(unit->name));
>>>>> +
>>>>> +     list_add_tail(&unit->list, &dev->entities);
>>>>> +
>>>>> +     dev->gpio_unit = unit;
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +int uvc_gpio_init_irq(struct uvc_device *dev)
>>>>> +{
>>>>> +     struct uvc_entity *unit = dev->gpio_unit;
>>>>> +     int ret;
>>>>> +
>>>>> +     if (!unit || unit->gpio.irq < 0)
>>>>> +             return 0;
>>>>> +
>>>>> +     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
>>>>> +                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
>>>>> +                                IRQF_TRIGGER_RISING,
>>>>> +                                "uvc_privacy_gpio", dev);
>>>>> +
>>>>> +     unit->gpio.initialized = !ret;
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +void uvc_gpio_deinit(struct uvc_device *dev)
>>>>> +{
>>>>> +     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
>>>>> +             return;
>>>>> +
>>>>> +     free_irq(dev->gpio_unit->gpio.irq, dev);
>>>>> +}
>>>>> +
>>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>>>> index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..aef96b96499ce09ffa286c51793482afd9832097 100644
>>>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>>>> @@ -683,6 +683,8 @@ do {                                                                      \
>>>>>   */
>>>>>
>>>>>  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
>>>>> +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
>>>>> +                                 unsigned int extra_size);
>>>>>
>>>>>  /* Video buffers queue management. */
>>>>>  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type);
>>>>> @@ -829,4 +831,8 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
>>>>>  size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
>>>>>                           size_t size);
>>>>>
>>>>> +/* gpio */
>>>>> +int uvc_gpio_parse(struct uvc_device *dev);
>>>>> +int uvc_gpio_init_irq(struct uvc_device *dev);
>>>>> +void uvc_gpio_deinit(struct uvc_device *dev);
>>>>>  #endif
>>>>>
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
> 
> 
> 

Re: [PATCH 6/8] media: uvcvideo: Factor out gpio functions to its own file

[Ricardo Ribalda]

Hi Hans

On Mon, 28 Apr 2025 at 16:07, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 23-Apr-25 00:35, Ricardo Ribalda wrote:
> > On Wed, 23 Apr 2025 at 06:25, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >>
> >> On Wed, Apr 23, 2025 at 06:20:09AM +0800, Ricardo Ribalda wrote:
> >>> Hi Laurent
> >>>
> >>> On Wed, 23 Apr 2025 at 05:28, Laurent Pinchart
> >>> <laurent.pinchart@ideasonboard.com> wrote:
> >>>>
> >>>> Hi Ricardo,
> >>>>
> >>>> Thank you for the patch.
> >>>>
> >>>> Hans raised an issue on "[PATCH v3 2/8] media: uvcvideo: Factor out gpio
> >>>> functions to its own file" (part of "[PATCH v3 0/8] media: uvcvideo:
> >>>> Implement the Privacy GPIO as a evdev"), asking if GPIO handling should
> >>>> still use a uvc_entity if it moves to a evdev. There are implications on
> >>>> this series too. Unless I'm mistaken, I haven't seen a reply from you to
> >>>> my last e-mail. Can we please first finish that discussion ?
> >>>
> >>> Are you referring to:
> >>> https://lore.kernel.org/all/0dfb780b-f2dc-43ed-a67d-afd5f50bb88f@redhat.com/
> >>> ?
> >>
> >> I was referring to https://lore.kernel.org/all/20241125214523.GW19381@pendragon.ideasonboard.com/
> >
> > I believe the three of us agreed to remove the entity. Am I missing something?
>
> That is what I remember too.
>
> 2 other remarks:
>
> 1. About this patch, what is this patch doing in *this* series, outside of exporting
> uvc_alloc_entity(), I don't think we need this here. So for v2 I would prefer to
> have this replaced with a patch just making uvc_alloc_entity() non static.
>
> That avoids unnecessary dependencies between this series and the GPIO privacy control
> use evdev series. Any conflicts from exporting uvc_alloc_entity() in this series should
> be trivial to fix.

will do

>
> 2. About the series making the GPIO privacy control use evdev, if I've understood
> things correctly the main motivation for that was power-consumption reasons and with
> the granular power management series sitting in uvc/next those reasons are gone ?

For ChromeOS that was the main motivation, you are correct. But I
still see the value of unifying the userspace API.

If you want to review that set (with low prio) that would be appreciated.

Regards!

>
> It would still be good to move to evdev to unify the userspace API with various
> x86 laptop EC/ACPI drivers, but AFAIK this is a somewhat lower priority series to
> get merged now because the power-consumption issues are resolved now, right ?
>
> Regards,
>
> Hans
>
>
>
> >>>> On Thu, Apr 03, 2025 at 07:16:17PM +0000, Ricardo Ribalda wrote:
> >>>>> This is just a refactor patch, no new functionality is added.
> >>>>>
> >>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>>>> ---
> >>>>>  drivers/media/usb/uvc/Makefile     |   3 +-
> >>>>>  drivers/media/usb/uvc/uvc_driver.c | 121 +-----------------------------------
> >>>>>  drivers/media/usb/uvc/uvc_gpio.c   | 123 +++++++++++++++++++++++++++++++++++++
> >>>>>  drivers/media/usb/uvc/uvcvideo.h   |   6 ++
> >>>>>  4 files changed, 133 insertions(+), 120 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> >>>>> index 4f9eee4f81ab6436a8b90324a688a149b2c3bcd1..85514b6e538fbb8284e574ca14700f2d749e1a2e 100644
> >>>>> --- a/drivers/media/usb/uvc/Makefile
> >>>>> +++ b/drivers/media/usb/uvc/Makefile
> >>>>> @@ -1,6 +1,7 @@
> >>>>>  # SPDX-License-Identifier: GPL-2.0
> >>>>>  uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
> >>>>> -               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
> >>>>> +               uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o \
> >>>>> +               uvc_gpio.o
> >>>>>  ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
> >>>>>  uvcvideo-objs  += uvc_entity.o
> >>>>>  endif
> >>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> >>>>> index da24a655ab68cc0957762f2b67387677c22224d1..b52e1ff401e24f69b867b5e975cda4260463e760 100644
> >>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
> >>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >>>>> @@ -8,7 +8,6 @@
> >>>>>
> >>>>>  #include <linux/atomic.h>
> >>>>>  #include <linux/bits.h>
> >>>>> -#include <linux/gpio/consumer.h>
> >>>>>  #include <linux/kernel.h>
> >>>>>  #include <linux/list.h>
> >>>>>  #include <linux/module.h>
> >>>>> @@ -792,8 +791,8 @@ static const u8 uvc_media_transport_input_guid[16] =
> >>>>>       UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> >>>>>  static const u8 uvc_processing_guid[16] = UVC_GUID_UVC_PROCESSING;
> >>>>>
> >>>>> -static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
> >>>>> -             unsigned int num_pads, unsigned int extra_size)
> >>>>> +struct uvc_entity *u16 type, u16 id, unsigned int num_pads,
> >>>>> +                                 unsigned int extra_size)
> >>>>>  {
> >>>>>       struct uvc_entity *entity;
> >>>>>       unsigned int num_inputs;
> >>>>> @@ -1242,122 +1241,6 @@ static int uvc_parse_control(struct uvc_device *dev)
> >>>>>       return 0;
> >>>>>  }
> >>>>>
> >>>>> -/* -----------------------------------------------------------------------------
> >>>>> - * Privacy GPIO
> >>>>> - */
> >>>>> -
> >>>>> -static void uvc_gpio_event(struct uvc_device *dev)
> >>>>> -{
> >>>>> -     struct uvc_entity *unit = dev->gpio_unit;
> >>>>> -     struct uvc_video_chain *chain;
> >>>>> -     u8 new_val;
> >>>>> -
> >>>>> -     if (!unit)
> >>>>> -             return;
> >>>>> -
> >>>>> -     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> >>>>> -
> >>>>> -     /* GPIO entities are always on the first chain. */
> >>>>> -     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> >>>>> -     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> >>>>> -}
> >>>>> -
> >>>>> -static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> >>>>> -                         u8 cs, void *data, u16 size)
> >>>>> -{
> >>>>> -     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> >>>>> -             return -EINVAL;
> >>>>> -
> >>>>> -     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> >>>>> -
> >>>>> -     return 0;
> >>>>> -}
> >>>>> -
> >>>>> -static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> >>>>> -                          u8 cs, u8 *caps)
> >>>>> -{
> >>>>> -     if (cs != UVC_CT_PRIVACY_CONTROL)
> >>>>> -             return -EINVAL;
> >>>>> -
> >>>>> -     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> >>>>> -     return 0;
> >>>>> -}
> >>>>> -
> >>>>> -static irqreturn_t uvc_gpio_irq(int irq, void *data)
> >>>>> -{
> >>>>> -     struct uvc_device *dev = data;
> >>>>> -
> >>>>> -     uvc_gpio_event(dev);
> >>>>> -     return IRQ_HANDLED;
> >>>>> -}
> >>>>> -
> >>>>> -static int uvc_gpio_parse(struct uvc_device *dev)
> >>>>> -{
> >>>>> -     struct uvc_entity *unit;
> >>>>> -     struct gpio_desc *gpio_privacy;
> >>>>> -     int irq;
> >>>>> -
> >>>>> -     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> >>>>> -                                            GPIOD_IN);
> >>>>> -     if (!gpio_privacy)
> >>>>> -             return 0;
> >>>>> -
> >>>>> -     if (IS_ERR(gpio_privacy))
> >>>>> -             return dev_err_probe(&dev->intf->dev,
> >>>>> -                                  PTR_ERR(gpio_privacy),
> >>>>> -                                  "Can't get privacy GPIO\n");
> >>>>> -
> >>>>> -     irq = gpiod_to_irq(gpio_privacy);
> >>>>> -     if (irq < 0)
> >>>>> -             return dev_err_probe(&dev->intf->dev, irq,
> >>>>> -                                  "No IRQ for privacy GPIO\n");
> >>>>> -
> >>>>> -     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> >>>>> -     if (!unit)
> >>>>> -             return -ENOMEM;
> >>>>> -
> >>>>> -     unit->gpio.gpio_privacy = gpio_privacy;
> >>>>> -     unit->gpio.irq = irq;
> >>>>> -     unit->gpio.bControlSize = 1;
> >>>>> -     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> >>>>> -     unit->gpio.bmControls[0] = 1;
> >>>>> -     unit->get_cur = uvc_gpio_get_cur;
> >>>>> -     unit->get_info = uvc_gpio_get_info;
> >>>>> -     strscpy(unit->name, "GPIO", sizeof(unit->name));
> >>>>> -
> >>>>> -     list_add_tail(&unit->list, &dev->entities);
> >>>>> -
> >>>>> -     dev->gpio_unit = unit;
> >>>>> -
> >>>>> -     return 0;
> >>>>> -}
> >>>>> -
> >>>>> -static int uvc_gpio_init_irq(struct uvc_device *dev)
> >>>>> -{
> >>>>> -     struct uvc_entity *unit = dev->gpio_unit;
> >>>>> -     int ret;
> >>>>> -
> >>>>> -     if (!unit || unit->gpio.irq < 0)
> >>>>> -             return 0;
> >>>>> -
> >>>>> -     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> >>>>> -                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> >>>>> -                                IRQF_TRIGGER_RISING,
> >>>>> -                                "uvc_privacy_gpio", dev);
> >>>>> -
> >>>>> -     unit->gpio.initialized = !ret;
> >>>>> -
> >>>>> -     return ret;
> >>>>> -}
> >>>>> -
> >>>>> -static void uvc_gpio_deinit(struct uvc_device *dev)
> >>>>> -{
> >>>>> -     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> >>>>> -             return;
> >>>>> -
> >>>>> -     free_irq(dev->gpio_unit->gpio.irq, dev);
> >>>>> -}
> >>>>> -
> >>>>>  /* ------------------------------------------------------------------------
> >>>>>   * UVC device scan
> >>>>>   */
> >>>>> diff --git a/drivers/media/usb/uvc/uvc_gpio.c b/drivers/media/usb/uvc/uvc_gpio.c
> >>>>> new file mode 100644
> >>>>> index 0000000000000000000000000000000000000000..30e3e6dd22cbc9cfee420dde7f7f64dbdce499b9
> >>>>> --- /dev/null
> >>>>> +++ b/drivers/media/usb/uvc/uvc_gpio.c
> >>>>> @@ -0,0 +1,123 @@
> >>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
> >>>>> +/*
> >>>>> + *      uvc_gpio.c  --  USB Video Class driver
> >>>>> + *
> >>>>> + *      Copyright 2025 Google LLC
> >>>>> + */
> >>>>> +
> >>>>> +#include <linux/kernel.h>
> >>>>> +#include <linux/gpio/consumer.h>
> >>>>> +#include "uvcvideo.h"
> >>>>> +
> >>>>> +static void uvc_gpio_event(struct uvc_device *dev)
> >>>>> +{
> >>>>> +     struct uvc_entity *unit = dev->gpio_unit;
> >>>>> +     struct uvc_video_chain *chain;
> >>>>> +     u8 new_val;
> >>>>> +
> >>>>> +     if (!unit)
> >>>>> +             return;
> >>>>> +
> >>>>> +     new_val = gpiod_get_value_cansleep(unit->gpio.gpio_privacy);
> >>>>> +
> >>>>> +     /* GPIO entities are always on the first chain. */
> >>>>> +     chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> >>>>> +     uvc_ctrl_status_event(chain, unit->controls, &new_val);
> >>>>> +}
> >>>>> +
> >>>>> +static int uvc_gpio_get_cur(struct uvc_device *dev, struct uvc_entity *entity,
> >>>>> +                         u8 cs, void *data, u16 size)
> >>>>> +{
> >>>>> +     if (cs != UVC_CT_PRIVACY_CONTROL || size < 1)
> >>>>> +             return -EINVAL;
> >>>>> +
> >>>>> +     *(u8 *)data = gpiod_get_value_cansleep(entity->gpio.gpio_privacy);
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static int uvc_gpio_get_info(struct uvc_device *dev, struct uvc_entity *entity,
> >>>>> +                          u8 cs, u8 *caps)
> >>>>> +{
> >>>>> +     if (cs != UVC_CT_PRIVACY_CONTROL)
> >>>>> +             return -EINVAL;
> >>>>> +
> >>>>> +     *caps = UVC_CONTROL_CAP_GET | UVC_CONTROL_CAP_AUTOUPDATE;
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static irqreturn_t uvc_gpio_irq(int irq, void *data)
> >>>>> +{
> >>>>> +     struct uvc_device *dev = data;
> >>>>> +
> >>>>> +     uvc_gpio_event(dev);
> >>>>> +     return IRQ_HANDLED;
> >>>>> +}
> >>>>> +
> >>>>> +int uvc_gpio_parse(struct uvc_device *dev)
> >>>>> +{
> >>>>> +     struct uvc_entity *unit;
> >>>>> +     struct gpio_desc *gpio_privacy;
> >>>>> +     int irq;
> >>>>> +
> >>>>> +     gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> >>>>> +                                            GPIOD_IN);
> >>>>> +     if (!gpio_privacy)
> >>>>> +             return 0;
> >>>>> +
> >>>>> +     if (IS_ERR(gpio_privacy))
> >>>>> +             return dev_err_probe(&dev->intf->dev,
> >>>>> +                                  PTR_ERR(gpio_privacy),
> >>>>> +                                  "Can't get privacy GPIO\n");
> >>>>> +
> >>>>> +     irq = gpiod_to_irq(gpio_privacy);
> >>>>> +     if (irq < 0)
> >>>>> +             return dev_err_probe(&dev->intf->dev, irq,
> >>>>> +                                  "No IRQ for privacy GPIO\n");
> >>>>> +
> >>>>> +     unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> >>>>> +     if (!unit)
> >>>>> +             return -ENOMEM;
> >>>>> +
> >>>>> +     unit->gpio.gpio_privacy = gpio_privacy;
> >>>>> +     unit->gpio.irq = irq;
> >>>>> +     unit->gpio.bControlSize = 1;
> >>>>> +     unit->gpio.bmControls = (u8 *)unit + sizeof(*unit);
> >>>>> +     unit->gpio.bmControls[0] = 1;
> >>>>> +     unit->get_cur = uvc_gpio_get_cur;
> >>>>> +     unit->get_info = uvc_gpio_get_info;
> >>>>> +     strscpy(unit->name, "GPIO", sizeof(unit->name));
> >>>>> +
> >>>>> +     list_add_tail(&unit->list, &dev->entities);
> >>>>> +
> >>>>> +     dev->gpio_unit = unit;
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +int uvc_gpio_init_irq(struct uvc_device *dev)
> >>>>> +{
> >>>>> +     struct uvc_entity *unit = dev->gpio_unit;
> >>>>> +     int ret;
> >>>>> +
> >>>>> +     if (!unit || unit->gpio.irq < 0)
> >>>>> +             return 0;
> >>>>> +
> >>>>> +     ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> >>>>> +                                IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> >>>>> +                                IRQF_TRIGGER_RISING,
> >>>>> +                                "uvc_privacy_gpio", dev);
> >>>>> +
> >>>>> +     unit->gpio.initialized = !ret;
> >>>>> +
> >>>>> +     return ret;
> >>>>> +}
> >>>>> +
> >>>>> +void uvc_gpio_deinit(struct uvc_device *dev)
> >>>>> +{
> >>>>> +     if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> >>>>> +             return;
> >>>>> +
> >>>>> +     free_irq(dev->gpio_unit->gpio.irq, dev);
> >>>>> +}
> >>>>> +
> >>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> >>>>> index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..aef96b96499ce09ffa286c51793482afd9832097 100644
> >>>>> --- a/drivers/media/usb/uvc/uvcvideo.h
> >>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> >>>>> @@ -683,6 +683,8 @@ do {                                                                      \
> >>>>>   */
> >>>>>
> >>>>>  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> >>>>> +struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, unsigned int num_pads,
> >>>>> +                                 unsigned int extra_size);
> >>>>>
> >>>>>  /* Video buffers queue management. */
> >>>>>  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type);
> >>>>> @@ -829,4 +831,8 @@ void uvc_debugfs_cleanup_stream(struct uvc_streaming *stream);
> >>>>>  size_t uvc_video_stats_dump(struct uvc_streaming *stream, char *buf,
> >>>>>                           size_t size);
> >>>>>
> >>>>> +/* gpio */
> >>>>> +int uvc_gpio_parse(struct uvc_device *dev);
> >>>>> +int uvc_gpio_init_irq(struct uvc_device *dev);
> >>>>> +void uvc_gpio_deinit(struct uvc_device *dev);
> >>>>>  #endif
> >>>>>
> >>
> >> --
> >> Regards,
> >>
> >> Laurent Pinchart
> >
> >
> >
>


-- 
Ricardo Ribalda

Re: [PATCH 3/8] media: v4l: fwnode: Support acpi devices for v4l2_fwnode_device_parse

[Ricardo Ribalda]

Hi Sakari

On Tue, 22 Apr 2025 at 11:41, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Hans, Ricardo,
>
> On Tue, Apr 22, 2025 at 10:44:41AM +0200, Hans de Goede wrote:
> > Hi Ricardo,
> >
> > On 22-Apr-25 2:23 AM, Ricardo Ribalda wrote:
> > > Hi Sakari
> > >
> > > On Sun, 13 Apr 2025 at 17:50, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > >>
> > >> Hi Ricardo,
> > >>
> > >> Thanks for the patch.
> > >>
> > >> On Thu, Apr 03, 2025 at 07:16:14PM +0000, Ricardo Ribalda wrote:
> > >>> This patch modifies v4l2_fwnode_device_parse() to support ACPI devices.
> > >>>
> > >>> We initially add support only for orientation via the ACPI _PLD method.
> > >>>
> > >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > >>> ---
> > >>>  drivers/media/v4l2-core/v4l2-fwnode.c | 58 +++++++++++++++++++++++++++++++----
> > >>>  1 file changed, 52 insertions(+), 6 deletions(-)
> > >>>
> > >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > >>> index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..81563c36b6436bb61e1c96f2a5ede3fa9d64dab3 100644
> > >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > >>> @@ -15,6 +15,7 @@
> > >>>   * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > >>>   */
> > >>>  #include <linux/acpi.h>
> > >>> +#include <acpi/acpi_bus.h>
> > >>>  #include <linux/kernel.h>
> > >>>  #include <linux/mm.h>
> > >>>  #include <linux/module.h>
> > >>> @@ -807,16 +808,47 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode,
> > >>>  }
> > >>>  EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link);
> > >>>
> > >>> -int v4l2_fwnode_device_parse(struct device *dev,
> > >>> -                          struct v4l2_fwnode_device_properties *props)
> > >>> +static int v4l2_fwnode_device_parse_acpi(struct device *dev,
> > >>> +                                      struct v4l2_fwnode_device_properties *props)
> > >>> +{
> > >>> +     struct acpi_pld_info *pld;
> > >>> +     int ret = 0;
> > >>> +
> > >>> +     if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) {
> > >>> +             dev_dbg(dev, "acpi _PLD call failed\n");
> > >>> +             return 0;
> > >>> +     }
> > >>
> > >> You could have software nodes in an ACPI system as well as DT-aligned
> > >> properties. They're not the primary means to convey this information still.
> > >>
> > >> How about returning e.g. -ENODATA here if _PLD doesn't exist for the device
> > >> and then proceeding to parse properties as in DT?
> > >
> > > Do you mean that there can be devices with ACPI handles that can also
> > > have DT properties?
> >
> > Yes it is possible to embed DT properties in ACPI, but I don't
> > think that is really applicable here.
>
> This is determined by
> Documentation/firmware-guide/acpi/DSD-properties-rules.rst . So rotation
> and orientation shouldn't come from _DSD properties on ACPI systems.

Doesn't this contradict what DisCo does?


if (!fwnode_property_present(adev_fwnode, "rotation")) {
struct acpi_pld_info *pld;

if (acpi_get_physical_device_location(handle, &pld)) {
swnodes->dev_props[NEXT_PROPERTY(prop_index, DEV_ROTATION)] =
PROPERTY_ENTRY_U32("rotation",
   pld->rotation * 45U);
kfree(pld);
}
}

It seems to first check for the rotation property, and then check _DSD.

If I send a v2, shall I also replace DisCo even if that means
reverting its logic?


>
> >
> > But we also have secondary software-fwnodes which are used
> > extensively on x86 to set device-properties on devices by
> > platform code to deal with ACPI tables sometimes having
> > incomplete information.
> >
> > For example atm _PLD is already being parsed in:
> >
> > drivers/media/pci/intel/ipu-bridge.c and that is then used to add
> > a standard "orientation" device-property on the sensor device.
> >
> > This is actually something which I guess we can drop once your
> > patches are in, since those should then do the same in a more
> > generic manner.
>
> DisCo for Imaging support currently also digs this information from _PDL
> (see init_crs_csi2_swnodes() in drivers/acpi/mipi-disco-img.c), but this
> is only done if a _CRS CSI-2 descriptor is present. It could also be done
> for devices with the IPU Windows specific ACPI objects and it would be a
> natural location for handing quirks -- there are some
> unrelated Dell DSDT quirks already.
>
> >
> > > What shall we do if _PLD contradicts the DT property? What takes precedence?
> >
> > As for priorities, at east for rotation it seems that we are going
> > to need some quirks, I already have a few Dell laptops where it seems
> > that the sensor is upside down and parsing the rotation field in
> > the IPU6 specific SSDB ACPI package does not yield a 180° rotation,
> > so we are going to need some quirks.
> >
> > I expect these quirks to live in the bridge code, while your helper
> > will be called from sensor drivers, so in order to allow quirks to
> > override things, I think that first the "orientation" device-property
> > should be checked (which the ACPI glue code we have can set before
> > the sensor driver binds) and only then should _PLD be checked.
> >
> > IOW _PLD should be seen as the fallback, because ACPI tables are
> > often a copy and paste job so it can very well contain wrong info
> > copy-pasted from some example ACPI code or from another hw model.
>
> Unfortunately that does happen. :-(
>
> --
> Regards,
>
> Sakari Ailus



-- 
Ricardo Ribalda