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

[Sakari Ailus]

Hi Hermes,

On Mon, Sep 21, 2026 at 02:24:29AM +0000, Hermes.Wu@xxxxxxxxxx wrote:
> Hi Sakari,
> 
> Thank you for the clarification.
> 
> > Is this AI-generated?
> 
> I used AI assistance to help understand this review comment and draft parts
> of patches 20/21 because I was not certain about the expected locking model.
> I reviewed the result before sending it, but my conclusion about the state
> lock was incorrect. I should have asked for clarification instead of making
> the claim about what most drivers do. Sorry about that.
> 
> > Please use the control handler's lock as the state lock, too. This is
> > what most modern drivers do, too.
> 
> Do you mean that I should leave the control handler using its own internal
> lock and assign that lock to the subdev state before calling
> v4l2_subdev_init_finalize(), as follows?
> 
> 	sd->state_lock = sd->ctrl_handler->lock;

Correct.

> 
> I am also still unsure about the separate it6625_lock. The chip's MCU
> processes one register-driven command at a time, and operations such as
> issuing a command followed by polling for completion must not be
> interleaved. This lock also serializes CEC, IRQ/polling, EDID and debugfs
> paths that do not run under the control/state lock.

I'm not asking for that, just that you'd use an existing lock instead of
adding one more.

> 
> Should it6625_lock therefore remain as a separate MCU transaction lock, with
> the control/state lock acquired first whenever both locks are needed?

Sounds good to me.

-- 
Regards,

Sakari Ailus