On Thu, Oct 26, 2023 at 10:10:14AM +0000, Sakari Ailus wrote: > On Thu, Oct 26, 2023 at 12:51:42PM +0300, Laurent Pinchart wrote: > > On Thu, Oct 26, 2023 at 08:25:04AM +0000, Sakari Ailus wrote: > > > On Thu, Oct 26, 2023 at 09:26:40AM +0200, Hans Verkuil wrote: > > > > On 26/10/2023 09:03, Sakari Ailus wrote: > > > > > Rename the sub-devices state information access functions, removing > > > > > "_stream" from them. This makes them shorter and so more convenient to > > > > > use. No other sets of functions will be needed to access this information. > > > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > > > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> > > > > > --- > > > > > drivers/media/i2c/ds90ub913.c | 3 +-- > > > > > drivers/media/i2c/ds90ub953.c | 3 +-- > > > > > drivers/media/i2c/ds90ub960.c | 12 ++++----- > > > > > .../platform/nxp/imx8-isi/imx8-isi-crossbar.c | 10 +++---- > > > > > drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++---------- > > > > > include/media/v4l2-subdev.h | 19 +++++++------ > > > > > 6 files changed, 34 insertions(+), 40 deletions(-) > > > > > > > > > > diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c > > > > > index 8e9ebed09f64..8bb6be956780 100644 > > > > > --- a/drivers/media/i2c/ds90ub913.c > > > > > +++ b/drivers/media/i2c/ds90ub913.c > > > > > @@ -424,8 +424,7 @@ static int ub913_set_fmt(struct v4l2_subdev *sd, > > > > > } > > > > > > > > > > /* Set sink format */ > > > > > - fmt = v4l2_subdev_state_get_stream_format(state, format->pad, > > > > > - format->stream); > > > > > + fmt = v4l2_subdev_state_get_format(state, format->pad, format->stream); > > > > > if (!fmt) > > > > > return -EINVAL; > > > > > > > > If we want shorter names, how about replacing _get_ with _g_? And of course > > > > _set_ with _s_. I would prefer that, but it's up to you. > > > > > > These functions doesn't actually "get" something, but return a pointer to > > > the data. There's thus no corresponding set. I think I'll keep it as-is for > > > now but I think "_get" could as well be removed here. > > > > > > > > > > > Since we are changing function names here, I wonder if this isn't a good > > > > opportunity to straighten out some more confusing names. > > > > > > > > I noticed that in v4l2-subdev.h "fmt" typically refers to v4l2_subdev_format, > > > > while "format" tends to refer to v4l2_mbus_framefmt. > > > > > > > > I find this really confusing, especially if you do not use these functions > > > > regularly. > > > > > > > > My personal preference would be to use "format" for things relating to > > > > v4l2_subdev_format and "ffmt" for v4l2_mbus_framefmt. Or possibly just "fmt" > > > > if people don't like "ffmt". It would signal the type of format a lot better. > > > > > > I'll use ffmt for the name of the function. > > > > I would avoid this, it will make things even more confusing by adding > > yet another naming scheme. If we want to clean things up (and I think we > > should), then it should be done on top, and span the entire subsystem. > > I could live with that, too, but I think it'll be someone else changing the > naming afterwards. Let's first agree on a new naming scheme :-) Then someone can write patches. I know it's tedious work, I'm about to submit a series that renames .init_cfg() to .init_state() and it was painful to do. Renaming functions should be OK with the help of coccinelle, but renaming variables to comply with the new naming scheme hurts. -- Regards, Laurent Pinchart
