[alsa-devel] [PATCH 0/6] ASoC: fsl_sai: Fix some minor issues
This series patches fix some minor issues in the new fsl_sai driver.
@Mark Brown I's so busy these days that I couldn't send these, which should have been review comments, in time. So some of them might be too minor to become a meaningful patch but I still sent them to see if you would like to apply.
Thank you.
Nicolin Chen (6): ASoC: fsl_sai: Keep symmetry for clk_enable() and clk_disable() ASoC: fsl_sai: Use snd_pcm_format_width() ASoC: fsl_sai: Drop useless channels check in hw_params() ASoC: fsl_sai: Drop useless ret in startup() ASoC: fsl_sai: Make dev_err information neater ASoC: fsl_sai: Sort local variable in general way
sound/soc/fsl/fsl_sai.c | 71 ++++++++++++++++--------------------------------- 1 file changed, 23 insertions(+), 48 deletions(-)
There are two functions haven't clk_disable_unprepare() if having error. Thus fix them.
Signed-off-by: Nicolin Chen Guangyu.Chen@freescale.com --- sound/soc/fsl/fsl_sai.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index 50a797e..e3e8aa1 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -111,7 +111,7 @@ static int fsl_sai_set_dai_sysclk(struct snd_soc_dai *cpu_dai, dev_err(cpu_dai->dev, "Cannot set SAI's transmitter sysclk: %d\n", ret); - return ret; + goto err_clk; }
ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq, @@ -120,12 +120,13 @@ static int fsl_sai_set_dai_sysclk(struct snd_soc_dai *cpu_dai, dev_err(cpu_dai->dev, "Cannot set SAI's receiver sysclk: %d\n", ret); - return ret; + goto err_clk; }
+err_clk: clk_disable_unprepare(sai->clk);
- return 0; + return ret; }
static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai *cpu_dai, @@ -222,7 +223,7 @@ static int fsl_sai_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) dev_err(cpu_dai->dev, "Cannot set SAI's transmitter format: %d\n", ret); - return ret; + goto err_clk; }
ret = fsl_sai_set_dai_fmt_tr(cpu_dai, fmt, FSL_FMT_RECEIVER); @@ -230,12 +231,13 @@ static int fsl_sai_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) dev_err(cpu_dai->dev, "Cannot set SAI's receiver format: %d\n", ret); - return ret; + goto err_clk; }
+err_clk: clk_disable_unprepare(sai->clk);
- return 0; + return ret; }
static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
Use common helper function snd_pcm_format_width() to make code neater.
Signed-off-by: Nicolin Chen Guangyu.Chen@freescale.com --- sound/soc/fsl/fsl_sai.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index e3e8aa1..6f1e8aa 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -244,9 +244,10 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *cpu_dai) { - u32 val_cr4, val_cr5, val_mr, reg_cr4, reg_cr5, reg_mr, word_width; + u32 val_cr4, val_cr5, val_mr, reg_cr4, reg_cr5, reg_mr; unsigned int channels = params_channels(params); struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai); + u32 word_width = snd_pcm_format_width(params_format(params));
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { reg_cr4 = FSL_SAI_TCR4; @@ -267,20 +268,6 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, val_cr5 &= ~FSL_SAI_CR5_W0W_MASK; val_cr5 &= ~FSL_SAI_CR5_FBT_MASK;
- switch (params_format(params)) { - case SNDRV_PCM_FORMAT_S16_LE: - word_width = 16; - break; - case SNDRV_PCM_FORMAT_S20_3LE: - word_width = 20; - break; - case SNDRV_PCM_FORMAT_S24_LE: - word_width = 24; - break; - default: - return -EINVAL; - } - val_cr4 |= FSL_SAI_CR4_SYWD(word_width); val_cr5 |= FSL_SAI_CR5_WNW(word_width); val_cr5 |= FSL_SAI_CR5_W0W(word_width);
SAi only supports two data channels on hardware level and the driver also does register the min->1 and max->2, so no need to check channels.
Signed-off-by: Nicolin Chen Guangyu.Chen@freescale.com --- sound/soc/fsl/fsl_sai.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index 6f1e8aa..f899e82 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -278,10 +278,7 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, val_cr5 |= FSL_SAI_CR5_FBT(0);
val_cr4 |= FSL_SAI_CR4_FRSZ(channels); - if (channels == 2 || channels == 1) - val_mr = ~0UL - ((1 << channels) - 1); - else - return -EINVAL; + val_mr = ~0UL - ((1 << channels) - 1);
sai_writel(sai, val_cr4, sai->base + reg_cr4); sai_writel(sai, val_cr5, sai->base + reg_cr5);
We can save this ret to make the code neater.
Signed-off-by: Nicolin Chen Guangyu.Chen@freescale.com --- sound/soc/fsl/fsl_sai.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index f899e82..d9ce79e 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -334,12 +334,9 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd, static int fsl_sai_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *cpu_dai) { - int ret; struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
- ret = clk_prepare_enable(sai->clk); - - return ret; + return clk_prepare_enable(sai->clk); }
static void fsl_sai_shutdown(struct snd_pcm_substream *substream,
Since using dev_err() there's no need to mention SAI any more, it will print the full name of the driver -- fsl_sai.
Signed-off-by: Nicolin Chen Guangyu.Chen@freescale.com --- sound/soc/fsl/fsl_sai.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index d9ce79e..ba59015 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -108,18 +108,14 @@ static int fsl_sai_set_dai_sysclk(struct snd_soc_dai *cpu_dai, ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq, FSL_FMT_TRANSMITTER); if (ret) { - dev_err(cpu_dai->dev, - "Cannot set SAI's transmitter sysclk: %d\n", - ret); + dev_err(cpu_dai->dev, "Cannot set tx sysclk: %d\n", ret); goto err_clk; }
ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq, FSL_FMT_RECEIVER); if (ret) { - dev_err(cpu_dai->dev, - "Cannot set SAI's receiver sysclk: %d\n", - ret); + dev_err(cpu_dai->dev, "Cannot set rx sysclk: %d\n", ret); goto err_clk; }
@@ -220,17 +216,13 @@ static int fsl_sai_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
ret = fsl_sai_set_dai_fmt_tr(cpu_dai, fmt, FSL_FMT_TRANSMITTER); if (ret) { - dev_err(cpu_dai->dev, - "Cannot set SAI's transmitter format: %d\n", - ret); + dev_err(cpu_dai->dev, "Cannot set tx format: %d\n", ret); goto err_clk; }
ret = fsl_sai_set_dai_fmt_tr(cpu_dai, fmt, FSL_FMT_RECEIVER); if (ret) { - dev_err(cpu_dai->dev, - "Cannot set SAI's receiver format: %d\n", - ret); + dev_err(cpu_dai->dev, "Cannot set rx format: %d\n", ret); goto err_clk; }
Generally we would write code for local variable like: static new_func() { struct xxx *yyy; ... int ret; }
But this driver only follows this pattern for some functions, not all. Thus this patch sorts the local variable in the general way.
Signed-off-by: Nicolin Chen Guangyu.Chen@freescale.com --- sound/soc/fsl/fsl_sai.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index ba59015..73a11c8 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -53,8 +53,8 @@ static inline void sai_writel(struct fsl_sai *sai, static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai, int clk_id, unsigned int freq, int fsl_dir) { - u32 val_cr2, reg_cr2; struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai); + u32 val_cr2, reg_cr2;
if (fsl_dir == FSL_FMT_TRANSMITTER) reg_cr2 = FSL_SAI_TCR2; @@ -90,8 +90,8 @@ static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai, static int fsl_sai_set_dai_sysclk(struct snd_soc_dai *cpu_dai, int clk_id, unsigned int freq, int dir) { - int ret; struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai); + int ret;
if (dir == SND_SOC_CLOCK_IN) return 0; @@ -128,8 +128,8 @@ err_clk: static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai *cpu_dai, unsigned int fmt, int fsl_dir) { - u32 val_cr2, val_cr3, val_cr4, reg_cr2, reg_cr3, reg_cr4; struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai); + u32 val_cr2, val_cr3, val_cr4, reg_cr2, reg_cr3, reg_cr4;
if (fsl_dir == FSL_FMT_TRANSMITTER) { reg_cr2 = FSL_SAI_TCR2; @@ -207,8 +207,8 @@ static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai *cpu_dai,
static int fsl_sai_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) { - int ret; struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai); + int ret;
ret = clk_prepare_enable(sai->clk); if (ret) @@ -236,9 +236,9 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *cpu_dai) { + struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai); u32 val_cr4, val_cr5, val_mr, reg_cr4, reg_cr5, reg_mr; unsigned int channels = params_channels(params); - struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai); u32 word_width = snd_pcm_format_width(params_format(params));
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { @@ -394,10 +394,10 @@ static const struct snd_soc_component_driver fsl_component = {
static int fsl_sai_probe(struct platform_device *pdev) { - int ret; + struct device_node *np = pdev->dev.of_node; struct fsl_sai *sai; struct resource *res; - struct device_node *np = pdev->dev.of_node; + int ret;
sai = devm_kzalloc(&pdev->dev, sizeof(*sai), GFP_KERNEL); if (!sai)
Hi Nicolin,
Thanks very much for you work.
They are all look good to me.
Reviewed-by: Xiubo Li Li.Xiubo@freescale.com
Best regards, Xiubo
This series patches fix some minor issues in the new fsl_sai driver.
@Mark Brown I's so busy these days that I couldn't send these, which should have been review comments, in time. So some of them might be too minor to become a meaningful patch but I still sent them to see if you would like to apply.
Thank you.
Nicolin Chen (6): ASoC: fsl_sai: Keep symmetry for clk_enable() and clk_disable() ASoC: fsl_sai: Use snd_pcm_format_width() ASoC: fsl_sai: Drop useless channels check in hw_params() ASoC: fsl_sai: Drop useless ret in startup() ASoC: fsl_sai: Make dev_err information neater ASoC: fsl_sai: Sort local variable in general way
sound/soc/fsl/fsl_sai.c | 71 ++++++++++++++++--------------------------------
1 file changed, 23 insertions(+), 48 deletions(-)
-- 1.8.4
participants (3)
-
Li.Xiubo＠freescale.com -
Mark Brown -
Nicolin Chen