diff mbox series

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

Message ID 20220123160857.24161-2-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: Centralize MIPI CSI-2 data types in shared header | expand

Commit Message

Laurent Pinchart Jan. 23, 2022, 4:08 p.m. UTC 
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

Comments

Niklas Söderlund Jan. 24, 2022, 9:06 a.m. UTC | #1 
Hi Laurent,

Thanks for your work.

On 2022-01-23 18:08:52 +0200, 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@ideasonboard.com>

I like this effort. I have not double checked each DT code with the spec 
but choose to trust you on those.

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  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 */
> +
> +#endif /* _MEDIA_MIPI_CSI2_H */
> -- 
> Regards,
> 
> Laurent Pinchart
>
Pratyush Yadav Jan. 24, 2022, 3:22 p.m. UTC | #2 
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@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 */

IIUC there is currently no way to actually capture packets with these 
data types, and these are added here for completeness's sake, right?

> +
> +/* 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.

> +#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.
Kieran Bingham Jan. 26, 2022, 9:47 a.m. UTC | #3 
Hi Laurent

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

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?

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.

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
>
Dave Stevenson Jan. 26, 2022, 11:50 a.m. UTC | #4 
Hi Laurent and Pratyush

On Mon, 24 Jan 2022 at 15:22, Pratyush Yadav <p.yadav@ti.com> 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@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 */
>
> 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.
Laurent Pinchart Jan. 26, 2022, 11:57 a.m. UTC | #5 
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 */
Laurent Pinchart Jan. 26, 2022, 11:59 a.m. UTC | #6 
Hello,

On Wed, Jan 26, 2022 at 11:50:21AM +0000, Dave Stevenson wrote:
> On Mon, 24 Jan 2022 at 15:22, Pratyush Yadav <p.yadav@ti.com> wrote:
> > 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@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 */
> >
> > 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.

I don't have access to more a more recent version of the CSI-2
specification, but I've gathered RAW16, RAW20 and RAW24 from out-of-tree
code. Sakari, could you confirm those values ?

> [1] https://www.mipi.org/specifications/csi-2
> 
> > > +#define MIPI_CSI2_DT_USER_DEFINED(n) (0x30 + (n))    /* 0..7 */
> > > +
> > > +#endif /* _MEDIA_MIPI_CSI2_H */
> >
> > 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.

I agree. Patches are welcome :-)
diff mbox series

Patch

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 */
+
+#endif /* _MEDIA_MIPI_CSI2_H */