Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver

From: Mark Brown <broonie@kernel.org>
To: Katsuhiro Suzuki <suzuki.katsuhiro@socionext.com>
Cc: alsa-devel@alsa-project.org, "Rob Herring" <robh+dt@kernel.org>,
	devicetree@vger.kernel.org, "Yamada,
	Masahiro/山田 真弘" <yamada.masahiro@socionext.com>,
	"Masami Hiramatsu" <masami.hiramatsu@linaro.org>,
	"Jassi Brar" <jaswinder.singh@linaro.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver
Date: Tue, 5 Dec 2017 12:14:40 +0000	[thread overview]
Message-ID: <20171205121440.GC11658@finisterre> (raw)
In-Reply-To: <002b01d36d84$51d80aa0$f5881fe0$@socionext.com>

[-- Attachment #1: Type: text/plain, Size: 2448 bytes --]

On Tue, Dec 05, 2017 at 01:48:39PM +0900, Katsuhiro Suzuki wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> > Is there a mux in the SoC here?

> Sorry for confusing, It's not mux.

> uniphier_srcport_reset() resets HW SRC (sampling rate converter) block.
> Audio data out ports of UniPhier audio system have HW SRC.

Is the SRC just a single block sitting between the DMA and the external
audio port or is there more going on?  Some of the other code made me
think the hardware was more flexible than this (all the writing to
registers with names like RXSEL for example).

> > > +#endif /* CONFIG_SND_SOC_UNIPHIER_LD11 */

> > Why is there an ifdef here?  There's no other conditional code in here,
> > it seems pointless.

> This config is used to support or not LD11 SoC.
> aio-ld11.c is not build and 'uniphier_aio_ldxx_spec' is undefined if this config is disabled.
> 
> aio-ld11.c defines SoC dependent resources (port, HW ring buffer, DMA ch, etc.)
> and fixed settings.
> I know it's better to move such information into device-tree, but register areas of
> UniPhier's audio system is very strange and interleaved. It's hard to split each nodes...

I'd expect this code to be structured more like a library - have a
driver that handles the specific IPs then have it call into a shared
block of code that does the generic bits.  Though in this case the
device specific bit looks like a couple of tiny data tables so I'm not
sure it's worth making it conditional or separate at all.

> > This looks awfully like compressed audio support...  should there be
> > integration with the compressed audio API/

> Thanks, I'll try it. Is there Documentation in sound/designes/compress-offload.rst?
> And best sample is... Intel's driver?

Yes.

> (Summary)
> I think I should fix as follows:

>   - Split DMA, DAI patches from large one
>   - Validate parameters in hw_params
>   - Add description about HW SRC (or fix bad name 'srcport')
>   - Add comments about uniphier_aiodma_irq()
>   - Expose clocking and audio routing to userspace, or at the very
>     least machine driver configuration
>   - Support compress-audio API for S/PDIF

> and post V2.

At least.  I do think we need to get to the bottom of how flexible the
hardware is first though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]