On 2/21/2020 6:30 PM, Mark Brown wrote:
On Thu, Feb 20, 2020 at 12:04:45PM +0530, Sameer Pujar wrote:
+++ b/sound/soc/tegra/tegra210_dmic.c @@ -0,0 +1,515 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- tegra210_dmic.c - Tegra210 DMIC driver
- Copyright (c) 2020 NVIDIA CORPORATION. All rights reserved.
Please make the entire comment a C++ one so things look more intentional.
- /* Below enables all filters - DCR, LP and SC */
- { TEGRA210_DMIC_DBG_CTRL, 0xe },
So this isn't the hardware default?
No, the HW default is 0x2. Also, we are repurposing the LP filter for applying gain.
- srate = params_rate(params);
- if (dmic->srate_override)
srate = dmic->srate_override;How does this work for userspace? If we just ignore the sample rate we were asked for I'd expect that the application would get upset.
- if (strstr(kcontrol->id.name, "Boost Gain"))
dmic->boost_gain = value;Volume controls should end in "Volume".
- else if (strstr(kcontrol->id.name, "Audio Channels"))
dmic->audio_ch_override = value;This is something that would usually come from hw_params?
Yes, hw_params is where it is taken from. The additional override is optional and would not usually need to be set by user. However, we have certain other modules, like multiplexer and demultiplexer (proposed to be upstreamed in the near future), where no. of channels get changed at the output. When one or more such modules are connected in the path, hw_params does not reflect the channels post multiplex/demultiplex, hence this override would be needed.
- else if (strstr(kcontrol->id.name, "LR Polarity Select"))
dmic->lrsel = value;This and some of the others look like they're describing details of how the board is wired up so I'd not expect them to be runtime selectable?
No, these are not board wiring. OSR, polarity, etc. are configurable within the DMIC (within Tegra). Of course, these controls are optional and the default works just fine. Also, to clarify here, 'runtime selectable' does not mean these parameters can be modified while a recording session is in progress. These parameters (optional) need to set up before a session is started. User can close the recording, reconfigure the parameters, then start a new session, without needing reboot.
- SND_SOC_DAPM_MIC("Dummy Input", NULL),
This is just the microphone that happens to be attached, isn't it? If so that's a weird name.
It is not necessary for an actual mic to be connected to the pin. Recording will work even when the pin is left open, capture being silence. All the drivers, whether for playback or for capture, work OK even without any physical end-point, hence the "dummy" I/O.
+static const char * const tegra210_dmic_mono_conv_text[] = {
- "ZERO", "COPY",
+};
It'd be more idiomatic for ALSA to write these as Zero and Copy.
- SOC_ENUM_EXT("Channel Select", tegra210_dmic_ch_enum,
tegra210_dmic_get_control, tegra210_dmic_put_control),- SOC_ENUM_EXT("Mono To Stereo",
tegra210_dmic_mono_conv_enum, tegra210_dmic_get_control,tegra210_dmic_put_control),- SOC_ENUM_EXT("Stereo To Mono",
tegra210_dmic_stereo_conv_enum, tegra210_dmic_get_control,tegra210_dmic_put_control),I'd expect these to be in DAPM.
These are finer controls and I would guess are outside what can be described by DAPM. E.g. - say the user wants mono capture, but the board has stereo mics connected. Default configuration is that the Left channel would be propagated. The stereo_to_mono option offers additional control - to select the Right channel, or mix L and R into one. As mentioned earlier, much of the controls here are not necessary to be set for a basic use case to work. These are advanced settings.