Re: [PATCH 4/5] media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder

[Ezequiel Garcia <ezequiel@collabora.com>]

Hi Mirela,

Thanks for the patch. It's nice to see support for i.MX8!

On Wed, 2019-11-06 at 17:20 +0200, Mirela Rabulea wrote:
> V4L2 driver for the JPEG encoder/decoder from i.MX8QXP/i.MX8QM application
> processors.
> The multi-planar buffers API is used.
> 
> Baseline and extended sequential jpeg decoding is supported.
> Progressive jpeg decoding is not supported by the IP.
> Supports encode and decode of various formats:
>      YUV444, YUV422, YUV420, RGB, ARGB, Gray
> YUV420 is the only multi-planar format supported.
> Minimum resolution is 64 x 64, maximum 8192 x 8192.
> The alignment requirements for the resolution depend on the format,
> multiple of 16 resolutions should work for all formats.
> 
> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> ---
>  drivers/media/platform/Kconfig                |    2 +
>  drivers/media/platform/Makefile               |    1 +
>  drivers/media/platform/imx-jpeg/Kconfig       |   10 +
>  drivers/media/platform/imx-jpeg/Makefile      |    3 +
>  drivers/media/platform/imx-jpeg/mxc-jpeg-hw.c |  168 ++
>  drivers/media/platform/imx-jpeg/mxc-jpeg-hw.h |  140 ++
>  drivers/media/platform/imx-jpeg/mxc-jpeg.c    | 2266 +++++++++++++++++++++++++
>  drivers/media/platform/imx-jpeg/mxc-jpeg.h    |  187 ++
>  8 files changed, 2777 insertions(+)
>  create mode 100644 drivers/media/platform/imx-jpeg/Kconfig
>  create mode 100644 drivers/media/platform/imx-jpeg/Makefile
>  create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg-hw.c
>  create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg-hw.h
>  create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg.c
>  create mode 100644 drivers/media/platform/imx-jpeg/mxc-jpeg.h
> 
> [..]

> +};
> +
> +/*
> + * default configuration stream, 64x64 yuv422
> + * split by JPEG marker, so it's easier to modify & use
> + */
> +static const unsigned char jpeg_soi[] = {0xFF, 0xD8};
> +static const unsigned char jpeg_app0[] = {0xFF, 0xE0,

I think it's time to re-consider creating some common code
for drivers that deal with JPEG parsing. I don't know
exactly how this should be done, but it's worth a try.

Having been there, it might sound unfair to request you to
take such action. However, someone has to do these kinds
of cleanups and improvements, sooner or later, if we want
to keep a clean codebase.

Hope this makes sense.

Regards,
Ezequiel