Hi Andy,
andriy.shevchenko@xxxxxxxxxxxxxxx wrote on Sun, 20 Feb 2022 12:56:02
+0200:
> On Fri, Feb 18, 2022 at 07:12:22PM +0100, Miquel Raynal wrote:
> > The Renesas RZN1 DMA IP is a 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.
>
> ...
Thanks for the review!
>
> > +dw_dmac-y := platform.o dmamux.o
>
> We do not need this on other platforms, please make sure we have no dangling
> code on, e.g., x86.
>
> ...
>
> > + /* The of_node_put() will be done in the core for the node */
> > + master = map->req_idx < dmamux->dmac_requests ? 0 : 1;
>
> The opposite conditional will be better, no?`
I guess this is a matter of taste. I prefer the current writing but I
will change it.
>
> ...
>
> > + dmamux->used_chans |= BIT(map->req_idx);
> > + ret = r9a06g032_syscon_set_dmamux(BIT(map->req_idx),
> > + val ? BIT(map->req_idx) : 0);
>
>
> Cleaner to do
>
> u32 mask = BIT(...);
> ...
>
> dmamux->used_chans |= mask;
> ret = r9a06g032_syscon_set_dmamux(mask, val ? mask : 0);
Fine.
>
> ...
>
> > +static const struct of_device_id rzn1_dmac_match[] __maybe_unused = {
> > + { .compatible = "renesas,rzn1-dma", },
> > + {},
>
> No comma for terminator entry.
Mmh, ok.
>
> > +};
>
> ...
>
> > + if (!node)
> > + return -ENODEV;
>
> Dup check, why not to simply try for phandle first?
I'll drop it.
>
> ...
>
> > + if (of_property_read_u32(dmac_node, "dma-requests",
> > + &dmamux->dmac_requests)) {
>
> One line?
Ok.
>
> > + dev_err(&pdev->dev, "Missing DMAC requests information\n");
> > + of_node_put(dmac_node);
> > + return -EINVAL;
>
> First put node, then simply use dev_err_probe().
I don't get the point here. dev_err_probe() is useful when -EPROBE_DEFER
can be returned, right? I don't understand what it would bring here nor
how I should use it to simplify error handling.
>
> > + }
>
> ...
>
> > +static const struct of_device_id rzn1_dmamux_match[] = {
> > + { .compatible = "renesas,rzn1-dmamux", },
> > + {},
>
> No comma.
Ok.
>
> > +};
>
Thanks,
Miquèl
