Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,


On 04/10/2016 14:40, Thomas Petazzoni wrote:
Hello,

On Tue,  4 Oct 2016 11:46:19 +0200, Mylène Josserand wrote:
Add the digital sun8i audio codec which handles the base register
(without DAI).

I'm not sure what you mean by "which handles the base register".

I wanted to explain that it is registers for audio codec and not PRCM ones. This is, maybe, unclear (and useless ?).


diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig
index 7aee95a..9e287b0 100644
--- a/sound/soc/sunxi/Kconfig
+++ b/sound/soc/sunxi/Kconfig
@@ -27,6 +27,15 @@ config SND_SUN4I_SPDIF
 	  Say Y or M to add support for the S/PDIF audio block in the Allwinner
 	  A10 and affiliated SoCs.

+config SND_SUN8I_CODEC
+	tristate "Allwinner SUN8I audio codec"
+	select REGMAP_MMIO
+        help

Indentation issue here, it should be intended with one tab, not spaces.

You probably also want a "depends on OF" here.

Yes, thanks !


+/* CODEC_OFFSET represents the offset of the codec registers
+ * and not all the DAI registers
+ */

This is not the proper comment style I believe for audio code, it
should be:

/*
 * ...
 */

+#define CODEC_OFFSET				0x200

Do you really need this CODEC_OFFSET macro? Why not simply use directly
the right offsets? I.e instead of:

  #define SUN8I_SYSCLK_CTL			(0x20c - CODEC_OFFSET)

use:

  #define SUN8I_SYSCLK_CTL			0xc

I thought it could be easier to find registers using offset but I guess that register's names are enough.


+#define CODEC_BASSADDRESS			0x01c22c00

This define is not used anywhere.

Yes, sorry, I forgot to remove it.


+#define SUN8I_SYSCLK_CTL			(0x20c - CODEC_OFFSET)
+#define SUN8I_SYSCLK_CTL_AIF1CLK_ENA		(11)
+#define SUN8I_SYSCLK_CTL_SYSCLK_ENA		(3)
+#define SUN8I_SYSCLK_CTL_SYSCLK_SRC		(0)

Parenthesis around single values are not really useful.

+#define SUN8I_MOD_CLK_ENA			(0x210 - CODEC_OFFSET)
+#define SUN8I_MOD_CLK_ENA_AIF1			(15)
+#define SUN8I_MOD_CLK_ENA_DAC			(2)
+#define SUN8I_MOD_RST_CTL			(0x214 - CODEC_OFFSET)
+#define SUN8I_MOD_RST_CTL_AIF1			(15)
+#define SUN8I_MOD_RST_CTL_DAC			(2)
+#define SUN8I_SYS_SR_CTRL			(0x218 - CODEC_OFFSET)
+#define SUN8I_SYS_SR_CTRL_AIF1_FS		(12)
+#define SUN8I_SYS_SR_CTRL_AIF2_FS		(8)
+#define SUN8I_AIF1CLK_CTRL			(0x240 - CODEC_OFFSET)
+#define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD	(15)
+#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV	(14)
+#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV	(13)
+#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV	(9)
+#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV	(6)
+#define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ	(4)
+#define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT	(2)
+#define SUN8I_AIF1_DACDAT_CTRL			(0x248 - CODEC_OFFSET)
+#define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA	(15)
+#define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA	(14)
+#define SUN8I_DAC_DIG_CTRL			(0x320 - CODEC_OFFSET)
+#define SUN8I_DAC_DIG_CTRL_ENDA		(15)
+#define SUN8I_DAC_MXR_SRC			(0x330 - CODEC_OFFSET)
+#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA0L (15)
+#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA1L (14)
+#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF2DACL (13)
+#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_ADCL	(12)
+#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA0R (11)
+#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA1R (10)
+#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF2DACR (9)
+#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_ADCR	(8)

Indentation of the value is not very clean for those last defines.

+static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	struct sun8i_codec *scodec = snd_soc_codec_get_drvdata(dai->codec);
+	unsigned long value;

I'm not sure "unsigned long" is a very good choice here, it's going to
be a 64 bits integer on 64 bits platform. I'd suggest to use "u32",
which also seems to be what's used in _set_fmt() function of the
sun4i-i2s.c driver.

Agreed, thanks !



+static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
+				 struct snd_pcm_hw_params *params,
+				 struct snd_soc_dai *dai)
+{
+	int rs_value  = 0;

Two spaces before the = sign, not needed. Is the initialization to 0
really needed? Also, this should be a u32.

ditto


+	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
+			   0x3 << SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ,

Maybe a #define value to replace the hardcoded 0x3 ?

+			   rs_value << SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ);
+
+	/* calculate bclk_lrck_div Ratio */
+	bclk_lrck_div = sample_resolution * 2;
+	switch (bclk_lrck_div) {
+	case 16:
+		bclk_lrck_div = 0;
+		break;
+	case 32:
+		bclk_lrck_div = 1;
+		break;
+	case 64:
+		bclk_lrck_div = 2;
+		break;
+	case 128:
+		bclk_lrck_div = 3;
+		break;
+	case 256:
+		bclk_lrck_div = 4;
+		break;

This could quite easily be replaced by a formula, if you don't care
about error checking:

	bclk_lrck_div = log2(bclk_lrck_div) - 4;

Of course, if you care about error checking, this switch is nicer.

+	default:

So there's no error checking if the value is not supported?

You are right. I guess it should return -EINVAL.

[snip]



+static struct snd_soc_dai_driver sun8i_codec_dai = {
+	.name = "sun8i",
+	/* playback capabilities */
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_192000 |
+			SNDRV_PCM_RATE_KNOT,
+		.formats = SNDRV_PCM_FMTBIT_S8 |
+			SNDRV_PCM_FMTBIT_S16_LE |
+			SNDRV_PCM_FMTBIT_S18_3LE |
+			SNDRV_PCM_FMTBIT_S20_3LE |
+			SNDRV_PCM_FMTBIT_S24_LE |
+			SNDRV_PCM_FMTBIT_S32_LE,
+	},
+	/* pcm operations */
+	.ops = &sun8i_codec_dai_ops,
+};
+EXPORT_SYMBOL(sun8i_codec_dai);

This EXPORT_SYMBOL looks wrong. First because it doesn't seem to be
used outside of this module. And second because using EXPORT_SYMBOL on
a function defined as static doesn't make much sense, as the "static"
qualifier limits the visibility of the symbol to the current
compilation unit.


Yes, sorry, I missed it from the clean-up of the original driver.

[snip]

+static int sun8i_codec_probe(struct platform_device *pdev)
+{
+	struct resource *res_base;
+	struct sun8i_codec *scodec;
+	void __iomem *base;
+
+	scodec = devm_kzalloc(&pdev->dev, sizeof(*scodec), GFP_KERNEL);
+	if (!scodec)
+		return -ENOMEM;
+
+	scodec->dev = &pdev->dev;
+
+	/* Get the clocks from the DT */
+	scodec->clk_module = devm_clk_get(&pdev->dev, "codec");
+	if (IS_ERR(scodec->clk_module)) {
+		dev_err(&pdev->dev, "Failed to get the module clock\n");
+		return PTR_ERR(scodec->clk_module);
+	}
+	if (clk_prepare_enable(scodec->clk_module))
+		pr_err("err:open failed;\n");

Grr, pr_err, not good. Plus you want to return with an error from the
probe() function.

Oh, sorry for that ugly use :(


+
+	scodec->clk_apb = devm_clk_get(&pdev->dev, "apb");
+	if (IS_ERR(scodec->clk_apb)) {
+		dev_err(&pdev->dev, "Failed to get the apb clock\n");
+		return PTR_ERR(scodec->clk_apb);
+	}
+	if (clk_prepare_enable(scodec->clk_apb))
+		pr_err("err:open failed;\n");

Ditto. + unprepare/disable the previous clock.

[snip]

ack, thank you for the review!


--
Mylène Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux