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;
Date:   Mon, 10 Aug 2020 19:59:33 +0100
From:   Mark Brown <broonie@kernel.org>
To:     Jiaxin Yu <jiaxin.yu@mediatek.com>
Cc:     matthias.bgg@gmail.com, robh+dt@kernel.org, tiwai@suse.com,
        linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, howie.huang@mediatek.com,
        tzungbi@google.com, eason.yen@mediatek.com,
        shane.chien@mediatek.com
Subject: Re: [PATCH v2 1/2] ASoC: mediatek: mt6359: add codec driver
Message-ID: <20200810185933.GI6438@sirena.org.uk>
References: <1597028754-7732-1-git-send-email-jiaxin.yu@mediatek.com>
 <1597028754-7732-2-git-send-email-jiaxin.yu@mediatek.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="tctmm6wHVGT/P6vA"
Content-Disposition: inline
In-Reply-To: <1597028754-7732-2-git-send-email-jiaxin.yu@mediatek.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--tctmm6wHVGT/P6vA
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Aug 10, 2020 at 11:05:53AM +0800, Jiaxin Yu wrote:

> +void mt6359_set_playback_gpio(struct snd_soc_component *cmpnt)
> +{
> +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);

> +void mt6359_reset_playback_gpio(struct snd_soc_component *cmpnt)
> +{
> +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);

> +void mt6359_set_capture_gpio(struct snd_soc_component *cmpnt)
> +{

> +void mt6359_reset_capture_gpio(struct snd_soc_component *cmpnt)
> +{

What are these, should they not be managed through gpiolib and/or
pinctrl?

> +/* use only when doing mtkaif calibraiton at the boot time */
> +static int mt6359_set_dcxo(struct mt6359_priv *priv, bool enable)
> +{
> +	regmap_update_bits(priv->regmap, MT6359_DCXO_CW12,
> +			   0x1 << RG_XO_AUDIO_EN_M_SFT,
> +			   (enable ? 1 : 0) << RG_XO_AUDIO_EN_M_SFT);
> +	return 0;

Either don't have a return value or use the result of
regmap_update_bits().  There's similar issues with some other functions
in here.

> +int mt6359_mtkaif_calibration_enable(struct snd_soc_pcm_runtime *rtd)
> +{

> +EXPORT_SYMBOL_GPL(mt6359_mtkaif_calibration_enable);

Why is this exported?

> +static void hp_aux_feedback_loop_gain_ramp(struct mt6359_priv *priv, bool up)
> +{
> +	int i = 0, stage = 0;
> +
> +	/* Reduce HP aux feedback loop gain step by step */
> +	for (i = 0; i <= 0xf; i++) {
> +		stage = up ? i : 0xf - i;

Please write normal conditional statements, it helps legibility.

> +static int mt6359_put_volsw(struct snd_kcontrol *kcontrol,
> +			    struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +			snd_soc_kcontrol_component(kcontrol);
> +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(component);
> +	struct soc_mixer_control *mc =
> +			(struct soc_mixer_control *)kcontrol->private_value;
> +	unsigned int reg;
> +	int index = ucontrol->value.integer.value[0];
> +	int ret;
> +
> +	ret = snd_soc_put_volsw(kcontrol, ucontrol);
> +	if (ret < 0)
> +		return ret;

So we make the volume change actually take effect...

> +	switch (mc->reg) {
> +	case MT6359_ZCD_CON2:
> +		regmap_read(priv->regmap, MT6359_ZCD_CON2, &reg);
> +		priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTL] =
> +			(reg >> RG_AUDHPLGAIN_SFT) & RG_AUDHPLGAIN_MASK;
> +		priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTR] =
> +			(reg >> RG_AUDHPRGAIN_SFT) & RG_AUDHPRGAIN_MASK;
> +		break;

...then read the value that was set and store it elsewhere.  What's
going on here?

> +/*HP MUX */
> +static const char * const hp_in_mux_map[] = {
> +	"Open",
> +	"LoudSPK Playback",
> +	"Audio Playback",
> +	"Test Mode",
> +	"HP Impedance",
> +	"undefined1",
> +	"undefined2",
> +	"undefined3",
> +};

Why expose undefined (and presumably out of spec) values to userspace?

> +static int mt_clksq_event(struct snd_soc_dapm_widget *w,
> +			  struct snd_kcontrol *kcontrol,
> +			  int event)
> +{
> +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);
> +
> +	dev_dbg(priv->dev, "%s(), event = 0x%x\n", __func__, event);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		/* audio clk source from internal dcxo */
> +		regmap_update_bits(priv->regmap, MT6359_AUDENC_ANA_CON23,
> +				   RG_CLKSQ_IN_SEL_TEST_MASK_SFT,
> +				   0x0);

This also appeared to be controlled in _set_clkseq() - are we sure that
things couldn't get confused about the state?

> +	/* HP damp circuit enable */
> +	/*Enable HPRN/HPLN output 4K to VCM */

Spaces around the /* */

> +static int mt_hp_event(struct snd_soc_dapm_widget *w,
> +		       struct snd_kcontrol *kcontrol,
> +		       int event)
> +{
> +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);
> +	unsigned int mux = dapm_kcontrol_get_value(w->kcontrols[0]);
> +	int device = DEVICE_HP;
> +
> +	dev_dbg(priv->dev, "%s(), event 0x%x, dev_counter[DEV_HP] %d, mux %u\n",
> +		__func__, event, priv->dev_counter[device], mux);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		priv->dev_counter[device]++;
> +		if (priv->dev_counter[device] > 1)
> +			break;	/* already enabled, do nothing */
> +		else if (priv->dev_counter[device] <= 0)

Why are we doing additional refcounting on top of what DAPM is doing?
This seems like there should be at least one widget representing the
shared bits of the audio path.

> +#define MT6359_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
> +			SNDRV_PCM_FMTBIT_U16_LE | SNDRV_PCM_FMTBIT_U16_BE |\
> +			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\
> +			SNDRV_PCM_FMTBIT_U24_LE | SNDRV_PCM_FMTBIT_U24_BE |\
> +			SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE |\
> +			SNDRV_PCM_FMTBIT_U32_LE | SNDRV_PCM_FMTBIT_U32_BE)

The driver doesn't appear to configure anything except the sample rate -
how are all these formats supported?

> +	/* hp gain ctl default choose ZCD */
> +	priv->hp_gain_ctl = HP_GAIN_CTL_ZCD;
> +	hp_gain_ctl_select(priv, priv->hp_gain_ctl);

Why not use the hardware default?

> +	mt6359_codec_init_reg(cmpnt);
> +
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTL] = 8;
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTR] = 8;
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP1] = 3;
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP2] = 3;
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP3] = 3;

Same here.

> +	ret = regulator_enable(priv->avdd_reg);
> +	if (ret) {
> +		dev_err(priv->dev, "%s(), failed to enable regulator!\n",
> +			__func__);
> +		return ret;
> +	}

Perhaps make this a DAPM widget?

> +	priv->avdd_reg = devm_regulator_get(&pdev->dev, "vaud18");
> +	if (IS_ERR(priv->avdd_reg)) {
> +		dev_err(&pdev->dev, "%s(), have no vaud18 supply", __func__);
> +		return PTR_ERR(priv->avdd_reg);
> +	}

It's better to print error codes to help people debugging problems.

> +static const struct of_device_id mt6359_of_match[] = {
> +	{.compatible = "mediatek,mt6359-sound",},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mt6359_of_match);

We don't need a compatible here, we know that this device is here since
it's part of the parent device and isn't something that might appear in
another device.  This is reflecting the Linux driver model, not the
hardware.

--tctmm6wHVGT/P6vA
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAl8xmRQACgkQJNaLcl1U
h9AwIgf/ayU+/rwIHTkJ6VHfejmdeAG1IuGX9PJk/02XeL8eLs6JB9rj9SSoU2pO
G0QckxHzOBj3IgDORpJy47BJv7U2KM5LH5XctLsQGImG9/M+eZcQ/1lAymOqJIcx
PEzpnBRXrIwwJYwWw+mr3EZB0fqpDUW2mXiNQ7izPT2AVi7zb0DFXFNaptVHxXoc
EeL/LPmaj53Gla+Yzf7jgx7D3aVqD2jBvSbn7ZUdxC9UZ251BNUhgfp3qL0AoUZS
maoxcbO9NL3J9L03l9EGTOUY1lqIqV086dq9Li6SShSkhBa75Qi62p5ekPMqzNZo
iOIMD+8g6aPEauTaCnT4ozpXpwiPvA==
=J0i4
-----END PGP SIGNATURE-----

--tctmm6wHVGT/P6vA--