Date:   Thu, 30 Mar 2017 18:18:40 -0700
From:   Ryan Lee <ryans.lee@...imintegrated.com>
To:     Mark Brown <broonie@...nel.org>
CC:     Liam Girdwood <lgirdwood@...il.com>, <robh+dt@...nel.org>,
        <mark.rutland@....com>, <perex@...ex.cz>, <tiwai@...e.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
        Arnd Bergmann <arnd@...db.de>,
        <ckeepax@...nsource.wolfsonmicro.com>, <lars@...afoo.de>,
        <bardliao@...ltek.com>, <nh6z@...z.net>, <KCHSU0@...oton.com>,
        Axel Lin <axel.lin@...ics.com>, <romain.perier@...labora.com>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        <oder_chiou@...ltek.com>, <Paul.Handrigan@...rus.com>,
        <alsa-devel@...a-project.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Dylan Reid <dgreid@...gle.com>
Subject: Re: [PATCH v3] ASoC: Add support for Maxim Integrated MAX98927 Amplifier

On Wed, Mar 29, 2017 at 7:23 AM, Mark Brown <broonie@...nel.org> wrote:
> On Wed, Mar 29, 2017 at 09:53:48AM +0900, Ryan Lee wrote:
>
>> +     case SND_SOC_DAIFMT_CBS_CFM:
>> +             mode = MAX98927_PCM_MASTER_MODE_HYBRID;
>> +     default:
>> +             dev_err(codec->dev, "DAI clock mode unsupported");
>
> Either we don't support _CBS_CFM or there's a missing break there.

This was removed since CVS_CFM is not supported.

>
>> +     switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>> +     case SND_SOC_DAIFMT_I2S:
>> +             max98927->iface |= SND_SOC_DAIFMT_I2S;
>> +
>> +     break;
>
> Please use the kernel coding style as you do in the rest of this
> function.

Modified as requested.

>
>> +     int reg = MAX98927_R0022_PCM_CLK_SETUP;
>> +     int mask = MAX98927_PCM_CLK_SETUP_BSEL_MASK;
>
>> +     regmap_update_bits(max98927->regmap, reg, mask, value);
>
> reg and mask have exactly one user (as you'd expect), just use the
> constants directly here to make things clearer.

Removed reg and mask.

>
>> +     switch (snd_pcm_format_width(params_format(params))) {
>> +     case 16:
>> +             chan_sz = MAX98927_PCM_MODE_CFG_CHANSZ_16;
>> +             max98927->ch_size = 16;
>
> You could just assign ch_size directly.

Sorry I don't fully understand this.
Anyway 'ch_size' will be directly updated from snd_pcm_format_width
information after modification.

>
>> +     /* sampling rate configuration */
>> +     switch (params_rate(params)) {
>> +     case 8000:
>> +             sampling_rate |= MAX98927_PCM_SR_SET1_SR_8000;
>
> sampling_rate is only ever set to a real value in this switch statement,
> you're not oring with anything else so you can just use an assignment
> which would be a lot clearer (it's not obvious what else might go in
> there) and the initial assignment to 0 for this and chan_sz can only
> mask errors.

Removed 'oring'.

>
>> +     /* set sampling rate of IV */
>> +     if (max98927->interleave_mode &&
>> +             sampling_rate > MAX98927_PCM_SR_SET1_SR_16000)
>> +             regmap_update_bits(max98927->regmap,
>
> Please use the kernel coding style, indent the second line of the if
> with the ( so that it doesn't look like part of the conditional code.

Modified as requested.

>
>> +     switch (reg) {
>> +     case MAX98927_R0001_INT_RAW1 ... MAX98927_R0028_ICC_RX_EN_B:
>
>
>> +             return true;
>> +     }
>> +     return false;
>
> It'd be clearer to put the return false in the switch statement like the
> return true, and also consistent with the _volatile_reg() function.

Modified as requested.

>
>> +static const char * const max98927_speaker_source_text[] = {
>> +     "i2s", "reserved", "tone", "pdm"
>> +};
>
> This looks like it should be a DAPM control.

Removed this and added it to 'dai_set_format' function.

>
>> +static const char * const max98927_monomix_output_text[] = {
>> +     "ch_0", "ch_1", "ch_1_2_div"
>> +};
>
> Similarly here.

Removed this since it was already added to DAPM control.

>
>> +     SOC_SINGLE_TLV("Digital Gain", MAX98927_R0036_AMP_VOL_CTRL,
>> +             0, (1<<MAX98927_AMP_VOL_WIDTH)-1, 0,
>> +             max98927_digital_tlv),
>
> All volume controls should end with Volume as per control-names.rst.

Modified as requested.

>
>> +     SOC_SINGLE("Amp DSP Enable", MAX98927_R0052_BROWNOUT_EN,
>> +             MAX98927_BROWNOUT_DSP_SHIFT, 1, 0),
>
> All on/off switches should end with Switch as per control-names.rst.

Modified as requested.

>
>> +static int max98927_probe(struct snd_soc_codec *codec)
>> +{
>
>> +       /* Check Revision ID */
>> +       ret = regmap_read(max98927->regmap,
>> +               MAX98927_R01FF_REV_ID, &reg);
>
> Basic device identification and setup should be done at the chip level
> probe not at the CODEC level so that if there are problems we fail as
> early as possible and so that diagnostic information is available to
> users as soon as possible, even if there's no sound card for the device
> in the system.

Moved to i2c_probe function.

>
>> +     /* Set inital volume (+13dB) */
>
> As with all other CODEC drivers you should leave the hardware defaults
> alone, what makes sense for your system may not make sense for other
> systems and the hardware defaults are a fixed thing.

We recommend to use static +13dB gain when our speaker protection
algorithm is running on DSP. It is not hardware default but mostly
+13dB is being used as default.

>
>> +     /* Boost Output Voltage & Current limit */
>> +     regmap_write(max98927->regmap,
>> +             MAX98927_R0040_BOOST_CTRL0,
>> +             0x1C);
>> +     regmap_write(max98927->regmap,
>> +             MAX98927_R0042_BOOST_CTRL1,
>> +             0x3E);
>
> This should be system specific, these values might be unsafe in some
> systems.

Same as above, most user will use maximum voltage & current limit
value to take advantage of 10V boost amplifier.
I added one more control to change the current limit value.

>
>> +err:
>> +     if (max98927)
>> +             devm_kfree(&i2c->dev, max98927);
>
> There is no need to explicitly free devm_ allocated memory, that's the
> point of devm.

Removed the code.

Powered by blists - more mailing lists