On 07/04/2022 09:23, Tomi Valkeinen wrote:
> On 06/04/2022 16:51, Hans Verkuil wrote:
>>
>>
>> On 24/03/2022 09:00, Tomi Valkeinen wrote:
>>> Add v4l2_subdev_get_fmt() helper function which implements
>>> v4l2_subdev_pad_ops.get_fmt using active state. Subdev drivers that
>>> support active state and do not need to do anything special in their
>>> get_fmt op can use this helper directly for v4l2_subdev_pad_ops.get_fmt.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
>>> ---
>>> drivers/media/v4l2-core/v4l2-subdev.c | 18 ++++++++++++++++++
>>> include/media/v4l2-subdev.h | 17 +++++++++++++++++
>>> 2 files changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>> index d8d1c9ef4dc4..cbc5ebff0656 100644
>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>> @@ -1029,6 +1029,24 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
>>> }
>>> EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
>>> +int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
>>> + struct v4l2_subdev_format *format)
>>> +{
>>> + struct v4l2_mbus_framefmt *fmt;
>>> +
>>> + if (format->pad >= sd->entity.num_pads)
>>> + return -EINVAL;
>>> +
>>> + fmt = v4l2_subdev_get_try_format(sd, state, format->pad);
>>
>> Let's use the new v4l2_subdev_get_pad_format helper here.
>
> Right.
>
>>> + if (!fmt)
>>> + return -EINVAL;
>>> +
>>> + format->format = *fmt;
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_fmt);
>>> +
>>> #endif /* CONFIG_MEDIA_CONTROLLER */
>>> void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>> index 700ce376b22c..491bdbb1670c 100644
>>> --- a/include/media/v4l2-subdev.h
>>> +++ b/include/media/v4l2-subdev.h
>>> @@ -1300,6 +1300,23 @@ v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
>>> return sd->active_state;
>>> }
>>> +/**
>>> + * v4l2_subdev_get_fmt() - Fill format based on state
>>> + * @sd: subdevice
>>> + * @state: subdevice state
>>> + * @format: pointer to &struct v4l2_subdev_format
>>> + *
>>> + * Fill @format->format field based on the information in the @format struct.
>>> + *
>>> + * This function can be used by the subdev drivers which support active state to
>>> + * implement v4l2_subdev_pad_ops.get_fmt if the subdev driver does not need to
>>> + * do anything special in their get_fmt op.
>>> + *
>>> + * Returns 0 on success, error value otherwise.
>>> + */
>>> +int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
>>> + struct v4l2_subdev_format *format);
>>
>> My main concern here is the function name: perhaps a prefix like
>> v4l2_subdev_pad_op_get_fmt (or perhaps just _op_ without 'pad') makes
>> it easier to see that this is a pad op helper.
>
> The function can and will be used also in other places. E.g. driver's set_fmt may use it to allow only setting the sink pad:
>
> if (format->pad == MY_SOURCE_PAD)
> return v4l2_subdev_get_fmt(sd, state, format);
>
> That's perhaps the only other use for the function, as it takes struct v4l2_subdev_format as a parameter, and I think usually that struct is only used with the set_fmt/get_fmt pad ops.
>
> So, I guess "v4l2_subdev_pad_op_get_fmt" is suitable even in the example use case above. Then again, we do have other similar helper funcs without any extra prefixes, e.g. media_entity_operations:
>
> .get_fwnode_pad = v4l2_subdev_get_fwnode_pad_1_to_1,
> .link_validate = v4l2_subdev_link_validate,
> .has_route = v4l2_subdev_has_route
I personally think those should be renamed as well. But it is no big deal either.
If a helper is specifically meant to be used as an op function, then it should
be reflected in the name IMHO. If it can be used elsewhere as well, then it
becomes a gray area. In this particular case I am fine either way.
Regards,
Hans
>
> Tomi
