* [PATCH v2 0/2] Add mediatek codec mt6359 driver @ 2020-08-10 3:05 Jiaxin Yu 2020-08-10 3:05 ` [PATCH v2 2/2] dt-bindings: mediatek: mt6359: add codec document Jiaxin Yu [not found] ` <1597028754-7732-2-git-send-email-jiaxin.yu@mediatek.com> 0 siblings, 2 replies; 9+ messages in thread From: Jiaxin Yu @ 2020-08-10 3:05 UTC (permalink / raw) To: broonie, matthias.bgg, robh+dt, tiwai, linux-kernel Cc: alsa-devel, shane.chien, howie.huang, Jiaxin Yu, tzungbi, linux-mediatek, eason.yen, linux-arm-kernel Add mediatek codec (MT6359) driver MT6359 support playback and capture 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" v2 changes: 1. Move playback_gpio/capture_gpio to the machine driver. 2. Fix mic_type(ACC/DMIC/DCC/DCC_*) at init stage. 3. Move devm_regulor_get to mt6359_platform_driver_probe. 4. Add relulator_disable in remove function. 5. Use stereo controls to the volume control. 6. Use SOC_ENUM_SINGLE_DECL instead of SOC_VALUE_ENUM_SINGLE_DECL. 7. Cleanup unused code. v1 changes: 1.lkml link: https://lkml.org/lkml/2020/3/5/1257 Jiaxin Yu (2): ASoC: mediatek: mt6359: add codec driver dt-bindings: mediatek: mt6359: add codec document .../devicetree/bindings/sound/mt6359.yaml | 68 + sound/soc/codecs/Kconfig | 8 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/mt6359.c | 2966 ++++++++++++++++++++ sound/soc/codecs/mt6359.h | 2653 +++++++++++++++++ 5 files changed, 5697 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/mt6359.yaml create mode 100644 sound/soc/codecs/mt6359.c create mode 100644 sound/soc/codecs/mt6359.h -- 1.8.1.1.dirty _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] dt-bindings: mediatek: mt6359: add codec document 2020-08-10 3:05 [PATCH v2 0/2] Add mediatek codec mt6359 driver Jiaxin Yu @ 2020-08-10 3:05 ` Jiaxin Yu [not found] ` <1597028754-7732-2-git-send-email-jiaxin.yu@mediatek.com> 1 sibling, 0 replies; 9+ messages in thread From: Jiaxin Yu @ 2020-08-10 3:05 UTC (permalink / raw) To: broonie, matthias.bgg, robh+dt, tiwai, linux-kernel Cc: alsa-devel, shane.chien, howie.huang, Jiaxin Yu, tzungbi, linux-mediatek, eason.yen, linux-arm-kernel This patch adds MediaTek MT6359 codec document. Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com> --- .../devicetree/bindings/sound/mt6359.yaml | 68 ++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/mt6359.yaml diff --git a/Documentation/devicetree/bindings/sound/mt6359.yaml b/Documentation/devicetree/bindings/sound/mt6359.yaml new file mode 100644 index 0000000..cd59dbe --- /dev/null +++ b/Documentation/devicetree/bindings/sound/mt6359.yaml @@ -0,0 +1,68 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/mt6359.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Mediatek MT6359 Codec Device Tree Bindings + +maintainers: + - Eason Yen <eason.yen@mediatek.com> + - Jiaxin Yu <jiaxin.yu@mediatek.com> + - Shane Chien <shane.chien@mediatek.com> + +description: | + The communication between MT6359 and SoC is through Mediatek PMIC wrapper. + For more detail, please visit Mediatek PMIC wrapper documentation. + Must be a child node of PMIC wrapper. + +properties: + compatible: + const: mediatek,mt6359-sound + + mediatek,dmic-mode: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + Indicates how many data pins are used to transmit two channels of PDM + signal. 0 means two wires, 1 means one wire. Default value is 0. + enum: + - 0 # one wire + - 1 # two wires + + mediatek,mic-type-0: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + Specifies the type of mic type connected to adc0 + + enum: + - 0 # IDLE - mic in turn-off status + - 1 # ACC - analog mic with alternating coupling + - 2 # DMIC - digital mic + - 3 # DCC - analog mic with direct couping + - 4 # DCC_ECM_DIFF - analog electret condenser mic with differential mode + - 5 # DCC_ECM_SINGLE - analog electret condenser mic with single mode + + mediatek,mic-type-1: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + Specifies the type of mic type connected to adc1 + + mediatek,mic-type-2: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + Specifies the type of mic type connected to adc2 + +required: + - compatible + +additionalProperties: false + +examples: + - | + mt6359codec: mt6359codec { + compatible = "mediatek,mt6359-sound"; + mediatek,dmic-mode = <0>; + mediatek,mic-type-0 = <2>; + }; + +... -- 1.8.1.1.dirty _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
[parent not found: <1597028754-7732-2-git-send-email-jiaxin.yu@mediatek.com>]
* Re: [PATCH v2 1/2] ASoC: mediatek: mt6359: add codec driver [not found] ` <1597028754-7732-2-git-send-email-jiaxin.yu@mediatek.com> @ 2020-08-10 8:13 ` Tzung-Bi Shih 2020-08-10 18:59 ` Mark Brown 1 sibling, 0 replies; 9+ messages in thread From: Tzung-Bi Shih @ 2020-08-10 8:13 UTC (permalink / raw) To: Jiaxin Yu Cc: ALSA development, howie.huang, Takashi Iwai, Rob Herring, Linux Kernel Mailing List, shane.chien, Mark Brown, linux-mediatek, eason.yen, Matthias Brugger, linux-arm-kernel On Mon, Aug 10, 2020 at 11:11 AM Jiaxin Yu <jiaxin.yu@mediatek.com> wrote: > > Add the mt6359 codec driver. > > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com> Reviewed-by: Tzung-Bi Shih <tzungbi@google.com> This patch also reviewed few rounds on https://crrev.com/c/2299951 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] ASoC: mediatek: mt6359: add codec driver [not found] ` <1597028754-7732-2-git-send-email-jiaxin.yu@mediatek.com> 2020-08-10 8:13 ` [PATCH v2 1/2] ASoC: mediatek: mt6359: add codec driver Tzung-Bi Shih @ 2020-08-10 18:59 ` Mark Brown 2020-08-12 7:29 ` Jiaxin Yu 1 sibling, 1 reply; 9+ messages in thread From: Mark Brown @ 2020-08-10 18:59 UTC (permalink / raw) To: Jiaxin Yu Cc: alsa-devel, shane.chien, howie.huang, tiwai, linux-kernel, tzungbi, robh+dt, linux-mediatek, eason.yen, matthias.bgg, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 6258 bytes --] On Mon, Aug 10, 2020 at 11:05:53AM +0800, Jiaxin Yu wrote: > +void mt6359_set_playback_gpio(struct snd_soc_component *cmpnt) > +{ > + struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt); > +void mt6359_reset_playback_gpio(struct snd_soc_component *cmpnt) > +{ > + struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt); > +void mt6359_set_capture_gpio(struct snd_soc_component *cmpnt) > +{ > +void mt6359_reset_capture_gpio(struct snd_soc_component *cmpnt) > +{ What are these, should they not be managed through gpiolib and/or pinctrl? > +/* use only when doing mtkaif calibraiton at the boot time */ > +static int mt6359_set_dcxo(struct mt6359_priv *priv, bool enable) > +{ > + regmap_update_bits(priv->regmap, MT6359_DCXO_CW12, > + 0x1 << RG_XO_AUDIO_EN_M_SFT, > + (enable ? 1 : 0) << RG_XO_AUDIO_EN_M_SFT); > + return 0; Either don't have a return value or use the result of regmap_update_bits(). There's similar issues with some other functions in here. > +int mt6359_mtkaif_calibration_enable(struct snd_soc_pcm_runtime *rtd) > +{ > +EXPORT_SYMBOL_GPL(mt6359_mtkaif_calibration_enable); Why is this exported? > +static void hp_aux_feedback_loop_gain_ramp(struct mt6359_priv *priv, bool up) > +{ > + int i = 0, stage = 0; > + > + /* Reduce HP aux feedback loop gain step by step */ > + for (i = 0; i <= 0xf; i++) { > + stage = up ? i : 0xf - i; Please write normal conditional statements, it helps legibility. > +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; So we make the volume change actually take effect... > + 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; ...then read the value that was set and store it elsewhere. What's going on here? > +/*HP MUX */ > +static const char * const hp_in_mux_map[] = { > + "Open", > + "LoudSPK Playback", > + "Audio Playback", > + "Test Mode", > + "HP Impedance", > + "undefined1", > + "undefined2", > + "undefined3", > +}; Why expose undefined (and presumably out of spec) values to userspace? > +static int mt_clksq_event(struct snd_soc_dapm_widget *w, > + struct snd_kcontrol *kcontrol, > + int event) > +{ > + struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm); > + struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt); > + > + dev_dbg(priv->dev, "%s(), event = 0x%x\n", __func__, event); > + > + switch (event) { > + case SND_SOC_DAPM_PRE_PMU: > + /* audio clk source from internal dcxo */ > + regmap_update_bits(priv->regmap, MT6359_AUDENC_ANA_CON23, > + RG_CLKSQ_IN_SEL_TEST_MASK_SFT, > + 0x0); This also appeared to be controlled in _set_clkseq() - are we sure that things couldn't get confused about the state? > + /* HP damp circuit enable */ > + /*Enable HPRN/HPLN output 4K to VCM */ Spaces around the /* */ > +static int mt_hp_event(struct snd_soc_dapm_widget *w, > + struct snd_kcontrol *kcontrol, > + int event) > +{ > + struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm); > + struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt); > + unsigned int mux = dapm_kcontrol_get_value(w->kcontrols[0]); > + int device = DEVICE_HP; > + > + dev_dbg(priv->dev, "%s(), event 0x%x, dev_counter[DEV_HP] %d, mux %u\n", > + __func__, event, priv->dev_counter[device], mux); > + > + switch (event) { > + case SND_SOC_DAPM_PRE_PMU: > + priv->dev_counter[device]++; > + if (priv->dev_counter[device] > 1) > + break; /* already enabled, do nothing */ > + else if (priv->dev_counter[device] <= 0) Why are we doing additional refcounting on top of what DAPM is doing? This seems like there should be at least one widget representing the shared bits of the audio path. > +#define MT6359_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\ > + SNDRV_PCM_FMTBIT_U16_LE | SNDRV_PCM_FMTBIT_U16_BE |\ > + SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\ > + SNDRV_PCM_FMTBIT_U24_LE | SNDRV_PCM_FMTBIT_U24_BE |\ > + SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE |\ > + SNDRV_PCM_FMTBIT_U32_LE | SNDRV_PCM_FMTBIT_U32_BE) The driver doesn't appear to configure anything except the sample rate - how are all these formats supported? > + /* hp gain ctl default choose ZCD */ > + priv->hp_gain_ctl = HP_GAIN_CTL_ZCD; > + hp_gain_ctl_select(priv, priv->hp_gain_ctl); Why not use the hardware default? > + mt6359_codec_init_reg(cmpnt); > + > + 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; Same here. > + ret = regulator_enable(priv->avdd_reg); > + if (ret) { > + dev_err(priv->dev, "%s(), failed to enable regulator!\n", > + __func__); > + return ret; > + } Perhaps make this a DAPM widget? > + priv->avdd_reg = devm_regulator_get(&pdev->dev, "vaud18"); > + if (IS_ERR(priv->avdd_reg)) { > + dev_err(&pdev->dev, "%s(), have no vaud18 supply", __func__); > + return PTR_ERR(priv->avdd_reg); > + } It's better to print error codes to help people debugging problems. > +static const struct of_device_id mt6359_of_match[] = { > + {.compatible = "mediatek,mt6359-sound",}, > + {} > +}; > +MODULE_DEVICE_TABLE(of, mt6359_of_match); We don't need a compatible here, we know that this device is here since it's part of the parent device and isn't something that might appear in another device. This is reflecting the Linux driver model, not the hardware. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] ASoC: mediatek: mt6359: add codec driver 2020-08-10 18:59 ` Mark Brown @ 2020-08-12 7:29 ` Jiaxin Yu 2020-08-12 12:05 ` Mark Brown 0 siblings, 1 reply; 9+ messages in thread From: Jiaxin Yu @ 2020-08-12 7:29 UTC (permalink / raw) To: Mark Brown Cc: alsa-devel, shane.chien, howie.huang, tiwai, linux-kernel, tzungbi, robh+dt, linux-mediatek, eason.yen, matthias.bgg, linux-arm-kernel On Mon, 2020-08-10 at 19:59 +0100, Mark Brown wrote: > On Mon, Aug 10, 2020 at 11:05:53AM +0800, Jiaxin Yu wrote: > > > +void mt6359_set_playback_gpio(struct snd_soc_component *cmpnt) > > +{ > > + struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt); > > > +void mt6359_reset_playback_gpio(struct snd_soc_component *cmpnt) > > +{ > > + struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt); > > > +void mt6359_set_capture_gpio(struct snd_soc_component *cmpnt) > > +{ > > > +void mt6359_reset_capture_gpio(struct snd_soc_component *cmpnt) > > +{ > > What are these, should they not be managed through gpiolib and/or > pinctrl? These are the functions that control the mux of input or output signal. I refer to the other codec drivers, most of them are manipulate the regs in their codec drivers also. Maybe it's easier to control? > > +/* use only when doing mtkaif calibraiton at the boot time */ > > +static int mt6359_set_dcxo(struct mt6359_priv *priv, bool enable) > > +{ > > + regmap_update_bits(priv->regmap, MT6359_DCXO_CW12, > > + 0x1 << RG_XO_AUDIO_EN_M_SFT, > > + (enable ? 1 : 0) << RG_XO_AUDIO_EN_M_SFT); > > + return 0; > > Either don't have a return value or use the result of > regmap_update_bits(). There's similar issues with some other functions > in here. > Yes, I will refine them. > > +int mt6359_mtkaif_calibration_enable(struct snd_soc_pcm_runtime *rtd) > > +{ > > > +EXPORT_SYMBOL_GPL(mt6359_mtkaif_calibration_enable); > > Why is this exported? > This function is exported to machine driver to do calibration when registering. The interface between MT6359 and MTK SoC is a special interface that named MTKAIF. Therefore, if MT6359 is to be used normally, it needs to rely on the platform for calibration when registering. > > +static void hp_aux_feedback_loop_gain_ramp(struct mt6359_priv *priv, bool up) > > +{ > > + int i = 0, stage = 0; > > + > > + /* Reduce HP aux feedback loop gain step by step */ > > + for (i = 0; i <= 0xf; i++) { > > + stage = up ? i : 0xf - i; > > Please write normal conditional statements, it helps legibility. > /* Increase/Reduce HP aux feedback loop gain step by step */ This is to prevent sudden changes in volume. > > +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; > > So we make the volume change actually take effect... > > > + 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; > > ..then read the value that was set and store it elsewhere. What's > going on here? > Because we can adjust the volume at two points, before and during the playback or capture. In addition, we will do volume ramp when opening the driver. If we set the volume before opening the driver, we need get the value to do volume ramp during opening the driver. > > +/*HP MUX */ > > +static const char * const hp_in_mux_map[] = { > > + "Open", > > + "LoudSPK Playback", > > + "Audio Playback", > > + "Test Mode", > > + "HP Impedance", > > + "undefined1", > > + "undefined2", > > + "undefined3", > > +}; > > Why expose undefined (and presumably out of spec) values to userspace? > Removed them. > > +static int mt_clksq_event(struct snd_soc_dapm_widget *w, > > + struct snd_kcontrol *kcontrol, > > + int event) > > +{ > > + struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm); > > + struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt); > > + > > + dev_dbg(priv->dev, "%s(), event = 0x%x\n", __func__, event); > > + > > + switch (event) { > > + case SND_SOC_DAPM_PRE_PMU: > > + /* audio clk source from internal dcxo */ > > + regmap_update_bits(priv->regmap, MT6359_AUDENC_ANA_CON23, > > + RG_CLKSQ_IN_SEL_TEST_MASK_SFT, > > + 0x0); > > This also appeared to be controlled in _set_clkseq() - are we sure that > things couldn't get confused about the state? > _set_clkseq() will only be run at the time of mtkaif calibraiton, and the calibration only run once when the codec driver probe. So it won't get confused about the state. But it really only needs to be set up once at mt6359_codec_init_reg(). I will remove the unnecesary settings. > > + /* HP damp circuit enable */ > > + /*Enable HPRN/HPLN output 4K to VCM */ > > Spaces around the /* */ > Sorry, fixed it. > > +static int mt_hp_event(struct snd_soc_dapm_widget *w, > > + struct snd_kcontrol *kcontrol, > > + int event) > > +{ > > + struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm); > > + struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt); > > + unsigned int mux = dapm_kcontrol_get_value(w->kcontrols[0]); > > + int device = DEVICE_HP; > > + > > + dev_dbg(priv->dev, "%s(), event 0x%x, dev_counter[DEV_HP] %d, mux %u\n", > > + __func__, event, priv->dev_counter[device], mux); > > + > > + switch (event) { > > + case SND_SOC_DAPM_PRE_PMU: > > + priv->dev_counter[device]++; > > + if (priv->dev_counter[device] > 1) > > + break; /* already enabled, do nothing */ > > + else if (priv->dev_counter[device] <= 0) > > Why are we doing additional refcounting on top of what DAPM is doing? > This seems like there should be at least one widget representing the > shared bits of the audio path. > We have "HPL Mux" and "HPR Mux", there will be two paths enabled when playback throuh HP. But actually they share the same control sequences. So in order to prevent setting it one more time we do additional refcouting. > > +#define MT6359_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\ > > + SNDRV_PCM_FMTBIT_U16_LE | SNDRV_PCM_FMTBIT_U16_BE |\ > > + SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\ > > + SNDRV_PCM_FMTBIT_U24_LE | SNDRV_PCM_FMTBIT_U24_BE |\ > > + SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE |\ > > + SNDRV_PCM_FMTBIT_U32_LE | SNDRV_PCM_FMTBIT_U32_BE) > > The driver doesn't appear to configure anything except the sample rate - > how are all these formats supported? > The interface between MT6359 and MTK SoC is a special interface that named MTKAIF. Because of its unique hardware settings, it really doesn't to set format argument. The capability of pcm driver mainly depends on cpu-dai driver to limit. > > + /* hp gain ctl default choose ZCD */ > > + priv->hp_gain_ctl = HP_GAIN_CTL_ZCD; > > + hp_gain_ctl_select(priv, priv->hp_gain_ctl); > > Why not use the hardware default? > We have two ways to control the volume, there is to select ZCD. Because the other one it not often used, ZCD is used by default. > > + mt6359_codec_init_reg(cmpnt); > > + > > + 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; > > Same here. > Removed them. > > + ret = regulator_enable(priv->avdd_reg); > > + if (ret) { > > + dev_err(priv->dev, "%s(), failed to enable regulator!\n", > > + __func__); > > + return ret; > > + } > > Perhaps make this a DAPM widget? > "vaud18" needs to be always on so we enable it when codec probe. > > + priv->avdd_reg = devm_regulator_get(&pdev->dev, "vaud18"); > > + if (IS_ERR(priv->avdd_reg)) { > > + dev_err(&pdev->dev, "%s(), have no vaud18 supply", __func__); > > + return PTR_ERR(priv->avdd_reg); > > + } > > It's better to print error codes to help people debugging problems. > Yes, I will print the error codes. > > +static const struct of_device_id mt6359_of_match[] = { > > + {.compatible = "mediatek,mt6359-sound",}, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, mt6359_of_match); > > We don't need a compatible here, we know that this device is here since > it's part of the parent device and isn't something that might appear in > another device. This is reflecting the Linux driver model, not the > hardware. Removed them. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] ASoC: mediatek: mt6359: add codec driver 2020-08-12 7:29 ` Jiaxin Yu @ 2020-08-12 12:05 ` Mark Brown 2020-08-13 15:40 ` Jiaxin Yu 0 siblings, 1 reply; 9+ messages in thread From: Mark Brown @ 2020-08-12 12:05 UTC (permalink / raw) To: Jiaxin Yu Cc: alsa-devel, shane.chien, howie.huang, tiwai, linux-kernel, tzungbi, robh+dt, linux-mediatek, eason.yen, matthias.bgg, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 3199 bytes --] On Wed, Aug 12, 2020 at 03:29:13PM +0800, Jiaxin Yu wrote: > On Mon, 2020-08-10 at 19:59 +0100, Mark Brown wrote: > > On Mon, Aug 10, 2020 at 11:05:53AM +0800, Jiaxin Yu wrote: > > > +void mt6359_set_playback_gpio(struct snd_soc_component *cmpnt) > > > +{ > > > + struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt); > > What are these, should they not be managed through gpiolib and/or > > pinctrl? > These are the functions that control the mux of input or output > signal. I refer to the other codec drivers, most of them are manipulate > the regs in their codec drivers also. Maybe it's easier to control? These functions are exported for other drivers to use rather than something done internally by the driver - if this were internal to the driver it'd not be a big deal but when it's for use by another driver it'd be better to go through standard interfaces. > > > +int mt6359_mtkaif_calibration_enable(struct snd_soc_pcm_runtime *rtd) > > > +{ > > > +EXPORT_SYMBOL_GPL(mt6359_mtkaif_calibration_enable); > > Why is this exported? > This function is exported to machine driver to do calibration when > registering. The interface between MT6359 and MTK SoC is a special > interface that named MTKAIF. Therefore, if MT6359 is to be used > normally, it needs to rely on the platform for calibration when > registering. So this needs the SoC to do something as part of callibration? > > > + switch (event) { > > > + case SND_SOC_DAPM_PRE_PMU: > > > + priv->dev_counter[device]++; > > > + if (priv->dev_counter[device] > 1) > > > + break; /* already enabled, do nothing */ > > > + else if (priv->dev_counter[device] <= 0) > > Why are we doing additional refcounting on top of what DAPM is doing? > > This seems like there should be at least one widget representing the > > shared bits of the audio path. > We have "HPL Mux" and "HPR Mux", there will be two paths enabled when > playback throuh HP. But actually they share the same control sequences. > So in order to prevent setting it one more time we do additional > refcouting. That sounds like you should just have one output path defined in DAPM and merge the left and right signals together rather than maintaining them separately. > > > + /* hp gain ctl default choose ZCD */ > > > + priv->hp_gain_ctl = HP_GAIN_CTL_ZCD; > > > + hp_gain_ctl_select(priv, priv->hp_gain_ctl); > > Why not use the hardware default? > We have two ways to control the volume, there is to select ZCD. Because > the other one it not often used, ZCD is used by default. Why not just let the user pick at runtime? > > > + ret = regulator_enable(priv->avdd_reg); > > > + if (ret) { > > > + dev_err(priv->dev, "%s(), failed to enable regulator!\n", > > > + __func__); > > > + return ret; > > > + } > > Perhaps make this a DAPM widget? > "vaud18" needs to be always on so we enable it when codec probe. If it is just supposed to be left on all the time it's better to do this in the main device model probe rather than the component probe. The component probe should usually only be used for things that need the rest of the card to be there. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] ASoC: mediatek: mt6359: add codec driver 2020-08-12 12:05 ` Mark Brown @ 2020-08-13 15:40 ` Jiaxin Yu 2020-08-13 15:44 ` Mark Brown 0 siblings, 1 reply; 9+ messages in thread From: Jiaxin Yu @ 2020-08-13 15:40 UTC (permalink / raw) To: Mark Brown Cc: alsa-devel, shane.chien, howie.huang, tiwai, linux-kernel, tzungbi, robh+dt, linux-mediatek, eason.yen, matthias.bgg, linux-arm-kernel On Wed, 2020-08-12 at 13:05 +0100, Mark Brown wrote: > On Wed, Aug 12, 2020 at 03:29:13PM +0800, Jiaxin Yu wrote: > > On Mon, 2020-08-10 at 19:59 +0100, Mark Brown wrote: > > > On Mon, Aug 10, 2020 at 11:05:53AM +0800, Jiaxin Yu wrote: > > > > > +void mt6359_set_playback_gpio(struct snd_soc_component *cmpnt) > > > > +{ > > > > + struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt); > > > > What are these, should they not be managed through gpiolib and/or > > > pinctrl? > > > These are the functions that control the mux of input or output > > signal. I refer to the other codec drivers, most of them are manipulate > > the regs in their codec drivers also. Maybe it's easier to control? > > These functions are exported for other drivers to use rather than > something done internally by the driver - if this were internal to the > driver it'd not be a big deal but when it's for use by another driver > it'd be better to go through standard interfaces. > Can we move this part of the operation to the codec dai driver ops, such as .startup and .shutdown? Because originally these functions are exported to machine driver's dai_link .startup and .shutdown ops. > > > > +int mt6359_mtkaif_calibration_enable(struct snd_soc_pcm_runtime *rtd) > > > > +{ > > > > > +EXPORT_SYMBOL_GPL(mt6359_mtkaif_calibration_enable); > > > > Why is this exported? > > > This function is exported to machine driver to do calibration when > > registering. The interface between MT6359 and MTK SoC is a special > > interface that named MTKAIF. Therefore, if MT6359 is to be used > > normally, it needs to rely on the platform for calibration when > > registering. > > So this needs the SoC to do something as part of callibration? > Yes, the side of MTKAIF in SoC part named adda, we need config it before calibration. We will also upstream machine/platform driver that use this codec driver later. > > > > + switch (event) { > > > > + case SND_SOC_DAPM_PRE_PMU: > > > > + priv->dev_counter[device]++; > > > > + if (priv->dev_counter[device] > 1) > > > > + break; /* already enabled, do nothing */ > > > > + else if (priv->dev_counter[device] <= 0) > > > > Why are we doing additional refcounting on top of what DAPM is doing? > > > This seems like there should be at least one widget representing the > > > shared bits of the audio path. > > > We have "HPL Mux" and "HPR Mux", there will be two paths enabled when > > playback throuh HP. But actually they share the same control sequences. > > So in order to prevent setting it one more time we do additional > > refcouting. > > That sounds like you should just have one output path defined in DAPM > and merge the left and right signals together rather than maintaining > them separately. > Yes, it's would better if they are combined into one mixer control that named "HP Mux". > > > > + /* hp gain ctl default choose ZCD */ > > > > + priv->hp_gain_ctl = HP_GAIN_CTL_ZCD; > > > > + hp_gain_ctl_select(priv, priv->hp_gain_ctl); > > > > Why not use the hardware default? > > > We have two ways to control the volume, there is to select ZCD. Because > > the other one it not often used, ZCD is used by default. > > Why not just let the user pick at runtime? > For another adjustment method, we didn't upstream it, so only ZCD reserved. And the hardware default setting is ZCD, so we can removed these lines. > > > > + ret = regulator_enable(priv->avdd_reg); > > > > + if (ret) { > > > > + dev_err(priv->dev, "%s(), failed to enable regulator!\n", > > > > + __func__); > > > > + return ret; > > > > + } > > > > Perhaps make this a DAPM widget? > > > "vaud18" needs to be always on so we enable it when codec probe. > > If it is just supposed to be left on all the time it's better to do this > in the main device model probe rather than the component probe. The > component probe should usually only be used for things that need the > rest of the card to be there. Ok, I will correct it. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] ASoC: mediatek: mt6359: add codec driver 2020-08-13 15:40 ` Jiaxin Yu @ 2020-08-13 15:44 ` Mark Brown 2020-08-14 10:27 ` Jiaxin Yu 0 siblings, 1 reply; 9+ messages in thread From: Mark Brown @ 2020-08-13 15:44 UTC (permalink / raw) To: Jiaxin Yu Cc: alsa-devel, shane.chien, howie.huang, tiwai, linux-kernel, tzungbi, robh+dt, linux-mediatek, eason.yen, matthias.bgg, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 1165 bytes --] On Thu, Aug 13, 2020 at 11:40:00PM +0800, Jiaxin Yu wrote: > On Wed, 2020-08-12 at 13:05 +0100, Mark Brown wrote: > > These functions are exported for other drivers to use rather than > > something done internally by the driver - if this were internal to the > > driver it'd not be a big deal but when it's for use by another driver > > it'd be better to go through standard interfaces. > Can we move this part of the operation to the codec dai driver ops, such > as .startup and .shutdown? Because originally these functions are > exported to machine driver's dai_link .startup and .shutdown ops. If it's internal to the driver sure. > > So this needs the SoC to do something as part of callibration? > Yes, the side of MTKAIF in SoC part named adda, we need config it before > calibration. We will also upstream machine/platform driver that use this > codec driver later. It would probably be better to just leave this out for now then add the required bits to the CODEC driver as part of a patch series that also adds the machine driver that uses it so it's clear how it all fits together. It sounds like it should be fine but this'd be easier to review. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] ASoC: mediatek: mt6359: add codec driver 2020-08-13 15:44 ` Mark Brown @ 2020-08-14 10:27 ` Jiaxin Yu 0 siblings, 0 replies; 9+ messages in thread From: Jiaxin Yu @ 2020-08-14 10:27 UTC (permalink / raw) To: Mark Brown Cc: alsa-devel, shane.chien, howie.huang, tiwai, linux-kernel, tzungbi, robh+dt, linux-mediatek, eason.yen, matthias.bgg, linux-arm-kernel On Thu, 2020-08-13 at 16:44 +0100, Mark Brown wrote: > On Thu, Aug 13, 2020 at 11:40:00PM +0800, Jiaxin Yu wrote: > > On Wed, 2020-08-12 at 13:05 +0100, Mark Brown wrote: > > > > These functions are exported for other drivers to use rather than > > > something done internally by the driver - if this were internal to the > > > driver it'd not be a big deal but when it's for use by another driver > > > it'd be better to go through standard interfaces. > > > Can we move this part of the operation to the codec dai driver ops, such > > as .startup and .shutdown? Because originally these functions are > > exported to machine driver's dai_link .startup and .shutdown ops. > > If it's internal to the driver sure. > > > > So this needs the SoC to do something as part of callibration? > > > Yes, the side of MTKAIF in SoC part named adda, we need config it before > > calibration. We will also upstream machine/platform driver that use this > > codec driver later. > > It would probably be better to just leave this out for now then add the > required bits to the CODEC driver as part of a patch series that also > adds the machine driver that uses it so it's clear how it all fits > together. It sounds like it should be fine but this'd be easier to > review. Ok, I'll remove the functions that related to the calibration, and upstream them again as part of a patch series that adds the machine driver later. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-08-14 10:30 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-10 3:05 [PATCH v2 0/2] Add mediatek codec mt6359 driver Jiaxin Yu 2020-08-10 3:05 ` [PATCH v2 2/2] dt-bindings: mediatek: mt6359: add codec document Jiaxin Yu [not found] ` <1597028754-7732-2-git-send-email-jiaxin.yu@mediatek.com> 2020-08-10 8:13 ` [PATCH v2 1/2] ASoC: mediatek: mt6359: add codec driver Tzung-Bi Shih 2020-08-10 18:59 ` Mark Brown 2020-08-12 7:29 ` Jiaxin Yu 2020-08-12 12:05 ` Mark Brown 2020-08-13 15:40 ` Jiaxin Yu 2020-08-13 15:44 ` Mark Brown 2020-08-14 10:27 ` Jiaxin Yu
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).