October 2019 - Alsa-devel - mailman.alsa-project.org

[alsa-devel] [PATCH -next] ALSA: intel_hdmi: Remove dev_err() on platform_get_irq() failure
by YueHaibing 25 Oct '19

25 Oct '19

platform_get_irq() will call dev_err() itself on failure, so there is no need for the driver to also do this. This is detected by coccinelle. Signed-off-by: YueHaibing <yuehaibing(a)huawei.com> --- sound/x86/intel_hdmi_audio.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 5fd4e32..cd389d2 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -1708,10 +1708,8 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) /* get resources */ irq = platform_get_irq(pdev, 0); - if (irq < 0) { - dev_err(&pdev->dev, "Could not get irq resource: %d\n", irq); + if (irq < 0) return irq; - } res_mmio = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res_mmio) { -- 2.7.4

2 1

[alsa-devel] [PATCH][next] ASoC: rt5677: Add missing null check for failed allocation of rt5677_dsp
by Colin King 25 Oct '19

25 Oct '19

From: Colin Ian King <colin.king(a)canonical.com> The allocation of rt5677_dsp can potentially fail and return null, so add a null check and return -ENOMEM on a memory allocation failure. Addresses-Coverity: ("Dereference null return") Fixes: a0e0d135427c ("ASoC: rt5677: Add a PCM device for streaming hotword via SPI") Signed-off-by: Colin Ian King <colin.king(a)canonical.com> --- sound/soc/codecs/rt5677-spi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/soc/codecs/rt5677-spi.c b/sound/soc/codecs/rt5677-spi.c index 36c02d200cfc..3a17643fcd9f 100644 --- a/sound/soc/codecs/rt5677-spi.c +++ b/sound/soc/codecs/rt5677-spi.c @@ -376,6 +376,8 @@ static int rt5677_spi_pcm_probe(struct snd_soc_component *component) rt5677_dsp = devm_kzalloc(component->dev, sizeof(*rt5677_dsp), GFP_KERNEL); + if (!rt5677_dsp) + return -ENOMEM; rt5677_dsp->dev = &g_spi->dev; mutex_init(&rt5677_dsp->dma_lock); INIT_DELAYED_WORK(&rt5677_dsp->copy_work, rt5677_spi_copy_work); -- 2.20.1

3 2

[alsa-devel] [PATCH v2] ASoC: soc-dpcm: tidyup for_each_dpcm_xx() macro
by Kuninori Morimoto 25 Oct '19

25 Oct '19

From: Kuninori Morimoto <kuninori.morimoto.gx(a)renesas.com> for_each_dpcm_xx() macro is using "dpcm" as parameter (1), but, it is also struct member (2). #define for_each_dpcm_fe(be, stream, dpcm) \ list_for_each_entry(dpcm, &(be)->dpcm[stream]...) ^^^^(1) ^^^^(2) Thus, it will be compile error if user not used "dpcm" as parameter for_each_dpcm_fe(be, stream, dp) ^^ This patch fixup it. Reported-by: Pavel Machek <pavel(a)denx.de> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx(a)renesas.com> --- v1 -> v2 - based on mark/for-5.5 branch include/sound/soc-dpcm.h | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h index e55aeb0..b654ebf 100644 --- a/include/sound/soc-dpcm.h +++ b/include/sound/soc-dpcm.h @@ -103,15 +103,15 @@ struct snd_soc_dpcm_runtime { int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */ }; -#define for_each_dpcm_fe(be, stream, dpcm) \ - list_for_each_entry(dpcm, &(be)->dpcm[stream].fe_clients, list_fe) - -#define for_each_dpcm_be(fe, stream, dpcm) \ - list_for_each_entry(dpcm, &(fe)->dpcm[stream].be_clients, list_be) -#define for_each_dpcm_be_safe(fe, stream, dpcm, _dpcm) \ - list_for_each_entry_safe(dpcm, _dpcm, &(fe)->dpcm[stream].be_clients, list_be) -#define for_each_dpcm_be_rollback(fe, stream, dpcm) \ - list_for_each_entry_continue_reverse(dpcm, &(fe)->dpcm[stream].be_clients, list_be) +#define for_each_dpcm_fe(be, stream, _dpcm) \ + list_for_each_entry(_dpcm, &(be)->dpcm[stream].fe_clients, list_fe) + +#define for_each_dpcm_be(fe, stream, _dpcm) \ + list_for_each_entry(_dpcm, &(fe)->dpcm[stream].be_clients, list_be) +#define for_each_dpcm_be_safe(fe, stream, _dpcm, __dpcm) \ + list_for_each_entry_safe(_dpcm, __dpcm, &(fe)->dpcm[stream].be_clients, list_be) +#define for_each_dpcm_be_rollback(fe, stream, _dpcm) \ + list_for_each_entry_continue_reverse(_dpcm, &(fe)->dpcm[stream].be_clients, list_be) /* can this BE stop and free */ int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe, -- 2.7.4

2 1

[alsa-devel] [PATCH] ASoC: rt5677-spi: fix sparse warnings
by Curtis Malainey 25 Oct '19

25 Oct '19

Fix bugs reported by kbuild test robot Fixes: a0e0d135427c ("ASoC: rt5677: Add a PCM device for streaming hotword via SPI") Reported-by: kbuild test robot <lkp(a)intel.com> Signed-off-by: Curtis Malainey <cujomalainey(a)chromium.org> --- sound/soc/codecs/rt5677-spi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sound/soc/codecs/rt5677-spi.c b/sound/soc/codecs/rt5677-spi.c index 36c02d200cfc..958871b342ea 100644 --- a/sound/soc/codecs/rt5677-spi.c +++ b/sound/soc/codecs/rt5677-spi.c @@ -145,7 +145,7 @@ static int rt5677_spi_hw_free( snd_soc_component_get_drvdata(component); mutex_lock(&rt5677_dsp->dma_lock); - rt5677_dsp->substream = 0; + rt5677_dsp->substream = NULL; mutex_unlock(&rt5677_dsp->dma_lock); return snd_pcm_lib_free_vmalloc_buffer(substream); @@ -362,7 +362,7 @@ static void rt5677_spi_copy_work(struct work_struct *work) mutex_unlock(&rt5677_dsp->dma_lock); } -struct page *rt5677_spi_pcm_page( +static struct page *rt5677_spi_pcm_page( struct snd_soc_component *component, struct snd_pcm_substream *substream, unsigned long offset) -- 2.24.0.rc0.303.g954a862665-goog

2 1

[alsa-devel] [PATCH trivial] ASoC: pxa: poodle: Spelling s/enpoints/endpoints/, s/connetion/connection/
by Geert Uytterhoeven 25 Oct '19

25 Oct '19

Fix misspelling of "endpoints" and "connection". Signed-off-by: Geert Uytterhoeven <geert+renesas(a)glider.be> --- sound/soc/pxa/poodle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/pxa/poodle.c b/sound/soc/pxa/poodle.c index 48d5c2252b10081d..59ef04d0467a622d 100644 --- a/sound/soc/pxa/poodle.c +++ b/sound/soc/pxa/poodle.c @@ -56,7 +56,7 @@ static void poodle_ext_control(struct snd_soc_dapm_context *dapm) snd_soc_dapm_disable_pin(dapm, "Headphone Jack"); } - /* set the enpoints to their new connetion states */ + /* set the endpoints to their new connection states */ if (poodle_spk_func == POODLE_SPK_ON) snd_soc_dapm_enable_pin(dapm, "Ext Spk"); else -- 2.17.1

2 1

[alsa-devel] [PATCH trivial] ASoC: Spelling s/configr/configur/
by Geert Uytterhoeven 25 Oct '19

25 Oct '19

Fix misspellings of "configuration" and "configure". Signed-off-by: Geert Uytterhoeven <geert+renesas(a)glider.be> --- include/sound/wm8904.h | 2 +- sound/soc/codecs/cx2072x.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/sound/wm8904.h b/include/sound/wm8904.h index 14074405f5012f9a..88ac1870510ec063 100644 --- a/include/sound/wm8904.h +++ b/include/sound/wm8904.h @@ -120,7 +120,7 @@ * DRC configurations are specified with a label and a set of register * values to write (the enable bits will be ignored). At runtime an * enumerated control will be presented for each DRC block allowing - * the user to choose the configration to use. + * the user to choose the configuration to use. * * Configurations may be generated by hand or by using the DRC control * panel provided by the WISCE - see http://www.wolfsonmicro.com/wisce/ diff --git a/sound/soc/codecs/cx2072x.c b/sound/soc/codecs/cx2072x.c index 1c1ba7bea4d81969..2ad00ed21bec6c59 100644 --- a/sound/soc/codecs/cx2072x.c +++ b/sound/soc/codecs/cx2072x.c @@ -1507,7 +1507,7 @@ static int cx2072x_probe(struct snd_soc_component *codec) regmap_multi_reg_write(cx2072x->regmap, cx2072x_reg_init, ARRAY_SIZE(cx2072x_reg_init)); - /* configre PortC as input device */ + /* configure PortC as input device */ regmap_update_bits(cx2072x->regmap, CX2072X_PORTC_PIN_CTRL, 0x20, 0x20); -- 2.17.1

2 1

[alsa-devel] [PATCH v2 0/2] ASoC: SOF: make hw_info platform specific
by Pierre-Louis Bossart 25 Oct '19

25 Oct '19

The SOF core blindly sets the hw_info, when it should really be platform-specific. This move enable us to temporarily fix PulseAudio issues resulting from inaccuracies in the hw_ptr position provided by the firmware on Baytrail-Broadwell. This is the first time we experiment with the GitHub low-level API to automagically keep track of 'approvers' by inserting a Reviewed-by tag when merging a pull-request, thanks to Ranjani for her initial work and bearing with my habit of breaking things :-) v2: make the code apply on Mark's tree. Sorry about that bad patch. Pierre-Louis Bossart (2): ASoC: SOF: define INFO_ flags in dsp_ops ASoC: SOF: Intel: only support INFO_BATCH for legacy platforms sound/soc/sof/imx/imx8.c | 7 +++++++ sound/soc/sof/intel/apl.c | 7 +++++++ sound/soc/sof/intel/bdw.c | 9 ++++++++- sound/soc/sof/intel/byt.c | 21 +++++++++++++++++++++ sound/soc/sof/intel/cnl.c | 7 +++++++ sound/soc/sof/pcm.c | 8 +++----- sound/soc/sof/sof-priv.h | 3 +++ 7 files changed, 56 insertions(+), 6 deletions(-) -- 2.20.1

2 4

[alsa-devel] [PATCH] ASoC: Intel: eve: Enable mclk and ssp sclk early for rt5514
by Brent Lu 25 Oct '19

25 Oct '19

The first DMIC capture always fail (zero sequence data from PCM port) after using DSP hotwording function (i.e. Google assistant). The DMIC is attached to rt5514 which also require eaily mclk/sclk like rt5663. Therefore we add a dapm route to provide ssp mclk/sclk early to rt5514. The rt5514 is attached to SSP0 and the rt5663 is attached to SSP1 so we create another supply widget to provide SSP0 mclk/sclk to rt5514. Signed-off-by: Brent Lu <brent.lu(a)intel.com> --- .../soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 136 ++++++++++++++------- 1 file changed, 92 insertions(+), 44 deletions(-) diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c index dc09a85..1755efa 100644 --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c @@ -53,8 +53,10 @@ struct kbl_codec_private { struct snd_soc_jack kabylake_headset; struct list_head hdmi_pcm_list; struct snd_soc_jack kabylake_hdmi[2]; - struct clk *mclk; - struct clk *sclk; + struct clk *ssp0_mclk; + struct clk *ssp0_sclk; + struct clk *ssp1_mclk; + struct clk *ssp1_sclk; }; enum { @@ -77,13 +79,31 @@ static const struct snd_kcontrol_new kabylake_controls[] = { }; static int platform_clock_control(struct snd_soc_dapm_widget *w, - struct snd_kcontrol *k, int event) + struct snd_kcontrol *k, int event, int ssp_num) { struct snd_soc_dapm_context *dapm = w->dapm; struct snd_soc_card *card = dapm->card; struct kbl_codec_private *priv = snd_soc_card_get_drvdata(card); + struct clk *mclk, *sclk; + unsigned long sclk_rate; int ret = 0; + switch (ssp_num) { + case 0: + mclk = priv->ssp0_mclk; + sclk = priv->ssp0_sclk; + sclk_rate = 6144000; + break; + case 1: + mclk = priv->ssp1_mclk; + sclk = priv->ssp1_sclk; + sclk_rate = 3072000; + break; + default: + dev_err(card->dev, "Invalid ssp_num %d\n", ssp_num); + return -EINVAL; + } + /* * MCLK/SCLK need to be ON early for a successful synchronization of * codec internal clock. And the clocks are turned off during @@ -92,37 +112,39 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w, switch (event) { case SND_SOC_DAPM_PRE_PMU: /* Enable MCLK */ - ret = clk_set_rate(priv->mclk, 24000000); + ret = clk_set_rate(mclk, 24000000); if (ret < 0) { - dev_err(card->dev, "Can't set rate for mclk, err: %d\n", - ret); + dev_err(card->dev, "Can't set rate for ssp%d_mclk, err: %d\n", + ssp_num, ret); return ret; } - ret = clk_prepare_enable(priv->mclk); + ret = clk_prepare_enable(mclk); if (ret < 0) { - dev_err(card->dev, "Can't enable mclk, err: %d\n", ret); + dev_err(card->dev, "Can't enable ssp%d_mclk, err: %d\n", + ssp_num, ret); return ret; } /* Enable SCLK */ - ret = clk_set_rate(priv->sclk, 3072000); + ret = clk_set_rate(sclk, sclk_rate); if (ret < 0) { - dev_err(card->dev, "Can't set rate for sclk, err: %d\n", - ret); - clk_disable_unprepare(priv->mclk); + dev_err(card->dev, "Can't set rate for ssp%d_sclk, err: %d\n", + ssp_num, ret); + clk_disable_unprepare(mclk); return ret; } - ret = clk_prepare_enable(priv->sclk); + ret = clk_prepare_enable(sclk); if (ret < 0) { - dev_err(card->dev, "Can't enable sclk, err: %d\n", ret); - clk_disable_unprepare(priv->mclk); + dev_err(card->dev, "Can't enable ssp%d_sclk, err: %d\n", + ssp_num, ret); + clk_disable_unprepare(mclk); } break; case SND_SOC_DAPM_POST_PMD: - clk_disable_unprepare(priv->mclk); - clk_disable_unprepare(priv->sclk); + clk_disable_unprepare(mclk); + clk_disable_unprepare(sclk); break; default: return 0; @@ -131,6 +153,18 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w, return 0; } +static int platform_clock_control_ssp0(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *k, int event) +{ + return platform_clock_control(w, k, event, 0); +} + +static int platform_clock_control_ssp1(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *k, int event) +{ + return platform_clock_control(w, k, event, 1); +} + static const struct snd_soc_dapm_widget kabylake_widgets[] = { SND_SOC_DAPM_HP("Headphone Jack", NULL), SND_SOC_DAPM_MIC("Headset Mic", NULL), @@ -139,15 +173,17 @@ static const struct snd_soc_dapm_widget kabylake_widgets[] = { SND_SOC_DAPM_MIC("DMIC", NULL), SND_SOC_DAPM_SPK("HDMI1", NULL), SND_SOC_DAPM_SPK("HDMI2", NULL), - SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0, - platform_clock_control, SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_SUPPLY("Platform Clock SSP0", SND_SOC_NOPM, 0, 0, + platform_clock_control_ssp0, SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_SUPPLY("Platform Clock SSP1", SND_SOC_NOPM, 0, 0, + platform_clock_control_ssp1, SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), - }; static const struct snd_soc_dapm_route kabylake_map[] = { /* Headphones */ - { "Headphone Jack", NULL, "Platform Clock" }, + { "Headphone Jack", NULL, "Platform Clock SSP1" }, { "Headphone Jack", NULL, "HPOL" }, { "Headphone Jack", NULL, "HPOR" }, @@ -156,7 +192,7 @@ static const struct snd_soc_dapm_route kabylake_map[] = { { "Right Spk", NULL, "Right BE_OUT" }, /* other jacks */ - { "Headset Mic", NULL, "Platform Clock" }, + { "Headset Mic", NULL, "Platform Clock SSP1" }, { "IN1P", NULL, "Headset Mic" }, { "IN1N", NULL, "Headset Mic" }, @@ -180,6 +216,7 @@ static const struct snd_soc_dapm_route kabylake_map[] = { { "ssp0 Rx", NULL, "Right HiFi Capture" }, /* DMIC */ + { "DMIC", NULL, "Platform Clock SSP0" }, { "DMIC1L", NULL, "DMIC" }, { "DMIC1R", NULL, "DMIC" }, { "DMIC2L", NULL, "DMIC" }, @@ -704,6 +741,29 @@ static struct snd_soc_card kabylake_audio_card = { .late_probe = kabylake_card_late_probe, }; +static int kabylake_audio_clk_get(struct device *dev, const char *id, + struct clk **clk) +{ + int ret = 0; + + if (!clk) + return -EINVAL; + + *clk = devm_clk_get(dev, id); + if (IS_ERR(*clk)) { + ret = PTR_ERR(*clk); + if (ret == -ENOENT) { + dev_info(dev, "Failed to get %s, defer probe\n", id); + return -EPROBE_DEFER; + } + + dev_err(dev, "Failed to get %s with err:%d\n", id, ret); + return ret; + } + + return ret; +} + static int kabylake_audio_probe(struct platform_device *pdev) { struct kbl_codec_private *ctx; @@ -724,33 +784,21 @@ static int kabylake_audio_probe(struct platform_device *pdev) dmic_constraints = mach->mach_params.dmic_num == 2 ? &constraints_dmic_2ch : &constraints_dmic_channels; - ctx->mclk = devm_clk_get(&pdev->dev, "ssp1_mclk"); - if (IS_ERR(ctx->mclk)) { - ret = PTR_ERR(ctx->mclk); - if (ret == -ENOENT) { - dev_info(&pdev->dev, - "Failed to get ssp1_mclk, defer probe\n"); - return -EPROBE_DEFER; - } + ret = kabylake_audio_clk_get(&pdev->dev, "ssp0_mclk", &ctx->ssp0_mclk); + if (ret != 0) + return ret; - dev_err(&pdev->dev, "Failed to get ssp1_mclk with err:%d\n", - ret); + ret = kabylake_audio_clk_get(&pdev->dev, "ssp0_sclk", &ctx->ssp0_sclk); + if (ret != 0) return ret; - } - ctx->sclk = devm_clk_get(&pdev->dev, "ssp1_sclk"); - if (IS_ERR(ctx->sclk)) { - ret = PTR_ERR(ctx->sclk); - if (ret == -ENOENT) { - dev_info(&pdev->dev, - "Failed to get ssp1_sclk, defer probe\n"); - return -EPROBE_DEFER; - } + ret = kabylake_audio_clk_get(&pdev->dev, "ssp1_mclk", &ctx->ssp1_mclk); + if (ret != 0) + return ret; - dev_err(&pdev->dev, "Failed to get ssp1_sclk with err:%d\n", - ret); + ret = kabylake_audio_clk_get(&pdev->dev, "ssp1_sclk", &ctx->ssp1_sclk); + if (ret != 0) return ret; - } return devm_snd_soc_register_card(&pdev->dev, &kabylake_audio_card); } -- 2.7.4

3 2

Re: [alsa-devel] [PATCH] ASoC: fsl_asrc: refine the setting of internal clock divider
by S.j. Wang 25 Oct '19

25 Oct '19

Hi > > On Wed, Oct 23, 2019 at 06:25:20AM +0000, S.j. Wang wrote: > > > On Thu, Oct 17, 2019 at 02:21:08PM +0800, Shengjiu Wang wrote: > > > > For P2P output, the output divider should align with the output > > > > sample > > > > > > I think we should avoid "P2P" (or "M2M") keyword in the mainline > > > code as we know M2M will never get merged while somebody working > > > with the mainline and caring about new feature might be confused. > > > > Ok. But we still curious that is there a way to upstream m2m? > > Hmm..I would love to see that happening. Here is an old discussion that > you may want to take a look: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail > man.alsa-project.org%2Fpipermail%2Falsa-devel%2F2014- > May%2F076797.html&data=02%7C01%7Cshengjiu.wang%40nxp.com%7 > Ce902d2bac4254d2faa0f08d757ecac0e%7C686ea1d3bc2b4c6fa92cd99c5c301 > 635%7C0%7C0%7C637074546320396681&sdata=bg%2BLwRQnUPhW8f > mE972O%2F53MyVftJkK140PSnmC%2FDKQ%3D&reserved=0 > > > > It makes sense to me, yet I feel that the delay at the beginning of > > > the audio playback might be longer as a compromise. I am okay with > > > this decision though... > > > > > > > The maximum divider of asrc clock is 1024, but there is no > > > > judgement for this limitaion in driver, which may cause the > > > > divider setting not correct. > > > > > > > > For non-ideal ratio mode, the clock rate should divide the sample > > > > rate with no remainder, and the quotient should be less than 1024. > > > > > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang(a)nxp.com> > > > > > @@ -351,7 +352,9 @@ static int fsl_asrc_config_pair(struct > > > > fsl_asrc_pair > > > *pair) > > > > /* We only have output clock for ideal ratio mode */ > > > > clk = asrc_priv->asrck_clk[clk_index[ideal ? OUT : IN]]; > > > > > > > > - div[IN] = clk_get_rate(clk) / inrate; > > > > + clk_rate = clk_get_rate(clk); > > > > > > The fsl_asrc.c file has config.inclk being set to INCLK_NONE and > > > this sets the "ideal" in this function to true. So, although we tend > > > to not use ideal ratio setting for p2p cases, yet the input clock is > > > still not physically connected, so we still use output clock for div[IN] > calculation? > > > > For p2p case, it can be ideal or non-ideal. For non-ideal, we still > > use Output clock for div calculation. > > > > > > > > I am thinking something simplier: if we decided not to use ideal > > > ratio for "P2P", instead of adding "bool p2p" with the confusing > > > "ideal" in this function, could we just set config.inclk to the same > > > clock as the output one for "P2P"? By doing so, "P2P" won't go > > > through ideal ratio mode while still having a clock rate from the output > clock for div[IN] calculation here. > > > > Bool p2p is to force output rate to be sample rate, no impact to ideal > > Ratio mode. > > I just realized that the function has a bottom part for ideal mode > exclusively -- if we treat p2p as !ideal, those configurations will be missing. > So you're right, should have an extra boolean variable. > > > > > > > > + rem[IN] = do_div(clk_rate, inrate); > > > > + div[IN] = (u32)clk_rate; > > > > if (div[IN] == 0) { > > > > > > Could we check div[IN] and rem[IN] here? Like: > > > if (div[IN] == 0 || div[IN] > 1024) { > > > pair_err(); > > > goto out; > > > } > > > > > > if (!ideal && rem[IN]) { > > > pair_err(); > > > goto out; > > > } > > > > > > According to your commit log, I think the max-1024 limitation should > > > be applied to all cases, not confined to "!ideal" cases right? And > > > we should add some comments also, indicating it is limited by hardware. > > > > For ideal mode, my test result is the divider not impact the output result. > > Which means it is ok for ideal mode even divider is not correct... > > OK. > > > > > > > > pair_err("failed to support input sample rate %dHz > > > > by > > > asrck_%x\n", > > > > inrate, clk_index[ideal ? OUT : > > > > IN]); @@ > > > > -360,11 +363,20 @@ static int fsl_asrc_config_pair(struct > > > > fsl_asrc_pair *pair) > > > > > > > > clk = asrc_priv->asrck_clk[clk_index[OUT]]; > > > > > > > > - /* Use fixed output rate for Ideal Ratio mode (INCLK_NONE) */ > > > > - if (ideal) > > > > - div[OUT] = clk_get_rate(clk) / IDEAL_RATIO_RATE; > > > > - else > > > > - div[OUT] = clk_get_rate(clk) / outrate; > > > > + /* > > > > + * When P2P mode, output rate should align with the out > samplerate. > > > > + * if set too high output rate, there will be lots of Overload. > > > > + * When M2M mode, output rate should also need to align with > > > > + the out > > > > > > For this "should", do you actually mean "M2M could also"? Sorry, I'm > > > just trying to understand everyting here, not intentionally being picky at > words. > > > My understanding is that we still keep the ideal ratio setting > > > because "M2M" still uses it. > > > > We use IDEAL_RATIO_RATE as output rate for m2m mode, it likes a Tricky > > operation, in order to improve the performance. I think The correct > > operation is to use the real output rate, but the performance Is bad. > > So it is a compromise. > > I see. > > > > > > > > + * samplerate, but M2M must use less time to achieve good > > > performance. > > > > + */ > > > > + clk_rate = clk_get_rate(clk); > > > > + if (p2p || !ideal) { > > > > + rem[OUT] = do_div(clk_rate, outrate); > > > > + div[OUT] = clk_rate; > > > > + } else { > > > > + rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE); > > > > + div[OUT] = clk_rate; > > > > + } > > > > > > > > if (div[OUT] == 0) { > > > > pair_err("failed to support output sample rate %dHz > > > > by asrck_%x\n", @@ -372,6 +384,16 @@ static int > > > > fsl_asrc_config_pair(struct > > > fsl_asrc_pair *pair) > > > > return -EINVAL; > > > > } > > > > > > > > + if (!ideal && (div[IN] > 1024 || div[OUT] > 1024 || > > > > + rem[IN] != 0 || rem[OUT] != 0)) { > > > > + if (!ideal && (div[IN] > 1024 || div[OUT] > 1024 || rem[IN] > > > > + || rem[OUT] != 0)) { > > > > > > So for ideal == true, these limitaions are not applied any more? > > > Remember that the "ideal" is true for "p2p == true" cases here. > > > > No, not applied. for ideal, the div don't impact the output result > > Even it is not accurate. > > I see. > > > > > > > > + pair_err("The divider can't be used for non ideal mode\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + /* Divider range is [1, 1024] */ > > > > + div[IN] = min_t(u32, 1024, div[IN]); > > > > + div[OUT] = min_t(u32, 1024, div[OUT]); > > > > > > Hmm, this looks like we want to allow ideal ratio cases and p2p > > > cases to operate any way, even if the divider wasn't within the > > > range to get the in/out rates from the output clock? > > > > Yes. We still allow the p2p = true, ideal = false. Note that p2p is > > not Equal to ideal. > > Got it. > > Overall, I feel it's better to have a naming to state the purpose of using > ideal configurations without the IDEAL_RATIO_RATE setup. > bool use_ideal_rate; > And we can put into the asrc_config structure if there's no major problem. > Asrc_config may exposed to user, I don't think user need to care about The using of ideal rate or not. > So the condition check for the calculation would be: > + if (ideal && config->use_ideal_rate) > + rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE); > + else > + rem[OUT] = do_div(clk_rate, outrate); > + div[OUT] = clk_rate; > > And for that if (!ideal && div[IN]....rem[OUT]), I feel it would be clear to > have them separately, as the existing "div[IN] == 0" > and "div[OUT] == 0" checks, so that we can tell users which side of the > divider is out of range and what the sample rate and clock rate are. > Do you mean need to combine this judgement with "div[IN] == 0" Or "div[OUT] == 0"? Best regards Wang shengjiu

2 1

Re: [alsa-devel] [PATCH] ASoC: fsl_esai: Add spin lock to protect reset and stop
by S.j. Wang 25 Oct '19

25 Oct '19

Hi > > On Wed, Oct 23, 2019 at 03:29:49PM +0800, Shengjiu Wang wrote: > > xrun may happen at the end of stream, the > > trigger->fsl_esai_trigger_stop maybe called in the middle of > > fsl_esai_hw_reset, this may cause esai in wrong state after stop, and > > there may be endless xrun interrupt. > > What about fsl_esai_trigger_start? It touches ESAI_xFCR_xFEN bit that is > being checked in the beginning of fsl_esai_hw_reset. > > Could the scenario below be possible also? > > 1) ESAI TX starts > 2) Xrun happens to TX > 3) Starting fsl_esai_hw_reset (enabled[TX] = true; enabled[RX] = false) > 4) ESAI RX starts > 5) Finishing fsl_esai_hw_reset (enabled[RX] is still false) > > Good catch, this may possible. Will update in v2. Best regards Wang shengjiu

1 0