Re: [PATCH 1/2] media: media-entity.h: Add iterator for entity data links

[Daniel Scally <djrscally@gmail.com>]

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@gmail.com>
>> ---
>>  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
>>