Hi Laurent,

Thank you for your feedback!

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: 26 February 2021 13:05
Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-
Car devices

Hi Fabrizio,

Thank you for the patch.

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

quoted

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

quoted

quoted

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

quoted

quoted

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

+++++++++++++++++++++++++++++++

quoted

quoted

 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

quoted

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

quoted

+static long rcar_dab_unlocked_ioctl(struct file *file, unsigned int

cmd,

quoted

quoted

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

quoted

quoted

+{
+       u32 mode;
+
+       for (mode = 0; mode < ARRAY_SIZE(rcar_dab_fft_size_lut);

mode++)

quoted

quoted

+               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);

quoted

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

quoted

+                          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);

quoted

quoted

+       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()

quoted

instead.

quoted

+
+struct rcar_dab_fft_req {
+       int points;                     /*
+                                        * The number of points to

use.

quoted

quoted

+                                        * Legal values are 256, 512,

1024, and

quoted

quoted

+                                        * 2048.
+                                        */
+       unsigned char ofdm_number;      /*
+                                        * Orthogonal Frequency

Division

quoted

quoted

+                                        * Multiplexing (OFDM).
+                                        * Minimum value is 1, maximum

value is

quoted

quoted

+                                        * 255.
+                                        */
+       void __user *input_address;     /*
+                                        * User space address for the

input

quoted

quoted

+                                        * buffer.
+                                        */
+       void __user *output_address;    /*
+                                        * User space address for the

output

quoted

quoted

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

Will give it a try

Thanks,
Fab

--
Regards,

Laurent Pinchart

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