Re: [PATCH v9 3/3] media: i2c: Introduce a driver for the Techwell TW9900 decoder

[Paul Kocialkowski]

Hi Mehdi,

On Fri 17 Nov 23, 16:42, Mehdi Djait wrote:
> The Techwell video decoder supports PAL, NTSC standards and
> has a parallel BT.656 output interface.
> 
> This commit adds support for this device, with basic support
> for NTSC and PAL, along with brightness and contrast controls.
> 
> The TW9900 is capable of automatic standard detection. This
> driver is implemented with support for PAL and NTSC
> autodetection.

Very nice work on this iteration, I am quite happy with it!

There is just one cosmetic comment below. With that fixed, you can add my:
Reviewed-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>

Cheers,

Paul

> Signed-off-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> ---
>  MAINTAINERS                |   6 +
>  drivers/media/i2c/Kconfig  |  15 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/tw9900.c | 777 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 799 insertions(+)
>  create mode 100644 drivers/media/i2c/tw9900.c

[...]

> diff --git a/drivers/media/i2c/tw9900.c b/drivers/media/i2c/tw9900.c
> new file mode 100644
> index 000000000000..895ca459087b
> --- /dev/null
> +++ b/drivers/media/i2c/tw9900.c
> @@ -0,0 +1,777 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the Techwell TW9900 multi-standard video decoder.
> + *
> + * Copyright (C) 2018 Fuzhou Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2020 Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx>
> + * Copyright (C) 2023 Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <media/media-entity.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define TW9900_REG_CHIP_ID			0x00
> +#define TW9900_REG_CHIP_STATUS			0x01
> +#define TW9900_REG_CHIP_STATUS_VDLOSS		BIT(7)
> +#define TW9900_REG_CHIP_STATUS_HLOCK		BIT(6)
> +#define TW9900_REG_OUT_FMT_CTL			0x03
> +#define TW9900_REG_OUT_FMT_CTL_STANDBY		0xA7
> +#define TW9900_REG_OUT_FMT_CTL_STREAMING	0xA0
> +#define TW9900_REG_CKHY_HSDLY			0x04
> +#define TW9900_REG_OUT_CTRL_I			0x05
> +#define TW9900_REG_ANALOG_CTL			0x06
> +#define TW9900_REG_CROP_HI			0x07
> +#define TW9900_REG_VDELAY_LO			0x08
> +#define TW9900_REG_VACTIVE_LO			0x09
> +#define TW9900_REG_HACTIVE_LO			0x0B
> +#define TW9900_REG_CNTRL1			0x0C
> +#define TW9900_REG_BRIGHT_CTL			0x10
> +#define TW9900_REG_CONTRAST_CTL			0x11
> +#define TW9900_REG_VBI_CNTL			0x19
> +#define TW9900_REG_ANAL_CTL_II			0x1A
> +#define TW9900_REG_OUT_CTRL_II			0x1B
> +#define TW9900_REG_STD				0x1C
> +#define TW9900_REG_STD_AUTO_PROGRESS		BIT(7)
> +#define TW9900_STDNOW_MASK			GENMASK(6, 4)
> +#define TW9900_REG_STDR				0x1D
> +#define TW9900_REG_MISSCNT			0x26
> +#define TW9900_REG_MISC_CTL_II			0x2F
> +#define TW9900_REG_VVBI				0x55

Please reintroduce the newline here...

> +#define TW9900_CHIP_ID				0x00
> +#define TW9900_STD_NTSC_M			0
> +#define TW9900_STD_PAL_BDGHI			1
> +#define TW9900_STD_AUTO				7

... and here to separate between register definitions and more general values.

> +#define TW9900_VIDEO_POLL_TRIES			20

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature