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

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

Acked-by: Nicolin Chen <nicoleots...@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]);

Reply via email to