Hello Laurent,
On 12/05/2015 11:44 PM, Laurent Pinchart wrote:
> Hi Javier,
>
> Thank you for the patch.
>
Thanks for your review.
> On Monday 07 September 2015 16:10:25 Javier Martinez Canillas wrote:
>
> [snip]
>
>> From 8be356e77eeefdc5c0738dd429205f3398c5b76c Mon Sep 17 00:00:00 2001
>> From: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>> Date: Thu, 3 Sep 2015 13:46:06 +0200
>> Subject: [PATCH v2 4/5] [media] uvcvideo: create pad links after subdev
>> registration
>>
>> The uvc driver creates the pads links before the media entity is
>> registered with the media device. This doesn't work now that obj
>> IDs are used to create links so the media_device has to be set.
>>
>> Move entities registration logic before pads links creation.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>> ---
>>
>> Changes since v1:
>> - Don't try to register a UVC entity subdev for type UVC_TT_STREAMING.
>>
>> drivers/media/usb/uvc/uvc_entity.c | 23 +++++++++++++++++++----
>> 1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_entity.c
>> b/drivers/media/usb/uvc/uvc_entity.c index 429e428ccd93..7f82b65b238e
>> 100644
>> --- a/drivers/media/usb/uvc/uvc_entity.c
>> +++ b/drivers/media/usb/uvc/uvc_entity.c
>> @@ -26,6 +26,15 @@
>> static int uvc_mc_register_entity(struct uvc_video_chain *chain,
>> struct uvc_entity *entity)
>> {
>> + if (UVC_ENTITY_TYPE(entity) == UVC_TT_STREAMING)
>> + return 0;
>> +
>> + return v4l2_device_register_subdev(&chain->dev->vdev,
>> &entity->subdev);
>> +}
>> +
>> +static int uvc_mc_create_pads_links(struct uvc_video_chain *chain,
>> + struct uvc_entity *entity)
>> +{
>> const u32 flags = MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE;
>> struct media_entity *sink;
>> unsigned int i;
>> @@ -62,10 +71,7 @@ static int uvc_mc_register_entity(struct uvc_video_chain
>> *chain, return ret;
>> }
>>
>> - if (UVC_ENTITY_TYPE(entity) == UVC_TT_STREAMING)
>> - return 0;
>> -
>> - return v4l2_device_register_subdev(&chain->dev->vdev,
>> &entity->subdev);
>> + return 0;
>> }
>>
>> static struct v4l2_subdev_ops uvc_subdev_ops = {
>> @@ -124,5 +130,14 @@ int uvc_mc_register_entities(struct uvc_video_chain
>> *chain) }
>> }
>>
>> + list_for_each_entry(entity, &chain->entities, chain) {
>> + ret = uvc_mc_create_pads_links(chain, entity);
>
> You can call this uvc_mc_create_links(), there's no other type of links in the
> driver.
>
Ok.
>> + if (ret < 0) {
>> + uvc_printk(KERN_INFO, "Failed to create pads links
>> for "
>
> Same here, I'd s/pad links/links/.
>
Ok.
>> + "entity %u\n", entity->id);
>> + return ret;
>> + }
>> + }
>
> This creates three loops, and I think that's one too much. The reason why init
> and register are separate is that the latter creates links, which requires all
> entities to be initialized. If you move link create after registration I
> believe you can init and register in a single loop (just move the
> v4l2_device_register_subdev() call in the appropriate location in
> uvc_mc_init_entity()) and then create links in a second loop.
>
You are right, I'll simplify this to only have two loops as suggested.
>> return 0;
>> }
>
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
