diff mbox

[1/3] ARM: at91: dt: add header to define at_hdmac configuration

Message ID 1369930103-11963-2-git-send-email-ludovic.desroches@atmel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ludovic Desroches May 30, 2013, 4:08 p.m. UTC 
From: Ludovic Desroches <ludovic.desroches@atmel.com>

DMA-cell content is a concatenation of several values. In order to keep this
stuff human readable, macros are introduced.

The values for the FIFO configuration are not the same as the ones used in the
configuration register in order to keep backward compatibility. Most devices
use the half FIFO configuration but USART ones have to use the ASAP
configuration. This parameter was not initially planed to be into the at91 dma
dt binding. The third cell will be used to store this parameter, it will
become a concatenation of the FIFO configuration and of the peripheral ID. In
order to keep backward compatibility i.e. FIFO configuration is equal to 0, we
have to perform a translation since the value to put in the register to set
half FIFO is 1.

Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 include/dt-bindings/dma/at91.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 include/dt-bindings/dma/at91.h

Comments

Arnd Bergmann May 31, 2013, 9:30 p.m. UTC | #1 
On Thursday 30 May 2013, ludovic.desroches@atmel.com wrote:
> +/*
> + * Source and/or destination peripheral ID
> + */
> +#define AT91_DMA_CFG_PER_ID_MASK       (0xff)
> +#define AT91_DMA_CFG_PER_ID(id)                (id & AT91_DMA_CFG_PER_ID_MASK)
> +

I'm a little worried about macros like this one spreading in the name
of readability, and I don't see this as actual value-add here:

The output of the macro is always the same as the input, and passing
an invalid number still results in an invalid output and no error message,
it actually prevents us from warning about the mistake at run-time.

The other macros are fine.

	Arnd
Ludovic Desroches June 3, 2013, 7:32 a.m. UTC | #2 
On Fri, May 31, 2013 at 11:30:29PM +0200, Arnd Bergmann wrote:
> On Thursday 30 May 2013, ludovic.desroches@atmel.com wrote:
> > +/*
> > + * Source and/or destination peripheral ID
> > + */
> > +#define AT91_DMA_CFG_PER_ID_MASK       (0xff)
> > +#define AT91_DMA_CFG_PER_ID(id)                (id & AT91_DMA_CFG_PER_ID_MASK)
> > +
> 
> I'm a little worried about macros like this one spreading in the name
> of readability, and I don't see this as actual value-add here:
> 
> The output of the macro is always the same as the input, and passing
> an invalid number still results in an invalid output and no error message,
> it actually prevents us from warning about the mistake at run-time.
> 

You are right, this macro is here only for readability in order to not have
something like this:

dmas = <&dma0 AT91_DMA_CFG_FIFOCFG_ASAP | 13>

In this case, I prefer having a macro instead of using a numeric value which
can stand for other things than the peripheral id and which could conflict
with other parameters such as the FIFO configuration.

If adding this macro is really an issue, it could be removed of course but
in this case I am in favour of readability.

Ludovic

> The other macros are fine.
> 
> 	Arnd
Nicolas Ferre June 4, 2013, 1:17 p.m. UTC | #3 
On 03/06/2013 09:32, Ludovic Desroches :
> On Fri, May 31, 2013 at 11:30:29PM +0200, Arnd Bergmann wrote:
>> On Thursday 30 May 2013, ludovic.desroches@atmel.com wrote:
>>> +/*
>>> + * Source and/or destination peripheral ID
>>> + */
>>> +#define AT91_DMA_CFG_PER_ID_MASK       (0xff)
>>> +#define AT91_DMA_CFG_PER_ID(id)                (id & AT91_DMA_CFG_PER_ID_MASK)
>>> +
>>
>> I'm a little worried about macros like this one spreading in the name
>> of readability, and I don't see this as actual value-add here:
>>
>> The output of the macro is always the same as the input, and passing
>> an invalid number still results in an invalid output and no error message,
>> it actually prevents us from warning about the mistake at run-time.
>>
>
> You are right, this macro is here only for readability in order to not have
> something like this:
>
> dmas = <&dma0 AT91_DMA_CFG_FIFOCFG_ASAP | 13>
>
> In this case, I prefer having a macro instead of using a numeric value which
> can stand for other things than the peripheral id and which could conflict
> with other parameters such as the FIFO configuration.
>
> If adding this macro is really an issue, it could be removed of course but
> in this case I am in favor of readability.

Arnd,

I think that the case highlighted by Ludovic is indeed a case where 
someone can be confused by the presentation of data in the cell... even 
if I like the use of macro as implemented by Ludovic, I do not have a 
strong opinion about this.

Best regards,
diff mbox

Patch

diff --git a/include/dt-bindings/dma/at91.h b/include/dt-bindings/dma/at91.h
new file mode 100644
index 0000000..e835037
--- /dev/null
+++ b/include/dt-bindings/dma/at91.h
@@ -0,0 +1,27 @@ 
+/*
+ * This header provides macros for at91 dma bindings.
+ *
+ * Copyright (C) 2013 Ludovic Desroches <ludovic.desroches@atmel.com>
+ *
+ * GPLv2 only
+ */
+
+#ifndef __DT_BINDINGS_AT91_DMA_H__
+#define __DT_BINDINGS_AT91_DMA_H__
+
+/*
+ * Source and/or destination peripheral ID
+ */
+#define AT91_DMA_CFG_PER_ID_MASK	(0xff)
+#define AT91_DMA_CFG_PER_ID(id)		(id & AT91_DMA_CFG_PER_ID_MASK)
+
+/*
+ * FIFO configuration: it defines when a request is serviced.
+ */
+#define AT91_DMA_CFG_FIFOCFG_OFFSET	(8)
+#define AT91_DMA_CFG_FIFOCFG_MASK	(0xf << AT91_DMA_CFG_FIFOCFG_OFFSET)
+#define AT91_DMA_CFG_FIFOCFG_HALF	(0x0 << AT91_DMA_CFG_FIFOCFG_OFFSET)	/* half FIFO (default behavior) */
+#define AT91_DMA_CFG_FIFOCFG_ALAP	(0x1 << AT91_DMA_CFG_FIFOCFG_OFFSET)	/* largest defined AHB burst */
+#define AT91_DMA_CFG_FIFOCFG_ASAP	(0x2 << AT91_DMA_CFG_FIFOCFG_OFFSET)	/* single AHB access */
+
+#endif /* __DT_BINDINGS_AT91_DMA_H__ */