[PATCH v4 0/3] media: i2c: Introduce driver for the TW9900 decoder

[PATCH v4 0/3] media: i2c: Introduce driver for the TW9900 decoder

[Maxime Chevallier]

Hello everyone,

This is the fourth version of the series adding support for the Techwell
TW9900 multi standard decoder. It's a pretty simple decoder compared to
the TW9910, since it doesn't have a built-in scaler/crop engine.

This V4 addresses one warning detected by the 0day bot, along with a
binding example validation error.

Any feedback is appreciated,

Thanks,

Maxime

Maxime Chevallier (3):
  dt-bindings: vendor-prefixes: Add techwell vendor prefix
  media: dt-bindings: media: i2c: Add bindings for TW9900
  media: i2c: Introduce a driver for the Techwell TW9900 decoder

 .../devicetree/bindings/media/i2c/tw9900.yaml |  64 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/media/i2c/Kconfig                     |  11 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/tw9900.c                    | 617 ++++++++++++++++++
 6 files changed, 701 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/tw9900.yaml
 create mode 100644 drivers/media/i2c/tw9900.c

-- 
2.25.4

[PATCH v4 1/3] dt-bindings: vendor-prefixes: Add techwell vendor prefix

[Maxime Chevallier]

Add prefix for Techwell, Inc.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 041ae90b0d8f..467b59d8513b 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1095,6 +1095,8 @@ patternProperties:
     description: TechNexion
   "^technologic,.*":
     description: Technologic Systems
+  "^techwell,.*":
+    description: Techwell, Inc.
   "^tempo,.*":
     description: Tempo Semiconductor
   "^techstar,.*":
-- 
2.25.4

[PATCH v4 2/3] media: dt-bindings: media: i2c: Add bindings for TW9900

[Maxime Chevallier]

The Techwell TW9900 is a video decoder supporting multiple input
standards, such as PAL, NTSC and SECAM, and outputs a BT.656 video
signal.

It's designed to be low-power, posesses some features such as a
programmable comb-filter, and automatic input standard detection.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2->V3 : Fix the example not compiling due to a typo in the reset-gpios
node.

V3->V4 : Add the missing reset-gpios node to the binding

 .../devicetree/bindings/media/i2c/tw9900.yaml | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/tw9900.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/tw9900.yaml b/Documentation/devicetree/bindings/media/i2c/tw9900.yaml
new file mode 100644
index 000000000000..d27196e37416
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/tw9900.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/tw9900.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Techwell TW9900 NTSC/PAL/SECAM video decoder
+
+maintainers:
+  - Maxime Chevallier <maxime.chevallier@bootlin.com>
+
+description:
+  The tw9900 is a multi-standard video decoder, supporting NTSC, PAL and SECAM
+  standards with auto-detection features.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - techwell,tw9900
+
+  reg:
+    maxItems: 1
+
+  vdd-supply:
+    description: VDD power supply
+
+  reset-gpios:
+    description: GPIO descriptor for the RESET input pin
+    maxItems: 1
+
+  port:
+    type: object
+    description:
+      A node containing a single endpoint as doucmented in
+      Documentation/devicetree/bindings/media/video-interfaces.txt
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            tw9900: tw9900@44 {
+                    compatible = "techwell,tw9900";
+                    reg = <0x44>;
+
+                    vdd-supply = <&tw9900_supply>;
+                    reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+
+                    port {
+                            tw9900_out: endpoint {
+                                    remote-endpoint = <&vip_in>;
+                            };
+                    };
+            };
+    };
-- 
2.25.4

Re: [PATCH v4 2/3] media: dt-bindings: media: i2c: Add bindings for TW9900

[Rob Herring]

On Fri, Feb 19, 2021 at 09:15:13AM +0100, Maxime Chevallier wrote:
> The Techwell TW9900 is a video decoder supporting multiple input
> standards, such as PAL, NTSC and SECAM, and outputs a BT.656 video
> signal.
> 
> It's designed to be low-power, posesses some features such as a
> programmable comb-filter, and automatic input standard detection.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> V2->V3 : Fix the example not compiling due to a typo in the reset-gpios
> node.
> 
> V3->V4 : Add the missing reset-gpios node to the binding
> 
>  .../devicetree/bindings/media/i2c/tw9900.yaml | 64 +++++++++++++++++++

Use the compatible string for the filename.

>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/tw9900.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/tw9900.yaml b/Documentation/devicetree/bindings/media/i2c/tw9900.yaml
> new file mode 100644
> index 000000000000..d27196e37416
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/tw9900.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/tw9900.yaml#

Don't forget to update this...

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Techwell TW9900 NTSC/PAL/SECAM video decoder
> +
> +maintainers:
> +  - Maxime Chevallier <maxime.chevallier@bootlin.com>
> +
> +description:
> +  The tw9900 is a multi-standard video decoder, supporting NTSC, PAL and SECAM
> +  standards with auto-detection features.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - techwell,tw9900
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: VDD power supply
> +
> +  reset-gpios:
> +    description: GPIO descriptor for the RESET input pin
> +    maxItems: 1
> +
> +  port:
> +    type: object
> +    description:
> +      A node containing a single endpoint as doucmented in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt

It's now video-interfaces.yaml, but you aren't using anything from it. 
You need to reference graph.yaml though. See the examples in the tree 
now.

> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            tw9900: tw9900@44 {
> +                    compatible = "techwell,tw9900";
> +                    reg = <0x44>;
> +
> +                    vdd-supply = <&tw9900_supply>;
> +                    reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
> +
> +                    port {
> +                            tw9900_out: endpoint {
> +                                    remote-endpoint = <&vip_in>;
> +                            };
> +                    };
> +            };
> +    };
> -- 
> 2.25.4
> 

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

[Maxime Chevallier]

The Techwell video decoder supports PAL, NTSC and SECAM input formats,
and outputs a BT.656 signal.

This commit adds support for this device, with basic support for NTSC
and PAL, along with brightness and contrast controls.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
v1 -> v2: Set the media entity type to decoder, and implement the
s_std/g_std ops

V2 ->V3 : Fix coding-style issues, and remove the use of the bulk API
for regulators. Make the driver select the media-controller and
V4L2-subdev APIs.

V3->V4: Fix a warning about an uninitialized ret variable in probe()

 MAINTAINERS                |   6 +
 drivers/media/i2c/Kconfig  |  11 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/tw9900.c | 617 +++++++++++++++++++++++++++++++++++++
 4 files changed, 635 insertions(+)
 create mode 100644 drivers/media/i2c/tw9900.c

diff --git a/MAINTAINERS b/MAINTAINERS
index bfc1b86e3e73..dde9f7bdd720 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17425,6 +17425,12 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/media/rc/ttusbir.c
 
+TECHWELL TW9900 VIDEO DECODER
+M:	Maxime Chevallier <maxime.chevallier@bootlin.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+F:	drivers/media/i2c/tw9900.c
+
 TECHWELL TW9910 VIDEO DECODER
 L:	linux-media@vger.kernel.org
 S:	Orphan
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 2b9d81e4794a..5f902557c8e5 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -426,6 +426,17 @@ config VIDEO_TW2804
 	  To compile this driver as a module, choose M here: the
 	  module will be called tw2804.
 
+config VIDEO_TW9900
+	tristate "Techwell TW9900 video decoder"
+	depends on VIDEO_V4L2 && I2C
+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	help
+	  Support for the Techwell tw9900 multi-standard video decoder.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called tw9900.
+
 config VIDEO_TW9903
 	tristate "Techwell TW9903 video decoder"
 	depends on VIDEO_V4L2 && I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index a3149dce21bb..9e7df99201c6 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_VIDEO_TVP5150) += tvp5150.o
 obj-$(CONFIG_VIDEO_TVP514X) += tvp514x.o
 obj-$(CONFIG_VIDEO_TVP7002) += tvp7002.o
 obj-$(CONFIG_VIDEO_TW2804) += tw2804.o
+obj-$(CONFIG_VIDEO_TW9900) += tw9900.o
 obj-$(CONFIG_VIDEO_TW9903) += tw9903.o
 obj-$(CONFIG_VIDEO_TW9906) += tw9906.o
 obj-$(CONFIG_VIDEO_TW9910) += tw9910.o
diff --git a/drivers/media/i2c/tw9900.c b/drivers/media/i2c/tw9900.c
new file mode 100644
index 000000000000..635268211982
--- /dev/null
+++ b/drivers/media/i2c/tw9900.c
@@ -0,0 +1,617 @@
+// 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@bootlin.com>
+ */
+
+#include <linux/clk.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 <linux/sysfs.h>
+#include <linux/timer.h>
+#include <linux/delay.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_VLOCK	0x08
+#define TW9900_REG_CHIP_STATUS_VDLOSS	0x80
+#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_SEL	0x1C
+#define TW9900_REG_MISSCNT	0x26
+#define TW9900_REG_MISC_CTL_II	0x2F
+#define TW9900_REG_VVBI		0x55
+
+#define TW9900_CHIP_ID		0x00
+
+#define VSYNC_POLL_INTERVAL_MS	20
+#define VSYNC_WAIT_MAX_POLLS	50
+
+struct regval {
+	u8 addr;
+	u8 val;
+};
+
+struct tw9900_mode {
+	u32 width;
+	u32 height;
+	u32 skip_top;
+	u32 std;
+	u32 field;
+	const struct regval *reg_list;
+	int n_regs;
+};
+
+struct tw9900 {
+	struct i2c_client *client;
+	struct gpio_desc *reset_gpio;
+	struct regulator *regulator;
+
+	bool streaming;
+
+	struct v4l2_subdev subdev;
+	struct v4l2_ctrl_handler hdl;
+	struct media_pad pad;
+
+	struct timer_list timer;
+	struct work_struct work_i2c_poll;
+
+	const struct tw9900_mode *cur_mode;
+};
+
+#define to_tw9900(sd) container_of(sd, struct tw9900, subdev)
+
+static const struct regval tw9900_init_regs[] = {
+	{ TW9900_REG_MISC_CTL_II,	0xE6 },
+	{ TW9900_REG_MISSCNT,		0x24 },
+	{ TW9900_REG_OUT_FMT_CTL,	0xA7 },
+	{ TW9900_REG_ANAL_CTL_II,	0x0A },
+	{ TW9900_REG_VDELAY_LO,		0x19 },
+	{ TW9900_REG_STD_SEL,		0x00 },
+	{ TW9900_REG_VACTIVE_LO,	0xF0 },
+	{ TW9900_REG_STD_SEL,		0x07 },
+	{ TW9900_REG_CKHY_HSDLY,	0x40 },
+	{ TW9900_REG_ANALOG_CTL,	0x80 },
+	{ TW9900_REG_CNTRL1,		0xDC },
+	{ TW9900_REG_OUT_CTRL_I,	0x98 },
+};
+
+static const struct regval tw9900_pal_regs[] = {
+	{ TW9900_REG_STD_SEL,		0x01 },
+};
+
+static const struct regval tw9900_ntsc_regs[] = {
+	{ TW9900_REG_OUT_FMT_CTL,	0xA4 },
+	{ TW9900_REG_VDELAY_LO,		0x12 },
+	{ TW9900_REG_VACTIVE_LO,	0xF0 },
+	{ TW9900_REG_CROP_HI,		0x02 },
+	{ TW9900_REG_HACTIVE_LO,	0xD0 },
+	{ TW9900_REG_VBI_CNTL,		0x01 },
+	{ TW9900_REG_STD_SEL,		0x00 },
+};
+
+static const struct tw9900_mode supported_modes[] = {
+	{
+		.width = 720,
+		.height = 576,
+		.skip_top = 0,
+		.std = V4L2_STD_PAL,
+		.field = V4L2_FIELD_NONE,
+		.reg_list = tw9900_pal_regs,
+		.n_regs = ARRAY_SIZE(tw9900_pal_regs),
+	},
+	{
+		.width = 720,
+		.height = 480,
+		.skip_top = 0,
+		.std = V4L2_STD_NTSC,
+		.field = V4L2_FIELD_NONE,
+		.reg_list = tw9900_ntsc_regs,
+		.n_regs = ARRAY_SIZE(tw9900_ntsc_regs),
+	},
+};
+
+static int tw9900_write_reg(struct i2c_client *client, u8 reg, u8 val)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client, reg, val);
+
+	if (ret < 0)
+		dev_err(&client->dev, "write reg error: %d\n", ret);
+
+	return ret;
+}
+
+static int tw9900_write_array(struct i2c_client *client,
+			      const struct regval *regs, int n_regs)
+{
+	int i, ret = 0;
+
+	for (i = 0; ret == 0 && i <= n_regs; i++)
+		ret = tw9900_write_reg(client, regs[i].addr, regs[i].val);
+
+	return ret;
+}
+
+static inline u8 tw9900_read_reg(struct i2c_client *client, u8 reg)
+{
+	return i2c_smbus_read_byte_data(client, reg);
+}
+
+static void tw9900_fill_fmt(const struct tw9900_mode *mode,
+			    struct v4l2_mbus_framefmt *fmt)
+{
+	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
+	fmt->width = mode->width;
+	fmt->height = mode->height;
+	fmt->field = mode->field;
+	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
+}
+
+static int tw9900_set_fmt(struct v4l2_subdev *sd,
+			  struct v4l2_subdev_pad_config *cfg,
+			  struct v4l2_subdev_format *fmt)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
+
+	tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt);
+
+	mbus_fmt->width = tw9900->cur_mode->width;
+	mbus_fmt->height = tw9900->cur_mode->height;
+
+	return 0;
+}
+
+static int tw9900_get_fmt(struct v4l2_subdev *sd,
+			  struct v4l2_subdev_pad_config *cfg,
+			  struct v4l2_subdev_format *fmt)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
+
+	tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt);
+
+	return 0;
+}
+
+static int tw9900_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index >= 1)
+		return -EINVAL;
+
+	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
+
+	return 0;
+}
+
+static int tw9900_enum_frame_sizes(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_pad_config *cfg,
+				   struct v4l2_subdev_frame_size_enum *fse)
+{
+	u32 index = fse->index;
+
+	if (index >= 1)
+		return -EINVAL;
+
+	fse->code = MEDIA_BUS_FMT_UYVY8_2X8;
+
+	fse->min_width  = supported_modes[index].width;
+	fse->max_width  = supported_modes[index].width;
+	fse->max_height = supported_modes[index].height;
+	fse->min_height = supported_modes[index].height;
+
+	return 0;
+}
+
+static int tw9900_power_on(struct tw9900 *tw9900)
+{
+	int ret;
+	struct device *dev = &tw9900->client->dev;
+
+	if (tw9900->reset_gpio)
+		gpiod_set_value_cansleep(tw9900->reset_gpio, 1);
+
+	ret = regulator_enable(tw9900->regulator);
+	if (ret < 0)
+		goto error;
+
+	usleep_range(50000, 52000);
+
+	if (tw9900->reset_gpio)
+		gpiod_set_value_cansleep(tw9900->reset_gpio, 0);
+
+	usleep_range(1000, 2000);
+
+	ret = tw9900_write_array(tw9900->client, tw9900_init_regs,
+				 ARRAY_SIZE(tw9900_init_regs));
+	if (ret) {
+		dev_err(dev, "Failed to init tw9900\n");
+		goto error;
+	}
+
+	return 0;
+
+error:
+
+	return ret;
+}
+
+static void tw9900_power_off(struct tw9900 *tw9900)
+{
+	if (tw9900->reset_gpio)
+		gpiod_set_value_cansleep(tw9900->reset_gpio, 1);
+
+	regulator_disable(tw9900->regulator);
+}
+
+static int tw9900_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct tw9900 *tw9900 = container_of(ctrl->handler, struct tw9900, hdl);
+
+	if (pm_runtime_suspended(&tw9900->client->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_BRIGHTNESS:
+		tw9900_write_reg(tw9900->client, 0x10, (u8)ctrl->val);
+		break;
+	case V4L2_CID_CONTRAST:
+		tw9900_write_reg(tw9900->client, 0x11, (u8)ctrl->val);
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int tw9900_s_stream(struct v4l2_subdev *sd, int on)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+	struct i2c_client *client = tw9900->client;
+	int i, ret = 0;
+
+	on = !!on;
+	if (on == tw9900->streaming)
+		return 0;
+
+	if (on) {
+		ret = pm_runtime_get_sync(&tw9900->client->dev);
+		if (ret < 0) {
+			pm_runtime_put_noidle(&client->dev);
+			return ret;
+		}
+
+		ret = v4l2_ctrl_handler_setup(sd->ctrl_handler);
+		if (ret)
+			goto put_and_return;
+
+		ret = tw9900_write_array(tw9900->client,
+					 tw9900->cur_mode->reg_list,
+					 tw9900->cur_mode->n_regs);
+		if (ret)
+			goto put_and_return;
+
+		/* Wait for VSync lock */
+		for (i = 0; i < VSYNC_WAIT_MAX_POLLS; i++) {
+			u8 status = tw9900_read_reg(tw9900->client,
+						    TW9900_REG_CHIP_STATUS);
+			if (!(status & TW9900_REG_CHIP_STATUS_VDLOSS) &&
+			    (status & TW9900_REG_CHIP_STATUS_VLOCK))
+				break;
+
+			msleep(VSYNC_POLL_INTERVAL_MS);
+		}
+
+		ret = tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL,
+				       TW9900_REG_OUT_FMT_CTL_STREAMING);
+		if (ret)
+			goto put_and_return;
+
+	} else {
+		tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL,
+				 TW9900_REG_OUT_FMT_CTL_STANDBY);
+		pm_runtime_put(&client->dev);
+	}
+
+	tw9900->streaming = on;
+
+	return ret;
+
+put_and_return:
+	pm_runtime_put(&client->dev);
+
+	return ret;
+}
+
+static int tw9900_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+	struct v4l2_mbus_framefmt *try_fmt;
+
+	try_fmt = v4l2_subdev_get_try_format(sd, fh->pad, 0);
+
+	/* Initialize try_fmt */
+	tw9900_fill_fmt(tw9900->cur_mode, try_fmt);
+
+	return 0;
+}
+
+static int tw9900_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct tw9900 *tw9900 = to_tw9900(sd);
+
+	return tw9900_power_on(tw9900);
+}
+
+static int tw9900_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct tw9900 *tw9900 = to_tw9900(sd);
+
+	tw9900_power_off(tw9900);
+
+	return 0;
+}
+
+static int tw9900_subscribe_event(struct v4l2_subdev *sd,
+				  struct v4l2_fh *fh,
+				  struct v4l2_event_subscription *sub)
+{
+	switch (sub->type) {
+	case V4L2_EVENT_SOURCE_CHANGE:
+		return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
+	case V4L2_EVENT_CTRL:
+		return v4l2_ctrl_subdev_subscribe_event(sd, fh, sub);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct tw9900_mode *tw9900_get_mode_from_std(v4l2_std_id std)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(supported_modes); i++)
+		if (supported_modes[i].std == std)
+			return &supported_modes[i];
+
+	return NULL;
+}
+
+static int tw9900_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+	const struct tw9900_mode *mode;
+	int ret;
+
+	if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
+		return -EINVAL;
+
+	mode = tw9900_get_mode_from_std(norm);
+	if (!mode)
+		return -EINVAL;
+
+	ret = tw9900_write_array(tw9900->client, mode->reg_list, mode->n_regs);
+	if (ret)
+		return ret;
+
+	tw9900->cur_mode = mode;
+
+	return 0;
+}
+
+static int tw9900_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+
+	*norm = tw9900->cur_mode->std;
+
+	return 0;
+}
+
+static const struct dev_pm_ops tw9900_pm_ops = {
+	SET_RUNTIME_PM_OPS(tw9900_runtime_suspend,
+			   tw9900_runtime_resume, NULL)
+};
+
+static const struct v4l2_subdev_core_ops tw9900_core_ops = {
+	.subscribe_event	= tw9900_subscribe_event,
+	.unsubscribe_event	= v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_video_ops tw9900_video_ops = {
+	.s_std		= tw9900_s_std,
+	.g_std		= tw9900_g_std,
+	.s_stream	= tw9900_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops tw9900_pad_ops = {
+	.enum_mbus_code		= tw9900_enum_mbus_code,
+	.enum_frame_size	= tw9900_enum_frame_sizes,
+	.get_fmt		= tw9900_get_fmt,
+	.set_fmt		= tw9900_set_fmt,
+};
+
+static const struct v4l2_subdev_ops tw9900_subdev_ops = {
+	.core	= &tw9900_core_ops,
+	.video	= &tw9900_video_ops,
+	.pad	= &tw9900_pad_ops,
+};
+
+static const struct v4l2_ctrl_ops tw9900_ctrl_ops = {
+	.s_ctrl	= tw9900_s_ctrl,
+};
+
+static const struct v4l2_subdev_internal_ops tw9900_internal_ops = {
+	.open	= tw9900_open,
+};
+
+static int tw9900_check_id(struct tw9900 *tw9900,
+			   struct i2c_client *client)
+{
+	struct device *dev = &tw9900->client->dev;
+	u8 id;
+
+	id = tw9900_read_reg(client, TW9900_CHIP_ID);
+
+	if (id != TW9900_CHIP_ID) {
+		dev_err(dev, "Unexpected decoder id(%04x)\n", id);
+		return -EINVAL;
+	}
+
+	dev_info(dev, "Detected TW9900 (%04x) decoder\n", TW9900_CHIP_ID);
+
+	return 0;
+}
+
+static int tw9900_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct v4l2_ctrl_handler *hdl;
+	struct tw9900 *tw9900;
+	int ret = 0;
+
+	tw9900 = devm_kzalloc(dev, sizeof(*tw9900), GFP_KERNEL);
+	if (!tw9900)
+		return -ENOMEM;
+
+	tw9900->client = client;
+	tw9900->cur_mode = &supported_modes[0];
+
+	tw9900->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(tw9900->reset_gpio))
+		tw9900->reset_gpio = NULL;
+
+	tw9900->regulator = devm_regulator_get(&tw9900->client->dev, "vdd");
+	if (IS_ERR(tw9900->regulator)) {
+		dev_err(dev, "Failed to get power regulator\n");
+		return ret;
+	}
+
+	v4l2_i2c_subdev_init(&tw9900->subdev, client, &tw9900_subdev_ops);
+	tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+				V4L2_SUBDEV_FL_HAS_EVENTS;
+
+	hdl = &tw9900->hdl;
+
+	v4l2_ctrl_handler_init(hdl, 2);
+
+	v4l2_ctrl_new_std(hdl, &tw9900_ctrl_ops, V4L2_CID_BRIGHTNESS,
+			  -128, 127, 1, 0);
+	v4l2_ctrl_new_std(hdl, &tw9900_ctrl_ops, V4L2_CID_CONTRAST,
+			  0, 255, 1, 0x60);
+
+	tw9900->subdev.ctrl_handler = hdl;
+	if (hdl->error) {
+		int err = hdl->error;
+
+		v4l2_ctrl_handler_free(hdl);
+		return err;
+	}
+
+	ret = tw9900_power_on(tw9900);
+	if (ret)
+		return ret;
+
+	ret = tw9900_check_id(tw9900, client);
+	if (ret)
+		goto err_power_off;
+
+	tw9900->subdev.internal_ops = &tw9900_internal_ops;
+	tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	tw9900->pad.flags = MEDIA_PAD_FL_SOURCE;
+	tw9900->subdev.entity.function = MEDIA_ENT_F_DV_DECODER;
+
+	ret = media_entity_pads_init(&tw9900->subdev.entity, 1, &tw9900->pad);
+	if (ret < 0)
+		goto err_power_off;
+
+	ret = v4l2_async_register_subdev(&tw9900->subdev);
+	if (ret) {
+		dev_err(dev, "v4l2 async register subdev failed\n");
+		goto err_clean_entity;
+	}
+
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_idle(dev);
+
+	return 0;
+
+err_clean_entity:
+	media_entity_cleanup(&tw9900->subdev.entity);
+err_power_off:
+	tw9900_power_off(tw9900);
+
+	return ret;
+}
+
+static int tw9900_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct tw9900 *tw9900 = to_tw9900(sd);
+
+	v4l2_async_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
+
+	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev))
+		tw9900_power_off(tw9900);
+	pm_runtime_set_suspended(&client->dev);
+
+	return 0;
+}
+
+static const struct of_device_id tw9900_of_match[] = {
+	{ .compatible = "techwell,tw9900" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tw9900_of_match);
+
+static struct i2c_driver tw9900_i2c_driver = {
+	.driver = {
+		.name		= "tw9900",
+		.pm		= &tw9900_pm_ops,
+		.of_match_table	= tw9900_of_match
+	},
+	.probe	= tw9900_probe,
+	.remove	= tw9900_remove,
+};
+
+module_i2c_driver(tw9900_i2c_driver);
+
+MODULE_DESCRIPTION("tw9900 decoder driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.4

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

[Hans Verkuil]

Hi Maxime,

Some more code review comments:

On 19/02/2021 09:15, Maxime Chevallier wrote:
> The Techwell video decoder supports PAL, NTSC and SECAM input formats,
> and outputs a BT.656 signal.
> 
> This commit adds support for this device, with basic support for NTSC
> and PAL, along with brightness and contrast controls.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> v1 -> v2: Set the media entity type to decoder, and implement the
> s_std/g_std ops
> 
> V2 ->V3 : Fix coding-style issues, and remove the use of the bulk API
> for regulators. Make the driver select the media-controller and
> V4L2-subdev APIs.
> 
> V3->V4: Fix a warning about an uninitialized ret variable in probe()
> 
>  MAINTAINERS                |   6 +
>  drivers/media/i2c/Kconfig  |  11 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/tw9900.c | 617 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 635 insertions(+)
>  create mode 100644 drivers/media/i2c/tw9900.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bfc1b86e3e73..dde9f7bdd720 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17425,6 +17425,12 @@ L:	linux-media@vger.kernel.org
>  S:	Maintained
>  F:	drivers/media/rc/ttusbir.c
>  
> +TECHWELL TW9900 VIDEO DECODER
> +M:	Maxime Chevallier <maxime.chevallier@bootlin.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +F:	drivers/media/i2c/tw9900.c
> +
>  TECHWELL TW9910 VIDEO DECODER
>  L:	linux-media@vger.kernel.org
>  S:	Orphan
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 2b9d81e4794a..5f902557c8e5 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -426,6 +426,17 @@ config VIDEO_TW2804
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called tw2804.
>  
> +config VIDEO_TW9900
> +	tristate "Techwell TW9900 video decoder"
> +	depends on VIDEO_V4L2 && I2C
> +	select MEDIA_CONTROLLER
> +	select VIDEO_V4L2_SUBDEV_API
> +	help
> +	  Support for the Techwell tw9900 multi-standard video decoder.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called tw9900.
> +
>  config VIDEO_TW9903
>  	tristate "Techwell TW9903 video decoder"
>  	depends on VIDEO_V4L2 && I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index a3149dce21bb..9e7df99201c6 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_VIDEO_TVP5150) += tvp5150.o
>  obj-$(CONFIG_VIDEO_TVP514X) += tvp514x.o
>  obj-$(CONFIG_VIDEO_TVP7002) += tvp7002.o
>  obj-$(CONFIG_VIDEO_TW2804) += tw2804.o
> +obj-$(CONFIG_VIDEO_TW9900) += tw9900.o
>  obj-$(CONFIG_VIDEO_TW9903) += tw9903.o
>  obj-$(CONFIG_VIDEO_TW9906) += tw9906.o
>  obj-$(CONFIG_VIDEO_TW9910) += tw9910.o
> diff --git a/drivers/media/i2c/tw9900.c b/drivers/media/i2c/tw9900.c
> new file mode 100644
> index 000000000000..635268211982
> --- /dev/null
> +++ b/drivers/media/i2c/tw9900.c
> @@ -0,0 +1,617 @@
> +// 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@bootlin.com>
> + */
> +
> +#include <linux/clk.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 <linux/sysfs.h>
> +#include <linux/timer.h>
> +#include <linux/delay.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_VLOCK	0x08
> +#define TW9900_REG_CHIP_STATUS_VDLOSS	0x80
> +#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_SEL	0x1C
> +#define TW9900_REG_MISSCNT	0x26
> +#define TW9900_REG_MISC_CTL_II	0x2F
> +#define TW9900_REG_VVBI		0x55
> +
> +#define TW9900_CHIP_ID		0x00
> +
> +#define VSYNC_POLL_INTERVAL_MS	20
> +#define VSYNC_WAIT_MAX_POLLS	50
> +
> +struct regval {
> +	u8 addr;
> +	u8 val;
> +};
> +
> +struct tw9900_mode {
> +	u32 width;
> +	u32 height;
> +	u32 skip_top;
> +	u32 std;
> +	u32 field;
> +	const struct regval *reg_list;
> +	int n_regs;
> +};
> +
> +struct tw9900 {
> +	struct i2c_client *client;
> +	struct gpio_desc *reset_gpio;
> +	struct regulator *regulator;
> +
> +	bool streaming;
> +
> +	struct v4l2_subdev subdev;
> +	struct v4l2_ctrl_handler hdl;
> +	struct media_pad pad;
> +
> +	struct timer_list timer;
> +	struct work_struct work_i2c_poll;
> +
> +	const struct tw9900_mode *cur_mode;
> +};
> +
> +#define to_tw9900(sd) container_of(sd, struct tw9900, subdev)
> +
> +static const struct regval tw9900_init_regs[] = {
> +	{ TW9900_REG_MISC_CTL_II,	0xE6 },
> +	{ TW9900_REG_MISSCNT,		0x24 },
> +	{ TW9900_REG_OUT_FMT_CTL,	0xA7 },
> +	{ TW9900_REG_ANAL_CTL_II,	0x0A },
> +	{ TW9900_REG_VDELAY_LO,		0x19 },
> +	{ TW9900_REG_STD_SEL,		0x00 },
> +	{ TW9900_REG_VACTIVE_LO,	0xF0 },
> +	{ TW9900_REG_STD_SEL,		0x07 },
> +	{ TW9900_REG_CKHY_HSDLY,	0x40 },
> +	{ TW9900_REG_ANALOG_CTL,	0x80 },
> +	{ TW9900_REG_CNTRL1,		0xDC },
> +	{ TW9900_REG_OUT_CTRL_I,	0x98 },
> +};
> +
> +static const struct regval tw9900_pal_regs[] = {
> +	{ TW9900_REG_STD_SEL,		0x01 },
> +};
> +
> +static const struct regval tw9900_ntsc_regs[] = {
> +	{ TW9900_REG_OUT_FMT_CTL,	0xA4 },
> +	{ TW9900_REG_VDELAY_LO,		0x12 },
> +	{ TW9900_REG_VACTIVE_LO,	0xF0 },
> +	{ TW9900_REG_CROP_HI,		0x02 },
> +	{ TW9900_REG_HACTIVE_LO,	0xD0 },
> +	{ TW9900_REG_VBI_CNTL,		0x01 },
> +	{ TW9900_REG_STD_SEL,		0x00 },
> +};
> +
> +static const struct tw9900_mode supported_modes[] = {
> +	{
> +		.width = 720,
> +		.height = 576,
> +		.skip_top = 0,
> +		.std = V4L2_STD_PAL,
> +		.field = V4L2_FIELD_NONE,
> +		.reg_list = tw9900_pal_regs,
> +		.n_regs = ARRAY_SIZE(tw9900_pal_regs),
> +	},
> +	{
> +		.width = 720,
> +		.height = 480,
> +		.skip_top = 0,
> +		.std = V4L2_STD_NTSC,
> +		.field = V4L2_FIELD_NONE,
> +		.reg_list = tw9900_ntsc_regs,
> +		.n_regs = ARRAY_SIZE(tw9900_ntsc_regs),
> +	},
> +};
> +
> +static int tw9900_write_reg(struct i2c_client *client, u8 reg, u8 val)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, reg, val);
> +
> +	if (ret < 0)
> +		dev_err(&client->dev, "write reg error: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int tw9900_write_array(struct i2c_client *client,
> +			      const struct regval *regs, int n_regs)
> +{
> +	int i, ret = 0;
> +
> +	for (i = 0; ret == 0 && i <= n_regs; i++)
> +		ret = tw9900_write_reg(client, regs[i].addr, regs[i].val);
> +
> +	return ret;
> +}
> +
> +static inline u8 tw9900_read_reg(struct i2c_client *client, u8 reg)
> +{
> +	return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static void tw9900_fill_fmt(const struct tw9900_mode *mode,
> +			    struct v4l2_mbus_framefmt *fmt)
> +{
> +	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +	fmt->width = mode->width;
> +	fmt->height = mode->height;
> +	fmt->field = mode->field;
> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +}
> +
> +static int tw9900_set_fmt(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_pad_config *cfg,
> +			  struct v4l2_subdev_format *fmt)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> +
> +	tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt);
> +
> +	mbus_fmt->width = tw9900->cur_mode->width;
> +	mbus_fmt->height = tw9900->cur_mode->height;

These two lines are already done in tw9900_fill_fmt.

> +
> +	return 0;
> +}
> +
> +static int tw9900_get_fmt(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_pad_config *cfg,
> +			  struct v4l2_subdev_format *fmt)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> +
> +	tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt);
> +
> +	return 0;
> +}

In fact, tw9900_set_fmt is identical to tw9900_get_fmt. I would just drop
tw9900_set_fmt and point both .get_fmt and .set_fmt to the same function.

> +
> +static int tw9900_enum_mbus_code(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_pad_config *cfg,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->index >= 1)
> +		return -EINVAL;
> +
> +	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +
> +	return 0;
> +}
> +
> +static int tw9900_enum_frame_sizes(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_pad_config *cfg,
> +				   struct v4l2_subdev_frame_size_enum *fse)
> +{
> +	u32 index = fse->index;
> +
> +	if (index >= 1)
> +		return -EINVAL;
> +
> +	fse->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +
> +	fse->min_width  = supported_modes[index].width;
> +	fse->max_width  = supported_modes[index].width;
> +	fse->max_height = supported_modes[index].height;
> +	fse->min_height = supported_modes[index].height;
> +
> +	return 0;
> +}

This function is not typically used by video receivers since the framesize is
fixed depending on the standard. So there is nothing to enumerate.

It is wrong in any case since it reports just supported_modes[0] (i.e. PAL)
even if the current mode is NTSC. But it can just be dropped for video receivers.

> +
> +static int tw9900_power_on(struct tw9900 *tw9900)
> +{
> +	int ret;
> +	struct device *dev = &tw9900->client->dev;
> +
> +	if (tw9900->reset_gpio)
> +		gpiod_set_value_cansleep(tw9900->reset_gpio, 1);
> +
> +	ret = regulator_enable(tw9900->regulator);
> +	if (ret < 0)
> +		goto error;
> +
> +	usleep_range(50000, 52000);
> +
> +	if (tw9900->reset_gpio)
> +		gpiod_set_value_cansleep(tw9900->reset_gpio, 0);
> +
> +	usleep_range(1000, 2000);
> +
> +	ret = tw9900_write_array(tw9900->client, tw9900_init_regs,
> +				 ARRAY_SIZE(tw9900_init_regs));
> +	if (ret) {
> +		dev_err(dev, "Failed to init tw9900\n");
> +		goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +
> +	return ret;
> +}
> +
> +static void tw9900_power_off(struct tw9900 *tw9900)
> +{
> +	if (tw9900->reset_gpio)
> +		gpiod_set_value_cansleep(tw9900->reset_gpio, 1);
> +
> +	regulator_disable(tw9900->regulator);
> +}
> +
> +static int tw9900_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct tw9900 *tw9900 = container_of(ctrl->handler, struct tw9900, hdl);
> +
> +	if (pm_runtime_suspended(&tw9900->client->dev))
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_BRIGHTNESS:
> +		tw9900_write_reg(tw9900->client, 0x10, (u8)ctrl->val);
> +		break;
> +	case V4L2_CID_CONTRAST:
> +		tw9900_write_reg(tw9900->client, 0x11, (u8)ctrl->val);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int tw9900_s_stream(struct v4l2_subdev *sd, int on)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	struct i2c_client *client = tw9900->client;
> +	int i, ret = 0;
> +
> +	on = !!on;
> +	if (on == tw9900->streaming)
> +		return 0;
> +
> +	if (on) {
> +		ret = pm_runtime_get_sync(&tw9900->client->dev);
> +		if (ret < 0) {
> +			pm_runtime_put_noidle(&client->dev);
> +			return ret;
> +		}
> +
> +		ret = v4l2_ctrl_handler_setup(sd->ctrl_handler);
> +		if (ret)
> +			goto put_and_return;
> +
> +		ret = tw9900_write_array(tw9900->client,
> +					 tw9900->cur_mode->reg_list,
> +					 tw9900->cur_mode->n_regs);
> +		if (ret)
> +			goto put_and_return;
> +
> +		/* Wait for VSync lock */
> +		for (i = 0; i < VSYNC_WAIT_MAX_POLLS; i++) {
> +			u8 status = tw9900_read_reg(tw9900->client,
> +						    TW9900_REG_CHIP_STATUS);
> +			if (!(status & TW9900_REG_CHIP_STATUS_VDLOSS) &&
> +			    (status & TW9900_REG_CHIP_STATUS_VLOCK))
> +				break;
> +
> +			msleep(VSYNC_POLL_INTERVAL_MS);
> +		}

Why do you have to wait for a vsync lock?

Most drivers just start regardless of lock. If there is no lock, then there
is either no data being streamed (so the DMA of the video bridge will be idle
as well) or it is just transmitting noise (typical for SDTV receivers). At least
until a valid signal appears eventually.

> +
> +		ret = tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL,
> +				       TW9900_REG_OUT_FMT_CTL_STREAMING);
> +		if (ret)
> +			goto put_and_return;
> +
> +	} else {
> +		tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL,
> +				 TW9900_REG_OUT_FMT_CTL_STANDBY);
> +		pm_runtime_put(&client->dev);
> +	}
> +
> +	tw9900->streaming = on;
> +
> +	return ret;
> +
> +put_and_return:
> +	pm_runtime_put(&client->dev);
> +
> +	return ret;
> +}
> +
> +static int tw9900_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	struct v4l2_mbus_framefmt *try_fmt;
> +
> +	try_fmt = v4l2_subdev_get_try_format(sd, fh->pad, 0);
> +
> +	/* Initialize try_fmt */
> +	tw9900_fill_fmt(tw9900->cur_mode, try_fmt);
> +
> +	return 0;
> +}

Since the format is fixed based on the current standard, there is no point
in initializing try_fmt as it won't be used. So just drop tw9900_open altogether.

> +
> +static int tw9900_runtime_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +
> +	return tw9900_power_on(tw9900);
> +}
> +
> +static int tw9900_runtime_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +
> +	tw9900_power_off(tw9900);
> +
> +	return 0;
> +}
> +
> +static int tw9900_subscribe_event(struct v4l2_subdev *sd,
> +				  struct v4l2_fh *fh,
> +				  struct v4l2_event_subscription *sub)
> +{
> +	switch (sub->type) {
> +	case V4L2_EVENT_SOURCE_CHANGE:
> +		return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
> +	case V4L2_EVENT_CTRL:
> +		return v4l2_ctrl_subdev_subscribe_event(sd, fh, sub);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct tw9900_mode *tw9900_get_mode_from_std(v4l2_std_id std)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(supported_modes); i++)
> +		if (supported_modes[i].std == std)
> +			return &supported_modes[i];
> +
> +	return NULL;
> +}
> +
> +static int tw9900_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	const struct tw9900_mode *mode;
> +	int ret;
> +
> +	if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
> +		return -EINVAL;
> +
> +	mode = tw9900_get_mode_from_std(norm);
> +	if (!mode)
> +		return -EINVAL;
> +
> +	ret = tw9900_write_array(tw9900->client, mode->reg_list, mode->n_regs);
> +	if (ret)
> +		return ret;
> +
> +	tw9900->cur_mode = mode;
> +
> +	return 0;
> +}
> +
> +static int tw9900_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +
> +	*norm = tw9900->cur_mode->std;
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops tw9900_pm_ops = {
> +	SET_RUNTIME_PM_OPS(tw9900_runtime_suspend,
> +			   tw9900_runtime_resume, NULL)
> +};
> +
> +static const struct v4l2_subdev_core_ops tw9900_core_ops = {
> +	.subscribe_event	= tw9900_subscribe_event,
> +	.unsubscribe_event	= v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_video_ops tw9900_video_ops = {
> +	.s_std		= tw9900_s_std,
> +	.g_std		= tw9900_g_std,

Can the tw9900 detect the standard? (I.e. PAL, SECAM, NTSC)

If so, you should implement querystd. I see that none of the other tw*.c drivers
support this, so I suspect there is no hardware support for this.

You also must implement g_tvnorms to report the TV standards that the hardware
can understand.

> +	.s_stream	= tw9900_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops tw9900_pad_ops = {
> +	.enum_mbus_code		= tw9900_enum_mbus_code,
> +	.enum_frame_size	= tw9900_enum_frame_sizes,
> +	.get_fmt		= tw9900_get_fmt,
> +	.set_fmt		= tw9900_set_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops tw9900_subdev_ops = {
> +	.core	= &tw9900_core_ops,
> +	.video	= &tw9900_video_ops,
> +	.pad	= &tw9900_pad_ops,
> +};
> +
> +static const struct v4l2_ctrl_ops tw9900_ctrl_ops = {
> +	.s_ctrl	= tw9900_s_ctrl,
> +};
> +
> +static const struct v4l2_subdev_internal_ops tw9900_internal_ops = {
> +	.open	= tw9900_open,
> +};
> +
> +static int tw9900_check_id(struct tw9900 *tw9900,
> +			   struct i2c_client *client)
> +{
> +	struct device *dev = &tw9900->client->dev;
> +	u8 id;
> +
> +	id = tw9900_read_reg(client, TW9900_CHIP_ID);
> +
> +	if (id != TW9900_CHIP_ID) {
> +		dev_err(dev, "Unexpected decoder id(%04x)\n", id);
> +		return -EINVAL;
> +	}
> +
> +	dev_info(dev, "Detected TW9900 (%04x) decoder\n", TW9900_CHIP_ID);
> +
> +	return 0;
> +}
> +
> +static int tw9900_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct v4l2_ctrl_handler *hdl;
> +	struct tw9900 *tw9900;
> +	int ret = 0;
> +
> +	tw9900 = devm_kzalloc(dev, sizeof(*tw9900), GFP_KERNEL);
> +	if (!tw9900)
> +		return -ENOMEM;
> +
> +	tw9900->client = client;
> +	tw9900->cur_mode = &supported_modes[0];
> +
> +	tw9900->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(tw9900->reset_gpio))
> +		tw9900->reset_gpio = NULL;
> +
> +	tw9900->regulator = devm_regulator_get(&tw9900->client->dev, "vdd");
> +	if (IS_ERR(tw9900->regulator)) {
> +		dev_err(dev, "Failed to get power regulator\n");
> +		return ret;
> +	}
> +
> +	v4l2_i2c_subdev_init(&tw9900->subdev, client, &tw9900_subdev_ops);
> +	tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +				V4L2_SUBDEV_FL_HAS_EVENTS;
> +
> +	hdl = &tw9900->hdl;
> +
> +	v4l2_ctrl_handler_init(hdl, 2);
> +
> +	v4l2_ctrl_new_std(hdl, &tw9900_ctrl_ops, V4L2_CID_BRIGHTNESS,
> +			  -128, 127, 1, 0);
> +	v4l2_ctrl_new_std(hdl, &tw9900_ctrl_ops, V4L2_CID_CONTRAST,
> +			  0, 255, 1, 0x60);
> +
> +	tw9900->subdev.ctrl_handler = hdl;
> +	if (hdl->error) {
> +		int err = hdl->error;
> +
> +		v4l2_ctrl_handler_free(hdl);
> +		return err;
> +	}
> +
> +	ret = tw9900_power_on(tw9900);
> +	if (ret)
> +		return ret;
> +
> +	ret = tw9900_check_id(tw9900, client);
> +	if (ret)
> +		goto err_power_off;
> +
> +	tw9900->subdev.internal_ops = &tw9900_internal_ops;
> +	tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;

This is a duplicate of a similar '|=' above.

> +	tw9900->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	tw9900->subdev.entity.function = MEDIA_ENT_F_DV_DECODER;
> +
> +	ret = media_entity_pads_init(&tw9900->subdev.entity, 1, &tw9900->pad);
> +	if (ret < 0)
> +		goto err_power_off;
> +
> +	ret = v4l2_async_register_subdev(&tw9900->subdev);
> +	if (ret) {
> +		dev_err(dev, "v4l2 async register subdev failed\n");
> +		goto err_clean_entity;
> +	}
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_idle(dev);
> +
> +	return 0;
> +
> +err_clean_entity:
> +	media_entity_cleanup(&tw9900->subdev.entity);
> +err_power_off:
> +	tw9900_power_off(tw9900);
> +
> +	return ret;
> +}
> +
> +static int tw9900_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +
> +	v4l2_async_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +
> +	pm_runtime_disable(&client->dev);
> +	if (!pm_runtime_status_suspended(&client->dev))
> +		tw9900_power_off(tw9900);
> +	pm_runtime_set_suspended(&client->dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tw9900_of_match[] = {
> +	{ .compatible = "techwell,tw9900" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, tw9900_of_match);
> +
> +static struct i2c_driver tw9900_i2c_driver = {
> +	.driver = {
> +		.name		= "tw9900",
> +		.pm		= &tw9900_pm_ops,
> +		.of_match_table	= tw9900_of_match
> +	},
> +	.probe	= tw9900_probe,
> +	.remove	= tw9900_remove,
> +};
> +
> +module_i2c_driver(tw9900_i2c_driver);
> +
> +MODULE_DESCRIPTION("tw9900 decoder driver");
> +MODULE_LICENSE("GPL v2");
> 

Regards,

	Hans

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

[Maxime Chevallier]

Hi Hans, and thanks for the review.

On Thu, 4 Mar 2021 16:37:53 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:

>Hi Maxime,
>
>Some more code review comments:
>

>> +static int tw9900_set_fmt(struct v4l2_subdev *sd,
>> +			  struct v4l2_subdev_pad_config *cfg,
>> +			  struct v4l2_subdev_format *fmt)
>> +{
>> +	struct tw9900 *tw9900 = to_tw9900(sd);
>> +	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
>> +
>> +	tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt);
>> +
>> +	mbus_fmt->width = tw9900->cur_mode->width;
>> +	mbus_fmt->height = tw9900->cur_mode->height;  
>
>These two lines are already done in tw9900_fill_fmt.

Yes right, I'll remove that

[...]

>> +
>> +	return 0;
>> +}
>> +
>> +static int tw9900_get_fmt(struct v4l2_subdev *sd,
>> +			  struct v4l2_subdev_pad_config *cfg,
>> +			  struct v4l2_subdev_format *fmt)
>> +{
>> +	struct tw9900 *tw9900 = to_tw9900(sd);
>> +	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
>> +
>> +	tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt);
>> +
>> +	return 0;
>> +}  
>
>In fact, tw9900_set_fmt is identical to tw9900_get_fmt. I would just drop
>tw9900_set_fmt and point both .get_fmt and .set_fmt to the same function.

OK, that will be cleaner indeed

[...]

>> +
>> +static int tw9900_enum_mbus_code(struct v4l2_subdev *sd,
>> +				 struct v4l2_subdev_pad_config *cfg,
>> +				 struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> +	if (code->index >= 1)
>> +		return -EINVAL;
>> +
>> +	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
>> +
>> +	return 0;
>> +}
>> +
>> +static int tw9900_enum_frame_sizes(struct v4l2_subdev *sd,
>> +				   struct v4l2_subdev_pad_config *cfg,
>> +				   struct v4l2_subdev_frame_size_enum *fse)
>> +{
>> +	u32 index = fse->index;
>> +
>> +	if (index >= 1)
>> +		return -EINVAL;
>> +
>> +	fse->code = MEDIA_BUS_FMT_UYVY8_2X8;
>> +
>> +	fse->min_width  = supported_modes[index].width;
>> +	fse->max_width  = supported_modes[index].width;
>> +	fse->max_height = supported_modes[index].height;
>> +	fse->min_height = supported_modes[index].height;
>> +
>> +	return 0;
>> +}  
>
>This function is not typically used by video receivers since the framesize is
>fixed depending on the standard. So there is nothing to enumerate.
>
>It is wrong in any case since it reports just supported_modes[0] (i.e. PAL)
>even if the current mode is NTSC. But it can just be dropped for video receivers.

Ok thanks, I'll drop that then.

[...]

>> +		/* Wait for VSync lock */
>> +		for (i = 0; i < VSYNC_WAIT_MAX_POLLS; i++) {
>> +			u8 status = tw9900_read_reg(tw9900->client,
>> +						    TW9900_REG_CHIP_STATUS);
>> +			if (!(status & TW9900_REG_CHIP_STATUS_VDLOSS) &&
>> +			    (status & TW9900_REG_CHIP_STATUS_VLOCK))
>> +				break;
>> +
>> +			msleep(VSYNC_POLL_INTERVAL_MS);
>> +		}  
>
>Why do you have to wait for a vsync lock?
>
>Most drivers just start regardless of lock. If there is no lock, then there
>is either no data being streamed (so the DMA of the video bridge will be idle
>as well) or it is just transmitting noise (typical for SDTV receivers). At least
>until a valid signal appears eventually.

In this case, it will transmit noise.

As stated a bit below, this was implemented because this decoder
actually supports automatic standard detection, but the reported
standard can only be read once the vsync lock is acquired.

So this is a remainder of what I implemented to try to get the standard
detection work, but I can drop that for now.

[...]

>> +static int tw9900_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> +{
>> +	struct tw9900 *tw9900 = to_tw9900(sd);
>> +	struct v4l2_mbus_framefmt *try_fmt;
>> +
>> +	try_fmt = v4l2_subdev_get_try_format(sd, fh->pad, 0);
>> +
>> +	/* Initialize try_fmt */
>> +	tw9900_fill_fmt(tw9900->cur_mode, try_fmt);
>> +
>> +	return 0;
>> +}  
>
>Since the format is fixed based on the current standard, there is no point
>in initializing try_fmt as it won't be used. So just drop tw9900_open altogether.

Ok I'll drop that :)

[...]

>> +static const struct v4l2_subdev_video_ops tw9900_video_ops = {
>> +	.s_std		= tw9900_s_std,
>> +	.g_std		= tw9900_g_std,  
>
>Can the tw9900 detect the standard? (I.e. PAL, SECAM, NTSC)
>
>If so, you should implement querystd. I see that none of the other tw*.c drivers
>support this, so I suspect there is no hardware support for this.

So, there's hardware support for this, and I've been trying to get this
to work for a while. I've come to a point where the standard detection
"almost" works, but detects the wrong standard about once every 10
occurences. I don't know if this is due to the hardware I'm testing
this on, my setup, or the decoder itself.

This is in a setup where the standard can change on the fly (I have 2
cameras, one that streams PAL, one that streams NTSC, that are connected
to the TW9900 through a switch, and I have to make so that we can
detect a standard change (due to switching a camera) on the fly while
the stream is started.

The standard detection is also a process that is quite long and that
has to be manually started, and then checked regularly to see if the
decoder successfully identified a standard.

I do have a followup question, which is when querystd() would be called
in a "normal" scenarion (I feel that my usecase seems a bit off-track
compared to classic usecases). Is it when the stream is started, or
stopped ?

>You also must implement g_tvnorms to report the TV standards that the hardware
>can understand.

Ok I didn't know about that, I'll implement that then.

[...]

>> +
>> +	tw9900->subdev.internal_ops = &tw9900_internal_ops;
>> +	tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;  
>
>This is a duplicate of a similar '|=' above.

My bad, I'll remove that line.


>
>Regards,
>
>	Hans

Thanks,

Maxime