| Message ID | 65e96ca15afd4a282b122f3ea8b13642cf4614c7.1568025083.git.shengjiu.wang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | update supported sample format | expand |
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 >
Hi > > 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 > As you know, we have another M2M function internally, when user want to Set the format through M2M API, it is better to use snd_pcm_format_t instead the Width, for snd_pcm_format_t include two property, data with and physical width In driver some place need data width, some place need physical width. For example how to distinguish S24_LE and S24_3LE in driver, DMA setting needs The physical width, but ASRC need data width. Another purpose is that we have another new designed ASRC, which support more Formats, I would like it can share same API with this ASRC, using snd_pcm_format_t That we can use the common API, like snd_pcm_format_linear, snd_pcm_format_big_endian to get the property of the format, which is needed by driver. Best regards Wang shengjiu
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. > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> Acked-by: Nicolin Chen <nicoleotsuka@gmail.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 >
On Tue, Sep 10, 2019 at 02:22:06AM +0000, S.j. Wang wrote: > Hi > > > > > 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 > > > > As you know, we have another M2M function internally, when user want to > Set the format through M2M API, it is better to use snd_pcm_format_t instead the > Width, for snd_pcm_format_t include two property, data with and physical width > In driver some place need data width, some place need physical width. > For example how to distinguish S24_LE and S24_3LE in driver, DMA setting needs > The physical width, but ASRC need data width. > > Another purpose is that we have another new designed ASRC, which support more > Formats, I would like it can share same API with this ASRC, using snd_pcm_format_t > That we can use the common API, like snd_pcm_format_linear, > snd_pcm_format_big_endian to get the property of the format, which is needed by > driver. I see. Just acked the patch.
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; };
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. 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(-)