Re: [PATCH v2 3/3] ASoC: fsl_easrc: Add EASRC ASoC CPU DAI and platform drivers
Hi
> >
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/Kconfig | 10 +
> > sound/soc/fsl/Makefile |2 +
> > sound/soc/fsl/fsl_asrc_common.h |1 +
> > sound/soc/fsl/fsl_easrc.c | 2265 +++
> > sound/soc/fsl/fsl_easrc.h | 668 +
> > sound/soc/fsl/fsl_easrc_dma.c | 440 ++
>
> I see a 90% similarity between fsl_asrc_dma and fsl_easrc_dma files.
> Would it be possible reuse the existing code? Could share structures from
> my point of view, just like it reuses "enum asrc_pair_index", I know
> differentiating "pair" and "context" is a big point here though.
>
> A possible quick solution for that, off the top of my head, could be:
>
> 1) in fsl_asrc_common.h
>
> struct fsl_asrc {
>
> };
>
> struct fsl_asrc_pair {
>
> };
>
> 2) in fsl_easrc.h
>
> /* Renaming shared structures */
> #define fsl_easrc fsl_asrc
> #define fsl_easrc_context fsl_asrc_pair
>
> May be a good idea to see if others have some opinion too.
>
We need to modify the fsl_asrc and fsl_asrc_pair, let them
To be used by both driver, also we need to put the specific
Definition for each module to same struct, right?
>
> > +static const struct regmap_config fsl_easrc_regmap_config = {
> > + .readable_reg = fsl_easrc_readable_reg,
> > + .volatile_reg = fsl_easrc_volatile_reg,
> > + .writeable_reg = fsl_easrc_writeable_reg,
>
> Can we use regmap_range and regmap_access_table?
>
Can the regmap_range support discontinuous registers? The
reg_stride = 4.
Best regards
Wang shengjiu
Re: [alsa-devel] [PATCH V2 1/2] ASoC: dt-bindings: fsl_asrc: add compatible string for imx8qm
Hi Rob, Nicolin > > Hi Rob > > > > On Wed, Oct 30, 2019 at 07:41:26PM +0800, Shengjiu Wang wrote: > > > In order to support the two asrc modules in imx8qm, we need to add > > > compatible string "fsl,imx8qm-asrc0" and "fsl,imx8qm-asrc1" > > > > Are the blocks different in some way? > > > > If not, why do you need to distinguish them? > > > The internal clock mapping is different for each module. > > Or we can use one compatible string, but need add another property > "fsl,asrc-clk-map" to distinguish the different clock map. > > The change is in below. > > Which one do you think is better? > > Required properties: > > - - compatible : Contains "fsl,imx35-asrc" or "fsl,imx53-asrc". > + - compatible : Contains "fsl,imx35-asrc", "fsl,imx53-asrc", > + "fsl,imx8qm-asrc". > >- reg: Offset and length of the register set for > the device. > > @@ -35,6 +36,11 @@ Required properties: > > - fsl,asrc-width: Defines a mutual sample width used by DPCM Back > Ends. > > + - fsl,asrc-clk-map : Defines clock map used in driver. which is required > + by imx8qm > + <0> - select the map for asrc0 > + <1> - select the map for asrc1 > + > Optional properties: > I will do a update for this change in v3. Best regards Wang shengjiu
Re: [PATCH V2 1/2] ASoC: dt-bindings: fsl_asrc: add compatible string for imx8qm
Hi Rob > > On Wed, Oct 30, 2019 at 07:41:26PM +0800, Shengjiu Wang wrote: > > In order to support the two asrc modules in imx8qm, we need to add > > compatible string "fsl,imx8qm-asrc0" and "fsl,imx8qm-asrc1" > > Are the blocks different in some way? > > If not, why do you need to distinguish them? > The internal clock mapping is different for each module. Or we can use one compatible string, but need add another property "fsl,asrc-clk-map" to distinguish the different clock map. The change is in below. Which one do you think is better? Required properties: - - compatible : Contains "fsl,imx35-asrc" or "fsl,imx53-asrc". + - compatible : Contains "fsl,imx35-asrc", "fsl,imx53-asrc", + "fsl,imx8qm-asrc". - reg: Offset and length of the register set for the device. @@ -35,6 +36,11 @@ Required properties: - fsl,asrc-width: Defines a mutual sample width used by DPCM Back Ends. + - fsl,asrc-clk-map : Defines clock map used in driver. which is required + by imx8qm + <0> - select the map for asrc0 + <1> - select the map for asrc1 + Optional properties: Best regards Wang shengjiu
Re: [alsa-devel] [PATCH] ASoC: fsl_audmix: Add spin lock to protect tdms
Hi
>
> Hi Shengjiu,
>
> Comments inline.
>
> On Wed, Nov 6, 2019 at 9:30 AM Shengjiu Wang
> wrote:
> >
> > Audmix support two substream, When two substream start to run, the
> > trigger function may be called by two substream in same time, that the
> > priv->tdms may be updated wrongly.
> >
> > The expected priv->tdms is 0x3, but sometimes the result is 0x2, or
> > 0x1.
> >
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/fsl_audmix.c | 6 ++ sound/soc/fsl/fsl_audmix.h | 1
> > +
> > 2 files changed, 7 insertions(+)
> >
> > diff --git a/sound/soc/fsl/fsl_audmix.c b/sound/soc/fsl/fsl_audmix.c
> > index c7e4e9757dce..a1db1bce330f 100644
> > --- a/sound/soc/fsl/fsl_audmix.c
> > +++ b/sound/soc/fsl/fsl_audmix.c
> > @@ -286,6 +286,7 @@ static int fsl_audmix_dai_trigger(struct
> snd_pcm_substream *substream, int cmd,
> > struct snd_soc_dai *dai) {
> > struct fsl_audmix *priv = snd_soc_dai_get_drvdata(dai);
> > + unsigned long lock_flags;
> >
> > /* Capture stream shall not be handled */
> > if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> > @@ -295,12 +296,16 @@ static int fsl_audmix_dai_trigger(struct
> snd_pcm_substream *substream, int cmd,
> > case SNDRV_PCM_TRIGGER_START:
> > case SNDRV_PCM_TRIGGER_RESUME:
> > case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > + spin_lock_irqsave(>lock, lock_flags);
>
> Why do we need to disable interrupts here? I assume that lock is only
> used in process context.
>
It is in atomic context, so I think it is ok to disable interrupt.
Best regards
Wang shengjiu
Re: [PATCH V2 1/2] ASoC: dt-bindings: fsl_asrc: add compatible string for imx8qm
Hi Rob > > Hi > > > > On Wed, Oct 30, 2019 at 07:41:26PM +0800, Shengjiu Wang wrote: > > > In order to support the two asrc modules in imx8qm, we need to add > > > compatible string "fsl,imx8qm-asrc0" and "fsl,imx8qm-asrc1" > > > > Are the blocks different in some way? > > > > If not, why do you need to distinguish them? > > > The internal clock mapping is different for each module. > Or we can use one compatible string, but need add another property "fsl,asrc-clk-map" to distinguish the different clock map. The change is in below. Which one do you think is better? Required properties: - - compatible : Contains "fsl,imx35-asrc" or "fsl,imx53-asrc". + - compatible : Contains "fsl,imx35-asrc", "fsl,imx53-asrc", + "fsl,imx8qm-asrc". - reg: Offset and length of the register set for the device. @@ -35,6 +36,11 @@ Required properties: - fsl,asrc-width: Defines a mutual sample width used by DPCM Back Ends. + - fsl,asrc-clk-map : Defines clock map used in driver. which is required + by imx8qm + <0> - select the map for asrc0 + <1> - select the map for asrc1 + Optional properties: Best regards Wang shengjiu
RE: [EXT] Re: [PATCH V2 1/2] ASoC: dt-bindings: fsl_asrc: add compatible string for imx8qm
Hi > > On Wed, Oct 30, 2019 at 07:41:26PM +0800, Shengjiu Wang wrote: > > In order to support the two asrc modules in imx8qm, we need to add > > compatible string "fsl,imx8qm-asrc0" and "fsl,imx8qm-asrc1" > > Are the blocks different in some way? > > If not, why do you need to distinguish them? > The internal clock mapping is different for each module. Best regards Wang Shengjiu
Re: [PATCH 2/2] ASoC: fsl_asrc: Add support for imx8qm
Hi
>
> On Tue, Oct 29, 2019 at 05:17:09PM +0800, Shengjiu Wang wrote:
> > There are two asrc module in imx8qm, each module has different clock
> > configuration, and the DMA type is EDMA.
> >
> > So in this patch, we define the new clocks, refine the clock map, and
> > include struct fsl_asrc_soc_data for different soc usage.
> >
> > The EDMA channel is fixed with each dma request, one dma request
> > corresponding to one dma channel. So we need to request dma channel
> > with dma request of asrc module.
> >
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/fsl_asrc.c | 91 +---
> > sound/soc/fsl/fsl_asrc.h | 65 +-
> > sound/soc/fsl/fsl_asrc_dma.c | 39
> > 3 files changed, 167 insertions(+), 28 deletions(-)
>
> > diff --git a/sound/soc/fsl/fsl_asrc_dma.c
> > b/sound/soc/fsl/fsl_asrc_dma.c index d6146de9acd2..dbb07a486504
> 100644
> > --- a/sound/soc/fsl/fsl_asrc_dma.c
> > +++ b/sound/soc/fsl/fsl_asrc_dma.c
> > @@ -199,19 +199,40 @@ static int fsl_asrc_dma_hw_params(struct
> > snd_soc_component *component,
> >
> > /* Get DMA request of Back-End */
> > tmp_chan = dma_request_slave_channel(dev_be, tx ? "tx" : "rx");
> > - tmp_data = tmp_chan->private;
> > - pair->dma_data.dma_request = tmp_data->dma_request;
> > - dma_release_channel(tmp_chan);
> > + /* tmp_chan may be NULL for it is already allocated by Back-End */
> > + if (tmp_chan) {
> > + tmp_data = tmp_chan->private;
> > + if (tmp_data)
> > + pair->dma_data.dma_request =
> > + tmp_data->dma_request;
>
> If this patch is supposed to add a !tmp_chan case for EDMA, we probably
> shouldn't mute the !tmp_data case because dma_request will be NULL,
> although the code previously didn't have a check either. I mean we might
> need to error-out for !tmp_chan. Or...
> is this intentional?
>
Yes, intentional. May be we can change to
if (!asrc_priv->soc->use_edma) {
/* Get DMA request of Back-End */
tmp_chan = dma_request_slave_channel(dev_be, tx ? "tx" : "rx");
tmp_data = tmp_chan->private;
pair->dma_data.dma_request = tmp_data->dma_request;
dma_release_channel(tmp_chan);
/* Get DMA request of Front-End */
tmp_chan = fsl_asrc_get_dma_channel(pair, dir);
tmp_data = tmp_chan->private;
pair->dma_data.dma_request2 = tmp_data->dma_request;
pair->dma_data.peripheral_type = tmp_data->peripheral_type;
pair->dma_data.priority = tmp_data->priority;
dma_release_channel(tmp_chan);
}
Best regards
Wang shengjiu
Re: [PATCH V2] ASoC: fsl_esai: Add spin lock to protect reset, stop and start
Hi
>
> On Fri, Oct 25, 2019 at 03:13:53PM +0800, Shengjiu Wang wrote:
> > xrun may happen at the end of stream, the
> > trigger->fsl_esai_trigger_stop maybe called in the middle of
> > fsl_esai_hw_reset, this may cause esai in wrong state after stop, and
> > there may be endless xrun interrupt.
> >
> > This issue may also happen with trigger->fsl_esai_trigger_start.
> >
> > So Add spin lock to lock those functions.
> >
> > Fixes: 7ccafa2b3879 ("ASoC: fsl_esai: recover the channel swap after
> > xrun")
> > Signed-off-by: Shengjiu Wang
>
> Some small comments inline. Once they are addressed, please add:
>
> Acked-by: Nicolin Chen
>
> Thanks
>
> > ---
> > Change in v2
> > -add lock for fsl_esai_trigger_start.
> >
> > sound/soc/fsl/fsl_esai.c | 12
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> > 37b14c48b537..9b28e2af26e4 100644
> > --- a/sound/soc/fsl/fsl_esai.c
> > +++ b/sound/soc/fsl/fsl_esai.c
> > @@ -33,6 +33,7 @@
> > * @fsysclk: system clock source to derive HCK, SCK and FS
> > * @spbaclk: SPBA clock (optional, depending on SoC design)
> > * @task: tasklet to handle the reset operation
> > + * @lock: spin lock to handle reset and stop behavior
>
> Should be "between hw_reset() and trigger()" now.
>
> > * @fifo_depth: depth of tx/rx FIFO
> > * @slot_width: width of each DAI slot
> > * @slots: number of slots
> > @@ -56,6 +57,7 @@ struct fsl_esai {
> > struct clk *fsysclk;
> > struct clk *spbaclk;
> > struct tasklet_struct task;
> > + spinlock_t lock; /* Protect reset and stop */
>
> We can drop the comments here since you add it to the top.
Here is required by the checkpatch.pl, so still is needed.
Best regards
Wang shengjiu
Re: [PATCH] ASoC: fsl_esai: Add spin lock to protect reset and stop
Hi > > On Wed, Oct 23, 2019 at 03:29:49PM +0800, Shengjiu Wang wrote: > > xrun may happen at the end of stream, the > > trigger->fsl_esai_trigger_stop maybe called in the middle of > > fsl_esai_hw_reset, this may cause esai in wrong state after stop, and > > there may be endless xrun interrupt. > > What about fsl_esai_trigger_start? It touches ESAI_xFCR_xFEN bit that is > being checked in the beginning of fsl_esai_hw_reset. > > Could the scenario below be possible also? > > 1) ESAI TX starts > 2) Xrun happens to TX > 3) Starting fsl_esai_hw_reset (enabled[TX] = true; enabled[RX] = false) > 4) ESAI RX starts > 5) Finishing fsl_esai_hw_reset (enabled[RX] is still false) > > Good catch, this may possible. Will update in v2. Best regards Wang shengjiu
Re: [PATCH] ASoC: fsl_asrc: refine the setting of internal clock divider
Hi
>
> On Wed, Oct 23, 2019 at 06:25:20AM +0000, S.j. Wang wrote:
> > > On Thu, Oct 17, 2019 at 02:21:08PM +0800, Shengjiu Wang wrote:
> > > > For P2P output, the output divider should align with the output
> > > > sample
> > >
> > > I think we should avoid "P2P" (or "M2M") keyword in the mainline
> > > code as we know M2M will never get merged while somebody working
> > > with the mainline and caring about new feature might be confused.
> >
> > Ok. But we still curious that is there a way to upstream m2m?
>
> Hmm..I would love to see that happening. Here is an old discussion that
> you may want to take a look:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail
> man.alsa-project.org%2Fpipermail%2Falsa-devel%2F2014-
> May%2F076797.htmldata=02%7C01%7Cshengjiu.wang%40nxp.com%7
> Ce902d2bac4254d2faa0f08d757ecac0e%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C637074546320396681sdata=bg%2BLwRQnUPhW8f
> mE972O%2F53MyVftJkK140PSnmC%2FDKQ%3Dreserved=0
>
> > > It makes sense to me, yet I feel that the delay at the beginning of
> > > the audio playback might be longer as a compromise. I am okay with
> > > this decision though...
> > >
> > > > The maximum divider of asrc clock is 1024, but there is no
> > > > judgement for this limitaion in driver, which may cause the
> > > > divider setting not correct.
> > > >
> > > > For non-ideal ratio mode, the clock rate should divide the sample
> > > > rate with no remainder, and the quotient should be less than 1024.
> > > >
> > > > Signed-off-by: Shengjiu Wang
>
> > > > @@ -351,7 +352,9 @@ static int fsl_asrc_config_pair(struct
> > > > fsl_asrc_pair
> > > *pair)
> > > > /* We only have output clock for ideal ratio mode */
> > > > clk = asrc_priv->asrck_clk[clk_index[ideal ? OUT : IN]];
> > > >
> > > > - div[IN] = clk_get_rate(clk) / inrate;
> > > > + clk_rate = clk_get_rate(clk);
> > >
> > > The fsl_asrc.c file has config.inclk being set to INCLK_NONE and
> > > this sets the "ideal" in this function to true. So, although we tend
> > > to not use ideal ratio setting for p2p cases, yet the input clock is
> > > still not physically connected, so we still use output clock for div[IN]
> calculation?
> >
> > For p2p case, it can be ideal or non-ideal. For non-ideal, we still
> > use Output clock for div calculation.
> >
> > >
> > > I am thinking something simplier: if we decided not to use ideal
> > > ratio for "P2P", instead of adding "bool p2p" with the confusing
> > > "ideal" in this function, could we just set config.inclk to the same
> > > clock as the output one for "P2P"? By doing so, "P2P" won't go
> > > through ideal ratio mode while still having a clock rate from the output
> clock for div[IN] calculation here.
> >
> > Bool p2p is to force output rate to be sample rate, no impact to ideal
> > Ratio mode.
>
> I just realized that the function has a bottom part for ideal mode
> exclusively -- if we treat p2p as !ideal, those configurations will be
> missing.
> So you're right, should have an extra boolean variable.
>
> > >
> > > > + rem[IN] = do_div(clk_rate, inrate);
> > > > + div[IN] = (u32)clk_rate;
> > > > if (div[IN] == 0) {
> > >
> > > Could we check div[IN] and rem[IN] here? Like:
> > > if (div[IN] == 0 || div[IN] > 1024) {
> > > pair_err();
> > > goto out;
> > > }
> > >
> > > if (!ideal && rem[IN]) {
> > > pair_err();
> > > goto out;
> > > }
> > >
> > > According to your commit log, I think the max-1024 limitation should
> > > be applied to all cases, not confined to "!ideal" cases right? And
> > > we should add some comments also, indicating it is limited by hardware.
> >
> > For ideal mode, my test result is the divider not impact the output
> > result.
> > Which means it is ok for ideal mode even divider is not correct...
>
> OK.
>
> > >
> > > > pair_err("failed to support input sample rate %dHz
> > > > by
> > > asrck_%x\n",
> > > > inrate, clk_index[ideal ? OUT :
> > >
Re: [PATCH] ASoC: fsl_asrc: refine the setting of internal clock divider
Hi
>
> On Thu, Oct 17, 2019 at 02:21:08PM +0800, Shengjiu Wang wrote:
> > For P2P output, the output divider should align with the output sample
>
> I think we should avoid "P2P" (or "M2M") keyword in the mainline code as
> we know M2M will never get merged while somebody working with the
> mainline and caring about new feature might be confused.
Ok. But we still curious that is there a way to upstream m2m?
>
> > rate, if use ideal sample rate, there will be a lot of overload, which
> > would cause underrun.
>
> If I understand it correctly, setting to ideal ratio provides a faster
> converting
> speed but increases the load of the processor of ASRC. So we choose a
> slower converting speed here since real- time playback mode doesn't really
> need a faster conversion?
Yes. Slower speed is enough for real-time playback
>
> It makes sense to me, yet I feel that the delay at the beginning of the audio
> playback might be longer as a compromise. I am okay with this decision
> though...
>
> > The maximum divider of asrc clock is 1024, but there is no judgement
> > for this limitaion in driver, which may cause the divider setting not
> > correct.
> >
> > For non-ideal ratio mode, the clock rate should divide the sample rate
> > with no remainder, and the quotient should be less than 1024.
> >
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/fsl_asrc.c | 40
> > +++-
> > 1 file changed, 31 insertions(+), 9 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index
> > 0bf91a6f54b9..44d05ec28bd3 100644
> > --- a/sound/soc/fsl/fsl_asrc.c
> > +++ b/sound/soc/fsl/fsl_asrc.c
> > @@ -260,7 +260,7 @@ static int fsl_asrc_set_ideal_ratio(struct
> fsl_asrc_pair *pair,
> > * of struct asrc_config which includes in/output sample rate, width,
> channel
> > * and clock settings.
> > */
> > -static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
> > +static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool p2p)
> > {
> > struct asrc_config *config = pair->config;
> > struct fsl_asrc *asrc_priv = pair->asrc_priv; @@ -268,7 +268,8
> > @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
> > 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];
> > + u32 clk_index[2], div[2], rem[2];
> > + u64 clk_rate;
> > int in, out, channels;
> > int pre_proc, post_proc;
> > struct clk *clk;
> > @@ -351,7 +352,9 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
> *pair)
> > /* We only have output clock for ideal ratio mode */
> > clk = asrc_priv->asrck_clk[clk_index[ideal ? OUT : IN]];
> >
> > - div[IN] = clk_get_rate(clk) / inrate;
> > + clk_rate = clk_get_rate(clk);
>
> The fsl_asrc.c file has config.inclk being set to INCLK_NONE and this sets the
> "ideal" in this function to true. So, although we tend to not use ideal ratio
> setting for p2p cases, yet the input clock is still not physically connected,
> so
> we still use output clock for div[IN] calculation?
For p2p case, it can be ideal or non-ideal. For non-ideal, we still use
Output clock for div calculation.
>
> I am thinking something simplier: if we decided not to use ideal ratio for
> "P2P", instead of adding "bool p2p" with the confusing "ideal" in this
> function, could we just set config.inclk to the same clock as the output one
> for "P2P"? By doing so, "P2P" won't go through ideal ratio mode while still
> having a clock rate from the output clock for div[IN] calculation here.
Bool p2p is to force output rate to be sample rate, no impact to ideal
Ratio mode.
>
> > + rem[IN] = do_div(clk_rate, inrate);
> > + div[IN] = (u32)clk_rate;
> > if (div[IN] == 0) {
>
> Could we check div[IN] and rem[IN] here? Like:
> if (div[IN] == 0 || div[IN] > 1024) {
> pair_err();
> goto out;
> }
>
> if (!ideal && rem[IN]) {
> pair_err();
> goto out;
> }
>
> According to your commit log, I think the max-1024 limitation should be
> applied to all cases, not confined to "!ideal" cases right? And we should
> add some comments also, indicating it is limited by hardware.
For ideal mode, my test result is the divider not impact the output result.
Which means it is ok for ideal mode even divider is not correct...
>
> > pair_err("failed to support input sample rate %dHz by
> asrck_%x\n",
> > inrate, clk_index[ideal ? OUT : IN]); @@
> > -360,11 +363,20 @@ static int fsl_asrc_config_pair(struct
> > fsl_asrc_pair *pair)
> >
> > clk = asrc_priv->asrck_clk[clk_index[OUT]];
> >
> > - /* Use fixed output rate for Ideal Ratio mode (INCLK_NONE) */
> > - if (ideal)
> > - div[OUT] = clk_get_rate(clk) / IDEAL_RATIO_RATE;
Re: [PATCH V5 4/4] ASoC: fsl_asrc: Fix error with S24_3LE format bitstream in i.MX8
Hi
> Just a small concern...
>
> On Thu, Sep 26, 2019 at 09:29:51AM +0800, Shengjiu Wang wrote:
> > static int fsl_asrc_dma_startup(struct snd_pcm_substream *substream)
> > {
> > +
> > + release_pair = false;
> > + ret = snd_soc_set_runtime_hwparams(substream,
> > + _imx_hardware);
>
> This set_runtime_hwparams() always returns 0 for now, but if one day it
> changes and it fails here, kfree() will be still ignored although the
> startup()
> gets error-out.
>
> We could avoid this if we continue to ignore the return value like the
> current code. Or we may check ret at kfree() also?
I like to ignore the return value.
Best regards
Wang shengjiu
Re: [PATCH V4 4/4] ASoC: fsl_asrc: Fix error with S24_3LE format bitstream in i.MX8
Hi
> On Tue, Sep 24, 2019 at 06:52:35PM +0800, Shengjiu Wang wrote:
> > There is error "aplay: pcm_write:2023: write error: Input/output error"
> > on i.MX8QM/i.MX8QXP platform for S24_3LE format.
> >
> > In i.MX8QM/i.MX8QXP, the DMA is EDMA, which don't support 24bit
> > sample, but we didn't add any constraint, that cause issues.
> >
> > So we need to query the caps of dma, then update the hw parameters
> > according to the caps.
> >
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/fsl_asrc.c | 4 +--
> > sound/soc/fsl/fsl_asrc.h | 3 ++
> > sound/soc/fsl/fsl_asrc_dma.c | 59
> > +++-
> > 3 files changed, 56 insertions(+), 10 deletions(-)
> >
> > @@ -270,12 +268,17 @@ static int fsl_asrc_dma_hw_free(struct
> > snd_pcm_substream *substream)
> >
> > static int fsl_asrc_dma_startup(struct snd_pcm_substream *substream)
> > {
> > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> > struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > struct snd_pcm_runtime *runtime = substream->runtime;
> > struct snd_soc_component *component =
> snd_soc_rtdcom_lookup(rtd,
> > DRV_NAME);
> > + struct snd_dmaengine_dai_dma_data *dma_data;
> > struct device *dev = component->dev;
> > struct fsl_asrc *asrc_priv = dev_get_drvdata(dev);
> > struct fsl_asrc_pair *pair;
> > + struct dma_chan *tmp_chan = NULL;
> > + u8 dir = tx ? OUT : IN;
> > + int ret = 0;
> >
> > pair = kzalloc(sizeof(struct fsl_asrc_pair), GFP_KERNEL);
>
> Sorry, I didn't catch it previously. We would need to release this memory
> also for all error-out paths, as the code doesn't have any error-out routine,
> prior to applying this change.
>
> > if (!pair)
> > @@ -285,11 +288,51 @@ static int fsl_asrc_dma_startup(struct
> > snd_pcm_substream *substream)
>
> > + /* Request a dummy pair, which will be released later.
> > + * Request pair function needs channel num as input, for this
> > + * dummy pair, we just request "1" channel temporary.
> > + */
>
> "temporary" => "temporarily"
>
> > + ret = fsl_asrc_request_pair(1, pair);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to request asrc pair\n");
> > + return ret;
> > + }
> > +
> > + /* Request a dummy dma channel, which will be release later. */
>
> "release" => "released"
Ok, will update them.
Best regards
Wang shengjiu
Re: [PATCH V3 4/4] ASoC: fsl_asrc: Fix error with S24_3LE format bitstream in i.MX8
Hi
>
> One issue for error-out and some nit-pickings inline. Thanks.
>
> On Thu, Sep 19, 2019 at 08:11:42PM +0800, Shengjiu Wang wrote:
> > There is error "aplay: pcm_write:2023: write error: Input/output error"
> > on i.MX8QM/i.MX8QXP platform for S24_3LE format.
> >
> > In i.MX8QM/i.MX8QXP, the DMA is EDMA, which don't support 24bit
> > sample, but we didn't add any constraint, that cause issues.
> >
> > So we need to query the caps of dma, then update the hw parameters
> > according to the caps.
> >
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/fsl_asrc.c | 4 +--
> > sound/soc/fsl/fsl_asrc.h | 3 +++
> > sound/soc/fsl/fsl_asrc_dma.c | 52
> > +++-
> > 3 files changed, 50 insertions(+), 9 deletions(-)
> >
> > @@ -276,6 +274,11 @@ static int fsl_asrc_dma_startup(struct
> snd_pcm_substream *substream)
> > struct device *dev = component->dev;
> > struct fsl_asrc *asrc_priv = dev_get_drvdata(dev);
> > struct fsl_asrc_pair *pair;
> > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> > + u8 dir = tx ? OUT : IN;
> > + struct dma_chan *tmp_chan;
> > + struct snd_dmaengine_dai_dma_data *dma_data;
>
> Nit: would it be possible to reorganize these a bit? Usually we put struct
> things together unless there is a dependency, similar to
> fsl_asrc_dma_hw_params().
>
> > @@ -285,9 +288,44 @@ static int fsl_asrc_dma_startup(struct
> > snd_pcm_substream *substream)
> >
> > runtime->private_data = pair;
> >
> > + /* Request a temp pair, which is release in the end */
>
> Nit: "which will be released later" or "and will release it later"? And could
> we use a work like "dummy"? Or at least I would love to see the comments
> explaining the parameter "1"
> in the function call below.
>
> > + ret = fsl_asrc_request_pair(1, pair);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to request asrc pair\n");
> > + return ret;
> > + }
> > +
> > + tmp_chan = fsl_asrc_get_dma_channel(pair, dir);
> > + if (!tmp_chan) {
> > + dev_err(dev, "can't get dma channel\n");
>
> Could we align with other error messages using "failed to"?
>
> > + ret = snd_soc_set_runtime_hwparams(substream,
> _imx_hardware);
> > + if (ret)
> > + return ret;
> > +
> [...]
> > + dma_release_channel(tmp_chan);
> > + fsl_asrc_release_pair(pair);
>
> I think we need an "out:" here for those error-out routines to goto.
> Otherwise, it'd be a pair leak?
>
> > +
>
> Could we drop this? There is a blank line below already :)
>
Will update them.
Best regards
Wang Shengjiu
RE: [PATCH V2 3/4] ASoC: pcm_dmaengine: Extract snd_dmaengine_pcm_set_runtime_hwparams
Hi > > When set the runtime hardware parameters, we may need to query the > capability of DMA to complete the parameters. > > This patch is to Extract this operation from > dmaengine_pcm_set_runtime_hwparams function to a separate function > snd_dmaengine_pcm_set_runtime_hwparams, that other components > which need this feature can call this function. > > Signed-off-by: Shengjiu Wang > --- kbuild test robot report compile issue, will resend v3 after fixing. Best regards Wang shengjiu
Re: [PATCH 2/3] ASoC: fsl_asrc: update supported sample format
Hi > > On Fri, Sep 13, 2019 at 05:48:40AM +0000, S.j. Wang wrote: > > Hi > > > > > > > > On Tue, Sep 10, 2019 at 02:07:25AM +, S.j. Wang wrote: > > > > > On Mon, Sep 09, 2019 at 06:33:20PM -0400, Shengjiu Wang wrote: > > > > > > The ASRC support 24bit/16bit/8bit input width, so S20_3LE > > > > > > format should not be supported, it is word width is 20bit. > > > > > > > > > > I thought 3LE used 24-bit physical width. And the driver assigns > > > > > ASRC_WIDTH_24_BIT to "width" for all non-16bit cases, so 20-bit > > > > > would go for that 24-bit slot also. I don't clearly recall if I > > > > > had explicitly tested S20_3LE, but I feel it should work since I put > there... > > > > > > > > For S20_3LE, the width is 20bit, but the ASRC only support 24bit, > > > > if set the ASRMCR1n.IWD= 24bit, because the actual width is 20 > > > > bit, the volume is Lower than expected, it likes 24bit data right > > > > shift 4 > bit. > > > > So it is not supported. > > > > > > Hmm..S20_3LE right-aligns 20 bits in a 24-bit slot? I thought > > > they're left aligned... > > > > > > If this is the case...shouldn't we have the same lower-volume > > > problem for all hardwares that support S20_3LE now? > > > > Actually some hardware/module when they do transmission from FIFO to > > shift register, they can select the start bit, for example from the > > 20th bit. but not all module have this capability. > > > > For ASRC, it haven't. IWD can only cover the data width, there is no > > Other bit for slot width. > > Okay..let's drop the S20_3LE then. But would it be possible for you to > elaborate the reasoning into the commit message also? Just for case when > people ask why we remove it simply. > > Thanks OK. Best regards Wang shengjiu
RE: [EXT] Re: [PATCH 2/3] ASoC: fsl_asrc: update supported sample format
Hi > > On Tue, Sep 10, 2019 at 02:07:25AM +0000, S.j. Wang wrote: > > > On Mon, Sep 09, 2019 at 06:33:20PM -0400, Shengjiu Wang wrote: > > > > The ASRC support 24bit/16bit/8bit input width, so S20_3LE format > > > > should not be supported, it is word width is 20bit. > > > > > > I thought 3LE used 24-bit physical width. And the driver assigns > > > ASRC_WIDTH_24_BIT to "width" for all non-16bit cases, so 20-bit > > > would go for that 24-bit slot also. I don't clearly recall if I had > > > explicitly tested S20_3LE, but I feel it should work since I put there... > > > > For S20_3LE, the width is 20bit, but the ASRC only support 24bit, if > > set the ASRMCR1n.IWD= 24bit, because the actual width is 20 bit, the > > volume is Lower than expected, it likes 24bit data right shift 4 bit. > > So it is not supported. > > Hmm..S20_3LE right-aligns 20 bits in a 24-bit slot? I thought they're left > aligned... > > If this is the case...shouldn't we have the same lower-volume problem for > all hardwares that support S20_3LE now? Actually some hardware/module when they do transmission from FIFO to shift register, they can select the start bit, for example from the 20th bit. but not all module have this capability. For ASRC, it haven't. IWD can only cover the data width, there is no Other bit for slot width. Best regards Wang shengjiu
RE: [EXT] Re: [PATCH 3/3] ASoC: fsl_asrc: Fix error with S24_3LE format bitstream in i.MX8
Hi
>
> On Mon, Sep 09, 2019 at 06:33:21PM -0400, Shengjiu Wang wrote:
> > There is error "aplay: pcm_write:2023: write error: Input/output error"
> > on i.MX8QM/i.MX8QXP platform for S24_3LE format.
> >
> > In i.MX8QM/i.MX8QXP, the DMA is EDMA, which don't support 24bit
> > sample, but we didn't add any constraint, that cause issues.
> >
> > So we need to query the caps of dma, then update the hw parameters
> > according to the caps.
>
> > @@ -285,8 +293,81 @@ static int fsl_asrc_dma_startup(struct
> > snd_pcm_substream *substream)
> >
> > runtime->private_data = pair;
> >
> > - snd_pcm_hw_constraint_integer(substream->runtime,
> > - SNDRV_PCM_HW_PARAM_PERIODS);
> > + ret = snd_pcm_hw_constraint_integer(substream->runtime,
> > + SNDRV_PCM_HW_PARAM_PERIODS);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to set pcm hw params periods\n");
> > + return ret;
> > + }
> > +
> > + dma_data = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
> > +
> > + /* Request a temp pair, which is release in the end */
> > + fsl_asrc_request_pair(1, pair);
>
> Not sure if it'd be practical, but a pair request could fail. Will probably
> need
> to check return value.
>
> And a quick feeling is that below code is mostly identical to what is in the
> soc-generic-dmaengine-pcm.c file. So I'm wondering if we could abstract a
> helper function somewhere in the ASoC core: Mark?
>
> Thanks
> Nicolin
>
Yes, it refers to the code in soc-generic-dmaengine-pcm.c, if there is a common
API, this is helpful.
Best regards
Wang shengjiu
RE: [EXT] Re: [PATCH 1/3] ASoC: fsl_asrc: Use in(out)put_format instead of in(out)put_word_width
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
Re: [PATCH 2/3] ASoC: fsl_asrc: update supported sample format
Hi > > On Mon, Sep 09, 2019 at 06:33:20PM -0400, Shengjiu Wang wrote: > > The ASRC support 24bit/16bit/8bit input width, so S20_3LE format > > should not be supported, it is word width is 20bit. > > I thought 3LE used 24-bit physical width. And the driver assigns > ASRC_WIDTH_24_BIT to "width" for all non-16bit cases, so 20-bit would go > for that 24-bit slot also. I don't clearly recall if I had explicitly tested > S20_3LE, but I feel it should work since I put there... > > Thanks > Nicolin > For S20_3LE, the width is 20bit, but the ASRC only support 24bit, if set the ASRMCR1n.IWD= 24bit, because the actual width is 20 bit, the volume is Lower than expected, it likes 24bit data right shift 4 bit. So it is not supported. Best regards Wang shengjiu
Re: [PATCH] ASoC: imx-audmux: Add driver suspend and resume to support MEGA Fast
Hi Mark > > On Fri, Aug 16, 2019 at 01:03:14AM -0400, Shengjiu Wang wrote: > > > + for (i = 0; i < reg_max; i++) > > + regcache[i] = readl(audmux_base + i * 4); > > If only there were some framework which provided a register cache! Yes, next step I can refine this driver to use the regmap. Best regards Wang shengjiu
Re: [PATCH V3 2/2] ASoC: fsl_esai: recover the channel swap after xrun
>
> Hi Shengjiu,
>
> Mostly looks good to me, just some small comments.
>
> On Mon, Jul 08, 2019 at 02:38:52PM +0800, shengjiu.w...@nxp.com wrote:
>
> > +static void fsl_esai_hw_reset(unsigned long arg) {
> > + struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
> > + u32 saisr, tfcr, rfcr;
> > + bool tx = true, rx = false, enabled[2];
>
> Could we swap the lines of u32 and bool? It'd look better.
>
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > +ESAI_xCR_xPR_MASK, ESAI_xCR_xPR);
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> > +ESAI_xCR_xPR_MASK, ESAI_xCR_xPR);
>
> Let's add a line of comments for these two:
> /* Enforce ESAI personal resets for both TX and RX */
>
> > + /*
> > + * Restore registers by regcache_sync, and ignore
> > + * return value
> > + */
>
> Could fit into single-line?
>
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > +ESAI_xCR_xPR_MASK, 0);
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> > +ESAI_xCR_xPR_MASK, 0);
> > +
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
> > +ESAI_PRRC_PDC_MASK, ESAI_PRRC_PDC(ESAI_GPIO));
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_PCRC,
> > +ESAI_PCRC_PC_MASK, ESAI_PCRC_PC(ESAI_GPIO));
>
> Could remove the blank line and add a line of comments:
> /* Remove ESAI personal resets by configuring PCRC and PRRC also */
>
> Btw, I still feel this personal reset can be stuffed into one of the wrapper
> functions. But let's keep this simple for now.
>
> > + regmap_read(esai_priv->regmap, REG_ESAI_SAISR, );
>
> Why do we read saisr here? All its bits would get cleared by the hardware
> reset. If it's a must to clear again, we should add a line of comments to
> emphasize it.
This line can be removed.
Best regards
Wang Shengjiu
Re: [PATCH V2 2/2] ASoC: fsl_esai: recover the channel swap after xrun
> > > > + > > > > + /* restore registers by regcache_sync */ > > > > + fsl_esai_register_restore(esai_priv); > > > > + > > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, > > > > +ESAI_xCR_xPR_MASK, 0); > > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR, > > > > +ESAI_xCR_xPR_MASK, 0); > > > > > > And just for curious, can (or shall) we stuff this personal reset to > > > the reset() function? I found this one is a part of the reset > > > routine being mentioned in the RM -- it was done after ESAI reset is > done via ECR register. > > > > > > > There is a problem to do this, TPR/RPR need to be clear after > > configure the control register. (TCCR, TCR). So it seems not only one > > place (reset function) need to be changed. > > Do you know (or remember) why we suddenly involve this TPR/PRP? > The driver has no problem so far, even if we don't have them. > > The "personal reset" sounds like a feature that we would use to reset TX or > RX individually, while this hw_reset() does a full reset for both TX and RX. > So I wonder whether they're necessary. The hw_reset flow is suggested by design team, so involve TRP/RPP is from them, I don't know the detail. Best regards Wang shengjiu
Re: [PATCH V2 2/2] ASoC: fsl_esai: recover the channel swap after xrun
> > > + > > + /* restore registers by regcache_sync */ > > + fsl_esai_register_restore(esai_priv); > > + > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, > > +ESAI_xCR_xPR_MASK, 0); > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR, > > +ESAI_xCR_xPR_MASK, 0); > > And just for curious, can (or shall) we stuff this personal reset to the > reset() > function? I found this one is a part of the reset routine being mentioned in > the RM -- it was done after ESAI reset is done via ECR register. > There is a problem to do this, TPR/RPR need to be clear after configure the control register. (TCCR, TCR). So it seems not only one place (reset function) need to be changed. Best regards Wang shengjiu
RE: [RFC/RFT PATCH v2] ASoC: fsl_esai: Revert "ETDR and TX0~5 registers are non volatile"
Hi
>
> Commit 8973112aa41b ("ASoC: fsl_esai: ETDR and TX0~5 registers are non
> volatile") removed TX data registers from the volatile_reg list and appended
> default values for them. However, being data registers of TX, they should
> not have been removed from the list because they should not be cached --
> see the following reason.
>
> When doing regcache_sync(), this operation might accidentally write some
> dirty data to these registers, in case that cached data happen to be
> different from the default ones, which might also result in a channel shift or
> swap situation, since the number of write-via-sync operations at ETDR
> would very unlikely match the channel number.
>
> So this patch reverts the original commit to keep TX data registers in
> volatile_reg list in order to prevent them from being written by
> regcache_sync().
>
> Note: this revert is not a complete revert as it keeps those macros of
> registers remaining in the default value list while the original commit also
> changed other entries in the list. And this patch isn't very necessary to Cc
> stable tree since there has been always a FIFO reset operation around the
> regcache_sync() call, even prior to this reverted commit.
>
> Signed-off-by: Nicolin Chen
> Cc: Shengjiu Wang
> ---
> Hi Mark,
> In case there's no objection against the patch, I'd still like to wait for a
> Tested-by from NXP folks before submitting it. Thanks!
bool regmap_volatile(struct regmap *map, unsigned int reg)
{
if (!map->format.format_write && !regmap_readable(map, reg))
return false;
Actually with this patch, the regcache_sync will write the 0 to ETDR, even
It is declared volatile, the reason is that in regmap_volatile(), the first
condition
(!map->format.format_write && !regmap_readable(map, reg)) is true.
So the regmap_volatile will return false.
And in regcache_reg_needs_sync(), because there is no default value
It will return true, then the ETDR need be synced, and be written 0.
Here is the code for regcache_default_sync()
static int regcache_default_sync(struct regmap *map, unsigned int min,
unsigned int max)
{
unsigned int reg;
for (reg = min; reg <= max; reg += map->reg_stride) {
unsigned int val;
int ret;
if (regmap_volatile(map, reg) ||
!regmap_writeable(map, reg))
continue;
ret = regcache_read(map, reg, );
if (ret)
return ret;
if (!regcache_reg_needs_sync(map, reg, val))
continue;
map->cache_bypass = true;
ret = _regmap_write(map, reg, val);
map->cache_bypass = false;
Best regards
Wang shengjiu
Re: [PATCH] ASoC: fsl_esai: fix the channel swap issue after xrun
Hi > > > > > Sounds like a bug to me...should fix it first by marking the > > > > > data registers as volatile. > > > > > > > > > The ETDR is a writable register, it is not volatile. Even we > > > > change it to Volatile, I don't think we can't avoid this issue. > > > > for the regcache_sync Just to write this register, it is correct > > > > behavior. > > > > > > Is that so? Quoting the comments of regcache_sync(): > > > "* regcache_sync - Sync the register cache with the hardware. > > > * > > > * @map: map to configure. > > > * > > > * Any registers that should not be synced should be marked as > > > * volatile." > > > > > > If regcache_sync() does sync volatile registers too as you said, I > > > don't mind having this FIFO reset WAR for now, though I think this > > > mismatch between the comments and the actual behavior then should > get people's attention. > > > > > > Thank you > > > > ETDR is not volatile, if we mark it is volatile, is it correct? > > Well, you have a point -- it might not be ideally true, but it sounds like a > correct fix to me according to this comments. > > We can wait for Mark's comments or just send a patch to the mail list for > review. > > Thanks you I test this patch, we don't need to reset the FIFO, and regcache_sync didn't Write the ETDR even the EDTR is not volatile. This fault maybe caused by Legacy, in the beginning we add this patch in internal branch, there maybe Something cause this issue, but now can't reproduced. So I will remove the reset of FIFO. Best regards Wang Shengjiu
Re: [PATCH] ASoC: fsl_esai: fix the channel swap issue after xrun
Hi > On Thu, May 23, 2019 at 09:53:42AM +0000, S.j. Wang wrote: > > > > + /* > > > > + * Add fifo reset here, because the regcache_sync will > > > > + * write one more data to ETDR. > > > > + * Which will cause channel shift. > > > > > > Sounds like a bug to me...should fix it first by marking the data > > > registers as volatile. > > > > > The ETDR is a writable register, it is not volatile. Even we change it > > to Volatile, I don't think we can't avoid this issue. for the > > regcache_sync Just to write this register, it is correct behavior. > > Is that so? Quoting the comments of regcache_sync(): > "* regcache_sync - Sync the register cache with the hardware. > * > * @map: map to configure. > * > * Any registers that should not be synced should be marked as > * volatile." > > If regcache_sync() does sync volatile registers too as you said, I don't mind > having this FIFO reset WAR for now, though I think this mismatch between > the comments and the actual behavior then should get people's attention. > > Thank you ETDR is not volatile, if we mark it is volatile, is it correct? Bets regards Wang shengjiu
Re: [PATCH] ASoC: fsl_esai: fix the channel swap issue after xrun
Hi > > + /* > > + * Add fifo reset here, because the regcache_sync will > > + * write one more data to ETDR. > > + * Which will cause channel shift. > > Sounds like a bug to me...should fix it first by marking the data registers as > volatile. > The ETDR is a writable register, it is not volatile. Even we change it to Volatile, I don't think we can't avoid this issue. for the regcache_sync Just to write this register, it is correct behavior. Best regards Wang shengjiu
[PATCH] ASoC: fsl_esai: fix the channel swap issue after xrun
There is chip errata ERR008000, the reference doc is
(https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf),
The issue is "While using ESAI transmit or receive and
an underrun/overrun happens, channel swap may occur.
The only recovery mechanism is to reset the ESAI."
In this commit add a tasklet to handle reset of ESAI
after xrun happens
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_esai.c | 166 +++
1 file changed, 166 insertions(+)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 10d2210c91ef..149972894c95 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -52,17 +52,20 @@ struct fsl_esai {
struct clk *extalclk;
struct clk *fsysclk;
struct clk *spbaclk;
+ struct tasklet_struct task;
u32 fifo_depth;
u32 slot_width;
u32 slots;
u32 tx_mask;
u32 rx_mask;
+ u32 tx_channels;
u32 hck_rate[2];
u32 sck_rate[2];
bool hck_dir[2];
bool sck_div[2];
bool slave_mode;
bool synchronous;
+ bool reset_at_xrun;
char name[32];
};
@@ -71,8 +74,14 @@ static irqreturn_t esai_isr(int irq, void *devid)
struct fsl_esai *esai_priv = (struct fsl_esai *)devid;
struct platform_device *pdev = esai_priv->pdev;
u32 esr;
+ u32 saisr;
regmap_read(esai_priv->regmap, REG_ESAI_ESR, );
+ regmap_read(esai_priv->regmap, REG_ESAI_SAISR, );
+
+ if ((saisr & (ESAI_SAISR_TUE | ESAI_SAISR_ROE))
+ && esai_priv->reset_at_xrun)
+ tasklet_schedule(_priv->task);
if (esr & ESAI_ESR_TINIT_MASK)
dev_dbg(>dev, "isr: Transmission Initialized\n");
@@ -552,6 +561,9 @@ static int fsl_esai_trigger(struct snd_pcm_substream
*substream, int cmd,
u32 pins = DIV_ROUND_UP(channels, esai_priv->slots);
u32 mask;
+ if (tx)
+ esai_priv->tx_channels = channels;
+
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_RESUME:
@@ -585,10 +597,16 @@ static int fsl_esai_trigger(struct snd_pcm_substream
*substream, int cmd,
regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
+ ESAI_xCR_xEIE_MASK, ESAI_xCR_xEIE);
+
break;
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
+ ESAI_xCR_xEIE_MASK, 0);
+
regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK, 0);
regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
@@ -618,6 +636,145 @@ static const struct snd_soc_dai_ops fsl_esai_dai_ops = {
.set_tdm_slot = fsl_esai_set_dai_tdm_slot,
};
+static void fsl_esai_reset(unsigned long arg)
+{
+ struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
+ u32 saisr;
+ u32 tsma, tsmb, rsma, rsmb, tcr, rcr, tfcr, rfcr;
+ int i;
+
+ /*
+* stop the tx & rx
+*/
+ regmap_read(esai_priv->regmap, REG_ESAI_TSMA, );
+ regmap_read(esai_priv->regmap, REG_ESAI_TSMB, );
+ regmap_read(esai_priv->regmap, REG_ESAI_RSMA, );
+ regmap_read(esai_priv->regmap, REG_ESAI_RSMB, );
+
+ regmap_read(esai_priv->regmap, REG_ESAI_TCR, );
+ regmap_read(esai_priv->regmap, REG_ESAI_RCR, );
+
+ regmap_read(esai_priv->regmap, REG_ESAI_TFCR, );
+ regmap_read(esai_priv->regmap, REG_ESAI_RFCR, );
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_xEIE_MASK | ESAI_xCR_TE_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
+ ESAI_xCR_xEIE_MASK | ESAI_xCR_RE_MASK, 0);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMA,
+ ESAI_xSMA_xS_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMB,
+ ESAI_xSMB_xS_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMA,
+ ESAI_xSMA_xS_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMB,
+ ESAI_xSMB_xS_MASK, 0);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
+ ESAI_xFCR_xFR | ESAI_xFCR_xFEN, ESAI_xFCR_xFR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
+ ESAI_xFCR_xFR, 0);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
+ ESAI_xFCR_xFR | ESAI_xFCR_xFEN, ESAI_xFCR_xFR);
+
[PATCH RESEND V6 3/3] ASoC: fsl_asrc: Unify the supported input and output rate
Unify the supported input and output rate, add the
12kHz/24kHz/128kHz to the support list
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
---
sound/soc/fsl/fsl_asrc.c | 32 +++-
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index a8d6710f2541..cbbf6257f08a 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -27,13 +27,14 @@
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
/* Corresponding to process_option */
-static int supported_input_rate[] = {
- 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
- 96000, 176400, 192000,
+static unsigned int supported_asrc_rate[] = {
+ 5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000,
+ 64000, 88200, 96000, 128000, 176400, 192000,
};
-static int supported_asrc_rate[] = {
- 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000,
176400, 192000,
+static struct snd_pcm_hw_constraint_list fsl_asrc_rate_constraints = {
+ .count = ARRAY_SIZE(supported_asrc_rate),
+ .list = supported_asrc_rate,
};
/**
@@ -293,11 +294,11 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
*pair)
ideal = config->inclk == INCLK_NONE;
/* Validate input and output sample rates */
- for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++)
- if (inrate == supported_input_rate[in])
+ for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++)
+ if (inrate == supported_asrc_rate[in])
break;
- if (in == ARRAY_SIZE(supported_input_rate)) {
+ if (in == ARRAY_SIZE(supported_asrc_rate)) {
pair_err("unsupported input sample rate: %dHz\n", inrate);
return -EINVAL;
}
@@ -311,7 +312,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
return -EINVAL;
}
- if ((outrate >= 8000 && outrate <= 3) &&
+ if ((outrate >= 5512 && outrate <= 3) &&
(outrate > 24 * inrate || inrate > 8 * outrate)) {
pair_err("exceed supported ratio range [1/24, 8] for \
inrate/outrate: %d/%d\n", inrate, outrate);
@@ -486,7 +487,9 @@ static int fsl_asrc_dai_startup(struct snd_pcm_substream
*substream,
snd_pcm_hw_constraint_step(substream->runtime, 0,
SNDRV_PCM_HW_PARAM_CHANNELS, 2);
- return 0;
+
+ return snd_pcm_hw_constraint_list(substream->runtime, 0,
+ SNDRV_PCM_HW_PARAM_RATE, _asrc_rate_constraints);
}
static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
@@ -599,7 +602,6 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai)
return 0;
}
-#define FSL_ASRC_RATES SNDRV_PCM_RATE_8000_192000
#define FSL_ASRC_FORMATS (SNDRV_PCM_FMTBIT_S24_LE | \
SNDRV_PCM_FMTBIT_S16_LE | \
SNDRV_PCM_FMTBIT_S20_3LE)
@@ -610,14 +612,18 @@ static struct snd_soc_dai_driver fsl_asrc_dai = {
.stream_name = "ASRC-Playback",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.capture = {
.stream_name = "ASRC-Capture",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.ops = _asrc_dai_ops,
--
2.21.0
[PATCH RESEND V6 2/3] ASoC: fsl_asrc: replace the process_option table with function
When we want to support more sample rate, for example 12kHz/24kHz
we need update the process_option table, if we want to support more
sample rate next time, the table need to be updated again. which
is not flexible.
We got a function fsl_asrc_sel_proc to replace the table, which can
give the pre-processing and post-processing options according to
the sample rate.
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
---
sound/soc/fsl/fsl_asrc.c | 71 +---
1 file changed, 51 insertions(+), 20 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index ea035c12a325..a8d6710f2541 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -26,24 +26,6 @@
#define pair_dbg(fmt, ...) \
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
-/* Sample rates are aligned with that defined in pcm.h file */
-static const u8 process_option[][12][2] = {
- /* 8kHz 11.025kHz 16kHz 22.05kHz 32kHz 44.1kHz 48kHz 64kHz 88.2kHz
96kHz 176kHz 192kHz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 5512Hz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 8kHz */
- {{0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 11025Hz */
- {{1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 16kHz */
- {{1, 2}, {1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 22050Hz */
- {{1, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 0}, {0, 0}, {0, 0},}, /* 32kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 44.1kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 2}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 48kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 1}, {1, 2}, {0, 2}, {0, 2}, {0, 1}, {0,
1}, {0, 1}, {0, 1}, {0, 0},}, /* 64kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 88.2kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 96kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 176kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 192kHz */
-};
-
/* Corresponding to process_option */
static int supported_input_rate[] = {
5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
@@ -79,6 +61,52 @@ static unsigned char output_clk_map_imx53[] = {
static unsigned char *clk_map[2];
+/**
+ * Select the pre-processing and post-processing options
+ * Make sure to exclude following unsupported cases before
+ * calling this function:
+ * 1) inrate > 8.125 * outrate
+ * 2) inrate > 16.125 * outrate
+ *
+ * inrate: input sample rate
+ * outrate: output sample rate
+ * pre_proc: return value for pre-processing option
+ * post_proc: return value for post-processing option
+ */
+static void fsl_asrc_sel_proc(int inrate, int outrate,
+int *pre_proc, int *post_proc)
+{
+ bool post_proc_cond2;
+ bool post_proc_cond0;
+
+ /* select pre_proc between [0, 2] */
+ if (inrate * 8 > 33 * outrate)
+ *pre_proc = 2;
+ else if (inrate * 8 > 15 * outrate) {
+ if (inrate > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+ } else if (inrate < 76000)
+ *pre_proc = 0;
+ else if (inrate > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+
+ /* Condition for selection of post-processing */
+ post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) ||
+ (inrate > 56000 && outrate < 56000);
+ post_proc_cond0 = inrate * 23 < outrate * 8;
+
+ if (post_proc_cond2)
+ *post_proc = 2;
+ else if (post_proc_cond0)
+ *post_proc = 0;
+ else
+ *post_proc = 1;
+}
+
/**
* Request ASRC pair
*
@@ -239,6 +267,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
u32 inrate, outrate, indiv, outdiv;
u32 clk_index[2], div[2];
int in, out, channels;
+ int pre_proc, post_proc;
struct clk *clk;
bool ideal;
@@ -377,11 +406,13 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
*pair)
ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index),
ASRCTR_IDR(index) | ASRCTR_USR(index));
+
[PATCH RESEND V6 1/3] ASoC: fsl_asrc: Fix the issue about unsupported rate
When the output sample rate is [8kHz, 30kHz], the limitation
of the supported ratio range is [1/24, 8]. In the driver
we use (8kHz, 30kHz) instead of [8kHz, 30kHz].
So this patch is to fix this issue and the potential rounding
issue with divider.
Fixes: fff6e03c7b65 ("ASoC: fsl_asrc: add support for 8-30kHz
output sample rate")
Cc:
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
---
sound/soc/fsl/fsl_asrc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 0b937924d2e4..ea035c12a325 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -282,8 +282,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
return -EINVAL;
}
- if ((outrate > 8000 && outrate < 3) &&
- (outrate/inrate > 24 || inrate/outrate > 8)) {
+ if ((outrate >= 8000 && outrate <= 3) &&
+ (outrate > 24 * inrate || inrate > 8 * outrate)) {
pair_err("exceed supported ratio range [1/24, 8] for \
inrate/outrate: %d/%d\n", inrate, outrate);
return -EINVAL;
--
2.21.0
[PATCH RESEND V6 0/3] Support more sample rate in asrc
Support more sample rate in asrc Shengjiu Wang (3): ASoC: fsl_asrc: Fix the issue about unsupported rate ASoC: fsl_asrc: replace the process_option table with function ASoC: fsl_asrc: Unify the supported input and output rate Changes in RESEND V6 - change the Content-Transfer-Encoding to "quoted-printable", for - "base64" can't be applied Changes in v6 - add acked-by - fixed minor issue according to comments in v5 Changes in v5 - fix the [1/24, 8] - move fsl_asrc_sel_proc before setting Changes in v4 - add patch to Fix the [8kHz, 30kHz] open set issue. Changes in v3 - remove FSL_ASRC_RATES - refine fsl_asrc_sel_proc according to comments Changes in v2 - add more comments in code - add commit "Unify the supported input and output rate" sound/soc/fsl/fsl_asrc.c | 105 ++- 1 file changed, 71 insertions(+), 34 deletions(-) -- 2.21.0
Re: [PATCH V4] ASoC: fsl_esai: Add pm runtime function
Hi > Hi Mark, > On Fri, May 03, 2019 at 01:27:31PM +0900, Mark Brown wrote: > > On Thu, May 02, 2019 at 09:13:58AM +0000, S.j. Wang wrote: > > > > > I am checking, but I don't know why this patch failed in your side. > > > I Tried to apply this patch on for-5.1, for 5.2, for-linus and > > > for-next, all are Successful. The git is > git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git. > > > > > I can't reproduce your problem. Is there any my operation wrong? > > > > The error message I got was: > > > > Applying: ASoC: fsl_esai: Add pm runtime function > > error: patch failed: sound/soc/fsl/fsl_esai.c:9 > > error: sound/soc/fsl/fsl_esai.c: patch does not apply Patch failed at > > 0001 ASoC: fsl_esai: Add pm runtime function > > > > which is the header addition. I can't spot any obvious issues > > visually looking at the patch, only thing I can think is some kind of > > whitespace damage somewhere. > > I downloaded this v4 from patchwork and resubmitted a v5 for a test. > Would you please try to apply that one? > > If my v5 works vs. having merge conflict at v4, maybe something wrong with > Git version of Shengjiu's? I compared my v5 and his > v4 using vimdiff, there is no much difference of whitespace. > > Thanks > Nicolin We find that maybe it is caused by the Transfer-Encoding format. We sent the patch by the --transfer-encoding=8bit, but in the receiver side it shows: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 It may be caused by our company's mail server. We are checking... Best regards Wang shengjiu
RE: [EXT] Re: [PATCH V4] ASoC: fsl_esai: Add pm runtime function
Hi Mark > On Sun, Apr 28, 2019 at 02:24:54AM +0000, S.j. Wang wrote: > > Add pm runtime support and move clock handling there. > > Close the clocks at suspend to reduce the power consumption. > > > > fsl_esai_suspend is replaced by pm_runtime_force_suspend. > > fsl_esai_resume is replaced by pm_runtime_force_resume. > > This doesn't apply against for-5.2 again. Sorry about this, I think this one > is > due to some messups with my scripts which caused some patches to be > dropped for a while (and it's likely to be what happened the last time as > well). Can you check and resend again please? Like I say sorry about this, I > think it's my mistake. I am checking, but I don't know why this patch failed in your side. I Tried to apply this patch on for-5.1, for 5.2, for-linus and for-next, all are Successful. The git is git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git. I can't reproduce your problem. Is there any my operation wrong? Best regards Wang shengjiu
[PATCH V4] ASoC: fsl_esai: Add pm runtime function
Add pm runtime support and move clock handling there.
Close the clocks at suspend to reduce the power consumption.
fsl_esai_suspend is replaced by pm_runtime_force_suspend.
fsl_esai_resume is replaced by pm_runtime_force_resume.
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
---
Changes in v4
-resend base on for-5.2
Changes in v3
-refine the commit comments.
-add acked-by
Changes in v2
-refine the commit comments.
-move regcache_mark_dirty to runtime suspend.
sound/soc/fsl/fsl_esai.c | 141 ++-
1 file changed, 77 insertions(+), 64 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index bad0dfed6b68..10d2210c91ef 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
@@ -466,30 +467,6 @@ static int fsl_esai_startup(struct snd_pcm_substream
*substream,
struct snd_soc_dai *dai)
{
struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
- int ret;
-
- /*
-* Some platforms might use the same bit to gate all three or two of
-* clocks, so keep all clocks open/close at the same time for safety
-*/
- ret = clk_prepare_enable(esai_priv->coreclk);
- if (ret)
- return ret;
- if (!IS_ERR(esai_priv->spbaclk)) {
- ret = clk_prepare_enable(esai_priv->spbaclk);
- if (ret)
- goto err_spbaclk;
- }
- if (!IS_ERR(esai_priv->extalclk)) {
- ret = clk_prepare_enable(esai_priv->extalclk);
- if (ret)
- goto err_extalck;
- }
- if (!IS_ERR(esai_priv->fsysclk)) {
- ret = clk_prepare_enable(esai_priv->fsysclk);
- if (ret)
- goto err_fsysclk;
- }
if (!dai->active) {
/* Set synchronous mode */
@@ -506,16 +483,6 @@ static int fsl_esai_startup(struct snd_pcm_substream
*substream,
return 0;
-err_fsysclk:
- if (!IS_ERR(esai_priv->extalclk))
- clk_disable_unprepare(esai_priv->extalclk);
-err_extalck:
- if (!IS_ERR(esai_priv->spbaclk))
- clk_disable_unprepare(esai_priv->spbaclk);
-err_spbaclk:
- clk_disable_unprepare(esai_priv->coreclk);
-
- return ret;
}
static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
@@ -576,20 +543,6 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
*substream,
return 0;
}
-static void fsl_esai_shutdown(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
-{
- struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
-
- if (!IS_ERR(esai_priv->fsysclk))
- clk_disable_unprepare(esai_priv->fsysclk);
- if (!IS_ERR(esai_priv->extalclk))
- clk_disable_unprepare(esai_priv->extalclk);
- if (!IS_ERR(esai_priv->spbaclk))
- clk_disable_unprepare(esai_priv->spbaclk);
- clk_disable_unprepare(esai_priv->coreclk);
-}
-
static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *dai)
{
@@ -658,7 +611,6 @@ static int fsl_esai_trigger(struct snd_pcm_substream
*substream, int cmd,
static const struct snd_soc_dai_ops fsl_esai_dai_ops = {
.startup = fsl_esai_startup,
- .shutdown = fsl_esai_shutdown,
.trigger = fsl_esai_trigger,
.hw_params = fsl_esai_hw_params,
.set_sysclk = fsl_esai_set_dai_sysclk,
@@ -947,6 +899,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
return ret;
}
+ pm_runtime_enable(>dev);
+
+ regcache_cache_only(esai_priv->regmap, true);
+
ret = imx_pcm_dma_init(pdev, IMX_ESAI_DMABUF_SIZE);
if (ret)
dev_err(>dev, "failed to init imx pcm dma: %d\n", ret);
@@ -954,6 +910,13 @@ static int fsl_esai_probe(struct platform_device *pdev)
return ret;
}
+static int fsl_esai_remove(struct platform_device *pdev)
+{
+ pm_runtime_disable(>dev);
+
+ return 0;
+}
+
static const struct of_device_id fsl_esai_dt_ids[] = {
{ .compatible = "fsl,imx35-esai", },
{ .compatible = "fsl,vf610-esai", },
@@ -961,22 +924,35 @@ static int fsl_esai_probe(struct platform_device *pdev)
};
MODULE_DEVICE_TABLE(of, fsl_esai_dt_ids);
-#ifdef CONFIG_PM_SLEEP
-static int fsl_esai_suspend(struct device *dev)
-{
- struct fsl_esai *esai = dev_get_drvdata(dev);
-
- regcache_cache_only(esai->regmap, true);
- regcache_mark_dirty(esai->regmap);
-
- return 0;
-}
-
-static int fsl_esai_resume(struct device *dev)
+#ifdef CONFIG_PM
+static int fsl_esai_runtime_resume(struct device *dev)
{
struct fsl_esai *esai = dev_get_drvdata(dev);
int ret;
+ /*
+* Some platforms might use the same bit to
Re: [PATCH V3] ASoC: fsl_esai: Add pm runtime function
Hi Mark > On Fri, Apr 26, 2019 at 10:51:15AM +0000, S.j. Wang wrote: > > > On Mon, Apr 22, 2019 at 02:31:55AM +0000, S.j. Wang wrote: > > > > Add pm runtime support and move clock handling there. > > > > Close the clocks at suspend to reduce the power consumption. > > > > > fsl_esai_suspend is replaced by pm_runtime_force_suspend. > > > > fsl_esai_resume is replaced by pm_runtime_force_resume. > > > > This doesn't apply against current code, please check and resend. > > > Which branch are you using? I tried for-next and for-linus, both Are > > successful applied. > > I'm applying against for-5.2, though if it depends on a patch queued for > 5.1 that's fine, I can just merge that up - please just resend. I think I > did try > merging 5.1 though... I think may be caused by the patch " ASoC: fsl_esai: Fix missing break in switch statement", so I resend them both base on for-5.2. best regards wang shengjiu
[PATCH V6] ASoC: fsl_esai: Fix missing break in switch statement
case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be
independent of each other, so replace fall-through with break.
Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
Cc:
---
Changes in V6
- resend base one for-5.2
Changes in v5
- remove new line after Fixes
Changes in v4
- Add acked-by
Changes in v3
- Update subject line and cc stable
Changes in v2
- Fix "Fixes" tag
sound/soc/fsl/fsl_esai.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index c7410bbfd2af..bad0dfed6b68 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id,
break;
case ESAI_HCKT_EXTAL:
ecr |= ESAI_ECR_ETI;
- /* fall through */
+ break;
case ESAI_HCKR_EXTAL:
ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
break;
--
1.9.1
Re: [PATCH V3] ASoC: fsl_esai: Add pm runtime function
Hi Mark > > On Mon, Apr 22, 2019 at 02:31:55AM +0000, S.j. Wang wrote: > > Add pm runtime support and move clock handling there. > > Close the clocks at suspend to reduce the power consumption. > > > > fsl_esai_suspend is replaced by pm_runtime_force_suspend. > > fsl_esai_resume is replaced by pm_runtime_force_resume. > > This doesn't apply against current code, please check and resend. Which branch are you using? I tried for-next and for-linus, both Are successful applied. Best regards Wang shengjiu
[PATCH V6 3/3] ASoC: fsl_asrc: Unify the supported input and output rate
Unify the supported input and output rate, add the
12kHz/24kHz/128kHz to the support list
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
---
sound/soc/fsl/fsl_asrc.c | 32 +++-
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index a8d6710f2541..cbbf6257f08a 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -27,13 +27,14 @@
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
/* Corresponding to process_option */
-static int supported_input_rate[] = {
- 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
- 96000, 176400, 192000,
+static unsigned int supported_asrc_rate[] = {
+ 5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000,
+ 64000, 88200, 96000, 128000, 176400, 192000,
};
-static int supported_asrc_rate[] = {
- 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000,
176400, 192000,
+static struct snd_pcm_hw_constraint_list fsl_asrc_rate_constraints = {
+ .count = ARRAY_SIZE(supported_asrc_rate),
+ .list = supported_asrc_rate,
};
/**
@@ -293,11 +294,11 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
*pair)
ideal = config->inclk == INCLK_NONE;
/* Validate input and output sample rates */
- for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++)
- if (inrate == supported_input_rate[in])
+ for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++)
+ if (inrate == supported_asrc_rate[in])
break;
- if (in == ARRAY_SIZE(supported_input_rate)) {
+ if (in == ARRAY_SIZE(supported_asrc_rate)) {
pair_err("unsupported input sample rate: %dHz\n", inrate);
return -EINVAL;
}
@@ -311,7 +312,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
return -EINVAL;
}
- if ((outrate >= 8000 && outrate <= 3) &&
+ if ((outrate >= 5512 && outrate <= 3) &&
(outrate > 24 * inrate || inrate > 8 * outrate)) {
pair_err("exceed supported ratio range [1/24, 8] for \
inrate/outrate: %d/%d\n", inrate, outrate);
@@ -486,7 +487,9 @@ static int fsl_asrc_dai_startup(struct snd_pcm_substream
*substream,
snd_pcm_hw_constraint_step(substream->runtime, 0,
SNDRV_PCM_HW_PARAM_CHANNELS, 2);
- return 0;
+
+ return snd_pcm_hw_constraint_list(substream->runtime, 0,
+ SNDRV_PCM_HW_PARAM_RATE, _asrc_rate_constraints);
}
static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
@@ -599,7 +602,6 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai)
return 0;
}
-#define FSL_ASRC_RATES SNDRV_PCM_RATE_8000_192000
#define FSL_ASRC_FORMATS (SNDRV_PCM_FMTBIT_S24_LE | \
SNDRV_PCM_FMTBIT_S16_LE | \
SNDRV_PCM_FMTBIT_S20_3LE)
@@ -610,14 +612,18 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai)
.stream_name = "ASRC-Playback",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.capture = {
.stream_name = "ASRC-Capture",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.ops = _asrc_dai_ops,
--
1.9.1
[PATCH V6 2/3] ASoC: fsl_asrc: replace the process_option table with function
When we want to support more sample rate, for example 12kHz/24kHz
we need update the process_option table, if we want to support more
sample rate next time, the table need to be updated again. which
is not flexible.
We got a function fsl_asrc_sel_proc to replace the table, which can
give the pre-processing and post-processing options according to
the sample rate.
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
---
sound/soc/fsl/fsl_asrc.c | 71 ++--
1 file changed, 51 insertions(+), 20 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index ea035c12a325..a8d6710f2541 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -26,24 +26,6 @@
#define pair_dbg(fmt, ...) \
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
-/* Sample rates are aligned with that defined in pcm.h file */
-static const u8 process_option[][12][2] = {
- /* 8kHz 11.025kHz 16kHz 22.05kHz 32kHz 44.1kHz 48kHz 64kHz 88.2kHz
96kHz 176kHz 192kHz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 5512Hz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 8kHz */
- {{0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 11025Hz */
- {{1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 16kHz */
- {{1, 2}, {1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 22050Hz */
- {{1, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 0}, {0, 0}, {0, 0},}, /* 32kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 44.1kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 2}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 48kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 1}, {1, 2}, {0, 2}, {0, 2}, {0, 1}, {0,
1}, {0, 1}, {0, 1}, {0, 0},}, /* 64kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 88.2kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 96kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 176kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 192kHz */
-};
-
/* Corresponding to process_option */
static int supported_input_rate[] = {
5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
@@ -80,6 +62,52 @@
static unsigned char *clk_map[2];
/**
+ * Select the pre-processing and post-processing options
+ * Make sure to exclude following unsupported cases before
+ * calling this function:
+ * 1) inrate > 8.125 * outrate
+ * 2) inrate > 16.125 * outrate
+ *
+ * inrate: input sample rate
+ * outrate: output sample rate
+ * pre_proc: return value for pre-processing option
+ * post_proc: return value for post-processing option
+ */
+static void fsl_asrc_sel_proc(int inrate, int outrate,
+int *pre_proc, int *post_proc)
+{
+ bool post_proc_cond2;
+ bool post_proc_cond0;
+
+ /* select pre_proc between [0, 2] */
+ if (inrate * 8 > 33 * outrate)
+ *pre_proc = 2;
+ else if (inrate * 8 > 15 * outrate) {
+ if (inrate > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+ } else if (inrate < 76000)
+ *pre_proc = 0;
+ else if (inrate > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+
+ /* Condition for selection of post-processing */
+ post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) ||
+ (inrate > 56000 && outrate < 56000);
+ post_proc_cond0 = inrate * 23 < outrate * 8;
+
+ if (post_proc_cond2)
+ *post_proc = 2;
+ else if (post_proc_cond0)
+ *post_proc = 0;
+ else
+ *post_proc = 1;
+}
+
+/**
* Request ASRC pair
*
* It assigns pair by the order of A->C->B because allocation of pair B,
@@ -239,6 +267,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
u32 inrate, outrate, indiv, outdiv;
u32 clk_index[2], div[2];
int in, out, channels;
+ int pre_proc, post_proc;
struct clk *clk;
bool ideal;
@@ -377,11 +406,13 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
*pair)
ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index),
ASRCTR_IDR(index) |
[PATCH V6 1/3] ASoC: fsl_asrc: Fix the issue about unsupported rate
When the output sample rate is [8kHz, 30kHz], the limitation
of the supported ratio range is [1/24, 8]. In the driver
we use (8kHz, 30kHz) instead of [8kHz, 30kHz].
So this patch is to fix this issue and the potential rounding
issue with divider.
Fixes: fff6e03c7b65 ("ASoC: fsl_asrc: add support for 8-30kHz
output sample rate")
Cc:
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
---
sound/soc/fsl/fsl_asrc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 0b937924d2e4..ea035c12a325 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -282,8 +282,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
return -EINVAL;
}
- if ((outrate > 8000 && outrate < 3) &&
- (outrate/inrate > 24 || inrate/outrate > 8)) {
+ if ((outrate >= 8000 && outrate <= 3) &&
+ (outrate > 24 * inrate || inrate > 8 * outrate)) {
pair_err("exceed supported ratio range [1/24, 8] for \
inrate/outrate: %d/%d\n", inrate, outrate);
return -EINVAL;
--
1.9.1
[PATCH V6 0/3] Support more sample rate in asrc
Support more sample rate in asrc Shengjiu Wang (3): ASoC: fsl_asrc: Fix the issue about unsupported rate ASoC: fsl_asrc: replace the process_option table with function ASoC: fsl_asrc: Unify the supported input and output rate Changes in v6 - add acked-by - fixed minor issue according to comments in v5 Changes in v5 - fix the [1/24, 8] - move fsl_asrc_sel_proc before setting Changes in v4 - add patch to Fix the [8kHz, 30kHz] open set issue. Changes in v3 - remove FSL_ASRC_RATES - refine fsl_asrc_sel_proc according to comments Changes in v2 - add more comments in code - add commit "Unify the supported input and output rate" sound/soc/fsl/fsl_asrc.c | 105 --- 1 file changed, 71 insertions(+), 34 deletions(-) -- 1.9.1
Re: [PATCH V5 2/3] ASoC: fsl_asrc: replace the process_option table with function
Hi > > > On Mon, Apr 22, 2019 at 03:15:34AM +0000, S.j. Wang wrote: > > Hi > > > > > > > > > > > On Mon, Apr 22, 2019 at 02:32:35AM +, S.j. Wang wrote: > > > > When we want to support more sample rate, for example > 12kHz/24kHz > > > we > > > > need update the process_option table, if we want to support more > > > > sample rate next time, the table need to be updated again. which > > > > is not flexible. > > > > > > > > We got a function fsl_asrc_sel_proc to replace the table, which > > > > can give the pre-processing and post-processing options according > > > > to the sample rate. > > > > > > > > Signed-off-by: Shengjiu Wang > > > > > > A couple of more small comments. > > > > > > And please add this when you resend: > > > Acked-by: Nicolin Chen > > > > > > > + * Unsupport cases: Tsout > 8.125 * Tsin, Tsout > 16.125 * Tsin > > > > > > Since we have a ratio validation somewhere else, it's okay to drop > > > this line - > > > - it may confuse people since the function no longer checks these > > > unsupported cases. > > > > I add this for may be in the future we forget the limitation. Just for a > reminder. > > Okay. Let's use something more practical like: > > +* Make sure to exclude following unsupported cases before calling the > function: > +* 1) outrate > 8.125 * inrate > +* 2) outrate > 16.125 * inrate Ok.
Re: [PATCH V5 2/3] ASoC: fsl_asrc: replace the process_option table with function
Hi > > > On Mon, Apr 22, 2019 at 02:32:35AM +0000, S.j. Wang wrote: > > When we want to support more sample rate, for example 12kHz/24kHz > we > > need update the process_option table, if we want to support more > > sample rate next time, the table need to be updated again. which is > > not flexible. > > > > We got a function fsl_asrc_sel_proc to replace the table, which can > > give the pre-processing and post-processing options according to the > > sample rate. > > > > Signed-off-by: Shengjiu Wang > > A couple of more small comments. > > And please add this when you resend: > Acked-by: Nicolin Chen > > > + * Unsupport cases: Tsout > 8.125 * Tsin, Tsout > 16.125 * Tsin > > Since we have a ratio validation somewhere else, it's okay to drop this line - > - it may confuse people since the function no longer checks these > unsupported cases. I add this for may be in the future we forget the limitation. Just for a reminder. Best regards Wang shengjiu > > > +static int fsl_asrc_sel_proc(int inrate, int outrate, > > I think "void" type should be just fine as we made sure there is no > unsupported cases running in this function.
[PATCH V5 3/3] ASoC: fsl_asrc: Unify the supported input and output rate
Unify the supported input and output rate, add the
12kHz/24kHz/128kHz to the support list
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 32 +++-
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 4675ea4e8204..85c278effda1 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -27,13 +27,14 @@
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
/* Corresponding to process_option */
-static int supported_input_rate[] = {
- 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
- 96000, 176400, 192000,
+static unsigned int supported_asrc_rate[] = {
+ 5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000,
+ 64000, 88200, 96000, 128000, 176400, 192000,
};
-static int supported_asrc_rate[] = {
- 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000,
176400, 192000,
+static struct snd_pcm_hw_constraint_list fsl_asrc_rate_constraints = {
+ .count = ARRAY_SIZE(supported_asrc_rate),
+ .list = supported_asrc_rate,
};
/**
@@ -292,11 +293,11 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
*pair)
ideal = config->inclk == INCLK_NONE;
/* Validate input and output sample rates */
- for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++)
- if (inrate == supported_input_rate[in])
+ for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++)
+ if (inrate == supported_asrc_rate[in])
break;
- if (in == ARRAY_SIZE(supported_input_rate)) {
+ if (in == ARRAY_SIZE(supported_asrc_rate)) {
pair_err("unsupported input sample rate: %dHz\n", inrate);
return -EINVAL;
}
@@ -310,7 +311,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
return -EINVAL;
}
- if ((outrate >= 8000 && outrate <= 3) &&
+ if ((outrate >= 5512 && outrate <= 3) &&
(outrate > 24 * inrate || inrate > 8 * outrate)) {
pair_err("exceed supported ratio range [1/24, 8] for \
inrate/outrate: %d/%d\n", inrate, outrate);
@@ -485,7 +486,9 @@ static int fsl_asrc_dai_startup(struct snd_pcm_substream
*substream,
snd_pcm_hw_constraint_step(substream->runtime, 0,
SNDRV_PCM_HW_PARAM_CHANNELS, 2);
- return 0;
+
+ return snd_pcm_hw_constraint_list(substream->runtime, 0,
+ SNDRV_PCM_HW_PARAM_RATE, _asrc_rate_constraints);
}
static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
@@ -598,7 +601,6 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai)
return 0;
}
-#define FSL_ASRC_RATES SNDRV_PCM_RATE_8000_192000
#define FSL_ASRC_FORMATS (SNDRV_PCM_FMTBIT_S24_LE | \
SNDRV_PCM_FMTBIT_S16_LE | \
SNDRV_PCM_FMTBIT_S20_3LE)
@@ -609,14 +611,18 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai)
.stream_name = "ASRC-Playback",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.capture = {
.stream_name = "ASRC-Capture",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.ops = _asrc_dai_ops,
--
1.9.1
[PATCH V5 2/3] ASoC: fsl_asrc: replace the process_option table with function
When we want to support more sample rate, for example 12kHz/24kHz
we need update the process_option table, if we want to support more
sample rate next time, the table need to be updated again. which
is not flexible.
We got a function fsl_asrc_sel_proc to replace the table, which can
give the pre-processing and post-processing options according to
the sample rate.
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 70 ++--
1 file changed, 50 insertions(+), 20 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index ea035c12a325..4675ea4e8204 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -26,24 +26,6 @@
#define pair_dbg(fmt, ...) \
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
-/* Sample rates are aligned with that defined in pcm.h file */
-static const u8 process_option[][12][2] = {
- /* 8kHz 11.025kHz 16kHz 22.05kHz 32kHz 44.1kHz 48kHz 64kHz 88.2kHz
96kHz 176kHz 192kHz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 5512Hz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 8kHz */
- {{0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 11025Hz */
- {{1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 16kHz */
- {{1, 2}, {1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 22050Hz */
- {{1, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 0}, {0, 0}, {0, 0},}, /* 32kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 44.1kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 2}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 48kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 1}, {1, 2}, {0, 2}, {0, 2}, {0, 1}, {0,
1}, {0, 1}, {0, 1}, {0, 0},}, /* 64kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 88.2kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 96kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 176kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 192kHz */
-};
-
/* Corresponding to process_option */
static int supported_input_rate[] = {
5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
@@ -80,6 +62,51 @@
static unsigned char *clk_map[2];
/**
+ * Select the pre-processing and post-processing options
+ * Unsupport cases: Tsout > 8.125 * Tsin, Tsout > 16.125 * Tsin
+ *
+ * inrate: input sample rate
+ * outrate: output sample rate
+ * pre_proc: return value for pre-processing option
+ * post_proc: return value for post-processing option
+ */
+static int fsl_asrc_sel_proc(int inrate, int outrate,
+int *pre_proc, int *post_proc)
+{
+ bool post_proc_cond2;
+ bool post_proc_cond0;
+
+ /* select pre_proc between [0, 2] */
+ if (inrate * 8 > 33 * outrate)
+ *pre_proc = 2;
+ else if (inrate * 8 > 15 * outrate) {
+ if (inrate > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+ } else if (inrate < 76000)
+ *pre_proc = 0;
+ else if (inrate > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+
+ /* Condition for selection of post-processing */
+ post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) ||
+ (inrate > 56000 && outrate < 56000);
+ post_proc_cond0 = inrate * 23 < outrate * 8;
+
+ if (post_proc_cond2)
+ *post_proc = 2;
+ else if (post_proc_cond0)
+ *post_proc = 0;
+ else
+ *post_proc = 1;
+
+ return 0;
+}
+
+/**
* Request ASRC pair
*
* It assigns pair by the order of A->C->B because allocation of pair B,
@@ -239,6 +266,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
u32 inrate, outrate, indiv, outdiv;
u32 clk_index[2], div[2];
int in, out, channels;
+ int pre_proc, post_proc;
struct clk *clk;
bool ideal;
@@ -377,11 +405,13 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
*pair)
ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index),
ASRCTR_IDR(index) | ASRCTR_USR(index));
+ fsl_asrc_sel_proc(inrate, outrate, _proc, _proc);
+
/* Apply
[PATCH V5 1/3] ASoC: fsl_asrc: Fix the issue about unsupported rate
When the output sample rate is [8kHz, 30kHz], the limitation
of the supported ratio range is (1/24, 8). In the driver
we use (8kHz, 30kHz) instead of [8kHz, 30kHz].
So this patch is to fix this issue and the potential rounding
issue with divider.
Fixes: fff6e03c7b65 ("ASoC: fsl_asrc: add support for 8-30kHz
output sample rate")
Cc:
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 0b937924d2e4..ea035c12a325 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -282,8 +282,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
return -EINVAL;
}
- if ((outrate > 8000 && outrate < 3) &&
- (outrate/inrate > 24 || inrate/outrate > 8)) {
+ if ((outrate >= 8000 && outrate <= 3) &&
+ (outrate > 24 * inrate || inrate > 8 * outrate)) {
pair_err("exceed supported ratio range [1/24, 8] for \
inrate/outrate: %d/%d\n", inrate, outrate);
return -EINVAL;
--
1.9.1
[PATCH V5 0/3] Support more sample rate in asrc
Support more sample rate in asrc Shengjiu Wang (3): ASoC: fsl_asrc: Fix the issue about unsupported rate ASoC: fsl_asrc: replace the process_option table with function ASoC: fsl_asrc: Unify the supported input and output rate Changes in v5 - fix the [1/24, 8] - move fsl_asrc_sel_proc before setting Changes in v4 - add patch to Fix the [8kHz, 30kHz] open set issue. Changes in v3 - remove FSL_ASRC_RATES - refine fsl_asrc_sel_proc according to comments Changes in v2 - add more comments in code - add commit "Unify the supported input and output rate" sound/soc/fsl/fsl_asrc.c | 104 +++ 1 file changed, 70 insertions(+), 34 deletions(-) -- 1.9.1
[PATCH V3] ASoC: fsl_esai: Add pm runtime function
Add pm runtime support and move clock handling there.
Close the clocks at suspend to reduce the power consumption.
fsl_esai_suspend is replaced by pm_runtime_force_suspend.
fsl_esai_resume is replaced by pm_runtime_force_resume.
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
---
Changes in v3
-refine the commit comments.
-add acked-by
Changes in v2
-refine the commit comments.
-move regcache_mark_dirty to runtime suspend.
sound/soc/fsl/fsl_esai.c | 141 ++-
1 file changed, 77 insertions(+), 64 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index c7410bbfd2af..022368c8a074 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
@@ -466,30 +467,6 @@ static int fsl_esai_startup(struct snd_pcm_substream
*substream,
struct snd_soc_dai *dai)
{
struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
- int ret;
-
- /*
-* Some platforms might use the same bit to gate all three or two of
-* clocks, so keep all clocks open/close at the same time for safety
-*/
- ret = clk_prepare_enable(esai_priv->coreclk);
- if (ret)
- return ret;
- if (!IS_ERR(esai_priv->spbaclk)) {
- ret = clk_prepare_enable(esai_priv->spbaclk);
- if (ret)
- goto err_spbaclk;
- }
- if (!IS_ERR(esai_priv->extalclk)) {
- ret = clk_prepare_enable(esai_priv->extalclk);
- if (ret)
- goto err_extalck;
- }
- if (!IS_ERR(esai_priv->fsysclk)) {
- ret = clk_prepare_enable(esai_priv->fsysclk);
- if (ret)
- goto err_fsysclk;
- }
if (!dai->active) {
/* Set synchronous mode */
@@ -506,16 +483,6 @@ static int fsl_esai_startup(struct snd_pcm_substream
*substream,
return 0;
-err_fsysclk:
- if (!IS_ERR(esai_priv->extalclk))
- clk_disable_unprepare(esai_priv->extalclk);
-err_extalck:
- if (!IS_ERR(esai_priv->spbaclk))
- clk_disable_unprepare(esai_priv->spbaclk);
-err_spbaclk:
- clk_disable_unprepare(esai_priv->coreclk);
-
- return ret;
}
static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
@@ -576,20 +543,6 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
*substream,
return 0;
}
-static void fsl_esai_shutdown(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
-{
- struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
-
- if (!IS_ERR(esai_priv->fsysclk))
- clk_disable_unprepare(esai_priv->fsysclk);
- if (!IS_ERR(esai_priv->extalclk))
- clk_disable_unprepare(esai_priv->extalclk);
- if (!IS_ERR(esai_priv->spbaclk))
- clk_disable_unprepare(esai_priv->spbaclk);
- clk_disable_unprepare(esai_priv->coreclk);
-}
-
static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *dai)
{
@@ -658,7 +611,6 @@ static int fsl_esai_trigger(struct snd_pcm_substream
*substream, int cmd,
static const struct snd_soc_dai_ops fsl_esai_dai_ops = {
.startup = fsl_esai_startup,
- .shutdown = fsl_esai_shutdown,
.trigger = fsl_esai_trigger,
.hw_params = fsl_esai_hw_params,
.set_sysclk = fsl_esai_set_dai_sysclk,
@@ -947,6 +899,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
return ret;
}
+ pm_runtime_enable(>dev);
+
+ regcache_cache_only(esai_priv->regmap, true);
+
ret = imx_pcm_dma_init(pdev, IMX_ESAI_DMABUF_SIZE);
if (ret)
dev_err(>dev, "failed to init imx pcm dma: %d\n", ret);
@@ -954,6 +910,13 @@ static int fsl_esai_probe(struct platform_device *pdev)
return ret;
}
+static int fsl_esai_remove(struct platform_device *pdev)
+{
+ pm_runtime_disable(>dev);
+
+ return 0;
+}
+
static const struct of_device_id fsl_esai_dt_ids[] = {
{ .compatible = "fsl,imx35-esai", },
{ .compatible = "fsl,vf610-esai", },
@@ -961,22 +924,35 @@ static int fsl_esai_probe(struct platform_device *pdev)
};
MODULE_DEVICE_TABLE(of, fsl_esai_dt_ids);
-#ifdef CONFIG_PM_SLEEP
-static int fsl_esai_suspend(struct device *dev)
-{
- struct fsl_esai *esai = dev_get_drvdata(dev);
-
- regcache_cache_only(esai->regmap, true);
- regcache_mark_dirty(esai->regmap);
-
- return 0;
-}
-
-static int fsl_esai_resume(struct device *dev)
+#ifdef CONFIG_PM
+static int fsl_esai_runtime_resume(struct device *dev)
{
struct fsl_esai *esai = dev_get_drvdata(dev);
int ret;
+ /*
+* Some platforms might use the same bit to gate all three or two of
+*
Re: [PATCH V4 3/3] ASoC: fsl_asrc: Unify the supported input and output rate
Hi.
>
> On Fri, Apr 19, 2019 at 10:23:56AM +0000, S.j. Wang wrote:
> > Unify the supported input and output rate, add the 12kHz/24kHz/128kHz
> > to the support list
> >
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/fsl_asrc.c | 32 +++-
> > 1 file changed, 19 insertions(+), 13 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index
> > 2c4bbc3499db..0d06e738264a 100644
> > --- a/sound/soc/fsl/fsl_asrc.c
> > +++ b/sound/soc/fsl/fsl_asrc.c
> > @@ -27,13 +27,14 @@
> > dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
> > ##__VA_ARGS__)
> >
> > /* Corresponding to process_option */ -static int
> > supported_input_rate[] = {
> > - 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
> > - 96000, 176400, 192000,
> > +static unsigned int supported_asrc_rate[] = {
> > + 5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000,
> > + 64000, 88200, 96000, 128000, 176400, 192000,
> > };
> >
> > -static int supported_asrc_rate[] = {
> > - 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000,
> 176400, 192000,
> > +static struct snd_pcm_hw_constraint_list fsl_asrc_rate_constraints = {
> > + .count = ARRAY_SIZE(supported_asrc_rate),
> > + .list = supported_asrc_rate,
> > };
> >
> > /**
> > @@ -293,11 +294,11 @@ static int fsl_asrc_config_pair(struct
> fsl_asrc_pair *pair)
> > ideal = config->inclk == INCLK_NONE;
> >
> > /* Validate input and output sample rates */
> > - for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++)
> > - if (inrate == supported_input_rate[in])
> > + for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++)
> > + if (inrate == supported_asrc_rate[in])
> > break;
>
> Not sure if we still need it upon having hw_constraint. Maybe m2m needs it?
Yes.
Best regards
Wang shengjiu
Re: [PATCH V4 2/3] ASoC: fsl_asrc: replace the process_option table with function
Hi
>
>
> On Fri, Apr 19, 2019 at 10:23:53AM +0000, S.j. Wang wrote:
>
> > @@ -289,6 +318,12 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
> *pair)
> > return -EINVAL;
> > }
> >
> > + ret = fsl_asrc_sel_proc(inrate, outrate, _proc, _proc);
>
> Since the function always return 0, I am thinking of treating this function as
> a lookup function, and then moving this call right before the register
> settings -- as we have already made sure that both inrate and outrate are
> supported.
Ok.
>
> > + if (ret) {
> > + pair_err("No supported pre-processing options\n");
> > + return ret;
> > + }
>
> And probably no longer need this error-out. If there's a new limitation
> related to this function, I believe we can add it to the rate validation
> section as we are doing now -- better to have rate validation code at one
> place.
>
Ok.
> > @@ -380,8 +415,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
> *pair)
> > /* Apply configurations for pre- and post-processing */
>
> Here:
> - /* Apply configurations for pre- and post-processing */
> + /* Select and apply configurations for pre- and post-processing */
> + fsl_asrc_sel_proc(inrate, outrate, _proc, _proc);
> > regmap_update_bits(asrc_priv->regmap, REG_ASRCFG,
> > ASRCFG_PREMODi_MASK(index) |
> ASRCFG_POSTMODi_MASK(index),
> > -ASRCFG_PREMOD(index, process_option[in][out][0]) |
> > -ASRCFG_POSTMOD(index, process_option[in][out][1]));
> > +ASRCFG_PREMOD(index, pre_proc) |
> > +ASRCFG_POSTMOD(index, post_proc));
RE: [EXT] Re: [PATCH V4 1/3] ASoC: fsl_asrc: Fix the issue about unsupported rate
Hi
>
>
> On Fri, Apr 19, 2019 at 10:23:50AM +0000, S.j. Wang wrote:
> > When the output sample rate is [8kHz, 30kHz], the limitation of the
> > supported ratio range is (1/24, 8). In the driver we use (8kHz, 30kHz)
> > instead of [8kHz, 30kHz].
> > So this patch is to fix this issue and the potential rounding issue
> > with divider.
> >
> > Fixes: fff6e03c7b65 ("ASoC: fsl_asrc: add support for 8-30kHz output
> > sample rate")
> > Cc:
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/fsl_asrc.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index
> > 0b937924d2e4..5b8adc7fb117 100644
> > --- a/sound/soc/fsl/fsl_asrc.c
> > +++ b/sound/soc/fsl/fsl_asrc.c
> > @@ -282,10 +282,10 @@ static int fsl_asrc_config_pair(struct
> fsl_asrc_pair *pair)
> > return -EINVAL;
> > }
> >
> > - if ((outrate > 8000 && outrate < 3) &&
> > - (outrate/inrate > 24 || inrate/outrate > 8)) {
> > - pair_err("exceed supported ratio range [1/24, 8] for \
> > - inrate/outrate: %d/%d\n", inrate, outrate);
> > + if ((outrate >= 8000 && outrate <= 3) &&
> > + (outrate > 24 * inrate || inrate > 8 * outrate)) {
> > + pair_err("exceed supported ratio range (1/24, 8) for
> > + inrate/outrate: %d/%d\n",
>
> Using one of the conditions:
> if (inrate > 8 * outrate)
> pair_err();
>
> This means:
> if (inrate <= 8 * outrate)
> /* Everything is fine */
>
> So the supported ratio range is still [1/24, 8] right?
>
Oh, yes, should still [1/24, 8].
> Thanks
[alsa-devel] [PATCH V2] ASoC: fsl_esai: Add pm runtime function
Hi
>
>
> Add pm runtime support and move clock handling there.
> fsl_esai_suspend is replaced by pm_runtime_force_suspend.
> fsl_esai_resume is replaced by pm_runtime_force_resume.
>
> Signed-off-by: Shengjiu Wang
> ---
> Changes in v2
> -refine the commit comments.
> -move regcache_mark_dirty to runtime suspend.
>
> sound/soc/fsl/fsl_esai.c | 141 ++
> -
> 1 file changed, 77 insertions(+), 64 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> bad0dfed6b68..10d2210c91ef 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -466,30 +467,6 @@ static int fsl_esai_startup(struct
> snd_pcm_substream *substream,
> struct snd_soc_dai *dai) {
> struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
> - int ret;
> -
> - /*
> -* Some platforms might use the same bit to gate all three or two of
> -* clocks, so keep all clocks open/close at the same time for safety
> -*/
> - ret = clk_prepare_enable(esai_priv->coreclk);
> - if (ret)
> - return ret;
> - if (!IS_ERR(esai_priv->spbaclk)) {
> - ret = clk_prepare_enable(esai_priv->spbaclk);
> - if (ret)
> - goto err_spbaclk;
> - }
> - if (!IS_ERR(esai_priv->extalclk)) {
> - ret = clk_prepare_enable(esai_priv->extalclk);
> - if (ret)
> - goto err_extalck;
> - }
> - if (!IS_ERR(esai_priv->fsysclk)) {
> - ret = clk_prepare_enable(esai_priv->fsysclk);
> - if (ret)
> - goto err_fsysclk;
> - }
>
> if (!dai->active) {
> /* Set synchronous mode */ @@ -506,16 +483,6 @@ static int
> fsl_esai_startup(struct snd_pcm_substream *substream,
>
> return 0;
>
> -err_fsysclk:
> - if (!IS_ERR(esai_priv->extalclk))
> - clk_disable_unprepare(esai_priv->extalclk);
> -err_extalck:
> - if (!IS_ERR(esai_priv->spbaclk))
> - clk_disable_unprepare(esai_priv->spbaclk);
> -err_spbaclk:
> - clk_disable_unprepare(esai_priv->coreclk);
> -
> - return ret;
> }
>
> static int fsl_esai_hw_params(struct snd_pcm_substream *substream, @@
> -576,20 +543,6 @@ static int fsl_esai_hw_params(struct
> snd_pcm_substream *substream,
> return 0;
> }
>
> -static void fsl_esai_shutdown(struct snd_pcm_substream *substream,
> - struct snd_soc_dai *dai)
> -{
> - struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
> -
> - if (!IS_ERR(esai_priv->fsysclk))
> - clk_disable_unprepare(esai_priv->fsysclk);
> - if (!IS_ERR(esai_priv->extalclk))
> - clk_disable_unprepare(esai_priv->extalclk);
> - if (!IS_ERR(esai_priv->spbaclk))
> - clk_disable_unprepare(esai_priv->spbaclk);
> - clk_disable_unprepare(esai_priv->coreclk);
> -}
> -
> static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
> struct snd_soc_dai *dai) { @@ -658,7 +611,6 @@
> static int
> fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
>
> static const struct snd_soc_dai_ops fsl_esai_dai_ops = {
> .startup = fsl_esai_startup,
> - .shutdown = fsl_esai_shutdown,
> .trigger = fsl_esai_trigger,
> .hw_params = fsl_esai_hw_params,
> .set_sysclk = fsl_esai_set_dai_sysclk, @@ -947,6 +899,10 @@ static int
> fsl_esai_probe(struct platform_device *pdev)
> return ret;
> }
>
> + pm_runtime_enable(>dev);
> +
I just have a question, do I need to add pm_runtime_idle(>dev)?
Best regards
Wang shengjiu
[PATCH V4 3/3] ASoC: fsl_asrc: Unify the supported input and output rate
Unify the supported input and output rate, add the
12kHz/24kHz/128kHz to the support list
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 32 +++-
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 2c4bbc3499db..0d06e738264a 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -27,13 +27,14 @@
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
/* Corresponding to process_option */
-static int supported_input_rate[] = {
- 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
- 96000, 176400, 192000,
+static unsigned int supported_asrc_rate[] = {
+ 5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000,
+ 64000, 88200, 96000, 128000, 176400, 192000,
};
-static int supported_asrc_rate[] = {
- 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000,
176400, 192000,
+static struct snd_pcm_hw_constraint_list fsl_asrc_rate_constraints = {
+ .count = ARRAY_SIZE(supported_asrc_rate),
+ .list = supported_asrc_rate,
};
/**
@@ -293,11 +294,11 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
*pair)
ideal = config->inclk == INCLK_NONE;
/* Validate input and output sample rates */
- for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++)
- if (inrate == supported_input_rate[in])
+ for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++)
+ if (inrate == supported_asrc_rate[in])
break;
- if (in == ARRAY_SIZE(supported_input_rate)) {
+ if (in == ARRAY_SIZE(supported_asrc_rate)) {
pair_err("unsupported input sample rate: %dHz\n", inrate);
return -EINVAL;
}
@@ -311,7 +312,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
return -EINVAL;
}
- if ((outrate >= 8000 && outrate <= 3) &&
+ if ((outrate >= 5512 && outrate <= 3) &&
(outrate > 24 * inrate || inrate > 8 * outrate)) {
pair_err("exceed supported ratio range (1/24, 8) for
inrate/outrate: %d/%d\n",
inrate, outrate);
@@ -490,7 +491,9 @@ static int fsl_asrc_dai_startup(struct snd_pcm_substream
*substream,
snd_pcm_hw_constraint_step(substream->runtime, 0,
SNDRV_PCM_HW_PARAM_CHANNELS, 2);
- return 0;
+
+ return snd_pcm_hw_constraint_list(substream->runtime, 0,
+ SNDRV_PCM_HW_PARAM_RATE, _asrc_rate_constraints);
}
static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
@@ -603,7 +606,6 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai)
return 0;
}
-#define FSL_ASRC_RATES SNDRV_PCM_RATE_8000_192000
#define FSL_ASRC_FORMATS (SNDRV_PCM_FMTBIT_S24_LE | \
SNDRV_PCM_FMTBIT_S16_LE | \
SNDRV_PCM_FMTBIT_S20_3LE)
@@ -614,14 +616,18 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai)
.stream_name = "ASRC-Playback",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.capture = {
.stream_name = "ASRC-Capture",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.ops = _asrc_dai_ops,
--
1.9.1
[PATCH V4 2/3] ASoC: fsl_asrc: replace the process_option table with function
When we want to support more sample rate, for example 12kHz/24kHz
we need update the process_option table, if we want to support more
sample rate next time, the table need to be updated again. which
is not flexible.
We got a function fsl_asrc_sel_proc to replace the table, which can
give the pre-processing and post-processing options according to
the sample rate.
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 75 +++-
1 file changed, 55 insertions(+), 20 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 5b8adc7fb117..2c4bbc3499db 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -26,24 +26,6 @@
#define pair_dbg(fmt, ...) \
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
-/* Sample rates are aligned with that defined in pcm.h file */
-static const u8 process_option[][12][2] = {
- /* 8kHz 11.025kHz 16kHz 22.05kHz 32kHz 44.1kHz 48kHz 64kHz 88.2kHz
96kHz 176kHz 192kHz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 5512Hz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 8kHz */
- {{0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 11025Hz */
- {{1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 16kHz */
- {{1, 2}, {1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 22050Hz */
- {{1, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 0}, {0, 0}, {0, 0},}, /* 32kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 44.1kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 2}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 48kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 1}, {1, 2}, {0, 2}, {0, 2}, {0, 1}, {0,
1}, {0, 1}, {0, 1}, {0, 0},}, /* 64kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 88.2kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 96kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 176kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 192kHz */
-};
-
/* Corresponding to process_option */
static int supported_input_rate[] = {
5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
@@ -80,6 +62,51 @@
static unsigned char *clk_map[2];
/**
+ * Select the pre-processing and post-processing options
+ * Unsupport cases: Tsout > 8.125 * Tsin, Tsout > 16.125 * Tsin
+ *
+ * inrate: input sample rate
+ * outrate: output sample rate
+ * pre_proc: return value for pre-processing option
+ * post_proc: return value for post-processing option
+ */
+static int fsl_asrc_sel_proc(int inrate, int outrate,
+int *pre_proc, int *post_proc)
+{
+ bool post_proc_cond2;
+ bool post_proc_cond0;
+
+ /* select pre_proc between [0, 2] */
+ if (inrate * 8 > 33 * outrate)
+ *pre_proc = 2;
+ else if (inrate * 8 > 15 * outrate) {
+ if (inrate > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+ } else if (inrate < 76000)
+ *pre_proc = 0;
+ else if (inrate > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+
+ /* Condition for selection of post-processing */
+ post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) ||
+ (inrate > 56000 && outrate < 56000);
+ post_proc_cond0 = inrate * 23 < outrate * 8;
+
+ if (post_proc_cond2)
+ *post_proc = 2;
+ else if (post_proc_cond0)
+ *post_proc = 0;
+ else
+ *post_proc = 1;
+
+ return 0;
+}
+
+/**
* Request ASRC pair
*
* It assigns pair by the order of A->C->B because allocation of pair B,
@@ -239,8 +266,10 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
u32 inrate, outrate, indiv, outdiv;
u32 clk_index[2], div[2];
int in, out, channels;
+ int pre_proc, post_proc;
struct clk *clk;
bool ideal;
+ int ret;
if (!config) {
pair_err("invalid pair config\n");
@@ -289,6 +318,12 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
return -EINVAL;
}
+ ret = fsl_asrc_sel_proc(inrate, outrate, _proc, _proc);
+ if (ret) {
+
[PATCH V4 1/3] ASoC: fsl_asrc: Fix the issue about unsupported rate
When the output sample rate is [8kHz, 30kHz], the limitation
of the supported ratio range is (1/24, 8). In the driver
we use (8kHz, 30kHz) instead of [8kHz, 30kHz].
So this patch is to fix this issue and the potential rounding
issue with divider.
Fixes: fff6e03c7b65 ("ASoC: fsl_asrc: add support for 8-30kHz
output sample rate")
Cc:
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 0b937924d2e4..5b8adc7fb117 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -282,10 +282,10 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
*pair)
return -EINVAL;
}
- if ((outrate > 8000 && outrate < 3) &&
- (outrate/inrate > 24 || inrate/outrate > 8)) {
- pair_err("exceed supported ratio range [1/24, 8] for \
- inrate/outrate: %d/%d\n", inrate, outrate);
+ if ((outrate >= 8000 && outrate <= 3) &&
+ (outrate > 24 * inrate || inrate > 8 * outrate)) {
+ pair_err("exceed supported ratio range (1/24, 8) for
inrate/outrate: %d/%d\n",
+ inrate, outrate);
return -EINVAL;
}
--
1.9.1
[PATCH V4 0/3] Support more sample rate in asrc
Support more sample rate in asrc Shengjiu Wang (3): ASoC: fsl_asrc: Fix the issue about unsupported rate ASoC: fsl_asrc: replace the process_option table with function ASoC: fsl_asrc: Unify the supported input and output rate Changes in v4 - add patch to Fix the [8kHz, 30kHz] open set issue. Changes in v3 - remove FSL_ASRC_RATES - refine fsl_asrc_sel_proc according to comments Changes in v2 - add more comments in code - add commit "Unify the supported input and output rate" sound/soc/fsl/fsl_asrc.c | 113 --- 1 file changed, 77 insertions(+), 36 deletions(-) -- 1.9.1
Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
Hi
>
>
> On Thu, Apr 18, 2019 at 09:37:06AM +0000, S.j. Wang wrote:
> > > > > And this is according to IMX6DQRM:
> > > > > Limited support for the case when output sampling rates is
> > > > > between 8kHz and 30kHz. The limitation is the supported ratio
> > > > > (Fsin/Fsout) range as between 1/24 to 8
> > > > >
> > > > > This should cover your 8.125 condition already, even if having
> > > > > an outrate range between [8KHz, 30KHz] check, since an outrate
> > > > > above 30KHz will not have an inrate bigger than 8.125 times of
> > > > > it, given the maximum input rate is 192KHz.
> > > > >
> > > > > So I think that we can just drop that 8.125 condition from your
> > > > > change and there's no need to error out any more.
> > > > >
> > > > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported.
> > > > This is not covered by
> > > >
> > > > if ((outrate > 8000 && outrate < 3) &&
> > > > (outrate/inrate > 24 || inrate/outrate > 8)) {
> > >
> > > Good catch. The range should be [8KHz, 30KHz] vs. (8KHz, 32KHz) in
> > > the code. Then I think the fix should be at both lines:
> > >
> > > - if ((outrate > 8000 && outrate < 3) &&
> > > - (outrate/inrate > 24 || inrate/outrate > 8)) {
> > > + if ((outrate >= 8000 && outrate =< 3) &&
> > > + (outrate > 24 * inrate || inrate > 8 * outrate)) {
> > >
> > > Overall, I think we should fix this instead of adding an extra one,
> > > since it is very likely saying the same thing.
> >
> > Actually if outrate < 8kHz, there will be issue too.
>
> Here is the thing, the RM doesn't explicitly state that ASRC can support a
> lower output sample rate than 8KHz. And I actually had a concern when
> reviewing your PATCH-2, as the table of supported output sample rate no
> longer matches RM.
>
> If you've verified a lower output sample rate working solid with the
> process_option function, that means our driver can go beyond the
> limitation mentioned in the RM, then I believe [8KHz, 32KHz] should be
> updated too -- that says we can do:
> - if ((outrate > 8000 && outrate < 3) &&
> - (outrate/inrate > 24 || inrate/outrate > 8)) {
> + if ((outrate >= 5512 && outrate =< 3) &&
> + (outrate > 24 * inrate || inrate > 8 * outrate)) {
>
> Actually "ourate > 24 * inrate" is kind of pointless for range [5KHz, 32KHz]
> but we can keep it since it matches RM.
Ok, will send v4.
Best regards
Wang shengjiu
[PATCH V2] ASoC: fsl_esai: Add pm runtime function
Add pm runtime support and move clock handling there.
fsl_esai_suspend is replaced by pm_runtime_force_suspend.
fsl_esai_resume is replaced by pm_runtime_force_resume.
Signed-off-by: Shengjiu Wang
---
Changes in v2
-refine the commit comments.
-move regcache_mark_dirty to runtime suspend.
sound/soc/fsl/fsl_esai.c | 141 ++-
1 file changed, 77 insertions(+), 64 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index bad0dfed6b68..10d2210c91ef 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
@@ -466,30 +467,6 @@ static int fsl_esai_startup(struct snd_pcm_substream
*substream,
struct snd_soc_dai *dai)
{
struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
- int ret;
-
- /*
-* Some platforms might use the same bit to gate all three or two of
-* clocks, so keep all clocks open/close at the same time for safety
-*/
- ret = clk_prepare_enable(esai_priv->coreclk);
- if (ret)
- return ret;
- if (!IS_ERR(esai_priv->spbaclk)) {
- ret = clk_prepare_enable(esai_priv->spbaclk);
- if (ret)
- goto err_spbaclk;
- }
- if (!IS_ERR(esai_priv->extalclk)) {
- ret = clk_prepare_enable(esai_priv->extalclk);
- if (ret)
- goto err_extalck;
- }
- if (!IS_ERR(esai_priv->fsysclk)) {
- ret = clk_prepare_enable(esai_priv->fsysclk);
- if (ret)
- goto err_fsysclk;
- }
if (!dai->active) {
/* Set synchronous mode */
@@ -506,16 +483,6 @@ static int fsl_esai_startup(struct snd_pcm_substream
*substream,
return 0;
-err_fsysclk:
- if (!IS_ERR(esai_priv->extalclk))
- clk_disable_unprepare(esai_priv->extalclk);
-err_extalck:
- if (!IS_ERR(esai_priv->spbaclk))
- clk_disable_unprepare(esai_priv->spbaclk);
-err_spbaclk:
- clk_disable_unprepare(esai_priv->coreclk);
-
- return ret;
}
static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
@@ -576,20 +543,6 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
*substream,
return 0;
}
-static void fsl_esai_shutdown(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
-{
- struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
-
- if (!IS_ERR(esai_priv->fsysclk))
- clk_disable_unprepare(esai_priv->fsysclk);
- if (!IS_ERR(esai_priv->extalclk))
- clk_disable_unprepare(esai_priv->extalclk);
- if (!IS_ERR(esai_priv->spbaclk))
- clk_disable_unprepare(esai_priv->spbaclk);
- clk_disable_unprepare(esai_priv->coreclk);
-}
-
static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *dai)
{
@@ -658,7 +611,6 @@ static int fsl_esai_trigger(struct snd_pcm_substream
*substream, int cmd,
static const struct snd_soc_dai_ops fsl_esai_dai_ops = {
.startup = fsl_esai_startup,
- .shutdown = fsl_esai_shutdown,
.trigger = fsl_esai_trigger,
.hw_params = fsl_esai_hw_params,
.set_sysclk = fsl_esai_set_dai_sysclk,
@@ -947,6 +899,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
return ret;
}
+ pm_runtime_enable(>dev);
+
+ regcache_cache_only(esai_priv->regmap, true);
+
ret = imx_pcm_dma_init(pdev, IMX_ESAI_DMABUF_SIZE);
if (ret)
dev_err(>dev, "failed to init imx pcm dma: %d\n", ret);
@@ -954,6 +910,13 @@ static int fsl_esai_probe(struct platform_device *pdev)
return ret;
}
+static int fsl_esai_remove(struct platform_device *pdev)
+{
+ pm_runtime_disable(>dev);
+
+ return 0;
+}
+
static const struct of_device_id fsl_esai_dt_ids[] = {
{ .compatible = "fsl,imx35-esai", },
{ .compatible = "fsl,vf610-esai", },
@@ -961,22 +924,35 @@ static int fsl_esai_probe(struct platform_device *pdev)
};
MODULE_DEVICE_TABLE(of, fsl_esai_dt_ids);
-#ifdef CONFIG_PM_SLEEP
-static int fsl_esai_suspend(struct device *dev)
-{
- struct fsl_esai *esai = dev_get_drvdata(dev);
-
- regcache_cache_only(esai->regmap, true);
- regcache_mark_dirty(esai->regmap);
-
- return 0;
-}
-
-static int fsl_esai_resume(struct device *dev)
+#ifdef CONFIG_PM
+static int fsl_esai_runtime_resume(struct device *dev)
{
struct fsl_esai *esai = dev_get_drvdata(dev);
int ret;
+ /*
+* Some platforms might use the same bit to gate all three or two of
+* clocks, so keep all clocks open/close at the same time for safety
+*/
+ ret = clk_prepare_enable(esai->coreclk);
+ if (ret)
+
Re: [PATCH] ASoC: fsl_esai: Add pm runtime function
Hi > On Thu, Apr 18, 2019 at 10:15:24AM +0000, S.j. Wang wrote: > > > On Thu, Apr 18, 2019 at 02:00:12AM -0700, Nicolin Chen wrote: > > > > On Thu, Apr 18, 2019 at 03:29:09AM +, S.j. Wang wrote: > > > > > Just for curiosity, we had similar situation on imx6sx, so we > > > > added suspend/resume with regcache. Why will the clock enable > > > > state be lost too? Does CCM on imx8 (might not be called CCM > > > > though) have any difference? What about clock rate settings? > > > > That sounds like a bug somewhere else - I'd expect that after resume > > > the clocking would be restored to the state it was in before suspend. > > > There is limitation in our internal design. That is in imx8 the power > > of subsystem will be disabled at suspend, include the clock state , clock > rate. > > Right, that's fairly normal but usually it'd be restored as part of the resume > process? > > > This patch is to enable the pm runtime, so I think it is better to > > move the clock operation to pm runtime, and close the clock at > > suspend to reduce the power. > > It's definitely good to turn the clock off as much as possible, yes. Thanks, will send v2. Best regards Wang shengjiu
Re: [PATCH] ASoC: fsl_esai: Add pm runtime function
Hi > > > On Thu, Apr 18, 2019 at 03:29:09AM +0000, S.j. Wang wrote: > > In imx8 when systerm enter suspend state, the power of subsystem will > > be off, the clock enable state will be lost and register configuration > > Just for curiosity, we had similar situation on imx6sx, so we added > suspend/resume with regcache. Why will the clock enable state be lost too? > Does CCM on imx8 (might not be called CCM > though) have any difference? What about clock rate settings? > > > will be lost. So the driver need to enter runtime suspend state in > > suspend. > > > With this implementation the suspend function almost same as runtime > > suspend function, so remove the suspend function, just use > > pm_runtime_force_suspend instead, and same for the resume function. > > > > And also need to move clock enablement to runtime resume and clock > > disablement to runtime suspend. > > > > -static int fsl_esai_suspend(struct device *dev) > > - regcache_cache_only(esai->regmap, true); > > - regcache_mark_dirty(esai->regmap); > > > +static int fsl_esai_runtime_resume(struct device *dev) > > regcache_cache_only(esai->regmap, false); > > + regcache_mark_dirty(esai->regmap); > > Why move the regcache_mark_dirty from suspend to resume? > (I am not saying it's wrong but wondering if this is the preferable way.) Seems I should not do this change. I will remain regcache_mark_dirty(esai->regmap); at its old place. Best regards Wang shengjiu
Re: [PATCH] ASoC: fsl_esai: Add pm runtime function
Hi > > On Thu, Apr 18, 2019 at 02:00:12AM -0700, Nicolin Chen wrote: > > On Thu, Apr 18, 2019 at 03:29:09AM +0000, S.j. Wang wrote: > > > > In imx8 when systerm enter suspend state, the power of subsystem > > > will be off, the clock enable state will be lost and register > > > configuration > > > Just for curiosity, we had similar situation on imx6sx, so we added > > suspend/resume with regcache. Why will the clock enable state be lost > > too? Does CCM on imx8 (might not be called CCM > > though) have any difference? What about clock rate settings? > > That sounds like a bug somewhere else - I'd expect that after resume the > clocking would be restored to the state it was in before suspend. There is limitation in our internal design. That is in imx8 the power of subsystem will be disabled at suspend, include the clock state , clock rate. I should not add it in comments, please ignore them, I will change the description. This patch is to enable the pm runtime, so I think it is better to move the clock operation to pm runtime, and close the clock at suspend to reduce the power. Best regards Wang shengjiu
Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
Hi
>
> On Thu, Apr 18, 2019 at 08:50:48AM +0000, S.j. Wang wrote:
> > > And this is according to IMX6DQRM:
> > > Limited support for the case when output sampling rates is
> > > between 8kHz and 30kHz. The limitation is the supported ratio
> > > (Fsin/Fsout) range as between 1/24 to 8
> > >
> > > This should cover your 8.125 condition already, even if having an
> > > outrate range between [8KHz, 30KHz] check, since an outrate above
> > > 30KHz will not have an inrate bigger than 8.125 times of it, given
> > > the maximum input rate is 192KHz.
> > >
> > > So I think that we can just drop that 8.125 condition from your
> > > change and there's no need to error out any more.
> > >
> > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported.
> > This is not covered by
> >
> > if ((outrate > 8000 && outrate < 3) &&
> > (outrate/inrate > 24 || inrate/outrate > 8)) {
>
> Good catch. The range should be [8KHz, 30KHz] vs. (8KHz, 32KHz) in the
> code. Then I think the fix should be at both lines:
>
> - if ((outrate > 8000 && outrate < 3) &&
> - (outrate/inrate > 24 || inrate/outrate > 8)) {
> + if ((outrate >= 8000 && outrate =< 3) &&
> + (outrate > 24 * inrate || inrate > 8 * outrate)) {
>
> Overall, I think we should fix this instead of adding an extra one, since it
> is
> very likely saying the same thing.
Actually if outrate < 8kHz, there will be issue too.
Best regards
Wang shengjiu
Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
Hi
>
>
> On Thu, Apr 18, 2019 at 02:37:03AM +0000, S.j. Wang wrote:
> > > Here:
> > > > + /* Does not support cases: Tsout > 8.125 * Tsin */
> > > > + if (inrate * 8 > 65 * outrate)
>
> Though it might not matter any more (see my last comments), it should be
> "inrate > 8.125 * outrate" in the comments.
>
> > > > + return -EINVAL;
> > > And here:
> > > > + ret = fsl_asrc_sel_proc(inrate, outrate, _proc, _proc);
> > > > + if (ret) {
> > > > + pair_err("No supported pre-processing options\n");
> > > > + return ret;
> > > > + }
> > >
> > > Instead of a general message, I was thinking of a more specific one
> > > by telling users that the ratio between the two rates isn't
> > > supported -- something similar to what I suggested previously:
> > >
> > > pair_err("Does not support %d (input) > 8.125 * %d (output)\n",
> > > outrate, inrate);
> > >
>
> > In fsl_asrc_sel_proc, we can't call the pair_err for there is no
> > struct fsl_asrc_pair *pair in the argument. Do you think we need to
> > add this argument?
>
> I's thinking of adding it to the top of fsl_asrc_config_pair() as a part of
> inrate-outrate-validation, however, I found that actually we already have a
> similar check in the early routine:
> if ((outrate > 8000 && outrate < 3) &&
> (outrate/inrate > 24 || inrate/outrate > 8)) {
> pair_err("exceed supported ratio range [1/24, 8] for \
> inrate/outrate: %d/%d\n", inrate, outrate);
> return -EINVAL;
> }
>
> And this is according to IMX6DQRM:
> Limited support for the case when output sampling rates is
> between 8kHz and 30kHz. The limitation is the supported ratio
> (Fsin/Fsout) range as between 1/24 to 8
>
> This should cover your 8.125 condition already, even if having an outrate
> range between [8KHz, 30KHz] check, since an outrate above 30KHz will not
> have an inrate bigger than 8.125 times of it, given the maximum input rate
> is 192KHz.
>
> So I think that we can just drop that 8.125 condition from your change and
> there's no need to error out any more.
>
No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported.
This is not covered by
if ((outrate > 8000 && outrate < 3) &&
(outrate/inrate > 24 || inrate/outrate > 8)) {
> However, we do need a patch to fix a potential rounding issue:
> - (outrate/inrate > 24 || inrate/outrate > 8)) {
> + (outrate > 24 * inrate || inrate > 8 * outrate)) {
>
> Should fix the missing whitespace also. And it will be needed to send to
> stable kernel too. Will you help submit a change?
>
> Thanks
[PATCH] ASoC: fsl_esai: Add pm runtime function
In imx8 when systerm enter suspend state, the power of subsystem will
be off, the clock enable state will be lost and register configuration
will be lost. So the driver need to enter runtime suspend state in
suspend.
With this implementation the suspend function almost same as runtime
suspend function, so remove the suspend function, just use
pm_runtime_force_suspend instead, and same for the resume function.
And also need to move clock enablement to runtime resume and clock
disablement to runtime suspend.
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_esai.c | 141 ++-
1 file changed, 77 insertions(+), 64 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index bad0dfed6b68..b1e27db3752b 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
@@ -466,30 +467,6 @@ static int fsl_esai_startup(struct snd_pcm_substream
*substream,
struct snd_soc_dai *dai)
{
struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
- int ret;
-
- /*
-* Some platforms might use the same bit to gate all three or two of
-* clocks, so keep all clocks open/close at the same time for safety
-*/
- ret = clk_prepare_enable(esai_priv->coreclk);
- if (ret)
- return ret;
- if (!IS_ERR(esai_priv->spbaclk)) {
- ret = clk_prepare_enable(esai_priv->spbaclk);
- if (ret)
- goto err_spbaclk;
- }
- if (!IS_ERR(esai_priv->extalclk)) {
- ret = clk_prepare_enable(esai_priv->extalclk);
- if (ret)
- goto err_extalck;
- }
- if (!IS_ERR(esai_priv->fsysclk)) {
- ret = clk_prepare_enable(esai_priv->fsysclk);
- if (ret)
- goto err_fsysclk;
- }
if (!dai->active) {
/* Set synchronous mode */
@@ -506,16 +483,6 @@ static int fsl_esai_startup(struct snd_pcm_substream
*substream,
return 0;
-err_fsysclk:
- if (!IS_ERR(esai_priv->extalclk))
- clk_disable_unprepare(esai_priv->extalclk);
-err_extalck:
- if (!IS_ERR(esai_priv->spbaclk))
- clk_disable_unprepare(esai_priv->spbaclk);
-err_spbaclk:
- clk_disable_unprepare(esai_priv->coreclk);
-
- return ret;
}
static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
@@ -576,20 +543,6 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
*substream,
return 0;
}
-static void fsl_esai_shutdown(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
-{
- struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
-
- if (!IS_ERR(esai_priv->fsysclk))
- clk_disable_unprepare(esai_priv->fsysclk);
- if (!IS_ERR(esai_priv->extalclk))
- clk_disable_unprepare(esai_priv->extalclk);
- if (!IS_ERR(esai_priv->spbaclk))
- clk_disable_unprepare(esai_priv->spbaclk);
- clk_disable_unprepare(esai_priv->coreclk);
-}
-
static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *dai)
{
@@ -658,7 +611,6 @@ static int fsl_esai_trigger(struct snd_pcm_substream
*substream, int cmd,
static const struct snd_soc_dai_ops fsl_esai_dai_ops = {
.startup = fsl_esai_startup,
- .shutdown = fsl_esai_shutdown,
.trigger = fsl_esai_trigger,
.hw_params = fsl_esai_hw_params,
.set_sysclk = fsl_esai_set_dai_sysclk,
@@ -947,6 +899,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
return ret;
}
+ pm_runtime_enable(>dev);
+
+ regcache_cache_only(esai_priv->regmap, true);
+
ret = imx_pcm_dma_init(pdev, IMX_ESAI_DMABUF_SIZE);
if (ret)
dev_err(>dev, "failed to init imx pcm dma: %d\n", ret);
@@ -954,6 +910,13 @@ static int fsl_esai_probe(struct platform_device *pdev)
return ret;
}
+static int fsl_esai_remove(struct platform_device *pdev)
+{
+ pm_runtime_disable(>dev);
+
+ return 0;
+}
+
static const struct of_device_id fsl_esai_dt_ids[] = {
{ .compatible = "fsl,imx35-esai", },
{ .compatible = "fsl,vf610-esai", },
@@ -961,23 +924,37 @@ static int fsl_esai_probe(struct platform_device *pdev)
};
MODULE_DEVICE_TABLE(of, fsl_esai_dt_ids);
-#ifdef CONFIG_PM_SLEEP
-static int fsl_esai_suspend(struct device *dev)
-{
- struct fsl_esai *esai = dev_get_drvdata(dev);
-
- regcache_cache_only(esai->regmap, true);
- regcache_mark_dirty(esai->regmap);
-
- return 0;
-}
-
-static int fsl_esai_resume(struct device *dev)
+#ifdef CONFIG_PM
+static int fsl_esai_runtime_resume(struct device *dev)
{
struct fsl_esai *esai = dev_get_drvdata(dev);
Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
Hi
>
> Hi Shengjiu,
>
> This looks better. Just a couple of more small comments inline.
>
> On Wed, Apr 17, 2019 at 09:06:18AM +, S.j. Wang wrote:
>
> > +static int fsl_asrc_sel_proc(int inrate, int outrate, int *pre_proc,
> > + int *post_proc)
>
> Just a nit: it looks better by grouping them two-two.
>
> static int fsl_asrc_sel_proc(int inrate, int outrate,
> int *pre_proc, int *post_proc)
>
> > + /* Condition for selection of post-processing */
> > + post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) ||
> > + (inrate > 56000 && outrate < 56000);
>
> Could align the indentation:
> post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) ||
> (inrate > 56000 && outrate < 56000);
>
> Here:
> > + /* Does not support cases: Tsout > 8.125 * Tsin */
> > + if (inrate * 8 > 65 * outrate)
> > + return -EINVAL;
> And here:
> > + ret = fsl_asrc_sel_proc(inrate, outrate, _proc, _proc);
> > + if (ret) {
> > + pair_err("No supported pre-processing options\n");
> > + return ret;
> > + }
>
> Instead of a general message, I was thinking of a more specific one by
> telling users that the ratio between the two rates isn't supported --
> something similar to what I suggested previously:
>
> pair_err("Does not support %d (input) > 8.125 * %d (output)\n",
> outrate, inrate);
>
In fsl_asrc_sel_proc, we can't call the pair_err for there is no
struct fsl_asrc_pair *pair in the argument. Do you think we need to
add this argument?
> Thanks
> Nicolin
[PATCH V3 2/2] ASoC: fsl_asrc: Unify the supported input and output rate
Unify the supported input and output rate, add the
12kHz/24kHz/128kHz to the support list
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index d34d539d01f2..b0d19b787bb8 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -27,13 +27,14 @@
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
/* Corresponding to process_option */
-static int supported_input_rate[] = {
- 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
- 96000, 176400, 192000,
+static unsigned int supported_asrc_rate[] = {
+ 5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000,
+ 64000, 88200, 96000, 128000, 176400, 192000,
};
-static int supported_asrc_rate[] = {
- 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000,
176400, 192000,
+static struct snd_pcm_hw_constraint_list fsl_asrc_rate_constraints = {
+ .count = ARRAY_SIZE(supported_asrc_rate),
+ .list = supported_asrc_rate,
};
/**
@@ -296,11 +297,11 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
*pair)
ideal = config->inclk == INCLK_NONE;
/* Validate input and output sample rates */
- for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++)
- if (inrate == supported_input_rate[in])
+ for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++)
+ if (inrate == supported_asrc_rate[in])
break;
- if (in == ARRAY_SIZE(supported_input_rate)) {
+ if (in == ARRAY_SIZE(supported_asrc_rate)) {
pair_err("unsupported input sample rate: %dHz\n", inrate);
return -EINVAL;
}
@@ -493,7 +494,9 @@ static int fsl_asrc_dai_startup(struct snd_pcm_substream
*substream,
snd_pcm_hw_constraint_step(substream->runtime, 0,
SNDRV_PCM_HW_PARAM_CHANNELS, 2);
- return 0;
+
+ return snd_pcm_hw_constraint_list(substream->runtime, 0,
+ SNDRV_PCM_HW_PARAM_RATE, _asrc_rate_constraints);
}
static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
@@ -606,7 +609,6 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai)
return 0;
}
-#define FSL_ASRC_RATES SNDRV_PCM_RATE_8000_192000
#define FSL_ASRC_FORMATS (SNDRV_PCM_FMTBIT_S24_LE | \
SNDRV_PCM_FMTBIT_S16_LE | \
SNDRV_PCM_FMTBIT_S20_3LE)
@@ -617,14 +619,18 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai)
.stream_name = "ASRC-Playback",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.capture = {
.stream_name = "ASRC-Capture",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.ops = _asrc_dai_ops,
--
1.9.1
[PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
When we want to support more sample rate, for example 12kHz/24kHz
we need update the process_option table, if we want to support more
sample rate next time, the table need to be updated again. which
is not flexible.
We got a function fsl_asrc_sel_proc to replace the table, which can
give the pre-processing and post-processing options according to
the sample rate.
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 78 +++-
1 file changed, 58 insertions(+), 20 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 0b937924d2e4..d34d539d01f2 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -26,24 +26,6 @@
#define pair_dbg(fmt, ...) \
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
-/* Sample rates are aligned with that defined in pcm.h file */
-static const u8 process_option[][12][2] = {
- /* 8kHz 11.025kHz 16kHz 22.05kHz 32kHz 44.1kHz 48kHz 64kHz 88.2kHz
96kHz 176kHz 192kHz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 5512Hz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 8kHz */
- {{0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 11025Hz */
- {{1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 16kHz */
- {{1, 2}, {1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 22050Hz */
- {{1, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 0}, {0, 0}, {0, 0},}, /* 32kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 44.1kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 2}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 48kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 1}, {1, 2}, {0, 2}, {0, 2}, {0, 1}, {0,
1}, {0, 1}, {0, 1}, {0, 0},}, /* 64kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 88.2kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 96kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 176kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 192kHz */
-};
-
/* Corresponding to process_option */
static int supported_input_rate[] = {
5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
@@ -80,6 +62,54 @@
static unsigned char *clk_map[2];
/**
+ * Select the pre-processing and post-processing options
+ *
+ * inrate: input sample rate
+ * outrate: output sample rate
+ * pre_proc: return value for pre-processing option
+ * post_proc: return value for post-processing option
+ */
+static int fsl_asrc_sel_proc(int inrate, int outrate, int *pre_proc,
+int *post_proc)
+{
+ bool post_proc_cond2;
+ bool post_proc_cond0;
+
+ /* Does not support cases: Tsout > 8.125 * Tsin */
+ if (inrate * 8 > 65 * outrate)
+ return -EINVAL;
+
+ /* Otherwise, select pre_proc between [0, 2] */
+ if (inrate * 8 > 33 * outrate)
+ *pre_proc = 2;
+ else if (inrate * 8 > 15 * outrate) {
+ if (inrate > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+ } else if (inrate < 76000)
+ *pre_proc = 0;
+ else if (inrate > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+
+ /* Condition for selection of post-processing */
+ post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) ||
+ (inrate > 56000 && outrate < 56000);
+ post_proc_cond0 = inrate * 23 < outrate * 8;
+
+ if (post_proc_cond2)
+ *post_proc = 2;
+ else if (post_proc_cond0)
+ *post_proc = 0;
+ else
+ *post_proc = 1;
+
+ return 0;
+}
+
+/**
* Request ASRC pair
*
* It assigns pair by the order of A->C->B because allocation of pair B,
@@ -239,8 +269,10 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
u32 inrate, outrate, indiv, outdiv;
u32 clk_index[2], div[2];
int in, out, channels;
+ int pre_proc, post_proc;
struct clk *clk;
bool ideal;
+ int ret;
if (!config) {
pair_err("invalid pair config\n");
@@ -289,6 +321,12 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
return -EINVAL;
}
+ ret =
[PATCH V3 0/2] Support more sample rate in asrc
Support more sample rate in asrc Shengjiu Wang (2): ASoC: fsl_asrc: replace the process_option table with function ASoC: fsl_asrc: Unify the supported input and output rate Changes in v3 - remove FSL_ASRC_RATES - refine fsl_asrc_sel_proc according to comments Changes in v2 - add more comments in code - add commit "Unify the supported input and output rate" sound/soc/fsl/fsl_asrc.c | 108 +-- 1 file changed, 76 insertions(+), 32 deletions(-) -- 1.9.1
Re: [PATCH V2 1/2] ASoC: fsl_asrc: replace the process_option table with function
Hi
>
>
> On Thu, Apr 11, 2019 at 09:39:06AM +0000, S.j. Wang wrote:
>
> > +/*
> > + * Select the pre-processing and post-processing options
>
> By aligning with other function comments:
> /**
> * Select the pre-processing and post-processing options
>
> > + *
> > + * Fsin: input sample rate
> > + * Fsout: output sample rate
>
> I would suggest to use inrate and outrate to keep naming consistent.
>
> > + * pre_proc: return value for pre-processing option
> > + * post_proc: return value for post-processing option */ static int
> > +fsl_asrc_sel_proc(int Fsin, int Fsout, int *pre_proc, int *post_proc)
> > +{
> > + bool det_out_op2_cond;
> > + bool det_out_op0_cond;
>
> By looking at the comments below. Probably better to rename them
> bool post_proc_cond2, post_proc_cond0;
>
> > + /* Codition for selection of post-processing */
>
> "Codition" -> "Conditions"
>
> > + det_out_op2_cond = (((Fsin * 15 > Fsout * 16) & (Fsout < 56000)) |
> > + ((Fsin > 56000) & (Fsout <
> > + 56000)));
>
> Combining Daniel's comments + indentation alignment:
> det_out_op2_cond = (Fsin * 15 > Fsout * 16 && Fsout < 56000) ||
>(Fsin > 56000 && Fsout < 56000);
>
> > + det_out_op0_cond = (Fsin * 23 < Fsout * 8);
> > +
> > + /*
> > + * unsupported case: Tsout>16.125*Tsin, and Tsout>8.125*Tsin.
>
> Funny thing is that there'd be no point in checking the 16.125, if Tsout is
> bigger than 8.125 times of Tsin, i.e. only one condition:
> Tsout > 8.125 * Tsin
>
> > + * Tsout>16.125*Tsin -> Fsin * 8 > 129 * Fsout
> > + * Tsout>8.125*Tsin -> Fsin * 8 > 65 * Fsout
> > + * Tsout>4.125*Tsin -> Fsin * 8 > 33 * Fsout
> > + * Tsout>1.875*Tsin -> Fsin * 8 > 15 * Fsout
>
> Took me a while to understand what it is saying
>
> Better to write like this:
> /* Does not support cases: Tsout > 8.125 * Tsin */
> if (Fsin * 8 > 65 * Fsout) {
> pair_err("Does not support %d > 8.125 * %d\n", Fsout, Fsin);
> return -EINVAL;
> }
>
> /* Otherwise, select pre_proc between [0, 2] */
> if (Fsin * 8 > 33 * Fsout)
> > + *pre_proc = 2;
> > + else if (Fsin * 8 > 15 * Fsout) {
> > + if (Fsin > 152000)
> > + *pre_proc = 2;
> > + else
> > + *pre_proc = 1;
> > + } else if (Fsin < 76000)
> > + *pre_proc = 0;
> > + else if (Fsin > 152000)
> > + *pre_proc = 2;
> > + else
> > + *pre_proc = 1;
>
> <== Would look better by moving post_cond calculations here.
>
> > + if (det_out_op2_cond)
> > + *post_proc = 2;
> > + else if (det_out_op0_cond)
> > + *post_proc = 0;
> > + else
> > + *post_proc = 1;
>
> And we could remove this check below:
> > + /* unsupported options */
> > + if (*pre_proc == 4 || *pre_proc == 5)
> > + return -EINVAL;
>
> So basically we are doing:
> 1) Error out unsupported cases
> 2) Select pre_proc
> 3) Calculate conditions for post_proc
> 4) Select post_proc
>
> Thanks
Thanks for reviewing, will send v3.
[PATCH V2 2/2] ASoC: fsl_asrc: Unify the supported input and output rate
Unify the supported input and output rate, add the
12kHz/24kHz/128kHz to the support list
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 29 ++---
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 5857d383d962..44bcc4a7b23b 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -27,13 +27,14 @@
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
/* Corresponding to process_option */
-static int supported_input_rate[] = {
- 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
- 96000, 176400, 192000,
+static unsigned int supported_asrc_rate[] = {
+ 5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000,
+ 64000, 88200, 96000, 128000, 176400, 192000,
};
-static int supported_asrc_rate[] = {
- 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000,
176400, 192000,
+static struct snd_pcm_hw_constraint_list fsl_asrc_rate_constraints = {
+ .count = ARRAY_SIZE(supported_asrc_rate),
+ .list = supported_asrc_rate,
};
/**
@@ -305,11 +306,11 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
*pair)
ideal = config->inclk == INCLK_NONE;
/* Validate input and output sample rates */
- for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++)
- if (inrate == supported_input_rate[in])
+ for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++)
+ if (inrate == supported_asrc_rate[in])
break;
- if (in == ARRAY_SIZE(supported_input_rate)) {
+ if (in == ARRAY_SIZE(supported_asrc_rate)) {
pair_err("unsupported input sample rate: %dHz\n", inrate);
return -EINVAL;
}
@@ -502,7 +503,9 @@ static int fsl_asrc_dai_startup(struct snd_pcm_substream
*substream,
snd_pcm_hw_constraint_step(substream->runtime, 0,
SNDRV_PCM_HW_PARAM_CHANNELS, 2);
- return 0;
+
+ return snd_pcm_hw_constraint_list(substream->runtime, 0,
+ SNDRV_PCM_HW_PARAM_RATE, _asrc_rate_constraints);
}
static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
@@ -626,14 +629,18 @@ static int fsl_asrc_dai_probe(struct snd_soc_dai *dai)
.stream_name = "ASRC-Playback",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.capture = {
.stream_name = "ASRC-Capture",
.channels_min = 1,
.channels_max = 10,
- .rates = FSL_ASRC_RATES,
+ .rate_min = 5512,
+ .rate_max = 192000,
+ .rates = SNDRV_PCM_RATE_KNOT,
.formats = FSL_ASRC_FORMATS,
},
.ops = _asrc_dai_ops,
--
1.9.1
[PATCH V2 1/2] ASoC: fsl_asrc: replace the process_option table with function
When we want to support more sample rate, for example 12kHz/24kHz
we need update the process_option table, if we want to support more
sample rate next time, the table need to be updated again. which
is not flexible.
We got a function fsl_asrc_sel_proc to replace the table, which can
give the pre-processing and post-processing options according to
the sample rate.
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 87 +---
1 file changed, 67 insertions(+), 20 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 0b937924d2e4..5857d383d962 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -26,24 +26,6 @@
#define pair_dbg(fmt, ...) \
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
-/* Sample rates are aligned with that defined in pcm.h file */
-static const u8 process_option[][12][2] = {
- /* 8kHz 11.025kHz 16kHz 22.05kHz 32kHz 44.1kHz 48kHz 64kHz 88.2kHz
96kHz 176kHz 192kHz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 5512Hz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 8kHz */
- {{0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 11025Hz */
- {{1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 16kHz */
- {{1, 2}, {1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 22050Hz */
- {{1, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 0}, {0, 0}, {0, 0},}, /* 32kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 44.1kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 2}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 48kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 1}, {1, 2}, {0, 2}, {0, 2}, {0, 1}, {0,
1}, {0, 1}, {0, 1}, {0, 0},}, /* 64kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 88.2kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 96kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 176kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 192kHz */
-};
-
/* Corresponding to process_option */
static int supported_input_rate[] = {
5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
@@ -79,6 +61,63 @@
static unsigned char *clk_map[2];
+/*
+ * Select the pre-processing and post-processing options
+ *
+ * Fsin: input sample rate
+ * Fsout: output sample rate
+ * pre_proc: return value for pre-processing option
+ * post_proc: return value for post-processing option
+ */
+static int fsl_asrc_sel_proc(int Fsin, int Fsout, int *pre_proc, int
*post_proc)
+{
+ bool det_out_op2_cond;
+ bool det_out_op0_cond;
+
+ /* Codition for selection of post-processing */
+ det_out_op2_cond = (((Fsin * 15 > Fsout * 16) & (Fsout < 56000)) |
+ ((Fsin > 56000) & (Fsout < 56000)));
+ det_out_op0_cond = (Fsin * 23 < Fsout * 8);
+
+ /*
+* unsupported case: Tsout>16.125*Tsin, and Tsout>8.125*Tsin.
+* Tsout>16.125*Tsin -> Fsin * 8 > 129 * Fsout
+* Tsout>8.125*Tsin -> Fsin * 8 > 65 * Fsout
+* Tsout>4.125*Tsin -> Fsin * 8 > 33 * Fsout
+* Tsout>1.875*Tsin -> Fsin * 8 > 15 * Fsout
+*/
+ if (Fsin * 8 > 129 * Fsout)
+ *pre_proc = 5;
+ else if (Fsin * 8 > 65 * Fsout)
+ *pre_proc = 4;
+ else if (Fsin * 8 > 33 * Fsout)
+ *pre_proc = 2;
+ else if (Fsin * 8 > 15 * Fsout) {
+ if (Fsin > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+ } else if (Fsin < 76000)
+ *pre_proc = 0;
+ else if (Fsin > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+
+ if (det_out_op2_cond)
+ *post_proc = 2;
+ else if (det_out_op0_cond)
+ *post_proc = 0;
+ else
+ *post_proc = 1;
+
+ /* unsupported options */
+ if (*pre_proc == 4 || *pre_proc == 5)
+ return -EINVAL;
+
+ return 0;
+}
+
/**
* Request ASRC pair
*
@@ -239,8 +278,10 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
u32 inrate, outrate, indiv, outdiv;
u32 clk_index[2], div[2];
int in, out, channels;
+ int pre_proc, post_proc;
struct
[PATCH V2 0/2] Support more sample rate in asrc
Support more sample rate in asrc Shengjiu Wang (2): ASoC: fsl_asrc: replace the process_option table with function ASoC: fsl_asrc: Unify the supported input and output rate Changes in v2 - add more comments in code - add commit "Unify the supported input and output rate" sound/soc/fsl/fsl_asrc.c | 116 ++- 1 file changed, 85 insertions(+), 31 deletions(-) -- 1.9.1
[PATCH V5] ASoC: fsl_esai: Fix missing break in switch statement
case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be
independent of each other, so replace fall-through with break.
Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
Cc:
---
Changes in v5
- remove new line after Fixes
Changes in v4
- Add acked-by
Changes in v3
- Update subject line and cc stable
Changes in v2
- Fix "Fixes" tag
sound/soc/fsl/fsl_esai.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 3623aa9a6f2e..15202a637197 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id,
break;
case ESAI_HCKT_EXTAL:
ecr |= ESAI_ECR_ETI;
- /* fall through */
+ break;
case ESAI_HCKR_EXTAL:
ecr |= ESAI_ECR_ERI;
break;
--
1.9.1
RE: [EXT] Re: [PATCH V2] ASoC: fsl_esai: replace fall-through with break
Hi Mark > > On Wed, Apr 10, 2019 at 02:42:45AM +0000, S.j. Wang wrote: > > case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be independent > of > > each other, so replace fall-through with break. > > This doesn't apply against current code, please check and resend. Thanks, have sent v4 for update subject according to Gustavo's comments. [PATCH V4] ASoC: fsl_esai: Fix missing break in switch statement Best regards Wang shengjiu
[PATCH V4] ASoC: fsl_esai: Fix missing break in switch statement
case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be
independent of each other, so replace fall-through with break.
Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
Cc:
---
Change in v4
- Add Acked-by and cc stable
- change the subject
sound/soc/fsl/fsl_esai.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 3623aa9a6f2e..15202a637197 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id,
break;
case ESAI_HCKT_EXTAL:
ecr |= ESAI_ECR_ETI;
- /* fall through */
+ break;
case ESAI_HCKR_EXTAL:
ecr |= ESAI_ECR_ERI;
break;
--
1.9.1
RE: [EXT] [alsa-devel] [PATCH V3] ASoC: fsl_esai: Fix missing break in switch statement
Hi
>
>
> case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be independent
> of each other, so replace fall-through with break.
>
> Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
>
> Signed-off-by: Shengjiu Wang
> Cc:
Forget to add Acked-by: Nicolin Chen , will send v4,
sorry.
> ---
> changes in v3
> - add cc stable
> - change the subject
>
> sound/soc/fsl/fsl_esai.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> 3623aa9a6f2e..15202a637197 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai
> *dai, int clk_id,
> break;
> case ESAI_HCKT_EXTAL:
> ecr |= ESAI_ECR_ETI;
> - /* fall through */
> + break;
> case ESAI_HCKR_EXTAL:
> ecr |= ESAI_ECR_ERI;
> break;
> --
[PATCH V3] ASoC: fsl_esai: Fix missing break in switch statement
case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be
independent of each other, so replace fall-through with break.
Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
Signed-off-by: Shengjiu Wang
Cc:
---
changes in v3
- add cc stable
- change the subject
sound/soc/fsl/fsl_esai.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 3623aa9a6f2e..15202a637197 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id,
break;
case ESAI_HCKT_EXTAL:
ecr |= ESAI_ECR_ETI;
- /* fall through */
+ break;
case ESAI_HCKR_EXTAL:
ecr |= ESAI_ECR_ERI;
break;
--
1.9.1
RE: [EXT] Re: [PATCH V2] ASoC: fsl_esai: replace fall-through with break
Hi
>
>
> On 4/9/19 9:42 PM, S.j. Wang wrote:
> > case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be independent
> of
> > each other, so replace fall-through with break.
> >
> I think you should change the subject line to:
>
> fix missing break in switch statement
>
> ...because you are fixing a bug, and it's important to put emphasis on that in
> the subject line.
>
> Also, notice that this bug has been out there for more than 5 years now, so
> you should also tag this for stable.
>
> Thanks
> --
> Gustavo
>
Ok, will send v3.
>
> > Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
> >
> > Signed-off-by: Shengjiu Wang
> > ---
> > Changes in v2
> > - fix the fixes tag.
> >
> > sound/soc/fsl/fsl_esai.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> > c7410bbfd2af..bad0dfed6b68 100644
> > --- a/sound/soc/fsl/fsl_esai.c
> > +++ b/sound/soc/fsl/fsl_esai.c
> > @@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai
> *dai, int clk_id,
> > break;
> > case ESAI_HCKT_EXTAL:
> > ecr |= ESAI_ECR_ETI;
> > - /* fall through */
> > + break;
> > case ESAI_HCKR_EXTAL:
> > ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
> > break;
> >
RE: [EXT] Re: [PATCH] ASoC: fsl_asrc: replace the process_option table with function
Hi > > > On Wed, Apr 10, 2019 at 08:26:59AM +0000, S.j. Wang wrote: > > > Is it possible to update the table? It'd be way quicker to use > > > lookup table than real-time calculation all the time. I believe you > > > can simply calculate all the values out for 12KHz and 24KHz since > > > you have the function. If there are certain combinations of these > > > two not being supported, then we could mark it with a special value and > add an if-check to error out. > > > > > > > Yes, but I think the function should be more flexible, if someday we > > need to support Other sample rate, only need to update the list. > > Given the fact that the owner of the function cannot give more comments, I > feel the function wouldn't be very maintainable as none of us understands it > well. On the other hand, you'll need to update the supported I/O rate lists > anyway, so why not just update the table as well? The supported sample > rates from ALSA are limited too. Overall, I think that continue using a lookup > table wins. Alsa support SNDRV_PCM_RATE_KNOT, we can define the rate that we want To support, and use function is more flexible, and we have use the function Internally for long time, it is stable
RE: [EXT] Re: [PATCH] ASoC: fsl_asrc: replace the process_option table with function
> -Original Message- > From: Nicolin Chen > Sent: Wednesday, April 10, 2019 4:01 PM > To: S.j. Wang > Cc: ti...@kernel.org; xiubo@gmail.com; feste...@gmail.com; > broo...@kernel.org; alsa-de...@alsa-project.org; linuxppc- > d...@lists.ozlabs.org > Subject: Re: [EXT] Re: [PATCH] ASoC: fsl_asrc: replace the process_option > table with function > > WARNING: This email was created outside of NXP. DO NOT CLICK links or > attachments unless you recognize the sender and know the content is safe. > > > > On Wed, Apr 10, 2019 at 07:22:31AM +, S.j. Wang wrote: > > > The table was copied directly from the Reference Manual. We also > > > have listed all supported input and output sample rates just right behind > that table. > > > If there're missing rates, we probably should update those two lists also? > > > Otherwise, how could we have a driver limiting both I/O sample rates > > > while we still see something not in the table? > > > > > > > Yes, I plan to send another patch to update the in/out rate list. Do > > I need To merge that to this commit? Actually we want to support 12k > > and 24KHz > > Please send separate patches but in one series. And a question: > > Is it possible to update the table? It'd be way quicker to use lookup table > than real-time calculation all the time. I believe you can simply calculate > all > the values out for 12KHz and 24KHz since you have the function. If there are > certain combinations of these two not being supported, then we could mark > it with a special value and add an if-check to error out. > Yes, but I think the function should be more flexible, if someday we need to support Other sample rate, only need to update the list. > > > > +static int proc_autosel(int Fsin, int Fsout, int *pre_proc, int > > > > +*post_proc) > > > > > > Please add some comments to this function to explain what it does, > > > and how it works. And better to rename it to something like > "fsl_asrc_sel_proc". > > > > > Yes, some comments should be added, but not so detail, because this > > function > > As much comments as possible. > > > Is get from the design team, but the owner has left. > > OK...that's sad... > > > > Another thing confuses me: so we could have supported sample rates > > > in the list but the hardware might not support some of them because > > > we couldn't calculate their processing options? > > > > No, just want to support 12k, 24KHz, or others as customer like. > > I was confused because the I/O rate lists not getting updated. > It makes sense now if you are abort to update them.
RE: [EXT] Re: [PATCH] ASoC: fsl_asrc: replace the process_option table with function
Hi
>
> On Wed, Apr 10, 2019 at 03:15:26AM +0000, S.j. Wang wrote:
> > The table is not flexible if supported sample rate is not in the
> > table, so use a function to replace it.
>
> Could you please elaborate a bit the special use case here?
>
> The table was copied directly from the Reference Manual. We also have
> listed all supported input and output sample rates just right behind that
> table.
> If there're missing rates, we probably should update those two lists also?
> Otherwise, how could we have a driver limiting both I/O sample rates while
> we still see something not in the table?
>
Yes, I plan to send another patch to update the in/out rate list. Do I need
To merge that to this commit? Actually we want to support 12k and 24kHz
> > +static int proc_autosel(int Fsin, int Fsout, int *pre_proc, int
> > +*post_proc)
>
> Please add some comments to this function to explain what it does, and how
> it works. And better to rename it to something like "fsl_asrc_sel_proc".
>
Yes, some comments should be added, but not so detail, because this function
Is get from the design team, but the owner has left.
> > +{
> > + bool det_out_op2_cond;
> > + bool det_out_op0_cond;
> > +
> > + det_out_op2_cond = (((Fsin * 15 > Fsout * 16) & (Fsout < 56000)) |
> > + ((Fsin > 56000) & (Fsout < 56000)));
> > + det_out_op0_cond = (Fsin * 23 < Fsout * 8);
>
> "detect output option condition"? Please explain a bit or add comments to
> explain.
>
> > +
> > + /*
> > + * Not supported case: Tsout>16.125*Tsin, and Tsout>8.125*Tsin.
>
> Could be "unsupported". And it should fit within one line:
> /* Unsupported case: Tsout > 16.125 * Tsin, and Tsout > 8.125 * Tsin
> */
>
> > + */
> > + if (Fsin * 8 > 129 * Fsout)
> > + *pre_proc = 5;
> > + else if (Fsin * 8 > 65 * Fsout)
> > + *pre_proc = 4;
> > + else if (Fsin * 8 > 33 * Fsout)
> > + *pre_proc = 2;
> > + else if (Fsin * 8 > 15 * Fsout) {
> > + if (Fsin > 152000)
> > + *pre_proc = 2;
> > + else
> > + *pre_proc = 1;
> > + } else if (Fsin < 76000)
> > + *pre_proc = 0;
> > + else if (Fsin > 152000)
> > + *pre_proc = 2;
> > + else
> > + *pre_proc = 1;
> > +
> > + if (det_out_op2_cond)
> > + *post_proc = 2;
> > + else if (det_out_op0_cond)
> > + *post_proc = 0;
> > + else
> > + *post_proc = 1;
> > +
> > + if (*pre_proc == 4 || *pre_proc == 5)
> > + return -EINVAL;
>
> I think you'd better add some necessary comments here too.
>
> > @@ -377,11 +404,17 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
> *pair)
> > ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index),
> > ASRCTR_IDR(index) | ASRCTR_USR(index));
> >
> > + ret = proc_autosel(inrate, outrate, _proc, _proc);
> > + if (ret) {
> > + pair_err("No supported pre-processing options\n");
> > + return ret;
> > + }
>
> I think we should do this earlier in this function, once We know the inrate
> and outrate, instead of having all register being configured then going for an
> error-out.
Ok.
>
> Another thing confuses me: so we could have supported sample rates in the
> list but the hardware might not support some of them because we couldn't
> calculate their processing options?
No, just want to support 12k, 24KHz, or others as customer like.
[PATCH] ASoC: fsl_asrc: replace the process_option table with function
The table is not flexible if supported sample rate is not in the
table, so use a function to replace it.
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_asrc.c | 73 +++-
1 file changed, 53 insertions(+), 20 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 0b937924d2e4..a57c6c829060 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -26,24 +26,6 @@
#define pair_dbg(fmt, ...) \
dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
##__VA_ARGS__)
-/* Sample rates are aligned with that defined in pcm.h file */
-static const u8 process_option[][12][2] = {
- /* 8kHz 11.025kHz 16kHz 22.05kHz 32kHz 44.1kHz 48kHz 64kHz 88.2kHz
96kHz 176kHz 192kHz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 5512Hz */
- {{0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 8kHz */
- {{0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 11025Hz */
- {{1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 16kHz */
- {{1, 2}, {1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0,
0}, {0, 0}, {0, 0}, {0, 0},}, /* 22050Hz */
- {{1, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 0}, {0, 0}, {0, 0},}, /* 32kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 44.1kHz */
- {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 2}, {0, 1}, {0, 1}, {0,
1}, {0, 1}, {0, 0}, {0, 0},}, /* 48kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 1}, {1, 2}, {0, 2}, {0, 2}, {0, 1}, {0,
1}, {0, 1}, {0, 1}, {0, 0},}, /* 64kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 88.2kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1,
1}, {1, 1}, {1, 1}, {1, 1},}, /* 96kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 176kHz */
- {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2,
1}, {2, 1}, {2, 1}, {2, 1},}, /* 192kHz */
-};
-
/* Corresponding to process_option */
static int supported_input_rate[] = {
5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
@@ -79,6 +61,49 @@
static unsigned char *clk_map[2];
+static int proc_autosel(int Fsin, int Fsout, int *pre_proc, int *post_proc)
+{
+ bool det_out_op2_cond;
+ bool det_out_op0_cond;
+
+ det_out_op2_cond = (((Fsin * 15 > Fsout * 16) & (Fsout < 56000)) |
+ ((Fsin > 56000) & (Fsout < 56000)));
+ det_out_op0_cond = (Fsin * 23 < Fsout * 8);
+
+ /*
+* Not supported case: Tsout>16.125*Tsin, and Tsout>8.125*Tsin.
+*/
+ if (Fsin * 8 > 129 * Fsout)
+ *pre_proc = 5;
+ else if (Fsin * 8 > 65 * Fsout)
+ *pre_proc = 4;
+ else if (Fsin * 8 > 33 * Fsout)
+ *pre_proc = 2;
+ else if (Fsin * 8 > 15 * Fsout) {
+ if (Fsin > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+ } else if (Fsin < 76000)
+ *pre_proc = 0;
+ else if (Fsin > 152000)
+ *pre_proc = 2;
+ else
+ *pre_proc = 1;
+
+ if (det_out_op2_cond)
+ *post_proc = 2;
+ else if (det_out_op0_cond)
+ *post_proc = 0;
+ else
+ *post_proc = 1;
+
+ if (*pre_proc == 4 || *pre_proc == 5)
+ return -EINVAL;
+
+ return 0;
+}
+
/**
* Request ASRC pair
*
@@ -239,8 +264,10 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
u32 inrate, outrate, indiv, outdiv;
u32 clk_index[2], div[2];
int in, out, channels;
+ int pre_proc, post_proc;
struct clk *clk;
bool ideal;
+ int ret;
if (!config) {
pair_err("invalid pair config\n");
@@ -377,11 +404,17 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
*pair)
ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index),
ASRCTR_IDR(index) | ASRCTR_USR(index));
+ ret = proc_autosel(inrate, outrate, _proc, _proc);
+ if (ret) {
+ pair_err("No supported pre-processing options\n");
+ return ret;
+ }
+
/* Apply configurations for pre- and post-processing */
regmap_update_bits(asrc_priv->regmap, REG_ASRCFG,
ASRCFG_PREMODi_MASK(index) |
ASRCFG_POSTMODi_MASK(index),
- ASRCFG_PREMOD(index,
[PATCH V2] ASoC: fsl_esai: replace fall-through with break
case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be independent of
each other, so replace fall-through with break.
Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
Signed-off-by: Shengjiu Wang
---
Changes in v2
- fix the fixes tag.
sound/soc/fsl/fsl_esai.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index c7410bbfd2af..bad0dfed6b68 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id,
break;
case ESAI_HCKT_EXTAL:
ecr |= ESAI_ECR_ETI;
- /* fall through */
+ break;
case ESAI_HCKR_EXTAL:
ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
break;
--
1.9.1
RE: [EXT] Re: [PATCH V1] ASoC: fsl_esai: replace fall-through with break
Hi > > Hi Gustavo, > > On Mon, Apr 08, 2019 at 10:20:25PM -0500, Gustavo A. R. Silva wrote: > > >>> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c > > >>> index > > >>> c7410bbfd2af..bad0dfed6b68 100644 > > >>> --- a/sound/soc/fsl/fsl_esai.c > > >>> +++ b/sound/soc/fsl/fsl_esai.c > > >>> @@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct > > >> snd_soc_dai *dai, int clk_id, > > >>> break; > > >>> case ESAI_HCKT_EXTAL: > > >>> ecr |= ESAI_ECR_ETI; > > >> > > >> Also, you should use a simple assignment operator "=" instead of > > >> "|=" in both cases. > > > > > > The result is same for "=" and "|=", because there is "ecr = 0" in > > > beginning of This function. > > > > > > > Following that same logic, then why not use "+=" instead? > > > > The point is: is "|=" or any other assignment operator other than "=" > necessary? > > The answer in this case is: no, it is not. So, go for the simple one > > and avoid any unnecessary confusion. > > I would like to keep "|=" here, just in case that someday it'd be easier to > insert something to ecr before this chunk. So please get easy on this one. > > Thanks > Nicolin Thanks for reviewing, I will send v2. Best regards Wang shengjiu
RE: [EXT] Re: [PATCH V1] ASoC: fsl_esai: replace fall-through with break
Hi Gustavo
>
>
> On 4/8/19 4:28 AM, S.j. Wang wrote:
> > case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be
> independent of
> > each other, so replace fall-through with break.
> >
> If this is correct, then you should use the following "Fixes" tag instead,
> which is the one that introduced the bug:
>
> Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
>
> > Fixes: 16bbeb2b43c3 ("ASoC: fsl_esai: Mark expected switch
> > fall-through")
> >
>
> because this didn't change any functionality.
Ok, this will be updated.
>
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/fsl_esai.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> > c7410bbfd2af..bad0dfed6b68 100644
> > --- a/sound/soc/fsl/fsl_esai.c
> > +++ b/sound/soc/fsl/fsl_esai.c
> > @@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct
> snd_soc_dai *dai, int clk_id,
> > break;
> > case ESAI_HCKT_EXTAL:
> > ecr |= ESAI_ECR_ETI;
>
> Also, you should use a simple assignment operator "=" instead of "|=" in
> both cases.
The result is same for "=" and "|=", because there is "ecr = 0" in beginning of
This function.
>
> > - /* fall through */
> > + break;
> > case ESAI_HCKR_EXTAL:
> > ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
> > break;
> >
>
> Thanks
> --
> Gustavo
[PATCH V1] ASoC: fsl_esai: replace fall-through with break
case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be independent of
each other, so replace fall-through with break.
Fixes: 16bbeb2b43c3 ("ASoC: fsl_esai: Mark expected switch fall-through")
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_esai.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index c7410bbfd2af..bad0dfed6b68 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id,
break;
case ESAI_HCKT_EXTAL:
ecr |= ESAI_ECR_ETI;
- /* fall through */
+ break;
case ESAI_HCKR_EXTAL:
ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
break;
--
1.9.1
[PATCH V3] ASoC: fsl_esai: Support synchronous mode
In ESAI synchronous mode, the clock is generated by Tx, So
we should always set registers of Tx which relate with the
bit clock and frame clock generation (TCCR, TCR, ECR), even
there is only Rx is working.
Signed-off-by: Shengjiu Wang
Acked-by: Nicolin Chen
---
Changes in v3
- fix the indentation
sound/soc/fsl/fsl_esai.c | 21 ++---
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 3623aa9a6f2e..c7410bbfd2af 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -218,7 +218,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id,
{
struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
struct clk *clksrc = esai_priv->extalclk;
- bool tx = clk_id <= ESAI_HCKT_EXTAL;
+ bool tx = (clk_id <= ESAI_HCKT_EXTAL || esai_priv->synchronous);
bool in = dir == SND_SOC_CLOCK_IN;
u32 ratio, ecr = 0;
unsigned long clk_rate;
@@ -253,7 +253,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id,
ecr |= ESAI_ECR_ETI;
/* fall through */
case ESAI_HCKR_EXTAL:
- ecr |= ESAI_ECR_ERI;
+ ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
break;
default:
return -EINVAL;
@@ -537,10 +537,18 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
*substream,
bclk = params_rate(params) * slot_width * esai_priv->slots;
- ret = fsl_esai_set_bclk(dai, tx, bclk);
+ ret = fsl_esai_set_bclk(dai, esai_priv->synchronous || tx, bclk);
if (ret)
return ret;
+ mask = ESAI_xCR_xSWS_MASK;
+ val = ESAI_xCR_xSWS(slot_width, width);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
+ /* Recording in synchronous mode needs to set TCR also */
+ if (!tx && esai_priv->synchronous)
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, mask, val);
+
/* Use Normal mode to support monaural audio */
regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
@@ -556,10 +564,9 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
*substream,
regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx), mask, val);
- mask = ESAI_xCR_xSWS_MASK | (tx ? ESAI_xCR_PADC : 0);
- val = ESAI_xCR_xSWS(slot_width, width) | (tx ? ESAI_xCR_PADC : 0);
-
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
+ if (tx)
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_PADC, ESAI_xCR_PADC);
/* Remove ESAI personal reset by configuring ESAI_PCRC and ESAI_PRRC */
regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
--
1.9.1
RE: [PATCH V2] ASoC: fsl_esai: Support synchronous mode
Hi
>
> This looks better :)
>
> On Wed, Apr 03, 2019 at 10:07:40AM +0000, S.j. Wang wrote:
> > @@ -218,7 +218,7 @@ static int fsl_esai_set_dai_sysclk(struct
> > snd_soc_dai *dai, int clk_id, {
> > struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
> > struct clk *clksrc = esai_priv->extalclk;
> > - bool tx = clk_id <= ESAI_HCKT_EXTAL;
> > + bool tx = (clk_id <= ESAI_HCKT_EXTAL || esai_priv->synchronous);
> > bool in = dir == SND_SOC_CLOCK_IN;
> > u32 ratio, ecr = 0;
> > unsigned long clk_rate;
> > @@ -253,7 +253,7 @@ static int fsl_esai_set_dai_sysclk(struct
> snd_soc_dai *dai, int clk_id,
> > ecr |= ESAI_ECR_ETI;
> > /* fall through */
>
> Btw, I am also wondering if the fall through here is a bug
> Because I don't recall that there is a specific reason to fall through here.
> Can you please help confirm? Perhaps we need to submit a separate fix as
> well by replacing it with a "break;".
Yes, I think there is issue here, will submit another patch for it.
>
> > case ESAI_HCKR_EXTAL:
> > - ecr |= ESAI_ECR_ERI;
> > + ecr |= esai_priv->synchronous ? ESAI_ECR_ETI :
> ESAI_ECR_ERI;
> > break;
> > default:
> > return -EINVAL;
>
> > @@ -537,10 +537,18 @@ static int fsl_esai_hw_params(struct
> > snd_pcm_substream *substream,
> >
> > bclk = params_rate(params) * slot_width * esai_priv->slots;
> >
> > - ret = fsl_esai_set_bclk(dai, tx, bclk);
> > + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous || tx, bclk);
> > if (ret)
> > return ret;
> >
> > + mask = ESAI_xCR_xSWS_MASK;
> > + val = ESAI_xCR_xSWS(slot_width, width);
> > +
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask,
> val);
> > + /* Recording in synchronous mode needs to set TCR also */
> > + if (!tx && esai_priv->synchronous)
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> mask, val);
> > +
> > /* Use Normal mode to support monaural audio */
> > regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> >ESAI_xCR_xMOD_MASK,
> params_channels(params) > 1 ?
> > @@ -556,10 +564,9 @@ static int fsl_esai_hw_params(struct
> > snd_pcm_substream *substream,
> >
> > regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx), mask,
> val);
> >
> > - mask = ESAI_xCR_xSWS_MASK | (tx ? ESAI_xCR_PADC : 0);
> > - val = ESAI_xCR_xSWS(slot_width, width) | (tx ? ESAI_xCR_PADC : 0);
> > -
> > - regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask,
> val);
> > + if (tx)
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > + ESAI_xCR_PADC, ESAI_xCR_PADC);
>
> Mind aligning the indentation here like the one below?
> regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> ESAI_xCR_PADC, ESAI_xCR_PADC);
Ok. Will send v3.
>
> Once you fix the indentation, add this:
>
> Acked-by: Nicolin Chen
>
> Thanks
[PATCH V2] ASoC: fsl_esai: Support synchronous mode
In ESAI synchronous mode, the clock is generated by Tx, So
we should always set registers of Tx which relate with the
bit clock and frame clock generation (TCCR, TCR, ECR), even
there is only Rx is working.
Signed-off-by: Shengjiu Wang
---
changes in v2
- refine the patch according Nicolin's comments
- merge plain setting.
sound/soc/fsl/fsl_esai.c | 21 ++---
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 3623aa9a6f2e..f2e7b27f8447 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -218,7 +218,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id,
{
struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
struct clk *clksrc = esai_priv->extalclk;
- bool tx = clk_id <= ESAI_HCKT_EXTAL;
+ bool tx = (clk_id <= ESAI_HCKT_EXTAL || esai_priv->synchronous);
bool in = dir == SND_SOC_CLOCK_IN;
u32 ratio, ecr = 0;
unsigned long clk_rate;
@@ -253,7 +253,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id,
ecr |= ESAI_ECR_ETI;
/* fall through */
case ESAI_HCKR_EXTAL:
- ecr |= ESAI_ECR_ERI;
+ ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
break;
default:
return -EINVAL;
@@ -537,10 +537,18 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
*substream,
bclk = params_rate(params) * slot_width * esai_priv->slots;
- ret = fsl_esai_set_bclk(dai, tx, bclk);
+ ret = fsl_esai_set_bclk(dai, esai_priv->synchronous || tx, bclk);
if (ret)
return ret;
+ mask = ESAI_xCR_xSWS_MASK;
+ val = ESAI_xCR_xSWS(slot_width, width);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
+ /* Recording in synchronous mode needs to set TCR also */
+ if (!tx && esai_priv->synchronous)
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, mask, val);
+
/* Use Normal mode to support monaural audio */
regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
@@ -556,10 +564,9 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
*substream,
regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx), mask, val);
- mask = ESAI_xCR_xSWS_MASK | (tx ? ESAI_xCR_PADC : 0);
- val = ESAI_xCR_xSWS(slot_width, width) | (tx ? ESAI_xCR_PADC : 0);
-
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
+ if (tx)
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_PADC, ESAI_xCR_PADC);
/* Remove ESAI personal reset by configuring ESAI_PCRC and ESAI_PRRC */
regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
--
1.9.1
RE: [PATCH] ASoC: fsl_esai: Support synchronous mode
Hi
>
> > > On Mon, Apr 01, 2019 at 11:39:10AM +0000, S.j. Wang wrote:
> > > > In ESAI synchronous mode, the clock is generated by Tx, So we
> > > > should always set registers of Tx which relate with the bit clock
> > > > and frame clock generation (TCCR, TCR, ECR), even there is only Rx is
> working.
> > > >
> > > > Signed-off-by: Shengjiu Wang
> > > > ---
> > > > sound/soc/fsl/fsl_esai.c | 28 +++-
> > > > 1 file changed, 27 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> > > > index
> > > > 3623aa9a6f2e..d9fcddd55c02 100644
> > > > --- a/sound/soc/fsl/fsl_esai.c
> > > > +++ b/sound/soc/fsl/fsl_esai.c
> > > > @@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct
> > > snd_soc_dai *dai, int clk_id,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > + if (esai_priv->synchronous && !tx) {
> > > > + switch (clk_id) {
> > > > + case ESAI_HCKR_FSYS:
> > > > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
> > > > + freq,
> > > > dir);
> > > > + break;
> > > > + case ESAI_HCKR_EXTAL:
> > > > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
> > > > + freq,
> > > > dir);
> > >
> > > Not sure why you call set_dai_sysclk inside set_dai_sysclk again. It
> > > feels very confusing to do so, especially without a comments.
> >
> > For sync mode, only RX is enabled, the register of tx should be set,
> > so call the Set_dai_sysclk again.
>
> Yea, I understood that. But why not just replace RX with TX on the register-
> writing level? Do we need to set both TCCR and RCCR? Your change in
> hw_params() only sets TCCR inside fsl_esai_set_bclk(), so we probably only
> need to change TCCR for recordings running in sync mode, right?
>
> From the commit message, it feels like that only the clock-related fields in
> the TX registers need to be set. Things like calculation and setting the
> direction of HCKx pin don't need to run again.
>
> > > > @@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct
> > > > snd_pcm_substream *substream,
> > > >
> > > > bclk = params_rate(params) * slot_width * esai_priv->slots;
> > > >
> > > > - ret = fsl_esai_set_bclk(dai, tx, bclk);
> > > > + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx,
> > > > +bclk);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > + if (esai_priv->synchronous && !tx) {
> > > > + /* Use Normal mode to support monaural audio */
> > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > > + ESAI_xCR_xMOD_MASK,
> > > params_channels(params) > 1 ?
> > > > + ESAI_xCR_xMOD_NETWORK : 0);
> > > > +
> > > > + mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
> > > > + val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
> > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > mask, val);
> > > > + }
> > >
> > > Does synchronous mode require to set both TCR and RCR? or just TCR?
>
> > Both TCR and RCR. RCR will be set in normal flow.
>
> OK. Settings both xCRs makes sense. Would you please try this:
>
> ===
> @@ -537,14 +552,20 @@ static int fsl_esai_hw_params(struct
> snd_pcm_substream *substream,
>
> bclk = params_rate(params) * slot_width * esai_priv->slots;
>
> - ret = fsl_esai_set_bclk(dai, tx, bclk);
> + /* Synchronous mode uses TX clock generator */
> + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous || tx,
> + bclk);
> if (ret)
> return ret;
>
> + mask = ESAI_xCR_xMOD_MASK | ESAI_xCR_xSWS_MASK;
> + val = ESAI_xCR_xSWS(slot_width, width);
> /* Use Normal mode to support monaural audio */
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> -
RE: [PATCH] ASoC: fsl_esai: Support synchronous mode
Hi
>
> Shengjiu,
>
> On Mon, Apr 01, 2019 at 11:39:10AM +0000, S.j. Wang wrote:
> > In ESAI synchronous mode, the clock is generated by Tx, So we should
> > always set registers of Tx which relate with the bit clock and frame
> > clock generation (TCCR, TCR, ECR), even there is only Rx is working.
> >
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/fsl_esai.c | 28 +++-
> > 1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> > 3623aa9a6f2e..d9fcddd55c02 100644
> > --- a/sound/soc/fsl/fsl_esai.c
> > +++ b/sound/soc/fsl/fsl_esai.c
> > @@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct
> snd_soc_dai *dai, int clk_id,
> > return -EINVAL;
> > }
> >
> > + if (esai_priv->synchronous && !tx) {
> > + switch (clk_id) {
> > + case ESAI_HCKR_FSYS:
> > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
> > + freq, dir);
> > + break;
> > + case ESAI_HCKR_EXTAL:
> > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
> > + freq, dir);
>
> Not sure why you call set_dai_sysclk inside set_dai_sysclk again. It feels
> very
> confusing to do so, especially without a comments.
For sync mode, only RX is enabled, the register of tx should be set, so call
the
Set_dai_sysclk again.
>
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + }
> > +
> > /* Bypass divider settings if the requirement doesn't change */
> > if (freq == esai_priv->hck_rate[tx] && dir == esai_priv->hck_dir[tx])
> > return 0;
> > @@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct
> > snd_pcm_substream *substream,
> >
> > bclk = params_rate(params) * slot_width * esai_priv->slots;
> >
> > - ret = fsl_esai_set_bclk(dai, tx, bclk);
> > + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx,
> > +bclk);
> > if (ret)
> > return ret;
> >
> > + if (esai_priv->synchronous && !tx) {
> > + /* Use Normal mode to support monaural audio */
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > + ESAI_xCR_xMOD_MASK,
> params_channels(params) > 1 ?
> > + ESAI_xCR_xMOD_NETWORK : 0);
> > +
> > + mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
> > + val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> mask, val);
> > + }
>
> Does synchronous mode require to set both TCR and RCR? or just TCR?
> The code behind this part is doing the same setting to RCR. If that is not
> needed any more for a synchronous recording, we should reuse it instead
> of inserting a piece of redundant code. Otherwise, if we need to set both,
> we should have two regmap_update_bits operations back-to-back for TCR
> and RCR (and other registers too).
Both TCR and RCR. RCR will be set in normal flow.
>
> > +
> > /* Use Normal mode to support monaural audio */
> > regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> >ESAI_xCR_xMOD_MASK,
> params_channels(params) > 1 ?
>
> In case that we only need to set TCR (more likely I feel), it would feel less
> confusing to me, if we changed REG_ESAI_xCR(tx) here, for example, to
> REG_ESAI_xCR(tx || sync). Yea, please add to the top a 'bool sync =
> esai_priv->synchronous;'.
>
> Similarly, for ECR_ETO and ECR_ERO:
> (tx || sync) ? ESAI_ECR_ETO : ESAI_ECR_ERO;
Both TCR and RCR should be set.
[PATCH] ASoC: fsl_esai: Support synchronous mode
In ESAI synchronous mode, the clock is generated by Tx, So
we should always set registers of Tx which relate with the
bit clock and frame clock generation (TCCR, TCR, ECR), even
there is only Rx is working.
Signed-off-by: Shengjiu Wang
---
sound/soc/fsl/fsl_esai.c | 28 +++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 3623aa9a6f2e..d9fcddd55c02 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai
*dai, int clk_id,
return -EINVAL;
}
+ if (esai_priv->synchronous && !tx) {
+ switch (clk_id) {
+ case ESAI_HCKR_FSYS:
+ fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
+ freq, dir);
+ break;
+ case ESAI_HCKR_EXTAL:
+ fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
+ freq, dir);
+ break;
+ default:
+ return -EINVAL;
+ }
+ }
+
/* Bypass divider settings if the requirement doesn't change */
if (freq == esai_priv->hck_rate[tx] && dir == esai_priv->hck_dir[tx])
return 0;
@@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
*substream,
bclk = params_rate(params) * slot_width * esai_priv->slots;
- ret = fsl_esai_set_bclk(dai, tx, bclk);
+ ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx, bclk);
if (ret)
return ret;
+ if (esai_priv->synchronous && !tx) {
+ /* Use Normal mode to support monaural audio */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
+ ESAI_xCR_xMOD_NETWORK : 0);
+
+ mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
+ val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, mask, val);
+ }
+
/* Use Normal mode to support monaural audio */
regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
--
1.9.1
RE: [alsa-devel] [PATCH V4] ASoC: fsl_esai: fix channel swap issue when stream starts
Hi Mark
Can this patch be accepted? Or need I do any update?
Best regards
Wang shengjiu
>
> There is very low possibility ( < 0.1% ) that channel swap happened in
> beginning when multi output/input pin is enabled. The issue is that
> hardware can't send data to correct pin in the beginning with the normal
> enable flow.
>
> This is hardware issue, but there is no errata, the workaround flow is that:
> Each time playback/recording, firstly clear the xSMA/xSMB, then enable
> TE/RE, then enable xSMB and xSMA (xSMB must be enabled before xSMA).
> Which is to use the xSMA as the trigger start register, previously the xCR_TE
> or xCR_RE is the bit for starting.
>
> Fixes commit 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")
> Cc:
> Reviewed-by: Fabio Estevam
> Acked-by: Nicolin Chen
> Signed-off-by: Shengjiu Wang
> ---
> sound/soc/fsl/fsl_esai.c | 47 +---
> ---
> 1 file changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> afe67c865330..3623aa9a6f2e 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -54,6 +54,8 @@ struct fsl_esai {
> u32 fifo_depth;
> u32 slot_width;
> u32 slots;
> + u32 tx_mask;
> + u32 rx_mask;
> u32 hck_rate[2];
> u32 sck_rate[2];
> bool hck_dir[2];
> @@ -361,21 +363,13 @@ static int fsl_esai_set_dai_tdm_slot(struct
> snd_soc_dai *dai, u32 tx_mask,
> regmap_update_bits(esai_priv->regmap, REG_ESAI_TCCR,
> ESAI_xCCR_xDC_MASK, ESAI_xCCR_xDC(slots));
>
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMA,
> -ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(tx_mask));
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_TSMB,
> -ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(tx_mask));
> -
> regmap_update_bits(esai_priv->regmap, REG_ESAI_RCCR,
> ESAI_xCCR_xDC_MASK, ESAI_xCCR_xDC(slots));
>
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMA,
> -ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(rx_mask));
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_RSMB,
> -ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(rx_mask));
> -
> esai_priv->slot_width = slot_width;
> esai_priv->slots = slots;
> + esai_priv->tx_mask = tx_mask;
> + esai_priv->rx_mask = rx_mask;
>
> return 0;
> }
> @@ -596,6 +590,7 @@ static int fsl_esai_trigger(struct snd_pcm_substream
> *substream, int cmd,
> bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> u8 i, channels = substream->runtime->channels;
> u32 pins = DIV_ROUND_UP(channels, esai_priv->slots);
> + u32 mask;
>
> switch (cmd) {
> case SNDRV_PCM_TRIGGER_START:
> @@ -608,15 +603,38 @@ static int fsl_esai_trigger(struct
> snd_pcm_substream *substream, int cmd,
> for (i = 0; tx && i < channels; i++)
> regmap_write(esai_priv->regmap, REG_ESAI_ETDR,
> 0x0);
>
> + /*
> + * When set the TE/RE in the end of enablement flow, there
> + * will be channel swap issue for multi data line case.
> + * In order to workaround this issue, we switch the bit
> + * enablement sequence to below sequence
> + * 1) clear the xSMB & xSMA: which is done in probe and
> + * stop state.
> + * 2) set TE/RE
> + * 3) set xSMB
> + * 4) set xSMA: xSMA is the last one in this flow, which
> + * will trigger esai to start.
> + */
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> tx ? ESAI_xCR_TE_MASK :
> ESAI_xCR_RE_MASK,
> tx ? ESAI_xCR_TE(pins) :
> ESAI_xCR_RE(pins));
> + mask = tx ? esai_priv->tx_mask : esai_priv->rx_mask;
> +
> + regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMB(tx),
> +ESAI_xSMB_xS_MASK,
> ESAI_xSMB_xS(mask));
> + regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMA(tx),
> +ESAI_xSMA_xS_MASK,
> ESAI_xSMA_xS(mask));
> +
> break;
> case SNDRV_PCM_TRIGGER_SUSPEND:
> case SNDRV_PCM_TRIGGER_STOP:
> case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> tx ? ESAI_xCR_TE_MASK :
> ESAI_xCR_RE_MASK, 0);
> + regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMA(tx),
> +ESAI_xSMA_xS_MASK, 0);
> + regmap_update_bits(esai_priv->regmap,
> REG_ESAI_xSMB(tx),
> +ESAI_xSMB_xS_MASK, 0);
>
> /* Disable and reset FIFO */
> regmap_update_bits(esai_priv->regmap,
[PATCH V3] ASoC: fsl_asrc: add constraint for the asrc of older version
There is a constraint for the channel number setting on the
asrc of older version (e.g. imx35), the channel number should
be even, odd number isn't valid.
So add this constraint when the asrc of older version is used.
Acked-by: Nicolin Chen
Signed-off-by: Shengjiu Wang
---
Changes in v3
- fix comments with Nicolin's review
sound/soc/fsl/fsl_asrc.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 528e8b108422..0b937924d2e4 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -445,6 +445,19 @@ struct dma_chan *fsl_asrc_get_dma_channel(struct
fsl_asrc_pair *pair, bool dir)
}
EXPORT_SYMBOL_GPL(fsl_asrc_get_dma_channel);
+static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
+
+ /* Odd channel number is not valid for older ASRC (channel_bits==3) */
+ if (asrc_priv->channel_bits == 3)
+ snd_pcm_hw_constraint_step(substream->runtime, 0,
+ SNDRV_PCM_HW_PARAM_CHANNELS, 2);
+
+ return 0;
+}
+
static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
@@ -539,6 +552,7 @@ static int fsl_asrc_dai_trigger(struct snd_pcm_substream
*substream, int cmd,
}
static const struct snd_soc_dai_ops fsl_asrc_dai_ops = {
+ .startup = fsl_asrc_dai_startup,
.hw_params= fsl_asrc_dai_hw_params,
.hw_free = fsl_asrc_dai_hw_free,
.trigger = fsl_asrc_dai_trigger,
--
1.9.1
RE: [PATCH V2] ASoC: fsl_asrc: add constraint for the asrc of older version
Hi
> On Fri, Mar 01, 2019 at 08:37:08AM +0000, S.j. Wang wrote:
> > There is constraint for the channel number setting on the
>
> nit: "a constraint"
>
> > asrc of older version (e.g. imx35), the channel number should be even,
> > odd number isn't valid.
>
> > +static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
> > + struct snd_soc_dai *dai)
> > +{
> > + struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
> > +
> > + /* channel_bits = 3 means older version on imx35*/
>
> Space between '5' and '*'. And better to make it clear:
>
> /* Odd channel number is not valid for older ASRC (channel_bits==3)
> */
>
> > + if (asrc_priv->channel_bits == 3)
> > + snd_pcm_hw_constraint_step(substream->runtime, 0,
> > +
> SNDRV_PCM_HW_PARAM_CHANNELS, 2);
>
> For your next version,
>
> Acked-by: Nicolin Chen
Ok, thanks, will send v3.
>
> Cheers
RE: [alsa-devel] [PATCH] ASoC: cs42xx8: Remove S32_LE in format list
Hi > On 3/1/19 12:32 AM, S.j. Wang wrote: > > This case is covered by S24_LE I think. The S32_LE means the data is > > 32bit and slot width Is 32bit, this is not in data sheet. > > The problem is that if you have 32-bit samples in your audio file, and you > want to play them, then software (e.g. alsalib) will need to convert the > audio to 24-bit before sending it to hardware. This is unnecessary because > the hardware can "convert" the sample to 24-bit automatically by ignoring > the lower 8 bits. > > I think a lot of codecs do this already. Ok. Thanks for reviewing, I will drop this patch. Best regards Wang shengjiu
[PATCH V2] ASoC: fsl_asrc: add constraint for the asrc of older version
There is constraint for the channel number setting on the
asrc of older version (e.g. imx35), the channel number should
be even, odd number isn't valid.
So add this constraint when the asrc of older version is used.
Signed-off-by: Shengjiu Wang
---
changes in v2
- switch to add constraint in startup function
sound/soc/fsl/fsl_asrc.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 528e8b108422..31494f225037 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -445,6 +445,19 @@ struct dma_chan *fsl_asrc_get_dma_channel(struct
fsl_asrc_pair *pair, bool dir)
}
EXPORT_SYMBOL_GPL(fsl_asrc_get_dma_channel);
+static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
+
+ /* channel_bits = 3 means older version on imx35*/
+ if (asrc_priv->channel_bits == 3)
+ snd_pcm_hw_constraint_step(substream->runtime, 0,
+ SNDRV_PCM_HW_PARAM_CHANNELS, 2);
+
+ return 0;
+}
+
static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
@@ -539,6 +552,7 @@ static int fsl_asrc_dai_trigger(struct snd_pcm_substream
*substream, int cmd,
}
static const struct snd_soc_dai_ops fsl_asrc_dai_ops = {
+ .startup = fsl_asrc_dai_startup,
.hw_params= fsl_asrc_dai_hw_params,
.hw_free = fsl_asrc_dai_hw_free,
.trigger = fsl_asrc_dai_trigger,
--
1.9.1
RE: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
Hi
>
> On Fri, Mar 01, 2019 at 06:55:25AM +0000, S.j. Wang wrote:
>
> > > Alternatively, I feel instead of error-out at here, should we add a
> > > HW constraint or at least fence it off at the beginning of the
> > > hw_params()? This is actually nothing specific to the pair-request
> > > function but a hardware constraint.
> >
> > How about add constraint in startup?
> > static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
> >struct snd_soc_dai *dai) {
> >struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
> >
> >if (asrc_priv->channel_bits == 3) {
> >snd_pcm_hw_constraint_step(substream->runtime, 0,
> > SNDRV_PCM_HW_PARAM_CHANNELS, 2);
> >}
> >
> >return 0;
> > }
>
> Yea, that looks good to me. Better to have a line of comments to tell that
> "bits==3" means older version -- maybe we should have something much
> more clear than using channel_bits but it is fine for now since they only
> differ here.
Ok , will send v2.
