ASoC: adau1761: Reset codec at probe and shutdown

Commit Message

Niklas Carlsson Aug. 5, 2022, 10:21 p.m. UTC

From: Niklas Carlsson <[e-mail]>

ADAU1X61 doesn't have a reset pin, which leads to the codec retaining
the register values after the device has been rebooted without a power
cycle. This causes the driver to be out of sync with the codec.

Simulating a hardware reset ensures that the codec is in a defined
state.

Link: https://lore.kernel.org/all/[e-mail]/
Link: https://lore.kernel.org/all/[e-mail]/
Signed-off-by: Niklas Carlsson <[e-mail]>
---
 sound/soc/codecs/adau1761-i2c.c |  6 +++++
 sound/soc/codecs/adau1761-spi.c |  6 +++++
 sound/soc/codecs/adau1761.c     | 43 +++++++++++++++++++++++++++++++++
 sound/soc/codecs/adau1761.h     |  1 +
 4 files changed, 56 insertions(+)

Comments

Mark Brown Aug. 8, 2022, 11:54 a.m. UTC | #1

On Sat, Aug 06, 2022 at 12:21:20AM +0200, Niklas Carlsson wrote:

> +	 * Steps for performing the reset:
> +	 *   1) Make sure that the cache is marked as dirty by writing all
> +	 *      default values directly to the cache.

Why?  If there's some need to mark the cache as dirty there's a function
that directly does that.  Note especially that a cache sync will
explicitly not write any default values to the hardware if it knows
about them.

> +	 *   2) Enable the core clock which is needed for writing all registers
> +	 *      except CLOCK_CONTROL.
> +	 *
> +	 *   3) Use regcache_sync() for synchronizing the dirty cache back to
> +	 *      the hardware.

We then need to disable clock control at the end (which the code does
but the comment doesn't).  It might be better to just have comments next
to the individual steps here.

I'd expect something more like

	enable clock
	for each register except CLOCK_CONTROL write the default
	disable clock

here, no faffing with the cache.  You could use two bulk writes to do
the writes of registers below and above CLOCK_CONTROL if that's the goal
in doing a cache sync.

Patch

diff --git a/sound/soc/codecs/adau1761-i2c.c b/sound/soc/codecs/adau1761-i2c.c
index 0683caf86aea..23bffbd74469 100644
--- a/sound/soc/codecs/adau1761-i2c.c
+++ b/sound/soc/codecs/adau1761-i2c.c
@@ -36,6 +36,11 @@  static int adau1761_i2c_remove(struct i2c_client *client)
 	return 0;
 }
 
+static void adau1761_i2c_shutdown(struct i2c_client *client)
+{
+	adau1761_shutdown(&client->dev);
+}
+
 static const struct i2c_device_id adau1761_i2c_ids[] = {
 	{ "adau1361", ADAU1361 },
 	{ "adau1461", ADAU1761 },
@@ -63,6 +68,7 @@  static struct i2c_driver adau1761_i2c_driver = {
 	},
 	.probe_new = adau1761_i2c_probe,
 	.remove = adau1761_i2c_remove,
+	.shutdown = adau1761_i2c_shutdown,
 	.id_table = adau1761_i2c_ids,
 };
 module_i2c_driver(adau1761_i2c_driver);
diff --git a/sound/soc/codecs/adau1761-spi.c b/sound/soc/codecs/adau1761-spi.c
index 7c9242c2ff94..5bbbc3d3b5be 100644
--- a/sound/soc/codecs/adau1761-spi.c
+++ b/sound/soc/codecs/adau1761-spi.c
@@ -50,6 +50,11 @@  static void adau1761_spi_remove(struct spi_device *spi)
 	adau17x1_remove(&spi->dev);
 }
 
+static void adau1761_spi_shutdown(struct spi_device *spi)
+{
+	adau1761_shutdown(&spi->dev);
+}
+
 static const struct spi_device_id adau1761_spi_id[] = {
 	{ "adau1361", ADAU1361 },
 	{ "adau1461", ADAU1761 },
@@ -77,6 +82,7 @@  static struct spi_driver adau1761_spi_driver = {
 	},
 	.probe = adau1761_spi_probe,
 	.remove = adau1761_spi_remove,
+	.shutdown = adau1761_spi_shutdown,
 	.id_table = adau1761_spi_id,
 };
 module_spi_driver(adau1761_spi_driver);
diff --git a/sound/soc/codecs/adau1761.c b/sound/soc/codecs/adau1761.c
index 8f887227981f..ac544e1e8dc7 100644
--- a/sound/soc/codecs/adau1761.c
+++ b/sound/soc/codecs/adau1761.c
@@ -974,6 +974,39 @@  static struct snd_soc_dai_driver adau1761_dai_driver = {
 	.ops = &adau17x1_dai_ops,
 };
 
+static void adau1761_reset(struct regmap *regmap)
+{
+	size_t i;
+
+	/*
+	 * Handle the lack of a reset pin in ADAU1X61 by manually writing all
+	 * registers to their default values.
+	 *
+	 * Steps for performing the reset:
+	 *   1) Make sure that the cache is marked as dirty by writing all
+	 *      default values directly to the cache.
+	 *
+	 *   2) Enable the core clock which is needed for writing all registers
+	 *      except CLOCK_CONTROL.
+	 *
+	 *   3) Use regcache_sync() for synchronizing the dirty cache back to
+	 *      the hardware.
+	 */
+
+	regcache_cache_only(regmap, true);
+	for (i = 0; i < ARRAY_SIZE(adau1761_reg_defaults); i++)
+		regmap_write(regmap, adau1761_reg_defaults[i].reg,
+			     adau1761_reg_defaults[i].def);
+	regcache_cache_only(regmap, false);
+
+	regmap_update_bits(regmap, ADAU17X1_CLOCK_CONTROL,
+			   ADAU17X1_CLOCK_CONTROL_SYSCLK_EN,
+			   ADAU17X1_CLOCK_CONTROL_SYSCLK_EN);
+	regcache_sync(regmap);
+	regmap_update_bits(regmap, ADAU17X1_CLOCK_CONTROL,
+			   ADAU17X1_CLOCK_CONTROL_SYSCLK_EN, 0);
+}
+
 int adau1761_probe(struct device *dev, struct regmap *regmap,
 	enum adau17x1_type type, void (*switch_mode)(struct device *dev))
 {
@@ -997,6 +1030,8 @@  int adau1761_probe(struct device *dev, struct regmap *regmap,
 	if (ret)
 		return ret;
 
+	adau1761_reset(regmap);
+
 	/* Enable cache only mode as we could miss writes before bias level
 	 * reaches standby and the core clock is enabled */
 	regcache_cache_only(regmap, true);
@@ -1006,6 +1041,14 @@  int adau1761_probe(struct device *dev, struct regmap *regmap,
 }
 EXPORT_SYMBOL_GPL(adau1761_probe);
 
+void adau1761_shutdown(struct device *dev)
+{
+	struct adau *adau = dev_get_drvdata(dev);
+
+	adau1761_reset(adau->regmap);
+}
+EXPORT_SYMBOL_GPL(adau1761_shutdown);
+
 const struct regmap_config adau1761_regmap_config = {
 	.val_bits = 8,
 	.reg_bits = 16,
diff --git a/sound/soc/codecs/adau1761.h b/sound/soc/codecs/adau1761.h
index 7beabf448ad1..98d698ebde27 100644
--- a/sound/soc/codecs/adau1761.h
+++ b/sound/soc/codecs/adau1761.h
@@ -16,6 +16,7 @@  struct device;
 
 int adau1761_probe(struct device *dev, struct regmap *regmap,
 	enum adau17x1_type type, void (*switch_mode)(struct device *dev));
+void adau1761_shutdown(struct device *dev);
 
 extern const struct regmap_config adau1761_regmap_config;