Re: [PATCH v2 1/2] ASoC: Add ak4619 codec support

On Tue, Jun 18, 2024 at 12:36:43AM +0000, Kuninori Morimoto wrote:

Looks mostly good, a few small nits:

+static int ak4619_put_deemph(struct snd_kcontrol *kcontrol,

• 		struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
• struct ak4619_priv *ak4619 = snd_soc_component_get_drvdata(component);
• int deemph_en = ucontrol->value.integer.value[0];
• if (deemph_en > 1)

• return -EINVAL;
  

We should also check for negative values here, those are also out of range (many other drivers are buggy).

• ak4619->deemph_en = deemph_en;
• ak4619_set_deemph(component);
• return 0;

+}

This won't return 1 on change so will miss generating events confusing some UIs, the mixer-test selftest should notice this and the range check issue.

• /* Only slave mode is support */
• switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
• case SND_SOC_DAIFMT_CBS_CFS:

• break;
  

• default:

• return -EINVAL;
  

• }

Please update for the modern names.

+static const struct regmap_config ak4619_regmap_cfg = {

• .reg_bits = 8,
• .val_bits = 8,
• .max_register = 0x14,
• .reg_defaults = ak4619_reg_defaults,
• .num_reg_defaults = ARRAY_SIZE(ak4619_reg_defaults),
• .cache_type = REGCACHE_RBTREE,

+};

Unless there's a specific reason it's probably best to use _MAPLE for the cache, not that it's likely to make a difference to a driver with such a small regmap.