Re: [PATCH] ASoC: fsl_asrc: replace the process_option table with function

[Nicolin Chen <nicoleotsuka@gmail.com>]

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?