Re: [PATCH v5 7/7] ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers

A few small comments -- trying to improve readability.

On Mon, Mar 09, 2020 at 11:58:34AM +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@nxp.com Signed-off-by: Cosmin-Gabriel Samoila cosmin.samoila@nxp.com

---

sound/soc/fsl/Kconfig | 11 + sound/soc/fsl/Makefile | 2 + sound/soc/fsl/fsl_easrc.c | 2111 +++++++++++++++++++++++++++++++++++++ sound/soc/fsl/fsl_easrc.h | 651 ++++++++++++ 4 files changed, 2775 insertions(+) create mode 100644 sound/soc/fsl/fsl_easrc.c create mode 100644 sound/soc/fsl/fsl_easrc.h

diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c

+static int fsl_easrc_resampler_config(struct fsl_asrc *easrc) +{

• struct device *dev = &easrc->pdev->dev;
• struct fsl_easrc_priv *easrc_priv = easrc->private;
• struct asrc_firmware_hdr *hdr = easrc_priv->firmware_hdr;
• struct interp_params *interp = easrc_priv->interp;
• struct interp_params *selected_interp = NULL;
• unsigned int num_coeff;
• unsigned int i;
• u64 *arr;
• u32 *r;
• int ret;
• if (!hdr) {

• dev_err(dev, "firmware not loaded!\n");
  

• return -ENODEV;
  

• }
• for (i = 0; i < hdr->interp_scen; i++) {

• if ((interp[i].num_taps - 1) ==
  

•     bits_taps_to_val(easrc_priv->rs_num_taps)) {
  

Could fit everything under 80 characters:

+ if ((interp[i].num_taps - 1) != + bits_taps_to_val(easrc_priv->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;

+static int fsl_easrc_normalize_filter(struct fsl_asrc *easrc,

• 		      u64 *infilter,
  

• 		      u64 *outfilter,
  

• 		      int shift)
  

+{

• struct device *dev = &easrc->pdev->dev;
• u64 coef = *infilter;

• s64 exp = (coef & 0x7ff0000000000000ll) >> 52;

Hmm...by masking 0x7ff0000000000000ll, MSB (sign bit) is gone. Would the result still possibly be a negative value?

• /*

• * If exponent is zero (value == 0), or 7ff (value == NaNs)
  

• * dont touch the content
  

• */
  

• if (((coef & 0x7ff0000000000000ll) == 0) ||

•    ((coef & 0x7ff0000000000000ll) == ((u64)0x7ff << 52))) {
  

You have extracted "exp" already: + if (exp == 0 || (u64)exp & 0x7ff == 0x7ff)

• *outfilter = coef;
  

Could simply a bit by returning here: + return 0; + } Then:

+ /* coef * 2^shift == exp + shift */ + exp += shift; + + if ((shift > 0 && exp >= 2047) || (shift < 0 && exp <= 0)) { + dev_err(dev, "coef out of range\n"); + return -EINVAL; + } + + outcoef = (u64)(coef & 0x800FFFFFFFFFFFFFll) + ((u64)exp << 52); + *outfilter = outcoef;

+static int fsl_easrc_write_pf_coeff_mem(struct fsl_asrc *easrc, int ctx_id,

• 			u64 *arr, int n_taps, int shift)
  

+{

• if (!arr) {

• dev_err(dev, "NULL buffer\n");
  

Could it be slightly more specific?

+static int fsl_easrc_prefilter_config(struct fsl_asrc *easrc,

• 		      unsigned int ctx_id)
  

+{

• ctx_priv->in_filled_sample = bits_taps_to_val(easrc_priv->rs_num_taps) / 2;
• ctx_priv->out_missed_sample = ctx_priv->in_filled_sample *

• 			  ctx_priv->out_params.sample_rate /
  

• 			  ctx_priv->in_params.sample_rate;
  

There are quite a few references to sample_rate and sample_format here, so we could use some local variables to cache them:

+ in_s_rate = ctx_priv->in_params.sample_rate; + out_s_rate = ctx_priv->out_params.sample_rate; + in_s_fmt = ctx_priv->in_params.sample_format; + out_s_fmt = ctx_priv->out_params.sample_format;

+static int fsl_easrc_config_slot(struct fsl_asrc *easrc, unsigned int ctx_id) +{

• struct fsl_easrc_priv *easrc_priv = easrc->private;
• struct fsl_asrc_pair *ctx = easrc->pair[ctx_id];
• int req_channels = ctx->channels;
• int start_channel = 0, avail_channel;
• struct fsl_easrc_slot *slot0, *slot1;
• int i, ret;
• if (req_channels <= 0)

• return -EINVAL;
  

• for (i = 0; i < EASRC_CTX_MAX_NUM; i++) {

• slot0 = &easrc_priv->slot[i][0];
  

• slot1 = &easrc_priv->slot[i][1];
  

• if (slot0->busy && slot1->busy)
  

• 	continue;
  

Could merge the duplication by doing: + struct fsl_easrc_slot *slot = NULL; ... + else if ((slot0->busy && slot0->ctx_index == ctx->index) || + (slot1->busy && slot1->ctx_index == ctx->index)) + continue; + else if (!slot0->busy) + slot = slot0; + else if (!slot1->busy) + slot = slot1; + + if (!slot) + continue; + + avail_channel = fsl_easrc_max_ch_for_slot(ctx, slot); + if (avail_channel <= 0) + continue; + + slot->slot_index = 0; + + ret = fsl_easrc_config_one_slot(ctx, slot, i, &req_channels, + &start_channel, &avail_channel); + if (ret) + return ret; + + if (req_channels > 0) + continue; + else + break;

# Please double check before doing copy-n-paste.

+int fsl_easrc_config_context(struct fsl_asrc *easrc, unsigned int ctx_id) +{

• /* Context Input FIFO Watermark */
• regmap_update_bits(easrc->regmap, REG_EASRC_CC(ctx_id),

• 	   EASRC_CC_FIFO_WTMK_MASK,
  

• 	   EASRC_CC_FIFO_WTMK(ctx_priv->in_params.fifo_wtmk));
  

• /* Context Output FIFO Watermark */
• regmap_update_bits(easrc->regmap, REG_EASRC_COC(ctx_id),

• 	   EASRC_COC_FIFO_WTMK_MASK,
  

• 	   EASRC_COC_FIFO_WTMK(ctx_priv->out_params.fifo_wtmk - 1));
  

Why a "-1" here vs. no "-1" for input FIFO? Could probably put the reason in the comments?

+void fsl_easrc_release_context(struct fsl_asrc_pair *ctx) +{

• unsigned long lock_flags;
• struct fsl_asrc *easrc;
• struct device *dev;
• int ret;
• if (!ctx)

• return;
  

• easrc = ctx->asrc;
• dev = &easrc->pdev->dev;
• spin_lock_irqsave(&easrc->lock, lock_flags);
• ret = fsl_easrc_release_slot(easrc, ctx->index);

Where is this "ret" used?

• easrc->channel_avail += ctx->channels;
• easrc->pair[ctx->index] = NULL;
• spin_unlock_irqrestore(&easrc->lock, lock_flags);

+}

+void fsl_easrc_dump_firmware(struct fsl_asrc *easrc)

Hmm..where is this function being used? From outside?

+int fsl_easrc_get_firmware(struct fsl_asrc *easrc)

static?

If it's being called from an outsider, it might be safer to check easrc->private pointer too?

+{

• struct fsl_easrc_priv *easrc_priv;

Could probably clean up some wrappings with: + struct firmware **fw_p;

• u32 pnum, inum, offset;

+ u8 *data;

• int ret;
• if (!easrc)

• return -EINVAL;
  

• easrc_priv = easrc->private;

+ fw_p = &easrc_priv->fw;

• ret = request_firmware(&easrc_priv->fw, easrc_priv->fw_name,

• 	       &easrc->pdev->dev);
  

+ ret = request_firmware(fw_p, easrc_priv->fw_name, &easrc->pdev->dev);

• if (ret)

• return ret;
  

+ data = easrc_priv->fw->data;

And replace all data references.

+static int fsl_easrc_get_fifo_addr(u8 dir, enum asrc_pair_index index) +{

• return REG_EASRC_FIFO(dir, index);

Maybe an inline type or simply a macro?

+static int fsl_easrc_probe(struct platform_device *pdev) +{

: + irq = platform_get_irq(pdev, 0);

• if (irq < 0) {

• dev_err(&pdev->dev, "no irq for node %s\n",
  

• 	dev_name(&pdev->dev));
  

Probably could save some wrappings in this function if we have a: struct device *dev = &pdev->dev;

And dev_err() prints dev_name() actually, so it'd be better use: + dev_err(dev, "no irq for node %pOF\n", np);

• ret = of_property_read_string(np,

• 		      "fsl,easrc-ram-script-name",
  

• 		      &easrc_priv->fw_name);
  

• if (ret) {

• dev_err(&pdev->dev, "failed to get firmware name\n");
  

• return ret;
  

• }

Could we move this to the place where we parse DT bindings?

+static int fsl_easrc_runtime_resume(struct device *dev) +{

• for (i = ASRC_PAIR_A; i < EASRC_CTX_MAX_NUM; i++) {

• ctx = easrc->pair[i];
  

• if (ctx) {
  

Could do this to save some indentations from following lines: + if (!ctx) + continue;

• 	ctx_priv = ctx->private;
  

• 	fsl_easrc_set_rs_ratio(ctx);
  

• 	ctx_priv->out_missed_sample = ctx_priv->in_filled_sample *
  

• 					  ctx_priv->out_params.sample_rate /
  

• 					  ctx_priv->in_params.sample_rate;
  

• 	if (ctx_priv->in_filled_sample * ctx_priv->out_params.sample_rate
  

• 			% ctx_priv->in_params.sample_rate != 0)
  

• 		ctx_priv->out_missed_sample += 1;
  

• 	ret = fsl_easrc_write_pf_coeff_mem(easrc, i,
  

• 					   ctx_priv->st1_coeff,
  

• 					   ctx_priv->st1_num_taps,
  

• 					   ctx_priv->st1_addexp);
  

• 	if (ret)
  

• 		goto disable_mem_clk;
  

• 	ret = fsl_easrc_write_pf_coeff_mem(easrc, i,
  

• 					   ctx_priv->st2_coeff,
  

• 					   ctx_priv->st2_num_taps,
  

• 					   ctx_priv->st2_addexp);
  

• 	if (ret)
  

• 		goto disable_mem_clk;
  

diff --git a/sound/soc/fsl/fsl_easrc.h b/sound/soc/fsl/fsl_easrc.h +struct fsl_easrc_slot {

• bool busy;
• int ctx_index;
• int slot_index;
• int num_channel; /*maximum is 8*/

Spaces for comments: /* maximum is 8 */

+/**

• • fsl_easrc_priv: EASRC private data
• • @slot: slot setting
• • @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
• • @rs_num_taps: resample filter taps, 32, 64, or 128
• • @bps_i2c958: bits per sample of IEC958

i2c or iec?