Re: [PATCH] media: dt-bindings: media: renesas,csi2: Update data-lanes property

[Niklas Söderlund]

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@xxxxxxxxxxxxxx>
> > > > ---
> > > >  .../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