Re: [PATCH v3 7/7] media: platform: rcar_drif: Add DRIF support

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

Hi Ramesh,

Thank you for the patch.

On Tuesday 07 Feb 2017 15:02:37 Ramesh Shanmugasundaram wrote:
> This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3 SoCs.
> The driver exposes each instance of DRIF as a V4L2 SDR device. A DRIF
> device represents a channel and each channel can have one or two
> sub-channels respectively depending on the target board.
> 
> DRIF supports only Rx functionality. It receives samples from a RF
> frontend tuner chip it is interfaced with. The combination of DRIF and the
> tuner device, which is registered as a sub-device, determines the receive
> sample rate and format.
> 
> In order to be compliant as a V4L2 SDR device, DRIF needs to bind with
> the tuner device, which can be provided by a third party vendor. DRIF acts
> as a slave device and the tuner device acts as a master transmitting the
> samples. The driver allows asynchronous binding of a tuner device that
> is registered as a v4l2 sub-device. The driver can learn about the tuner
> it is interfaced with based on port endpoint properties of the device in
> device tree. The V4L2 SDR device inherits the controls exposed by the
> tuner device.
> 
> The device can also be configured to use either one or both of the data
> pins at runtime based on the master (tuner) configuration.
> 
> Signed-off-by: Ramesh Shanmugasundaram
> <ramesh.shanmugasundaram-kTT6dE0pTRh9uiUsa/gSgQ@public.gmane.org>
> ---
>  drivers/media/platform/Kconfig     |   25 +
>  drivers/media/platform/Makefile    |    1 +
>  drivers/media/platform/rcar_drif.c | 1534 +++++++++++++++++++++++++++++++++
>  3 files changed, 1560 insertions(+)
>  create mode 100644 drivers/media/platform/rcar_drif.c

[snip]

> diff --git a/drivers/media/platform/rcar_drif.c
> b/drivers/media/platform/rcar_drif.c new file mode 100644
> index 0000000..88950e3
> --- /dev/null
> +++ b/drivers/media/platform/rcar_drif.c
> @@ -0,0 +1,1534 @@

[snip]

> +/*
> + * The R-Car DRIF is a receive only MSIOF like controller with an
> + * external master device driving the SCK. It receives data into a FIFO,
> + * then this driver uses the SYS-DMAC engine to move the data from
> + * the device to memory.
> + *
> + * Each DRIF channel DRIFx (as per datasheet) contains two internal
> + * channels DRIFx0 & DRIFx1 within itself with each having its own
> resources
> + * like module clk, register set, irq and dma. These internal channels
> share
> + * common CLK & SYNC from master. The two data pins D0 & D1 shall be
> + * considered to represent the two internal channels. This internal split
> + * is not visible to the master device.
> + *
> + * Depending on the master device, a DRIF channel can use
> + *  (1) both internal channels (D0 & D1) to receive data in parallel (or)
> + *  (2) one internal channel (D0 or D1) to receive data
> + *
> + * The primary design goal of this controller is to act as Digitial Radio

s/Digitial/Digital/

> + * Interface that receives digital samples from a tuner device. Hence the
> + * driver exposes the device as a V4L2 SDR device. In order to qualify as
> + * a V4L2 SDR device, it should possess tuner interface as mandated by the
> + * framework. This driver expects a tuner driver (sub-device) to bind
> + * asynchronously with this device and the combined drivers shall expose
> + * a V4L2 compliant SDR device. The DRIF driver is independent of the
> + * tuner vendor.
> + *
> + * The DRIF h/w can support I2S mode and Frame start synchronization pulse
> mode.
> + * This driver is tested for I2S mode only because of the availability of
> + * suitable master devices. Hence, not all configurable options of DRIF h/w
> + * like lsb/msb first, syncdl, dtdl etc. are exposed via DT and I2S
> defaults
> + * are used. These can be exposed later if needed after testing.
> + */

[snip]

> +#define to_rcar_drif_buf_pair(sdr, ch_num,
> idx)	(sdr->ch[!(ch_num)]->buf[idx])

You should enclose both sdr and idx in parenthesis, as they can be 
expressions.

> +
> +#define for_each_rcar_drif_channel(ch, ch_mask)			\
> +	for_each_set_bit(ch, ch_mask, RCAR_DRIF_MAX_CHANNEL)
> +
> +static const unsigned int num_hwbufs = 32;

Is there a specific reason to make this a static const instead of a #define ?

> +/* Debug */
> +static unsigned int debug;
> +module_param(debug, uint, 0644);
> +MODULE_PARM_DESC(debug, "activate debug info");
> +
> +#define rdrif_dbg(level, sdr, fmt, arg...)				\
> +	v4l2_dbg(level, debug, &sdr->v4l2_dev, fmt, ## arg)
> +
> +#define rdrif_err(sdr, fmt, arg...)					\
> +	dev_err(sdr->v4l2_dev.dev, fmt, ## arg)
> +
> +/* Stream formats */
> +struct rcar_drif_format {
> +	u32	pixelformat;
> +	u32	buffersize;
> +	u32	wdlen;
> +	u32	num_ch;
> +};
> +
> +/* Format descriptions for capture */
> +static const struct rcar_drif_format formats[] = {
> +	{
> +		.pixelformat	= V4L2_SDR_FMT_PCU16BE,
> +		.buffersize	= RCAR_SDR_BUFFER_SIZE,
> +		.wdlen		= 16,
> +		.num_ch	= 2,

How about aligning the = as in the other lines ?

num_ch is always set to 2. Should we remove it for now, and add it back later 
when we'll support single-channel formats ? I think we should avoid carrying 
dead code.

> +	},
> +	{
> +		.pixelformat	= V4L2_SDR_FMT_PCU18BE,
> +		.buffersize	= RCAR_SDR_BUFFER_SIZE,
> +		.wdlen		= 18,
> +		.num_ch	= 2,
> +	},
> +	{
> +		.pixelformat	= V4L2_SDR_FMT_PCU20BE,
> +		.buffersize	= RCAR_SDR_BUFFER_SIZE,
> +		.wdlen		= 20,
> +		.num_ch	= 2,
> +	},
> +};
> +
> +static const unsigned int NUM_FORMATS = ARRAY_SIZE(formats);

Same question here, can't this be a define ? I think I'd even avoid 
NUM_FORMATS completely and use ARRAY_SIZE(formats) directly in the code, to 
make the boundary check more explicit when iterating over the array.

> +
> +/* Buffer for a received frame from one or both internal channels */
> +struct rcar_drif_frame_buf {
> +	/* Common v4l buffer stuff -- must be first */
> +	struct vb2_v4l2_buffer vb;
> +	struct list_head list;
> +};
> +
> +struct rcar_drif_async_subdev {
> +	struct v4l2_subdev *sd;
> +	struct v4l2_async_subdev asd;
> +};
> +
> +/* DMA buffer */
> +struct rcar_drif_hwbuf {
> +	void *addr;			/* CPU-side address */
> +	unsigned int status;		/* Buffer status flags */
> +};
> +
> +/* Internal channel */
> +struct rcar_drif {
> +	struct rcar_drif_sdr *sdr;	/* Group device */
> +	struct platform_device *pdev;	/* Channel's pdev */
> +	void __iomem *base;		/* Base register address */
> +	resource_size_t start;		/* I/O resource offset */
> +	struct dma_chan *dmach;		/* Reserved DMA channel */
> +	struct clk *clkp;		/* Module clock */
> +	struct rcar_drif_hwbuf *buf[RCAR_DRIF_MAX_NUM_HWBUFS]; /* H/W bufs */
> +	dma_addr_t dma_handle;		/* Handle for all bufs */
> +	unsigned int num;		/* Channel number */
> +	bool acting_sdr;		/* Channel acting as SDR device */
> +};
> +
> +/* DRIF V4L2 SDR */
> +struct rcar_drif_sdr {
> +	struct device *dev;		/* Platform device */
> +	struct video_device *vdev;	/* V4L2 SDR device */
> +	struct v4l2_device v4l2_dev;	/* V4L2 device */
> +
> +	/* Videobuf2 queue and queued buffers list */
> +	struct vb2_queue vb_queue;
> +	struct list_head queued_bufs;
> +	spinlock_t queued_bufs_lock;	/* Protects queued_bufs */
> +
> +	struct mutex v4l2_mutex;	/* To serialize ioctls */
> +	struct mutex vb_queue_mutex;	/* To serialize streaming ioctls */
> +	struct v4l2_ctrl_handler ctrl_hdl;	/* SDR control handler */
> +	struct v4l2_async_notifier notifier;	/* For subdev (tuner) */
> +
> +	/* Current V4L2 SDR format array index */
> +	unsigned int fmt_idx;

Instead of storing the index I would store a pointer to the corresponding 
rcar_drif_format, looking up information about the current format will then be 
easier.

> +
> +	/* Device tree SYNC properties */
> +	u32 mdr1;
> +
> +	/* Internals */
> +	struct rcar_drif *ch[RCAR_DRIF_MAX_CHANNEL]; /* DRIFx0,1 */
> +	unsigned long hw_ch_mask;	/* Enabled channels per DT */
> +	unsigned long cur_ch_mask;	/* Used channels for an SDR FMT */
> +	u32 num_hw_ch;			/* Num of DT enabled channels */
> +	u32 num_cur_ch;			/* Num of used channels */
> +	u32 hwbuf_size;			/* Each DMA buffer size */
> +	u32 produced;			/* Buffers produced by sdr dev */
> +};
> +
> +/* Allocate buffer context */
> +static int rcar_drif_alloc_bufctxt(struct rcar_drif_sdr *sdr)
> +{
> +	struct rcar_drif_hwbuf *bufctx;
> +	unsigned int i, idx;
> +
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		bufctx = kcalloc(num_hwbufs, sizeof(*bufctx), GFP_KERNEL);

How about embedding the buffer contexts in the rcar_drif structure instead of 
just storing pointers there ? The rcar_drif_hwbuf structure is pretty small, 
it won't make a big difference, and will simplify the code.

> +		if (!bufctx)
> +			return -ENOMEM;
> +
> +		for (idx = 0; idx < num_hwbufs; idx++)
> +			sdr->ch[i]->buf[idx] = bufctx + idx;
> +	}
> +	return 0;
> +}

[snip]

> +/* Release DMA channel */
> +static void rcar_drif_release_dmachannel(struct rcar_drif_sdr *sdr)

I would name the function rcar_drif_release_dma_channels as it handles all 
channels. Same for rcar_drif_alloc_dma_channels.

> +{
> +	unsigned int i;
> +
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask)
> +		if (sdr->ch[i]->dmach) {
> +			dma_release_channel(sdr->ch[i]->dmach);
> +			sdr->ch[i]->dmach = NULL;
> +		}
> +}
> +
> +/* Allocate DMA channel */
> +static int rcar_drif_alloc_dmachannel(struct rcar_drif_sdr *sdr)
> +{
> +	struct dma_slave_config dma_cfg;
> +	unsigned int i;
> +	int ret = -ENODEV;
> +
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		struct rcar_drif *ch = sdr->ch[i];
> +
> +		ch->dmach = dma_request_slave_channel(&ch->pdev->dev, "rx");
> +		if (!ch->dmach) {
> +			rdrif_err(sdr, "ch%u: dma channel req failed\n", i);
> +			goto dmach_error;
> +		}
> +
> +		/* Configure slave */
> +		memset(&dma_cfg, 0, sizeof(dma_cfg));
> +		dma_cfg.src_addr = (phys_addr_t)(ch->start + 
RCAR_DRIF_SIRFDR);
> +		dma_cfg.dst_addr = 0;

This isn't needed as you memset the whole structure to 0.

> +		dma_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +		ret = dmaengine_slave_config(ch->dmach, &dma_cfg);
> +		if (ret) {
> +			rdrif_err(sdr, "ch%u: dma slave config failed\n", i);
> +			goto dmach_error;
> +		}
> +	}
> +	return 0;
> +
> +dmach_error:
> +	rcar_drif_release_dmachannel(sdr);
> +	return ret;
> +}

[snip]

> +/* Set MDR defaults */
> +static inline void rcar_drif_set_mdr1(struct rcar_drif_sdr *sdr)
> +{
> +	unsigned int i;
> +
> +	/* Set defaults for enabled internal channels */
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		/* Refer MSIOF section in manual for this register setting */
> +		writel(RCAR_DRIF_SITMDR1_PCON,
> +		       sdr->ch[i]->base + RCAR_DRIF_SITMDR1);

I would create a rcar_drif_write(struct rcar_drif *ch, u32 offset, u32 data) 
function, the code will become clearer. Same for the read operation.

> +		/* Setup MDR1 value */
> +		writel(sdr->mdr1, sdr->ch[i]->base + RCAR_DRIF_SIRMDR1);
> +
> +		rdrif_dbg(2, sdr, "ch%u: mdr1 = 0x%08x",
> +			  i, readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR1));

Once you've debugged the driver I'm not sure those debugging statements are 
still needed.

> +	}
> +}
> +
> +/* Extract bitlen and wdcnt from given word length */
> +static int rcar_drif_convert_wdlen(struct rcar_drif_sdr *sdr,
> +				   u32 wdlen, u32 *bitlen, u32 *wdcnt)
> +{
> +	unsigned int i, nr_wds;
> +
> +	/* FIFO register size is 32 bits */
> +	for (i = 0; i < 32; i++) {
> +		nr_wds = wdlen % (32 - i);
> +		if (nr_wds == 0) {
> +			*bitlen = 32 - i;
> +			*wdcnt = wdlen / *bitlen;

Can't you store the bitlen and wdcnt values in the rcar_drif_format structure 
instead of recomputing them every time ?

> +			break;
> +		}
> +	}
> +
> +	/* Sanity check range */
> +	if (i == 32 || !(*bitlen >= 8 && *bitlen <= 32) ||
> +	    !(*wdcnt >= 1 && *wdcnt <= 64)) {
> +		rdrif_err(sdr, "invalid wdlen %u configured\n", wdlen);
> +		return -EINVAL;

You shouldn't have invalid wdlen values in the driver. I would remove this 
check as it makes error handling in the caller more complex.

> +	}
> +
> +	return 0;
> +}
> +
> +/* Set DRIF receive format */
> +static int rcar_drif_set_format(struct rcar_drif_sdr *sdr)
> +{
> +	u32 bitlen, wdcnt, wdlen;
> +	unsigned int i;
> +	int ret = -EINVAL;
> +
> +	wdlen = formats[sdr->fmt_idx].wdlen;
> +	rdrif_dbg(2, sdr, "setfmt: idx %u, wdlen %u, num_ch %u\n",
> +		  sdr->fmt_idx, wdlen, formats[sdr->fmt_idx].num_ch);
> +
> +	/* Sanity check */
> +	if (formats[sdr->fmt_idx].num_ch > sdr->num_cur_ch) {
> +		rdrif_err(sdr, "fmt idx %u current ch %u mismatch\n",
> +			  sdr->fmt_idx, sdr->num_cur_ch);
> +		return ret;

This should never happen, it should be caught at set format time.

> +	}
> +
> +	/* Get bitlen & wdcnt from wdlen */
> +	ret = rcar_drif_convert_wdlen(sdr, wdlen, &bitlen, &wdcnt);
> +	if (ret)
> +		return ret;
> +
> +	/* Setup group, bitlen & wdcnt */
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		u32 mdr;
> +
> +		/* Two groups */
> +		mdr = RCAR_DRIF_MDR_GRPCNT(2) | RCAR_DRIF_MDR_BITLEN(bitlen) |
> +		       RCAR_DRIF_MDR_WDCNT(wdcnt);
> +		writel(mdr, sdr->ch[i]->base + RCAR_DRIF_SIRMDR2);
> +
> +		mdr = RCAR_DRIF_MDR_BITLEN(bitlen) | 
RCAR_DRIF_MDR_WDCNT(wdcnt);
> +		writel(mdr, sdr->ch[i]->base + RCAR_DRIF_SIRMDR3);
> +
> +		rdrif_dbg(2, sdr, "ch%u: new mdr[2,3] = 0x%08x, 0x%08x\n",
> +			  i, readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR2),
> +			  readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR3));
> +	}
> +	return ret;
> +}
> +
> +/* Release DMA buffers */
> +static void rcar_drif_release_buf(struct rcar_drif_sdr *sdr)
> +{
> +	unsigned int i;
> +
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		struct rcar_drif *ch = sdr->ch[i];
> +
> +		/* First entry contains the dma buf ptr */
> +		if (ch->buf[0] && ch->buf[0]->addr) {
> +			dma_free_coherent(&ch->pdev->dev,
> +					  sdr->hwbuf_size * num_hwbufs,
> +					  ch->buf[0]->addr, ch->dma_handle);
> +			ch->buf[0]->addr = NULL;
> +		}
> +	}
> +}
> +
> +/* Request DMA buffers */
> +static int rcar_drif_request_buf(struct rcar_drif_sdr *sdr)
> +{
> +	int ret = -ENOMEM;
> +	unsigned int i, j;
> +	void *addr;
> +
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		struct rcar_drif *ch = sdr->ch[i];
> +
> +		/* Allocate DMA buffers */
> +		addr = dma_alloc_coherent(&ch->pdev->dev,
> +					  sdr->hwbuf_size * num_hwbufs,
> +					  &ch->dma_handle, GFP_KERNEL);
> +		if (!addr) {
> +			rdrif_err(sdr,
> +			"ch%u: dma alloc failed. num_hwbufs %u size %u\n",
> +			i, num_hwbufs, sdr->hwbuf_size);
> +			goto alloc_error;
> +		}
> +
> +		/* Split the chunk and populate bufctxt */
> +		for (j = 0; j < num_hwbufs; j++) {
> +			ch->buf[j]->addr = addr + (j * sdr->hwbuf_size);
> +			ch->buf[j]->status = 0;
> +		}
> +	}
> +
> +	return 0;
> +
> +alloc_error:
> +	return ret;
> +}
> +
> +/* Setup vb_queue minimum buffer requirements */
> +static int rcar_drif_queue_setup(struct vb2_queue *vq,
> +			unsigned int *num_buffers, unsigned int *num_planes,
> +			unsigned int sizes[], struct device *alloc_devs[])
> +{
> +	struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vq);
> +
> +	/* Need at least 16 buffers */
> +	if (vq->num_buffers + *num_buffers < 16)
> +		*num_buffers = 16 - vq->num_buffers;
> +
> +	*num_planes = 1;
> +	sizes[0] = PAGE_ALIGN(formats[sdr->fmt_idx].buffersize);
> +
> +	rdrif_dbg(2, sdr, "num_bufs %d sizes[0] %d\n", *num_buffers, 
sizes[0]);
> +	return 0;
> +}
> +
> +/* Enqueue buffer */
> +static void rcar_drif_buf_queue(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vb->vb2_queue);
> +	struct rcar_drif_frame_buf *fbuf =
> +			container_of(vbuf, struct rcar_drif_frame_buf, vb);
> +	unsigned long flags;
> +
> +	rdrif_dbg(2, sdr, "buf_queue idx %u\n", vb->index);
> +	spin_lock_irqsave(&sdr->queued_bufs_lock, flags);
> +	list_add_tail(&fbuf->list, &sdr->queued_bufs);
> +	spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags);
> +}
> +
> +/* Get a frame buf from list */
> +static struct rcar_drif_frame_buf *
> +rcar_drif_get_fbuf(struct rcar_drif_sdr *sdr)
> +{
> +	struct rcar_drif_frame_buf *fbuf;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&sdr->queued_bufs_lock, flags);
> +	fbuf = list_first_entry_or_null(&sdr->queued_bufs, struct
> +					rcar_drif_frame_buf, list);
> +	if (!fbuf) {
> +		/*
> +		 * App is late in enqueing buffers. Samples lost & there will
> +		 * be a gap in sequence number when app recovers
> +		 */
> +		rdrif_dbg(1, sdr, "\napp late: prod %u\n", sdr->produced);
> +		sdr->produced++; /* Increment the produced count anyway */
> +		spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags);
> +		return NULL;
> +	}
> +	list_del(&fbuf->list);
> +	spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags);
> +
> +	return fbuf;
> +}
> +
> +static inline bool rcar_drif_buf_pairs_done(struct rcar_drif_hwbuf *buf1,
> +					    struct rcar_drif_hwbuf *buf2)
> +{
> +	return (buf1->status & buf2->status & RCAR_DRIF_BUF_DONE);
> +}
> +
> +/* Channel DMA complete */
> +static void rcar_drif_channel_complete(struct rcar_drif *ch, u32 idx)
> +{
> +	u32 str;
> +
> +	ch->buf[idx]->status |= RCAR_DRIF_BUF_DONE;
> +
> +	/* Check for DRIF errors */
> +	str = readl(ch->base + RCAR_DRIF_SISTR);
> +	if (unlikely(str & RCAR_DRIF_RFOVF)) {
> +		/* Writing the same clears it */
> +		writel(str, ch->base + RCAR_DRIF_SISTR);
> +
> +		/* Overflow: some samples are lost */
> +		ch->buf[idx]->status |= RCAR_DRIF_BUF_OVERFLOW;
> +	}
> +}
> +
> +/* Deliver buffer to user */
> +static void rcar_drif_deliver_buf(struct rcar_drif *ch)
> +{
> +	struct rcar_drif_sdr *sdr = ch->sdr;
> +	u32 idx = sdr->produced % num_hwbufs;
> +	struct rcar_drif_frame_buf *fbuf;
> +	bool overflow = false;
> +
> +	rcar_drif_channel_complete(ch, idx);
> +
> +	if (sdr->num_cur_ch == RCAR_DRIF_MAX_CHANNEL) {
> +		struct rcar_drif_hwbuf *bufi, *bufq;
> +
> +		if (ch->num) {
> +			bufi = to_rcar_drif_buf_pair(sdr, ch->num, idx);
> +			bufq = ch->buf[idx];
> +		} else {
> +			bufi = ch->buf[idx];
> +			bufq = to_rcar_drif_buf_pair(sdr, ch->num, idx);
> +		}
> +
> +		/* Check if both DMA buffers are done */
> +		if (!rcar_drif_buf_pairs_done(bufi, bufq))
> +			return;
> +
> +		/* Clear buf done status */
> +		bufi->status &= ~RCAR_DRIF_BUF_DONE;
> +		bufq->status &= ~RCAR_DRIF_BUF_DONE;
> +
> +		/* Get fbuf */
> +		fbuf = rcar_drif_get_fbuf(sdr);
> +		if (!fbuf)
> +			return;
> +
> +		memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0),
> +		       bufi->addr, sdr->hwbuf_size);
> +		memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0) + sdr-
>hwbuf_size,
> +		       bufq->addr, sdr->hwbuf_size);

Ouch ! That's a high data rate memcpy that can be avoided. Why don't you DMA 
directly to the vb2 buffers ? You will need to use videobuf2-dma-contig 
instead of videobuf2-vmalloc, but apart from that there should be no issue.

> +		if ((bufi->status | bufq->status) & RCAR_DRIF_BUF_OVERFLOW) {
> +			overflow = true;
> +			/* Clear the flag in status */
> +			bufi->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> +			bufq->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> +		}
> +	} else {
> +		struct rcar_drif_hwbuf *bufiq;
> +
> +		/* Get fbuf */
> +		fbuf = rcar_drif_get_fbuf(sdr);
> +		if (!fbuf)
> +			return;
> +
> +		bufiq = ch->buf[idx];
> +
> +		memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0),
> +		       bufiq->addr, sdr->hwbuf_size);
> +
> +		if (bufiq->status & RCAR_DRIF_BUF_OVERFLOW) {
> +			overflow = true;
> +			/* Clear the flag in status */
> +			bufiq->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> +		}
> +	}
> +
> +	rdrif_dbg(2, sdr, "ch%u: prod %u\n", ch->num, sdr->produced);
> +
> +	fbuf->vb.field = V4L2_FIELD_NONE;
> +	fbuf->vb.sequence = sdr->produced++;
> +	fbuf->vb.vb2_buf.timestamp = ktime_get_ns();
> +	vb2_set_plane_payload(&fbuf->vb.vb2_buf, 0,
> +			      formats[sdr->fmt_idx].buffersize);
> +
> +	/* Set error state on overflow */
> +	if (overflow)
> +		vb2_buffer_done(&fbuf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> +	else
> +		vb2_buffer_done(&fbuf->vb.vb2_buf, VB2_BUF_STATE_DONE);

Maybe

	vb2_buffer_done(&fbuf->vb.vb2_buf,
			overflow ? VB2_BUF_STATE_ERROR: VB2_BUF_STATE_DONE);

> +}
> +
> +/* DMA callback for each stage */
> +static void rcar_drif_dma_complete(void *dma_async_param)
> +{
> +	struct rcar_drif *ch = dma_async_param;
> +	struct rcar_drif_sdr *sdr = ch->sdr;
> +
> +	mutex_lock(&sdr->vb_queue_mutex);

Isn't the complete callback potentially called in interrupt context ? I know 
the rcar-dmac driver uses a threaded interrupt handler for that, but is that a 
guarantee of the DMA engine API ?

> +
> +	/* DMA can be terminated while the callback was waiting on lock */
> +	if (!vb2_is_streaming(&sdr->vb_queue))

Can it ? The streaming flag is cleared after the stop_streaming operation is 
called, which will terminate all DMA transfers synchronously.

> +		goto stopped;
> +
> +	rcar_drif_deliver_buf(ch);
> +stopped:
> +	mutex_unlock(&sdr->vb_queue_mutex);
> +}
> +
> +static int rcar_drif_qbuf(struct rcar_drif *ch)
> +{
> +	struct rcar_drif_sdr *sdr = ch->sdr;
> +	dma_addr_t addr = ch->dma_handle;
> +	struct dma_async_tx_descriptor *rxd;
> +	dma_cookie_t cookie;
> +	int ret = -EIO;
> +
> +	/* Setup cyclic DMA with given buffers */
> +	rxd = dmaengine_prep_dma_cyclic(ch->dmach, addr,
> +					sdr->hwbuf_size * num_hwbufs,
> +					sdr->hwbuf_size, DMA_DEV_TO_MEM,
> +					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> +	if (!rxd) {
> +		rdrif_err(sdr, "ch%u: prep dma cyclic failed\n", ch->num);
> +		return ret;
> +	}
> +
> +	/* Submit descriptor */
> +	rxd->callback = rcar_drif_dma_complete;
> +	rxd->callback_param = ch;
> +	cookie = dmaengine_submit(rxd);
> +	if (dma_submit_error(cookie)) {
> +		rdrif_err(sdr, "ch%u: dma submit failed\n", ch->num);
> +		return ret;
> +	}
> +
> +	dma_async_issue_pending(ch->dmach);
> +	return 0;
> +}
> +
> +/* Enable reception */
> +static int rcar_drif_enable_rx(struct rcar_drif_sdr *sdr)
> +{
> +	unsigned int i;
> +	u32 ctr;
> +	int ret;
> +
> +	/*
> +	 * When both internal channels are enabled, they can be synchronized
> +	 * only by the master
> +	 */
> +
> +	/* Enable receive */
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		ctr = readl(sdr->ch[i]->base + RCAR_DRIF_SICTR);
> +		ctr |= (RCAR_DRIF_SICTR_RX_RISING_EDGE |
> +			 RCAR_DRIF_SICTR_RX_EN);
> +		writel(ctr, sdr->ch[i]->base + RCAR_DRIF_SICTR);
> +	}
> +
> +	/* Check receive enabled */
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		ret = readl_poll_timeout(sdr->ch[i]->base + RCAR_DRIF_SICTR,
> +					 ctr, ctr & RCAR_DRIF_SICTR_RX_EN,
> +					 2, 500000);

A 2µs sleep for a 500ms total timeout seems very low to me, that will stress 
the CPU. Same comment for the other locations where you use 
readl_poll_timeout.

How long does the channel typically take to get enabled ?

> +		if (ret) {
> +			rdrif_err(sdr, "ch%u: rx en failed. ctr 0x%08x\n",
> +				  i, readl(sdr->ch[i]->base + 
RCAR_DRIF_SICTR));
> +			break;
> +		}
> +	}
> +	return ret;
> +}
> +
> +/* Disable reception */
> +static void rcar_drif_disable_rx(struct rcar_drif_sdr *sdr)
> +{
> +	unsigned int i;
> +	u32 ctr;
> +	int ret;
> +
> +	/* Disable receive */
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		ctr = readl(sdr->ch[i]->base + RCAR_DRIF_SICTR);
> +		ctr &= ~RCAR_DRIF_SICTR_RX_EN;
> +		writel(ctr, sdr->ch[i]->base + RCAR_DRIF_SICTR);
> +	}
> +
> +	/* Check receive disabled */
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		ret = readl_poll_timeout(sdr->ch[i]->base + RCAR_DRIF_SICTR,
> +					 ctr, !(ctr & RCAR_DRIF_SICTR_RX_EN),
> +					 2, 500000);

How long does the channel typically take to get disabled ?

> +		if (ret)
> +			dev_warn(&sdr->vdev->dev,
> +			"ch%u: failed to disable rx. ctr 0x%08x\n",
> +			i, readl(sdr->ch[i]->base + RCAR_DRIF_SICTR));
> +	}
> +}
> +
> +/* Start channel */
> +static int rcar_drif_start_channel(struct rcar_drif *ch)
> +{
> +	struct rcar_drif_sdr *sdr = ch->sdr;
> +	u32 ctr, str;
> +	int ret;
> +
> +	/* Reset receive */
> +	writel(RCAR_DRIF_SICTR_RESET, ch->base + RCAR_DRIF_SICTR);
> +	ret = readl_poll_timeout(ch->base + RCAR_DRIF_SICTR,
> +					 ctr, !(ctr & RCAR_DRIF_SICTR_RESET),

The alignment is weird.

> +					 2, 500000);
> +	if (ret) {
> +		rdrif_err(sdr, "ch%u: failed to reset rx. ctr 0x%08x\n",
> +			  ch->num, readl(ch->base + RCAR_DRIF_SICTR));
> +		return ret;
> +	}
> +
> +	/* Queue buffers for DMA */
> +	ret = rcar_drif_qbuf(ch);
> +	if (ret)
> +		return ret;
> +
> +	/* Clear status register flags */
> +	str = RCAR_DRIF_RFFUL | RCAR_DRIF_REOF | RCAR_DRIF_RFSERR |
> +		RCAR_DRIF_RFUDF | RCAR_DRIF_RFOVF;
> +	writel(str, ch->base + RCAR_DRIF_SISTR);
> +
> +	/* Enable DMA receive interrupt */
> +	writel(0x00009000, ch->base + RCAR_DRIF_SIIER);
> +
> +	return ret;
> +}
> +
> +/* Start receive operation */
> +static int rcar_drif_start(struct rcar_drif_sdr *sdr)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		ret = rcar_drif_start_channel(sdr->ch[i]);
> +		if (ret)
> +			goto start_error;
> +	}
> +
> +	sdr->produced = 0;
> +	ret = rcar_drif_enable_rx(sdr);
> +start_error:

Don't you need to stop the channels that were successfully started if an error 
occurs ?

> +	return ret;
> +}
> +
> +/* Stop channel */
> +static void rcar_drif_stop_channel(struct rcar_drif *ch)
> +{
> +	struct rcar_drif_sdr *sdr = ch->sdr;
> +	int ret, retries = 3;
> +
> +	/* Disable DMA receive interrupt */
> +	writel(0x00000000, ch->base + RCAR_DRIF_SIIER);
> +
> +	do {
> +		/* Terminate all DMA transfers */
> +		ret = dmaengine_terminate_sync(ch->dmach);
> +		if (!ret)
> +			break;
> +		rdrif_dbg(2, sdr, "stop retry\n");
> +	} while (--retries);

Why do you need to retry the terminate operation, why does it fail ?

> +	WARN_ON(!retries);
> +}

[snip]

> +/* Start streaming */
> +static int rcar_drif_start_streaming(struct vb2_queue *vq, unsigned int
> count)
> +{
> +	struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vq);
> +	unsigned int i, j;
> +	int ret;
> +
> +	mutex_lock(&sdr->v4l2_mutex);

I'm surprised, aren't the start_streaming and stop_streaming operations called 
with the video device lock held already by the v4l2-ioctl layer ? I think they 
should be, if they're not there's probably a bug somewhere.

> +	for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> +		ret = clk_prepare_enable(sdr->ch[i]->clkp);
> +		if (ret)
> +			goto start_error;
> +	}
> +
> +	/* Set default MDRx settings */
> +	rcar_drif_set_mdr1(sdr);
> +
> +	/* Set new format */
> +	ret = rcar_drif_set_format(sdr);
> +	if (ret)
> +		goto start_error;
> +
> +	if (sdr->num_cur_ch == RCAR_DRIF_MAX_CHANNEL)
> +		sdr->hwbuf_size =
> +		formats[sdr->fmt_idx].buffersize / RCAR_DRIF_MAX_CHANNEL;
> +	else
> +		sdr->hwbuf_size = formats[sdr->fmt_idx].buffersize;
> +
> +	rdrif_dbg(1, sdr, "num_hwbufs %u, hwbuf_size %u\n",
> +		num_hwbufs, sdr->hwbuf_size);
> +
> +	/* Alloc DMA channel */
> +	ret = rcar_drif_alloc_dmachannel(sdr);
> +	if (ret)
> +		goto start_error;
> +
> +	/* Alloc buf context */
> +	ret = rcar_drif_alloc_bufctxt(sdr);
> +	if (ret)
> +		goto start_error;
> +
> +	/* Request buffers */
> +	ret = rcar_drif_request_buf(sdr);
> +	if (ret)
> +		goto start_error;
> +
> +	/* Start Rx */
> +	ret = rcar_drif_start(sdr);
> +	if (ret)
> +		goto start_error;
> +
> +	mutex_unlock(&sdr->v4l2_mutex);
> +	rdrif_dbg(1, sdr, "started\n");
> +	return ret;
> +
> +start_error:

As there's a single error label I would call this "error". Up to you.

> +	rcar_drif_release_queued_bufs(sdr, VB2_BUF_STATE_QUEUED);
> +	rcar_drif_release_buf(sdr);
> +	rcar_drif_release_bufctxt(sdr);
> +	rcar_drif_release_dmachannel(sdr);
> +	for (j = 0; j < i; j++)
> +		clk_disable_unprepare(sdr->ch[j]->clkp);
> +
> +	mutex_unlock(&sdr->v4l2_mutex);
> +	return ret;
> +}

[snip]

> +/* Vb2 ops */
> +static struct vb2_ops rcar_drif_vb2_ops = {

You can make this static const.

> +	.queue_setup            = rcar_drif_queue_setup,
> +	.buf_queue              = rcar_drif_buf_queue,
> +	.start_streaming        = rcar_drif_start_streaming,
> +	.stop_streaming         = rcar_drif_stop_streaming,
> +	.wait_prepare		= vb2_ops_wait_prepare,
> +	.wait_finish		= vb2_ops_wait_finish,
> +};

[snip]

> +static int rcar_drif_g_fmt_sdr_cap(struct file *file, void *priv,
> +				   struct v4l2_format *f)
> +{
> +	struct rcar_drif_sdr *sdr = video_drvdata(file);
> +
> +	f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> +	f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;
> +	memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));

I believe the core ioctl handling code already does this for you. Same for the 
other ioctl handlers in 

> +	return 0;
> +}
> +
> +static int rcar_drif_s_fmt_sdr_cap(struct file *file, void *priv,
> +				   struct v4l2_format *f)
> +{
> +	struct rcar_drif_sdr *sdr = video_drvdata(file);
> +	struct vb2_queue *q = &sdr->vb_queue;
> +	unsigned int i;
> +
> +	if (vb2_is_busy(q))
> +		return -EBUSY;
> +
> +	memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
> +	for (i = 0; i < NUM_FORMATS; i++) {
> +		if (formats[i].pixelformat == f->fmt.sdr.pixelformat) {

The code would become more readable (at least in my opinion) if you just added 
a break here, and moved the code below after the loop. In case the requested 
format isn't found (i == NUM_FORMATS) you can then set i to 0 and proceed, 
that will select the first available format as a default.

> +			sdr->fmt_idx  = i;
> +			f->fmt.sdr.buffersize = formats[i].buffersize;
> +
> +			/*
> +			 * If a format demands one channel only out of two
> +			 * enabled channels, pick the 0th channel.
> +			 */
> +			if (formats[i].num_ch < sdr->num_hw_ch) {
> +				sdr->cur_ch_mask = BIT(0);
> +				sdr->num_cur_ch = formats[i].num_ch;
> +			} else {
> +				sdr->cur_ch_mask = sdr->hw_ch_mask;
> +				sdr->num_cur_ch = sdr->num_hw_ch;
> +			}
> +
> +			rdrif_dbg(1, sdr, "cur: idx %u mask %lu num %u\n",
> +				  i, sdr->cur_ch_mask, sdr->num_cur_ch);
> +			return 0;
> +		}
> +	}
> +
> +	if (rcar_drif_set_default_format(sdr)) {
> +		rdrif_err(sdr, "cannot set default format\n");
> +		return -EINVAL;
> +	}
> +
> +	f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> +	f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;
> +	return 0;
> +}
> +
> +static int rcar_drif_try_fmt_sdr_cap(struct file *file, void *priv,
> +				     struct v4l2_format *f)
> +{
> +	struct rcar_drif_sdr *sdr = video_drvdata(file);
> +	unsigned int i;
> +
> +	memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
> +	for (i = 0; i < NUM_FORMATS; i++) {
> +		if (formats[i].pixelformat == f->fmt.sdr.pixelformat) {
> +			f->fmt.sdr.buffersize = formats[i].buffersize;
> +			return 0;
> +		}
> +	}
> +
> +	f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> +	f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;

The result of the TRY_FMT ioctl should not depend on the currently configured 
format. I would return a fixed format (for instance the first one in the 
formats array) in the default case.

> +	return 0;
> +}
> +
> +/* Tuner subdev ioctls */
> +static int rcar_drif_enum_freq_bands(struct file *file, void *priv,
> +				     struct v4l2_frequency_band *band)
> +{
> +	struct rcar_drif_sdr *sdr = video_drvdata(file);
> +	struct v4l2_subdev *sd;
> +	int ret = 0;
> +
> +	v4l2_device_for_each_subdev(sd, &sdr->v4l2_dev) {
> +		ret = v4l2_subdev_call(sd, tuner, enum_freq_bands, band);

This won't work as-is when you'll have multiple subdevs. As the driver only 
supports a single connected subdev at the moment, I suggest storing a pointer 
to that subdev in the rcar_drif_sdr structure, and calling operations on that 
subdev explicitly instead of looping over all subdevs. The comment holds for 
all other ioctls.

> +		if (ret)
> +			break;
> +	}
> +	return ret;
> +}

[snip]

> +static int rcar_drif_notify_bound(struct v4l2_async_notifier *notifier,
> +				   struct v4l2_subdev *subdev,
> +				   struct v4l2_async_subdev *asd)
> +{
> +	struct rcar_drif_sdr *sdr =
> +		container_of(notifier, struct rcar_drif_sdr, notifier);
> +
> +	/* Nothing to do at this point */

If there's nothing to do you can just leave the bound callback unimplemented, 
it's optional.

> +	rdrif_dbg(2, sdr, "bound asd: %s\n", asd->match.of.node->name);
> +	return 0;
> +}
> +
> +/* Sub-device registered notification callback */
> +static int rcar_drif_notify_complete(struct v4l2_async_notifier *notifier)
> +{
> +	struct rcar_drif_sdr *sdr =
> +		container_of(notifier, struct rcar_drif_sdr, notifier);
> +	struct v4l2_subdev *sd;
> +	int ret;
> +
> +	sdr->v4l2_dev.ctrl_handler = &sdr->ctrl_hdl;
> +
> +	ret = v4l2_device_register_subdev_nodes(&sdr->v4l2_dev);
> +	if (ret) {
> +		rdrif_err(sdr, "failed register subdev nodes ret %d\n", ret);
> +		return ret;
> +	}

Do you need to expose subdev nodes to userspace ? Can't everything be handled 
from the V4L2 SDR node ?

> +	v4l2_device_for_each_subdev(sd, &sdr->v4l2_dev) {
> +		ret = v4l2_ctrl_add_handler(sdr->v4l2_dev.ctrl_handler,
> +					    sd->ctrl_handler, NULL);

Shouldn't you undo this somewhere when unbinding the subdevs ?

> +		if (ret) {
> +			rdrif_err(sdr, "failed ctrl add hdlr ret %d\n", ret);
> +			return ret;
> +		}
> +	}
> +	rdrif_dbg(2, sdr, "notify complete\n");
> +	return 0;
> +}

[snip]

> +/* Parse sub-devs (tuner) to find a matching device */
> +static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr,
> +				   struct device *dev)
> +{
> +	struct v4l2_async_notifier *notifier = &sdr->notifier;
> +	struct rcar_drif_async_subdev *rsd;
> +	struct device_node *node;
> +
> +	notifier->subdevs = devm_kzalloc(dev, sizeof(*notifier->subdevs),
> +					 GFP_KERNEL);
> +	if (!notifier->subdevs)
> +		return -ENOMEM;
> +
> +	node = of_graph_get_next_endpoint(dev->of_node, NULL);
> +	if (!node)
> +		return 0;
> +
> +	rsd = devm_kzalloc(dev, sizeof(*rsd), GFP_KERNEL);
> +	if (!rsd) {
> +		of_node_put(node);

If you move the allocation above of_graph_get_next_endpoint() you won't have 
to call of_node_put() in the error path.

> +		return -ENOMEM;
> +	}
> +
> +	notifier->subdevs[notifier->num_subdevs] = &rsd->asd;
> +	rsd->asd.match.of.node = of_graph_get_remote_port_parent(node);

Aren't you missing an of_node_put() on the returned node ? Or does the async 
framework take care of that ?

> +	of_node_put(node);
> +	if (!rsd->asd.match.of.node) {
> +		dev_warn(dev, "bad remote port parent\n");
> +		return -EINVAL;
> +	}
> +
> +	rsd->asd.match_type = V4L2_ASYNC_MATCH_OF;
> +	notifier->num_subdevs++;
> +
> +	/* Get the endpoint properties */
> +	rcar_drif_get_ep_properties(sdr, node);
> +	return 0;
> +}
> +
> +/* Check if the given device is the primary bond */
> +static bool rcar_drif_primary_bond(struct platform_device *pdev)
> +{
> +	if (of_find_property(pdev->dev.of_node, "renesas,primary-bond", NULL))
> +		return true;
> +
> +	return false;

How about

	return of_property_read_bool(pdev->dev.of_node,
				     "renesas,primary-bond");

> +}
> +
> +/* Get the bonded platform dev if enabled */
> +static struct platform_device *rcar_drif_enabled_bond(struct
> platform_device *p)
> +{
> +	struct device_node *np;
> +
> +	np = of_parse_phandle(p->dev.of_node, "renesas,bonding", 0);

The function takes a reference to np, you need to call of_node_put() on it 
(only if the returned pointer isn't NULL).

> +	if (np && of_device_is_available(np))
> +		return of_find_device_by_node(np);

of_find_device_by_node() takes a reference to the returned device, you need to 
call device_put() on it when you don't need it anymore.


> +	return NULL;
> +}
> +
> +/* Proble internal channel */
> +static int rcar_drif_channel_probe(struct platform_device *pdev)
> +{
> +	struct rcar_drif *ch;
> +	struct resource	*res;
> +	void __iomem *base;
> +	struct clk *clkp;

Maybe s/clkp/clk/ ?

> +	int ret;
> +
> +	/* Peripheral clock */
> +	clkp = devm_clk_get(&pdev->dev, "fck");
> +	if (IS_ERR(clkp)) {
> +		ret = PTR_ERR(clkp);
> +		dev_err(&pdev->dev, "clk get failed (%d)\n", ret);
> +		return ret;
> +	}

Isn't the clock managed automatically by runtime PM ?

> +	/* Register map */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base)) {
> +		ret = PTR_ERR(base);
> +		dev_err(&pdev->dev, "ioremap failed (%d)\n", ret);
> +		return ret;

devm_ioremap_resource() already prints an error message, you can remove this 
one.

> +	}
> +
> +	/* Reserve memory for enabled channel */
> +	ch = devm_kzalloc(&pdev->dev, sizeof(*ch), GFP_KERNEL);
> +	if (!ch) {
> +		ret = PTR_ERR(ch);
> +		dev_err(&pdev->dev, "failed alloc channel\n");

Memory allocation failures already print error messages, you can remove this 
one.

> +		return ret;
> +	}
> +	ch->pdev = pdev;
> +	ch->clkp = clkp;
> +	ch->base = base;
> +	ch->start = res->start;

If you allocated the ch structure first you could set the fields directly 
without a need for local variables.

> +	platform_set_drvdata(pdev, ch);
> +	return 0;
> +}
> +
> +static int rcar_drif_probe(struct platform_device *pdev)
> +{
> +	struct rcar_drif *ch, *b_ch = NULL;
> +	struct platform_device *b_pdev;
> +	struct rcar_drif_sdr *sdr;
> +	int ret;
> +
> +	/* Probe internal channel */
> +	ret = rcar_drif_channel_probe(pdev);
> +	if (ret)
> +		return ret;

I would have done it the other way around, inlining the 
rcar_drif_channel_probe() function here as that's the common case, and moving 
the V4L2 SDR device initialization code to a different function.

> +	/* Check if both channels of the bond are enabled */
> +	b_pdev = rcar_drif_enabled_bond(pdev);
> +	if (b_pdev) {
> +		/* Check if current channel acting as primary-bond */
> +		if (!rcar_drif_primary_bond(pdev)) {
> +			dev_notice(&pdev->dev, "probed\n");
> +			return 0;
> +		}
> +
> +		/* Check if the other device is probed */
> +		b_ch = platform_get_drvdata(b_pdev);
> +		if (!b_ch) {
> +			dev_info(&pdev->dev, "defer probe\n");
> +			return -EPROBE_DEFER;
> +		}

Isn't this all very racy ? What if the other channel's device is removed while 
this one is probed ?

> +		/* Set the other channel number */
> +		b_ch->num = 1;

Reading data from the other channel's private structure is one thing, but 
writing it makes me shiver :-S Could we make it so that 0 is the slave and 1 
the master ? That way you would set ch->num = 1 instead of b_ch->num = 1, 
keeping all modifications to the private structure local to the device being 
probed.

> +	}
> +
> +	/* Channel acting as SDR instance */
> +	ch = platform_get_drvdata(pdev);
> +	ch->acting_sdr = true;
> +
> +	/* Reserve memory for SDR structure */
> +	sdr = devm_kzalloc(&pdev->dev, sizeof(*sdr), GFP_KERNEL);
> +	if (!sdr) {
> +		ret = PTR_ERR(sdr);
> +		dev_err(&pdev->dev, "failed alloc drif context\n");
> +		return ret;
> +	}
> +	sdr->dev = &pdev->dev;
> +	sdr->hw_ch_mask = BIT(ch->num);
> +
> +	/* Establish links between SDR and channel(s) */
> +	ch->sdr = sdr;
> +	sdr->ch[ch->num] = ch;
> +	if (b_ch) {
> +		sdr->ch[b_ch->num] = b_ch;
> +		b_ch->sdr = sdr;
> +		sdr->hw_ch_mask |= BIT(b_ch->num);
> +	}
> +	sdr->num_hw_ch = hweight_long(sdr->hw_ch_mask);
> +
> +	/* Validate any supported format for enabled channels */
> +	ret = rcar_drif_set_default_format(sdr);
> +	if (ret) {
> +		dev_err(sdr->dev, "failed to set default format\n");
> +		return ret;
> +	}
> +
> +	/* Set defaults */
> +	sdr->hwbuf_size = RCAR_DRIF_DEFAULT_HWBUF_SIZE;
> +
> +	mutex_init(&sdr->v4l2_mutex);
> +	mutex_init(&sdr->vb_queue_mutex);
> +	spin_lock_init(&sdr->queued_bufs_lock);
> +	INIT_LIST_HEAD(&sdr->queued_bufs);
> +
> +	/* Init videobuf2 queue structure */
> +	sdr->vb_queue.type = V4L2_BUF_TYPE_SDR_CAPTURE;
> +	sdr->vb_queue.io_modes = VB2_READ | VB2_MMAP | VB2_DMABUF;
> +	sdr->vb_queue.drv_priv = sdr;
> +	sdr->vb_queue.buf_struct_size = sizeof(struct rcar_drif_frame_buf);
> +	sdr->vb_queue.ops = &rcar_drif_vb2_ops;
> +	sdr->vb_queue.mem_ops = &vb2_vmalloc_memops;
> +	sdr->vb_queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> +
> +	/* Init videobuf2 queue */
> +	ret = vb2_queue_init(&sdr->vb_queue);
> +	if (ret) {
> +		dev_err(sdr->dev, "could not initialize vb2 queue\n");
> +		return ret;
> +	}
> +
> +	/* Register the v4l2_device */
> +	ret = v4l2_device_register(&pdev->dev, &sdr->v4l2_dev);
> +	if (ret) {
> +		dev_err(sdr->dev, "failed v4l2_device_register (%d)\n", ret);

Maybe "failed to register V4L2 device" to make it a real sentence ? :-)

> +		return ret;
> +	}
> +
> +	/*
> +	 * Parse subdevs after v4l2_device_register because if the subdev
> +	 * is already probed, bound and complete will be called immediately
> +	 */
> +	ret = rcar_drif_parse_subdevs(sdr, &pdev->dev);
> +	if (ret)
> +		goto err_unreg_v4l2;
> +
> +	sdr->notifier.bound = rcar_drif_notify_bound;
> +	sdr->notifier.complete = rcar_drif_notify_complete;
> +
> +	v4l2_ctrl_handler_init(&sdr->ctrl_hdl, 10);

Possibly a stupid question, why 10, if you don't create any control in this 
driver ?

> +	/* Register notifier */
> +	ret = v4l2_async_notifier_register(&sdr->v4l2_dev, &sdr->notifier);
> +	if (ret < 0) {
> +		dev_err(sdr->dev, "notifier registration failed (%d)\n", ret);
> +		goto err_free_ctrls;
> +	}
> +
> +	/* Init video_device structure */
> +	sdr->vdev = video_device_alloc();
> +	if (!sdr->vdev) {
> +		ret = -ENOMEM;
> +		goto err_unreg_notif;
> +	}
> +	snprintf(sdr->vdev->name, sizeof(sdr->vdev->name), "R-Car DRIF");
> +	sdr->vdev->fops = &rcar_drif_fops;
> +	sdr->vdev->ioctl_ops = &rcar_drif_ioctl_ops;
> +	sdr->vdev->release = video_device_release;
> +	sdr->vdev->lock = &sdr->v4l2_mutex;
> +	sdr->vdev->queue = &sdr->vb_queue;
> +	sdr->vdev->queue->lock = &sdr->vb_queue_mutex;
> +	sdr->vdev->ctrl_handler = &sdr->ctrl_hdl;
> +	sdr->vdev->v4l2_dev = &sdr->v4l2_dev;
> +	sdr->vdev->device_caps = V4L2_CAP_SDR_CAPTURE | V4L2_CAP_TUNER |
> +		V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
> +	video_set_drvdata(sdr->vdev, sdr);
> +
> +	/* Register V4L2 SDR device */
> +	ret = video_register_device(sdr->vdev, VFL_TYPE_SDR, -1);
> +	if (ret) {
> +		dev_err(sdr->dev, "failed video_register_device (%d)\n", ret);

Same here, "failed to register video device" ?

> +		goto err_unreg_notif;
> +	}
> +
> +	dev_notice(sdr->dev, "probed\n");

Do you think this message is really useful ? I believe it would just add a bit 
more noise to the kernel log, without any real use.

> +	return 0;
> +
> +err_unreg_notif:
> +	video_device_release(sdr->vdev);
> +	v4l2_async_notifier_unregister(&sdr->notifier);
> +err_free_ctrls:
> +	v4l2_ctrl_handler_free(&sdr->ctrl_hdl);
> +err_unreg_v4l2:
> +	v4l2_device_unregister(&sdr->v4l2_dev);
> +	return ret;
> +}
> +
> +static int rcar_drif_remove(struct platform_device *pdev)
> +{
> +	struct rcar_drif *ch = platform_get_drvdata(pdev);
> +	struct rcar_drif_sdr *sdr = ch->sdr;
> +
> +	if (!ch->acting_sdr) {

Isn't it possible to check the channel number instead and remove the 
acting_sdr field ?

> +		/* Nothing to do */
> +		dev_notice(&pdev->dev, "removed\n");
> +		return 0;
> +	}
> +
> +	/* SDR instance */
> +	v4l2_ctrl_handler_free(sdr->vdev->ctrl_handler);
> +	v4l2_async_notifier_unregister(&sdr->notifier);
> +	v4l2_device_unregister(&sdr->v4l2_dev);
> +	video_unregister_device(sdr->vdev);
> +	dev_notice(&pdev->dev, "removed\n");

Even more than the probed message, I think this one can go away.

> +	return 0;
> +}
> +
> +static int __maybe_unused rcar_drif_suspend(struct device *dev)
> +{
> +	return 0;

Maybe a /* FIXME: Implement suspend/resume support */ ?

> +}
> +
> +static int __maybe_unused rcar_drif_resume(struct device *dev)
> +{
> +	return 0;

Same here ?

> +}

[snip]

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html