Re: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Rajmohan,

On Mon, Nov 06, 2017 at 07:32:52PM +0000, Mani, Rajmohan wrote:
> Hi Sakari, Yong,
> 
> > Subject: Re: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
> > 
> > Hi Yong,
> > 
> > Thanks for the update!
> > 
> > I took a final glance, there are a few matters that still need to be addressed but
> > then I think we're done. Please see below.
> > 
> > Then we'll need an entry in the MAINTAINERS file in the kernel root directory.
> > Could you add an entry for this driver in v8?
> > 
> > On Thu, Nov 02, 2017 at 03:00:01PM -0500, Yong Zhi wrote:
> > ...
> > > +static int cio2_fbpt_rearrange(struct cio2_device *cio2, struct
> > > +cio2_queue *q) {
> > > +	unsigned int i, j;
> > > +
> > > +	for (i = 0, j = q->bufs_first; i < CIO2_MAX_BUFFERS;
> > > +		i++, j = (j + 1) % CIO2_MAX_BUFFERS)
> > > +		if (q->bufs[j])
> > > +			break;
> > > +
> > > +	if (i == CIO2_MAX_BUFFERS)
> > > +		return 0;
> > 
> > You always return 0. The return type could be void.
> > 
> > You could also move this function just above the function using it (it's a single
> > location).
> > 
> > > +
> > > +	if (j) {
> > > +		arrange(q->fbpt, sizeof(struct cio2_fbpt_entry) *
> > CIO2_MAX_LOPS,
> > > +			CIO2_MAX_BUFFERS, j);
> > > +		arrange(q->bufs, sizeof(struct cio2_buffer *),
> > > +			CIO2_MAX_BUFFERS, j);
> > > +	}
> > > +
> > > +	/*
> > > +	 * DMA clears the valid bit when accessing the buffer.
> > > +	 * When stopping stream in suspend callback, some of the buffers
> > > +	 * may be in invalid state. After resume, when DMA meets the invalid
> > > +	 * buffer, it will halt and stop receiving new data.
> > > +	 * To avoid DMA halting, set the valid bit for all buffers in FBPT.
> > > +	 */
> > > +	for (i = 0; i < CIO2_MAX_BUFFERS; i++)
> > > +		cio2_fbpt_entry_enable(cio2, q->fbpt + i * CIO2_MAX_LOPS);
> > > +
> > > +	return 0;
> > > +}
> > 
> > ...
> > 
> > > +static int cio2_vb2_start_streaming(struct vb2_queue *vq, unsigned
> > > +int count) {
> > > +	struct cio2_queue *q = vb2q_to_cio2_queue(vq);
> > > +	struct cio2_device *cio2 = vb2_get_drv_priv(vq);
> > > +	int r;
> > > +
> > > +	cio2->cur_queue = q;
> > > +	atomic_set(&q->frame_sequence, 0);
> > > +
> > > +	r = pm_runtime_get_sync(&cio2->pci_dev->dev);
> > > +	if (r < 0) {
> > > +		dev_info(&cio2->pci_dev->dev, "failed to set power %d\n", r);
> > > +		pm_runtime_put(&cio2->pci_dev->dev);
> 
> Shouldn't we use pm_runtime_put_noidle() here, so the usage_count gets decremented?

Yeah, pm_runtime_put_noidle() would be perhaps more appropriate. The
difference is that the noidle variant won't suspend the device (but it was
already suspended to begin with as resuming failed).

> 
> > > +		return r;
> > > +	}
> > > +
> > > +	r = media_pipeline_start(&q->vdev.entity, &q->pipe);
> > > +	if (r)
> > > +		goto fail_pipeline;
> > > +
> > > +	r = cio2_hw_init(cio2, q);
> > > +	if (r)
> > > +		goto fail_hw;
> > > +
> > > +	/* Start streaming on sensor */
> > > +	r = v4l2_subdev_call(q->sensor, video, s_stream, 1);
> > > +	if (r)
> > > +		goto fail_csi2_subdev;
> > > +
> > > +	cio2->streaming = true;
> > > +
> > > +	return 0;
> > > +
> > > +fail_csi2_subdev:
> > > +	cio2_hw_exit(cio2, q);
> > > +fail_hw:
> > > +	media_pipeline_stop(&q->vdev.entity);
> > > +fail_pipeline:
> > > +	dev_dbg(&cio2->pci_dev->dev, "failed to start streaming (%d)\n", r);
> > > +	cio2_vb2_return_all_buffers(q);
> > 
> > I believe there should be
> > 
> > 	pm_runtime_put(&cio2->pci_dev->dev);
> > 
> > here. You should also add a label and use goto from where you do
> > pm_runtime_put() in error handling in this function in order to make this
> > cleaner.
> > 
> > > +
> > > +	return r;
> > > +}
> > 
> > --
> > Kind regards,
> > 
> > Sakari Ailus
> > sakari.ailus@linux.intel.com

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