On Fri, 2012-04-20 at 12:20 -0500, Ricardo Neri wrote:
> Hi Tomi,
> 
> Thanks for your comments!
> 
> On 04/20/2012 07:03 AM, Tomi Valkeinen wrote:

> > I can't say anything about the parameters for audio_config, so I trust
> > they are ok. But some comments about the functions:
> 
> Fwiw, I have been testing this approach on OMAP4 and OMAP5 during the 
> last week and it seems to work fine. :) Perhaps in the future I can 
> extend it to cover speaker mapping for the multichannel use-case.

Ok. I'll try to go through the rests of patches from you today.

> > I don't like foo_enable(bool enable) style functions. While it does
> > reduce the amount of code a bit, I think it's much less readable than
> > separate foo_enable() and foo_disable() functions.
> 
> I will split the functions audio_enable into audio_enable and 
> audio_disable and audio_start into audio_start and audio_stop.
> >
> > audio_detect sounds a bit misleading to me... You're not detecting
> > anything, but asking if audio is supported. So... "audio_supported()" or
> > something?
> 
> I was looking for a name that denotes that audio support is dynamic: 
> while a display may be capable of playing audio, it may be supported 
> only for some configurations (e.g., VESA vs CEA video timings). 
> Something like audio_supported_now() would be fully descriptive but I 
> guess audio_supported() looks nicer.

Right, I see. I agree that audio_supported() doesn't convey all the
information about the context. But audio_supported_now() does sound a
bit odd =).

> > Is there need for the audio_config function, or could the audio configs
> > be given with audio_start call?
> 
> I propose to have separate functions for configuration and audio start 
> to provide full flexibility. The users of the DSS audio interface should 
> be able to decide when to configure and when to start the audio; 
> otherwise, synchronization issues may arise. For instance, on OMAP HDMI 
> IPs, the audio configuration must be done while the audio wrapper is 
> disabled; but the DMA transfers must start after enabling the audio 
> wrapper (or FIFO underflow and audio channel shifting may occur). As the 
> DMA tranfers are started by a different driver (e.g., ASoC), DSS does 
> control it and, instead, should provide functionality to config and 
> start audio when required.
> 
> Furthermore, IMHO, having a config function to effectively configure and 
> a start function to effectively start audio contributes to improve 
> readability.

Ok, I agree.

 Tomi