>-----Original Message-----
>From: Mark Brown [mailto:broo...@kernel.org]
>Sent: Thursday, January 4, 2018 9:14 AM
>To: Ryan Lee <ryans....@maximintegrated.com>
>Cc: lgirdw...@gmail.com; pe...@perex.cz; ti...@suse.com; a...@arndb.de;
>a...@ti.com; robert.jarz...@free.fr; supercraig0...@gmail.com;
>jbru...@baylibre.com; dannenb...@ti.com; romain.per...@collabora.com;
>bryce.fergu...@rockwellcollins.com; kuninori.morimoto...@renesas.com; m-
>steckl...@ti.com; alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org;
>ryan.lee.ma...@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.
