Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200509031919.9006-1-steves.lee@maximintegrated.com>
In-Reply-To: <20200509031919.9006-1-steves.lee@maximintegrated.com>
From:   Tzung-Bi Shih <tzungbi@google.com>
Date:   Mon, 11 May 2020 19:16:19 +0800
Message-ID: <CA+Px+wUdavr-qqEEng86ZAuwx++J-qB3va7p28EjOx-K9SyBtg@mail.gmail.com>
Subject: Re: [PATCH 2/2] ASoC: max98390: Added Amplifier Driver
To:     Steve Lee <steves.lee.maxim@gmail.com>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, ckeepax@opensource.cirrus.com,
        geert@linux-m68k.org, rf@opensource.wolfsonmicro.com,
        =?UTF-8?B?U2h1bWluZyBb6IyD5pu46YqYXQ==?= <shumingf@realtek.com>,
        srinivas.kandagatla@linaro.org,
        Krzysztof Kozlowski <krzk@kernel.org>, dmurphy@ti.com,
        jack.yu@realtek.com, nuno.sa@analog.com,
        steves.lee@maximintegrated.com,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        ALSA development <alsa-devel@alsa-project.org>,
        ryan.lee.maxim@gmail.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

(The patch passed 2 round review in https://crrev.com/c/2083354)

On Sun, May 10, 2020 at 4:23 PM Steve Lee <steves.lee.maxim@gmail.com> wrote:
> +static int max98390_dai_set_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
> +{
> +       struct snd_soc_component *component = codec_dai->component;
> +       struct max98390_priv *max98390 =
> +               snd_soc_component_get_drvdata(component);
> +       unsigned int mode;
> +       unsigned int format;
> +       unsigned int invert;
> +
> +       dev_dbg(component->dev, "%s: fmt 0x%08X\n", __func__, fmt);
> +
> +       switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +       case SND_SOC_DAIFMT_CBS_CFS:
> +               mode = MAX98390_PCM_MASTER_MODE_SLAVE;
> +               break;
> +       case SND_SOC_DAIFMT_CBM_CFM:
> +               max98390->master = true;
> +               mode = MAX98390_PCM_MASTER_MODE_MASTER;
> +               break;
> +       default:
> +               dev_err(component->dev, "DAI clock mode unsupported\n");
> +               return -EINVAL;
> +       }
> +
> +       regmap_update_bits(max98390->regmap,
> +               MAX98390_PCM_MASTER_MODE,
> +               MAX98390_PCM_MASTER_MODE_MASK,
> +               mode);
> +
> +       switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> +       case SND_SOC_DAIFMT_NB_NF:
> +               break;
> +       case SND_SOC_DAIFMT_IB_NF:
> +               invert = MAX98390_PCM_MODE_CFG_PCM_BCLKEDGE;
> +               break;
> +       default:
> +               dev_err(component->dev, "DAI invert mode unsupported\n");
> +               return -EINVAL;
> +       }
> +
> +       regmap_update_bits(max98390->regmap,
> +               MAX98390_PCM_MODE_CFG,
> +               MAX98390_PCM_MODE_CFG_PCM_BCLKEDGE,
> +               invert);
invert will be uninitialized in the SND_SOC_DAIFMT_NB_NF case.

> +static int max98390_dai_hw_params(struct snd_pcm_substream *substream,
> +               struct snd_pcm_hw_params *params,
> +       struct snd_soc_dai *dai)
Lack of 1 tab indent.

> +static int max98390_adaptive_rdc_get(struct snd_kcontrol *kcontrol,
> +               struct snd_ctl_elem_value *ucontrol)
> +{
> +       int rdc, rdc0;
> +       struct snd_soc_component *component =
> +               snd_soc_kcontrol_component(kcontrol);
> +       struct max98390_priv *max98390 =
> +               snd_soc_component_get_drvdata(component);
> +
> +       regmap_read(max98390->regmap, THERMAL_RDC_RD_BACK_BYTE1, &rdc);
> +       regmap_read(max98390->regmap, THERMAL_RDC_RD_BACK_BYTE0, &rdc0);
> +       rdc0 |= rdc << 8;
> +       ucontrol->value.integer.value[0] = rdc0;
ucontrol->value.integer.value[0] = rdc0 | (rdc << 8);

> +static int max98390_dsm_init(struct snd_soc_component *component)
> +{
> +       int ret;
> +       const char *filename;
> +       struct max98390_priv *max98390 =
> +               snd_soc_component_get_drvdata(component);
> +       const struct firmware *fw = NULL;
> +       char *dsm_param = NULL;
Don't need to initialize fw and dsm_param in the case.

> +
> +       filename = "dsm_param.bin";
Either:
- initialize when declaring the variable
- remove the variable and inline into request_firmware() call

> +       ret = request_firmware(&fw, filename, component->dev);
> +       if (ret) {
> +               dev_err(component->dev,
> +                       "Failed to acquire dsm params: %d\n", ret);
> +               goto err;
> +       }
> +
> +       dev_info(component->dev,
> +               "max98390: param fw size %d\n",
> +               fw->size);
> +       dsm_param = (char *)fw->data;
> +       dsm_param += MAX98390_DSM_PAYLOAD_OFFSET;
> +       regmap_bulk_write(max98390->regmap, DSM_EQ_BQ1_B0_BYTE0,
> +               dsm_param,
> +               fw->size - MAX98390_DSM_PAYLOAD_OFFSET);
> +       release_firmware(fw);
> +       regmap_write(max98390->regmap, MAX98390_R23E1_DSP_GLOBAL_EN, 0x01);
> +
> +err:
> +       return ret;
> +}

> +static int max98390_probe(struct snd_soc_component *component)
> +{
> +       struct max98390_priv *max98390 =
> +               snd_soc_component_get_drvdata(component);
> +
> +       /* Update dsm bin param */
This comment makes more sense if before max98390_dsm_init().

> +       regmap_write(max98390->regmap, MAX98390_SOFTWARE_RESET, 0x01);
> +       /* Sleep reset settle time */
> +       msleep(20);
> +       max98390_dsm_init(component);

> +       /* Check Revision ID */
> +       ret = regmap_read(max98390->regmap,
> +               MAX98390_R24FF_REV_ID, &reg);
> +       if (ret < 0) {
if (ret)