On 11/24/2010 12:29 PM, Mark Brown wrote:
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.
Thanks :-)
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);
Okay.
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.
Okay, regular spin_lock should be okay right?
- 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.
Okay, will change.
/* 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.
Essentially this code is overriding the settings in the hw_params switch statement for the combined clocks case. This will need to be overridden each time hw_params is called. Doing it here seems logical since atmel_ssc_dai:hw_params does the original setting. It keeps the machine drivers simpler too.
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?
Okay. Could you point me at an example for this please.
Thanks, ~Ryan