On Sat, Jan 20, 2024 at 06:12:38PM +0800, Zhu Ning wrote:
> 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).
Attachment:
signature.asc
Description: PGP signature
