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

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

[Arnd Bergmann]

On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> 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?

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

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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

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

[Fabrizio Castro]

Hi Arnd,

Thanks for your feedback!

> From: Arnd Bergmann <arnd@kernel.org>
> Sent: 26 February 2021 10:38
> Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-
> Car devices
> 
> On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> 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 wasn't entirely sure were to put it to be honest as I couldn't find
anything similar, that's why "misc" for v1, to mainly get a feedback
and advice.

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

I hear you, the trouble is that the modules of this IP should be seen
as part of a SW and HW processing pipeline.
Some of the SW components of the pipeline can be proprietary, and 
unfortunately we don't have access (yet) to a framework that allows us to
test and demonstrate the whole pipeline, for the moment we can only test
things in isolation. It'll take us a while to come up with a full solution
(or to have access to one), and in the meantime our SoCs come with an IP
that can't be used because there is no driver for it (yet).

After discussing things further with Geert and Laurent, I think they
are right in saying that the best path for this is probably to add this
driver under "drivers/staging" as an interim solution, so that the IP will
be accessible by developers, and when we have everything we need (a fully
fledged processing framework), we will able to integrate it better with
the other components (if possible).

> 
> 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?

This IP is similar to a DMA to some extent, in the sense that it takes
input data from a buffer in memory and it writes output data onto a buffer
in memory, and of course it does some processing in between.
It's not directly connected to any other Radio related IP.
The user space application can read DAB IQ samples from the R-Car DRIF
interface (via driver drivers/media/platform/rcar_drif.c, or from other
sources since this IP is decoupled from DRIF), and then when it's time
to accelerate FFT, FIC, or MSC, it can request services to the DAB IP, so
that the app can go on and use the processor to do some work, while the DAB
IP processes things.
A framework to connect and exchange processing blocks (either SW or HW) and
interfaces is therefore vital to properly place a kernel implementation
in the great scheme of things, in its absence a simple driver can help
us and users to integrate DAB acceleration within an interim DAB processing
pipeline, and that will lead to better interfaces in the future.

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

Will add

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

Thanks.

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

Will do

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

Thanks,
Fab


> 
>       Arnd

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

[Fabrizio Castro]

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

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

[Ezequiel Garcia]

Hello everyone,

On Mon, 1 Mar 2021 at 14:36, Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
>
> Hi Arnd,
>
> Thanks for your feedback!
>
> > From: Arnd Bergmann <arnd@kernel.org>
> > Sent: 26 February 2021 10:38
> > Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-
> > Car devices
> >
> > On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro
> > <fabrizio.castro.jz@renesas.com> 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 wasn't entirely sure were to put it to be honest as I couldn't find
> anything similar, that's why "misc" for v1, to mainly get a feedback
> and advice.
>
> >
> > 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.
>
> I hear you, the trouble is that the modules of this IP should be seen
> as part of a SW and HW processing pipeline.
> Some of the SW components of the pipeline can be proprietary, and
> unfortunately we don't have access (yet) to a framework that allows us to
> test and demonstrate the whole pipeline, for the moment we can only test
> things in isolation. It'll take us a while to come up with a full solution
> (or to have access to one), and in the meantime our SoCs come with an IP
> that can't be used because there is no driver for it (yet).
>
> After discussing things further with Geert and Laurent, I think they
> are right in saying that the best path for this is probably to add this
> driver under "drivers/staging" as an interim solution, so that the IP will
> be accessible by developers, and when we have everything we need (a fully
> fledged processing framework), we will able to integrate it better with
> the other components (if possible).
>
> >
> > 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?
>
> This IP is similar to a DMA to some extent, in the sense that it takes
> input data from a buffer in memory and it writes output data onto a buffer
> in memory, and of course it does some processing in between.

That sounds like something that can fit V4L2 MEM2MEM driver.
You queue two buffers and an operation, and then run a job.

> It's not directly connected to any other Radio related IP.
> The user space application can read DAB IQ samples from the R-Car DRIF
> interface (via driver drivers/media/platform/rcar_drif.c, or from other
> sources since this IP is decoupled from DRIF), and then when it's time
> to accelerate FFT, FIC, or MSC, it can request services to the DAB IP, so
> that the app can go on and use the processor to do some work, while the DAB
> IP processes things.
> A framework to connect and exchange processing blocks (either SW or HW) and
> interfaces is therefore vital to properly place a kernel implementation
> in the great scheme of things, in its absence a simple driver can help

I'm not entirely sure we are missing a framework. What's missing in
V4L2 MEM2MEM?

Before considering drivers/staging, it would be interesting to figure
out if V4L2 can do it as-is, and if not, discuss what's missing.

Thanks,
Ezequiel

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

[Fabrizio Castro]

Hello Ezequiel,


Thanks for your feedback!

> From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Sent: 02 March 2021 11:17
> Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-
> Car devices
> 
> Hello everyone,
> 
> On Mon, 1 Mar 2021 at 14:36, Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> >
> > Hi Arnd,
> >
> > Thanks for your feedback!
> >
> > > From: Arnd Bergmann <arnd@kernel.org>
> > > Sent: 26 February 2021 10:38
> > > Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas
> R-
> > > Car devices
> > >
> > > On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro
> > > <fabrizio.castro.jz@renesas.com> 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 wasn't entirely sure were to put it to be honest as I couldn't find
> > anything similar, that's why "misc" for v1, to mainly get a feedback
> > and advice.
> >
> > >
> > > 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.
> >
> > I hear you, the trouble is that the modules of this IP should be seen
> > as part of a SW and HW processing pipeline.
> > Some of the SW components of the pipeline can be proprietary, and
> > unfortunately we don't have access (yet) to a framework that allows us
> to
> > test and demonstrate the whole pipeline, for the moment we can only test
> > things in isolation. It'll take us a while to come up with a full
> solution
> > (or to have access to one), and in the meantime our SoCs come with an IP
> > that can't be used because there is no driver for it (yet).
> >
> > After discussing things further with Geert and Laurent, I think they
> > are right in saying that the best path for this is probably to add this
> > driver under "drivers/staging" as an interim solution, so that the IP
> will
> > be accessible by developers, and when we have everything we need (a
> fully
> > fledged processing framework), we will able to integrate it better with
> > the other components (if possible).
> >
> > >
> > > 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?
> >
> > This IP is similar to a DMA to some extent, in the sense that it takes
> > input data from a buffer in memory and it writes output data onto a
> buffer
> > in memory, and of course it does some processing in between.
> 
> That sounds like something that can fit V4L2 MEM2MEM driver.
> You queue two buffers and an operation, and then run a job.

V4L2 MEM2MEM seems good for this in general, however the DAB IP is going
to be shared by multiple processing pipelines (as usually we have several
DRIF interfaces, and only 1 DAB IP), and for what concerns FFT specifically
there is only 1 FFT module inside the DAB IP.
My understanding is that the capabilities of V4L2 MEM2MEM devices are
configured for the specific pipeline, but in the this context user space
would have to continuously switch the capabilities of the DAB IP (at the
right moment) to allow processing for data streams requiring different
capabilities.

Am I wrong?

> 
> > It's not directly connected to any other Radio related IP.
> > The user space application can read DAB IQ samples from the R-Car DRIF
> > interface (via driver drivers/media/platform/rcar_drif.c, or from other
> > sources since this IP is decoupled from DRIF), and then when it's time
> > to accelerate FFT, FIC, or MSC, it can request services to the DAB IP,
> so
> > that the app can go on and use the processor to do some work, while the
> DAB
> > IP processes things.
> > A framework to connect and exchange processing blocks (either SW or HW)
> and
> > interfaces is therefore vital to properly place a kernel implementation
> > in the great scheme of things, in its absence a simple driver can help
> 
> I'm not entirely sure we are missing a framework. What's missing in
> V4L2 MEM2MEM?

I was referring to a user space framework (I should have been more specific
with my previous email).

> 
> Before considering drivers/staging, it would be interesting to figure
> out if V4L2 can do it as-is, and if not, discuss what's missing.

I think an interim solution would allow us and users to evaluate things a
little bit better, so that we can integrate this IP with V4L2 later on.

Thanks,
Fab

> 
> Thanks,
> Ezequiel

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

[Laurent Pinchart]

Hi Fabrizio,

On Tue, Mar 02, 2021 at 12:20:17PM +0000, Fabrizio Castro wrote:
> On 02 March 2021 11:17, Ezequiel Garcia wrote:
> > On Mon, 1 Mar 2021 at 14:36, Fabrizio Castro wrote:
> > > On 26 February 2021 10:38, 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 wasn't entirely sure were to put it to be honest as I couldn't find
> > > anything similar, that's why "misc" for v1, to mainly get a feedback
> > > and advice.
> > >
> > > > 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.
> > >
> > > I hear you, the trouble is that the modules of this IP should be seen
> > > as part of a SW and HW processing pipeline.
> > > Some of the SW components of the pipeline can be proprietary, and
> > > unfortunately we don't have access (yet) to a framework that allows us to
> > > test and demonstrate the whole pipeline, for the moment we can only test
> > > things in isolation. It'll take us a while to come up with a full solution
> > > (or to have access to one), and in the meantime our SoCs come with an IP
> > > that can't be used because there is no driver for it (yet).
> > >
> > > After discussing things further with Geert and Laurent, I think they
> > > are right in saying that the best path for this is probably to add this
> > > driver under "drivers/staging" as an interim solution, so that the IP will
> > > be accessible by developers, and when we have everything we need (a fully
> > > fledged processing framework), we will able to integrate it better with
> > > the other components (if possible).
> > >
> > > > 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?
> > >
> > > This IP is similar to a DMA to some extent, in the sense that it takes
> > > input data from a buffer in memory and it writes output data onto a buffer
> > > in memory, and of course it does some processing in between.
> > 
> > That sounds like something that can fit V4L2 MEM2MEM driver.
> > You queue two buffers and an operation, and then run a job.
> 
> V4L2 MEM2MEM seems good for this in general, however the DAB IP is going
> to be shared by multiple processing pipelines (as usually we have several
> DRIF interfaces, and only 1 DAB IP), and for what concerns FFT specifically
> there is only 1 FFT module inside the DAB IP.
> My understanding is that the capabilities of V4L2 MEM2MEM devices are
> configured for the specific pipeline, but in the this context user space
> would have to continuously switch the capabilities of the DAB IP (at the
> right moment) to allow processing for data streams requiring different
> capabilities.
> 
> Am I wrong?

V4L2 M2M devices can be opened multiple times, but different processes,
which can configure the device in different ways, and submit jobs that
are then scheduled by the V4L2 M2M framework.

> > > It's not directly connected to any other Radio related IP.
> > > The user space application can read DAB IQ samples from the R-Car DRIF
> > > interface (via driver drivers/media/platform/rcar_drif.c, or from other
> > > sources since this IP is decoupled from DRIF), and then when it's time
> > > to accelerate FFT, FIC, or MSC, it can request services to the DAB IP, so
> > > that the app can go on and use the processor to do some work, while the DAB
> > > IP processes things.
> > > A framework to connect and exchange processing blocks (either SW or HW) and
> > > interfaces is therefore vital to properly place a kernel implementation
> > > in the great scheme of things, in its absence a simple driver can help
> > 
> > I'm not entirely sure we are missing a framework. What's missing in
> > V4L2 MEM2MEM?
> 
> I was referring to a user space framework (I should have been more specific
> with my previous email).
> 
> > Before considering drivers/staging, it would be interesting to figure
> > out if V4L2 can do it as-is, and if not, discuss what's missing.
> 
> I think an interim solution would allow us and users to evaluate things a
> little bit better, so that we can integrate this IP with V4L2 later on.

-- 
Regards,

Laurent Pinchart

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

[Fabrizio Castro]

Hi Laurent,

Thanks for your feedback!

> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 02 March 2021 12:32
> Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-
> Car devices
> 
> Hi Fabrizio,
> 
> On Tue, Mar 02, 2021 at 12:20:17PM +0000, Fabrizio Castro wrote:
> > On 02 March 2021 11:17, Ezequiel Garcia wrote:
> > > On Mon, 1 Mar 2021 at 14:36, Fabrizio Castro wrote:
> > > > On 26 February 2021 10:38, 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 wasn't entirely sure were to put it to be honest as I couldn't
> find
> > > > anything similar, that's why "misc" for v1, to mainly get a feedback
> > > > and advice.
> > > >
> > > > > 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.
> > > >
> > > > I hear you, the trouble is that the modules of this IP should be
> seen
> > > > as part of a SW and HW processing pipeline.
> > > > Some of the SW components of the pipeline can be proprietary, and
> > > > unfortunately we don't have access (yet) to a framework that allows
> us to
> > > > test and demonstrate the whole pipeline, for the moment we can only
> test
> > > > things in isolation. It'll take us a while to come up with a full
> solution
> > > > (or to have access to one), and in the meantime our SoCs come with
> an IP
> > > > that can't be used because there is no driver for it (yet).
> > > >
> > > > After discussing things further with Geert and Laurent, I think they
> > > > are right in saying that the best path for this is probably to add
> this
> > > > driver under "drivers/staging" as an interim solution, so that the
> IP will
> > > > be accessible by developers, and when we have everything we need (a
> fully
> > > > fledged processing framework), we will able to integrate it better
> with
> > > > the other components (if possible).
> > > >
> > > > > 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?
> > > >
> > > > This IP is similar to a DMA to some extent, in the sense that it
> takes
> > > > input data from a buffer in memory and it writes output data onto a
> buffer
> > > > in memory, and of course it does some processing in between.
> > >
> > > That sounds like something that can fit V4L2 MEM2MEM driver.
> > > You queue two buffers and an operation, and then run a job.
> >
> > V4L2 MEM2MEM seems good for this in general, however the DAB IP is going
> > to be shared by multiple processing pipelines (as usually we have
> several
> > DRIF interfaces, and only 1 DAB IP), and for what concerns FFT
> specifically
> > there is only 1 FFT module inside the DAB IP.
> > My understanding is that the capabilities of V4L2 MEM2MEM devices are
> > configured for the specific pipeline, but in the this context user space
> > would have to continuously switch the capabilities of the DAB IP (at the
> > right moment) to allow processing for data streams requiring different
> > capabilities.
> >
> > Am I wrong?
> 
> V4L2 M2M devices can be opened multiple times, but different processes,
> which can configure the device in different ways, and submit jobs that
> are then scheduled by the V4L2 M2M framework.

I'll give it a try in due time then.

I think the clock related patches are worth merging.

Thanks,
Fab

> 
> > > > It's not directly connected to any other Radio related IP.
> > > > The user space application can read DAB IQ samples from the R-Car
> DRIF
> > > > interface (via driver drivers/media/platform/rcar_drif.c, or from
> other
> > > > sources since this IP is decoupled from DRIF), and then when it's
> time
> > > > to accelerate FFT, FIC, or MSC, it can request services to the DAB
> IP, so
> > > > that the app can go on and use the processor to do some work, while
> the DAB
> > > > IP processes things.
> > > > A framework to connect and exchange processing blocks (either SW or
> HW) and
> > > > interfaces is therefore vital to properly place a kernel
> implementation
> > > > in the great scheme of things, in its absence a simple driver can
> help
> > >
> > > I'm not entirely sure we are missing a framework. What's missing in
> > > V4L2 MEM2MEM?
> >
> > I was referring to a user space framework (I should have been more
> specific
> > with my previous email).
> >
> > > Before considering drivers/staging, it would be interesting to figure
> > > out if V4L2 can do it as-is, and if not, discuss what's missing.
> >
> > I think an interim solution would allow us and users to evaluate things
> a
> > little bit better, so that we can integrate this IP with V4L2 later on.
> 
> --
> Regards,
> 
> Laurent Pinchart

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

[Geert Uytterhoeven]

Hi Fabrizio,

On Wed, Mar 3, 2021 at 11:20 AM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Sent: 02 March 2021 12:32
> > Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-
> > Car devices
> >
> > On Tue, Mar 02, 2021 at 12:20:17PM +0000, Fabrizio Castro wrote:
> > > On 02 March 2021 11:17, Ezequiel Garcia wrote:
> > > > On Mon, 1 Mar 2021 at 14:36, Fabrizio Castro wrote:
> > > > > On 26 February 2021 10:38, 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>

[...]

> I think the clock related patches are worth merging.

Indeed. I had already marked them to be applied, after you received
confirmation about the parent clocks.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Fabrizio Castro]

Hi Geert,

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 03 March 2021 10:27
> Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-
> Car devices
> 
> Hi Fabrizio,
> 
> On Wed, Mar 3, 2021 at 11:20 AM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Sent: 02 March 2021 12:32
> > > Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas
> R-
> > > Car devices
> > >
> > > On Tue, Mar 02, 2021 at 12:20:17PM +0000, Fabrizio Castro wrote:
> > > > On 02 March 2021 11:17, Ezequiel Garcia wrote:
> > > > > On Mon, 1 Mar 2021 at 14:36, Fabrizio Castro wrote:
> > > > > > On 26 February 2021 10:38, 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>
> 
> [...]
> 
> > I think the clock related patches are worth merging.
> 
> Indeed. I had already marked them to be applied, after you received
> confirmation about the parent clocks.

Thanks,
Fab

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But
> when I'm talking to journalists I just say "programmer" or something like
> that.
>                                 -- Linus Torvalds