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

[Hans de Goede]

Hi Sakari,

On 17-Mar-26 18:53, Sakari Ailus wrote:

...

>>>> +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.
>> As Hans mentioned, we can put active format and crop in the t4ka3_data
>> or keep the helpers.
> 
> What prevents you doing:
> 
> 	struct v4l2_subdev_state *active_state =
> 		v4l2_subdev_get_locked_active_state(&sensor->sd);
> 	struct v4l2_rect *r = v4l2_subdev_state_get_crop(active_state, 0);
> 
> in the code? In a lot of the cases you could simply pass the state to the
> function using it as the caller already has it.

Ok, I've done a quick audit of the code and it indeed seems that
in some cases, especially in t4ka3_set_pad_format() these helpers
are used unnecessary (and even buggy in case of the crop in
t4ka3_set_pad_format()).

So I agree it is probably better to avoid these and in functions
where we have a sd_state pass the result of:

v4l2_subdev_state_get_format(sd_state, [0|sel->pad])

and:

v4l2_subdev_state_get_crop(sd_state, [0|sel->pad])

to helpers which currently rely on t4ka3_get_active_[format|crop]()
such as t4ka3_calc_mode().

E.g. all callers of t4ka3_calc_mode() already get sd_state passed
in; and both callers also already both should get crop and fmt
from the sd_state (t4ka3_set_pad_format() wrongly uses the
functions to get the active fmt/crop for this).

So we can simply pass the already retrieved fmt + crop
into t4ka3_calc_mode().

As for other calles of t4ka3_get_active_[format|crop]() if
they really do not have sd_state access then lets just
write out the code as suggested by Sakari.

Kate, let me know if you need any help with this.

Regards,

Hans