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