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.
Attachment:
signature.asc
Description: PGP signature
