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. 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@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 > > >
