Re: [PATCH 10/13] media: v4l2-subdev: Add debug prints to v4l2_subdev_collect_streams()

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Fri, Jun 20, 2025 at 01:23:23AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Thu, Jun 19, 2025 at 11:15:43AM +0300, Sakari Ailus wrote:
> > Print streams found by v4l2_subdev_collect_streams() at debug level.
> 
> Explaining why you found this useful could help.
> 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-subdev.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 932fca795d4a..c549a462dac7 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -2219,6 +2219,8 @@ static void v4l2_subdev_collect_streams(struct v4l2_subdev *sd,
> >  		*found_streams = BIT_ULL(0);
> >  		*enabled_streams =
> >  			(sd->enabled_pads & BIT_ULL(pad)) ? BIT_ULL(0) : 0;
> > +		dev_dbg(sd->dev,
> > +			"collect_streams: sub-device does not support streams\n");
> 
> You print the subdev name below, I would print it here too. Maybe
> 
> 			"collect_streams: '%s':%u: streams not supported\n",
> 			sd->entity.name, pad);

Makes sense.

> 
> >  		return;
> >  	}
> >  
> > @@ -2237,6 +2239,9 @@ static void v4l2_subdev_collect_streams(struct v4l2_subdev *sd,
> >  		if (src_cfg->enabled)
> >  			*enabled_streams |= BIT_ULL(src_cfg->stream);
> >  	}
> 
> Please add a blank line here.
> 
> > +	dev_dbg(sd->dev,
> > +		"collect_streams: %s:%u found %#llx enabled %#llx\n",
> 
> 		"collect_streams: '%s':%u: found %#llx, enabled: %#llx\n",
> 
> or possibly
> 
> 		"%s: '%s':%u: found %#llx, enabled: %#llx\n", __func__,
> 
> (same above).

I prefer collect_streams: only as it's shorter, there's no possibility for
confusion in any case.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you!

> 
> > +		sd->entity.name, pad, *found_streams, *enabled_streams);
> >  }
> >  
> >  static void v4l2_subdev_set_streams_enabled(struct v4l2_subdev *sd,
> 

-- 
Regards,

Sakari Ailus