LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Eason Yen <eason.yen@mediatek.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
	jiaxin.yu@mediatek.com, linux-kernel@vger.kernel.org,
	linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org,
	wsd_upstream@mediatek.com
Subject: Re: [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver
Date: Mon, 9 Mar 2020 13:13:46 +0000	[thread overview]
Message-ID: <20200309131346.GF4101@sirena.org.uk> (raw)
In-Reply-To: <1583465622-16628-3-git-send-email-eason.yen@mediatek.com>

[-- Attachment #1: Type: text/plain, Size: 6497 bytes --]

On Fri, Mar 06, 2020 at 11:33:42AM +0800, Eason Yen wrote:

> +static void capture_gpio_reset(struct mt6359_priv *priv)
> +{
> +	/* set pad_aud_*_miso to GPIO mode and dir input
> +	 * reason:
> +	 * pad_aud_clk_miso, because when playback only the miso_clk
> +	 * will also have 26m, so will have power leak
> +	 * pad_aud_dat_miso*, because the pin is used as boot strap
> +	 */

This looks like things that might be better exposed via pinctrl and
gpiolib for board specific configuration - what exactly are these GPIOs
doing?  A lot of this code looks like it might be board specific.

> +/* use only when not govern by DAPM */
> +static int mt6359_set_dcxo(struct mt6359_priv *priv, bool enable)
> +{

Why might this sometimes be controlled outside of DAPM?

> +/* reg idx for -40dB*/
> +#define PGA_MINUS_40_DB_REG_VAL 0x1f
> +#define HP_PGA_MINUS_40_DB_REG_VAL 0x3f
> +static const char *const dl_pga_gain[] = {
> +	"8Db", "7Db", "6Db", "5Db", "4Db",
> +	"3Db", "2Db", "1Db", "0Db", "-1Db",
> +	"-2Db", "-3Db",	"-4Db", "-5Db", "-6Db",
> +	"-7Db", "-8Db", "-9Db", "-10Db", "-40Db"
> +};
> +
> +static const char *const hp_dl_pga_gain[] = {
> +	"8Db", "7Db", "6Db", "5Db", "4Db",
> +	"3Db", "2Db", "1Db", "0Db", "-1Db",
> +	"-2Db", "-3Db",	"-4Db", "-5Db", "-6Db",
> +	"-7Db", "-8Db", "-9Db", "-10Db", "-11Db",
> +	"-12Db", "-13Db", "-14Db", "-15Db", "-16Db",
> +	"-17Db", "-18Db", "-19Db", "-20Db", "-21Db",
> +	"-22Db", "-40Db"
> +};

I can't see any users of these tables in the driver?  That's good since
the driver should be translating these into TLV controls rather than
using enums but these variables aren't used then so should be removed.

> +
> +	if (!is_valid_hp_pga_idx(from) || !is_valid_hp_pga_idx(to))
> +		dev_warn(priv->dev, "%s(), volume index is not valid, from %d, to %d\n",
> +			 __func__, from, to);

Shouldn't we return an error then?

> +
> +	dev_info(priv->dev, "%s(), from %d, to %d\n",
> +		 __func__, from, to);

At most this should be a dev_dbg(), having a dev_info() log is going to
be far too verbose.  There's a lot of these in the driver.

> +static const char *const mic_type_mux_map[] = {
> +	"Idle",
> +	"ACC",
> +	"DMIC",
> +	"DCC",
> +	"DCC_ECM_DIFF",
> +	"DCC_ECM_SINGLE",
> +	"VOW_ACC",
> +	"VOW_DMIC",
> +	"VOW_DMIC_LP",
> +	"VOW_DCC",
> +	"VOW_DCC_ECM_DIFF",
> +	"VOW_DCC_ECM_SINGLE"
> +};

This looks like something that should be being set by DT or other
platform data rather than at runtime - we're not likely to change from a
digital to analogue microphone at runtime for example.

> +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;
> +
> +	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;

It's unclear what's going on with this custom function.  As far as I can
see all it's doing is taking a copy of the gain setting for later use by
rampig functions but since we're immediately writing the set value into
the register map surely we don't need that as we can just read the value
back from the register?

> +static const struct snd_kcontrol_new mt6359_snd_controls[] = {
> +	/* dl pga gain */
> +	SOC_SINGLE_EXT_TLV("HeadsetL Volume",
> +			   MT6359_ZCD_CON2, 0, 0x1E, 0,
> +			   snd_soc_get_volsw, mt6359_put_volsw,
> +			   hp_playback_tlv),
> +	SOC_SINGLE_EXT_TLV("HeadsetR Volume",
> +			   MT6359_ZCD_CON2, 7, 0x1E, 0,
> +			   snd_soc_get_volsw, mt6359_put_volsw,
> +			   hp_playback_tlv),
> +	SOC_SINGLE_EXT_TLV("LineoutL Volume",
> +			   MT6359_ZCD_CON1, 0, 0x12, 0,
> +			   snd_soc_get_volsw, mt6359_put_volsw, playback_tlv),
> +	SOC_SINGLE_EXT_TLV("LineoutR Volume",
> +			   MT6359_ZCD_CON1, 7, 0x12, 0,
> +			   snd_soc_get_volsw, mt6359_put_volsw, playback_tlv),

These should be stereo controls not pairs of mono controls.

> +	/* ul pga gain */
> +	SOC_SINGLE_EXT_TLV("PGAL Volume",
> +			   MT6359_AUDENC_ANA_CON0, RG_AUDPREAMPLGAIN_SFT, 4, 0,
> +			   snd_soc_get_volsw, mt6359_put_volsw, capture_tlv),
> +	SOC_SINGLE_EXT_TLV("PGAR Volume",
> +			   MT6359_AUDENC_ANA_CON1, RG_AUDPREAMPRGAIN_SFT, 4, 0,
> +			   snd_soc_get_volsw, mt6359_put_volsw, capture_tlv),

Same here.

> +static const char * const lo_in_mux_map[] = {
> +	"Open", "Playback_L_DAC", "Playback", "Test Mode"
> +};
> +
> +static int lo_in_mux_map_value[] = {
> +	0x0, 0x1, 0x2, 0x3,
> +};

Why use a value enum here, a normal mux should be fine?

> +static int mt_delay_250_event(struct snd_soc_dapm_widget *w,
> +			      struct snd_kcontrol *kcontrol,
> +			      int event)
> +{
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +	case SND_SOC_DAPM_PRE_PMD:
> +		usleep_range(250, 270);

Why would having a sleep before power down be useful?

> +static int mt6359_codec_probe(struct snd_soc_component *cmpnt)
> +{
> +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);
> +	int ret;
> +
> +	snd_soc_component_init_regmap(cmpnt, priv->regmap);
> +
> +	snd_soc_add_component_controls(cmpnt,
> +				       mt6359_snd_vow_controls,
> +				       ARRAY_SIZE(mt6359_snd_vow_controls));

Use the controls member of the component driver struct.

> +	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;

These should be left at the hardware defaults.

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

The driver should request resources during device model probe rather
than component probe.

> +	ret = regulator_enable(priv->avdd_reg);
> +	if (ret)
> +		return ret;
> +

There's nothing to disable this on remove.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2020-03-09 13:13 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-06  3:33 [PATCH 0/2] Add mediatek codec mt6359 driver Eason Yen
2020-03-06  3:33 ` [PATCH 1/2] ASoC: mediatek: mt6359: add codec document Eason Yen
2020-03-12 19:05   ` Rob Herring
2020-03-06  3:33 ` [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver Eason Yen
2020-03-09 13:13   ` Mark Brown [this message]
2020-03-11  9:22     ` Eason Yen
2020-03-11 12:12       ` Mark Brown
2020-03-12  6:43         ` Eason Yen
2020-03-12 19:08           ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200309131346.GF4101@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=eason.yen@mediatek.com \
    --cc=jiaxin.yu@mediatek.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=wsd_upstream@mediatek.com \
    --subject='Re: [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).