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

[Hans de Goede <johannes.goede@oss.qualcomm.com>]

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.

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 ?

<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.

Regards,

Hans