Re: [PATCH 1/2] media: media-entity.h: Add iterator for entity data links

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Jun 22, 2022 at 11:08:59AM +0200, Jacopo Mondi wrote:
> Hi Dan
> 
> On Tue, Jun 21, 2022 at 05:34:56PM +0100, Daniel Scally wrote:
> > Iterating over the links for an entity is a somewhat common need
> > through the media subsystem, but generally the assumption is that
> > they will all be data links. To meet that assumption add a new macro
> > that iterates through an entity's links and skips non-data links.
> 
> Do you foresee usages of a similar iterator but for ancillary (or
> interface) links ?
> 
> In that case you could add a 'link_type' flag to
> __media_entity_next_data_link
> 
> >
> > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > ---
> >  include/media/media-entity.h | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index a9a1c0ec5d1c..b13f67f33508 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -550,6 +550,32 @@ static inline bool media_entity_enum_intersects(
> >  				 min(ent_enum1->idx_max, ent_enum2->idx_max));
> >  }
> >
> > +static inline struct media_link *
> 
> Isn't this a bit too much for inlining ? Also I heard many times that
> it's not worth anymore trying to outsmart the compiler and inline is
> discouraged in most cases ? (and it kind of makes sense to me, but I
> sometimes wonder if that's some form of cargo cult..)

That's right, but in .h files you need to manually inline, otherwise
you'll end up with one copy per compilation unit.

> > +__media_entity_next_data_link(struct media_entity *entity,
> > +			      struct media_link *pos)
> > +{
> > +	if (!pos) {
> > +		list_for_each_entry(pos, &entity->links, list)
> 
> nit: coding style requires you to have braces
> 
> ------------------------------------------------------------------------------
> from Documentation/process/coding-style.rst:
> Also, use braces when a loop contains more than a single simple statement:
> 
> .. code-block:: c
> 
> 	while (condition) {
> 		if (test)
> 			do_something();
> 	}
> ------------------------------------------------------------------------------
> 
> > +			if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> > +			    MEDIA_LNK_FL_DATA_LINK)
> > +				return pos;
> > +
> > +		return NULL;
> > +	}
> > +
> > +	list_for_each_entry_continue(pos, &entity->links, list)
> > +		if ((pos->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> > +		    MEDIA_LNK_FL_DATA_LINK)
> > +			return pos;
> > +
> > +	return NULL;
> 
> I wonder if the same could be achieved with list_for_each_entry_from() ?
> 
> 	pos = pos ? list_next_entry(pos, list)
> 		  : list_first_entry(&entity->links, typeof(*pos), list);
> 
>         list_for_each_entry_from(pos, ...) {
>                 if (...)
>                         return pos;
> 
>         }
> 
>         return NULL;

That's even better than what I've suggested.

> If I'm not mistaken the two versions are functionally equivalent..
> 
> The iterator seems a good idea. Do you plan to use it for
> "media: rkisp1: Don't create data links for non-sensor subdevs" too,
> or changing the list of subdevs to iterate is enough there ?
> 
> > +}
> > +
> > +#define for_each_media_entity_data_link(entity, pos)		\
> > +	for (pos = __media_entity_next_data_link(entity, NULL);	\
> > +	     pos;						\
> > +	     pos = __media_entity_next_data_link(entity, pos))
> > +
> >  /**
> >   * gobj_to_entity - returns the struct &media_entity pointer from the
> >   *	@gobj contained on it.

-- 
Regards,

Laurent Pinchart