| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1583995387.19248.93.camel@mtkswgap22>
Date: Thu, 12 Mar 2020 14:43:07 +0800
From: Eason Yen <eason.yen@...iatek.com>
To: Mark Brown <broonie@...nel.org>
CC: Matthias Brugger <matthias.bgg@...il.com>,
<jiaxin.yu@...iatek.com>, <linux-kernel@...r.kernel.org>,
<linux-mediatek@...ts.infradead.org>, <devicetree@...r.kernel.org>,
<wsd_upstream@...iatek.com>
Subject: Re: [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver
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.
Powered by blists - more mailing lists