[alsa-devel] [PATCH 0/3] ASoC: mxs-saif error handling
Hi,
This series contains error handling patches for mxs-saif. The first patch is a comment about the possible command sequence of trigger functions.
Regards,
Markus Pargmann
Markus Pargmann (3): ASoC: snd_soc_dai_ops trigger function description ASoC: mxs-saif: Store saif state ASoC: mxs-saif: Handle errors in trigger function
include/sound/soc-dai.h | 7 +++++++ sound/soc/mxs/mxs-saif.c | 30 +++++++++++++++++++++++++----- sound/soc/mxs/mxs-saif.h | 5 +++++ 3 files changed, 37 insertions(+), 5 deletions(-)
Add a comment to the trigger function in snd_soc_dai_ops struct about possible command sequences.
Signed-off-by: Markus Pargmann mpa@pengutronix.de --- include/sound/soc-dai.h | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index ae9a227..0f2e5da 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -166,6 +166,13 @@ struct snd_soc_dai_ops { struct snd_soc_dai *); int (*prepare)(struct snd_pcm_substream *, struct snd_soc_dai *); + /* + * NOTE: Commands passed to the trigger function are not necessarily + * compatible with the current state of the dai. For example this + * sequence of commands is possible: START STOP STOP. + * So do not unconditionally use refcounting functions in the trigger + * function, e.g. clk_enable/disable. + */ int (*trigger)(struct snd_pcm_substream *, int, struct snd_soc_dai *); int (*bespoke_trigger)(struct snd_pcm_substream *, int,
Trigger commands may be passed multiple times. To avoid errors with clk_enable/disable, store the saif state and return if saif is already running/stopped.
Signed-off-by: Markus Pargmann mpa@pengutronix.de --- sound/soc/mxs/mxs-saif.c | 8 ++++++++ sound/soc/mxs/mxs-saif.h | 5 +++++ 2 files changed, 13 insertions(+)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index b56b8a0..c8ead01 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -503,6 +503,9 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + if (saif->state == MXS_SAIF_STATE_RUNNING) + return 0; + dev_dbg(cpu_dai->dev, "start\n");
clk_enable(master_saif->clk); @@ -543,6 +546,7 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd, }
master_saif->ongoing = 1; + saif->state = MXS_SAIF_STATE_RUNNING;
dev_dbg(saif->dev, "CTRL 0x%x STAT 0x%x\n", __raw_readl(saif->base + SAIF_CTRL), @@ -555,6 +559,9 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + if (saif->state == MXS_SAIF_STATE_STOPPED) + return 0; + dev_dbg(cpu_dai->dev, "stop\n");
/* wait a while for the current sample to complete */ @@ -575,6 +582,7 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd, }
master_saif->ongoing = 0; + saif->state = MXS_SAIF_STATE_STOPPED;
break; default: diff --git a/sound/soc/mxs/mxs-saif.h b/sound/soc/mxs/mxs-saif.h index 53eaa4b..fbaf7ba 100644 --- a/sound/soc/mxs/mxs-saif.h +++ b/sound/soc/mxs/mxs-saif.h @@ -124,6 +124,11 @@ struct mxs_saif {
u32 fifo_underrun; u32 fifo_overrun; + + enum { + MXS_SAIF_STATE_STOPPED, + MXS_SAIF_STATE_RUNNING, + } state; };
extern int mxs_saif_put_mclk(unsigned int saif_id);
Hi,
Trigger commands may be passed multiple times. To avoid errors with clk_enable/disable, store the saif state and return if saif is already running/stopped.
Signed-off-by: Markus Pargmann mpa@pengutronix.de
sound/soc/mxs/mxs-saif.c | 8 ++++++++ sound/soc/mxs/mxs-saif.h | 5 +++++ 2 files changed, 13 insertions(+)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index b56b8a0..c8ead01 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -503,6 +503,9 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
if (saif->state == MXS_SAIF_STATE_RUNNING)return 0;dev_dbg(cpu_dai->dev, "start\n");
clk_enable(master_saif->clk);
@@ -543,6 +546,7 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd, }
master_saif->ongoing = 1;
saif->state = MXS_SAIF_STATE_RUNNING;
It seems to me that you could use the already existing variable 'ongoing' that already reflects the state like you need it.
Lothar Waßmann
Hi,
On Fri, Oct 11, 2013 at 12:31:46PM +0200, Lothar Waßmann wrote:
Hi,
Trigger commands may be passed multiple times. To avoid errors with clk_enable/disable, store the saif state and return if saif is already running/stopped.
Signed-off-by: Markus Pargmann mpa@pengutronix.de
sound/soc/mxs/mxs-saif.c | 8 ++++++++ sound/soc/mxs/mxs-saif.h | 5 +++++ 2 files changed, 13 insertions(+)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index b56b8a0..c8ead01 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -503,6 +503,9 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
if (saif->state == MXS_SAIF_STATE_RUNNING)return 0;dev_dbg(cpu_dai->dev, "start\n");
clk_enable(master_saif->clk);
@@ -543,6 +546,7 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd, }
master_saif->ongoing = 1;
saif->state = MXS_SAIF_STATE_RUNNING;It seems to me that you could use the already existing variable 'ongoing' that already reflects the state like you need it.
'ongoing' can only be used if master_saif == saif. Otherwise this variable does not reflect the state of the saif. For example there could be parallel audio playback and recording, setting the variable only on the master_saif->ongoing but not for the second saif.
Regards,
Markus Pargmann
Signed-off-by: Markus Pargmann mpa@pengutronix.de --- sound/soc/mxs/mxs-saif.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index c8ead01..fc3d89b 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -494,6 +494,7 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd, struct mxs_saif *saif = snd_soc_dai_get_drvdata(cpu_dai); struct mxs_saif *master_saif; u32 delay; + int ret;
master_saif = mxs_saif_get_master(saif); if (!master_saif) @@ -508,21 +509,32 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd,
dev_dbg(cpu_dai->dev, "start\n");
- clk_enable(master_saif->clk); - if (!master_saif->mclk_in_use) - __raw_writel(BM_SAIF_CTRL_RUN, - master_saif->base + SAIF_CTRL + MXS_SET_ADDR); + ret = clk_enable(master_saif->clk); + if (ret) { + dev_err(saif->dev, "Failed to enable master clock\n"); + return ret; + }
/* * If the saif's master is not himself, we also need to enable * itself clk for its internal basic logic to work. */ if (saif != master_saif) { - clk_enable(saif->clk); + ret = clk_enable(saif->clk); + if (ret) { + dev_err(saif->dev, "Failed to enable master clock\n"); + clk_disable(master_saif->clk); + return ret; + } + __raw_writel(BM_SAIF_CTRL_RUN, saif->base + SAIF_CTRL + MXS_SET_ADDR); }
+ if (!master_saif->mclk_in_use) + __raw_writel(BM_SAIF_CTRL_RUN, + master_saif->base + SAIF_CTRL + MXS_SET_ADDR); + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { /* * write data to saif data register to trigger
participants (3)
-
Lothar Waßmann -
Mark Brown -
Markus Pargmann