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