Re: [PATCH v2 25/30] adv748x: csi2: only allow formats on sink pads

[Jacopo Mondi]

Hi Niklas,

On Fri, Nov 02, 2018 at 12:31:39AM +0100, Niklas Söderlund wrote:
> Once the CSI-2 subdevice of the ADV748X becomes aware of multiplexed
> streams the format of the source pad is of no value as it carries
> multiple streams. Prepare for this by explicitly denying setting a
> format on anything but the sink pad.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c 
> b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 8a7cc713c7adfcc1..9f2c49221a8ddebc 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -163,6 +163,9 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
>       struct adv748x_state *state = tx->state;
>       struct v4l2_mbus_framefmt *mbusformat;
>
> +     if (sdformat->pad != ADV748X_CSI2_SINK)
> +             return -EINVAL;
> +
>       mbusformat = adv748x_csi2_get_pad_format(sd, cfg, sdformat->pad,
>                                                sdformat->which);
>       if (!mbusformat)
> @@ -186,6 +189,9 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>       struct v4l2_mbus_framefmt *mbusformat;
>       int ret = 0;
>
> +     if (sdformat->pad != ADV748X_CSI2_SINK)
> +             return -EINVAL;
> +

The adv748x set_format on the source pad already ignores the provided
format argument and just replicates the format applied on the sink on
itself. I agree this change makes sense, but then you should remove
the big if switch a few lines here below.

        if (sdformat->pad == ADV748X_CSI2_SOURCE) {

With your ack, I can make this change in the forthcoming v3.

Thanks
   j

PS: I'm thinking of sending the v4l2-mux core patches (01->23 of v2)
separate from adv748x and rcar-csi2 ones, to ease review, and maybe
speed up inclusion of the core changes. What would your (and Sakari's
and Laurent's) opinion on that?

>       mbusformat = adv748x_csi2_get_pad_format(sd, cfg, sdformat->pad,
>                                                sdformat->which);
>       if (!mbusformat)
> --
> 2.19.1
>

signature.asc
Description: PGP signature