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
