| 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
| ||
|
Message-ID: <CABff4NS7L4EsqNALMOmyTY-Q=ZyDct9JXP1C2Y-XcnhXf48+=A@mail.gmail.com>
Date: Tue, 12 May 2020 18:17:44 +0900
From: Steve Lee <steves.lee.maxim@...il.com>
To: Tzung-Bi Shih <tzungbi@...gle.com>
Cc: Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>, ckeepax@...nsource.cirrus.com,
geert@...ux-m68k.org, rf@...nsource.wolfsonmicro.com,
Shuming [θζΈι] <shumingf@...ltek.com>,
Srini Kandagatla <srinivas.kandagatla@...aro.org>,
Krzysztof Kozlowski <krzk@...nel.org>, dmurphy@...com,
jack.yu@...ltek.com, nuno.sa@...log.com,
steves.lee@...imintegrated.com,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
ALSA development <alsa-devel@...a-project.org>,
ryan.lee.maxim@...il.com
Subject: Re: [PATCH 2/2] ASoC: max98390: Added Amplifier Driver
On Mon, May 11, 2020 at 8:16 PM Tzung-Bi Shih <tzungbi@...gle.com> wrote:
>
> (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@...il.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.
Thank you for feedback.
I've add initial value for invert.
>
> > +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.
Done as requested.
>
> > +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);
Done as requested.
>
> > +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.
Done as requested.
>
> > +
> > + filename = "dsm_param.bin";
> Either:
> - initialize when declaring the variable
> - remove the variable and inline into request_firmware() call
I've added initialized at declare.
>
> > + 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().
Done as requested.
>
> > + 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, ®);
> > + if (ret < 0) {
> if (ret)
Done as requested.
Powered by blists - more mailing lists