[PATCH] rcar-csi2: Allow configuring of video standard

[PATCH] rcar-csi2: Allow configuring of video standard

[Niklas Söderlund]

Allow the hardware to to do proper field detection for interlaced field
formats by implementing s_std() and g_std(). Depending on which video
standard is selected the driver needs to setup the hardware to correctly
identify fields.

Later versions of the datasheet have also been updated to make it clear
that FLD register should be set to 0 when dealing with none interlaced
field formats.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 33 +++++++++++++++++++--
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index f3099f3e536d808a..664d3784be2b9db9 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -361,6 +361,7 @@ struct rcar_csi2 {
 	struct v4l2_subdev *remote;
 
 	struct v4l2_mbus_framefmt mf;
+	v4l2_std_id std;
 
 	struct mutex lock;
 	int stream_count;
@@ -389,6 +390,22 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
 	iowrite32(data, priv->base + reg);
 }
 
+static int rcsi2_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
+{
+	struct rcar_csi2 *priv = sd_to_csi2(sd);
+
+	priv->std = std;
+	return 0;
+}
+
+static int rcsi2_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
+{
+	struct rcar_csi2 *priv = sd_to_csi2(sd);
+
+	*std = priv->std;
+	return 0;
+}
+
 static void rcsi2_standby_mode(struct rcar_csi2 *priv, int on)
 {
 	if (!on) {
@@ -475,7 +492,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
 static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 {
 	const struct rcar_csi2_format *format;
-	u32 phycnt, vcdt = 0, vcdt2 = 0;
+	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
 	unsigned int i;
 	int mbps, ret;
 
@@ -507,6 +524,15 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 			vcdt2 |= vcdt_part << ((i % 2) * 16);
 	}
 
+	if (priv->mf.field != V4L2_FIELD_NONE) {
+		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
+
+		if (priv->std & V4L2_STD_525_60)
+			fld |= FLD_FLD_NUM(2);
+		else
+			fld |= FLD_FLD_NUM(1);
+	}
+
 	phycnt = PHYCNT_ENABLECLK;
 	phycnt |= (1 << priv->lanes) - 1;
 
@@ -519,8 +545,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 	rcsi2_write(priv, PHTC_REG, 0);
 
 	/* Configure */
-	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
-		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
+	rcsi2_write(priv, FLD_REG, fld);
 	rcsi2_write(priv, VCDT_REG, vcdt);
 	if (vcdt2)
 		rcsi2_write(priv, VCDT2_REG, vcdt2);
@@ -662,6 +687,8 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
 }
 
 static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
+	.s_std = rcsi2_s_std,
+	.g_std = rcsi2_g_std,
 	.s_stream = rcsi2_s_stream,
 };
 
-- 
2.20.1

Re: [PATCH] rcar-csi2: Allow configuring of video standard

[Sergei Shtylyov]

Hello!

On 17.02.2019 1:57, Niklas Söderlund wrote:

> Allow the hardware to to do proper field detection for interlaced field
> formats by implementing s_std() and g_std(). Depending on which video
> standard is selected the driver needs to setup the hardware to correctly
> identify fields.
> 
> Later versions of the datasheet have also been updated to make it clear
> that FLD register should be set to 0 when dealing with none interlaced

    Non-interlaced, perhaps?

> field formats.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

Re: [PATCH] rcar-csi2: Allow configuring of video standard

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 4033 bytes --]

Hi Niklas,
    where is this patch supposed to be applied on?

I tried master, media master, renesas-drivers and your rcar-csi2 and
v4l2/mux branches, but it fails on all of them :(

What am I doing wrong?

On Sat, Feb 16, 2019 at 11:57:58PM +0100, Niklas Söderlund wrote:
> Allow the hardware to to do proper field detection for interlaced field
> formats by implementing s_std() and g_std(). Depending on which video
> standard is selected the driver needs to setup the hardware to correctly
> identify fields.
>
> Later versions of the datasheet have also been updated to make it clear
> that FLD register should be set to 0 when dealing with none interlaced
> field formats.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 33 +++++++++++++++++++--
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index f3099f3e536d808a..664d3784be2b9db9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -361,6 +361,7 @@ struct rcar_csi2 {
>  	struct v4l2_subdev *remote;
>
>  	struct v4l2_mbus_framefmt mf;
> +	v4l2_std_id std;
>
>  	struct mutex lock;
>  	int stream_count;
> @@ -389,6 +390,22 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
>  	iowrite32(data, priv->base + reg);
>  }
>
> +static int rcsi2_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
> +{
> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +
> +	priv->std = std;
> +	return 0;

Nit: (almost) all other functions in the file have an empty line
before return...

> +}
> +
> +static int rcsi2_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> +{
> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +
> +	*std = priv->std;

Should priv->std be initialized or STD_UNKNOWN is fine?

> +	return 0;
> +}
> +
>  static void rcsi2_standby_mode(struct rcar_csi2 *priv, int on)
>  {
>  	if (!on) {
> @@ -475,7 +492,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  {
>  	const struct rcar_csi2_format *format;
> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
>  	unsigned int i;
>  	int mbps, ret;
>
> @@ -507,6 +524,15 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
>  	}
>
> +	if (priv->mf.field != V4L2_FIELD_NONE) {

I cannot tell where rcsi2_start_receiver() is called, as I don't have
it in my local version, but I suppose it has been break out from
rcsi2_start() has they set the same register. So this is called at
s_stream() time and priv->mf at set_format() time, right?

> +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> +
> +		if (priv->std & V4L2_STD_525_60)
> +			fld |= FLD_FLD_NUM(2);
> +		else
> +			fld |= FLD_FLD_NUM(1);

I haven't been able to find an explanation on why the field detection
depends on this specific video standard... I guess it is defined in some
standard I'm ignorant of, so I assume this is correct :)

Thanks
   j

> +	}
> +
>  	phycnt = PHYCNT_ENABLECLK;
>  	phycnt |= (1 << priv->lanes) - 1;
>
> @@ -519,8 +545,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	rcsi2_write(priv, PHTC_REG, 0);
>
>  	/* Configure */
> -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> +	rcsi2_write(priv, FLD_REG, fld);
>  	rcsi2_write(priv, VCDT_REG, vcdt);
>  	if (vcdt2)
>  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> @@ -662,6 +687,8 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
>  }
>
>  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> +	.s_std = rcsi2_s_std,
> +	.g_std = rcsi2_g_std,
>  	.s_stream = rcsi2_s_stream,
>  };
>
> --
> 2.20.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] rcar-csi2: Allow configuring of video standard

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2019-02-17 19:41:40 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>     where is this patch supposed to be applied on?
> 
> I tried master, media master, renesas-drivers and your rcar-csi2 and
> v4l2/mux branches, but it fails on all of them :(
> 
> What am I doing wrong?

You answered your own question in a later mail, thanks for that ;-)

> 
> On Sat, Feb 16, 2019 at 11:57:58PM +0100, Niklas Söderlund wrote:
> > Allow the hardware to to do proper field detection for interlaced field
> > formats by implementing s_std() and g_std(). Depending on which video
> > standard is selected the driver needs to setup the hardware to correctly
> > identify fields.
> >
> > Later versions of the datasheet have also been updated to make it clear
> > that FLD register should be set to 0 when dealing with none interlaced
> > field formats.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 33 +++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index f3099f3e536d808a..664d3784be2b9db9 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -361,6 +361,7 @@ struct rcar_csi2 {
> >  	struct v4l2_subdev *remote;
> >
> >  	struct v4l2_mbus_framefmt mf;
> > +	v4l2_std_id std;
> >
> >  	struct mutex lock;
> >  	int stream_count;
> > @@ -389,6 +390,22 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
> >  	iowrite32(data, priv->base + reg);
> >  }
> >
> > +static int rcsi2_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
> > +{
> > +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +
> > +	priv->std = std;
> > +	return 0;
> 
> Nit: (almost) all other functions in the file have an empty line
> before return...

Will fix.

> 
> > +}
> > +
> > +static int rcsi2_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> > +{
> > +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +
> > +	*std = priv->std;
> 
> Should priv->std be initialized or STD_UNKNOWN is fine?

STD_UNKNOWN is fine as if the standard is not explicitly set by the user 
it is in fact unknown.

> 
> > +	return 0;
> > +}
> > +
> >  static void rcsi2_standby_mode(struct rcar_csi2 *priv, int on)
> >  {
> >  	if (!on) {
> > @@ -475,7 +492,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  {
> >  	const struct rcar_csi2_format *format;
> > -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> > +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> >  	unsigned int i;
> >  	int mbps, ret;
> >
> > @@ -507,6 +524,15 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >  	}
> >
> > +	if (priv->mf.field != V4L2_FIELD_NONE) {
> 
> I cannot tell where rcsi2_start_receiver() is called, as I don't have
> it in my local version, but I suppose it has been break out from
> rcsi2_start() has they set the same register. So this is called at
> s_stream() time and priv->mf at set_format() time, right?

Yes.

> 
> > +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> > +
> > +		if (priv->std & V4L2_STD_525_60)
> > +			fld |= FLD_FLD_NUM(2);
> > +		else
> > +			fld |= FLD_FLD_NUM(1);
> 
> I haven't been able to find an explanation on why the field detection
> depends on this specific video standard... I guess it is defined in some
> standard I'm ignorant of, so I assume this is correct :)

It defines temporal order of field transmission (TOP or BOTTOM) is 
transmitted first. PAL and NTSC differs in this regard and the R-Car 
CSI-2 needs to be informed of what standard is received.

See: 
https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html

> 
> Thanks
>    j
> 
> > +	}
> > +
> >  	phycnt = PHYCNT_ENABLECLK;
> >  	phycnt |= (1 << priv->lanes) - 1;
> >
> > @@ -519,8 +545,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  	rcsi2_write(priv, PHTC_REG, 0);
> >
> >  	/* Configure */
> > -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> > -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> > +	rcsi2_write(priv, FLD_REG, fld);
> >  	rcsi2_write(priv, VCDT_REG, vcdt);
> >  	if (vcdt2)
> >  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> > @@ -662,6 +687,8 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> >  }
> >
> >  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> > +	.s_std = rcsi2_s_std,
> > +	.g_std = rcsi2_g_std,
> >  	.s_stream = rcsi2_s_stream,
> >  };
> >
> > --
> > 2.20.1
> >



-- 
Regards,
Niklas Söderlund

Re: [PATCH] rcar-csi2: Allow configuring of video standard

[Niklas Söderlund]

Hi Sergei,

Thanks for your feedback.

On 2019-02-17 11:47:28 +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 17.02.2019 1:57, Niklas Söderlund wrote:
> 
> > Allow the hardware to to do proper field detection for interlaced field
> > formats by implementing s_std() and g_std(). Depending on which video
> > standard is selected the driver needs to setup the hardware to correctly
> > identify fields.
> > 
> > Later versions of the datasheet have also been updated to make it clear
> > that FLD register should be set to 0 when dealing with none interlaced
> 
>    Non-interlaced, perhaps?

Thanks, will fix in v2.

> 
> > field formats.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> [...]
> 
> MBR, Sergei

-- 
Regards,
Niklas Söderlund

Re: [PATCH] rcar-csi2: Allow configuring of video standard

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sat, Feb 16, 2019 at 11:57:58PM +0100, Niklas Söderlund wrote:
> Allow the hardware to to do proper field detection for interlaced field
> formats by implementing s_std() and g_std(). Depending on which video
> standard is selected the driver needs to setup the hardware to correctly
> identify fields.

I don't think this belongs to the CSI-2 receiver. Standards are really
an analog concept, and should be handled by the analog front-end. At the
CSI-2 level there's no concept of analog standard anymore.

> Later versions of the datasheet have also been updated to make it clear
> that FLD register should be set to 0 when dealing with none interlaced
> field formats.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 33 +++++++++++++++++++--
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index f3099f3e536d808a..664d3784be2b9db9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -361,6 +361,7 @@ struct rcar_csi2 {
>  	struct v4l2_subdev *remote;
>  
>  	struct v4l2_mbus_framefmt mf;
> +	v4l2_std_id std;
>  
>  	struct mutex lock;
>  	int stream_count;
> @@ -389,6 +390,22 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
>  	iowrite32(data, priv->base + reg);
>  }
>  
> +static int rcsi2_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
> +{
> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +
> +	priv->std = std;
> +	return 0;
> +}
> +
> +static int rcsi2_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> +{
> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +
> +	*std = priv->std;
> +	return 0;
> +}
> +
>  static void rcsi2_standby_mode(struct rcar_csi2 *priv, int on)
>  {
>  	if (!on) {
> @@ -475,7 +492,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  {
>  	const struct rcar_csi2_format *format;
> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
>  	unsigned int i;
>  	int mbps, ret;
>  
> @@ -507,6 +524,15 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
>  	}
>  
> +	if (priv->mf.field != V4L2_FIELD_NONE) {
> +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> +
> +		if (priv->std & V4L2_STD_525_60)
> +			fld |= FLD_FLD_NUM(2);
> +		else
> +			fld |= FLD_FLD_NUM(1);
> +	}
> +
>  	phycnt = PHYCNT_ENABLECLK;
>  	phycnt |= (1 << priv->lanes) - 1;
>  
> @@ -519,8 +545,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	rcsi2_write(priv, PHTC_REG, 0);
>  
>  	/* Configure */
> -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> +	rcsi2_write(priv, FLD_REG, fld);
>  	rcsi2_write(priv, VCDT_REG, vcdt);
>  	if (vcdt2)
>  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> @@ -662,6 +687,8 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
>  }
>  
>  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> +	.s_std = rcsi2_s_std,
> +	.g_std = rcsi2_g_std,
>  	.s_stream = rcsi2_s_stream,
>  };
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH] rcar-csi2: Allow configuring of video standard

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-03-07 02:13:18 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sat, Feb 16, 2019 at 11:57:58PM +0100, Niklas Söderlund wrote:
> > Allow the hardware to to do proper field detection for interlaced field
> > formats by implementing s_std() and g_std(). Depending on which video
> > standard is selected the driver needs to setup the hardware to correctly
> > identify fields.
> 
> I don't think this belongs to the CSI-2 receiver. Standards are really
> an analog concept, and should be handled by the analog front-end. At the
> CSI-2 level there's no concept of analog standard anymore.

I agree it should be handled by the analog front-end. This is patch just 
lets the user propagate the information in the pipeline. The driver 
could instead find its source subdevice in the media graph and ask which 
standard it's supplying.

I wrestled a bit with this and went with this approach as it then works 
the same as with other format information, such as dimensions and pixel 
format. If the driver acquires the standard by itself why should it no 
the same for the format? I'm willing to change this but I would like to 
understand where the divider for format propagating in kernel and 
user-space is :-)

Also what if there are subdevices between rcar-csi2 and the analog 
front-end which do not support the g_std operation?

> 
> > Later versions of the datasheet have also been updated to make it clear
> > that FLD register should be set to 0 when dealing with none interlaced
> > field formats.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 33 +++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index f3099f3e536d808a..664d3784be2b9db9 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -361,6 +361,7 @@ struct rcar_csi2 {
> >  	struct v4l2_subdev *remote;
> >  
> >  	struct v4l2_mbus_framefmt mf;
> > +	v4l2_std_id std;
> >  
> >  	struct mutex lock;
> >  	int stream_count;
> > @@ -389,6 +390,22 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
> >  	iowrite32(data, priv->base + reg);
> >  }
> >  
> > +static int rcsi2_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
> > +{
> > +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +
> > +	priv->std = std;
> > +	return 0;
> > +}
> > +
> > +static int rcsi2_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> > +{
> > +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +
> > +	*std = priv->std;
> > +	return 0;
> > +}
> > +
> >  static void rcsi2_standby_mode(struct rcar_csi2 *priv, int on)
> >  {
> >  	if (!on) {
> > @@ -475,7 +492,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  {
> >  	const struct rcar_csi2_format *format;
> > -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> > +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> >  	unsigned int i;
> >  	int mbps, ret;
> >  
> > @@ -507,6 +524,15 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >  	}
> >  
> > +	if (priv->mf.field != V4L2_FIELD_NONE) {
> > +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> > +
> > +		if (priv->std & V4L2_STD_525_60)
> > +			fld |= FLD_FLD_NUM(2);
> > +		else
> > +			fld |= FLD_FLD_NUM(1);
> > +	}
> > +
> >  	phycnt = PHYCNT_ENABLECLK;
> >  	phycnt |= (1 << priv->lanes) - 1;
> >  
> > @@ -519,8 +545,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  	rcsi2_write(priv, PHTC_REG, 0);
> >  
> >  	/* Configure */
> > -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> > -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> > +	rcsi2_write(priv, FLD_REG, fld);
> >  	rcsi2_write(priv, VCDT_REG, vcdt);
> >  	if (vcdt2)
> >  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> > @@ -662,6 +687,8 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> >  }
> >  
> >  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> > +	.s_std = rcsi2_s_std,
> > +	.g_std = rcsi2_g_std,
> >  	.s_stream = rcsi2_s_stream,
> >  };
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund

Re: [PATCH] rcar-csi2: Allow configuring of video standard

[Laurent Pinchart]

Hi Niklas,

On Thu, Mar 07, 2019 at 01:22:36AM +0100, Niklas Söderlund wrote:
> On 2019-03-07 02:13:18 +0200, Laurent Pinchart wrote:
> > On Sat, Feb 16, 2019 at 11:57:58PM +0100, Niklas Söderlund wrote:
> >> Allow the hardware to to do proper field detection for interlaced field
> >> formats by implementing s_std() and g_std(). Depending on which video
> >> standard is selected the driver needs to setup the hardware to correctly
> >> identify fields.
> > 
> > I don't think this belongs to the CSI-2 receiver. Standards are really
> > an analog concept, and should be handled by the analog front-end. At the
> > CSI-2 level there's no concept of analog standard anymore.
> 
> I agree it should be handled by the analog front-end. This is patch just 
> lets the user propagate the information in the pipeline. The driver 
> could instead find its source subdevice in the media graph and ask which 
> standard it's supplying.
> 
> I wrestled a bit with this and went with this approach as it then works 
> the same as with other format information, such as dimensions and pixel 
> format. If the driver acquires the standard by itself why should it no 
> the same for the format? I'm willing to change this but I would like to 
> understand where the divider for format propagating in kernel and 
> user-space is :-)
> 
> Also what if there are subdevices between rcar-csi2 and the analog 
> front-end which do not support the g_std operation?

My point is that the analog standard shouldn't be propagated at all,
neither inside the kernel nor in userspace, as it is not applicable to
CSI-2.

> >> Later versions of the datasheet have also been updated to make it clear
> >> that FLD register should be set to 0 when dealing with none interlaced
> >> field formats.
> >> 
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> ---
> >>  drivers/media/platform/rcar-vin/rcar-csi2.c | 33 +++++++++++++++++++--
> >>  1 file changed, 30 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> index f3099f3e536d808a..664d3784be2b9db9 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> @@ -361,6 +361,7 @@ struct rcar_csi2 {
> >>  	struct v4l2_subdev *remote;
> >>  
> >>  	struct v4l2_mbus_framefmt mf;
> >> +	v4l2_std_id std;
> >>  
> >>  	struct mutex lock;
> >>  	int stream_count;
> >> @@ -389,6 +390,22 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
> >>  	iowrite32(data, priv->base + reg);
> >>  }
> >>  
> >> +static int rcsi2_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
> >> +{
> >> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> >> +
> >> +	priv->std = std;
> >> +	return 0;
> >> +}
> >> +
> >> +static int rcsi2_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> >> +{
> >> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> >> +
> >> +	*std = priv->std;
> >> +	return 0;
> >> +}
> >> +
> >>  static void rcsi2_standby_mode(struct rcar_csi2 *priv, int on)
> >>  {
> >>  	if (!on) {
> >> @@ -475,7 +492,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>  {
> >>  	const struct rcar_csi2_format *format;
> >> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> >> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> >>  	unsigned int i;
> >>  	int mbps, ret;
> >>  
> >> @@ -507,6 +524,15 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >>  	}
> >>  
> >> +	if (priv->mf.field != V4L2_FIELD_NONE) {
> >> +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> >> +
> >> +		if (priv->std & V4L2_STD_525_60)
> >> +			fld |= FLD_FLD_NUM(2);
> >> +		else
> >> +			fld |= FLD_FLD_NUM(1);
> >> +	}
> >> +
> >>  	phycnt = PHYCNT_ENABLECLK;
> >>  	phycnt |= (1 << priv->lanes) - 1;
> >>  
> >> @@ -519,8 +545,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>  	rcsi2_write(priv, PHTC_REG, 0);
> >>  
> >>  	/* Configure */
> >> -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> >> -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> >> +	rcsi2_write(priv, FLD_REG, fld);
> >>  	rcsi2_write(priv, VCDT_REG, vcdt);
> >>  	if (vcdt2)
> >>  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> >> @@ -662,6 +687,8 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> >>  }
> >>  
> >>  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> >> +	.s_std = rcsi2_s_std,
> >> +	.g_std = rcsi2_g_std,
> >>  	.s_stream = rcsi2_s_stream,
> >>  };
> >>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH] rcar-csi2: Allow configuring of video standard

[Niklas Söderlund]

Hi Laurent,

On 2019-03-07 02:26:45 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Thu, Mar 07, 2019 at 01:22:36AM +0100, Niklas Söderlund wrote:
> > On 2019-03-07 02:13:18 +0200, Laurent Pinchart wrote:
> > > On Sat, Feb 16, 2019 at 11:57:58PM +0100, Niklas Söderlund wrote:
> > >> Allow the hardware to to do proper field detection for interlaced field
> > >> formats by implementing s_std() and g_std(). Depending on which video
> > >> standard is selected the driver needs to setup the hardware to correctly
> > >> identify fields.
> > > 
> > > I don't think this belongs to the CSI-2 receiver. Standards are really
> > > an analog concept, and should be handled by the analog front-end. At the
> > > CSI-2 level there's no concept of analog standard anymore.
> > 
> > I agree it should be handled by the analog front-end. This is patch just 
> > lets the user propagate the information in the pipeline. The driver 
> > could instead find its source subdevice in the media graph and ask which 
> > standard it's supplying.
> > 
> > I wrestled a bit with this and went with this approach as it then works 
> > the same as with other format information, such as dimensions and pixel 
> > format. If the driver acquires the standard by itself why should it no 
> > the same for the format? I'm willing to change this but I would like to 
> > understand where the divider for format propagating in kernel and 
> > user-space is :-)
> > 
> > Also what if there are subdevices between rcar-csi2 and the analog 
> > front-end which do not support the g_std operation?
> 
> My point is that the analog standard shouldn't be propagated at all,
> neither inside the kernel nor in userspace, as it is not applicable to
> CSI-2.

This is not related to CSI-2 if I understand it. It is related to the 
outputed field signal on the parallel output from CSI-2 facing the VINs.  
See chapter "25.4.5 FLD Signal" in the datasheet.

> 
> > >> Later versions of the datasheet have also been updated to make it clear
> > >> that FLD register should be set to 0 when dealing with none interlaced
> > >> field formats.
> > >> 
> > >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >> ---
> > >>  drivers/media/platform/rcar-vin/rcar-csi2.c | 33 +++++++++++++++++++--
> > >>  1 file changed, 30 insertions(+), 3 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> index f3099f3e536d808a..664d3784be2b9db9 100644
> > >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> @@ -361,6 +361,7 @@ struct rcar_csi2 {
> > >>  	struct v4l2_subdev *remote;
> > >>  
> > >>  	struct v4l2_mbus_framefmt mf;
> > >> +	v4l2_std_id std;
> > >>  
> > >>  	struct mutex lock;
> > >>  	int stream_count;
> > >> @@ -389,6 +390,22 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
> > >>  	iowrite32(data, priv->base + reg);
> > >>  }
> > >>  
> > >> +static int rcsi2_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
> > >> +{
> > >> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > >> +
> > >> +	priv->std = std;
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +static int rcsi2_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> > >> +{
> > >> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > >> +
> > >> +	*std = priv->std;
> > >> +	return 0;
> > >> +}
> > >> +
> > >>  static void rcsi2_standby_mode(struct rcar_csi2 *priv, int on)
> > >>  {
> > >>  	if (!on) {
> > >> @@ -475,7 +492,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> > >>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> > >>  {
> > >>  	const struct rcar_csi2_format *format;
> > >> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> > >> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> > >>  	unsigned int i;
> > >>  	int mbps, ret;
> > >>  
> > >> @@ -507,6 +524,15 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> > >>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> > >>  	}
> > >>  
> > >> +	if (priv->mf.field != V4L2_FIELD_NONE) {
> > >> +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> > >> +
> > >> +		if (priv->std & V4L2_STD_525_60)
> > >> +			fld |= FLD_FLD_NUM(2);
> > >> +		else
> > >> +			fld |= FLD_FLD_NUM(1);
> > >> +	}
> > >> +
> > >>  	phycnt = PHYCNT_ENABLECLK;
> > >>  	phycnt |= (1 << priv->lanes) - 1;
> > >>  
> > >> @@ -519,8 +545,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> > >>  	rcsi2_write(priv, PHTC_REG, 0);
> > >>  
> > >>  	/* Configure */
> > >> -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> > >> -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> > >> +	rcsi2_write(priv, FLD_REG, fld);
> > >>  	rcsi2_write(priv, VCDT_REG, vcdt);
> > >>  	if (vcdt2)
> > >>  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> > >> @@ -662,6 +687,8 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> > >>  }
> > >>  
> > >>  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> > >> +	.s_std = rcsi2_s_std,
> > >> +	.g_std = rcsi2_g_std,
> > >>  	.s_stream = rcsi2_s_stream,
> > >>  };
> > >>  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund

Re: [PATCH] rcar-csi2: Allow configuring of video standard

[Laurent Pinchart]

Hi Niklas,

(CC'ing Sakari)

On Thu, Mar 07, 2019 at 01:38:20AM +0100, Niklas Söderlund wrote:
> On 2019-03-07 02:26:45 +0200, Laurent Pinchart wrote:
> > On Thu, Mar 07, 2019 at 01:22:36AM +0100, Niklas Söderlund wrote:
> >> On 2019-03-07 02:13:18 +0200, Laurent Pinchart wrote:
> >>> On Sat, Feb 16, 2019 at 11:57:58PM +0100, Niklas Söderlund wrote:
> >>>> Allow the hardware to to do proper field detection for interlaced field
> >>>> formats by implementing s_std() and g_std(). Depending on which video
> >>>> standard is selected the driver needs to setup the hardware to correctly
> >>>> identify fields.
> >>> 
> >>> I don't think this belongs to the CSI-2 receiver. Standards are really
> >>> an analog concept, and should be handled by the analog front-end. At the
> >>> CSI-2 level there's no concept of analog standard anymore.
> >> 
> >> I agree it should be handled by the analog front-end. This is patch just 
> >> lets the user propagate the information in the pipeline. The driver 
> >> could instead find its source subdevice in the media graph and ask which 
> >> standard it's supplying.
> >> 
> >> I wrestled a bit with this and went with this approach as it then works 
> >> the same as with other format information, such as dimensions and pixel 
> >> format. If the driver acquires the standard by itself why should it no 
> >> the same for the format? I'm willing to change this but I would like to 
> >> understand where the divider for format propagating in kernel and 
> >> user-space is :-)
> >> 
> >> Also what if there are subdevices between rcar-csi2 and the analog 
> >> front-end which do not support the g_std operation?
> > 
> > My point is that the analog standard shouldn't be propagated at all,
> > neither inside the kernel nor in userspace, as it is not applicable to
> > CSI-2.
> 
> This is not related to CSI-2 if I understand it. It is related to the 
> outputed field signal on the parallel output from CSI-2 facing the VINs.  
> See chapter "25.4.5 FLD Signal" in the datasheet.

I would solely use the information contained in v4l2_mbus_framefmt. You
could use the frame height for instance to detect the type of standard.

> >>>> Later versions of the datasheet have also been updated to make it clear
> >>>> that FLD register should be set to 0 when dealing with none interlaced
> >>>> field formats.
> >>>> 
> >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>>> ---
> >>>>  drivers/media/platform/rcar-vin/rcar-csi2.c | 33 +++++++++++++++++++--
> >>>>  1 file changed, 30 insertions(+), 3 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >>>> index f3099f3e536d808a..664d3784be2b9db9 100644
> >>>> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >>>> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >>>> @@ -361,6 +361,7 @@ struct rcar_csi2 {
> >>>>  	struct v4l2_subdev *remote;
> >>>>  
> >>>>  	struct v4l2_mbus_framefmt mf;
> >>>> +	v4l2_std_id std;
> >>>>  
> >>>>  	struct mutex lock;
> >>>>  	int stream_count;
> >>>> @@ -389,6 +390,22 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
> >>>>  	iowrite32(data, priv->base + reg);
> >>>>  }
> >>>>  
> >>>> +static int rcsi2_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
> >>>> +{
> >>>> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> >>>> +
> >>>> +	priv->std = std;
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static int rcsi2_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> >>>> +{
> >>>> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> >>>> +
> >>>> +	*std = priv->std;
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>  static void rcsi2_standby_mode(struct rcar_csi2 *priv, int on)
> >>>>  {
> >>>>  	if (!on) {
> >>>> @@ -475,7 +492,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >>>>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>>>  {
> >>>>  	const struct rcar_csi2_format *format;
> >>>> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> >>>> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> >>>>  	unsigned int i;
> >>>>  	int mbps, ret;
> >>>>  
> >>>> @@ -507,6 +524,15 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>>>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >>>>  	}
> >>>>  
> >>>> +	if (priv->mf.field != V4L2_FIELD_NONE) {
> >>>> +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> >>>> +
> >>>> +		if (priv->std & V4L2_STD_525_60)
> >>>> +			fld |= FLD_FLD_NUM(2);
> >>>> +		else
> >>>> +			fld |= FLD_FLD_NUM(1);
> >>>> +	}
> >>>> +
> >>>>  	phycnt = PHYCNT_ENABLECLK;
> >>>>  	phycnt |= (1 << priv->lanes) - 1;
> >>>>  
> >>>> @@ -519,8 +545,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>>>  	rcsi2_write(priv, PHTC_REG, 0);
> >>>>  
> >>>>  	/* Configure */
> >>>> -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> >>>> -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> >>>> +	rcsi2_write(priv, FLD_REG, fld);
> >>>>  	rcsi2_write(priv, VCDT_REG, vcdt);
> >>>>  	if (vcdt2)
> >>>>  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> >>>> @@ -662,6 +687,8 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> >>>>  }
> >>>>  
> >>>>  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> >>>> +	.s_std = rcsi2_s_std,
> >>>> +	.g_std = rcsi2_g_std,
> >>>>  	.s_stream = rcsi2_s_stream,
> >>>>  };
> >>>>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH] rcar-csi2: Allow configuring of video standard

[Hans Verkuil]

On 2/16/19 11:57 PM, Niklas Söderlund wrote:
> Allow the hardware to to do proper field detection for interlaced field
> formats by implementing s_std() and g_std(). Depending on which video
> standard is selected the driver needs to setup the hardware to correctly
> identify fields.
> 
> Later versions of the datasheet have also been updated to make it clear
> that FLD register should be set to 0 when dealing with none interlaced
> field formats.

Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

The G/S_STD and QUERYSTD ioctls are specific for SDTV video receivers
(composite, S-Video, analog tuner) and can't be used for CSI devices.

struct v4l2_mbus_framefmt already has a 'field' value that is explicit
about the field ordering (TB vs BT) or the field ordering can be deduced
from the frame height (FIELD_INTERLACED).

Regards,

	Hans

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 33 +++++++++++++++++++--
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index f3099f3e536d808a..664d3784be2b9db9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -361,6 +361,7 @@ struct rcar_csi2 {
>  	struct v4l2_subdev *remote;
>  
>  	struct v4l2_mbus_framefmt mf;
> +	v4l2_std_id std;
>  
>  	struct mutex lock;
>  	int stream_count;
> @@ -389,6 +390,22 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
>  	iowrite32(data, priv->base + reg);
>  }
>  
> +static int rcsi2_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
> +{
> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +
> +	priv->std = std;
> +	return 0;
> +}
> +
> +static int rcsi2_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> +{
> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +
> +	*std = priv->std;
> +	return 0;
> +}
> +
>  static void rcsi2_standby_mode(struct rcar_csi2 *priv, int on)
>  {
>  	if (!on) {
> @@ -475,7 +492,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  {
>  	const struct rcar_csi2_format *format;
> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
>  	unsigned int i;
>  	int mbps, ret;
>  
> @@ -507,6 +524,15 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
>  	}
>  
> +	if (priv->mf.field != V4L2_FIELD_NONE) {
> +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> +
> +		if (priv->std & V4L2_STD_525_60)
> +			fld |= FLD_FLD_NUM(2);
> +		else
> +			fld |= FLD_FLD_NUM(1);
> +	}
> +
>  	phycnt = PHYCNT_ENABLECLK;
>  	phycnt |= (1 << priv->lanes) - 1;
>  
> @@ -519,8 +545,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	rcsi2_write(priv, PHTC_REG, 0);
>  
>  	/* Configure */
> -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> +	rcsi2_write(priv, FLD_REG, fld);
>  	rcsi2_write(priv, VCDT_REG, vcdt);
>  	if (vcdt2)
>  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> @@ -662,6 +687,8 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
>  }
>  
>  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> +	.s_std = rcsi2_s_std,
> +	.g_std = rcsi2_g_std,
>  	.s_stream = rcsi2_s_stream,
>  };
>  
> 

Re: [PATCH] rcar-csi2: Allow configuring of video standard

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

On 2019-03-08 15:12:15 +0100, Hans Verkuil wrote:
> On 2/16/19 11:57 PM, Niklas Söderlund wrote:
> > Allow the hardware to to do proper field detection for interlaced field
> > formats by implementing s_std() and g_std(). Depending on which video
> > standard is selected the driver needs to setup the hardware to correctly
> > identify fields.
> > 
> > Later versions of the datasheet have also been updated to make it clear
> > that FLD register should be set to 0 when dealing with none interlaced
> > field formats.
> 
> Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> The G/S_STD and QUERYSTD ioctls are specific for SDTV video receivers
> (composite, S-Video, analog tuner) and can't be used for CSI devices.
> 
> struct v4l2_mbus_framefmt already has a 'field' value that is explicit
> about the field ordering (TB vs BT) or the field ordering can be deduced
> from the frame height (FIELD_INTERLACED).

I will drop this patch and write a new one using field and height as 
suggested by you and Laurent. Thanks for the suggestion!

> 
> Regards,
> 
> 	Hans
> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 33 +++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index f3099f3e536d808a..664d3784be2b9db9 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -361,6 +361,7 @@ struct rcar_csi2 {
> >  	struct v4l2_subdev *remote;
> >  
> >  	struct v4l2_mbus_framefmt mf;
> > +	v4l2_std_id std;
> >  
> >  	struct mutex lock;
> >  	int stream_count;
> > @@ -389,6 +390,22 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
> >  	iowrite32(data, priv->base + reg);
> >  }
> >  
> > +static int rcsi2_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
> > +{
> > +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +
> > +	priv->std = std;
> > +	return 0;
> > +}
> > +
> > +static int rcsi2_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> > +{
> > +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +
> > +	*std = priv->std;
> > +	return 0;
> > +}
> > +
> >  static void rcsi2_standby_mode(struct rcar_csi2 *priv, int on)
> >  {
> >  	if (!on) {
> > @@ -475,7 +492,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  {
> >  	const struct rcar_csi2_format *format;
> > -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> > +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> >  	unsigned int i;
> >  	int mbps, ret;
> >  
> > @@ -507,6 +524,15 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >  	}
> >  
> > +	if (priv->mf.field != V4L2_FIELD_NONE) {
> > +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> > +
> > +		if (priv->std & V4L2_STD_525_60)
> > +			fld |= FLD_FLD_NUM(2);
> > +		else
> > +			fld |= FLD_FLD_NUM(1);
> > +	}
> > +
> >  	phycnt = PHYCNT_ENABLECLK;
> >  	phycnt |= (1 << priv->lanes) - 1;
> >  
> > @@ -519,8 +545,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  	rcsi2_write(priv, PHTC_REG, 0);
> >  
> >  	/* Configure */
> > -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> > -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> > +	rcsi2_write(priv, FLD_REG, fld);
> >  	rcsi2_write(priv, VCDT_REG, vcdt);
> >  	if (vcdt2)
> >  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> > @@ -662,6 +687,8 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> >  }
> >  
> >  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> > +	.s_std = rcsi2_s_std,
> > +	.g_std = rcsi2_g_std,
> >  	.s_stream = rcsi2_s_stream,
> >  };
> >  
> > 
> 

-- 
Regards,
Niklas Söderlund