-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Thursday, January 4, 2018 9:14 AM To: Ryan Lee RyanS.Lee@maximintegrated.com Cc: lgirdwood@gmail.com; perex@perex.cz; tiwai@suse.com; arnd@arndb.de; afd@ti.com; robert.jarzmik@free.fr; supercraig0719@gmail.com; jbrunet@baylibre.com; dannenberg@ti.com; romain.perier@collabora.com; bryce.ferguson@rockwellcollins.com; kuninori.morimoto.gx@renesas.com; m- stecklein@ti.com; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; ryan.lee.maxim@gmail.com Subject: Re: [V3 2/2] ASoC: max98373: Added Amplifier Driver
On Wed, Jan 03, 2018 at 10:39:17AM -0800, Ryan Lee wrote:
This looks mostly good. There are a few smaller issues but I think at this point it's most sensible to apply and fix those incrementally so I'll do that, please follow up with patches fixing the remaining issues.
Thank you. Let me follow up with patches fixing the remaining issues.
--- /dev/null +++ b/sound/soc/codecs/max98373.c @@ -0,0 +1,971 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (c) 2017, Maxim Integrated */
SPDX headers are supposed to be C++ comments, please send a followup patch fixing this.
+static int max98373_get_bclk_sel(int bclk) {
- int i;
- /* match BCLKs per LRCLK */
- for (i = 0; i < ARRAY_SIZE(bclk_sel_table); i++) {
if (bclk_sel_table[i] == bclk)return i + 2;- }
- return 0;
+} +static int max98373_set_clock(struct snd_soc_codec *codec,
Missing blank line between the functions.
OK. Thanks.
- }
- /* set DAI_SR to correct LRCLK frequency */
Another missing blank line.
OK. Let me fix.
+static int max98373_dai_tdm_slot(struct snd_soc_dai *dai,
- unsigned int tx_mask, unsigned int rx_mask,
- int slots, int slot_width)
+{
- struct snd_soc_codec *codec = dai->codec;
- struct max98373_priv *max98373 =
snd_soc_codec_get_drvdata(codec);
- int bsel = 0;
- unsigned int chan_sz = 0;
- unsigned int mask;
- int x, slot_found;
- max98373->tdm_mode = true;
This should really also support disabling TDM mode - if the parameters are all 0 just turn TDM off. Again can be fixed in a followup.
OK. Let me apply it.
+SOC_SINGLE_TLV("DHT Gain Min", MAX98373_R20D1_DHT_CFG,
- MAX98373_DHT_SPK_GAIN_MIN_SHIFT, 9, 0,
+max98373_dht_spkgain_min_tlv), SOC_SINGLE_TLV("DHT Rot Pnt",
MAX98373_R20D1_DHT_CFG,
- MAX98373_DHT_ROT_PNT_SHIFT, 15, 0,
max98373_dht_rotation_point_tlv),
+SOC_SINGLE_TLV("DHT Attack Step", MAX98373_R20D2_DHT_ATTACK_CFG,
- MAX98373_DHT_ATTACK_STEP_SHIFT, 4, 0,
max98373_dht_step_size_tlv),
+SOC_SINGLE_TLV("DHT Release Step",
MAX98373_R20D3_DHT_RELEASE_CFG,
- MAX98373_DHT_RELEASE_STEP_SHIFT, 4, 0,
max98373_dht_step_size_tlv),
You should add a Volume on the end of these control names so that userspace knows how to display them properly; it's a little confusing as they're not actually gains but it tends to work out better. Same for most of the other TLV controls.
OK. Thanks for your feedback.