Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-Car devices
From: Arnd Bergmann
Date: Fri Feb 26 2021 - 05:40:09 EST
On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro
<fabrizio.castro.jz@xxxxxxxxxxx> wrote:
>
>
The DAB hardware accelerator found on R-Car E3 and R-Car M3-N devices is
>
a hardware accelerator for software DAB demodulators.
>
It consists of one FFT (Fast Fourier Transform) module and one decoder
>
module, compatible with DAB specification (ETSI EN 300 401 and
>
ETSI TS 102 563).
>
The decoder module can perform FIC decoding and MSC decoding processing
>
from de-puncture to final decoded result.
>
>
This patch adds a device driver to support the FFT module only.
>
>
Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
>
---
>
MAINTAINERS | 7 ++
>
drivers/misc/Kconfig | 1 +
>
drivers/misc/Makefile | 1 +
>
drivers/misc/rcar_dab/Kconfig | 11 ++
>
drivers/misc/rcar_dab/Makefile | 8 ++
>
drivers/misc/rcar_dab/rcar_dev.c | 176 +++++++++++++++++++++++++++++++
>
drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++++++++++
>
drivers/misc/rcar_dab/rcar_fft.c | 160 ++++++++++++++++++++++++++++
>
include/uapi/linux/rcar_dab.h | 35 ++++++
Can you explain why this is not in drivers/media/?
I don't think we want a custom ioctl interface for a device that implements
a generic specification. My first feeling would be that this should not
have a user-level API but instead get called by the DAB radio driver.
What is the intended usage model here? I assume the idea is to
use it in an application that receives audio or metadata from DAB.
What driver do you use for that?
>
+static long rcar_dab_unlocked_ioctl(struct file *file, unsigned int cmd,
>
+ unsigned long arg)
>
+{
>
+ void __user *argp = (void __user *)arg;
>
+ struct rcar_dab *dab;
>
+ int ret;
>
+
>
+ dab = container_of(file->private_data, struct rcar_dab, misc);
>
+
>
+ switch (cmd) {
>
+ case RCAR_DAB_IOC_FFT:
>
+ if (!access_ok(argp, sizeof(struct rcar_dab_fft_req)))
>
+ return -EFAULT;
>
+ ret = rcar_dab_fft(dab, argp);
>
+ break;
>
+ default:
>
+ ret = -ENOTTY;
>
+ }
>
+
>
+ return ret;
>
+}
>
+
>
+static const struct file_operations rcar_dab_fops = {
>
+ .owner = THIS_MODULE,
>
+ .unlocked_ioctl = rcar_dab_unlocked_ioctl,
>
+};
There should be a '.compat_ioctl = compat_ptr_ioctl'
entry, provided that the arguments are compatible between
32-bit and 64-bit user space.
>
+
>
+static int rcar_dab_fft_init(struct rcar_dab *dab, struct rcar_dab_fft_req *fft)
>
+{
>
+ u32 mode;
>
+
>
+ for (mode = 0; mode < ARRAY_SIZE(rcar_dab_fft_size_lut); mode++)
>
+ if (rcar_dab_fft_size_lut[mode] == fft->points)
>
+ break;
>
+ if (mode == ARRAY_SIZE(rcar_dab_fft_size_lut))
>
+ return -EINVAL;
>
+ if (fft->ofdm_number == 0)
>
+ return -EINVAL;
>
+
>
+ rcar_dab_write(dab, RCAR_DAB_FFTSSR, mode);
>
+ rcar_dab_write(dab, RCAR_DAB_FFTNUMOFDMR, fft->ofdm_number);
>
+ rcar_dab_write(dab, RCAR_DAB_FFTINADDR, (u32)dab->fft.dma_input_buf);
>
+ rcar_dab_write(dab, RCAR_DAB_FFTOUTADDR, (u32)dab->fft.dma_output_buf);
Maybe use lower_32_bits() instead of the (u32) cast.
For clarity, you may also want to specifically ask for a 32-bit DMA mask
in the probe function, with a comment that describes what the hardware
limitation is.
>
+
>
+ if (copy_from_user(dab->fft.input_buffer, fft_req->input_address,
>
+ buffer_size)) {
>
+ mutex_unlock(&dab->fft.lock);
>
+ return -EFAULT;
>
+ }
>
+
>
+ dab->fft.done = false;
>
+ ret = rcar_dab_fft_init(dab, fft_req);
>
+ if (ret) {
>
+ mutex_unlock(&dab->fft.lock);
>
+ return ret;
>
+ }
>
+
>
+ rcar_dab_fft_enable(dab);
>
+ wait_event_interruptible_timeout(dab->fft.wait, dab->fft.done, HZ);
>
+ if (!dab->fft.done) {
>
+ rcar_dab_fft_disable(dab);
>
+ ret = -EFAULT;
-EFAULT doesn't look like the right error for timeout or signal
handling. Better check the return code from wait_event_interruptible_timeout()
instead.
>
+
>
+struct rcar_dab_fft_req {
>
+ int points; /*
>
+ * The number of points to use.
>
+ * Legal values are 256, 512, 1024, and
>
+ * 2048.
>
+ */
>
+ unsigned char ofdm_number; /*
>
+ * Orthogonal Frequency Division
>
+ * Multiplexing (OFDM).
>
+ * Minimum value is 1, maximum value is
>
+ * 255.
>
+ */
>
+ void __user *input_address; /*
>
+ * User space address for the input
>
+ * buffer.
>
+ */
>
+ void __user *output_address; /*
>
+ * User space address for the output
>
+ * buffer.
>
+ */
>
+};
Please read Documentation/driver-api/ioctl.rst and make this a portable
data structure.
Arnd