Re: [PATCH v2] media: rcar-vin: Add support to select data pins for YCbCr422-8bit input

[Lad, Prabhakar]

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.

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

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