Re: [PATCH v5 12/24] media: entity: Add an iterator helper for connected pads

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

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@linux.intel.com>
> 
> 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@linux.intel.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> - Make __media_entity_next_routed_pad() return NULL and adjust the
>   iterator to handle that
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  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 ?

> +
> +/**
> + * 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 ?

> + *
> + * @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.

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 ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + */
> +#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,

Laurent Pinchart