Re: [PATCH v6 2/9] media: subdev: Add set_pad_by_ep() callback to internal ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Severe layering violation by tightly coupling the core Media Controller framework with V4L2-specific firmware parsing.
- [Low] Typo in the callback documentation.
--

commit 3e6e9fb5a2a23ceb5cd559adeba28603dd1f9e0f
Author: Frank Li <Frank.Li@nxp.com>

media: subdev: Add set_pad_by_ep() callback to internal ops
    
This commit adds a set_pad_by_ep() callback to v4l2_subdev_internal_ops.
The callback is invoked while parsing firmware node endpoints and allows
subdevice drivers to configure media pad properties based on endpoint
information.

> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d256b7ec8f848..eb652eb76d33f 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -935,6 +935,10 @@ struct v4l2_subdev_ops {
>   *	the v4l2_subdev structure. It is almost certainly required for any
>   *	sub-device that sets the V4L2_SUBDEV_FL_HAS_DEVNODE flag.
>   *
> + * @set_pad_by_ep: Set pad informaiton by fwnode endpoint, parsed fwnode already

[Severity: Low]
This isn't a bug, but there is a typo in information (informaiton).

> + *		   saved into pad->vep. return < 0 means can't support this type
> + *		   endpoint. Set pad->flags according to pad->vep information.
> + *
>   * .. note::
>   *	Never call this from drivers, only the v4l2 framework can call
>   *	these ops.
> @@ -947,6 +951,7 @@ struct v4l2_subdev_internal_ops {
>  	int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
>  	int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
>  	void (*release)(struct v4l2_subdev *sd);
> +	int (*set_pad_by_ep)(struct v4l2_subdev *sd, struct media_pad *pad);

[Severity: Medium]
Does this callback signature enforce an architectural layering violation?

By omitting the vep argument, the callback design expects the parsed V4L2
endpoint configuration to be passed via pad->vep. struct media_pad is part of
the generic Media Controller API, which is shared across various subsystems
like ALSA and DVB.

Embedding the V4L2-specific struct v4l2_fwnode_endpoint directly into the
generic struct media_pad creates an inappropriate subsystem dependency and
wastes memory for all non-V4L2 media pads.

Could the V4L2-specific endpoint be passed directly as an argument to the
callback, such as:
int (*set_pad_by_ep)(struct v4l2_subdev *sd, struct media_pad *pad,
                     struct v4l2_fwnode_endpoint *vep);

>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-imx8qxp_pcam-v6-0-4b3f45920d2f@nxp.com?part=2