Re: [PATCH v4 06/18] dmaengine: st_fdma: Add STMicroelectronics FDMA engine driver support

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Wed 25 May 09:06 PDT 2016, Peter Griffin wrote:

> diff --git a/drivers/dma/st_fdma.c b/drivers/dma/st_fdma.c
[..]
> +
> +static int st_fdma_alloc_chan_res(struct dma_chan *chan)
> +{
> +	struct st_fdma_chan *fchan = to_st_fdma_chan(chan);
> +	int ret;
> +
> +	/* Create the dma pool for descriptor allocation */
> +	fchan->node_pool = dma_pool_create(dev_name(&chan->dev->device),
> +					    fchan->fdev->dev,
> +					    sizeof(struct st_fdma_hw_node),
> +					    __alignof__(struct st_fdma_hw_node),
> +					    0);
> +
> +	if (!fchan->node_pool) {
> +		dev_err(fchan->fdev->dev, "unable to allocate desc pool\n");
> +		return -ENOMEM;
> +	}
> +
> +
> +	ret = rproc_boot(fchan->fdev->rproc);
> +	if (ret) {
> +		dev_err(fchan->fdev->dev, "rproc_boot failed\n");

rproc_boot(), rproc_fw_boot() and xp70_rproc_start() has already given
you specific error messages; drop this generic message.

Don't you want to destroy the dma_pool if this happens?

> +		return ret;
> +	}
> +
> +	dev_dbg(fchan->fdev->dev, "alloc ch_id:%d type:%d\n",
> +		fchan->vchan.chan.chan_id, fchan->cfg.type);
> +
> +	return 0;
> +}
> +
[..]
> +
> +static const struct st_fdma_driverdata fdma_mpe31_stih407_11 = {
> +	.name = "STiH407",
> +	.id = 0,
> +};
> +
> +static const struct st_fdma_driverdata fdma_mpe31_stih407_12 = {
> +	.name = "STiH407",
> +	.id = 1,
> +};
> +
> +static const struct st_fdma_driverdata fdma_mpe31_stih407_13 = {
> +	.name = "STiH407",
> +	.id = 2,
> +};
> +
> +static const struct of_device_id st_fdma_match[] = {
> +	{ .compatible = "st,stih407-fdma-mpe31-11"
> +	  , .data = &fdma_mpe31_stih407_11 },
> +	{ .compatible = "st,stih407-fdma-mpe31-12"
> +	  , .data = &fdma_mpe31_stih407_12 },
> +	{ .compatible = "st,stih407-fdma-mpe31-13"
> +	  , .data = &fdma_mpe31_stih407_13 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, st_fdma_match);
> +
> +static int st_fdma_parse_dt(struct platform_device *pdev,
> +			const struct st_fdma_driverdata *drvdata,
> +			struct st_fdma_dev *fdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	int ret;
> +
> +	if (!np)
> +		goto err;

ret is left uninitialized here. Also as no error handling is done, just
return your error directly.

> +
> +	ret = of_property_read_u32(np, "dma-channels", &fdev->nr_channels);
> +	if (ret)
> +		goto err;
> +
> +	snprintf(fdev->fw_name, FW_NAME_SIZE, "fdma_%s_%d.elf",
> +		drvdata->name, drvdata->id);
> +
> +err:
> +	return ret;
> +}
> +#define FDMA_DMA_BUSWIDTHS	(BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | \
> +				 BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | \
> +				 BIT(DMA_SLAVE_BUSWIDTH_3_BYTES) | \
> +				 BIT(DMA_SLAVE_BUSWIDTH_4_BYTES))
> +
> +static int st_fdma_probe(struct platform_device *pdev)
> +{
> +	struct st_fdma_dev *fdev;
> +	const struct of_device_id *match;
> +	struct device_node *np = pdev->dev.of_node;
> +	const struct st_fdma_driverdata *drvdata;
> +	int ret, i;
> +
> +	match = of_match_device((st_fdma_match), &pdev->dev);
> +	if (!match || !match->data) {
> +		dev_err(&pdev->dev, "No device match found\n");
> +		return -ENODEV;
> +	}
> +
> +	drvdata = match->data;

Use of_device_get_match_data() instead. This also allows you to keep the
of_match_table and driverdata at the end of the file.

[..]
> +
> +	fdev->rproc = xp70_rproc_alloc(pdev, fdev->fw_name);
> +	if (!fdev->rproc) {
> +		dev_err(&pdev->dev, "xp70_rproc_init failed\n");

All (non-development) error paths in xp70_rproc_alloc() will have
already printed a more specific error message, so no need to print
another generic on here.

> +		ret = PTR_ERR(fdev->rproc);
> +		goto err;
> +	}
> +

Regards,
Bjorn