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

[Mark Brown <broonie@kernel.org>]

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

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.

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

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

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

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

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