| Message ID | 1662373788-19561-1-git-send-email-shengjiu.wang@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: fsl_asrc: Add initialization finishing check in runtime resume | expand |
On Mon, Sep 5, 2022 at 9:15 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote: > On Mon, Sep 5, 2022 at 3:47 AM Shengjiu Wang <shengjiu.wang@nxp.com> > wrote: > > @@ -1295,6 +1301,17 @@ static int fsl_asrc_runtime_resume(struct device > *dev) > > regmap_update_bits(asrc->regmap, REG_ASRCTR, > > ASRCTR_ASRCEi_ALL_MASK, asrctr); > > > > + /* Wait for status of initialization for every enabled pairs */ > > + do { > > + udelay(5); > > + regmap_read(asrc->regmap, REG_ASRCFG, ®); > > + reg = (reg >> ASRCFG_INIRQi_SHIFT(0)) & 0x7; > > + } while ((reg != ((asrctr >> ASRCTR_ASRCEi_SHIFT(0)) & 0x7)) && > --retry); > > + > > + /* FIXME: Doesn't treat initialization timeout as error */ > > + if (!retry) > > + dev_warn(dev, "initialization isn't finished\n"); > > Any reason why not just dev_err? Just hesitate to use dev_err. if use dev_err, then should return an error. May one of the pairs is finished, it still can continue. Best regards Wang Shengjiu
On Mon, Sep 5, 2022 at 6:54 PM Shengjiu Wang <shengjiu.wang@gmail.com> wrote: >> > + /* Wait for status of initialization for every enabled pairs */ >> > + do { >> > + udelay(5); >> > + regmap_read(asrc->regmap, REG_ASRCFG, ®); >> > + reg = (reg >> ASRCFG_INIRQi_SHIFT(0)) & 0x7; >> > + } while ((reg != ((asrctr >> ASRCTR_ASRCEi_SHIFT(0)) & 0x7)) && --retry); >> > + >> > + /* FIXME: Doesn't treat initialization timeout as error */ >> > + if (!retry) >> > + dev_warn(dev, "initialization isn't finished\n"); >> >> Any reason why not just dev_err? > > Just hesitate to use dev_err. if use dev_err, then should return an error. > May one of the pairs is finished, it still can continue. Makes sense. In that case, why "FIXME" :)
On Tue, Sep 6, 2022 at 5:50 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote: > On Mon, Sep 5, 2022 at 6:54 PM Shengjiu Wang <shengjiu.wang@gmail.com> > wrote: > > >> > + /* Wait for status of initialization for every enabled pairs > */ > >> > + do { > >> > + udelay(5); > >> > + regmap_read(asrc->regmap, REG_ASRCFG, ®); > >> > + reg = (reg >> ASRCFG_INIRQi_SHIFT(0)) & 0x7; > >> > + } while ((reg != ((asrctr >> ASRCTR_ASRCEi_SHIFT(0)) & 0x7)) > && --retry); > >> > + > >> > + /* FIXME: Doesn't treat initialization timeout as error */ > >> > + if (!retry) > >> > + dev_warn(dev, "initialization isn't finished\n"); > >> > >> Any reason why not just dev_err? > > > > Just hesitate to use dev_err. if use dev_err, then should return an > error. > > May one of the pairs is finished, it still can continue. > > Makes sense. In that case, why "FIXME" :) > Just want to have a record/note here, need to care about this warning. Best regards Wang shengjiu
On Tue, Sep 6, 2022 at 3:50 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote: >> >> > + /* Wait for status of initialization for every enabled pairs */ >> >> > + do { >> >> > + udelay(5); >> >> > + regmap_read(asrc->regmap, REG_ASRCFG, ®); >> >> > + reg = (reg >> ASRCFG_INIRQi_SHIFT(0)) & 0x7; >> >> > + } while ((reg != ((asrctr >> ASRCTR_ASRCEi_SHIFT(0)) & 0x7)) && --retry); >> >> > + >> >> > + /* FIXME: Doesn't treat initialization timeout as error */ >> >> > + if (!retry) >> >> > + dev_warn(dev, "initialization isn't finished\n"); >> >> >> >> Any reason why not just dev_err? >> > >> > Just hesitate to use dev_err. if use dev_err, then should return an error. >> > May one of the pairs is finished, it still can continue. >> >> Makes sense. In that case, why "FIXME" :) > Just want to have a record/note here, need to care about this warning. "FIXME" feels like something is wrong and literally means that it is waiting for a fix/solution. In your case, it's not waiting for a fix at all, but just an annotation? So, shouldn't it be just "Note:"?
On Tue, Sep 6, 2022 at 7:42 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote: > On Tue, Sep 6, 2022 at 3:50 AM Shengjiu Wang <shengjiu.wang@gmail.com> > wrote: > >> >> > + /* Wait for status of initialization for every enabled > pairs */ > >> >> > + do { > >> >> > + udelay(5); > >> >> > + regmap_read(asrc->regmap, REG_ASRCFG, ®); > >> >> > + reg = (reg >> ASRCFG_INIRQi_SHIFT(0)) & 0x7; > >> >> > + } while ((reg != ((asrctr >> ASRCTR_ASRCEi_SHIFT(0)) & > 0x7)) && --retry); > >> >> > + > >> >> > + /* FIXME: Doesn't treat initialization timeout as error */ > >> >> > + if (!retry) > >> >> > + dev_warn(dev, "initialization isn't finished\n"); > >> >> > >> >> Any reason why not just dev_err? > >> > > >> > Just hesitate to use dev_err. if use dev_err, then should return an > error. > >> > May one of the pairs is finished, it still can continue. > >> > >> Makes sense. In that case, why "FIXME" :) > > > Just want to have a record/note here, need to care about this warning. > > "FIXME" feels like something is wrong and literally means that it is > waiting for a fix/solution. In your case, it's not waiting for a fix > at all, but just an annotation? So, shouldn't it be just "Note:"? > ok, let me update it. best regards wang shengjiu
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index aa5edf32d988..bae263b90ac1 100644 --- a/sound/soc/fsl/fsl_asrc.c +++ b/sound/soc/fsl/fsl_asrc.c @@ -20,6 +20,7 @@ #define IDEAL_RATIO_DECIMAL_DEPTH 26 #define DIVIDER_NUM 64 +#define INIT_TRY_NUM 50 #define pair_err(fmt, ...) \ dev_err(&asrc->pdev->dev, "Pair %c: " fmt, 'A' + index, ##__VA_ARGS__) @@ -579,7 +580,7 @@ static void fsl_asrc_start_pair(struct fsl_asrc_pair *pair) { struct fsl_asrc *asrc = pair->asrc; enum asrc_pair_index index = pair->index; - int reg, retry = 10, i; + int reg, retry = INIT_TRY_NUM, i; /* Enable the current pair */ regmap_update_bits(asrc->regmap, REG_ASRCTR, @@ -592,6 +593,10 @@ static void fsl_asrc_start_pair(struct fsl_asrc_pair *pair) reg &= ASRCFG_INIRQi_MASK(index); } while (!reg && --retry); + /* FIXME: Doesn't treat initialization timeout as error */ + if (!retry) + dev_warn(&asrc->pdev->dev, "initialization isn't finished\n"); + /* Make the input fifo to ASRC STALL level */ regmap_read(asrc->regmap, REG_ASRCNCR, ®); for (i = 0; i < pair->channels * 4; i++) @@ -1257,6 +1262,7 @@ static int fsl_asrc_runtime_resume(struct device *dev) { struct fsl_asrc *asrc = dev_get_drvdata(dev); struct fsl_asrc_priv *asrc_priv = asrc->private; + int reg, retry = INIT_TRY_NUM; int i, ret; u32 asrctr; @@ -1295,6 +1301,17 @@ static int fsl_asrc_runtime_resume(struct device *dev) regmap_update_bits(asrc->regmap, REG_ASRCTR, ASRCTR_ASRCEi_ALL_MASK, asrctr); + /* Wait for status of initialization for every enabled pairs */ + do { + udelay(5); + regmap_read(asrc->regmap, REG_ASRCFG, ®); + reg = (reg >> ASRCFG_INIRQi_SHIFT(0)) & 0x7; + } while ((reg != ((asrctr >> ASRCTR_ASRCEi_SHIFT(0)) & 0x7)) && --retry); + + /* FIXME: Doesn't treat initialization timeout as error */ + if (!retry) + dev_warn(dev, "initialization isn't finished\n"); + return 0; disable_asrck_clk:
If the initialization is not finished, then filling input data to the FIFO may fail. So it is better to add initialization finishing check in the runtime resume for suspend & resume case. And consider the case of three instances working in parallel, increase the retry times to 50 for more initialization time. Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> --- sound/soc/fsl/fsl_asrc.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)