On Wed, Sep 17, 2008 at 11:06:54AM -0400, Frank Mandarino wrote:
Mark Brown wrote:
- switch (div_id) {
- case AT91SSC_CMR_DIV:
/** The same master clock divider is used for both* transmit and receive, so if a value has already* been set, it must match this value.*/if (ssc_p->cmr_div == 0)ssc_p->cmr_div = div;elseif (div != ssc_p->cmr_div)return -EBUSY;break;
What happens if the user wants to change the master clock divider at runtime - for example, when changing sample rates?
This is code from at91-ssc.c. I really didn't consider the case of changing the sample rate on an open substream. This logic could be updated to allow the new divider value if there is only one substream open.
Ah, right - on further inspection I see that cmr_div is reset when the stream is shut down. That's fine since it means that the clocks can be reconfigured when reopening the device which is the case I was worried about.
Changing the dividers on an active stream is unlikely to work well so it's perfectly reasonable to not support it.
start_event = channels == 1? 4: 7;
This would be much clearer if it were expanded into multiple statements.
This was a little clearer in at91-ssc.c:
start_event = channels == 1 ? AT91_SSC_START_FALLING_RF : AT91_SSC_START_EDGE_RF;
Perhaps these constant definitions are no longer available it the latest kernel. Are there updated definitions to use instead of magic numbers?
Also, I'm fine with using multiple statements if that helps readability.
I wasn't so worried about the magic numbers as the combination of assignment and an equality test without even any brackets. I can see what's going on but it's certainly not the most transparent way of writing it.
These may as well be removed - if someone implements suspend/resume support they can add them then then.
Is there a reason that suspend/resume was removed? It is really important for embedded systems.
Yeah, I did wonder, though there are plenty of embedded systems that aren't particular power sensitive for one reason or another.