Re: [PATCH 4/6] media: rcar-vin: Handle CLOCKENB pin polarity

[jacopo mondi <jacopo@jmondi.org>]

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

Hi Niklas,

On Thu, May 17, 2018 at 12:11:03AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> I'm happy that you dig into this as it clearly needs doing!
>
> On 2018-05-16 18:32:30 +0200, Jacopo Mondi wrote:
> > Handle CLOCKENB pin polarity, or use HSYNC in its place if polarity is
> > not specified.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index ac07f99..7a84eae 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -123,6 +123,8 @@
> >  /* Video n Data Mode Register 2 bits */
> >  #define VNDMR2_VPS		(1 << 30)
> >  #define VNDMR2_HPS		(1 << 29)
> > +#define VNDMR2_CES		(1 << 28)
> > +#define VNDMR2_CHS		(1 << 23)
> >  #define VNDMR2_FTEV		(1 << 17)
> >  #define VNDMR2_VLV(n)		((n & 0xf) << 12)
> >
> > @@ -691,6 +693,15 @@ static int rvin_setup(struct rvin_dev *vin)
> >  		dmr2 |= VNDMR2_VPS;
> >
> >  	/*
> > +	 * Clock-enable active level select.
> > +	 * Use HSYNC as enable if not specified
> > +	 */
> > +	if (vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW)
> > +		dmr2 |= VNDMR2_CES;
> > +	else if (!(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_HIGH))
> > +		dmr2 |= VNDMR2_CHS;
>
> After studying the datasheet for a while I'm getting more and more
> convinced this should be (with context leftout in this patch context)
> something like this:
>
> 	/* Hsync Signal Polarity Select */
> 	if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
> 	        dmr2 |= VNDMR2_HPS;
>
> 	/* Vsync Signal Polarity Select */
> 	if (!(vin->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
> 	        dmr2 |= VNDMR2_VPS;
>
> 	/* Clock Enable Signal Polarity Select */
> 	if (!(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW))
> 	        dmr2 |= VNDMR2_CES;

No, set CES if V4L2_MBUS_DATA_ACTIVE_LOW is specified, not the other
way around. See the CES bit description:

        Clock Enable Signal Polarity Select
        This bit specifies the polarity of the input clock enable signal in the ITU-
        R BT.601.
        0: Active high
        1: Active low
>
> 	/* Use HSYNC as clock enable if VIn_CLKENB is not available. */
> 	if (!(vin->mbus_cfg.flags & (V4L2_MBUS_DATA_ACTIVE_LOW | V4L2_MBUS_DATA_ACTIVE_HIGH)))
> 		dmr2 |= VNDMR2_CHS;
>
> Or am I misunderstanding something?

Isn't that what my code is doing?

if (flags & LOW)
        dmr2 |= CES;
else if (!(flags & HIGH)) // if we get here, LOW is not set too
        dmr2 |= CHS;

Anyway, if you agree with my previous replies, we should set CHS only
when running with explicit syncs (V4L2_MBUS_PARALLEL).

Thanks
  j
>
> > +
> > +	/*
> >  	 * Output format
> >  	 */
> >  	switch (vin->format.pixelformat) {
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund

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