[RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio

[RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio

[Viorel Suman (OSS)]

From: Viorel Suman <viorel.suman@nxp.com>

Using GPIO API seems not a way to go for the case
when the same reset GPIO is used to control several codecs.
For a such case we can use the "gpio-reset" driver
and the "shared" reset API to manage the reset GPIO -
to deassert the reset when the first codec is powered up
and assert it when there is no codec in use.
DTS is supposed to look as follows:

/ {
    ak4458_reset: gpio-reset {
       compatible = "gpio-reset";
       reset-gpios = <&pca6416 4 GPIO_ACTIVE_LOW>;
       #reset-cells = <0>;
       initially-in-reset;
    };
};

&i2c3 {
    pca6416: gpio@20 {
       compatible = "ti,tca6416";
       reg = <0x20>;
       gpio-controller;
       #gpio-cells = <2>;
    };

    ak4458_1: ak4458@10 {
       compatible = "asahi-kasei,ak4458";
       reg = <0x10>;
       resets = <&ak4458_reset>;
    };

    ak4458_2: ak4458@11 {
       compatible = "asahi-kasei,ak4458";
       reg = <0x11>;
       resets = <&ak4458_reset>;
    };

    ak4458_3: ak4458@12 {
       compatible = "asahi-kasei,ak4458";
       reg = <0x12>;
       resets = <&ak4458_reset>;
    };
};

Signed-off-by: Viorel Suman <viorel.suman@nxp.com>
---
 sound/soc/codecs/ak4458.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/sound/soc/codecs/ak4458.c b/sound/soc/codecs/ak4458.c
index 1010c9ee2e83..f27727cb1382 100644
--- a/sound/soc/codecs/ak4458.c
+++ b/sound/soc/codecs/ak4458.c
@@ -13,6 +13,7 @@
 #include <linux/of_gpio.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <sound/initval.h>
 #include <sound/pcm_params.h>
@@ -45,7 +46,7 @@ struct ak4458_priv {
 	const struct ak4458_drvdata *drvdata;
 	struct device *dev;
 	struct regmap *regmap;
-	struct gpio_desc *reset_gpiod;
+	struct reset_control *reset;
 	struct gpio_desc *mute_gpiod;
 	int digfil;	/* SSLOW, SD, SLOW bits */
 	int fs;		/* sampling rate */
@@ -597,17 +598,17 @@ static struct snd_soc_dai_driver ak4497_dai = {
 
 static void ak4458_power_off(struct ak4458_priv *ak4458)
 {
-	if (ak4458->reset_gpiod) {
-		gpiod_set_value_cansleep(ak4458->reset_gpiod, 0);
-		usleep_range(1000, 2000);
+	if (ak4458->reset) {
+		reset_control_assert(ak4458->reset);
+		msleep(20);
 	}
 }
 
 static void ak4458_power_on(struct ak4458_priv *ak4458)
 {
-	if (ak4458->reset_gpiod) {
-		gpiod_set_value_cansleep(ak4458->reset_gpiod, 1);
-		usleep_range(1000, 2000);
+	if (ak4458->reset) {
+		reset_control_deassert(ak4458->reset);
+		msleep(20);
 	}
 }
 
@@ -685,7 +686,6 @@ static int __maybe_unused ak4458_runtime_resume(struct device *dev)
 	if (ak4458->mute_gpiod)
 		gpiod_set_value_cansleep(ak4458->mute_gpiod, 1);
 
-	ak4458_power_off(ak4458);
 	ak4458_power_on(ak4458);
 
 	regcache_cache_only(ak4458->regmap, false);
@@ -771,10 +771,9 @@ static int ak4458_i2c_probe(struct i2c_client *i2c)
 
 	ak4458->drvdata = of_device_get_match_data(&i2c->dev);
 
-	ak4458->reset_gpiod = devm_gpiod_get_optional(ak4458->dev, "reset",
-						      GPIOD_OUT_LOW);
-	if (IS_ERR(ak4458->reset_gpiod))
-		return PTR_ERR(ak4458->reset_gpiod);
+	ak4458->reset = devm_reset_control_get_optional_shared(ak4458->dev, NULL);
+	if (IS_ERR(ak4458->reset))
+		return PTR_ERR(ak4458->reset);
 
 	ak4458->mute_gpiod = devm_gpiod_get_optional(ak4458->dev, "mute",
 						     GPIOD_OUT_LOW);
-- 
2.26.2

Re: [RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio

[Mark Brown]

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

On Tue, Nov 17, 2020 at 12:20:36AM +0200, Viorel Suman (OSS) wrote:

>  static void ak4458_power_off(struct ak4458_priv *ak4458)
>  {
> -	if (ak4458->reset_gpiod) {
> -		gpiod_set_value_cansleep(ak4458->reset_gpiod, 0);
> -		usleep_range(1000, 2000);
> +	if (ak4458->reset) {
> +		reset_control_assert(ak4458->reset);
> +		msleep(20);

We should really leave the support for doing this via GPIO in place for
backwards compatibility I think, we could mark it as deprecated in the
binding document.  Otherwise this makes sense to me and solves a real
problem we have with the handling of resets so we should look into doing
this for new bindings.

One thing I'm not clear on is if there's some way to ensure that we
don't have different instances of the device resetting each other
without them noticing?  Shouldn't be an issue in practice for the use
here.

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

RE: [RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio

[Viorel Suman]

> On Tue, Nov 17, 2020 at 12:20:36AM +0200, Viorel Suman (OSS) wrote:
> 
> >  static void ak4458_power_off(struct ak4458_priv *ak4458)  {
> > -	if (ak4458->reset_gpiod) {
> > -		gpiod_set_value_cansleep(ak4458->reset_gpiod, 0);
> > -		usleep_range(1000, 2000);
> > +	if (ak4458->reset) {
> > +		reset_control_assert(ak4458->reset);
> > +		msleep(20);
> 
> We should really leave the support for doing this via GPIO in place for backwards
> compatibility I think, we could mark it as deprecated in the binding document.
> Otherwise this makes sense to me and solves a real problem we have with the
> handling of resets so we should look into doing this for new bindings.
> 
> One thing I'm not clear on is if there's some way to ensure that we don't have
> different instances of the device resetting each other without them noticing?
> Shouldn't be an issue in practice for the use here.

The way to ensure that we don't have different instances of the device resetting each
other is to rely on the way the "shared" reset is handled by reset API: 
==========
+	ak4458->reset = devm_reset_control_get_optional_shared(ak4458->dev, NULL);
+	if (IS_ERR(ak4458->reset))
+		return PTR_ERR(ak4458->reset);
==========

/Viorel

Re: [RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio

[Mark Brown]

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

On Tue, Nov 17, 2020 at 06:17:36PM +0000, Viorel Suman wrote:
> > On Tue, Nov 17, 2020 at 12:20:36AM +0200, Viorel Suman (OSS) wrote:

> > One thing I'm not clear on is if there's some way to ensure that we don't have
> > different instances of the device resetting each other without them noticing?
> > Shouldn't be an issue in practice for the use here.

> The way to ensure that we don't have different instances of the device resetting each
> other is to rely on the way the "shared" reset is handled by reset API: 
> ==========
> +	ak4458->reset = devm_reset_control_get_optional_shared(ak4458->dev, NULL);
> +	if (IS_ERR(ak4458->reset))
> +		return PTR_ERR(ak4458->reset);
> ==========

Flip side of that then, how do we know when a reset has actually
happened?

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

RE: [RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio

[Viorel Suman]

> On Tue, Nov 17, 2020 at 06:17:36PM +0000, Viorel Suman wrote:
> > > On Tue, Nov 17, 2020 at 12:20:36AM +0200, Viorel Suman (OSS) wrote:
> 
> > > One thing I'm not clear on is if there's some way to ensure that we
> > > don't have different instances of the device resetting each other without
> them noticing?
> > > Shouldn't be an issue in practice for the use here.
> 
> > The way to ensure that we don't have different instances of the device
> > resetting each other is to rely on the way the "shared" reset is handled by
> reset API:
> > ==========
> > +	ak4458->reset = devm_reset_control_get_optional_shared(ak4458-
> >dev, NULL);
> > +	if (IS_ERR(ak4458->reset))
> > +		return PTR_ERR(ak4458->reset);
> > ==========
> 
> Flip side of that then, how do we know when a reset has actually happened?

I don't see how this can be achieved - I'd imagine some "shared" reset
framework notification mechanism calling back all "listeners" in the moment
the assert/deassert actually happened, there is no such mechanism currently
implemented.

In this specific case the GPIO purpose is to just to power on/off all codecs.
In my view with this approach it's enough to know that all codecs will be
powered on the first _deassert_ call and will be powered off on the last
_assert_ call.

/Viorel 

Re: [RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio

[Mark Brown]

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

On Thu, Nov 19, 2020 at 04:22:42PM +0000, Viorel Suman wrote:

> > Flip side of that then, how do we know when a reset has actually happened?

> I don't see how this can be achieved - I'd imagine some "shared" reset
> framework notification mechanism calling back all "listeners" in the moment
> the assert/deassert actually happened, there is no such mechanism currently
> implemented.

Yes, I'd expect some notification via callback or sometihng.

> In this specific case the GPIO purpose is to just to power on/off all codecs.
> In my view with this approach it's enough to know that all codecs will be
> powered on the first _deassert_ call and will be powered off on the last
> _assert_ call.

In general it can be useful to know if the device was actually reset
since then you can skip any reinitialization you might need to do due to
that in cases where the reset didn't actually end up happening.  Not a
blocker but it would be useful.

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

Re: [RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio

[Peter Ujfalusi]

Hi,

On 17/11/2020 0.20, Viorel Suman (OSS) wrote:
> From: Viorel Suman <viorel.suman@nxp.com>
> 
> Using GPIO API seems not a way to go for the case
> when the same reset GPIO is used to control several codecs.
> For a such case we can use the "gpio-reset" driver
> and the "shared" reset API to manage the reset GPIO -
> to deassert the reset when the first codec is powered up
> and assert it when there is no codec in use.
> DTS is supposed to look as follows:
> 
> / {
>     ak4458_reset: gpio-reset {
>        compatible = "gpio-reset";
>        reset-gpios = <&pca6416 4 GPIO_ACTIVE_LOW>;
>        #reset-cells = <0>;
>        initially-in-reset;

I can not find anything resembling to this in next-20201119.

Where is the implementation and documentation for this gpio-reset?

>     };
> };
> 
> &i2c3 {
>     pca6416: gpio@20 {
>        compatible = "ti,tca6416";
>        reg = <0x20>;
>        gpio-controller;
>        #gpio-cells = <2>;
>     };
> 
>     ak4458_1: ak4458@10 {
>        compatible = "asahi-kasei,ak4458";
>        reg = <0x10>;
>        resets = <&ak4458_reset>;
>     };
> 
>     ak4458_2: ak4458@11 {
>        compatible = "asahi-kasei,ak4458";
>        reg = <0x11>;
>        resets = <&ak4458_reset>;
>     };
> 
>     ak4458_3: ak4458@12 {
>        compatible = "asahi-kasei,ak4458";
>        reg = <0x12>;
>        resets = <&ak4458_reset>;
>     };
> };
> 
> Signed-off-by: Viorel Suman <viorel.suman@nxp.com>
> ---
>  sound/soc/codecs/ak4458.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/codecs/ak4458.c b/sound/soc/codecs/ak4458.c
> index 1010c9ee2e83..f27727cb1382 100644
> --- a/sound/soc/codecs/ak4458.c
> +++ b/sound/soc/codecs/ak4458.c
> @@ -13,6 +13,7 @@
>  #include <linux/of_gpio.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <sound/initval.h>
>  #include <sound/pcm_params.h>
> @@ -45,7 +46,7 @@ struct ak4458_priv {
>  	const struct ak4458_drvdata *drvdata;
>  	struct device *dev;
>  	struct regmap *regmap;
> -	struct gpio_desc *reset_gpiod;
> +	struct reset_control *reset;
>  	struct gpio_desc *mute_gpiod;
>  	int digfil;	/* SSLOW, SD, SLOW bits */
>  	int fs;		/* sampling rate */
> @@ -597,17 +598,17 @@ static struct snd_soc_dai_driver ak4497_dai = {
>  
>  static void ak4458_power_off(struct ak4458_priv *ak4458)
>  {
> -	if (ak4458->reset_gpiod) {
> -		gpiod_set_value_cansleep(ak4458->reset_gpiod, 0);
> -		usleep_range(1000, 2000);
> +	if (ak4458->reset) {
> +		reset_control_assert(ak4458->reset);
> +		msleep(20);
>  	}
>  }
>  
>  static void ak4458_power_on(struct ak4458_priv *ak4458)
>  {
> -	if (ak4458->reset_gpiod) {
> -		gpiod_set_value_cansleep(ak4458->reset_gpiod, 1);
> -		usleep_range(1000, 2000);
> +	if (ak4458->reset) {
> +		reset_control_deassert(ak4458->reset);
> +		msleep(20);
>  	}
>  }
>  
> @@ -685,7 +686,6 @@ static int __maybe_unused ak4458_runtime_resume(struct device *dev)
>  	if (ak4458->mute_gpiod)
>  		gpiod_set_value_cansleep(ak4458->mute_gpiod, 1);
>  
> -	ak4458_power_off(ak4458);
>  	ak4458_power_on(ak4458);
>  
>  	regcache_cache_only(ak4458->regmap, false);
> @@ -771,10 +771,9 @@ static int ak4458_i2c_probe(struct i2c_client *i2c)
>  
>  	ak4458->drvdata = of_device_get_match_data(&i2c->dev);
>  
> -	ak4458->reset_gpiod = devm_gpiod_get_optional(ak4458->dev, "reset",
> -						      GPIOD_OUT_LOW);
> -	if (IS_ERR(ak4458->reset_gpiod))
> -		return PTR_ERR(ak4458->reset_gpiod);
> +	ak4458->reset = devm_reset_control_get_optional_shared(ak4458->dev, NULL);
> +	if (IS_ERR(ak4458->reset))
> +		return PTR_ERR(ak4458->reset);

The binding documentation must be updated and you must support the gpio
way as well.

When I had this discussion around using the reset framework for shared
enable and/or reset pins it was suggested that _if_ such a driver makes
sense then it should internally handle (by using magic strings) the
fallback and work with pre-reset binding.

>  
>  	ak4458->mute_gpiod = devm_gpiod_get_optional(ak4458->dev, "mute",
>  						     GPIOD_OUT_LOW);
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

RE: [RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio

[Viorel Suman]

Hi Peter,

> DTS is supposed to look as follows:
> >
> > / {
> >     ak4458_reset: gpio-reset {
> >        compatible = "gpio-reset";
> >        reset-gpios = <&pca6416 4 GPIO_ACTIVE_LOW>;
> >        #reset-cells = <0>;
> >        initially-in-reset;
> 
> I can not find anything resembling to this in next-20201119.
> Where is the implementation and documentation for this gpio-reset?

The board schematics is not publicly available; some info may be seen in DTS files below:
https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8mm-evk.dts?h=imx_5.4.24_2.1.0
https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8mm-ab2.dts?h=imx_5.4.24_2.1.0
https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8mp-ab2.dts?h=imx_5.4.24_2.1.0

In examples above the GPIO is handled by machine driver - wrong approach given that
it requires machine driver being probed before codec driver.

> > -	ak4458->reset_gpiod = devm_gpiod_get_optional(ak4458->dev,
> "reset",
> > -						      GPIOD_OUT_LOW);
> > -	if (IS_ERR(ak4458->reset_gpiod))
> > -		return PTR_ERR(ak4458->reset_gpiod);
> > +	ak4458->reset = devm_reset_control_get_optional_shared(ak4458-
> >dev, NULL);
> > +	if (IS_ERR(ak4458->reset))
> > +		return PTR_ERR(ak4458->reset);
> 
> The binding documentation must be updated and you must support the gpio
> way as well.

Sure, make sense.

> When I had this discussion around using the reset framework for shared
> enable and/or reset pins it was suggested that _if_ such a driver makes
> sense then it should internally handle (by using magic strings) the fallback
> and work with pre-reset binding.

Thanks, would appreciate if you point me to the discussion you had.

Viorel

Re: [RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio

[Peter Ujfalusi]

Hi Viorel,

On 19/11/2020 18.24, Viorel Suman wrote:
> Hi Peter,
> 
>> DTS is supposed to look as follows:
>>>
>>> / {
>>>     ak4458_reset: gpio-reset {
>>>        compatible = "gpio-reset";
>>>        reset-gpios = <&pca6416 4 GPIO_ACTIVE_LOW>;
>>>        #reset-cells = <0>;
>>>        initially-in-reset;
>>
>> I can not find anything resembling to this in next-20201119.
>> Where is the implementation and documentation for this gpio-reset?
> 
> The board schematics is not publicly available; some info may be seen in DTS files below:
> https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8mm-evk.dts?h=imx_5.4.24_2.1.0
> https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8mm-ab2.dts?h=imx_5.4.24_2.1.0
> https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8mp-ab2.dts?h=imx_5.4.24_2.1.0
> 
> In examples above the GPIO is handled by machine driver - wrong approach given that
> it requires machine driver being probed before codec driver.

Right, so this gpio-reset driver is not in mainline. You are adding
support for something which does not exists ;)

> 
>>> -	ak4458->reset_gpiod = devm_gpiod_get_optional(ak4458->dev,
>> "reset",
>>> -						      GPIOD_OUT_LOW);
>>> -	if (IS_ERR(ak4458->reset_gpiod))
>>> -		return PTR_ERR(ak4458->reset_gpiod);
>>> +	ak4458->reset = devm_reset_control_get_optional_shared(ak4458-
>>> dev, NULL);
>>> +	if (IS_ERR(ak4458->reset))
>>> +		return PTR_ERR(ak4458->reset);
>>
>> The binding documentation must be updated and you must support the gpio
>> way as well.
> 
> Sure, make sense.
> 
>> When I had this discussion around using the reset framework for shared
>> enable and/or reset pins it was suggested that _if_ such a driver makes
>> sense then it should internally handle (by using magic strings) the fallback
>> and work with pre-reset binding.
> 
> Thanks, would appreciate if you point me to the discussion you had.

There were few iterations of it when I finally given up, I can quickly
find rfc v2:
https://lkml.org/lkml/2019/10/30/311

Probably in earlier or later series the reset framework was also discussed.

I ended up using GPIOD_FLAGS_BIT_NONEXCLUSIVE in the pcm3168a driver.
https://lkml.org/lkml/2019/11/13/411
- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki