| Message ID | 1526488352-898-5-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; /* 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? > + > + /* > * Output format > */ > switch (vin->format.pixelformat) { > -- > 2.7.4 >
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
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; + + /* * Output format */ switch (vin->format.pixelformat) {
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(+)