Hi Lad, On 2020-08-03 20:17:54 +0100, Lad, Prabhakar wrote: > Hi Niklas, > > Thank you for the review. > > On Mon, Aug 3, 2020 at 7:06 PM Niklas <niklas.soderlund@xxxxxxxxxxxx> wrote: > > > > Hi Lad, > > > > Thanks for your work. > > > > On 2020-08-03 17:02:53 +0100, Lad Prabhakar wrote: > > > Select the data pins for YCbCr422-8bit input format depending on > > > bus_width and data_shift passed as part of DT. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > > > --- > > > Changes for v2: > > > * Dropped DT binding documentation patch > > > * Select the data pins depending on bus-width and data-shift > > > > I like this v2 much better then v1, nice work! > > > > > > > > v1 - > > > https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=323799 > > > --- > > > drivers/media/platform/rcar-vin/rcar-core.c | 5 +++++ > > > drivers/media/platform/rcar-vin/rcar-dma.c | 7 +++++++ > > > drivers/media/platform/rcar-vin/rcar-vin.h | 5 +++++ > > > 3 files changed, 17 insertions(+) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > > index 7440c8965d27..55005d86928d 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > > @@ -624,6 +624,11 @@ static int rvin_parallel_parse_v4l2(struct device *dev, > > > vin->parallel = rvpe; > > > vin->parallel->mbus_type = vep->bus_type; > > > > > > + /* select VInDATA[15:8] pins for YCbCr422-8bit format */ > > > + if (vep->bus.parallel.bus_width == BUS_WIDTH_8 && > > > + vep->bus.parallel.data_shift == DATA_SHIFT_8) > > > + vin->parallel->ycbcr_8b_g = true; > > > + > > > > I would store the bus_width and bus_shift values in the struct > > rvin_parallel_entity and evaluate them in place rater then create a flag > > for this specific use-case.. > > > Ok will do that. > > > Also according to the documentation is the check correct? Do we not wish > > to use the new mode when bus_width == 16 and bus_shift == 8. The check > > you have here seems to describe a 8 lane bus where 0 lanes are used. > > > bus-width is the actual data lines used, so bus_width == 16 and > bus_shift == 8 would mean use lines 23:8, so just check for bus_width > == 8 and bus_shift == 8 should be sufficient. As you and Geert points out I was wrong, they should indeed both be 8. > > > I think you should also verify that bus_shift is either 0 or 8 as that > > is all the driver supports. > > > Not sure if thats correct.In that case this patch wont make sense, I > believed we agreed upon we determine the YDS depending on both > bus-width and bus-shift. I'm sorry I think I lost you :-) The driver is not capable of supporting bus_width = 8 and bus_shift = 2 right? Maybe we are talking about different things. What I tried to say (updated with the knowledge of that bus_width should indeed be 8 and not 16) was that would it make sens to with bus_width=8 allow for a bus_shift value other then 0 or 8? What for example would the driver do if the value was 2? > > On iWave G21D-Q7 for VI2 interface VI2_G* pins are connected to SoC > and for VIN3 interface Vi3_DATA* pins are connected. So in this case > the capture only works for VIN2 only if YDS bit is set for 8-bit 422, > and for VIN3 capture only works if YDS is 0 > > &vin2 { > status = "okay"; > pinctrl-0 = <&vin2_pins>; > pinctrl-names = "default"; > > port { > #address-cells = <1>; > #size-cells = <0>; > > vin2ep: endpoint { > remote-endpoint = <&ov7725_2>; > bus-width = <8>; > data-shift = <8>; > }; > }; > }; > > &vin3 { > status = "okay"; > pinctrl-0 = <&vin3_pins>; > pinctrl-names = "default"; > > port { > #address-cells = <1>; > #size-cells = <0>; > > vin3ep: endpoint { > remote-endpoint = <&ov7725_3>; > bus-width = <8>; > }; > }; > }; > > > > > switch (vin->parallel->mbus_type) { > > > case V4L2_MBUS_PARALLEL: > > > vin_dbg(vin, "Found PARALLEL media bus\n"); > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > > > index 1a30cd036371..5db483877d65 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) > > > > This should be grouped with the other VNDMR2_* macros and not on its > > own. Also it should be sorted so it should be inserted between > > VNDMR2_CES and VNDMR2_FTEV. > > > > Also I know BIT() is a nice macro but the rest of the driver uses (1 << > > 22), please do the same for this one. > > > Sure will take care of it. > > > > + > > > /* 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; > > > > dmr2 is already unitized and YDS is cleared, no need to clear it again > > if you don't wish to set it. Taking this and the comments above into > > account this would become something like (not tested), > > > Agreed. > > > switch (vin->mbus_code) { > > case MEDIA_BUS_FMT_UYVY8_2X8: > > if (vin->parallel->bus_width == 16 && vin->parallel->bus_shift == 8) > > dmr2 |= VNDMR2_YDS; > > break; > > default: > > break; > > } > > > > > } > > > > > > /* > > > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h > > > index c19d077ce1cb..3126fee9a89b 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h > > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h > > > @@ -87,6 +87,9 @@ struct rvin_video_format { > > > u8 bpp; > > > }; > > > > > > +#define BUS_WIDTH_8 8 > > > +#define DATA_SHIFT_8 8 > > > > As pointed out by Geert, not so useful, use 8 in the code :-) > > > Agreed will drop it. > > Cheers, > Prabhakar -- Regards, Niklas Söderlund
