Hi Jacopo
On 22/06/2022 10:08, 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
Ooh this is a great idea - I'm not sure we'd need it for the ancillary
links right now but in the future when the flash gets linked then it
could crop up. I'll add the flag, thanks.
>
>> 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..)
Probably - I'll move the function to media-entity.c instead.
>
>> +__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();
> }
> ------------------------------------------------------------------------------
Good point, thanks :)
>> + 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..
Yes that's much better - thanks!
>
> 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 ?
No it's a slightly different problem there actually, I'm going to switch
that to the list of async subdevs in the same way the ipu3-cio2 works as
you suggested instead.
> 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
>>
