* [PATCH V3 0/2] Support more sample rate in asrc @ 2019-04-17 9:06 S.j. Wang 2019-04-17 9:06 ` [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function S.j. Wang 2019-04-17 9:06 ` [PATCH V3 2/2] ASoC: fsl_asrc: Unify the supported input and output rate S.j. Wang 0 siblings, 2 replies; 11+ messages in thread From: S.j. Wang @ 2019-04-17 9:06 UTC (permalink / raw) To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, alsa-devel Cc: linuxppc-dev, linux-kernel Support more sample rate in asrc Shengjiu Wang (2): ASoC: fsl_asrc: replace the process_option table with function ASoC: fsl_asrc: Unify the supported input and output rate Changes in v3 - remove FSL_ASRC_RATES - refine fsl_asrc_sel_proc according to comments Changes in v2 - add more comments in code - add commit "Unify the supported input and output rate" sound/soc/fsl/fsl_asrc.c | 108 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 76 insertions(+), 32 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function 2019-04-17 9:06 [PATCH V3 0/2] Support more sample rate in asrc S.j. Wang @ 2019-04-17 9:06 ` S.j. Wang 2019-04-17 18:52 ` Nicolin Chen 2019-04-17 9:06 ` [PATCH V3 2/2] ASoC: fsl_asrc: Unify the supported input and output rate S.j. Wang 1 sibling, 1 reply; 11+ messages in thread From: S.j. Wang @ 2019-04-17 9:06 UTC (permalink / raw) To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, alsa-devel Cc: linuxppc-dev, linux-kernel When we want to support more sample rate, for example 12kHz/24kHz we need update the process_option table, if we want to support more sample rate next time, the table need to be updated again. which is not flexible. We got a function fsl_asrc_sel_proc to replace the table, which can give the pre-processing and post-processing options according to the sample rate. Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> --- sound/soc/fsl/fsl_asrc.c | 78 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 58 insertions(+), 20 deletions(-) diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index 0b937924d2e4..d34d539d01f2 100644 --- a/sound/soc/fsl/fsl_asrc.c +++ b/sound/soc/fsl/fsl_asrc.c @@ -26,24 +26,6 @@ #define pair_dbg(fmt, ...) \ dev_dbg(&asrc_priv->pdev->dev, "Pair %c: " fmt, 'A' + index, ##__VA_ARGS__) -/* Sample rates are aligned with that defined in pcm.h file */ -static const u8 process_option[][12][2] = { - /* 8kHz 11.025kHz 16kHz 22.05kHz 32kHz 44.1kHz 48kHz 64kHz 88.2kHz 96kHz 176kHz 192kHz */ - {{0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0},}, /* 5512Hz */ - {{0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0},}, /* 8kHz */ - {{0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0},}, /* 11025Hz */ - {{1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0},}, /* 16kHz */ - {{1, 2}, {1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0},}, /* 22050Hz */ - {{1, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0},}, /* 32kHz */ - {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0},}, /* 44.1kHz */ - {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0},}, /* 48kHz */ - {{2, 2}, {2, 2}, {2, 2}, {2, 1}, {1, 2}, {0, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0},}, /* 64kHz */ - {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1, 1}, {1, 1}, {1, 1}, {1, 1},}, /* 88.2kHz */ - {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1, 1}, {1, 1}, {1, 1}, {1, 1},}, /* 96kHz */ - {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2, 1}, {2, 1}, {2, 1}, {2, 1},}, /* 176kHz */ - {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2, 1}, {2, 1}, {2, 1}, {2, 1},}, /* 192kHz */ -}; - /* Corresponding to process_option */ static int supported_input_rate[] = { 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, @@ -80,6 +62,54 @@ static unsigned char *clk_map[2]; /** + * Select the pre-processing and post-processing options + * + * inrate: input sample rate + * outrate: output sample rate + * pre_proc: return value for pre-processing option + * post_proc: return value for post-processing option + */ +static int fsl_asrc_sel_proc(int inrate, int outrate, int *pre_proc, + int *post_proc) +{ + bool post_proc_cond2; + bool post_proc_cond0; + + /* Does not support cases: Tsout > 8.125 * Tsin */ + if (inrate * 8 > 65 * outrate) + return -EINVAL; + + /* Otherwise, select pre_proc between [0, 2] */ + if (inrate * 8 > 33 * outrate) + *pre_proc = 2; + else if (inrate * 8 > 15 * outrate) { + if (inrate > 152000) + *pre_proc = 2; + else + *pre_proc = 1; + } else if (inrate < 76000) + *pre_proc = 0; + else if (inrate > 152000) + *pre_proc = 2; + else + *pre_proc = 1; + + /* Condition for selection of post-processing */ + post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) || + (inrate > 56000 && outrate < 56000); + post_proc_cond0 = inrate * 23 < outrate * 8; + + if (post_proc_cond2) + *post_proc = 2; + else if (post_proc_cond0) + *post_proc = 0; + else + *post_proc = 1; + + return 0; +} + +/** * Request ASRC pair * * It assigns pair by the order of A->C->B because allocation of pair B, @@ -239,8 +269,10 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair) u32 inrate, outrate, indiv, outdiv; u32 clk_index[2], div[2]; int in, out, channels; + int pre_proc, post_proc; struct clk *clk; bool ideal; + int ret; if (!config) { pair_err("invalid pair config\n"); @@ -289,6 +321,12 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair) return -EINVAL; } + ret = fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc); + if (ret) { + pair_err("No supported pre-processing options\n"); + return ret; + } + /* Validate input and output clock sources */ clk_index[IN] = clk_map[IN][config->inclk]; clk_index[OUT] = clk_map[OUT][config->outclk]; @@ -380,8 +418,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair) /* Apply configurations for pre- and post-processing */ regmap_update_bits(asrc_priv->regmap, REG_ASRCFG, ASRCFG_PREMODi_MASK(index) | ASRCFG_POSTMODi_MASK(index), - ASRCFG_PREMOD(index, process_option[in][out][0]) | - ASRCFG_POSTMOD(index, process_option[in][out][1])); + ASRCFG_PREMOD(index, pre_proc) | + ASRCFG_POSTMOD(index, post_proc)); return fsl_asrc_set_ideal_ratio(pair, inrate, outrate); } -- 1.9.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function 2019-04-17 9:06 ` [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function S.j. Wang @ 2019-04-17 18:52 ` Nicolin Chen 0 siblings, 0 replies; 11+ messages in thread From: Nicolin Chen @ 2019-04-17 18:52 UTC (permalink / raw) To: S.j. Wang Cc: alsa-devel, timur, Xiubo.Lee, festevam, linux-kernel, broonie, linuxppc-dev Hi Shengjiu, This looks better. Just a couple of more small comments inline. On Wed, Apr 17, 2019 at 09:06:18AM +0000, S.j. Wang wrote: > +static int fsl_asrc_sel_proc(int inrate, int outrate, int *pre_proc, > + int *post_proc) Just a nit: it looks better by grouping them two-two. static int fsl_asrc_sel_proc(int inrate, int outrate, int *pre_proc, int *post_proc) > + /* Condition for selection of post-processing */ > + post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) || > + (inrate > 56000 && outrate < 56000); Could align the indentation: post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) || (inrate > 56000 && outrate < 56000); Here: > + /* Does not support cases: Tsout > 8.125 * Tsin */ > + if (inrate * 8 > 65 * outrate) > + return -EINVAL; And here: > + ret = fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc); > + if (ret) { > + pair_err("No supported pre-processing options\n"); > + return ret; > + } Instead of a general message, I was thinking of a more specific one by telling users that the ratio between the two rates isn't supported -- something similar to what I suggested previously: pair_err("Does not support %d (input) > 8.125 * %d (output)\n", outrate, inrate); Thanks Nicolin ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH V3 2/2] ASoC: fsl_asrc: Unify the supported input and output rate 2019-04-17 9:06 [PATCH V3 0/2] Support more sample rate in asrc S.j. Wang 2019-04-17 9:06 ` [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function S.j. Wang @ 2019-04-17 9:06 ` S.j. Wang 1 sibling, 0 replies; 11+ messages in thread From: S.j. Wang @ 2019-04-17 9:06 UTC (permalink / raw) To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, alsa-devel Cc: linuxppc-dev, linux-kernel Unify the supported input and output rate, add the 12kHz/24kHz/128kHz to the support list Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> --- sound/soc/fsl/fsl_asrc.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index d34d539d01f2..b0d19b787bb8 100644 --- a/sound/soc/fsl/fsl_asrc.c +++ b/sound/soc/fsl/fsl_asrc.c @@ -27,13 +27,14 @@ dev_dbg(&asrc_priv->pdev->dev, "Pair %c: " fmt, 'A' + index, ##__VA_ARGS__) /* Corresponding to process_option */ -static int supported_input_rate[] = { - 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, - 96000, 176400, 192000, +static unsigned int supported_asrc_rate[] = { + 5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000, + 64000, 88200, 96000, 128000, 176400, 192000, }; -static int supported_asrc_rate[] = { - 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000, 176400, 192000, +static struct snd_pcm_hw_constraint_list fsl_asrc_rate_constraints = { + .count = ARRAY_SIZE(supported_asrc_rate), + .list = supported_asrc_rate, }; /** @@ -296,11 +297,11 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair) ideal = config->inclk == INCLK_NONE; /* Validate input and output sample rates */ - for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++) - if (inrate == supported_input_rate[in]) + for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++) + if (inrate == supported_asrc_rate[in]) break; - if (in == ARRAY_SIZE(supported_input_rate)) { + if (in == ARRAY_SIZE(supported_asrc_rate)) { pair_err("unsupported input sample rate: %dHz\n", inrate); return -EINVAL; } @@ -493,7 +494,9 @@ static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream, snd_pcm_hw_constraint_step(substream->runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, 2); - return 0; + + return snd_pcm_hw_constraint_list(substream->runtime, 0, + SNDRV_PCM_HW_PARAM_RATE, &fsl_asrc_rate_constraints); } static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream, @@ -606,7 +609,6 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai) return 0; } -#define FSL_ASRC_RATES SNDRV_PCM_RATE_8000_192000 #define FSL_ASRC_FORMATS (SNDRV_PCM_FMTBIT_S24_LE | \ SNDRV_PCM_FMTBIT_S16_LE | \ SNDRV_PCM_FMTBIT_S20_3LE) @@ -617,14 +619,18 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai) .stream_name = "ASRC-Playback", .channels_min = 1, .channels_max = 10, - .rates = FSL_ASRC_RATES, + .rate_min = 5512, + .rate_max = 192000, + .rates = SNDRV_PCM_RATE_KNOT, .formats = FSL_ASRC_FORMATS, }, .capture = { .stream_name = "ASRC-Capture", .channels_min = 1, .channels_max = 10, - .rates = FSL_ASRC_RATES, + .rate_min = 5512, + .rate_max = 192000, + .rates = SNDRV_PCM_RATE_KNOT, .formats = FSL_ASRC_FORMATS, }, .ops = &fsl_asrc_dai_ops, -- 1.9.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function @ 2019-04-18 2:37 S.j. Wang 2019-04-18 8:03 ` Nicolin Chen 0 siblings, 1 reply; 11+ messages in thread From: S.j. Wang @ 2019-04-18 2:37 UTC (permalink / raw) To: Nicolin Chen Cc: alsa-devel, timur, Xiubo.Lee, festevam, linux-kernel, broonie, linuxppc-dev Hi > > Hi Shengjiu, > > This looks better. Just a couple of more small comments inline. > > On Wed, Apr 17, 2019 at 09:06:18AM +0000, S.j. Wang wrote: > > > +static int fsl_asrc_sel_proc(int inrate, int outrate, int *pre_proc, > > + int *post_proc) > > Just a nit: it looks better by grouping them two-two. > > static int fsl_asrc_sel_proc(int inrate, int outrate, > int *pre_proc, int *post_proc) > > > + /* Condition for selection of post-processing */ > > + post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) || > > + (inrate > 56000 && outrate < 56000); > > Could align the indentation: > post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) || > (inrate > 56000 && outrate < 56000); > > Here: > > + /* Does not support cases: Tsout > 8.125 * Tsin */ > > + if (inrate * 8 > 65 * outrate) > > + return -EINVAL; > And here: > > + ret = fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc); > > + if (ret) { > > + pair_err("No supported pre-processing options\n"); > > + return ret; > > + } > > Instead of a general message, I was thinking of a more specific one by > telling users that the ratio between the two rates isn't supported -- > something similar to what I suggested previously: > > pair_err("Does not support %d (input) > 8.125 * %d (output)\n", > outrate, inrate); > In fsl_asrc_sel_proc, we can't call the pair_err for there is no struct fsl_asrc_pair *pair in the argument. Do you think we need to add this argument? > Thanks > Nicolin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function 2019-04-18 2:37 [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function S.j. Wang @ 2019-04-18 8:03 ` Nicolin Chen 0 siblings, 0 replies; 11+ messages in thread From: Nicolin Chen @ 2019-04-18 8:03 UTC (permalink / raw) To: S.j. Wang Cc: alsa-devel, timur, Xiubo.Lee, festevam, linux-kernel, broonie, linuxppc-dev On Thu, Apr 18, 2019 at 02:37:03AM +0000, S.j. Wang wrote: > > Here: > > > + /* Does not support cases: Tsout > 8.125 * Tsin */ > > > + if (inrate * 8 > 65 * outrate) Though it might not matter any more (see my last comments), it should be "inrate > 8.125 * outrate" in the comments. > > > + return -EINVAL; > > And here: > > > + ret = fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc); > > > + if (ret) { > > > + pair_err("No supported pre-processing options\n"); > > > + return ret; > > > + } > > > > Instead of a general message, I was thinking of a more specific one by > > telling users that the ratio between the two rates isn't supported -- > > something similar to what I suggested previously: > > > > pair_err("Does not support %d (input) > 8.125 * %d (output)\n", > > outrate, inrate); > > > In fsl_asrc_sel_proc, we can't call the pair_err for there is no > struct fsl_asrc_pair *pair in the argument. Do you think we need to > add this argument? I's thinking of adding it to the top of fsl_asrc_config_pair() as a part of inrate-outrate-validation, however, I found that actually we already have a similar check in the early routine: if ((outrate > 8000 && outrate < 30000) && (outrate/inrate > 24 || inrate/outrate > 8)) { pair_err("exceed supported ratio range [1/24, 8] for \ inrate/outrate: %d/%d\n", inrate, outrate); return -EINVAL; } And this is according to IMX6DQRM: Limited support for the case when output sampling rates is between 8kHz and 30kHz. The limitation is the supported ratio (Fsin/Fsout) range as between 1/24 to 8 This should cover your 8.125 condition already, even if having an outrate range between [8KHz, 30KHz] check, since an outrate above 30KHz will not have an inrate bigger than 8.125 times of it, given the maximum input rate is 192KHz. So I think that we can just drop that 8.125 condition from your change and there's no need to error out any more. However, we do need a patch to fix a potential rounding issue: - (outrate/inrate > 24 || inrate/outrate > 8)) { + (outrate > 24 * inrate || inrate > 8 * outrate)) { Should fix the missing whitespace also. And it will be needed to send to stable kernel too. Will you help submit a change? Thanks ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function @ 2019-04-18 8:50 S.j. Wang 2019-04-18 9:05 ` Nicolin Chen 0 siblings, 1 reply; 11+ messages in thread From: S.j. Wang @ 2019-04-18 8:50 UTC (permalink / raw) To: Nicolin Chen Cc: alsa-devel, timur, Xiubo.Lee, festevam, linux-kernel, broonie, linuxppc-dev Hi > > > On Thu, Apr 18, 2019 at 02:37:03AM +0000, S.j. Wang wrote: > > > Here: > > > > + /* Does not support cases: Tsout > 8.125 * Tsin */ > > > > + if (inrate * 8 > 65 * outrate) > > Though it might not matter any more (see my last comments), it should be > "inrate > 8.125 * outrate" in the comments. > > > > > + return -EINVAL; > > > And here: > > > > + ret = fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc); > > > > + if (ret) { > > > > + pair_err("No supported pre-processing options\n"); > > > > + return ret; > > > > + } > > > > > > Instead of a general message, I was thinking of a more specific one > > > by telling users that the ratio between the two rates isn't > > > supported -- something similar to what I suggested previously: > > > > > > pair_err("Does not support %d (input) > 8.125 * %d (output)\n", > > > outrate, inrate); > > > > > > In fsl_asrc_sel_proc, we can't call the pair_err for there is no > > struct fsl_asrc_pair *pair in the argument. Do you think we need to > > add this argument? > > I's thinking of adding it to the top of fsl_asrc_config_pair() as a part of > inrate-outrate-validation, however, I found that actually we already have a > similar check in the early routine: > if ((outrate > 8000 && outrate < 30000) && > (outrate/inrate > 24 || inrate/outrate > 8)) { > pair_err("exceed supported ratio range [1/24, 8] for \ > inrate/outrate: %d/%d\n", inrate, outrate); > return -EINVAL; > } > > And this is according to IMX6DQRM: > Limited support for the case when output sampling rates is > between 8kHz and 30kHz. The limitation is the supported ratio > (Fsin/Fsout) range as between 1/24 to 8 > > This should cover your 8.125 condition already, even if having an outrate > range between [8KHz, 30KHz] check, since an outrate above 30KHz will not > have an inrate bigger than 8.125 times of it, given the maximum input rate > is 192KHz. > > So I think that we can just drop that 8.125 condition from your change and > there's no need to error out any more. > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported. This is not covered by if ((outrate > 8000 && outrate < 30000) && (outrate/inrate > 24 || inrate/outrate > 8)) { > However, we do need a patch to fix a potential rounding issue: > - (outrate/inrate > 24 || inrate/outrate > 8)) { > + (outrate > 24 * inrate || inrate > 8 * outrate)) { > > Should fix the missing whitespace also. And it will be needed to send to > stable kernel too. Will you help submit a change? > > Thanks ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function 2019-04-18 8:50 S.j. Wang @ 2019-04-18 9:05 ` Nicolin Chen 0 siblings, 0 replies; 11+ messages in thread From: Nicolin Chen @ 2019-04-18 9:05 UTC (permalink / raw) To: S.j. Wang Cc: alsa-devel, timur, Xiubo.Lee, festevam, linux-kernel, broonie, linuxppc-dev On Thu, Apr 18, 2019 at 08:50:48AM +0000, S.j. Wang wrote: > > And this is according to IMX6DQRM: > > Limited support for the case when output sampling rates is > > between 8kHz and 30kHz. The limitation is the supported ratio > > (Fsin/Fsout) range as between 1/24 to 8 > > > > This should cover your 8.125 condition already, even if having an outrate > > range between [8KHz, 30KHz] check, since an outrate above 30KHz will not > > have an inrate bigger than 8.125 times of it, given the maximum input rate > > is 192KHz. > > > > So I think that we can just drop that 8.125 condition from your change and > > there's no need to error out any more. > > > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported. > This is not covered by > > if ((outrate > 8000 && outrate < 30000) && > (outrate/inrate > 24 || inrate/outrate > 8)) { Good catch. The range should be [8KHz, 30KHz] vs. (8KHz, 32KHz) in the code. Then I think the fix should be at both lines: - if ((outrate > 8000 && outrate < 30000) && - (outrate/inrate > 24 || inrate/outrate > 8)) { + if ((outrate >= 8000 && outrate =< 30000) && + (outrate > 24 * inrate || inrate > 8 * outrate)) { Overall, I think we should fix this instead of adding an extra one, since it is very likely saying the same thing. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
@ 2019-04-18 9:37 S.j. Wang
2019-04-18 19:17 ` Nicolin Chen
0 siblings, 1 reply; 11+ messages in thread
From: S.j. Wang @ 2019-04-18 9:37 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel, timur, Xiubo.Lee, festevam, linux-kernel, broonie,
linuxppc-dev
Hi
>
> On Thu, Apr 18, 2019 at 08:50:48AM +0000, S.j. Wang wrote:
> > > And this is according to IMX6DQRM:
> > > Limited support for the case when output sampling rates is
> > > between 8kHz and 30kHz. The limitation is the supported ratio
> > > (Fsin/Fsout) range as between 1/24 to 8
> > >
> > > This should cover your 8.125 condition already, even if having an
> > > outrate range between [8KHz, 30KHz] check, since an outrate above
> > > 30KHz will not have an inrate bigger than 8.125 times of it, given
> > > the maximum input rate is 192KHz.
> > >
> > > So I think that we can just drop that 8.125 condition from your
> > > change and there's no need to error out any more.
> > >
> > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported.
> > This is not covered by
> >
> > if ((outrate > 8000 && outrate < 30000) &&
> > (outrate/inrate > 24 || inrate/outrate > 8)) {
>
> Good catch. The range should be [8KHz, 30KHz] vs. (8KHz, 32KHz) in the
> code. Then I think the fix should be at both lines:
>
> - if ((outrate > 8000 && outrate < 30000) &&
> - (outrate/inrate > 24 || inrate/outrate > 8)) {
> + if ((outrate >= 8000 && outrate =< 30000) &&
> + (outrate > 24 * inrate || inrate > 8 * outrate)) {
>
> Overall, I think we should fix this instead of adding an extra one, since it is
> very likely saying the same thing.
Actually if outrate < 8kHz, there will be issue too.
Best regards
Wang shengjiu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function 2019-04-18 9:37 S.j. Wang @ 2019-04-18 19:17 ` Nicolin Chen 0 siblings, 0 replies; 11+ messages in thread From: Nicolin Chen @ 2019-04-18 19:17 UTC (permalink / raw) To: S.j. Wang Cc: alsa-devel, timur, Xiubo.Lee, festevam, linux-kernel, broonie, linuxppc-dev On Thu, Apr 18, 2019 at 09:37:06AM +0000, S.j. Wang wrote: > > > > And this is according to IMX6DQRM: > > > > Limited support for the case when output sampling rates is > > > > between 8kHz and 30kHz. The limitation is the supported ratio > > > > (Fsin/Fsout) range as between 1/24 to 8 > > > > > > > > This should cover your 8.125 condition already, even if having an > > > > outrate range between [8KHz, 30KHz] check, since an outrate above > > > > 30KHz will not have an inrate bigger than 8.125 times of it, given > > > > the maximum input rate is 192KHz. > > > > > > > > So I think that we can just drop that 8.125 condition from your > > > > change and there's no need to error out any more. > > > > > > > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported. > > > This is not covered by > > > > > > if ((outrate > 8000 && outrate < 30000) && > > > (outrate/inrate > 24 || inrate/outrate > 8)) { > > > > Good catch. The range should be [8KHz, 30KHz] vs. (8KHz, 32KHz) in the > > code. Then I think the fix should be at both lines: > > > > - if ((outrate > 8000 && outrate < 30000) && > > - (outrate/inrate > 24 || inrate/outrate > 8)) { > > + if ((outrate >= 8000 && outrate =< 30000) && > > + (outrate > 24 * inrate || inrate > 8 * outrate)) { > > > > Overall, I think we should fix this instead of adding an extra one, since it is > > very likely saying the same thing. > > Actually if outrate < 8kHz, there will be issue too. Here is the thing, the RM doesn't explicitly state that ASRC can support a lower output sample rate than 8KHz. And I actually had a concern when reviewing your PATCH-2, as the table of supported output sample rate no longer matches RM. If you've verified a lower output sample rate working solid with the process_option function, that means our driver can go beyond the limitation mentioned in the RM, then I believe [8KHz, 32KHz] should be updated too -- that says we can do: - if ((outrate > 8000 && outrate < 30000) && - (outrate/inrate > 24 || inrate/outrate > 8)) { + if ((outrate >= 5512 && outrate =< 30000) && + (outrate > 24 * inrate || inrate > 8 * outrate)) { Actually "ourate > 24 * inrate" is kind of pointless for range [5KHz, 32KHz] but we can keep it since it matches RM. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
@ 2019-04-19 10:21 S.j. Wang
0 siblings, 0 replies; 11+ messages in thread
From: S.j. Wang @ 2019-04-19 10:21 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel, timur, Xiubo.Lee, festevam, linux-kernel, broonie,
linuxppc-dev
Hi
>
>
> On Thu, Apr 18, 2019 at 09:37:06AM +0000, S.j. Wang wrote:
> > > > > And this is according to IMX6DQRM:
> > > > > Limited support for the case when output sampling rates is
> > > > > between 8kHz and 30kHz. The limitation is the supported ratio
> > > > > (Fsin/Fsout) range as between 1/24 to 8
> > > > >
> > > > > This should cover your 8.125 condition already, even if having
> > > > > an outrate range between [8KHz, 30KHz] check, since an outrate
> > > > > above 30KHz will not have an inrate bigger than 8.125 times of
> > > > > it, given the maximum input rate is 192KHz.
> > > > >
> > > > > So I think that we can just drop that 8.125 condition from your
> > > > > change and there's no need to error out any more.
> > > > >
> > > > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported.
> > > > This is not covered by
> > > >
> > > > if ((outrate > 8000 && outrate < 30000) &&
> > > > (outrate/inrate > 24 || inrate/outrate > 8)) {
> > >
> > > Good catch. The range should be [8KHz, 30KHz] vs. (8KHz, 32KHz) in
> > > the code. Then I think the fix should be at both lines:
> > >
> > > - if ((outrate > 8000 && outrate < 30000) &&
> > > - (outrate/inrate > 24 || inrate/outrate > 8)) {
> > > + if ((outrate >= 8000 && outrate =< 30000) &&
> > > + (outrate > 24 * inrate || inrate > 8 * outrate)) {
> > >
> > > Overall, I think we should fix this instead of adding an extra one,
> > > since it is very likely saying the same thing.
> >
> > Actually if outrate < 8kHz, there will be issue too.
>
> Here is the thing, the RM doesn't explicitly state that ASRC can support a
> lower output sample rate than 8KHz. And I actually had a concern when
> reviewing your PATCH-2, as the table of supported output sample rate no
> longer matches RM.
>
> If you've verified a lower output sample rate working solid with the
> process_option function, that means our driver can go beyond the
> limitation mentioned in the RM, then I believe [8KHz, 32KHz] should be
> updated too -- that says we can do:
> - if ((outrate > 8000 && outrate < 30000) &&
> - (outrate/inrate > 24 || inrate/outrate > 8)) {
> + if ((outrate >= 5512 && outrate =< 30000) &&
> + (outrate > 24 * inrate || inrate > 8 * outrate)) {
>
> Actually "ourate > 24 * inrate" is kind of pointless for range [5KHz, 32KHz]
> but we can keep it since it matches RM.
Ok, will send v4.
Best regards
Wang shengjiu
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-04-19 10:23 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-17 9:06 [PATCH V3 0/2] Support more sample rate in asrc S.j. Wang 2019-04-17 9:06 ` [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function S.j. Wang 2019-04-17 18:52 ` Nicolin Chen 2019-04-17 9:06 ` [PATCH V3 2/2] ASoC: fsl_asrc: Unify the supported input and output rate S.j. Wang 2019-04-18 2:37 [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function S.j. Wang 2019-04-18 8:03 ` Nicolin Chen 2019-04-18 8:50 S.j. Wang 2019-04-18 9:05 ` Nicolin Chen 2019-04-18 9:37 S.j. Wang 2019-04-18 19:17 ` Nicolin Chen 2019-04-19 10:21 S.j. Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).