Re: [PATCH v10 5/9] dmaengine: dw: dmamux: Introduce RZN1 DMA router support

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Miquel,

On Wed, Apr 13, 2022 at 10:00 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> geert@linux-m68k.org wrote on Wed, 13 Apr 2022 09:53:09 +0200:
> > On Tue, Apr 12, 2022 at 9:39 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > The Renesas RZN1 DMA IP is based on a DW core, with eg. an additional
> > > dmamux register located in the system control area which can take up to
> > > 32 requests (16 per DMA controller). Each DMA channel can be wired to
> > > two different peripherals.
> > >
> > > We need two additional information from the 'dmas' property: the channel
> > > (bit in the dmamux register) that must be accessed and the value of the
> > > mux for this channel.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >
> > Thanks for your patch!
> >
> > > --- /dev/null
> > > +++ b/drivers/dma/dw/rzn1-dmamux.c
> > > @@ -0,0 +1,160 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2022 Schneider-Electric
> > > + * Author: Miquel Raynal <miquel.raynal@bootlin.com
> > > + * Based on TI crossbar driver written by Peter Ujfalusi <peter.ujfalusi@ti.com>
> > > + */
> > > +#include <linux/bitops.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_dma.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/soc/renesas/r9a06g032-sysctrl.h>
> > > +#include <linux/types.h>
> > > +
> > > +#define RNZ1_DMAMUX_NCELLS 6
> > > +#define RZN1_DMAMUX_LINES 64
> > > +#define RZN1_DMAMUX_MAX_LINES 16
> > > +
> > > +struct rzn1_dmamux_data {
> > > +       struct dma_router dmarouter;
> > > +       unsigned long *used_chans;
> >
> > Why a pointer?
> >
> > > +static int rzn1_dmamux_probe(struct platform_device *pdev)
> > > +{
> > > +       struct device_node *mux_node = pdev->dev.of_node;
> > > +       const struct of_device_id *match;
> > > +       struct device_node *dmac_node;
> > > +       struct rzn1_dmamux_data *dmamux;
> > > +
> > > +       dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
> > > +       if (!dmamux)
> > > +               return -ENOMEM;
> > > +
> > > +       dmamux->used_chans = devm_bitmap_zalloc(&pdev->dev, 2 * RZN1_DMAMUX_MAX_LINES,
> > > +                                               GFP_KERNEL);
> >
> > ... Oh, you want to allocate the bitmap separately, although you
> > know it's just a single long.
> >
> > You might as well declare it in rzn1_dmamux_data as:
> >
> >     unsigned long used_chans[BITS_TO_LONGS(2 * RZN1_DMAMUX_MAX_LINES)];
>
> I've done that in versions v1..v8 and it was explicitly requested by
> Ilpo that I used something more specific like a bitmap (or an idr, but
> I don't think it fits well here). So now I'm using a bitmap...

Right, my bad ;-)

    DECLARE_BITMAP(used_chans, 2 * RZN1_DMAMUX_MAX_LINES);

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