Hi Laurent,
On 27/11/2023 10:07, Laurent Pinchart wrote:
> The subdev .init_cfg() operation is affected by two issues:
>
> - It has long been extended to initialize a whole v4l2_subdev_state
> instead of just a v4l2_subdev_pad_config, but its name has stuck
> around.
>
> - Despite operating on a whole subdev state and not being directly
> exposed to the subdev users (either in-kernel or through the userspace
> API), .init_cfg() is categorized as a subdev pad operation.
>
> This participates in making the subdev API confusing for new developers.
> Fix it by renaming the operation to .init_state(), and make it a subdev
> internal operation.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Acked-by: Michael Riesch <michael.riesch@xxxxxxxxxxxxxx> # for imx415
> Acked-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx> # for vimc
> ---
> Changes since v3:
>
> - Rebase on top of the new stm32-dcmipp driver
> - Fix uninitialized variable in __v4l2_subdev_state_alloc() due to bad
> rebase
>
> Changes since v2:
>
> - Rebase on top of the latest media tree
>
> Changes since v1:
>
> - Fix compilation of the vsp1 driver
> ---
<snip>
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.c b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> index 0280b8ff7ba9..0a5a7f9cc870 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> @@ -170,33 +170,6 @@ vsp1_entity_get_pad_selection(struct vsp1_entity *entity,
> }
> }
>
> -/*
> - * vsp1_entity_init_cfg - Initialize formats on all pads
> - * @subdev: V4L2 subdevice
> - * @sd_state: V4L2 subdev state
> - *
> - * Initialize all pad formats with default values in the given subdev state.
> - * This function can be used as a handler for the subdev pad::init_cfg
> - * operation.
> - */
> -int vsp1_entity_init_cfg(struct v4l2_subdev *subdev,
> - struct v4l2_subdev_state *sd_state)
> -{
> - unsigned int pad;
> -
> - for (pad = 0; pad < subdev->entity.num_pads - 1; ++pad) {
> - struct v4l2_subdev_format format = {
> - .pad = pad,
> - .which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
> - : V4L2_SUBDEV_FORMAT_ACTIVE,
> - };
> -
> - v4l2_subdev_call(subdev, pad, set_fmt, sd_state, &format);
> - }
> -
> - return 0;
> -}
> -
> /*
> * vsp1_subdev_get_pad_format - Subdev pad get_fmt handler
> * @subdev: V4L2 subdevice
> @@ -424,6 +397,29 @@ int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev,
> return ret;
> }
>
> +static int vsp1_entity_init_state(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_state *sd_state)
> +{
> + unsigned int pad;
> +
> + /* Initialize all pad formats with default values. */
> + for (pad = 0; pad < subdev->entity.num_pads - 1; ++pad) {
> + struct v4l2_subdev_format format = {
> + .pad = pad,
> + .which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
> + : V4L2_SUBDEV_FORMAT_ACTIVE,
> + };
> +
> + v4l2_subdev_call(subdev, pad, set_fmt, sd_state, &format);
> + }
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops vsp1_entity_internal_ops = {
> + .init_state = vsp1_entity_init_state,
> +};
> +
> /* -----------------------------------------------------------------------------
> * Media Operations
> */
> @@ -658,6 +654,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
> /* Initialize the V4L2 subdev. */
> subdev = &entity->subdev;
> v4l2_subdev_init(subdev, ops);
> + subdev->internal_ops = &vsp1_entity_internal_ops;
>
> subdev->entity.function = function;
> subdev->entity.ops = &vsp1->media_ops;
> @@ -666,7 +663,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
> snprintf(subdev->name, sizeof(subdev->name), "%s %s",
> dev_name(vsp1->dev), name);
>
> - vsp1_entity_init_cfg(subdev, NULL);
> + vsp1_entity_init_state(subdev, NULL);
This is the only media driver that calls init_cfg/state directly.
That's a bit unexpected, and perhaps this could use a comment. That
can be a follow-up patch as well.
>
> /*
> * Allocate the subdev state to store formats and selection
In any case, you can add my:
Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
to this patch.
Regards,
Hans
