[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y1hkG6NVFS08WTIg@paasikivi.fi.intel.com>
Date: Tue, 25 Oct 2022 22:32:59 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Jacopo Mondi <jacopo@...ndi.org>,
Niklas Söderlund
<niklas.soderlund@...natech.se>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
Geert Uytterhoeven <geert+renesas@...der.be>,
linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
Biju Das <biju.das.jz@...renesas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v3 4/4] media: platform: Add Renesas RZ/G2L CRU driver
Hi Prabhakar,
A few comments below... apologies for not reviewing this earlier. Looks
good in general but there are a few points that need some attention.
On Wed, Oct 05, 2022 at 12:43:43AM +0100, Prabhakar wrote:
...
> +static int rzg2l_cru_ip_pre_streamon(struct v4l2_subdev *sd, u32 flags)
> +{
> + struct rzg2l_cru_dev *cru;
> + int ret;
> +
> + cru = v4l2_get_subdevdata(sd);
> +
> + if (!cru->is_csi)
> + return -EINVAL;
> +
> + ret = v4l2_subdev_call(cru->ip.remote, video, pre_streamon, 0);
If you're calling pre_streamon successfully, you'll have to have an
equivalent number of post_streamoff calls.
...
> +static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
> +{
> + struct media_pipeline *pipe;
> + struct v4l2_subdev *sd;
> + struct media_pad *pad;
> + unsigned long flags;
> + int ret;
> +
> + pad = media_pad_remote_pad_first(&cru->pad);
> + if (!pad)
> + return -EPIPE;
> +
> + sd = media_entity_to_v4l2_subdev(pad->entity);
> +
> + if (!on) {
> + media_pipeline_stop(&cru->vdev.entity);
> + return v4l2_subdev_call(sd, video, s_stream, 0);
Ditto.
> + }
> +
> + ret = rzg2l_cru_mc_validate_format(cru, sd, pad);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_subdev_call(sd, video, pre_streamon, 0);
> + if (ret == -ENOIOCTLCMD)
> + ret = 0;
> + if (ret)
> + return ret;
For all cases below where streaming on doesn't succeed, you'll have to have
a call of post_streamoff.
> +
> + spin_lock_irqsave(&cru->qlock, flags);
> +
> + /* Select a video input */
> + if (cru->is_csi)
> + rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
> +
> + /* Cancel the software reset for image processing block */
> + rzg2l_cru_write(cru, CRUnRST, CRUnRST_VRESETN);
> +
> + /* Disable and clear the interrupt before using */
> + rzg2l_cru_write(cru, CRUnIE, 0);
> + rzg2l_cru_write(cru, CRUnINTS, 0x001f000f);
> +
> + /* Initialize the AXI master */
> + rzg2l_cru_initialize_axi(cru);
> +
> + /* Initialize image convert */
> + ret = rzg2l_cru_initialize_image_conv(cru);
> + if (ret) {
> + spin_unlock_irqrestore(&cru->qlock, flags);
> + return ret;
> + }
> +
> + /* Enable interrupt */
> + rzg2l_cru_write(cru, CRUnIE, CRUnIE_EFE);
> +
> + /* Enable image processing reception */
> + rzg2l_cru_write(cru, ICnEN, ICnEN_ICEN);
> +
> + spin_unlock_irqrestore(&cru->qlock, flags);
> +
> + pipe = sd->entity.pipe ? sd->entity.pipe : &cru->vdev.pipe;
> + ret = media_pipeline_start(&cru->vdev.entity, pipe);
> + if (ret)
> + return ret;
> +
> + clk_disable_unprepare(cru->vclk);
> +
> + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> + if (ret == -ENOIOCTLCMD)
> + ret = 0;
> + if (ret) {
> + /* enable back vclk so that release() disables it */
> + clk_prepare_enable(cru->vclk);
> + media_pipeline_stop(&cru->vdev.entity);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(cru->vclk);
What will happen if enabling vclk will fail here? (Or above?)
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
--
Kind regards,
Sakari Ailus
Powered by blists - more mailing lists