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

Mark Brown
2020-02-21T13:01:08Z

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

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?

> + 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?

> + 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?

> + 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.

> +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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]