Re: [PATCH v11 03/13] media: staging/imx7: add imx7 CSI subdev driver

Hi Rui,

A few more comments below.

On Thu, Jan 24, 2019 at 04:09:18PM +0000, Rui Miguel Silva wrote:

This is something that has been the problem in the imx driver --- you can't
free the control handler while it's exposed to the user space.

It's already listed in the TODO file though.

+		csi->sink = NULL;
+	}
+
+init:
+	if (csi->sink || csi->src_sd)
+		imx7_csi_init(csi);
+	else
+		imx7_csi_deinit(csi);
+
+unlock:
+	mutex_unlock(&csi->lock);
+
+	return ret;
+}
+
+static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
+				      struct media_link *link,
+				      struct v4l2_subdev_format *source_fmt,
+				      struct v4l2_subdev_format *sink_fmt)
+{
+	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+	struct v4l2_fwnode_endpoint upstream_ep = {};
+	int ret;
+
+	ret = v4l2_subdev_link_validate_default(sd, link, source_fmt, sink_fmt);
+	if (ret)
+		return ret;
+
+	ret = imx7_csi_get_upstream_endpoint(csi, &upstream_ep, true);
+	if (ret) {
+		v4l2_err(&csi->sd, "failed to find upstream endpoint\n");
+		return ret;
+	}
+
+	mutex_lock(&csi->lock);
+
+	csi->upstream_ep = upstream_ep;
+	csi->is_csi2 = (upstream_ep.bus_type == V4L2_MBUS_CSI2_DPHY);
+
+	mutex_unlock(&csi->lock);
+
+	return 0;
+}
+
+static void imx7_csi_update_buf(struct imx7_csi *csi, dma_addr_t phys,
+				int buf_num)
+{
+	if (buf_num == 1)
+		imx7_csi_reg_write(csi, phys, CSI_CSIDMASA_FB2);
+	else
+		imx7_csi_reg_write(csi, phys, CSI_CSIDMASA_FB1);
+}
+
+static void imx7_csi_setup_vb2_buf(struct imx7_csi *csi)
+{
+	struct imx_media_video_dev *vdev = csi->vdev;
+	struct imx_media_buffer *buf;
+	struct vb2_buffer *vb2_buf;
+	dma_addr_t phys[2];
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		buf = imx_media_capture_device_next_buf(vdev);
+		if (buf) {
+			csi->active_vb2_buf[i] = buf;
+			vb2_buf = &buf->vbuf.vb2_buf;
+			phys[i] = vb2_dma_contig_plane_dma_addr(vb2_buf, 0);
+		} else {
+			csi->active_vb2_buf[i] = NULL;
+			phys[i] = csi->underrun_buf.phys;
+		}
+
+		imx7_csi_update_buf(csi, phys[i], i);
+	}
+}
+
+static void imx7_csi_dma_unsetup_vb2_buf(struct imx7_csi *csi,
+					 enum vb2_buffer_state return_status)
+{
+	struct imx_media_buffer *buf;
+	int i;
+
+	/* return any remaining active frames with return_status */
+	for (i = 0; i < 2; i++) {
+		buf = csi->active_vb2_buf[i];
+		if (buf) {
+			struct vb2_buffer *vb = &buf->vbuf.vb2_buf;
+
+			vb->timestamp = ktime_get_ns();
+			vb2_buffer_done(vb, return_status);
+		}
+	}
+}
+
+static void imx7_csi_vb2_buf_done(struct imx7_csi *csi)
+{
+	struct imx_media_video_dev *vdev = csi->vdev;
+	struct imx_media_buffer *done, *next;
+	struct vb2_buffer *vb;
+	dma_addr_t phys;
+
+	done = csi->active_vb2_buf[csi->buf_num];
+	if (done) {
+		done->vbuf.field = vdev->fmt.fmt.pix.field;
+		done->vbuf.sequence = csi->frame_sequence;
+		vb = &done->vbuf.vb2_buf;
+		vb->timestamp = ktime_get_ns();
+		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
+	}
+	csi->frame_sequence++;
+
+	/* get next queued buffer */
+	next = imx_media_capture_device_next_buf(vdev);
+	if (next) {
+		phys = vb2_dma_contig_plane_dma_addr(&next->vbuf.vb2_buf, 0);
+		csi->active_vb2_buf[csi->buf_num] = next;
+	} else {
+		phys = csi->underrun_buf.phys;
+		csi->active_vb2_buf[csi->buf_num] = NULL;
+	}
+
+	imx7_csi_update_buf(csi, phys, csi->buf_num);
+}
+
+static irqreturn_t imx7_csi_irq_handler(int irq, void *data)
+{
+	struct imx7_csi *csi =  data;
+	unsigned long status;
+
+	spin_lock(&csi->irqlock);
+
+	status = imx7_csi_irq_clear(csi);
+
+	if (status & BIT_RFF_OR_INT) {
+		dev_warn(csi->dev, "Rx fifo overflow\n");
+		imx7_csi_error_recovery(csi);
+	}
+
+	if (status & BIT_HRESP_ERR_INT) {
+		dev_warn(csi->dev, "Hresponse error detected\n");
+		imx7_csi_error_recovery(csi);
+	}
+
+	if (status & BIT_ADDR_CH_ERR_INT) {
+		imx7_csi_hw_disable(csi);
+
+		imx7_csi_dma_reflash(csi);
+
+		imx7_csi_hw_enable(csi);
+	}
+
+	if ((status & BIT_DMA_TSF_DONE_FB1) &&
+	    (status & BIT_DMA_TSF_DONE_FB2)) {
+		/*
+		 * For both FB1 and FB2 interrupter bits set case,
+		 * CSI DMA is work in one of FB1 and FB2 buffer,
+		 * but software can not know the state.
+		 * Skip it to avoid base address updated
+		 * when csi work in field0 and field1 will write to
+		 * new base address.
+		 */
+	} else if (status & BIT_DMA_TSF_DONE_FB1) {
+		csi->buf_num = 0;
+	} else if (status & BIT_DMA_TSF_DONE_FB2) {
+		csi->buf_num = 1;
+	}
+
+	if ((status & BIT_DMA_TSF_DONE_FB1) ||
+	    (status & BIT_DMA_TSF_DONE_FB2)) {
+		imx7_csi_vb2_buf_done(csi);
+
+		if (csi->last_eof) {
+			complete(&csi->last_eof_completion);
+			csi->last_eof = false;
+		}
+	}
+
+	spin_unlock(&csi->irqlock);
+
+	return IRQ_HANDLED;
+}
+
+static int imx7_csi_dma_start(struct imx7_csi *csi)
+{
+	struct imx_media_video_dev *vdev = csi->vdev;
+	struct v4l2_pix_format *out_pix = &vdev->fmt.fmt.pix;
+	int ret;
+
+	ret = imx_media_alloc_dma_buf(csi->md, &csi->underrun_buf,
+				      out_pix->sizeimage);
+	if (ret < 0) {
+		v4l2_warn(&csi->sd, "consider increasing the CMA area\n");
+		return ret;
+	}
+
+	csi->frame_sequence = 0;
+	csi->last_eof = false;
+	init_completion(&csi->last_eof_completion);
+
+	imx7_csi_setup_vb2_buf(csi);
+
+	return 0;
+}
+
+static void imx7_csi_dma_stop(struct imx7_csi *csi)
+{
+	unsigned long timeout_jiffies;
+	unsigned long flags;
+	int ret;
+
+	/* mark next EOF interrupt as the last before stream off */
+	spin_lock_irqsave(&csi->irqlock, flags);
+	csi->last_eof = true;
+	spin_unlock_irqrestore(&csi->irqlock, flags);
+
+	/*
+	 * and then wait for interrupt handler to mark completion.
+	 */
+	timeout_jiffies = msecs_to_jiffies(IMX_MEDIA_EOF_TIMEOUT);
+	ret = wait_for_completion_timeout(&csi->last_eof_completion,
+					  timeout_jiffies);
+	if (ret == 0)
+		v4l2_warn(&csi->sd, "wait last EOF timeout\n");
+
+	imx7_csi_hw_disable_irq(csi);
+
+	imx7_csi_dma_unsetup_vb2_buf(csi, VB2_BUF_STATE_ERROR);
+
+	imx_media_free_dma_buf(csi->md, &csi->underrun_buf);
+}
+
+static int imx7_csi_configure(struct imx7_csi *csi)
+{
+	struct imx_media_video_dev *vdev = csi->vdev;
+	struct v4l2_pix_format *out_pix = &vdev->fmt.fmt.pix;
+	__u32 in_code = csi->format_mbus[IMX7_CSI_PAD_SINK].code;
+	u32 cr1, cr18;
+
+	if (out_pix->field == V4L2_FIELD_INTERLACED) {
+		imx7_csi_deinterlace_enable(csi, true);
+		imx7_csi_buf_stride_set(csi, out_pix->width);
+	} else {
+		imx7_csi_deinterlace_enable(csi, false);
+		imx7_csi_buf_stride_set(csi, 0);
+	}
+
+	imx7_csi_set_imagpara(csi, out_pix->width, out_pix->height);
+
+	if (!csi->is_csi2)
+		return 0;
+
+	cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
+	cr1 &= ~BIT_GCLK_MODE;
+
+	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
+	cr18 &= BIT_MIPI_DATA_FORMAT_MASK;
+	cr18 |= BIT_DATA_FROM_MIPI;
+
+	switch (out_pix->pixelformat) {
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_YUYV:
+		cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
+		break;
+	case V4L2_PIX_FMT_SBGGR8:
+		cr18 |= BIT_MIPI_DATA_FORMAT_RAW8;
+		break;
+	case V4L2_PIX_FMT_SBGGR16:
+		if (in_code == MEDIA_BUS_FMT_SBGGR10_1X10)
+			cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
+		else if (in_code == MEDIA_BUS_FMT_SBGGR12_1X12)
+			cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
+		else if (in_code == MEDIA_BUS_FMT_SBGGR14_1X14)
+			cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
+		cr1 |= BIT_PIXEL_BIT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
+	imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
+
+	return 0;
+}
+
+static int imx7_csi_enable(struct imx7_csi *csi)
+{
+	imx7_csi_sw_reset(csi);
+
+	if (csi->is_csi2) {
+		imx7_csi_dmareq_rff_enable(csi);
+		imx7_csi_hw_enable_irq(csi);
+		imx7_csi_hw_enable(csi);
+		return 0;
+	}
+
+	return 0;
+}
+
+static void imx7_csi_disable(struct imx7_csi *csi)
+{
+	imx7_csi_dmareq_rff_disable(csi);
+
+	imx7_csi_hw_disable_irq(csi);
+
+	imx7_csi_buf_stride_set(csi, 0);
+
+	imx7_csi_hw_disable(csi);
+}
+
+static int imx7_csi_streaming_start(struct imx7_csi *csi)
+{
+	int ret;
+
+	ret = imx7_csi_dma_start(csi);
+	if (ret < 0)
+		return ret;
+
+	ret = imx7_csi_configure(csi);
+	if (ret < 0)
+		goto dma_stop;
+
+	imx7_csi_enable(csi);
+
+	return 0;
+
+dma_stop:
+	imx7_csi_dma_stop(csi);
+
+	return ret;
+}
+
+static int imx7_csi_streaming_stop(struct imx7_csi *csi)
+{
+	imx7_csi_dma_stop(csi);
+
+	imx7_csi_disable(csi);
+
+	return 0;
+}
+
+static int imx7_csi_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+	int ret = 0;
+
+	mutex_lock(&csi->lock);
+
+	if (!csi->src_sd || !csi->sink) {
+		ret = -EPIPE;
+		goto out_unlock;
+	}
+
+	if (csi->is_streaming == !!enable)
+		goto out_unlock;
+
+	if (enable) {
+		ret = v4l2_subdev_call(csi->src_sd, video, s_stream, 1);
+		if (ret < 0)
+			goto out_unlock;
+
+		ret = imx7_csi_streaming_start(csi);
+		if (ret < 0) {
+			v4l2_subdev_call(csi->src_sd, video, s_stream, 0);
+			goto out_unlock;
+		}
+	} else {
+		imx7_csi_streaming_stop(csi);
+
+		v4l2_subdev_call(csi->src_sd, video, s_stream, 0);
+	}
+
+	csi->is_streaming = !!enable;
+
+out_unlock:
+	mutex_unlock(&csi->lock);
+
+	return ret;
+}
+
+static struct v4l2_mbus_framefmt *imx7_csi_get_format(struct imx7_csi *csi,
+					struct v4l2_subdev_pad_config *cfg,
+					unsigned int pad,
+					enum v4l2_subdev_format_whence which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		return v4l2_subdev_get_try_format(&csi->sd, cfg, pad);
+
+	return &csi->format_mbus[pad];
+}
+
+static int imx7_csi_enum_mbus_code(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_pad_config *cfg,
+				   struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+	const struct imx_media_pixfmt *in_cc;
+	struct v4l2_mbus_framefmt *in_fmt;
+	int ret = 0;
+
+	mutex_lock(&csi->lock);
+
+	in_fmt = imx7_csi_get_format(csi, cfg, IMX7_CSI_PAD_SINK, code->which);
+
+	in_cc = imx_media_find_mbus_format(in_fmt->code, CS_SEL_ANY, true);
+
+	switch (code->pad) {
+	case IMX7_CSI_PAD_SINK:
+		ret = imx_media_enum_mbus_format(&code->code, code->index,
+						 CS_SEL_ANY, true);
+		break;
+	case IMX7_CSI_PAD_SRC:
+		if (code->index != 0) {
+			ret = -EINVAL;
+			goto out_unlock;
+		}
+
+		code->code = in_fmt->code;
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+out_unlock:
+	mutex_unlock(&csi->lock);
+
+	return ret;
+}
+
+static int imx7_csi_get_fmt(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_pad_config *cfg,
+			    struct v4l2_subdev_format *sdformat)
+{
+	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+	struct v4l2_mbus_framefmt *fmt;
+	int ret = 0;
+
+	mutex_lock(&csi->lock);
+
+	fmt = imx7_csi_get_format(csi, cfg, sdformat->pad, sdformat->which);
+	if (!fmt) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	sdformat->format = *fmt;
+
+out_unlock:
+	mutex_unlock(&csi->lock);
+
+	return ret;
+}
+
+static void imx7_csi_try_fmt(struct imx7_csi *csi,
+			     struct v4l2_subdev_pad_config *cfg,
+			     struct v4l2_subdev_format *sdformat,
+			     const struct imx_media_pixfmt **cc)
+{
+	const struct imx_media_pixfmt *in_cc;
+	struct v4l2_mbus_framefmt *in_fmt;
+	u32 code;
+
+	in_fmt = imx7_csi_get_format(csi, cfg, IMX7_CSI_PAD_SINK,
+				     sdformat->which);
+	if (!in_fmt)
+		return;
+
+	switch (sdformat->pad) {
+	case IMX7_CSI_PAD_SRC:
+		in_cc = imx_media_find_mbus_format(in_fmt->code, CS_SEL_ANY,
+						   true);
+
+		sdformat->format.width = in_fmt->width;
+		sdformat->format.height = in_fmt->height;
+		sdformat->format.code = in_fmt->code;
+		*cc = in_cc;
+
+		sdformat->format.colorspace = in_fmt->colorspace;
+		sdformat->format.xfer_func = in_fmt->xfer_func;
+		sdformat->format.quantization = in_fmt->quantization;
+		sdformat->format.ycbcr_enc = in_fmt->ycbcr_enc;
+		break;
+	case IMX7_CSI_PAD_SINK:
+		*cc = imx_media_find_mbus_format(sdformat->format.code,
+						 CS_SEL_ANY, true);
+		if (!*cc) {
+			imx_media_enum_mbus_format(&code, 0, CS_SEL_ANY, false);
+			*cc = imx_media_find_mbus_format(code, CS_SEL_ANY,
+							 false);
+			sdformat->format.code = (*cc)->codes[0];
+		}
+
+		imx_media_fill_default_mbus_fields(&sdformat->format, in_fmt,
+						   false);
+		break;
+	default:
+		break;
+	}
+}
+
+static int imx7_csi_set_fmt(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_pad_config *cfg,
+			    struct v4l2_subdev_format *sdformat)
+{
+	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+	struct imx_media_video_dev *vdev = csi->vdev;
+	const struct imx_media_pixfmt *outcc;
+	struct v4l2_mbus_framefmt *outfmt;
+	struct v4l2_pix_format vdev_fmt;
+	const struct imx_media_pixfmt *cc;
+	struct v4l2_mbus_framefmt *fmt;
+	struct v4l2_subdev_format format;
+	int ret = 0;
+
+	if (sdformat->pad >= IMX7_CSI_PADS_NUM)
+		return -EINVAL;
+
+	mutex_lock(&csi->lock);
+
+	if (csi->is_streaming) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	imx7_csi_try_fmt(csi, cfg, sdformat, &cc);
+
+	fmt = imx7_csi_get_format(csi, cfg, sdformat->pad, sdformat->which);
+	if (!fmt) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	*fmt = sdformat->format;
+
+	if (sdformat->pad == IMX7_CSI_PAD_SINK) {
+		/* propagate format to source pads */
+		format.pad = IMX7_CSI_PAD_SRC;
+		format.which = sdformat->which;
+		format.format = sdformat->format;
+		imx7_csi_try_fmt(csi, cfg, &format, &outcc);
+
+		outfmt = imx7_csi_get_format(csi, cfg, IMX7_CSI_PAD_SRC,
+					     sdformat->which);
+		*outfmt = format.format;
+
+		if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+			csi->cc[IMX7_CSI_PAD_SRC] = outcc;
+	}
+
+	if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY)
+		goto out_unlock;
+
+	csi->cc[sdformat->pad] = cc;
+
+	/* propagate output pad format to capture device */
+	imx_media_mbus_fmt_to_pix_fmt(&vdev_fmt,
+				      &csi->format_mbus[IMX7_CSI_PAD_SRC],
+				      csi->cc[IMX7_CSI_PAD_SRC]);
+	mutex_unlock(&csi->lock);
+	imx_media_capture_device_set_format(vdev, &vdev_fmt);

This is setting the V4L2 pixel format that is separate from the media bus
format set on the sub-device pad. No other driver does this to my knowledge
(apart from imx). This should be probably added to the TODO file.

+
+	return 0;
+
+out_unlock:
+	mutex_unlock(&csi->lock);
+
+	return ret;
+}
+
+static int imx7_csi_registered(struct v4l2_subdev *sd)
+{
+	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+	int ret;
+	int i;
+
+	csi->md = dev_get_drvdata(sd->v4l2_dev->dev);
+
+	for (i = 0; i < IMX7_CSI_PADS_NUM; i++) {
+		csi->pad[i].flags = (i == IMX7_CSI_PAD_SINK) ?
+			MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
+
+		/* set a default mbus format  */
+		ret = imx_media_init_mbus_fmt(&csi->format_mbus[i],
+					      800, 600, 0, V4L2_FIELD_NONE,
+					      &csi->cc[i]);
+		if (ret < 0)
+			return ret;
+
+		/* init default frame interval */
+		csi->frame_interval[i].numerator = 1;
+		csi->frame_interval[i].denominator = 30;
+	}
+
+	ret = media_entity_pads_init(&sd->entity, IMX7_CSI_PADS_NUM, csi->pad);
+	if (ret < 0)
+		return ret;
+
+	ret = imx_media_capture_device_register(csi->vdev);
+	if (ret < 0)
+		return ret;
+
+	ret = imx_media_add_video_device(csi->md, csi->vdev);
+	if (ret < 0) {
+		imx_media_capture_device_unregister(csi->vdev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void imx7_csi_unregistered(struct v4l2_subdev *sd)
+{
+	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+
+	imx_media_capture_device_unregister(csi->vdev);

You could call imx_media_capture_device_unregister() directly.

+}
+
+static int imx7_csi_init_cfg(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_pad_config *cfg)
+{
+	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+	struct v4l2_mbus_framefmt *mf;
+	int ret;
+	int i;
+
+	for (i = 0; i < IMX7_CSI_PADS_NUM; i++) {
+		mf = v4l2_subdev_get_try_format(sd, cfg, i);
+
+		ret = imx_media_init_mbus_fmt(mf, 800, 600, 0, V4L2_FIELD_NONE,
+					      &csi->cc[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct media_entity_operations imx7_csi_entity_ops = {
+	.link_setup	= imx7_csi_link_setup,
+	.link_validate	= v4l2_subdev_link_validate,
+};
+
+static const struct v4l2_subdev_video_ops imx7_csi_video_ops = {
+	.s_stream		= imx7_csi_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops imx7_csi_pad_ops = {
+	.init_cfg =		imx7_csi_init_cfg,
+	.enum_mbus_code =	imx7_csi_enum_mbus_code,
+	.get_fmt =		imx7_csi_get_fmt,
+	.set_fmt =		imx7_csi_set_fmt,
+	.link_validate =	imx7_csi_pad_link_validate,
+};
+
+static const struct v4l2_subdev_ops imx7_csi_subdev_ops = {
+	.video =	&imx7_csi_video_ops,
+	.pad =		&imx7_csi_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops imx7_csi_internal_ops = {
+	.registered	= imx7_csi_registered,
+	.unregistered	= imx7_csi_unregistered,
+};
+
+static int imx7_csi_parse_endpoint(struct device *dev,
+				   struct v4l2_fwnode_endpoint *vep,
+				   struct v4l2_async_subdev *asd)
+{
+	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
+}
+
+static int imx7_csi_clocks_get(struct imx7_csi *csi)
+{
+	struct device *dev = csi->dev;
+	int i;
+
+	csi->num_clks = ARRAY_SIZE(imx7_csi_clk_id);
+	csi->clks = devm_kcalloc(dev, csi->num_clks, sizeof(*csi->clks),
+				 GFP_KERNEL);
+
+	if (!csi->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < csi->num_clks; i++)
+		csi->clks[i].id = imx7_csi_clk_id[i];
+
+	return devm_clk_bulk_get(dev, csi->num_clks, csi->clks);
+}
+
+static int imx7_csi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct imx_media_dev *imxmd;
+	struct imx7_csi *csi;
+	struct resource *res;
+	int ret;
+
+	csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
+	if (!csi)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, &csi->sd);
+	csi->dev = dev;
+
+	ret = imx7_csi_clocks_get(csi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to get clocks");
+		return -ENODEV;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	csi->irq = platform_get_irq(pdev, 0);
+	if (!res || csi->irq < 0) {
+		dev_err(dev, "Missing platform resources data\n");
+		return -ENODEV;
+	}
+
+	csi->regbase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(csi->regbase)) {
+		dev_err(dev, "Failed platform resources map\n");
+		return -ENODEV;
+	}
+
+	spin_lock_init(&csi->irqlock);
+	mutex_init(&csi->lock);
+
+	/* install interrupt handler */
+	ret = devm_request_irq(dev, csi->irq, imx7_csi_irq_handler, 0, "csi",
+			       (void *)csi);
+	if (ret < 0) {
+		dev_err(dev, "Request CSI IRQ failed.\n");
+		ret = -ENODEV;
+		goto destroy_mutex;
+	}
+
+	/* add media device */
+	imxmd = imx_media_dev_init(dev);
+	if (IS_ERR(imxmd)) {
+		ret = PTR_ERR(imxmd);
+		goto destroy_mutex;
+	}
+
+	ret = imx_media_of_add_csi(imxmd, node);
+	if (ret < 0)
+		goto cleanup;
+
+	ret = imx_media_dev_notifier_register(imxmd);
+	if (ret < 0)
+		goto cleanup;
+
+	csi->imxmd = imxmd;
+	v4l2_subdev_init(&csi->sd, &imx7_csi_subdev_ops);
+	v4l2_set_subdevdata(&csi->sd, csi);
+	csi->sd.internal_ops = &imx7_csi_internal_ops;
+	csi->sd.entity.ops = &imx7_csi_entity_ops;
+	csi->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+	csi->sd.dev = &pdev->dev;
+	csi->sd.owner = THIS_MODULE;
+	csi->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
+	csi->sd.grp_id = IMX_MEDIA_GRP_ID_CSI;
+	snprintf(csi->sd.name, sizeof(csi->sd.name), "csi");
+
+	csi->vdev = imx_media_capture_device_init(&csi->sd, IMX7_CSI_PAD_SRC);
+	if (IS_ERR(csi->vdev))
+		return PTR_ERR(csi->vdev);
+
+	v4l2_ctrl_handler_init(&csi->ctrl_hdlr, 0);
+	csi->sd.ctrl_handler = &csi->ctrl_hdlr;
+
+	ret = v4l2_async_register_fwnode_subdev(&csi->sd,
+					sizeof(struct v4l2_async_subdev),
+					NULL, 0, imx7_csi_parse_endpoint);
+	if (ret)
+		goto free;
+
+	return 0;
+
+free:
+	v4l2_ctrl_handler_free(&csi->ctrl_hdlr);
+	imx_media_capture_device_remove(csi->vdev);
+
+cleanup:
+	v4l2_async_notifier_cleanup(&imxmd->notifier);
+	v4l2_device_unregister(&imxmd->v4l2_dev);
+	media_device_cleanup(&imxmd->md);
+
+destroy_mutex:
+	mutex_destroy(&csi->lock);
+
+	return ret;
+}
+
+static int imx7_csi_remove(struct platform_device *pdev)
+{
+	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+	struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+	struct imx_media_dev *imxmd = csi->imxmd;
+
+	imx_media_capture_device_remove(csi->vdev);

I think you're missing v4l2_async_notifier_unregister() here.

+	v4l2_async_notifier_cleanup(&imxmd->notifier);
+	v4l2_device_unregister(&imxmd->v4l2_dev);

Where is the media device unregistered? That should come before freeing
control handlers --- the same for unregistering video devices.

+	media_device_cleanup(&imxmd->md);
+	media_entity_cleanup(&sd->entity);
+	v4l2_async_unregister_subdev(sd);
+	v4l2_ctrl_handler_free(&csi->ctrl_hdlr);
+	mutex_destroy(&csi->lock);
+
+	return 0;
+}
+
+static const struct of_device_id imx7_csi_of_match[] = {
+	{ .compatible = "fsl,imx7-csi" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
+
+static struct platform_driver imx7_csi_driver = {
+	.probe = imx7_csi_probe,
+	.remove = imx7_csi_remove,
+	.driver = {
+		.of_match_table = imx7_csi_of_match,
+		.name = "imx7-csi",
+	},
+};
+module_platform_driver(imx7_csi_driver);
+
+MODULE_DESCRIPTION("i.MX7 CSI subdev driver");
+MODULE_AUTHOR("Rui Miguel Silva [off-list ref]");
+MODULE_LICENSE("GPL");

"GPL" or "GPL v2"?

+MODULE_ALIAS("platform:imx7-csi");

-- 
Sakari Ailus
sakari.ailus@linux.intel.com