Hi Niklas, Thank you for the review. On Fri, Jul 24, 2020 at 8:37 PM Niklas <niklas.soderlund@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> > > Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > > --- > > 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
