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@xxxxxxxxx>
> > ---
> > 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
