Re: [PATCH v7 11/14] media: v4l2-subdev: Add v4l2_subdev_call_ci_state_{active,try}

Hi Hans, Laurent,

On Mon, Aug 10, 2026 at 06:32:07PM +0300, Laurent Pinchart wrote:
> On Mon, Aug 10, 2026 at 04:12:31PM +0200, Hans Verkuil wrote:
> > On 07/08/2026 14:24, Sakari Ailus wrote:
> > > Add v4l2_subdev_call_ci_state_active(), and
> > > v4l2_subdev_call_ci_state_try() to call sub-device pad ops that
> > > take struct v4l2_subdev_client_info pointer as an argument. These ops
> > > cannot be called using v4l2_subdev_call_state_active() or
> > > v4l2_subdev_call_state_try() as the client_info argument precedes the
> > > state argument.
> > 
> > So if we have to jump through all these hoops just because the client_info
> > pointer precedes the state pointer in the pad op argument list, wouldn't it
> > be better to swap the order? For example by moving the client_info pointer
> > as the last argument?
> > 
> > Honestly, these macros are getting really hard to follow, and I'm not sure
> > it is worth it just to keep the client_info before the state pointer. Yes, that's
> > the logical order, but at the price of some very hard to read defines.
> > 
> > Or am I missing something?
> 
> Those are exactly the points I raised in the review of v6 :-)

And my answer then was that I prefer some additional complexity on the
framework side -- where we have a single implementation of this -- over
pushing less than ideal APIs to all the drivers.

To give some idea, we currently have about 200 drivers implementing the
set_fmt() pad op. I also expect the number of pad ops where we need client
info will grow over time, making this quite a bit more visible in the
drivers than the scope (set_fmt, get_selection and set_selection)
introduced in this set.

-- 
Kind regards,

Sakari Ailus