[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@starfivetech.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,
On Wed, Jun 07, 2023 at 04:14:39PM +0800, Walker Chen wrote:
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.
As covered in submitting-patches.rst please submit one patch per change rather than combining multiple changes into a single patch, it makes things much easier to review and handle.
- 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;
I'm not sure this change to use numbers here is a win - the _BIT definitions look fine (I might've called them _SHIFT but whatever).
-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);
This rename wasn't mentioned in the changelog.
On 2023/6/7 19:44, Mark Brown wrote:
On Wed, Jun 07, 2023 at 04:14:39PM +0800, Walker Chen wrote:
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.
As covered in submitting-patches.rst please submit one patch per change rather than combining multiple changes into a single patch, it makes things much easier to review and handle.
Hi Mark,
Thanks for your review. OK, I will submit a single patch for each change in the next version.
- 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;I'm not sure this change to use numbers here is a win - the _BIT definitions look fine (I might've called them _SHIFT but whatever).
This is Claudiu's advice. Using the macro BIT() to replace these definition of *_BIT, it will result in big changes in the code. Please refer to previous comments: https://lore.kernel.org/all/143e2fa2-e85d-8036-4f74-ca250c026c1b@microchip.c...
@Claudiu What do think about this ?
-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);
This rename wasn't mentioned in the changelog.
Will be added in the change log.
Best regards, Walker
On Thu, Jun 08, 2023 at 10:15:03AM +0800, Walker Chen wrote:
On 2023/6/7 19:44, Mark Brown wrote:
(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;
I'm not sure this change to use numbers here is a win - the _BIT definitions look fine (I might've called them _SHIFT but whatever).
This is Claudiu's advice. Using the macro BIT() to replace these definition of *_BIT, it will result in big changes in the code.
I'm questioning doing a change at all.
Please refer to previous comments: https://lore.kernel.org/all/143e2fa2-e85d-8036-4f74-ca250c026c1b@microchip.c...
I can't find the comments you're referring to in there.
On 2023/6/8 18:15, Mark Brown wrote:
On Thu, Jun 08, 2023 at 10:15:03AM +0800, Walker Chen wrote:
On 2023/6/7 19:44, Mark Brown wrote:
(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;I'm not sure this change to use numbers here is a win - the _BIT definitions look fine (I might've called them _SHIFT but whatever).
This is Claudiu's advice. Using the macro BIT() to replace these definition of *_BIT, it will result in big changes in the code.
I'm questioning doing a change at all.
Please refer to previous comments: https://lore.kernel.org/all/143e2fa2-e85d-8036-4f74-ca250c026c1b@microchip.c...
I can't find the comments you're referring to in there.
You should see the following comments in the link above:
#define CLKPOL_BIT 5#define TRITXEN_BIT 4#define ELM_BIT 3#define SYNCM_BIT 2#define MS_BIT 1
Instead of these *_BIT defines as plain numbers you can defined them using BIT() macro and use macros in place instead of
On Thu, Jun 08, 2023 at 06:43:09PM +0800, Walker Chen wrote:
On 2023/6/8 18:15, Mark Brown wrote:
I can't find the comments you're referring to in there.
You should see the following comments in the link above:
#define CLKPOL_BIT 5#define TRITXEN_BIT 4#define ELM_BIT 3#define SYNCM_BIT 2#define MS_BIT 1
Instead of these *_BIT defines as plain numbers you can defined them using BIT() macro and use macros in place instead of
The usual pattern is to have defines for both the shift and the mask, not just one.
On 2023/6/8 18:50, Mark Brown wrote:
On Thu, Jun 08, 2023 at 06:43:09PM +0800, Walker Chen wrote:
On 2023/6/8 18:15, Mark Brown wrote:
I can't find the comments you're referring to in there.
You should see the following comments in the link above:
#define CLKPOL_BIT 5#define TRITXEN_BIT 4#define ELM_BIT 3#define SYNCM_BIT 2#define MS_BIT 1Instead of these *_BIT defines as plain numbers you can defined them using BIT() macro and use macros in place instead of
The usual pattern is to have defines for both the shift and the mask, not just one.
OK, I see. It's not necessary to make these changes. Thanks.
Best regards, Walker
On 08.06.2023 13:43, Walker Chen wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
On 2023/6/8 18:15, Mark Brown wrote:
On Thu, Jun 08, 2023 at 10:15:03AM +0800, Walker Chen wrote:
On 2023/6/7 19:44, Mark Brown wrote:
(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;I'm not sure this change to use numbers here is a win - the _BIT definitions look fine (I might've called them _SHIFT but whatever).
This is Claudiu's advice. Using the macro BIT() to replace these definition of *_BIT, it will result in big changes in the code.
I'm questioning doing a change at all.
Please refer to previous comments: https://lore.kernel.org/all/143e2fa2-e85d-8036-4f74-ca250c026c1b@microchip.c...
I can't find the comments you're referring to in there.
You should see the following comments in the link above:
#define CLKPOL_BIT 5#define TRITXEN_BIT 4#define ELM_BIT 3#define SYNCM_BIT 2#define MS_BIT 1Instead of these *_BIT defines as plain numbers you can defined them using BIT() macro and use macros in place instead of
As mentioned in [1] I sent that by accident. Please ignore it and sorry for confusion.
[1] https://lore.kernel.org/all/7a1a3ac3-10ec-9935-bca1-023cec6c0024@microchip.c...
participants (2)
-
Claudiu.Beznea＠microchip.com -
Mark Brown -
Walker Chen