Date:   Wed, 7 Jun 2023 16:14:39 +0800
From:   Walker Chen <walker.chen@...rfivetech.com>
To:     Mark Brown <broonie@...nel.org>,
        Claudiu Beznea <Claudiu.Beznea@...rochip.com>,
        Dan Carpenter <dan.carpenter@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Takashi Iwai <tiwai@...e.com>,
        "Jaroslav Kysela" <perex@...ex.cz>,
        Walker Chen <walker.chen@...rfivetech.com>
CC:     <alsa-devel@...a-project.org>, <linux-kernel@...r.kernel.org>
Subject: [PATCH v1] ASoC: starfive: Cleanup and fix error check for JH7110 TDM

Some minor issues were found during addtional testing and static
analysis. The patch fixed these minor issues.
1.Use BIT() macro to indicate configuration for TDM registers.

2.Fix the check for devm_reset_control_array_get_exclusive return
value. The devm_reset_control_array_get_exclusive() function may return
NULL if it's an optional request. If optional is intended then NULL
should not be treated as an error case, but as a special kind of success
case. So here the IS_ERR() is used to check better.

Fixes: fd4762b6b5cf ("ASoC: starfive: Add JH7110 TDM driver")
Signed-off-by: Walker Chen <walker.chen@...rfivetech.com>
---
Fix the following issue:
https://lore.kernel.org/all/ZH7t6Nc+NTcGpq%2F3@moroto/
---
 sound/soc/starfive/jh7110_tdm.c | 200 +++++++++++---------------------
 1 file changed, 68 insertions(+), 132 deletions(-)

diff --git a/sound/soc/starfive/jh7110_tdm.c b/sound/soc/starfive/jh7110_tdm.c
index 973b910d2d3e..139ff091672e 100644
--- a/sound/soc/starfive/jh7110_tdm.c
+++ b/sound/soc/starfive/jh7110_tdm.c
@@ -24,63 +24,33 @@
 #include <sound/soc.h>
 #include <sound/soc-dai.h>
 
+/* Register offsets for JH7110 TDM device */
 #define TDM_PCMGBCR			0x00
-	#define PCMGBCR_MASK		0x1e
-	#define PCMGBCR_ENABLE		BIT(0)
-	#define PCMGBCR_TRITXEN		BIT(4)
-	#define CLKPOL_BIT		5
-	#define TRITXEN_BIT		4
-	#define ELM_BIT			3
-	#define SYNCM_BIT		2
-	#define MS_BIT			1
 #define TDM_PCMTXCR			0x04
-	#define PCMTXCR_TXEN		BIT(0)
-	#define IFL_BIT			11
-	#define WL_BIT			8
-	#define SSCALE_BIT		4
-	#define SL_BIT			2
-	#define LRJ_BIT			1
 #define TDM_PCMRXCR			0x08
-	#define PCMRXCR_RXEN		BIT(0)
-	#define PCMRXCR_RXSL_MASK	0xc
-	#define PCMRXCR_RXSL_16BIT	0x4
-	#define PCMRXCR_RXSL_32BIT	0x8
-	#define PCMRXCR_SCALE_MASK	0xf0
-	#define PCMRXCR_SCALE_1CH	0x10
 #define TDM_PCMDIV			0x0c
 
-#define JH7110_TDM_FIFO			0x170c0000
-#define JH7110_TDM_FIFO_DEPTH		32
+/* Bit definition for TDM_PCMGBCR */
+#define TDM_PCMGBCR_SYNCM_LONG		BIT(2)
+#define TDM_PCMGBCR_MS_SLAVE		BIT(1)
+#define TDM_PCMGBCR_EN			BIT(0)
 
-enum TDM_MASTER_SLAVE_MODE {
-	TDM_AS_MASTER = 0,
-	TDM_AS_SLAVE,
-};
+/* Bit definition for TDM_PCMTXCR */
+#define TDM_PCMTXCR_LEFT_J		BIT(1)
+#define TDM_PCMTXCR_TXEN		BIT(0)
 
-enum TDM_CLKPOL {
-	/* tx raising and rx falling */
-	TDM_TX_RASING_RX_FALLING = 0,
-	/* tx falling and rx raising */
-	TDM_TX_FALLING_RX_RASING,
-};
+/* Bit definition for TDM_PCMRXCR */
+#define TDM_PCMRXCR_LEFT_J		BIT(1)
+#define TDM_PCMRXCR_RXEN		BIT(0)
 
-enum TDM_ELM {
-	/* only work while SYNCM=0 */
-	TDM_ELM_LATE = 0,
-	TDM_ELM_EARLY,
-};
-
-enum TDM_SYNCM {
-	/* short frame sync */
-	TDM_SYNCM_SHORT = 0,
-	/* long frame sync */
-	TDM_SYNCM_LONG,
-};
+#define JH7110_TDM_FIFO			0x170c0000
+#define JH7110_TDM_FIFO_DEPTH		32
 
-enum TDM_IFL {
-	/* FIFO to send or received : half-1/2, Quarter-1/4 */
-	TDM_FIFO_HALF = 0,
-	TDM_FIFO_QUARTER,
+enum TDM_SL {
+	/* send or received slot length */
+	TDM_8BIT_SLOT_LEN = 0,
+	TDM_16BIT_SLOT_LEN,
+	TDM_32BIT_SLOT_LEN,
 };
 
 enum TDM_WL {
@@ -92,43 +62,24 @@ enum TDM_WL {
 	TDM_32BIT_WORD_LEN,
 };
 
-enum TDM_SL {
-	/* send or received slot length */
-	TDM_8BIT_SLOT_LEN = 0,
-	TDM_16BIT_SLOT_LEN,
-	TDM_32BIT_SLOT_LEN,
-};
-
-enum TDM_LRJ {
-	/* left-justify or right-justify */
-	TDM_RIGHT_JUSTIFY = 0,
-	TDM_LEFT_JUSTIFT,
-};
-
-struct tdm_chan_cfg {
-	enum TDM_IFL ifl;
-	enum TDM_WL  wl;
-	unsigned char sscale;
-	enum TDM_SL  sl;
-	enum TDM_LRJ lrj;
-	unsigned char enable;
-};
-
 struct jh7110_tdm_dev {
 	void __iomem *tdm_base;
 	struct device *dev;
 	struct clk_bulk_data clks[6];
 	struct reset_control *resets;
 
-	enum TDM_CLKPOL clkpolity;
-	enum TDM_ELM	elm;
-	enum TDM_SYNCM	syncm;
-	enum TDM_MASTER_SLAVE_MODE ms_mode;
+	/* related to PCMTXCR */
+	u16 txwl;
+	u16 txsscale;
+	u16 txsl;
 
-	struct tdm_chan_cfg tx;
-	struct tdm_chan_cfg rx;
+	/* related to PCMRXCR */
+	u16 rxwl;
+	u16 rxsscale;
+	u16 rxsl;
 
 	u16 syncdiv;
+	u16 syncm;
 	u32 samplerate;
 	u32 pcmclk;
 
@@ -166,13 +117,13 @@ static void jh7110_tdm_start(struct jh7110_tdm_dev *tdm,
 	u32 data;
 
 	data = jh7110_tdm_readl(tdm, TDM_PCMGBCR);
-	jh7110_tdm_writel(tdm, TDM_PCMGBCR, data | PCMGBCR_ENABLE);
+	jh7110_tdm_writel(tdm, TDM_PCMGBCR, data | TDM_PCMGBCR_EN);
 
 	/* restore context */
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		jh7110_tdm_writel(tdm, TDM_PCMTXCR, tdm->saved_pcmtxcr | PCMTXCR_TXEN);
+		jh7110_tdm_writel(tdm, TDM_PCMTXCR, tdm->saved_pcmtxcr | TDM_PCMTXCR_TXEN);
 	else
-		jh7110_tdm_writel(tdm, TDM_PCMRXCR, tdm->saved_pcmrxcr | PCMRXCR_RXEN);
+		jh7110_tdm_writel(tdm, TDM_PCMRXCR, tdm->saved_pcmrxcr | TDM_PCMRXCR_RXEN);
 }
 
 static void jh7110_tdm_stop(struct jh7110_tdm_dev *tdm,
@@ -182,11 +133,11 @@ static void jh7110_tdm_stop(struct jh7110_tdm_dev *tdm,
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		val = jh7110_tdm_readl(tdm, TDM_PCMTXCR);
-		val &= ~PCMTXCR_TXEN;
+		val &= ~TDM_PCMTXCR_TXEN;
 		jh7110_tdm_writel(tdm, TDM_PCMTXCR, val);
 	} else {
 		val = jh7110_tdm_readl(tdm, TDM_PCMRXCR);
-		val &= ~PCMRXCR_RXEN;
+		val &= ~TDM_PCMRXCR_RXEN;
 		jh7110_tdm_writel(tdm, TDM_PCMRXCR, val);
 	}
 }
@@ -195,15 +146,15 @@ static int jh7110_tdm_syncdiv(struct jh7110_tdm_dev *tdm)
 {
 	u32 sl, sscale, syncdiv;
 
-	if (tdm->rx.sl >= tdm->tx.sl)
-		sl = tdm->rx.sl;
+	if (tdm->rxsl >= tdm->txsl)
+		sl = tdm->rxsl;
 	else
-		sl = tdm->tx.sl;
+		sl = tdm->txsl;
 
-	if (tdm->rx.sscale >= tdm->tx.sscale)
-		sscale = tdm->rx.sscale;
+	if (tdm->rxsscale >= tdm->txsscale)
+		sscale = tdm->rxsscale;
 	else
-		sscale = tdm->tx.sscale;
+		sscale = tdm->txsscale;
 
 	syncdiv = tdm->pcmclk / tdm->samplerate - 1;
 
@@ -212,10 +163,10 @@ static int jh7110_tdm_syncdiv(struct jh7110_tdm_dev *tdm)
 		return -EINVAL;
 	}
 
-	if (tdm->syncm == TDM_SYNCM_LONG &&
-	    (tdm->rx.sscale <= 1 || tdm->tx.sscale <= 1) &&
+	if (tdm->syncm == TDM_PCMGBCR_SYNCM_LONG &&
+	    (tdm->rxsscale <= 1 || tdm->txsscale <= 1) &&
 	    ((syncdiv + 1) <= sl)) {
-		dev_err(tdm->dev, "Wrong syncdiv! It must be (syncdiv+1) > max[tx.sl, rx.sl]\n");
+		dev_err(tdm->dev, "Wrong syncdiv! It must be (syncdiv+1) > max[txsl, rxsl]\n");
 		return -EINVAL;
 	}
 
@@ -233,17 +184,15 @@ static int jh7110_tdm_config(struct jh7110_tdm_dev *tdm,
 	if (ret)
 		return ret;
 
-	datarx = (tdm->rx.ifl << IFL_BIT) |
-		  (tdm->rx.wl << WL_BIT) |
-		  (tdm->rx.sscale << SSCALE_BIT) |
-		  (tdm->rx.sl << SL_BIT) |
-		  (tdm->rx.lrj << LRJ_BIT);
+	datarx = (tdm->rxwl << 8) |
+		  (tdm->rxsscale << 4) |
+		  (tdm->rxsl << 2) |
+		  TDM_PCMRXCR_LEFT_J;
 
-	datatx = (tdm->tx.ifl << IFL_BIT) |
-		  (tdm->tx.wl << WL_BIT) |
-		  (tdm->tx.sscale << SSCALE_BIT) |
-		  (tdm->tx.sl << SL_BIT) |
-		  (tdm->tx.lrj << LRJ_BIT);
+	datatx = (tdm->txwl << 8) |
+		  (tdm->txsscale << 4) |
+		  (tdm->txsl << 2) |
+		  TDM_PCMTXCR_LEFT_J;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		jh7110_tdm_writel(tdm, TDM_PCMTXCR, datatx);
@@ -346,7 +295,8 @@ static int jh7110_tdm_hw_params(struct snd_pcm_substream *substream,
 				struct snd_soc_dai *dai)
 {
 	struct jh7110_tdm_dev *tdm = snd_soc_dai_get_drvdata(dai);
-	int chan_wl, chan_sl, chan_nr;
+	int chan_nr;
+	unsigned short chan_wl, chan_sl;
 	unsigned int data_width;
 	unsigned int dma_bus_width;
 	struct snd_dmaengine_dai_dma_data *dma_data = NULL;
@@ -389,15 +339,15 @@ static int jh7110_tdm_hw_params(struct snd_pcm_substream *substream,
 	}
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		tdm->tx.wl = chan_wl;
-		tdm->tx.sl = chan_sl;
-		tdm->tx.sscale = chan_nr;
+		tdm->txwl = chan_wl;
+		tdm->txsl = chan_sl;
+		tdm->txsscale = chan_nr;
 		tdm->play_dma_data.addr_width = dma_bus_width;
 		dma_data = &tdm->play_dma_data;
 	} else {
-		tdm->rx.wl = chan_wl;
-		tdm->rx.sl = chan_sl;
-		tdm->rx.sscale = chan_nr;
+		tdm->rxwl = chan_wl;
+		tdm->rxsl = chan_sl;
+		tdm->rxsscale = chan_nr;
 		tdm->capture_dma_data.addr_width = dma_bus_width;
 		dma_data = &tdm->capture_dma_data;
 	}
@@ -444,15 +394,17 @@ static int jh7110_tdm_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 	struct jh7110_tdm_dev *tdm = snd_soc_dai_get_drvdata(cpu_dai);
 	unsigned int gbcr;
 
+	gbcr = tdm->syncm;
+
 	/* set master/slave audio interface */
 	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
 	case SND_SOC_DAIFMT_BP_FP:
 		/* cpu is master */
-		tdm->ms_mode = TDM_AS_MASTER;
+		gbcr &= ~TDM_PCMGBCR_MS_SLAVE;
 		break;
 	case SND_SOC_DAIFMT_BC_FC:
 		/* codec is master */
-		tdm->ms_mode = TDM_AS_SLAVE;
+		gbcr |= TDM_PCMGBCR_MS_SLAVE;
 		break;
 	case SND_SOC_DAIFMT_BC_FP:
 	case SND_SOC_DAIFMT_BP_FC:
@@ -462,10 +414,6 @@ static int jh7110_tdm_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 		return -EINVAL;
 	}
 
-	gbcr = (tdm->clkpolity << CLKPOL_BIT) |
-		(tdm->elm << ELM_BIT) |
-		(tdm->syncm << SYNCM_BIT) |
-		(tdm->ms_mode << MS_BIT);
 	jh7110_tdm_writel(tdm, TDM_PCMGBCR, gbcr);
 
 	return 0;
@@ -537,18 +485,7 @@ static const struct snd_dmaengine_pcm_config jh7110_dmaengine_pcm_config = {
 
 static void jh7110_tdm_init_params(struct jh7110_tdm_dev *tdm)
 {
-	tdm->clkpolity = TDM_TX_RASING_RX_FALLING;
-	tdm->elm = TDM_ELM_LATE;
-	tdm->syncm = TDM_SYNCM_SHORT;
-
-	tdm->rx.ifl = TDM_FIFO_HALF;
-	tdm->tx.ifl = TDM_FIFO_HALF;
-	tdm->rx.wl = TDM_16BIT_WORD_LEN;
-	tdm->tx.wl = TDM_16BIT_WORD_LEN;
-	tdm->rx.sscale = 2;
-	tdm->tx.sscale = 2;
-	tdm->rx.lrj = TDM_LEFT_JUSTIFT;
-	tdm->tx.lrj = TDM_LEFT_JUSTIFT;
+	tdm->syncm = 0;
 
 	tdm->play_dma_data.addr = JH7110_TDM_FIFO;
 	tdm->play_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
@@ -580,10 +517,9 @@ static int jh7110_tdm_clk_reset_get(struct platform_device *pdev,
 	}
 
 	tdm->resets = devm_reset_control_array_get_exclusive(&pdev->dev);
-	if (IS_ERR_OR_NULL(tdm->resets)) {
-		ret = PTR_ERR(tdm->resets);
-		dev_err(&pdev->dev, "Failed to get tdm resets");
-		return ret;
+	if (IS_ERR(tdm->resets)) {
+		dev_err(&pdev->dev, "Failed to get tdm resets\n");
+		return PTR_ERR(tdm->resets);
 	}
 
 	return 0;
@@ -649,12 +585,12 @@ static int jh7110_tdm_dev_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id jh7110_tdm_of_match[] = {
+static const struct of_device_id jh7110_tdm_match[] = {
 	{ .compatible = "starfive,jh7110-tdm", },
 	{}
 };
 
-MODULE_DEVICE_TABLE(of, jh7110_tdm_of_match);
+MODULE_DEVICE_TABLE(of, jh7110_tdm_match);
 
 static const struct dev_pm_ops jh7110_tdm_pm_ops = {
 	RUNTIME_PM_OPS(jh7110_tdm_runtime_suspend,
@@ -666,7 +602,7 @@ static const struct dev_pm_ops jh7110_tdm_pm_ops = {
 static struct platform_driver jh7110_tdm_driver = {
 	.driver = {
 		.name = "jh7110-tdm",
-		.of_match_table = jh7110_tdm_of_match,
+		.of_match_table = jh7110_tdm_match,
 		.pm = pm_ptr(&jh7110_tdm_pm_ops),
 	},
 	.probe = jh7110_tdm_probe,
-- 
2.17.1

Powered by blists - more mailing lists