| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <002b01d36d84$51d80aa0$f5881fe0$@socionext.com>
Date: Tue, 5 Dec 2017 13:48:39 +0900
From: "Katsuhiro Suzuki" <suzuki.katsuhiro@...ionext.com>
To: "'Mark Brown'" <broonie@...nel.org>,
Suzuki, Katsuhiro/鈴木 勝博
<suzuki.katsuhiro@...ionext.com>
Cc: <alsa-devel@...a-project.org>, "Rob Herring" <robh+dt@...nel.org>,
<devicetree@...r.kernel.org>,
Yamada, Masahiro/山田 真弘
<yamada.masahiro@...ionext.com>,
"Masami Hiramatsu" <masami.hiramatsu@...aro.org>,
"Jassi Brar" <jaswinder.singh@...aro.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver
Hello,
Thanks a lot for your review.
> -----Original Message-----
> From: Mark Brown [mailto:broonie@...nel.org]
> Sent: Tuesday, December 5, 2017 3:40 AM
> To: Suzuki, Katsuhiro/鈴木 勝博 <suzuki.katsuhiro@...ionext.com>
> Cc: alsa-devel@...a-project.org; Rob Herring <robh+dt@...nel.org>; devicetree@...r.kernel.org; Yamada, Masahiro/山
> 田 真弘 <yamada.masahiro@...ionext.com>; Masami Hiramatsu <masami.hiramatsu@...aro.org>; Jassi Brar
> <jaswinder.singh@...aro.org>; linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.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
Powered by blists - more mailing lists