Re: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
On Fri, Mar 01, 2019 at 06:55:25AM +, 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. signature.asc Description: PGP signature
RE: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
Hi > > On Fri, Mar 01, 2019 at 06:55:25AM +, 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.
Re: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
On Fri, Mar 01, 2019 at 06:55:25AM +, 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.
RE: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
Hi > > Hi Shengjiu, > > On Fri, Mar 01, 2019 at 02:32:38AM +, 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 > > --- > > 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
Re: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version
Hi Shengjiu, On Fri, Mar 01, 2019 at 02:32:38AM +, 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 > --- > 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.