* RE: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
@ 2019-03-01 6:55 ` S.j. Wang
0 siblings, 0 replies; 9+ messages in thread
From: S.j. Wang @ 2019-03-01 6:55 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org
Hi
>
> Hi Shengjiu,
>
> On Fri, Mar 01, 2019 at 02:32:38AM +0000, S.j. Wang wrote:
> > 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 protection when the asrc of older version is used.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > sound/soc/fsl/fsl_asrc.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index
> > 528e8b108422..b3b3c5e15ef1 100644
> > --- a/sound/soc/fsl/fsl_asrc.c
> > +++ b/sound/soc/fsl/fsl_asrc.c
> > @@ -109,7 +109,8 @@ static int fsl_asrc_request_pair(int channels, struct
> fsl_asrc_pair *pair)
> > if (index == ASRC_INVALID_PAIR) {
> > dev_err(dev, "all pairs are busy now\n");
> > ret = -EBUSY;
> > - } else if (asrc_priv->channel_avail < channels) {
> > + } else if (asrc_priv->channel_avail < channels ||
> > + (asrc_priv->channel_bits < 4 && channels % 2 != 0)) {
> > dev_err(dev, "can't afford required channels: %d\n",
> channels);
>
> I feel it'd be better to have another else-if, since the existing error message
> is against something else.
>
> + } else if (asrc_priv->channel_bits < 4 && channels & 1) {
> + /* old version of ASRC has channel_bits = 3 */
> + dev_err(dev, "does not support odd channel number\n");
> + ret = -EINVAL;
>
> 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;
}
Best regards
Wang shengjiu
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
2019-03-01 6:55 ` S.j. Wang
(?)
@ 2019-03-01 8:08 ` Nicolin Chen
2019-03-01 8:32 ` S.j. Wang
-1 siblings, 1 reply; 9+ messages in thread
From: Nicolin Chen @ 2019-03-01 8:08 UTC (permalink / raw)
To: S.j. Wang
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
2019-03-01 8:08 ` Nicolin Chen
@ 2019-03-01 8:32 ` S.j. Wang
0 siblings, 0 replies; 9+ messages in thread
From: S.j. Wang @ 2019-03-01 8:32 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
@ 2019-03-01 8:32 ` S.j. Wang
0 siblings, 0 replies; 9+ messages in thread
From: S.j. Wang @ 2019-03-01 8:32 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
2019-03-01 6:55 ` S.j. Wang
@ 2019-03-01 12:38 ` Mark Brown
-1 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2019-03-01 12:38 UTC (permalink / raw)
To: S.j. Wang
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com, Nicolin Chen,
linuxppc-dev@lists.ozlabs.org
[-- Attachment #1.1: Type: text/plain, Size: 1017 bytes --]
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;
> }
Yes, that's definitely good - the general idea is that we should never
need to return an error from hw_params() as the constraints code will
have filtered out any invalid configurations before they get that far.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
@ 2019-03-01 12:38 ` Mark Brown
0 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2019-03-01 12:38 UTC (permalink / raw)
To: S.j. Wang
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com, Nicolin Chen,
linuxppc-dev@lists.ozlabs.org
[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]
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;
> }
Yes, that's definitely good - the general idea is that we should never
need to return an error from hw_params() as the constraints code will
have filtered out any invalid configurations before they get that far.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread