From: "Katsuhiro Suzuki" <suzuki.katsuhiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
To: "'Mark Brown'" <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
"Suzuki,
Katsuhiro/鈴木 勝博"
<suzuki.katsuhiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org,
"Rob Herring" <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "Yamada,
Masahiro/山田 真弘"
<yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>,
"Masami Hiramatsu"
<masami.hiramatsu-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>,
"Jassi Brar"
<jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver
Date: Tue, 5 Dec 2017 13:48:39 +0900 [thread overview]
Message-ID: <002b01d36d84$51d80aa0$f5881fe0$@socionext.com> (raw)
In-Reply-To: <20171204183934.rd4vin22ktukwpip-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
Hello,
Thanks a lot for your review.
> -----Original Message-----
> From: Mark Brown [mailto:broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Tuesday, December 5, 2017 3:40 AM
> To: Suzuki, Katsuhiro/鈴木 勝博 <suzuki.katsuhiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
> Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org; Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Yamada, Masahiro/山
> 田 真弘 <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>; Masami Hiramatsu <masami.hiramatsu-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>; Jassi Brar
> <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver
>
> On Wed, Nov 22, 2017 at 08:43:18PM +0900, Katsuhiro Suzuki wrote:
>
> > sound/soc/uniphier/Makefile | 4 +
> > sound/soc/uniphier/aio-core.c | 368 +++++++++++++++++++++
> > sound/soc/uniphier/aio-dma.c | 266 +++++++++++++++
> > sound/soc/uniphier/aio-regctrl.c | 699 +++++++++++++++++++++++++++++++++++++++
> > sound/soc/uniphier/aio-regctrl.h | 495 +++++++++++++++++++++++++++
> > sound/soc/uniphier/aio.h | 261 +++++++++++++++
>
> Please split this up more, it looks like there's at least two or three
> drivers in here and it winds up being quite large. There's at least a
> DMA and a DAI driver. Looking through this my overall impression is
> that this is a fairly large and complex audio subsystem with some DSP
> and routing capacity which is being handled in a board specific fashion
> rather than generically but it's kind of hard to tell as there's not
> much description of what's going on so I'm needing to reverse engineer
> things from the driver.
>
> The code itself looks fairly clean, it's mainly a case of trying to
> figure out if it's doing what it's supposed to with the limited
> documentation.
>
> > +int uniphier_aio_hw_params(struct snd_pcm_substream *substream,
> > + struct snd_pcm_hw_params *params,
> > + struct snd_soc_dai *dai)
> > +{
> > + struct uniphier_aio *aio = uniphier_priv(dai);
> > + struct uniphier_aio_sub *sub = &aio->sub[substream->stream];
> > +
> > + sub->params = *params;
> > + sub->setting = 1;
>
> So we don't validate the params at all?
>
> > + uniphier_aio_port_reset(sub);
> > + uniphier_aio_srcport_reset(sub);
>
> 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.
> > +static const struct of_device_id uniphier_aio_of_match[] = {
> > +#ifdef CONFIG_SND_SOC_UNIPHIER_LD11
> > + {
> > + .compatible = "socionext,uniphier-ld11-aio",
> > + .data = &uniphier_aio_ld11_spec,
> > + },
> > + {
> > + .compatible = "socionext,uniphier-ld20-aio",
> > + .data = &uniphier_aio_ld20_spec,
> > + },
> > +#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...
> > + for (j = 0; j < ARRAY_SIZE(aio->sub); j++) {
> > + struct uniphier_aio_sub *sub = &aio->sub[j];
> > +
> > + if (!sub->running)
> > + continue;
> > +
> > + spin_lock(&sub->spin);
> > + uniphier_aio_rb_sync(sub);
> > + uniphier_aio_rb_clear_int(sub);
> > + spin_unlock(&sub->spin);
>
> It's not 100% obvious what this does... a comment might help.
>
> > +int uniphier_aio_chip_init(struct uniphier_aio_chip *chip)
> > +{
> > + struct regmap *r = chip->regmap;
> > +
> > + regmap_update_bits(r, A2APLLCTR0,
> > + A2APLLCTR0_APLLXPOW_MASK,
> > + A2APLLCTR0_APLLXPOW_PWON);
> > +
> > + regmap_update_bits(r, A2APLLCTR1, A2APLLCTR1_APLL_MASK,
> > + A2APLLCTR1_APLLF2_33MHZ | A2APLLCTR1_APLLA2_33MHZ |
> > + A2APLLCTR1_APLLF1_36MHZ | A2APLLCTR1_APLLA1_36MHZ);
> > +
> > + regmap_update_bits(r, A2EXMCLKSEL0,
> > + A2EXMCLKSEL0_EXMCLK_MASK,
> > + A2EXMCLKSEL0_EXMCLK_OUTPUT);
> > +
> > + regmap_update_bits(r, A2AIOINPUTSEL, A2AIOINPUTSEL_RXSEL_MASK,
> > + A2AIOINPUTSEL_RXSEL_PCMI1_HDMIRX1 |
> > + A2AIOINPUTSEL_RXSEL_PCMI2_SIF |
> > + A2AIOINPUTSEL_RXSEL_PCMI3_EVEA |
> > + A2AIOINPUTSEL_RXSEL_IECI1_HDMIRX1);
>
> This definitely looks like there's some clocking and audio routing
> within the SoC which should be exposed to userspace, or at the very
> least machine driver configuration rather than being hard coded.
>
> > + switch (pc) {
> > + case IEC61937_PC_AC3:
> > + repet = OPORTMXREPET_STRLENGTH_AC3 |
> > + OPORTMXREPET_PMLENGTH_AC3;
> > + pause |= OPORTMXPAUDAT_PAUSEPD_AC3;
> > + break;
> > + case IEC61937_PC_MPA:
> > + repet = OPORTMXREPET_STRLENGTH_MPA |
> > + OPORTMXREPET_PMLENGTH_MPA;
> > + pause |= OPORTMXPAUDAT_PAUSEPD_MPA;
> > + break;
> > + case IEC61937_PC_MP3:
> > + repet = OPORTMXREPET_STRLENGTH_MP3 |
> > + OPORTMXREPET_PMLENGTH_MP3;
> > + pause |= OPORTMXPAUDAT_PAUSEPD_MP3;
> > + break;
>
> 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?
(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.
Regards,
--
Katsuhiro Suzuki
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-12-05 4:48 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-22 11:43 [PATCH 0/8] add UniPhier audio system support Katsuhiro Suzuki
2017-11-22 11:43 ` [PATCH 1/8] ASoC: spdif: Add S32_LE support for S/PDIF dummy codec drivers Katsuhiro Suzuki
2017-12-04 18:48 ` Applied "ASoC: spdif: Add S32_LE support for S/PDIF dummy codec drivers" to the asoc tree Mark Brown
2017-11-22 11:43 ` [PATCH 2/8] ASoC: uniphier: add DT bindings documentation for UniPhier EVEA Katsuhiro Suzuki
2017-11-26 19:02 ` Rob Herring
2017-12-04 18:48 ` Applied "ASoC: uniphier: add DT bindings documentation for UniPhier EVEA" to the asoc tree Mark Brown
2017-11-22 11:43 ` [PATCH 3/8] ASoC: uniphier: add DT bindings documentation for UniPhier AIO Katsuhiro Suzuki
[not found] ` <20171122114321.29196-4-suzuki.katsuhiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
2017-11-26 19:04 ` Rob Herring
2018-02-12 12:54 ` Applied "ASoC: uniphier: add DT bindings documentation for UniPhier AIO" to the asoc tree Mark Brown
2017-11-22 11:43 ` [PATCH 4/8] ASoC: uniphier: add support for UniPhier EVEA codec Katsuhiro Suzuki
2017-12-04 18:20 ` Mark Brown
2017-12-05 0:58 ` Masahiro Yamada
2017-12-05 11:59 ` Mark Brown
2017-12-04 18:48 ` Applied "ASoC: uniphier: add support for UniPhier EVEA codec" to the asoc tree Mark Brown
2017-11-22 11:43 ` [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver Katsuhiro Suzuki
2017-12-04 18:39 ` Mark Brown
[not found] ` <20171204183934.rd4vin22ktukwpip-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
2017-12-05 4:48 ` Katsuhiro Suzuki [this message]
2017-12-05 12:14 ` Mark Brown
2017-12-06 6:03 ` Katsuhiro Suzuki
2017-12-06 12:58 ` Mark Brown
2017-12-11 9:21 ` Katsuhiro Suzuki
2017-12-11 15:16 ` Mark Brown
2017-12-11 17:48 ` [alsa-devel] " Vinod Koul
2017-12-12 4:33 ` Katsuhiro Suzuki
[not found] ` <20171122114321.29196-1-suzuki.katsuhiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
2017-11-22 11:43 ` [PATCH 6/8] ASoC: uniphier: add support for UniPhier LD11/LD20 " Katsuhiro Suzuki
2017-11-22 11:43 ` [PATCH 7/8] MAINTAINERS: add entries for UniPhier ASoC sound drivers Katsuhiro Suzuki
[not found] ` <20171122114321.29196-8-suzuki.katsuhiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
2017-12-04 18:48 ` Applied "MAINTAINERS: add entries for UniPhier ASoC sound drivers" to the asoc tree Mark Brown
2017-11-22 11:43 ` [PATCH 8/8] arm64: dts: uniphier: add sound node for UniPhier Katsuhiro Suzuki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='002b01d36d84$51d80aa0$f5881fe0$@socionext.com' \
--to=suzuki.katsuhiro-uwylwvc0a2jby3ivrkzq2a@public.gmane.org \
--cc=alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org \
--cc=broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
--cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
--cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=masami.hiramatsu-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
--cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
--cc=yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).