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

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

[Hermes Wu via B4 Relay]

Review feedback came in on "[PATCH v10 2/2] media: i2c: add driver for
ITE IT6625/IT6626" and, separately, on "[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: moving the current format
   out of driver-private fields and into the pad format, sharing the
   control handler's own lock as the subdev state lock instead of
   introducing another private mutex for it, keeping the existing
   it6625_lock independent as the lock for serialized hardware/register
   operations and driver-private timing state (kept separate chiefly
   for the MCU host interface's own multi-step transactions), 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 was refined
during v1's review -- see patch 20's and patch 21's commit messages in
this revision for the current design (the control handler's own lock
shared as the subdev state lock, it6625_lock kept independent for
serialized hardware/register operations and driver-private timing
state), isolated to those two commits and kept separate from the
mechanical/bug-fix commits that precede them.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
Changes in v2:
- Patch 02: propagate it6625_initial_setup()'s register-write errors
  and it6625_v4l2_sd_ctrl_update()'s control errors out of probe()
  instead of discarding them; probe() now fails via dev_err_probe() on
  either
- Patch 12: use HZ_PER_KHZ (kHz to Hz) instead of the wrongly-named
  KHZ_PER_MHZ -- both are numerically 1000, so this has no functional
  effect (found by sashiko.dev)
- Patch 13: define the detected-timings register-layout structs at file
  scope, immediately above the function that uses them, instead of as
  local variables
- Patch 17: demote -EPROBE_DEFER to dev_dbg() with dev_err_probe()
  instead of logging it as an error on every retried probe (found by
  sashiko.dev)
- Patch 19: add an err_clean_ctrl_handler label instead of freeing the
  control handler inline on a media_entity_pads_init() failure
- Patch 20: share the control handler's own lock as the subdev state lock
  (sd->state_lock = sd->ctrl_handler->lock) instead of a separate shared
  mutex for both, keep it6625_lock as an independent
  MCU/register-transaction lock, and take the active state's lock
  explicitly at every call site that reaches it outside a core-locked
  path (probe-time setup, an IRQ callback, and the two ioctls the core
  doesn't pre-lock a state for). Make it6625_init_state() seed every
  state -- active and TRY alike -- from the current it6625->timings and
  the default format index (so a TRY state opened after signal detection
  still reflects live detected timings, not frozen boot defaults) instead
  of branching on whether the active state exists yet, and unify the
  TRY/ACTIVE commit tail in it6625_set_fmt(). Trim the commit message and
  move the lock-interleaving trace out of it.
- Patch 21: take it6625_lock explicitly in it6625_enable_streams()/
  it6625_disable_streams(), since the core-held state lock is no longer
  the same mutex after patch 20's locking-model change.
- Rebased onto current origin/next (b38d06ad1e1 -> 2dcdfb625c3);
  it6625_set_fmt() now takes the unused const struct
  v4l2_subdev_client_info *ci parameter added by commit 7eef49c16461
  ("media: v4l2-subdev: Add struct v4l2_subdev_client_info pointer to
  pad ops")
- Link to v1: https://lore.kernel.org/r/20260918-upstream-it6625-follow-up-patch-v1-0-78d72d7886a5@ite.com.tw

---
Hermes Wu (21):
      dt-bindings: media: ite,it6625: document the default CSI-2 bus type
      media: i2c: it6625: propagate initial-setup and 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                         | 567 ++++++++++++---------
 2 files changed, 318 insertions(+), 251 deletions(-)
---
base-commit: 2dcdfb625c3b8fe87454e19dfbc54b3e3f0ad70e
change-id: 20260917-upstream-it6625-follow-up-patch-b81b34266c43

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

[PATCH v2 01/21] dt-bindings: media: 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>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
---
 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 v2 02/21] media: i2c: it6625: propagate initial-setup and control-update errors

[Hermes Wu via B4 Relay]

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

it6625_initial_setup() discarded every register write's return value
internally, and probe() logged but did not propagate a failed
it6625_v4l2_sd_ctrl_update() call; either way, probe() always
registered the subdev regardless of failure.

Make both functions return int, propagate the first error, and fail
probe() with dev_err_probe() when either one fails.

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 | 64 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 25f9e61a7b03ff228ed61a99c41248036035881f..485e1667c7bea34b152ff3dde09a928f334d71aa 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)
@@ -940,9 +948,10 @@ static inline unsigned int fps_from_bt_timings(const struct v4l2_bt_timings *t)
 				 V4L2_DV_BT_FRAME_WIDTH(t));
 }
 
-static void it6625_initial_setup(struct it6625 *it6625)
+static int it6625_initial_setup(struct it6625 *it6625)
 {
 	int val = 0;
+	int err;
 
 	guard(mutex)(&it6625->it6625_lock);
 
@@ -968,13 +977,28 @@ static void it6625_initial_setup(struct it6625 *it6625)
 	if (it6625->port_num == 2)
 		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_CONTROL, 0x00);
-	it6625_write_byte(it6625, REG_RX_CFG, 0x00);
+	err = it6625_write_byte(it6625, REG_MIPI_CFG, val);
+	if (err)
+		return err;
+
+	err = it6625_write_byte(it6625, REG_MIPI_DATA_TYPE, it6625->csi_format);
+	if (err)
+		return err;
 
-	it6625_set_bits(it6625, REG_HOST_CTRL_INT, B_CONFIG_UPDATE, B_CONFIG_UPDATE);
-	it6625_wait_for_status(it6625, REG_HOST_CTRL_INT, 0x00, 25);
+	err = it6625_write_byte(it6625, REG_MIPI_CONTROL, 0x00);
+	if (err)
+		return err;
+
+	err = it6625_write_byte(it6625, REG_RX_CFG, 0x00);
+	if (err)
+		return err;
+
+	err = it6625_set_bits(it6625, REG_HOST_CTRL_INT, B_CONFIG_UPDATE,
+			      B_CONFIG_UPDATE);
+	if (err)
+		return err;
+
+	return it6625_wait_for_status(it6625, REG_HOST_CTRL_INT, 0x00, 25);
 }
 
 static int it6625_cec_adap_enable(struct cec_adapter *adap, bool enable)
@@ -1136,9 +1160,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,8 +2324,17 @@ 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_initial_setup(it6625);
+	if (err) {
+		dev_err_probe(it6625->dev, err, "failed initial hardware setup");
+		goto err_clean_debugfs;
+	}
+
+	err = it6625_v4l2_sd_ctrl_update(sd);
+	if (err) {
+		dev_err_probe(it6625->dev, err, "failed to update controls");
+		goto err_clean_debugfs;
+	}
 
 	err = v4l2_async_register_subdev(sd);
 	if (err < 0) {

-- 
2.34.1

[PATCH v2 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 485e1667c7bea34b152ff3dde09a928f334d71aa..7cc2ba0d26b8dbc439807e350434137f102d9d9f 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 v2 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 7cc2ba0d26b8dbc439807e350434137f102d9d9f..b09b331b20364a8e169429bb309d1112ec83d919 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 v2 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 b09b331b20364a8e169429bb309d1112ec83d919..b747c08e092c7b35994cb36bb5bf7b99495282cb 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 v2 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 b747c08e092c7b35994cb36bb5bf7b99495282cb..9382a64706919d380e8318c3139b42a4a7b2fd55 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;
 	}
@@ -1895,11 +1891,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 v2 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 9382a64706919d380e8318c3139b42a4a7b2fd55..b48252a71bc1ed788de2df79c5fdaf7895f6e0c4 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 v2 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 b48252a71bc1ed788de2df79c5fdaf7895f6e0c4..0fe828cc5aff6d61957c3f0a781033dc94b7754b 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 v2 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 0fe828cc5aff6d61957c3f0a781033dc94b7754b..8d368fd5a64cd815c7a9013c863804c32eee7fc1 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);
@@ -996,10 +996,8 @@ static int 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));
 
@@ -1025,7 +1023,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 v2 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 8d368fd5a64cd815c7a9013c863804c32eee7fc1..7bf008a3ac311c09e17ef7dc25adbed893d25137 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 v2 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 7bf008a3ac311c09e17ef7dc25adbed893d25137..b72b1fdc8f6b96c807bf767b82a953a89b08120a 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 v2 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 HZ_PER_KHZ to convert the kHz register
value to the Hz value V4L2 DV timings expect. 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 b72b1fdc8f6b96c807bf767b82a953a89b08120a..f77eb3ed63f7695cf9da277b46d9bab4a526e065 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 * HZ_PER_KHZ;
 }
 
 static int it6625_read_edid(struct it6625 *it6625, u8 *edid, int start_block,

-- 
2.34.1

[PATCH v2 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 named
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 at file
scope, immediately above the function that uses them -- this driver
accesses many such register areas, so keep the layout struct separate
from its one caller instead of declaring it locally. 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 | 48 ++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index f77eb3ed63f7695cf9da277b46d9bab4a526e065..7582c12c268e1878f15c0f3c6030aacf8e78a5a4 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -765,14 +765,33 @@ static void it6625_hpd_delayed_work(struct work_struct *work)
 	it6625_update_config(it6625);
 }
 
+/* REG_H_ACTIVE_1..REG_V_ACTIVE_0 */
+struct it6625_active_size_regs {
+	__be16 h_active;
+	__be16 v_active;
+};
+
+static_assert(sizeof(struct it6625_active_size_regs) == 4);
+
+/* REG_H_FP_1..REG_V_BP_0 */
+struct it6625_porch_regs {
+	__be16 hfrontporch;
+	__be16 hsync;
+	__be16 hbackporch;
+	__be16 vfrontporch;
+	__be16 vsync;
+	__be16 vbackporch;
+};
+
+static_assert(sizeof(struct it6625_porch_regs) == 12);
+
 static int it6625_get_detected_timings(struct it6625 *it6625,
 				       struct v4l2_dv_timings *timings)
 {
 	struct v4l2_bt_timings *bt = &timings->bt;
+	struct it6625_active_size_regs active;
+	struct it6625_porch_regs porch;
 	int val;
-	unsigned int width, height;
-	u8 buffer[4];
-	u8 buffer2[12];
 
 	if (no_signal(it6625)) {
 		dev_err(it6625->dev, "no signal detected");
@@ -792,24 +811,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 v2 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.

Fixes: 142e5f00bd57 ("media: i2c: add driver for ITE IT6625/IT6626")
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 7582c12c268e1878f15c0f3c6030aacf8e78a5a4..cb201dea540a0ad0953b55bef5ea6d8e8f16d5b1 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,
 };
 
@@ -1874,6 +1881,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 =
@@ -1887,10 +1896,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 v2 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 cb201dea540a0ad0953b55bef5ea6d8e8f16d5b1..c972a74dd5d63655d531579cfd4e8dc3f72cb7ae 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -1454,20 +1454,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 v2 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 c972a74dd5d63655d531579cfd4e8dc3f72cb7ae..7637418c5a214d32f75d197a1e0ba1bffe7235f9 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -1366,26 +1366,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");
@@ -1399,13 +1385,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 v2 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 | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 7637418c5a214d32f75d197a1e0ba1bffe7235f9..01200acb6bda5cdd759caf70642fcd910284a8b8 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -2074,9 +2074,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;
@@ -2122,33 +2119,24 @@ 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) {
-		dev_err(dev, "failed to parse endpoint: %d", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to parse endpoint");
 
 	if (endpoint.bus_type != V4L2_MBUS_CSI2_DPHY &&
 	    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;
@@ -2158,15 +2146,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 v2 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 01200acb6bda5cdd759caf70642fcd910284a8b8..bc01e87490eeb478b33bf82f77652d6a2c5610cb 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) {
@@ -1648,8 +1648,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;
@@ -1729,8 +1729,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 v2 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 bc01e87490eeb478b33bf82f77652d6a2c5610cb..c3361727311d1ebbed072d9e7d25967231121cf5 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -2164,33 +2164,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] = {
@@ -2276,9 +2249,23 @@ 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);
+		goto err_clean_ctrl_handler;
+	}
 
 	err = v4l2_ctrl_handler_setup(sd->ctrl_handler);
 	if (err)
@@ -2337,6 +2324,7 @@ static int it6625_probe(struct i2c_client *client)
 	cec_unregister_adapter(it6625->cec_adap);
 err_clean_hdl:
 	media_entity_cleanup(&sd->entity);
+err_clean_ctrl_handler:
 	v4l2_ctrl_handler_free(&it6625->hdl);
 
 err_clean_work_queues:

-- 
2.34.1

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

[Hermes Wu via B4 Relay]

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

Store the negotiated media-bus format in the V4L2 subdev state instead
of duplicating it in driver-private fields. This keeps ACTIVE and TRY
formats under the framework's state management and keeps the active
format synchronized with the configured DV timings.

Use the control handler's lock as the subdev state lock. Keep
it6625_lock independent for MCU transactions.

Initialize every state from the current DV timings and default format
index, use the core get_fmt implementation, and update the active format
whenever the configured timings change.

Make ACTIVE set_fmt reject changes while streaming and commit the new
format only after hardware programming succeeds. TRY changes remain
state-only and do not program hardware.

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

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index c3361727311d1ebbed072d9e7d25967231121cf5..1127d050d82522e202f042832e3772fd88e63ca9 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -255,7 +255,7 @@ struct it6625 {
 	struct regmap *it6625_regmap;
 	enum it6625_chip_type chip_type;
 
-	/* protects concurrent access to the chip's registers and state */
+	/* Serializes MCU transactions. */
 	struct mutex it6625_lock;
 	/* serializes the complete VIDIOC_S_EDID sequence against itself */
 	struct mutex edid_lock;
@@ -291,8 +291,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;
 
@@ -918,10 +916,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);
 
@@ -929,27 +928,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)
@@ -964,9 +970,20 @@ static inline unsigned int fps_from_bt_timings(const struct v4l2_bt_timings *t)
 
 static int it6625_initial_setup(struct it6625 *it6625)
 {
+	struct v4l2_subdev *sd = &it6625->sd;
+	struct v4l2_subdev_state *state;
+	struct v4l2_mbus_framefmt *fmt;
+	int idx;
 	int val = 0;
 	int err;
 
+	state = v4l2_subdev_lock_and_get_active_state(sd);
+	fmt = v4l2_subdev_state_get_format(state, 0);
+	idx = it6625_csi_mbus_code_idx(fmt->code);
+	if (idx < 0)
+		idx = 0;
+	v4l2_subdev_unlock_state(state);
+
 	guard(mutex)(&it6625->it6625_lock);
 
 	/*
@@ -995,7 +1012,8 @@ static int it6625_initial_setup(struct it6625 *it6625)
 	if (err)
 		return err;
 
-	err = it6625_write_byte(it6625, REG_MIPI_DATA_TYPE, it6625->csi_format);
+	err = it6625_write_byte(it6625, REG_MIPI_DATA_TYPE,
+				it6625_formats[idx].csi_format);
 	if (err)
 		return err;
 
@@ -1163,10 +1181,32 @@ static void it6625_get_timings(struct it6625 *it6625,
 	*timings = it6625->timings;
 }
 
+/*
+ * Project a DV-timings struct's width/height/field onto a pad format.
+ * Caller must hold it6625_lock and, separately, whichever subdev
+ * state fmt belongs to.
+ */
+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)
 {
-	guard(mutex)(&it6625->it6625_lock);
-	memset(&it6625->timings, 0, sizeof(it6625->timings));
+	struct v4l2_subdev *sd = &it6625->sd;
+	struct v4l2_subdev_state *state = v4l2_subdev_lock_and_get_active_state(sd);
+	struct v4l2_mbus_framefmt *fmt = v4l2_subdev_state_get_format(state, 0);
+
+	scoped_guard(mutex, &it6625->it6625_lock) {
+		memset(&it6625->timings, 0, sizeof(it6625->timings));
+		it6625_fill_timings_format(&it6625->timings, fmt);
+	}
+
+	v4l2_subdev_unlock_state(state);
 }
 
 static void it6625_irq_hdmi_5v_change(struct it6625 *it6625)
@@ -1369,6 +1409,7 @@ static void it6625_polling_work(struct work_struct *work)
 static int it6625_log_status(struct v4l2_subdev *sd)
 {
 	struct it6625 *it6625 = sd_to_6625(sd);
+	struct v4l2_subdev_state *state;
 	struct v4l2_dv_timings timings, configured_timings;
 	struct v4l2_bt_timings bt;
 	u32 mbus_fmt_code;
@@ -1383,11 +1424,17 @@ static int it6625_log_status(struct v4l2_subdev *sd)
 	v4l2_print_dv_timings(sd->name, "Configured format: ",
 			      &configured_timings, true);
 
-	/* snapshot together so the reported pair was actually configured together */
+	/*
+	 * VIDIOC_LOG_STATUS isn't core-locked, so take both locks
+	 * ourselves; snapshot together so the reported pair was
+	 * actually configured together.
+	 */
+	state = v4l2_subdev_lock_and_get_active_state(sd);
 	scoped_guard(mutex, &it6625->it6625_lock) {
-		mbus_fmt_code = it6625->mbus_fmt_code;
+		mbus_fmt_code = v4l2_subdev_state_get_format(state, 0)->code;
 		bt = it6625->timings.bt;
 	}
+	v4l2_subdev_unlock_state(state);
 
 	v4l2_info(sd, "CSI format: %#x @ %uHz", mbus_fmt_code, fps_from_bt_timings(&bt));
 
@@ -1438,17 +1485,32 @@ static int
 it6625_update_timings_if_changed(struct it6625 *it6625,
 				 const struct v4l2_dv_timings *timings)
 {
-	guard(mutex)(&it6625->it6625_lock);
+	struct v4l2_subdev *sd = &it6625->sd;
+	struct v4l2_subdev_state *state;
+	struct v4l2_mbus_framefmt *fmt;
+	int ret;
 
-	if (v4l2_match_dv_timings(&it6625->timings, timings, 0, false))
-		return 0;
+	/* .s_dv_timings isn't core-locked, so take both locks ourselves */
+	state = v4l2_subdev_lock_and_get_active_state(sd);
+	fmt = v4l2_subdev_state_get_format(state, 0);
 
-	if (!v4l2_valid_dv_timings(timings, it6625_get_timings_cap(it6625), NULL, NULL))
-		return -ERANGE;
+	scoped_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;
+			it6625_fill_timings_format(&it6625->timings, fmt);
+			ret = 1;
+		}
+	}
 
-	it6625->timings = *timings;
+	v4l2_subdev_unlock_state(state);
 
-	return 1;
+	return ret;
 }
 
 static int it6625_enum_dv_timings(struct v4l2_subdev *sd,
@@ -1480,8 +1542,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,
@@ -1609,85 +1670,59 @@ static inline u32 format_to_colorspace(u8 csi_format)
 	}
 }
 
-static int it6625_get_fmt(struct v4l2_subdev *sd,
+static int it6625_set_fmt(struct v4l2_subdev *sd,
+			  const struct v4l2_subdev_client_info *ci,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *format)
 {
 	struct it6625 *it6625 = sd_to_6625(sd);
-	struct v4l2_dv_timings timings;
+	struct v4l2_mbus_framefmt *fmt;
+	u8 csi_format;
+	int idx;
+	int ret;
 
 	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;
-		}
+	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__, format->format.code);
+		idx = 0;
 	}
 
-	return 0;
-}
+	csi_format = it6625_formats[idx].csi_format;
 
-static int it6625_set_fmt(struct v4l2_subdev *sd,
-			  const struct v4l2_subdev_client_info *ci,
-			  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;
-	int ret;
+	/* fmt already carries width/height/field for this state; leave alone */
+	fmt = v4l2_subdev_state_get_format(sd_state, format->pad);
 
-	ret = it6625_get_fmt(sd, sd_state, format);
-	format->format.code = mbus_fmt_code;
+	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+		if (v4l2_subdev_is_streaming(sd))
+			return -EBUSY;
 
-	if (ret)
-		return ret;
+		guard(mutex)(&it6625->it6625_lock);
 
-	ret = it6625_csi_mbus_code_idx(mbus_fmt_code);
+		ret = it6625_enable_stream_locked(it6625, false);
+		if (ret)
+			return ret;
 
-	if (ret < 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;
+		ret = it6625_set_mipi_config_locked(it6625, csi_format);
+		if (ret)
+			return ret;
 	}
 
-	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		struct v4l2_mbus_framefmt *fmt;
+	/*
+	 * TRY: commit unconditionally. ACTIVE: commit only after the
+	 * hardware programming above actually succeeded.
+	 */
+	fmt->code = it6625_formats[idx].mbus_fmt_code;
+	fmt->colorspace = format_to_colorspace(csi_format);
+	format->format = *fmt;
 
-		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;
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
 		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);
-	}
-
-	format->format.colorspace = format_to_colorspace(it6625_formats[ret].csi_format);
 
 	return 0;
 }
@@ -1804,7 +1839,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,
@@ -1824,8 +1859,12 @@ 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_mbus_framefmt *fmt = v4l2_subdev_state_get_format(sd_state, 0);
 
+	scoped_guard(mutex, &it6625->it6625_lock)
+		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);
 
@@ -1866,6 +1905,7 @@ static int it6625_v4l2_init_controls(struct v4l2_subdev *sd)
 			   it6625->csi_lanes == 3;
 
 	v4l2_ctrl_handler_init(hdl, 4);
+
 	it6625->ctrl_5v_detect =
 		v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_DV_RX_POWER_PRESENT,
 				  0, 1, 0, 0);
@@ -2074,8 +2114,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;
@@ -2267,9 +2305,16 @@ static int it6625_probe(struct i2c_client *client)
 		goto err_clean_ctrl_handler;
 	}
 
+	sd->state_lock = sd->ctrl_handler->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),
@@ -2280,7 +2325,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);
@@ -2288,7 +2333,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);
@@ -2322,6 +2367,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);
 err_clean_ctrl_handler:
@@ -2359,12 +2406,15 @@ static void it6625_remove(struct i2c_client *client)
 
 	debugfs_remove_recursive(it6625->debugfs_dir);
 	cec_unregister_adapter(it6625->cec_adap);
+
+	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 v2 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.

Both ops take it6625_lock explicitly around the MCU transactions. This
device has a single, non-multiplexed source pad, so the core's implicit
stream 0 is sufficient and V4L2_SUBDEV_FL_STREAMS is not set.

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

diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
index 1127d050d82522e202f042832e3772fd88e63ca9..10619df51a2274a4deb155710d6aa46c950f10a9 100644
--- a/drivers/media/i2c/it6625.c
+++ b/drivers/media/i2c/it6625.c
@@ -935,12 +935,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;
@@ -1538,11 +1532,24 @@ 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(it6625, enable);
+	guard(mutex)(&it6625->it6625_lock);
+	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);
+
+	guard(mutex)(&it6625->it6625_lock);
+	return it6625_enable_stream_locked(it6625, false);
 }
 
 static int it6625_enum_mbus_code(struct v4l2_subdev *sd,
@@ -1833,7 +1840,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 = {
@@ -1848,6 +1855,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 v2 05/21] media: i2c: it6625: drop stale GCC < 4.4.6 workaround

[Hans Verkuil]

On 30/09/2026 08:41, Hermes Wu via B4 Relay wrote:
> 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.

FYI: I'm dropping this patch. It might be OK against the oldest gcc version,
but for the oldest llvm version this is still needed.

Regards,

	Hans

> 
> 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 b09b331b20364a8e169429bb309d1112ec83d919..b747c08e092c7b35994cb36bb5bf7b99495282cb 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 |
>