On Fri, Oct 25, 2019 at 03:13:22PM +0800, Shengjiu Wang wrote:

The output divider should align with the output sample rate, if use ideal sample rate, there will be a lot of overload, which would cause underrun.

The maximum divider of asrc clock is 1024, but there is no judgement for this limitaion in driver, which may cause the divider

typo: "limitaion" => "limitation"

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@nxp.com

And some comments inline. Please add my ack once they are fixed:

Acked-by: Nicolin Chen nicoleotsuka@gmail.com

Thanks

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index 0bf91a6f54b9..89cf333154c7 100644 --- a/sound/soc/fsl/fsl_asrc.c +++ b/sound/soc/fsl/fsl_asrc.c @@ -259,8 +259,11 @@ static int fsl_asrc_set_ideal_ratio(struct fsl_asrc_pair *pair,

• It configures those ASRC registers according to a configuration instance
• of struct asrc_config which includes in/output sample rate, width, channel
• and clock settings.

• • Note:
• • use_ideal_rate = true is need by some case which need higher performance.

I feel we can have a detailed one here and drop those inline comments, e.g.:

+ * Note: + * The ideal ratio configuration can work with a flexible clock rate setting. + * Using IDEAL_RATIO_RATE gives a faster converting speed but overloads ASRC. + * For a regular audio playback, the clock rate should not be slower than an + * clock rate aligning with the output sample rate; For a use case requiring + * faster conversion, set use_ideal_rate to have the faster speed.

@@ -351,8 +355,10 @@ 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;
• if (div[IN] == 0) {

• clk_rate = clk_get_rate(clk);
• rem[IN] = do_div(clk_rate, inrate);
• div[IN] = (u32)clk_rate;

• if (div[IN] == 0 || (!ideal && (div[IN] > 1024 || rem[IN] != 0))) {

Should have some comments to explain this like: /* * The divider range is [1, 1024], defined by the hardware. For non- * ideal ratio configuration, clock rate has to be strictly aligned * with the sample rate. For ideal ratio configuration, clock rates * only result in different converting speeds. So remainder does not * matter, as long as we keep the divider within its valid range. */

pair_err("failed to support input sample rate %dHz by asrck_%x\n",
		inrate, clk_index[ideal ? OUT : IN]);
return -EINVAL;

And move the min() behind this if-condition with no more comments: + div[IN] = min_t(u32, 1024, div[IN]);

@@ -360,18 +366,29 @@ 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;
  

• /*

• * Output rate should be align with the out samplerate. If set too
  

• * high output rate, there will be lots of Overload.
  

• * But some case need higher performance, then we can use
  

• * IDEAL_RATIO_RATE specifically for such case.
  

• */
  

Can drop this since we have the detailed comments at the top.

• clk_rate = clk_get_rate(clk);
• if (ideal && use_ideal_rate)

• rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
  

  else

• div[OUT] = clk_get_rate(clk) / outrate;
  

• rem[OUT] = do_div(clk_rate, outrate);
  

• div[OUT] = clk_rate;

• if (div[OUT] == 0) {

And add before this if-condition:

/* Output divider has the same limitation as the input one */

• if (div[OUT] == 0 || (!ideal && (div[OUT] > 1024 || rem[OUT] != 0))) { pair_err("failed to support output sample rate %dHz by asrck_%x\n", outrate, clk_index[OUT]); return -EINVAL; }

• /* Divider range is [1, 1024] */

Can drop this too.

• div[IN] = min_t(u32, 1024, div[IN]);
• div[OUT] = min_t(u32, 1024, div[OUT]);