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.
WARNING: multiple messages have this Message-ID (diff)
From: Eason Yen <eason.yen@mediatek.com> To: Mark Brown <broonie@kernel.org> Cc: devicetree@vger.kernel.org, wsd_upstream@mediatek.com, linux-kernel@vger.kernel.org, jiaxin.yu@mediatek.com, linux-mediatek@lists.infradead.org, Matthias Brugger <matthias.bgg@gmail.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. _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek
next prev parent reply other threads:[~2020-03-12 6:43 UTC|newest] Thread overview: 18+ 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 ` Eason Yen 2020-03-06 3:33 ` [PATCH 1/2] ASoC: mediatek: mt6359: add codec document Eason Yen 2020-03-06 3:33 ` Eason Yen 2020-03-12 19:05 ` Rob Herring 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-06 3:33 ` Eason Yen 2020-03-09 13:13 ` Mark Brown 2020-03-09 13:13 ` Mark Brown 2020-03-11 9:22 ` Eason Yen 2020-03-11 9:22 ` Eason Yen 2020-03-11 12:12 ` Mark Brown 2020-03-11 12:12 ` Mark Brown 2020-03-12 6:43 ` Eason Yen [this message] 2020-03-12 6:43 ` Eason Yen 2020-03-12 19:08 ` Mark Brown 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 \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.