| Message ID | 20250218-apple-codec-changes-v2-20-932760fd7e07@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: tas27{64,70}: improve support for Apple codec variants | expand |
On Tue, Feb 18, 2025 at 06:35:54PM +1000, James Calligeros wrote: > Multiple amps can be connected to the same SDZ GPIO. Using raw GPIOs for > this breaks, as there is no concept of refcounting/sharing. In order to > model these platforms, introduce support for an SDZ "regulator". This > allows us to represent the SDZ GPIO as a simple regulator-fixed, and > then the regulator core takes care of refcounting so that all codecs are > only powered down once all the driver instances are in the suspend > state. I get that the reference counting that the regulator API does is useful here but this isn't a regulator so shouldn't be exposed as such, particularly since this winds up being visible in the DT ABI. I could've sworn that someone did some helpers for this case but now I go looking I can't find them, we certainly don't use any in the regulator core. > @@ -210,7 +220,7 @@ static const struct snd_soc_dapm_widget tas2764_dapm_widgets[] = { > SND_SOC_DAPM_DAC("DAC", NULL, SND_SOC_NOPM, 0, 0), > SND_SOC_DAPM_OUTPUT("OUT"), > SND_SOC_DAPM_SIGGEN("VMON"), > - SND_SOC_DAPM_SIGGEN("IMON") > + SND_SOC_DAPM_SIGGEN("IMON"), > }; > Spurious unrelated change.
Hi Mark, On Wed, Feb 19, 2025 at 1:33 AM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Feb 18, 2025 at 06:35:54PM +1000, James Calligeros wrote: > > > Multiple amps can be connected to the same SDZ GPIO. Using raw GPIOs for > > this breaks, as there is no concept of refcounting/sharing. In order to > > model these platforms, introduce support for an SDZ "regulator". This > > allows us to represent the SDZ GPIO as a simple regulator-fixed, and > > then the regulator core takes care of refcounting so that all codecs are > > only powered down once all the driver instances are in the suspend > > state. > > I get that the reference counting that the regulator API does is useful > here but this isn't a regulator so shouldn't be exposed as such, > particularly since this winds up being visible in the DT ABI. I > could've sworn that someone did some helpers for this case but now I go > looking I can't find them, we certainly don't use any in the regulator > core. From what I recall, no attempt at shared GPIO infrastructure has actually landed. The multiple {de}assertions of SDZ put each chip on the same line into an unusable state that requires a full power cycle to clear, so we can't live without handling the shared GPIO somewhat sensibly. One alternative off the top of my head is adding a dummy reset controller to the DTs and integrating it into the ASoC machine driver (which we have downstream). We could then put the GPIO behind a shared reset line, and hit that instead of the GPIO. This does seem a little complex/odd, and IIRC we considered this at some point and decided against it. Is there any other option that may work here? I'm open to ideas. Regards, James
On Wed, Feb 19, 2025 at 02:47:04PM +1000, James Calligeros wrote: > On Wed, Feb 19, 2025 at 1:33 AM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Feb 18, 2025 at 06:35:54PM +1000, James Calligeros wrote: > > I get that the reference counting that the regulator API does is useful > > here but this isn't a regulator so shouldn't be exposed as such, > > particularly since this winds up being visible in the DT ABI. I > > could've sworn that someone did some helpers for this case but now I go > > looking I can't find them, we certainly don't use any in the regulator > > core. > From what I recall, no attempt at shared GPIO infrastructure has actually > landed. The multiple {de}assertions of SDZ put each chip on the same line Yeah, I can't find anything. Perhaps I was thinking of the reset API, most of the other users were reset lines so it's plausible someone started and then just ended up with the reset API instead. > into an unusable state that requires a full power cycle to clear, so > we can't live without > handling the shared GPIO somewhat sensibly. > One alternative off the top of my head is adding a dummy reset controller > to the DTs and integrating it into the ASoC machine driver (which we have > downstream). We could then put the GPIO behind a shared reset line, and hit > that instead of the GPIO. This does seem a little complex/odd, and IIRC we > considered this at some point and decided against it. I'm not sure that's particularly better than the regulator version TBH, it's still got the problem of showing up in the device ABI. > Is there any other option that may work here? I'm open to ideas. Perhaps it's time to bite the bullet and do the shared GPIO API? regulator could certainly use it (and has a bunch of code, we could probably just pull that out and wrap an API around it?) and now there's this too. You could possibly also open code, but that does beg the question about the shared API.
diff --git a/sound/soc/codecs/tas2764.c b/sound/soc/codecs/tas2764.c index 7b69ab94c4bbd5f074d57a42f71b32f5fd63d560..5d89d47c1667c1067f88169575b7b76e9a25bda4 100644 --- a/sound/soc/codecs/tas2764.c +++ b/sound/soc/codecs/tas2764.c @@ -35,6 +35,7 @@ struct tas2764_priv { struct snd_soc_component *component; struct gpio_desc *reset_gpio; struct gpio_desc *sdz_gpio; + struct regulator *sdz_reg; struct regmap *regmap; struct device *dev; int irq; @@ -154,6 +155,8 @@ static int tas2764_codec_suspend(struct snd_soc_component *component) if (tas2764->sdz_gpio) gpiod_set_value_cansleep(tas2764->sdz_gpio, 0); + regulator_disable(tas2764->sdz_reg); + regcache_cache_only(tas2764->regmap, true); regcache_mark_dirty(tas2764->regmap); @@ -165,19 +168,26 @@ static int tas2764_codec_resume(struct snd_soc_component *component) struct tas2764_priv *tas2764 = snd_soc_component_get_drvdata(component); int ret; + ret = regulator_enable(tas2764->sdz_reg); + + if (ret) { + dev_err(tas2764->dev, "Failed to enable regulator\n"); + return ret; + } + if (tas2764->sdz_gpio) { gpiod_set_value_cansleep(tas2764->sdz_gpio, 1); - usleep_range(1000, 2000); } - ret = tas2764_update_pwr_ctrl(tas2764); + usleep_range(1000, 2000); + regcache_cache_only(tas2764->regmap, false); + + ret = regcache_sync(tas2764->regmap); if (ret < 0) return ret; - regcache_cache_only(tas2764->regmap, false); - - return regcache_sync(tas2764->regmap); + return tas2764_update_pwr_ctrl(tas2764); } #else #define tas2764_codec_suspend NULL @@ -210,7 +220,7 @@ static const struct snd_soc_dapm_widget tas2764_dapm_widgets[] = { SND_SOC_DAPM_DAC("DAC", NULL, SND_SOC_NOPM, 0, 0), SND_SOC_DAPM_OUTPUT("OUT"), SND_SOC_DAPM_SIGGEN("VMON"), - SND_SOC_DAPM_SIGGEN("IMON") + SND_SOC_DAPM_SIGGEN("IMON"), }; static const struct snd_soc_dapm_route tas2764_audio_map[] = { @@ -698,11 +708,18 @@ static int tas2764_codec_probe(struct snd_soc_component *component) tas2764->component = component; + ret = regulator_enable(tas2764->sdz_reg); + if (ret != 0) { + dev_err(tas2764->dev, "Failed to enable regulator: %d\n", ret); + return ret; + } + if (tas2764->sdz_gpio) { gpiod_set_value_cansleep(tas2764->sdz_gpio, 1); - usleep_range(1000, 2000); } + usleep_range(1000, 2000); + tas2764_reset(tas2764); regmap_reinit_cache(tas2764->regmap, &tas2764_i2c_regmap); @@ -777,6 +794,9 @@ static int tas2764_codec_probe(struct snd_soc_component *component) static void tas2764_codec_remove(struct snd_soc_component *component) { + struct tas2764_priv *tas2764 = snd_soc_component_get_drvdata(component); + + regulator_disable(tas2764->sdz_reg); sysfs_remove_groups(&component->dev->kobj, tas2764_sysfs_groups); } @@ -879,6 +899,11 @@ static int tas2764_parse_dt(struct device *dev, struct tas2764_priv *tas2764) { int ret = 0; + tas2764->sdz_reg = devm_regulator_get(dev, "SDZ"); + if (IS_ERR(tas2764->sdz_reg)) + return dev_err_probe(dev, PTR_ERR(tas2764->sdz_reg), + "Failed to get SDZ supply\n"); + tas2764->reset_gpio = devm_gpiod_get_optional(tas2764->dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(tas2764->reset_gpio)) {