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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2161 bytes --]

On Tue, Aug 25, 2015 at 03:26:54PM +0530, maruthi srinivas wrote:
> On Tue, Aug 25, 2015 at 11:36 AM, Mark Brown <broonie@kernel.org> wrote:

> > Please explain this in more detail, shared register ranges are very
> > common and are the sort of things MFDs are supposed to help with.

> In our case, ACP I2S driver need not do a  'devm_ioremap_resource' to
> get mmio base.

That sounds like a MFD type problem...

> ACP audio IP (DMA + I2S+ Others) registers can be accessed, using
> GPU's MMIO base.
> During GPU driver design, it was decided that all the register access
> for entire GPU MMIO
> aperture (includes ACP and others) to be done in GPU module only.
> This is implemented in another patch in this patch series using a
> abstraction layer.

That sounds like converting the Designware driver to use regmap and
providing a regmap would enable code sharing (you can provide a regmap
for accessors if you don't use it in the main driver).

> >> 2. The designware driver depends on the CLKDEV framework which we
> >> don't currently support.

> > You need to support the clock API, it's very easy to do so so there is
> > no excuse for doing something custom here.

> Codec acts as master in our case to provide clock to i2s controller and
> there wasn't a need to use clock APIs unlike in existing designware i2s driver.
> There is no custom implementation.

So you just need to add slave mode support to the driver.  Again not a
reason to just copy the code.

> >> 3. Our hardware does not support S16_LE

> > If you have modified the designware IP to remove this support (why would
> > anyone do that?) it's a trivial quirk, if the restriction comes from
> > some other part of the system like the DMA driver then the constraint
> > will come from that part of the system.

> There is a bug in ACP SoC implementation (which combines internal DMA,
> designware I2S
> and other blocks) for 16bit and lower resolution. I felt , it would be
> better to limit functionality
> in I2S DAI capabilities. I will put  this limitation in DMA driver
> capabilities, to represent overall
> sound card capabilities, if you suggest.

A quirk would also do the job.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]