Re: [PATCH 1/6] media: Define MIPI CSI-2 data types in a shared header file

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

Hi Kieran,

On Wed, Jan 26, 2022 at 09:47:12AM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-01-23 16:08:52)
> > There are many CSI-2-related drivers in the media subsystem that come
> > with their own macros to handle the CSI-2 data types (or just hardcode
> > the numerical values). Provide a shared header with definitions for
> > those data types that driver can use.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  include/media/mipi-csi2.h | 45 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >  create mode 100644 include/media/mipi-csi2.h
> > 
> > diff --git a/include/media/mipi-csi2.h b/include/media/mipi-csi2.h
> > new file mode 100644
> > index 000000000000..392794e5badd
> > --- /dev/null
> > +++ b/include/media/mipi-csi2.h
> > @@ -0,0 +1,45 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * MIPI CSI-2 Data Types
> > + *
> > + * Copyright (C) 2022 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > + */
> > +
> > +#ifndef _MEDIA_MIPI_CSI2_H
> > +#define _MEDIA_MIPI_CSI2_H
> > +
> > +/* Short packet data types */
> > +#define MIPI_CSI2_DT_FS                        0x00
> > +#define MIPI_CSI2_DT_FE                        0x01
> > +#define MIPI_CSI2_DT_LS                        0x02
> > +#define MIPI_CSI2_DT_LE                        0x03
> > +#define MIPI_CSI2_DT_GENERIC_SHORT(n)  (0x08 + (n))    /* 0..7 */
> > +
> > +/* Long packet data types */
> > +#define MIPI_CSI2_DT_NULL              0x10
> > +#define MIPI_CSI2_DT_BLANKING          0x11
> > +#define MIPI_CSI2_DT_EMBEDDED_8B       0x12
> > +#define MIPI_CSI2_DT_YUV420_8B         0x18
> > +#define MIPI_CSI2_DT_YUV420_10B                0x19
> > +#define MIPI_CSI2_DT_YUV420_8B_LEGACY  0x1a
> > +#define MIPI_CSI2_DT_YUV420_8B_CS      0x1c
> > +#define MIPI_CSI2_DT_YUV420_10B_CS     0x1d
> > +#define MIPI_CSI2_DT_YUV422_8B         0x1e
> > +#define MIPI_CSI2_DT_YUV422_10B                0x1f
> > +#define MIPI_CSI2_DT_RGB444            0x20
> > +#define MIPI_CSI2_DT_RGB555            0x21
> > +#define MIPI_CSI2_DT_RGB565            0x22
> > +#define MIPI_CSI2_DT_RGB666            0x23
> > +#define MIPI_CSI2_DT_RGB888            0x24
> > +#define MIPI_CSI2_DT_RAW24             0x27
> > +#define MIPI_CSI2_DT_RAW6              0x28
> > +#define MIPI_CSI2_DT_RAW7              0x29
> > +#define MIPI_CSI2_DT_RAW8              0x2a
> > +#define MIPI_CSI2_DT_RAW10             0x2b
> > +#define MIPI_CSI2_DT_RAW12             0x2c
> > +#define MIPI_CSI2_DT_RAW14             0x2d
> > +#define MIPI_CSI2_DT_RAW16             0x2e
> > +#define MIPI_CSI2_DT_RAW20             0x2f
> > +#define MIPI_CSI2_DT_USER_DEFINED(n)   (0x30 + (n))    /* 0..7 */
> 
> I don't have an easy way to validate those values right now so as with
> Niklas I'll leave those to your judgement, and Pratyush's review.
> 
> Also along side Pratyush's comment, I concur that the mapping tables too
> could be common, but I suspect that's an even bigger topic as maybe that
> falls into the trap of also being common to DRM formats...

Same information could be shared. I usually push back against
centralizing the mapping between media bus codes and pixel formats, as
that's device-specific, but the CSI-2 specification has an informative
section with recommended memory formats, so that at least could be
shared among drivers.

> And finally, are these defines in a location that can be accessible from
> device tree? Or would it have to be further duplicated there still?

They're not, but we can move them if needed.

> For instance, the bindings for the Xilinx CSI2 RX explicitly list DT
> values to specify as the xlnx,csi-pxl-format which I think should also
> come from this common header definition.

That's a good point. I don't see how it could work with the DT schema
though. At the moment, we have

  xlnx,csi-pxl-format:
    description: [...]
    $ref: /schemas/types.yaml#/definitions/uint32
    oneOf:
      - minimum: 0x1e
        maximum: 0x24
      - minimum: 0x28
        maximum: 0x2f

and as far as I know, you can't #include a C header in the schema
itself. It could be done in the examples and the device trees themselves
though.

> For the patches here so far, I can't see anything stark that is wrong
> so for the series:
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> as further extending this to the device tree bindings can be done on
> top.
> 
> > +
> > +#endif /* _MEDIA_MIPI_CSI2_H */

-- 
Regards,

Laurent Pinchart