On Tue, Jun 04, 2024 at 06:17:02AM +0000, Jack Yu wrote:
> +static int rt1318_init_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> + struct rt1318_priv *rt1318 = snd_soc_component_get_drvdata(component);
> +
> + rt1318->rt1318_init = ucontrol->value.integer.value[0];
> +
> + if (rt1318->rt1318_init)
> + rt1318_reg_init(component);
> +
> + return 0;
> +}
So this control resets the CODEC - what's the story here? Really it
should be a boolean, and if you run mixer-test it'll tell you you're not
returning 1 for value changes - please run mixer-test, there are a
number of other errors that it will detect here.
> +static int rt1318_dvol_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> + struct rt1318_priv *rt1318 = snd_soc_component_get_drvdata(component);
> +
> + rt1318->rt1318_dvol = ucontrol->value.integer.value[0];
> +
> + if (rt1318->rt1318_dvol <= RT1318_DVOL_STEP) {
> + regmap_write(rt1318->regmap, RT1318_DA_VOL_L_8, rt1318->rt1318_dvol >> 8);
> + regmap_write(rt1318->regmap, RT1318_DA_VOL_L_1_7, rt1318->rt1318_dvol & 0xff);
> + regmap_write(rt1318->regmap, RT1318_DA_VOL_R_8, rt1318->rt1318_dvol >> 8);
> + regmap_write(rt1318->regmap, RT1318_DA_VOL_R_1_7, rt1318->rt1318_dvol & 0xff);
This will happily accept negative values which you probably don't want.
> + } else {
> + rt1318->rt1318_dvol = RT1318_DVOL_STEP;
> + dev_err(component->dev, "Unsupported volume gain\n");
The driver shouldn't allow userspace to spam the kernel log like this,
it can be used to mask issues.
> +static const struct snd_kcontrol_new rt1318_snd_controls[] = {
> + SOC_SINGLE_EXT("rt1318 digital volume", SND_SOC_NOPM, 0, 383, 0,
> + rt1318_dvol_get, rt1318_dvol_put),
No need to include the part number in controls, users aren't going to
care. The general style for ALSA controls is capitalised too.
Attachment:
signature.asc
Description: PGP signature
