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

[Sedji Gaouaou <sedji.gaouaou@atmel.com>]

Hi Frank,
Frank Mandarino a écrit :
> 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.
> 
I agree with you as well. I wanted to use the drivers/misc/atmel-ssc in 
the dai because it is a common arch between atmel ARM and AVR core.
I will have a look at the at91-ssc code and at the eti_b1_wm8731.c file 
to see what changes should be done.
> 
>>> +		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?

My mistake, I will use your constant def.
> 
>>> +#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.
I removed resume/suspend because I didn't have the time to write it...
I wanted to add it in a next patch, but maybe I shoul do it now.

Mark, concerning your other comments I am working on it.
I will send another patch as soon as it is finished.

Regards,
Sedji