Re: [PATCH v4 1/2] dma: at_xdmac: creation of the atmel eXtended DMA Controller driver

On Monday 15 September 2014, Ludovic Desroches wrote:

 
+config AT_XDMAC
+	tristate "Atmel XDMA support"
+	depends on ARCH_AT91
+	select DMA_ENGINE
+	help
+	  Support the Atmel XDMA controller.

This should depend on (ARCH_AT91 || COMPILE_TEST) and built on x86 to
get into the usual automated build checkers.

+static struct dma_chan *at_xdmac_xlate(struct of_phandle_args *dma_spec,
+				       struct of_dma *of_dma)
+{
+	struct at_xdmac		*atxdmac = of_dma->of_dma_data;
+	struct at_xdmac_chan	*atchan;
+	struct dma_chan		*chan;
+	struct device		*dev = atxdmac->dma.dev;
+	dma_cap_mask_t 		mask;
+
+	if (dma_spec->args_count != 2) {
+		dev_err(dev, "dma phandler args: bad number of args\n");
+		return NULL;
+	}
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);

The mask is unused, just remove it.

+	chan = dma_get_any_slave_channel(&atxdmac->dma);
+	if (!chan) {
+		dev_err(dev, "can't get a dma channel\n");
+		return NULL;
+	}
+
+	atchan = to_at_xdmac_chan(chan);
+	atchan->memif = AT91_XDMAC_DT_GET_MEM_IF(dma_spec->args[0]);
+	atchan->perif = AT91_XDMAC_DT_GET_PER_IF(dma_spec->args[0]);
+	atchan->perid = AT91_XDMAC_DT_GET_PERID(dma_spec->args[1]);
+	dev_info(dev, "chan dt cfg: memif=%u perif=%u perid=%u\n",
+		 atchan->memif, atchan->perif, atchan->perid);

Maybe dev_dbg instead of dev_info?

I think having three cells would be nicer here, so you can get rid of the
macros.

The header is only used in one file, so just move the contents into that file.

+
+static inline void __iomem *at_xdmac_chan_reg_base(struct at_xdmac *atxdmac, unsigned int chan_nb)
+{
+	return (void __iomem *)(atxdmac->regs + (AT_XDMAC_CHAN_REG_BASE + chan_nb * 0x40));
+}

That type cast should not be needed. Is atxdmac->regs not already a void __iomem *

+#define at_xdmac_read(atxdmac, reg) readl_relaxed((atxdmac)->regs + (reg))
+#define at_xdmac_write(atxdmac, reg, value) \
+	writel_relaxed((value), (atxdmac)->regs + (reg))
+
+#define at_xdmac_chan_read(atchan, reg) readl_relaxed((atchan)->ch_regs + (reg))
+#define at_xdmac_chan_write(atchan, reg, value) writel_relaxed((value), (atchan)->ch_regs + (reg))

Is writel_relaxed the right accessor here? I haven't reviewed all uses of this,
but you have to ensure that every use that needs synchronization against the
actual DMA has explicit barriers here.
(0x2 << AT91_DMA_CFG_FIFOCFG_OFFSET)	/* single AHB access */

 
+
+/* ---------- XDMAC ---------- */
+#define AT91_XDMAC_DT_MEM_IF_MASK	(0x1)
+#define AT91_XDMAC_DT_MEM_IF_OFFSET	(16)
+#define AT91_XDMAC_DT_MEM_IF(mem_if)	(((mem_if) & AT91_XDMAC_DT_MEM_IF_MASK) \
+					<< AT91_XDMAC_DT_MEM_IF_OFFSET)
+#define AT91_XDMAC_DT_GET_MEM_IF(cfg)	(((cfg) >> AT91_XDMAC_DT_MEM_IF_OFFSET) \
+					& AT91_XDMAC_DT_MEM_IF_MASK)
+
+#define AT91_XDMAC_DT_PER_IF_MASK	(0x1)
+#define AT91_XDMAC_DT_PER_IF_OFFSET	(0)
+#define AT91_XDMAC_DT_PER_IF(per_if)	(((per_if) & AT91_XDMAC_DT_PER_IF_MASK) \
+					<< AT91_XDMAC_DT_PER_IF_OFFSET)
+#define AT91_XDMAC_DT_GET_PER_IF(cfg)	(((cfg) >> AT91_XDMAC_DT_PER_IF_OFFSET) \
+					& AT91_XDMAC_DT_PER_IF_MASK)
+
+#define AT91_XDMAC_DT_PERID_MASK	(0x7f)
+#define AT91_XDMAC_DT_PERID_OFFSET	(24)
+#define AT91_XDMAC_DT_PERID(perid)	(((perid) & AT91_XDMAC_DT_PERID_MASK) \
+					<< AT91_XDMAC_DT_PERID_OFFSET)
+#define AT91_XDMAC_DT_GET_PERID(cfg)	(((cfg) >> AT91_XDMAC_DT_PERID_OFFSET) \
+					& AT91_XDMAC_DT_PERID_MASK)

As mentioned, I think it would be much better to keep the macros inside of the
driver and not visible to the binding, so you can use simple numbers in DT.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html