Re: [PATCH v2] rcar-csi2: Propagate the FLD signal for NTSC and PAL

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

 



Hi Laurent,

Thanks for your feedback.

On 2019-03-11 11:09:01 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sat, Mar 09, 2019 at 12:51:57AM +0100, Niklas Söderlund wrote:
> > Depending on which video standard is used the driver needs to setup the
> > hardware to correctly handle fields. If stream is identified as NTSC
> > or PAL setup field detection and propagate the field detection signal.
> > 
> > Later versions of the datasheet have been updated to make it clear
> > that FLD register should be set to 0 when dealing with non-interlaced
> > field formats.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > ---
> > 
> > Hi,
> > 
> > This patch depends on [PATCH v2 0/2] rcar-csi2: Use standby mode instead of resetting
> > 
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 7a1c9b549e0fffc6..d9b29dbbcc2949de 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -475,7 +475,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 +507,16 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >  	}
> >  
> > +	if (priv->mf.field != V4L2_FIELD_NONE &&
> 
> Shouldn't this be
> 
> 	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> 
> If the CSI-2 receiver gets a top/bottom-only or sequential field order I
> would expect it not to toggle the field signal.

For some reason I mixed all interlaced formats in to the mix while now 
it's clear ALTERNATE is the only one which make sens, thanks!

> 
> > +	    (priv->mf.height == 240 || priv->mf.height == 288)) {
> 
> I think you can drop this part of the check.

I added it to guard so this special case only would trigger for PAL and 
NTSC resolutions. But I think I agree with you that I might be over 
cautious.

> 
> > +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> > +
> > +		if (priv->mf.height == 240)
> > +			fld |= FLD_FLD_NUM(2);
> > +		else
> > +			fld |= FLD_FLD_NUM(1);
> 
> How does this work ? Looking at the datasheet, I was expecting
> FLD_DET_SEL field to be set to 01 in order for the field signal to
> toggle every frame.

I thought so too then I read 26.4.5 FLD Signal which fits what is done 
in the BSP code and fits with how the hardware behaves.

> 
> >+	}
> > +
> >  	phycnt = PHYCNT_ENABLECLK;
> >  	phycnt |= (1 << priv->lanes) - 1;
> >  
> > @@ -519,8 +529,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);
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux