Re: [PATCH v2 2/6] ASoC: samsung: Add Samsung Low Power Audio Subsystem driver

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

On 06/29/2016 11:57 PM, Mark Brown wrote:
> On Thu, Jun 23, 2016 at 12:45:11PM +0200, Sylwester Nawrocki wrote:
...
>> > This patch adds LPASS driver. The LPASS (Low Power Audio Subsystem)
>> > is a special subsystem that supports audio playback with low power
>> > consumption. This is a minimal driver which prepares resources for
>> > IP blocks like I2S, audio DMA and UART.
>> > Also system power ops are added to ensure the Audio Subsystem is
>> > operational after system suspend/resume cycle.
>
> This is so trivial that I'm wondering why you even need it, it does
> essentially nothing except for manage power on and off, and that isn't
> synced up with the rest of the audio subsystem in any discernable way.
> Previous Samsung SoCs have quite happily ignored their LPASS subsystems
> in mainline.  As things stand I'm concerned that systems may get broken
> if more functionality is added without the joining up with the rest of
> the audio hardware via a card, there's not even the hooks which would
> allow that.

Sorry, I didn't notice this e-mail earlier.  With previous Exynos versions
the LPASS (or AudioSS) was mainly about the embedded audio DSP processor
(at least WRT to SFRs), which was not required for boards supported in 
the mainline kernel. Since e.g. Exynos5433 the LPASS SFR region contains 
also entries related to other IP blocks, like I2S or DMAC.  Even though 
functionality covered by these registers is still rather trivial, like 
SW resets and unmasking interrupts, it's essential for the whole audio 
subsystem operation. The intention was to have in future the LPASS driver 
covering any functionality provided by the embedded audio dedicated MCU.
I'm afraid we have to handle those power sequences in central place to
ensure proper SoC operation. I would also rather avoid adding any exynos 
quirks to the PL330 DMAC driver. 

I agree this driver is currently detached from the rest, while it would
be good at the card to ensure the required resources are prepared.
How about making LPASS registering a component and then adding it at
the card as aux_dev? I would move some parts to the component's probe()
callback and with component's probe_order specified a proper 
initialization sequence could be ensured.