Re: [PATCH v4 24/29] media: v4l2-subdev: Introduce v4l2_subdev_get_frame_desc()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Apr 22, 2026 at 11:26:56AM +0300, Sakari Ailus wrote:
> On Wed, Apr 22, 2026 at 01:18:17AM +0300, Laurent Pinchart wrote:
> > > > > > > +static int get_mipi_dt_for_mbus(u32 code)
> > > > 
> > > > v4l2_subdev_get_mipi_dt_for_mbus()
> > > > 
> > > > Or move the function to v4l2-common.c, name it mipi_csi2_dt_for_mbus()
> > > > and declare it in include/media/mipi-csi2.h.
> > > 
> > > How about calling it mipi_csi2_dt_for_mbus()? I think it can be located in
> > > v4l2-common.c for now, no need to add a separate file just for this; all
> > > users would effectively be related to V4L2 anyway.
> > 
> > Yes, that's what I proposed :-)
> 
> Well, that's good then. :-)
> 
> ...
> 
> > > > > > > +int v4l2_subdev_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > > > > > > +			       struct v4l2_mbus_frame_desc *desc)
> > > > > > > +{
> > > > > > > +	struct v4l2_subdev_format subdev_fmt = {
> > > > > > > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > > > > > > +		.pad = pad,
> > > > > > > +	};
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	if (v4l2_subdev_has_op(sd, pad, get_frame_desc)) {
> > > > > > > +		unsigned int type = desc->type;
> > > > > > > +
> > > > > > > +		ret = v4l2_subdev_call(sd, pad, get_frame_desc, pad, desc);
> > > > > > > +
> > > > > > > +		if (desc->type != type)
> > > > > > > +			return -EINVAL;
> > > > 
> > > > I'd add a dev_err() here. There are .get_frame_desc() callers that check
> > > > if the returned type matches what they expect and log an error
> > > > otherwise. When using this helper the check can't be performed in the
> > > > callera any more, leading to possibly hard to debug issues if no message
> > > > is printed.
> > > 
> > > dev_err_once()?
> > 
> > Is there a need to limit it to printing the message once only ? It will
> > only occur if an incompatible source is connected, which shouldn't
> > happen in normal circumstances.
> 
> Yes, but still enables filling logs with that message. A single one in this
> case should be enough.

Is it user-triggerable without a serious bug in drivers ?

-- 
Regards,

Laurent Pinchart