Hi Fabrizio,

Thank you for the patch.

On Fri, Feb 26, 2021 at 11:37:44AM +0100, Arnd Bergmann wrote:

On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro wrote:

quoted

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@renesas.com>
---
 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?

I second Arnd here, a standard API would be best.

quoted

+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.

quoted

+
+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.

quoted

+
+       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.

quoted

+
+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.

We've suffered enough with DMA to user pointers. Let's use dmabuf
instead.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel