From: Shengjiu Wang <[email protected]>
recover the channel swap after xrun
Shengjiu Wang (2):
ASoC: fsl_esai: Wrap some operations to be functions
ASoC: fsl_esai: recover the channel swap after xrun
sound/soc/fsl/fsl_esai.c | 267 ++++++++++++++++++++++++++++-----------
1 file changed, 194 insertions(+), 73 deletions(-)
Changes in v2
- add one patch for wrap operations to functions.
- fix some coding style issue
--
2.21.0
From: Shengjiu Wang <[email protected]>
Extract the operation to be functions, to improve the
readability.
In this patch, fsl_esai_init, fsl_esai_register_restore,
fsl_esai_trigger_start and fsl_esai_trigger_stop are
extracted.
Signed-off-by: Shengjiu Wang <[email protected]>
---
sound/soc/fsl/fsl_esai.c | 191 ++++++++++++++++++++++++---------------
1 file changed, 118 insertions(+), 73 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 10d2210c91ef..20039ae9893b 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -35,6 +35,7 @@
* @fifo_depth: depth of tx/rx FIFO
* @slot_width: width of each DAI slot
* @slots: number of slots
+ * @channels: channel num for tx or rx
* @hck_rate: clock rate of desired HCKx clock
* @sck_rate: clock rate of desired SCKx clock
* @hck_dir: the direction of HCKx pads
@@ -57,6 +58,7 @@ struct fsl_esai {
u32 slots;
u32 tx_mask;
u32 rx_mask;
+ u32 channels[2];
u32 hck_rate[2];
u32 sck_rate[2];
bool hck_dir[2];
@@ -543,64 +545,131 @@ static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
return 0;
}
-static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
- struct snd_soc_dai *dai)
+static int fsl_esai_init(struct fsl_esai *esai_priv)
{
- struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
- bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
- u8 i, channels = substream->runtime->channels;
+ struct platform_device *pdev = esai_priv->pdev;
+ int ret;
+
+ /* Reset ESAI unit */
+ ret = regmap_update_bits(esai_priv->regmap, REG_ESAI_ECR,
+ ESAI_ECR_ESAIEN_MASK | ESAI_ECR_ERST_MASK,
+ ESAI_ECR_ESAIEN | ESAI_ECR_ERST);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to reset ESAI: %d\n", ret);
+ return ret;
+ }
+
+ /*
+ * We need to enable ESAI so as to access some of its registers.
+ * Otherwise, we would fail to dump regmap from user space.
+ */
+ ret = regmap_update_bits(esai_priv->regmap, REG_ESAI_ECR,
+ ESAI_ECR_ESAIEN_MASK | ESAI_ECR_ERST_MASK,
+ ESAI_ECR_ESAIEN);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to enable ESAI: %d\n", ret);
+ return ret;
+ }
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
+ ESAI_PRRC_PDC_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_PCRC,
+ ESAI_PCRC_PC_MASK, 0);
+
+ return 0;
+}
+
+static int fsl_esai_register_restore(struct fsl_esai *esai_priv)
+{
+ int ret;
+ /* FIFO reset for safety */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
+ ESAI_xFCR_xFR, ESAI_xFCR_xFR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR,
+ ESAI_xFCR_xFR, ESAI_xFCR_xFR);
+
+ regcache_mark_dirty(esai_priv->regmap);
+ ret = regcache_sync(esai_priv->regmap);
+ if (ret)
+ return ret;
+
+ /* FIFO reset done */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR, ESAI_xFCR_xFR, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RFCR, ESAI_xFCR_xFR, 0);
+
+ return 0;
+}
+
+static void fsl_esai_trigger_start(struct fsl_esai *esai_priv, bool tx)
+{
+ u8 i, channels = esai_priv->channels[tx];
u32 pins = DIV_ROUND_UP(channels, esai_priv->slots);
u32 mask;
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
+ ESAI_xFCR_xFEN_MASK, ESAI_xFCR_xFEN);
+
+ /* Write initial words reqiured by ESAI as normal procedure */
+ for (i = 0; tx && i < channels; i++)
+ regmap_write(esai_priv->regmap, REG_ESAI_ETDR, 0x0);
+
+ /*
+ * When set the TE/RE in the end of enablement flow, there
+ * will be channel swap issue for multi data line case.
+ * In order to workaround this issue, we switch the bit
+ * enablement sequence to below sequence
+ * 1) clear the xSMB & xSMA: which is done in probe and
+ * stop state.
+ * 2) set TE/RE
+ * 3) set xSMB
+ * 4) set xSMA: xSMA is the last one in this flow, which
+ * will trigger esai to start.
+ */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
+ tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK,
+ tx ? ESAI_xCR_TE(pins) : ESAI_xCR_RE(pins));
+ mask = tx ? esai_priv->tx_mask : esai_priv->rx_mask;
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
+ ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(mask));
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
+ ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
+}
+
+static void fsl_esai_trigger_stop(struct fsl_esai *esai_priv, bool tx)
+{
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
+ tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
+ ESAI_xSMA_xS_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
+ ESAI_xSMB_xS_MASK, 0);
+
+ /* Disable and reset FIFO */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
+ ESAI_xFCR_xFR | ESAI_xFCR_xFEN, ESAI_xFCR_xFR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
+ ESAI_xFCR_xFR, 0);
+}
+
+static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
+ struct snd_soc_dai *dai)
+{
+ struct fsl_esai *esai_priv = snd_soc_dai_get_drvdata(dai);
+ bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
+
+ esai_priv->channels[tx] = substream->runtime->channels;
+
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_RESUME:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
- ESAI_xFCR_xFEN_MASK, ESAI_xFCR_xFEN);
-
- /* Write initial words reqiured by ESAI as normal procedure */
- for (i = 0; tx && i < channels; i++)
- regmap_write(esai_priv->regmap, REG_ESAI_ETDR, 0x0);
-
- /*
- * When set the TE/RE in the end of enablement flow, there
- * will be channel swap issue for multi data line case.
- * In order to workaround this issue, we switch the bit
- * enablement sequence to below sequence
- * 1) clear the xSMB & xSMA: which is done in probe and
- * stop state.
- * 2) set TE/RE
- * 3) set xSMB
- * 4) set xSMA: xSMA is the last one in this flow, which
- * will trigger esai to start.
- */
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
- tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK,
- tx ? ESAI_xCR_TE(pins) : ESAI_xCR_RE(pins));
- mask = tx ? esai_priv->tx_mask : esai_priv->rx_mask;
-
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
- ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(mask));
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
- ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
-
+ fsl_esai_trigger_start(esai_priv, tx);
break;
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
- tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK, 0);
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
- ESAI_xSMA_xS_MASK, 0);
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMB(tx),
- ESAI_xSMB_xS_MASK, 0);
-
- /* Disable and reset FIFO */
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
- ESAI_xFCR_xFR | ESAI_xFCR_xFEN, ESAI_xFCR_xFR);
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
- ESAI_xFCR_xFR, 0);
+ fsl_esai_trigger_stop(esai_priv, tx);
break;
default:
return -EINVAL;
@@ -866,22 +935,9 @@ static int fsl_esai_probe(struct platform_device *pdev)
dev_set_drvdata(&pdev->dev, esai_priv);
- /* Reset ESAI unit */
- ret = regmap_write(esai_priv->regmap, REG_ESAI_ECR, ESAI_ECR_ERST);
- if (ret) {
- dev_err(&pdev->dev, "failed to reset ESAI: %d\n", ret);
+ ret = fsl_esai_init(esai_priv);
+ if (ret)
return ret;
- }
-
- /*
- * We need to enable ESAI so as to access some of its registers.
- * Otherwise, we would fail to dump regmap from user space.
- */
- ret = regmap_write(esai_priv->regmap, REG_ESAI_ECR, ESAI_ECR_ESAIEN);
- if (ret) {
- dev_err(&pdev->dev, "failed to enable ESAI: %d\n", ret);
- return ret;
- }
esai_priv->tx_mask = 0xFFFFFFFF;
esai_priv->rx_mask = 0xFFFFFFFF;
@@ -955,20 +1011,10 @@ static int fsl_esai_runtime_resume(struct device *dev)
regcache_cache_only(esai->regmap, false);
- /* FIFO reset for safety */
- regmap_update_bits(esai->regmap, REG_ESAI_TFCR,
- ESAI_xFCR_xFR, ESAI_xFCR_xFR);
- regmap_update_bits(esai->regmap, REG_ESAI_RFCR,
- ESAI_xFCR_xFR, ESAI_xFCR_xFR);
-
- ret = regcache_sync(esai->regmap);
+ ret = fsl_esai_register_restore(esai);
if (ret)
goto err_regcache_sync;
- /* FIFO reset done */
- regmap_update_bits(esai->regmap, REG_ESAI_TFCR, ESAI_xFCR_xFR, 0);
- regmap_update_bits(esai->regmap, REG_ESAI_RFCR, ESAI_xFCR_xFR, 0);
-
return 0;
err_regcache_sync:
@@ -991,7 +1037,6 @@ static int fsl_esai_runtime_suspend(struct device *dev)
struct fsl_esai *esai = dev_get_drvdata(dev);
regcache_cache_only(esai->regmap, true);
- regcache_mark_dirty(esai->regmap);
if (!IS_ERR(esai->fsysclk))
clk_disable_unprepare(esai->fsysclk);
--
2.21.0
From: Shengjiu Wang <[email protected]>
There is chip errata ERR008000, the reference doc is
(https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf),
The issue is "While using ESAI transmit or receive and
an underrun/overrun happens, channel swap may occur.
The only recovery mechanism is to reset the ESAI."
This issue exist in imx3/imx5/imx6(partial) series.
In this commit add a tasklet to handle reset of ESAI
after xrun happens to recover the channel swap.
Signed-off-by: Shengjiu Wang <[email protected]>
---
sound/soc/fsl/fsl_esai.c | 76 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 20039ae9893b..8c92e49ad6d8 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -32,6 +32,7 @@
* @extalclk: esai clock source to derive HCK, SCK and FS
* @fsysclk: system clock source to derive HCK, SCK and FS
* @spbaclk: SPBA clock (optional, depending on SoC design)
+ * @task: tasklet to handle the reset operation
* @fifo_depth: depth of tx/rx FIFO
* @slot_width: width of each DAI slot
* @slots: number of slots
@@ -42,6 +43,7 @@
* @sck_div: if using PSR/PM dividers for SCKx clock
* @slave_mode: if fully using DAI slave mode
* @synchronous: if using tx/rx synchronous mode
+ * @reset_at_xrun: flags for enable reset operaton
* @name: driver name
*/
struct fsl_esai {
@@ -53,6 +55,7 @@ struct fsl_esai {
struct clk *extalclk;
struct clk *fsysclk;
struct clk *spbaclk;
+ struct tasklet_struct task;
u32 fifo_depth;
u32 slot_width;
u32 slots;
@@ -65,6 +68,7 @@ struct fsl_esai {
bool sck_div[2];
bool slave_mode;
bool synchronous;
+ bool reset_at_xrun;
char name[32];
};
@@ -73,8 +77,16 @@ static irqreturn_t esai_isr(int irq, void *devid)
struct fsl_esai *esai_priv = (struct fsl_esai *)devid;
struct platform_device *pdev = esai_priv->pdev;
u32 esr;
+ u32 saisr;
regmap_read(esai_priv->regmap, REG_ESAI_ESR, &esr);
+ regmap_read(esai_priv->regmap, REG_ESAI_SAISR, &saisr);
+
+ if ((saisr & (ESAI_SAISR_TUE | ESAI_SAISR_ROE)) &&
+ esai_priv->reset_at_xrun) {
+ dev_dbg(&pdev->dev, "reset module for xrun\n");
+ tasklet_schedule(&esai_priv->task);
+ }
if (esr & ESAI_ESR_TINIT_MASK)
dev_dbg(&pdev->dev, "isr: Transmission Initialized\n");
@@ -634,10 +646,17 @@ static void fsl_esai_trigger_start(struct fsl_esai *esai_priv, bool tx)
ESAI_xSMB_xS_MASK, ESAI_xSMB_xS(mask));
regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
ESAI_xSMA_xS_MASK, ESAI_xSMA_xS(mask));
+
+ /* Enable Exception interrupt */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
+ ESAI_xCR_xEIE_MASK, ESAI_xCR_xEIE);
}
static void fsl_esai_trigger_stop(struct fsl_esai *esai_priv, bool tx)
{
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
+ ESAI_xCR_xEIE_MASK, 0);
+
regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
tx ? ESAI_xCR_TE_MASK : ESAI_xCR_RE_MASK, 0);
regmap_update_bits(esai_priv->regmap, REG_ESAI_xSMA(tx),
@@ -652,6 +671,53 @@ static void fsl_esai_trigger_stop(struct fsl_esai *esai_priv, bool tx)
ESAI_xFCR_xFR, 0);
}
+static void fsl_esai_reset(unsigned long arg)
+{
+ struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
+ u32 saisr, tfcr, rfcr;
+
+ /* save the registers */
+ regmap_read(esai_priv->regmap, REG_ESAI_TFCR, &tfcr);
+ regmap_read(esai_priv->regmap, REG_ESAI_RFCR, &rfcr);
+
+ /* stop the tx & rx */
+ fsl_esai_trigger_stop(esai_priv, 1);
+ fsl_esai_trigger_stop(esai_priv, 0);
+
+ /* reset the esai, and restore the registers */
+ fsl_esai_init(esai_priv);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_xPR_MASK,
+ ESAI_xCR_xPR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
+ ESAI_xCR_xPR_MASK,
+ ESAI_xCR_xPR);
+
+ /* restore registers by regcache_sync */
+ fsl_esai_register_restore(esai_priv);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_xPR_MASK, 0);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
+ ESAI_xCR_xPR_MASK, 0);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
+ ESAI_PRRC_PDC_MASK,
+ ESAI_PRRC_PDC(ESAI_GPIO));
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_PCRC,
+ ESAI_PCRC_PC_MASK,
+ ESAI_PCRC_PC(ESAI_GPIO));
+
+ regmap_read(esai_priv->regmap, REG_ESAI_SAISR, &saisr);
+
+ /* restart tx / rx, if they already enabled */
+ if (tfcr & ESAI_xFCR_xFEN)
+ fsl_esai_trigger_start(esai_priv, 1);
+ if (rfcr & ESAI_xFCR_xFEN)
+ fsl_esai_trigger_start(esai_priv, 0);
+}
+
static int fsl_esai_trigger(struct snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *dai)
{
@@ -856,6 +922,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
esai_priv->pdev = pdev;
snprintf(esai_priv->name, sizeof(esai_priv->name), "%pOFn", np);
+ if (of_device_is_compatible(np, "fsl,vf610-esai") ||
+ of_device_is_compatible(np, "fsl,imx35-esai"))
+ esai_priv->reset_at_xrun = true;
+
/* Get the addresses and IRQ */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
regs = devm_ioremap_resource(&pdev->dev, res);
@@ -955,6 +1025,9 @@ static int fsl_esai_probe(struct platform_device *pdev)
return ret;
}
+ tasklet_init(&esai_priv->task, fsl_esai_reset,
+ (unsigned long)esai_priv);
+
pm_runtime_enable(&pdev->dev);
regcache_cache_only(esai_priv->regmap, true);
@@ -968,7 +1041,10 @@ static int fsl_esai_probe(struct platform_device *pdev)
static int fsl_esai_remove(struct platform_device *pdev)
{
+ struct fsl_esai *esai_priv = platform_get_drvdata(pdev);
+
pm_runtime_disable(&pdev->dev);
+ tasklet_kill(&esai_priv->task);
return 0;
}
--
2.21.0
Looks good to me, yet two small comments inline.
Please add this to this patch in the next version:
Acked-by: Nicolin Chen <[email protected]>
On Wed, Jul 03, 2019 at 02:42:04PM +0800, [email protected] wrote:
> +static int fsl_esai_register_restore(struct fsl_esai *esai_priv)
> +{
> + int ret;
> + /* FIFO reset for safety */
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TFCR,
Checkpatch script would probably warn this. Usually we add a blank
line after variable declarations.
> @@ -866,22 +935,9 @@ static int fsl_esai_probe(struct platform_device *pdev)
>
> dev_set_drvdata(&pdev->dev, esai_priv);
>
> - /* Reset ESAI unit */
> - ret = regmap_write(esai_priv->regmap, REG_ESAI_ECR, ESAI_ECR_ERST);
> - if (ret) {
> - dev_err(&pdev->dev, "failed to reset ESAI: %d\n", ret);
> + ret = fsl_esai_init(esai_priv);
Could we rename this function to fsl_easi_hw_init() or something
clear like fsl_esai_register_init? fsl_easi_init() feels like a
driver init() function to me.
Thank you
Nicolin
On Wed, Jul 03, 2019 at 02:42:05PM +0800, [email protected] wrote:
> From: Shengjiu Wang <[email protected]>
>
> There is chip errata ERR008000, the reference doc is
> (https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf),
>
> The issue is "While using ESAI transmit or receive and
> an underrun/overrun happens, channel swap may occur.
> The only recovery mechanism is to reset the ESAI."
>
> This issue exist in imx3/imx5/imx6(partial) series.
>
> In this commit add a tasklet to handle reset of ESAI
> after xrun happens to recover the channel swap.
>
> Signed-off-by: Shengjiu Wang <[email protected]>
> ---
> sound/soc/fsl/fsl_esai.c | 76 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index 20039ae9893b..8c92e49ad6d8 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> +static void fsl_esai_reset(unsigned long arg)
Similarly fsl_esai_hw_reset? This one isn't really that bad though,
yet it feels better to have function naming in a similar style.
> +{
> + struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
> + u32 saisr, tfcr, rfcr;
> +
> + /* save the registers */
> + regmap_read(esai_priv->regmap, REG_ESAI_TFCR, &tfcr);
> + regmap_read(esai_priv->regmap, REG_ESAI_RFCR, &rfcr);
Instead of having this implicit comments, we could have:
+ bool tx = true, rx = false, enabled[2];
+
+ regmap_read(esai_priv->regmap, REG_ESAI_TFCR, &tfcr);
+ regmap_read(esai_priv->regmap, REG_ESAI_RFCR, &rfcr);
+ enabled[tx] = tfcr & ESAI_xFCR_xFEN;
+ enabled[rx] = rfcr & ESAI_xFCR_xFEN;
> +
> + /* stop the tx & rx */
> + fsl_esai_trigger_stop(esai_priv, 1);
> + fsl_esai_trigger_stop(esai_priv, 0);
And we could reuse the boolean 'tx' and 'rx' here.
> +
> + /* reset the esai, and restore the registers */
> + fsl_esai_init(esai_priv);
> +
[...]
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_xPR_MASK,
> + ESAI_xCR_xPR);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> + ESAI_xCR_xPR_MASK,
> + ESAI_xCR_xPR);
Mask and value might fit into one line?
> +
> + /* restore registers by regcache_sync */
> + fsl_esai_register_restore(esai_priv);
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_xPR_MASK, 0);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> + ESAI_xCR_xPR_MASK, 0);
And just for curious, can (or shall) we stuff this personal reset
to the reset() function? I found this one is a part of the reset
routine being mentioned in the RM -- it was done after ESAI reset
is done via ECR register.
[...]
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
> + ESAI_PRRC_PDC_MASK,
> + ESAI_PRRC_PDC(ESAI_GPIO));
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_PCRC,
> + ESAI_PCRC_PC_MASK,
> + ESAI_PCRC_PC(ESAI_GPIO));
Mask and value might fit into one line?
On 2019-07-03 08:42, [email protected] wrote:
> +static void fsl_esai_reset(unsigned long arg)
> +{
> + struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
> + u32 saisr, tfcr, rfcr;
> +
> + /* save the registers */
> + regmap_read(esai_priv->regmap, REG_ESAI_TFCR, &tfcr);
> + regmap_read(esai_priv->regmap, REG_ESAI_RFCR, &rfcr);
> +
> + /* stop the tx & rx */
> + fsl_esai_trigger_stop(esai_priv, 1);
> + fsl_esai_trigger_stop(esai_priv, 0);
> +
> + /* reset the esai, and restore the registers */
> + fsl_esai_init(esai_priv);
<comment below applies>
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_xPR_MASK,
> + ESAI_xCR_xPR);
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> + ESAI_xCR_xPR_MASK,
> + ESAI_xCR_xPR);
> +
> + /* restore registers by regcache_sync */
> + fsl_esai_register_restore(esai_priv);
> +
Both _init and _restore may fail given their declaration in 1/2 "ASoC:
fsl_esai: Wrap some operations to be functions" yet here you simply
ignore the return values.
If failure of said functions is permissive, it might be a good place for
a comment.
Czarek
>
> > +
> > + /* restore registers by regcache_sync */
> > + fsl_esai_register_restore(esai_priv);
> > +
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > + ESAI_xCR_xPR_MASK, 0);
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> > + ESAI_xCR_xPR_MASK, 0);
>
> And just for curious, can (or shall) we stuff this personal reset to the reset()
> function? I found this one is a part of the reset routine being mentioned in
> the RM -- it was done after ESAI reset is done via ECR register.
>
There is a problem to do this, TPR/RPR need to be clear after configure the control
register. (TCCR, TCR). So it seems not only one place (reset function) need to be
changed.
Best regards
Wang shengjiu
On Fri, Jul 05, 2019 at 07:03:47AM +0000, S.j. Wang wrote:
> >
> > > +
> > > + /* restore registers by regcache_sync */
> > > + fsl_esai_register_restore(esai_priv);
> > > +
> > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > + ESAI_xCR_xPR_MASK, 0);
> > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> > > + ESAI_xCR_xPR_MASK, 0);
> >
> > And just for curious, can (or shall) we stuff this personal reset to the reset()
> > function? I found this one is a part of the reset routine being mentioned in
> > the RM -- it was done after ESAI reset is done via ECR register.
> >
>
> There is a problem to do this, TPR/RPR need to be clear after configure the control
> register. (TCCR, TCR). So it seems not only one place (reset function) need to be
> changed.
Do you know (or remember) why we suddenly involve this TPR/PRP?
The driver has no problem so far, even if we don't have them.
The "personal reset" sounds like a feature that we would use to
reset TX or RX individually, while this hw_reset() does a full
reset for both TX and RX. So I wonder whether they're necessary.
> > > > +
> > > > + /* restore registers by regcache_sync */
> > > > + fsl_esai_register_restore(esai_priv);
> > > > +
> > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > > + ESAI_xCR_xPR_MASK, 0);
> > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
> > > > + ESAI_xCR_xPR_MASK, 0);
> > >
> > > And just for curious, can (or shall) we stuff this personal reset to
> > > the reset() function? I found this one is a part of the reset
> > > routine being mentioned in the RM -- it was done after ESAI reset is
> done via ECR register.
> > >
> >
> > There is a problem to do this, TPR/RPR need to be clear after
> > configure the control register. (TCCR, TCR). So it seems not only one
> > place (reset function) need to be changed.
>
> Do you know (or remember) why we suddenly involve this TPR/PRP?
> The driver has no problem so far, even if we don't have them.
>
> The "personal reset" sounds like a feature that we would use to reset TX or
> RX individually, while this hw_reset() does a full reset for both TX and RX.
> So I wonder whether they're necessary.
The hw_reset flow is suggested by design team, so involve TRP/RPP is from
them, I don't know the detail.
Best regards
Wang shengjiu