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

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

[Mehdi Djait]

Hello everyone,

V7 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.

The v6 is based on the fifth iteration of the series introducing the
tw9900 driver: sent 01 Apr 2021 [1]

v6 => v7:
- added powerdown-gpios and input ports to dt-bindings
- added #include <linux/bitfield.h> to fix a Warning from the kernel
  robot
- removed a dev_info and replaced a dev_err by dev_err_probe

v5 => v6:
- dropped .skip_top and .field in the supported_modes
- added error handling for the i2c writes/reads
- added the colorimetry information to fill_fmt
- removed pm_runtime
- added the g_input_status callback
- dropped SECAM
- dropped the non-standard PAL/NTSC variants

Any feedback is appreciated,

Mehdi Djait

media_tree, base-commit: 2c1bae27df787c9535e48cc27bbd11c3c3e0a235

[1] https://lore.kernel.org/linux-media/20210401070802.1685823-1-maxime.chevallier@bootlin.com/


Mehdi Djait (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

 .../bindings/media/i2c/techwell,tw9900.yaml   |  82 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/media/i2c/Kconfig                     |  12 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/tw9900.c                    | 648 ++++++++++++++++++
 6 files changed, 751 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/techwell,tw9900.yaml
 create mode 100644 drivers/media/i2c/tw9900.c

-- 
2.41.0

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

[Mehdi Djait]

Add prefix for Techwell, Inc.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Mehdi Djait <mehdi.djait@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 573578db9509..08b74f725142 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1357,6 +1357,8 @@ patternProperties:
     description: Technologic Systems
   "^techstar,.*":
     description: Shenzhen Techstar Electronics Co., Ltd.
+  "^techwell,.*":
+    description: Techwell, Inc.
   "^teejet,.*":
     description: TeeJet
   "^teltonika,.*":
-- 
2.41.0

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

[Mehdi Djait]

The Techwell TW9900 is a video decoder supporting multiple input
standards, such as PAL and NTSC, 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: Mehdi Djait <mehdi.djait@bootlin.com>
---
V6->V7: 
- added powerdown-gpios and input ports
- used 4 spaces for example identation

V5->V6: 
- This commit had a "Reviewed-by: Rob Herring <robh@kernel.org>" Tag but
  decided not to collect it because the last Iteration was more than 2
  years ago
- removed SECAM from the mentioned standards
- changed maintainer

V4->V5: 
- renamed the file to match the compatible string, and referenced
  the graph.yaml schema

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

V2->V3: 
- fix the example not compiling due to a typo in the reset-gpios
  node.

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

diff --git a/Documentation/devicetree/bindings/media/i2c/techwell,tw9900.yaml b/Documentation/devicetree/bindings/media/i2c/techwell,tw9900.yaml
new file mode 100644
index 000000000000..244289a9a3e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/techwell,tw9900.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/techwell,tw9900.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Techwell TW9900 NTSC/PAL video decoder
+
+maintainers:
+  - Mehdi Djait <mehdi.djait@bootlin.com>
+
+description:
+  The tw9900 is a multi-standard video decoder, supporting NTSC, PAL standards
+  with auto-detection features.
+
+properties:
+  compatible:
+    const: techwell,tw9900
+
+  reg:
+    maxItems: 1
+
+  vdd-supply:
+    description: VDD power supply
+
+  reset-gpios:
+    description: GPIO descriptor for the RESET input pin
+    maxItems: 1
+
+  powerdown-gpios:
+    description: GPIO descriptor for the POWERDOWN input pin
+    maxItems: 1
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    properties:
+      port@3:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Video port for the decoder output.
+
+    patternProperties:
+      "^port@[0-2]$":
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Analog video input port.
+
+    required:
+      - port@3
+
+required:
+  - compatible
+  - ports
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        video-decoder@44 {
+            compatible = "techwell,tw9900";
+            reg = <0x44>;
+
+            vdd-supply = <&tw9900_supply>;
+            reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@3 {
+                    reg = <3>;
+                    endpoint {
+                        remote-endpoint = <&vip_in>;
+                    };
+                };
+            };
+        };
+    };
-- 
2.41.0

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

On Mon, Oct 16, 2023 at 03:58:32PM +0200, Mehdi Djait wrote:
> The Techwell TW9900 is a video decoder supporting multiple input
> standards, such as PAL and NTSC, 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: Mehdi Djait <mehdi.djait@bootlin.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Rob Herring]

On Mon, Oct 16, 2023 at 03:58:32PM +0200, Mehdi Djait wrote:
> The Techwell TW9900 is a video decoder supporting multiple input
> standards, such as PAL and NTSC, 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: Mehdi Djait <mehdi.djait@bootlin.com>
> ---
> V6->V7: 
> - added powerdown-gpios and input ports
> - used 4 spaces for example identation
> 
> V5->V6: 
> - This commit had a "Reviewed-by: Rob Herring <robh@kernel.org>" Tag but
>   decided not to collect it because the last Iteration was more than 2
>   years ago
> - removed SECAM from the mentioned standards
> - changed maintainer
> 
> V4->V5: 
> - renamed the file to match the compatible string, and referenced
>   the graph.yaml schema
> 
> V3->V4: 
> - add the missing reset-gpios node to the binding
> 
> V2->V3: 
> - fix the example not compiling due to a typo in the reset-gpios
>   node.
> 
>  .../bindings/media/i2c/techwell,tw9900.yaml   | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/techwell,tw9900.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/techwell,tw9900.yaml b/Documentation/devicetree/bindings/media/i2c/techwell,tw9900.yaml
> new file mode 100644
> index 000000000000..244289a9a3e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/techwell,tw9900.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/techwell,tw9900.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Techwell TW9900 NTSC/PAL video decoder
> +
> +maintainers:
> +  - Mehdi Djait <mehdi.djait@bootlin.com>
> +
> +description:
> +  The tw9900 is a multi-standard video decoder, supporting NTSC, PAL standards
> +  with auto-detection features.
> +
> +properties:
> +  compatible:
> +    const: techwell,tw9900
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: VDD power supply
> +
> +  reset-gpios:
> +    description: GPIO descriptor for the RESET input pin
> +    maxItems: 1
> +
> +  powerdown-gpios:
> +    description: GPIO descriptor for the POWERDOWN input pin
> +    maxItems: 1
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    properties:
> +      port@3:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Video port for the decoder output.
> +
> +    patternProperties:
> +      "^port@[0-2]$":
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Analog video input port.

You need to define each port (0, 1, 2). Are those each component or 
composite, s-video, and component? The latter would make more sense as I 
don't think there would be a need to route each component to different 
place. It's logically just 1 stream.

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

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

[Mehdi Djait]

The Techwell video decoder supports PAL, NTSC 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.

The TW9900 is capable of doing automatic standard detection, this is
implemented with support for PAL and NTSC autodetection.

Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
---
V6->V7: 
- added #include <linux/bitfield.h> to fix a Warning from the kernel
  robot
- removed a dev_info and replaced a dev_err by dev_err_probe

V5->V6: 
- dropped .skip_top and .field in the supported_modes
- added error handling for the i2c writes/reads
- added the colorimetry information to fill_fmt
- removed pm_runtime
- added the g_input_status callback
- dropped SECAM
- dropped the non-standard PAL/NTSC variants

V4->V5: 
- Added .querystd() and .g_tvnorms(), dropped the .open() and
  unified the g_fmt() / s_fmt().

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

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.

V1->V2: 
- Set the media entity type to decoder, and implement the
  s_std/g_std ops

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 90f13281d297..164dfb016f18 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21121,6 +21121,12 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/media/rc/ttusbir.c
 
+TECHWELL TW9900 VIDEO DECODER
+M:	Mehdi Djait <mehdi.djait@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 74ff833ff48c..1bd412d90cee 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1175,6 +1175,18 @@ 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_DEV && I2C
+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	help
+	  Support for the Techwell tw9900 multi-standard video decoder.
+	  It supports NTSC, PAL standards with auto-detection features.
+
+	  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_DEV && I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 80b00d39b48f..ec318e1fb0c3 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -135,6 +135,7 @@ obj-$(CONFIG_VIDEO_TVP514X) += tvp514x.o
 obj-$(CONFIG_VIDEO_TVP5150) += tvp5150.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..41776992cb4e
--- /dev/null
+++ b/drivers/media/i2c/tw9900.c
@@ -0,0 +1,648 @@
+// 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>
+ * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.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_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_SEL	0x1C
+#define TW9900_REG_STD_SEL_AUTODETECT_IN_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
+
+#define TW9900_CHIP_ID		0x00
+
+#define VSYNC_POLL_INTERVAL_MS	20
+#define VSYNC_WAIT_MAX_POLLS	50
+
+#define TW9900_STD_NTSC_M	0
+#define TW9900_STD_PAL_BDGHI	1
+#define TW9900_STD_AUTO		7
+
+#define TW9900_VIDEO_POLL_TIMEOUT 20
+
+struct regval {
+	u8 addr;
+	u8 val;
+};
+
+struct tw9900_mode {
+	u32 width;
+	u32 height;
+	u32 std;
+	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 = 480,
+		.std = V4L2_STD_NTSC,
+		.reg_list = tw9900_ntsc_regs,
+		.n_regs = ARRAY_SIZE(tw9900_ntsc_regs),
+	},
+	{
+		.width = 720,
+		.height = 576,
+		.std = V4L2_STD_PAL,
+		.reg_list = tw9900_pal_regs,
+		.n_regs = ARRAY_SIZE(tw9900_pal_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);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int tw9900_read_reg(struct i2c_client *client, u8 reg)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, reg);
+	if (ret < 0)
+		dev_err(&client->dev, "read reg error: %d\n", ret);
+
+	return ret;
+}
+
+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 = V4L2_FIELD_NONE;
+	fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
+	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
+	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
+}
+
+static int tw9900_cfg_fmt(struct v4l2_subdev *sd,
+			  struct v4l2_subdev_state *sd_state,
+			  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_state *sd_state,
+				 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_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");
+
+	return ret;
+
+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);
+
+	switch (ctrl->id) {
+	case V4L2_CID_BRIGHTNESS:
+		return tw9900_write_reg(tw9900->client, 0x10, (u8)ctrl->val);
+	case V4L2_CID_CONTRAST:
+		return tw9900_write_reg(tw9900->client, 0x11, (u8)ctrl->val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int tw9900_s_stream(struct v4l2_subdev *sd, int on)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+	struct i2c_client *client = tw9900->client;
+	int ret;
+
+	on = !!on;
+	if (on == tw9900->streaming)
+		return 0;
+
+	if (on) {
+		ret = v4l2_ctrl_handler_setup(sd->ctrl_handler);
+		if (ret)
+			return ret;
+
+		ret = tw9900_write_array(tw9900->client,
+					 tw9900->cur_mode->reg_list,
+					 tw9900->cur_mode->n_regs);
+		if (ret)
+			return ret;
+
+		ret = tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL,
+				       TW9900_REG_OUT_FMT_CTL_STREAMING);
+		if (ret)
+			return ret;
+
+	} else {
+		ret = tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL,
+				       TW9900_REG_OUT_FMT_CTL_STANDBY);
+		if (ret)
+			return ret;
+	}
+
+	tw9900->streaming = on;
+
+	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;
+
+	if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
+		return -EINVAL;
+
+	mode = tw9900_get_mode_from_std(norm);
+	if (!mode)
+		return -EINVAL;
+
+	tw9900->cur_mode = mode;
+
+	return 0;
+}
+
+static int tw9900_get_stream_std(struct tw9900 *tw9900,
+				 v4l2_std_id *std_id)
+{
+	int std, ret;
+
+	ret = tw9900_read_reg(tw9900->client, TW9900_REG_STD_SEL);
+	if (ret < 0) {
+		*std_id = V4L2_STD_UNKNOWN;
+		return ret;
+	}
+
+	std = FIELD_GET(TW9900_STDNOW_MASK, ret);
+	switch (std) {
+	case TW9900_STD_NTSC_M:
+		*std_id = V4L2_STD_NTSC;
+		break;
+	case TW9900_STD_PAL_BDGHI:
+		*std_id = V4L2_STD_PAL;
+		break;
+	case TW9900_STD_AUTO:
+		*std_id = V4L2_STD_UNKNOWN;
+		break;
+	default:
+		*std_id = V4L2_STD_UNKNOWN;
+		break;
+	}
+
+	return 0;
+}
+
+static int tw9900_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+
+	*std = tw9900->cur_mode->std;
+
+	return 0;
+}
+
+static int tw9900_start_autodetect(struct tw9900 *tw9900)
+{
+	int ret;
+
+	ret = tw9900_write_reg(tw9900->client, TW9900_REG_STDR,
+			       BIT(TW9900_STD_NTSC_M) |
+			       BIT(TW9900_STD_PAL_BDGHI));
+	if (ret)
+		return ret;
+
+	ret = tw9900_write_reg(tw9900->client, TW9900_REG_STD_SEL,
+			       TW9900_STD_AUTO);
+	if (ret)
+		return ret;
+
+	ret = tw9900_write_reg(tw9900->client, TW9900_REG_STDR,
+			       BIT(TW9900_STD_NTSC_M) |
+			       BIT(TW9900_STD_PAL_BDGHI) |
+			       BIT(TW9900_STD_AUTO));
+	if (ret)
+		return ret;
+
+	/* Autodetect takes a while to start, and during the starting sequence
+	 * the autodetection status is reported as done.
+	 */
+	msleep(30);
+
+	return 0;
+}
+
+static int tw9900_cancel_autodetect(struct tw9900 *tw9900)
+{
+	return tw9900_s_std(&tw9900->subdev, tw9900->cur_mode->std);
+}
+
+static int tw9900_detect_done(struct tw9900 *tw9900, bool *done)
+{
+	int ret;
+
+	ret = tw9900_read_reg(tw9900->client, TW9900_REG_STD_SEL);
+	if (ret < 0)
+		return ret;
+
+	*done = !(ret & TW9900_REG_STD_SEL_AUTODETECT_IN_PROGRESS);
+
+	return 0;
+}
+
+static int tw9900_querystd(struct v4l2_subdev *sd, v4l2_std_id *std_id)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+	bool done = false;
+	int i, ret;
+
+	if (tw9900->streaming)
+		return -EBUSY;
+
+	ret = tw9900_start_autodetect(tw9900);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < TW9900_VIDEO_POLL_TIMEOUT; i++) {
+		ret = tw9900_detect_done(tw9900, &done);
+		if (ret)
+			return ret;
+
+		if (done)
+			break;
+
+		msleep(20);
+	}
+
+	if (!done) {
+		tw9900_cancel_autodetect(tw9900);
+		return -EBUSY;
+	}
+
+	return tw9900_get_stream_std(tw9900, std_id);
+}
+
+static int tw9900_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *norm)
+{
+	*norm = V4L2_STD_NTSC | V4L2_STD_PAL;
+
+	return 0;
+}
+
+static int tw9900_g_input_status(struct v4l2_subdev *sd, u32 *status)
+{
+	struct tw9900 *tw9900 = to_tw9900(sd);
+	int ret;
+
+	ret = tw9900_read_reg(tw9900->client, TW9900_REG_CHIP_STATUS);
+	if (ret < 0)
+		return ret;
+
+	*status = ret & TW9900_REG_CHIP_STATUS_HLOCK ? 0 : V4L2_IN_ST_NO_SIGNAL;
+
+	return 0;
+}
+
+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,
+	.querystd	= tw9900_querystd,
+	.g_tvnorms	= tw9900_g_tvnorms,
+	.g_input_status = tw9900_g_input_status,
+	.s_stream	= tw9900_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops tw9900_pad_ops = {
+	.enum_mbus_code		= tw9900_enum_mbus_code,
+	.get_fmt		= tw9900_cfg_fmt,
+	.set_fmt		= tw9900_cfg_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 int tw9900_check_id(struct tw9900 *tw9900,
+			   struct i2c_client *client)
+{
+	struct device *dev = &tw9900->client->dev;
+	int ret;
+
+	ret = tw9900_read_reg(client, TW9900_CHIP_ID);
+	if (ret < 0)
+		return ret;
+
+	if (ret != TW9900_CHIP_ID) {
+		dev_err(dev, "Unexpected decoder id(0x%x)\n", ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int tw9900_probe(struct i2c_client *client)
+{
+	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))
+		return dev_err_probe(dev, PTR_ERR(tw9900->regulator),
+				     "Failed to get power regulator\n");
+
+	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;
+
+	ret = v4l2_ctrl_handler_init(hdl, 2);
+	if (ret)
+		return ret;
+
+	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.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;
+	}
+
+	return 0;
+
+err_clean_entity:
+	media_entity_cleanup(&tw9900->subdev.entity);
+err_power_off:
+	tw9900_power_off(tw9900);
+
+	return ret;
+}
+
+static void tw9900_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+
+	v4l2_async_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
+}
+
+static const struct i2c_device_id tw9900_id[] = {
+	{ "tw9900", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tw9900_id);
+
+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",
+		.of_match_table	= tw9900_of_match
+	},
+	.probe	  = tw9900_probe,
+	.remove	  = tw9900_remove,
+	.id_table = tw9900_id,
+};
+
+module_i2c_driver(tw9900_i2c_driver);
+
+MODULE_DESCRIPTION("tw9900 decoder driver");
+MODULE_LICENSE("GPL");
-- 
2.41.0

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

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 24322 bytes --]

Hi Mehdi,

On Mon 16 Oct 23, 15:58, Mehdi Djait wrote:
> The Techwell video decoder supports PAL, NTSC input formats,

PAL and NTSC are not formats, they are standards or interfaces.
Also no preceeding comma before "and".

> and outputs a BT.656 signal.

You could mention this is a parallel interface (also signal is not the best
wording).

> 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 doing automatic standard detection, this is
> implemented with support for PAL and NTSC autodetection.

Please mention that this an I2C (or SMBUS) device.

> Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> ---
> V6->V7: 
> - added #include <linux/bitfield.h> to fix a Warning from the kernel
>   robot
> - removed a dev_info and replaced a dev_err by dev_err_probe
> 
> V5->V6: 
> - dropped .skip_top and .field in the supported_modes
> - added error handling for the i2c writes/reads
> - added the colorimetry information to fill_fmt
> - removed pm_runtime
> - added the g_input_status callback
> - dropped SECAM
> - dropped the non-standard PAL/NTSC variants
> 
> V4->V5: 
> - Added .querystd() and .g_tvnorms(), dropped the .open() and
>   unified the g_fmt() / s_fmt().
> 
> V3->V4: 
> - Fix a warning about an uninitialized ret variable in probe()
> 
> 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.
> 
> V1->V2: 
> - Set the media entity type to decoder, and implement the
>   s_std/g_std ops
> 
>  MAINTAINERS                |   6 +
>  drivers/media/i2c/Kconfig  |  12 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/tw9900.c | 648 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 667 insertions(+)
>  create mode 100644 drivers/media/i2c/tw9900.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90f13281d297..164dfb016f18 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21121,6 +21121,12 @@ L:	linux-media@vger.kernel.org
>  S:	Maintained
>  F:	drivers/media/rc/ttusbir.c
>  
> +TECHWELL TW9900 VIDEO DECODER
> +M:	Mehdi Djait <mehdi.djait@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 74ff833ff48c..1bd412d90cee 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1175,6 +1175,18 @@ 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_DEV && I2C
> +	select MEDIA_CONTROLLER
> +	select VIDEO_V4L2_SUBDEV_API
> +	help
> +	  Support for the Techwell tw9900 multi-standard video decoder.
> +	  It supports NTSC, PAL standards with auto-detection features.
> +
> +	  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_DEV && I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 80b00d39b48f..ec318e1fb0c3 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -135,6 +135,7 @@ obj-$(CONFIG_VIDEO_TVP514X) += tvp514x.o
>  obj-$(CONFIG_VIDEO_TVP5150) += tvp5150.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..41776992cb4e
> --- /dev/null
> +++ b/drivers/media/i2c/tw9900.c
> @@ -0,0 +1,648 @@
> +// 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>
> + * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.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_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_SEL	0x1C
> +#define TW9900_REG_STD_SEL_AUTODETECT_IN_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
> +
> +#define TW9900_CHIP_ID		0x00
> +
> +#define VSYNC_POLL_INTERVAL_MS	20
> +#define VSYNC_WAIT_MAX_POLLS	50
> +
> +#define TW9900_STD_NTSC_M	0
> +#define TW9900_STD_PAL_BDGHI	1
> +#define TW9900_STD_AUTO		7
> +
> +#define TW9900_VIDEO_POLL_TIMEOUT 20
> +
> +struct regval {
> +	u8 addr;
> +	u8 val;
> +};
> +
> +struct tw9900_mode {
> +	u32 width;
> +	u32 height;
> +	u32 std;
> +	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;

These two are not used anywhere, please get rid of them (and corresponding
headers too).

> +
> +	const struct tw9900_mode *cur_mode;

Unless I'm mistaken you need to have a mutex here to serialize access to
hardware and for the current mode configuration.

> +};
> +
> +#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 = 480,
> +		.std = V4L2_STD_NTSC,
> +		.reg_list = tw9900_ntsc_regs,
> +		.n_regs = ARRAY_SIZE(tw9900_ntsc_regs),
> +	},
> +	{
> +		.width = 720,
> +		.height = 576,
> +		.std = V4L2_STD_PAL,
> +		.reg_list = tw9900_pal_regs,
> +		.n_regs = ARRAY_SIZE(tw9900_pal_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);

Is this an SMBUS device in particular? Or is there any reason to use the SMBUS
API instead of the general I2C API?

> +	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++) {

The ret == 0 check is not useful here, since you're checking it in the loop
already. Also <= instead of < looks very, very suspicious. Please double-check.

> +		ret = tw9900_write_reg(client, regs[i].addr, regs[i].val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tw9900_read_reg(struct i2c_client *client, u8 reg)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, reg);

Same comment about i2c vs smbus.

> +	if (ret < 0)
> +		dev_err(&client->dev, "read reg error: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +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 = V4L2_FIELD_NONE;
> +	fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> +}
> +
> +static int tw9900_cfg_fmt(struct v4l2_subdev *sd,

You might have to differentiate between set_fmt/get_fmt to return -EBUSY
if streaming is on in set_fmt. However I understand it will just copy the
current mode in both cases, but this might still be required to follow v4l2
semantics (please double-check).

> +			  struct v4l2_subdev_state *sd_state,
> +			  struct v4l2_subdev_format *fmt)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;

Should be mutex-protected (for cur_mode access).

> +
> +	tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt);
> +
> +	return 0;
> +}
> +
> +static int tw9900_enum_mbus_code(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *sd_state,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->index >= 1)

Make this > 0, it will be easier to read/understand.

> +		return -EINVAL;
> +
> +	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +
> +	return 0;
> +}
> +
> +static int tw9900_power_on(struct tw9900 *tw9900)
> +{
> +	int ret;
> +	struct device *dev = &tw9900->client->dev;

Define ret last here.

> +
> +	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");
> +
> +	return ret;
> +
> +error:
> +

Remove this newline.

> +	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);
> +}

Power on/off would be best folded into runtime pm.

> +static int tw9900_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct tw9900 *tw9900 = container_of(ctrl->handler, struct tw9900, hdl);
> +

This function should be mutex-protected to serialize register access.

> +	switch (ctrl->id) {
> +	case V4L2_CID_BRIGHTNESS:
> +		return tw9900_write_reg(tw9900->client, 0x10, (u8)ctrl->val);

Use the define, not the raw address.

> +	case V4L2_CID_CONTRAST:
> +		return tw9900_write_reg(tw9900->client, 0x11, (u8)ctrl->val);

Ditto.

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int tw9900_s_stream(struct v4l2_subdev *sd, int on)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	struct i2c_client *client = tw9900->client;
> +	int ret;

This function should be mutex-protected to serialize register access.

> +
> +	on = !!on;
> +	if (on == tw9900->streaming)

It's a lot more readable and clear to avoid the !!on and do:
if ((tw9900->streaming && on) || (!tw9900->streaming && !on))

Also there's a chance that the framework already does this verification (please
double-check).

> +		return 0;
> +
> +	if (on) {

You should power on here.

> +		ret = v4l2_ctrl_handler_setup(sd->ctrl_handler);

Once this is mutex-protected, you need to use __v4l2_ctrl_handler_setup.

> +		if (ret)
> +			return ret;
> +
> +		ret = tw9900_write_array(tw9900->client,
> +					 tw9900->cur_mode->reg_list,
> +					 tw9900->cur_mode->n_regs);
> +		if (ret)
> +			return ret;
> +
> +		ret = tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL,
> +				       TW9900_REG_OUT_FMT_CTL_STREAMING);
> +		if (ret)
> +			return ret;
> +
> +	} else {
> +		ret = tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL,
> +				       TW9900_REG_OUT_FMT_CTL_STANDBY);
> +		if (ret)
> +			return ret;

You should power off here.

> +	}
> +
> +	tw9900->streaming = on;
> +
> +	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)

Please stay consistent in how these are called: std or norm but pick one and
stick to it.

> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	const struct tw9900_mode *mode;

Should be mutex-protected (for cur_mode).

> +
> +	if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
> +		return -EINVAL;
> +
> +	mode = tw9900_get_mode_from_std(norm);

This function is only called once from here so you can fold it in.

> +	if (!mode)
> +		return -EINVAL;
> +
> +	tw9900->cur_mode = mode;
> +
> +	return 0;
> +}
> +
> +static int tw9900_get_stream_std(struct tw9900 *tw9900,
> +				 v4l2_std_id *std_id)

Same comment about variable naming inconsistency.

> +{
> +	int std, ret;

Should be mutex-protected.

> +
> +	ret = tw9900_read_reg(tw9900->client, TW9900_REG_STD_SEL);
> +	if (ret < 0) {
> +		*std_id = V4L2_STD_UNKNOWN;
> +		return ret;
> +	}
> +
> +	std = FIELD_GET(TW9900_STDNOW_MASK, ret);
> +	switch (std) {
> +	case TW9900_STD_NTSC_M:
> +		*std_id = V4L2_STD_NTSC;
> +		break;
> +	case TW9900_STD_PAL_BDGHI:
> +		*std_id = V4L2_STD_PAL;
> +		break;
> +	case TW9900_STD_AUTO:
> +		*std_id = V4L2_STD_UNKNOWN;
> +		break;
> +	default:
> +		*std_id = V4L2_STD_UNKNOWN;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tw9900_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);

Should be mutex-protected.

> +
> +	*std = tw9900->cur_mode->std;
> +
> +	return 0;
> +}
> +
> +static int tw9900_start_autodetect(struct tw9900 *tw9900)
> +{
> +	int ret;
> +
> +	ret = tw9900_write_reg(tw9900->client, TW9900_REG_STDR,
> +			       BIT(TW9900_STD_NTSC_M) |
> +			       BIT(TW9900_STD_PAL_BDGHI));
> +	if (ret)
> +		return ret;
> +
> +	ret = tw9900_write_reg(tw9900->client, TW9900_REG_STD_SEL,
> +			       TW9900_STD_AUTO);
> +	if (ret)
> +		return ret;
> +
> +	ret = tw9900_write_reg(tw9900->client, TW9900_REG_STDR,
> +			       BIT(TW9900_STD_NTSC_M) |
> +			       BIT(TW9900_STD_PAL_BDGHI) |
> +			       BIT(TW9900_STD_AUTO));
> +	if (ret)
> +		return ret;
> +
> +	/* Autodetect takes a while to start, and during the starting sequence
> +	 * the autodetection status is reported as done.
> +	 */
> +	msleep(30);
> +
> +	return 0;
> +}
> +
> +static int tw9900_cancel_autodetect(struct tw9900 *tw9900)
> +{
> +	return tw9900_s_std(&tw9900->subdev, tw9900->cur_mode->std);
> +}
> +
> +static int tw9900_detect_done(struct tw9900 *tw9900, bool *done)
> +{
> +	int ret;
> +
> +	ret = tw9900_read_reg(tw9900->client, TW9900_REG_STD_SEL);
> +	if (ret < 0)
> +		return ret;
> +
> +	*done = !(ret & TW9900_REG_STD_SEL_AUTODETECT_IN_PROGRESS);
> +
> +	return 0;
> +}
> +
> +static int tw9900_querystd(struct v4l2_subdev *sd, v4l2_std_id *std_id)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	bool done = false;
> +	int i, ret;
> +

Should be mutex-protected.

> +	if (tw9900->streaming)
> +		return -EBUSY;
> +
> +	ret = tw9900_start_autodetect(tw9900);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < TW9900_VIDEO_POLL_TIMEOUT; i++) {
> +		ret = tw9900_detect_done(tw9900, &done);
> +		if (ret)
> +			return ret;
> +
> +		if (done)
> +			break;
> +
> +		msleep(20);
> +	}
> +
> +	if (!done) {
> +		tw9900_cancel_autodetect(tw9900);
> +		return -EBUSY;
> +	}
> +
> +	return tw9900_get_stream_std(tw9900, std_id);
> +}
> +
> +static int tw9900_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *norm)
> +{
> +	*norm = V4L2_STD_NTSC | V4L2_STD_PAL;
> +
> +	return 0;
> +}
> +
> +static int tw9900_g_input_status(struct v4l2_subdev *sd, u32 *status)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	int ret;
> +

Should be mutex-protected.

> +	ret = tw9900_read_reg(tw9900->client, TW9900_REG_CHIP_STATUS);
> +	if (ret < 0)
> +		return ret;
> +
> +	*status = ret & TW9900_REG_CHIP_STATUS_HLOCK ? 0 : V4L2_IN_ST_NO_SIGNAL;
> +
> +	return 0;
> +}
> +
> +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,
> +	.querystd	= tw9900_querystd,
> +	.g_tvnorms	= tw9900_g_tvnorms,
> +	.g_input_status = tw9900_g_input_status,
> +	.s_stream	= tw9900_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops tw9900_pad_ops = {
> +	.enum_mbus_code		= tw9900_enum_mbus_code,
> +	.get_fmt		= tw9900_cfg_fmt,
> +	.set_fmt		= tw9900_cfg_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 int tw9900_check_id(struct tw9900 *tw9900,
> +			   struct i2c_client *client)
> +{
> +	struct device *dev = &tw9900->client->dev;
> +	int ret;
> +
> +	ret = tw9900_read_reg(client, TW9900_CHIP_ID);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret != TW9900_CHIP_ID) {
> +		dev_err(dev, "Unexpected decoder id(0x%x)\n", ret);
> +		return -EINVAL;

Maybe -ENODEV instead.

> +	}
> +
> +	return 0;
> +}
> +
> +static int tw9900_probe(struct i2c_client *client)
> +{
> +	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);

From the looks of the code using it, it seems to be optional. 
This will return -ENOENT if the gpio is not present, not NULL.
You probably want to use devm_gpiod_get_optional instead.

Also the binding should make it clear that this is optional.

> +	if (IS_ERR(tw9900->reset_gpio))
> +		tw9900->reset_gpio = NULL;

You don't want to do this, since it will ignore all errors (including ones
were you do supply a GPIO but the provider fails to make it available).

You should dev_err_probe too.

> +
> +	tw9900->regulator = devm_regulator_get(&tw9900->client->dev, "vdd");

This is not marked as a required property in the binding. If this is optional,
the code should reflect it (devm_regulator_get_optional + conditionals).
If it's mandatory (which is probably the case), the binding should be updated to
have vdd-supply as a required property.

> +	if (IS_ERR(tw9900->regulator))
> +		return dev_err_probe(dev, PTR_ERR(tw9900->regulator),
> +				     "Failed to get power regulator\n");
> +
> +	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;
> +
> +	ret = v4l2_ctrl_handler_init(hdl, 2);
> +	if (ret)
> +		return ret;
> +
> +	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;
> +

You should power off after checking the id.

Right now you're keeping the device powered at all times after it was probed,
which is not right. Consider switching to runtime pm too.

> +	tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;

You're already setting this earlier.

> +	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;
> +	}
> +
> +	return 0;
> +
> +err_clean_entity:
> +	media_entity_cleanup(&tw9900->subdev.entity);
> +err_power_off:
> +	tw9900_power_off(tw9900);
> +
> +	return ret;
> +}
> +
> +static void tw9900_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +
> +	v4l2_async_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +}
> +
> +static const struct i2c_device_id tw9900_id[] = {
> +	{ "tw9900", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tw9900_id);
> +
> +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",
> +		.of_match_table	= tw9900_of_match
> +	},
> +	.probe	  = tw9900_probe,
> +	.remove	  = tw9900_remove,
> +	.id_table = tw9900_id,
> +};
> +
> +module_i2c_driver(tw9900_i2c_driver);
> +
> +MODULE_DESCRIPTION("tw9900 decoder driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.41.0
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mehdi Djait]

Hi Paul,

thank you for the review!

On Thu, Nov 02, 2023 at 11:03:42AM +0100, Paul Kocialkowski wrote:
> > +static int tw9900_write_reg(struct i2c_client *client, u8 reg, u8 val)
> > +{
> > +	int ret;
> > +
> > +	ret = i2c_smbus_write_byte_data(client, reg, val);
> 
> Is this an SMBUS device in particular? Or is there any reason to use the SMBUS
> API instead of the general I2C API?
> 

I think I will keep using the SMBUS API here. The reason is in the
kernel documentation:

--------------------------------------------------------------------------------
If you write a driver for some I2C device, please try to use the SMBus commands 
if at all possible (if the device uses only that subset of the I2C protocol). 
This makes it possible to use the device driver on both SMBus adapters and I2C 
adapters (the SMBus command set is automatically translated to I2C on I2C 
adapters, but plain I2C commands can not be handled at all on most pure SMBus 
adapters).
--------------------------------------------------------------------------------

And the vast majority of the drivers under /media/i2c are using the
SMBUS API.

> > +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 = V4L2_FIELD_NONE;
> > +	fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> > +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> > +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> > +}
> > +
> > +static int tw9900_cfg_fmt(struct v4l2_subdev *sd,
> 
> You might have to differentiate between set_fmt/get_fmt to return -EBUSY
> if streaming is on in set_fmt. However I understand it will just copy the
> current mode in both cases, but this might still be required to follow v4l2
> semantics (please double-check).
> 

This should be done in the driver calling the pad subdev_call set_fmt,
right ?

--
Kind Regards
Mehdi Djait

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

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2884 bytes --]

Hi Mehdi,

On Mon 06 Nov 23, 15:58, Mehdi Djait wrote:
> Hi Paul,
> 
> thank you for the review!
> 
> On Thu, Nov 02, 2023 at 11:03:42AM +0100, Paul Kocialkowski wrote:
> > > +static int tw9900_write_reg(struct i2c_client *client, u8 reg, u8 val)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = i2c_smbus_write_byte_data(client, reg, val);
> > 
> > Is this an SMBUS device in particular? Or is there any reason to use the SMBUS
> > API instead of the general I2C API?
> > 
> 
> I think I will keep using the SMBUS API here. The reason is in the
> kernel documentation:
> 
> --------------------------------------------------------------------------------
> If you write a driver for some I2C device, please try to use the SMBus commands 
> if at all possible (if the device uses only that subset of the I2C protocol). 
> This makes it possible to use the device driver on both SMBus adapters and I2C 
> adapters (the SMBus command set is automatically translated to I2C on I2C 
> adapters, but plain I2C commands can not be handled at all on most pure SMBus 
> adapters).
> --------------------------------------------------------------------------------
> 
> And the vast majority of the drivers under /media/i2c are using the
> SMBUS API.

That is a good reason, so let's keep it that way then.

> > > +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 = V4L2_FIELD_NONE;
> > > +	fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > > +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> > > +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> > > +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> > > +}
> > > +
> > > +static int tw9900_cfg_fmt(struct v4l2_subdev *sd,
> > 
> > You might have to differentiate between set_fmt/get_fmt to return -EBUSY
> > if streaming is on in set_fmt. However I understand it will just copy the
> > current mode in both cases, but this might still be required to follow v4l2
> > semantics (please double-check).
> > 
> 
> This should be done in the driver calling the pad subdev_call set_fmt,
> right ?

Well the two things are distinct, even though it's not obvious to think about
a case where you wouldn't have a video device to grab the frames.

For instance you can see this being done here:
https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov5648.c#L2259

I'm just not sure about what the V4L2 subdev API mandates. It would be useful
to find some piece of documentation that clarifies the requirement.

Cheers,

Paul

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mehdi Djait]

Hi Paul,

On Mon, Nov 06, 2023 at 04:25:18PM +0100, Paul Kocialkowski wrote:
> > > > +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 = V4L2_FIELD_NONE;
> > > > +	fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > > > +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> > > > +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> > > > +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> > > > +}
> > > > +
> > > > +static int tw9900_cfg_fmt(struct v4l2_subdev *sd,
> > > 
> > > You might have to differentiate between set_fmt/get_fmt to return -EBUSY
> > > if streaming is on in set_fmt. However I understand it will just copy the
> > > current mode in both cases, but this might still be required to follow v4l2
> > > semantics (please double-check).
> > > 
> > 
> > This should be done in the driver calling the pad subdev_call set_fmt,
> > right ?
> 
> Well the two things are distinct, even though it's not obvious to think about
> a case where you wouldn't have a video device to grab the frames.
> 
> For instance you can see this being done here:
> https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov5648.c#L2259
> 
> I'm just not sure about what the V4L2 subdev API mandates. It would be useful
> to find some piece of documentation that clarifies the requirement.

Ok, I will split the functions then.

--
Kind Regards
Mehdi Djait

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

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2371 bytes --]

Hi,

On Mon 06 Nov 23, 16:49, Mehdi Djait wrote:
> Hi Paul,
> 
> On Mon, Nov 06, 2023 at 04:25:18PM +0100, Paul Kocialkowski wrote:
> > > > > +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 = V4L2_FIELD_NONE;
> > > > > +	fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > > > > +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> > > > > +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> > > > > +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> > > > > +}
> > > > > +
> > > > > +static int tw9900_cfg_fmt(struct v4l2_subdev *sd,
> > > > 
> > > > You might have to differentiate between set_fmt/get_fmt to return -EBUSY
> > > > if streaming is on in set_fmt. However I understand it will just copy the
> > > > current mode in both cases, but this might still be required to follow v4l2
> > > > semantics (please double-check).
> > > > 
> > > 
> > > This should be done in the driver calling the pad subdev_call set_fmt,
> > > right ?
> > 
> > Well the two things are distinct, even though it's not obvious to think about
> > a case where you wouldn't have a video device to grab the frames.
> > 
> > For instance you can see this being done here:
> > https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov5648.c#L2259
> > 
> > I'm just not sure about what the V4L2 subdev API mandates. It would be useful
> > to find some piece of documentation that clarifies the requirement.
> 
> Ok, I will split the functions then.

I was more interested in finding out the right answer rather than having you
follow an example from another driver. You can see the docs at:
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-fmt.html#return-value

So I think it's a good measure to return -EBUSY here. You can just have set_fmt
call into get_fmt with an extra check before.

Also see the part about the which field. I'm not sure it should have
implications in your case but better double-check that too.

Cheers,

Paul

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]