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@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 */
> > +
> > +/* 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@xxxxxxxxxxxxxxxx>
>
> as further extending this to the device tree bindings can be done on
> top.
>
> > +
> > +#endif /* _MEDIA_MIPI_CSI2_H */
--
Regards,
Laurent Pinchart
