Thu, Jun 01, 2023 at 08:45:27AM +0300, Nikita Shubin kirjoitti:
> - find register range from the device tree
> - get clocks, interrupts from device tree
...
> --- a/arch/arm/mach-ep93xx/dma.c
> +++ b/arch/arm/mach-ep93xx/dma.c
> @@ -19,6 +19,7 @@
> #include <linux/init.h>
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> +#include <linux/of.h>
> #include <linux/platform_device.h>
>
> #include <linux/platform_data/dma-ep93xx.h>
Stray change.
...
> --- a/drivers/dma/ep93xx_dma.c
> +++ b/drivers/dma/ep93xx_dma.c
> @@ -20,6 +20,7 @@
> #include <linux/dmaengine.h>
> #include <linux/module.h>
> #include <linux/mod_devicetable.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
...
> - struct ep93xx_dma_chan channels[];
> + struct ep93xx_dma_chan *channels;
Why? This is helpful to allocate main structure and channels in one go.
...
> @@ -875,9 +881,11 @@ static int ep93xx_dma_alloc_chan_resources(struct dma_chan *chan)
> if (!edmac->edma->m2m) {
> if (!data)
> return -EINVAL;
> +
> if (data->port < EP93XX_DMA_I2S1 ||
> data->port > EP93XX_DMA_IRDA)
> return -EINVAL;
> +
> if (data->direction != ep93xx_dma_chan_direction(chan))
> return -EINVAL;
> } else {
Seems unrelated change.
> ep93xx_dma_advance_work(to_ep93xx_dma_chan(chan));
> }
...
> +static const struct of_device_id ep93xx_dma_of_ids[] = {
> + { .compatible = "cirrus,ep9301-dma-m2p", .data = &edma_m2p },
> + { .compatible = "cirrus,ep9301-dma-m2m", .data = &edma_m2m },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ep93xx_dma_of_ids);
Move this closer to the real user (see below).
Also this ID table shouldn't be under ifdeffery.
...
> + struct device_node *np = pdev->dev.of_node;
> + const struct of_device_id *match = of_match_node(ep93xx_dma_of_ids, pdev->dev.of_node);
> + const struct ep93xx_edma_data *data = match->data;
No NULL check? Why do you have this duplication, btw?
> + struct dma_device *dma_dev = &edma->dma_dev;
> + int num_channels;
> + int i;
> +
> + match = of_match_device((ep93xx_dma_of_ids), &pdev->dev);
> + if (!match || !match->data) {
> + dev_err(&pdev->dev, "No device match found\n");
> + return -ENODEV;
> + }
Use of_device_get_match_data().
...
> + edma->channels = devm_kzalloc(&pdev->dev,
> + num_channels * sizeof(struct ep93xx_dma_chan),
> + GFP_KERNEL);
> + if (!edma->channels)
> + return -ENOMEM;
Simply no. See below what to do.
...
> + for (i = 0; i < num_channels; i++) {
> + struct ep93xx_dma_chan *edmac = &edma->channels[i];
> +
> + edmac->chan.device = dma_dev;
> + edmac->regs = devm_platform_ioremap_resource(pdev, i);
No error check?
> + edmac->irq = platform_get_irq(pdev, i);
No error check?
> + edmac->edma = edma;
> +
> + edmac->clk = of_clk_get(np, i);
Can this actually use clk_get() or its devm_*() variant?
> +
Redundant blank line.
> + if (IS_ERR(edmac->clk)) {
> + dev_warn(&pdev->dev, "failed to get clock\n");
> + continue;
> + }
> +
> + spin_lock_init(&edmac->lock);
> + INIT_LIST_HEAD(&edmac->active);
> + INIT_LIST_HEAD(&edmac->queue);
> + INIT_LIST_HEAD(&edmac->free_list);
> + tasklet_setup(&edmac->tasklet, ep93xx_dma_tasklet);
> +
> + list_add_tail(&edmac->chan.device_node,
> + &dma_dev->channels);
> + }
...
> - edma_size = pdata->num_channels * sizeof(struct ep93xx_dma_chan);
> - edma = kzalloc(sizeof(*edma) + edma_size, GFP_KERNEL);
> - if (!edma)
> + edma->channels = devm_kzalloc(&pdev->dev,
> + pdata->num_channels * sizeof(struct ep93xx_dma_chan),
> + GFP_KERNEL);
> + if (!edma->channels)
> return -ENOMEM;
No. Just include overflow.h and use struct_size().
...
> + edma = devm_kzalloc(&pdev->dev, sizeof(*edma), GFP_KERNEL);
> +
No error check?!
...
> + if (platform_get_device_id(pdev))
> + ret = ep93xx_init_from_pdata(pdev, edma);
> + else
> + ret = ep93xx_dma_of_probe(pdev, edma);
> +
Redundant blank line.
> + if (ret)
> + return ret;
--
With Best Regards,
Andy Shevchenko
