Re: [PATCH v3 4/4] media: platform: Add Renesas RZ/G2L CRU driver

[Sakari Ailus]

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