From: Ryan Lee <RyanS.Lee@maximintegrated.com>
To: Mark Brown <broonie@kernel.org>
CC: "lgirdwood@gmail.com" <lgirdwood@gmail.com>,
        "perex@perex.cz" <perex@perex.cz>,
        "tiwai@suse.com" <tiwai@suse.com>, "arnd@arndb.de" <arnd@arndb.de>,
        "afd@ti.com" <afd@ti.com>,
        "robert.jarzmik@free.fr" <robert.jarzmik@free.fr>,
        "supercraig0719@gmail.com" <supercraig0719@gmail.com>,
        "jbrunet@baylibre.com" <jbrunet@baylibre.com>,
        "dannenberg@ti.com" <dannenberg@ti.com>,
        "romain.perier@collabora.com" <romain.perier@collabora.com>,
        "bryce.ferguson@rockwellcollins.com" 
        <bryce.ferguson@rockwellcollins.com>,
        "kuninori.morimoto.gx@renesas.com" <kuninori.morimoto.gx@renesas.com>,
        "m-stecklein@ti.com" <m-stecklein@ti.com>,
        "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "ryan.lee.maxim@gmail.com" <ryan.lee.maxim@gmail.com>
Subject: RE: [V3 2/2] ASoC: max98373: Added Amplifier Driver
Thread-Topic: [V3 2/2] ASoC: max98373: Added Amplifier Driver
Thread-Index: AQHThMIwGuDhyppEZ0aZeZ+lJxx/U6Nj9TyAgAGRcaA=
Date: Fri, 5 Jan 2018 17:10:35 +0000
Message-ID: <CY1PR11MB08413C66C71341011E4877BAE71C0@CY1PR11MB0841.namprd11.prod.outlook.com>
References: <1515004757-22267-1-git-send-email-ryans.lee@maximintegrated.com>
 <20180104171345.GI10774@sirena.org.uk>
In-Reply-To: <20180104171345.GI10774@sirena.org.uk>
Accept-Language: en-US, ko-KR
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 03e8040b-a1d7-46de-7e72-08d5545f3c4a
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Jan 2018 17:10:35.1996
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: fbd909df-ea69-4788-a554-f24b7854ad03
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR11MB0841
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Return-Path: <linux-kernel-owner@vger.kernel.org>

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