[PATCH 00/21] media: i2c: it6625: address review feedback and adopt subdev state

[PATCH 00/21] media: i2c: it6625: address review feedback and adopt subdev state

[Hermes Wu via B4 Relay]

Sakari Ailus reviewed "[PATCH v10 2/2] media: i2c: add driver for ITE
IT6625/IT6626" and, separately, "[PATCH v1 1/2]" (the dt-bindings patch)
after both had already been merged into next. This series addresses
that feedback as a follow-up, since the original commits can no longer
be amended.

Scope of this series:
 - one dt-bindings fix documenting the CSI-2 bus-type default;
 - one control-update error-handling fix;
 - mechanical and style cleanups (declaration ordering, unsigned loop
   indices, redundant boilerplate, unaligned-access helpers, an early
   return conversion);
 - a link-frequency reporting fix for one-/two-trio C-PHY configurations,
   found while implementing a related style request;
 - tightening DT endpoint parsing to require the documented endpoint
   instead of silently defaulting;
 - folding subdev initialization into probe() for explicit error
   handling;
 - adopting the subdev active-state model: sharing the driver mutex as
   both the subdev state lock and the control handler lock, moving the
   current format out of driver-private fields and into the pad format,
   and calling v4l2_subdev_init_finalize()/v4l2_subdev_cleanup();
 - converting from the deprecated s_stream video op to the
   enable_streams/disable_streams pad ops, keeping
   v4l2_subdev_s_stream_helper for legacy callers.

The locking-model change is the one part of this series most likely to
need another look: it changes what the driver's own mutex protects and
who else already holds it by the time driver code runs. It's isolated
to its own commit with an explicit trace of every lock acquisition site
against the actual core locking contract, kept separate from the
mechanical/bug-fix commits that precede it.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
Hermes Wu (21):
      media: dt-bindings: ite,it6625: document the default CSI-2 bus type
      media: i2c: it6625: propagate control-update errors
      media: i2c: it6625: default the debug module parameter to 0
      media: i2c: it6625: drop unused bus field from struct it6625
      media: i2c: it6625: drop stale GCC < 4.4.6 workaround
      media: i2c: it6625: use unsigned int loop indices in table lookups
      media: i2c: it6625: drop redundant parentheses in status helpers
      media: i2c: it6625: make the audio sampling-rate table static const
      media: i2c: it6625: tidy CEC buffer init and a continuation line
      media: i2c: it6625: clean up it6625_wait_for_status()
      media: i2c: it6625: use unsigned int indices in EDID read/write
      media: i2c: it6625: use unaligned/units helpers to decode pixel clock
      media: i2c: it6625: decode detected timings via typed register structs
      media: i2c: it6625: fix link-frequency reporting for one-/two-trio C-PHY
      media: i2c: it6625: use early returns in it6625_update_timings_if_changed()
      media: i2c: it6625: drop the private CSI-format name table
      media: i2c: it6625: require a DT endpoint and simplify endpoint parsing
      media: i2c: it6625: finish reverse fir-tree declaration order
      media: i2c: it6625: fold subdev initialization into probe
      media: i2c: it6625: use centrally managed active state
      media: i2c: it6625: use enable_streams and disable_streams

 .../devicetree/bindings/media/i2c/ite,it6625.yaml  |   2 +
 drivers/media/i2c/it6625.c                         | 519 +++++++++++----------
 2 files changed, 282 insertions(+), 239 deletions(-)
---
base-commit: b38d06ad1e13c32970d58039c154f009b81a3368
change-id: 20260917-upstream-it6625-follow-up-patch-b81b34266c43

Best regards,
-- 
Hermes Wu <Hermes.wu@ite.com.tw>

[PATCH 01/21] media: dt-bindings: ite,it6625: document the default CSI-2 bus type

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

bus-type is optional under port@0 and port@1's endpoint for ite,it6625
(the allOf/if/then clause only requires it for ite,it6626, which
supports both C-PHY and D-PHY and needs disambiguation). With bus-type
omitted, the actual runtime default is D-PHY:
v4l2_fwnode_endpoint_parse_csi2_bus() falls back to
V4L2_MBUS_CSI2_DPHY whenever data-lanes is present, which this binding
already requires. Document that default explicitly instead of leaving
it unstated.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 Documentation/devicetree/bindings/media/i2c/ite,it6625.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ite,it6625.yaml b/Documentation/devicetree/bindings/media/i2c/ite,it6625.yaml
index 756fe655f534f85b23b7741d06710c956f9694d8..9b621f726249189255692da02049ebcbf1d5b43c 100644
--- a/Documentation/devicetree/bindings/media/i2c/ite,it6625.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ite,it6625.yaml
@@ -65,6 +65,7 @@ properties:
                 enum:
                   - 1 # MEDIA_BUS_TYPE_CSI2_CPHY
                   - 4 # MEDIA_BUS_TYPE_CSI2_DPHY
+                default: 4 # MEDIA_BUS_TYPE_CSI2_DPHY
 
             required:
               - data-lanes
@@ -88,6 +89,7 @@ properties:
                 enum:
                   - 1 # MEDIA_BUS_TYPE_CSI2_CPHY
                   - 4 # MEDIA_BUS_TYPE_CSI2_DPHY
+                default: 4 # MEDIA_BUS_TYPE_CSI2_DPHY
 
             required:
               - data-lanes

-- 
2.34.1

[PATCH 02/21] media: i2c: it6625: propagate control-update errors

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

it6625_v4l2_sd_ctrl_update() discarded the return value of all three
v4l2_ctrl_s_ctrl() calls it makes. Make it return int, run the updates
sequentially, and return the first error; both call sites now check
and log it.

None of the three controls has a driver .ops, so the only failure path
in v4l2_ctrl_s_ctrl() is range validation, and every value this driver
passes is always in range -- this is an error-handling correctness fix
responsive to review, not a fix for an observed runtime failure.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index da17e5d5ce9e2dd1060abba1598b0e3f78857e9c..5d267676606e340ad0ee5ad2b78e5acb50a32980 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -889,11 +889,19 @@ static int it6625_s_ctrl_audio_present(struct v4l2_subdev *sd)
 				audio_present(it6625));
 }
 
-static void it6625_v4l2_sd_ctrl_update(struct v4l2_subdev *sd)
+static int it6625_v4l2_sd_ctrl_update(struct v4l2_subdev *sd)
 {
-	it6625_s_ctrl_detect_hdmi_5v(sd);
-	it6625_s_ctrl_audio_sampling_rate(sd);
-	it6625_s_ctrl_audio_present(sd);
+	int ret;
+
+	ret = it6625_s_ctrl_detect_hdmi_5v(sd);
+	if (ret)
+		return ret;
+
+	ret = it6625_s_ctrl_audio_sampling_rate(sd);
+	if (ret)
+		return ret;
+
+	return it6625_s_ctrl_audio_present(sd);
 }
 
 static void it6625_enable_stream_locked(struct it6625 *it6625, bool enable)
@@ -1136,9 +1144,12 @@ static void it6625_clear_timings(struct it6625 *it6625)
 static void it6625_irq_hdmi_5v_change(struct it6625 *it6625)
 {
 	struct v4l2_subdev *sd = &it6625->sd;
+	int ret;
 
 	it6625_clear_timings(it6625);
-	it6625_v4l2_sd_ctrl_update(sd);
+	ret = it6625_v4l2_sd_ctrl_update(sd);
+	if (ret)
+		dev_err(it6625->dev, "%s: failed to update controls: %d", __func__, ret);
 }
 
 static void it6625_irq_hdcp_change(struct it6625 *it6625)
@@ -2297,7 +2308,9 @@ static int it6625_probe(struct i2c_client *client)
 	it6625_debugfs_init(it6625, client);
 
 	it6625_initial_setup(it6625);
-	it6625_v4l2_sd_ctrl_update(sd);
+	err = it6625_v4l2_sd_ctrl_update(sd);
+	if (err)
+		dev_err(it6625->dev, "%s: failed to update controls: %d", __func__, err);
 
 	err = v4l2_async_register_subdev(sd);
 	if (err < 0) {

-- 
2.34.1

[PATCH 03/21] media: i2c: it6625: default the debug module parameter to 0

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

Printing all debug information by default is noisy for a normal boot;
default debug to 0 like the module parameter's own description implies.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 5d267676606e340ad0ee5ad2b78e5acb50a32980..42bdcc3af326ceb6c529b8da35836c49c8c43643 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -29,7 +29,7 @@
 #include <media/v4l2-fwnode.h>
 #include <uapi/linux/it6625.h>
 
-static int debug = 3;
+static int debug;
 module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "debug level (0-3)");
 

-- 
2.34.1

[PATCH 04/21] media: i2c: it6625: drop unused bus field from struct it6625

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

struct it6625::bus has no reader anywhere in the driver -- the mipi_csi2
configuration is produced fresh in it6625_get_mbus_config() and parsed
locally in it6625_parse_endpoint(), neither of which touches this field.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 42bdcc3af326ceb6c529b8da35836c49c8c43643..e18baa6e1a1933f301d9d3057c2c08411f104077 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -268,7 +268,6 @@ struct it6625 {
 	struct mutex if_state_lock;
 
 	struct v4l2_subdev sd;
-	struct v4l2_mbus_config_mipi_csi2 bus;
 	struct video_device *vdev;
 	struct media_pad pad;
 	struct v4l2_ctrl_handler hdl;

-- 
2.34.1

[PATCH 05/21] media: i2c: it6625: drop stale GCC < 4.4.6 workaround

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

The kernel-wide minimum GCC version is 8.1 (scripts/min-tool-version.sh),
so the explicit .reserved = { 0 } initializer kept for GCC < 4.4.6 is no
longer needed in either v4l2_dv_timings_cap initializer.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index e18baa6e1a1933f301d9d3057c2c08411f104077..97780c3d2d8fd3567ef18b3d2fc46b4e1f0758d7 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -331,8 +331,6 @@ static const s64 it6625_link_freq[] = {
  */
 static const struct v4l2_dv_timings_cap it6625_timings_cap = {
 	.type = V4L2_DV_BT_656_1120,
-	/* keep this initialization for compatibility with GCC < 4.4.6 */
-	.reserved = { 0 },
 
 	V4L2_INIT_BT_TIMINGS(640, 3840, 480, 2160, 27000000, 300000000,
 			     V4L2_DV_BT_STD_CEA861 | V4L2_DV_BT_STD_DMT |
@@ -347,7 +345,6 @@ static const struct v4l2_dv_timings_cap it6625_timings_cap = {
  */
 static const struct v4l2_dv_timings_cap it6626_cphy_3trio_timings_cap = {
 	.type = V4L2_DV_BT_656_1120,
-	.reserved = { 0 },
 
 	V4L2_INIT_BT_TIMINGS(640, 3840, 480, 2160, 27000000, 594000000,
 			     V4L2_DV_BT_STD_CEA861 | V4L2_DV_BT_STD_DMT |

-- 
2.34.1

[PATCH 06/21] media: i2c: it6625: use unsigned int loop indices in table lookups

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

it6625_csi_format_idx(), it6625_csi_mbus_code_idx(), and
it6625_regdump_print() iterate with a plain-signed loop index against
an unsigned bound. Declare the index as unsigned int directly in the
for() statement instead.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 97780c3d2d8fd3567ef18b3d2fc46b4e1f0758d7..9077f66187c647f85e41e571ff0d52e5e2ffdfdf 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -375,9 +375,7 @@ static const struct it6625_format_info {
 
 static inline int it6625_csi_format_idx(u8 csi_format)
 {
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(it6625_formats); i++) {
+	for (unsigned int i = 0; i < ARRAY_SIZE(it6625_formats); i++) {
 		if (it6625_formats[i].csi_format == csi_format)
 			return i;
 	}
@@ -387,9 +385,7 @@ static inline int it6625_csi_format_idx(u8 csi_format)
 
 static inline int it6625_csi_mbus_code_idx(u32 mbus_fmt_code)
 {
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(it6625_formats); i++) {
+	for (unsigned int i = 0; i < ARRAY_SIZE(it6625_formats); i++) {
 		if (it6625_formats[i].mbus_fmt_code == mbus_fmt_code)
 			return i;
 	}
@@ -1878,11 +1874,9 @@ static int it6625_v4l2_init_controls(struct v4l2_subdev *sd)
 
 static void it6625_regdump_print(struct seq_file *s, const u8 *reg_buf)
 {
-	int i;
-
 	seq_puts(s, "     0x00 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 0x09 0x0A 0x0B 0x0C 0x0D 0x0E 0x0F\n");
 
-	for (i = 0; i < 256; i++) {
+	for (unsigned int i = 0; i < 256; i++) {
 		if (i % 16 == 0)
 			seq_printf(s, "[%02X] ", i & 0xF0);
 		seq_printf(s, "0x%02X ", reg_buf[i]);

-- 
2.34.1

[PATCH 07/21] media: i2c: it6625: drop redundant parentheses in status helpers

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

is_hdmi(), hdmi_5v_power_present(), no_signal(), and audio_present()
wrap their ternary condition and true-branch bitwise-AND in
parentheses that C's operator precedence already makes unnecessary:
'<' binds tighter than '&', and '&' binds tighter than '?:'. Drop them;
behavior is unchanged.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 9077f66187c647f85e41e571ff0d52e5e2ffdfdf..62fabf6169fc24364e484cf573b0fc1af174afbb 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -554,7 +554,7 @@ static inline bool is_hdmi(struct it6625 *it6625)
 	int val;
 
 	val = it6625_read_byte(it6625, REG_RX_STATUS);
-	return (val < 0) ? false : (val & B_RX_HDMI);
+	return val < 0 ? false : val & B_RX_HDMI;
 }
 
 static inline bool hdmi_5v_power_present(struct it6625 *it6625)
@@ -562,7 +562,7 @@ static inline bool hdmi_5v_power_present(struct it6625 *it6625)
 	int val;
 
 	val = it6625_read_byte(it6625, REG_RX_STATUS);
-	return (val < 0) ? false : (val & B_RX_5V);
+	return val < 0 ? false : val & B_RX_5V;
 }
 
 static inline bool no_signal(struct it6625 *it6625)
@@ -570,7 +570,7 @@ static inline bool no_signal(struct it6625 *it6625)
 	int val;
 
 	val = it6625_read_byte(it6625, REG_RX_STATUS);
-	return (val < 0) ? true : !(val & B_RX_STABLE);
+	return val < 0 ? true : !(val & B_RX_STABLE);
 }
 
 static inline bool audio_present(struct it6625 *it6625)
@@ -578,7 +578,7 @@ static inline bool audio_present(struct it6625 *it6625)
 	int val;
 
 	val = it6625_read_byte(it6625, REG_RX_STATUS);
-	return (val < 0) ? false : (val & B_RX_AUD_ON);
+	return val < 0 ? false : val & B_RX_AUD_ON;
 }
 
 static int get_audio_sampling_rate(struct it6625 *it6625)

-- 
2.34.1

[PATCH 08/21] media: i2c: it6625: make the audio sampling-rate table static const

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

s_fsid_map in get_audio_sampling_rate() is a fixed lookup table
rebuilt on the stack on every call; make it static const so it's
emitted once as read-only data instead. While touching this
declaration, move it ahead of the plain int locals per reverse
fir-tree ordering.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 62fabf6169fc24364e484cf573b0fc1af174afbb..9b1d9edca8f383f4f55918bda259a1b053d7f41b 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -583,9 +583,7 @@ static inline bool audio_present(struct it6625 *it6625)
 
 static int get_audio_sampling_rate(struct it6625 *it6625)
 {
-	int fs_id;
-	int i, freq = 0;
-	const struct fs_id_map {
+	static const struct fs_id_map {
 		u8 fs_id;
 		u32 freq;
 	} s_fsid_map[] = {
@@ -610,6 +608,8 @@ static int get_audio_sampling_rate(struct it6625 *it6625)
 		{ AUD1411K, 1411200 },
 		{ AUD1536K, 1536000 },
 	};
+	int fs_id;
+	int i, freq = 0;
 
 	if (no_signal(it6625) || !audio_present(it6625))
 		return 0;

-- 
2.34.1

[PATCH 09/21] media: i2c: it6625: tidy CEC buffer init and a continuation line

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

it6625_hpd_delayed_work(): wrap the container_of() assignment after
'=' instead of splitting the argument list mid-parenthesis.

it6625_cec_adap_enable(): assign the cmds[] buffer at declaration
instead of two separate statements, matching the pattern already used
elsewhere in this file.

it6625_cec_adap_log_addr(): add the missing spaces inside the cmds[]
initializer's braces.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 9b1d9edca8f383f4f55918bda259a1b053d7f41b..bc6b0a9a0bb005b30983a30f8ca685f885ab78c6 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -757,8 +757,8 @@ static void it6625_enable_hpd(struct it6625 *it6625)
 
 static void it6625_hpd_delayed_work(struct work_struct *work)
 {
-	struct it6625 *it6625 = container_of(work,
-			struct it6625, hpd_delayed_work.work);
+	struct it6625 *it6625 =
+		container_of(work, struct it6625, hpd_delayed_work.work);
 	int val = 0;
 
 	guard(mutex)(&it6625->it6625_lock);
@@ -980,10 +980,8 @@ static void it6625_initial_setup(struct it6625 *it6625)
 static int it6625_cec_adap_enable(struct cec_adapter *adap, bool enable)
 {
 	struct it6625 *it6625 = adap->priv;
-	u8 cmds[2];
+	u8 cmds[2] = { CMD_SET_CEC_ENABLE, enable ? 1 : 0 };
 
-	cmds[0] = CMD_SET_CEC_ENABLE;
-	cmds[1] = enable ? 1 : 0;
 	guard(mutex)(&it6625->it6625_lock);
 	it6625_write_command(it6625, cmds, sizeof(cmds));
 
@@ -1009,7 +1007,7 @@ static void it6625_cec_reset_la(struct it6625 *it6625, bool keep_enabled)
 static int it6625_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
 {
 	struct it6625 *it6625 = adap->priv;
-	u8 cmds[2] = {CMD_SET_CEC_LA, log_addr};
+	u8 cmds[2] = { CMD_SET_CEC_LA, log_addr };
 
 	dev_dbg(it6625->dev, "%s: la=%d", __func__, log_addr);
 

-- 
2.34.1

[PATCH 10/21] media: i2c: it6625: clean up it6625_wait_for_status()

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

Use USEC_PER_MSEC instead of a bare 1000 multiplier for the
read_poll_timeout() sleep/timeout arguments, and add the
linux/time64.h include it comes from. Drop the needless (int) cast on
rval, which is already declared int. Downgrade the unconditional
per-call status log from dev_info() to dev_dbg(), since it fires on
every call, not just failures.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index bc6b0a9a0bb005b30983a30f8ca685f885ab78c6..b580256d32f51465a620b639fc0134366cc6fcc1 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -16,6 +16,7 @@
 #include <linux/of_graph.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/time64.h>
 #include <linux/timer.h>
 #include <linux/v4l2-dv-timings.h>
 #include <linux/videodev2.h>
@@ -502,11 +503,11 @@ static int it6625_wait_for_status(struct it6625 *it6625, u8 reg, u8 val,
 	int timeout_round_ms = DIV_ROUND_UP(timeout_ms, sleep_ms) * sleep_ms;
 
 	status = read_poll_timeout(it6625_read_byte, rval, rval == val,
-				   sleep_ms * 1000,
-				   timeout_round_ms * 1000,
+				   sleep_ms * USEC_PER_MSEC,
+				   timeout_round_ms * USEC_PER_MSEC,
 				   false, it6625, reg);
 
-	dev_info(dev, "%s status = %d %d", __func__, status, (int)rval);
+	dev_dbg(dev, "%s status = %d %d", __func__, status, rval);
 	if (status < 0) {
 		dev_err(dev, "%s err status = %d", __func__, status);
 		return -ETIMEDOUT;

-- 
2.34.1

[PATCH 11/21] media: i2c: it6625: use unsigned int indices in EDID read/write

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

it6625_read_edid() and it6625_write_edid() iterate their block loop
with signed i/bank_ctrl locals holding only non-negative values; make
them unsigned int and keep err signed. Reorder the declarations to put
the pointer before the scalars while these functions are already being
touched.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index b580256d32f51465a620b639fc0134366cc6fcc1..328bd99f27cf84722caa184302cddeb9c3e667fb 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -660,8 +660,9 @@ static u64 it6625_get_pclk(struct it6625 *it6625)
 static int it6625_read_edid(struct it6625 *it6625, u8 *edid, int start_block,
 			    int num_blocks)
 {
-	int i, bank_ctrl, err = 0;
 	struct device *dev = it6625->dev;
+	unsigned int i, bank_ctrl;
+	int err = 0;
 
 	if (!edid) {
 		dev_err(dev, "edid buffer is NULL");
@@ -699,8 +700,9 @@ static int it6625_read_edid(struct it6625 *it6625, u8 *edid, int start_block,
 static int it6625_write_edid(struct it6625 *it6625, u8 *edid, int start_block,
 			     int num_blocks)
 {
-	int i, bank_ctrl, err = 0;
 	struct device *dev = it6625->dev;
+	unsigned int i, bank_ctrl;
+	int err = 0;
 
 	if (start_block < 0 || num_blocks <= 0 ||
 	    start_block > EDID_NUM_BLOCKS_MAX ||

-- 
2.34.1

[PATCH 12/21] media: i2c: it6625: use unaligned/units helpers to decode pixel clock

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

it6625_get_pclk() manually assembled a big-endian u32 from a 4-byte
buffer with a shift-and-OR sequence, and multiplied by a bare 1000.
Read the register range with sizeof(ck), decode it with
get_unaligned_be32(), and use the KHZ_PER_MHZ constant. Add the
linux/unaligned.h and linux/units.h includes these need. While
touching this declaration block, reorder it per reverse fir-tree.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 328bd99f27cf84722caa184302cddeb9c3e667fb..60c79a2277941621c7aa83189244b706a5505d66 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -18,6 +18,8 @@
 #include <linux/slab.h>
 #include <linux/time64.h>
 #include <linux/timer.h>
+#include <linux/unaligned.h>
+#include <linux/units.h>
 #include <linux/v4l2-dv-timings.h>
 #include <linux/videodev2.h>
 #include <linux/workqueue.h>
@@ -633,28 +635,22 @@ static int get_audio_sampling_rate(struct it6625 *it6625)
 
 static u64 it6625_get_pclk(struct it6625 *it6625)
 {
-	u32 pclk;
 	u8 ck[4];
+	u32 pclk;
 	int ret;
 
-	ret = it6625_read_bytes(it6625, REG_VID_PCLK, ck, 4);
+	ret = it6625_read_bytes(it6625, REG_VID_PCLK, ck, sizeof(ck));
 	if (ret < 0) {
 		dev_err(it6625->dev, "failed to read pixel clock");
 		return 0;
 	}
 
-	pclk = ck[0];
-	pclk <<= 8;
-	pclk |= ck[1];
-	pclk <<= 8;
-	pclk |= ck[2];
-	pclk <<= 8;
-	pclk |= ck[3];
+	pclk = get_unaligned_be32(ck);
 
 	v4l2_dbg(1, debug, &it6625->sd, "%s: pclk=%u (%08x)",
 		 __func__, pclk, pclk);
 
-	return (u64)pclk * 1000;
+	return (u64)pclk * KHZ_PER_MHZ;
 }
 
 static int it6625_read_edid(struct it6625 *it6625, u8 *edid, int start_block,

-- 
2.34.1

[PATCH 13/21] media: i2c: it6625: decode detected timings via typed register structs

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

it6625_get_detected_timings() manually assembled each 16-bit field from
raw byte-buffer offsets with a shift-and-add sequence. Define two local
structs of __be16 fields matching the contiguous REG_H_ACTIVE_1..
REG_V_ACTIVE_0 and REG_H_FP_1..REG_V_BP_0 register layouts, read
directly into them, and decode each field with be16_to_cpu(). Guard
each struct's size with static_assert() against the expected register
range width.

Every member is 2 bytes wide and naturally aligned, so the struct is
laid out with no padding -- this is safe because the struct is the I2C
read target itself, not a cast over a pre-existing raw buffer.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 60c79a2277941621c7aa83189244b706a5505d66..90b87dbf54fcfc7ad2a1245d4594beee24a193ca 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -769,10 +769,22 @@ static int it6625_get_detected_timings(struct it6625 *it6625,
 				       struct v4l2_dv_timings *timings)
 {
 	struct v4l2_bt_timings *bt = &timings->bt;
+	struct {
+		__be16 h_active;
+		__be16 v_active;
+	} active;
+	struct {
+		__be16 hfrontporch;
+		__be16 hsync;
+		__be16 hbackporch;
+		__be16 vfrontporch;
+		__be16 vsync;
+		__be16 vbackporch;
+	} porch;
 	int val;
-	unsigned int width, height;
-	u8 buffer[4];
-	u8 buffer2[12];
+
+	static_assert(sizeof(active) == 4);
+	static_assert(sizeof(porch) == 12);
 
 	if (no_signal(it6625)) {
 		dev_err(it6625->dev, "no signal detected");
@@ -792,24 +804,21 @@ static int it6625_get_detected_timings(struct it6625 *it6625,
 	bt->interlaced = val & B_INTERLACE ?
 			 V4L2_DV_INTERLACED : V4L2_DV_PROGRESSIVE;
 
-	if (it6625_read_bytes(it6625, REG_H_ACTIVE_1, buffer, 4) < 0)
+	if (it6625_read_bytes(it6625, REG_H_ACTIVE_1, (u8 *)&active, sizeof(active)) < 0)
 		return -EIO;
 
-	width = ((buffer[0] & 0xff) << 8) + buffer[1];
-	height = ((buffer[2] & 0xff) << 8) + buffer[3];
-
-	bt->width = width;
-	bt->height = height;
+	bt->width = be16_to_cpu(active.h_active);
+	bt->height = be16_to_cpu(active.v_active);
 
-	if (it6625_read_bytes(it6625, REG_H_FP_1, buffer2, 12) < 0)
+	if (it6625_read_bytes(it6625, REG_H_FP_1, (u8 *)&porch, sizeof(porch)) < 0)
 		return -EIO;
 
-	bt->hfrontporch = ((buffer2[0] & 0xff) << 8) + buffer2[1];
-	bt->hsync = ((buffer2[2] & 0xff) << 8) + buffer2[3];
-	bt->hbackporch = ((buffer2[4] & 0xff) << 8) + buffer2[5];
-	bt->vfrontporch = ((buffer2[6] & 0xff) << 8) + buffer2[7];
-	bt->vsync = ((buffer2[8] & 0xff) << 8) + buffer2[9];
-	bt->vbackporch = ((buffer2[10] & 0xff) << 8) + buffer2[11];
+	bt->hfrontporch = be16_to_cpu(porch.hfrontporch);
+	bt->hsync = be16_to_cpu(porch.hsync);
+	bt->hbackporch = be16_to_cpu(porch.hbackporch);
+	bt->vfrontporch = be16_to_cpu(porch.vfrontporch);
+	bt->vsync = be16_to_cpu(porch.vsync);
+	bt->vbackporch = be16_to_cpu(porch.vbackporch);
 
 	bt->pixelclock = it6625_get_pclk(it6625);
 	if (bt->interlaced == V4L2_DV_INTERLACED) {

-- 
2.34.1

[PATCH 14/21] media: i2c: it6625: fix link-frequency reporting for one-/two-trio C-PHY

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

it6625_v4l2_init_controls() selected the 2.5 Gsym/s link-frequency
menu entry for any C-PHY configuration, but it6625_get_timings_cap()
only raises the DV-timings pixel-clock ceiling for three-trio C-PHY,
matching the actually-tested hardware capability. A one-/two-trio
C-PHY device was reporting an inflated V4L2_CID_LINK_FREQ.

Split the shared two-entry array into two single-entry arrays and
select between them with the same condition it6625_get_timings_cap()
uses (bus_type == V4L2_MBUS_CSI2_CPHY && csi_lanes == 3; C-PHY is only
ever set for IT6626, so this is equivalent to that function's chip-type
check as well). Name them for what they actually cover rather than for
a PHY type alone, since one-/two-trio C-PHY uses the low-rate array
too, not a "D-PHY" array.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 90b87dbf54fcfc7ad2a1245d4594beee24a193ca..882eda8b836d655585ba45b0a33d1c068bb3a8ee 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -317,11 +317,18 @@ struct it6625 {
 };
 
 /*
- * Index 0: D-PHY (4-lane). Index 1: C-PHY (3-trio) -- the confirmed
- * hardware max C-PHY capability, tested single-port/three-trio.
+ * Reported link frequency for every topology except the reference
+ * exception below: D-PHY (any lane count) and one-/two-trio C-PHY.
  */
-static const s64 it6625_link_freq[] = {
+static const s64 it6625_link_freq_default[] = {
 	445500000,
+};
+
+/*
+ * IT6626 C-PHY, three trios: the confirmed hardware max C-PHY
+ * capability, tested single-port/three-trio.
+ */
+static const s64 it6626_cphy_3trio_link_freq[] = {
 	2500000000LL,
 };
 
@@ -1850,6 +1857,8 @@ static int it6625_v4l2_init_controls(struct v4l2_subdev *sd)
 {
 	struct it6625 *it6625 = sd_to_6625(sd);
 	struct v4l2_ctrl_handler *hdl = &it6625->hdl;
+	bool cphy_3trio = it6625->bus_type == V4L2_MBUS_CSI2_CPHY &&
+			   it6625->csi_lanes == 3;
 
 	v4l2_ctrl_handler_init(hdl, 4);
 	it6625->ctrl_5v_detect =
@@ -1863,10 +1872,9 @@ static int it6625_v4l2_init_controls(struct v4l2_subdev *sd)
 	it6625->ctrl_audio_present =
 		v4l2_ctrl_new_custom(hdl, &it6625_ctrl_audio_present, NULL);
 	it6625->ctrl_link_freq =
-		v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
-				       ARRAY_SIZE(it6625_link_freq) - 1,
-				       it6625->bus_type == V4L2_MBUS_CSI2_CPHY ? 1 : 0,
-				       it6625_link_freq);
+		v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ, 0, 0,
+				       cphy_3trio ? it6626_cphy_3trio_link_freq :
+						    it6625_link_freq_default);
 	if (hdl->error) {
 		v4l2_err(sd, "Failed to initialize controls");
 		v4l2_ctrl_handler_free(hdl);

-- 
2.34.1

[PATCH 15/21] media: i2c: it6625: use early returns in it6625_update_timings_if_changed()

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

Convert the if/else if/else chain that only ever assigns a single
ret value to early returns. guard(mutex)(...) is scoped to the whole
function body, so an early return still unlocks correctly.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 882eda8b836d655585ba45b0a33d1c068bb3a8ee..db962bc100cad9f4d71fa4156b10ff5b4c952a7c 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -1431,20 +1431,17 @@ static int
 it6625_update_timings_if_changed(struct it6625 *it6625,
 				 const struct v4l2_dv_timings *timings)
 {
-	int ret;
-
 	guard(mutex)(&it6625->it6625_lock);
-	if (v4l2_match_dv_timings(&it6625->timings, timings, 0, false)) {
-		ret = 0;
-	} else if (!v4l2_valid_dv_timings(timings, it6625_get_timings_cap(it6625),
-					  NULL, NULL)) {
-		ret = -ERANGE;
-	} else {
-		it6625->timings = *timings;
-		ret = 1;
-	}
 
-	return ret;
+	if (v4l2_match_dv_timings(&it6625->timings, timings, 0, false))
+		return 0;
+
+	if (!v4l2_valid_dv_timings(timings, it6625_get_timings_cap(it6625), NULL, NULL))
+		return -ERANGE;
+
+	it6625->timings = *timings;
+
+	return 1;
 }
 
 static int it6625_enum_dv_timings(struct v4l2_subdev *sd,

-- 
2.34.1

[PATCH 16/21] media: i2c: it6625: drop the private CSI-format name table

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

it6625_csi_format_name(), used only by it6625_log_status(), maintains
a driver-local name table for a value V4L2 already exposes as a
media-bus format code. No in-kernel helper converts MEDIA_BUS_FMT_*
codes to printable names, so report the raw media-bus code as %#x
instead. it6625_log_status() now snapshots it6625->mbus_fmt_code
(the value actually used elsewhere as the driver's representation of
the current format) under it6625_lock instead of the separate
csi_format field.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index db962bc100cad9f4d71fa4156b10ff5b4c952a7c..4a54de2eb42bba7a6efc84c0b43cad4d2465d3a7 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -1343,26 +1343,12 @@ static void it6625_polling_work(struct work_struct *work)
 	it6625_interrupt_handler(it6625);
 }
 
-static const char *it6625_csi_format_name(u8 csi_format)
-{
-	switch (csi_format) {
-	case CSI_YUV422_8b:
-		return "YUV422 8bit";
-	case CSI_RGB888:
-		return "RGB888 8bit";
-	case CSI_YUV444_8b:
-		return "YUV444 8bit";
-	default:
-		return "unknown";
-	}
-}
-
 static int it6625_log_status(struct v4l2_subdev *sd)
 {
 	struct it6625 *it6625 = sd_to_6625(sd);
 	struct v4l2_dv_timings timings, configured_timings;
 	struct v4l2_bt_timings bt;
-	u8 csi_format;
+	u32 mbus_fmt_code;
 
 	if (it6625_get_detected_timings(it6625, &timings))
 		v4l2_info(sd, "No video detected");
@@ -1376,13 +1362,11 @@ static int it6625_log_status(struct v4l2_subdev *sd)
 
 	/* snapshot together so the reported pair was actually configured together */
 	scoped_guard(mutex, &it6625->it6625_lock) {
-		csi_format = it6625->csi_format;
+		mbus_fmt_code = it6625->mbus_fmt_code;
 		bt = it6625->timings.bt;
 	}
 
-	v4l2_info(sd, "CSI format: %s @ %uHz",
-		  it6625_csi_format_name(csi_format),
-		  fps_from_bt_timings(&bt));
+	v4l2_info(sd, "CSI format: %#x @ %uHz", mbus_fmt_code, fps_from_bt_timings(&bt));
 
 	it6625_show_avi_infoframe(it6625);
 

-- 
2.34.1

[PATCH 17/21] media: i2c: it6625: require a DT endpoint and simplify endpoint parsing

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

it6625_init_data() preset csi_lanes/port_num/bus_type before DT
parsing ran, and it6625_parse_endpoint() fell back to those hardcoded
defaults whenever no CSI-2 endpoint node was found instead of failing.
The binding requires port@0, so a missing endpoint should surface as a
probe error, not silently apply a hardcoded D-PHY/4-lane
configuration.

Drop the presets from it6625_init_data() -- these values must come
only from DT -- and delete the no-endpoint fallback entirely rather
than reshaping it. This is safe: of_fwnode_handle(NULL) returns NULL,
and v4l2_fwnode_endpoint_alloc_parse() -> __v4l2_fwnode_endpoint_parse()
already returns -EPROBE_DEFER for a NULL fwnode before touching
anything else, which is a strictly better result for the no-endpoint
case than a driver-local -EINVAL.

While here, consolidate the three -EINVAL return sites in
it6625_parse_endpoint() through a single error-path label instead of
repeating v4l2_fwnode_endpoint_free() at each one.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 4a54de2eb42bba7a6efc84c0b43cad4d2465d3a7..5fa8441f073691425c64fbd2b4e537129a43a2b7 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -2050,9 +2050,6 @@ static void it6625_init_data(struct it6625 *it6625)
 	static struct v4l2_dv_timings default_timing =
 			V4L2_DV_BT_CEA_1920X1080P60;
 
-	it6625->csi_lanes = 4;
-	it6625->port_num = 1;
-	it6625->bus_type = V4L2_MBUS_CSI2_DPHY;
 	it6625->csi_format = it6625_formats[0].csi_format;
 	it6625->mbus_fmt_code = it6625_formats[0].mbus_fmt_code;
 	it6625->timings = default_timing;
@@ -2098,13 +2095,6 @@ static int it6625_parse_endpoint(struct it6625 *it6625)
 			of_node_put(port_ep);
 	}
 
-	if (!ep) {
-		it6625->port_num = 1;
-		dev_dbg(dev, "no CSI-2 endpoint node found, using default %u CSI lanes",
-			it6625->csi_lanes);
-		return 0;
-	}
-
 	ret = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(ep), &endpoint);
 	of_node_put(ep);
 	if (ret) {
@@ -2116,15 +2106,15 @@ static int it6625_parse_endpoint(struct it6625 *it6625)
 	    endpoint.bus_type != V4L2_MBUS_CSI2_CPHY) {
 		dev_err(dev, "unsupported bus type %d, expected CSI-2 D-PHY or C-PHY",
 			endpoint.bus_type);
-		v4l2_fwnode_endpoint_free(&endpoint);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_free_endpoint;
 	}
 
 	if (endpoint.bus_type == V4L2_MBUS_CSI2_CPHY &&
 	    it6625->chip_type != IT6626_CHIP) {
 		dev_err(dev, "IT6625 does not support C-PHY, only IT6626 does");
-		v4l2_fwnode_endpoint_free(&endpoint);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_free_endpoint;
 	}
 
 	max_lanes = (endpoint.bus_type == V4L2_MBUS_CSI2_CPHY) ? 3 : 4;
@@ -2134,15 +2124,17 @@ static int it6625_parse_endpoint(struct it6625 *it6625)
 		dev_err(dev,
 			"invalid number of CSI data lanes: %u (max %u for this bus type)",
 			endpoint.bus.mipi_csi2.num_data_lanes, max_lanes);
-		v4l2_fwnode_endpoint_free(&endpoint);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_free_endpoint;
 	}
 
 	it6625->csi_lanes = endpoint.bus.mipi_csi2.num_data_lanes;
 	it6625->bus_type = endpoint.bus_type;
+
+out_free_endpoint:
 	v4l2_fwnode_endpoint_free(&endpoint);
 
-	return 0;
+	return ret;
 }
 
 static int it6625_parse_dt(struct it6625 *it6625)

-- 
2.34.1

[PATCH 18/21] media: i2c: it6625: finish reverse fir-tree declaration order

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

Reorder the remaining declarations that don't follow reverse fir-tree
order (struct/pointer types before plain scalars): the five register
accessor helpers it6625_read_byte()/write_byte()/set_bits()/
read_bytes()/write_bytes(), where a plain int was declared ahead of
the struct device *dev pointer, and it6625_set_fmt()/it6625_s_edid(),
where an initialized wider-type local was declared after a plain int.
Swept the rest of the file for the same pattern; no other functions
need it.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 5fa8441f073691425c64fbd2b4e537129a43a2b7..40cd413e0ed49c118421ce3dec77bb2df08cf042 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -433,9 +433,9 @@ static int it6625_regmap_i2c_init(struct i2c_client *client,
 
 static int it6625_read_byte(struct it6625 *it6625, u8 reg)
 {
+	struct device *dev = it6625->dev;
 	unsigned int val;
 	int err;
-	struct device *dev = it6625->dev;
 
 	err = regmap_read(it6625->it6625_regmap, reg, &val);
 	if (err < 0) {
@@ -448,8 +448,8 @@ static int it6625_read_byte(struct it6625 *it6625, u8 reg)
 
 static int it6625_write_byte(struct it6625 *it6625, u8 reg, u8 val)
 {
-	int err;
 	struct device *dev = it6625->dev;
+	int err;
 
 	err = regmap_write(it6625->it6625_regmap, reg, val);
 	if (err < 0) {
@@ -462,8 +462,8 @@ static int it6625_write_byte(struct it6625 *it6625, u8 reg, u8 val)
 
 static int it6625_set_bits(struct it6625 *it6625, u8 reg, u8 mask, u8 val)
 {
-	int err;
 	struct device *dev = it6625->dev;
+	int err;
 
 	err = regmap_update_bits(it6625->it6625_regmap, reg, mask, val);
 	if (err < 0) {
@@ -476,8 +476,8 @@ static int it6625_set_bits(struct it6625 *it6625, u8 reg, u8 mask, u8 val)
 
 static int it6625_read_bytes(struct it6625 *it6625, u8 reg, u8 *buf, int len)
 {
-	int err;
 	struct device *dev = it6625->dev;
+	int err;
 
 	err = regmap_bulk_read(it6625->it6625_regmap, reg, buf, len);
 	if (err < 0) {
@@ -490,8 +490,8 @@ static int it6625_read_bytes(struct it6625 *it6625, u8 reg, u8 *buf, int len)
 
 static int it6625_write_bytes(struct it6625 *it6625, u8 reg, u8 *buf, int len)
 {
-	int err;
 	struct device *dev = it6625->dev;
+	int err;
 
 	err = regmap_bulk_write(it6625->it6625_regmap, reg, buf, len);
 	if (err < 0) {
@@ -1624,8 +1624,8 @@ static int it6625_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_format *format)
 {
 	struct it6625 *it6625 = sd_to_6625(sd);
-	int ret;
 	u32 mbus_fmt_code = format->format.code;
+	int ret;
 
 	ret = it6625_get_fmt(sd, sd_state, format);
 	format->format.code = mbus_fmt_code;
@@ -1705,8 +1705,8 @@ static int it6625_s_edid(struct v4l2_subdev *sd,
 			 struct v4l2_subdev_edid *edid)
 {
 	struct it6625 *it6625 = sd_to_6625(sd);
-	int err;
 	u16 parent_pa = CEC_PHYS_ADDR_INVALID;
+	int err;
 
 	if (edid->pad != 0) {
 		v4l2_err(sd, "invalid pad %d", edid->pad);

-- 
2.34.1

[PATCH 19/21] media: i2c: it6625: fold subdev initialization into probe

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

it6625_init_v4l2_subdev() split subdev/control-handler/media-entity
initialization out of probe(), which made error handling harder to
follow across the two functions and forced probe() to normalize every
init_v4l2_subdev() failure to -ENOMEM regardless of the real error
(e.g. a media_entity_pads_init() failure was reported to the caller as
-ENOMEM instead of its actual code).

Inline it into it6625_probe() so initialization and its unwind path
are visible together, and propagate the real error from
it6625_v4l2_init_controls() instead of replacing it. Cleanup behavior
on each failure path is unchanged: it6625_v4l2_init_controls() already
frees the control handler internally before returning an error, and a
media_entity_pads_init() failure still frees it explicitly before
unwinding, so neither path double-frees it through the later
err_clean_hdl label.

State finalization (v4l2_subdev_init_finalize()/cleanup()) is
deliberately left for a follow-up change, to keep this a pure
restructuring and keep the locking-model transition atomic on its own.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 46 +++++++++++++++++-----------------------------
 1 file changed, 17 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 40cd413e0ed49c118421ce3dec77bb2df08cf042..ec6aaa878471ff210264a35e4aa66dfb1e63ed3b 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -2142,33 +2142,6 @@ static int it6625_parse_dt(struct it6625 *it6625)
 	return it6625_parse_endpoint(it6625);
 }
 
-static int it6625_init_v4l2_subdev(struct it6625 *it6625)
-{
-	struct v4l2_subdev *sd = &it6625->sd;
-	int err;
-
-	sd->dev = it6625->dev;
-
-	v4l2_i2c_subdev_init(sd, it6625->i2c_client, &it6625_ops);
-	sd->internal_ops = &it6625_internal_ops;
-	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
-	if (it6625_v4l2_init_controls(sd)) {
-		dev_err(it6625->dev, "Failed to initialize v4l2 controls");
-		return -ENOMEM;
-	}
-
-	it6625->pad.flags = MEDIA_PAD_FL_SOURCE;
-	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
-	err = media_entity_pads_init(&sd->entity, 1, &it6625->pad);
-	if (err < 0) {
-		dev_err(it6625->dev, "%s %d err=%d", __func__, __LINE__, err);
-		v4l2_ctrl_handler_free(sd->ctrl_handler);
-		return err;
-	}
-
-	return 0;
-}
-
 static int it6625_check_device(struct it6625 *it6625)
 {
 	static const u8 chip_ids[][2] = {
@@ -2254,9 +2227,24 @@ static int it6625_probe(struct i2c_client *client)
 	}
 
 	sd = &it6625->sd;
-	err = it6625_init_v4l2_subdev(it6625);
-	if (err)
+	v4l2_i2c_subdev_init(sd, it6625->i2c_client, &it6625_ops);
+	sd->internal_ops = &it6625_internal_ops;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+
+	err = it6625_v4l2_init_controls(sd);
+	if (err) {
+		dev_err(it6625->dev, "failed to initialize v4l2 controls: %d", err);
 		goto err_clean_work_queues;
+	}
+
+	it6625->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	err = media_entity_pads_init(&sd->entity, 1, &it6625->pad);
+	if (err < 0) {
+		dev_err(it6625->dev, "%s %d err=%d", __func__, __LINE__, err);
+		v4l2_ctrl_handler_free(sd->ctrl_handler);
+		goto err_clean_work_queues;
+	}
 
 	err = v4l2_ctrl_handler_setup(sd->ctrl_handler);
 	if (err)

-- 
2.34.1

[PATCH 20/21] media: i2c: it6625: use centrally managed active state

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

Adopt the subdev active-state model instead of a private driver mutex
and driver-local format fields:

 - Share it6625_lock as both sd->state_lock and sd->ctrl_handler->lock
   (set hdl->lock right after v4l2_ctrl_handler_init(), which resets
   it, and before any control is created), matching the documented
   model in Documentation/driver-api/media/v4l2-subdev.rst. Call
   v4l2_subdev_init_finalize() after media_entity_pads_init() and
   before any code that touches active state, always before
   v4l2_async_register_subdev().

 - Remove csi_format/mbus_fmt_code from struct it6625 and store the
   complete format -- code, colorspace, and width/height/field derived
   from the configured DV timings -- in the pad format of each
   v4l2_subdev_state instead. it6625_set_fmt() now operates directly
   on the state the core hands it (already locked, and for ACTIVE
   already sd->active_state) instead of taking it6625_lock itself.
   it6625_get_fmt() is deleted; .get_fmt is now the core's
   v4l2_subdev_get_fmt(). it6625_init_state() seeds a state from the
   current active format when one exists (later TRY-state allocation)
   or from driver defaults when it doesn't (the very first, pre-
   assignment call that becomes the active state itself).

 - Keep it6625->timings as driver-private DV-timings state, but project
   its width/height/field onto the active pad format every time it
   changes (it6625_update_timings_if_changed(), it6625_clear_timings())
   so VIDIOC_SUBDEV_G_FMT stays consistent with
   VIDIOC_SUBDEV_G_DV_TIMINGS. it6625_initial_setup() and
   it6625_log_status() now read the media-bus code from the locked
   active format instead of the removed fields.

 - Make ACTIVE .set_fmt() transactional: return -EBUSY while streaming,
   and commit the new code/colorspace to active state only after the
   MIPI register writes that implement it actually succeed. This
   requires it6625_enable_stream_locked(), it6625_enable_stream(), and
   it6625_set_mipi_config_locked() to return int and propagate the
   first register/config-update failure; all existing callers are
   updated.

Every it6625_lock acquisition site was re-audited against the core's
actual locking contract (which ioctls the core state-locks, what
v4l2_ctrl_handler_init()/_free() and v4l2_subdev_cleanup() actually
touch) to confirm none of the driver's own lock-taking helpers are
reachable from a path where the core already holds it6625_lock, and
that init/teardown ordering keeps hdl->lock valid whenever
v4l2_ctrl_handler_free() runs. The full trace is recorded in
it6625-follow-up-patch/patch19-lock-audit.md (not part of this commit)
for review -- it is static analysis only, since no IT6625/IT6626
hardware is available here to exercise this with lockdep enabled.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 220 ++++++++++++++++++++++++++++-----------------
 1 file changed, 138 insertions(+), 82 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index ec6aaa878471ff210264a35e4aa66dfb1e63ed3b..550eb95c6f74a81fc11cc055b4590159ac50e969 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -255,7 +255,15 @@ struct it6625 {
 	struct regmap *it6625_regmap;
 	enum it6625_chip_type chip_type;
 
-	/* protects concurrent access to the chip's registers and state */
+	/*
+	 * Protects concurrent access to the chip's registers and state.
+	 * Also shared as sd.state_lock and hdl.lock (see
+	 * Documentation/driver-api/media/v4l2-subdev.rst), so the V4L2
+	 * core already holds it across .get_fmt/.set_fmt/.enable_streams/
+	 * .disable_streams and control updates -- callers reached only
+	 * through those paths must use the *_locked() helpers instead of
+	 * taking it again.
+	 */
 	struct mutex it6625_lock;
 	/* serializes the complete VIDIOC_S_EDID sequence against itself */
 	struct mutex edid_lock;
@@ -291,8 +299,6 @@ struct it6625 {
 	u8 csi_lanes;
 	u8 port_num;
 	enum v4l2_mbus_type bus_type;
-	u8 csi_format;
-	u32 mbus_fmt_code;
 	/* number of EDID blocks currently loaded, protected by edid_lock */
 	u8 edid_blocks;
 
@@ -911,10 +917,11 @@ static int it6625_v4l2_sd_ctrl_update(struct v4l2_subdev *sd)
 	return it6625_s_ctrl_audio_present(sd);
 }
 
-static void it6625_enable_stream_locked(struct it6625 *it6625, bool enable)
+static int it6625_enable_stream_locked(struct it6625 *it6625, bool enable)
 {
 	struct v4l2_subdev *sd = &it6625->sd;
 	int val;
+	int err;
 
 	lockdep_assert_held(&it6625->it6625_lock);
 
@@ -922,27 +929,34 @@ static void it6625_enable_stream_locked(struct it6625 *it6625, bool enable)
 		 __func__, enable ? "en" : "dis");
 
 	val = enable ? B_MIPI_OUTPUT : 0;
-	it6625_set_bits(it6625, REG_MIPI_CONTROL, B_MIPI_OUTPUT, val);
-	it6625_update_config(it6625);
+	err = it6625_set_bits(it6625, REG_MIPI_CONTROL, B_MIPI_OUTPUT, val);
+	if (err < 0)
+		return err;
+
+	return it6625_update_config(it6625);
 }
 
-static void it6625_enable_stream(struct it6625 *it6625, bool enable)
+static int it6625_enable_stream(struct it6625 *it6625, bool enable)
 {
 	guard(mutex)(&it6625->it6625_lock);
-	it6625_enable_stream_locked(it6625, enable);
+	return it6625_enable_stream_locked(it6625, enable);
 }
 
-static void it6625_set_mipi_config_locked(struct it6625 *it6625, u32 cfg_val)
+static int it6625_set_mipi_config_locked(struct it6625 *it6625, u32 cfg_val)
 {
 	u8 mipi_data_type;
+	int err;
 
 	lockdep_assert_held(&it6625->it6625_lock);
 
 	dev_dbg(it6625->dev, "mipi_data_type = 0x%x", cfg_val);
 
 	mipi_data_type = cfg_val & 0xFF;
-	it6625_write_byte(it6625, REG_MIPI_DATA_TYPE, mipi_data_type);
-	it6625_update_config(it6625);
+	err = it6625_write_byte(it6625, REG_MIPI_DATA_TYPE, mipi_data_type);
+	if (err < 0)
+		return err;
+
+	return it6625_update_config(it6625);
 }
 
 static inline unsigned int fps_from_bt_timings(const struct v4l2_bt_timings *t)
@@ -957,10 +971,18 @@ static inline unsigned int fps_from_bt_timings(const struct v4l2_bt_timings *t)
 
 static void it6625_initial_setup(struct it6625 *it6625)
 {
+	struct v4l2_subdev *sd = &it6625->sd;
+	struct v4l2_mbus_framefmt *fmt;
+	int idx;
 	int val = 0;
 
 	guard(mutex)(&it6625->it6625_lock);
 
+	fmt = v4l2_subdev_state_get_format(v4l2_subdev_get_locked_active_state(sd), 0);
+	idx = it6625_csi_mbus_code_idx(fmt->code);
+	if (idx < 0)
+		idx = 0;
+
 	/*
 	 * REG_MIPI_CFG[0:2] lane count field: 1 lane -> 0, 2 lanes -> 1,
 	 * 3 lanes (C-PHY only) -> 3, 4 lanes (D-PHY only) -> 3.
@@ -984,7 +1006,7 @@ static void it6625_initial_setup(struct it6625 *it6625)
 		val |= FIELD_PREP(B_MIPI_SPLIT, 1);
 
 	it6625_write_byte(it6625, REG_MIPI_CFG, val);
-	it6625_write_byte(it6625, REG_MIPI_DATA_TYPE, it6625->csi_format);
+	it6625_write_byte(it6625, REG_MIPI_DATA_TYPE, it6625_formats[idx].csi_format);
 	it6625_write_byte(it6625, REG_MIPI_CONTROL, 0x00);
 	it6625_write_byte(it6625, REG_RX_CFG, 0x00);
 
@@ -1140,10 +1162,29 @@ static void it6625_get_timings(struct it6625 *it6625,
 	*timings = it6625->timings;
 }
 
+/*
+ * Project a DV-timings struct's width/height/field onto an active pad
+ * format. Caller must hold it6625_lock (== the active state's lock).
+ */
+static void it6625_fill_timings_format(const struct v4l2_dv_timings *timings,
+				       struct v4l2_mbus_framefmt *fmt)
+{
+	fmt->width = timings->bt.width;
+	fmt->height = timings->bt.height;
+	fmt->field = timings->bt.interlaced == V4L2_DV_INTERLACED ?
+		     V4L2_FIELD_INTERLACED : V4L2_FIELD_NONE;
+}
+
 static void it6625_clear_timings(struct it6625 *it6625)
 {
+	struct v4l2_subdev *sd = &it6625->sd;
+	struct v4l2_mbus_framefmt *fmt;
+
 	guard(mutex)(&it6625->it6625_lock);
 	memset(&it6625->timings, 0, sizeof(it6625->timings));
+
+	fmt = v4l2_subdev_state_get_format(v4l2_subdev_get_locked_active_state(sd), 0);
+	it6625_fill_timings_format(&it6625->timings, fmt);
 }
 
 static void it6625_irq_hdmi_5v_change(struct it6625 *it6625)
@@ -1362,7 +1403,10 @@ static int it6625_log_status(struct v4l2_subdev *sd)
 
 	/* snapshot together so the reported pair was actually configured together */
 	scoped_guard(mutex, &it6625->it6625_lock) {
-		mbus_fmt_code = it6625->mbus_fmt_code;
+		struct v4l2_mbus_framefmt *fmt =
+			v4l2_subdev_state_get_format(v4l2_subdev_get_locked_active_state(sd), 0);
+
+		mbus_fmt_code = fmt->code;
 		bt = it6625->timings.bt;
 	}
 
@@ -1415,6 +1459,9 @@ static int
 it6625_update_timings_if_changed(struct it6625 *it6625,
 				 const struct v4l2_dv_timings *timings)
 {
+	struct v4l2_subdev *sd = &it6625->sd;
+	struct v4l2_mbus_framefmt *fmt;
+
 	guard(mutex)(&it6625->it6625_lock);
 
 	if (v4l2_match_dv_timings(&it6625->timings, timings, 0, false))
@@ -1425,6 +1472,9 @@ it6625_update_timings_if_changed(struct it6625 *it6625,
 
 	it6625->timings = *timings;
 
+	fmt = v4l2_subdev_state_get_format(v4l2_subdev_get_locked_active_state(sd), 0);
+	it6625_fill_timings_format(&it6625->timings, fmt);
+
 	return 1;
 }
 
@@ -1457,8 +1507,7 @@ static int it6625_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct it6625 *it6625 = sd_to_6625(sd);
 
-	it6625_enable_stream(it6625, enable);
-	return 0;
+	return it6625_enable_stream(it6625, enable);
 }
 
 static int it6625_enum_mbus_code(struct v4l2_subdev *sd,
@@ -1586,84 +1635,56 @@ static inline u32 format_to_colorspace(u8 csi_format)
 	}
 }
 
-static int it6625_get_fmt(struct v4l2_subdev *sd,
-			  struct v4l2_subdev_state *sd_state,
-			  struct v4l2_subdev_format *format)
-{
-	struct it6625 *it6625 = sd_to_6625(sd);
-	struct v4l2_dv_timings timings;
-
-	if (format->pad != 0)
-		return -EINVAL;
-
-	it6625_get_timings(it6625, &timings);
-	format->format.width = timings.bt.width;
-	format->format.height = timings.bt.height;
-	format->format.field = timings.bt.interlaced == V4L2_DV_INTERLACED ?
-			       V4L2_FIELD_INTERLACED : V4L2_FIELD_NONE;
-
-	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		struct v4l2_mbus_framefmt *fmt;
-
-		fmt = v4l2_subdev_state_get_format(sd_state, format->pad);
-		format->format.code = fmt->code;
-		format->format.colorspace = fmt->colorspace;
-	} else {
-		scoped_guard(mutex, &it6625->it6625_lock) {
-			format->format.colorspace =
-				format_to_colorspace(it6625->csi_format);
-			format->format.code = it6625->mbus_fmt_code;
-		}
-	}
-
-	return 0;
-}
-
 static int it6625_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *format)
 {
 	struct it6625 *it6625 = sd_to_6625(sd);
-	u32 mbus_fmt_code = format->format.code;
+	struct v4l2_mbus_framefmt *fmt;
+	u32 colorspace;
+	int idx;
 	int ret;
 
-	ret = it6625_get_fmt(sd, sd_state, format);
-	format->format.code = mbus_fmt_code;
-
-	if (ret)
-		return ret;
-
-	ret = it6625_csi_mbus_code_idx(mbus_fmt_code);
+	if (format->pad != 0)
+		return -EINVAL;
 
-	if (ret < 0) {
+	idx = it6625_csi_mbus_code_idx(format->format.code);
+	if (idx < 0) {
 		v4l2_dbg(1, debug, sd,
 			 "%s: unsupported format code 0x%x, falling back to default",
-			 __func__, mbus_fmt_code);
-		ret = 0;
-		mbus_fmt_code = it6625_formats[ret].mbus_fmt_code;
-		format->format.code = mbus_fmt_code;
+			 __func__, format->format.code);
+		idx = 0;
 	}
 
-	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		struct v4l2_mbus_framefmt *fmt;
+	colorspace = format_to_colorspace(it6625_formats[idx].csi_format);
 
-		fmt = v4l2_subdev_state_get_format(sd_state, format->pad);
-		fmt->code = format->format.code;
-		fmt->colorspace = format_to_colorspace(it6625_formats[ret].csi_format);
-		format->format.colorspace = fmt->colorspace;
+	/* fmt already carries this state's width/height/field; leave them alone */
+	fmt = v4l2_subdev_state_get_format(sd_state, format->pad);
+
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+		fmt->code = it6625_formats[idx].mbus_fmt_code;
+		fmt->colorspace = colorspace;
+		format->format = *fmt;
 		v4l2_dbg(1, debug, sd, "%s: try format code = 0x%x",
 			 __func__, format->format.code);
 		return 0;
 	}
 
-	scoped_guard(mutex, &it6625->it6625_lock) {
-		it6625->csi_format = it6625_formats[ret].csi_format;
-		it6625->mbus_fmt_code = format->format.code;
-		it6625_enable_stream_locked(it6625, false);
-		it6625_set_mipi_config_locked(it6625, it6625->csi_format);
-	}
+	if (v4l2_subdev_is_streaming(sd))
+		return -EBUSY;
 
-	format->format.colorspace = format_to_colorspace(it6625_formats[ret].csi_format);
+	ret = it6625_enable_stream_locked(it6625, false);
+	if (ret)
+		return ret;
+
+	ret = it6625_set_mipi_config_locked(it6625, it6625_formats[idx].csi_format);
+	if (ret)
+		return ret;
+
+	/* commit to active state only after hardware programming succeeded */
+	fmt->code = it6625_formats[idx].mbus_fmt_code;
+	fmt->colorspace = colorspace;
+	format->format = *fmt;
 
 	return 0;
 }
@@ -1780,7 +1801,7 @@ static const struct v4l2_subdev_video_ops it6625_video_ops = {
 static const struct v4l2_subdev_pad_ops it6625_pad_ops = {
 	.enum_mbus_code = it6625_enum_mbus_code,
 	.set_fmt = it6625_set_fmt,
-	.get_fmt = it6625_get_fmt,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.get_edid = it6625_g_edid,
 	.set_edid = it6625_s_edid,
 	.enum_dv_timings = it6625_enum_dv_timings,
@@ -1800,8 +1821,26 @@ static const struct v4l2_subdev_ops it6625_ops = {
 static int it6625_init_state(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_state *sd_state)
 {
+	struct it6625 *it6625 = sd_to_6625(sd);
+	struct v4l2_subdev_state *active = v4l2_subdev_get_locked_active_state(sd);
 	struct v4l2_mbus_framefmt *fmt = v4l2_subdev_state_get_format(sd_state, 0);
 
+	/*
+	 * The very first call initializes what becomes sd->active_state
+	 * itself, before it's assigned -- active is NULL then, and this
+	 * state gets the driver's own defaults. Every later call (opening
+	 * a new file handle) initializes a fresh TRY state while the
+	 * active state already exists and is locked by the same mutex
+	 * (state->lock is assigned before init_state() runs), so seed it
+	 * from the current active format instead of reverting to boot
+	 * defaults.
+	 */
+	if (active) {
+		*fmt = *v4l2_subdev_state_get_format(active, 0);
+		return 0;
+	}
+
+	it6625_fill_timings_format(&it6625->timings, fmt);
 	fmt->code = it6625_formats[0].mbus_fmt_code;
 	fmt->colorspace = format_to_colorspace(it6625_formats[0].csi_format);
 
@@ -1842,6 +1881,8 @@ static int it6625_v4l2_init_controls(struct v4l2_subdev *sd)
 			   it6625->csi_lanes == 3;
 
 	v4l2_ctrl_handler_init(hdl, 4);
+	hdl->lock = &it6625->it6625_lock;
+
 	it6625->ctrl_5v_detect =
 		v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_DV_RX_POWER_PRESENT,
 				  0, 1, 0, 0);
@@ -2050,8 +2091,6 @@ static void it6625_init_data(struct it6625 *it6625)
 	static struct v4l2_dv_timings default_timing =
 			V4L2_DV_BT_CEA_1920X1080P60;
 
-	it6625->csi_format = it6625_formats[0].csi_format;
-	it6625->mbus_fmt_code = it6625_formats[0].mbus_fmt_code;
 	it6625->timings = default_timing;
 	/* firmware ships with a verified 2-block default EDID in EDID RAM */
 	it6625->edid_blocks = 2;
@@ -2246,9 +2285,16 @@ static int it6625_probe(struct i2c_client *client)
 		goto err_clean_work_queues;
 	}
 
+	sd->state_lock = &it6625->it6625_lock;
+	err = v4l2_subdev_init_finalize(sd);
+	if (err) {
+		dev_err(it6625->dev, "%s %d err=%d", __func__, __LINE__, err);
+		goto err_clean_hdl;
+	}
+
 	err = v4l2_ctrl_handler_setup(sd->ctrl_handler);
 	if (err)
-		goto err_clean_hdl;
+		goto err_clean_state;
 
 	it6625->cec_adap = cec_allocate_adapter(&it6625_cec_adap_ops,
 						it6625, dev_name(it6625->dev),
@@ -2259,7 +2305,7 @@ static int it6625_probe(struct i2c_client *client)
 	if (IS_ERR(it6625->cec_adap)) {
 		err = PTR_ERR(it6625->cec_adap);
 		dev_err(it6625->dev, "%s %d", __func__, __LINE__);
-		goto err_clean_hdl;
+		goto err_clean_state;
 	}
 
 	err = cec_register_adapter(it6625->cec_adap, &client->dev);
@@ -2267,7 +2313,7 @@ static int it6625_probe(struct i2c_client *client)
 		dev_err(it6625->dev, "%s: failed to register the cec device", __func__);
 		cec_delete_adapter(it6625->cec_adap);
 		it6625->cec_adap = NULL;
-		goto err_clean_hdl;
+		goto err_clean_state;
 	}
 
 	it6625_debugfs_init(it6625, client);
@@ -2294,6 +2340,8 @@ static int it6625_probe(struct i2c_client *client)
 	v4l2_debugfs_if_free(it6625->infoframes);
 	debugfs_remove_recursive(it6625->debugfs_dir);
 	cec_unregister_adapter(it6625->cec_adap);
+err_clean_state:
+	v4l2_subdev_cleanup(sd);
 err_clean_hdl:
 	media_entity_cleanup(&sd->entity);
 	v4l2_ctrl_handler_free(&it6625->hdl);
@@ -2330,12 +2378,20 @@ static void it6625_remove(struct i2c_client *client)
 
 	debugfs_remove_recursive(it6625->debugfs_dir);
 	cec_unregister_adapter(it6625->cec_adap);
+
+	/*
+	 * v4l2_subdev_cleanup()/v4l2_ctrl_handler_free() take it6625_lock
+	 * (shared as state_lock/hdl.lock), so they must run before it's
+	 * destroyed.
+	 */
+	v4l2_subdev_cleanup(sd);
+	media_entity_cleanup(&sd->entity);
+	v4l2_ctrl_handler_free(&it6625->hdl);
+
 	mutex_destroy(&it6625->it6625_lock);
 	mutex_destroy(&it6625->edid_lock);
 	mutex_destroy(&it6625->if_read_lock);
 	mutex_destroy(&it6625->if_state_lock);
-	media_entity_cleanup(&sd->entity);
-	v4l2_ctrl_handler_free(&it6625->hdl);
 }
 
 static const struct i2c_device_id it6625_id[] = {

-- 
2.34.1

[PATCH 21/21] media: i2c: it6625: use enable_streams and disable_streams

[Hermes Wu via B4 Relay]

From: Hermes Wu <Hermes.wu@ite.com.tw>

The .s_stream video op is deprecated; add .enable_streams()/
.disable_streams() pad ops instead and keep v4l2_subdev_s_stream_helper
for legacy .s_stream callers, matching the same-device-class precedent
in lt6911uxe.c.

it6625_enable_streams()/it6625_disable_streams() call
it6625_enable_stream_locked() directly rather than the removed
lock-taking it6625_enable_stream() wrapper: v4l2_subdev_enable_streams()/
disable_streams() already lock the active state -- and hence
it6625_lock, shared as sd->state_lock since the previous change --
before calling into these ops.

Do not set V4L2_SUBDEV_FL_STREAMS: this device has a single,
non-multiplexed source pad, so the core's implicit stream 0 is
sufficient and v4l2_subdev_s_stream_helper() already handles that case
without it. Error handling is unchanged from the locked helper these
ops now call directly, so streaming failures are reported to the core
instead of being silently absorbed.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/media/i2c/it6625.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 550eb95c6f74a81fc11cc055b4590159ac50e969..680f27a4587eccfb863bb091c1dede290e57e909 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -936,12 +936,6 @@ static int it6625_enable_stream_locked(struct it6625 *it6625, bool enable)
 	return it6625_update_config(it6625);
 }
 
-static int it6625_enable_stream(struct it6625 *it6625, bool enable)
-{
-	guard(mutex)(&it6625->it6625_lock);
-	return it6625_enable_stream_locked(it6625, enable);
-}
-
 static int it6625_set_mipi_config_locked(struct it6625 *it6625, u32 cfg_val)
 {
 	u8 mipi_data_type;
@@ -1503,11 +1497,22 @@ static int it6625_dv_timings_cap(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int it6625_s_stream(struct v4l2_subdev *sd, int enable)
+static int it6625_enable_streams(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state,
+				 u32 pad, u64 streams_mask)
+{
+	struct it6625 *it6625 = sd_to_6625(sd);
+
+	return it6625_enable_stream_locked(it6625, true);
+}
+
+static int it6625_disable_streams(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state,
+				  u32 pad, u64 streams_mask)
 {
 	struct it6625 *it6625 = sd_to_6625(sd);
 
-	return it6625_enable_stream(it6625, enable);
+	return it6625_enable_stream_locked(it6625, false);
 }
 
 static int it6625_enum_mbus_code(struct v4l2_subdev *sd,
@@ -1795,7 +1800,7 @@ static const struct v4l2_subdev_core_ops it6625_core_ops = {
 
 static const struct v4l2_subdev_video_ops it6625_video_ops = {
 	.g_input_status = it6625_g_input_status,
-	.s_stream = it6625_s_stream,
+	.s_stream = v4l2_subdev_s_stream_helper,
 };
 
 static const struct v4l2_subdev_pad_ops it6625_pad_ops = {
@@ -1810,6 +1815,8 @@ static const struct v4l2_subdev_pad_ops it6625_pad_ops = {
 	.s_dv_timings = it6625_pad_s_dv_timings,
 	.g_dv_timings = it6625_pad_g_dv_timings,
 	.query_dv_timings = it6625_pad_query_dv_timings,
+	.enable_streams = it6625_enable_streams,
+	.disable_streams = it6625_disable_streams,
 };
 
 static const struct v4l2_subdev_ops it6625_ops = {

-- 
2.34.1

Re: [PATCH 02/21] media: i2c: it6625: propagate control-update errors

[Sakari Ailus]

Hi Hermes,

On Fri, Sep 18, 2026 at 04:57:17PM +0800, Hermes Wu via B4 Relay wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> it6625_v4l2_sd_ctrl_update() discarded the return value of all three
> v4l2_ctrl_s_ctrl() calls it makes. Make it return int, run the updates
> sequentially, and return the first error; both call sites now check
> and log it.
> 
> None of the three controls has a driver .ops, so the only failure path
> in v4l2_ctrl_s_ctrl() is range validation, and every value this driver
> passes is always in range -- this is an error-handling correctness fix
> responsive to review, not a fix for an observed runtime failure.
> 
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/media/i2c/it6625.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
> index da17e5d5ce9e2dd1060abba1598b0e3f78857e9c..5d267676606e340ad0ee5ad2b78e5acb50a32980 100644
> --- a/drivers/media/i2c/it6625.c
> +++ b/drivers/media/i2c/it6625.c
> @@ -889,11 +889,19 @@ static int it6625_s_ctrl_audio_present(struct v4l2_subdev *sd)
>  				audio_present(it6625));
>  }
>  
> -static void it6625_v4l2_sd_ctrl_update(struct v4l2_subdev *sd)
> +static int it6625_v4l2_sd_ctrl_update(struct v4l2_subdev *sd)
>  {
> -	it6625_s_ctrl_detect_hdmi_5v(sd);
> -	it6625_s_ctrl_audio_sampling_rate(sd);
> -	it6625_s_ctrl_audio_present(sd);
> +	int ret;
> +
> +	ret = it6625_s_ctrl_detect_hdmi_5v(sd);
> +	if (ret)
> +		return ret;
> +
> +	ret = it6625_s_ctrl_audio_sampling_rate(sd);
> +	if (ret)
> +		return ret;
> +
> +	return it6625_s_ctrl_audio_present(sd);
>  }
>  
>  static void it6625_enable_stream_locked(struct it6625 *it6625, bool enable)
> @@ -1136,9 +1144,12 @@ static void it6625_clear_timings(struct it6625 *it6625)
>  static void it6625_irq_hdmi_5v_change(struct it6625 *it6625)
>  {
>  	struct v4l2_subdev *sd = &it6625->sd;
> +	int ret;
>  
>  	it6625_clear_timings(it6625);
> -	it6625_v4l2_sd_ctrl_update(sd);
> +	ret = it6625_v4l2_sd_ctrl_update(sd);
> +	if (ret)
> +		dev_err(it6625->dev, "%s: failed to update controls: %d", __func__, ret);
>  }
>  
>  static void it6625_irq_hdcp_change(struct it6625 *it6625)
> @@ -2297,7 +2308,9 @@ static int it6625_probe(struct i2c_client *client)
>  	it6625_debugfs_init(it6625, client);
>  
>  	it6625_initial_setup(it6625);

What if this fails?

> -	it6625_v4l2_sd_ctrl_update(sd);
> +	err = it6625_v4l2_sd_ctrl_update(sd);
> +	if (err)
> +		dev_err(it6625->dev, "%s: failed to update controls: %d", __func__, err);

Shouldn't you return an error in this case?

dev_err_probe() might be better here, too.

>  
>  	err = v4l2_async_register_subdev(sd);
>  	if (err < 0) {
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH 13/21] media: i2c: it6625: decode detected timings via typed register structs

[Sakari Ailus]

Hi Hermes,

Thank you for the patches.

On Fri, Sep 18, 2026 at 04:57:28PM +0800, Hermes Wu via B4 Relay wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> it6625_get_detected_timings() manually assembled each 16-bit field from
> raw byte-buffer offsets with a shift-and-add sequence. Define two local
> structs of __be16 fields matching the contiguous REG_H_ACTIVE_1..
> REG_V_ACTIVE_0 and REG_H_FP_1..REG_V_BP_0 register layouts, read
> directly into them, and decode each field with be16_to_cpu(). Guard
> each struct's size with static_assert() against the expected register
> range width.
> 
> Every member is 2 bytes wide and naturally aligned, so the struct is
> laid out with no padding -- this is safe because the struct is the I2C
> read target itself, not a cast over a pre-existing raw buffer.
> 
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/media/i2c/it6625.c | 41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
> index 60c79a2277941621c7aa83189244b706a5505d66..90b87dbf54fcfc7ad2a1245d4594beee24a193ca 100644
> --- a/drivers/media/i2c/it6625.c
> +++ b/drivers/media/i2c/it6625.c
> @@ -769,10 +769,22 @@ static int it6625_get_detected_timings(struct it6625 *it6625,
>  				       struct v4l2_dv_timings *timings)
>  {
>  	struct v4l2_bt_timings *bt = &timings->bt;
> +	struct {
> +		__be16 h_active;
> +		__be16 v_active;
> +	} active;
> +	struct {
> +		__be16 hfrontporch;
> +		__be16 hsync;
> +		__be16 hbackporch;
> +		__be16 vfrontporch;
> +		__be16 vsync;
> +		__be16 vbackporch;
> +	} porch;

The driver accesses many such register areas, I'd define these separate
from the functions that use them. This isn't the only one case.

Alternatively you could define each register separately, which is what most
drivers do, albeit the usage pattern in this driver is a bit atypical so I
think using structs for this indeed could make sense.

>  	int val;
> -	unsigned int width, height;
> -	u8 buffer[4];
> -	u8 buffer2[12];
> +
> +	static_assert(sizeof(active) == 4);
> +	static_assert(sizeof(porch) == 12);
>  
>  	if (no_signal(it6625)) {
>  		dev_err(it6625->dev, "no signal detected");
> @@ -792,24 +804,21 @@ static int it6625_get_detected_timings(struct it6625 *it6625,
>  	bt->interlaced = val & B_INTERLACE ?
>  			 V4L2_DV_INTERLACED : V4L2_DV_PROGRESSIVE;
>  
> -	if (it6625_read_bytes(it6625, REG_H_ACTIVE_1, buffer, 4) < 0)
> +	if (it6625_read_bytes(it6625, REG_H_ACTIVE_1, (u8 *)&active, sizeof(active)) < 0)
>  		return -EIO;
>  
> -	width = ((buffer[0] & 0xff) << 8) + buffer[1];
> -	height = ((buffer[2] & 0xff) << 8) + buffer[3];
> -
> -	bt->width = width;
> -	bt->height = height;
> +	bt->width = be16_to_cpu(active.h_active);
> +	bt->height = be16_to_cpu(active.v_active);
>  
> -	if (it6625_read_bytes(it6625, REG_H_FP_1, buffer2, 12) < 0)
> +	if (it6625_read_bytes(it6625, REG_H_FP_1, (u8 *)&porch, sizeof(porch)) < 0)
>  		return -EIO;
>  
> -	bt->hfrontporch = ((buffer2[0] & 0xff) << 8) + buffer2[1];
> -	bt->hsync = ((buffer2[2] & 0xff) << 8) + buffer2[3];
> -	bt->hbackporch = ((buffer2[4] & 0xff) << 8) + buffer2[5];
> -	bt->vfrontporch = ((buffer2[6] & 0xff) << 8) + buffer2[7];
> -	bt->vsync = ((buffer2[8] & 0xff) << 8) + buffer2[9];
> -	bt->vbackporch = ((buffer2[10] & 0xff) << 8) + buffer2[11];
> +	bt->hfrontporch = be16_to_cpu(porch.hfrontporch);
> +	bt->hsync = be16_to_cpu(porch.hsync);
> +	bt->hbackporch = be16_to_cpu(porch.hbackporch);
> +	bt->vfrontporch = be16_to_cpu(porch.vfrontporch);
> +	bt->vsync = be16_to_cpu(porch.vsync);
> +	bt->vbackporch = be16_to_cpu(porch.vbackporch);
>  
>  	bt->pixelclock = it6625_get_pclk(it6625);
>  	if (bt->interlaced == V4L2_DV_INTERLACED) {
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 19/21] media: i2c: it6625: fold subdev initialization into probe

[Sakari Ailus]

Hi Hermes,

Overall the patches are nice, thank you. One comment below...

On Fri, Sep 18, 2026 at 04:57:34PM +0800, Hermes Wu via B4 Relay wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> it6625_init_v4l2_subdev() split subdev/control-handler/media-entity
> initialization out of probe(), which made error handling harder to
> follow across the two functions and forced probe() to normalize every
> init_v4l2_subdev() failure to -ENOMEM regardless of the real error
> (e.g. a media_entity_pads_init() failure was reported to the caller as
> -ENOMEM instead of its actual code).
> 
> Inline it into it6625_probe() so initialization and its unwind path
> are visible together, and propagate the real error from
> it6625_v4l2_init_controls() instead of replacing it. Cleanup behavior
> on each failure path is unchanged: it6625_v4l2_init_controls() already
> frees the control handler internally before returning an error, and a
> media_entity_pads_init() failure still frees it explicitly before
> unwinding, so neither path double-frees it through the later
> err_clean_hdl label.
> 
> State finalization (v4l2_subdev_init_finalize()/cleanup()) is
> deliberately left for a follow-up change, to keep this a pure
> restructuring and keep the locking-model transition atomic on its own.
> 
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/media/i2c/it6625.c | 46 +++++++++++++++++-----------------------------
>  1 file changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
> index 40cd413e0ed49c118421ce3dec77bb2df08cf042..ec6aaa878471ff210264a35e4aa66dfb1e63ed3b 100644
> --- a/drivers/media/i2c/it6625.c
> +++ b/drivers/media/i2c/it6625.c
> @@ -2142,33 +2142,6 @@ static int it6625_parse_dt(struct it6625 *it6625)
>  	return it6625_parse_endpoint(it6625);
>  }
>  
> -static int it6625_init_v4l2_subdev(struct it6625 *it6625)
> -{
> -	struct v4l2_subdev *sd = &it6625->sd;
> -	int err;
> -
> -	sd->dev = it6625->dev;
> -
> -	v4l2_i2c_subdev_init(sd, it6625->i2c_client, &it6625_ops);
> -	sd->internal_ops = &it6625_internal_ops;
> -	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> -	if (it6625_v4l2_init_controls(sd)) {
> -		dev_err(it6625->dev, "Failed to initialize v4l2 controls");
> -		return -ENOMEM;
> -	}
> -
> -	it6625->pad.flags = MEDIA_PAD_FL_SOURCE;
> -	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> -	err = media_entity_pads_init(&sd->entity, 1, &it6625->pad);
> -	if (err < 0) {
> -		dev_err(it6625->dev, "%s %d err=%d", __func__, __LINE__, err);
> -		v4l2_ctrl_handler_free(sd->ctrl_handler);
> -		return err;
> -	}
> -
> -	return 0;
> -}
> -
>  static int it6625_check_device(struct it6625 *it6625)
>  {
>  	static const u8 chip_ids[][2] = {
> @@ -2254,9 +2227,24 @@ static int it6625_probe(struct i2c_client *client)
>  	}
>  
>  	sd = &it6625->sd;
> -	err = it6625_init_v4l2_subdev(it6625);
> -	if (err)
> +	v4l2_i2c_subdev_init(sd, it6625->i2c_client, &it6625_ops);
> +	sd->internal_ops = &it6625_internal_ops;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> +
> +	err = it6625_v4l2_init_controls(sd);
> +	if (err) {
> +		dev_err(it6625->dev, "failed to initialize v4l2 controls: %d", err);
>  		goto err_clean_work_queues;
> +	}
> +
> +	it6625->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	err = media_entity_pads_init(&sd->entity, 1, &it6625->pad);
> +	if (err < 0) {
> +		dev_err(it6625->dev, "%s %d err=%d", __func__, __LINE__, err);
> +		v4l2_ctrl_handler_free(sd->ctrl_handler);

Please add a new label instead of freeing the control handler here.

> +		goto err_clean_work_queues;
> +	}
>  
>  	err = v4l2_ctrl_handler_setup(sd->ctrl_handler);
>  	if (err)
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 20/21] media: i2c: it6625: use centrally managed active state

[Sakari Ailus]

Hi Hermes,

On Fri, Sep 18, 2026 at 04:57:35PM +0800, Hermes Wu via B4 Relay wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> Adopt the subdev active-state model instead of a private driver mutex
> and driver-local format fields:
> 
>  - Share it6625_lock as both sd->state_lock and sd->ctrl_handler->lock
>    (set hdl->lock right after v4l2_ctrl_handler_init(), which resets
>    it, and before any control is created), matching the documented
>    model in Documentation/driver-api/media/v4l2-subdev.rst. Call
>    v4l2_subdev_init_finalize() after media_entity_pads_init() and
>    before any code that touches active state, always before
>    v4l2_async_register_subdev().
> 
>  - Remove csi_format/mbus_fmt_code from struct it6625 and store the
>    complete format -- code, colorspace, and width/height/field derived
>    from the configured DV timings -- in the pad format of each
>    v4l2_subdev_state instead. it6625_set_fmt() now operates directly
>    on the state the core hands it (already locked, and for ACTIVE
>    already sd->active_state) instead of taking it6625_lock itself.
>    it6625_get_fmt() is deleted; .get_fmt is now the core's
>    v4l2_subdev_get_fmt(). it6625_init_state() seeds a state from the
>    current active format when one exists (later TRY-state allocation)
>    or from driver defaults when it doesn't (the very first, pre-
>    assignment call that becomes the active state itself).
> 
>  - Keep it6625->timings as driver-private DV-timings state, but project
>    its width/height/field onto the active pad format every time it
>    changes (it6625_update_timings_if_changed(), it6625_clear_timings())
>    so VIDIOC_SUBDEV_G_FMT stays consistent with
>    VIDIOC_SUBDEV_G_DV_TIMINGS. it6625_initial_setup() and
>    it6625_log_status() now read the media-bus code from the locked
>    active format instead of the removed fields.
> 
>  - Make ACTIVE .set_fmt() transactional: return -EBUSY while streaming,
>    and commit the new code/colorspace to active state only after the
>    MIPI register writes that implement it actually succeed. This
>    requires it6625_enable_stream_locked(), it6625_enable_stream(), and
>    it6625_set_mipi_config_locked() to return int and propagate the
>    first register/config-update failure; all existing callers are
>    updated.

I don't think all the above is relevant in the commit message.

> 
> Every it6625_lock acquisition site was re-audited against the core's
> actual locking contract (which ioctls the core state-locks, what
> v4l2_ctrl_handler_init()/_free() and v4l2_subdev_cleanup() actually
> touch) to confirm none of the driver's own lock-taking helpers are
> reachable from a path where the core already holds it6625_lock, and
> that init/teardown ordering keeps hdl->lock valid whenever
> v4l2_ctrl_handler_free() runs. The full trace is recorded in
> it6625-follow-up-patch/patch19-lock-audit.md (not part of this commit)
> for review -- it is static analysis only, since no IT6625/IT6626
> hardware is available here to exercise this with lockdep enabled.

This paragraph could go to the cover letter.

> 
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/media/i2c/it6625.c | 220 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 138 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
> index ec6aaa878471ff210264a35e4aa66dfb1e63ed3b..550eb95c6f74a81fc11cc055b4590159ac50e969 100644
> --- a/drivers/media/i2c/it6625.c
> +++ b/drivers/media/i2c/it6625.c
> @@ -255,7 +255,15 @@ struct it6625 {
>  	struct regmap *it6625_regmap;
>  	enum it6625_chip_type chip_type;
>  
> -	/* protects concurrent access to the chip's registers and state */
> +	/*
> +	 * Protects concurrent access to the chip's registers and state.
> +	 * Also shared as sd.state_lock and hdl.lock (see

This doesn't hold anymore, does it? Typically drivers do without such locks
as the subdev state lock is used instead -- assigning your own lock also
has the effect the same lock is used for try states, too.

> +	 * Documentation/driver-api/media/v4l2-subdev.rst), so the V4L2
> +	 * core already holds it across .get_fmt/.set_fmt/.enable_streams/
> +	 * .disable_streams and control updates -- callers reached only
> +	 * through those paths must use the *_locked() helpers instead of
> +	 * taking it again.
> +	 */
>  	struct mutex it6625_lock;
>  	/* serializes the complete VIDIOC_S_EDID sequence against itself */
>  	struct mutex edid_lock;
> @@ -291,8 +299,6 @@ struct it6625 {
>  	u8 csi_lanes;
>  	u8 port_num;
>  	enum v4l2_mbus_type bus_type;
> -	u8 csi_format;
> -	u32 mbus_fmt_code;
>  	/* number of EDID blocks currently loaded, protected by edid_lock */
>  	u8 edid_blocks;
>  
> @@ -911,10 +917,11 @@ static int it6625_v4l2_sd_ctrl_update(struct v4l2_subdev *sd)
>  	return it6625_s_ctrl_audio_present(sd);
>  }
>  
> -static void it6625_enable_stream_locked(struct it6625 *it6625, bool enable)
> +static int it6625_enable_stream_locked(struct it6625 *it6625, bool enable)
>  {
>  	struct v4l2_subdev *sd = &it6625->sd;
>  	int val;
> +	int err;
>  
>  	lockdep_assert_held(&it6625->it6625_lock);
>  
> @@ -922,27 +929,34 @@ static void it6625_enable_stream_locked(struct it6625 *it6625, bool enable)
>  		 __func__, enable ? "en" : "dis");
>  
>  	val = enable ? B_MIPI_OUTPUT : 0;
> -	it6625_set_bits(it6625, REG_MIPI_CONTROL, B_MIPI_OUTPUT, val);
> -	it6625_update_config(it6625);
> +	err = it6625_set_bits(it6625, REG_MIPI_CONTROL, B_MIPI_OUTPUT, val);
> +	if (err < 0)
> +		return err;
> +
> +	return it6625_update_config(it6625);
>  }
>  
> -static void it6625_enable_stream(struct it6625 *it6625, bool enable)
> +static int it6625_enable_stream(struct it6625 *it6625, bool enable)
>  {
>  	guard(mutex)(&it6625->it6625_lock);
> -	it6625_enable_stream_locked(it6625, enable);
> +	return it6625_enable_stream_locked(it6625, enable);
>  }
>  
> -static void it6625_set_mipi_config_locked(struct it6625 *it6625, u32 cfg_val)
> +static int it6625_set_mipi_config_locked(struct it6625 *it6625, u32 cfg_val)
>  {
>  	u8 mipi_data_type;
> +	int err;
>  
>  	lockdep_assert_held(&it6625->it6625_lock);
>  
>  	dev_dbg(it6625->dev, "mipi_data_type = 0x%x", cfg_val);
>  
>  	mipi_data_type = cfg_val & 0xFF;
> -	it6625_write_byte(it6625, REG_MIPI_DATA_TYPE, mipi_data_type);
> -	it6625_update_config(it6625);
> +	err = it6625_write_byte(it6625, REG_MIPI_DATA_TYPE, mipi_data_type);
> +	if (err < 0)
> +		return err;
> +
> +	return it6625_update_config(it6625);
>  }
>  
>  static inline unsigned int fps_from_bt_timings(const struct v4l2_bt_timings *t)
> @@ -957,10 +971,18 @@ static inline unsigned int fps_from_bt_timings(const struct v4l2_bt_timings *t)
>  
>  static void it6625_initial_setup(struct it6625 *it6625)
>  {
> +	struct v4l2_subdev *sd = &it6625->sd;
> +	struct v4l2_mbus_framefmt *fmt;
> +	int idx;
>  	int val = 0;
>  
>  	guard(mutex)(&it6625->it6625_lock);
>  
> +	fmt = v4l2_subdev_state_get_format(v4l2_subdev_get_locked_active_state(sd), 0);
> +	idx = it6625_csi_mbus_code_idx(fmt->code);
> +	if (idx < 0)
> +		idx = 0;
> +
>  	/*
>  	 * REG_MIPI_CFG[0:2] lane count field: 1 lane -> 0, 2 lanes -> 1,
>  	 * 3 lanes (C-PHY only) -> 3, 4 lanes (D-PHY only) -> 3.
> @@ -984,7 +1006,7 @@ static void it6625_initial_setup(struct it6625 *it6625)
>  		val |= FIELD_PREP(B_MIPI_SPLIT, 1);
>  
>  	it6625_write_byte(it6625, REG_MIPI_CFG, val);
> -	it6625_write_byte(it6625, REG_MIPI_DATA_TYPE, it6625->csi_format);
> +	it6625_write_byte(it6625, REG_MIPI_DATA_TYPE, it6625_formats[idx].csi_format);
>  	it6625_write_byte(it6625, REG_MIPI_CONTROL, 0x00);
>  	it6625_write_byte(it6625, REG_RX_CFG, 0x00);
>  
> @@ -1140,10 +1162,29 @@ static void it6625_get_timings(struct it6625 *it6625,
>  	*timings = it6625->timings;
>  }
>  
> +/*
> + * Project a DV-timings struct's width/height/field onto an active pad
> + * format. Caller must hold it6625_lock (== the active state's lock).
> + */
> +static void it6625_fill_timings_format(const struct v4l2_dv_timings *timings,
> +				       struct v4l2_mbus_framefmt *fmt)
> +{
> +	fmt->width = timings->bt.width;
> +	fmt->height = timings->bt.height;
> +	fmt->field = timings->bt.interlaced == V4L2_DV_INTERLACED ?
> +		     V4L2_FIELD_INTERLACED : V4L2_FIELD_NONE;
> +}
> +
>  static void it6625_clear_timings(struct it6625 *it6625)
>  {
> +	struct v4l2_subdev *sd = &it6625->sd;
> +	struct v4l2_mbus_framefmt *fmt;
> +
>  	guard(mutex)(&it6625->it6625_lock);
>  	memset(&it6625->timings, 0, sizeof(it6625->timings));
> +
> +	fmt = v4l2_subdev_state_get_format(v4l2_subdev_get_locked_active_state(sd), 0);
> +	it6625_fill_timings_format(&it6625->timings, fmt);
>  }
>  
>  static void it6625_irq_hdmi_5v_change(struct it6625 *it6625)
> @@ -1362,7 +1403,10 @@ static int it6625_log_status(struct v4l2_subdev *sd)
>  
>  	/* snapshot together so the reported pair was actually configured together */
>  	scoped_guard(mutex, &it6625->it6625_lock) {
> -		mbus_fmt_code = it6625->mbus_fmt_code;
> +		struct v4l2_mbus_framefmt *fmt =
> +			v4l2_subdev_state_get_format(v4l2_subdev_get_locked_active_state(sd), 0);
> +
> +		mbus_fmt_code = fmt->code;
>  		bt = it6625->timings.bt;
>  	}
>  
> @@ -1415,6 +1459,9 @@ static int
>  it6625_update_timings_if_changed(struct it6625 *it6625,
>  				 const struct v4l2_dv_timings *timings)
>  {
> +	struct v4l2_subdev *sd = &it6625->sd;
> +	struct v4l2_mbus_framefmt *fmt;
> +
>  	guard(mutex)(&it6625->it6625_lock);
>  
>  	if (v4l2_match_dv_timings(&it6625->timings, timings, 0, false))
> @@ -1425,6 +1472,9 @@ it6625_update_timings_if_changed(struct it6625 *it6625,
>  
>  	it6625->timings = *timings;
>  
> +	fmt = v4l2_subdev_state_get_format(v4l2_subdev_get_locked_active_state(sd), 0);
> +	it6625_fill_timings_format(&it6625->timings, fmt);
> +
>  	return 1;
>  }
>  
> @@ -1457,8 +1507,7 @@ static int it6625_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct it6625 *it6625 = sd_to_6625(sd);
>  
> -	it6625_enable_stream(it6625, enable);
> -	return 0;
> +	return it6625_enable_stream(it6625, enable);
>  }
>  
>  static int it6625_enum_mbus_code(struct v4l2_subdev *sd,
> @@ -1586,84 +1635,56 @@ static inline u32 format_to_colorspace(u8 csi_format)
>  	}
>  }
>  
> -static int it6625_get_fmt(struct v4l2_subdev *sd,
> -			  struct v4l2_subdev_state *sd_state,
> -			  struct v4l2_subdev_format *format)
> -{
> -	struct it6625 *it6625 = sd_to_6625(sd);
> -	struct v4l2_dv_timings timings;
> -
> -	if (format->pad != 0)
> -		return -EINVAL;
> -
> -	it6625_get_timings(it6625, &timings);
> -	format->format.width = timings.bt.width;
> -	format->format.height = timings.bt.height;
> -	format->format.field = timings.bt.interlaced == V4L2_DV_INTERLACED ?
> -			       V4L2_FIELD_INTERLACED : V4L2_FIELD_NONE;
> -
> -	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		struct v4l2_mbus_framefmt *fmt;
> -
> -		fmt = v4l2_subdev_state_get_format(sd_state, format->pad);
> -		format->format.code = fmt->code;
> -		format->format.colorspace = fmt->colorspace;
> -	} else {
> -		scoped_guard(mutex, &it6625->it6625_lock) {
> -			format->format.colorspace =
> -				format_to_colorspace(it6625->csi_format);
> -			format->format.code = it6625->mbus_fmt_code;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  static int it6625_set_fmt(struct v4l2_subdev *sd,
>  			  struct v4l2_subdev_state *sd_state,
>  			  struct v4l2_subdev_format *format)
>  {
>  	struct it6625 *it6625 = sd_to_6625(sd);
> -	u32 mbus_fmt_code = format->format.code;
> +	struct v4l2_mbus_framefmt *fmt;
> +	u32 colorspace;
> +	int idx;
>  	int ret;
>  
> -	ret = it6625_get_fmt(sd, sd_state, format);
> -	format->format.code = mbus_fmt_code;
> -
> -	if (ret)
> -		return ret;
> -
> -	ret = it6625_csi_mbus_code_idx(mbus_fmt_code);
> +	if (format->pad != 0)
> +		return -EINVAL;
>  
> -	if (ret < 0) {
> +	idx = it6625_csi_mbus_code_idx(format->format.code);
> +	if (idx < 0) {
>  		v4l2_dbg(1, debug, sd,
>  			 "%s: unsupported format code 0x%x, falling back to default",
> -			 __func__, mbus_fmt_code);
> -		ret = 0;
> -		mbus_fmt_code = it6625_formats[ret].mbus_fmt_code;
> -		format->format.code = mbus_fmt_code;
> +			 __func__, format->format.code);
> +		idx = 0;
>  	}
>  
> -	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		struct v4l2_mbus_framefmt *fmt;
> +	colorspace = format_to_colorspace(it6625_formats[idx].csi_format);
>  
> -		fmt = v4l2_subdev_state_get_format(sd_state, format->pad);
> -		fmt->code = format->format.code;
> -		fmt->colorspace = format_to_colorspace(it6625_formats[ret].csi_format);
> -		format->format.colorspace = fmt->colorspace;
> +	/* fmt already carries this state's width/height/field; leave them alone */
> +	fmt = v4l2_subdev_state_get_format(sd_state, format->pad);
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		fmt->code = it6625_formats[idx].mbus_fmt_code;
> +		fmt->colorspace = colorspace;
> +		format->format = *fmt;

Could you rework the code to keep this for active and try paths?

>  		v4l2_dbg(1, debug, sd, "%s: try format code = 0x%x",
>  			 __func__, format->format.code);
>  		return 0;
>  	}
>  
> -	scoped_guard(mutex, &it6625->it6625_lock) {
> -		it6625->csi_format = it6625_formats[ret].csi_format;
> -		it6625->mbus_fmt_code = format->format.code;
> -		it6625_enable_stream_locked(it6625, false);
> -		it6625_set_mipi_config_locked(it6625, it6625->csi_format);
> -	}
> +	if (v4l2_subdev_is_streaming(sd))
> +		return -EBUSY;
>  
> -	format->format.colorspace = format_to_colorspace(it6625_formats[ret].csi_format);
> +	ret = it6625_enable_stream_locked(it6625, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = it6625_set_mipi_config_locked(it6625, it6625_formats[idx].csi_format);

There are a few lines longer than 80; please split unless there's a
tangible reason to do otherwise.

> +	if (ret)
> +		return ret;
> +
> +	/* commit to active state only after hardware programming succeeded */
> +	fmt->code = it6625_formats[idx].mbus_fmt_code;
> +	fmt->colorspace = colorspace;
> +	format->format = *fmt;
>  
>  	return 0;
>  }
> @@ -1780,7 +1801,7 @@ static const struct v4l2_subdev_video_ops it6625_video_ops = {
>  static const struct v4l2_subdev_pad_ops it6625_pad_ops = {
>  	.enum_mbus_code = it6625_enum_mbus_code,
>  	.set_fmt = it6625_set_fmt,
> -	.get_fmt = it6625_get_fmt,
> +	.get_fmt = v4l2_subdev_get_fmt,
>  	.get_edid = it6625_g_edid,
>  	.set_edid = it6625_s_edid,
>  	.enum_dv_timings = it6625_enum_dv_timings,
> @@ -1800,8 +1821,26 @@ static const struct v4l2_subdev_ops it6625_ops = {
>  static int it6625_init_state(struct v4l2_subdev *sd,
>  			     struct v4l2_subdev_state *sd_state)
>  {
> +	struct it6625 *it6625 = sd_to_6625(sd);
> +	struct v4l2_subdev_state *active = v4l2_subdev_get_locked_active_state(sd);

This function should work the same way independently of whether the state
is active or not.

>  	struct v4l2_mbus_framefmt *fmt = v4l2_subdev_state_get_format(sd_state, 0);
>  
> +	/*
> +	 * The very first call initializes what becomes sd->active_state
> +	 * itself, before it's assigned -- active is NULL then, and this
> +	 * state gets the driver's own defaults. Every later call (opening
> +	 * a new file handle) initializes a fresh TRY state while the
> +	 * active state already exists and is locked by the same mutex
> +	 * (state->lock is assigned before init_state() runs), so seed it
> +	 * from the current active format instead of reverting to boot
> +	 * defaults.
> +	 */
> +	if (active) {
> +		*fmt = *v4l2_subdev_state_get_format(active, 0);
> +		return 0;
> +	}
> +
> +	it6625_fill_timings_format(&it6625->timings, fmt);
>  	fmt->code = it6625_formats[0].mbus_fmt_code;
>  	fmt->colorspace = format_to_colorspace(it6625_formats[0].csi_format);
>  
> @@ -1842,6 +1881,8 @@ static int it6625_v4l2_init_controls(struct v4l2_subdev *sd)
>  			   it6625->csi_lanes == 3;
>  
>  	v4l2_ctrl_handler_init(hdl, 4);
> +	hdl->lock = &it6625->it6625_lock;
> +
>  	it6625->ctrl_5v_detect =
>  		v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_DV_RX_POWER_PRESENT,
>  				  0, 1, 0, 0);
> @@ -2050,8 +2091,6 @@ static void it6625_init_data(struct it6625 *it6625)
>  	static struct v4l2_dv_timings default_timing =
>  			V4L2_DV_BT_CEA_1920X1080P60;
>  
> -	it6625->csi_format = it6625_formats[0].csi_format;
> -	it6625->mbus_fmt_code = it6625_formats[0].mbus_fmt_code;
>  	it6625->timings = default_timing;
>  	/* firmware ships with a verified 2-block default EDID in EDID RAM */
>  	it6625->edid_blocks = 2;
> @@ -2246,9 +2285,16 @@ static int it6625_probe(struct i2c_client *client)
>  		goto err_clean_work_queues;
>  	}
>  
> +	sd->state_lock = &it6625->it6625_lock;
> +	err = v4l2_subdev_init_finalize(sd);
> +	if (err) {
> +		dev_err(it6625->dev, "%s %d err=%d", __func__, __LINE__, err);
> +		goto err_clean_hdl;
> +	}
> +
>  	err = v4l2_ctrl_handler_setup(sd->ctrl_handler);
>  	if (err)
> -		goto err_clean_hdl;
> +		goto err_clean_state;
>  
>  	it6625->cec_adap = cec_allocate_adapter(&it6625_cec_adap_ops,
>  						it6625, dev_name(it6625->dev),
> @@ -2259,7 +2305,7 @@ static int it6625_probe(struct i2c_client *client)
>  	if (IS_ERR(it6625->cec_adap)) {
>  		err = PTR_ERR(it6625->cec_adap);
>  		dev_err(it6625->dev, "%s %d", __func__, __LINE__);
> -		goto err_clean_hdl;
> +		goto err_clean_state;
>  	}
>  
>  	err = cec_register_adapter(it6625->cec_adap, &client->dev);
> @@ -2267,7 +2313,7 @@ static int it6625_probe(struct i2c_client *client)
>  		dev_err(it6625->dev, "%s: failed to register the cec device", __func__);
>  		cec_delete_adapter(it6625->cec_adap);
>  		it6625->cec_adap = NULL;
> -		goto err_clean_hdl;
> +		goto err_clean_state;
>  	}
>  
>  	it6625_debugfs_init(it6625, client);
> @@ -2294,6 +2340,8 @@ static int it6625_probe(struct i2c_client *client)
>  	v4l2_debugfs_if_free(it6625->infoframes);
>  	debugfs_remove_recursive(it6625->debugfs_dir);
>  	cec_unregister_adapter(it6625->cec_adap);
> +err_clean_state:
> +	v4l2_subdev_cleanup(sd);
>  err_clean_hdl:
>  	media_entity_cleanup(&sd->entity);
>  	v4l2_ctrl_handler_free(&it6625->hdl);
> @@ -2330,12 +2378,20 @@ static void it6625_remove(struct i2c_client *client)
>  
>  	debugfs_remove_recursive(it6625->debugfs_dir);
>  	cec_unregister_adapter(it6625->cec_adap);
> +
> +	/*
> +	 * v4l2_subdev_cleanup()/v4l2_ctrl_handler_free() take it6625_lock
> +	 * (shared as state_lock/hdl.lock), so they must run before it's
> +	 * destroyed.
> +	 */

This comment is hardly useful; the same applies to pretty much all drivers
using sub-device state and V4L2 controls.

> +	v4l2_subdev_cleanup(sd);
> +	media_entity_cleanup(&sd->entity);
> +	v4l2_ctrl_handler_free(&it6625->hdl);
> +
>  	mutex_destroy(&it6625->it6625_lock);
>  	mutex_destroy(&it6625->edid_lock);
>  	mutex_destroy(&it6625->if_read_lock);
>  	mutex_destroy(&it6625->if_state_lock);
> -	media_entity_cleanup(&sd->entity);
> -	v4l2_ctrl_handler_free(&it6625->hdl);
>  }
>  
>  static const struct i2c_device_id it6625_id[] = {
> 

-- 
Regards,

Sakari Ailus

RE: [PATCH 02/21] media: i2c: it6625: propagate control-update errors

[Hermes.Wu]

Hi Sakari

Thanks for review.

>Hi Hermes,
>
>On Fri, Sep 18, 2026 at 04:57:17PM +0800, Hermes Wu via B4 Relay wrote:
>> From: Hermes Wu <Hermes.wu@ite.com.tw>
>> 
>> it6625_v4l2_sd_ctrl_update() discarded the return value of all three
>> v4l2_ctrl_s_ctrl() calls it makes. Make it return int, run the updates 
>> sequentially, and return the first error; both call sites now check 
>> and log it.
>> 
>> None of the three controls has a driver .ops, so the only failure path 
>> in v4l2_ctrl_s_ctrl() is range validation, and every value this driver 
>> passes is always in range -- this is an error-handling correctness fix 
>> responsive to review, not a fix for an observed runtime failure.
>> 
>> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
>> ---
>>  drivers/media/i2c/it6625.c | 25 +++++++++++++++++++------
>>  1 file changed, 19 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c 
>> index 
>> da17e5d5ce9e2dd1060abba1598b0e3f78857e9c..5d267676606e340ad0ee5ad2b78e
>> 5acb50a32980 100644
>> --- a/drivers/media/i2c/it6625.c
>> +++ b/drivers/media/i2c/it6625.c
>> @@ -889,11 +889,19 @@ static int it6625_s_ctrl_audio_present(struct v4l2_subdev *sd)
>>  				audio_present(it6625));
>>  }
>>  
>> -static void it6625_v4l2_sd_ctrl_update(struct v4l2_subdev *sd)
>> +static int it6625_v4l2_sd_ctrl_update(struct v4l2_subdev *sd)
>>  {
>> -	it6625_s_ctrl_detect_hdmi_5v(sd);
>> -	it6625_s_ctrl_audio_sampling_rate(sd);
>> -	it6625_s_ctrl_audio_present(sd);
>> +	int ret;
>> +
>> +	ret = it6625_s_ctrl_detect_hdmi_5v(sd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = it6625_s_ctrl_audio_sampling_rate(sd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return it6625_s_ctrl_audio_present(sd);
>>  }
>>  
>>  static void it6625_enable_stream_locked(struct it6625 *it6625, bool 
>> enable) @@ -1136,9 +1144,12 @@ static void it6625_clear_timings(struct 
>> it6625 *it6625)  static void it6625_irq_hdmi_5v_change(struct it6625 
>> *it6625)  {
>>  	struct v4l2_subdev *sd = &it6625->sd;
>> +	int ret;
>>  
>>  	it6625_clear_timings(it6625);
>> -	it6625_v4l2_sd_ctrl_update(sd);
>> +	ret = it6625_v4l2_sd_ctrl_update(sd);
>> +	if (ret)
>> +		dev_err(it6625->dev, "%s: failed to update controls: %d", __func__, 
>> +ret);
>>  }
>>  
>>  static void it6625_irq_hdcp_change(struct it6625 *it6625) @@ -2297,7 
>> +2308,9 @@ static int it6625_probe(struct i2c_client *client)
>>  	it6625_debugfs_init(it6625, client);
>>  
>>  	it6625_initial_setup(it6625);
>
>What if this fails?

will fix in v2
>> -	it6625_v4l2_sd_ctrl_update(sd);
>> +	err = it6625_v4l2_sd_ctrl_update(sd);
>> +	if (err)
>> +		dev_err(it6625->dev, "%s: failed to update controls: %d", __func__, 
>> +err);
>
>Shouldn't you return an error in this case?
>
will fix in v2

>dev_err_probe() might be better here, too.
>
>>  
>>  	err = v4l2_async_register_subdev(sd);
>>  	if (err < 0) {
>> 
>
>--
>Regards,
>
>Sakari Ailus
>

RE: [PATCH 20/21] media: i2c: it6625: use centrally managed active state

[Hermes.Wu]

Hi Sakari,

Thank you for the review. 

> I don't think all the above is relevant in the commit message.

Agreed, trimmed.

> This paragraph could go to the cover letter.

Agreed, moved the lock-audit-trace paragraph there.

> +	/*
> +	 * Protects concurrent access to the chip's registers and state.
> +	 * Also shared as sd.state_lock and hdl.lock (see
> [...]
>
> This doesn't hold anymore, does it? Typically drivers do without such
> locks as the subdev state lock is used instead -- assigning your own lock
> also has the effect the same lock is used for try states, too.

You're right, and on reflection sharing it was the wrong call, not just
an under-explained one. it6625_lock is an MCU/register lock: the
chip's internal MCU firmware processes one register-driven command at
a time, and some commands are multi-step I2C transactions (write a
config register, then poll REG_HOST_CTRL_INT/B_CONFIG_UPDATE via
it6625_wait_for_status()) that must not be interleaved with another
such command. That's a real, separate constraint, but it isn't a
reason to also make it the subdev state lock or the control handler's
lock -- neither the pad format nor the control handler is chip state.

Dropped both aliasing assignments (sd->state_lock, hdl->lock) and let
the core give the subdev state and the control handler their own
independent locks, like most drivers do. it6625_lock stays exactly
what it always was: a private mutex taken explicitly around MCU/
register access. That did mean re-auditing every caller that used to
reach the active pad format under the implicit
state_lock == it6625_lock assumption -- it6625_initial_setup(),
it6625_clear_timings(), it6625_log_status(), and
it6625_update_timings_if_changed() now take the active state's lock
explicitly (the last two aren't core-locked to begin with:
VIDIOC_LOG_STATUS and VIDIOC_SUBDEV_S_DV_TIMINGS aren't in
subdev_ioctl_get_state()'s switch in v4l2-subdev.c), nesting
it6625_lock inside where they also touch it6625->timings. The rule
going forward: a subdev state lock, when held, is always the outer
lock; it6625_lock nests inside it, never the reverse.

> Could you rework the code to keep this for active and try paths?

Agreed. Moved the ACTIVE-only hardware programming into its own guarded
block and let both TRY and ACTIVE fall through to one shared commit tail
instead of duplicating it.

> There are a few lines longer than 80; please split unless there's a
> tangible reason to do otherwise.

Agreed. Hoisted the repeated it6625_formats[idx].csi_format lookup into
a local variable, which gets the it6625_set_mipi_config_locked() call
(and the neighboring comment) under 80 columns without an awkward wrap.

> +	if (active) {
> +		*fmt = *v4l2_subdev_state_get_format(active, 0);
> +		return 0;
> +	}
>
> This function should work the same way independently of whether the
> state is active or not.

Agreed. I was special-casing on whether sd->active_state existed yet, to
seed a new TRY state from the current active format instead of reverting
to boot defaults. Reworked it to always set the same defaults regardless
of which state it's initializing -- it6625->timings and the default
format index -- following the same unconditional-defaults pattern
hm1246_init_state() uses for the same active/TRY distinction. The
core already holds whichever state init_state() is initializing before
calling in, so reading it6625->timings under a nested it6625_lock (see
the locking reply above) is safe there too; a TRY state opened after
signal detection still reflects the live detected width/height/field,
and the code always starts from the default format index, same as
it6625_init_data() seeds it at probe.

> This comment is hardly useful; the same applies to pretty much all
> drivers using sub-device state and V4L2 controls.

Agreed, dropped it.

Kind regards,

Hermes

Re: [PATCH 20/21] media: i2c: it6625: use centrally managed active state

[Sakari Ailus]

Hi Hermes,

On Fri, Sep 18, 2026 at 11:19:38AM +0000, Hermes.Wu@ite.com.tw wrote:
> Dropped both aliasing assignments (sd->state_lock, hdl->lock) and let
> the core give the subdev state and the control handler their own
> independent locks, like most drivers do. it6625_lock stays exactly

Is this AI-generated?

Please use the control handler's lock as the state lock, too. This is what
most modern drivers do, too.

-- 
Regards,

Sakari Ailus