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

Laurent Pinchart
2026-04-22T10:54:46Z

On Wed, Apr 22, 2026 at 01:48:58PM +0300, Sakari Ailus wrote:
> On Wed, Apr 22, 2026 at 01:47:29PM +0300, Laurent Pinchart wrote:
> > On Wed, Apr 22, 2026 at 01:02:32PM +0300, Sakari Ailus wrote:
> > > On Wed, Apr 22, 2026 at 12:02:56PM +0300, Laurent Pinchart wrote:
> > > > > > > > > > > +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 ?
> > >
> > > No. But one message still tells about the problem, doesn't it?
> >
> > Yes, but it then means a reboot is necessary during development every
> > time this error occurs. It also means that if two errors originate from
> > different callers only one message will be printed, which is also not
> > optimal for development.
> >
> > If this error was triggerable by userspace in normal circumstances then
> > I'd still avoid _once() but go for dev_dbg().
>
> Presumably anyone debugging this would have dynamic debug enabled here.
>
> So dev_dbg()?

I proposed dev_err() as that's what some existing drivers do, but I'd be
OK going for dev_dbg(). You may want to ask maintainers of the drivers
that will be affected.

--
Regards,

Laurent Pinchart