On Thu, Apr 04, 2024 at 08:17:09PM +0000, Sakari Ailus wrote:
> 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:
> > >> 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.
See https://lore.kernel.org/linux-media/20240404220312.8019-1-laurent.pinchart@xxxxxxxxxxxxxxxx
--
Regards,
Laurent Pinchart
