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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CY1PR11MB08413C66C71341011E4877BAE71C0@CY1PR11MB0841.namprd11.prod.outlook.com>
Date:   Fri, 5 Jan 2018 17:10:35 +0000
From:   Ryan Lee <RyanS.Lee@...imintegrated.com>
To:     Mark Brown <broonie@...nel.org>
CC:     "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "perex@...ex.cz" <perex@...ex.cz>,
        "tiwai@...e.com" <tiwai@...e.com>, "arnd@...db.de" <arnd@...db.de>,
        "afd@...com" <afd@...com>,
        "robert.jarzmik@...e.fr" <robert.jarzmik@...e.fr>,
        "supercraig0719@...il.com" <supercraig0719@...il.com>,
        "jbrunet@...libre.com" <jbrunet@...libre.com>,
        "dannenberg@...com" <dannenberg@...com>,
        "romain.perier@...labora.com" <romain.perier@...labora.com>,
        "bryce.ferguson@...kwellcollins.com" 
        <bryce.ferguson@...kwellcollins.com>,
        "kuninori.morimoto.gx@...esas.com" <kuninori.morimoto.gx@...esas.com>,
        "m-stecklein@...com" <m-stecklein@...com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "ryan.lee.maxim@...il.com" <ryan.lee.maxim@...il.com>
Subject: RE: [V3 2/2] ASoC: max98373: Added Amplifier Driver

>-----Original Message-----
>From: Mark Brown [mailto:broonie@...nel.org]
>Sent: Thursday, January 4, 2018 9:14 AM
>To: Ryan Lee <RyanS.Lee@...imintegrated.com>
>Cc: lgirdwood@...il.com; perex@...ex.cz; tiwai@...e.com; arnd@...db.de;
>afd@...com; robert.jarzmik@...e.fr; supercraig0719@...il.com;
>jbrunet@...libre.com; dannenberg@...com; romain.perier@...labora.com;
>bryce.ferguson@...kwellcollins.com; kuninori.morimoto.gx@...esas.com; m-
>stecklein@...com; alsa-devel@...a-project.org; linux-kernel@...r.kernel.org;
>ryan.lee.maxim@...il.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.