Hi Ricardo,
Thank you for the patch, and thank you for splitting it out of 6/8, it
makes review easier.
On Fri, Mar 12, 2021 at 01:48:27PM +0100, Ricardo Ribalda wrote:
> Pass the chain instead of the device. We want to keed the reference to
> the chain that controls belong to.
>
> We need to delay the initialization of the controls after the chains
> have been initialized.
>
> This is a cleanup needed for the next patches.
>
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 41 ++++++++++++++++++++----------
> drivers/media/usb/uvc/uvc_driver.c | 8 +++---
> 2 files changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index b3dde98499f4..90ecdc24d70a 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2057,7 +2057,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
> /*
> * Add a control mapping to a given control.
> */
> -static int __uvc_ctrl_add_mapping(struct uvc_device *dev,
> +static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
> {
> struct uvc_control_mapping *map;
> @@ -2086,7 +2086,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev,
> map->set = uvc_set_le_value;
>
> list_add_tail(&map->list, &ctrl->info.mappings);
> - uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n",
> + uvc_dbg(chain->dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n",
> map->name, ctrl->info.entity, ctrl->info.selector);
>
> return 0;
> @@ -2168,7 +2168,7 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> goto done;
> }
>
> - ret = __uvc_ctrl_add_mapping(dev, ctrl, mapping);
> + ret = __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> if (ret < 0)
> atomic_dec(&dev->nmappings);
>
> @@ -2244,7 +2244,8 @@ static void uvc_ctrl_prune_entity(struct uvc_device *dev,
> * Add control information and hardcoded stock control mappings to the given
> * device.
> */
> -static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl)
> +static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
> + struct uvc_control *ctrl)
> {
> const struct uvc_control_info *info = uvc_ctrls;
> const struct uvc_control_info *iend = info + ARRAY_SIZE(uvc_ctrls);
> @@ -2263,14 +2264,14 @@ static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl)
> for (; info < iend; ++info) {
> if (uvc_entity_match_guid(ctrl->entity, info->entity) &&
> ctrl->index == info->index) {
> - uvc_ctrl_add_info(dev, ctrl, info);
> + uvc_ctrl_add_info(chain->dev, ctrl, info);
> /*
> * Retrieve control flags from the device. Ignore errors
> * and work with default flag values from the uvc_ctrl
> * array when the device doesn't properly implement
> * GET_INFO on standard controls.
> */
> - uvc_ctrl_get_flags(dev, ctrl, &ctrl->info);
> + uvc_ctrl_get_flags(chain->dev, ctrl, &ctrl->info);
> break;
> }
> }
> @@ -2281,22 +2282,20 @@ static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl)
> for (; mapping < mend; ++mapping) {
> if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
> ctrl->info.selector == mapping->selector)
> - __uvc_ctrl_add_mapping(dev, ctrl, mapping);
> + __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> }
> }
>
> /*
> * Initialize device controls.
> */
> -int uvc_ctrl_init_device(struct uvc_device *dev)
> +static int uvc_ctrl_init_chain(struct uvc_video_chain *chain)
> {
> struct uvc_entity *entity;
> unsigned int i;
>
> - INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work);
> -
> /* Walk the entities list and instantiate controls */
> - list_for_each_entry(entity, &dev->entities, list) {
> + list_for_each_entry(entity, &chain->entities, chain) {
> struct uvc_control *ctrl;
> unsigned int bControlSize = 0, ncontrols;
> u8 *bmControls = NULL;
> @@ -2316,7 +2315,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> }
>
> /* Remove bogus/blacklisted controls */
> - uvc_ctrl_prune_entity(dev, entity);
> + uvc_ctrl_prune_entity(chain->dev, entity);
>
> /* Count supported controls and allocate the controls array */
> ncontrols = memweight(bmControls, bControlSize);
> @@ -2338,7 +2337,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> ctrl->entity = entity;
> ctrl->index = i;
>
> - uvc_ctrl_init_ctrl(dev, ctrl);
> + uvc_ctrl_init_ctrl(chain, ctrl);
> ctrl++;
> }
> }
> @@ -2346,6 +2345,22 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> return 0;
> }
>
> +int uvc_ctrl_init_device(struct uvc_device *dev)
> +{
> + struct uvc_video_chain *chain;
> + int ret;
> +
> + INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work);
> +
> + list_for_each_entry(chain, &dev->chains, list) {
> + ret = uvc_ctrl_init_chain(chain);
> + if (ret)
> + return ret;
> + }
> +
> + return ret;
ret will be initialized if dev->chains is empty. This shouldn't happen,
but static analyzers may complain. I'd return 0 instead.
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> +}
> +
> /*
> * Cleanup device controls.
> */
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 30ef2a3110f7..35873cf2773d 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2423,14 +2423,14 @@ static int uvc_probe(struct usb_interface *intf,
> if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
> goto error;
>
> - /* Initialize controls. */
> - if (uvc_ctrl_init_device(dev) < 0)
> - goto error;
> -
> /* Scan the device for video chains. */
> if (uvc_scan_device(dev) < 0)
> goto error;
>
> + /* Initialize controls. */
> + if (uvc_ctrl_init_device(dev) < 0)
> + goto error;
> +
> /* Register video device nodes. */
> if (uvc_register_chains(dev) < 0)
> goto error;
--
Regards,
Laurent Pinchart
