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