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 <redacted>
---
 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.

"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

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.