>-----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. _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
