[PATCH v2 0/2] ASoC: rt1015p: delay 300ms for waiting calibration

[PATCH v2 0/2] ASoC: rt1015p: delay 300ms for waiting calibration

[Tzung-Bi Shih]

The 1st patch moves SDB control from DAI ops trigger to DAPM event
(per review comments in v1).

The 2nd patch adds the 300ms delay for waiting calibration.

Changes from v1:
(https://patchwork.kernel.org/project/alsa-devel/patch/20201209033742.3825973-1-tzungbi@google.com/)
- Move the delay from trigger to DAPM event.

Tzung-Bi Shih (2):
  ASoC: rt1015p: move SDB control from trigger to DAPM
  ASoC: rt1015p: delay 300ms after SDB pulling high for calibration

 sound/soc/codecs/rt1015p.c | 54 +++++++++++++-------------------------
 1 file changed, 18 insertions(+), 36 deletions(-)

-- 
2.29.2.576.ga3fc446d84-goog

[PATCH v2 1/2] ASoC: rt1015p: move SDB control from trigger to DAPM

[Tzung-Bi Shih]

Moves SDB control from DAI ops trigger to DAPM.

As long as BCLK and LRCLK are ready, SDB can be toggled earlier.

Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
 sound/soc/codecs/rt1015p.c | 49 ++++++++++----------------------------
 1 file changed, 12 insertions(+), 37 deletions(-)

diff --git a/sound/soc/codecs/rt1015p.c b/sound/soc/codecs/rt1015p.c
index 59bb60682270..397083ee5b37 100644
--- a/sound/soc/codecs/rt1015p.c
+++ b/sound/soc/codecs/rt1015p.c
@@ -19,60 +19,40 @@
 
 struct rt1015p_priv {
 	struct gpio_desc *sdb;
-	int sdb_switch;
 };
 
-static int rt1015p_daiops_trigger(struct snd_pcm_substream *substream,
-		int cmd, struct snd_soc_dai *dai)
+static int rt1015p_sdb_event(struct snd_soc_dapm_widget *w,
+		struct snd_kcontrol *kcontrol, int event)
 {
-	struct snd_soc_component *component = dai->component;
+	struct snd_soc_component *component =
+		snd_soc_dapm_to_component(w->dapm);
 	struct rt1015p_priv *rt1015p =
 		snd_soc_component_get_drvdata(component);
 
 	if (!rt1015p->sdb)
 		return 0;
 
-	switch (cmd) {
-	case SNDRV_PCM_TRIGGER_START:
-	case SNDRV_PCM_TRIGGER_RESUME:
-	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		if (rt1015p->sdb_switch) {
-			gpiod_set_value(rt1015p->sdb, 1);
-			dev_dbg(component->dev, "set sdb to 1");
-		}
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		gpiod_set_value(rt1015p->sdb, 1);
+		dev_dbg(component->dev, "set sdb to 1");
 		break;
-	case SNDRV_PCM_TRIGGER_STOP:
-	case SNDRV_PCM_TRIGGER_SUSPEND:
-	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+	case SND_SOC_DAPM_POST_PMD:
 		gpiod_set_value(rt1015p->sdb, 0);
 		dev_dbg(component->dev, "set sdb to 0");
 		break;
+	default:
+		break;
 	}
 
 	return 0;
 }
 
-static int rt1015p_sdb_event(struct snd_soc_dapm_widget *w,
-		struct snd_kcontrol *kcontrol, int event)
-{
-	struct snd_soc_component *component =
-		snd_soc_dapm_to_component(w->dapm);
-	struct rt1015p_priv *rt1015p =
-		snd_soc_component_get_drvdata(component);
-
-	if (event & SND_SOC_DAPM_POST_PMU)
-		rt1015p->sdb_switch = 1;
-	else if (event & SND_SOC_DAPM_POST_PMD)
-		rt1015p->sdb_switch = 0;
-
-	return 0;
-}
-
 static const struct snd_soc_dapm_widget rt1015p_dapm_widgets[] = {
 	SND_SOC_DAPM_OUTPUT("Speaker"),
 	SND_SOC_DAPM_OUT_DRV_E("SDB", SND_SOC_NOPM, 0, 0, NULL, 0,
 			rt1015p_sdb_event,
-			SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
+			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
 };
 
 static const struct snd_soc_dapm_route rt1015p_dapm_routes[] = {
@@ -91,10 +71,6 @@ static const struct snd_soc_component_driver rt1015p_component_driver = {
 	.non_legacy_dai_naming	= 1,
 };
 
-static const struct snd_soc_dai_ops rt1015p_dai_ops = {
-	.trigger        = rt1015p_daiops_trigger,
-};
-
 static struct snd_soc_dai_driver rt1015p_dai_driver = {
 	.name = "HiFi",
 	.playback = {
@@ -104,7 +80,6 @@ static struct snd_soc_dai_driver rt1015p_dai_driver = {
 		.channels_min	= 1,
 		.channels_max	= 2,
 	},
-	.ops    = &rt1015p_dai_ops,
 };
 
 static int rt1015p_platform_probe(struct platform_device *pdev)
-- 
2.29.2.576.ga3fc446d84-goog

[PATCH v2 2/2] ASoC: rt1015p: delay 300ms after SDB pulling high for calibration

[Tzung-Bi Shih]

RT1015p needs 300ms delay after SDB pulling high for internal
calibration during the power on sequence.

Delays 300ms right before data sends out to avoid data truncated.

Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
 sound/soc/codecs/rt1015p.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/soc/codecs/rt1015p.c b/sound/soc/codecs/rt1015p.c
index 397083ee5b37..bba638e1eb6a 100644
--- a/sound/soc/codecs/rt1015p.c
+++ b/sound/soc/codecs/rt1015p.c
@@ -4,6 +4,7 @@
 //
 // Copyright 2020 The Linux Foundation. All rights reserved.
 
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
@@ -19,6 +20,7 @@
 
 struct rt1015p_priv {
 	struct gpio_desc *sdb;
+	bool calib_done;
 };
 
 static int rt1015p_sdb_event(struct snd_soc_dapm_widget *w,
@@ -36,6 +38,11 @@ static int rt1015p_sdb_event(struct snd_soc_dapm_widget *w,
 	case SND_SOC_DAPM_PRE_PMU:
 		gpiod_set_value(rt1015p->sdb, 1);
 		dev_dbg(component->dev, "set sdb to 1");
+
+		if (!rt1015p->calib_done) {
+			msleep(300);
+			rt1015p->calib_done = true;
+		}
 		break;
 	case SND_SOC_DAPM_POST_PMD:
 		gpiod_set_value(rt1015p->sdb, 0);
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH v2 1/2] ASoC: rt1015p: move SDB control from trigger to DAPM

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

On Thu, Dec 10, 2020 at 11:36:16AM +0800, Tzung-Bi Shih wrote:

> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		gpiod_set_value(rt1015p->sdb, 1);
> +		dev_dbg(component->dev, "set sdb to 1");

Now this is in DAPM it's not in atomic context so it'd be more friendly
to use gpiod_set_value_cansleep() so that it'll work even if the GPIO
isn't atomic safe.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] ASoC: rt1015p: delay 300ms after SDB pulling high for calibration

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 270 bytes --]

On Thu, Dec 10, 2020 at 11:36:17AM +0800, Tzung-Bi Shih wrote:

> +		if (!rt1015p->calib_done) {
> +			msleep(300);
> +			rt1015p->calib_done = true;
> +		}

Might we need to reset calib_done over suspend?  If the device looses
power I guess it forgets the calibration.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] ASoC: rt1015p: move SDB control from trigger to DAPM

[Tzung-Bi Shih]

On Thu, Dec 10, 2020 at 11:40 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Dec 10, 2020 at 11:36:16AM +0800, Tzung-Bi Shih wrote:
>
> > +     switch (event) {
> > +     case SND_SOC_DAPM_PRE_PMU:
> > +             gpiod_set_value(rt1015p->sdb, 1);
> > +             dev_dbg(component->dev, "set sdb to 1");
>
> Now this is in DAPM it's not in atomic context so it'd be more friendly
> to use gpiod_set_value_cansleep() so that it'll work even if the GPIO
> isn't atomic safe.

Strange, I thought I should get a warning message because of the
following statement in gpiod_set_value (but it didn't):
WARN_ON(desc->gdev->chip->can_sleep);

Also I dumped the value from gpiod_cansleep(rt1015p->sdb), it returned 0.

Is it safe to call gpiod_set_value_cansleep() even if the GPIO
controller doesn't support it?  (I guess yes, _cansleep could be just
an advice)

Re: [PATCH v2 2/2] ASoC: rt1015p: delay 300ms after SDB pulling high for calibration

[Tzung-Bi Shih]

On Thu, Dec 10, 2020 at 11:42 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Dec 10, 2020 at 11:36:17AM +0800, Tzung-Bi Shih wrote:
>
> > +             if (!rt1015p->calib_done) {
> > +                     msleep(300);
> > +                     rt1015p->calib_done = true;
> > +             }
>
> Might we need to reset calib_done over suspend?  If the device looses
> power I guess it forgets the calibration.

In our environment, the power VBAT and AVDD are still on even if the
system suspends.  In more low power mode, AVDD could probably lose.

But agree with you, in general, suppose the device needs to calibrate
again after suspend/resume is better.

Re: [PATCH v2 1/2] ASoC: rt1015p: move SDB control from trigger to DAPM

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

On Fri, Dec 11, 2020 at 12:34:52AM +0800, Tzung-Bi Shih wrote:
> On Thu, Dec 10, 2020 at 11:40 PM Mark Brown <broonie@kernel.org> wrote:

> > Now this is in DAPM it's not in atomic context so it'd be more friendly
> > to use gpiod_set_value_cansleep() so that it'll work even if the GPIO
> > isn't atomic safe.

> Strange, I thought I should get a warning message because of the
> following statement in gpiod_set_value (but it didn't):
> WARN_ON(desc->gdev->chip->can_sleep);

> Also I dumped the value from gpiod_cansleep(rt1015p->sdb), it returned 0.

> Is it safe to call gpiod_set_value_cansleep() even if the GPIO
> controller doesn't support it?  (I guess yes, _cansleep could be just
> an advice)

Yes, absolutely - it's the other way around.  _cansleep() is the caller
saying that they support using a GPIO that might sleep when updating
(eg, one connected over I2C), it works with both GPIOs that might sleep
and GPIOs that are atomic safe.  Without _cansleep() only atomic safe
GPIO controllers can be used.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] ASoC: rt1015p: delay 300ms after SDB pulling high for calibration

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

On Fri, Dec 11, 2020 at 12:47:47AM +0800, Tzung-Bi Shih wrote:
> On Thu, Dec 10, 2020 at 11:42 PM Mark Brown <broonie@kernel.org> wrote:

> > Might we need to reset calib_done over suspend?  If the device looses
> > power I guess it forgets the calibration.

> In our environment, the power VBAT and AVDD are still on even if the
> system suspends.  In more low power mode, AVDD could probably lose.

> But agree with you, in general, suppose the device needs to calibrate
> again after suspend/resume is better.

Yeah, even on that system there might be a problem if the system goes
into hibernate instead of suspend (especially if it hibernates due to
running out of battery!).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]