Re: [PATCH] media: rcar-vin: Add support for RAW10

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Jun 18, 2024 at 05:41:03PM +0300, Laurent Pinchart wrote:
> Hi Geert,
> 
> On Wed, Apr 17, 2024 at 03:34:36PM +0200, Geert Uytterhoeven wrote:
> > On Wed, Apr 17, 2024 at 2:06 PM Niklas Söderlund wrote:
> > > Some R-Car SoCs are capable of capturing RAW10. Add support for it
> > > using the V4L2_PIX_FMT_Y10 pixel format, which I think is the correct
> > > format to express RAW10 unpacked to users.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > Thanks for your patch!
> > 
> > I am no VIN or V4L2 expert, but the register bits LGTM, so
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > 
> > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > @@ -780,6 +782,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > >         case MEDIA_BUS_FMT_Y8_1X8:
> > >                 vnmc |= VNMC_INF_RAW8;
> > >                 break;
> > > +       case MEDIA_BUS_FMT_Y10_1X10:
> > > +               vnmc |= VNMC_INF_RGB666;
> > 
> > The actual meaning of this bit is not uniform across all SoCs.
> > On R-Car V3U it means (partial) 16 bpp, on R-Car Gen3 it means 18 bpp.
> 
> The INF bits have different meanings depending on the VIN input. What
> you refer to above for V3U is for the CSI-2 input, while for the rest of
> Gen3 you quote the values for the parallel input. Value 111 is
> documented as "prohibit" for the CSI-2 input on the rest of Gen3.

To be precise, for V3U the documentation indicates "Input from Channel
Selector", not CSI-2. V3U has no parallel input.

The macros for the INF bits mix names for different types of inputs, it
could be a good idea to clean this up.

> > > +               break;
> > >         default:
> > >                 break;
> > >         }

-- 
Regards,

Laurent Pinchart