Re: [PATCH] fixes! [max9286]: Validate link formats

From: Jacopo Mondi <jacopo@jmondi.org>
To: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: sakari.ailus@iki.fi, linux-media@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH] fixes! [max9286]: Validate link formats
Date: Tue, 19 May 2020 09:56:01 +0200	[thread overview]
Message-ID: <20200519075601.wdykflbxgvqvfl3x@uno.localdomain> (raw)
In-Reply-To: <20200518161159.2185855-1-kieran.bingham+renesas@ideasonboard.com>

Hi Kieran,

On Mon, May 18, 2020 at 05:11:59PM +0100, Kieran Bingham wrote:
> Disallow setting a format on the source link, but enable link validation
> by returning the format of the first bound source when getting the
> format of the source pad.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index ef824d2b26b8..6c01595936d7 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -711,7 +711,10 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  	struct max9286_priv *priv = sd_to_max9286(sd);
>  	struct v4l2_mbus_framefmt *cfg_fmt;
>
> -	if (format->pad >= MAX9286_SRC_PAD)
> +	/* \todo: Multiplexed streams support

I know where \todo comes from, but it's usually TODO.
Also
        /*
         * TODO:

if the comment is multiline

> +	 * Prevent setting the format on the shared multiplexed bus.
> +	 */
> +	if (format->pad == MAX9286_SRC_PAD)
>  		return -EINVAL;
>
>  	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
> @@ -743,11 +746,17 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
>  	struct v4l2_mbus_framefmt *cfg_fmt;
> +	unsigned int pad = format->pad;
>
> -	if (format->pad >= MAX9286_SRC_PAD)
> -		return -EINVAL;
> +	/* \todo: Multiplexed Stream Support
> +	 * Support link validation by returning the format of the first bound
> +	 * link. All links must have the same format, as we do not support
> +	 * mixing, and matching of cameras connected to the max9286.
> +	 */
> +	if (format->pad == MAX9286_SRC_PAD)

You can use pad

> +		pad = ffs(priv->bound_sources);
>
> -	cfg_fmt = max9286_get_pad_format(priv, cfg, format->pad, format->which);
> +	cfg_fmt = max9286_get_pad_format(priv, cfg, pad, format->which);

Or you could add one entry to struct max9286_priv.fmt for the source
pad format, set the format there when is set on one sink, and just
drop the check on the pad index in get_fmt. Up to you.

Please squash in v10

Thanks
  j

>  	if (!cfg_fmt)
>  		return -EINVAL;
>
> --
> 2.25.1
>