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

From: "Katsuhiro Suzuki" <suzuki.katsuhiro@socionext.com>
To: "'Mark Brown'" <broonie@kernel.org>,
	"Suzuki, Katsuhiro/鈴木 勝博" <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: Wed, 6 Dec 2017 15:03:18 +0900	[thread overview]
Message-ID: <004801d36e57$ea1940d0$be4bc270$@socionext.com> (raw)
In-Reply-To: <20171205121440.GC11658@finisterre>

Hello,

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Tuesday, December 5, 2017 9:15 PM
> To: Suzuki, Katsuhiro/鈴木 勝博 <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
> 
> 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.
> 

Thank you, I set it.

> > > 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).
> 

This hardware has 2 types of HW SRC. First type sit before audio port and
cannot change routing. I use it in this driver. Second type is like a loopback,
but this block is not used in this driver to simplify the first version.

Type 1:
  Mem -> DMA -> SRC -> Out Port -> External pin
Type 2:
  Mem -> DMA -> SRC -> Out Port -> In Port -> DMA -> Mem

> > > > +#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.
> 

Sorry... I agree your opinion, but I can't imagine the detail.

I think my driver has structure as follows (ex. startup):
  DAI: uniphier_aio_startup()@aio-core.c
  Lib: uniphier_aio_init()@aio-regctrl.c
  SoC specific: uniphier_aio_ld11_spec@aio-ld11.c

Am I wrong? Would you mean split the functions in aio-regctl.[ch] to other
kernel module? I wonder if you could tell me the example from existing
drivers. I'll try to fix my driver like as it.

> > > 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.

Yes, indeed. This hardware is more flexible and complex, but now I (and our
company) don't use it. Of course, I don't want to hide some features of this
hardware from ALSA people. I should try to upstream all features in the future,
I think.

Regards,
--
Katsuhiro Suzuki