Re: [PATCH v5 05/24] media: entity: Add iterator helper for entity pads

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 18/04/2021 20:52, Laurent Pinchart wrote:
> Hi Tomi and Jacopo,
>
> Thank you for the patch.
>
> On Thu, Apr 15, 2021 at 04:04:31PM +0300, Tomi Valkeinen wrote:
> > From: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> >
> > Add an iterator helper to easily cycle through all pads in an entity and
> > use it in media-entity and media-device code where appropriate.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > ---
> >   drivers/media/mc/mc-device.c | 13 ++++++-------
> >   drivers/media/mc/mc-entity.c | 11 ++++++-----
> >   include/media/media-entity.h | 12 ++++++++++++
> >   3 files changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> > index 9e56d2ad6b94..704ef1360eba 100644
> > --- a/drivers/media/mc/mc-device.c
> > +++ b/drivers/media/mc/mc-device.c
> > @@ -581,7 +581,7 @@ static void __media_device_unregister_entity(struct media_entity *entity)
> >   	struct media_device *mdev = entity->graph_obj.mdev;
> >   	struct media_link *link, *tmp;
> >   	struct media_interface *intf;
> > -	unsigned int i;
> > +	struct media_pad *iter;
> >   
> >   	ida_free(&mdev->entity_internal_idx, entity->internal_idx);
> >   
> > @@ -597,8 +597,8 @@ static void __media_device_unregister_entity(struct media_entity *entity)
> >   	__media_entity_remove_links(entity);
> >   
> >   	/* Remove all pads that belong to this entity */
> > -	for (i = 0; i < entity->num_pads; i++)
> > -		media_gobj_destroy(&entity->pads[i].graph_obj);
> > +	media_entity_for_each_pad(entity, iter)
> > +		media_gobj_destroy(&iter->graph_obj);
> >   
> >   	/* Remove the entity */
> >   	media_gobj_destroy(&entity->graph_obj);
> > @@ -617,7 +617,7 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> >   					      struct media_entity *entity)
> >   {
> >   	struct media_entity_notify *notify, *next;
> > -	unsigned int i;
> > +	struct media_pad *iter;
> >   	int ret;
> >   
> >   	if (entity->function == MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN ||
> > @@ -646,9 +646,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> >   	media_gobj_create(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
> >   
> >   	/* Initialize objects at the pads */
> > -	for (i = 0; i < entity->num_pads; i++)
> > -		media_gobj_create(mdev, MEDIA_GRAPH_PAD,
> > -			       &entity->pads[i].graph_obj);
> > +	media_entity_for_each_pad(entity, iter)
> > +		media_gobj_create(mdev, MEDIA_GRAPH_PAD, &iter->graph_obj);
> >   
> >   	/* invoke entity_notify callbacks */
> >   	list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index 401fddf320e7..830841e0cd28 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -198,7 +198,8 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> >   			   struct media_pad *pads)
> >   {
> >   	struct media_device *mdev = entity->graph_obj.mdev;
> > -	unsigned int i;
> > +	struct media_pad *iter;
> > +	unsigned int i = 0;
> >   
> >   	if (num_pads >= MEDIA_ENTITY_MAX_PADS)
> >   		return -E2BIG;
> > @@ -209,12 +210,12 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> >   	if (mdev)
> >   		mutex_lock(&mdev->graph_mutex);
> >   
> > -	for (i = 0; i < num_pads; i++) {
> > -		pads[i].entity = entity;
> > -		pads[i].index = i;
> > +	media_entity_for_each_pad(entity, iter) {
> > +		iter->entity = entity;
> > +		iter->index = i++;
> >   		if (mdev)
> >   			media_gobj_create(mdev, MEDIA_GRAPH_PAD,
> > -					&entity->pads[i].graph_obj);
> > +					&iter->graph_obj);
> >   	}
> >   
> >   	if (mdev)
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 2d45344ca527..52b1a1cab57a 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -1102,3 +1102,15 @@ void media_remove_intf_links(struct media_interface *intf);
> >   	 (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)
> >   
> >   #endif
> > +
> > +/**
> > + * media_entity_for_each_pad - Iterate on all pads in an entity
> > + * @entity: The entity the pads belong to
> > + * @iter: The iterator pad
>
> I would name the variable pad, here and in the code above.

'iter' name is used in other similar functions/macros here, so I'd 
rather keep this the same.

 Tomi