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

[Eason Yen]

Dear Mark,

	Thanks for your viewing.

On Wed, 2020-03-11 at 12:12 +0000, Mark Brown wrote:
> On Wed, Mar 11, 2020 at 05:22:24PM +0800, Eason Yen wrote:
> > On Mon, 2020-03-09 at 13:13 +0000, Mark Brown wrote:
> > > On Fri, Mar 06, 2020 at 11:33:42AM +0800, Eason Yen wrote:
> 
> > > 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.
> 
> > MT6359 has some gpios (pad_aud_*) for downlink/uplink.
> > And these pins is connected between AP part and PMIC part.
> > (1) For AP part, user need to set gpio pinmux for these gpio using DT.
> > (2) For pmic part, gpio are configured at codec driver by default.
> 
> > For PMIC part, user need to set in these register :
> > GPIO_MODE1/GPIO_MODE2/GPIO_MODE3
> 
> > The following functions are used to set:
> > - playback_gpio_set/playback_gpio_reset
> > - capture_gpio_set/capture_gpio_reset
> > - vow_gpio_set/vow_gpio_reset
> 
> This sounds like it should be handled at the machine driver level, it's
> possible some system integrator will wire things up differently.
> 

machine driver will set default at booting stage to execute
mt6359_mtkaif_calibration_enable and mt6359_mtkaif_calibration_disable.

And at runtime stage, it is triggered by mt_dl_gpio_event and
mt_ul_gpio_event while playback or capture.


> > > > +/* 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?
> 
> > mt6359_set_dcxo is used at mt6359_mtkaif_calibration_enable/disable.
> 
> > mtkaif_calibration process needs be completed at booting stage once.
> > And it has a specific control sequence provided by codec designer.
> > So it need to be controlled outside of DAPM.
> 
> OK, this should explicitly say that this is for use during calibration
> then.
> 
> > > > +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.
> 
> > For mic1, it's mic_type can set one of mic_type_mux_map[] at different
> > scenario.
> > (1) When mic1 is not used, it should be set as "Idle"
> > (2) When mic1 is ACC mode and used at normal capture scenario, it should
> > be set as "ACC"
> > (3) When mic1 is ACC mode and used at Voice Wakeup scenario, it should
> > be set as "VOW_ACC"
> 
> That still doesn't mean you should have control over things like if the
> microphone is single ended or differential at runtime.  This at least
> needs to be a higher level control, it should be integrated with both
> board data and DAPM.  You can at least select idle mode with DAPM, and
> you may be able to select voice wakeup that way too (by looking at where
> things are routed).
> 

OK. So it is better to fix mic_type (ACC/DMIC/DCC/DCC_*) at init stage
because it will not be changed at runtime.

And use another dpam mux or kcontrol to enable/disable vow for low power
scenario.

Is it right?

> > > > +	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.
> 
> > It is more flexible for customization.
> 
> > For example, customer may use lineout path for stereo speaker amp.
> > And for specific amp, it need different gain on channel L and channel R.
> 
> You can set the gains of stereo pairs independently, that's not a
> problem.
> 
> > > > +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?
> > > 
> 
> > Could I modify as follow:
> 
> > /* LOL MUX */
> > enum {
> > 	LO_MUX_OPEN = 0,
> > 	LO_MUX_L_DAC,
> > 	LO_MUX_3RD_DAC,
> > 	LO_MUX_TEST_MODE,
> > 	LO_MUX_MASK = 0x3,
> > };
> 
> > static const char * const lo_in_mux_map[] = {
> > 	"Open", "Playback_L_DAC", "Playback", "Test Mode"
> > };
> 
> > static int lo_in_mux_map_value[] = {
> > 	LO_MUX_OPEN,
> > 	LO_MUX_L_DAC,
> > 	LO_MUX_3RD_DAC,
> > 	LO_MUX_TEST_MODE,
> > };
> 
> Why bother with the value mapping at all?
> 

ok, I will refine it as follow. is it ok?

And remove 
/* remove it
static int lo_in_mux_map_value[] = {
	0x0, 0x1, 0x2, 0x3,
};
*/

enum {
	LO_MUX_OPEN = 0,
	LO_MUX_L_DAC,
	LO_MUX_3RD_DAC,
	LO_MUX_TEST_MODE,
	LO_MUX_MASK = 0x3,
};

static const char * const lo_in_mux_map[] = {
	"Open", "Playback_L_DAC", "Playback", "Test Mode"
};

static SOC_ENUM_SINGLE_DECL(lo_in_mux_map_enum,
			    SND_SOC_NOPM, 0, lo_in_mux_map);

static const struct snd_kcontrol_new lo_in_mux_control =
	SOC_DAPM_ENUM("LO Select", lo_in_mux_map_enum);


The refine will apply on RCV MUX and HP MUX ,too.


> > > > +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?
> 
> > It is based on designer's control sequence to add some delay while
> > PMU/PMD.
> 
> But how does the designer know when the sequence starts?  Don't they
> mean to have a delay *after* power down?
> 

For PMU, designer think 
"AUD_CK" --> wait at least 250ms --> "AUDIF_CK" --> next ...

For PMD, designer think 
"AUDIF_CK" --> wait at least 250ms --> "AUD_CK" --> next ...

	SND_SOC_DAPM_SUPPLY_S("ZCD13M_CK", SUPPLY_SEQ_TOP_CK,
			      MT6359_AUD_TOP_CKPDN_CON0,
			      RG_ZCD13M_CK_PDN_SFT, 1, NULL, 0),

	SND_SOC_DAPM_SUPPLY_S("AUD_CK", SUPPLY_SEQ_TOP_CK_LAST,
			      MT6359_AUD_TOP_CKPDN_CON0,
			      RG_AUD_CK_PDN_SFT, 1,
			      mt_delay_250_event,
			      SND_SOC_DAPM_POST_PMU | 	SND_SOC_DAPM_PRE_PMD),

	SND_SOC_DAPM_SUPPLY_S("AUDIF_CK", SUPPLY_SEQ_TOP_CK,
			      MT6359_AUD_TOP_CKPDN_CON0,
			      RG_AUDIF_CK_PDN_SFT, 1, NULL, 0),

So I add a mt_delay_250_event while "AUD_CK" POST_PMU and PRE_PMD.


> > > > +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.
> 
> > Do you mean that I should merge mt6359_snd_vow_controls into
> > mt6359_snd_controls?
> 
> Yes, you're unconditionally registering these so there's no sense in
> splitting them.
> 
> > > > +	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.
> 
> > Do you mean that it need be requested at mt6359_platform_driver_probe()
> > instead of mt6359_codec_probe() ?
> 
> Yes.
> 
> > > > +	ret = regulator_enable(priv->avdd_reg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> 
> > > There's nothing to disable this on remove.
> 
> > Do you mean that I should add a remove function to execute
> > regulator_disable()?
> 
> Yes.