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

[Mark Brown <broonie@kernel.org>]

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

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.

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