| Message ID | 1554866131-12088-1-git-send-email-shengjiu.wang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: fsl_asrc: replace the process_option table with function | expand |
On Wed, Apr 10, 2019 at 03:15:26AM +0000, S.j. Wang wrote: > The table is not flexible if supported sample rate is not in the > table, so use a function to replace it. Could you please elaborate a bit the special use case here? The table was copied directly from the Reference Manual. We also have listed all supported input and output sample rates just right behind that table. If there're missing rates, we probably should update those two lists also? Otherwise, how could we have a driver limiting both I/O sample rates while we still see something not in the table? > +static int proc_autosel(int Fsin, int Fsout, int *pre_proc, int *post_proc) Please add some comments to this function to explain what it does, and how it works. And better to rename it to something like "fsl_asrc_sel_proc". > +{ > + bool det_out_op2_cond; > + bool det_out_op0_cond; > + > + det_out_op2_cond = (((Fsin * 15 > Fsout * 16) & (Fsout < 56000)) | > + ((Fsin > 56000) & (Fsout < 56000))); > + det_out_op0_cond = (Fsin * 23 < Fsout * 8); "detect output option condition"? Please explain a bit or add comments to explain. > + > + /* > + * Not supported case: Tsout>16.125*Tsin, and Tsout>8.125*Tsin. Could be "unsupported". And it should fit within one line: /* Unsupported case: Tsout > 16.125 * Tsin, and Tsout > 8.125 * Tsin */ > + */ > + if (Fsin * 8 > 129 * Fsout) > + *pre_proc = 5; > + else if (Fsin * 8 > 65 * Fsout) > + *pre_proc = 4; > + else if (Fsin * 8 > 33 * Fsout) > + *pre_proc = 2; > + else if (Fsin * 8 > 15 * Fsout) { > + if (Fsin > 152000) > + *pre_proc = 2; > + else > + *pre_proc = 1; > + } else if (Fsin < 76000) > + *pre_proc = 0; > + else if (Fsin > 152000) > + *pre_proc = 2; > + else > + *pre_proc = 1; > + > + if (det_out_op2_cond) > + *post_proc = 2; > + else if (det_out_op0_cond) > + *post_proc = 0; > + else > + *post_proc = 1; > + > + if (*pre_proc == 4 || *pre_proc == 5) > + return -EINVAL; I think you'd better add some necessary comments here too. > @@ -377,11 +404,17 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair) > ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index), > ASRCTR_IDR(index) | ASRCTR_USR(index)); > > + ret = proc_autosel(inrate, outrate, &pre_proc, &post_proc); > + if (ret) { > + pair_err("No supported pre-processing options\n"); > + return ret; > + } I think we should do this earlier in this function, once We know the inrate and outrate, instead of having all register being configured then going for an error-out. Another thing confuses me: so we could have supported sample rates in the list but the hardware might not support some of them because we couldn't calculate their processing options?
Hi > > On Wed, Apr 10, 2019 at 03:15:26AM +0000, S.j. Wang wrote: > > The table is not flexible if supported sample rate is not in the > > table, so use a function to replace it. > > Could you please elaborate a bit the special use case here? > > The table was copied directly from the Reference Manual. We also have > listed all supported input and output sample rates just right behind that table. > If there're missing rates, we probably should update those two lists also? > Otherwise, how could we have a driver limiting both I/O sample rates while > we still see something not in the table? > Yes, I plan to send another patch to update the in/out rate list. Do I need To merge that to this commit? Actually we want to support 12k and 24kHz > > +static int proc_autosel(int Fsin, int Fsout, int *pre_proc, int > > +*post_proc) > > Please add some comments to this function to explain what it does, and how > it works. And better to rename it to something like "fsl_asrc_sel_proc". > Yes, some comments should be added, but not so detail, because this function Is get from the design team, but the owner has left. > > +{ > > + bool det_out_op2_cond; > > + bool det_out_op0_cond; > > + > > + det_out_op2_cond = (((Fsin * 15 > Fsout * 16) & (Fsout < 56000)) | > > + ((Fsin > 56000) & (Fsout < 56000))); > > + det_out_op0_cond = (Fsin * 23 < Fsout * 8); > > "detect output option condition"? Please explain a bit or add comments to > explain. > > > + > > + /* > > + * Not supported case: Tsout>16.125*Tsin, and Tsout>8.125*Tsin. > > Could be "unsupported". And it should fit within one line: > /* Unsupported case: Tsout > 16.125 * Tsin, and Tsout > 8.125 * Tsin */ > > > + */ > > + if (Fsin * 8 > 129 * Fsout) > > + *pre_proc = 5; > > + else if (Fsin * 8 > 65 * Fsout) > > + *pre_proc = 4; > > + else if (Fsin * 8 > 33 * Fsout) > > + *pre_proc = 2; > > + else if (Fsin * 8 > 15 * Fsout) { > > + if (Fsin > 152000) > > + *pre_proc = 2; > > + else > > + *pre_proc = 1; > > + } else if (Fsin < 76000) > > + *pre_proc = 0; > > + else if (Fsin > 152000) > > + *pre_proc = 2; > > + else > > + *pre_proc = 1; > > + > > + if (det_out_op2_cond) > > + *post_proc = 2; > > + else if (det_out_op0_cond) > > + *post_proc = 0; > > + else > > + *post_proc = 1; > > + > > + if (*pre_proc == 4 || *pre_proc == 5) > > + return -EINVAL; > > I think you'd better add some necessary comments here too. > > > @@ -377,11 +404,17 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair > *pair) > > ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index), > > ASRCTR_IDR(index) | ASRCTR_USR(index)); > > > > + ret = proc_autosel(inrate, outrate, &pre_proc, &post_proc); > > + if (ret) { > > + pair_err("No supported pre-processing options\n"); > > + return ret; > > + } > > I think we should do this earlier in this function, once We know the inrate > and outrate, instead of having all register being configured then going for an > error-out. Ok. > > Another thing confuses me: so we could have supported sample rates in the > list but the hardware might not support some of them because we couldn't > calculate their processing options? No, just want to support 12k, 24KHz, or others as customer like.
On Wed, Apr 10, 2019 at 07:22:31AM +0000, S.j. Wang wrote: > > The table was copied directly from the Reference Manual. We also have > > listed all supported input and output sample rates just right behind that table. > > If there're missing rates, we probably should update those two lists also? > > Otherwise, how could we have a driver limiting both I/O sample rates while > > we still see something not in the table? > > > > Yes, I plan to send another patch to update the in/out rate list. Do I need > To merge that to this commit? Actually we want to support 12k and 24KHz Please send separate patches but in one series. And a question: Is it possible to update the table? It'd be way quicker to use lookup table than real-time calculation all the time. I believe you can simply calculate all the values out for 12KHz and 24KHz since you have the function. If there are certain combinations of these two not being supported, then we could mark it with a special value and add an if-check to error out. > > > +static int proc_autosel(int Fsin, int Fsout, int *pre_proc, int > > > +*post_proc) > > > > Please add some comments to this function to explain what it does, and how > > it works. And better to rename it to something like "fsl_asrc_sel_proc". > > > Yes, some comments should be added, but not so detail, because this function As much comments as possible. > Is get from the design team, but the owner has left. OK...that's sad... > > Another thing confuses me: so we could have supported sample rates in the > > list but the hardware might not support some of them because we couldn't > > calculate their processing options? > > No, just want to support 12k, 24KHz, or others as customer like. I was confused because the I/O rate lists not getting updated. It makes sense now if you are abort to update them.
> -----Original Message----- > From: Nicolin Chen <nicoleotsuka@gmail.com> > Sent: Wednesday, April 10, 2019 4:01 PM > To: S.j. Wang <shengjiu.wang@nxp.com> > Cc: timur@kernel.org; Xiubo.Lee@gmail.com; festevam@gmail.com; > broonie@kernel.org; alsa-devel@alsa-project.org; linuxppc- > dev@lists.ozlabs.org > Subject: Re: [EXT] Re: [PATCH] ASoC: fsl_asrc: replace the process_option > table with function > > WARNING: This email was created outside of NXP. DO NOT CLICK links or > attachments unless you recognize the sender and know the content is safe. > > > > On Wed, Apr 10, 2019 at 07:22:31AM +0000, S.j. Wang wrote: > > > The table was copied directly from the Reference Manual. We also > > > have listed all supported input and output sample rates just right behind > that table. > > > If there're missing rates, we probably should update those two lists also? > > > Otherwise, how could we have a driver limiting both I/O sample rates > > > while we still see something not in the table? > > > > > > > Yes, I plan to send another patch to update the in/out rate list. Do > > I need To merge that to this commit? Actually we want to support 12k > > and 24KHz > > Please send separate patches but in one series. And a question: > > Is it possible to update the table? It'd be way quicker to use lookup table > than real-time calculation all the time. I believe you can simply calculate all > the values out for 12KHz and 24KHz since you have the function. If there are > certain combinations of these two not being supported, then we could mark > it with a special value and add an if-check to error out. > Yes, but I think the function should be more flexible, if someday we need to support Other sample rate, only need to update the list. > > > > +static int proc_autosel(int Fsin, int Fsout, int *pre_proc, int > > > > +*post_proc) > > > > > > Please add some comments to this function to explain what it does, > > > and how it works. And better to rename it to something like > "fsl_asrc_sel_proc". > > > > > Yes, some comments should be added, but not so detail, because this > > function > > As much comments as possible. > > > Is get from the design team, but the owner has left. > > OK...that's sad... > > > > Another thing confuses me: so we could have supported sample rates > > > in the list but the hardware might not support some of them because > > > we couldn't calculate their processing options? > > > > No, just want to support 12k, 24KHz, or others as customer like. > > I was confused because the I/O rate lists not getting updated. > It makes sense now if you are abort to update them.
On Wed, Apr 10, 2019 at 08:26:59AM +0000, S.j. Wang wrote: > > Is it possible to update the table? It'd be way quicker to use lookup table > > than real-time calculation all the time. I believe you can simply calculate all > > the values out for 12KHz and 24KHz since you have the function. If there are > > certain combinations of these two not being supported, then we could mark > > it with a special value and add an if-check to error out. > > > > Yes, but I think the function should be more flexible, if someday we need to support > Other sample rate, only need to update the list. Given the fact that the owner of the function cannot give more comments, I feel the function wouldn't be very maintainable as none of us understands it well. On the other hand, you'll need to update the supported I/O rate lists anyway, so why not just update the table as well? The supported sample rates from ALSA are limited too. Overall, I think that continue using a lookup table wins.
Hi > > > On Wed, Apr 10, 2019 at 08:26:59AM +0000, S.j. Wang wrote: > > > Is it possible to update the table? It'd be way quicker to use > > > lookup table than real-time calculation all the time. I believe you > > > can simply calculate all the values out for 12KHz and 24KHz since > > > you have the function. If there are certain combinations of these > > > two not being supported, then we could mark it with a special value and > add an if-check to error out. > > > > > > > Yes, but I think the function should be more flexible, if someday we > > need to support Other sample rate, only need to update the list. > > Given the fact that the owner of the function cannot give more comments, I > feel the function wouldn't be very maintainable as none of us understands it > well. On the other hand, you'll need to update the supported I/O rate lists > anyway, so why not just update the table as well? The supported sample > rates from ALSA are limited too. Overall, I think that continue using a lookup > table wins. Alsa support SNDRV_PCM_RATE_KNOT, we can define the rate that we want To support, and use function is more flexible, and we have use the function Internally for long time, it is stable
On Wed, Apr 10, 2019 at 10:34:05AM +0000, S.j. Wang wrote: > > On Wed, Apr 10, 2019 at 08:26:59AM +0000, S.j. Wang wrote: > > > > Is it possible to update the table? It'd be way quicker to use > > > > lookup table than real-time calculation all the time. I believe you > > > > can simply calculate all the values out for 12KHz and 24KHz since > > > > you have the function. If there are certain combinations of these > > > > two not being supported, then we could mark it with a special value and > > add an if-check to error out. > > > > > > > > > > Yes, but I think the function should be more flexible, if someday we > > > need to support Other sample rate, only need to update the list. > > > > Given the fact that the owner of the function cannot give more comments, I > > feel the function wouldn't be very maintainable as none of us understands it > > well. On the other hand, you'll need to update the supported I/O rate lists > > anyway, so why not just update the table as well? The supported sample > > rates from ALSA are limited too. Overall, I think that continue using a lookup > > table wins. > > Alsa support SNDRV_PCM_RATE_KNOT, we can define the rate that we want > To support, and use function is more flexible, and we have use the function > Internally for long time, it is stable
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index 0b937924d2e4..a57c6c829060 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, @@ -79,6 +61,49 @@ static unsigned char *clk_map[2]; +static int proc_autosel(int Fsin, int Fsout, int *pre_proc, int *post_proc) +{ + bool det_out_op2_cond; + bool det_out_op0_cond; + + det_out_op2_cond = (((Fsin * 15 > Fsout * 16) & (Fsout < 56000)) | + ((Fsin > 56000) & (Fsout < 56000))); + det_out_op0_cond = (Fsin * 23 < Fsout * 8); + + /* + * Not supported case: Tsout>16.125*Tsin, and Tsout>8.125*Tsin. + */ + if (Fsin * 8 > 129 * Fsout) + *pre_proc = 5; + else if (Fsin * 8 > 65 * Fsout) + *pre_proc = 4; + else if (Fsin * 8 > 33 * Fsout) + *pre_proc = 2; + else if (Fsin * 8 > 15 * Fsout) { + if (Fsin > 152000) + *pre_proc = 2; + else + *pre_proc = 1; + } else if (Fsin < 76000) + *pre_proc = 0; + else if (Fsin > 152000) + *pre_proc = 2; + else + *pre_proc = 1; + + if (det_out_op2_cond) + *post_proc = 2; + else if (det_out_op0_cond) + *post_proc = 0; + else + *post_proc = 1; + + if (*pre_proc == 4 || *pre_proc == 5) + return -EINVAL; + + return 0; +} + /** * Request ASRC pair * @@ -239,8 +264,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"); @@ -377,11 +404,17 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair) ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index), ASRCTR_IDR(index) | ASRCTR_USR(index)); + ret = proc_autosel(inrate, outrate, &pre_proc, &post_proc); + if (ret) { + pair_err("No supported pre-processing options\n"); + return ret; + } + /* 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); }
The table is not flexible if supported sample rate is not in the table, so use a function to replace it. Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> --- sound/soc/fsl/fsl_asrc.c | 73 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 20 deletions(-)