All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property
@ 2022-01-13 10:32 Lad Prabhakar
  2022-01-17  8:11 ` Jacopo Mondi
  0 siblings, 1 reply; 9+ messages in thread
From: Lad Prabhakar @ 2022-01-13 10:32 UTC (permalink / raw)
  To: Niklas Söderlund, Mauro Carvalho Chehab, Rob Herring
  Cc: Geert Uytterhoeven, Prabhakar, Biju Das, Lad Prabhakar,
	linux-media, linux-renesas-soc, devicetree, linux-kernel

CSI-2 (CSI4LNK0) on R-Car and RZ/G2 supports 4-lane mode which is already
handled by rcar-csi2.c driver. This patch updates the data-lanes property
to describe the same.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../devicetree/bindings/media/renesas,csi2.yaml          | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/renesas,csi2.yaml b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
index e6a036721082..064a0a4c5737 100644
--- a/Documentation/devicetree/bindings/media/renesas,csi2.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
@@ -67,7 +67,14 @@ properties:
                 maxItems: 1
 
               data-lanes:
-                maxItems: 1
+                items:
+                  minItems: 1
+                  maxItems: 4
+                  items:
+                    - const: 1
+                    - const: 2
+                    - const: 3
+                    - const: 4
 
             required:
               - clock-lanes
-- 
2.17.1


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

* Re: [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property
  2022-01-13 10:32 [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property Lad Prabhakar
@ 2022-01-17  8:11 ` Jacopo Mondi
  2022-01-17  9:23   ` Niklas Söderlund
  2022-01-18  9:11   ` Lad, Prabhakar
  0 siblings, 2 replies; 9+ messages in thread
From: Jacopo Mondi @ 2022-01-17  8:11 UTC (permalink / raw)
  To: Lad Prabhakar
  Cc: Niklas Söderlund, Mauro Carvalho Chehab, Rob Herring,
	Geert Uytterhoeven, Prabhakar, Biju Das, linux-media,
	linux-renesas-soc, devicetree, linux-kernel

Hello Prabhakar,

On Thu, Jan 13, 2022 at 10:32:14AM +0000, Lad Prabhakar wrote:
> CSI-2 (CSI4LNK0) on R-Car and RZ/G2 supports 4-lane mode which is already
> handled by rcar-csi2.c driver. This patch updates the data-lanes property
> to describe the same.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../devicetree/bindings/media/renesas,csi2.yaml          | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/media/renesas,csi2.yaml b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> index e6a036721082..064a0a4c5737 100644
> --- a/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> @@ -67,7 +67,14 @@ properties:
>                  maxItems: 1
>
>                data-lanes:
> -                maxItems: 1
> +                items:
> +                  minItems: 1
> +                  maxItems: 4
> +                  items:
> +                    - const: 1
> +                    - const: 2
> +                    - const: 3
> +                    - const: 4

Seeing "maxItems: 1" there confuses me too, as the property is an
array of data-lanes, but I'm afraid your change does not what you
intend as it would allow you to specify the number of data lanes as an
integer rather than as an array.

I think it would probably be correct to set

                data-lanes: true

(maybe maxItems: 1 is correct already)

And restrict the number of valid combinations in the board DTS file
with a construct like:

    data-lanes:
      oneOf:
        - items:
            - const: 1
            - const: 2
            - const: 3
            - const: 4
        - items:
            - const: 1
            - const: 2

Thanks
   j

>
>              required:
>                - clock-lanes
> --
> 2.17.1
>

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

* Re: [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property
  2022-01-17  8:11 ` Jacopo Mondi
@ 2022-01-17  9:23   ` Niklas Söderlund
  2022-01-17 10:00     ` Jacopo Mondi
  2022-01-18  9:11   ` Lad, Prabhakar
  1 sibling, 1 reply; 9+ messages in thread
From: Niklas Söderlund @ 2022-01-17  9:23 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring,
	Geert Uytterhoeven, Prabhakar, Biju Das, linux-media,
	linux-renesas-soc, devicetree, linux-kernel

Hello Jacopo,

On 2022-01-17 09:11:10 +0100, Jacopo Mondi wrote:
> Hello Prabhakar,
> 
> On Thu, Jan 13, 2022 at 10:32:14AM +0000, Lad Prabhakar wrote:
> > CSI-2 (CSI4LNK0) on R-Car and RZ/G2 supports 4-lane mode which is already
> > handled by rcar-csi2.c driver. This patch updates the data-lanes property
> > to describe the same.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../devicetree/bindings/media/renesas,csi2.yaml          | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/renesas,csi2.yaml b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > index e6a036721082..064a0a4c5737 100644
> > --- a/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > +++ b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > @@ -67,7 +67,14 @@ properties:
> >                  maxItems: 1
> >
> >                data-lanes:
> > -                maxItems: 1
> > +                items:
> > +                  minItems: 1
> > +                  maxItems: 4
> > +                  items:
> > +                    - const: 1
> > +                    - const: 2
> > +                    - const: 3
> > +                    - const: 4
> 
> Seeing "maxItems: 1" there confuses me too, as the property is an
> array of data-lanes, but I'm afraid your change does not what you
> intend as it would allow you to specify the number of data lanes as an
> integer rather than as an array.
> 
> I think it would probably be correct to set
> 
>                 data-lanes: true
> 
> (maybe maxItems: 1 is correct already)
> 
> And restrict the number of valid combinations in the board DTS file
> with a construct like:
> 
>     data-lanes:
>       oneOf:
>         - items:
>             - const: 1
>             - const: 2
>             - const: 3
>             - const: 4
>         - items:
>             - const: 1
>             - const: 2

I don't think this is correct, what if data lanes 2 and 3 are used?

> 
> Thanks
>    j
> 
> >
> >              required:
> >                - clock-lanes
> > --
> > 2.17.1
> >

-- 
Kind Regards,
Niklas Söderlund

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

* Re: [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property
  2022-01-17  9:23   ` Niklas Söderlund
@ 2022-01-17 10:00     ` Jacopo Mondi
  2022-01-18 10:33       ` Niklas Söderlund
  0 siblings, 1 reply; 9+ messages in thread
From: Jacopo Mondi @ 2022-01-17 10:00 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring,
	Geert Uytterhoeven, Prabhakar, Biju Das, linux-media,
	linux-renesas-soc, devicetree, linux-kernel

Hi Niklas,

On Mon, Jan 17, 2022 at 10:23:28AM +0100, Niklas Söderlund wrote:
> Hello Jacopo,
>
> On 2022-01-17 09:11:10 +0100, Jacopo Mondi wrote:
> > Hello Prabhakar,
> >
> > On Thu, Jan 13, 2022 at 10:32:14AM +0000, Lad Prabhakar wrote:
> > > CSI-2 (CSI4LNK0) on R-Car and RZ/G2 supports 4-lane mode which is already
> > > handled by rcar-csi2.c driver. This patch updates the data-lanes property
> > > to describe the same.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  .../devicetree/bindings/media/renesas,csi2.yaml          | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/renesas,csi2.yaml b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > index e6a036721082..064a0a4c5737 100644
> > > --- a/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > +++ b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > @@ -67,7 +67,14 @@ properties:
> > >                  maxItems: 1
> > >
> > >                data-lanes:
> > > -                maxItems: 1
> > > +                items:
> > > +                  minItems: 1
> > > +                  maxItems: 4
> > > +                  items:
> > > +                    - const: 1
> > > +                    - const: 2
> > > +                    - const: 3
> > > +                    - const: 4
> >
> > Seeing "maxItems: 1" there confuses me too, as the property is an
> > array of data-lanes, but I'm afraid your change does not what you
> > intend as it would allow you to specify the number of data lanes as an
> > integer rather than as an array.
> >
> > I think it would probably be correct to set
> >
> >                 data-lanes: true
> >
> > (maybe maxItems: 1 is correct already)
> >
> > And restrict the number of valid combinations in the board DTS file
> > with a construct like:
> >
> >     data-lanes:
> >       oneOf:
> >         - items:
> >             - const: 1
> >             - const: 2
> >             - const: 3
> >             - const: 4
> >         - items:
> >             - const: 1
> >             - const: 2
>
> I don't think this is correct, what if data lanes 2 and 3 are used?
>

These were examples that allow you to accept <1 2> and <1 2 3 4> as
valid properties. If other combinations are accepted they can be
specified there, in your example, <2 3> with

             - items:
               - const: 2
               - const: 3

As lane re-reordering is quite unusual as a feature (afaik) there are
usually just an handful of supported combinations for 1, 2 and 4 data
lanes setups.

If full lane re-ordering is supported then it's enough to set
data-lanes: true and accepts all combinations.

Also, the reason why imho the property should go in the board DTS and
not in the SoC .dtsi is that not all the available data lanes of the
IP-core might be routed out on a specific board.

That's at least my understanding which I would be glad to be disproved
as specifying the valid combinations in each board dts is rather
un-convenient.

Thanks
   j

> >
> > Thanks
> >    j
> >
> > >
> > >              required:
> > >                - clock-lanes
> > > --
> > > 2.17.1
> > >
>
> --
> Kind Regards,
> Niklas Söderlund

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

* Re: [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property
  2022-01-17  8:11 ` Jacopo Mondi
  2022-01-17  9:23   ` Niklas Söderlund
@ 2022-01-18  9:11   ` Lad, Prabhakar
  2022-01-18 10:55     ` Lad, Prabhakar
  2022-01-18 16:34     ` Jacopo Mondi
  1 sibling, 2 replies; 9+ messages in thread
From: Lad, Prabhakar @ 2022-01-18  9:11 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Lad Prabhakar, Niklas Söderlund, Mauro Carvalho Chehab,
	Rob Herring, Geert Uytterhoeven, Biju Das, linux-media,
	Linux-Renesas,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML

Hi Jacopo,

Thank you for the review.

On Mon, Jan 17, 2022 at 8:11 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hello Prabhakar,
>
> On Thu, Jan 13, 2022 at 10:32:14AM +0000, Lad Prabhakar wrote:
> > CSI-2 (CSI4LNK0) on R-Car and RZ/G2 supports 4-lane mode which is already
> > handled by rcar-csi2.c driver. This patch updates the data-lanes property
> > to describe the same.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../devicetree/bindings/media/renesas,csi2.yaml          | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/renesas,csi2.yaml b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > index e6a036721082..064a0a4c5737 100644
> > --- a/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > +++ b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > @@ -67,7 +67,14 @@ properties:
> >                  maxItems: 1
> >
> >                data-lanes:
> > -                maxItems: 1
> > +                items:
> > +                  minItems: 1
> > +                  maxItems: 4
> > +                  items:
> > +                    - const: 1
> > +                    - const: 2
> > +                    - const: 3
> > +                    - const: 4
>
> Seeing "maxItems: 1" there confuses me too, as the property is an
> array of data-lanes, but I'm afraid your change does not what you
> intend as it would allow you to specify the number of data lanes as an
> integer rather than as an array.
>
Agreed, what do you think of the below instead?

            properties:
              data-lanes:
                minItems: 1
                maxItems: 4
                items:
                  maximum: 4

The above should handle all the possible mix and match of the lanes.

> I think it would probably be correct to set
>
>                 data-lanes: true
>
> (maybe maxItems: 1 is correct already)
>
> And restrict the number of valid combinations in the board DTS file
> with a construct like:
>
>     data-lanes:
>       oneOf:
>         - items:
>             - const: 1
>             - const: 2
>             - const: 3
>             - const: 4
>         - items:
>             - const: 1
>             - const: 2
>
I haven't come across dts files having such constraints is it allowed,
could you point me to a example.

Cheers,
Prabhakar

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

* Re: [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property
  2022-01-17 10:00     ` Jacopo Mondi
@ 2022-01-18 10:33       ` Niklas Söderlund
  2022-01-18 16:22         ` Jacopo Mondi
  0 siblings, 1 reply; 9+ messages in thread
From: Niklas Söderlund @ 2022-01-18 10:33 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring,
	Geert Uytterhoeven, Prabhakar, Biju Das, linux-media,
	linux-renesas-soc, devicetree, linux-kernel

Hi Jacopo,

Thanks for your feedback.

On 2022-01-17 11:00:40 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Mon, Jan 17, 2022 at 10:23:28AM +0100, Niklas Söderlund wrote:
> > Hello Jacopo,
> >
> > On 2022-01-17 09:11:10 +0100, Jacopo Mondi wrote:
> > > Hello Prabhakar,
> > >
> > > On Thu, Jan 13, 2022 at 10:32:14AM +0000, Lad Prabhakar wrote:
> > > > CSI-2 (CSI4LNK0) on R-Car and RZ/G2 supports 4-lane mode which is already
> > > > handled by rcar-csi2.c driver. This patch updates the data-lanes property
> > > > to describe the same.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  .../devicetree/bindings/media/renesas,csi2.yaml          | 9 ++++++++-
> > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/renesas,csi2.yaml b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > > index e6a036721082..064a0a4c5737 100644
> > > > --- a/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > > @@ -67,7 +67,14 @@ properties:
> > > >                  maxItems: 1
> > > >
> > > >                data-lanes:
> > > > -                maxItems: 1
> > > > +                items:
> > > > +                  minItems: 1
> > > > +                  maxItems: 4
> > > > +                  items:
> > > > +                    - const: 1
> > > > +                    - const: 2
> > > > +                    - const: 3
> > > > +                    - const: 4
> > >
> > > Seeing "maxItems: 1" there confuses me too, as the property is an
> > > array of data-lanes, but I'm afraid your change does not what you
> > > intend as it would allow you to specify the number of data lanes as an
> > > integer rather than as an array.
> > >
> > > I think it would probably be correct to set
> > >
> > >                 data-lanes: true
> > >
> > > (maybe maxItems: 1 is correct already)
> > >
> > > And restrict the number of valid combinations in the board DTS file
> > > with a construct like:
> > >
> > >     data-lanes:
> > >       oneOf:
> > >         - items:
> > >             - const: 1
> > >             - const: 2
> > >             - const: 3
> > >             - const: 4
> > >         - items:
> > >             - const: 1
> > >             - const: 2
> >
> > I don't think this is correct, what if data lanes 2 and 3 are used?
> >
> 
> These were examples that allow you to accept <1 2> and <1 2 3 4> as
> valid properties. If other combinations are accepted they can be
> specified there, in your example, <2 3> with
> 
>              - items:
>                - const: 2
>                - const: 3
> 
> As lane re-reordering is quite unusual as a feature (afaik) there are
> usually just an handful of supported combinations for 1, 2 and 4 data
> lanes setups.

R-Car CSI-2 hardware and driver supports full lane swapping, see the 
LSWAP register and usage of struct rcar_csi2.lane_swap.

I think it's a good idea to extend the binding description to limit the 
data-lanes property to an array of max 4 items where each value use is 
ether a 1, 2, 3 or 4. But it must allow for any combination of the 
values.

> 
> If full lane re-ordering is supported then it's enough to set
> data-lanes: true and accepts all combinations.
> 
> Also, the reason why imho the property should go in the board DTS and
> not in the SoC .dtsi is that not all the available data lanes of the
> IP-core might be routed out on a specific board.
> 
> That's at least my understanding which I would be glad to be disproved
> as specifying the valid combinations in each board dts is rather
> un-convenient.
> 
> Thanks
>    j
> 
> > >
> > > Thanks
> > >    j
> > >
> > > >
> > > >              required:
> > > >                - clock-lanes
> > > > --
> > > > 2.17.1
> > > >
> >
> > --
> > Kind Regards,
> > Niklas Söderlund

-- 
Kind Regards,
Niklas Söderlund

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

* Re: [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property
  2022-01-18  9:11   ` Lad, Prabhakar
@ 2022-01-18 10:55     ` Lad, Prabhakar
  2022-01-18 16:34     ` Jacopo Mondi
  1 sibling, 0 replies; 9+ messages in thread
From: Lad, Prabhakar @ 2022-01-18 10:55 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Lad Prabhakar, Niklas Söderlund, Mauro Carvalho Chehab,
	Rob Herring, Geert Uytterhoeven, Biju Das, linux-media,
	Linux-Renesas,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML

On Tue, Jan 18, 2022 at 9:11 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
>
> Hi Jacopo,
>
> Thank you for the review.
>
> On Mon, Jan 17, 2022 at 8:11 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hello Prabhakar,
> >
> > On Thu, Jan 13, 2022 at 10:32:14AM +0000, Lad Prabhakar wrote:
> > > CSI-2 (CSI4LNK0) on R-Car and RZ/G2 supports 4-lane mode which is already
> > > handled by rcar-csi2.c driver. This patch updates the data-lanes property
> > > to describe the same.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  .../devicetree/bindings/media/renesas,csi2.yaml          | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/renesas,csi2.yaml b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > index e6a036721082..064a0a4c5737 100644
> > > --- a/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > +++ b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > @@ -67,7 +67,14 @@ properties:
> > >                  maxItems: 1
> > >
> > >                data-lanes:
> > > -                maxItems: 1
> > > +                items:
> > > +                  minItems: 1
> > > +                  maxItems: 4
> > > +                  items:
> > > +                    - const: 1
> > > +                    - const: 2
> > > +                    - const: 3
> > > +                    - const: 4
> >
> > Seeing "maxItems: 1" there confuses me too, as the property is an
> > array of data-lanes, but I'm afraid your change does not what you
> > intend as it would allow you to specify the number of data lanes as an
> > integer rather than as an array.
> >
> Agreed, what do you think of the below instead?
>
>             properties:
>               data-lanes:
>                 minItems: 1
>                 maxItems: 4
uniqueItems: true

can go in as well, to avoid duplicate lane numbers.

>                 items:
>                   maximum: 4
>
Cheers,
Prabhakar

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

* Re: [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property
  2022-01-18 10:33       ` Niklas Söderlund
@ 2022-01-18 16:22         ` Jacopo Mondi
  0 siblings, 0 replies; 9+ messages in thread
From: Jacopo Mondi @ 2022-01-18 16:22 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring,
	Geert Uytterhoeven, Prabhakar, Biju Das, linux-media,
	linux-renesas-soc, devicetree, linux-kernel

Hi Niklas,

On Tue, Jan 18, 2022 at 11:33:08AM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2022-01-17 11:00:40 +0100, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Mon, Jan 17, 2022 at 10:23:28AM +0100, Niklas Söderlund wrote:
> > > Hello Jacopo,
> > >
> > > On 2022-01-17 09:11:10 +0100, Jacopo Mondi wrote:
> > > > Hello Prabhakar,
> > > >
> > > > On Thu, Jan 13, 2022 at 10:32:14AM +0000, Lad Prabhakar wrote:
> > > > > CSI-2 (CSI4LNK0) on R-Car and RZ/G2 supports 4-lane mode which is already
> > > > > handled by rcar-csi2.c driver. This patch updates the data-lanes property
> > > > > to describe the same.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > >  .../devicetree/bindings/media/renesas,csi2.yaml          | 9 ++++++++-
> > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,csi2.yaml b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > > > index e6a036721082..064a0a4c5737 100644
> > > > > --- a/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > > > @@ -67,7 +67,14 @@ properties:
> > > > >                  maxItems: 1
> > > > >
> > > > >                data-lanes:
> > > > > -                maxItems: 1
> > > > > +                items:
> > > > > +                  minItems: 1
> > > > > +                  maxItems: 4
> > > > > +                  items:
> > > > > +                    - const: 1
> > > > > +                    - const: 2
> > > > > +                    - const: 3
> > > > > +                    - const: 4
> > > >
> > > > Seeing "maxItems: 1" there confuses me too, as the property is an
> > > > array of data-lanes, but I'm afraid your change does not what you
> > > > intend as it would allow you to specify the number of data lanes as an
> > > > integer rather than as an array.
> > > >
> > > > I think it would probably be correct to set
> > > >
> > > >                 data-lanes: true
> > > >
> > > > (maybe maxItems: 1 is correct already)
> > > >
> > > > And restrict the number of valid combinations in the board DTS file
> > > > with a construct like:
> > > >
> > > >     data-lanes:
> > > >       oneOf:
> > > >         - items:
> > > >             - const: 1
> > > >             - const: 2
> > > >             - const: 3
> > > >             - const: 4
> > > >         - items:
> > > >             - const: 1
> > > >             - const: 2
> > >
> > > I don't think this is correct, what if data lanes 2 and 3 are used?
> > >
> >
> > These were examples that allow you to accept <1 2> and <1 2 3 4> as
> > valid properties. If other combinations are accepted they can be
> > specified there, in your example, <2 3> with
> >
> >              - items:
> >                - const: 2
> >                - const: 3
> >
> > As lane re-reordering is quite unusual as a feature (afaik) there are
> > usually just an handful of supported combinations for 1, 2 and 4 data
> > lanes setups.
>
> R-Car CSI-2 hardware and driver supports full lane swapping, see the
> LSWAP register and usage of struct rcar_csi2.lane_swap.

Uh, I missed that. So indeed restricting the possible combinations in
the .dts file is a no-go :0

>
> I think it's a good idea to extend the binding description to limit the
> data-lanes property to an array of max 4 items where each value use is
> ether a 1, 2, 3 or 4. But it must allow for any combination of the
> values.
>

Agreed then.

Looking at the definition in video-interfaces.txt
  data-lanes:
    $ref: /schemas/types.yaml#/definitions/uint32-array
    minItems: 1
    maxItems: 8
    items:
      # Assume up to 9 physical lane indices
      maximum: 8

Should the R-Car CSI-2 bindings report (validated with
dt_binding_check)

      data-lanes:
        maxItems: 4
        items:
          maximum: 4

Thanks
   j

> >
> > If full lane re-ordering is supported then it's enough to set
> > data-lanes: true and accepts all combinations.
> >
> > Also, the reason why imho the property should go in the board DTS and
> > not in the SoC .dtsi is that not all the available data lanes of the
> > IP-core might be routed out on a specific board.
> >
> > That's at least my understanding which I would be glad to be disproved
> > as specifying the valid combinations in each board dts is rather
> > un-convenient.
> >
> > Thanks
> >    j
> >
> > > >
> > > > Thanks
> > > >    j
> > > >
> > > > >
> > > > >              required:
> > > > >                - clock-lanes
> > > > > --
> > > > > 2.17.1
> > > > >
> > >
> > > --
> > > Kind Regards,
> > > Niklas Söderlund
>
> --
> Kind Regards,
> Niklas Söderlund

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

* Re: [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property
  2022-01-18  9:11   ` Lad, Prabhakar
  2022-01-18 10:55     ` Lad, Prabhakar
@ 2022-01-18 16:34     ` Jacopo Mondi
  1 sibling, 0 replies; 9+ messages in thread
From: Jacopo Mondi @ 2022-01-18 16:34 UTC (permalink / raw)
  To: Lad, Prabhakar
  Cc: Lad Prabhakar, Niklas Söderlund, Mauro Carvalho Chehab,
	Rob Herring, Geert Uytterhoeven, Biju Das, linux-media,
	Linux-Renesas,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML

Hi Prabhakar

On Tue, Jan 18, 2022 at 09:11:42AM +0000, Lad, Prabhakar wrote:
> Hi Jacopo,
>
> Thank you for the review.
>
> On Mon, Jan 17, 2022 at 8:11 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hello Prabhakar,
> >
> > On Thu, Jan 13, 2022 at 10:32:14AM +0000, Lad Prabhakar wrote:
> > > CSI-2 (CSI4LNK0) on R-Car and RZ/G2 supports 4-lane mode which is already
> > > handled by rcar-csi2.c driver. This patch updates the data-lanes property
> > > to describe the same.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  .../devicetree/bindings/media/renesas,csi2.yaml          | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/renesas,csi2.yaml b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > index e6a036721082..064a0a4c5737 100644
> > > --- a/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > +++ b/Documentation/devicetree/bindings/media/renesas,csi2.yaml
> > > @@ -67,7 +67,14 @@ properties:
> > >                  maxItems: 1
> > >
> > >                data-lanes:
> > > -                maxItems: 1
> > > +                items:
> > > +                  minItems: 1
> > > +                  maxItems: 4
> > > +                  items:
> > > +                    - const: 1
> > > +                    - const: 2
> > > +                    - const: 3
> > > +                    - const: 4
> >
> > Seeing "maxItems: 1" there confuses me too, as the property is an
> > array of data-lanes, but I'm afraid your change does not what you
> > intend as it would allow you to specify the number of data lanes as an
> > integer rather than as an array.
> >
> Agreed, what do you think of the below instead?
>
>             properties:
>               data-lanes:
>                 minItems: 1
>                 maxItems: 4
>                 items:
>                   maximum: 4
>

Sorry, I should have read your reply first :)
even better with your suggested uniqueItems

> The above should handle all the possible mix and match of the lanes.
>
> > I think it would probably be correct to set
> >
> >                 data-lanes: true
> >
> > (maybe maxItems: 1 is correct already)
> >
> > And restrict the number of valid combinations in the board DTS file
> > with a construct like:
> >
> >     data-lanes:
> >       oneOf:
> >         - items:
> >             - const: 1
> >             - const: 2
> >             - const: 3
> >             - const: 4
> >         - items:
> >             - const: 1
> >             - const: 2
> >
> I haven't come across dts files having such constraints is it allowed,
> could you point me to a example.

I see some

Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml-        properties:
Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml:          data-lanes:
Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml-            oneOf:
Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml-              - items:
Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml-                  - const: 1
Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml-                  - const: 2
Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml-                  - const: 3
Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml-                  - const: 4
Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml-              - items:
Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml-                  - const: 1
Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml-                  - const: 2
Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml-

Documentation/devicetree/bindings/media/i2c/imx258.yaml-        properties:
Documentation/devicetree/bindings/media/i2c/imx258.yaml:          data-lanes:
Documentation/devicetree/bindings/media/i2c/imx258.yaml-            oneOf:
Documentation/devicetree/bindings/media/i2c/imx258.yaml-              - items:
Documentation/devicetree/bindings/media/i2c/imx258.yaml-                  - const: 1
Documentation/devicetree/bindings/media/i2c/imx258.yaml-                  - const: 2
Documentation/devicetree/bindings/media/i2c/imx258.yaml-                  - const: 3
Documentation/devicetree/bindings/media/i2c/imx258.yaml-                  - const: 4
Documentation/devicetree/bindings/media/i2c/imx258.yaml-              - items:
Documentation/devicetree/bindings/media/i2c/imx258.yaml-                  - const: 1
Documentation/devicetree/bindings/media/i2c/imx258.yaml-                  - const: 2

Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml-        properties:
Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml:          data-lanes:
Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml-            anyOf:
Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml-              - items:
Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml-                  - const: 1
Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml-                  - const: 2
Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml-              - items:
Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml-                  - const: 1
Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml-                  - const: 2
Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml-                  - const: 3
Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml-                  - const: 4

But yes, most bindings simply report

              data-lanes:
                minItems: 1
                maxItems: 4

Which allows all combinations, including repetitions, so they're
probably wrong.

Thanks
  j

>
> Cheers,
> Prabhakar

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

end of thread, other threads:[~2022-01-18 16:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-13 10:32 [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property Lad Prabhakar
2022-01-17  8:11 ` Jacopo Mondi
2022-01-17  9:23   ` Niklas Söderlund
2022-01-17 10:00     ` Jacopo Mondi
2022-01-18 10:33       ` Niklas Söderlund
2022-01-18 16:22         ` Jacopo Mondi
2022-01-18  9:11   ` Lad, Prabhakar
2022-01-18 10:55     ` Lad, Prabhakar
2022-01-18 16:34     ` Jacopo Mondi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.