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@xxxxxxx>
> Signed-off-by: Cosmin-Gabriel Samoila <cosmin.samoila@xxxxxxx>
> ---
> 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?
