From: andy.shevchenko@gmail.com
To: Nikita Shubin <nikita.shubin@maquefel.me>
Cc: Alexander Sverdlin <alexander.sverdlin@gmail.com>,
Arnd Bergmann <arnd@arndb.de>,
Linus Walleij <linus.walleij@linaro.org>,
Hartley Sweeten <hsweeten@visionengravers.com>,
Russell King <linux@armlinux.org.uk>,
Vinod Koul <vkoul@kernel.org>,
Michael Peters <mpeters@embeddedts.com>,
Kris Bahnsen <kris@embeddedts.com>,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org
Subject: Re: [PATCH v1 22/43] dma: cirrus: add DT support for Cirrus EP93xx
Date: Sat, 3 Jun 2023 23:39:05 +0300 [thread overview]
Message-ID: <ZHuk6Tc8_MNzGxO4@surfacebook> (raw)
In-Reply-To: <20230601054549.10843-4-nikita.shubin@maquefel.me>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-06-03 20:39 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20230424123522.18302-1-nikita.shubin@maquefel.me>
2023-04-24 12:34 ` [PATCH 01/43] gpio: ep93xx: split device in multiple Nikita Shubin
2023-04-24 12:34 ` [PATCH 34/43] ARM: dts: add device tree for ep93xx Soc Nikita Shubin
2023-04-24 11:28 ` Arnd Bergmann
2023-04-28 15:13 ` Nikita Shubin
2023-04-28 21:56 ` Kris Bahnsen
2023-04-24 12:34 ` [PATCH 35/43] ARM: ep93xx: DT for the Cirrus ep93xx SoC platforms Nikita Shubin
2023-06-01 5:33 ` [PATCH v1 01/43] gpio: ep93xx: split device in multiple Nikita Shubin
2023-06-02 1:50 ` andy.shevchenko
2023-06-15 16:56 ` Nikita Shubin
2023-06-01 5:45 ` [PATCH v1 20/43] net: cirrus: add DT support for Cirrus EP93xx Nikita Shubin
2023-06-02 7:27 ` Linus Walleij
2023-06-02 12:09 ` Andrew Lunn
2023-06-03 20:30 ` andy.shevchenko
2023-06-04 15:51 ` Alexander Sverdlin
2023-06-01 5:45 ` [PATCH v1 22/43] dma: " Nikita Shubin
2023-06-03 20:39 ` andy.shevchenko [this message]
2023-06-01 5:45 ` [PATCH v1 35/43] ARM: dts: add device tree for ep93xx Soc Nikita Shubin
2023-06-01 8:30 ` Krzysztof Kozlowski
2023-07-05 16:06 ` Nikita Shubin
2023-06-01 5:45 ` [PATCH v1 36/43] ARM: ep93xx: DT for the Cirrus ep93xx SoC platforms Nikita Shubin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZHuk6Tc8_MNzGxO4@surfacebook \
--to=andy.shevchenko@gmail.com \
--cc=alexander.sverdlin@gmail.com \
--cc=arnd@arndb.de \
--cc=dmaengine@vger.kernel.org \
--cc=hsweeten@visionengravers.com \
--cc=kris@embeddedts.com \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=mpeters@embeddedts.com \
--cc=nikita.shubin@maquefel.me \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).