Re: [PATCH v15 7/7] media: nuvoton: Add driver for NPCM video capture and encoding engine

[Hans Verkuil]

Hi Marvin,

Just one small comment:

On 20/09/2023 04:28, Marvin Lin wrote:
> Add driver for Video Capture/Differentiation Engine (VCD) and Encoding
> Compression Engine (ECE) present on Nuvoton NPCM SoCs. As described in
> the datasheet NPCM750D_DS_Rev_1.0, the VCD can capture frames from
> digital video input and compare two frames in memory, and then the ECE
> can compress the frame data into HEXTILE format. This driver implements
> V4L2 interfaces and provides user controls to support KVM feature, also
> tested with VNC Viewer ver.6.22.826 and openbmc/obmc-ikvm.
> 
> Signed-off-by: Marvin Lin <milkfafa@xxxxxxxxx>
> ---
>  MAINTAINERS                                 |   12 +
>  drivers/media/platform/Kconfig              |    1 +
>  drivers/media/platform/Makefile             |    1 +
>  drivers/media/platform/nuvoton/Kconfig      |   15 +
>  drivers/media/platform/nuvoton/Makefile     |    2 +
>  drivers/media/platform/nuvoton/npcm-regs.h  |  152 ++
>  drivers/media/platform/nuvoton/npcm-video.c | 1830 +++++++++++++++++++
>  7 files changed, 2013 insertions(+)
>  create mode 100644 drivers/media/platform/nuvoton/Kconfig
>  create mode 100644 drivers/media/platform/nuvoton/Makefile
>  create mode 100644 drivers/media/platform/nuvoton/npcm-regs.h
>  create mode 100644 drivers/media/platform/nuvoton/npcm-video.c
> 

<snip>

> diff --git a/drivers/media/platform/nuvoton/npcm-video.c b/drivers/media/platform/nuvoton/npcm-video.c
> new file mode 100644
> index 000000000000..aabb6077a128
> --- /dev/null
> +++ b/drivers/media/platform/nuvoton/npcm-video.c
> @@ -0,0 +1,1830 @@

<snip>

> +
> +/*
> + * This control value is set when a buffer is dequeued by userspace, i.e. in
> + * npcm_video_buf_finish function.
> + */
> +static const struct v4l2_ctrl_config npcm_ctrl_rect_count = {
> +	.id = V4L2_CID_NPCM_RECT_COUNT,
> +	.name = "NPCM Compressed Hextile Rectangle Count",

This name is too long, it should be max 31 characters. Dropping the "Compressed " part
might be best, I think that word isn't needed.

> +	.type = V4L2_CTRL_TYPE_INTEGER,
> +	.min = 0,
> +	.max = (MAX_WIDTH / RECT_W) * (MAX_HEIGHT / RECT_H),
> +	.step = 1,
> +	.def = 0,
> +};

With this fixed, and the sparse warnings fixed, it should be ready for merging.

Regards,

	Hans