RE: [V3 2/2] ASoC: max98373: Added Amplifier Driver

[Ryan Lee]

>-----Original Message-----
>From: Mark Brown [mailto:broonie@xxxxxxxxxx]
>Sent: Thursday, January 4, 2018 9:14 AM
>To: Ryan Lee <RyanS.Lee@xxxxxxxxxxxxxxxxxxx>
>Cc: lgirdwood@xxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx; arnd@xxxxxxxx;
>afd@xxxxxx; robert.jarzmik@xxxxxxx; supercraig0719@xxxxxxxxx;
>jbrunet@xxxxxxxxxxxx; dannenberg@xxxxxx; romain.perier@xxxxxxxxxxxxx;
>bryce.ferguson@xxxxxxxxxxxxxxxxxxx; kuninori.morimoto.gx@xxxxxxxxxxx; m-
>stecklein@xxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>ryan.lee.maxim@xxxxxxxxx
>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.