Hi Andy,

andriy.shevchenko@linux.intel.com wrote on Sun, 20 Feb 2022 12:56:02
+0200:

On Fri, Feb 18, 2022 at 07:12:22PM +0100, Miquel Raynal wrote:

quoted

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!

quoted

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

...

quoted

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

...

quoted

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

...

quoted

+static const struct of_device_id rzn1_dmac_match[] __maybe_unused = {
+	{ .compatible = "renesas,rzn1-dma", },
+	{},  

No comma for terminator entry.

Mmh, ok.

quoted

+};  

...

quoted

+	if (!node)
+		return -ENODEV;  

Dup check, why not to simply try for phandle first?

I'll drop it.

...

quoted

+	if (of_property_read_u32(dmac_node, "dma-requests",
+				 &dmamux->dmac_requests)) {  

One line?

Ok.

quoted

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

quoted

+	}  

...

quoted

+static const struct of_device_id rzn1_dmamux_match[] = {
+	{ .compatible = "renesas,rzn1-dmamux", },
+	{},  

No comma.

Ok.

quoted

+};  

Thanks,
Miquèl