Re: [alsa-devel] [PATCH 1/3] ASoC: fsl_asrc: Use in(out)put_format instead of in(out)put_word_width

On Mon, Sep 09, 2019 at 06:33:19PM -0400, Shengjiu Wang wrote:

snd_pcm_format_t is more formal than enum asrc_word_width, which has two property, width and physical width, which is more accurate than enum asrc_word_width. So it is better to use in(out)put_format instead of in(out)put_word_width.

Hmm...I don't really see the benefit of using snd_pcm_format_t here...I mean, I know it's a generic one, and would understand if we use it as a param for a common API. But this patch merely packs the "width" by intentionally using this snd_pcm_format_t and then adds another translation to unpack it.. I feel it's a bit overcomplicated. Or am I missing something?

And I feel it's not necessary to use ALSA common format in our own "struct asrc_config" since it is more IP/register specific.

Thanks Nicolin

Signed-off-by: Shengjiu Wang shengjiu.wang@nxp.com

sound/soc/fsl/fsl_asrc.c | 56 +++++++++++++++++++++++++++------------- sound/soc/fsl/fsl_asrc.h | 4 +-- 2 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index cfa40ef6b1ca..4d3804a1ea55 100644 --- a/sound/soc/fsl/fsl_asrc.c +++ b/sound/soc/fsl/fsl_asrc.c @@ -265,6 +265,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair) struct asrc_config *config = pair->config; struct fsl_asrc *asrc_priv = pair->asrc_priv; enum asrc_pair_index index = pair->index;

• enum asrc_word_width input_word_width;
• enum asrc_word_width output_word_width; u32 inrate, outrate, indiv, outdiv; u32 clk_index[2], div[2]; int in, out, channels;

@@ -283,9 +285,32 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair) return -EINVAL; }

• /* Validate output width */
• if (config->output_word_width == ASRC_WIDTH_8_BIT) {

• pair_err("does not support 8bit width output\n");
  

• switch (snd_pcm_format_width(config->input_format)) {
• case 8:

• input_word_width = ASRC_WIDTH_8_BIT;
  

• break;
  

• case 16:

• input_word_width = ASRC_WIDTH_16_BIT;
  

• break;
  

• case 24:

• input_word_width = ASRC_WIDTH_24_BIT;
  

• break;
  

• default:

• pair_err("does not support this input format, %d\n",
  

• 	 config->input_format);
  

• return -EINVAL;
  

• }
• switch (snd_pcm_format_width(config->output_format)) {
• case 16:

• output_word_width = ASRC_WIDTH_16_BIT;
  

• break;
  

• case 24:

• output_word_width = ASRC_WIDTH_24_BIT;
  

• break;
  

• default:

• pair_err("does not support this output format, %d\n",
  

• 	 config->output_format);
  

  return -EINVAL; }

@@ -383,8 +408,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair) /* Implement word_width configurations */ regmap_update_bits(asrc_priv->regmap, REG_ASRMCR1(index), ASRMCR1i_OW16_MASK | ASRMCR1i_IWD_MASK,

• 	   ASRMCR1i_OW16(config->output_word_width) |
  

• 	   ASRMCR1i_IWD(config->input_word_width));
  

• 	   ASRMCR1i_OW16(output_word_width) |
  

• 	   ASRMCR1i_IWD(input_word_width));
  
  

  /* Enable BUFFER STALL */ regmap_update_bits(asrc_priv->regmap, REG_ASRMCR(index),

@@ -497,13 +522,13 @@ static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);

• int width = params_width(params); struct snd_pcm_runtime *runtime = substream->runtime; struct fsl_asrc_pair *pair = runtime->private_data; unsigned int channels = params_channels(params); unsigned int rate = params_rate(params); struct asrc_config config;
• int word_width, ret;

• snd_pcm_format_t format;

• int ret;

  ret = fsl_asrc_request_pair(channels, pair); if (ret) {

@@ -513,15 +538,10 @@ static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,

pair->config = &config;

• if (width == 16)

• width = ASRC_WIDTH_16_BIT;
  

• else

• width = ASRC_WIDTH_24_BIT;
  

• if (asrc_priv->asrc_width == 16)

• word_width = ASRC_WIDTH_16_BIT;
  

• format = SNDRV_PCM_FORMAT_S16_LE;
  

  else

• word_width = ASRC_WIDTH_24_BIT;
  

• format = SNDRV_PCM_FORMAT_S24_LE;
  
  

  config.pair = pair->index; config.channel_num = channels;

@@ -529,13 +549,13 @@ static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream, config.outclk = OUTCLK_ASRCK1_CLK;

if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {

• config.input_word_width   = width;
  

• config.output_word_width  = word_width;
  

• config.input_format   = params_format(params);
  

• config.output_format  = format;
  

  config.input_sample_rate = rate; config.output_sample_rate = asrc_priv->asrc_rate; } else {

• config.input_word_width   = word_width;
  

• config.output_word_width  = width;
  

• config.input_format   = format;
  

• config.output_format  = params_format(params);
  

  config.input_sample_rate = asrc_priv->asrc_rate; config.output_sample_rate = rate; }

diff --git a/sound/soc/fsl/fsl_asrc.h b/sound/soc/fsl/fsl_asrc.h index c60075112570..38af485bdd22 100644 --- a/sound/soc/fsl/fsl_asrc.h +++ b/sound/soc/fsl/fsl_asrc.h @@ -342,8 +342,8 @@ struct asrc_config { unsigned int dma_buffer_size; unsigned int input_sample_rate; unsigned int output_sample_rate;

• enum asrc_word_width input_word_width;
• enum asrc_word_width output_word_width;

• snd_pcm_format_t input_format;
• snd_pcm_format_t output_format; enum asrc_inclk inclk; enum asrc_outclk outclk;

};

2.21.0