| Message ID | 20240226-audio-i350-v1-0-4fa1cea1667f@baylibre.com |
|---|---|
| Headers | show |
| Series | Add audio support for the MediaTek Genio 350-evk board | expand |
Il 26/02/24 15:01, Alexandre Mergnat ha scritto: > This serie aim to add the following audio support for the Genio 350-evk: > - Playback > - 2ch Headset Jack (Earphone) > - 1ch Line-out Jack (Speaker) > - 8ch HDMI Tx > - Capture > - 1ch DMIC (On-board Digital Microphone) > - 1ch AMIC (On-board Analogic Microphone) > - 1ch Headset Jack (External Analogic Microphone) > > Of course, HDMI playback need the MT8365 display patches [1] and a DTS > change documented in "mediatek,mt8365-mt6357.yaml". > > [1]: https://lore.kernel.org/all/20231023-display-support-v1-0-5c860ed5c33b@baylibre.com/ > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> Actually, I am cooking a series (I'm finishing the testing....) that brings quite a bit of cleanups in MTK ASoC, including the commonization of the machine driver probe, with the dai-link DT nodes, and which also modernizes most of the existing drivers to use that instead. If you wait for a day or two, your mt8365-mt6357.c driver's probe function can be shrunk to ~3 lines or something like that.. very easily :-) Cheers, Angelo > --- > Alexandre Mergnat (15): > ASoC: dt-bindings: mediatek,mt8365-afe: Add audio afe document > ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card document > dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC > ASoC: mediatek: mt8365: Add common header > SoC: mediatek: mt8365: support audio clock control > ASoC: mediatek: mt8365: Add I2S DAI support > ASoC: mediatek: mt8365: Add ADDA DAI support > ASoC: mediatek: mt8365: Add DMIC DAI support > ASoC: mediatek: mt8365: Add PCM DAI support > ASoC: mediatek: mt8365: Add platform driver > ASoC: mediatek: Add MT8365 support > arm64: defconfig: enable mt8365 sound > arm64: dts: mediatek: add mt6357 audio codec support > arm64: dts: mediatek: add afe support for mt8365 SoC > arm64: dts: mediatek: add audio support for mt8365-evk > > Fabien Parent (1): > mfd: mt6397-core: register mt6357 sound codec > > Nicolas Belin (2): > ASoc: mediatek: mt8365: Add a specific soundcard for EVK > ASoC: codecs: mt6357: add MT6357 codec > > .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 11 + > .../bindings/sound/mediatek,mt8365-afe.yaml | 164 ++ > .../bindings/sound/mediatek,mt8365-mt6357.yaml | 127 ++ > arch/arm64/boot/dts/mediatek/mt6357.dtsi | 6 +- > arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 95 +- > arch/arm64/boot/dts/mediatek/mt8365.dtsi | 47 +- > arch/arm64/configs/defconfig | 2 + > drivers/mfd/mt6397-core.c | 3 + > sound/soc/codecs/Kconfig | 7 + > sound/soc/codecs/Makefile | 2 + > sound/soc/codecs/mt6357.c | 1805 +++++++++++++++ > sound/soc/codecs/mt6357.h | 674 ++++++ > sound/soc/mediatek/Kconfig | 20 + > sound/soc/mediatek/Makefile | 1 + > sound/soc/mediatek/mt8365/Makefile | 15 + > sound/soc/mediatek/mt8365/mt8365-afe-clk.c | 455 ++++ > sound/soc/mediatek/mt8365/mt8365-afe-clk.h | 55 + > sound/soc/mediatek/mt8365/mt8365-afe-common.h | 495 +++++ > sound/soc/mediatek/mt8365/mt8365-afe-pcm.c | 2347 ++++++++++++++++++++ > sound/soc/mediatek/mt8365/mt8365-dai-adda.c | 355 +++ > sound/soc/mediatek/mt8365/mt8365-dai-dmic.c | 475 ++++ > sound/soc/mediatek/mt8365/mt8365-dai-i2s.c | 901 ++++++++ > sound/soc/mediatek/mt8365/mt8365-dai-pcm.c | 298 +++ > sound/soc/mediatek/mt8365/mt8365-mt6357.c | 379 ++++ > sound/soc/mediatek/mt8365/mt8365-reg.h | 987 ++++++++ > 25 files changed, 9718 insertions(+), 8 deletions(-) > --- > base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d > change-id: 20240226-audio-i350-4e11da088e55 > > Best regards,
Il 26/02/24 15:01, amergnat@baylibre.com ha scritto: > From: Nicolas Belin <nbelin@baylibre.com> > > Add a specific soundcard for mt8365-evk. It supports audio jack > in/out, dmics, the amic and lineout. > > Signed-off-by: Nicolas Belin <nbelin@baylibre.com> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > sound/soc/mediatek/mt8365/mt8365-mt6357.c | 379 ++++++++++++++++++++++++++++++ > 1 file changed, 379 insertions(+) > > diff --git a/sound/soc/mediatek/mt8365/mt8365-mt6357.c b/sound/soc/mediatek/mt8365/mt8365-mt6357.c > new file mode 100644 > index 000000000000..5192b35458e6 > --- /dev/null > +++ b/sound/soc/mediatek/mt8365/mt8365-mt6357.c > @@ -0,0 +1,379 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Mediatek MT8365 Sound Card driver > + * > + * Copyright (c) 2024 MediaTek Inc. > + * Authors: Nicolas Belin <nbelin@baylibre.com> > + */ > + > +#include <linux/module.h> > +#include <linux/of_gpio.h> > +#include <sound/soc.h> > +#include <sound/pcm_params.h> > +#include "mt8365-afe-common.h" > +#include <linux/pinctrl/consumer.h> > +#include "../common/mtk-soundcard-driver.h" > + > +enum PINCTRL_PIN_STATE { Aren't those supposed to be AFE pin states? Anyway, lower case name for enums please, and no zero initializer for the first entry. enum mt8365_pin_state { PIN_STATE_DEFAULT, .... }; > + PIN_STATE_DEFAULT = 0, > + PIN_STATE_DMIC, > + PIN_STATE_MISO_OFF, > + PIN_STATE_MISO_ON, > + PIN_STATE_MOSI_OFF, > + PIN_STATE_MOSI_ON, > + PIN_STATE_MAX > +}; > + > +static const char * const mt8365_mt6357_pin_str[PIN_STATE_MAX] = { > + "aud_default", > + "aud_dmic", > + "aud_miso_off", > + "aud_miso_on", > + "aud_mosi_off", > + "aud_mosi_on", > +}; > + > +struct mt8365_mt6357_priv { > + struct pinctrl *pinctrl; > + struct pinctrl_state *pin_states[PIN_STATE_MAX]; > +}; > + > +struct mt8365_dai_link_prop { > + char *name; > + unsigned int link_id; > +}; This structure is unused. > + > +enum { > + /* FE */ > + DAI_LINK_DL1_PLAYBACK = 0, > + DAI_LINK_DL2_PLAYBACK, > + DAI_LINK_AWB_CAPTURE, > + DAI_LINK_VUL_CAPTURE, > + /* BE */ > + DAI_LINK_2ND_I2S_INTF, > + DAI_LINK_DMIC, > + DAI_LINK_INT_ADDA, > + DAI_LINK_NUM > +}; > + > +static const struct snd_soc_dapm_widget mt8365_mt6357_widgets[] = { > + SND_SOC_DAPM_OUTPUT("HDMI Out"), > +}; > + > +static const struct snd_soc_dapm_route mt8365_mt6357_routes[] = { > + {"HDMI Out", NULL, "2ND I2S Playback"}, > + {"DMIC In", NULL, "MICBIAS0"}, > +}; > + > +static int mt8365_mt6357_int_adda_startup(struct snd_pcm_substream *substream) > +{ > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + struct mt8365_mt6357_priv *priv = snd_soc_card_get_drvdata(rtd->card); > + int ret = 0; > + > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { > + if (IS_ERR(priv->pin_states[PIN_STATE_MOSI_ON])) > + return ret; > + > + ret = pinctrl_select_state(priv->pinctrl, > + priv->pin_states[PIN_STATE_MOSI_ON]); > + if (ret) > + dev_err(rtd->card->dev, "%s failed to select state %d\n", > + __func__, ret); > + } > + > + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { > + if (IS_ERR(priv->pin_states[PIN_STATE_MISO_ON])) > + return ret; > + > + ret = pinctrl_select_state(priv->pinctrl, > + priv->pin_states[PIN_STATE_MISO_ON]); > + if (ret) > + dev_err(rtd->card->dev, "%s failed to select state %d\n", > + __func__, ret); > + } > + > + return 0; > +} > + > +static void mt8365_mt6357_int_adda_shutdown(struct snd_pcm_substream *substream) > +{ > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + struct mt8365_mt6357_priv *priv = snd_soc_card_get_drvdata(rtd->card); > + int ret = 0; > + > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { > + if (IS_ERR(priv->pin_states[PIN_STATE_MOSI_OFF])) > + return; > + > + ret = pinctrl_select_state(priv->pinctrl, > + priv->pin_states[PIN_STATE_MOSI_OFF]); > + if (ret) > + dev_err(rtd->card->dev, "%s failed to select state %d\n", > + __func__, ret); > + } > + > + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { > + if (IS_ERR(priv->pin_states[PIN_STATE_MISO_OFF])) > + return; > + > + ret = pinctrl_select_state(priv->pinctrl, > + priv->pin_states[PIN_STATE_MISO_OFF]); > + if (ret) > + dev_err(rtd->card->dev, "%s failed to select state %d\n", > + __func__, ret); > + } > + > +} > + > +static const struct snd_soc_ops mt8365_mt6357_int_adda_ops = { > + .startup = mt8365_mt6357_int_adda_startup, > + .shutdown = mt8365_mt6357_int_adda_shutdown, > +}; > + > +SND_SOC_DAILINK_DEFS(playback1, > + DAILINK_COMP_ARRAY(COMP_CPU("DL1")), > + DAILINK_COMP_ARRAY(COMP_DUMMY()), > + DAILINK_COMP_ARRAY(COMP_EMPTY())); > +SND_SOC_DAILINK_DEFS(playback2, > + DAILINK_COMP_ARRAY(COMP_CPU("DL2")), > + DAILINK_COMP_ARRAY(COMP_DUMMY()), > + DAILINK_COMP_ARRAY(COMP_EMPTY())); > +SND_SOC_DAILINK_DEFS(awb_capture, > + DAILINK_COMP_ARRAY(COMP_CPU("AWB")), > + DAILINK_COMP_ARRAY(COMP_DUMMY()), > + DAILINK_COMP_ARRAY(COMP_EMPTY())); > +SND_SOC_DAILINK_DEFS(vul, > + DAILINK_COMP_ARRAY(COMP_CPU("VUL")), > + DAILINK_COMP_ARRAY(COMP_DUMMY()), > + DAILINK_COMP_ARRAY(COMP_EMPTY())); > + > +SND_SOC_DAILINK_DEFS(i2s3, > + DAILINK_COMP_ARRAY(COMP_CPU("2ND I2S")), > + DAILINK_COMP_ARRAY(COMP_DUMMY()), > + DAILINK_COMP_ARRAY(COMP_EMPTY())); > +SND_SOC_DAILINK_DEFS(dmic, > + DAILINK_COMP_ARRAY(COMP_CPU("DMIC")), > + DAILINK_COMP_ARRAY(COMP_DUMMY()), > + DAILINK_COMP_ARRAY(COMP_EMPTY())); > +SND_SOC_DAILINK_DEFS(primary_codec, > + DAILINK_COMP_ARRAY(COMP_CPU("INT ADDA")), > + DAILINK_COMP_ARRAY(COMP_CODEC("mt6357-sound", "mt6357-snd-codec-aif1")), > + DAILINK_COMP_ARRAY(COMP_EMPTY())); > + > +/* Digital audio interface glue - connects codec <---> CPU */ > +static struct snd_soc_dai_link mt8365_mt6357_dais[] = { > + /* Front End DAI links */ > + [DAI_LINK_DL1_PLAYBACK] = { > + .name = "DL1_FE", > + .stream_name = "MultiMedia1_PLayback", > + .id = DAI_LINK_DL1_PLAYBACK, > + .trigger = { > + SND_SOC_DPCM_TRIGGER_POST, > + SND_SOC_DPCM_TRIGGER_POST > + }, > + .dynamic = 1, > + .dpcm_playback = 1, > + .dpcm_merged_rate = 1, > + SND_SOC_DAILINK_REG(playback1), > + }, > + [DAI_LINK_DL2_PLAYBACK] = { > + .name = "DL2_FE", > + .stream_name = "MultiMedia2_PLayback", > + .id = DAI_LINK_DL2_PLAYBACK, > + .trigger = { > + SND_SOC_DPCM_TRIGGER_POST, > + SND_SOC_DPCM_TRIGGER_POST > + }, > + .dynamic = 1, > + .dpcm_playback = 1, > + .dpcm_merged_rate = 1, > + SND_SOC_DAILINK_REG(playback2), > + }, > + [DAI_LINK_AWB_CAPTURE] = { > + .name = "AWB_FE", > + .stream_name = "DL1_AWB_Record", > + .id = DAI_LINK_AWB_CAPTURE, > + .trigger = { > + SND_SOC_DPCM_TRIGGER_POST, > + SND_SOC_DPCM_TRIGGER_POST > + }, > + .dynamic = 1, > + .dpcm_capture = 1, > + .dpcm_merged_rate = 1, > + SND_SOC_DAILINK_REG(awb_capture), > + }, > + [DAI_LINK_VUL_CAPTURE] = { > + .name = "VUL_FE", > + .stream_name = "MultiMedia1_Capture", > + .id = DAI_LINK_VUL_CAPTURE, > + .trigger = { > + SND_SOC_DPCM_TRIGGER_POST, > + SND_SOC_DPCM_TRIGGER_POST > + }, > + .dynamic = 1, > + .dpcm_capture = 1, > + .dpcm_merged_rate = 1, > + SND_SOC_DAILINK_REG(vul), > + }, > + /* Back End DAI links */ > + [DAI_LINK_2ND_I2S_INTF] = { > + .name = "2ND I2S BE", > + .no_pcm = 1, > + .id = DAI_LINK_2ND_I2S_INTF, > + .dai_fmt = SND_SOC_DAIFMT_I2S | > + SND_SOC_DAIFMT_NB_NF | > + SND_SOC_DAIFMT_CBS_CFS, > + .dpcm_playback = 1, > + .dpcm_capture = 1, > + SND_SOC_DAILINK_REG(i2s3), > + }, > + [DAI_LINK_DMIC] = { > + .name = "DMIC BE", > + .no_pcm = 1, > + .id = DAI_LINK_DMIC, > + .dpcm_capture = 1, > + SND_SOC_DAILINK_REG(dmic), > + }, > + [DAI_LINK_INT_ADDA] = { > + .name = "MTK Codec", > + .no_pcm = 1, > + .id = DAI_LINK_INT_ADDA, > + .dpcm_playback = 1, > + .dpcm_capture = 1, > + .ops = &mt8365_mt6357_int_adda_ops, > + SND_SOC_DAILINK_REG(primary_codec), > + }, > +}; > + > +static int pinctrl_setup(struct snd_soc_card *card, > + struct mt8365_mt6357_priv *priv, > + unsigned int pin_id) > +{ > + int ret = 0; > + /* if pin exist */ > + if (!IS_ERR(priv->pin_states[pin_id])) { > + ret = pinctrl_select_state(priv->pinctrl, > + priv->pin_states[pin_id]); > + if (ret) > + dev_err(card->dev, > + "%s failed to select state %d\n", > + __func__, ret); > + } > + return ret; > +} > + > +static int mt8365_mt6357_gpio_probe(struct snd_soc_card *card) > +{ > + struct mt8365_mt6357_priv *priv = snd_soc_card_get_drvdata(card); > + int ret = 0; > + int i; > + > + priv->pinctrl = devm_pinctrl_get(card->dev); > + if (IS_ERR(priv->pinctrl)) { > + ret = PTR_ERR(priv->pinctrl); > + dev_err(card->dev, "%s devm_pinctrl_get failed %d\n", > + __func__, ret); > + return ret; > + } > + > + for (i = 0 ; i < PIN_STATE_MAX ; i++) { > + priv->pin_states[i] = pinctrl_lookup_state(priv->pinctrl, > + mt8365_mt6357_pin_str[i]); > + if (IS_ERR(priv->pin_states[i])) { > + ret = PTR_ERR(priv->pin_states[i]); > + dev_err(card->dev, "%s Can't find pin state %s %d\n", > + __func__, mt8365_mt6357_pin_str[i], ret); > + } > + } > + > + /* Setup pins */ for (i = PIN_STATE_DEFAULT; i < PIN_STATE_MAX; i++) { /* Are you sure about that?!?! */ if (IS_ERR(priv->pin_states[i])) continue; ret = pinctrl_select_state(...) if (ret) { /* Should you go back to default? */ pinctrl_select_state(card, priv, PIN_STATE_DEFAULT); return dev_err_probe(card->dev, ret, "ARGH!\n"); /* :-) */ } }; > + ret |= pinctrl_setup(card, priv, PIN_STATE_DEFAULT); ....because OR'in return values is a big no. > + ret |= pinctrl_setup(card, priv, PIN_STATE_DMIC); > + ret |= pinctrl_setup(card, priv, PIN_STATE_MISO_OFF); > + ret |= pinctrl_setup(card, priv, PIN_STATE_MOSI_OFF); > + return 0; > + return ret; > +} > + > +static struct snd_soc_card mt8365_mt6357_card = { > + .name = "mt8365-evk", > + .owner = THIS_MODULE, > + .dai_link = mt8365_mt6357_dais, > + .num_links = ARRAY_SIZE(mt8365_mt6357_dais), > + .dapm_widgets = mt8365_mt6357_widgets, > + .num_dapm_widgets = ARRAY_SIZE(mt8365_mt6357_widgets), > + .dapm_routes = mt8365_mt6357_routes, > + .num_dapm_routes = ARRAY_SIZE(mt8365_mt6357_routes), > +}; > + > +static int mt8365_mt6357_dev_probe(struct platform_device *pdev) > +{ > + struct snd_soc_card *card = &mt8365_mt6357_card; > + struct device *dev = &pdev->dev; > + struct device_node *platform_node; > + struct mt8365_mt6357_priv *priv; > + int i, ret; > + > + card->dev = dev; > + ret = parse_dai_link_info(card); > + if (ret) > + goto err; > + > + platform_node = of_parse_phandle(dev->of_node, "mediatek,platform", 0); > + if (!platform_node) { > + dev_err(dev, "Property 'platform' missing or invalid\n"); > + return -EINVAL; > + } > + > + for (i = 0; i < card->num_links; i++) { > + if (mt8365_mt6357_dais[i].platforms->name) > + continue; > + mt8365_mt6357_dais[i].platforms->of_node = platform_node; > + } > + > + priv = devm_kzalloc(dev, sizeof(struct mt8365_mt6357_priv), > + GFP_KERNEL); > + if (!priv) { > + ret = -ENOMEM; > + dev_err(dev, "%s allocate card private data fail %d\n", > + __func__, ret); > + return ret; > + } > + > + snd_soc_card_set_drvdata(card, priv); > + > + mt8365_mt6357_gpio_probe(card); P.S.: with my cleanup this function would not need any parse_dai_link_info() call, nor phandle parsing, nor platform_node assignment, nor any call to devm_snd_soc_register_card(), nor.... ...practically you'd need to only allocate your priv (granted that you are not moving the gpio_probe() to the AFE driver) and the actual gpio_probe() call. But I'm not pressing to wait, anyway. Cheers, Angelo
Il 26/02/24 15:01, amergnat@baylibre.com ha scritto: > From: Nicolas Belin <nbelin@baylibre.com> > > Add the support of MT6357 PMIC audio codec. > > Signed-off-by: Nicolas Belin <nbelin@baylibre.com> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > sound/soc/codecs/Kconfig | 7 + > sound/soc/codecs/Makefile | 2 + > sound/soc/codecs/mt6357.c | 1805 +++++++++++++++++++++++++++++++++++++++++++++ > sound/soc/codecs/mt6357.h | 674 +++++++++++++++++ > 4 files changed, 2488 insertions(+) > > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig > index 59f9742e9ff4..9cf2b9b70472 100644 > --- a/sound/soc/codecs/Kconfig > +++ b/sound/soc/codecs/Kconfig > @@ -153,6 +153,7 @@ config SND_SOC_ALL_CODECS > imply SND_SOC_MC13783 > imply SND_SOC_ML26124 > imply SND_SOC_MT6351 > + imply SND_SOC_MT6357 > imply SND_SOC_MT6358 > imply SND_SOC_MT6359 > imply SND_SOC_MT6660 > @@ -2361,6 +2362,12 @@ config SND_SOC_ML26124 > config SND_SOC_MT6351 > tristate "MediaTek MT6351 Codec" > > +config SND_SOC_MT6357 > + tristate "MediaTek MT6357 Codec" > + help > + Enable support for the platform which uses MT6357 as > + external codec device. > + > config SND_SOC_MT6358 > tristate "MediaTek MT6358 Codec" > help > diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile > index f53baa2b9565..33e27612867e 100644 > --- a/sound/soc/codecs/Makefile > +++ b/sound/soc/codecs/Makefile > @@ -169,6 +169,7 @@ snd-soc-ml26124-objs := ml26124.o > 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-mt6357-objs := mt6357.o > snd-soc-mt6358-objs := mt6358.o > snd-soc-mt6359-objs := mt6359.o > snd-soc-mt6359-accdet-objs := mt6359-accdet.o > @@ -554,6 +555,7 @@ obj-$(CONFIG_SND_SOC_ML26124) += snd-soc-ml26124.o > 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_MT6357) += snd-soc-mt6357.o > obj-$(CONFIG_SND_SOC_MT6358) += snd-soc-mt6358.o > obj-$(CONFIG_SND_SOC_MT6359) += snd-soc-mt6359.o > obj-$(CONFIG_SND_SOC_MT6359_ACCDET) += mt6359-accdet.o > diff --git a/sound/soc/codecs/mt6357.c b/sound/soc/codecs/mt6357.c > new file mode 100644 > index 000000000000..13e95c227114 > --- /dev/null > +++ b/sound/soc/codecs/mt6357.c > @@ -0,0 +1,1805 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * MT6357 ALSA SoC audio codec driver > + * > + * Copyright (c) 2024 Baylibre > + * Author: Nicolas Belin <nbelin@baylibre.com> > + */ > + > +#include <linux/dma-mapping.h> > +#include <sound/soc.h> > +#include <sound/tlv.h> > +#include <linux/mfd/mt6397/core.h> > +#include <linux/regulator/consumer.h> > + > +#include "mt6357.h" > + > +static void set_playback_gpio(struct mt6357_priv *priv, bool enable) > +{ you're anyway always doing CLEAR_ALL, so you can do it outside of the if branch. regmap_write( ... CLEAR_ALL); if (enable) { ... } else { ... } > + if (enable) { > + /* set gpio mosi mode */ > + regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR, GPIO_MODE2_CLEAR_ALL); > + regmap_write(priv->regmap, MT6357_GPIO_MODE2_SET, GPIO8_MODE_SET_AUD_CLK_MOSI | > + GPIO9_MODE_SET_AUD_DAT_MOSI0 | > + GPIO10_MODE_SET_AUD_DAT_MOSI1 | > + GPIO11_MODE_SET_AUD_SYNC_MOSI); Are you sure that you need to write to MODE2_SET *and* to MODE2?! > + regmap_write(priv->regmap, MT6357_GPIO_MODE2, GPIO8_MODE_AUD_CLK_MOSI | > + GPIO9_MODE_AUD_DAT_MOSI0 | > + GPIO10_MODE_AUD_DAT_MOSI1 | > + GPIO11_MODE_AUD_SYNC_MOSI); > + } else { > + /* set pad_aud_*_mosi to GPIO mode and dir input > + * reason: > + * pad_aud_dat_mosi*, because the pin is used as boot strap > + */ > + regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR, GPIO_MODE2_CLEAR_ALL); > + regmap_write(priv->regmap, MT6357_GPIO_MODE2, GPIO8_MODE_GPIO | > + GPIO9_MODE_GPIO | > + GPIO10_MODE_GPIO | > + GPIO11_MODE_GPIO); > + regmap_update_bits(priv->regmap, MT6357_GPIO_DIR0, GPIO8_DIR_MASK | > + GPIO9_DIR_MASK | > + GPIO10_DIR_MASK | > + GPIO11_DIR_MASK, > + GPIO8_DIR_INPUT | > + GPIO9_DIR_INPUT | > + GPIO10_DIR_INPUT | > + GPIO11_DIR_INPUT); > + } > +} > + > +static void set_capture_gpio(struct mt6357_priv *priv, bool enable) > +{ same comment applies here. > + if (enable) { > + /* set gpio miso mode */ > + regmap_write(priv->regmap, MT6357_GPIO_MODE3_CLR, GPIO_MODE3_CLEAR_ALL); > + regmap_write(priv->regmap, MT6357_GPIO_MODE3_SET, GPIO12_MODE_SET_AUD_CLK_MISO | > + GPIO13_MODE_SET_AUD_DAT_MISO0 | > + GPIO14_MODE_SET_AUD_DAT_MISO1 | > + GPIO15_MODE_SET_AUD_SYNC_MISO); > + regmap_write(priv->regmap, MT6357_GPIO_MODE3, GPIO12_MODE_AUD_CLK_MISO | > + GPIO13_MODE_AUD_DAT_MISO0 | > + GPIO14_MODE_AUD_DAT_MISO1 | > + GPIO15_MODE_AUD_SYNC_MISO); > + } else { > + /* 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 > + */ > + regmap_write(priv->regmap, MT6357_GPIO_MODE3_CLR, GPIO_MODE3_CLEAR_ALL); > + regmap_write(priv->regmap, MT6357_GPIO_MODE3, GPIO12_MODE_GPIO | > + GPIO13_MODE_GPIO | > + GPIO14_MODE_GPIO | > + GPIO15_MODE_GPIO); > + regmap_update_bits(priv->regmap, MT6357_GPIO_DIR0, GPIO12_DIR_MASK | > + GPIO13_DIR_MASK | > + GPIO14_DIR_MASK | > + GPIO15_DIR_MASK, > + GPIO12_DIR_INPUT | > + GPIO13_DIR_INPUT | > + GPIO14_DIR_INPUT | > + GPIO15_DIR_INPUT); > + } > +} > + > +static void hp_main_output_ramp(struct mt6357_priv *priv, bool up) > +{ > + int i = 0, stage = 0; > + int target = 7; > + /* Enable/Reduce HPL/R main output stage step by step */ > + for (i = 0; i <= target; i++) { double 'i` initialization. > + stage = up ? i : target - i; > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON1, > + HPLOUT_STG_CTRL_VAUDP15_MASK, > + stage << HPLOUT_STG_CTRL_VAUDP15_SFT); > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON1, > + HPROUT_STG_CTRL_VAUDP15_MASK, > + stage << HPROUT_STG_CTRL_VAUDP15_SFT); > + usleep_range(600, 700); > + } > +} > + > +static void hp_aux_feedback_loop_gain_ramp(struct mt6357_priv *priv, bool up) > +{ > + int i = 0, stage = 0; > + /* Reduce HP aux feedback loop gain step by step */ > + for (i = 0; i <= 0xf; i++) { same > + stage = up ? i : 0xf - i; > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON6, > + HP_AUX_LOOP_GAIN_MASK, stage << HP_AUX_LOOP_GAIN_SFT); > + usleep_range(600, 700); > + } > +} > + > +static void hp_pull_down(struct mt6357_priv *priv, bool enable) > +{ > + if (enable) > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON2, > + HPP_SHORT_2VCM_VAUDP15_MASK, HPP_SHORT_2VCM_VAUDP15_ENABLE); > + else > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON2, > + HPP_SHORT_2VCM_VAUDP15_MASK, HPP_SHORT_2VCM_VAUDP15_DISABLE); > +} > + > +static bool is_valid_hp_pga_idx(int reg_idx) > +{ > + return (reg_idx >= DL_GAIN_8DB && reg_idx <= DL_GAIN_N_12DB) || reg_idx == DL_GAIN_N_40DB; > +} > + > +static void volume_ramp(struct mt6357_priv *priv, int lfrom, int lto, > + int rfrom, int rto, unsigned int reg_addr) > +{ > + int lcount, rcount, sleep = 0; > + > + if (!is_valid_hp_pga_idx(lfrom) || !is_valid_hp_pga_idx(lto)) > + pr_debug("%s(), invalid left volume index, from %d, to %d\n", > + __func__, lfrom, lto); > + > + if (!is_valid_hp_pga_idx(rfrom) || !is_valid_hp_pga_idx(rto)) > + pr_debug("%s(), invalid right volume index, from %d, to %d\n", > + __func__, rfrom, rto); > + > + if (lto > lfrom) > + lcount = 1; > + else > + lcount = -1; > + > + if (rto > rfrom) > + rcount = 1; > + else > + rcount = -1; > + > + while ((lto != lfrom) || (rto != rfrom)) { > + if (lto != lfrom) { > + lfrom += lcount; > + if (is_valid_hp_pga_idx(lfrom)) { > + regmap_update_bits(priv->regmap, reg_addr, DL_GAIN_REG_LEFT_MASK, > + lfrom << DL_GAIN_REG_LEFT_SHIFT); > + sleep = 1; > + } > + } > + if (rto != rfrom) { > + rfrom += rcount; > + if (is_valid_hp_pga_idx(rfrom)) { > + regmap_update_bits(priv->regmap, reg_addr, DL_GAIN_REG_RIGHT_MASK, > + rfrom << DL_GAIN_REG_RIGHT_SHIFT); > + sleep = 1; > + } > + } > + if (sleep) > + usleep_range(200, 300); > + } > +} > + > +static void lo_volume_ramp(struct mt6357_priv *priv, int lfrom, int lto, int rfrom, int rto) > +{ > + volume_ramp(priv, lfrom, lto, rfrom, rto, MT6357_ZCD_CON1); > +} > + > +static void hp_volume_ramp(struct mt6357_priv *priv, int lfrom, int lto, int rfrom, int rto) > +{ > + volume_ramp(priv, lfrom, lto, rfrom, rto, MT6357_ZCD_CON2); > +} > + > +static void hs_volume_ramp(struct mt6357_priv *priv, int from, int to) > +{ > + volume_ramp(priv, from, to, 0, 0, MT6357_ZCD_CON3); > +} > + > +static int mt6357_put_volsw(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(component); > + struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; > + unsigned int reg; > + int ret; > + > + ret = snd_soc_put_volsw(kcontrol, ucontrol); > + if (ret < 0) > + return ret; > + regmap_read(priv->regmap, mc->reg, ®) switch (mc->reg) { ... }; return 0; > + switch (mc->reg) { > + case MT6357_ZCD_CON2: > + regmap_read(priv->regmap, MT6357_ZCD_CON2, ®); > + priv->ana_gain[ANALOG_VOLUME_HPOUTL] = > + (reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT; > + priv->ana_gain[ANALOG_VOLUME_HPOUTR] = > + (reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT; > + break; > + case MT6357_ZCD_CON1: > + regmap_read(priv->regmap, MT6357_ZCD_CON1, ®); > + priv->ana_gain[ANALOG_VOLUME_LINEOUTL] = > + (reg & AUD_LOL_GAIN_MASK) >> AUD_LOL_GAIN_SFT; > + priv->ana_gain[ANALOG_VOLUME_LINEOUTR] = > + (reg & AUD_LOR_GAIN_MASK) >> AUD_LOR_GAIN_SFT; > + break; > + case MT6357_ZCD_CON3: > + regmap_read(priv->regmap, MT6357_ZCD_CON3, ®); > + priv->ana_gain[ANALOG_VOLUME_HSOUT] = > + (reg & AUD_HS_GAIN_MASK) >> AUD_HS_GAIN_SFT; > + break; > + case MT6357_AUDENC_ANA_CON0: > + case MT6357_AUDENC_ANA_CON1: > + regmap_read(priv->regmap, MT6357_AUDENC_ANA_CON0, ®); > + priv->ana_gain[ANALOG_VOLUME_MIC1] = > + (reg & AUDPREAMPLGAIN_MASK) >> AUDPREAMPLGAIN_SFT; > + regmap_read(priv->regmap, MT6357_AUDENC_ANA_CON1, ®); > + priv->ana_gain[ANALOG_VOLUME_MIC2] = > + (reg & AUDPREAMPRGAIN_MASK) >> AUDPREAMPRGAIN_SFT; > + break; > + } > + > + return ret; > +} > + ..snip.. > + > +static int adc_enable_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 mt6357_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: > + /* enable aud_pad TX fifos */ > + regmap_update_bits(priv->regmap, MT6357_AFE_AUD_PAD_TOP, > + AUD_PAD_TX_FIFO_NORMAL_PATH_MASK, > + AUD_PAD_TX_FIFO_NORMAL_PATH_ENABLE); > + /* UL turn on */ > + regmap_update_bits(priv->regmap, MT6357_AFE_UL_SRC_CON0_L, > + UL_SRC_ON_TMP_CTL_MASK, UL_SRC_ENABLE); > + /* Wait to avoid any pop noises */ > + msleep(100); > + //set the mic gains to the stored values Keep the comments style consistent please. > + regmap_update_bits(priv->regmap, MT6357_AUDENC_ANA_CON0, AUDPREAMPLGAIN_MASK, > + priv->ana_gain[ANALOG_VOLUME_MIC1] << AUDPREAMPLGAIN_SFT); > + regmap_update_bits(priv->regmap, MT6357_AUDENC_ANA_CON1, AUDPREAMPRGAIN_MASK, > + priv->ana_gain[ANALOG_VOLUME_MIC2] << AUDPREAMPRGAIN_SFT); > + break; > + case SND_SOC_DAPM_POST_PMD: > + /* UL turn off */ > + regmap_update_bits(priv->regmap, MT6357_AFE_UL_SRC_CON0_L, > + UL_SRC_ON_TMP_CTL_MASK, UL_SRC_DISABLE); > + /* disable aud_pad TX fifos */ > + regmap_update_bits(priv->regmap, MT6357_AFE_AUD_PAD_TOP, > + AUD_PAD_TX_FIFO_NORMAL_PATH_MASK, > + AUD_PAD_TX_FIFO_NORMAL_PATH_DISABLE); > + break; > + default: > + break; > + } > + > + return 0; > +} > + ..snip.. > + > +static int lol_mux_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 mt6357_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_POST_PMU: > + /* Set LO STB enhance circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON4, > + AUD_LOLOUT_STB_ENH_VAUDP15_MASK, > + AUD_LOLOUT_STB_ENH_VAUDP15_ENABLE); > + /* Enable LO driver bias circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON4, > + AUD_LOL_PWRUP_BIAS_VAUDP15_MASK, > + AUD_LOL_PWRUP_BIAS_VAUDP15_ENABLE); > + /* Enable LO driver core circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON4, > + AUD_LOL_PWRUP_VAUDP15_MASK, AUD_LOL_PWRUP_VAUDP15_ENABLE); > + /* Set LOL gain to normal gain step by step */ > + lo_volume_ramp(priv, DL_GAIN_N_40DB, priv->ana_gain[ANALOG_VOLUME_LINEOUTL], > + DL_GAIN_N_40DB, priv->ana_gain[ANALOG_VOLUME_LINEOUTR]); > + break; > + case SND_SOC_DAPM_PRE_PMD: > + Drop extra blank line. > + /* decrease LOL gain to minimum gain step by step */ > + lo_volume_ramp(priv, priv->ana_gain[ANALOG_VOLUME_LINEOUTL], DL_GAIN_N_40DB, > + priv->ana_gain[ANALOG_VOLUME_LINEOUTR], DL_GAIN_N_40DB); > + /* Disable LO driver core circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON4, > + AUD_LOL_PWRUP_VAUDP15_MASK, AUD_LOL_PWRUP_VAUDP15_DISABLE); > + /* Disable LO driver bias circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON4, > + AUD_LOL_PWRUP_BIAS_VAUDP15_MASK, > + AUD_LOL_PWRUP_BIAS_VAUDP15_DISABLE); > + /* Clear LO STB enhance circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON4, > + AUD_LOLOUT_STB_ENH_VAUDP15_MASK, > + AUD_LOLOUT_STB_ENH_VAUDP15_DISABLE); > + break; > + default: > + break; > + } > + > + return 0; > +} > + > +static int hs_mux_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 mt6357_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_POST_PMU: > + > + /* Set HS STB enhance circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON3, > + AUD_HSOUT_STB_ENH_VAUDP15_MASK, > + AUD_HSOUT_STB_ENH_VAUDP15_ENABLE); > + /* Enable HS driver bias circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON3, > + AUD_HS_PWRUP_BIAS_VAUDP15_MASK, > + AUD_HS_PWRUP_BIAS_VAUDP15_ENABLE); > + /* Enable HS driver core circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON3, > + AUD_HS_PWRUP_VAUDP15_MASK, AUD_HS_PWRUP_VAUDP15_ENABLE); > + /* Set HS gain to normal gain step by step */ > + hs_volume_ramp(priv, DL_GAIN_N_40DB, priv->ana_gain[ANALOG_VOLUME_HSOUT]); > + break; > + case SND_SOC_DAPM_PRE_PMD: > + same > + /* decrease HS gain to minimum gain step by step */ > + hs_volume_ramp(priv, priv->ana_gain[ANALOG_VOLUME_HSOUT], DL_GAIN_N_40DB); > + /* Disable HS driver core circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON3, > + AUD_HS_PWRUP_VAUDP15_MASK, AUD_HS_PWRUP_VAUDP15_DISABLE); > + /* Disable HS driver bias circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON3, > + AUD_HS_PWRUP_BIAS_VAUDP15_MASK, > + AUD_HS_PWRUP_BIAS_VAUDP15_ENABLE); > + /* Clear HS STB enhance circuits */ > + regmap_update_bits(priv->regmap, MT6357_AUDDEC_ANA_CON3, > + AUD_HSOUT_STB_ENH_VAUDP15_MASK, > + AUD_HSOUT_STB_ENH_VAUDP15_DISABLE); > + break; > + default: > + break; > + } > + > + return 0; > +} > + ..snip.. > + > +static unsigned int mt6357_read(struct snd_soc_component *codec, unsigned int reg) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + unsigned int val; > + > + pr_debug("%s() reg = 0x%x", __func__, reg); I'm not sure that you really need that pr_debug().... > + regmap_read(priv->regmap, reg, &val); > + return val; > +} > + > +static int mt6357_write(struct snd_soc_component *codec, unsigned int reg, unsigned int value) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + > + pr_debug("%s() reg = 0x%x value= 0x%x\n", __func__, reg, value); ...nor this one. > + regmap_update_bits(priv->regmap, reg, 0xffff, value); > + return 0; return regmap_update_bits(...); > +} > + > +static const struct snd_soc_component_driver mt6357_soc_component_driver = { > + .probe = mt6357_codec_probe, > + .read = mt6357_read, > + .write = mt6357_write, > + .controls = mt6357_controls, > + .num_controls = ARRAY_SIZE(mt6357_controls), > + .dapm_widgets = mt6357_dapm_widgets, > + .num_dapm_widgets = ARRAY_SIZE(mt6357_dapm_widgets), > + .dapm_routes = mt6357_dapm_routes, > + .num_dapm_routes = ARRAY_SIZE(mt6357_dapm_routes), > +}; > + > +static void mt6357_parse_dt(struct mt6357_priv *priv) > +{ > + int ret, voltage_index; > + u32 micbias_voltage_index = 0; > + struct device *dev = priv->dev; > + > + priv->pull_down_needed = false; > + if (of_property_read_bool(dev->of_node, "mediatek,hp-pull-down")) > + priv->pull_down_needed = true; > + > + micbias_voltage_index = MT6357_MICBIAS0_DEFAULT_VOLTAGE_INDEX; > + ret = of_property_read_u32(dev->of_node, "mediatek,micbias0-microvolt", &voltage_index); > + if (!ret) > + micbias_voltage_index = voltage_index; > + > + regmap_update_bits(priv->regmap, MT6357_AUDENC_ANA_CON8, AUD_MICBIAS0_VREF_MASK, > + micbias_voltage_index << AUD_MICBIAS0_VREF_SFT); > + > + micbias_voltage_index = MT6357_MICBIAS1_DEFAULT_VOLTAGE_INDEX; > + ret = of_property_read_u32(dev->of_node, "mediatek,micbias1-microvolt", &voltage_index); > + if (!ret) > + micbias_voltage_index = voltage_index; > + > + regmap_update_bits(priv->regmap, MT6357_AUDENC_ANA_CON9, AUD_MICBIAS1_VREF_MASK, > + micbias_voltage_index << AUD_MICBIAS1_VREF_SFT); > +} > + > +static int mt6357_platform_driver_probe(struct platform_device *pdev) > +{ > + struct mt6357_priv *priv; > + struct mt6397_chip *mt6397 = dev_get_drvdata(pdev->dev.parent); struct mt6397_chip *mt6397 = dev_get_drvdata(pdev->dev.parent); struct mt6357_priv *priv; int ret; looks better :-) > + int ret; > + > + ret = devm_regulator_get_enable(&pdev->dev, "vaud28"); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, "failed to enable vaud28 regulator\n"); > + > + priv = devm_kzalloc(&pdev->dev, > + sizeof(struct mt6357_priv), > + GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + dev_set_drvdata(&pdev->dev, priv); > + priv->dev = &pdev->dev; > + > + priv->regmap = mt6397->regmap; > + if (IS_ERR(priv->regmap)) > + return PTR_ERR(priv->regmap); > + > + mt6357_parse_dt(priv); > + > + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(64); > + if (!pdev->dev.dma_mask) > + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > + > + return devm_snd_soc_register_component(&pdev->dev, > + &mt6357_soc_component_driver, > + mtk_6357_dai_codecs, > + ARRAY_SIZE(mtk_6357_dai_codecs)); > +} > + > +static const struct of_device_id mt6357_of_match[] = { > + {.compatible = "mediatek,mt6357-sound",}, > + {} { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, mt6357_of_match); > + > +static struct platform_driver mt6357_platform_driver = { > + .driver = { > + .name = "mt6357-sound", > + .of_match_table = mt6357_of_match, > + }, > + .probe = mt6357_platform_driver_probe, > +}; > + > +module_platform_driver(mt6357_platform_driver) > + > +MODULE_DESCRIPTION("MT6357 ALSA SoC codec driver"); > +MODULE_AUTHOR("Nicolas Belin <nbelin@baylibre.com>"); > +MODULE_LICENSE("GPL"); Cheers, Angelo
Il 26/02/24 15:01, amergnat@baylibre.com ha scritto: > From: Fabien Parent <fparent@baylibre.com> > > Add MT6357 codec entry in the MFD driver. > > Signed-off-by: Fabien Parent <fparent@baylibre.com> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > --- > drivers/mfd/mt6397-core.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c > index 4449dde05021..4fd4a2da5ad7 100644 > --- a/drivers/mfd/mt6397-core.c > +++ b/drivers/mfd/mt6397-core.c > @@ -141,6 +141,9 @@ static const struct mfd_cell mt6357_devs[] = { > .num_resources = ARRAY_SIZE(mt6357_rtc_resources), > .resources = mt6357_rtc_resources, > .of_compatible = "mediatek,mt6357-rtc", > + }, { > + .name = "mt6357-sound", > + .of_compatible = "mediatek,mt6357-sound" > }, { > .name = "mtk-pmic-keys", > .num_resources = ARRAY_SIZE(mt6357_keys_resources), >
On Mon, Feb 26, 2024 at 03:01:50PM +0100, amergnat@baylibre.com wrote: > index 000000000000..13e95c227114 > --- /dev/null > +++ b/sound/soc/codecs/mt6357.c > @@ -0,0 +1,1805 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * MT6357 ALSA SoC audio codec driver > + * Please use a C++ comment for the whole comment to make it clearer that this is intentional. > +static void set_playback_gpio(struct mt6357_priv *priv, bool enable) > +{ > + if (enable) { > + /* set gpio mosi mode */ > + regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR, GPIO_MODE2_CLEAR_ALL); > + regmap_write(priv->regmap, MT6357_GPIO_MODE2_SET, GPIO8_MODE_SET_AUD_CLK_MOSI | > + GPIO9_MODE_SET_AUD_DAT_MOSI0 | > + GPIO10_MODE_SET_AUD_DAT_MOSI1 | > + GPIO11_MODE_SET_AUD_SYNC_MOSI); This would be a lot more legible if you worked out the values to set and then had a single set of writes, currently the indentation makes it very hard to read. Similarly for other similar functions. > +static int mt6357_put_volsw(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(component); > + struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; > + unsigned int reg; > + int ret; > + > + ret = snd_soc_put_volsw(kcontrol, ucontrol); > + if (ret < 0) > + return ret; > + > + switch (mc->reg) { > + case MT6357_ZCD_CON2: > + regmap_read(priv->regmap, MT6357_ZCD_CON2, ®); > + priv->ana_gain[ANALOG_VOLUME_HPOUTL] = > + (reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT; > + priv->ana_gain[ANALOG_VOLUME_HPOUTR] = > + (reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT; > + break; It would probably be less code and would definitely be clearer and simpler to just read the values when we need them rather than constatly keeping a cache separate to the register cache. > + /* ul channel swap */ > + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0), On/off controls should end in Switch. > +static const char * const hslo_mux_map[] = { > + "Open", "DACR", "Playback", "Test mode" > +}; > + > +static int hslo_mux_map_value[] = { > + 0x0, 0x1, 0x2, 0x3, > +}; Why not just use a normal mux here, there's no missing values or reordering? Similarly for other muxes. > +static unsigned int mt6357_read(struct snd_soc_component *codec, unsigned int reg) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + unsigned int val; > + > + pr_debug("%s() reg = 0x%x", __func__, reg); > + regmap_read(priv->regmap, reg, &val); > + return val; > +} Remove these, there are vastly more logging facilities as standard in the regmap core. > +/* Reg bit defines */ > +/* MT6357_GPIO_DIR0 */ > +#define GPIO8_DIR_MASK BIT(8) > +#define GPIO8_DIR_INPUT 0 Please namespace your defines, these look very generic.
On 26/02/2024 15:01, amergnat@baylibre.com wrote: > From: Nicolas Belin <nbelin@baylibre.com> > > Add a specific soundcard for mt8365-evk. It supports audio jack > in/out, dmics, the amic and lineout. > > Signed-off-by: Nicolas Belin <nbelin@baylibre.com> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> ... > +static int mt8365_mt6357_dev_probe(struct platform_device *pdev) > +{ > + struct snd_soc_card *card = &mt8365_mt6357_card; > + struct device *dev = &pdev->dev; > + struct device_node *platform_node; > + struct mt8365_mt6357_priv *priv; > + int i, ret; > + > + card->dev = dev; > + ret = parse_dai_link_info(card); > + if (ret) > + goto err; > + > + platform_node = of_parse_phandle(dev->of_node, "mediatek,platform", 0); > + if (!platform_node) { > + dev_err(dev, "Property 'platform' missing or invalid\n"); > + return -EINVAL; > + } > + > + for (i = 0; i < card->num_links; i++) { > + if (mt8365_mt6357_dais[i].platforms->name) > + continue; > + mt8365_mt6357_dais[i].platforms->of_node = platform_node; > + } > + > + priv = devm_kzalloc(dev, sizeof(struct mt8365_mt6357_priv), > + GFP_KERNEL); > + if (!priv) { > + ret = -ENOMEM; > + dev_err(dev, "%s allocate card private data fail %d\n", > + __func__, ret); Don't print anything on memory allocations failures. Run coccinelle/coccicheck to detect such trivial issues. > + return ret; > + } > + > + snd_soc_card_set_drvdata(card, priv); > + > + mt8365_mt6357_gpio_probe(card); > + > + ret = devm_snd_soc_register_card(dev, card); > + if (ret) > + dev_err(dev, "%s snd_soc_register_card fail %d\n", > + __func__, ret); > +err: > + of_node_put(platform_node); > + clean_card_reference(card); > + return ret; > +} > + > +static const struct of_device_id mt8365_mt6357_dt_match[] = { > + { .compatible = "mediatek,mt8365-mt6357", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, mt8365_mt6357_dt_match); > + > +static struct platform_driver mt8365_mt6357_driver = { > + .driver = { > + .name = "mt8365_mt6357", > + .of_match_table = mt8365_mt6357_dt_match, > + .pm = &snd_soc_pm_ops, > + }, > + .probe = mt8365_mt6357_dev_probe, > +}; > + > +module_platform_driver(mt8365_mt6357_driver); > + > +/* Module information */ > +MODULE_DESCRIPTION("MT8365 EVK SoC machine driver"); > +MODULE_AUTHOR("Nicolas Belin <nbelin@baylibre.com>"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("mt8365 mt6357 soc card"); This does not look like correct alias. Drop. > Best regards, Krzysztof
On 26/02/2024 15:01, Alexandre Mergnat wrote: > Add mt8365 platform driver. > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> ... > + memif->substream = substream; > + > + snd_pcm_hw_constraint_step(substream->runtime, 0, > + SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 16); > + > + snd_soc_set_runtime_hwparams(substream, mtk_afe_hardware); > + > + ret = snd_pcm_hw_constraint_integer(runtime, > + SNDRV_PCM_HW_PARAM_PERIODS); > + if (ret < 0) > + dev_err(afe->dev, "snd_pcm_hw_constraint_integer failed\n"); > + > + mt8365_afe_enable_main_clk(afe); > + return ret; > +} > + > +static void mt8365_afe_fe_shutdown(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + int memif_num = snd_soc_rtd_to_cpu(rtd, 0)->id; > + struct mtk_base_afe_memif *memif = &afe->memif[memif_num]; > + > + dev_dbg(afe->dev, "%s %s\n", __func__, memif->data->name); Drop trace-like prints. Tracing is for this. Before upstreaming driver from vendor repository, such stuff should be cleaned. > + > + memif->substream = NULL; > + > + mt8365_afe_disable_main_clk(afe); > +} > + > +static int mt8365_afe_fe_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id; > + struct mt8365_control_data *ctrl_data = &afe_priv->ctrl_data; > + struct mtk_base_afe_memif *memif = &afe->memif[dai_id]; > + struct mt8365_fe_dai_data *fe_data = &afe_priv->fe_data[dai_id]; > + const size_t request_size = params_buffer_bytes(params); > + unsigned int channels = params_channels(params); > + unsigned int rate = params_rate(params); > + int ret, fs = 0; > + unsigned int base_end_offset = 8; > + > + dev_info(afe->dev, > + "%s %s period = %d rate = %d channels = %d\n", > + __func__, memif->data->name, params_period_size(params), > + rate, channels); > + > + if (dai_id == MT8365_AFE_MEMIF_VUL2) { > + if (!ctrl_data->bypass_cm1) > + /* configure cm1 */ > + mt8365_afe_configure_cm(afe, > + MT8365_CM1, channels, rate); > + else > + regmap_update_bits(afe->regmap, AFE_CM1_CON0, > + CM_AFE_CM1_VUL_SEL, CM_AFE_CM1_VUL_SEL); > + } else if (dai_id == MT8365_AFE_MEMIF_TDM_IN) { > + if (!ctrl_data->bypass_cm2) > + /* configure cm2 */ > + mt8365_afe_configure_cm(afe, > + MT8365_CM2, channels, rate); > + else > + regmap_update_bits(afe->regmap, AFE_CM2_CON0, > + CM_AFE_CM2_TDM_SEL, ~CM_AFE_CM2_TDM_SEL); > + > + base_end_offset = 4; > + } > + > + if (request_size > fe_data->sram_size) { > + ret = snd_pcm_lib_malloc_pages(substream, request_size); > + if (ret < 0) { > + dev_err(afe->dev, > + "%s %s malloc pages %zu bytes failed %d\n", > + __func__, memif->data->name, request_size, ret); > + return ret; > + } > + > + fe_data->use_sram = false; > + > + mt8365_afe_emi_clk_on(afe); > + } else { > + struct snd_dma_buffer *dma_buf = &substream->dma_buffer; > + > + dma_buf->dev.type = SNDRV_DMA_TYPE_DEV; > + dma_buf->dev.dev = substream->pcm->card->dev; > + dma_buf->area = (unsigned char *)fe_data->sram_vir_addr; > + dma_buf->addr = fe_data->sram_phy_addr; > + dma_buf->bytes = request_size; > + snd_pcm_set_runtime_buffer(substream, dma_buf); > + > + fe_data->use_sram = true; > + } > + > + memif->phys_buf_addr = lower_32_bits(substream->runtime->dma_addr); > + memif->buffer_size = substream->runtime->dma_bytes; > + > + /* start */ > + regmap_write(afe->regmap, memif->data->reg_ofs_base, > + memif->phys_buf_addr); > + /* end */ > + regmap_write(afe->regmap, > + memif->data->reg_ofs_base + base_end_offset, > + memif->phys_buf_addr + memif->buffer_size - 1); > + > + /* set channel */ > + if (memif->data->mono_shift >= 0) { > + unsigned int mono = (params_channels(params) == 1) ? 1 : 0; > + > + if (memif->data->mono_reg < 0) > + dev_info(afe->dev, "%s mono_reg is NULL\n", __func__); > + else > + regmap_update_bits(afe->regmap, memif->data->mono_reg, > + 1 << memif->data->mono_shift, > + mono << memif->data->mono_shift); > + } > + > + /* set rate */ > + if (memif->data->fs_shift < 0) > + return 0; > + > + fs = afe->memif_fs(substream, params_rate(params)); > + > + if (fs < 0) > + return -EINVAL; > + > + if (memif->data->fs_reg < 0) > + dev_info(afe->dev, "%s fs_reg is NULL\n", __func__); > + else > + regmap_update_bits(afe->regmap, memif->data->fs_reg, > + memif->data->fs_maskbit << memif->data->fs_shift, > + fs << memif->data->fs_shift); > + > + return 0; > +} > + > +static int mt8365_afe_fe_hw_free(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id; > + struct mtk_base_afe_memif *memif = &afe->memif[dai_id]; > + struct mt8365_fe_dai_data *fe_data = &afe_priv->fe_data[dai_id]; > + int ret = 0; > + > + dev_dbg(afe->dev, "%s %s\n", __func__, memif->data->name); Drop > + > + if (fe_data->use_sram) { > + snd_pcm_set_runtime_buffer(substream, NULL); > + } else { > + ret = snd_pcm_lib_free_pages(substream); > + > + mt8365_afe_emi_clk_off(afe); > + } > + > + return ret; > +} > + > +static int mt8365_afe_fe_prepare(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id; > + struct mtk_base_afe_memif *memif = &afe->memif[dai_id]; > + > + /* set format */ > + if (memif->data->hd_reg >= 0) { > + switch (substream->runtime->format) { > + case SNDRV_PCM_FORMAT_S16_LE: > + regmap_update_bits(afe->regmap, memif->data->hd_reg, > + 3 << memif->data->hd_shift, > + 0 << memif->data->hd_shift); > + break; > + case SNDRV_PCM_FORMAT_S32_LE: > + regmap_update_bits(afe->regmap, memif->data->hd_reg, > + 3 << memif->data->hd_shift, > + 3 << memif->data->hd_shift); > + > + if (dai_id == MT8365_AFE_MEMIF_TDM_IN) { > + regmap_update_bits(afe->regmap, > + memif->data->hd_reg, > + 3 << memif->data->hd_shift, > + 1 << memif->data->hd_shift); > + regmap_update_bits(afe->regmap, > + memif->data->hd_reg, > + 1 << memif->data->hd_align_mshift, > + 1 << memif->data->hd_align_mshift); > + } > + break; > + case SNDRV_PCM_FORMAT_S24_LE: > + regmap_update_bits(afe->regmap, memif->data->hd_reg, > + 3 << memif->data->hd_shift, > + 1 << memif->data->hd_shift); > + break; > + default: > + return -EINVAL; > + } > + } > + > + mt8365_afe_irq_direction_enable(afe, > + memif->irq_usage, > + MT8365_AFE_IRQ_DIR_MCU); > + > + return 0; > +} > + > +int mt8365_afe_fe_trigger(struct snd_pcm_substream *substream, int cmd, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id; > + struct mt8365_control_data *ctrl_data = &afe_priv->ctrl_data; > + > + switch (cmd) { > + case SNDRV_PCM_TRIGGER_START: > + case SNDRV_PCM_TRIGGER_RESUME: > + /* enable channel merge */ > + if (dai_id == MT8365_AFE_MEMIF_VUL2 && > + !ctrl_data->bypass_cm1) { > + regmap_update_bits(afe->regmap, > + AFE_CM1_CON0, > + CM_AFE_CM_ON, CM_AFE_CM_ON); > + } else if (dai_id == MT8365_AFE_MEMIF_TDM_IN && > + !ctrl_data->bypass_cm2) { > + regmap_update_bits(afe->regmap, > + AFE_CM2_CON0, > + CM_AFE_CM_ON, CM_AFE_CM_ON); > + } > + break; > + case SNDRV_PCM_TRIGGER_STOP: > + case SNDRV_PCM_TRIGGER_SUSPEND: > + /* disable channel merge */ > + if (dai_id == MT8365_AFE_MEMIF_VUL2 && > + !ctrl_data->bypass_cm1) { > + regmap_update_bits(afe->regmap, > + AFE_CM1_CON0, > + CM_AFE_CM_ON, ~CM_AFE_CM_ON); > + } else if (dai_id == MT8365_AFE_MEMIF_TDM_IN && > + !ctrl_data->bypass_cm2) { > + regmap_update_bits(afe->regmap, > + AFE_CM2_CON0, > + CM_AFE_CM_ON, ~CM_AFE_CM_ON); > + } > + break; > + default: > + break; > + } > + > + return mtk_afe_fe_trigger(substream, cmd, dai); > +} > + > +static int mt8365_afe_hw_gain1_startup(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + > + mt8365_afe_enable_main_clk(afe); > + return 0; > +} > + > +static void mt8365_afe_hw_gain1_shutdown(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + struct mt8365_be_dai_data *be = > + &afe_priv->be_data[dai->id - MT8365_AFE_BACKEND_BASE]; > + > + const unsigned int stream = substream->stream; Why do you const local scalars? What is the point of this variable anyway? > + > + if (be->prepared[stream]) { > + regmap_update_bits(afe->regmap, AFE_GAIN1_CON0, > + AFE_GAIN1_CON0_EN_MASK, 0); > + be->prepared[stream] = false; > + } > + mt8365_afe_disable_main_clk(afe); > +} > + > +static int mt8365_afe_hw_gain1_prepare(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_pcm_runtime * const runtime = substream->runtime; > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + struct mt8365_be_dai_data *be = > + &afe_priv->be_data[dai->id - MT8365_AFE_BACKEND_BASE]; > + > + const unsigned int rate = runtime->rate; > + const unsigned int stream = substream->stream; So this const is everywhere? How does it change it from code safety? No one expects to overwrite such variables anyway, so there is no increase in readability, either. > + int fs; > + unsigned int val1 = 0, val2 = 0; > + > + if (be->prepared[stream]) { > + dev_info(afe->dev, "%s prepared already\n", __func__); > + return 0; > + } > + > + fs = mt8365_afe_fs_timing(rate); > + regmap_update_bits(afe->regmap, AFE_GAIN1_CON0, > + AFE_GAIN1_CON0_MODE_MASK, (unsigned int)fs<<4); > + > + regmap_read(afe->regmap, AFE_GAIN1_CON1, &val1); > + regmap_read(afe->regmap, AFE_GAIN1_CUR, &val2); > + if ((val1 & AFE_GAIN1_CON1_MASK) != (val2 & AFE_GAIN1_CUR_MASK)) > + regmap_update_bits(afe->regmap, AFE_GAIN1_CUR, > + AFE_GAIN1_CUR_MASK, val1); > + > + regmap_update_bits(afe->regmap, AFE_GAIN1_CON0, > + AFE_GAIN1_CON0_EN_MASK, 1); > + be->prepared[stream] = true; > + > + return 0; > +} > + > +static const struct snd_pcm_hardware mt8365_hostless_hardware = { > + .info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | > + SNDRV_PCM_INFO_MMAP_VALID), > + .period_bytes_min = 256, > + .period_bytes_max = 4 * 48 * 1024, > + .periods_min = 2, > + .periods_max = 256, > + .buffer_bytes_max = 8 * 48 * 1024, > + .fifo_size = 0, > +}; > + > +/* dai ops */ > +static int mtk_dai_hostless_startup(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct snd_pcm_runtime *runtime = substream->runtime; > + int ret; > + > + snd_soc_set_runtime_hwparams(substream, &mt8365_hostless_hardware); > + > + ret = snd_pcm_hw_constraint_integer(runtime, > + SNDRV_PCM_HW_PARAM_PERIODS); > + if (ret < 0) > + dev_info(afe->dev, "snd_pcm_hw_constraint_integer failed\n"); > + return ret; > +} > + > +/* FE DAIs */ > +static const struct snd_soc_dai_ops mt8365_afe_fe_dai_ops = { > + .startup = mt8365_afe_fe_startup, > + .shutdown = mt8365_afe_fe_shutdown, > + .hw_params = mt8365_afe_fe_hw_params, > + .hw_free = mt8365_afe_fe_hw_free, > + .prepare = mt8365_afe_fe_prepare, > + .trigger = mt8365_afe_fe_trigger, > +}; > + > +static const struct snd_soc_dai_ops mt8365_dai_hostless_ops = { > + .startup = mtk_dai_hostless_startup, > +}; > + ... > +static int mt8365_afe_cm2_io_input_mux_get(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + ucontrol->value.integer.value[0] = mCM2Input; > + > + return 0; > +} > + > +static int mt8365_afe_cm2_io_input_mux_put(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) Run checkpatch --strict > +{ > + struct snd_soc_dapm_context *dapm = > + snd_soc_dapm_kcontrol_dapm(kcontrol); > + struct snd_soc_component *comp = snd_soc_dapm_to_component(dapm); > + struct mtk_base_afe *afe = snd_soc_component_get_drvdata(comp); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + int ret; > + > + mCM2Input = ucontrol->value.enumerated.item[0]; > + > + afe_priv->cm2_mux_input = mCM2Input; > + ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol); > + > + return ret; > +} > + ... > + > +static const struct regmap_config mt8365_afe_regmap_config = { > + .reg_bits = 32, > + .reg_stride = 4, > + .val_bits = 32, > + .max_register = MAX_REGISTER, > + .cache_type = REGCACHE_NONE, > +}; > + > +static irqreturn_t mt8365_afe_irq_handler(int irq, void *dev_id) > +{ > + struct mtk_base_afe *afe = dev_id; > + unsigned int reg_value; > + unsigned int mcu_irq_mask; > + int i, ret; > + > + ret = regmap_read(afe->regmap, AFE_IRQ_MCU_STATUS, ®_value); > + if (ret) { > + dev_err(afe->dev, "%s irq status err\n", __func__); I can imagine happy user seeing flooded dmesg on each IRQ with these messages... > + reg_value = AFE_IRQ_STATUS_BITS; > + goto err_irq; > + } > + > + ret = regmap_read(afe->regmap, AFE_IRQ_MCU_EN, &mcu_irq_mask); > + if (ret) { > + dev_err(afe->dev, "%s irq mcu_en err\n", __func__); > + reg_value = AFE_IRQ_STATUS_BITS; > + goto err_irq; > + } > + > + /* only clr cpu irq */ > + reg_value &= mcu_irq_mask; > + > + for (i = 0; i < MT8365_AFE_MEMIF_NUM; i++) { > + struct mtk_base_afe_memif *memif = &afe->memif[i]; > + struct mtk_base_afe_irq *mcu_irq; > + > + if (memif->irq_usage < 0) > + continue; > + > + mcu_irq = &afe->irqs[memif->irq_usage]; > + > + if (!(reg_value & (1 << mcu_irq->irq_data->irq_clr_shift))) > + continue; > + > + snd_pcm_period_elapsed(memif->substream); > + } > + > +err_irq: > + /* clear irq */ > + regmap_write(afe->regmap, AFE_IRQ_MCU_CLR, > + reg_value & AFE_IRQ_STATUS_BITS); > + > + return IRQ_HANDLED; > +} > + > +static int __maybe_unused mt8365_afe_runtime_suspend(struct device *dev) > +{ > + return 0; > +} > + > +static int mt8365_afe_runtime_resume(struct device *dev) > +{ > + return 0; > +} > + > +static int __maybe_unused mt8365_afe_suspend(struct device *dev) > +{ > + struct mtk_base_afe *afe = dev_get_drvdata(dev); > + struct regmap *regmap = afe->regmap; > + int i; > + > + mt8365_afe_enable_main_clk(afe); > + > + if (!afe->reg_back_up) > + afe->reg_back_up = > + devm_kcalloc(dev, afe->reg_back_up_list_num, > + sizeof(unsigned int), GFP_KERNEL); > + > + for (i = 0; i < afe->reg_back_up_list_num; i++) > + regmap_read(regmap, afe->reg_back_up_list[i], > + &afe->reg_back_up[i]); > + > + mt8365_afe_disable_main_clk(afe); > + > + return 0; > +} > + > +static int __maybe_unused mt8365_afe_resume(struct device *dev) > +{ > + struct mtk_base_afe *afe = dev_get_drvdata(dev); > + struct regmap *regmap = afe->regmap; > + int i = 0; > + > + if (!afe->reg_back_up) { > + dev_dbg(dev, "%s no reg_backup\n", __func__); > + return 0; > + } > + > + mt8365_afe_enable_main_clk(afe); > + > + for (i = 0; i < afe->reg_back_up_list_num; i++) > + regmap_write(regmap, afe->reg_back_up_list[i], > + afe->reg_back_up[i]); > + > + mt8365_afe_disable_main_clk(afe); > + > + return 0; > +} > + > +static int __maybe_unused mt8365_afe_dev_runtime_suspend(struct device *dev) > +{ > + struct mtk_base_afe *afe = dev_get_drvdata(dev); > + > + dev_dbg(afe->dev, "%s suspend %d %d >>\n", __func__, > + pm_runtime_status_suspended(dev), afe->suspended); Drop silly trace-like prints. Tracing is for this. > + > + if (pm_runtime_status_suspended(dev) || afe->suspended) > + return 0; > + > + mt8365_afe_suspend(dev); > + > + afe->suspended = true; > + > + dev_dbg(afe->dev, "%s <<\n", __func__); Drop silly trace-like prints. Tracing is for this. > + > + return 0; > +} > + > +static int __maybe_unused mt8365_afe_dev_runtime_resume(struct device *dev) > +{ > + struct mtk_base_afe *afe = dev_get_drvdata(dev); > + > + dev_dbg(afe->dev, "%s suspend %d %d >>\n", __func__, > + pm_runtime_status_suspended(dev), afe->suspended); Drop silly trace-like prints. Tracing is for this. > + > + if (pm_runtime_status_suspended(dev) || !afe->suspended) > + return 0; > + > + mt8365_afe_resume(dev); > + > + afe->suspended = false; > + > + dev_dbg(afe->dev, "%s <<\n", __func__); Drop silly trace-like prints. Tracing is for this. > + > + return 0; > +} > + > +static int mt8365_afe_init_registers(struct mtk_base_afe *afe) > +{ > + size_t i; > + > + static struct { > + unsigned int reg; > + unsigned int mask; > + unsigned int val; > + } init_regs[] = { > + { AFE_CONN_24BIT, GENMASK(31, 0), GENMASK(31, 0) }, > + { AFE_CONN_24BIT_1, GENMASK(21, 0), GENMASK(21, 0) }, > + }; > + > + mt8365_afe_enable_main_clk(afe); > + > + for (i = 0; i < ARRAY_SIZE(init_regs); i++) > + regmap_update_bits(afe->regmap, init_regs[i].reg, > + init_regs[i].mask, init_regs[i].val); > + > + mt8365_afe_disable_main_clk(afe); > + > + return 0; > +} > + > +static int mt8365_afe_component_probe(struct snd_soc_component *component) > +{ > + return mtk_afe_add_sub_dai_control(component); > +} > + > +static const struct snd_soc_component_driver mt8365_afe_component = { > + .name = AFE_PCM_NAME, > + .probe = mt8365_afe_component_probe, > + .pointer = mtk_afe_pcm_pointer, > + .pcm_construct = mtk_afe_pcm_new, > +}; > + > +static const struct snd_soc_component_driver mt8365_afe_pcm_component = { > + .name = "mt8365-afe-pcm-dai", > +}; > + > +static int mt8365_dai_memif_register(struct mtk_base_afe *afe) > +{ > + struct mtk_base_afe_dai *dai; > + > + dai = devm_kzalloc(afe->dev, sizeof(*dai), GFP_KERNEL); > + if (!dai) > + return -ENOMEM; > + > + list_add(&dai->list, &afe->sub_dais); > + > + dai->dai_drivers = mt8365_memif_dai_driver; > + dai->num_dai_drivers = ARRAY_SIZE(mt8365_memif_dai_driver); > + > + dai->dapm_widgets = mt8365_memif_widgets; > + dai->num_dapm_widgets = ARRAY_SIZE(mt8365_memif_widgets); > + dai->dapm_routes = mt8365_memif_routes; > + dai->num_dapm_routes = ARRAY_SIZE(mt8365_memif_routes); > + return 0; > +} > + > +typedef int (*dai_register_cb)(struct mtk_base_afe *); > +static const dai_register_cb dai_register_cbs[] = { > + mt8365_dai_pcm_register, > + mt8365_dai_i2s_register, > + mt8365_dai_adda_register, > + mt8365_dai_dmic_register, > + mt8365_dai_memif_register, > +}; > + > +static int mt8365_afe_pcm_dev_probe(struct platform_device *pdev) > +{ > + struct mtk_base_afe *afe; > + struct mt8365_afe_private *afe_priv; > + struct device *dev; > + int ret, i, sel_irq; > + unsigned int irq_id; > + struct resource *res; > + > + afe = devm_kzalloc(&pdev->dev, sizeof(*afe), GFP_KERNEL); > + if (!afe) > + return -ENOMEM; > + platform_set_drvdata(pdev, afe); > + > + afe->platform_priv = devm_kzalloc(&pdev->dev, sizeof(*afe_priv), > + GFP_KERNEL); > + if (!afe->platform_priv) > + return -ENOMEM; > + > + afe_priv = afe->platform_priv; > + afe->dev = &pdev->dev; > + dev = afe->dev; > + > + spin_lock_init(&afe_priv->afe_ctrl_lock); > + mutex_init(&afe_priv->afe_clk_mutex); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + afe->base_addr = devm_ioremap_resource(&pdev->dev, res); Why not using helper combining these two? > + if (IS_ERR(afe->base_addr)) > + return PTR_ERR(afe->base_addr); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (res) { > + afe_priv->afe_sram_vir_addr = > + devm_ioremap_resource(&pdev->dev, res); > + if (!IS_ERR(afe_priv->afe_sram_vir_addr)) { > + afe_priv->afe_sram_phy_addr = res->start; > + afe_priv->afe_sram_size = resource_size(res); > + } > + } > + > + /* initial audio related clock */ > + ret = mt8365_afe_init_audio_clk(afe); > + if (ret) { > + dev_err(afe->dev, "mt8365_afe_init_audio_clk fail\n"); return dev_err_probe() > + return ret; > + } > + > + afe->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "top_audio_sel", > + afe->base_addr, &mt8365_afe_regmap_config); > + if (IS_ERR(afe->regmap)) > + return PTR_ERR(afe->regmap); > + > + /* memif % irq initialize*/ > + afe->memif_size = MT8365_AFE_MEMIF_NUM; > + afe->memif = devm_kcalloc(afe->dev, afe->memif_size, > + sizeof(*afe->memif), GFP_KERNEL); > + if (!afe->memif) > + return -ENOMEM; > + > + afe->irqs_size = MT8365_AFE_IRQ_NUM; > + afe->irqs = devm_kcalloc(afe->dev, afe->irqs_size, > + sizeof(*afe->irqs), GFP_KERNEL); > + if (!afe->irqs) > + return -ENOMEM; > + > + for (i = 0; i < afe->irqs_size; i++) > + afe->irqs[i].irq_data = &irq_data[i]; > + > + irq_id = platform_get_irq(pdev, 0); > + if (!irq_id) { That does not look correct. 0 is valid. Look at help of this function. > + dev_err(afe->dev, "np %s no irq\n", afe->dev->of_node->name); return dev_err_probe() > + return -ENXIO; > + } > + ret = devm_request_irq(afe->dev, irq_id, mt8365_afe_irq_handler, > + 0, "Afe_ISR_Handle", (void *)afe); > + if (ret) { > + dev_err(afe->dev, "could not request_irq\n"); return dev_err_probe() > + return ret; > + } > + > + /* init sub_dais */ > + INIT_LIST_HEAD(&afe->sub_dais); > + > + for (i = 0; i < ARRAY_SIZE(dai_register_cbs); i++) { > + ret = dai_register_cbs[i](afe); > + if (ret) { > + dev_warn(afe->dev, "dai register i %d fail, ret %d\n", > + i, ret); > + return ret; > + } > + } > + > + /* init dai_driver and component_driver */ > + ret = mtk_afe_combine_sub_dai(afe); > + if (ret) { > + dev_warn(afe->dev, "mtk_afe_combine_sub_dai fail, ret %d\n", > + ret); > + return ret; > + } > + > + for (i = 0; i < afe->memif_size; i++) { > + afe->memif[i].data = &memif_data[i]; > + sel_irq = memif_specified_irqs[i]; > + if (sel_irq >= 0) { > + afe->memif[i].irq_usage = sel_irq; > + afe->memif[i].const_irq = 1; > + afe->irqs[sel_irq].irq_occupyed = true; > + } else { > + afe->memif[i].irq_usage = -1; > + } > + } > + > + afe->mtk_afe_hardware = &mt8365_afe_hardware; > + afe->memif_fs = mt8365_memif_fs; > + afe->irq_fs = mt8365_irq_fs; > + > + pm_runtime_enable(&pdev->dev); > + > + pm_runtime_get_sync(&pdev->dev); > + afe->reg_back_up_list = mt8365_afe_backup_list; > + afe->reg_back_up_list_num = ARRAY_SIZE(mt8365_afe_backup_list); > + afe->runtime_resume = mt8365_afe_runtime_resume; > + afe->runtime_suspend = mt8365_afe_runtime_suspend; > + > + /* open afe pdn for dapm read/write audio register */ > + mt8365_afe_enable_top_cg(afe, MT8365_TOP_CG_AFE); > + > + /* Set 26m parent clk */ > + mt8365_afe_set_clk_parent(afe, > + afe_priv->clocks[MT8365_CLK_TOP_AUD_SEL], > + afe_priv->clocks[MT8365_CLK_CLK26M]); > + > + ret = devm_snd_soc_register_component(&pdev->dev, > + &mt8365_afe_component, NULL, 0); > + if (ret) { > + dev_warn(dev, "err_platform\n"); > + return ret; > + } > + > + ret = devm_snd_soc_register_component(&pdev->dev, > + &mt8365_afe_pcm_component, > + afe->dai_drivers, > + afe->num_dai_drivers); > + if (ret) { > + dev_warn(dev, "err_dai_component\n"); > + return ret; > + } > + > + mt8365_afe_init_registers(afe); > + > + dev_info(&pdev->dev, "MT8365 AFE driver initialized.\n"); Drop simple probe success messages everywhere. They are not needed and they don't bring useful information. Best regards, Krzysztof
On 26/02/2024 15:01, Alexandre Mergnat wrote: > Add Digital Micro Device Audio Interface support for MT8365 SoC. > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > + > +static int init_dmic_priv_data(struct mtk_base_afe *afe) > +{ > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + struct mt8365_dmic_data *dmic_priv; > + struct device_node *np = afe->dev->of_node; > + unsigned int temps[4]; > + int ret; > + > + dmic_priv = devm_kzalloc(afe->dev, sizeof(struct mt8365_dmic_data), > + GFP_KERNEL); You have very inconsistent style of coding. Some patches are done correctly, some repeast known issues. All over. This is sizeof(*). This comment (and all others) apply everywhere, just in case. Best regards, Krzysztof
On Mon, Feb 26, 2024 at 03:01:38PM +0100, Alexandre Mergnat wrote: > This serie aim to add the following audio support for the Genio 350-evk: > - Playback > - 2ch Headset Jack (Earphone) > - 1ch Line-out Jack (Speaker) > - 8ch HDMI Tx > - Capture > - 1ch DMIC (On-board Digital Microphone) > - 1ch AMIC (On-board Analogic Microphone) > - 1ch Headset Jack (External Analogic Microphone) > > Of course, HDMI playback need the MT8365 display patches [1] and a DTS > change documented in "mediatek,mt8365-mt6357.yaml". Given the number of custom controls here could you please post the output of mixer-test and pcm-test from a system with this driver running next time you post, this will help with review since it checks a bunch of things around the new controls.
On Mon, 26 Feb 2024 15:01:51 +0100, amergnat@baylibre.com wrote: > Add MT6357 codec entry in the MFD driver. > > Applied, thanks! [13/18] mfd: mt6397-core: register mt6357 sound codec commit: 79d98102a31ab777b4a6632d799ab2bc63654cf8 -- Lee Jones [李琼斯]
On 27/02/2024 16:06, Mark Brown wrote: > On Mon, Feb 26, 2024 at 03:01:38PM +0100, Alexandre Mergnat wrote: >> This serie aim to add the following audio support for the Genio 350-evk: >> - Playback >> - 2ch Headset Jack (Earphone) >> - 1ch Line-out Jack (Speaker) >> - 8ch HDMI Tx >> - Capture >> - 1ch DMIC (On-board Digital Microphone) >> - 1ch AMIC (On-board Analogic Microphone) >> - 1ch Headset Jack (External Analogic Microphone) >> >> Of course, HDMI playback need the MT8365 display patches [1] and a DTS >> change documented in "mediatek,mt8365-mt6357.yaml". > > Given the number of custom controls here could you please post the > output of mixer-test and pcm-test from a system with this driver running > next time you post, this will help with review since it checks a bunch > of things around the new controls. Hi Mark, I'm a bit lost for mixer-test and pcm-test. Currently, I cross-compile the alsa lib project to be able to build the tests and put it on my board. I can execute it, but I still have 2 issues: 1) I've a lot of missing module in my environment (Encode.so, Encode.pm, Symbol.pm, IO/Handle.pm, ...). AFAII, I've to cross compile the missing perl modules and install them in the rootfs 2) I don't know how to configure pcm-test.conf & Lenovo_ThinkPad_P1_Gen2.conf (or new file to match with my board). My test cmd: ./run_kselftest.sh -c alsa I'm wondering if I'm going to the wrong way because I don't find guide/readme whereas it's not trivial at all.
On 26/02/2024 16:25, AngeloGioacchino Del Regno wrote: >> + if (enable) { >> + /* set gpio mosi mode */ >> + regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR, >> GPIO_MODE2_CLEAR_ALL); >> + regmap_write(priv->regmap, MT6357_GPIO_MODE2_SET, >> GPIO8_MODE_SET_AUD_CLK_MOSI | >> + GPIO9_MODE_SET_AUD_DAT_MOSI0 | >> + GPIO10_MODE_SET_AUD_DAT_MOSI1 | >> + GPIO11_MODE_SET_AUD_SYNC_MOSI); > > Are you sure that you need to write to MODE2_SET *and* to MODE2?! This is downstream code and these registers aren't in my documentation. I've removed the MODE2_SET write and test the audio: it's still working. So I will keep the spurious write removed for v2. :)
Il 12/03/24 15:50, Alexandre Mergnat ha scritto: > > > On 26/02/2024 16:25, AngeloGioacchino Del Regno wrote: >>> + if (enable) { >>> + /* set gpio mosi mode */ >>> + regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR, GPIO_MODE2_CLEAR_ALL); >>> + regmap_write(priv->regmap, MT6357_GPIO_MODE2_SET, >>> GPIO8_MODE_SET_AUD_CLK_MOSI | >>> + GPIO9_MODE_SET_AUD_DAT_MOSI0 | >>> + GPIO10_MODE_SET_AUD_DAT_MOSI1 | >>> + GPIO11_MODE_SET_AUD_SYNC_MOSI); >> >> Are you sure that you need to write to MODE2_SET *and* to MODE2?! > > This is downstream code and these registers aren't in my documentation. > I've removed the MODE2_SET write and test the audio: it's still working. > > So I will keep the spurious write removed for v2. :) > Usually, MediaTek registers are laid out like "REG" being R/legacy-W and "REG_SET/CLR" for setting and clearing bits in "REG" internally, and that might account for internal latencies and such. Can you please try to remove the MODE2 write instead of the MODE2_SET one and check if that works? You're already using the SETCLR way when manipulating registers in here, so I would confidently expect that to work. Cheers, Angelo
On 26/02/2024 17:09, Mark Brown wrote: > On Mon, Feb 26, 2024 at 03:01:50PM +0100, amergnat@baylibre.com wrote: > >> index 000000000000..13e95c227114 >> --- /dev/null >> +++ b/sound/soc/codecs/mt6357.c >> @@ -0,0 +1,1805 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * MT6357 ALSA SoC audio codec driver >> + * > > Please use a C++ comment for the whole comment to make it clearer that > this is intentional. ok > >> +static void set_playback_gpio(struct mt6357_priv *priv, bool enable) >> +{ >> + if (enable) { >> + /* set gpio mosi mode */ >> + regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR, GPIO_MODE2_CLEAR_ALL); >> + regmap_write(priv->regmap, MT6357_GPIO_MODE2_SET, GPIO8_MODE_SET_AUD_CLK_MOSI | >> + GPIO9_MODE_SET_AUD_DAT_MOSI0 | >> + GPIO10_MODE_SET_AUD_DAT_MOSI1 | >> + GPIO11_MODE_SET_AUD_SYNC_MOSI); > > This would be a lot more legible if you worked out the values to set and > then had a single set of writes, currently the indentation makes it very > hard to read. Similarly for other similar functions. ok > >> +static int mt6357_put_volsw(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) >> +{ >> + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); >> + struct mt6357_priv *priv = snd_soc_component_get_drvdata(component); >> + struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; >> + unsigned int reg; >> + int ret; >> + >> + ret = snd_soc_put_volsw(kcontrol, ucontrol); >> + if (ret < 0) >> + return ret; >> + >> + switch (mc->reg) { >> + case MT6357_ZCD_CON2: >> + regmap_read(priv->regmap, MT6357_ZCD_CON2, ®); >> + priv->ana_gain[ANALOG_VOLUME_HPOUTL] = >> + (reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT; >> + priv->ana_gain[ANALOG_VOLUME_HPOUTR] = >> + (reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT; >> + break; > > It would probably be less code and would definitely be clearer and > simpler to just read the values when we need them rather than constatly > keeping a cache separate to the register cache. Actually you must save the values because the gain selected by the user will be override to do a ramp => volume_ramp(.....): - When you switch on the HP, you start from gain=-40db to final_gain (selected by user). - When you switch off the HP, you start from final_gain (selected by user) to gain=-40db. Also, the microphone's gain change when it's enabled/disabled. So, it can implemented differently but currently it's aligned with the other MTK codecs and I don't see any resource wasted here. > >> + /* ul channel swap */ >> + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0), > > On/off controls should end in Switch. Sorry, I don't understand your comment. Can you reword it please ? > >> +static const char * const hslo_mux_map[] = { >> + "Open", "DACR", "Playback", "Test mode" >> +}; >> + >> +static int hslo_mux_map_value[] = { >> + 0x0, 0x1, 0x2, 0x3, >> +}; > > Why not just use a normal mux here, there's no missing values or > reordering? Similarly for other muxes. I've dug into some other codecs and it's done like that, but I've probably misunderstood something. The only bad thing I see is enum is missing currently: enum { PGA_MUX_OPEN = 0, PGA_MUX_DACR, PGA_MUX_PB, PGA_MUX_TM, PGA_MUX_MASK = 0x3, }; static const char * const hslo_mux_map[] = { "Open", "DACR", "Playback", "Test mode" }; static int hslo_mux_map_value[] = { PGA_MUX_OPEN, PGA_MUX_DACR, PGA_MUX_PB, PGA_MUX_TM, }; > >> +static unsigned int mt6357_read(struct snd_soc_component *codec, unsigned int reg) >> +{ >> + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); >> + unsigned int val; >> + >> + pr_debug("%s() reg = 0x%x", __func__, reg); >> + regmap_read(priv->regmap, reg, &val); >> + return val; >> +} > > Remove these, there are vastly more logging facilities as standard in > the regmap core. ok > >> +/* Reg bit defines */ >> +/* MT6357_GPIO_DIR0 */ >> +#define GPIO8_DIR_MASK BIT(8) >> +#define GPIO8_DIR_INPUT 0 > > Please namespace your defines, these look very generic. ok
On 26/02/2024 17:09, Mark Brown wrote: >> index 000000000000..13e95c227114 >> --- /dev/null >> +++ b/sound/soc/codecs/mt6357.c >> @@ -0,0 +1,1805 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * MT6357 ALSA SoC audio codec driver >> + * > Please use a C++ comment for the whole comment to make it clearer that > this is intentional. If I do that, the checkpatch raise a warning: WARNING: Improper SPDX comment style for 'sound/soc/mediatek/mt8365/mt8365-afe-clk.c', please use '//' instead #22: FILE: sound/soc/mediatek/mt8365/mt8365-afe-clk.c:1: +/* SPDX-License-Identifier: GPL-2.0 WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 #22: FILE: sound/soc/mediatek/mt8365/mt8365-afe-clk.c:1: +/* SPDX-License-Identifier: GPL-2.0 even if I put: /* SPDX-License-Identifier: GPL-2.0 */ IMO, the checkpatch tool should be fixed/update first.
On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote: > On 26/02/2024 17:09, Mark Brown wrote: > > > + case MT6357_ZCD_CON2: > > > + regmap_read(priv->regmap, MT6357_ZCD_CON2, ®); > > > + priv->ana_gain[ANALOG_VOLUME_HPOUTL] = > > > + (reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT; > > > + priv->ana_gain[ANALOG_VOLUME_HPOUTR] = > > > + (reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT; > > > + break; > > It would probably be less code and would definitely be clearer and > > simpler to just read the values when we need them rather than constatly > > keeping a cache separate to the register cache. > Actually you must save the values because the gain selected by the user will > be override to do a ramp => volume_ramp(.....): > - When you switch on the HP, you start from gain=-40db to final_gain > (selected by user). > - When you switch off the HP, you start from final_gain (selected by user) > to gain=-40db. You can just read the value back when you need to do a ramp? > Also, the microphone's gain change when it's enabled/disabled. I don't understand what this means? > > > + /* ul channel swap */ > > > + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0), > > On/off controls should end in Switch. > Sorry, I don't understand your comment. Can you reword it please ? See control-names.rst. Run mixer-test on a card with this driver and fix all the issues it reports. > > > +static int hslo_mux_map_value[] = { > > > + 0x0, 0x1, 0x2, 0x3, > > > +}; > > Why not just use a normal mux here, there's no missing values or > > reordering? Similarly for other muxes. > I've dug into some other codecs and it's done like that, but I've probably > misunderstood something. > The only bad thing I see is enum is missing currently: > > enum { > PGA_MUX_OPEN = 0, > PGA_MUX_DACR, > PGA_MUX_PB, > PGA_MUX_TM, > PGA_MUX_MASK = 0x3, > }; The whole thing with explicitly specfying the mapping is just completely redundant, you may as well remove it.
On Wed, Mar 13, 2024 at 06:11:50PM +0100, Alexandre Mergnat wrote: > On 26/02/2024 17:09, Mark Brown wrote: > > > index 000000000000..13e95c227114 > > > --- /dev/null > > > +++ b/sound/soc/codecs/mt6357.c > > > @@ -0,0 +1,1805 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * MT6357 ALSA SoC audio codec driver > > Please use a C++ comment for the whole comment to make it clearer that > > this is intentional. > If I do that, the checkpatch raise a warning: > WARNING: Improper SPDX comment style for > 'sound/soc/mediatek/mt8365/mt8365-afe-clk.c', please use '//' instead > #22: FILE: sound/soc/mediatek/mt8365/mt8365-afe-clk.c:1: > +/* SPDX-License-Identifier: GPL-2.0 That's not a C++ comment so checkpatch is correctly warning?
On 13/03/2024 18:23, Mark Brown wrote: > On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote: >> On 26/02/2024 17:09, Mark Brown wrote: > >>>> + case MT6357_ZCD_CON2: >>>> + regmap_read(priv->regmap, MT6357_ZCD_CON2, ®); >>>> + priv->ana_gain[ANALOG_VOLUME_HPOUTL] = >>>> + (reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT; >>>> + priv->ana_gain[ANALOG_VOLUME_HPOUTR] = >>>> + (reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT; >>>> + break; > >>> It would probably be less code and would definitely be clearer and >>> simpler to just read the values when we need them rather than constatly >>> keeping a cache separate to the register cache. > >> Actually you must save the values because the gain selected by the user will >> be override to do a ramp => volume_ramp(.....): >> - When you switch on the HP, you start from gain=-40db to final_gain >> (selected by user). >> - When you switch off the HP, you start from final_gain (selected by user) >> to gain=-40db. > > You can just read the value back when you need to do a ramp? You can't. Because you will read -40db when HP isn't playing sound. That is why the gain is saved into the struct. Let me know, when you change de gain to do a ramp down (start from user gain to gain=-40db), next time for the ramp up, how/where do you find the user gain ? > >> Also, the microphone's gain change when it's enabled/disabled. > > I don't understand what this means? When microphone isn't capturing, the gain read back from the register is 0dB. I've put some logs in my code and do capture to show how it works: root@i350-evk:~# arecord -D hw:mt8365evk,2,0 -r 48000 -c2 -f s32_le -d 10 recorded_file.wav [Mar15 09:31] mt8365-afe-pcm 11220000.audio-controller: mt8365_afe_fe_hw_params AWB period = 6000 rate = 48000 channels = 2 [ +0.000126] mt8365-afe-pcm 11220000.audio-controller: mt8365_dai_int_adda_prepare 'Capture' rate = 48000 [ +0.107688] mt6357-sound mt6357-sound: TOTO set mic to stored value [ +10.072648] mt6357-sound mt6357-sound: TOTO set mic to 0dB root@i350-evk:~# arecord -D hw:mt8365evk,2,0 -r 48000 -c2 -f s32_le -d 10 recorded_file.wav [Mar15 09:32] mt8365-afe-pcm 11220000.audio-controller: mt8365_afe_fe_hw_params AWB period = 6000 rate = 48000 channels = 2 [ +0.000133] mt8365-afe-pcm 11220000.audio-controller: mt8365_dai_int_adda_prepare 'Capture' rate = 48000 [ +0.109418] mt6357-sound mt6357-sound: TOTO set mic to stored value [ +10.164197] mt6357-sound mt6357-sound: TOTO set mic to 0dB > >>>> + /* ul channel swap */ >>>> + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0), > >>> On/off controls should end in Switch. > >> Sorry, I don't understand your comment. Can you reword it please ? > > See control-names.rst. Run mixer-test on a card with this driver and > fix all the issues it reports. Ok the name is the issue for you AFAII. This control isn't for on/off but swap Left and Right. From the codec documentation: "Swaps audio UL L/R channel before UL SRC" This control is overkill, I will remove it I'm stuck to run mixer-test, please check the following message: https://lore.kernel.org/all/7ddad394-e880-4ef8-8591-cb803a2086ae@baylibre.com/
On Fri, Mar 15, 2024 at 12:01:12PM +0100, Alexandre Mergnat wrote: > On 13/03/2024 18:23, Mark Brown wrote: > > On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote: > > > Actually you must save the values because the gain selected by the user will > > > be override to do a ramp => volume_ramp(.....): > > > - When you switch on the HP, you start from gain=-40db to final_gain > > > (selected by user). > > > - When you switch off the HP, you start from final_gain (selected by user) > > > to gain=-40db. > > You can just read the value back when you need to do a ramp? > You can't. Because you will read -40db when HP isn't playing sound. That is > why the gain is saved into the struct. > Let me know, when you change de gain to do a ramp down (start from user gain > to gain=-40db), next time for the ramp up, how/where do you find the user > gain ? In the register. You only need to reset the gain to -40dB at the start of the ramp. > > > Also, the microphone's gain change when it's enabled/disabled. > > I don't understand what this means? > When microphone isn't capturing, the gain read back from the register is > 0dB. I've put some logs in my code and do capture to show how it works: Is this a property of the hardware or a property of your driver? > > > > > + /* ul channel swap */ > > > > > + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0), > > > > On/off controls should end in Switch. > > > Sorry, I don't understand your comment. Can you reword it please ? > > See control-names.rst. Run mixer-test on a card with this driver and > > fix all the issues it reports. > Ok the name is the issue for you AFAII. > This control isn't for on/off but swap Left and Right. > From the codec documentation: > "Swaps audio UL L/R channel before UL SRC" > This control is overkill, I will remove it This is turning the swapping on and off.
On Tue, Mar 12, 2024 at 09:58:05AM +0100, Alexandre Mergnat wrote: > I'm a bit lost for mixer-test and pcm-test. > Currently, I cross-compile the alsa lib project to be able to build the > tests and put it on my board. > I can execute it, but I still have 2 issues: > 1) I've a lot of missing module in my environment (Encode.so, Encode.pm, > Symbol.pm, IO/Handle.pm, ...). AFAII, I've to cross compile the missing perl > modules and install them in the rootfs These tests are both simple C programs... > 2) I don't know how to configure pcm-test.conf & > Lenovo_ThinkPad_P1_Gen2.conf (or new file to match with my board). The configuration is optional. > My test cmd: > ./run_kselftest.sh -c alsa Just run the programs directly. I'm only asking for the output from two of them anyway.
On 15/03/2024 15:30, Mark Brown wrote: > On Fri, Mar 15, 2024 at 12:01:12PM +0100, Alexandre Mergnat wrote: >> On 13/03/2024 18:23, Mark Brown wrote: >>> On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote: > >>>> Actually you must save the values because the gain selected by the user will >>>> be override to do a ramp => volume_ramp(.....): >>>> - When you switch on the HP, you start from gain=-40db to final_gain >>>> (selected by user). >>>> - When you switch off the HP, you start from final_gain (selected by user) >>>> to gain=-40db. > >>> You can just read the value back when you need to do a ramp? > >> You can't. Because you will read -40db when HP isn't playing sound. That is >> why the gain is saved into the struct. > >> Let me know, when you change de gain to do a ramp down (start from user gain >> to gain=-40db), next time for the ramp up, how/where do you find the user >> gain ? > > In the register. You only need to reset the gain to -40dB at the start > of the ramp. Sorry but I don't understand your logic, I'm not able to implement it... If I'm at -10dB and doing a ramp to reach -40dB, next time I will read the register the value will be -40dB. This implementation is also done in other MTK audio codec drivers. > >>>> Also, the microphone's gain change when it's enabled/disabled. > >>> I don't understand what this means? > >> When microphone isn't capturing, the gain read back from the register is >> 0dB. I've put some logs in my code and do capture to show how it works: > > Is this a property of the hardware or a property of your driver? At the end of the capture, the gain is set to 0dB by the driver. At the start of the capture, the gain is set to the setup gain. AFAII from the comment in the code, it's done to avoid the "pop noises". > >>>>>> + /* ul channel swap */ >>>>>> + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0), > >>>>> On/off controls should end in Switch. > >>>> Sorry, I don't understand your comment. Can you reword it please ? > >>> See control-names.rst. Run mixer-test on a card with this driver and >>> fix all the issues it reports. > >> Ok the name is the issue for you AFAII. >> This control isn't for on/off but swap Left and Right. >> From the codec documentation: >> "Swaps audio UL L/R channel before UL SRC" >> This control is overkill, I will remove it > > This is turning the swapping on and off.
On Fri, Mar 15, 2024 at 04:05:21PM +0100, Alexandre Mergnat wrote: > On 15/03/2024 15:30, Mark Brown wrote: > > > Let me know, when you change de gain to do a ramp down (start from user gain > > > to gain=-40db), next time for the ramp up, how/where do you find the user > > > gain ? > > In the register. You only need to reset the gain to -40dB at the start > > of the ramp. > Sorry but I don't understand your logic, I'm not able to implement it... > If I'm at -10dB and doing a ramp to reach -40dB, next time I will read the > register the value will be -40dB. After we've done the ramp and turned the amplifier off we can just restore the desired value? The hardware is not going to care what the volume is while it's not enabled. > This implementation is also done in other MTK audio codec drivers. Perhaps they should be updated too? > > > When microphone isn't capturing, the gain read back from the register is > > > 0dB. I've put some logs in my code and do capture to show how it works: > > Is this a property of the hardware or a property of your driver? > At the end of the capture, the gain is set to 0dB by the driver. > At the start of the capture, the gain is set to the setup gain. So that's a property of the driver then? > AFAII from the comment in the code, it's done to avoid the "pop noises". Yes, that's the usual reason to ramp gains. Though if you've just copied the code without checking that it's needed it's possible that this is something that's been fixed in current hardware.
On 15/03/2024 15:38, Mark Brown wrote: > On Tue, Mar 12, 2024 at 09:58:05AM +0100, Alexandre Mergnat wrote: > >> I'm a bit lost for mixer-test and pcm-test. >> Currently, I cross-compile the alsa lib project to be able to build the >> tests and put it on my board. > >> I can execute it, but I still have 2 issues: > >> 1) I've a lot of missing module in my environment (Encode.so, Encode.pm, >> Symbol.pm, IO/Handle.pm, ...). AFAII, I've to cross compile the missing perl >> modules and install them in the rootfs > > These tests are both simple C programs... > >> 2) I don't know how to configure pcm-test.conf & >> Lenovo_ThinkPad_P1_Gen2.conf (or new file to match with my board). > > The configuration is optional. > >> My test cmd: >> ./run_kselftest.sh -c alsa > > Just run the programs directly. I'm only asking for the output from two > of them anyway. ok
On 15/03/2024 16:15, Mark Brown wrote: > On Fri, Mar 15, 2024 at 04:05:21PM +0100, Alexandre Mergnat wrote: >> On 15/03/2024 15:30, Mark Brown wrote: > >>>> Let me know, when you change de gain to do a ramp down (start from user gain >>>> to gain=-40db), next time for the ramp up, how/where do you find the user >>>> gain ? > >>> In the register. You only need to reset the gain to -40dB at the start >>> of the ramp. > >> Sorry but I don't understand your logic, I'm not able to implement it... >> If I'm at -10dB and doing a ramp to reach -40dB, next time I will read the >> register the value will be -40dB. > > After we've done the ramp and turned the amplifier off we can just > restore the desired value? The hardware is not going to care what the > volume is while it's not enabled. If you do that, HP will be enabled at the saved gain, and after that you will do the ramp. To avoid pop, the driver should be rewrite to: Read gain in the reg and save it locally Set -40dB in the reg Enable HP Do ramp And for the shutdown: Read gain in the reg and save it locally Do ramp Disable HP Set saved gain in the reg To resume, that add 4 more steps to save 2 integers into the driver structure. IMHO, I don't think it make the code more readable or optimized, but I don't have a strong opinion about that, so if you think it's better, I will change it. > >> This implementation is also done in other MTK audio codec drivers. > > Perhaps they should be updated too? > >>>> When microphone isn't capturing, the gain read back from the register is >>>> 0dB. I've put some logs in my code and do capture to show how it works: > >>> Is this a property of the hardware or a property of your driver? > >> At the end of the capture, the gain is set to 0dB by the driver. >> At the start of the capture, the gain is set to the setup gain. > > So that's a property of the driver then? Yes > >> AFAII from the comment in the code, it's done to avoid the "pop noises". > > Yes, that's the usual reason to ramp gains. Though if you've just > copied the code without checking that it's needed it's possible that > this is something that's been fixed in current hardware. I did the test at 24dB with and without the "pop filter". Isn't big but I ear the pop at the start of the record without the "pop filter". To be clear, the algo/behavior of this code is an implementation based on the 6k+ lines downstream code for this specific audio codec. But the shape/style is based on upstreamed drivers like mt6358.c.
On Fri, Mar 15, 2024 at 06:36:19PM +0100, Alexandre Mergnat wrote: > On 15/03/2024 16:15, Mark Brown wrote: > > On Fri, Mar 15, 2024 at 04:05:21PM +0100, Alexandre Mergnat wrote: > > > > In the register. You only need to reset the gain to -40dB at the start > > > > of the ramp. > > > Sorry but I don't understand your logic, I'm not able to implement it... > > > If I'm at -10dB and doing a ramp to reach -40dB, next time I will read the > > > register the value will be -40dB. > > After we've done the ramp and turned the amplifier off we can just > > restore the desired value? The hardware is not going to care what the > > volume is while it's not enabled. > If you do that, HP will be enabled at the saved gain, and after that you > will do the ramp. To avoid pop, the driver should be rewrite to: So reset the volume to -40dB prior to turning the amplifier on... > Read gain in the reg and save it locally > Set -40dB in the reg > Enable HP > Do ramp ...as you yourself suggest? > > > AFAII from the comment in the code, it's done to avoid the "pop noises". > > Yes, that's the usual reason to ramp gains. Though if you've just > > copied the code without checking that it's needed it's possible that > > this is something that's been fixed in current hardware. > I did the test at 24dB with and without the "pop filter". Isn't big but I > ear the pop at the start of the record without the "pop filter". OK, it probably is still doing something then. > To be clear, the algo/behavior of this code is an implementation based on > the 6k+ lines downstream code for this specific audio codec. But the > shape/style is based on upstreamed drivers like mt6358.c. The Mediatek code has a bunch of issues, I wouldn't read too much into something being present in the code.
Hi Angelo On 26/02/2024 15:54, AngeloGioacchino Del Regno wrote: > Il 26/02/24 15:01, Alexandre Mergnat ha scritto: >> This serie aim to add the following audio support for the Genio 350-evk: >> - Playback >> - 2ch Headset Jack (Earphone) >> - 1ch Line-out Jack (Speaker) >> - 8ch HDMI Tx >> - Capture >> - 1ch DMIC (On-board Digital Microphone) >> - 1ch AMIC (On-board Analogic Microphone) >> - 1ch Headset Jack (External Analogic Microphone) >> >> Of course, HDMI playback need the MT8365 display patches [1] and a DTS >> change documented in "mediatek,mt8365-mt6357.yaml". >> >> [1]: >> https://lore.kernel.org/all/20231023-display-support-v1-0-5c860ed5c33b@baylibre.com/ >> >> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > > Actually, I am cooking a series (I'm finishing the testing....) that > brings quite > a bit of cleanups in MTK ASoC, including the commonization of the > machine driver > probe, with the dai-link DT nodes, and which also modernizes most of the > existing > drivers to use that instead. > > If you wait for a day or two, your mt8365-mt6357.c driver's probe > function can be > shrunk to ~3 lines or something like that.. very easily :-) Just to inform you. I'm aware of your serie. Currently, I've fixed my patches according to the comments. The next step will be to rebase my serie over yours and do the changes to be aligned with your new implementation. I've planned to review your serie during my last task, but it seems already approved and already (partially) merged into linux-next, sorry.
This serie aim to add the following audio support for the Genio 350-evk: - Playback - 2ch Headset Jack (Earphone) - 1ch Line-out Jack (Speaker) - 8ch HDMI Tx - Capture - 1ch DMIC (On-board Digital Microphone) - 1ch AMIC (On-board Analogic Microphone) - 1ch Headset Jack (External Analogic Microphone) Of course, HDMI playback need the MT8365 display patches [1] and a DTS change documented in "mediatek,mt8365-mt6357.yaml". [1]: https://lore.kernel.org/all/20231023-display-support-v1-0-5c860ed5c33b@baylibre.com/ Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> --- Alexandre Mergnat (15): ASoC: dt-bindings: mediatek,mt8365-afe: Add audio afe document ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card document dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC ASoC: mediatek: mt8365: Add common header SoC: mediatek: mt8365: support audio clock control ASoC: mediatek: mt8365: Add I2S DAI support ASoC: mediatek: mt8365: Add ADDA DAI support ASoC: mediatek: mt8365: Add DMIC DAI support ASoC: mediatek: mt8365: Add PCM DAI support ASoC: mediatek: mt8365: Add platform driver ASoC: mediatek: Add MT8365 support arm64: defconfig: enable mt8365 sound arm64: dts: mediatek: add mt6357 audio codec support arm64: dts: mediatek: add afe support for mt8365 SoC arm64: dts: mediatek: add audio support for mt8365-evk Fabien Parent (1): mfd: mt6397-core: register mt6357 sound codec Nicolas Belin (2): ASoc: mediatek: mt8365: Add a specific soundcard for EVK ASoC: codecs: mt6357: add MT6357 codec .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 11 + .../bindings/sound/mediatek,mt8365-afe.yaml | 164 ++ .../bindings/sound/mediatek,mt8365-mt6357.yaml | 127 ++ arch/arm64/boot/dts/mediatek/mt6357.dtsi | 6 +- arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 95 +- arch/arm64/boot/dts/mediatek/mt8365.dtsi | 47 +- arch/arm64/configs/defconfig | 2 + drivers/mfd/mt6397-core.c | 3 + sound/soc/codecs/Kconfig | 7 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/mt6357.c | 1805 +++++++++++++++ sound/soc/codecs/mt6357.h | 674 ++++++ sound/soc/mediatek/Kconfig | 20 + sound/soc/mediatek/Makefile | 1 + sound/soc/mediatek/mt8365/Makefile | 15 + sound/soc/mediatek/mt8365/mt8365-afe-clk.c | 455 ++++ sound/soc/mediatek/mt8365/mt8365-afe-clk.h | 55 + sound/soc/mediatek/mt8365/mt8365-afe-common.h | 495 +++++ sound/soc/mediatek/mt8365/mt8365-afe-pcm.c | 2347 ++++++++++++++++++++ sound/soc/mediatek/mt8365/mt8365-dai-adda.c | 355 +++ sound/soc/mediatek/mt8365/mt8365-dai-dmic.c | 475 ++++ sound/soc/mediatek/mt8365/mt8365-dai-i2s.c | 901 ++++++++ sound/soc/mediatek/mt8365/mt8365-dai-pcm.c | 298 +++ sound/soc/mediatek/mt8365/mt8365-mt6357.c | 379 ++++ sound/soc/mediatek/mt8365/mt8365-reg.h | 987 ++++++++ 25 files changed, 9718 insertions(+), 8 deletions(-) --- base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d change-id: 20240226-audio-i350-4e11da088e55 Best regards,