Hi Niklas,
On Mon, Mar 11, 2019 at 10:45:59PM +0100, Niklas Söderlund wrote:
> On 2019-03-11 11:09:01 +0200, Laurent Pinchart wrote:
> > 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.
Do we have a guarantee that all alternate sources will cycle the frame
number between 1 and 2 ? If not I think you should select based on the
LSB.
> > >+ }
> >> +
> >> 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
