Date: Tue, 5 Dec 2017 12:14:40 +0000
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,
        =?utf-8?B?WWFtYWRhLCBNYXNhaGlyby/lsbHnlLAg55yf5byY?= 
        <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
Message-ID: <20171205121440.GC11658@finisterre>
References: <20171122114321.29196-1-suzuki.katsuhiro@socionext.com>
 <20171122114321.29196-6-suzuki.katsuhiro@socionext.com>
 <20171204183934.rd4vin22ktukwpip@sirena.org.uk>
 <002b01d36d84$51d80aa0$f5881fe0$@socionext.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="9Ek0hoCL9XbhcSqy"
Content-Disposition: inline
In-Reply-To: <002b01d36d84$51d80aa0$f5881fe0$@socionext.com>
User-Agent: Mutt/1.9.1 (2017-09-22)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3129
Lines: 88


--9Ek0hoCL9XbhcSqy
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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 registe=
r areas of
> UniPhier's audio system is very strange and interleaved. It's hard to spl=
it 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-of=
fload.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.

--9Ek0hoCL9XbhcSqy
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAlomjbAACgkQJNaLcl1U
h9BFiAf+IRA7T3TCrKUXbnaZCZQS/tTyylp04bjSR6BzEl9m4O0kDG2WM4ZQS1RE
hjoD9fQlTMicA5hBMLarR1ojjpIzbVGjxeRIkcMzsVOpIZVBN9t36pc0WFeem5Ff
IdKFIusosePgfTnW3PeamVdyJ1xK/s+C2wBAA7AaWLwRQef+VHZdJ2XKOyei438d
D0VTLpoZvh7O/TpvWnCEIpv3rU3j2Qso3MtV7fRFazg2yanRt7kQm7nvmgx8/wT4
HJk/w9zpk5/mJwu5o3D/HeoyLYcpxDM5bocxYHrLmJVRqZFvf84HlYXP9eB1u/4y
vxCaVjPYtA6bCSEDq3DLXi9Ok595Ag==
=gUoc
-----END PGP SIGNATURE-----

--9Ek0hoCL9XbhcSqy--