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@xxxxxxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
