On 03/04/2024 10:43, Tomi Valkeinen wrote:
> On 03/04/2024 11:40, Laurent Pinchart wrote:
>> Hi Sakari,
>>
>> Thank you for the patch.
>>
>> On Tue, Apr 02, 2024 at 03:00:26AM +0300, Laurent Pinchart wrote:
>>> From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>>>
>>> Print the MUST_CONNECT pad flag for each pad.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>>> ---
>>> utils/media-ctl/media-ctl.c | 50 +++++++++++++++++++++----------------
>>> 1 file changed, 28 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
>>> index 2081f111f2db..1b40552253f1 100644
>>> --- a/utils/media-ctl/media-ctl.c
>>> +++ b/utils/media-ctl/media-ctl.c
>>> @@ -368,26 +368,6 @@ static const char *media_entity_subtype_to_string(unsigned type)
>>> }
>>> }
>>> -static const char *media_pad_type_to_string(unsigned flag)
>>> -{
>>> - static const struct {
>>> - __u32 flag;
>>> - const char *name;
>>> - } flags[] = {
>>> - { MEDIA_PAD_FL_SINK, "Sink" },
>>> - { MEDIA_PAD_FL_SOURCE, "Source" },
>>> - };
>>> -
>>> - unsigned int i;
>>> -
>>> - for (i = 0; i < ARRAY_SIZE(flags); i++) {
>>> - if (flags[i].flag & flag)
>>> - return flags[i].name;
>>> - }
>>> -
>>> - return "Unknown";
>>> -}
>>> -
>>> static void media_print_topology_dot(struct media_device *media)
>>> {
>>> unsigned int nents = media_get_entities_count(media);
>>> @@ -525,6 +505,25 @@ static void media_print_pad_text(struct media_entity *entity,
>>> v4l2_subdev_print_subdev_dv(entity);
>>> }
>>> +static unsigned int weight(uint32_t word)
>>> +{
>>> + unsigned int w = 0, i;
>>> +
>>> + for (i = 0; i < sizeof(word) << 3; i++, word >>= 1)
>>> + w += word & 1U;
>>> +
>>> + return w;
>>> +}
>>> +
>>> +static const char *comma(uint32_t flag, uint32_t prev_flags, uint32_t flags)
>>> +{
>>> + static const char *empty = "", *comma = ", ";
>>> + if (!(flag & flags))
>>> + return empty;
>>> +
>>> + return weight(prev_flags & flags) ? comma : empty;
>>
>> Unless I'm mistaken, we can write this
>>
>> return prev_flags & flags ? comma : empty;
>>
>> and drop the weight function.
>>
>>> +}
>>> +
>>> static void media_print_topology_text_entity(struct media_device *media,
>>> struct media_entity *entity)
>>> {
>>> @@ -567,8 +566,15 @@ static void media_print_topology_text_entity(struct media_device *media,
>>> for (j = 0; j < info->pads; j++) {
>>> const struct media_pad *pad = media_entity_get_pad(entity, j);
>>> - printf("\tpad%u: %s\n", j, media_pad_type_to_string(pad->flags));
>>> -
>>> + printf("\tpad%u: %s%s%s%s%s\n", j,
>>> + pad->flags & MEDIA_PAD_FL_SINK ? "Sink" : "",
>>> + comma(MEDIA_PAD_FL_SOURCE, MEDIA_PAD_FL_SINK,
>>> + pad->flags),
>>> + pad->flags & MEDIA_PAD_FL_SOURCE ? "Source" : "",
>>> + comma(MEDIA_PAD_FL_MUST_CONNECT,
>>> + MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE,
>>> + pad->flags),
>>> + pad->flags & MEDIA_PAD_FL_MUST_CONNECT ? "Must connect" : "");
>>
>> To be honest, this looks overly complicated. How about printing the
>> flags with a loop ?
>
> I was just about to reply that this looks a bit too "smart" to me... I'd prefer just a simple loop here for readability's and maintainability's sake, even if it's not as optimal.
Same comment from me :-)
Regards,
Hans
