[RFC PATCH] ASoC: ak4458: use reset control instead of reset gpio
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);
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.
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
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?
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
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.
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
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/dt... https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dt... https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dt...
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
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/dt... https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dt... https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dt...
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
participants (4)
-
Mark Brown -
Peter Ujfalusi -
Viorel Suman -
Viorel Suman (OSS)