Hi Hans, Tomi,
On Wed, Apr 03, 2024 at 10:56:55AM +0200, Hans Verkuil wrote:
> 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.
Correct. An earlier version of the patch used it and I forgot to remove it.
It should be possible to write this as:
static const char *comma(uint32_t flag, uint32_t prev_flags, uint32_t flags)
{
return flag & flags && prev_flags & flags ? ", " : "";
}
This nicely demonstrates C operator precedence.
> >>
> >>> +}
> >>> +
> >>> 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 :-)
The above gets it done as a single printf call. Perhaps it doesn't matter
much if it doesn't though, this isn't printk. Still do note that checking
whether the commas will be printed isn't trivial so replacing this with a
loop isn't necessarily making the code notably simpler.
--
Regards,
Sakari Ailus
