diff mbox series

[v9.2,2/9] fixes! [max9286]: Validate link formats

Message ID 20200610124623.51085-3-kieran@bingham.xyz (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series GMSL fixups ready for v10. | expand

Commit Message

Kieran Bingham June 10, 2020, 12:46 p.m. UTC 
From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

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 | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

Jacopo Mondi June 10, 2020, 3:02 p.m. UTC | #1 
Hi Kieran,
  a few small nits.

The patch is fine, feel free to squash it in v10.

On Wed, Jun 10, 2020 at 01:46:16PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> 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 | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index ef824d2b26b8..7e59391a5736 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -711,7 +711,11 @@ 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 Stream Support: Prevent setting the format on the shared
> +	 * multiplexed bus.
> +	 */

I am not sure I would mention multiplexed stream support, and this is
not a TODO item. Simply, max9286 does not allow any format conversion
on its source pad, so the format is propagated from one of its sink to
the source (assuming all sinks have the same format applied).

Quite a common thing, isn't it ?

> +	if (format->pad == MAX9286_SRC_PAD)
>  		return -EINVAL;
>
>  	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
> @@ -743,11 +747,18 @@ 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)

I was about to comment we'ra nowe allowing pads > MAX9286_SRC_PAD, but the
core actually checks that for us.

> -		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

nit: is the comma in 'mixing,' intentional ?
Same comment about multiplexed stream support in comment.

Theoretically, a set_fmt on a sink should propagate the format to the
source pad, and you could access it through
priv->fmts[MAX9286_SRC_PAD] and drop this check.

The result is actually the same, so it's up to you.

Thanks
  j

> +	 * the max9286.
> +	 */
> +	if (pad == MAX9286_SRC_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);
>  	if (!cfg_fmt)
>  		return -EINVAL;
>
> --
> 2.25.1
>
Kieran Bingham June 12, 2020, 1:22 p.m. UTC | #2 
Hi Jacopo,

On 10/06/2020 16:02, Jacopo Mondi wrote:
> Hi Kieran,
>   a few small nits.
> 
> The patch is fine, feel free to squash it in v10.

Thanks,

> 
> On Wed, Jun 10, 2020 at 01:46:16PM +0100, Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> 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 | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index ef824d2b26b8..7e59391a5736 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -711,7 +711,11 @@ 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 Stream Support: Prevent setting the format on the shared
>> +	 * multiplexed bus.
>> +	 */
> 
> I am not sure I would mention multiplexed stream support, and this is
> not a TODO item. Simply, max9286 does not allow any format conversion
> on its source pad, so the format is propagated from one of its sink to
> the source (assuming all sinks have the same format applied).
> 
> Quite a common thing, isn't it ?

Ok - I'll just drop the comment then.


> 
>> +	if (format->pad == MAX9286_SRC_PAD)
>>  		return -EINVAL;
>>
>>  	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
>> @@ -743,11 +747,18 @@ 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)
> 
> I was about to comment we'ra nowe allowing pads > MAX9286_SRC_PAD, but the
> core actually checks that for us.
> 
>> -		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
> 
> nit: is the comma in 'mixing,' intentional ?

No, it should be removed.

> Same comment about multiplexed stream support in comment.
> 
> Theoretically, a set_fmt on a sink should propagate the format to the
> source pad, and you could access it through
> priv->fmts[MAX9286_SRC_PAD] and drop this check.
> 
> The result is actually the same, so it's up to you.

I'll stick with what we've got then, and just remove those comments, if
you think they get in the way.



> 
> Thanks
>   j
> 
>> +	 * the max9286.
>> +	 */
>> +	if (pad == MAX9286_SRC_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);
>>  	if (!cfg_fmt)
>>  		return -EINVAL;
>>
>> --
>> 2.25.1
>>
diff mbox series

Patch

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index ef824d2b26b8..7e59391a5736 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -711,7 +711,11 @@  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 Stream Support: 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 +747,18 @@  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 (pad == MAX9286_SRC_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);
 	if (!cfg_fmt)
 		return -EINVAL;