Re: Re: [PATCH v3 03/10] ASoC: tegra: add Tegra210 based DMIC driver

[Viswanath L <viswanathl@nvidia.com>]

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.