RE: [PATCH v3 7/7] media: platform: rcar_drif: Add DRIF support
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 > > > > --- > > 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 > > 000..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->
Re: [PATCH v3 7/7] media: platform: rcar_drif: Add DRIF support
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
>
> ---
> 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 000..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,
> +
