Re: [PATCH v11] media: Add t4ka3 camera sensor driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

On Tue, Mar 17, 2026 at 01:24:57PM +0100, Hans de Goede wrote:
> Hi Sakari,
> 
> On 16-Mar-26 23:00, Sakari Ailus wrote:
> 
> <snip>
> 
> >> diff --git a/drivers/media/i2c/t4ka3.c b/drivers/media/i2c/t4ka3.c
> >> new file mode 100644
> >> index 000000000000..d9af5e51f7a8
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/t4ka3.c
> 
> <snip>
> 
> >> +static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
> >> +{
> >> +	struct v4l2_subdev_state *active_state =
> >> +		v4l2_subdev_get_locked_active_state(&sensor->sd);
> >> +
> >> +	return v4l2_subdev_state_get_format(active_state, 0);
> >> +}
> >> +
> >> +static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
> >> +{
> >> +	struct v4l2_subdev_state *active_state =
> >> +		v4l2_subdev_get_locked_active_state(&sensor->sd);
> >> +
> >> +	return v4l2_subdev_state_get_crop(active_state, 0);
> > 
> > Please avoid adding such helpers.
> 
> The problem is that we need to know the active-fmt/-crop in some places
> without access to it. E.g. when the vblank ctrl gets set this influences
> the range of the exposure control, so we need active_fmt.height to
> calculate the values to pass to v4l2_ctrl_modify_range() and we need
> this from a v4l2_ctrl_ops.s_ctrl callback which does not get passed
> in the (active) fmt.
> 
> Since the ctrl lock is used as the main sensor-driver lock too, 
> we can always safely call v4l2_subdev_get_locked_active_state()
> in these cases, since we are always holding the lock.

I replied to Kate's e-mail already, but indeed you should get the active
state if you really need the active state. In a lot of cases it's "any"
state which is already available to the caller of these functions.

> 
> The alternative would be to store a copy of the active fmt/crop
> inside struct t4ka3_data, but I thought that the whole direction
> for sensor drivers was to stop having (and needing to update) their
> own shadow copy of the active_state and instead direct use
> the active_state ?

Correct.

> 
> <snip>
> 
> >> +static int t4ka3_s_ctrl(struct v4l2_ctrl *ctrl)
> >> +{
> >> +	struct t4ka3_data *sensor = ctrl_to_t4ka3(ctrl);
> >> +	struct v4l2_mbus_framefmt *fmt;
> >> +	int ret;
> >> +
> >> +	/* Update exposure range on vblank changes */
> >> +	if (ctrl->id == V4L2_CID_VBLANK) {
> >> +		ret = t4ka3_update_exposure_range(sensor);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	fmt = t4ka3_get_active_format(sensor);
> > 
> > You could assign this in declaration.
> > 
> >> +
> >> +	/* Only apply changes to the controls if the device is powered up */
> >> +	if (!pm_runtime_get_if_in_use(sensor->sd.dev)) {
> >> +		t4ka3_set_bayer_order(sensor, fmt);
> > 
> > Does this call belong here?
> 
> Yes, if the hflip/vflip controls change then fmt->code needs to be
> updated to the now changed bayer-order. t4ka3_set_bayer_order()
> uses the cached ctrl->val values so it is cheap enough to
> always do this instead of checking if the changed ctrl is
> vflip or hflip.
> 
> In case the sensor is actually streaming and we don't hit this path,
> the t4ka3_t_vflip()helper will return -EBUSY since changing
> the active fmt while streaming is not a good idea.
> 
> Looking at this again, I do think that: t4ka3_t_vflip() should
> be renamed to t4ka3_update_hvflip() because the current name
> is weird.

This should also apply to non-active state (albeit implementation requires
implementing get_fmt locally). This will get "fixed" with the metadata
series eventually with generic raw formats.

-- 
Kind regards,

Sakari Ailus