Re: ALSA:AT91SAM9G20: Add support(DAI) for wolfson8731

[Mark Brown <broonie@sirena.org.uk>]

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;
> >> +		else
> >> +			if (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.