Re: [PATCH 2/4] ASoC: Add support for the Analog Devices AXI-I2S core

[Mark Brown <broonie@kernel.org>]

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

On Thu, Nov 28, 2013 at 08:02:50PM +0100, Lars-Peter Clausen wrote:

A few really minor things below.

> The patch also adds a simple ASoC PCM driver for the PL330 DMA controller using
> the generic dmaengine PCM driver. This driver is only temporary can be removed
> once we have all the infrastructure in place to query information like maximum
> transfer size directly from the dmaengine driver.

It's not really a driver as such, it's just specifying some extra
parameters - I'd expect you're going to need to keep the addresses at
least.  It's probably more accurate to say that at the minute there's
some assumptions in the driver about the DMA controller configuration
that will be required which should be removable in future?

> +config SND_SOC_ADI
> +	tristate "Audio support for Analog Devices reference designs"
> +	depends on SND_SOC && (MICROBLAZE || ARCH_ZYNQ || COMPILE_TEST)
> +	help
> +	  Audio support for various reference designs by Analog Devices.
> +

Given that this is a FPGA IP I'd also expect it to be available on
SOCFPGA and probably other things.  For the Designware I2S controller we
just don't have any platform dependency at all.

There's no need for the SND_SOC dependency, all the subdirectory
Kconfigs are only included in an if SND_SOC block.

> +	frame_size = AXI_I2S_BITS_PER_FRAME / 2 - 1;

This isn't obvious - usually the frame size is the number of bits per
frame.  Looking at the code I guess the hardware is more flexible than
the driver allows at the minute?

> +	regmap_write(i2s->regmap, AXI_I2S_REG_CLK_CTRL, (frame_size << 16) |
> +		bclk_div);

update_bits() would be more obvious, though for a memory mapped device
it really makes no difference.

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

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