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
Attachment:
signature.asc
Description: This is a digitally signed message part
