Re: [PATCH v4 3/9] media: v4l: subdev: Rename sub-device state information access functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> > Anyway, I'm just throwing this out here since I think it is confusing.

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux