RE: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-Car devices

[Fabrizio Castro <fabrizio.castro.jz@renesas.com>]

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:
> > On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro 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@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.
> 
> > > +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.
> 
> We've suffered enough with DMA to user pointers. Let's use dmabuf
> instead.

Will give it a try

Thanks,
Fab


> 
> --
> Regards,
> 
> Laurent Pinchart