[alsa-devel] [PATCH V5 3/3] ASoC: AMD: add AMD ASoC ACP-I2S driver

[Alex Deucher]

On Fri, Aug 21, 2015 at 12:17 PM, Mark Brown <broonie at kernel.org> wrote:
> On Fri, Aug 21, 2015 at 05:21:07PM +0530, maruthi srinivas wrote:
>> On Fri, Aug 21, 2015 at 4:48 AM, Mark Brown <broonie at kernel.org> wrote:
>
>> > We already have a driver for the DesignWare I2S controller.  To repeat
>> > the concern I raised in a slightly different bit of the code last time:
>
>> > | This doesn't appear to be a designware-i2s driver, we already have one
>> > | of those?
>
>> Our IP block includes few AMD IPs along with DesignWare I2S IP.  I have reused
>> code from Designware I2S controller from
>> sound/soc/dwc/designware_i2s.c and because
>> of the way IPs are coupled together, I couldn't use existing
>> Designware I2S driver as is.
>
> Could you be more specific about the way in which the IPs are coupled
> and the problems this causes please?
>
>> I have given credit to the original author in DRM patch copyright
>> header, where register I2S read/writes
>> are made. Do I need to add the same header in ASoC driver too ?
>
> What I'm looking for is actual code sharing where we use the same code
> for the I2S controller block or a clear and documented understanding of
> why it is not possible to share things.
>

Maruthi can clarify further, but it's not possible to use the
designware driver directly because:
1. The i2s registers are within the same MMIO aperture as our other
GPU registers.  Our GPU driver is designed in such a way that the
specific IP modules don't have direct access to the MMIO aperture.
They use functions provided by the core driver to access registers.
Thus the ACP IP module within the GPU driver does not have direct
access to the mmio base pointer in order to pass it on.
2. The designware driver depends on the CLKDEV framework which we
don't currently support.
3. Our hardware does not support S16_LE

Alex


>> Oh! I freed  in dma_close(), that were  allocated in dma_open(). Rest
>> of them  used devm_*
>
> OK.
>
>> > > +             /* Now configure DMA to transfer only first half of System RAM
>> > > +              * buffer before playback is triggered. This will overwrite
>> > > +              * zero-ed second half of SRAM buffer */
>> > > +             acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM,
>> > > +                                     PLAYBACK_START_DMA_DESCR_CH12,
>> > > +                                     1, 0);
>> >
>> > | Why?  The comments describe what's happening but it's not clear why it's
>> > | happening.
>
>> The reason for doing this is :  When C completes rendering, calls
>> period_elapsed() and informs
>> ALSA core there is free space to fill new data to system memory. In
>> the same irq handler, B is
>> DMA'ed to C to be ready by the time D completes rendering with
>> prefetched data (in trigger()).
>> when D completes rendering, new data fetched by ALSA core can be
>> DMA'ed  from A to D and
>> rendering is continued with C. This is done cyclically.
>
> My point here is that the internal documentation in the driver should be
> improved so that someone reading the code can tell why this is being
> done.  It doesn't need to be this full explanation but at least enough
> for people to be aware of the general issue.
>
>> > > +static struct snd_soc_dai_driver i2s_dai_driver = {
>> > > +     .playback = {
>> > > +                  .stream_name = "I2S Playback",
>> > > +                  .channels_min = 2,
>> > > +                  .channels_max = 2,
>
>> > Elsewhere support for 8 channels was declared and handled.
>
>> The board for which driver is developed, doesn't support more than 2 channels.
>
> This is a driver for the IP, not for the board - you may not be able to
> test everything but code should try to be as general as it can be.