RE: [PATCH 3/5] ASoC: codecs: ES8326: Adding new volume kcontrols
ES8326 features a headphone volume control register and four DAC volume control registers. We add new volume Kcontrols for these registers to enhance the configurability of the volume settings, providing users with greater flexibility.
This is much better integrated than the earlier version, but there's still a few issues.
+static int es8326_hplvol_set(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol)
+{
- struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
- struct es8326_priv *es8326 = snd_soc_component_get_drvdata(component);
- unsigned int hp_vol;
- if (ucontrol->value.integer.value[0] == 3) {
- dev_dbg(component->dev, "HPL_VOL does not change");
- return 0;
- }
This will silently ignore attempts to write the invalid value which isn't great any might confuse some userspace code, it would be better to do something like val = ucontrol->value.integer.value[0]; if (val >= 3) val++; (with corresponding changes to the other functions) so that we just skip over the invalid value without userspace being aware of it. We should also be validating that out of range values are rejected or at least constained to the relevant bitfield, other than 3 any invalid value will just be written straight into the register (rejecting should be more robust).
- regmap_read(es8326->regmap, ES8326_HP_VOL, &hp_vol);
- hp_vol &= 0x8F;
- hp_vol |= ucontrol->value.integer.value[0] << 4;
- regmap_write(es8326->regmap, ES8326_HP_VOL, hp_vol);
regmap_update_bits().
- return 0;
+}
Also please run mixer-test on your driver - for this control it'll tell you that this function isn't returning 1 when the value changes, meaning that events won't be generated when the value changes.
- SOC_SINGLE_TLV("HPL Playback Volume", ES8326_DACL_VOL, 0, 0xbf, 0, dac_vol_tlv),
- SOC_SINGLE_TLV("HPR Playback Volume", ES8326_DACR_VOL, 0, 0xbf, 0, dac_vol_tlv),
- SOC_SINGLE_TLV("SPKL Playback Volume", ES8326_SPKL_VOL, 0, 0xbf, 0, dac_vol_tlv),
- SOC_SINGLE_TLV("SPKR Playback Volume", ES8326_SPKR_VOL, 0, 0xbf, 0, dac_vol_tlv),
It would be *better* if these were stereo controls, but it's not essential.
- SOC_ENUM("HPVol SPKVol switch", hpvol_spkvol_switch),
Switch should have a capital letter (mixer-test should catch this as well).
I'll take your advice and submit a separate new patch
participants (1)
-
Zhu Ning