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

[Jiaxin Yu <jiaxin.yu@mediatek.com>]

On Wed, 2020-08-12 at 13:05 +0100, Mark Brown wrote:
> On Wed, Aug 12, 2020 at 03:29:13PM +0800, Jiaxin Yu wrote:
> > 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);
> 
> > > 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?
> 
> These functions are exported for other drivers to use rather than
> something done internally by the driver - if this were internal to the
> driver it'd not be a big deal but when it's for use by another driver
> it'd be better to go through standard interfaces.
> 

Can we move this part of the operation to the codec dai driver ops, such
as .startup and .shutdown? Because originally these functions are
exported to machine driver's dai_link .startup and .shutdown ops.

> > > > +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.
> 
> So this needs the SoC to do something as part of callibration?
> 

Yes, the side of MTKAIF in SoC part named adda, we need config it before
calibration. We will also upstream machine/platform driver that use this
codec driver later.

> > > > +	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.
> 
> That sounds like you should just have one output path defined in DAPM
> and merge the left and right signals together rather than maintaining
> them separately.
> 

Yes, it's would better if they are combined into one mixer control that
named "HP Mux".

> > > > +	/* 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. 
> 
> Why not just let the user pick at runtime?
> 

For another adjustment method, we didn't upstream it, so only ZCD
reserved. And the hardware default setting is ZCD, so we can removed
these lines.

> > > > +	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.
> 
> If it is just supposed to be left on all the time it's better to do this
> in the main device model probe rather than the component probe.  The
> component probe should usually only be used for things that need the
> rest of the card to be there.

Ok, I will correct it.