Re: [PATCH] ASoC: codecs: Add support for AK4458 DAC driver

[Cosmin Samoila]

Hi Mark,

> > +static const struct soc_enum ak4458_dac_enum[] = {
> > +/*0*/	SOC_ENUM_SINGLE(AK4458_01_CONTROL2, 1,
> > +			ARRAY_SIZE(ak4458_dem_select_texts),
> > +			ak4458_dem_select_texts),
> > +/*1*/	SOC_ENUM_SINGLE(AK4458_0A_CONTROL6, 0,
> The fact that you need these comments is why these arrays are a bad
> idea
> - just use individually named variables as other drivers do.
> 
> > 
> > +static const struct snd_kcontrol_new ak4458_snd_controls[] = {
> > +	SOC_SINGLE_TLV("AK4458 L1ch Digital Volume",
> > +		ÂÂÂÂÂÂÂAK4458_03_LCHATT, 0/*shift*/, 0xFF/*max
> > value*/,
> > +		ÂÂÂÂÂÂÂ0/*invert*/, latt_tlv),
> > +	SOC_SINGLE_TLV("AK4458 R1ch Digital Volume",
> > +		ÂÂÂÂÂÂÂAK4458_04_RCHATT, 0, 0xFF, 0, ratt_tlv),
> It'd be more idiomatic to combine these into stereo pairs than have
> them
> as single channel controls.
> 
> > 
> > +static const char * const ak4458_dac_select_texts[] = { "OFF",
> > "ON" };
> This looks like the users should be switch controls - what's the goal
> here?
> 
> > 

I think this is to allow users to switch off sound for all channels but
it seems silly to have 4 controls doing the same thing.
We now have two options:
- one control to switch off sound for all channels
- one controll per DAC

What do you mean by "users should be switch controls" ?
Should we use SND_SOC_DAPM_SWITCH instead of SND_SOC_DAPM_MUX?

We are still learing the inner details of ASoC.
Thank you for your help.

Cosmin