Re: [PATCH v4 5/9] media: v4l: subdev: Make stream argument optional in state access functions

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

On Fri, Oct 27, 2023 at 09:12:14AM +0000, Sakari Ailus wrote:
> On Fri, Oct 27, 2023 at 12:10:13PM +0300, Laurent Pinchart wrote:
> > > > > +/*
> > > > > + * Wrap v4l2_subdev_state_get_format(), allowing the function to be called with
> > > > > + * two or three arguments. The purpose of the __v4l2_subdev_state_get_format()
> > > > > + * macro below is to come up with the name of the function or macro to call,
> > > > > + * using the last two arguments (_stream and _pad). The selected function or
> > > > > + * macro is then called using the arguments specified by the caller. A similar
> > > > > + * arrangement is used for v4l2_subdev_state_crop() and
> > > > > + * v4l2_subdev_state_compose() below.
> > > > > + */
> > > > > +#define v4l2_subdev_state_get_format(...)				\
> > > > > +	__v4l2_subdev_state_get_format(__VA_ARGS__,			\
> > > > > +				       _stream, _pad)(__VA_ARGS__)
> > > > > +#define __v4l2_subdev_state_get_format(_1, _2, _3, ARG, ...)	\
> > > > 
> > > > How about renaming this macro to __v4l2_subdev_state_get_format_name ...
> > > > 
> > > > > +	__v4l2_subdev_state_get_format ## ARG
> > > > > +#define __v4l2_subdev_state_get_format_pad(state, pad)		\
> > > > > +	__v4l2_subdev_state_get_format_stream(state, pad, 0)
> > > > >  struct v4l2_mbus_framefmt *
> > > > > -v4l2_subdev_state_get_format(struct v4l2_subdev_state *state, unsigned int pad,
> > > > > -			     u32 stream);
> > > > > +__v4l2_subdev_state_get_format_stream(struct v4l2_subdev_state *state,
> > > > > +				      unsigned int pad, u32 stream);
> > > > 
> > > > ... and this function to __v4l2_subdev_state_get_format() ? That way the
> > > > macro used by drivers and the backend function will have the same name,
> > > > with a __ prefix for the function. I think it would be a bit cleaner.
> > > > Same below.
> > > > 
> > > > But now that I've written that, I realize you would need an additional
> > > > __v4l2_subdev_state_get_format_stream() macro. I'll let you decide if
> > > > you think that's cleaner or not.
> > > > 
> > > > 
> > > > You could also take it one step forward, and avoid three copies of the
> > > > same name selection macro:
> > > > 
> > > > #define __v4l2_subdev_state_get_macro(name, _1, _2, _3, ARG, ...)	\
> > > > 	__v4l2_subdev_state_get_ ## name ## ARG
> > > > 
> > > > #define v4l2_subdev_state_get_format(...)				\
> > > > 	__v4l2_subdev_state_get_macro(format, __VA_ARGS__,		\
> > > > 				      _stream, _pad)(__VA_ARGS__)
> > > 
> > > This seems like a good idea. How about calling it
> > > __v4l2_subdev_state_gen_call()? It better describes what it does I think.
> > 
> > Works for me. It's internal anyway.
> > 
> > > > #define __v4l2_subdev_state_get_format_pad(state, pad)			\
> > > > 	__v4l2_subdev_state_get_format(state, pad, 0)
> > > > #define __v4l2_subdev_state_get_format_stream(state, pad, stream)	\
> > > > 	__v4l2_subdev_state_get_format(state, pad, stream)
> > > 
> > > This one isn't needed.
> > 
> > You could indeed drop it if you remove the _stream argument to the
> > __v4l2_subdev_state_get_macro() macro above. I thought it would be nice
> > to keep it though, to make it more explicit, but I don't mind much.
> 
> This is certainly up to interpretation but I generally prefer fewer
> wrappers. :-)

Up to you.

> We can also make the function __v4l2_subdev_state_get_format() by just
> removing "_stream" from the macro call --- empty arguments are allowed.

Yes, that's what I meant.

-- 
Regards,

Laurent Pinchart