Hi Hans On Mon, 28 Apr 2025 at 16:07, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi, > > On 23-Apr-25 00:35, Ricardo Ribalda wrote: > > On Wed, 23 Apr 2025 at 06:25, Laurent Pinchart > > <laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxx/ > >>> ? > >> > >> I was referring to https://lore.kernel.org/all/20241125214523.GW19381@xxxxxxxxxxxxxxxxxxxxxxxxxx/ > > > > 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@xxxxxxxxxxxx> > >>>>> --- > >>>>> 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
