Re: [PATCH 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Miquel Raynal <miquel.raynal@bootlin.com>]

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