[PATCH] ASoC: rockchip: implement system suspend/resume for i2s

[PATCH] ASoC: rockchip: implement system suspend/resume for i2s

[Sugar Zhang]

restore hw registers after power loss during a suspend/resume cycle.

Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
---

 sound/soc/rockchip/rockchip_i2s.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index 574c6af..53970ac 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -614,9 +614,35 @@ static const struct of_device_id rockchip_i2s_match[] = {
 	{},
 };
 
+#ifdef CONFIG_PM_SLEEP
+static int rockchip_i2s_suspend(struct device *dev)
+{
+	struct rk_i2s_dev *i2s = dev_get_drvdata(dev);
+
+	regcache_mark_dirty(i2s->regmap);
+
+	return 0;
+}
+
+static int rockchip_i2s_resume(struct device *dev)
+{
+	struct rk_i2s_dev *i2s = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		return ret;
+	ret = regcache_sync(i2s->regmap);
+	pm_runtime_put(dev);
+
+	return ret;
+}
+#endif
+
 static const struct dev_pm_ops rockchip_i2s_pm_ops = {
 	SET_RUNTIME_PM_OPS(i2s_runtime_suspend, i2s_runtime_resume,
 			   NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(rockchip_i2s_suspend, rockchip_i2s_resume)
 };
 
 static struct platform_driver rockchip_i2s_driver = {
-- 
1.9.1

[PATCH] ASoC: rockchip: implement system suspend/resume for i2s

[Sugar Zhang]

restore hw registers after power loss during a suspend/resume cycle.

Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
---

 sound/soc/rockchip/rockchip_i2s.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index 574c6af..53970ac 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -614,9 +614,35 @@ static const struct of_device_id rockchip_i2s_match[] = {
 	{},
 };
 
+#ifdef CONFIG_PM_SLEEP
+static int rockchip_i2s_suspend(struct device *dev)
+{
+	struct rk_i2s_dev *i2s = dev_get_drvdata(dev);
+
+	regcache_mark_dirty(i2s->regmap);
+
+	return 0;
+}
+
+static int rockchip_i2s_resume(struct device *dev)
+{
+	struct rk_i2s_dev *i2s = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		return ret;
+	ret = regcache_sync(i2s->regmap);
+	pm_runtime_put(dev);
+
+	return ret;
+}
+#endif
+
 static const struct dev_pm_ops rockchip_i2s_pm_ops = {
 	SET_RUNTIME_PM_OPS(i2s_runtime_suspend, i2s_runtime_resume,
 			   NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(rockchip_i2s_suspend, rockchip_i2s_resume)
 };
 
 static struct platform_driver rockchip_i2s_driver = {
-- 
1.9.1

Re: [PATCH] ASoC: rockchip: implement system suspend/resume for i2s

[Doug Anderson]

Hi,

On Mon, Jul 4, 2016 at 7:45 PM, Sugar Zhang <sugar.zhang@rock-chips.com> wrote:
> restore hw registers after power loss during a suspend/resume cycle.
>
> Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> ---
>
>  sound/soc/rockchip/rockchip_i2s.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
> index 574c6af..53970ac 100644
> --- a/sound/soc/rockchip/rockchip_i2s.c
> +++ b/sound/soc/rockchip/rockchip_i2s.c
> @@ -614,9 +614,35 @@ static const struct of_device_id rockchip_i2s_match[] = {
>         {},
>  };
>
> +#ifdef CONFIG_PM_SLEEP
> +static int rockchip_i2s_suspend(struct device *dev)

Rather than #ifdef, I think that the currently suggested way to do
this is to use __maybe_unused, like:
  static __maybe_unused int rockchip_i2s_suspend(struct device *dev)

> +{
> +       struct rk_i2s_dev *i2s = dev_get_drvdata(dev);
> +
> +       regcache_mark_dirty(i2s->regmap);
> +
> +       return 0;
> +}

I do wonder a little bit if we should be doing this work in pm_runtime
instead of actually adding suspend/resume hooks.

I think that technically you end up losing state when your power
domain dies, right?  Looking at rk3399, I see that i2s is in
"pd_sdioaudio" along with "sdio, spi, i2s, spdif".  That means (I
think) that if all of those peripherals happen to runtime suspend at
the same time (or they are totally unused) then you'll lose state when
you are runtime suspended.  Then when you runtime resume you need to
restore.

Maybe in your case you never actually get into the situation where the
power domain turns off except during suspend/resume, but it seems
possible it could happen.

Am I understanding this all properly?  Maybe someone can correct me.
I'm still a bit of a PM Runtime noob.


-Doug

[PATCH] ASoC: rockchip: implement system suspend/resume for i2s

[Doug Anderson]

Hi,

On Mon, Jul 4, 2016 at 7:45 PM, Sugar Zhang <sugar.zhang@rock-chips.com> wrote:
> restore hw registers after power loss during a suspend/resume cycle.
>
> Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> ---
>
>  sound/soc/rockchip/rockchip_i2s.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
> index 574c6af..53970ac 100644
> --- a/sound/soc/rockchip/rockchip_i2s.c
> +++ b/sound/soc/rockchip/rockchip_i2s.c
> @@ -614,9 +614,35 @@ static const struct of_device_id rockchip_i2s_match[] = {
>         {},
>  };
>
> +#ifdef CONFIG_PM_SLEEP
> +static int rockchip_i2s_suspend(struct device *dev)

Rather than #ifdef, I think that the currently suggested way to do
this is to use __maybe_unused, like:
  static __maybe_unused int rockchip_i2s_suspend(struct device *dev)

> +{
> +       struct rk_i2s_dev *i2s = dev_get_drvdata(dev);
> +
> +       regcache_mark_dirty(i2s->regmap);
> +
> +       return 0;
> +}

I do wonder a little bit if we should be doing this work in pm_runtime
instead of actually adding suspend/resume hooks.

I think that technically you end up losing state when your power
domain dies, right?  Looking at rk3399, I see that i2s is in
"pd_sdioaudio" along with "sdio, spi, i2s, spdif".  That means (I
think) that if all of those peripherals happen to runtime suspend at
the same time (or they are totally unused) then you'll lose state when
you are runtime suspended.  Then when you runtime resume you need to
restore.

Maybe in your case you never actually get into the situation where the
power domain turns off except during suspend/resume, but it seems
possible it could happen.

Am I understanding this all properly?  Maybe someone can correct me.
I'm still a bit of a PM Runtime noob.


-Doug