Re: [PATCH 2/7] v4l2: replace video op g_mbus_fmt by pad op get_fmt
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
> >>
> >> 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
> >> Cc: Guennadi Liakhovetski
> >> Cc: Prabhakar Lad
> >> Cc: Kamil Debski
> >
> > [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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] v4l2: replace video op g_mbus_fmt by pad op get_fmt
On 06/15/2015 12:08 AM, Laurent Pinchart wrote:
> Hi Hans,
>
> (CC'ing Javier Martin)
>
> On Thursday 09 April 2015 12:21:23 Hans Verkuil wrote:
>> From: Hans Verkuil
>>
>> 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
>> Cc: Guennadi Liakhovetski
>> Cc: Prabhakar Lad
>> Cc: Kamil Debski
>
> [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.
Regards,
Hans
>
>> 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 = {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] v4l2: replace video op g_mbus_fmt by pad op get_fmt
Hi Hans,
(CC'ing Javier Martin)
On Thursday 09 April 2015 12:21:23 Hans Verkuil wrote:
> From: Hans Verkuil
>
> 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
> Cc: Guennadi Liakhovetski
> Cc: Prabhakar Lad
> Cc: Kamil Debski
[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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] v4l2: replace video op g_mbus_fmt by pad op get_fmt
Hi Hans, Thanks for the patch. On Thu, Apr 9, 2015 at 11:21 AM, Hans Verkuil wrote: > From: Hans Verkuil > > 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 > Cc: Guennadi Liakhovetski > Cc: Prabhakar Lad > Cc: Kamil Debski > --- [Snip] > drivers/media/i2c/tvp514x.c| 35 ++ > drivers/media/i2c/tvp7002.c| 28 --- > drivers/media/platform/am437x/am437x-vpfe.c| 6 +-- > drivers/media/platform/davinci/vpfe_capture.c | 19 For the above, Acked-by: Lad, Prabhakar Cheers, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] v4l2: replace video op g_mbus_fmt by pad op get_fmt
On Thu, 9 Apr 2015, Hans Verkuil wrote: > From: Hans Verkuil > > 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 > Cc: Guennadi Liakhovetski > Cc: Prabhakar Lad > Cc: Kamil Debski > --- for soc-camera: Acked-by: Guennadi Liakhovetski Thanks Guennadi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
