On Wed Sep 30 14:41:38 2026 +0800, Hermes Wu wrote:
> 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 <[email protected]>
> Signed-off-by: Hans Verkuil <[email protected]>
Patch committed.
Thanks,
Hans Verkuil
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 5f118d7fbb59..9900239b5474 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;
@@ -921,10 +919,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);
@@ -932,27 +931,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)
@@ -967,9 +973,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);
/*
@@ -998,7 +1015,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;
@@ -1166,10 +1184,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)
@@ -1372,6 +1412,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;
@@ -1386,11 +1427,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));
@@ -1441,17 +1488,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,
@@ -1483,8 +1545,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,
@@ -1612,85 +1673,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;
}
@@ -1807,7 +1842,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,
@@ -1827,8 +1862,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);
@@ -1869,6 +1908,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);
@@ -2077,8 +2117,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;
@@ -2270,9 +2308,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),
@@ -2283,7 +2328,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);
@@ -2291,7 +2336,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);
@@ -2325,6 +2370,8 @@ err_clean_debugfs:
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:
@@ -2362,12 +2409,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[] = {
_______________________________________________
linuxtv-commits mailing list -- [email protected]
To unsubscribe send an email to [email protected]
