Re: [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver

[Mark Brown <broonie@kernel.org>]

[-- 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 --]