[PATCH v2 0/2] ASoC: es8328: enable capture on the ES8388

[PATCH v2 0/2] ASoC: es8328: enable capture on the ES8388

[Bob Hui]

This series fixes capture on boards built around the ES8388.  That part
shares the driver with the ES8328, and the two differ in eleven power-on
register values and in the meaning of bit 7 of DAC Control 21, so the
driver has to know which part it is talking to before it can set either
of them up.

Changes in v2, from the review of v1:

- Reset the codec at probe time through SCP_RESET, so that the register
  cache describes a state the hardware is actually in.  SCP_RESET is not
  self-clearing, so the reference and the VMID settings are written back
  afterwards.
- Provide one set of power-on defaults per supported part, taken from the
  ES8328 datasheet Revision 1.0 and the ES8388 datasheet Revision 5.0,
  sections 6.1.1 to 6.3.30 in both cases.
- Look the variant up from the match data in the I2C and SPI glue instead
  of testing the compatible string, so the lookup also works on ACPI
  systems.  Both i2c_get_match_data() and spi_get_device_match_data()
  fall back to the legacy id table, and the I2C id table carries the
  variant as well.  The driver still has no ACPI HID table, as it never
  had one.
- Use a define for the SLRCK bit, and drop the of.h include which is no
  longer needed.
- Keep the regmap configuration indentation consistent.

The variant plumbing lives in patch 1 because the defaults have to be
chosen when the regmap is created, which happens in the bus glue before
the component is probed; patch 2 uses the same variant to decide whether
SLRCK applies.  Boards with an ES8328 keep the same register values as
before; the only change for them is the reset at probe time.

One observation from bringing this up: this silicon is not fully
readable.  Registers 0x28 and 0x29 read back as their power-on value
regardless of what is written, and bit 7 of register 0x01 is not
implemented, so writing 0xc0 reads back as 0x40.  That is one more reason
to treat the register cache as the record of what the driver programmed
rather than reading the state back.

Testing: on a QuarkPi-CA2 (RK3588S, ES8388) running v7.3, playback works
and capture goes from digital silence to working audio.  A register the
driver never writes was modified before a warm reboot and comes back at
its reset value once the driver probes, which shows that the reset and not
a power cycle restores the state.

Assisted-by: Hermes:deepseek-v4-flash
Signed-off-by: Bob Hui <bg9oxa@163.com>
---
Bob Hui (2):
      ASoC: es8328: reset the codec at probe time
      ASoC: es8328: set SLRCK for the ES8388

 sound/soc/codecs/es8328-i2c.c |  18 ++--
 sound/soc/codecs/es8328-spi.c |  11 ++-
 sound/soc/codecs/es8328.c     | 192 ++++++++++++++++++++++++++++++++++++++++--
 sound/soc/codecs/es8328.h     |  11 ++-
 4 files changed, 215 insertions(+), 17 deletions(-)
---
base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
change-id: 20261024-es8328-vol-init-6a52c17897cd

Best regards,
--  
Bob Hui <bg9oxa@163.com>

[PATCH v2 1/2] ASoC: es8328: reset the codec at probe time

[Bob Hui]

Nothing brings the codec into a known state when the driver probes: the
supplies may already be on, a warm reboot or a kexec keeps the registers
alive, and a driver rebind leaves whatever the previous instance
programmed.  Everything that reads a register back and writes it again,
from the DAI format to the mute control, then works from a value the
hardware never had.

Issue a control port reset while probing so the codec always starts from
its documented defaults.  SCP_RESET is not self-clearing, so the reference
and the VMID divider are written back afterwards, which is the documented
default value of the register.

Describe those defaults in the regmap as well, one table per supported
part, so the cache describes the codec instead of being filled in lazily
on first access.  The two parts differ in eleven registers, so one table
cannot describe both; the variant is looked up from the match data added
to the I2C and SPI glue, which also makes the driver work on ACPI
systems.

The values come from the ES8328 datasheet, Revision 1.0, sections 6.1.1
to 6.3.30, and from the ES8388 datasheet, Revision 5.0, same sections.

Tested on a QuarkPi-CA2 (RK3588S, ES8388): a register deliberately
modified before a warm reboot comes back at its reset value once the
driver probes, so it is the reset rather than a power cycle that restores
the state.

Assisted-by: Hermes:deepseek-v4-flash
Signed-off-by: Bob Hui <bg9oxa@163.com>
---
 sound/soc/codecs/es8328-i2c.c |  18 +++--
 sound/soc/codecs/es8328-spi.c |  11 ++-
 sound/soc/codecs/es8328.c     | 175 ++++++++++++++++++++++++++++++++++++++++--
 sound/soc/codecs/es8328.h     |   9 ++-
 4 files changed, 196 insertions(+), 17 deletions(-)

diff --git a/sound/soc/codecs/es8328-i2c.c b/sound/soc/codecs/es8328-i2c.c
index 36a4987e1a593..cb9392c0b8ea3 100644
--- a/sound/soc/codecs/es8328-i2c.c
+++ b/sound/soc/codecs/es8328-i2c.c
@@ -16,23 +16,29 @@
 #include "es8328.h"
 
 static const struct i2c_device_id es8328_id[] = {
-	{ .name = "es8328" },
-	{ .name = "es8388" },
+	{ .name = "es8328", .driver_data = ES8328_TYPE_ES8328 },
+	{ .name = "es8388", .driver_data = ES8328_TYPE_ES8388 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, es8328_id);
 
 static const struct of_device_id es8328_of_match[] = {
-	{ .compatible = "everest,es8328", },
-	{ .compatible = "everest,es8388", },
+	{ .compatible = "everest,es8328", .data = (void *)ES8328_TYPE_ES8328 },
+	{ .compatible = "everest,es8388", .data = (void *)ES8328_TYPE_ES8388 },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, es8328_of_match);
 
 static int es8328_i2c_probe(struct i2c_client *i2c)
 {
-	return es8328_probe(&i2c->dev,
-			devm_regmap_init_i2c(i2c, &es8328_regmap_config));
+	enum es8328_type type = (unsigned long)i2c_get_match_data(i2c);
+	const struct regmap_config *config;
+
+	config = type == ES8328_TYPE_ES8388 ? &es8388_regmap_config
+					    : &es8328_regmap_config;
+
+	return es8328_probe(&i2c->dev, devm_regmap_init_i2c(i2c, config),
+			    type);
 }
 
 static struct i2c_driver es8328_i2c_driver = {
diff --git a/sound/soc/codecs/es8328-spi.c b/sound/soc/codecs/es8328-spi.c
index 88e353ae52a10..606de21e98e1e 100644
--- a/sound/soc/codecs/es8328-spi.c
+++ b/sound/soc/codecs/es8328-spi.c
@@ -14,15 +14,20 @@
 #include "es8328.h"
 
 static const struct of_device_id es8328_of_match[] = {
-	{ .compatible = "everest,es8328", },
+	{ .compatible = "everest,es8328", .data = (void *)ES8328_TYPE_ES8328 },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, es8328_of_match);
 
 static int es8328_spi_probe(struct spi_device *spi)
 {
-	return es8328_probe(&spi->dev,
-			devm_regmap_init_spi(spi, &es8328_regmap_config));
+	enum es8328_type type = (unsigned long)spi_get_device_match_data(spi);
+	const struct regmap_config *config;
+
+	config = type == ES8328_TYPE_ES8388 ? &es8388_regmap_config
+					    : &es8328_regmap_config;
+
+	return es8328_probe(&spi->dev, devm_regmap_init_spi(spi, config), type);
 }
 
 static struct spi_driver es8328_spi_driver = {
diff --git a/sound/soc/codecs/es8328.c b/sound/soc/codecs/es8328.c
index aaa6646ad4c5e..f04e310262e79 100644
--- a/sound/soc/codecs/es8328.c
+++ b/sound/soc/codecs/es8328.c
@@ -78,6 +78,7 @@ static const char * const supply_names[ES8328_SUPPLY_NUM] = {
 struct es8328_priv {
 	struct regmap *regmap;
 	struct clk *clk;
+	enum es8328_type type;
 	int playback_fs;
 	bool deemph;
 	int mclkdiv2;
@@ -836,6 +837,30 @@ static int es8328_component_probe(struct snd_soc_component *component)
 		goto clk_fail;
 	}
 
+	/*
+	 * Put the codec into a known state: the supplies may already be on,
+	 * a warm reboot or a kexec keeps the registers alive, and a driver
+	 * rebind leaves whatever the previous instance programmed.  Without
+	 * this every read-modify-write in the driver works from a value the
+	 * hardware never had.  SCP_RESET is not self-clearing.
+	 */
+	ret = snd_soc_component_write(component, ES8328_CONTROL1,
+				      ES8328_CONTROL1_SCP_RESET);
+	if (ret) {
+		dev_err(component->dev, "unable to reset the codec\n");
+		clk_disable_unprepare(es8328->clk);
+		goto clk_fail;
+	}
+	usleep_range(1000, 5000);
+	ret = snd_soc_component_write(component, ES8328_CONTROL1,
+				      ES8328_CONTROL1_ENREF |
+				      ES8328_CONTROL1_VMIDSEL_500k);
+	if (ret) {
+		dev_err(component->dev, "unable to clear the codec reset\n");
+		clk_disable_unprepare(es8328->clk);
+		goto clk_fail;
+	}
+
 	return 0;
 
 clk_fail:
@@ -856,16 +881,150 @@ static void es8328_remove(struct snd_soc_component *component)
 			       es8328->supplies);
 }
 
+/*
+ * Power-on defaults from the ES8328 datasheet, Revision 1.0, sections
+ * 6.1.1 to 6.3.30.
+ */
+static const struct reg_default es8328_reg_defaults[] = {
+	{ ES8328_CONTROL1,	0x06 },
+	{ ES8328_CONTROL2,	0x1c },
+	{ ES8328_CHIPPOWER,	0xc3 },
+	{ ES8328_ADCPOWER,	0xfc },
+	{ ES8328_DACPOWER,	0xc0 },
+	{ ES8328_CHIPLOPOW1,	0x00 },
+	{ ES8328_CHIPLOPOW2,	0x00 },
+	{ ES8328_ANAVOLMANAG,	0x7c },
+	{ ES8328_MASTERMODE,	0x80 },
+	{ ES8328_ADCCONTROL1,	0x00 },
+	{ ES8328_ADCCONTROL2,	0x00 },
+	{ ES8328_ADCCONTROL3,	0x06 },
+	{ ES8328_ADCCONTROL4,	0x00 },
+	{ ES8328_ADCCONTROL5,	0x06 },
+	{ ES8328_ADCCONTROL6,	0x30 },
+	{ ES8328_ADCCONTROL7,	0x30 },
+	{ ES8328_ADCCONTROL8,	0xc0 },
+	{ ES8328_ADCCONTROL9,	0xc0 },
+	{ ES8328_ADCCONTROL10,	0x38 },
+	{ ES8328_ADCCONTROL11,	0xb0 },
+	{ ES8328_ADCCONTROL12,	0x32 },
+	{ ES8328_ADCCONTROL13,	0x06 },
+	{ ES8328_ADCCONTROL14,	0x00 },
+	{ ES8328_DACCONTROL1,	0x00 },
+	{ ES8328_DACCONTROL2,	0x06 },
+	{ ES8328_DACCONTROL3,	0x32 },
+	{ ES8328_DACCONTROL4,	0xc0 },
+	{ ES8328_DACCONTROL5,	0xc0 },
+	{ ES8328_DACCONTROL6,	0x08 },
+	{ ES8328_DACCONTROL7,	0x06 },
+	{ ES8328_DACCONTROL8,	0x1f },
+	{ ES8328_DACCONTROL9,	0xf7 },
+	{ ES8328_DACCONTROL10,	0xfd },
+	{ ES8328_DACCONTROL11,	0xff },
+	{ ES8328_DACCONTROL12,	0x1f },
+	{ ES8328_DACCONTROL13,	0xf7 },
+	{ ES8328_DACCONTROL14,	0xfd },
+	{ ES8328_DACCONTROL15,	0xff },
+	{ ES8328_DACCONTROL16,	0x00 },
+	{ ES8328_DACCONTROL17,	0x38 },
+	{ ES8328_DACCONTROL18,	0x38 },
+	{ ES8328_DACCONTROL19,	0x38 },
+	{ ES8328_DACCONTROL20,	0x38 },
+	{ ES8328_DACCONTROL21,	0x38 },
+	{ ES8328_DACCONTROL22,	0x38 },
+	{ ES8328_DACCONTROL23,	0x00 },
+	{ ES8328_LOUT1VOL,	0x00 },
+	{ ES8328_ROUT1VOL,	0x00 },
+	{ ES8328_LOUT2VOL,	0x00 },
+	{ ES8328_ROUT2VOL,	0x00 },
+	{ ES8328_MONOOUTVOL,	0x00 },
+	{ ES8328_DACCONTROL29,	0x00 },
+	{ ES8328_DACCONTROL30,	0x00 },
+};
+
+/*
+ * Power-on defaults from the ES8388 datasheet, Revision 5.0, sections
+ * 6.1.1 to 6.3.30.
+ */
+static const struct reg_default es8388_reg_defaults[] = {
+	{ ES8328_CONTROL1,	0x06 },
+	{ ES8328_CONTROL2,	0x5c },
+	{ ES8328_CHIPPOWER,	0xc3 },
+	{ ES8328_ADCPOWER,	0xfc },
+	{ ES8328_DACPOWER,	0xc0 },
+	{ ES8328_CHIPLOPOW1,	0x00 },
+	{ ES8328_CHIPLOPOW2,	0x00 },
+	{ ES8328_ANAVOLMANAG,	0x7c },
+	{ ES8328_MASTERMODE,	0x80 },
+	{ ES8328_ADCCONTROL1,	0x00 },
+	{ ES8328_ADCCONTROL2,	0x00 },
+	{ ES8328_ADCCONTROL3,	0x02 },
+	{ ES8328_ADCCONTROL4,	0x00 },
+	{ ES8328_ADCCONTROL5,	0x06 },
+	{ ES8328_ADCCONTROL6,	0x30 },
+	{ ES8328_ADCCONTROL7,	0x20 },
+	{ ES8328_ADCCONTROL8,	0xc0 },
+	{ ES8328_ADCCONTROL9,	0xc0 },
+	{ ES8328_ADCCONTROL10,	0x38 },
+	{ ES8328_ADCCONTROL11,	0xb0 },
+	{ ES8328_ADCCONTROL12,	0x32 },
+	{ ES8328_ADCCONTROL13,	0x06 },
+	{ ES8328_ADCCONTROL14,	0x00 },
+	{ ES8328_DACCONTROL1,	0x00 },
+	{ ES8328_DACCONTROL2,	0x06 },
+	{ ES8328_DACCONTROL3,	0x22 },
+	{ ES8328_DACCONTROL4,	0xc0 },
+	{ ES8328_DACCONTROL5,	0xc0 },
+	{ ES8328_DACCONTROL6,	0x08 },
+	{ ES8328_DACCONTROL7,	0x00 },
+	{ ES8328_DACCONTROL8,	0x1f },
+	{ ES8328_DACCONTROL9,	0xf7 },
+	{ ES8328_DACCONTROL10,	0xfd },
+	{ ES8328_DACCONTROL11,	0xff },
+	{ ES8328_DACCONTROL12,	0x1f },
+	{ ES8328_DACCONTROL13,	0xf7 },
+	{ ES8328_DACCONTROL14,	0xfd },
+	{ ES8328_DACCONTROL15,	0xff },
+	{ ES8328_DACCONTROL16,	0x00 },
+	{ ES8328_DACCONTROL17,	0x38 },
+	{ ES8328_DACCONTROL18,	0x28 },
+	{ ES8328_DACCONTROL19,	0x28 },
+	{ ES8328_DACCONTROL20,	0x38 },
+	{ ES8328_DACCONTROL21,	0x00 },
+	{ ES8328_DACCONTROL22,	0x00 },
+	{ ES8328_DACCONTROL23,	0x00 },
+	{ ES8328_LOUT1VOL,	0x00 },
+	{ ES8328_ROUT1VOL,	0x00 },
+	{ ES8328_LOUT2VOL,	0x00 },
+	{ ES8328_ROUT2VOL,	0x00 },
+	{ ES8328_MONOOUTVOL,	0x00 },
+	{ ES8328_DACCONTROL29,	0xaa },
+	{ ES8328_DACCONTROL30,	0xaa },
+};
+
 const struct regmap_config es8328_regmap_config = {
-	.reg_bits	= 8,
-	.val_bits	= 8,
-	.max_register	= ES8328_REG_MAX,
-	.cache_type	= REGCACHE_MAPLE,
-	.use_single_read = true,
-	.use_single_write = true,
+	.reg_bits		= 8,
+	.val_bits		= 8,
+	.max_register		= ES8328_REG_MAX,
+	.cache_type		= REGCACHE_MAPLE,
+	.reg_defaults		= es8328_reg_defaults,
+	.num_reg_defaults	= ARRAY_SIZE(es8328_reg_defaults),
+	.use_single_read	= true,
+	.use_single_write	= true,
 };
 EXPORT_SYMBOL_GPL(es8328_regmap_config);
 
+const struct regmap_config es8388_regmap_config = {
+	.reg_bits		= 8,
+	.val_bits		= 8,
+	.max_register		= ES8328_REG_MAX,
+	.cache_type		= REGCACHE_MAPLE,
+	.reg_defaults		= es8388_reg_defaults,
+	.num_reg_defaults	= ARRAY_SIZE(es8388_reg_defaults),
+	.use_single_read	= true,
+	.use_single_write	= true,
+};
+EXPORT_SYMBOL_GPL(es8388_regmap_config);
+
 static const struct snd_soc_component_driver es8328_component_driver = {
 	.probe			= es8328_component_probe,
 	.remove			= es8328_remove,
@@ -884,7 +1043,8 @@ static const struct snd_soc_component_driver es8328_component_driver = {
 	.endianness		= 1,
 };
 
-int es8328_probe(struct device *dev, struct regmap *regmap)
+int es8328_probe(struct device *dev, struct regmap *regmap,
+		 enum es8328_type type)
 {
 	struct es8328_priv *es8328;
 	int ret;
@@ -898,6 +1058,7 @@ int es8328_probe(struct device *dev, struct regmap *regmap)
 		return -ENOMEM;
 
 	es8328->regmap = regmap;
+	es8328->type = type;
 
 	for (i = 0; i < ARRAY_SIZE(es8328->supplies); i++)
 		es8328->supplies[i].supply = supply_names[i];
diff --git a/sound/soc/codecs/es8328.h b/sound/soc/codecs/es8328.h
index 9109f6b5b045a..e98328b29aeff 100644
--- a/sound/soc/codecs/es8328.h
+++ b/sound/soc/codecs/es8328.h
@@ -10,8 +10,15 @@
 
 struct device;
 
+enum es8328_type {
+	ES8328_TYPE_ES8328,
+	ES8328_TYPE_ES8388,
+};
+
 extern const struct regmap_config es8328_regmap_config;
-int es8328_probe(struct device *dev, struct regmap *regmap);
+extern const struct regmap_config es8388_regmap_config;
+int es8328_probe(struct device *dev, struct regmap *regmap,
+		 enum es8328_type type);
 
 #define ES8328_DACLVOL 46
 #define ES8328_DACRVOL 47

-- 
2.34.1

[PATCH v2 2/2] ASoC: es8328: set SLRCK for the ES8388

[Bob Hui]

The ES8388 has a single LRCK pin, so the DAC and the ADC have to take
their frame clock from that one pin.  With SLRCK clear, which is the
power-on default, the capture path never gets a frame clock and records
digital silence while playback keeps working, so boards look like they
have a dead microphone.

Bit 7 of DAC Control 21 is SLRCK on the ES8388 but LD2MO, left DAC to the
mono mixer, on the ES8328, so only set it for the ES8388, using the
variant looked up from the match data.

Tested on a QuarkPi-CA2 (RK3588S, ES8388): capture goes from digital
silence to working audio.

Assisted-by: Hermes:deepseek-v4-flash
Signed-off-by: Bob Hui <bg9oxa@163.com>
---
 sound/soc/codecs/es8328.c | 17 +++++++++++++++++
 sound/soc/codecs/es8328.h |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/sound/soc/codecs/es8328.c b/sound/soc/codecs/es8328.c
index f04e310262e7..3d9a1fe1b9a0 100644
--- a/sound/soc/codecs/es8328.c
+++ b/sound/soc/codecs/es8328.c
@@ -861,8 +861,25 @@ static int es8328_component_probe(struct snd_soc_component *component)
 		goto clk_fail;
 	}
 
+	/*
+	 * The ES8388 has a single LRCK pin, so the DAC and the ADC must
+	 * share it: without SLRCK the capture path never gets a frame
+	 * clock and records digital silence while playback still works.
+	 * Bit 7 of DAC Control 21 is SLRCK on the ES8388 and LD2MO (left
+	 * DAC to the mono mixer) on the ES8328.
+	 */
+	if (es8328->type == ES8328_TYPE_ES8388) {
+		ret = snd_soc_component_write(component, ES8328_DACCONTROL21,
+					      ES8328_DACCONTROL21_SLRCK);
+		if (ret)
+			goto err_slrck;
+	}
+
 	return 0;
 
+err_slrck:
+	clk_disable_unprepare(es8328->clk);
+
 clk_fail:
 	regulator_bulk_disable(ARRAY_SIZE(es8328->supplies),
 			       es8328->supplies);
diff --git a/sound/soc/codecs/es8328.h b/sound/soc/codecs/es8328.h
index e98328b29aef..4ac49825c355 100644
--- a/sound/soc/codecs/es8328.h
+++ b/sound/soc/codecs/es8328.h
@@ -232,6 +232,8 @@ int es8328_probe(struct device *dev, struct regmap *regmap,
 #define ES8328_DACCONTROL21_LI2MOVOL (7 << 3)
 #define ES8328_DACCONTROL21_LI2MO (1 << 6)
 #define ES8328_DACCONTROL21_LD2MO (1 << 7)
+/* Bit 7 is SLRCK on the ES8388 and LD2MO on the ES8328 */
+#define ES8328_DACCONTROL21_SLRCK (1 << 7)
 
 #define ES8328_DACCONTROL22	0x2c
 #define ES8328_DACCONTROL22_RI2MOVOL (7 << 3)

-- 
2.34.1

Re: [PATCH v2 2/2] ASoC: es8328: set SLRCK for the ES8388

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ASoC: es8328: missing ES8388 match entries in es8328-spi for SLRCK setup
--

commit db663b096dbe9f68fbdc51ad2211743c7793550e
Author: Bob Hui <bg9oxa@163.com>

ASoC: es8328: set SLRCK for the ES8388

This commit sets the SLRCK bit in ES8328_DACCONTROL21 during
es8328_component_probe() when the codec variant is ES8328_TYPE_ES8388 so that
the DAC and ADC share the single LRCK pin.

> diff --git a/sound/soc/codecs/es8328.c b/sound/soc/codecs/es8328.c
> index f04e310262e79..3d9a1fe1b9a00 100644
> --- a/sound/soc/codecs/es8328.c
> +++ b/sound/soc/codecs/es8328.c
> @@ -861,8 +861,25 @@ static int es8328_component_probe(struct snd_soc_component *component)
>  		goto clk_fail;
>  	}
>  
> +	/*
> +	 * The ES8388 has a single LRCK pin, so the DAC and the ADC must
> +	 * share it: without SLRCK the capture path never gets a frame
> +	 * clock and records digital silence while playback still works.
> +	 * Bit 7 of DAC Control 21 is SLRCK on the ES8388 and LD2MO (left
> +	 * DAC to the mono mixer) on the ES8328.
> +	 */
> +	if (es8328->type == ES8328_TYPE_ES8388) {
> +		ret = snd_soc_component_write(component, ES8328_DACCONTROL21,
> +					      ES8328_DACCONTROL21_SLRCK);
> +		if (ret)
> +			goto err_slrck;
> +	}

[Severity: Medium]
Can es8328->type ever be ES8328_TYPE_ES8388 when the codec is connected over
SPI?

In sound/soc/codecs/es8328-spi.c, es8328_spi_probe() checks for
ES8328_TYPE_ES8388 and passes the resolved type to es8328_probe():

    enum es8328_type type = (unsigned long)spi_get_device_match_data(spi);
    const struct regmap_config *config;

    config = type == ES8328_TYPE_ES8388 ? &es8388_regmap_config
                                        : &es8328_regmap_config;

    return es8328_probe(&spi->dev, devm_regmap_init_spi(spi, config), type);

However, es8328_of_match in sound/soc/codecs/es8328-spi.c only lists
"everest,es8328" and es8328_spi_driver has no spi_device_id table:

    static const struct of_device_id es8328_of_match[] = {
            { .compatible = "everest,es8328", .data = (void *)ES8328_TYPE_ES8328 },
            { }
    };

When an ES8388 is connected over SPI (with devicetree compatible
"everest,es8388", "everest,es8328"), it matches the "everest,es8328" fallback
entry with ES8328_TYPE_ES8328, so es8328_component_probe() will skip setting
ES8328_DACCONTROL21_SLRCK.

Should sound/soc/codecs/es8328-spi.c also include the "everest,es8388" match
entry (and corresponding SPI device IDs) to match sound/soc/codecs/es8328-i2c.c?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-es8328-vol-init-v2-0-d15b04c22f4f@163.com?part=2

Re: [PATCH v2 1/2] ASoC: es8328: reset the codec at probe time

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ASoC: es8328: NULL pointer dereference in es8328_spi_probe() match lookup
- [Medium] ASoC: es8328: missing regcache reinit on reset in es8328_component_probe()
- [Medium] ASoC: es8328: missing everest,es8388 match entry in es8328_spi_probe()
--

commit 011e1ea21d759f33d35a0271cc926730dcdcfe1b
Author: Bob Hui <bg9oxa@163.com>

ASoC: es8328: reset the codec at probe time

This commit issues a control port reset in es8328_component_probe() so the
codec starts from its hardware defaults even after a warm reboot or driver
rebind. It also adds per-variant reg_default tables for ES8328 and ES8388 and
selects the regmap configuration using I2C and SPI match data.

> diff --git a/sound/soc/codecs/es8328-spi.c b/sound/soc/codecs/es8328-spi.c
> index 88e353ae52a10..606de21e98e1e 100644
> --- a/sound/soc/codecs/es8328-spi.c
> +++ b/sound/soc/codecs/es8328-spi.c
> @@ -14,15 +14,20 @@
>  #include "es8328.h"
>  
>  static const struct of_device_id es8328_of_match[] = {
> -	{ .compatible = "everest,es8328", },
> +	{ .compatible = "everest,es8328", .data = (void *)ES8328_TYPE_ES8328 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, es8328_of_match);
>  
>  static int es8328_spi_probe(struct spi_device *spi)
>  {
> -	return es8328_probe(&spi->dev,
> -			devm_regmap_init_spi(spi, &es8328_regmap_config));
> +	enum es8328_type type = (unsigned long)spi_get_device_match_data(spi);

[Severity: High]
Does calling spi_get_device_match_data(spi) here cause a NULL pointer
dereference when probing an SPI-connected ES8328?

In sound/soc/codecs/es8328.h, ES8328_TYPE_ES8328 is the first enumerator in
enum es8328_type and evaluates to 0, so es8328_of_match in
sound/soc/codecs/es8328-spi.c stores NULL in .data.

When es8328_spi_probe() calls spi_get_device_match_data(spi),
device_get_match_data(&sdev->dev) returns NULL and spi_get_device_match_data()
falls back to spi_get_device_id(sdev):

drivers/spi/spi.c:spi_get_device_match_data() {
	match = device_get_match_data(&sdev->dev);
	if (match)
		return match;

	id = spi_get_device_id(sdev);
}

Because es8328_spi_driver does not define an .id_table, spi_get_device_id()
passes sdrv->id_table (NULL) to spi_match_id(), which immediately dereferences
id->name[0].

> +	const struct regmap_config *config;
> +
> +	config = type == ES8328_TYPE_ES8388 ? &es8388_regmap_config
> +					    : &es8328_regmap_config;

[Severity: Medium]
Can type ever equal ES8328_TYPE_ES8388 in es8328_spi_probe()?

In sound/soc/codecs/es8328-spi.c, es8328_of_match only lists the
everest,es8328 entry with ES8328_TYPE_ES8328 and omits everest,es8388, while
es8328_spi_driver defines no spi_device_id table.

Because Documentation/devicetree/bindings/sound/everest,es8328.yaml specifies
the ES8388 compatible list as everest,es8388 followed by everest,es8328, an
SPI-connected ES8388 will match the everest,es8328 fallback entry and use
es8328_regmap_config instead of es8388_regmap_config (and also skip the
ES8388-specific ES8328_DACCONTROL21_SLRCK setup added in follow-up commit
db663b096dbe9f68fbdc51ad2211743c7793550e).

Should es8328_of_match (and an spi_device_id table) in
sound/soc/codecs/es8328-spi.c include the es8388 entry as well?

[ ... ]

> diff --git a/sound/soc/codecs/es8328.c b/sound/soc/codecs/es8328.c
> index aaa6646ad4c5e..f04e310262e79 100644
> --- a/sound/soc/codecs/es8328.c
> +++ b/sound/soc/codecs/es8328.c

[ ... ]

> @@ -836,6 +837,30 @@ static int es8328_component_probe(struct snd_soc_component *component)
>  		goto clk_fail;
>  	}
>  
> +	/*
> +	 * Put the codec into a known state: the supplies may already be on,
> +	 * a warm reboot or a kexec keeps the registers alive, and a driver
> +	 * rebind leaves whatever the previous instance programmed.  Without
> +	 * this every read-modify-write in the driver works from a value the
> +	 * hardware never had.  SCP_RESET is not self-clearing.
> +	 */
> +	ret = snd_soc_component_write(component, ES8328_CONTROL1,
> +				      ES8328_CONTROL1_SCP_RESET);
> +	if (ret) {
> +		dev_err(component->dev, "unable to reset the codec\n");
> +		clk_disable_unprepare(es8328->clk);
> +		goto clk_fail;
> +	}
> +	usleep_range(1000, 5000);
> +	ret = snd_soc_component_write(component, ES8328_CONTROL1,
> +				      ES8328_CONTROL1_ENREF |
> +				      ES8328_CONTROL1_VMIDSEL_500k);

[Severity: Medium]
Does resetting the hardware in es8328_component_probe() leave the regmap cache
out of sync if the ASoC component is rebound?

es8328->regmap and its REGCACHE_MAPLE cache are initialized once during I2C or
SPI bus probe in es8328_i2c_probe() and es8328_spi_probe(), whereas the
control port reset is performed in es8328_component_probe() when the ASoC card
binds the component.

While the card is active, registers such as ES8328_MASTERMODE (in
es8328_set_dai_fmt()), ES8328_CONTROL2, ES8328_CHIPPOWER, and mixer controls
are updated in both hardware and es8328->regmap. If the machine driver or
sound card is unbound and rebound while the I2C/SPI codec device remains
bound, or if snd_soc_bind_card() fails or defers after soc_init_pcm_runtime()
and retries later, es8328_component_probe() runs again and resets all hardware
registers to their power-on defaults without reinitializing the regmap cache.

Subsequent snd_soc_component_update_bits() calls then compare against the
stale cached values from the previous bind and skip hardware writes when the
new value matches the stale cache (for example leaving ES8328_MASTERMODE at its
0x80 master-mode reset default when consumer mode 0x00 is requested). Note
that while the unused es8328->type field added in this patch is wired up in
follow-up commit db663b096dbe9f68fbdc51ad2211743c7793550e, the regcache is
still not reinitialized after ES8328_CONTROL1_SCP_RESET.

Should es8328_component_probe() reinitialize the regmap cache (or bypass cache
for the reset sequence and call regcache_mark_dirty() / reinit) when resetting
the codec?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-es8328-vol-init-v2-0-d15b04c22f4f@163.com?part=1