| Message ID | 1551407560-29950-1-git-send-email-shengjiu.wang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: fsl_asrc: add protection for the asrc of older version | expand |
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.
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
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.
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.
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.
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); ret = -EINVAL; } else {
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(-)