| Message ID | 20201116222036.343635-1-viorel.suman@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] ASoC: ak4458: use reset control instead of reset gpio | expand |
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? 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
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
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);