RE: [EXTERNAL] Re: [v4] ASoC: max98520: add max98520 audio amplifier driver

[George Song]

> > +static int max98520_dai_tdm_slot(struct snd_soc_dai *dai,
> > +                              unsigned int tx_mask, unsigned int rx_mask,
> > +                              int slots, int slot_width) {
> > +     struct snd_soc_component *component = dai->component;
> > +     struct max98520_priv *max98520 =
> > +             snd_soc_component_get_drvdata(component);
> > +     int bsel = 0;
> 
> nit-pick: useless initialization....
It will be removed useless initialization for bsel.
> 
> > +     unsigned int chan_sz = 0;
> > +
> > +     if (!tx_mask && !rx_mask && !slots && !slot_width)
> > +             max98520->tdm_mode = false;
> > +     else
> > +             max98520->tdm_mode = true;
> > +
> > +     /* BCLK configuration */
> > +     bsel = max98520_get_bclk_sel(slots * slot_width);
> 
> ... it's overridden here.
OK.
> 
> > +     if (bsel == 0) {
> > +             dev_err(component->dev, "BCLK %d not supported\n",
> > +                     slots * slot_width);
> > +             return -EINVAL;
> > +     }
> > +
> > +     regmap_update_bits(max98520->regmap,
> > +                        MAX98520_R2041_PCM_CLK_SETUP,
> > +                        MAX98520_PCM_CLK_SETUP_BSEL_MASK,
> > +                        bsel);
> > +
> > +     /* Channel size configuration */
> > +     switch (slot_width) {
> > +     case 16:
> > +             chan_sz = MAX98520_PCM_MODE_CFG_CHANSZ_16;
> > +             break;
> > +     case 24:
> > +             chan_sz = MAX98520_PCM_MODE_CFG_CHANSZ_24;
> > +             break;
> > +     case 32:
> > +             chan_sz = MAX98520_PCM_MODE_CFG_CHANSZ_32;
> > +             break;
> > +     default:
> > +             dev_err(component->dev, "format unsupported %d\n",
> > +                     slot_width);
> > +             return -EINVAL;
> > +     }
> > +
> > +     regmap_update_bits(max98520->regmap,
> > +                        MAX98520_R2040_PCM_MODE_CFG,
> > +                        MAX98520_PCM_MODE_CFG_CHANSZ_MASK, chan_sz);
> > +
> > +     /* Rx slot configuration */
> > +     regmap_update_bits(max98520->regmap,
> > +                        MAX98520_R2044_PCM_RX_SRC2,
> > +                        MAX98520_PCM_DMIX_CH0_SRC_MASK,
> > +                        rx_mask);
> > +     regmap_update_bits(max98520->regmap,
> > +                        MAX98520_R2044_PCM_RX_SRC2,
> > +                        MAX98520_PCM_DMIX_CH1_SRC_MASK,
> > +                        rx_mask << MAX98520_PCM_DMIX_CH1_SHIFT);
> > +
> > +     return 0;
> > +}
> > +
> > +#define MAX98520_RATES SNDRV_PCM_RATE_8000_192000
> > +
> > +#define MAX98520_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
> > +     SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
> > +
> > +static const struct snd_soc_dai_ops max98520_dai_ops = {
> > +     .set_fmt = max98520_dai_set_fmt,
> > +     .hw_params = max98520_dai_hw_params,
> > +     .set_tdm_slot = max98520_dai_tdm_slot, };
> > +
> > +static int max98520_dac_event(struct snd_soc_dapm_widget *w,
> > +                           struct snd_kcontrol *kcontrol, int event)
> > +{
> > +     struct snd_soc_component *component =
> > +             snd_soc_dapm_to_component(w->dapm);
> > +     struct max98520_priv *max98520 =
> > +             snd_soc_component_get_drvdata(component);
> > +
> > +     switch (event) {
> > +     case SND_SOC_DAPM_POST_PMU:
> 
> is it intentional that you use POST_PMU here? for symmetry with
> POST_PMD below, should this be PRE_PMU?
I`ve checked with modified PRE_PMU, but it`s no sound on my test environment.
It`s worked with POST_PMU for AMP on.
> 
> > +             dev_dbg(component->dev, " AMP ON\n");
> > +
> > +             regmap_write(max98520->regmap, MAX98520_R209F_AMP_EN,
> 1);
> > +             regmap_write(max98520->regmap, MAX98520_R210F_GLOBAL_EN,
> 1);
> > +             usleep_range(30000, 31000);
> > +             break;
> > +     case SND_SOC_DAPM_POST_PMD:
> > +             dev_dbg(component->dev, " AMP OFF\n");
> > +
> > +             regmap_write(max98520->regmap, MAX98520_R210F_GLOBAL_EN,
> 0);
> > +             regmap_write(max98520->regmap, MAX98520_R209F_AMP_EN,
> 0);
> > +             usleep_range(30000, 31000);
> > +             break;
> > +     default:
> > +             return 0;
> > +     }
> > +     return 0;
> > +}
> >
> > +static void max98520_reset(struct max98520_priv *max98520, struct
> > +device *dev) {
> > +     int ret, reg, count;
> > +
> > +     /* Software Reset */
> > +     ret = regmap_write(max98520->regmap, MAX98520_R2000_SW_RESET,
> 1);
> > +     if (ret)
> > +             dev_err(dev, "Reset command failed. (ret:%d)\n", ret);
> 
> return; ?
> 
> Trying to verify if the reset worked is the reset command failed
> seems unnecessary?
This function will be removed and added just regmap_write for SW_RESET register in proper places.
> 
> > +
> > +     count = 0;
> > +     while (count < 3) {
> > +             usleep_range(10000, 11000);
> > +             /* Software Reset Verification */
> > +             ret = regmap_read(max98520->regmap,
> MAX98520_R21FF_REVISION_ID, &reg);
> > +             if (!ret)
> > +                     return;
> > +
> > +             count++;
> > +     }
> > +     dev_err(dev, "Reset failed. (ret:%d)\n", ret); }
> 
> > +#ifdef CONFIG_PM_SLEEP
> 
> the recommendation is to remove these ifdefs and use __maybe_unused
> for pm functions.
It will be added __maybe_unused on them.
> 
> > +static int max98520_suspend(struct device *dev) {
> > +     struct max98520_priv *max98520 = dev_get_drvdata(dev);
> > +
> > +     regcache_cache_only(max98520->regmap, true);
> > +     regcache_mark_dirty(max98520->regmap);
> > +     return 0;
> > +}
> > +
> > +static int max98520_resume(struct device *dev) {
> > +     struct max98520_priv *max98520 = dev_get_drvdata(dev);
> > +
> > +     regcache_cache_only(max98520->regmap, false);
> > +     max98520_reset(max98520, dev);
> > +     regcache_sync(max98520->regmap);
> > +     return 0;
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops max98520_pm = {
> > +     SET_SYSTEM_SLEEP_PM_OPS(max98520_suspend, max98520_resume) };
> > +
> > +static const struct snd_soc_component_driver
> soc_codec_dev_max98520 = {
> > +     .probe                  = max98520_probe,
> > +     .controls               = max98520_snd_controls,
> > +     .num_controls           = ARRAY_SIZE(max98520_snd_controls),
> > +     .dapm_widgets           = max98520_dapm_widgets,
> > +     .num_dapm_widgets       = ARRAY_SIZE(max98520_dapm_widgets),
> > +     .dapm_routes            = max98520_audio_map,
> > +     .num_dapm_routes        = ARRAY_SIZE(max98520_audio_map),
> > +     .idle_bias_on           = 1,
> > +     .use_pmdown_time        = 1,
> > +     .endianness             = 1,
> > +     .non_legacy_dai_naming  = 1,
> > +};
> > +
> > +static const struct regmap_config max98520_regmap = {
> > +     .reg_bits = 16,
> > +     .val_bits = 8,
> > +     .max_register = MAX98520_R21FF_REVISION_ID,
> > +     .reg_defaults  = max98520_reg,
> > +     .num_reg_defaults = ARRAY_SIZE(max98520_reg),
> > +     .readable_reg = max98520_readable_register,
> > +     .volatile_reg = max98520_volatile_reg,
> > +     .cache_type = REGCACHE_RBTREE,
> > +};
> > +
> > +static void max98520_power_on(struct max98520_priv *max98520,
> bool
> > +poweron) {
> > +     if (max98520->reset_gpio)
> > +             gpiod_set_value_cansleep(max98520->reset_gpio,
> > +!poweron); }
> > +
> > +static int max98520_i2c_probe(struct i2c_client *i2c, const
> struct
> > +i2c_device_id *id) {
> > +     int ret = 0;
> 
> useless init
It will be removed useless initialization for bret.
> 
> > +     int reg = 0;
> > +     struct max98520_priv *max98520 = NULL;
> 
> useless init
It will be removed useless initialization for max98520.
> 
> > +     struct i2c_adapter *adapter = to_i2c_adapter(i2c->dev.parent);
> > +
> > +     ret = i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE |
> I2C_FUNC_SMBUS_BYTE_DATA);
> > +     if (!ret) {
> > +             dev_err(&i2c->dev, "I2C check functionality failed\n");
> > +             return -ENXIO;
> > +     }
> > +
> > +     max98520 = devm_kzalloc(&i2c->dev, sizeof(*max98520),
> > + GFP_KERNEL);
> > +
> > +     if (!max98520) {
> > +             ret = -ENOMEM;
> > +             return ret;
> 
> return -ENOMEM;
It will be modified return -ENOMEM; instead of ret = -ENOMEM;
> 
> > +     }
> > +     i2c_set_clientdata(i2c, max98520);
> > +
> > +     /* regmap initialization */
> > +     max98520->regmap =
> > +             devm_regmap_init_i2c(i2c, &max98520_regmap);
> 
> one line?
It will be modified by one line for it.
> 
> > +     if (IS_ERR(max98520->regmap)) {
> > +             ret = PTR_ERR(max98520->regmap);
> > +             dev_err(&i2c->dev,
> > +                     "Failed to allocate regmap: %d\n", ret);
> 
> one line?
It will be modified by one line for it.
> 
> > +             return ret;
> > +     }
> > +
> > +     /* Power on device */
> > +     max98520->reset_gpio = devm_gpiod_get_optional(&i2c->dev,
> "reset", GPIOD_OUT_HIGH);
> > +     if (max98520->reset_gpio) {
> > +             if (IS_ERR(max98520->reset_gpio)) {
> > +                     ret = PTR_ERR(max98520->reset_gpio);
> > +                     dev_err(&i2c->dev, "Unable to request GPIO
> pin: %d.\n", ret);
> > +                     return ret;
> > +             }
> > +
> > +             max98520_power_on(max98520, 1);
> > +     }
> > +
> > +     /* Check Revision ID */
> > +     ret = regmap_read(max98520->regmap,
> MAX98520_R21FF_REVISION_ID, &reg);
> > +     if (ret < 0) {
> > +             dev_err(&i2c->dev,
> > +                     "Failed to read: 0x%02X\n",
> MAX98520_R21FF_REVISION_ID);
> > +             return ret;
> > +     }
> > +     dev_info(&i2c->dev, "MAX98520 revisionID: 0x%02X\n", reg);
> > +
> > +     /* codec registration */
> > +     ret = devm_snd_soc_register_component(&i2c->dev,
> > +                                           &soc_codec_dev_max98520,
> > +             max98520_dai, ARRAY_SIZE(max98520_dai));
> 
> alignment?
It will be set alignment properly.
> 
> > +     if (ret < 0)
> > +             dev_err(&i2c->dev, "Failed to register codec: %d\n",
> > + ret);
> > +
> > +     return ret;
> > +}
> 
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id max98520_acpi_match[] = {
> > +     { "MX98520", 0 },
> 
> MX is not a valid ACPI/PNP vendor identifier but I suppose it's been
> used by Maxim for all products...
It will be removed them ACPI featuring code.