Re: [PATCH v6 09/16] media: v4l2-subdev: Always call get_fmt() if set_fmt() is unavailable

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

On Mon, Jul 20, 2026 at 10:44:11AM +0300, Sakari Ailus wrote:
> On Mon, Jul 20, 2026 at 09:53:37AM +0300, Laurent Pinchart wrote:
> > On Wed, Jul 01, 2026 at 03:26:26PM +0300, Sakari Ailus wrote:
> > > Many drivers implement set_fmt() pad op that simply returns the format
> > > just as get_fmt() would do, usually because the driver only supports a
> > > single one. As a driver should always implement set_fmt() either on its
> > > own or via get_fmt(), call get_fmt() in case set_fmt() isn't implemented
> > > by the driver. This allows removing a fair amount of boilerplate code as
> > > well as ensures that set_fmt() works whenever get_fmt() does.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-subdev.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 86be4d51c9a5..c2be59e4c4a7 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -260,8 +260,11 @@ static int call_set_fmt(struct v4l2_subdev *sd,
> > >  			struct v4l2_subdev_state *state,
> > >  			struct v4l2_subdev_format *format)
> > >  {
> > > -	return do_subdev_call(sd, check_format(sd, state, format), pad, set_fmt,
> > > -			      state, format);
> > > +	return check_format(sd, state, format) ? :
> > > +		sd->ops->pad->set_fmt ?
> > > +		sd->ops->pad->set_fmt(sd, state, format) :
> > > +		sd->ops->pad->get_fmt ?
> > > +		sd->ops->pad->get_fmt(sd, state, format) : -ENOIOCTLCMD;
> > 
> > This changes the behaviour: if neither operations are implemented, and
> > the format check fails, userspace will get an error from check_format()
> > instead of -ENOIOCTLCMD. It's probably not an issue in practice, but...
> 
> Indeed, that needs to be addressed.
> 
> > Also, the code becomes really unreadable.
> 
> I guess it's up to the reader; similar patterns are also used elsewhere in
> the file -- and I don't think I've written them. :-) I'm fine with the code
> you suggested below, too, and I'll use it for v7.

Thank you.

> > 
> > 	int ret;
> > 
> > 	ret = check_format(sd, state, format);
> > 	if (ret)
> > 		return ret;
> > 
> > 	if (sd->ops->pad->set_fmt)
> > 		return sd->ops->pad->set_fmt(sd, state, format);
> > 	if (sd->ops->pad->get_fmt)
> > 		return sd->ops->pad->get_fmt(sd, state, format);
> > 
> > 	return -ENOIOCTLCMD;
> > 
> > There's no tax on the number of lines of code.
> > 
> > If you want to avoid changing the behaviour, write
> > 
> > 	int ret;
> > 
> > 	if (!sd->ops->pad->set_fmt && !sd->ops->pad->get_fmt)
> > 		return -ENOIOCTLCMD;
> > 
> > 	ret = check_format(sd, state, format);
> > 	if (ret)
> > 		return ret;
> > 
> > 	if (sd->ops->pad->set_fmt)
> > 		return sd->ops->pad->set_fmt(sd, state, format);
> > 	else
> 
> This else isn't useful.

It's not required, but I find it useful as it more clearly conveys the
meaning here: if the set_fmt operation exists, call it, otherwise, call
get_fmt. I think it's different than a

 	if (sd->ops->pad->set_fmt)
 		return sd->ops->pad->set_fmt(sd, state, format);

	return -ENOIOCTLCMD;

where the last statement is a "catch all".

> > 		return sd->ops->pad->get_fmt(sd, state, format);
> > 
> > Re-reading patch 08/16, I think it would also be more readable without
> > quite a few of the ternary operators. I'll leave that one up to you as
> > the code is already hard to read before 08/16.

-- 
Regards,

Laurent Pinchart