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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
Sent: Tuesday, December 5, 2017 3:40 AM
To: Suzuki, Katsuhiro/鈴木 勝博 <redacted>
Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org; Rob Herring <redacted>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Yamada, Masahiro/山
田 真弘 [off-list ref]; Masami Hiramatsu [off-list ref]; Jassi Brar
[off-list ref]; 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:

quoted

 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.

quoted

+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?

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+	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