diff mbox series

[v3,2/8] media: uvcvideo: Factor out gpio functions to its own file

Message ID 20241112-uvc-subdev-v3-2-0ea573d41a18@chromium.org
State Superseded
Headers show
Series media: uvcvideo: Implement the Privacy GPIO as a evdev | expand

Commit Message

Ricardo Ribalda Nov. 12, 2024, 5:30 p.m. UTC
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 | 119 +------------------------------------
 drivers/media/usb/uvc/uvc_gpio.c   | 118 ++++++++++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |   8 +++
 4 files changed, 131 insertions(+), 117 deletions(-)

Comments

Hans de Goede Nov. 25, 2024, 2:45 p.m. UTC | #1
Hi Ricardo,

On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote:
> This is just a refactor patch, no new functionality is added.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

I guess this patch may need to change depending on if we want
to keep the GPIO handling as a UVC entity or not.

Laurent what is your take on this, should this stay as
a struct uvc_entity; or should the gpio_desc and input_device
be stored directly inside struct uvc_device as is done for
the snapshot-button input_device?


Also de we want a separate input_device for this or do
we re-use the snapshot button one ?

Since my plan is to open-up the permission on the device with
the SW_CAMERA_LENS_COVER to be equal to the /dev/video#
permissions sharing has the downside of allowing keylogging
of the snapshot button.

Either way (one or 2 input-devices) I don't have a strong
preference.

Regards,

Hans




> ---
>  drivers/media/usb/uvc/Makefile     |   3 +-
>  drivers/media/usb/uvc/uvc_driver.c | 119 +------------------------------------
>  drivers/media/usb/uvc/uvc_gpio.c   | 118 ++++++++++++++++++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   |   8 +++
>  4 files changed, 131 insertions(+), 117 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> index 4f9eee4f81ab..85514b6e538f 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 cd13bf01265d..5b48768a4f7f 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>
> @@ -775,9 +774,9 @@ 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_new_entity(struct uvc_device *dev, u16 type,
> -					       u16 id, unsigned int num_pads,
> -					       unsigned int extra_size)
> +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> +					u16 id, unsigned int num_pads,
> +					unsigned int extra_size)
>  {
>  	struct uvc_entity *entity;
>  	unsigned int num_inputs;
> @@ -1240,118 +1239,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 (IS_ERR_OR_NULL(gpio_privacy))
> -		return PTR_ERR_OR_ZERO(gpio_privacy);
> -
> -	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_new_entity(dev, UVC_EXT_GPIO_UNIT,
> -				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
> -	if (IS_ERR(unit))
> -		return PTR_ERR(unit);
> -
> -	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 000000000000..453739acbe8f
> --- /dev/null
> +++ b/drivers/media/usb/uvc/uvc_gpio.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *      uvc_gpio.c  --  USB Video Class driver
> + *
> + *      Copyright 2024 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 (IS_ERR_OR_NULL(gpio_privacy))
> +		return PTR_ERR_OR_ZERO(gpio_privacy);
> +
> +	irq = gpiod_to_irq(gpio_privacy);
> +	if (irq < 0)
> +		return dev_err_probe(&dev->udev->dev, irq,
> +				     "No IRQ for privacy GPIO\n");
> +
> +	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> +				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
> +	if (IS_ERR(unit))
> +		return PTR_ERR(unit);
> +
> +	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 965a789ed03e..91ed59b54d9a 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -673,6 +673,9 @@ do {									\
>  extern struct uvc_driver uvc_driver;
>  
>  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, 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,
> @@ -817,4 +820,9 @@ 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
>
Ricardo Ribalda Nov. 25, 2024, 3:10 p.m. UTC | #2
On Mon, 25 Nov 2024 at 15:45, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Ricardo,
>
> On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote:
> > This is just a refactor patch, no new functionality is added.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>
> I guess this patch may need to change depending on if we want
> to keep the GPIO handling as a UVC entity or not.

I have a v4 that removes the virtual uvc entity. Still with that
approach IMHO this patch makes sense.

I was planning to send it today, I am testing it right now.

>
> Laurent what is your take on this, should this stay as
> a struct uvc_entity; or should the gpio_desc and input_device
> be stored directly inside struct uvc_device as is done for
> the snapshot-button input_device?
>
>
> Also de we want a separate input_device for this or do
> we re-use the snapshot button one ?
>
> Since my plan is to open-up the permission on the device with
> the SW_CAMERA_LENS_COVER to be equal to the /dev/video#
> permissions sharing has the downside of allowing keylogging
> of the snapshot button.

A downside of having 2 devices is that userspace will have to either
figure out what evdev they want to use or listen to both....

I do not have a strong preference.


>
> Either way (one or 2 input-devices) I don't have a strong
> preference.
>
> Regards,
>
> Hans
>
>
>
>
> > ---
> >  drivers/media/usb/uvc/Makefile     |   3 +-
> >  drivers/media/usb/uvc/uvc_driver.c | 119 +------------------------------------
> >  drivers/media/usb/uvc/uvc_gpio.c   | 118 ++++++++++++++++++++++++++++++++++++
> >  drivers/media/usb/uvc/uvcvideo.h   |   8 +++
> >  4 files changed, 131 insertions(+), 117 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> > index 4f9eee4f81ab..85514b6e538f 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 cd13bf01265d..5b48768a4f7f 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>
> > @@ -775,9 +774,9 @@ 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_new_entity(struct uvc_device *dev, u16 type,
> > -                                            u16 id, unsigned int num_pads,
> > -                                            unsigned int extra_size)
> > +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> > +                                     u16 id, unsigned int num_pads,
> > +                                     unsigned int extra_size)
> >  {
> >       struct uvc_entity *entity;
> >       unsigned int num_inputs;
> > @@ -1240,118 +1239,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 (IS_ERR_OR_NULL(gpio_privacy))
> > -             return PTR_ERR_OR_ZERO(gpio_privacy);
> > -
> > -     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_new_entity(dev, UVC_EXT_GPIO_UNIT,
> > -                                 UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > -     if (IS_ERR(unit))
> > -             return PTR_ERR(unit);
> > -
> > -     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 000000000000..453739acbe8f
> > --- /dev/null
> > +++ b/drivers/media/usb/uvc/uvc_gpio.c
> > @@ -0,0 +1,118 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *      uvc_gpio.c  --  USB Video Class driver
> > + *
> > + *      Copyright 2024 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 (IS_ERR_OR_NULL(gpio_privacy))
> > +             return PTR_ERR_OR_ZERO(gpio_privacy);
> > +
> > +     irq = gpiod_to_irq(gpio_privacy);
> > +     if (irq < 0)
> > +             return dev_err_probe(&dev->udev->dev, irq,
> > +                                  "No IRQ for privacy GPIO\n");
> > +
> > +     unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> > +                                 UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > +     if (IS_ERR(unit))
> > +             return PTR_ERR(unit);
> > +
> > +     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 965a789ed03e..91ed59b54d9a 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -673,6 +673,9 @@ do {                                                                      \
> >  extern struct uvc_driver uvc_driver;
> >
> >  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> > +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, 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,
> > @@ -817,4 +820,9 @@ 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
> >
>
Hans de Goede Nov. 25, 2024, 3:46 p.m. UTC | #3
Hi,

On 25-Nov-24 4:10 PM, Ricardo Ribalda wrote:
> On Mon, 25 Nov 2024 at 15:45, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Ricardo,
>>
>> On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote:
>>> This is just a refactor patch, no new functionality is added.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>
>> I guess this patch may need to change depending on if we want
>> to keep the GPIO handling as a UVC entity or not.
> 
> I have a v4 that removes the virtual uvc entity. Still with that
> approach IMHO this patch makes sense.

Right I was not suggesting dropping it, but atm it is moving
the uvc entity creation around. I was wondering if that
moving around before removing it still makes sense ?

Note I have no preference either way if first moving it
and then dropping it is cleaner, or just easier because
of the history of this patch-set then IMHO either way
is fine.

> I was planning to send it today, I am testing it right now.
> 
>>
>> Laurent what is your take on this, should this stay as
>> a struct uvc_entity; or should the gpio_desc and input_device
>> be stored directly inside struct uvc_device as is done for
>> the snapshot-button input_device?
>>
>>
>> Also de we want a separate input_device for this or do
>> we re-use the snapshot button one ?
>>
>> Since my plan is to open-up the permission on the device with
>> the SW_CAMERA_LENS_COVER to be equal to the /dev/video#
>> permissions sharing has the downside of allowing keylogging
>> of the snapshot button.
> 
> A downside of having 2 devices is that userspace will have to either
> figure out what evdev they want to use or listen to both....

Right, so both would be picked up by the compositor and the
snapshot button is just another multimedia-key then, while
the compositor can use SW_CAMERA_LENS_COVER for its OSD.

The other would be of interest to libcamera.

I'm think we may need a naming convention for the evdev
with SW_CAMERA_LENS_COVER something like "* camera privacy"
or whatever then we can have a udev rule matching on that for
uaccess + libcamera can do a strstr on the name for
"camera privacy" and ignore other evdevs.

Regards,

Hans
Laurent Pinchart Nov. 25, 2024, 9:45 p.m. UTC | #4
On Mon, Nov 25, 2024 at 03:45:38PM +0100, Hans de Goede wrote:
> Hi Ricardo,
> 
> On 12-Nov-24 6:30 PM, Ricardo Ribalda wrote:
> > This is just a refactor patch, no new functionality is added.
> > 
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> I guess this patch may need to change depending on if we want
> to keep the GPIO handling as a UVC entity or not.
> 
> Laurent what is your take on this, should this stay as
> a struct uvc_entity; or should the gpio_desc and input_device
> be stored directly inside struct uvc_device as is done for
> the snapshot-button input_device?

If we stop exposing it as a subdev I'd rather not create a uvc_entity.

> Also de we want a separate input_device for this or do
> we re-use the snapshot button one ?
> 
> Since my plan is to open-up the permission on the device with
> the SW_CAMERA_LENS_COVER to be equal to the /dev/video#
> permissions sharing has the downside of allowing keylogging
> of the snapshot button.

Given that the only standardized usage of the button is to inform
applications they should capture a still image, I don't think that's
much of a problem. Devices can also report that their button is a
generic purpose button, but I would still expect its function to be
camera-related.

Can anyone think of an attack vector ?

> Either way (one or 2 input-devices) I don't have a strong
> preference.
> 
> > ---
> >  drivers/media/usb/uvc/Makefile     |   3 +-
> >  drivers/media/usb/uvc/uvc_driver.c | 119 +------------------------------------
> >  drivers/media/usb/uvc/uvc_gpio.c   | 118 ++++++++++++++++++++++++++++++++++++
> >  drivers/media/usb/uvc/uvcvideo.h   |   8 +++
> >  4 files changed, 131 insertions(+), 117 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
> > index 4f9eee4f81ab..85514b6e538f 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 cd13bf01265d..5b48768a4f7f 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>
> > @@ -775,9 +774,9 @@ 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_new_entity(struct uvc_device *dev, u16 type,
> > -					       u16 id, unsigned int num_pads,
> > -					       unsigned int extra_size)
> > +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> > +					u16 id, unsigned int num_pads,
> > +					unsigned int extra_size)
> >  {
> >  	struct uvc_entity *entity;
> >  	unsigned int num_inputs;
> > @@ -1240,118 +1239,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 (IS_ERR_OR_NULL(gpio_privacy))
> > -		return PTR_ERR_OR_ZERO(gpio_privacy);
> > -
> > -	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_new_entity(dev, UVC_EXT_GPIO_UNIT,
> > -				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > -	if (IS_ERR(unit))
> > -		return PTR_ERR(unit);
> > -
> > -	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 000000000000..453739acbe8f
> > --- /dev/null
> > +++ b/drivers/media/usb/uvc/uvc_gpio.c
> > @@ -0,0 +1,118 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *      uvc_gpio.c  --  USB Video Class driver
> > + *
> > + *      Copyright 2024 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 (IS_ERR_OR_NULL(gpio_privacy))
> > +		return PTR_ERR_OR_ZERO(gpio_privacy);
> > +
> > +	irq = gpiod_to_irq(gpio_privacy);
> > +	if (irq < 0)
> > +		return dev_err_probe(&dev->udev->dev, irq,
> > +				     "No IRQ for privacy GPIO\n");
> > +
> > +	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> > +				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
> > +	if (IS_ERR(unit))
> > +		return PTR_ERR(unit);
> > +
> > +	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 965a789ed03e..91ed59b54d9a 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -673,6 +673,9 @@ do {									\
> >  extern struct uvc_driver uvc_driver;
> >  
> >  struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
> > +struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, 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,
> > @@ -817,4 +820,9 @@ 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
> >
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
index 4f9eee4f81ab..85514b6e538f 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 cd13bf01265d..5b48768a4f7f 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>
@@ -775,9 +774,9 @@  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_new_entity(struct uvc_device *dev, u16 type,
-					       u16 id, unsigned int num_pads,
-					       unsigned int extra_size)
+struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
+					u16 id, unsigned int num_pads,
+					unsigned int extra_size)
 {
 	struct uvc_entity *entity;
 	unsigned int num_inputs;
@@ -1240,118 +1239,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 (IS_ERR_OR_NULL(gpio_privacy))
-		return PTR_ERR_OR_ZERO(gpio_privacy);
-
-	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_new_entity(dev, UVC_EXT_GPIO_UNIT,
-				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
-	if (IS_ERR(unit))
-		return PTR_ERR(unit);
-
-	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 000000000000..453739acbe8f
--- /dev/null
+++ b/drivers/media/usb/uvc/uvc_gpio.c
@@ -0,0 +1,118 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *      uvc_gpio.c  --  USB Video Class driver
+ *
+ *      Copyright 2024 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 (IS_ERR_OR_NULL(gpio_privacy))
+		return PTR_ERR_OR_ZERO(gpio_privacy);
+
+	irq = gpiod_to_irq(gpio_privacy);
+	if (irq < 0)
+		return dev_err_probe(&dev->udev->dev, irq,
+				     "No IRQ for privacy GPIO\n");
+
+	unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
+				    UVC_EXT_GPIO_UNIT_ID, 0, 1);
+	if (IS_ERR(unit))
+		return PTR_ERR(unit);
+
+	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 965a789ed03e..91ed59b54d9a 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -673,6 +673,9 @@  do {									\
 extern struct uvc_driver uvc_driver;
 
 struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
+struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, 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,
@@ -817,4 +820,9 @@  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