Re: [PATCHv3] ASoC: Add support for BCM2835

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Nov 18, 2013 at 11:28:31AM +0000, Florian Meier wrote:
> This driver adds support for digital audio (I2S)
> for the BCM2835 SoC that is used by the
> Raspberry Pi. External audio codecs can be
> connected to the Raspberry Pi via P5 header.
> 
> It relies on cyclic DMA engine support for BCM2835.
> 
> Signed-off-by: Florian Meier <florian.meier@koalo.de>
> ---
> 
> This successor of RFCv2 is no longer dependent on the dmaengine
> patch (of course you need it for running it). Therefore, it
> should cleanly apply.
> 
> Furthermore, some minor enhancements were applied. For example
> - Removal of bcm2835-pcm.c and .h
> - Adding bindings documentation
> - Adding possibility for setting bclk_ratio
> 
>  .../devicetree/bindings/sound/bcm2835-i2s.txt      |   22 +
>  sound/soc/Kconfig                                  |    1 +
>  sound/soc/Makefile                                 |    1 +
>  sound/soc/bcm/Kconfig                              |   10 +
>  sound/soc/bcm/Makefile                             |    5 +
>  sound/soc/bcm/bcm2835-i2s.c                        |  904 ++++++++++++++++++++
>  6 files changed, 943 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/bcm2835-i2s.txt
>  create mode 100644 sound/soc/bcm/Kconfig
>  create mode 100644 sound/soc/bcm/Makefile
>  create mode 100644 sound/soc/bcm/bcm2835-i2s.c
> 
> diff --git a/Documentation/devicetree/bindings/sound/bcm2835-i2s.txt b/Documentation/devicetree/bindings/sound/bcm2835-i2s.txt
> new file mode 100644
> index 0000000..7bf1d04
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/bcm2835-i2s.txt
> @@ -0,0 +1,22 @@
> +* Broadcom BCM2835 SoC I2S/PCM module
> +
> +Required properties:
> +- compatible: "brcm,bcm2835-i2s"
> +- reg: Register location and size
> +       <PCM base address, size
> +        PCM clock base address, size>;

I'd prefer something like:

- reg: A list of base address and size entries:
  * The first entry should cover the PCM registers
  * The second entry should cover the PCM clock registers

Are the clocks actually a sub-block, or are they actually a separate
unit that just happens to feed this one?

> +- dmas: List of DMA controller phandle and DMA request line ordered pairs.
> +- dma-names: Identifier string for each DMA request line in the dmas property.
> +  These strings correspond 1:1 with the ordered pairs in dmas.

It should be pointed out that "tx" and "rx" are expected.

> +
> +Example:
> +
> +bcm2835_i2s: i2s@7e203000 {
> +       compatible = "brcm,bcm2835-i2s";
> +       reg = < 0x7e203000 0x20
> +               0x7e101098 0x02>;

Please bracket entries individually:

reg = <0x7e203000 0x20>,
      <0x7e101098 0x02>;

> +
> +       dmas = <&dma 2
> +               &dma 3>;

Similarly:

dmas = <&dma 2>,
       <&dma 3>;

[...]

> +static void bcm2835_i2s_stop_clock(struct bcm2835_i2s_dev *dev)
> +{
> +       uint32_t clkreg;

s/uint32_t/u32/ (and elsewhere...)

[...]

> +       target_frequency = sampling_rate*bclk_ratio;

It would be nicer if there were space around binary operators here and
elsewhere.

> +       clk_src = BCM2835_CLK_SRC_OSC;
> +       mash = BCM2835_CLK_MASH_0;
> +
> +       if (bcm2835_clk_freq[clk_src] % target_frequency == 0
> +                       && bit_master && frame_master) {
> +               divi = bcm2835_clk_freq[clk_src]/target_frequency;
> +               divf = 0;
> +       } else {
> +               uint64_t dividend;

s/uint64_t/u64/

[...]

> +       ch2pos = bclk_ratio/2+data_delay;

Spacing would make this far nicer...

Thanks,
Mark.