Em Mon, 24 Aug 2015 16:13:10 -0600
Shuah Khan <shuahkhan@xxxxxxxxx> escreveu:
> On Sun, Aug 23, 2015 at 2:17 PM, Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxxxxxxx> wrote:
> > As entities are graph objects, let's embed media_gobj
> > on it. That ensures an unique ID for entities that can be
> > global along the entire media controller.
> >
> > For now, we'll keep the already existing entity ID. Such
> > field need to be dropped at some point, but for now, let's
> > not do this, to avoid needing to review all drivers and
> > the userspace apps.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
> > Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index e429605ca2c3..81d6a130efef 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -379,7 +379,6 @@ int __must_check __media_device_register(struct media_device *mdev,
> > if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0))
> > return -EINVAL;
> >
> > - mdev->entity_id = 1;
> > INIT_LIST_HEAD(&mdev->entities);
> > spin_lock_init(&mdev->lock);
> > mutex_init(&mdev->graph_mutex);
> > @@ -433,10 +432,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> > entity->parent = mdev;
> >
> > spin_lock(&mdev->lock);
> > - if (entity->id == 0)
> > - entity->id = mdev->entity_id++;
> > - else
> > - mdev->entity_id = max(entity->id + 1, mdev->entity_id);
> > + /* Initialize media_gobj embedded at the entity */
> > + media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
> > list_add_tail(&entity->list, &mdev->entities);
> > spin_unlock(&mdev->lock);
> >
> > @@ -459,6 +456,7 @@ void media_device_unregister_entity(struct media_entity *entity)
> > return;
> >
> > spin_lock(&mdev->lock);
> > + media_gobj_remove(&entity->graph_obj);
> > list_del(&entity->list);
> > spin_unlock(&mdev->lock);
> > entity->parent = NULL;
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index 4834172bf6f8..888cb88e19bf 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -43,7 +43,12 @@ void media_gobj_init(struct media_device *mdev,
> > enum media_gobj_type type,
> > struct media_gobj *gobj)
> > {
> > - /* For now, nothing to do */
> > + /* Create a per-type unique object ID */
> > + switch (type) {
> > + case MEDIA_GRAPH_ENTITY:
> > + gobj->id = media_gobj_gen_id(type, ++mdev->entity_id);
> > + break;
> > + }
> > }
>
> Unless there is a reason to split patches 4 and 5, it would make it lot
> easier to review if these two patches are combined. I had to go back to
> review media_gobj_gen_id(type, ++mdev->entity_id) for me to get a
> feel for what is done here.
>
> If combined, there is no need for skeleton media_gobj_init() and then
> followed by a second patch that fills it in. It will be easier to follow the
> new interface and its use.
I find easier to review when the interfaces (headers) are added first,
as diffs generally put them in the end ;)
This is actually a matter of personal taste, i guess.
>
> thanks,
> -- Shuah
> --
> 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
--
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
