LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Eason Yen <eason.yen@mediatek.com>
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: Wed, 11 Mar 2020 12:12:32 +0000	[thread overview]
Message-ID: <20200311121232.GB5411@sirena.org.uk> (raw)
In-Reply-To: <1583918544.19248.69.camel@mtkswgap22>

[-- 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 --]

  reply	other threads:[~2020-03-11 12:12 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 [this message]
2020-03-12  6:43         ` Eason Yen
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=20200311121232.GB5411@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=eason.yen@mediatek.com \
    --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).