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