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

[Jiaxin Yu]

On Mon, 2020-08-10 at 19:59 +0100, Mark Brown wrote:
> 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?

These are the functions that control the mux of input or output
signal. I refer to the other codec drivers, most of them are manipulate
the regs in their codec drivers also. Maybe it's easier to control?

> > +/* 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.
> 

Yes, I will refine them.

> > +int mt6359_mtkaif_calibration_enable(struct snd_soc_pcm_runtime *rtd)
> > +{
> 
> > +EXPORT_SYMBOL_GPL(mt6359_mtkaif_calibration_enable);
> 
> Why is this exported?
> 

This function is exported to machine driver to do calibration when
registering. The interface between MT6359 and MTK SoC is a special
interface that named MTKAIF. Therefore, if MT6359 is to be used
normally, it needs to rely on the platform for calibration when
registering.

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

/* Increase/Reduce HP aux feedback loop gain step by step */
This is to prevent sudden changes in volume.

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

Because we can adjust the volume at two points, before and during the
playback or capture. In addition, we will do volume ramp when opening
the driver. If we set the volume before opening the driver, we need get
the value to do volume ramp during opening the driver.

> > +/*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?
> 

Removed them.

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

_set_clkseq() will only be run at the time of mtkaif calibraiton, and
the calibration only run once when the codec driver probe. So it won't
get confused about the state. But it really only needs to be set up once
at mt6359_codec_init_reg(). I will remove the unnecesary settings.

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

Sorry, fixed it.

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

We have "HPL Mux" and "HPR Mux", there will be two paths enabled when
playback throuh HP. But actually they share the same control sequences.
So in order to prevent setting it one more time we do additional
refcouting.


> > +#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?
> 

The interface between MT6359 and MTK SoC is a special interface that
named MTKAIF. Because of its unique hardware settings, it really doesn't
to set format argument. The capability of pcm driver mainly depends on
cpu-dai driver to limit.

> > +	/* 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?
> 

We have two ways to control the volume, there is to select ZCD. Because
the other one it not often used, ZCD is used by 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.
> 

Removed them.

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

"vaud18" needs to be always on so we enable it when codec probe.

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

Yes, I will print the error codes.

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

Removed them.