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

[jacopo mondi]

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+rene...@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

signature.asc
Description: PGP signature