lkml.org 
[lkml]   [2023]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v1 22/43] dma: cirrus: add DT support for Cirrus EP93xx
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


\
 
 \ /
  Last update: 2023-06-03 22:39    [W:0.596 / U:0.140 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site