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?