[PATCH 0/2] adv7180: fix format and frame interval

[PATCH 0/2] adv7180: fix format and frame interval

[Niklas Söderlund]

Hi,

These first patch fix a issue which was discussed back in 2016 that the 
field format of the adv7180 should be V4L2_FIELD_ALTERNATE. While the 
second patch adds support for the g_frame_interval video op.

Work is loosely based on work by Steve Longerbeam posted in 2016. I have 
talked to Steven and he have agreed to me taking over the patches as he 
did not intend to continue his work on the original series.

The series is based on the latest media-tree and tested on Renesas 
Koelsch board.

Niklas Söderlund (2):
  adv7180: fix field type to V4L2_FIELD_ALTERNATE
  adv7180: add g_frame_interval support

 drivers/media/i2c/adv7180.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

-- 
2.18.0

[PATCH 1/2] adv7180: fix field type to V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

The ADV7180 and ADV7182 transmit whole fields, bottom field followed
by top (or vice-versa, depending on detected video standard). So
for chips that do not have support for explicitly setting the field
mode via I2P, set the field mode to V4L2_FIELD_ALTERNATE.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv7180.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 25d24a3f10a7cb4d..c2e24132e8c21d38 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -644,6 +644,9 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
 	fmt->width = 720;
 	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
 
+	if (state->field == V4L2_FIELD_ALTERNATE)
+		fmt->height /= 2;
+
 	return 0;
 }
 
@@ -711,11 +714,11 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
 
 	switch (format->format.field) {
 	case V4L2_FIELD_NONE:
-		if (!(state->chip_info->flags & ADV7180_FLAG_I2P))
-			format->format.field = V4L2_FIELD_INTERLACED;
-		break;
+		if (state->chip_info->flags & ADV7180_FLAG_I2P)
+			break;
+		/* fall through */
 	default:
-		format->format.field = V4L2_FIELD_INTERLACED;
+		format->format.field = V4L2_FIELD_ALTERNATE;
 		break;
 	}
 
@@ -1291,7 +1294,7 @@ static int adv7180_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	state->client = client;
-	state->field = V4L2_FIELD_INTERLACED;
+	state->field = V4L2_FIELD_ALTERNATE;
 	state->chip_info = (struct adv7180_chip_info *)id->driver_data;
 
 	state->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
-- 
2.18.0

[PATCH 2/2] adv7180: add g_frame_interval support

[Niklas Söderlund]

Implement g_frame_interval to return the current standard's frame
interval.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv7180.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index c2e24132e8c21d38..f72312efc471acd9 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -461,6 +461,22 @@ static int adv7180_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm)
 	return 0;
 }
 
+static int adv7180_g_frame_interval(struct v4l2_subdev *sd,
+				    struct v4l2_subdev_frame_interval *fi)
+{
+	struct adv7180_state *state = to_state(sd);
+
+	if (state->curr_norm & V4L2_STD_525_60) {
+		fi->interval.numerator = 1001;
+		fi->interval.denominator = 30000;
+	} else {
+		fi->interval.numerator = 1;
+		fi->interval.denominator = 25;
+	}
+
+	return 0;
+}
+
 static void adv7180_set_power_pin(struct adv7180_state *state, bool on)
 {
 	if (!state->pwdn_gpio)
@@ -820,6 +836,7 @@ static int adv7180_subscribe_event(struct v4l2_subdev *sd,
 static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 	.s_std = adv7180_s_std,
 	.g_std = adv7180_g_std,
+	.g_frame_interval = adv7180_g_frame_interval,
 	.querystd = adv7180_querystd,
 	.g_input_status = adv7180_g_input_status,
 	.s_routing = adv7180_s_routing,
-- 
2.18.0

Re: [PATCH 1/2] adv7180: fix field type to V4L2_FIELD_ALTERNATE

[Hans Verkuil]

On 17/07/18 14:30, Niklas Söderlund wrote:
> The ADV7180 and ADV7182 transmit whole fields, bottom field followed
> by top (or vice-versa, depending on detected video standard). So
> for chips that do not have support for explicitly setting the field
> mode via I2P, set the field mode to V4L2_FIELD_ALTERNATE.

What does I2P do? I know it was explained before, but that's a long time
ago :-)

In any case, it should be explained in the commit log as well.

I faintly remember that it was just line-doubling of each field, in which
case this code seems correct.

Have you checked other drivers that use this subdev? Are they affected by
this change?

Regards,

	Hans

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/i2c/adv7180.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 25d24a3f10a7cb4d..c2e24132e8c21d38 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -644,6 +644,9 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
>  	fmt->width = 720;
>  	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
>  
> +	if (state->field == V4L2_FIELD_ALTERNATE)
> +		fmt->height /= 2;
> +
>  	return 0;
>  }
>  
> @@ -711,11 +714,11 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
>  
>  	switch (format->format.field) {
>  	case V4L2_FIELD_NONE:
> -		if (!(state->chip_info->flags & ADV7180_FLAG_I2P))
> -			format->format.field = V4L2_FIELD_INTERLACED;
> -		break;
> +		if (state->chip_info->flags & ADV7180_FLAG_I2P)
> +			break;
> +		/* fall through */
>  	default:
> -		format->format.field = V4L2_FIELD_INTERLACED;
> +		format->format.field = V4L2_FIELD_ALTERNATE;
>  		break;
>  	}
>  
> @@ -1291,7 +1294,7 @@ static int adv7180_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  
>  	state->client = client;
> -	state->field = V4L2_FIELD_INTERLACED;
> +	state->field = V4L2_FIELD_ALTERNATE;
>  	state->chip_info = (struct adv7180_chip_info *)id->driver_data;
>  
>  	state->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> 

Re: [PATCH 1/2] adv7180: fix field type to V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

On 2018-07-17 15:12:41 +0200, Hans Verkuil wrote:
> On 17/07/18 14:30, Niklas Söderlund wrote:
> > The ADV7180 and ADV7182 transmit whole fields, bottom field followed
> > by top (or vice-versa, depending on detected video standard). So
> > for chips that do not have support for explicitly setting the field
> > mode via I2P, set the field mode to V4L2_FIELD_ALTERNATE.
> 
> What does I2P do? I know it was explained before, but that's a long time
> ago :-)

The best explanation I have is that I2P is interlaced to progressive and 
in my research I stopped at commit 851a54effbd808da ("[media] adv7180: 
Add I2P support").

I also vaguely remember reading somewhere that I2P support is planed to 
be removed.

> 
> In any case, it should be explained in the commit log as well.
> 
> I faintly remember that it was just line-doubling of each field, in which
> case this code seems correct.

If you still think I2P needs to be explained in the commit message I 
will do so in the next version.

> 
> Have you checked other drivers that use this subdev? Are they affected by
> this change?

I did a quick check what other users there are and in tree dts indicates 
imx6 and the sun9i-a80-cubieboard4 in addition to the Renesas boards. As 
I can only test on the Renesas hardware I have access to I had to 
trusted the acks from the patch from Steve which I dug out of patchwork 
[1]. His work stopped with a few comments on the code but it was acked 
by Lars-Peter who maintains the driver.

1. https://patchwork.linuxtv.org/patch/36193/

> 
> Regards,
> 
> 	Hans
> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/i2c/adv7180.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > index 25d24a3f10a7cb4d..c2e24132e8c21d38 100644
> > --- a/drivers/media/i2c/adv7180.c
> > +++ b/drivers/media/i2c/adv7180.c
> > @@ -644,6 +644,9 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
> >  	fmt->width = 720;
> >  	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> >  
> > +	if (state->field == V4L2_FIELD_ALTERNATE)
> > +		fmt->height /= 2;
> > +
> >  	return 0;
> >  }
> >  
> > @@ -711,11 +714,11 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
> >  
> >  	switch (format->format.field) {
> >  	case V4L2_FIELD_NONE:
> > -		if (!(state->chip_info->flags & ADV7180_FLAG_I2P))
> > -			format->format.field = V4L2_FIELD_INTERLACED;
> > -		break;
> > +		if (state->chip_info->flags & ADV7180_FLAG_I2P)
> > +			break;
> > +		/* fall through */
> >  	default:
> > -		format->format.field = V4L2_FIELD_INTERLACED;
> > +		format->format.field = V4L2_FIELD_ALTERNATE;
> >  		break;
> >  	}
> >  
> > @@ -1291,7 +1294,7 @@ static int adv7180_probe(struct i2c_client *client,
> >  		return -ENOMEM;
> >  
> >  	state->client = client;
> > -	state->field = V4L2_FIELD_INTERLACED;
> > +	state->field = V4L2_FIELD_ALTERNATE;
> >  	state->chip_info = (struct adv7180_chip_info *)id->driver_data;
> >  
> >  	state->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> > 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 1/2] adv7180: fix field type to V4L2_FIELD_ALTERNATE

[Hans Verkuil]

On 17/07/18 15:40, Niklas Söderlund wrote:
> Hi Hans,
> 
> Thanks for your feedback.
> 
> On 2018-07-17 15:12:41 +0200, Hans Verkuil wrote:
>> On 17/07/18 14:30, Niklas Söderlund wrote:
>>> The ADV7180 and ADV7182 transmit whole fields, bottom field followed
>>> by top (or vice-versa, depending on detected video standard). So
>>> for chips that do not have support for explicitly setting the field
>>> mode via I2P, set the field mode to V4L2_FIELD_ALTERNATE.
>>
>> What does I2P do? I know it was explained before, but that's a long time
>> ago :-)
> 
> The best explanation I have is that I2P is interlaced to progressive and 
> in my research I stopped at commit 851a54effbd808da ("[media] adv7180: 
> Add I2P support").
> 
> I also vaguely remember reading somewhere that I2P support is planed to 
> be removed.

I would just add a line saying:

"I2P converts fields into frames using an edge adaptive algorithm. The
frame rate is the same as the 'field rate': e.g. X fields per second
are now X frames per second."

BTW, does 'v4l2-compliance -f' pass with this patch series? Before running
this you should first select the correct input.

Regards,

	Hans

> 
>>
>> In any case, it should be explained in the commit log as well.
>>
>> I faintly remember that it was just line-doubling of each field, in which
>> case this code seems correct.
> 
> If you still think I2P needs to be explained in the commit message I 
> will do so in the next version.
> 
>>
>> Have you checked other drivers that use this subdev? Are they affected by
>> this change?
> 
> I did a quick check what other users there are and in tree dts indicates 
> imx6 and the sun9i-a80-cubieboard4 in addition to the Renesas boards. As 
> I can only test on the Renesas hardware I have access to I had to 
> trusted the acks from the patch from Steve which I dug out of patchwork 
> [1]. His work stopped with a few comments on the code but it was acked 
> by Lars-Peter who maintains the driver.
> 
> 1. https://patchwork.linuxtv.org/patch/36193/
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> ---
>>>  drivers/media/i2c/adv7180.c | 13 ++++++++-----
>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
>>> index 25d24a3f10a7cb4d..c2e24132e8c21d38 100644
>>> --- a/drivers/media/i2c/adv7180.c
>>> +++ b/drivers/media/i2c/adv7180.c
>>> @@ -644,6 +644,9 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
>>>  	fmt->width = 720;
>>>  	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
>>>  
>>> +	if (state->field == V4L2_FIELD_ALTERNATE)
>>> +		fmt->height /= 2;
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> @@ -711,11 +714,11 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
>>>  
>>>  	switch (format->format.field) {
>>>  	case V4L2_FIELD_NONE:
>>> -		if (!(state->chip_info->flags & ADV7180_FLAG_I2P))
>>> -			format->format.field = V4L2_FIELD_INTERLACED;
>>> -		break;
>>> +		if (state->chip_info->flags & ADV7180_FLAG_I2P)
>>> +			break;
>>> +		/* fall through */
>>>  	default:
>>> -		format->format.field = V4L2_FIELD_INTERLACED;
>>> +		format->format.field = V4L2_FIELD_ALTERNATE;
>>>  		break;
>>>  	}
>>>  
>>> @@ -1291,7 +1294,7 @@ static int adv7180_probe(struct i2c_client *client,
>>>  		return -ENOMEM;
>>>  
>>>  	state->client = client;
>>> -	state->field = V4L2_FIELD_INTERLACED;
>>> +	state->field = V4L2_FIELD_ALTERNATE;
>>>  	state->chip_info = (struct adv7180_chip_info *)id->driver_data;
>>>  
>>>  	state->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
>>>
>>
> 

Re: [PATCH 1/2] adv7180: fix field type to V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

Hi Hans,

On 2018-07-17 15:55:33 +0200, Hans Verkuil wrote:
> On 17/07/18 15:40, Niklas Söderlund wrote:
> > Hi Hans,
> > 
> > Thanks for your feedback.
> > 
> > On 2018-07-17 15:12:41 +0200, Hans Verkuil wrote:
> >> On 17/07/18 14:30, Niklas Söderlund wrote:
> >>> The ADV7180 and ADV7182 transmit whole fields, bottom field followed
> >>> by top (or vice-versa, depending on detected video standard). So
> >>> for chips that do not have support for explicitly setting the field
> >>> mode via I2P, set the field mode to V4L2_FIELD_ALTERNATE.
> >>
> >> What does I2P do? I know it was explained before, but that's a long time
> >> ago :-)
> > 
> > The best explanation I have is that I2P is interlaced to progressive and 
> > in my research I stopped at commit 851a54effbd808da ("[media] adv7180: 
> > Add I2P support").
> > 
> > I also vaguely remember reading somewhere that I2P support is planed to 
> > be removed.
> 
> I would just add a line saying:
> 
> "I2P converts fields into frames using an edge adaptive algorithm. The
> frame rate is the same as the 'field rate': e.g. X fields per second
> are now X frames per second."

Thanks, I will add this for v2.

> 
> BTW, does 'v4l2-compliance -f' pass with this patch series? Before running
> this you should first select the correct input.

I'm not sure I understand what you mean by selecting the correct input.  
I test this on Koelsch which is a Renesas Gen2 board and use the video 
node centric approach, so there are no MC magic involved in selecting 
the input.

Running 'v4l2-compliance -f' works as I expect it do do with these two 
patches applied.

# v4l2-compliance -f -d /dev/video26
v4l2-compliance SHA   : 5a870c8e3b55ba4ea255fde68c505a46bfee4e4e

Compliance test for device /dev/video26:

Driver Info:
	Driver name      : rcar_vin
	Card type        : R_Car_VIN
	Bus info         : platform:e6ef1000.video
	Driver version   : 4.18.0
	Capabilities     : 0x85200001
		Video Capture
		Read/Write
		Streaming
		Extended Pix Format
		Device Capabilities
	Device Caps      : 0x05200001
		Video Capture
		Read/Write
		Streaming
		Extended Pix Format

Required ioctls:
	test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
	test second /dev/video26 open: OK
	test VIDIOC_QUERYCAP: OK
	test VIDIOC_G/S_PRIORITY: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
	test VIDIOC_LOG_STATUS: OK

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
	test VIDIOC_QUERYCTRL: OK
	test VIDIOC_G/S_CTRL: OK
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 5 Private Controls: 1

Format ioctls (Input 0):
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
	test VIDIOC_G/S_PARM: OK
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK
	test VIDIOC_TRY_FMT: OK
	test VIDIOC_S_FMT: OK
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK
	test Composing: OK
	test Scaling: OK

Codec ioctls (Input 0):
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
	test VIDIOC_EXPBUF: OK

Test input 0:

Stream using all formats:
	test MMAP for Format NV16, Frame Size 2x4:
		Crop 720x480@0x0, Compose 6x4@0x0, Stride 32, Field None: OK   
		Crop 720x480@0x0, Compose 6x4@0x0, Stride 32, Field Top: OK   
		Crop 720x480@0x0, Compose 6x4@0x0, Stride 32, Field Bottom: OK   
		Crop 720x480@0x0, Compose 6x4@0x0, Stride 32, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 6x4@0x0, Stride 32, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 6x4@0x0, Stride 32, Field Interlaced Bottom-Top: OK   
	test MMAP for Format NV16, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 2048, Field None: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 2048, Field Top: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 2048, Field Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 2048, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 2048, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 2048, Field Interlaced Bottom-Top: OK   
	test MMAP for Format NV16, Frame Size 736x480:
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 736, Field None: OK   
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 736, Field Top: OK   
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 736, Field Bottom: OK   
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 736, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 736, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 736, Field Interlaced Bottom-Top: OK   
	test MMAP for Format YUYV, Frame Size 32x4:
		Crop 720x480@0x0, Compose 32x4@0x0, Stride 64, Field None: OK   
		Crop 720x480@0x0, Compose 32x4@0x0, Stride 64, Field Top: OK   
		Crop 720x480@0x0, Compose 32x4@0x0, Stride 64, Field Bottom: OK   
		Crop 720x480@0x0, Compose 32x4@0x0, Stride 64, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 32x4@0x0, Stride 64, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 32x4@0x0, Stride 64, Field Interlaced Bottom-Top: OK   
	test MMAP for Format YUYV, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field None: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Top: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Bottom-Top: OK   
	test MMAP for Format YUYV, Frame Size 736x480:
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 1472, Field None: OK   
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 1472, Field Top: OK   
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 1472, Field Bottom: OK   
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 1472, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 1472, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 1472, Field Interlaced Bottom-Top: OK   
	test MMAP for Format UYVY, Frame Size 2x4:
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field None: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Top: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Bottom: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Bottom-Top: OK   
	test MMAP for Format UYVY, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field None: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Top: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Bottom-Top: OK   
	test MMAP for Format UYVY, Frame Size 720x480:
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field None: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Top: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Bottom: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Bottom-Top: OK   
	test MMAP for Format RGBP, Frame Size 2x4:
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field None: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Top: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Bottom: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Bottom-Top: OK   
	test MMAP for Format RGBP, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field None: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Top: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Bottom-Top: OK   
	test MMAP for Format RGBP, Frame Size 720x480:
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field None: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Top: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Bottom: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Bottom-Top: OK   
	test MMAP for Format XR15, Frame Size 2x4:
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field None: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Top: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Bottom: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Bottom-Top: OK   
	test MMAP for Format XR15, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field None: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Top: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Bottom-Top: OK   
	test MMAP for Format XR15, Frame Size 720x480:
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field None: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Top: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Bottom: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Bottom-Top: OK   
	test MMAP for Format XR24, Frame Size 2x4:
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 8, Field None: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 8, Field Top: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 8, Field Bottom: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 8, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 8, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 8, Field Interlaced Bottom-Top: OK   
	test MMAP for Format XR24, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 8192, Field None: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 8192, Field Top: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 8192, Field Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 8192, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 8192, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 8192, Field Interlaced Bottom-Top: OK   
	test MMAP for Format XR24, Frame Size 720x480:
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 2880, Field None: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 2880, Field Top: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 2880, Field Bottom: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 2880, Field Interlaced: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 2880, Field Interlaced Top-Bottom: OK   
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 2880, Field Interlaced Bottom-Top: OK   
Total: 151, Succeeded: 151, Failed: 0, Warnings: 0

> 
> Regards,
> 
> 	Hans
> 
> > 
> >>
> >> In any case, it should be explained in the commit log as well.
> >>
> >> I faintly remember that it was just line-doubling of each field, in which
> >> case this code seems correct.
> > 
> > If you still think I2P needs to be explained in the commit message I 
> > will do so in the next version.
> > 
> >>
> >> Have you checked other drivers that use this subdev? Are they affected by
> >> this change?
> > 
> > I did a quick check what other users there are and in tree dts indicates 
> > imx6 and the sun9i-a80-cubieboard4 in addition to the Renesas boards. As 
> > I can only test on the Renesas hardware I have access to I had to 
> > trusted the acks from the patch from Steve which I dug out of patchwork 
> > [1]. His work stopped with a few comments on the code but it was acked 
> > by Lars-Peter who maintains the driver.
> > 
> > 1. https://patchwork.linuxtv.org/patch/36193/
> > 
> >>
> >> Regards,
> >>
> >> 	Hans
> >>
> >>>
> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>> ---
> >>>  drivers/media/i2c/adv7180.c | 13 ++++++++-----
> >>>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> >>> index 25d24a3f10a7cb4d..c2e24132e8c21d38 100644
> >>> --- a/drivers/media/i2c/adv7180.c
> >>> +++ b/drivers/media/i2c/adv7180.c
> >>> @@ -644,6 +644,9 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
> >>>  	fmt->width = 720;
> >>>  	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> >>>  
> >>> +	if (state->field == V4L2_FIELD_ALTERNATE)
> >>> +		fmt->height /= 2;
> >>> +
> >>>  	return 0;
> >>>  }
> >>>  
> >>> @@ -711,11 +714,11 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
> >>>  
> >>>  	switch (format->format.field) {
> >>>  	case V4L2_FIELD_NONE:
> >>> -		if (!(state->chip_info->flags & ADV7180_FLAG_I2P))
> >>> -			format->format.field = V4L2_FIELD_INTERLACED;
> >>> -		break;
> >>> +		if (state->chip_info->flags & ADV7180_FLAG_I2P)
> >>> +			break;
> >>> +		/* fall through */
> >>>  	default:
> >>> -		format->format.field = V4L2_FIELD_INTERLACED;
> >>> +		format->format.field = V4L2_FIELD_ALTERNATE;
> >>>  		break;
> >>>  	}
> >>>  
> >>> @@ -1291,7 +1294,7 @@ static int adv7180_probe(struct i2c_client *client,
> >>>  		return -ENOMEM;
> >>>  
> >>>  	state->client = client;
> >>> -	state->field = V4L2_FIELD_INTERLACED;
> >>> +	state->field = V4L2_FIELD_ALTERNATE;
> >>>  	state->chip_info = (struct adv7180_chip_info *)id->driver_data;
> >>>  
> >>>  	state->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> >>>
> >>
> > 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 0/2] adv7180: fix format and frame interval

[Hans Verkuil]

Hi Niklas,

On 17/07/18 14:30, Niklas Söderlund wrote:
> Hi,
> 
> These first patch fix a issue which was discussed back in 2016 that the 
> field format of the adv7180 should be V4L2_FIELD_ALTERNATE. While the 
> second patch adds support for the g_frame_interval video op.
> 
> Work is loosely based on work by Steve Longerbeam posted in 2016. I have 
> talked to Steven and he have agreed to me taking over the patches as he 
> did not intend to continue his work on the original series.
> 
> The series is based on the latest media-tree and tested on Renesas 
> Koelsch board.
> 
> Niklas Söderlund (2):
>   adv7180: fix field type to V4L2_FIELD_ALTERNATE
>   adv7180: add g_frame_interval support
> 
>  drivers/media/i2c/adv7180.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 

I've accepted these patches, but I was wondering if the code correctly
initializes the format? It sets the field in probe(), but where does it
set the initial format?

Should it implement the init_cfg op to initialize the pad configs?

Regards,

	Hans

Re: [PATCH 0/2] adv7180: fix format and frame interval

[Hans Verkuil]

On 27/07/18 09:59, Hans Verkuil wrote:
> Hi Niklas,
> 
> On 17/07/18 14:30, Niklas Söderlund wrote:
>> Hi,
>>
>> These first patch fix a issue which was discussed back in 2016 that the 
>> field format of the adv7180 should be V4L2_FIELD_ALTERNATE. While the 
>> second patch adds support for the g_frame_interval video op.
>>
>> Work is loosely based on work by Steve Longerbeam posted in 2016. I have 
>> talked to Steven and he have agreed to me taking over the patches as he 
>> did not intend to continue his work on the original series.
>>
>> The series is based on the latest media-tree and tested on Renesas 
>> Koelsch board.
>>
>> Niklas Söderlund (2):
>>   adv7180: fix field type to V4L2_FIELD_ALTERNATE
>>   adv7180: add g_frame_interval support
>>
>>  drivers/media/i2c/adv7180.c | 30 +++++++++++++++++++++++++-----
>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>
> 
> I've accepted these patches, 

Sorry, I replied to the v1, but I have accepted the v2 patches of course.

Regards,

	Hans

>but I was wondering if the code correctly
> initializes the format? It sets the field in probe(), but where does it
> set the initial format?
> 
> Should it implement the init_cfg op to initialize the pad configs?
> 
> Regards,
> 
> 	Hans
>