Re: [PATCH v3 1/4] ASoC: fsl_asrc: Change asrc_width to asrc_format
On Fri, Feb 28, 2020 at 2:40 PM Nicolin Chen wrote: > > On Fri, Feb 28, 2020 at 10:54:02AM +0800, Shengjiu Wang wrote: > > Hi > > > > On Fri, Feb 28, 2020 at 1:45 AM Nicolin Chen wrote: > > > > > > On Thu, Feb 27, 2020 at 01:10:19PM +0800, Shengjiu Wang wrote: > > > > On Thu, Feb 27, 2020 at 11:43 AM Nicolin Chen > > > > wrote: > > > > > > > > > > On Thu, Feb 27, 2020 at 10:41:55AM +0800, Shengjiu Wang wrote: > > > > > > asrc_format is more inteligent variable, which is align > > > > > > with the alsa definition snd_pcm_format_t. > > > > > > > > > > > > Signed-off-by: Shengjiu Wang > > > > > > --- > > > > > > sound/soc/fsl/fsl_asrc.c | 23 +++ > > > > > > sound/soc/fsl/fsl_asrc.h | 4 ++-- > > > > > > sound/soc/fsl/fsl_asrc_dma.c | 2 +- > > > > > > 3 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > > > > > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c > > > > > > index 0dcebc24c312..2b6a1643573c 100644 > > > > > > --- a/sound/soc/fsl/fsl_asrc.c > > > > > > +++ b/sound/soc/fsl/fsl_asrc.c > > > > > > > > > > > @@ -600,11 +599,6 @@ static int fsl_asrc_dai_hw_params(struct > > > > > > snd_pcm_substream *substream, > > > > > > > > > > > > pair->config = > > > > > > > > > > > > - if (asrc_priv->asrc_width == 16) > > > > > > - format = SNDRV_PCM_FORMAT_S16_LE; > > > > > > - else > > > > > > - format = SNDRV_PCM_FORMAT_S24_LE; > > > > > > > > > > It feels better to me that we have format settings in hw_params(). > > > > > > > > > > Why not let fsl_easrc align with this? Any reason that I'm missing? > > > > > > > > because the asrc_width is not formal, in the future we can direct > > > > > > Hmm..that's our DT binding. And I don't feel it is a problem > > > to be ASoC irrelative. > > > > > > > input the format from the dts. format involve the info about width. > > > > > > Is there such any formal ASoC binding? I don't see those PCM > > > formats under include/dt-bindings/ folder. How are we going > > > to involve those formats in DT? > > > > There is no formal binding of this case. > > > > I think it is not good to convert width to format, because, for example > > The thing is that fsl_easrc does the conversion too... It just > does in the probe instead of hw_params(), and then copies them > in the hw_params(). So I don't see obvious benefit by doing so. > > > width = 24, there is two option, we can select format S24_LE, or > > format S24_3LE, width is ambiguous for selecting. > > > > In EASRC, it support other two 24bit format U24_LE, U24_3LE . > > I understood the reason here, but am not seeing the necessity, > at this point. > > > if we use the format in DT, then it is clear for usage in driver. > > I think this is the thing that we should address first. If we > have such a binding being added with the new fsl_easrc driver, > I'd love to see the old driver aligning with the new one. > > Otherwise, we can keep the old way, and change it when the new > binding is ready. ok, I will change the binding this time in next version. best regards wang shengjiu
Re: [PATCH v3 1/4] ASoC: fsl_asrc: Change asrc_width to asrc_format
On Fri, Feb 28, 2020 at 10:54:02AM +0800, Shengjiu Wang wrote: > Hi > > On Fri, Feb 28, 2020 at 1:45 AM Nicolin Chen wrote: > > > > On Thu, Feb 27, 2020 at 01:10:19PM +0800, Shengjiu Wang wrote: > > > On Thu, Feb 27, 2020 at 11:43 AM Nicolin Chen > > > wrote: > > > > > > > > On Thu, Feb 27, 2020 at 10:41:55AM +0800, Shengjiu Wang wrote: > > > > > asrc_format is more inteligent variable, which is align > > > > > with the alsa definition snd_pcm_format_t. > > > > > > > > > > Signed-off-by: Shengjiu Wang > > > > > --- > > > > > sound/soc/fsl/fsl_asrc.c | 23 +++ > > > > > sound/soc/fsl/fsl_asrc.h | 4 ++-- > > > > > sound/soc/fsl/fsl_asrc_dma.c | 2 +- > > > > > 3 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c > > > > > index 0dcebc24c312..2b6a1643573c 100644 > > > > > --- a/sound/soc/fsl/fsl_asrc.c > > > > > +++ b/sound/soc/fsl/fsl_asrc.c > > > > > > > > > @@ -600,11 +599,6 @@ static int fsl_asrc_dai_hw_params(struct > > > > > snd_pcm_substream *substream, > > > > > > > > > > pair->config = > > > > > > > > > > - if (asrc_priv->asrc_width == 16) > > > > > - format = SNDRV_PCM_FORMAT_S16_LE; > > > > > - else > > > > > - format = SNDRV_PCM_FORMAT_S24_LE; > > > > > > > > It feels better to me that we have format settings in hw_params(). > > > > > > > > Why not let fsl_easrc align with this? Any reason that I'm missing? > > > > > > because the asrc_width is not formal, in the future we can direct > > > > Hmm..that's our DT binding. And I don't feel it is a problem > > to be ASoC irrelative. > > > > > input the format from the dts. format involve the info about width. > > > > Is there such any formal ASoC binding? I don't see those PCM > > formats under include/dt-bindings/ folder. How are we going > > to involve those formats in DT? > > There is no formal binding of this case. > > I think it is not good to convert width to format, because, for example The thing is that fsl_easrc does the conversion too... It just does in the probe instead of hw_params(), and then copies them in the hw_params(). So I don't see obvious benefit by doing so. > width = 24, there is two option, we can select format S24_LE, or > format S24_3LE, width is ambiguous for selecting. > > In EASRC, it support other two 24bit format U24_LE, U24_3LE . I understood the reason here, but am not seeing the necessity, at this point. > if we use the format in DT, then it is clear for usage in driver. I think this is the thing that we should address first. If we have such a binding being added with the new fsl_easrc driver, I'd love to see the old driver aligning with the new one. Otherwise, we can keep the old way, and change it when the new binding is ready.
Re: [PATCH v3 1/4] ASoC: fsl_asrc: Change asrc_width to asrc_format
Hi On Fri, Feb 28, 2020 at 1:45 AM Nicolin Chen wrote: > > On Thu, Feb 27, 2020 at 01:10:19PM +0800, Shengjiu Wang wrote: > > On Thu, Feb 27, 2020 at 11:43 AM Nicolin Chen > > wrote: > > > > > > On Thu, Feb 27, 2020 at 10:41:55AM +0800, Shengjiu Wang wrote: > > > > asrc_format is more inteligent variable, which is align > > > > with the alsa definition snd_pcm_format_t. > > > > > > > > Signed-off-by: Shengjiu Wang > > > > --- > > > > sound/soc/fsl/fsl_asrc.c | 23 +++ > > > > sound/soc/fsl/fsl_asrc.h | 4 ++-- > > > > sound/soc/fsl/fsl_asrc_dma.c | 2 +- > > > > 3 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c > > > > index 0dcebc24c312..2b6a1643573c 100644 > > > > --- a/sound/soc/fsl/fsl_asrc.c > > > > +++ b/sound/soc/fsl/fsl_asrc.c > > > > > > > @@ -600,11 +599,6 @@ static int fsl_asrc_dai_hw_params(struct > > > > snd_pcm_substream *substream, > > > > > > > > pair->config = > > > > > > > > - if (asrc_priv->asrc_width == 16) > > > > - format = SNDRV_PCM_FORMAT_S16_LE; > > > > - else > > > > - format = SNDRV_PCM_FORMAT_S24_LE; > > > > > > It feels better to me that we have format settings in hw_params(). > > > > > > Why not let fsl_easrc align with this? Any reason that I'm missing? > > > > because the asrc_width is not formal, in the future we can direct > > Hmm..that's our DT binding. And I don't feel it is a problem > to be ASoC irrelative. > > > input the format from the dts. format involve the info about width. > > Is there such any formal ASoC binding? I don't see those PCM > formats under include/dt-bindings/ folder. How are we going > to involve those formats in DT? There is no formal binding of this case. I think it is not good to convert width to format, because, for example width = 24, there is two option, we can select format S24_LE, or format S24_3LE, width is ambiguous for selecting. In EASRC, it support other two 24bit format U24_LE, U24_3LE . if we use the format in DT, then it is clear for usage in driver. best regards wang shengjiu
Re: [PATCH v3 1/4] ASoC: fsl_asrc: Change asrc_width to asrc_format
On Thu, Feb 27, 2020 at 01:10:19PM +0800, Shengjiu Wang wrote: > On Thu, Feb 27, 2020 at 11:43 AM Nicolin Chen wrote: > > > > On Thu, Feb 27, 2020 at 10:41:55AM +0800, Shengjiu Wang wrote: > > > asrc_format is more inteligent variable, which is align > > > with the alsa definition snd_pcm_format_t. > > > > > > Signed-off-by: Shengjiu Wang > > > --- > > > sound/soc/fsl/fsl_asrc.c | 23 +++ > > > sound/soc/fsl/fsl_asrc.h | 4 ++-- > > > sound/soc/fsl/fsl_asrc_dma.c | 2 +- > > > 3 files changed, 14 insertions(+), 15 deletions(-) > > > > > > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c > > > index 0dcebc24c312..2b6a1643573c 100644 > > > --- a/sound/soc/fsl/fsl_asrc.c > > > +++ b/sound/soc/fsl/fsl_asrc.c > > > > > @@ -600,11 +599,6 @@ static int fsl_asrc_dai_hw_params(struct > > > snd_pcm_substream *substream, > > > > > > pair->config = > > > > > > - if (asrc_priv->asrc_width == 16) > > > - format = SNDRV_PCM_FORMAT_S16_LE; > > > - else > > > - format = SNDRV_PCM_FORMAT_S24_LE; > > > > It feels better to me that we have format settings in hw_params(). > > > > Why not let fsl_easrc align with this? Any reason that I'm missing? > > because the asrc_width is not formal, in the future we can direct Hmm..that's our DT binding. And I don't feel it is a problem to be ASoC irrelative. > input the format from the dts. format involve the info about width. Is there such any formal ASoC binding? I don't see those PCM formats under include/dt-bindings/ folder. How are we going to involve those formats in DT?
Re: [PATCH v3 1/4] ASoC: fsl_asrc: Change asrc_width to asrc_format
On Thu, Feb 27, 2020 at 11:43 AM Nicolin Chen wrote: > > On Thu, Feb 27, 2020 at 10:41:55AM +0800, Shengjiu Wang wrote: > > asrc_format is more inteligent variable, which is align > > with the alsa definition snd_pcm_format_t. > > > > Signed-off-by: Shengjiu Wang > > --- > > sound/soc/fsl/fsl_asrc.c | 23 +++ > > sound/soc/fsl/fsl_asrc.h | 4 ++-- > > sound/soc/fsl/fsl_asrc_dma.c | 2 +- > > 3 files changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c > > index 0dcebc24c312..2b6a1643573c 100644 > > --- a/sound/soc/fsl/fsl_asrc.c > > +++ b/sound/soc/fsl/fsl_asrc.c > > > @@ -600,11 +599,6 @@ static int fsl_asrc_dai_hw_params(struct > > snd_pcm_substream *substream, > > > > pair->config = > > > > - if (asrc_priv->asrc_width == 16) > > - format = SNDRV_PCM_FORMAT_S16_LE; > > - else > > - format = SNDRV_PCM_FORMAT_S24_LE; > > It feels better to me that we have format settings in hw_params(). > > Why not let fsl_easrc align with this? Any reason that I'm missing? because the asrc_width is not formal, in the future we can direct input the format from the dts. format involve the info about width. best regards wang shengjiu
Re: [PATCH v3 1/4] ASoC: fsl_asrc: Change asrc_width to asrc_format
On Thu, Feb 27, 2020 at 10:41:55AM +0800, Shengjiu Wang wrote: > asrc_format is more inteligent variable, which is align > with the alsa definition snd_pcm_format_t. > > Signed-off-by: Shengjiu Wang > --- > sound/soc/fsl/fsl_asrc.c | 23 +++ > sound/soc/fsl/fsl_asrc.h | 4 ++-- > sound/soc/fsl/fsl_asrc_dma.c | 2 +- > 3 files changed, 14 insertions(+), 15 deletions(-) > > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c > index 0dcebc24c312..2b6a1643573c 100644 > --- a/sound/soc/fsl/fsl_asrc.c > +++ b/sound/soc/fsl/fsl_asrc.c > @@ -600,11 +599,6 @@ static int fsl_asrc_dai_hw_params(struct > snd_pcm_substream *substream, > > pair->config = > > - if (asrc_priv->asrc_width == 16) > - format = SNDRV_PCM_FORMAT_S16_LE; > - else > - format = SNDRV_PCM_FORMAT_S24_LE; It feels better to me that we have format settings in hw_params(). Why not let fsl_easrc align with this? Any reason that I'm missing?
[PATCH v3 1/4] ASoC: fsl_asrc: Change asrc_width to asrc_format
asrc_format is more inteligent variable, which is align
with the alsa definition snd_pcm_format_t.
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 23 +++
sound/soc/fsl/fsl_asrc.h | 4 ++--
sound/soc/fsl/fsl_asrc_dma.c | 2 +-
3 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 0dcebc24c312..2b6a1643573c 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -589,7 +589,6 @@ static int fsl_asrc_dai_hw_params(struct snd_pcm_substream
*substream,
unsigned int channels = params_channels(params);
unsigned int rate = params_rate(params);
struct asrc_config config;
- snd_pcm_format_t format;
int ret;
ret = fsl_asrc_request_pair(channels, pair);
@@ -600,11 +599,6 @@ static int fsl_asrc_dai_hw_params(struct snd_pcm_substream
*substream,
pair->config =
- if (asrc_priv->asrc_width == 16)
- format = SNDRV_PCM_FORMAT_S16_LE;
- else
- format = SNDRV_PCM_FORMAT_S24_LE;
-
config.pair = pair->index;
config.channel_num = channels;
config.inclk = INCLK_NONE;
@@ -612,11 +606,11 @@ static int fsl_asrc_dai_hw_params(struct
snd_pcm_substream *substream,
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
config.input_format = params_format(params);
- config.output_format = format;
+ config.output_format = asrc_priv->asrc_format;
config.input_sample_rate = rate;
config.output_sample_rate = asrc_priv->asrc_rate;
} else {
- config.input_format = format;
+ config.input_format = asrc_priv->asrc_format;
config.output_format = params_format(params);
config.input_sample_rate = asrc_priv->asrc_rate;
config.output_sample_rate = rate;
@@ -946,6 +940,7 @@ static int fsl_asrc_probe(struct platform_device *pdev)
int irq, ret, i;
u32 map_idx;
char tmp[16];
+ u32 width;
asrc_priv = devm_kzalloc(>dev, sizeof(*asrc_priv), GFP_KERNEL);
if (!asrc_priv)
@@ -1052,18 +1047,22 @@ static int fsl_asrc_probe(struct platform_device *pdev)
return ret;
}
- ret = of_property_read_u32(np, "fsl,asrc-width",
- _priv->asrc_width);
+ ret = of_property_read_u32(np, "fsl,asrc-width", );
if (ret) {
dev_err(>dev, "failed to get output width\n");
return ret;
}
- if (asrc_priv->asrc_width != 16 && asrc_priv->asrc_width != 24) {
+ if (width != 16 && width != 24) {
dev_warn(>dev, "unsupported width, switching to 24bit\n");
- asrc_priv->asrc_width = 24;
+ width = 24;
}
+ if (width == 24)
+ asrc_priv->asrc_format = SNDRV_PCM_FORMAT_S24_LE;
+ else
+ asrc_priv->asrc_format = SNDRV_PCM_FORMAT_S16_LE;
+
platform_set_drvdata(pdev, asrc_priv);
pm_runtime_enable(>dev);
spin_lock_init(_priv->lock);
diff --git a/sound/soc/fsl/fsl_asrc.h b/sound/soc/fsl/fsl_asrc.h
index 8a821132d9d0..4940fa0a7542 100644
--- a/sound/soc/fsl/fsl_asrc.h
+++ b/sound/soc/fsl/fsl_asrc.h
@@ -493,7 +493,7 @@ struct fsl_asrc_pair {
* @channel_avail: non-occupied channel numbers
* @clk_map: clock map for input/output clock
* @asrc_rate: default sample rate for ASoC Back-Ends
- * @asrc_width: default sample width for ASoC Back-Ends
+ * @asrc_format: default sample format for ASoC Back-Ends
* @regcache_cfg: store register value of REG_ASRCFG
*/
struct fsl_asrc {
@@ -514,7 +514,7 @@ struct fsl_asrc {
unsigned char *clk_map[2];
int asrc_rate;
- int asrc_width;
+ snd_pcm_format_t asrc_format;
u32 regcache_cfg;
};
diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
index 44e5924be870..f344360dcd24 100644
--- a/sound/soc/fsl/fsl_asrc_dma.c
+++ b/sound/soc/fsl/fsl_asrc_dma.c
@@ -230,7 +230,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component
*component,
return -EINVAL;
}
- if (asrc_priv->asrc_width == 16)
+ if (asrc_priv->asrc_format == SNDRV_PCM_FORMAT_S16_LE)
buswidth = DMA_SLAVE_BUSWIDTH_2_BYTES;
else
buswidth = DMA_SLAVE_BUSWIDTH_4_BYTES;
--
2.21.0
