Hi Laurent,
Many thanks for your time & the review comments. I have agreed to most of the comments and a few need further discussion. Could you please take a look at those?
> 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@xxxxxxxxxxxxxx>
> > ---
> > 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/
Agreed
>
> > + * 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.
Agreed.
>
> > +
> > +#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 ?
Just style only. The #define needs a RCAR_DRIF_ prefix and I used this value in few places. The #define makes the statements longer.
>
> > +/* 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 ?
Agreed
>
> 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.
Actually single channel support is tested internally. If single & dual channels are already supported any future change will be just adding the new SDR format only, which could be relatively trivial for this driver. To add new SDR formats today I need to enable appropriate format in the tuner code too, which could be done later on a need basis by us or others.
>
> > + },
> > + {
> > + .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.
Agreed.
>
> > +
> > +/* 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.
>
Agreed
> > +
> > + /* 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.
Agreed
>
> > + 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.
Agreed.
>
> > +{
> > + 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.
Agreed
>
> > + 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.
Agreed
>
> > + /* 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.
I would like to keep this statement please. This single register print would clarify if a user selected DT options and this could be enabled at runtime on a deployed board.
>
> > + }
> > +}
> > +
> > +/* 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 ?
Agreed
>
> > + 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.
Agreed
>
> > + }
> > +
> > + 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.
But, this is the set format function?
>
> > + }
> > +
> > + /* 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.
Yes. We thought about this issue and considered this approach as a trade-off to avoid DRIF overflow issue. Overflow happens DMAC fails to consume from DRIF FIFO on time (i.e.) when user app is busy processing samples and fails to queue buffers to DMAC. After an overflow, the device needs to reset (streaming stop/restart sequence). With cyclic DMA, we de-coupled the h/w and user buffers to avoid a costly device reset when user app is busy or not scheduled. Some samples will be lost but that is identified with the sequence numbers and the action/policy can be left to the user. Are you OK with this please?
>
> > + 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);
Agreed
>
> > +}
> > +
> > +/* 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 engine API mentions the callback will be called in tasklet context. Hmm... I could convert that into spin_lock_irq() if that's OK.
> > +
> > + /* 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.
rcar-dmac did not have device_synchronize support, when I started with this patch set. There is a comment in the terminal_all call
1224 /*
1225 * FIXME: No new interrupt can occur now, but the IRQ thread might still
1226 * be running.
1227 */
Hence, I was a bit paranoid.
>
> > + 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 ?
It takes ~6-7µs on my board. The manual did not specify the expected time and I used a worst case of 500ms as this is used in the on/off path only.
Would 7µs sleep time be acceptable instead of 2µs?
>
> > + 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 ?
Same as above comment.
>
> > + 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.
If I remember correctly, it is a checkpatch warning when I started with the patch set (Last year Oct timeframe).
>
> > + 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 ?
Thank you. I missed this :-(.
>
> > + 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 ?
Yes, I think it can be removed. I cannot remember why I added retry here because rcar-dmac terminate_all always seem to return 0. Last year, there used to be a WARN_ON in rcar-dmac code when channel halt fails (i.e.) it is not guaranteed that all transfers are stopped even after calling dmaengine_terminate_sync(). With the character version of this driver, I used to have a test app to start/stream/stop/restart sequence & hit the WARN_ON in dmac code sometimes (https://www.spinics.net/lists/linux-renesas-soc/msg04840.html). I may have added this return value check assuming it is passed on but I cannot see that even in the old code.
So yes, I agree with your comment.
>
> > + 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.
I did not see it. Please correct me if this is wrong
v4l_streamon
vb2_ioctl_streamon
vb2_streamon
vb2_core_streamon
vb2_start_streaming
q->ops->start_streaming => rcar_drif_start_streaming
>
> > + 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.
Agreed.
>
> > + 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.
Agreed
>
> > + .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
Agreed
>
> > + 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.
Agreed
>
> > + 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.
Agreed
>
> > + 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.
Agreed.
>
> > + 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.
Agreed.
>
> > + 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 ?
As of today, everything can be handled from the V4L2 SDR node with current MAX2175 subdev. If the tuner driver is enhanced later, this would help.
>
> > + 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.
Agreed
>
> > + 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 ?
You are right. of_node_put() on the returned node is missing. I will add it.
>
> > + 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");
Shall I remove this function itself and just use the property in the "if" condition please? It's used in one place only. I tend to not touch the bindings name/type as we have some sort of agreement after ~4months time :-)
>
> > +}
> > +
> > +/* 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).
Agreed
>
> > + 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.
Agreed & Thanks again.
>
>
> > + 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/ ?
Agreed
>
> > + 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 ?
I think the driver need to support runtime PM in order to manage the clock. Otherwise it just gets adds the clk to genpd (_prepare) without enable/disable. I need to double check this.
>
> > + /* 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.
Agreed
>
> > + }
> > +
> > + /* 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.
Agreed.
>
> > + 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.
Agreed.
>
> > + 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.
Agreed
>
> > + /* 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 ?
OK. Will holding the device_lock(&b_pdev->dev) is sufficient?
>
> > + /* 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.
Agreed.
>
> > + }
> > +
> > + /* 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 ? :-)
Agreed
>
> > + 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 ?
To accommodate the subdev controls.
>
> > + /* 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" ?
Agreed.
>
> > + 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.
OK. Will remove it.
>
> > + 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 ?
Agreed.
>
> > + /* 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.
Agreed.
>
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused rcar_drif_suspend(struct device *dev)
> > +{
> > + return 0;
>
> Maybe a /* FIXME: Implement suspend/resume support */ ?
Agreed.
>
> > +}
> > +
> > +static int __maybe_unused rcar_drif_resume(struct device *dev)
> > +{
> > + return 0;
>
> Same here ?
Agreed.
Thanks,
Ramesh
