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

[Kate Hsuan]

Hi Hans,

Thank you for providing the answer

On Tue, Mar 17, 2026 at 8:25 PM Hans de Goede
<johannes.goede@xxxxxxxxxxxxxxxx> 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.
>
> 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.

I'll change the name of it.

>
> Regards,
>
> Hans
>
>


-- 
BR,
Kate