Re: [RFC PATCH] Add combined clock support to Atmel SoC audio

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Nov 24, 2010 at 11:05:11AM +1300, Ryan Mallon wrote:

> This patch is posted as RFC since the approach is incomplete and a bit
> hackish. I am mostly interested in knowing if this is a sensible
> approach, and could be cleaned up for mainline inclusion, or if there is
> a better way to do this.

This doesn't look obviously hideous.

It should set the symmetric_rates flag when going into combined clocks
mode.  A few other comments:

> +			if (atomic_dec_and_test(&ssc_p->substreams_running))
> +				ssc_writel(ssc_p->ssc->regs, CR,
> +					   dma_params->mask->ssc_disable);

It'd seem clearer to use a regular lock here; probably safer also as you
could get a race between the test and the write which tries to revert
the change - ie a:

	dec_and_test
			inc
			read
			write
	write

type pattern.

> +	if (ssc_p->combined_clock == ATMEL_SSC_CLOCK_RX_ON_TX) {

How about just calling these defines ATMEL_SSC_CLOCK_TX or _RX?  You're
identifying the clock line to use for both cases, it's probably clearer
to say "use the RX lines" or "use the TX" lines than say "do one on the
other" if you see what I mean.

> +		/* RX clock is sourced from TK pin */
> +		rcmr &= ~SSC_BF(RCMR_CKS, 0x7);
> +		rcmr |=  SSC_BF(RCMR_CKS, SSC_CKS_CLOCK);

Is this done by the driver normally, or is it done by the machine
normally?  If it's normally done by the machine perhaps it should be
moved into the driver in all cases.

> +		if (dir == 1) {
> +			/*
> +			 * Set the TX clock period to the RX clock period
> +			 * FIXME - Is this okay if we are already doing TX?
> +			 */
> +			tcmr &= 0x00ffffff;
> +			tcmr |= rcmr & 0xff000000;

Should probably enforce a constraint to stop users doing something that
forces the change?