On Wed, Feb 26, 2025 at 06:49:48PM +0800, Zhang Yi wrote:
--- /dev/null +++ b/sound/soc/codecs/es8389.c @@ -0,0 +1,977 @@ +/*
- es8389.c -- ES8389/ES8390 ALSA SoC Audio Codec
This is missing the SPDX header, and the whole comment block should be a C++ as a result for consistency.
- Copyright (C) 2024 Everest Semiconductor Co., Ltd
2025?
+static const char *es8389_outl_mux_txt[] = {
- "normal",
- "DAC2 channel to DAC1 channel",
+};
+static const char *es8389_outr_mux_txt[] = {
- "normal",
- "DAC1 channel to DAC2 channel",
+};
"Normal".
+static const unsigned int es8389_out_mux_values[] = {
- 0, 1
+};
That's just a normal ENUM, it doesn't need to be a VALUE_ENUM.
- SOC_ENUM("PGA1 Select", es8389_pga_enum[0]),
- SOC_ENUM("PGA2 Select", es8389_pga_enum[1]),
Declare these as individual variables rather than using magic numbers to index into an array, it's much more robust and legible.
- SOC_DOUBLE("ADC OSR Volume ON", ES8389_ADC_MUTE_REG2F, 6, 7, 1, 0),
On/off controls should end in Switch, see control-names.rst.
- SOC_DOUBLE("ADC OUTPUT Invert", ES8389_ADC_HPF2_REG25, 5, 6, 1, 0),
Same here.
- SOC_DOUBLE("DAC OUTPUT Invert", ES8389_DAC_INV_REG45, 5, 6, 1, 0),
and here.
- SOC_SINGLE("Mix ADCR And DACR to DACR", ES8389_DAC_MIX_REG44, 0, 1, 0),
- SOC_SINGLE("Mix ADCL And DACL to DACL", ES8389_DAC_MIX_REG44, 1, 1, 0),
These should be DAPM controls.
+/* codec hifi mclk clock divider coefficients */ +static const struct _coeff_div coeff_div[] = {
- {32 ,256000 ,8000 ,0x00 ,0x57 ,0x84 ,0xD0 ,0x03 ,0xC1 ,0xB0 ,0x00 ,0x00 ,0x1F ,0x7F ,0xBF ,0xC0 ,0xFF ,0x7F ,0x01 ,0x12 ,0x00 ,0x09 ,0x19 ,0x07},
Usually we write NN, not NN ,
- default:
break;- }
regmap_update_bits(es8389->regmap, ES8389_ADC_REG20, ES8389_DAIFMT_MASK, state);regmap_update_bits(es8389->regmap, ES8389_DAC_REG40, ES8389_DAIFMT_MASK, state);/* clock inversion */- switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
Indentation here is weird.
+static void es8389_init(struct snd_soc_component *codec) +{
- struct es8389_private *es8389 = snd_soc_component_get_drvdata(codec);
- regmap_write(es8389->regmap, ES8389_ISO_CTL_REGF3, 0x00);
- regmap_write(es8389->regmap, ES8389_RESET_REG00, 0x7E);
- regmap_write(es8389->regmap, ES8389_ISO_CTL_REGF3, 0x38);
- regmap_write(es8389->regmap, ES8389_ADC_HPF1_REG24, 0x64);
- regmap_write(es8389->regmap, ES8389_ADC_HPF2_REG25, 0x04);
- regmap_write(es8389->regmap, ES8389_DAC_INV_REG45, 0x03);
What are all these magic writes doing - some of them are likely sensible but there's an awful lot of magic numbers here and some of the register names seem like things that people might want to configure.
- regmap_write(es8389->regmap, ES8389_MIC1_GAIN_REG72, 0x10);
- regmap_write(es8389->regmap, ES8389_MIC2_GAIN_REG73, 0x10);
These for example look very much like user controls. In general we stick with the silicon defaults to avoid issues with competing use caes wanting different values.
- //es8389_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
Just remove this if it's commented out.
+static struct regmap_config es8389_regmap = {
- .reg_bits = 8,
- .val_bits = 8,
- .max_register = ES8389_MAX_REGISTER,
- .volatile_reg = es8389_volatile_register,
- .cache_type = REGCACHE_RBTREE,
+};
Unless you've got a specific reason you should use _MAPLE not _RBTREE, it's a more modern data structure and makes decisions that tend to be better on current hardware.
+static int es8389_i2c_probe(struct i2c_client *i2c_client) +{
- struct es8389_private *es8389;
- int ret = -1;
- //unsigned int val;
More commented code.
- es8389->regmap = devm_regmap_init_i2c(i2c_client, &es8389_regmap);
- if (IS_ERR(es8389->regmap)) {
ret = PTR_ERR(es8389->regmap);dev_err(&i2c_client->dev, "regmap_init() failed: %d\n", ret);
Use dev_err_probe().
+++ b/sound/soc/codecs/es8389.h @@ -0,0 +1,139 @@ +/* +* ES8389.h -- ES8389 ALSA SoC Audio Codec
Indentation is weird - the *s should be aligned.