LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Eason Yen <eason.yen@mediatek.com>
To: Mark Brown <broonie@kernel.org>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
	<jiaxin.yu@mediatek.com>, <linux-kernel@vger.kernel.org>,
	<linux-mediatek@lists.infradead.org>,
	<devicetree@vger.kernel.org>, <wsd_upstream@mediatek.com>
Subject: Re: [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver
Date: Thu, 12 Mar 2020 14:43:07 +0800	[thread overview]
Message-ID: <1583995387.19248.93.camel@mtkswgap22> (raw)
In-Reply-To: <20200311121232.GB5411@sirena.org.uk>

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.


  reply	other threads:[~2020-03-12  6:43 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-06  3:33 [PATCH 0/2] Add mediatek codec mt6359 driver Eason Yen
2020-03-06  3:33 ` [PATCH 1/2] ASoC: mediatek: mt6359: add codec document Eason Yen
2020-03-12 19:05   ` Rob Herring
2020-03-06  3:33 ` [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver Eason Yen
2020-03-09 13:13   ` Mark Brown
2020-03-11  9:22     ` Eason Yen
2020-03-11 12:12       ` Mark Brown
2020-03-12  6:43         ` Eason Yen [this message]
2020-03-12 19:08           ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1583995387.19248.93.camel@mtkswgap22 \
    --to=eason.yen@mediatek.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=jiaxin.yu@mediatek.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=wsd_upstream@mediatek.com \
    --subject='Re: [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).