Re: [PATCH 2/7] v4l2: replace video op g_mbus_fmt by pad op get_fmt

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

Hi Hans,

On Monday 15 June 2015 12:25:37 Hans Verkuil wrote:
> On 06/15/2015 12:08 AM, Laurent Pinchart wrote:
> > On Thursday 09 April 2015 12:21:23 Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >> 
> >> The g_mbus_fmt video op is a duplicate of the pad op. Replace all uses
> >> by the get_fmt pad op and remove the video op.
> >> 
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> Cc: Prabhakar Lad <prabhakar.csengg@gmail.com>
> >> Cc: Kamil Debski <k.debski@samsung.com>
> > 
> > [snip]
> > 
> >> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> >> index f2f87b7..e4fa074 100644
> >> --- a/drivers/media/i2c/tvp5150.c
> >> +++ b/drivers/media/i2c/tvp5150.c
> >> @@ -828,14 +828,18 @@ static int tvp5150_enum_mbus_code(struct
> >> v4l2_subdev *sd,
> >> 	return 0;
> >>  }
> >> 
> >> -static int tvp5150_mbus_fmt(struct v4l2_subdev *sd,
> >> -			    struct v4l2_mbus_framefmt *f)
> >> +static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
> >> +		struct v4l2_subdev_pad_config *cfg,
> >> +		struct v4l2_subdev_format *format)
> >>  {
> >> +	struct v4l2_mbus_framefmt *f;
> >>  	struct tvp5150 *decoder = to_tvp5150(sd);
> >> 
> >> -	if (f == NULL)
> >> +	if (!format || format->pad)
> >>  		return -EINVAL;
> >> 
> >> +	f = &format->format;
> >> +
> >>  	tvp5150_reset(sd, 0);
> > 
> > This resets the device every time a get or set format is issued, even for
> > TRY formats. I don't think that's right.
> > 
> > Do you have any idea why this is needed ? The code was introduced in
> > commit ec2c4f3f93cb ("[media] media: tvp5150: Add mbus_fmt callbacks"),
> > with Javier listed as the author but Mauro being the only SoB.
> 
> I have no idea why this would be needed. I agree with you that it seems
> unnecessary. Note that I don't think this is ever used with TRY formats
> today, but still it doesn't look right for SET formats either.

How do we fix that, should we remove it and see what breaks ? :-)

> >>  	f->width = decoder->rect.width;
> >> 
> >> @@ -1069,9 +1073,6 @@ static const struct v4l2_subdev_tuner_ops
> >> tvp5150_tuner_ops = { static const struct v4l2_subdev_video_ops
> >> tvp5150_video_ops = {
> >>  	.s_std = tvp5150_s_std,
> >>  	.s_routing = tvp5150_s_routing,
> >> -	.s_mbus_fmt = tvp5150_mbus_fmt,
> >> -	.try_mbus_fmt = tvp5150_mbus_fmt,
> >> -	.g_mbus_fmt = tvp5150_mbus_fmt,
> >>  	.s_crop = tvp5150_s_crop,
> >>  	.g_crop = tvp5150_g_crop,
> >>  	.cropcap = tvp5150_cropcap,
> >> @@ -1086,6 +1087,8 @@ static const struct v4l2_subdev_vbi_ops
> >> tvp5150_vbi_ops = {
> >> 
> >>  static const struct v4l2_subdev_pad_ops tvp5150_pad_ops = {
> >>  	.enum_mbus_code = tvp5150_enum_mbus_code,
> >> +	.set_fmt = tvp5150_fill_fmt,
> >> +	.get_fmt = tvp5150_fill_fmt,
> >>  };
> >>  
> >>  static const struct v4l2_subdev_ops tvp5150_ops = {

-- 
Regards,

Laurent Pinchart