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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Feb 2018 10:22:16 +0000
From:   Cosmin Samoila <cosmin.samoila@....com>
To:     "broonie@...nel.org" <broonie@...nel.org>
CC:     dl-linux-imx <linux-imx@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "wakasugi.jb@...asahi-kasei.co.jp" <wakasugi.jb@...asahi-kasei.co.jp>,
        "mihai.serban@...il.com" <mihai.serban@...il.com>,
        "S.j. Wang" <shengjiu.wang@....com>,
        Daniel Baluta <daniel.baluta@....com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "mihai.serban@....com" <mihai.serban@....com>
Subject: Re: [PATCH] ASoC: codecs: Add support for AK4458 DAC driver

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