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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
