Hi Geert,

Thanks for the feedback.

Subject: Re: [PATCH v3 2/4] drivers: dma: sh: Add DMAC driver for RZ/G2L
SoC

Hi Biju,

On Fri, Jul 2, 2021 at 12:05 PM Biju Das [off-list ref]
wrote:

quoted

Add DMA Controller driver for RZ/G2L SoC.

Based on the work done by Chris Brandt for RZ/A DMA driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

quoted

--- /dev/null
+++ b/drivers/dma/sh/rz-dmac.c

quoted

+static void rz_dmac_set_dmars_register(struct rz_dmac *dmac, int nr,
+                                      u32 dmars) {
+       u32 dmars_offset = (nr / 2) * 4;
+       u32 dmars32;
+
+       dmars32 = rz_dmac_ext_readl(dmac, dmars_offset);
+       if (nr % 2) {
+               dmars32 &= 0x0000ffff;
+               dmars32 |= dmars << 16;
+       } else {
+               dmars32 &= 0xffff0000;
+               dmars32 |= dmars;
+       }

An alternative to Vinod's suggestion:

    shift = (nr %2) * 16;
    dmars32 &= ~(0xffff << shift);
    dmars32 |= dmars << shift;

OK.

quoted

+
+       rz_dmac_ext_writel(dmac, dmars32, dmars_offset); }

quoted

+static int rz_dmac_chan_probe(struct rz_dmac *dmac,
+                             struct rz_dmac_chan *channel,
+                             unsigned int index) {
+       struct platform_device *pdev = to_platform_device(dmac->dev);
+       struct rz_lmdesc *lmdesc;
+       char pdev_irqname[5];
+       char *irqname;
+       int ret;
+
+       channel->index = index;
+       channel->mid_rid = -EINVAL;
+
+       /* Request the channel interrupt. */
+       sprintf(pdev_irqname, "ch%u", index);
+       channel->irq = platform_get_irq_byname(pdev, pdev_irqname);
+       if (channel->irq < 0)
+               return -ENODEV;

Please propagate the error in channel->irq, which might be -EPROBE_DEFER.

OK.

quoted

+static int rz_dmac_parse_of(struct device *dev, struct rz_dmac *dmac)
+{
+       struct device_node *np = dev->of_node;
+       int ret;
+
+       ret = of_property_read_u32(np, "dma-channels", &dmac-
n_channels);
+       if (ret < 0) {
+               dev_err(dev, "unable to read dma-channels property\n");
+               return ret;
+       }
+
+       if (!dmac->n_channels || dmac->n_channels >

RZ_DMAC_MAX_CHANNELS) {

quoted

+               dev_err(dev, "invalid number of channels %u\n", dmac-
n_channels);
+               return -EINVAL;
+       }
+
+       return 0;
+}
+
+static int rz_dmac_probe(struct platform_device *pdev) {
+       const char *irqname = "error";
+       struct dma_device *engine;
+       struct rz_dmac *dmac;
+       int channel_num;
+       int ret, i;

unsigned int i;

OK.

quoted

+       int irq;
+
+       dmac = devm_kzalloc(&pdev->dev, sizeof(*dmac), GFP_KERNEL);
+       if (!dmac)
+               return -ENOMEM;
+
+       dmac->dev = &pdev->dev;
+       platform_set_drvdata(pdev, dmac);
+
+       ret = rz_dmac_parse_of(&pdev->dev, dmac);
+       if (ret < 0)
+               return ret;
+
+       dmac->channels = devm_kcalloc(&pdev->dev, dmac->n_channels,
+                                     sizeof(*dmac->channels),

GFP_KERNEL);

quoted

+       if (!dmac->channels)
+               return -ENOMEM;
+
+       /* Request resources */
+       dmac->base = devm_platform_ioremap_resource(pdev, 0);
+       if (IS_ERR(dmac->base))
+               return PTR_ERR(dmac->base);
+
+       dmac->ext_base = devm_platform_ioremap_resource(pdev, 1);
+       if (IS_ERR(dmac->ext_base))
+               return PTR_ERR(dmac->ext_base);
+
+       /* Register interrupt handler for error */
+       irq = platform_get_irq_byname(pdev, irqname);
+       if (irq < 0) {
+               dev_err(&pdev->dev, "no error IRQ specified\n");
+               return -ENODEV;

I'd say "return dev_err_probe(&pdev->dev, irq, ..);", but
platform_get_irq_byname() already prints an error message, so please just
use "return irq;" to propagate the error, which could be -EPROBE_DEFER.

OK. Will just return irq;

quoted

+       }

quoted

+static int rz_dmac_remove(struct platform_device *pdev) {
+       struct rz_dmac *dmac = platform_get_drvdata(pdev);
+       int i;

unsigned int i;

OK.

Regards,
Biju