Hi Jacopo,
There we go - looks like a viable solution to me.
I wonder/suspect if this is the issue the BSP team were having some
weeks ago too?
Anyway, I'm certainly happy with this.
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
On 16/04/2020 11:40, Jacopo Mondi wrote:
> Calculate the CSI-2 transmitter pixel rate using the one reported from
> sources.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> ---
> drivers/media/i2c/max9286.c | 50 +++++++++++++++++++++++++++++--------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 2d71205a1aad..3ef74ba10074 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -430,9 +430,46 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> * V4L2 Subdev
> */
>
> -static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
> +static int max9286_set_pixelrate(struct max9286_priv *priv)
> {
> - return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate);
> + struct max9286_source *source = NULL;
> + u64 pixelrate = 0;
> +
> + for_each_source(priv, source) {
> + struct v4l2_ctrl *ctrl;
> + u64 source_rate = 0;
> +
> + /* Pixel rate is mandatory to be reported by sources. */
> + ctrl = v4l2_ctrl_find(source->sd->ctrl_handler,
> + V4L2_CID_PIXEL_RATE);
> + if (!ctrl) {
> + pixelrate = 0;
> + break;
> + }
> +
> + /* All source must report the same pixel rate. */
> + source_rate = v4l2_ctrl_g_ctrl_int64(ctrl);
> + if (!pixelrate) {
> + pixelrate = source_rate;
> + } else if (pixelrate != source_rate) {
> + dev_err(&priv->client->dev,
> + "Unable to calculate pixel rate\n");
Hrm ... I would report "Source pixel rates are different\n"
... but I don't think that's a big deal.
> + return -EINVAL;
> + }
> + }
> +
> + if (!pixelrate) {
> + dev_err(&priv->client->dev,
> + "No pixel rate control available in sources\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * The CSI-2 transmitter pixel rate is the single source rate multiplied
> + * by the number of available sources.
> + */
> + return v4l2_ctrl_s_ctrl_int64(priv->pixelrate,
> + pixelrate * priv->nsources);
> }
>
> static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> @@ -496,7 +533,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> */
> max9286_configure_i2c(priv, false);
>
> - return 0;
> + return max9286_set_pixelrate(priv);
Aha - at first I thought this would call for every bind event, but we
already filter so this tail end of the function only executes on the
last bind :-)
> }
>
> static void max9286_notify_unbind(struct v4l2_async_notifier *notifier,
> @@ -670,7 +707,6 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> {
> struct max9286_priv *priv = sd_to_max9286(sd);
> struct v4l2_mbus_framefmt *cfg_fmt;
> - s64 pixelrate;
>
> if (format->pad >= MAX9286_SRC_PAD)
> return -EINVAL;
> @@ -695,12 +731,6 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> *cfg_fmt = format->format;
> mutex_unlock(&priv->mutex);
>
> - /* Update pixel rate for the CSI2 receiver */
> - pixelrate = cfg_fmt->width * cfg_fmt->height
> - * priv->nsources * 30 /*FPS*/;
> -
> - max9286_set_pixelrate(priv, pixelrate);
> -
> return 0;
> }
>
>
