| Message ID | 20190422061334.4243-1-akshu.agrawal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: DA7219: Fix failure in hw_params by not letting set_rate error out | expand |
Hi Akshu, On Mon, Apr 22, 2019 at 2:15 PM Agrawal, Akshu <Akshu.Agrawal@amd.com> wrote: > > We need to set minimum bclk 64x of wclk as this is hw constraint in one > of the component used. > Since, clk_set_rate and clk is enabled in machine driver the > clk_set_rate in hw_params of da7219 fails and errors out and when it > tries to override the value. > > In cases like these not only clk_set_rate of da7219 codec should fail > and not override the value but also should not error out. > > Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> > --- > sound/soc/codecs/da7219.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c > index 5f5fa3416af3..a041dbf442af 100644 > --- a/sound/soc/codecs/da7219.c > +++ b/sound/soc/codecs/da7219.c > @@ -1621,13 +1621,7 @@ static int da7219_hw_params(struct snd_pcm_substream *substream, > > if (bclk) { > bclk_rate = frame_size * sr; > - ret = clk_set_rate(bclk, bclk_rate); > - if (ret) { > - dev_err(component->dev, > - "Failed to set BCLK rate %lu: %d\n", > - bclk_rate, ret); > - return ret; > - } > + clk_set_rate(bclk, bclk_rate); I think this might mask the real issue when setting rate fails. A more fundamental problem might be what are the acceptable ratios between bclk and wclk. From existing code of da7219 I see there are only two choices of frame_size: 32 or 64. In da7219_hw_params: ... if (da7219->master && !da7219->tdm_en) { if ((word_len * DA7219_DAI_CH_NUM_MAX) <= 32) frame_size = 32; else frame_size = 64; And in https://lkml.org/lkml/2019/4/17/322 the choice from machine driver is 64 or higher. In cz_da7219_params: ... if (bclk < (wclk * 64)) bclk = wclk * 64; return da7219_clk_enable(substream, wclk, bclk); I don't know what is the best solution to avoid this conflict. Maybe just set a fixed ratio from device property that can best work for the combination of codecs on the machine? > } else { > ret = da7219_set_bclks_per_wclk(component, frame_size); > if (ret) { > -- > 2.17.1 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 4/22/2019 1:09 PM, Cheng-yi Chiang wrote: > Hi Akshu, > > > On Mon, Apr 22, 2019 at 2:15 PM Agrawal, Akshu <Akshu.Agrawal@amd.com> wrote: >> >> We need to set minimum bclk 64x of wclk as this is hw constraint in one >> of the component used. >> Since, clk_set_rate and clk is enabled in machine driver the >> clk_set_rate in hw_params of da7219 fails and errors out and when it >> tries to override the value. >> >> In cases like these not only clk_set_rate of da7219 codec should fail >> and not override the value but also should not error out. >> >> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> >> --- >> sound/soc/codecs/da7219.c | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c >> index 5f5fa3416af3..a041dbf442af 100644 >> --- a/sound/soc/codecs/da7219.c >> +++ b/sound/soc/codecs/da7219.c >> @@ -1621,13 +1621,7 @@ static int da7219_hw_params(struct snd_pcm_substream *substream, >> >> if (bclk) { >> bclk_rate = frame_size * sr; >> - ret = clk_set_rate(bclk, bclk_rate); >> - if (ret) { >> - dev_err(component->dev, >> - "Failed to set BCLK rate %lu: %d\n", >> - bclk_rate, ret); >> - return ret; >> - } >> + clk_set_rate(bclk, bclk_rate); > > I think this might mask the real issue when setting rate fails. Not doing an error check for clk_set_rate is not rare because clk_set_rate fails if the clk which you are trying to set is already enabled. Now we can add a is_prepared check before calling clk_set_rate but that would be redundant as clk_set_rate does the same check. > A more fundamental problem might be what are the acceptable ratios > between bclk and wclk. > acceptable ratios for da7219 from spec are: 00 = 32 BCLKS per WCLK 01 = 64 BCLKS per WCLK 10 = 128 BCLK per WCLK 11 = 256 BCLK per WCLK da7219 derives this ration from SR for a wclk. The same we do in machine driver. Conflict comes only in case of an exception where there is constraint from a hw component. Else when the audio is played on da7219 codec we could have let da7219 only set rate and enable clks. Thanks, Akshu > From existing code of da7219 I see there are only two choices of > frame_size: 32 or 64. > > In da7219_hw_params: > ... > if (da7219->master && !da7219->tdm_en) { > if ((word_len * DA7219_DAI_CH_NUM_MAX) <= 32) > frame_size = 32; > else > frame_size = 64; > > And in https://lkml.org/lkml/2019/4/17/322 > the choice from machine driver is 64 or higher. > > In cz_da7219_params: > ... > if (bclk < (wclk * 64)) > bclk = wclk * 64; > return da7219_clk_enable(substream, wclk, bclk); > > I don't know what is the best solution to avoid this conflict. > Maybe just set a fixed ratio from device property that can best work > for the combination of codecs on the machine? > > > > > >> } else { >> ret = da7219_set_bclks_per_wclk(component, frame_size); >> if (ret) { >> -- >> 2.17.1 >> >> _______________________________________________ >> Alsa-devel mailing list >> Alsa-devel@alsa-project.org >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index 5f5fa3416af3..a041dbf442af 100644 --- a/sound/soc/codecs/da7219.c +++ b/sound/soc/codecs/da7219.c @@ -1621,13 +1621,7 @@ static int da7219_hw_params(struct snd_pcm_substream *substream, if (bclk) { bclk_rate = frame_size * sr; - ret = clk_set_rate(bclk, bclk_rate); - if (ret) { - dev_err(component->dev, - "Failed to set BCLK rate %lu: %d\n", - bclk_rate, ret); - return ret; - } + clk_set_rate(bclk, bclk_rate); } else { ret = da7219_set_bclks_per_wclk(component, frame_size); if (ret) {
We need to set minimum bclk 64x of wclk as this is hw constraint in one of the component used. Since, clk_set_rate and clk is enabled in machine driver the clk_set_rate in hw_params of da7219 fails and errors out and when it tries to override the value. In cases like these not only clk_set_rate of da7219 codec should fail and not override the value but also should not error out. Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> --- sound/soc/codecs/da7219.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)