Re: [PATCH v6 5/8] dmaengine: dw: dmamux: Introduce RZN1 DMA router support

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 4 Apr 2022, Miquel Raynal 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.
> 
> Aside from the driver introduction, as these devices are described as
> subnodes of the system controller, we also need the system controller
> (clock) driver to populate its children manually. Starting from now on,
> one child can be the dmamux.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---

> +static void *rzn1_dmamux_route_allocate(struct of_phandle_args *dma_spec,
> +					struct of_dma *ofdma)
> +{
> +	struct platform_device *pdev = of_find_device_by_node(ofdma->of_node);
> +	struct rzn1_dmamux_data *dmamux = platform_get_drvdata(pdev);
> +	struct rzn1_dmamux_map *map;
> +	unsigned int dmac_idx, chan, val;
> +	u32 mask;
> +	int ret;
> +
> +	if (dma_spec->args_count != 6)
> +		return ERR_PTR(-EINVAL);
> +
> +	map = kzalloc(sizeof(*map), GFP_KERNEL);
> +	if (!map)
> +		return ERR_PTR(-ENOMEM);
> +
> +	chan = dma_spec->args[0];
> +	map->req_idx = dma_spec->args[4];
> +	val = dma_spec->args[5];
> +	dma_spec->args_count -= 2;
> +
> +	if (chan >= RZN1_DMAMUX_SPLIT) {
> +		dev_err(&pdev->dev, "Invalid DMA request line: %u\n", chan);
> +		ret = -EINVAL;
> +		goto free_map;
> +	}
> +
> +	if (map->req_idx >= RZN1_DMAMUX_LINES ||
> +	    (map->req_idx % RZN1_DMAMUX_SPLIT) != chan) {
> +		dev_err(&pdev->dev, "Invalid MUX request line: %u\n", map->req_idx);
> +		ret = -EINVAL;
> +		goto free_map;
> +	}
> +
> +	dmac_idx = map->req_idx < RZN1_DMAMUX_SPLIT ? 0 : 1;
> +	dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", dmac_idx);
> +	if (!dma_spec->np) {
> +		dev_err(&pdev->dev, "Can't get DMA master\n");
> +		ret = -EINVAL;
> +		goto free_map;
> +	}
> +
> +	dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n",
> +		map->req_idx, dmac_idx, chan);
> +
> +	mask = BIT(map->req_idx);
> +	mutex_lock(&dmamux->lock);
> +	dmamux->used_chans |= mask;
> +	ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0);
> +	mutex_unlock(&dmamux->lock);
> +	if (ret)
> +		goto free_mux;

Move this check before the unlock. Then free_mux path doesn't need to do
(re)lock as the mutex is still held. You might also want to consider 
renaming the label e.g. to chan_release_and_unlock.

> +
> +	return map;
> +
> +free_mux:
> +	mutex_lock(&dmamux->lock);
> +	dmamux->used_chans &= ~mask;
> +	mutex_unlock(&dmamux->lock);
> +free_map:
> +	kfree(map);
> +
> +	return ERR_PTR(ret);
> +}


-- 
 i.