Re: [PATCH] ASoC: fsl_asrc: Add an option to select internal ratio mode
On Tue, Jun 30, 2020 at 8:38 PM Fabio Estevam wrote: > > Hi Shengjiu, > > On Mon, Jun 29, 2020 at 11:10 AM Shengjiu Wang wrote: > > > +/** > > "/**" notation may confuse 'make htmldocs". Since this is a single > line comment you could do: > > /* Select proper clock source for internal ratio mode */ > > > > + * Select proper clock source for internal ratio mode > > + */ > > +static int fsl_asrc_select_clk(struct fsl_asrc_priv *asrc_priv, > > + struct fsl_asrc_pair *pair, > > + int in_rate, > > + int out_rate) > > +{ > > + struct fsl_asrc_pair_priv *pair_priv = pair->private; > > + struct asrc_config *config = pair_priv->config; > > + int rate[2], select_clk[2]; /* Array size 2 means IN and OUT */ > > + int clk_rate, clk_index; > > + int i = 0, j = 0; > > + bool clk_sel[2]; > > + > > + rate[0] = in_rate; > > + rate[1] = out_rate; > > + > > + /* Select proper clock source for internal ratio mode */ > > + for (j = 0; j < 2; j++) { > > + for (i = 0; i < ASRC_CLK_MAP_LEN; i++) { > > + clk_index = asrc_priv->clk_map[j][i]; > > + clk_rate = > > clk_get_rate(asrc_priv->asrck_clk[clk_index]); > > + if (clk_rate != 0 && (clk_rate / rate[j]) <= 1024 && > > + (clk_rate % rate[j]) == 0) > > + break; > > + } > > + > > + if (i == ASRC_CLK_MAP_LEN) { > > + select_clk[j] = OUTCLK_ASRCK1_CLK; > > + clk_sel[j] = false; > > + } else { > > + select_clk[j] = i; > > + clk_sel[j] = true; > > + } > > + } > > + > > + /* Switch to ideal ratio mode if there is no proper clock source */ > > + if (!clk_sel[IN] || !clk_sel[OUT]) > > + select_clk[IN] = INCLK_NONE; > > + > > + config->inclk = select_clk[IN]; > > + config->outclk = select_clk[OUT]; > > + > > + return 0; > > This new function always returns 0. Should it be converted to 'void' > type instead? > > > + ret = fsl_asrc_select_clk(asrc_priv, pair, > > + config.input_sample_rate, > > + config.output_sample_rate); > > + if (ret) { > > + dev_err(dai->dev, "fail to select clock\n"); > > fsl_asrc_select_clk() does not return error, so you could skip the > error checking. ok, will update the patch best regards wang shengjiu
Re: [PATCH] ASoC: fsl_asrc: Add an option to select internal ratio mode
Hi Shengjiu, On Mon, Jun 29, 2020 at 11:10 AM Shengjiu Wang wrote: > +/** "/**" notation may confuse 'make htmldocs". Since this is a single line comment you could do: /* Select proper clock source for internal ratio mode */ > + * Select proper clock source for internal ratio mode > + */ > +static int fsl_asrc_select_clk(struct fsl_asrc_priv *asrc_priv, > + struct fsl_asrc_pair *pair, > + int in_rate, > + int out_rate) > +{ > + struct fsl_asrc_pair_priv *pair_priv = pair->private; > + struct asrc_config *config = pair_priv->config; > + int rate[2], select_clk[2]; /* Array size 2 means IN and OUT */ > + int clk_rate, clk_index; > + int i = 0, j = 0; > + bool clk_sel[2]; > + > + rate[0] = in_rate; > + rate[1] = out_rate; > + > + /* Select proper clock source for internal ratio mode */ > + for (j = 0; j < 2; j++) { > + for (i = 0; i < ASRC_CLK_MAP_LEN; i++) { > + clk_index = asrc_priv->clk_map[j][i]; > + clk_rate = > clk_get_rate(asrc_priv->asrck_clk[clk_index]); > + if (clk_rate != 0 && (clk_rate / rate[j]) <= 1024 && > + (clk_rate % rate[j]) == 0) > + break; > + } > + > + if (i == ASRC_CLK_MAP_LEN) { > + select_clk[j] = OUTCLK_ASRCK1_CLK; > + clk_sel[j] = false; > + } else { > + select_clk[j] = i; > + clk_sel[j] = true; > + } > + } > + > + /* Switch to ideal ratio mode if there is no proper clock source */ > + if (!clk_sel[IN] || !clk_sel[OUT]) > + select_clk[IN] = INCLK_NONE; > + > + config->inclk = select_clk[IN]; > + config->outclk = select_clk[OUT]; > + > + return 0; This new function always returns 0. Should it be converted to 'void' type instead? > + ret = fsl_asrc_select_clk(asrc_priv, pair, > + config.input_sample_rate, > + config.output_sample_rate); > + if (ret) { > + dev_err(dai->dev, "fail to select clock\n"); fsl_asrc_select_clk() does not return error, so you could skip the error checking.
Re: [PATCH] ASoC: fsl_asrc: Add an option to select internal ratio mode
On Tue, Jun 30, 2020 at 4:09 AM Nicolin Chen wrote: > > On Mon, Jun 29, 2020 at 09:58:35PM +0800, Shengjiu Wang wrote: > > The ASRC not only supports ideal ratio mode, but also supports > > internal ratio mode. > > > > For internal rato mode, the rate of clock source should be divided > > with no remainder by sample rate, otherwise there is sound > > distortion. > > > > Add function fsl_asrc_select_clk() to find proper clock source for > > internal ratio mode, if the clock source is available then internal > > ratio mode will be selected. > > > > With change, the ideal ratio mode is not the only option for user. > > > > Signed-off-by: Shengjiu Wang > > --- > > > +static int fsl_asrc_select_clk(struct fsl_asrc_priv *asrc_priv, > > +struct fsl_asrc_pair *pair, > > +int in_rate, > > +int out_rate) > > +{ > > + struct fsl_asrc_pair_priv *pair_priv = pair->private; > > + struct asrc_config *config = pair_priv->config; > > + int rate[2], select_clk[2]; /* Array size 2 means IN and OUT */ > > + int clk_rate, clk_index; > > + int i = 0, j = 0; > > + bool clk_sel[2]; > > + > > + rate[0] = in_rate; > > + rate[1] = out_rate; > > + > > + /* Select proper clock source for internal ratio mode */ > > + for (j = 0; j < 2; j++) { > > + for (i = 0; i < ASRC_CLK_MAP_LEN; i++) { > > + clk_index = asrc_priv->clk_map[j][i]; > > + clk_rate = > > clk_get_rate(asrc_priv->asrck_clk[clk_index]); > > + /* Only match a perfect clock source with no > remainder */ > > > + if (clk_rate != 0 && (clk_rate / rate[j]) <= 1024 && > > + (clk_rate % rate[j]) == 0) > > + break; > > + } > > + > > + if (i == ASRC_CLK_MAP_LEN) { > > + select_clk[j] = OUTCLK_ASRCK1_CLK; > > + clk_sel[j] = false; > > + } else { > > + select_clk[j] = i; > > + clk_sel[j] = true; > > + } > > + } > > + > > + /* Switch to ideal ratio mode if there is no proper clock source */ > > + if (!clk_sel[IN] || !clk_sel[OUT]) > > + select_clk[IN] = INCLK_NONE; > > Could get rid of clk_set: > > for (j) { > for (i) { > if (match) > break; > } > > clk[j] = i; > } > > if (clk[IN] == ASRC_CLK_MAP_LEN || clk[OUT] == ASRC_CLK_MAP_LEN) > > And it only overrides clk[IN] setting but leaving clk[OUT] to > to the searching result. This means that clk[OUT] may be using > a clock source other than OUTCLK_ASRCK1_CLK if sel[IN] happens > to be false while sel[OUT] happens to be true. Not sure if it > is intended...but I feel it would probably be safer to use the > previous settings: INCLK_NONE + OUTCLK_ASRCK1_CLK? ok, will update the patch. best regards wang shengjiu
Re: [PATCH] ASoC: fsl_asrc: Add an option to select internal ratio mode
On Mon, Jun 29, 2020 at 09:58:35PM +0800, Shengjiu Wang wrote: > The ASRC not only supports ideal ratio mode, but also supports > internal ratio mode. > > For internal rato mode, the rate of clock source should be divided > with no remainder by sample rate, otherwise there is sound > distortion. > > Add function fsl_asrc_select_clk() to find proper clock source for > internal ratio mode, if the clock source is available then internal > ratio mode will be selected. > > With change, the ideal ratio mode is not the only option for user. > > Signed-off-by: Shengjiu Wang > --- > +static int fsl_asrc_select_clk(struct fsl_asrc_priv *asrc_priv, > +struct fsl_asrc_pair *pair, > +int in_rate, > +int out_rate) > +{ > + struct fsl_asrc_pair_priv *pair_priv = pair->private; > + struct asrc_config *config = pair_priv->config; > + int rate[2], select_clk[2]; /* Array size 2 means IN and OUT */ > + int clk_rate, clk_index; > + int i = 0, j = 0; > + bool clk_sel[2]; > + > + rate[0] = in_rate; > + rate[1] = out_rate; > + > + /* Select proper clock source for internal ratio mode */ > + for (j = 0; j < 2; j++) { > + for (i = 0; i < ASRC_CLK_MAP_LEN; i++) { > + clk_index = asrc_priv->clk_map[j][i]; > + clk_rate = > clk_get_rate(asrc_priv->asrck_clk[clk_index]); + /* Only match a perfect clock source with no remainder */ > + if (clk_rate != 0 && (clk_rate / rate[j]) <= 1024 && > + (clk_rate % rate[j]) == 0) > + break; > + } > + > + if (i == ASRC_CLK_MAP_LEN) { > + select_clk[j] = OUTCLK_ASRCK1_CLK; > + clk_sel[j] = false; > + } else { > + select_clk[j] = i; > + clk_sel[j] = true; > + } > + } > + > + /* Switch to ideal ratio mode if there is no proper clock source */ > + if (!clk_sel[IN] || !clk_sel[OUT]) > + select_clk[IN] = INCLK_NONE; Could get rid of clk_set: for (j) { for (i) { if (match) break; } clk[j] = i; } if (clk[IN] == ASRC_CLK_MAP_LEN || clk[OUT] == ASRC_CLK_MAP_LEN) And it only overrides clk[IN] setting but leaving clk[OUT] to to the searching result. This means that clk[OUT] may be using a clock source other than OUTCLK_ASRCK1_CLK if sel[IN] happens to be false while sel[OUT] happens to be true. Not sure if it is intended...but I feel it would probably be safer to use the previous settings: INCLK_NONE + OUTCLK_ASRCK1_CLK?