Message-ID: <20200219010905.GB32720@Asurada-Nvidia.nvidia.com>
Date:   Tue, 18 Feb 2020 17:09:05 -0800
From:   Nicolin Chen <nicoleotsuka@...il.com>
To:     Shengjiu Wang <shengjiu.wang@....com>
Cc:     timur@...nel.org, Xiubo.Lee@...il.com, festevam@...il.com,
        broonie@...nel.org, alsa-devel@...a-project.org,
        lgirdwood@...il.com, perex@...ex.cz, tiwai@...e.com,
        robh+dt@...nel.org, mark.rutland@....com,
        devicetree@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] ASoC: fsl_easrc: Add EASRC ASoC CPU DAI and
 platform drivers

On Tue, Feb 18, 2020 at 02:39:37PM +0800, Shengjiu Wang wrote:
> EASRC (Enhanced Asynchronous Sample Rate Converter) is a new IP module
> found on i.MX8MN. It is different with old ASRC module.
> 
> The primary features for the EASRC are as follows:
> - 4 Contexts - groups of channels with an independent time base
> - Fully independent and concurrent context control
> - Simultaneous processing of up to 32 audio channels
> - Programmable filter charachteristics for each context
> - 32, 24, 20, and 16-bit fixed point audio sample support
> - 32-bit floating point audio sample support
> - 8kHz to 384kHz sample rate
> - 1/16 to 8x sample rate conversion ratio
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@....com>
> ---
>  sound/soc/fsl/Kconfig           |   10 +
>  sound/soc/fsl/Makefile          |    2 +
>  sound/soc/fsl/fsl_asrc_common.h |    1 +
>  sound/soc/fsl/fsl_easrc.c       | 2265 +++++++++++++++++++++++++++++++
>  sound/soc/fsl/fsl_easrc.h       |  668 +++++++++
>  sound/soc/fsl/fsl_easrc_dma.c   |  440 ++++++

I see a 90% similarity between fsl_asrc_dma and fsl_easrc_dma files.
Would it be possible reuse the existing code? Could share structures
from my point of view, just like it reuses "enum asrc_pair_index", I
know differentiating "pair" and "context" is a big point here though.

A possible quick solution for that, off the top of my head, could be:

1) in fsl_asrc_common.h

	struct fsl_asrc {
		....
	};

	struct fsl_asrc_pair {
		....
	};

2) in fsl_easrc.h

	/* Renaming shared structures */
	#define fsl_easrc fsl_asrc
	#define fsl_easrc_context fsl_asrc_pair

May be a good idea to see if others have some opinion too.

> diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
> new file mode 100644
> index 000000000000..6fe2953317f2
> --- /dev/null
> +++ b/sound/soc/fsl/fsl_easrc.c
> +
> +/* set_rs_ratio
> + *
> + * According to the resample taps, calculate the resample ratio
> + */
> +static int set_rs_ratio(struct fsl_easrc_context *ctx)

"fsl_easrc_" prefix? Would be nice to have a formula in the comments.

> +/* resets the pointer of the coeff memory pointers */
> +static int fsl_coeff_mem_ptr_reset(struct fsl_easrc *easrc,
> +				   unsigned int ctx_id, int mem_type)
> +{
> +	/*  To reset the write pointer back to zero, the register field
> +	 *  ASRC_CTX_CTRL_EXT1x[PF_COEFF_MEM_RST] can be toggled from
> +	 *  0x0 to 0x1 to 0x0.
> +	 */

Please use the style:
	/*
	 * xxx
	 */

> +static int fsl_easrc_resampler_config(struct fsl_easrc *easrc)
> +{
> +	for (i = 0; i < hdr->interp_scen; i++) {
> +		if ((interp[i].num_taps - 1) ==
> +				bits_taps_to_val(easrc->rs_num_taps)) {

Could do below to save some indentations from the rest of the routine:
+		if ((interp[i].num_taps - 1) !=
+		    bits_taps_to_val(easrc->rs_num_taps))
+			continue;

> +			arr = interp[i].coeff;
> +			selected_interp = &interp[i];
> +			dev_dbg(dev, "Selected interp_filter: %u taps - %u phases\n",
> +				selected_interp->num_taps,
> +				selected_interp->num_phases);
> +			break;

> +/*****************************************************************************
> + *  Scale filter coefficients (64 bits float)
> + *  For input float32 normalized range (1.0,-1.0) -> output int[16,24,32]:
> + *      scale it by multiplying filter coefficients by 2^31
> + *  For input int[16, 24, 32] -> output float32
> + *      scale it by multiplying filter coefficients by 2^-15, 2^-23, 2^-31
> + *  input:
> + *      easrc:  Structure pointer of fsl_easrc
> + *      infilter : Pointer to non-scaled input filter
> + *      shift:  The multiply factor
> + *  output:
> + *      outfilter: scaled filter
> + *****************************************************************************/
> +static int NormalizedFilterForFloat32InIntOut(struct fsl_easrc *easrc,
> +					      u64 *infilter,
> +					      u64 *outfilter,
> +					      int shift)

Coding style looks very different, at comments and function naming.

> +{
> +	struct device *dev = &easrc->pdev->dev;
> +	u64 coef = *infilter;
> +	s64 exp  = (coef & 0x7ff0000000000000ll) >> 52;
> +	u64 outcoef;
> +
> +	/*
> +	 * If exponent is zero (value == 0), or 7ff (value == NaNs)
> +	 * dont touch the content
> +	 */
> +	if (((coef & 0x7ff0000000000000ll) == 0) ||
> +	    ((coef & 0x7ff0000000000000ll) == ((u64)0x7ff << 52))) {
> +		*outfilter = coef;
> +	} else {
> +		if ((shift > 0 && (shift + exp) >= 2047) ||
> +		    (shift < 0 && (exp + shift) <= 0)) {
> +			dev_err(dev, "coef error\n");
> +			return -EINVAL;
> +		}
> +
> +		/* coefficient * 2^shift ==>  coefficient_exp + shift */
> +		exp += shift;
> +		outcoef = (u64)(coef & 0x800FFFFFFFFFFFFFll) +
> +					((u64)exp << 52);
> +		*outfilter = outcoef;
> +	}
> +
> +	return 0;
> +}
> +
> +static int write_pf_coeff_mem(struct fsl_easrc *easrc, int ctx_id,
> +			      u64 *arr, int n_taps, int shift)

Function naming.

> +static int fsl_easrc_prefilter_config(struct fsl_easrc *easrc,
> +				      unsigned int ctx_id)
> +{
> +	struct fsl_easrc_context *ctx;
> +	struct asrc_firmware_hdr *hdr;
> +	struct prefil_params *prefil, *selected_prefil = NULL;
> +	struct device *dev;
> +	u32 inrate, outrate, offset = 0;
> +	int ret, i;
> +
> +	/* to modify prefilter coeficients, the user must perform
> +	 * a write in ASRC_PRE_COEFF_FIFO[COEFF_DATA] while the
> +	 * RUN_EN for that context is set to 0
> +	 */
> +	if (!easrc)
> +		return -ENODEV;

Hmm..I don't see the relationship between the comments and the code.

> +	if (ctx->out_params.sample_rate >= ctx->in_params.sample_rate) {
> +		if (ctx->out_params.sample_rate == ctx->in_params.sample_rate)
> +			regmap_update_bits(easrc->regmap,
> +					   REG_EASRC_CCE1(ctx_id),
> +					   EASRC_CCE1_RS_BYPASS_MASK,
> +					   EASRC_CCE1_RS_BYPASS);
> +
> +		if (ctx->in_params.sample_format == SNDRV_PCM_FORMAT_FLOAT_LE &&
> +		    ctx->out_params.sample_format != SNDRV_PCM_FORMAT_FLOAT_LE) {
> +			ctx->st1_num_taps = 1;
> +			ctx->st1_coeff    = &easrc->const_coeff;
> +			ctx->st1_num_exp  = 1;
> +			ctx->st2_num_taps = 0;
> +			ctx->st1_addexp = 31;
> +		} else if (ctx->in_params.sample_format != SNDRV_PCM_FORMAT_FLOAT_LE &&
> +			   ctx->out_params.sample_format == SNDRV_PCM_FORMAT_FLOAT_LE) {
> +			ctx->st1_num_taps = 1;
> +			ctx->st1_coeff    = &easrc->const_coeff;
> +			ctx->st1_num_exp  = 1;
> +			ctx->st2_num_taps = 0;
> +			ctx->st1_addexp -= ctx->in_params.fmt.addexp;
> +		} else {
> +			ctx->st1_num_taps = 1;
> +			ctx->st1_coeff    = &easrc->const_coeff;
> +			ctx->st1_num_exp  = 1;
> +			ctx->st2_num_taps = 0;

The first four lines of each path are completely duplicated. Probably
only needs to diff st1_addexp?

> +	} else {
> +		inrate = ctx->in_params.norm_rate;
> +		outrate = ctx->out_params.norm_rate;
> +
> +		hdr = easrc->firmware_hdr;
> +		prefil = easrc->prefil;
> +
> +		for (i = 0; i < hdr->prefil_scen; i++) {
> +			if (inrate == prefil[i].insr && outrate == prefil[i].outsr) {

Could do below to save indentations:
+			if (inrate != prefil[i].insr ||
+			    outrate != prefil[i].outsr)
+				continue;

> +		if (!selected_prefil) {
> +			dev_err(dev, "Conversion from in ratio %u(%u) to out ratio %u(%u) is not supported\n",
> +				ctx->in_params.sample_rate,
> +				inrate,
> +				ctx->out_params.sample_rate, outrate);

Could fit into single lines:
+				ctx->in_params.sample_rate, inrate,
+				ctx->out_params.sample_rate, outrate);

> +static int fsl_easrc_config_one_slot(struct fsl_easrc_context *ctx,
> +				     struct fsl_easrc_slot *slot,
> +				     unsigned int slot_idx,
> +				     unsigned int reg0,
> +				     unsigned int reg1,
> +				     unsigned int reg2,
> +				     unsigned int reg3,
> +				     unsigned int *req_channels,
> +				     unsigned int *start_channel,
> +				     unsigned int *avail_channel)

There could be some simplification for the parameters here:
1) slot_idx could be a part of struct fsl_easrc_slot?
2) reg0->reg3 could be a part of struct too, or use some macro
   calculating from slot_idx?

> +{
> +	struct fsl_easrc *easrc = ctx->easrc;
> +	int st1_chanxexp, st1_mem_alloc = 0, st2_mem_alloc = 0;
> +	unsigned int addr;
> +
> +	if (*req_channels <= *avail_channel) {
> +		slot->num_channel = *req_channels;
> +		slot->min_channel = *start_channel;
> +		slot->max_channel = *start_channel + *req_channels - 1;
> +		slot->ctx_index = ctx->index;
> +		slot->busy = true;
> +		*start_channel += *req_channels;
> +		*req_channels = 0;
> +	} else {
> +		slot->num_channel = *avail_channel;
> +		slot->min_channel = *start_channel;
> +		slot->max_channel = *start_channel + *avail_channel - 1;
> +		slot->ctx_index = ctx->index;
> +		slot->busy = true;
> +		*start_channel += *avail_channel;
> +		*req_channels -= *avail_channel;
> +	}

Could merge duplicated parts:
+	if (*req_channels <= *avail_channel) {
+		slot->num_channel = *req_channels;
+		*req_channels = 0;
+	} else {
+		slot->num_channel = *req_channels;
+		*req_channels -= *avail_channel;
+	};
+
+	slot->min_channel = *start_channel;
+	slot->max_channel = *start_channel + slot->num_channel - 1;
+	slot->ctx_index = ctx->index;
+	slot->busy = true;
+	*start_channel += slot->num_channel;

> +	if (ctx->st1_num_taps > 0) {
> +		if (ctx->st2_num_taps > 0)
> +			st1_mem_alloc =
> +				(ctx->st1_num_taps - 1) * slot->num_channel *
> +				ctx->st1_num_exp + slot->num_channel;
> +		else
> +			st1_mem_alloc = ctx->st1_num_taps * slot->num_channel;
> +
> +		slot->pf_mem_used = st1_mem_alloc;
> +		regmap_update_bits(easrc->regmap, reg2,
> +				   EASRC_DPCS0R2_ST1_MA_MASK,
> +				   EASRC_DPCS0R2_ST1_MA(st1_mem_alloc));
> +
> +		if (slot_idx == 1)
> +			addr = 0x1800 - st1_mem_alloc;

Where is this 0x1800 coming from?

> +int fsl_easrc_start_context(struct fsl_easrc_context *ctx)
> +{
> +			   EASRC_CC_FWMDE_MASK,
> +			   EASRC_CC_FWMDE);

> +			   EASRC_COC_FWMDE_MASK,
> +			   EASRC_COC_FWMDE);

> +			   EASRC_CC_EN_MASK,
> +			   EASRC_CC_EN);

They could fit into single lines.

> +static const struct regmap_config fsl_easrc_regmap_config = {
> +	.readable_reg = fsl_easrc_readable_reg,
> +	.volatile_reg = fsl_easrc_volatile_reg,
> +	.writeable_reg = fsl_easrc_writeable_reg,

Can we use regmap_range and regmap_access_table?

> +void easrc_dump_firmware(struct fsl_easrc *easrc)

fsl_easrc_dump_firmware?

> +{
> +	dev_dbg(dev, "Firmware v%u dump:\n", firm->firmware_version);
> +	pr_debug("Num prefitler scenarios: %u\n", firm->prefil_scen);
> +	pr_debug("Num interpolation scenarios: %u\n", firm->interp_scen);
> +	pr_debug("\nInterpolation scenarios:\n");

dev_dbg vs. pr_debug?

> +int easrc_get_firmware(struct fsl_easrc *easrc)

fsl_easrc_get_firmware?

> +static int fsl_easrc_probe(struct platform_device *pdev)
> +{
> +	/*Set default value*/

White space in the comments

> +	ret = of_property_read_u32(np, "fsl,asrc-rate",
> +				   &easrc->easrc_rate);

Could fit into one line.

> +	ret = of_property_read_u32(np, "fsl,asrc-width",
> +				   &width);

Ditto

> +static int fsl_easrc_runtime_resume(struct device *dev)
> +{
> +}
> +#endif /*CONFIG_PM*/

White space in the comments

> diff --git a/sound/soc/fsl/fsl_easrc.h b/sound/soc/fsl/fsl_easrc.h
> new file mode 100644
> index 000000000000..205f6ef3e1e3
> --- /dev/null
> +++ b/sound/soc/fsl/fsl_easrc.h
> @@ -0,0 +1,668 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2019 NXP
> + */
> +
> +#ifndef _FSL_EASRC_H
> +#define _FSL_EASRC_H
> +
> +#include <sound/asound.h>
> +#include <linux/miscdevice.h>

For miscdevice...

> +/**
> + * fsl_easrc: EASRC private data
> + *
> + * @pdev: platform device pointer
> + * @regmap: regmap handler
> + * @dma_params_rx: DMA parameters for receive channel
> + * @dma_params_tx: DMA parameters for transmit channel
> + * @ctx:  context pointer
> + * @slot: slot setting
> + * @mem_clk: clock source to access register
> + * @firmware_hdr:  the header of firmware
> + * @interp: pointer to interpolation filter coeff
> + * @prefil: pointer to prefilter coeff
> + * @fw: firmware of coeff table
> + * @fw_name: firmware name
> + * @paddr: physical address to the base address of registers
> + * @rs_num_taps:  resample filter taps, 32, 64, or 128
> + * @bps_i2c958: bits per sample of iec958
> + * @chn_avail: available channels, maximum 32
> + * @lock: spin lock for resource protection
> + * @easrc_rate: default sample rate for ASoC Back-Ends
> + * @easrc_format: default sample format for ASoC Back-Ends
> + */
> +
> +struct fsl_easrc {
> +	struct platform_device *pdev;
> +	struct regmap *regmap;
> +	struct miscdevice easrc_miscdev;

This is not being described in the comments area nor used in the
driver. Should probably stay downstream, or be added later when
the downstream feature gets upstream.

Powered by blists - more mailing lists