| Message ID | 20200518161159.2185855-1-kieran.bingham+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | fixes! [max9286]: Validate link formats | expand |
Hi Kieran On 18/05/2020 17:11, 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 > + * 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) > + pad = ffs(priv->bound_sources); of course this would have to be (ffs(priv->bound_sources) - 1) as the first pad index is 0 ;-) > > - 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; > >
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 >
On Mon, May 18, 2020 at 07:44:41PM +0100, Kieran Bingham wrote: > Hi Kieran > > On 18/05/2020 17:11, 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 > > + * 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) > > + pad = ffs(priv->bound_sources); > > of course this would have to be (ffs(priv->bound_sources) - 1) as the > first pad index is 0 ;-) You could be clever and use __ffs() instead. :-) > > > > > - 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; > > > > >
Hi Jacopo, On 19/05/2020 08:56, Jacopo Mondi wrote: > 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 to much time playing in doxygen ;-) > /* > * 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 Sure ;-) > >> + 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. > I'd prefer to reference the actual first source, rather than duplicating as that's the link that we're validating against. > Please squash in v10 Thanks, v10 soon!. Just need to determine the best route for the remaining dt cleanups. -- Kieran > Thanks > j > >> if (!cfg_fmt) >> return -EINVAL; >> >> -- >> 2.25.1 >>
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 + * 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) + 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;
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(-)