Hi Laurent,
Thanks for the review.
On Sun, Apr 18, 2021 at 09:20:03PM +0300, Laurent Pinchart wrote:
> Hi Tomi and Sakari,
>
> Thank you for the patch.
>
> 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.
>
> > +
> > +/**
> > + * 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.
>
> > + *
> > + * @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.
>
> 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.
>
> 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.
> > *
>
--
Kind regards,
Sakari Ailus
