Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] media: rcar-vin: Add support to select data pins
@ 2020-07-24 14:58 Lad Prabhakar
  2020-07-24 14:58 ` [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property Lad Prabhakar
  2020-07-24 14:58 ` [PATCH 2/2] media: rcar-vin: Add support to read " Lad Prabhakar
  0 siblings, 2 replies; 10+ messages in thread
From: Lad Prabhakar @ 2020-07-24 14:58 UTC (permalink / raw)
  To: Niklas, Mauro Carvalho Chehab, Rob Herring, linux-media,
	linux-renesas-soc
  Cc: devicetree, linux-kernel, Biju Das, Geert Uytterhoeven,
	Prabhakar, Lad Prabhakar

Hi All,

This patch series adds support to enable selecting data
lines via DT.

Cheers,
Prabhakar

Lad Prabhakar (2):
  dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g
    property
  media: rcar-vin: Add support to read renesas-vin-ycbcr-8b-g property

 Documentation/devicetree/bindings/media/renesas,vin.yaml | 13 +++++++++++++
 drivers/media/platform/rcar-vin/rcar-core.c              |  4 +++-
 drivers/media/platform/rcar-vin/rcar-dma.c               |  7 +++++++
 drivers/media/platform/rcar-vin/rcar-vin.h               |  2 ++
 4 files changed, 25 insertions(+), 1 deletion(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property
  2020-07-24 14:58 [PATCH 0/2] media: rcar-vin: Add support to select data pins Lad Prabhakar
@ 2020-07-24 14:58 ` Lad Prabhakar
  2020-07-24 19:37   ` Niklas
  2020-07-24 14:58 ` [PATCH 2/2] media: rcar-vin: Add support to read " Lad Prabhakar
  1 sibling, 1 reply; 10+ messages in thread
From: Lad Prabhakar @ 2020-07-24 14:58 UTC (permalink / raw)
  To: Niklas, Mauro Carvalho Chehab, Rob Herring, linux-media,
	linux-renesas-soc
  Cc: devicetree, linux-kernel, Biju Das, Geert Uytterhoeven,
	Prabhakar, Lad Prabhakar

Add a DT property "renesas-vin-ycbcr-8b-g" to select YCbCr422 8-bit data
input pins.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 Documentation/devicetree/bindings/media/renesas,vin.yaml | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
index 53c0a72..7dfb781 100644
--- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
@@ -106,6 +106,12 @@ properties:
 
           remote-endpoint: true
 
+          renesas-vin-ycbcr-8b-g:
+            type: boolean
+            description:
+              If present this property specifies to selects VIN_G[7:0] as data pins for YCbCr422 8-bit data.
+            default: false
+
         required:
           - remote-endpoint
 
@@ -168,6 +174,13 @@ properties:
 
               remote-endpoint: true
 
+              renesas-vin-ycbcr-8b-g:
+                type: boolean
+                description:
+                  If present this property specifies to selects VIN_G[7:0] as data pins for
+                  YCbCr422 8-bit data.
+                default: false
+
             required:
               - remote-endpoint
 
-- 
2.7.4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] media: rcar-vin: Add support to read renesas-vin-ycbcr-8b-g property
  2020-07-24 14:58 [PATCH 0/2] media: rcar-vin: Add support to select data pins Lad Prabhakar
  2020-07-24 14:58 ` [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property Lad Prabhakar
@ 2020-07-24 14:58 ` Lad Prabhakar
  1 sibling, 0 replies; 10+ messages in thread
From: Lad Prabhakar @ 2020-07-24 14:58 UTC (permalink / raw)
  To: Niklas, Mauro Carvalho Chehab, Rob Herring, linux-media,
	linux-renesas-soc
  Cc: devicetree, linux-kernel, Biju Das, Geert Uytterhoeven,
	Prabhakar, Lad Prabhakar

Add support to read "renesas-vin-ycbcr-8b-g" DT property and select
the data pins accordingly for YCbCr422-8bit input

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 4 +++-
 drivers/media/platform/rcar-vin/rcar-dma.c  | 7 +++++++
 drivers/media/platform/rcar-vin/rcar-vin.h  | 2 ++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 7440c89..5273110 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -624,6 +624,9 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
 	vin->parallel = rvpe;
 	vin->parallel->mbus_type = vep->bus_type;
 
+	vin->parallel->ycbcr_8b_g = fwnode_property_present(vep->base.local_fwnode,
+							    "renesas-vin-ycbcr-8b-g");
+
 	switch (vin->parallel->mbus_type) {
 	case V4L2_MBUS_PARALLEL:
 		vin_dbg(vin, "Found PARALLEL media bus\n");
@@ -659,7 +662,6 @@ static int rvin_parallel_init(struct rvin_dev *vin)
 
 	vin_dbg(vin, "Found parallel subdevice %pOF\n",
 		to_of_node(vin->parallel->asd.match.fwnode));
-
 	vin->notifier.ops = &rvin_parallel_notify_ops;
 	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
 	if (ret < 0) {
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 1a30cd0..5db4838 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -127,6 +127,8 @@
 #define VNDMR2_FTEV		(1 << 17)
 #define VNDMR2_VLV(n)		((n & 0xf) << 12)
 
+#define VNDMR2_YDS		BIT(22)
+
 /* Video n CSI2 Interface Mode Register (Gen3) */
 #define VNCSI_IFMD_DES1		(1 << 26)
 #define VNCSI_IFMD_DES0		(1 << 25)
@@ -698,6 +700,11 @@ static int rvin_setup(struct rvin_dev *vin)
 		/* Data Enable Polarity Select */
 		if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ENABLE_LOW)
 			dmr2 |= VNDMR2_CES;
+
+		if (vin->parallel->ycbcr_8b_g && vin->mbus_code == MEDIA_BUS_FMT_UYVY8_2X8)
+			dmr2 |= VNDMR2_YDS;
+		else
+			dmr2 &= ~VNDMR2_YDS;
 	}
 
 	/*
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index c19d077..62a4bf2 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -95,6 +95,7 @@ struct rvin_video_format {
  * @mbus_flags:	media bus configuration flags
  * @source_pad:	source pad of remote subdevice
  * @sink_pad:	sink pad of remote subdevice
+ * @ycbcr_8b_g:	select data pins for YCbCr422-8bit
  *
  */
 struct rvin_parallel_entity {
@@ -106,6 +107,7 @@ struct rvin_parallel_entity {
 
 	unsigned int source_pad;
 	unsigned int sink_pad;
+	bool ycbcr_8b_g;
 };
 
 /**
-- 
2.7.4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property
  2020-07-24 14:58 ` [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property Lad Prabhakar
@ 2020-07-24 19:37   ` Niklas
  2020-07-24 21:11     ` Lad, Prabhakar
  0 siblings, 1 reply; 10+ messages in thread
From: Niklas @ 2020-07-24 19:37 UTC (permalink / raw)
  To: Lad Prabhakar
  Cc: Mauro Carvalho Chehab, Rob Herring, linux-media,
	linux-renesas-soc, devicetree, linux-kernel, Biju Das,
	Geert Uytterhoeven, Prabhakar

Hi Lad,

Thanks for your patch.

On 2020-07-24 15:58:51 +0100, Lad Prabhakar wrote:
> Add a DT property "renesas-vin-ycbcr-8b-g" to select YCbCr422 8-bit data
> input pins.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  Documentation/devicetree/bindings/media/renesas,vin.yaml | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> index 53c0a72..7dfb781 100644
> --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> @@ -106,6 +106,12 @@ properties:
>  
>            remote-endpoint: true
>  
> +          renesas-vin-ycbcr-8b-g:

I think the preferred format for vendor specific properties are 
"<vendor>,<property>".

This nit apart I'm not sure a property is the right way here. Could it 
not be possible on some designs to have two different sensors one wired 
to DATA[7:0] and the other to DATA[15:8] and by controlling the 
VNDRM2_YDS register at runtime switch between the two? If so adding a DT 
property to hard-code one of the two options would prevent this. I fear 
we need to think of a runtime way to deal with this.

The best way to do that I think is to extend the port@0 node to allow 
for two endpoints, one for each of the two possible parallel sensors.  
This would then have to be expressed in the media graph and selection if 
YDS should be set or not depend on which media links are enabled.

> +            type: boolean
> +            description:
> +              If present this property specifies to selects VIN_G[7:0] as data pins for YCbCr422 8-bit data.
> +            default: false
> +
>          required:
>            - remote-endpoint
>  
> @@ -168,6 +174,13 @@ properties:
>  
>                remote-endpoint: true
>  
> +              renesas-vin-ycbcr-8b-g:
> +                type: boolean
> +                description:
> +                  If present this property specifies to selects VIN_G[7:0] as data pins for
> +                  YCbCr422 8-bit data.
> +                default: false
> +
>              required:
>                - remote-endpoint
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property
  2020-07-24 19:37   ` Niklas
@ 2020-07-24 21:11     ` Lad, Prabhakar
  2020-07-25  8:11       ` Niklas
  0 siblings, 1 reply; 10+ messages in thread
From: Lad, Prabhakar @ 2020-07-24 21:11 UTC (permalink / raw)
  To: Niklas
  Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, linux-media,
	Linux-Renesas,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Biju Das, Geert Uytterhoeven

Hi Niklas,

Thank you for the review.

On Fri, Jul 24, 2020 at 8:37 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
>
> Hi Lad,
>
> Thanks for your patch.
>
> On 2020-07-24 15:58:51 +0100, Lad Prabhakar wrote:
> > Add a DT property "renesas-vin-ycbcr-8b-g" to select YCbCr422 8-bit data
> > input pins.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > index 53c0a72..7dfb781 100644
> > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > @@ -106,6 +106,12 @@ properties:
> >
> >            remote-endpoint: true
> >
> > +          renesas-vin-ycbcr-8b-g:
>
> I think the preferred format for vendor specific properties are
> "<vendor>,<property>".
>
Indeed and I had it as renesas,vin-ycbcr-8b-g but dt_bindings_check
complained about it.

> This nit apart I'm not sure a property is the right way here. Could it
> not be possible on some designs to have two different sensors one wired
> to DATA[7:0] and the other to DATA[15:8] and by controlling the
> VNDRM2_YDS register at runtime switch between the two? If so adding a DT
> property to hard-code one of the two options would prevent this. I fear
> we need to think of a runtime way to deal with this.
>
Aha Gen2 and Gen3 hardware manuals have a bit different description
about the YDS field. (I was working R8a7742 SoC so I referred Gen2
manual)

> The best way to do that I think is to extend the port@0 node to allow
> for two endpoints, one for each of the two possible parallel sensors.
> This would then have to be expressed in the media graph and selection if
> YDS should be set or not depend on which media links are enabled.
>
In that case how do we handle endpoint matching each would have two
subdevs to be matched. And in case non media-ctl cases we cannot
switch between subdevs.

Cheers,
--Prabhakar

> > +            type: boolean
> > +            description:
> > +              If present this property specifies to selects VIN_G[7:0] as data pins for YCbCr422 8-bit data.
> > +            default: false
> > +
> >          required:
> >            - remote-endpoint
> >
> > @@ -168,6 +174,13 @@ properties:
> >
> >                remote-endpoint: true
> >
> > +              renesas-vin-ycbcr-8b-g:
> > +                type: boolean
> > +                description:
> > +                  If present this property specifies to selects VIN_G[7:0] as data pins for
> > +                  YCbCr422 8-bit data.
> > +                default: false
> > +
> >              required:
> >                - remote-endpoint
> >
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property
  2020-07-24 21:11     ` Lad, Prabhakar
@ 2020-07-25  8:11       ` Niklas
  2020-07-25 22:23         ` Lad, Prabhakar
  0 siblings, 1 reply; 10+ messages in thread
From: Niklas @ 2020-07-25  8:11 UTC (permalink / raw)
  To: Lad, Prabhakar
  Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, linux-media,
	Linux-Renesas,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Biju Das, Geert Uytterhoeven

Hi Lad,

On 2020-07-24 22:11:31 +0100, Lad, Prabhakar wrote:
> Hi Niklas,
> 
> Thank you for the review.
> 
> On Fri, Jul 24, 2020 at 8:37 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> >
> > Hi Lad,
> >
> > Thanks for your patch.
> >
> > On 2020-07-24 15:58:51 +0100, Lad Prabhakar wrote:
> > > Add a DT property "renesas-vin-ycbcr-8b-g" to select YCbCr422 8-bit data
> > > input pins.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > index 53c0a72..7dfb781 100644
> > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > @@ -106,6 +106,12 @@ properties:
> > >
> > >            remote-endpoint: true
> > >
> > > +          renesas-vin-ycbcr-8b-g:
> >
> > I think the preferred format for vendor specific properties are
> > "<vendor>,<property>".
> >
> Indeed and I had it as renesas,vin-ycbcr-8b-g but dt_bindings_check
> complained about it.

I see, what was the error?

> 
> > This nit apart I'm not sure a property is the right way here. Could it
> > not be possible on some designs to have two different sensors one wired
> > to DATA[7:0] and the other to DATA[15:8] and by controlling the
> > VNDRM2_YDS register at runtime switch between the two? If so adding a DT
> > property to hard-code one of the two options would prevent this. I fear
> > we need to think of a runtime way to deal with this.
> >
> Aha Gen2 and Gen3 hardware manuals have a bit different description
> about the YDS field. (I was working R8a7742 SoC so I referred Gen2
> manual)

Ahh, I think we should use the Gen3 names as I find them overall an 
improvement over the Gen2 ones.

> 
> > The best way to do that I think is to extend the port@0 node to allow
> > for two endpoints, one for each of the two possible parallel sensors.
> > This would then have to be expressed in the media graph and selection if
> > YDS should be set or not depend on which media links are enabled.
> >
> In that case how do we handle endpoint matching each would have two
> subdevs to be matched.

It would be handle in the same was as the multiple endpoints in port@1.

> And in case non media-ctl cases we cannot
> switch between subdevs.

For the Gen2 none media graph enabled mode this could be handled with 
the S_INPUT ioctl. For this feature to be merged however I it needs to 
be possible to select input both in Gen2 and Gen3 I'm afraid. I'm hoping 
to one day breakout the non MC part of this driver into a new one and 
mark it as deprecated and switch to the MC code paths for Gen2.

> 
> Cheers,
> --Prabhakar
> 
> > > +            type: boolean
> > > +            description:
> > > +              If present this property specifies to selects VIN_G[7:0] as data pins for YCbCr422 8-bit data.
> > > +            default: false
> > > +
> > >          required:
> > >            - remote-endpoint
> > >
> > > @@ -168,6 +174,13 @@ properties:
> > >
> > >                remote-endpoint: true
> > >
> > > +              renesas-vin-ycbcr-8b-g:
> > > +                type: boolean
> > > +                description:
> > > +                  If present this property specifies to selects VIN_G[7:0] as data pins for
> > > +                  YCbCr422 8-bit data.
> > > +                default: false
> > > +
> > >              required:
> > >                - remote-endpoint
> > >
> > > --
> > > 2.7.4
> > >
> >
> > --
> > Regards,
> > Niklas Söderlund

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property
  2020-07-25  8:11       ` Niklas
@ 2020-07-25 22:23         ` Lad, Prabhakar
  2020-07-31 20:19           ` Rob Herring
  2020-08-01  9:17           ` Niklas
  0 siblings, 2 replies; 10+ messages in thread
From: Lad, Prabhakar @ 2020-07-25 22:23 UTC (permalink / raw)
  To: Niklas
  Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, linux-media,
	Linux-Renesas,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Biju Das, Geert Uytterhoeven

Hi Niklas,

On Sat, Jul 25, 2020 at 9:11 AM Niklas <niklas.soderlund@ragnatech.se> wrote:
>
> Hi Lad,
>
> On 2020-07-24 22:11:31 +0100, Lad, Prabhakar wrote:
> > Hi Niklas,
> >
> > Thank you for the review.
> >
> > On Fri, Jul 24, 2020 at 8:37 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> > >
> > > Hi Lad,
> > >
> > > Thanks for your patch.
> > >
> > > On 2020-07-24 15:58:51 +0100, Lad Prabhakar wrote:
> > > > Add a DT property "renesas-vin-ycbcr-8b-g" to select YCbCr422 8-bit data
> > > > input pins.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > index 53c0a72..7dfb781 100644
> > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > @@ -106,6 +106,12 @@ properties:
> > > >
> > > >            remote-endpoint: true
> > > >
> > > > +          renesas-vin-ycbcr-8b-g:
> > >
> > > I think the preferred format for vendor specific properties are
> > > "<vendor>,<property>".
> > >
> > Indeed and I had it as renesas,vin-ycbcr-8b-g but dt_bindings_check
> > complained about it.
>
> I see, what was the error?
>
  CHKDT   Documentation/devicetree/bindings/media/renesas,vin.yaml
/home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
properties:port:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
{'type': 'boolean', 'description': 'If present this property specifies
to selects VIN_G[7:0] as data pins for YCbCr422 8-bit data.',
'default': False} is not valid under any of the given schemas
(Possible causes of the failure):
    /home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
properties:port:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
'not' is a required property

/home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
properties:ports:properties:port@0:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
{'type': 'boolean', 'description': 'If present this property specifies
to selects VIN_G[7:0] as data pins for YCbCr422 8-bit data.',
'default': False} is not valid under any of the given schemas
(Possible causes of the failure):
    /home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
properties:ports:properties:port@0:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
'not' is a required property

Documentation/devicetree/bindings/Makefile:20: recipe for target
'Documentation/devicetree/bindings/media/renesas,vin.example.dts'
failed
make[1]: *** [Documentation/devicetree/bindings/media/renesas,vin.example.dts]
Error 1
Makefile:1334: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2


> >
> > > This nit apart I'm not sure a property is the right way here. Could it
> > > not be possible on some designs to have two different sensors one wired
> > > to DATA[7:0] and the other to DATA[15:8] and by controlling the
> > > VNDRM2_YDS register at runtime switch between the two? If so adding a DT
> > > property to hard-code one of the two options would prevent this. I fear
> > > we need to think of a runtime way to deal with this.
> > >
> > Aha Gen2 and Gen3 hardware manuals have a bit different description
> > about the YDS field. (I was working R8a7742 SoC so I referred Gen2
> > manual)
>
> Ahh, I think we should use the Gen3 names as I find them overall an
> improvement over the Gen2 ones.
>
Agreed.

> >
> > > The best way to do that I think is to extend the port@0 node to allow
> > > for two endpoints, one for each of the two possible parallel sensors.
> > > This would then have to be expressed in the media graph and selection if
> > > YDS should be set or not depend on which media links are enabled.
> > >
> > In that case how do we handle endpoint matching each would have two
> > subdevs to be matched.
>
> It would be handle in the same was as the multiple endpoints in port@1.
>
> > And in case non media-ctl cases we cannot
> > switch between subdevs.
>
> For the Gen2 none media graph enabled mode this could be handled with
> the S_INPUT ioctl. For this feature to be merged however I it needs to
> be possible to select input both in Gen2 and Gen3 I'm afraid.
Ohh yes S_INPUT could be used to switch inputs. But  how do we decide
YDS needs to be enabled, for example with the below dts say vin3 is
parallel bus split into 2x 8-bit bus one connected to a ov5640 sensor
and other connected to ov7725 sensor. Should we use data-shift
property for the second vin endpoint (vin3ep1) to enable YDS ?

&i2c3 {
    pinctrl-0 = <&i2c3_pins>;
    pinctrl-names = "default";

    status = "okay";
    clock-frequency = <400000>;

    ov7725@21 {
        status = "disabled";
        compatible = "ovti,ov7725";
        reg = <0x21>;
        clocks = <&mclk_cam4>;

        port {
            ov7725_3: endpoint {
                bus-width = <8>;
                bus-type = <6>;
                remote-endpoint = <&vin3ep0>;
            };
        };
    };

    ov5640@3c {
        status = "okay";
        compatible = "ovti,ov5640";
        reg = <0x3c>;
        clocks = <&mclk_cam4>;
        clock-names = "xclk";

        port {
            ov5640_3: endpoint {
                bus-width = <8>;
                bus-type = <6>;
                remote-endpoint = <&vin3ep1>;
            };
        };
    };
};

&vin3 {
    status = "okay";
    pinctrl-0 = <&vin3_pins>;
    pinctrl-names = "default";

    port {
        #address-cells = <1>;
        #size-cells = <0>;

        vin3ep0: endpoint {
            remote-endpoint = <&ov7725_3>;
            bus-width = <8>;
        };

        vin3ep1: endpoint {
            remote-endpoint = <&ov5640_3>;
            bus-width = <8>;
        };

    };
};


>I'm hoping to one day breakout the non MC part of this driver into a new one and
> mark it as deprecated and switch to the MC code paths for Gen2.
>
That sounds like a good idea.

Cheers,
--Prabhakar

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property
  2020-07-25 22:23         ` Lad, Prabhakar
@ 2020-07-31 20:19           ` Rob Herring
  2020-08-01  9:17           ` Niklas
  1 sibling, 0 replies; 10+ messages in thread
From: Rob Herring @ 2020-07-31 20:19 UTC (permalink / raw)
  To: Lad, Prabhakar
  Cc: Niklas, Lad Prabhakar, Mauro Carvalho Chehab, linux-media,
	Linux-Renesas,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Biju Das, Geert Uytterhoeven

On Sat, Jul 25, 2020 at 11:23:13PM +0100, Lad, Prabhakar wrote:
> Hi Niklas,
> 
> On Sat, Jul 25, 2020 at 9:11 AM Niklas <niklas.soderlund@ragnatech.se> wrote:
> >
> > Hi Lad,
> >
> > On 2020-07-24 22:11:31 +0100, Lad, Prabhakar wrote:
> > > Hi Niklas,
> > >
> > > Thank you for the review.
> > >
> > > On Fri, Jul 24, 2020 at 8:37 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> > > >
> > > > Hi Lad,
> > > >
> > > > Thanks for your patch.
> > > >
> > > > On 2020-07-24 15:58:51 +0100, Lad Prabhakar wrote:
> > > > > Add a DT property "renesas-vin-ycbcr-8b-g" to select YCbCr422 8-bit data
> > > > > input pins.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 13 +++++++++++++
> > > > >  1 file changed, 13 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > index 53c0a72..7dfb781 100644
> > > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > @@ -106,6 +106,12 @@ properties:
> > > > >
> > > > >            remote-endpoint: true
> > > > >
> > > > > +          renesas-vin-ycbcr-8b-g:
> > > >
> > > > I think the preferred format for vendor specific properties are
> > > > "<vendor>,<property>".
> > > >
> > > Indeed and I had it as renesas,vin-ycbcr-8b-g but dt_bindings_check
> > > complained about it.
> >
> > I see, what was the error?
> >
>   CHKDT   Documentation/devicetree/bindings/media/renesas,vin.yaml
> /home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
> properties:port:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
> {'type': 'boolean', 'description': 'If present this property specifies
> to selects VIN_G[7:0] as data pins for YCbCr422 8-bit data.',
> 'default': False} is not valid under any of the given schemas
> (Possible causes of the failure):
>     /home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
> properties:port:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
> 'not' is a required property
> 
> /home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
> properties:ports:properties:port@0:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
> {'type': 'boolean', 'description': 'If present this property specifies
> to selects VIN_G[7:0] as data pins for YCbCr422 8-bit data.',
> 'default': False} is not valid under any of the given schemas
> (Possible causes of the failure):
>     /home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
> properties:ports:properties:port@0:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
> 'not' is a required property

The errors here aren't that helpful. The problem is 'default: false' 
doesn't make sense for a boolean type as false is always not present for 
DT.

Rob

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property
  2020-07-25 22:23         ` Lad, Prabhakar
  2020-07-31 20:19           ` Rob Herring
@ 2020-08-01  9:17           ` Niklas
  2020-08-03  9:31             ` Geert Uytterhoeven
  1 sibling, 1 reply; 10+ messages in thread
From: Niklas @ 2020-08-01  9:17 UTC (permalink / raw)
  To: Lad, Prabhakar
  Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, linux-media,
	Linux-Renesas,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Biju Das, Geert Uytterhoeven

Hi Lad,

On 2020-07-25 23:23:13 +0100, Lad, Prabhakar wrote:
> Hi Niklas,
> 
> On Sat, Jul 25, 2020 at 9:11 AM Niklas <niklas.soderlund@ragnatech.se> wrote:
> >
> > Hi Lad,
> >
> > On 2020-07-24 22:11:31 +0100, Lad, Prabhakar wrote:
> > > Hi Niklas,
> > >
> > > Thank you for the review.
> > >
> > > On Fri, Jul 24, 2020 at 8:37 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> > > >
> > > > Hi Lad,
> > > >
> > > > Thanks for your patch.
> > > >
> > > > On 2020-07-24 15:58:51 +0100, Lad Prabhakar wrote:
> > > > > Add a DT property "renesas-vin-ycbcr-8b-g" to select YCbCr422 8-bit data
> > > > > input pins.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 13 +++++++++++++
> > > > >  1 file changed, 13 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > index 53c0a72..7dfb781 100644
> > > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > @@ -106,6 +106,12 @@ properties:
> > > > >
> > > > >            remote-endpoint: true
> > > > >
> > > > > +          renesas-vin-ycbcr-8b-g:
> > > >
> > > > I think the preferred format for vendor specific properties are
> > > > "<vendor>,<property>".
> > > >
> > > Indeed and I had it as renesas,vin-ycbcr-8b-g but dt_bindings_check
> > > complained about it.
> >
> > I see, what was the error?
> >
>   CHKDT   Documentation/devicetree/bindings/media/renesas,vin.yaml
> /home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
> properties:port:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
> {'type': 'boolean', 'description': 'If present this property specifies
> to selects VIN_G[7:0] as data pins for YCbCr422 8-bit data.',
> 'default': False} is not valid under any of the given schemas
> (Possible causes of the failure):
>     /home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
> properties:port:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
> 'not' is a required property
> 
> /home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
> properties:ports:properties:port@0:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
> {'type': 'boolean', 'description': 'If present this property specifies
> to selects VIN_G[7:0] as data pins for YCbCr422 8-bit data.',
> 'default': False} is not valid under any of the given schemas
> (Possible causes of the failure):
>     /home/prasmi/work/renasas/g2n/renesas-devel/Documentation/devicetree/bindings/media/renesas,vin.yaml:
> properties:ports:properties:port@0:properties:endpoint:properties:renesas,vin-ycbcr-8b-g:
> 'not' is a required property
> 
> Documentation/devicetree/bindings/Makefile:20: recipe for target
> 'Documentation/devicetree/bindings/media/renesas,vin.example.dts'
> failed
> make[1]: *** [Documentation/devicetree/bindings/media/renesas,vin.example.dts]
> Error 1
> Makefile:1334: recipe for target 'dt_binding_check' failed
> make: *** [dt_binding_check] Error 2
> 
> 
> > >
> > > > This nit apart I'm not sure a property is the right way here. Could it
> > > > not be possible on some designs to have two different sensors one wired
> > > > to DATA[7:0] and the other to DATA[15:8] and by controlling the
> > > > VNDRM2_YDS register at runtime switch between the two? If so adding a DT
> > > > property to hard-code one of the two options would prevent this. I fear
> > > > we need to think of a runtime way to deal with this.
> > > >
> > > Aha Gen2 and Gen3 hardware manuals have a bit different description
> > > about the YDS field. (I was working R8a7742 SoC so I referred Gen2
> > > manual)
> >
> > Ahh, I think we should use the Gen3 names as I find them overall an
> > improvement over the Gen2 ones.
> >
> Agreed.
> 
> > >
> > > > The best way to do that I think is to extend the port@0 node to allow
> > > > for two endpoints, one for each of the two possible parallel sensors.
> > > > This would then have to be expressed in the media graph and selection if
> > > > YDS should be set or not depend on which media links are enabled.
> > > >
> > > In that case how do we handle endpoint matching each would have two
> > > subdevs to be matched.
> >
> > It would be handle in the same was as the multiple endpoints in port@1.
> >
> > > And in case non media-ctl cases we cannot
> > > switch between subdevs.
> >
> > For the Gen2 none media graph enabled mode this could be handled with
> > the S_INPUT ioctl. For this feature to be merged however I it needs to
> > be possible to select input both in Gen2 and Gen3 I'm afraid.
> Ohh yes S_INPUT could be used to switch inputs. But  how do we decide
> YDS needs to be enabled, for example with the below dts say vin3 is
> parallel bus split into 2x 8-bit bus one connected to a ov5640 sensor
> and other connected to ov7725 sensor. Should we use data-shift
> property for the second vin endpoint (vin3ep1) to enable YDS ?

Using data-shift is a great idea! If I understand your use-case you 
currently only have one sensor attached on the parallel bus right? If so 
we can postpone the multi sensor part until it's needed and just learn 
the VIN driver about data-shift. From the documentation,

  - data-shift: on the parallel data busses, if bus-width is used to 
    specify the number of data lines, data-shift can be used to specify 
    which data lines are used, e.g. "bus-width=<8>; data-shift=<2>;" 
    means, that lines 9:2 are used.

So in this case would not specifying data-shift=<8> solve the DT 
description problem? The VIN driver still needs to learn about this tho.

> 
> &i2c3 {
>     pinctrl-0 = <&i2c3_pins>;
>     pinctrl-names = "default";
> 
>     status = "okay";
>     clock-frequency = <400000>;
> 
>     ov7725@21 {
>         status = "disabled";
>         compatible = "ovti,ov7725";
>         reg = <0x21>;
>         clocks = <&mclk_cam4>;
> 
>         port {
>             ov7725_3: endpoint {
>                 bus-width = <8>;
>                 bus-type = <6>;
>                 remote-endpoint = <&vin3ep0>;
>             };
>         };
>     };
> 
>     ov5640@3c {
>         status = "okay";
>         compatible = "ovti,ov5640";
>         reg = <0x3c>;
>         clocks = <&mclk_cam4>;
>         clock-names = "xclk";
> 
>         port {
>             ov5640_3: endpoint {
>                 bus-width = <8>;
>                 bus-type = <6>;
>                 remote-endpoint = <&vin3ep1>;
>             };
>         };
>     };
> };
> 
> &vin3 {
>     status = "okay";
>     pinctrl-0 = <&vin3_pins>;
>     pinctrl-names = "default";
> 
>     port {
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         vin3ep0: endpoint {
>             remote-endpoint = <&ov7725_3>;
>             bus-width = <8>;
>         };
> 
>         vin3ep1: endpoint {
>             remote-endpoint = <&ov5640_3>;
>             bus-width = <8>;
>         };
> 
>     };
> };
> 
> 
> >I'm hoping to one day breakout the non MC part of this driver into a new one and
> > mark it as deprecated and switch to the MC code paths for Gen2.
> >
> That sounds like a good idea.
> 
> Cheers,
> --Prabhakar

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property
  2020-08-01  9:17           ` Niklas
@ 2020-08-03  9:31             ` Geert Uytterhoeven
  0 siblings, 0 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2020-08-03  9:31 UTC (permalink / raw)
  To: Niklas
  Cc: Lad, Prabhakar, Lad Prabhakar, Mauro Carvalho Chehab,
	Rob Herring, linux-media, Linux-Renesas,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Biju Das

Hi Niklas,

On Sat, Aug 1, 2020 at 11:18 AM Niklas <niklas.soderlund@ragnatech.se> wrote:
> On 2020-07-25 23:23:13 +0100, Lad, Prabhakar wrote:
> > On Sat, Jul 25, 2020 at 9:11 AM Niklas <niklas.soderlund@ragnatech.se> wrote:
> > > On 2020-07-24 22:11:31 +0100, Lad, Prabhakar wrote:
> > > > On Fri, Jul 24, 2020 at 8:37 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> > > > > On 2020-07-24 15:58:51 +0100, Lad Prabhakar wrote:
> > > > > > Add a DT property "renesas-vin-ycbcr-8b-g" to select YCbCr422 8-bit data
> > > > > > input pins.

> > > > > This nit apart I'm not sure a property is the right way here. Could it
> > > > > not be possible on some designs to have two different sensors one wired
> > > > > to DATA[7:0] and the other to DATA[15:8] and by controlling the
> > > > > VNDRM2_YDS register at runtime switch between the two? If so adding a DT
> > > > > property to hard-code one of the two options would prevent this. I fear
> > > > > we need to think of a runtime way to deal with this.
> > > > >
> > > > Aha Gen2 and Gen3 hardware manuals have a bit different description
> > > > about the YDS field. (I was working R8a7742 SoC so I referred Gen2
> > > > manual)
> > >
> > > Ahh, I think we should use the Gen3 names as I find them overall an
> > > improvement over the Gen2 ones.
> > >
> > Agreed.
> >
> > > >
> > > > > The best way to do that I think is to extend the port@0 node to allow
> > > > > for two endpoints, one for each of the two possible parallel sensors.
> > > > > This would then have to be expressed in the media graph and selection if
> > > > > YDS should be set or not depend on which media links are enabled.
> > > > >
> > > > In that case how do we handle endpoint matching each would have two
> > > > subdevs to be matched.
> > >
> > > It would be handle in the same was as the multiple endpoints in port@1.
> > >
> > > > And in case non media-ctl cases we cannot
> > > > switch between subdevs.
> > >
> > > For the Gen2 none media graph enabled mode this could be handled with
> > > the S_INPUT ioctl. For this feature to be merged however I it needs to
> > > be possible to select input both in Gen2 and Gen3 I'm afraid.
> > Ohh yes S_INPUT could be used to switch inputs. But  how do we decide
> > YDS needs to be enabled, for example with the below dts say vin3 is
> > parallel bus split into 2x 8-bit bus one connected to a ov5640 sensor
> > and other connected to ov7725 sensor. Should we use data-shift
> > property for the second vin endpoint (vin3ep1) to enable YDS ?
>
> Using data-shift is a great idea! If I understand your use-case you
> currently only have one sensor attached on the parallel bus right? If so
> we can postpone the multi sensor part until it's needed and just learn
> the VIN driver about data-shift. From the documentation,
>
>   - data-shift: on the parallel data busses, if bus-width is used to
>     specify the number of data lines, data-shift can be used to specify
>     which data lines are used, e.g. "bus-width=<8>; data-shift=<2>;"
>     means, that lines 9:2 are used.
>
> So in this case would not specifying data-shift=<8> solve the DT
> description problem? The VIN driver still needs to learn about this tho.

And the PFC drivers need to be extended with the data8_sft8 groups
(present in the BSP), right?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24 14:58 [PATCH 0/2] media: rcar-vin: Add support to select data pins Lad Prabhakar
2020-07-24 14:58 ` [PATCH 1/2] dt-bindings: media: renesas,vin: Document renesas-vin-ycbcr-8b-g property Lad Prabhakar
2020-07-24 19:37   ` Niklas
2020-07-24 21:11     ` Lad, Prabhakar
2020-07-25  8:11       ` Niklas
2020-07-25 22:23         ` Lad, Prabhakar
2020-07-31 20:19           ` Rob Herring
2020-08-01  9:17           ` Niklas
2020-08-03  9:31             ` Geert Uytterhoeven
2020-07-24 14:58 ` [PATCH 2/2] media: rcar-vin: Add support to read " Lad Prabhakar

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git