Add mediatek codec mt6359 driver
Subsystems (4)
cover Add mediatek codec mt6359 driver
Add mediatek codec (MT6359) driver
MT6359 support playback, capture and vow feature.
On downlink path, it includes three DACs for handset, headset, and lineout path. On unlink path, it includeds three ADCs for main mic, second mic, 3rd mic, and headset mic.
By scenario, select *_MUX widget to create damp path. And by select mic_type_mux to choose DMIC/AMIC/....
For example, select these MUX widget to create headset path
(1) DAC In Mux --> "Normal Path"
(2) HPL Mux --> "Audio Playback"
(3) HPR Mux --> "Audio Playback"
Eason Yen (2):
ASoC: mediatek: mt6359: add codec document
ASoC: codec: mediatek: add mt6359 codec driver
Documentation/devicetree/bindings/sound/mt6359.txt | 16 + sound/soc/codecs/Kconfig | 9 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/mt6359.c | 4217 ++++++++++++++++++++ sound/soc/codecs/mt6359.h | 3212 +++++++++++++++ 5 files changed, 7456 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/mt6359.txt create mode 100644 sound/soc/codecs/mt6359.c create mode 100644 sound/soc/codecs/mt6359.h
Signature · 1 line
1.9.1
PATCH 1/2 ASoC: mediatek: mt6359: add codec document
Add mt6359 codec document
--- Documentation/devicetree/bindings/sound/mt6359.txt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/mt6359.txt
Documentation/devicetree/bindings/sound/mt6359.txt new file +16 -0
@@ -0,0 +1,16 @@ +Mediatek MT6359 Audio Codec + +The communication between MT6358 and SoC is through Mediatek PMIC wrapper. +For more detail, please visit Mediatek PMIC wrapper documentation. + +Must be a child node of PMIC wrapper. + +Required properties: + +- compatible : "mediatek,mt6359-sound". + +Example: + +mt6359_snd { + compatible = "mediatek,mt6359-sound"; +};
Signature · 1 line
1.9.1
Rob Herring
maintainerRe: [PATCH 1/2] ASoC: mediatek: mt6359: add codec document
Quoted 7 lines · Eason Yen
Add mt6359 codec document
Signed-off-by Eason Yen <[email protected]>--- Documentation/devicetree/bindings/sound/mt6359.txt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/mt6359.txt
Please convert to a DT schema.
Quoted 7 lines
Documentation/devicetree/bindings/sound/mt6359.txt new file +0
-0
Documentation/devicetree/bindings/sound/mt6359.txt line 10
Quoted 10 lines
Documentation/devicetree/bindings/sound/mt6359.txt new file +10
-0@@ -0,0 +1,16 @@ +Mediatek MT6359 Audio Codec + +The communication between MT6358 and SoC is through Mediatek PMIC wrapper. +For more detail, please visit Mediatek PMIC wrapper documentation. + +Must be a child node of PMIC wrapper. + +Required properties: + +- compatible : "mediatek,mt6359-sound".
A compatible with no other properties is generally suspect. Don't you have clocks, connection to I2S or something, etc.
Documentation/devicetree/bindings/sound/mt6359.txt line 14
Quoted 4 lines
Documentation/devicetree/bindings/sound/mt6359.txt new file +4
-0+ +Example: + +mt6359_snd {
audio-codec {
Quoted 4 lines
Documentation/devicetree/bindings/sound/mt6359.txt new file +2
-0+ compatible = "mediatek,mt6359-sound"; +};Signature · 1 line
1.9.1
PATCH 2/2 ASoC: codec: mediatek: add mt6359 codec driver
In reply to Eason YenAdd mt6359 codec driver
--- sound/soc/codecs/Kconfig | 9 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/mt6359.c | 4217 +++++++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/mt6359.h | 3212 ++++++++++++++++++++++++++++++++++ 4 files changed, 7440 insertions(+) create mode 100644 sound/soc/codecs/mt6359.c create mode 100644 sound/soc/codecs/mt6359.h
sound/soc/codecs/Kconfig +9 -0
@@ -125,6 +125,7 @@ config SND_SOC_ALL_CODECS imply SND_SOC_ML26124 imply SND_SOC_MT6351 imply SND_SOC_MT6358 + imply SND_SOC_MT6359 imply SND_SOC_MT6660 imply SND_SOC_NAU8540 imply SND_SOC_NAU8810
@@ -1681,6 +1682,14 @@ config SND_SOC_MT6358 Enable support for the platform which uses MT6358 as external codec device. +config SND_SOC_MT6359 + tristate "MediaTek MT6359 Codec driver" + help + MediaTek MT6359 is a codec which contain + three ADC for uplink path(MICs), and + three DAC for downlink path(handset, + headset, lineout). + config SND_SOC_MT6660 tristate "Mediatek MT6660 Speaker Amplifier" depends on I2C
sound/soc/codecs/Makefile +2 -0
@@ -123,6 +123,7 @@ snd-soc-msm8916-analog-objs := msm8916-wcd-analog.o snd-soc-msm8916-digital-objs := msm8916-wcd-digital.o snd-soc-mt6351-objs := mt6351.o snd-soc-mt6358-objs := mt6358.o +snd-soc-mt6359-objs := mt6359.o snd-soc-mt6660-objs := mt6660.o snd-soc-nau8540-objs := nau8540.o snd-soc-nau8810-objs := nau8810.o
@@ -423,6 +424,7 @@ obj-$(CONFIG_SND_SOC_MSM8916_WCD_ANALOG) +=snd-soc-msm8916-analog.o obj-$(CONFIG_SND_SOC_MSM8916_WCD_DIGITAL) +=snd-soc-msm8916-digital.o obj-$(CONFIG_SND_SOC_MT6351) += snd-soc-mt6351.o obj-$(CONFIG_SND_SOC_MT6358) += snd-soc-mt6358.o +obj-$(CONFIG_SND_SOC_MT6359) += snd-soc-mt6359.o obj-$(CONFIG_SND_SOC_MT6660) += snd-soc-mt6660.o obj-$(CONFIG_SND_SOC_NAU8540) += snd-soc-nau8540.o obj-$(CONFIG_SND_SOC_NAU8810) += snd-soc-nau8810.o
sound/soc/codecs/mt6359.c new file +4217 -0
2,965 lines not on this page · raw message
sound/soc/codecs/mt6359.h new file +3212 -0
3,212 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Signature · 5 lines
1.9.1 _______________________________________________ Linux-mediatek mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-mediatek
Part of the diff is not shown here. View the raw message.
Mark Brown
maintainerRe: [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver
sound/soc/codecs/mt6359.c line 279
Quoted 8 lines · Eason Yen
sound/soc/codecs/mt6359.c new file +8
-0+static void capture_gpio_reset (struct mt6359_priv *priv) +{ +/* set pad_aud_*_miso to GPIO mode and dir input +* reason: +* pad_aud_clk_miso, because when playback only the miso_clk +* will also have 26m, so will have power leak +* pad_aud_dat_miso*, because the pin is used as boot strap +*/
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.
sound/soc/codecs/mt6359.c line 332
Quoted 3 lines
sound/soc/codecs/mt6359.c new file +3
-0+/* 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?
Quoted 9 lines
sound/soc/codecs/mt6359.c new file +9
-0+/* reg idx for -40dB*/ +#define PGA_MINUS_40_DB_REG_VAL 0x1f +#define HP_PGA_MINUS_40_DB_REG_VAL 0x3f +static const char *const dl_pga_gain[] = { +"8Db" ,"7Db" ,"6Db" ,"5Db" ,"4Db" , +"3Db" ,"2Db" ,"1Db" ,"0Db" ,"-1Db" , +"-2Db" ,"-3Db" ,"-4Db" ,"-5Db" ,"-6Db" , +"-7Db" ,"-8Db" ,"-9Db" ,"-10Db" ,"-40Db" +};
sound/soc/codecs/mt6359.c line 533
Quoted 10 lines
sound/soc/codecs/mt6359.c new file +10
-0+ +static const char *const hp_dl_pga_gain[] = { +"8Db" ,"7Db" ,"6Db" ,"5Db" ,"4Db" , +"3Db" ,"2Db" ,"1Db" ,"0Db" ,"-1Db" , +"-2Db" ,"-3Db" ,"-4Db" ,"-5Db" ,"-6Db" , +"-7Db" ,"-8Db" ,"-9Db" ,"-10Db" ,"-11Db" , +"-12Db" ,"-13Db" ,"-14Db" ,"-15Db" ,"-16Db" , +"-17Db" ,"-18Db" ,"-19Db" ,"-20Db" ,"-21Db" , +"-22Db" ,"-40Db" +};
I can't see any users of these tables in the driver? That's good since the driver should be translating these into TLV controls rather than using enums but these variables aren't used then so should be removed.
sound/soc/codecs/mt6359.c line 643
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+ +if (!is_valid_hp_pga_idx (from) || !is_valid_hp_pga_idx (to)) +dev_warn (priv->dev,"%s(), volume index is not valid, from %d, to %d\n" , + __func__, from, to);
Shouldn't we return an error then?
sound/soc/codecs/mt6359.c line 646
Quoted 3 lines
sound/soc/codecs/mt6359.c new file +3
-0+ +dev_info (priv->dev,"%s(), from %d, to %d\n" , + __func__, from, to);
At most this should be a dev_dbg(), having a dev_info() log is going to be far too verbose. There's a lot of these in the driver.
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+static const char *const mic_type_mux_map[] = { +"Idle" , +"ACC" , +"DMIC" ,
sound/soc/codecs/mt6359.c line 722
Quoted 10 lines
sound/soc/codecs/mt6359.c new file +10
-0+"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.
Quoted 14 lines
sound/soc/codecs/mt6359.c new file +14
-0+static int mt6359_put_volsw (struct snd_kcontrol *kcontrol, +struct snd_ctl_elem_value *ucontrol) +{ +struct snd_soc_component *component = +snd_soc_kcontrol_component (kcontrol); +struct mt6359_priv *priv =snd_soc_component_get_drvdata (component); +struct soc_mixer_control *mc = + (struct soc_mixer_control *)kcontrol->private_value; +unsigned int reg; +int index = ucontrol->value.integer.value[0 ]; +int ret; + + ret =snd_soc_put_volsw (kcontrol, ucontrol); +if (ret <0 )
sound/soc/codecs/mt6359.c line 778
Quoted 10 lines
sound/soc/codecs/mt6359.c new file +10
-0+return ret; + +switch (mc->reg) { +case MT6359_ZCD_CON2 : +regmap_read (priv->regmap,MT6359_ZCD_CON2 , ®); + priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTL ] = + (reg >>RG_AUDHPLGAIN_SFT ) &RG_AUDHPLGAIN_MASK ; + priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTR ] = + (reg >>RG_AUDHPRGAIN_SFT ) &RG_AUDHPRGAIN_MASK ; +break ;
It's unclear what's going on with this custom function. As far as I can see all it's doing is taking a copy of the gain setting for later use by rampig functions but since we're immediately writing the set value into the register map surely we don't need that as we can just read the value back from the register?
Quoted 6 lines
sound/soc/codecs/mt6359.c new file +6
-0+static const struct snd_kcontrol_new mt6359_snd_controls[] = { +/* dl pga gain */ +SOC_SINGLE_EXT_TLV ("HeadsetL Volume" , +MT6359_ZCD_CON2 ,0 ,0x1E ,0 , + snd_soc_get_volsw, mt6359_put_volsw, + hp_playback_tlv),
sound/soc/codecs/mt6359.c line 834
Quoted 10 lines
sound/soc/codecs/mt6359.c new file +10
-0+SOC_SINGLE_EXT_TLV ("HeadsetR Volume" , +MT6359_ZCD_CON2 ,7 ,0x1E ,0 , + snd_soc_get_volsw, mt6359_put_volsw, + hp_playback_tlv), +SOC_SINGLE_EXT_TLV ("LineoutL Volume" , +MT6359_ZCD_CON1 ,0 ,0x12 ,0 , + snd_soc_get_volsw, mt6359_put_volsw, playback_tlv), +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.
sound/soc/codecs/mt6359.c line 845
Quoted 7 lines
sound/soc/codecs/mt6359.c new file +7
-0+/* ul pga gain */ +SOC_SINGLE_EXT_TLV ("PGAL Volume" , +MT6359_AUDENC_ANA_CON0 ,RG_AUDPREAMPLGAIN_SFT ,4 ,0 , + snd_soc_get_volsw, mt6359_put_volsw, capture_tlv), +SOC_SINGLE_EXT_TLV ("PGAR Volume" , +MT6359_AUDENC_ANA_CON1 ,RG_AUDPREAMPRGAIN_SFT ,4 ,0 , + snd_soc_get_volsw, mt6359_put_volsw, capture_tlv),
Same here.
sound/soc/codecs/mt6359.c line 879
Quoted 7 lines
sound/soc/codecs/mt6359.c new file +7
-0+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?
sound/soc/codecs/mt6359.c line 2880
Quoted 8 lines
sound/soc/codecs/mt6359.c new file +8
-0+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?
sound/soc/codecs/mt6359.c line 4132
Quoted 10 lines
sound/soc/codecs/mt6359.c new file +10
-0+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.
sound/soc/codecs/mt6359.c line 4140
Quoted 5 lines
sound/soc/codecs/mt6359.c new file +5
-0+ priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTL ] =8 ; + priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTR ] =8 ; + priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP1 ] =3 ; + priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP2 ] =3 ; + priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP3 ] =3 ;
These should be left at the hardware defaults.
sound/soc/codecs/mt6359.c line 4146
Quoted 5 lines
sound/soc/codecs/mt6359.c new file +5
-0+ 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.
sound/soc/codecs/mt6359.c line 4151
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+ ret =regulator_enable (priv->avdd_reg); +if (ret) +return ret; +
There's nothing to disable this on remove.
Re: [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver
Dear Mark,
Thanks for your attentive review.
The following is my feedback.
Please kindly give suggestion, thanks.
Quoted 15 lines · Mark Brown
sound/soc/codecs/mt6359.c line 279
Quoted 8 lines · Eason Yen
sound/soc/codecs/mt6359.c new file +8
-0+static void capture_gpio_reset (struct mt6359_priv *priv) +{ +/* set pad_aud_*_miso to GPIO mode and dir input +* reason: +* pad_aud_clk_miso, because when playback only the miso_clk +* will also have 26m, so will have power leak +* pad_aud_dat_miso*, because the pin is used as boot strap +*/ 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
Quoted 6 lines
sound/soc/codecs/mt6359.c line 332
Quoted 3 lines
sound/soc/codecs/mt6359.c new file +3
-0+/* 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.
Quoted 15 lines
Quoted 15 lines
sound/soc/codecs/mt6359.c new file +15
-0+/* reg idx for -40dB*/ +#define PGA_MINUS_40_DB_REG_VAL 0x1f +#define HP_PGA_MINUS_40_DB_REG_VAL 0x3f +static const char *const dl_pga_gain[] = { +"8Db" ,"7Db" ,"6Db" ,"5Db" ,"4Db" , +"3Db" ,"2Db" ,"1Db" ,"0Db" ,"-1Db" , +"-2Db" ,"-3Db" ,"-4Db" ,"-5Db" ,"-6Db" , +"-7Db" ,"-8Db" ,"-9Db" ,"-10Db" ,"-40Db" +}; + +static const char *const hp_dl_pga_gain[] = { +"8Db" ,"7Db" ,"6Db" ,"5Db" ,"4Db" , +"3Db" ,"2Db" ,"1Db" ,"0Db" ,"-1Db" , +"-2Db" ,"-3Db" ,"-4Db" ,"-5Db" ,"-6Db" , +"-7Db" ,"-8Db" ,"-9Db" ,"-10Db" ,"-11Db" ,
Quoted 9 lines
sound/soc/codecs/mt6359.c line 533
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+"-12Db" ,"-13Db" ,"-14Db" ,"-15Db" ,"-16Db" , +"-17Db" ,"-18Db" ,"-19Db" ,"-20Db" ,"-21Db" , +"-22Db" ,"-40Db" +};I can't see any users of these tables in the driver? That's good since the driver should be translating these into TLV controls rather than using enums but these variables aren't used then so should be removed.
Yes, you are right. These enum are useless and I will remove it at next patch.
Quoted 7 lines
sound/soc/codecs/mt6359.c line 643
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+ +if (!is_valid_hp_pga_idx (from) || !is_valid_hp_pga_idx (to)) +dev_warn (priv->dev,"%s(), volume index is not valid, from %d, to %d\n" , + __func__, from, to);Shouldn't we return an error then?
Yes, you are right. I will add error handling in headset_volume_ramp() at next patch.
Quoted 7 lines
sound/soc/codecs/mt6359.c line 646
Quoted 3 lines
sound/soc/codecs/mt6359.c new file +3
-0+ +dev_info (priv->dev,"%s(), from %d, to %d\n" , + __func__, from, to);At most this should be a
dev_dbg(), having adev_info()log is going to be far too verbose. There's a lot of these in the driver.
ok, I will replace dev_info with dev_dgb for these degug logs.
Quoted 10 lines
Quoted 10 lines
sound/soc/codecs/mt6359.c new file +10
-0+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" ,
Quoted 9 lines
sound/soc/codecs/mt6359.c line 722
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+"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"
So mic's type may runtime changed by user scenario. And with different mic type, setting in mt_mic_bias_*_event() is also different.
Quoted 22 lines
Quoted 22 lines
sound/soc/codecs/mt6359.c new file +22
-0+static int mt6359_put_volsw (struct snd_kcontrol *kcontrol, +struct snd_ctl_elem_value *ucontrol) +{ +struct snd_soc_component *component = +snd_soc_kcontrol_component (kcontrol); +struct mt6359_priv *priv =snd_soc_component_get_drvdata (component); +struct soc_mixer_control *mc = + (struct soc_mixer_control *)kcontrol->private_value; +unsigned int reg; +int index = ucontrol->value.integer.value[0 ]; +int ret; + + ret =snd_soc_put_volsw (kcontrol, ucontrol); +if (ret <0 ) +return ret; + +switch (mc->reg) { +case MT6359_ZCD_CON2 : +regmap_read (priv->regmap,MT6359_ZCD_CON2 , ®); + priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTL ] = + (reg >>RG_AUDHPLGAIN_SFT ) &RG_AUDHPLGAIN_MASK ; + priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTR] =
Quoted 9 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ (reg >> RG_AUDHPRGAIN_SFT) & RG_AUDHPRGAIN_MASK; + break;It's unclear what's going on with this custom function. As far as I can see all it's doing is taking a copy of the gain setting for later use by rampig functions but since we're immediately writing the set value into the register map surely we don't need that as we can just read the value back from the register?
Yes, it saves gain setting at register map, and also save at priv->ana_gain[]. When device(headset/receiver/lineout) is enable or disable, it can take priv->ana_gain[] for gain ramping instead of reading back from register again.
It is referenced another codec driver(mt6358.c).
Quoted 10 lines
Quoted 10 lines
sound/soc/codecs/mt6359.c new file +10
-0+static const struct snd_kcontrol_new mt6359_snd_controls[] = { +/* dl pga gain */ +SOC_SINGLE_EXT_TLV ("HeadsetL Volume" , +MT6359_ZCD_CON2 ,0 ,0x1E ,0 , + snd_soc_get_volsw, mt6359_put_volsw, + hp_playback_tlv), +SOC_SINGLE_EXT_TLV ("HeadsetR Volume" , +MT6359_ZCD_CON2 ,7 ,0x1E ,0 , + snd_soc_get_volsw, mt6359_put_volsw, + hp_playback_tlv),
Quoted 9 lines
sound/soc/codecs/mt6359.c line 834
Quoted 6 lines
sound/soc/codecs/mt6359.c new file +6
-0+SOC_SINGLE_EXT_TLV ("LineoutL Volume" , +MT6359_ZCD_CON1 ,0 ,0x12 ,0 , + snd_soc_get_volsw, mt6359_put_volsw, playback_tlv), +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.
Quoted 9 lines
sound/soc/codecs/mt6359.c line 845
Quoted 7 lines
sound/soc/codecs/mt6359.c new file +7
-0+/* ul pga gain */ +SOC_SINGLE_EXT_TLV ("PGAL Volume" , +MT6359_AUDENC_ANA_CON0 ,RG_AUDPREAMPLGAIN_SFT ,4 ,0 , + snd_soc_get_volsw, mt6359_put_volsw, capture_tlv), +SOC_SINGLE_EXT_TLV ("PGAR Volume" , +MT6359_AUDENC_ANA_CON1 ,RG_AUDPREAMPRGAIN_SFT ,4 ,0 , + snd_soc_get_volsw, mt6359_put_volsw, capture_tlv),Same here.
Each mic may has it's different gain setting in different scenario.
For example, in phone calling scenario, mic1 is main mic and mic2 is reference mic.
And in video recording scenario, mic1 is reference mic and mic2 is main mic.
PGAL is for mic1, PGAR is for mic2, PGA3 is for mic3. We think it is more flexible in different user scenario.
Quoted 11 lines
sound/soc/codecs/mt6359.c line 879
Quoted 8 lines
sound/soc/codecs/mt6359.c new file +7
-0+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,
};Quoted 10 lines
sound/soc/codecs/mt6359.c line 2880
Quoted 8 lines
sound/soc/codecs/mt6359.c new file +8
-0+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.
For "AUD_CK" power widget:
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), Quoted 14 lines
sound/soc/codecs/mt6359.c line 4132
Quoted 11 lines
sound/soc/codecs/mt6359.c new file +10
-0+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?
mt6359_snd_controls is defined at :
static const struct snd_soc_component_driver mt6359_soc_component_driver
= {
.controls = mt6359_snd_controls,Quoted 7 lines
sound/soc/codecs/mt6359.c line 4140
Quoted 5 lines
sound/soc/codecs/mt6359.c new file +5
-0+ priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTL ] =8 ; + priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTR ] =8 ; + priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP1 ] =3 ; + priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP2 ] =3 ; + priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP3 ] =3 ;These should be left at the hardware defaults.
These setting can be removed because it is align with register default.
Quoted 10 lines
sound/soc/codecs/mt6359.c line 4146
Quoted 6 lines
sound/soc/codecs/mt6359.c new file +5
-0+ 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() ?
Quoted 6 lines
sound/soc/codecs/mt6359.c line 4151
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+ 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()?
static struct platform_driver mt6359_platform_driver = {
...
.probe = mt6359_platform_driver_probe, .remove = mt6359_platform_driver_remove, /* add this */ };
Great thanks for your review again.
Mark Brown
maintainerRe: [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver
Quoted 12 lines · Eason Yen
Quoted 5 lines · Mark Brown
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.
Quoted 7 lines
For PMIC part, user need to set in these register :
GPIO_MODE1/GPIO_MODE2/GPIO_MODE3The 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.
Quoted 11 lines
Quoted 5 lines
sound/soc/codecs/mt6359.c line 332
Quoted 3 lines
sound/soc/codecs/mt6359.c new file +3
-0+/* 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_dcxois used at mt6359_mtkaif_calibration_enable/disable.
mtkaif_calibrationprocess 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.
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
sound/soc/codecs/mt6359.c new file +10
-0+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" ,Quoted 8 lines
sound/soc/codecs/mt6359.c line 722
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+"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.
Quoted 7 lines
For mic1, it's
mic_typecan 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).
Quoted 10 lines
Quoted 5 lines
sound/soc/codecs/mt6359.c line 834
Quoted 3 lines
sound/soc/codecs/mt6359.c new file +3
-0+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.
Quoted 26 lines
Quoted 10 lines
sound/soc/codecs/mt6359.c line 879
Quoted 7 lines
sound/soc/codecs/mt6359.c new file +7
-0+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" };
Quoted 6 lines
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?
Quoted 11 lines
Quoted 10 lines
sound/soc/codecs/mt6359.c line 2880
Quoted 8 lines
sound/soc/codecs/mt6359.c new file +8
-0+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?
Quoted 2 lines
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?
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+static int mt6359_codec_probe (struct snd_soc_component *cmpnt) +{ +struct mt6359_priv *priv =snd_soc_component_get_drvdata (cmpnt); +int ret;Quoted 8 lines
sound/soc/codecs/mt6359.c line 4132
Quoted 6 lines
sound/soc/codecs/mt6359.c new file +6
-0+ +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.
Quoted 2 lines
Do you mean that I should merge
mt6359_snd_vow_controlsintomt6359_snd_controls?
Yes, you're unconditionally registering these so there's no sense in splitting them.
Quoted 11 lines
Quoted 8 lines
sound/soc/codecs/mt6359.c line 4146
Quoted 5 lines
sound/soc/codecs/mt6359.c new file +5
-0+ 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 ofmt6359_codec_probe()?
Yes.
Quoted 9 lines
Quoted 6 lines
sound/soc/codecs/mt6359.c line 4151
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+ 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.
Re: [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver
Dear Mark,
Thanks for your viewing.
Quoted 16 lines · Mark Brown
Quoted 14 lines · Eason Yen
Quoted 5 lines · Mark Brown
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
Quoted 8 lines
Quoted 4 lines
The following functions are used to set:
- playback_gpio_set/playback_gpio_reset
- capture_gpio_set/capture_gpio_reset
- vow_gpio_set/vow_gpio_resetThis 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.
Quoted 42 lines
Quoted 11 lines
Quoted 5 lines
sound/soc/codecs/mt6359.c line 332
Quoted 3 lines
sound/soc/codecs/mt6359.c new file +3
-0+/* 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_dcxois used at mt6359_mtkaif_calibration_enable/disable.
mtkaif_calibrationprocess 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.
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
sound/soc/codecs/mt6359.c new file +10
-0+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" ,Quoted 8 lines
sound/soc/codecs/mt6359.c line 722
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+"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.
Quoted 7 lines
For mic1, it's
mic_typecan 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"
Quoted 7 lines
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?
Quoted 40 lines
Quoted 10 lines
Quoted 5 lines
sound/soc/codecs/mt6359.c line 834
Quoted 3 lines
sound/soc/codecs/mt6359.c new file +3
-0+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.
Quoted 25 lines
Quoted 10 lines
sound/soc/codecs/mt6359.c line 879
Quoted 7 lines
sound/soc/codecs/mt6359.c new file +7
-0+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" };
Quoted 9 lines
Quoted 6 lines
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.
Quoted 11 lines
Quoted 10 lines
Quoted 10 lines
sound/soc/codecs/mt6359.c line 2880
Quoted 8 lines
sound/soc/codecs/mt6359.c new file +8
-0+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?
Quoted 6 lines
Quoted 2 lines
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.
Quoted 44 lines
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+static int mt6359_codec_probe (struct snd_soc_component *cmpnt) +{ +struct mt6359_priv *priv =snd_soc_component_get_drvdata (cmpnt); +int ret;Quoted 8 lines
sound/soc/codecs/mt6359.c line 4132
Quoted 6 lines
sound/soc/codecs/mt6359.c new file +6
-0+ +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.
Quoted 2 lines
Do you mean that I should merge
mt6359_snd_vow_controlsintomt6359_snd_controls?Yes, you're unconditionally registering these so there's no sense in splitting them.
Quoted 11 lines
Quoted 8 lines
sound/soc/codecs/mt6359.c line 4146
Quoted 5 lines
sound/soc/codecs/mt6359.c new file +5
-0+ 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 ofmt6359_codec_probe()?Yes.
Quoted 9 lines
Quoted 6 lines
sound/soc/codecs/mt6359.c line 4151
Quoted 4 lines
sound/soc/codecs/mt6359.c new file +4
-0+ 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.
Mark Brown
maintainerRe: [PATCH 2/2] ASoC: codec: mediatek: add mt6359 codec driver
Quoted 9 lines · Eason Yen
Quoted 7 lines · Mark Brown
Quoted 6 lines · Eason Yen
On Mon, 2020-03-09 at 13:13 +0000, Mark Brown wrote:
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
Quoted 8 lines
Quoted 2 lines
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_enableandmt6359_mtkaif_calibration_disable.And at runtime stage, it is triggered by
mt_dl_gpio_eventandmt_ul_gpio_eventwhile playback or capture.
What I'm suggesting is moving those to the machine driver (you could provide helpers in the CODEC driver for the common case I guess... I'd need to review).
Quoted 7 lines
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?
Yes.
Quoted 12 lines
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" };
Quoted 5 lines
static SOC_ENUM_SINGLE_DECL(lo_in_mux_map_enum,
SND_SOC_NOPM, 0, lo_in_mux_map);static const struct
snd_kcontrol_newlo_in_mux_control=SOC_DAPM_ENUM("LO Select", lo_in_mux_map_enum);
That looks OK.
Quoted 14 lines
Quoted 13 lines
Quoted 13 lines
Quoted 10 lines
sound/soc/codecs/mt6359.c line 2880
Quoted 8 lines
sound/soc/codecs/mt6359.c new file +8
-0+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.
Quoted 8 lines
Quoted 2 lines
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 ...
I think you need some comments about this in the code, it looks like a mistake - it relies on the use of sequenced widgets, you should reference that.