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