Re: [PATCH 1/1][RESEND] Driver for the Atmel on-chip SSC on AT32AP and AT91.

[Hans-Christian Egtvedt <hcegtvedt@atmel.com>]

On Thu, 2007-07-19 at 10:29 +0200, Andrew Victor wrote:
> hi Hans-Christian,
> 
> > This driver is used in the ALSA sound driver for the AT73C213 external DAC on
> > the ATSTK1000 development board for AVR32. This sound driver will be submitted
> > soon.
> 
> Seems to be more of an SCC unit allocation library, but OK.

Correct, makes it easier for other modules to request the SSC peripheral
and get access to the registers and irq.

The AT73C213 sound driver was submitted to the alsa-dev list.

> I've also forwarded a copy of the patch to Frank Mandarino
> <fmandarino@endrelia.com> for his comments.  He maintains the current
> AT91 SSC & ALSA support.  <sound/soc/at91/>
>
> > +/* SSC Receive Clock Mode Register */
> > +#define SSC_RCMR			0x00000010
> > +#define SSC_RCMR_CKG_SIZE			 2
> > +#define SSC_RCMR_CKG_OFFSET			 6
> > +#define SSC_RCMR_CKI_SIZE			 1
> > +#define SSC_RCMR_CKI_OFFSET			 5
> > +#define SSC_RCMR_CKO_SIZE			 3
> 
> I'm personally not a fan of the register & bit definition format the
> AVR32 driver seem to use.
> Can we not rather move the existing include/asm-arm/arch-at91/at91_ssc.h
> to somewhere shared as atmel_ssc.h ?

I disagree, by using the model above the user does not have to know the
bit field offsets as they will need with the defines in
include/asm-arm/arch-at91/at91_ssc.h.

Just an example from sound/soc/at91/at91-ssc.c

rcmr =	  (( ssc_p->rcmr_period		<< 24) & AT91_SSC_PERIOD)
			| (( 1				<< 16) & AT91_SSC_STTDLY)
			| (( AT91_SSC_START_FALLING_RF	     ) & AT91_SSC_START)
			| (( AT91_SSC_CK_RISING		     ) & AT91_SSC_CKI)
			| (( AT91_SSC_CKO_NONE		     ) & AT91_SSC_CKO)
			| (( AT91_SSC_CKS_DIV		     ) & AT91_SSC_CKS);

Would with the header style for atmel-ssc be:

rcmr = SSC_BF(RCMR_PERIOD, ssc_p->rcmr_period)
		| SSC_BF(RCMR_STTDLY, 1)
		| SSC_BF(RCMR_START, 4)
		| SSC_BF(RCMR_CKI, 1)
		| SSC_BF(RCMR_CKO, 0)
		| SSC_BF(RCMR_CKS, 0);

I find the latter more readable and compact, the user also does not need
to know the offset of the different bit-fields.

I agree that describing names for the different bit-fields is nice.
Similar to the way the AVR32 standalone headers are built up.

-- 
With kind regards,

Hans-Christian Egtvedt, siv.ing. (M.Sc.)
Applications Engineer - AVR32 System Solutions - Atmel Norway