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..)
> +__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;
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 ?
Thanks
j
> +}
> +
> +#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.
> --
> 2.25.1
>
