Re: [PATCH 3/6] media: rcar-vin: Handle data-active property

[jacopo mondi <jacopo@jmondi.org>]

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

Hi Niklas,

On Wed, May 16, 2018 at 11:58:47PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2018-05-16 18:32:29 +0200, Jacopo Mondi wrote:
> > The data-active property has to be specified when running with embedded
> > synchronization. The VIN peripheral can use HSYNC in place of CLOCKENB
> > when the CLOCKENB pin is not connected, this requires explicit
> > synchronization to be in use.
>
> Is this really the intent of the data-active property? I read the
> video-interfaces.txt document as such as if no hsync-active,
> vsync-active and data-active we should use the embedded synchronization
> else set the polarity for the requested pins. What am I not
> understanding here?

Almost correct.

The presence of hsync-active, vsync-active and field-evev-active
properties determinate the bus type we're running on. If none of the
is specified, the bus is marked 'BT656' and we assume the system is
using embedded synchronization.

data-active does not take part in the bus identification, and my
reasoning was the other way around as explained in reply to your
comment on [2/6], and as explained there my reasoning is probably
wrong, and we should set CHS -only- when running with explicit
synchronization, instead of making it mandatory when running with
embedded syncs.

Thanks and sorry for my confusion.

    j
>
> >
> > Now that the driver supports 'data-active' property, it makes not sense
> > to zero the mbus configuration flags when running with implicit synch
> > (V4L2_MBUS_BT656).
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index d3072e1..075d08f 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -531,15 +531,21 @@ static int rvin_digital_parse_v4l2(struct device *dev,
> >  		return -ENOTCONN;
> >
> >  	vin->mbus_cfg.type = vep->bus_type;
> > +	vin->mbus_cfg.flags = vep->bus.parallel.flags;
> >
> >  	switch (vin->mbus_cfg.type) {
> >  	case V4L2_MBUS_PARALLEL:
> >  		vin_dbg(vin, "Found PARALLEL media bus\n");
> > -		vin->mbus_cfg.flags = vep->bus.parallel.flags;
> >  		break;
> >  	case V4L2_MBUS_BT656:
> >  		vin_dbg(vin, "Found BT656 media bus\n");
> > -		vin->mbus_cfg.flags = 0;
> > +
> > +		if (!(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_HIGH) &&
> > +		    !(vin->mbus_cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW)) {
> > +			vin_err(vin,
> > +				"Missing data enable signal polarity property\n");
>
> I fear this can't be an error as that would break backward comp ability
> with existing dtb's.
>
> > +			return -EINVAL;
> > +		}
> >  		break;
> >  	default:
> >  		vin_err(vin, "Unknown media bus type\n");
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund

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