Hi Laurent and Pratyush On Mon, 24 Jan 2022 at 15:22, Pratyush Yadav <p.yadav@xxxxxx> wrote: > > Hi Laurent, > > Thanks for the patch. > > On 23/01/22 06:08PM, Laurent Pinchart wrote: > > 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@xxxxxxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxx> > > + */ > > + > > +#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 */ > > IIUC there is currently no way to actually capture packets with these > data types, and these are added here for completeness's sake, right? They aren't generally captured, but are of use. For Unicam we have a packet compare & capture trigger generally used for debug. However it can also be used for capturing the 16bit frame number attached to FS and FE events. It's been of use for devices such as Analog Devices ADV728[0|2]M which use the frame number to identify the field when sending interlaced content. > > + > > +/* 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 > > I have the CSI-2 spec v1.3, and it lists 0x27 as reserved under RGB > Image data, and I don't see a data type value for RAW24. Where did you > get this value from? > > > +#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 > > These two are also listed as reserved in the spec I have. Rest of the > values look good to me. I'm also only on v1.3, but otherwise agree that all the other values match. I see that MIPI are now up to v4.0, and their performance highlights[1] include RAW16 and RAW24 support, so I assume they have been added in a later revision. Dave [1] https://www.mipi.org/specifications/csi-2 > > +#define MIPI_CSI2_DT_USER_DEFINED(n) (0x30 + (n)) /* 0..7 */ > > + > > +#endif /* _MEDIA_MIPI_CSI2_H */ > > -- > > Regards, > > > > Laurent Pinchart > > > > I think this patch is a good idea in general, and it should remove a lot > of repetition in the drivers. > > BTW, I also see lots of drivers adding tables having mapping between > MBUS formats, FOURCC formats, bpp, data type, etc. It would be useful to > have those in a central place IMO. > > -- > Regards, > Pratyush Yadav > Texas Instruments Inc.
