[lkml]   [2017]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver

> -----Original Message-----
> From: Mark Brown []
> Sent: Tuesday, December 5, 2017 9:15 PM
> To: Suzuki, Katsuhiro/鈴木 勝博 <>
> Cc:; Rob Herring <>;
>; Yamada, Masahiro/山田 真弘
> <>; Masami Hiramatsu
> <>; Jassi Brar <>;
> 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,
> > 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
> 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.

Katsuhiro Suzuki

 \ /
  Last update: 2017-12-06 07:04    [W:0.153 / U:0.340 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site