Mark Brown wrote:
The only major issue I see with the patch is a documentation one: it's not clear to me reading the code how the atmel_ssc DAI driver relates to the existing at91_ssc driver. It may be that this is something that's obvious to someone familiar with the at91 hardware but just looking at the code it's not clear to me what the difference is between the two and when each should be used.
Looking at the code they appear to be similar to the point where they should be the same driver but it's entirely possible that I'm missing something here. I've CCed in Frank Mandarino who did the existing AT91 support. If they should be separate drivers then some comments should be added in the driver and the Kconfig help text explaning the situation.
I agree that the drivers should be combined. Unfortunately, at this time I am unable to contribute to this effort.
+/*
- Record SSC clock dividers for use in hw_params().
- */
+static int atmel_ssc_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
- int div_id, int div)
+{
- struct atmel_ssc_info *ssc_p = &ssc_info[cpu_dai->id];
- 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.
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.
+#ifdef CONFIG_PM +#define atmel_ssc_suspend NULL +#define atmel_ssc_resume NULL +#else +#define atmel_ssc_suspend NULL +#define atmel_ssc_resume NULL +#endif
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.
Regards, ../fam