Hi Laurent,
On Thu, Apr 29, 2021 at 04:33:48AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Tue, Apr 20, 2021 at 02:48:25PM +0300, Sakari Ailus wrote:
> > On Sun, Apr 18, 2021 at 09:20:03PM +0300, Laurent Pinchart wrote:
> > > On Thu, Apr 15, 2021 at 04:04:38PM +0300, Tomi Valkeinen wrote:
> > > > From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > >
> > > > Add a helper macro for iterating over pads that are connected through
> > > > enabled routes. This can be used to find all the connected pads within an
> > > > entity, for instance starting from the pad which has been obtained during
> > > > the graph walk.
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > > >
> > > > - Make __media_entity_next_routed_pad() return NULL and adjust the
> > > > iterator to handle that
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > > > ---
> > > > include/media/media-entity.h | 27 +++++++++++++++++++++++++++
> > > > 1 file changed, 27 insertions(+)
> > > >
> > > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > > index 73de1c335e4e..edd6f60ed6b4 100644
> > > > --- a/include/media/media-entity.h
> > > > +++ b/include/media/media-entity.h
> > > > @@ -916,6 +916,33 @@ __must_check int media_graph_walk_init(
> > > > bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
> > > > unsigned int pad1);
> > > >
> > > > +static inline struct media_pad *__media_entity_next_routed_pad(
> > > > + struct media_pad *start, struct media_pad *iter)
> > > > +{
> > > > + struct media_entity *entity = start->entity;
> > > > +
> > > > + for (; iter < &entity->pads[entity->num_pads]; iter++)
> > > > + if (media_entity_has_route(entity, start->index, iter->index))
> > > > + return iter;
> > >
> > > I'd use curly braces.
> > >
> > > > +
> > > > + return NULL;
> > > > +}
> > >
> > > Does this need to be inlined ?
> >
> > I guess it doesn't have to. It's used inside loops and it's rather small so
> > I think it should be fine that way.
>
> It may not be that small. I'd rather let the compiler decide whether to
> inline it or not.
Works for me.
>
> > > > +
> > > > +/**
> > > > + * media_entity_for_each_routed_pad - Iterate over entity pads connected by routes
> > >
> > > "routed" sounds a bit weird. Would media_entity_for_each_connected_pad()
> > > be a better name ?
> >
> > "Connected" is often used in context of links. We're dealing with routes
> > here, so I thought "routed" is appropriate to avoid confusion.
>
> I understand the confusion, maybe we can find a better term that would
> be different than "connected". "routed" really sounds weird in this
> context.
I'm fine with connected.
>
> > > > + *
> > > > + * @start: The stating pad
> > >
> > > s/stating/starting/
> > >
> > > > + * @iter: The iterator pad
> > > > + *
> > > > + * Iterate over all pads connected through routes from a given pad
> > >
> > > "from the @start pad"
> > >
> > > > + * within an entity. The iteration will include the starting pad itself.
> > >
> > > s/starting/@start/
> > >
> > > I wonder if it wouldn't be more logical to not include the start pad.
> > > That wouldn't match the current usage patterns, which would need to be
> > > adapted accordingly, but I'm worried that including the start pad will
> > > lead to annoying bugs in the future. Maybe I worry too much.
> >
> > The aim here is to find all pads that are routed to another pad within the
> > same entity. If you remove the start pad, it becomes a task harder than
> > difficult.
>
> Intuitively, "all pads that are routed to another pad" doesn't include
> the "another pad". I'm not opposed to including the start pad as that's
> what the current usage patterns need, but we should then rename the
> macro accordingly as its current name is counter-intuitive.
I'm certainly not opposed to that. But it shouldn't be too much longer than
what's already there.
>
> > > And now that I reread the patch, I also wonder if "start" is a good
> > > name, as it implies we start the enumeration from a given pad, while we
> > > enumerate all pads connected to a given pad. I'm not sure what a better
> > > name would be though, maybe just pad ?
> >
> > There are two pads here. Therefore explicitly calling them something else
> > makes sense IMO.
>
> Makes sense, but "start" isn't a good name as we're not starting
> anything.
"start" is not a verb here. It's where the iteration *starts*.
>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >
> > I agree on the comments I didn't reply to.
> >
> > Thank you!
> >
> > > > + */
> > > > +#define media_entity_for_each_routed_pad(start, iter) \
> > > > + for (iter = __media_entity_next_routed_pad( \
> > > > + start, (start)->entity->pads); \
> > > > + iter != NULL; \
> > > > + iter = __media_entity_next_routed_pad(start, iter + 1))
> > > > +
> > > > /**
> > > > * media_graph_walk_cleanup - Release resources used by graph walk.
> > > > *
>
--
Regards,
Sakari Ailus
