| 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
| ||
|
Message-ID: <37f78772-7296-d831-73e1-095a66df8002@free-electrons.com>
Date: Wed, 10 May 2017 08:58:09 +0200
From: Mylene Josserand <mylene.josserand@...e-electrons.com>
To: Chen-Yu Tsai <wens@...e.org>
Cc: Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Maxime Ripard <maxime.ripard@...e-electrons.com>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Linux-ALSA <alsa-devel@...a-project.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>
Subject: Re: [PATCH 1/2] ASoC: sun8i-codec: Add ADC support for a33
Hi Chen-Yu,
Thank you for the review!
On 03/05/2017 19:13, Chen-Yu Tsai wrote:
> Hi,
>
> On Wed, May 3, 2017 at 9:56 PM, Mylène Josserand
> <mylene.josserand@...e-electrons.com> wrote:
>> Add ADC support for the sun8i-codec driver.
>>
>> This driver uses all the microphones widgets and routes provided by the
>> analog part (sun8i-codec-analog).
>> Some digital configurations are needed by creating new ADC widgets
>> and routes.
>>
>> Signed-off-by: Mylène Josserand <mylene.josserand@...e-electrons.com>
>> ---
>> sound/soc/sunxi/sun8i-codec.c | 80 +++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 78 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
>> index 5723c3404f6b..b4938b06acec 100644
>> --- a/sound/soc/sunxi/sun8i-codec.c
>> +++ b/sound/soc/sunxi/sun8i-codec.c
>> @@ -37,9 +37,11 @@
>> #define SUN8I_SYSCLK_CTL_SYSCLK_SRC 0
>> #define SUN8I_MOD_CLK_ENA 0x010
>> #define SUN8I_MOD_CLK_ENA_AIF1 15
>> +#define SUN8I_MOD_CLK_ENA_ADC 3
>> #define SUN8I_MOD_CLK_ENA_DAC 2
>> #define SUN8I_MOD_RST_CTL 0x014
>> #define SUN8I_MOD_RST_CTL_AIF1 15
>> +#define SUN8I_MOD_RST_CTL_ADC 3
>> #define SUN8I_MOD_RST_CTL_DAC 2
>> #define SUN8I_SYS_SR_CTRL 0x018
>> #define SUN8I_SYS_SR_CTRL_AIF1_FS 12
>> @@ -54,9 +56,25 @@
>> #define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ 4
>> #define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ_16 (1 << 4)
>> #define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT 2
>> +#define SUN8I_AIF1_ADCDAT_CTRL 0x044
>> +#define SUN8I_AIF1_ADCDAT_CTRL_AIF1_DA0L_ENA 15
>> +#define SUN8I_AIF1_ADCDAT_CTRL_AIF1_DA0R_ENA 14
>> #define SUN8I_AIF1_DACDAT_CTRL 0x048
>> #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA 15
>> #define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA 14
>> +#define SUN8I_AIF1_MXR_SRC 0x04c
>> +#define SUN8I_AIF1_MXR_SRC_AD0L_MXL_SRC_AIF1DA0L 15
>> +#define SUN8I_AIF1_MXR_SRC_AD0L_MXL_SRC_AIF2DACL 14
>> +#define SUN8I_AIF1_MXR_SRC_AD0L_MXL_SRC_ADCL 13
>> +#define SUN8I_AIF1_MXR_SRC_AD0L_MXL_SRC_AIF2DACR 12
>> +#define SUN8I_AIF1_MXR_SRC_AD0R_MXR_SRC_AIF1DA0R 11
>> +#define SUN8I_AIF1_MXR_SRC_AD0R_MXR_SRC_AIF2DACR 10
>> +#define SUN8I_AIF1_MXR_SRC_AD0R_MXR_SRC_ADCR 9
>> +#define SUN8I_AIF1_MXR_SRC_AD0R_MXR_SRC_AIF2DACL 8
>> +#define SUN8I_ADC_DIG_CTRL 0x100
>> +#define SUN8I_ADC_DIG_CTRL_ENDA 15
>
> The register bit name in the manual is ENAD, as in ENable ADc.
Okay, I did not know that, thanks.
>
>> +#define SUN8I_ADC_DIG_CTRL_ADOUT_DTS 2
>> +#define SUN8I_ADC_DIG_CTRL_ADOUT_DLY 1
>> #define SUN8I_DAC_DIG_CTRL 0x120
>> #define SUN8I_DAC_DIG_CTRL_ENDA 15
>> #define SUN8I_DAC_MXR_SRC 0x130
>> @@ -276,10 +294,28 @@ static const struct snd_kcontrol_new sun8i_dac_mixer_controls[] = {
>> SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_ADCR, 1, 0),
>> };
>>
>> +static const struct snd_kcontrol_new sun8i_input_mixer_controls[] = {
>> + SOC_DAPM_DOUBLE("AIF1 Slot 0 Digital ADC Capture Switch",
>> + SUN8I_AIF1_MXR_SRC,
>> + SUN8I_AIF1_MXR_SRC_AD0L_MXL_SRC_AIF1DA0L,
>> + SUN8I_AIF1_MXR_SRC_AD0R_MXR_SRC_AIF1DA0R, 1, 0),
>> + SOC_DAPM_DOUBLE("AIF2 Digital ADC Capture Switch", SUN8I_AIF1_MXR_SRC,
>> + SUN8I_AIF1_MXR_SRC_AD0L_MXL_SRC_AIF2DACL,
>> + SUN8I_AIF1_MXR_SRC_AD0R_MXR_SRC_AIF2DACR, 1, 0),
>> + SOC_DAPM_DOUBLE("AIF1 Data Digital ADC Capture Switch", SUN8I_AIF1_MXR_SRC,
>> + SUN8I_AIF1_MXR_SRC_AD0L_MXL_SRC_ADCL,
>> + SUN8I_AIF1_MXR_SRC_AD0R_MXR_SRC_ADCR, 1, 0),
>> + SOC_DAPM_DOUBLE("AIF2 Inv Digital ADC Capture Switch", SUN8I_AIF1_MXR_SRC,
>> + SUN8I_AIF1_MXR_SRC_AD0L_MXL_SRC_AIF2DACR,
>> + SUN8I_AIF1_MXR_SRC_AD0R_MXR_SRC_AIF2DACL, 1, 0),
>> +};
>
> This group of mixer controls is only for AIF1 slot 0 capture.
> So in fact they should all read "AIF1 Slot 0 Mixer X Capture Switch",
> with X replaced by "AIF1 Slot 0", "AIF2", "ADC", and "AIF2 Inv".
>
> This hopefully informs the user that these controls are for the AIF1
> slot 0 mixer, and are capture switch from the various sources listed
> above.
Sounds good.
>
>> +
>> static const struct snd_soc_dapm_widget sun8i_codec_dapm_widgets[] = {
>> - /* Digital parts of the DACs */
>> + /* Digital parts of the DACs and ADC */
>> SND_SOC_DAPM_SUPPLY("DAC", SUN8I_DAC_DIG_CTRL, SUN8I_DAC_DIG_CTRL_ENDA,
>> 0, NULL, 0),
>> + SND_SOC_DAPM_SUPPLY("ADC", SUN8I_ADC_DIG_CTRL, SUN8I_ADC_DIG_CTRL_ENDA,
>> + 0, NULL, 0),
>>
>> /* Analog DAC AIF */
>> SND_SOC_DAPM_AIF_IN("AIF1 Slot 0 Left", "Playback", 0,
>> @@ -289,17 +325,31 @@ static const struct snd_soc_dapm_widget sun8i_codec_dapm_widgets[] = {
>> SUN8I_AIF1_DACDAT_CTRL,
>> SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA, 0),
>>
>> - /* DAC Mixers */
>> + /* Analog ADC AIF */
>> + SND_SOC_DAPM_AIF_IN("AIF1 Slot 0 Left ADC", "Capture", 0,
>> + SUN8I_AIF1_ADCDAT_CTRL,
>> + SUN8I_AIF1_ADCDAT_CTRL_AIF1_DA0L_ENA, 0),
>> + SND_SOC_DAPM_AIF_IN("AIF1 Slot 0 Right ADC", "Capture", 0,
>> + SUN8I_AIF1_ADCDAT_CTRL,
>> + SUN8I_AIF1_ADCDAT_CTRL_AIF1_DA0R_ENA, 0),
>
> You want to use SND_SOC_DAPM_AIF_OUT here, for captured audio out of
> the codec and to the system. These are the last widgets in the capture
> path.
ACK
>
>> +
>> + /* DAC and ADC Mixers */
>> SOC_MIXER_ARRAY("Left Digital DAC Mixer", SND_SOC_NOPM, 0, 0,
>> sun8i_dac_mixer_controls),
>> SOC_MIXER_ARRAY("Right Digital DAC Mixer", SND_SOC_NOPM, 0, 0,
>> sun8i_dac_mixer_controls),
>> + SOC_MIXER_ARRAY("Left Digital ADC Mixer", SND_SOC_NOPM, 0, 0,
>> + sun8i_input_mixer_controls),
>> + SOC_MIXER_ARRAY("Right Digital ADC Mixer", SND_SOC_NOPM, 0, 0,
>> + sun8i_input_mixer_controls),
>
> And these should read "AIF1 Slot 0 Left/Right Mixer".
ACK
>
>>
>> /* Clocks */
>> SND_SOC_DAPM_SUPPLY("MODCLK AFI1", SUN8I_MOD_CLK_ENA,
>> SUN8I_MOD_CLK_ENA_AIF1, 0, NULL, 0),
>> SND_SOC_DAPM_SUPPLY("MODCLK DAC", SUN8I_MOD_CLK_ENA,
>> SUN8I_MOD_CLK_ENA_DAC, 0, NULL, 0),
>> + SND_SOC_DAPM_SUPPLY("MODCLK ADC", SUN8I_MOD_CLK_ENA,
>> + SUN8I_MOD_CLK_ENA_ADC, 0, NULL, 0),
>> SND_SOC_DAPM_SUPPLY("AIF1", SUN8I_SYSCLK_CTL,
>> SUN8I_SYSCLK_CTL_AIF1CLK_ENA, 0, NULL, 0),
>> SND_SOC_DAPM_SUPPLY("SYSCLK", SUN8I_SYSCLK_CTL,
>> @@ -316,6 +366,12 @@ static const struct snd_soc_dapm_widget sun8i_codec_dapm_widgets[] = {
>> SUN8I_MOD_RST_CTL_AIF1, 0, NULL, 0),
>> SND_SOC_DAPM_SUPPLY("RST DAC", SUN8I_MOD_RST_CTL,
>> SUN8I_MOD_RST_CTL_DAC, 0, NULL, 0),
>> + SND_SOC_DAPM_SUPPLY("RST ADC", SUN8I_MOD_RST_CTL,
>> + SUN8I_MOD_RST_CTL_ADC, 0, NULL, 0),
>> +
>> + SND_SOC_DAPM_MIC("Headset Mic", NULL),
>> + SND_SOC_DAPM_MIC("Mic", NULL),
>
> These Mics are board level widgets. Since you are using simple-card,
> you should add them in the device tree in the simple-card device
> node, using the "simple-audio-card,widgets" property.
>
> You probably should have done so for the output side as well.
> If simple-card were fully-routed, the existing device tree simply
> wouldn't work.
Okay, I will add them in the simple-audio-card node.
>
>> +
>> };
>>
>> static const struct snd_soc_dapm_route sun8i_codec_dapm_routes[] = {
>> @@ -325,11 +381,16 @@ static const struct snd_soc_dapm_route sun8i_codec_dapm_routes[] = {
>> { "RST AIF1", NULL, "AIF1 PLL" },
>> { "MODCLK AFI1", NULL, "RST AIF1" },
>> { "DAC", NULL, "MODCLK AFI1" },
>> + { "ADC", NULL, "MODCLK AFI1" },
>
> This makes no sense. Why would AIF1's module clock feed the ADC? Same goes
> for the existing DAC route.
>
It was difficult for me to know how to route the clocks with the DAC/ADC
widgets. As the clocks are needed to have a ADC/DAC working, I created
this route but it is, maybe, not the correct way to do it.
>>
>> { "RST DAC", NULL, "SYSCLK" },
>> { "MODCLK DAC", NULL, "RST DAC" },
>> { "DAC", NULL, "MODCLK DAC" },
>>
>> + { "RST ADC", NULL, "SYSCLK" },
>> + { "MODCLK ADC", NULL, "RST ADC" },
>> + { "ADC", NULL, "MODCLK ADC" },
>
> This makes little sense either. The SYSCLK probably feeds the ADC's
> module clock.
> The MODCLK then feeds the ADC. The ADC reset feeds the ADC (or rather the ADC
> depends on its reset). The "ADC" widget here is actually just the enable switch.
> But we can use it as a kind of placeholder, to setup the dependencies correctly.
>
Yep, that it is.
What do you have in mind about the ADC widget as a "placeholder"?
> I wish we had named the widgets better, but alas they are part of the
> device tree
> binding, even though they are not documented, so we can not change the existing
> ones.
I agree about the widgets names, they are not very clear.
>
>> +
>> /* DAC Routes */
>> { "AIF1 Slot 0 Right", NULL, "DAC" },
>> { "AIF1 Slot 0 Left", NULL, "DAC" },
>> @@ -339,6 +400,12 @@ static const struct snd_soc_dapm_route sun8i_codec_dapm_routes[] = {
>> "AIF1 Slot 0 Left"},
>> { "Right Digital DAC Mixer", "AIF1 Slot 0 Digital DAC Playback Switch",
>> "AIF1 Slot 0 Right"},
>> +
>> + /* ADC routes */
>> + { "Left Digital ADC Mixer", "AIF1 Data Digital ADC Capture Switch",
>> + "AIF1 Slot 0 Left ADC" },
>> + { "Right Digital ADC Mixer", "AIF1 Data Digital ADC Capture Switch",
>> + "AIF1 Slot 0 Right ADC" },
>
> Same thing about the "ADC Mixer" names.
>
> And these routes are completely backwards. "AIF1 Slot 0 Left/Right ADC" are
> the output stream widgets. They are fed from "AIF1 Slot 0 Left/Right Mixer"
> if you use the names I suggested, or "Left/Right Digital ADC Mixer" originally.
You are right, it make sense now.
>
> You then need other routes feeding the mixer. Looks like you also need ADC
> placeholder widgets on the digital side here, so you can hook up the analog
> side in simple-card more easily.
Okay, I will try to improve it.
>
> If you can, please dump the DAPM routes into a directed graph to check
> everything
> is connected and makes sense. I believe you have a script that does this.
Yes, I tested it and the ADC mixers are connected to nothing so this
route does not make sense. I will rework it.
>> };
>>
>> static struct snd_soc_dai_ops sun8i_codec_dai_ops = {
>> @@ -356,6 +423,15 @@ static struct snd_soc_dai_driver sun8i_codec_dai = {
>> .rates = SNDRV_PCM_RATE_8000_192000,
>> .formats = SNDRV_PCM_FMTBIT_S16_LE,
>> },
>> + /* capture capabilities */
>> + .capture = {
>> + .stream_name = "Capture",
>> + .channels_min = 1,
>> + .channels_max = 2,
>> + .rates = SNDRV_PCM_RATE_8000_192000,
>> + .formats = SNDRV_PCM_FMTBIT_S16_LE,
>> + .sig_bits = 24,
>> + },
>> /* pcm operations */
>> .ops = &sun8i_codec_dai_ops,
>> };
>> --
>> 2.11.0
>>
Thank you again,
Best regards,
--
Mylène Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Powered by blists - more mailing lists