> 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
