| Message ID | 20220610082724.29256-1-jiaxin.yu@mediatek.com |
|---|---|
| Headers | show |
| Series | ASoC: mediatek: Add support for MT8186 SoC | expand |
Il 10/06/22 10:27, Jiaxin Yu ha scritto: > Add support for mt8186 board with mt6366, da7219 and max98357. > > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com> > --- > sound/soc/mediatek/Kconfig | 16 + > sound/soc/mediatek/mt8186/Makefile | 1 + > .../mt8186/mt8186-mt6366-da7219-max98357.c | 1002 +++++++++++++++++ > 3 files changed, 1019 insertions(+) > create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c > > diff --git a/sound/soc/mediatek/Kconfig b/sound/soc/mediatek/Kconfig > index f3c3b93226e4..cc93a0d42fe1 100644 > --- a/sound/soc/mediatek/Kconfig > +++ b/sound/soc/mediatek/Kconfig > @@ -164,6 +164,22 @@ config SND_SOC_MT8186 > Select Y if you have such device. > If unsure select "N". > > +config SND_SOC_MT8186_MT6366_DA7219_MAX98357 > + tristate "ASoC Audio driver for MT8186 with DA7219 MAX98357A codec" > + depends on I2C && GPIOLIB > + depends on SND_SOC_MT8186 && MTK_PMIC_WRAP > + select SND_SOC_MT6358 > + select SND_SOC_MAX98357A > + select SND_SOC_DA7219 > + select SND_SOC_BT_SCO > + select SND_SOC_DMIC > + select SND_SOC_HDMI_CODEC > + help > + This adds ASoC driver for Mediatek MT8186 boards > + with the MT6366(MT6358) DA7219 MAX98357A codecs. > + Select Y if you have such device. > + If unsure select "N". > + > config SND_SOC_MTK_BTCVSD > tristate "ALSA BT SCO CVSD/MSBC Driver" > help > diff --git a/sound/soc/mediatek/mt8186/Makefile b/sound/soc/mediatek/mt8186/Makefile > index bdca1a3b3ff0..e7ddbe74d9d5 100644 > --- a/sound/soc/mediatek/mt8186/Makefile > +++ b/sound/soc/mediatek/mt8186/Makefile > @@ -18,3 +18,4 @@ snd-soc-mt8186-afe-objs := \ > mt8186-mt6366-common.o > > obj-$(CONFIG_SND_SOC_MT8186) += snd-soc-mt8186-afe.o > +obj-$(CONFIG_SND_SOC_MT8186_MT6366_DA7219_MAX98357) += mt8186-mt6366-da7219-max98357.o > diff --git a/sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c b/sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c > new file mode 100644 > index 000000000000..5123754374b2 > --- /dev/null > +++ b/sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c > @@ -0,0 +1,1002 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// mt8186-mt6366-da7219-max98357.c > +// -- MT8186-MT6366-DA7219-MAX98357 ALSA SoC machine driver > +// > +// Copyright (c) 2022 MediaTek Inc. > +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com> > +// > + ...snip... > +} > + > +static int mt8186_da7219_i2s_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params) > +{ > + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); > + struct snd_soc_dai *codec_dai; > + unsigned int rate = params_rate(params); > + unsigned int mclk_fs_ratio = 256; > + unsigned int mclk_fs = rate * mclk_fs_ratio; > + unsigned int freq; > + int ret = 0, j; Why are you assigning 0 to ret here? You're reassigning just one line under here... > + > + ret = snd_soc_dai_set_sysclk(asoc_rtd_to_cpu(rtd, 0), 0, > + mclk_fs, SND_SOC_CLOCK_OUT); > + if (ret < 0) { > + dev_err(rtd->dev, "failed to set cpu dai sysclk: %d\n", ret); > + return ret; > + } > + > + for_each_rtd_codec_dais(rtd, j, codec_dai) { > + if (!strcmp(codec_dai->component->name, DA7219_DEV_NAME)) { > + ret = snd_soc_dai_set_sysclk(codec_dai, > + DA7219_CLKSRC_MCLK, > + mclk_fs, > + SND_SOC_CLOCK_IN); > + if (ret < 0) { > + dev_err(rtd->dev, "failed to set sysclk: %d\n", > + ret); > + return ret; > + } > + > + if ((rate % 8000) == 0) > + freq = DA7219_PLL_FREQ_OUT_98304; > + else > + freq = DA7219_PLL_FREQ_OUT_90316; > + > + ret = snd_soc_dai_set_pll(codec_dai, 0, > + DA7219_SYSCLK_PLL_SRM, > + 0, freq); > + if (ret) { > + dev_err(rtd->dev, "failed to start PLL: %d\n", > + ret); > + return ret; > + } > + } > + } > + > + return 0; > +} > + > +static int mt8186_da7219_i2s_hw_free(struct snd_pcm_substream *substream) > +{ > + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); > + struct snd_soc_dai *codec_dai; > + int ret = 0, j; > + > + for_each_rtd_codec_dais(rtd, j, codec_dai) { > + if (!strcmp(codec_dai->component->name, DA7219_DEV_NAME)) { > + ret = snd_soc_dai_set_pll(codec_dai, > + 0, DA7219_SYSCLK_MCLK, 0, 0); > + if (ret < 0) { > + dev_err(rtd->dev, "failed to stop PLL: %d\n", > + ret); return ret; > + break; > + } > + } > + } > + ... and return 0 here. > + return ret; > +} > + ..snip.. > + > +static const struct snd_soc_dapm_widget > +mt8186_mt6366_da7219_max98357_widgets[] = { > + SND_SOC_DAPM_SPK("SPK Out", NULL), In mt8183-da7219-max98357, this widget is called "Speakers": please use the same name, so that we can (at least partially) reuse ALSA UCM2 configurations. > + SND_SOC_DAPM_OUTPUT("HDMI Out"), ...and some Intel boards (bxt_da7219_max98357a and others) are calling these like "HDMI1", "HDMI2", etc. We have only one HDMI output, yes, but if we change this to "HDMI1" we can again increase UCM2 conf reusability. By the way, these comments are actually valid also for the other card driver mt6366-rt1019-rt5682s, so please model the names to commonize as much as possible. Look at https://github.com/alsa-project/alsa-ucm-conf/tree/master/ucm2 to understand what should be the final mixer names to achieve configuration commonization. > +}; > + > +static const struct snd_soc_dapm_route > +mt8186_mt6366_da7219_max98357_routes[] = { > + /* SPK */ > + { "SPK Out", NULL, "Speaker" }, > + /* HDMI */ > + { "HDMI Out", NULL, "TX" }, > +}; > + > +static const struct snd_kcontrol_new > +mt8186_mt6366_da7219_max98357_controls[] = { > + SOC_DAPM_PIN_SWITCH("SPK Out"), > + SOC_DAPM_PIN_SWITCH("HDMI Out"), > +}; > + > +static struct snd_soc_card mt8186_mt6366_da7219_max98357_soc_card = { > + .name = "mt8186_mt6366_da7219_max98357", > + .owner = THIS_MODULE, > + .dai_link = mt8186_mt6366_da7219_max98357_dai_links, > + .num_links = ARRAY_SIZE(mt8186_mt6366_da7219_max98357_dai_links), > + .controls = mt8186_mt6366_da7219_max98357_controls, > + .num_controls = ARRAY_SIZE(mt8186_mt6366_da7219_max98357_controls), > + .dapm_widgets = mt8186_mt6366_da7219_max98357_widgets, > + .num_dapm_widgets = ARRAY_SIZE(mt8186_mt6366_da7219_max98357_widgets), > + .dapm_routes = mt8186_mt6366_da7219_max98357_routes, > + .num_dapm_routes = ARRAY_SIZE(mt8186_mt6366_da7219_max98357_routes), > + .codec_conf = mt8186_mt6366_da7219_max98357_codec_conf, > + .num_configs = ARRAY_SIZE(mt8186_mt6366_da7219_max98357_codec_conf), > +}; > + > +static int mt8186_mt6366_da7219_max98357_dev_probe(struct platform_device *pdev) > +{ > + struct snd_soc_card *card; > + struct snd_soc_dai_link *dai_link; > + struct mt8186_mt6366_da7219_max98357_priv *priv; > + struct device_node *platform_node, *headset_codec, *playback_codec; > + int ret, i; > + > + card = (struct snd_soc_card *)of_device_get_match_data(&pdev->dev); > + if (!card) > + return -EINVAL; > + card->dev = &pdev->dev; > + > + platform_node = of_parse_phandle(pdev->dev.of_node, "mediatek,platform", 0); > + if (!platform_node) { > + ret = -EINVAL; > + dev_err_probe(&pdev->dev, ret, "Property 'platform' missing or invalid\n"); > + goto err_platform_node; You don't need this particular goto, you can just return in here. > + } Regards, Angelo
Il 10/06/22 10:27, Jiaxin Yu ha scritto: > Add mt8186-mt6366 common driver for mt8186 series machine. > > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com> > --- > sound/soc/mediatek/mt8186/Makefile | 1 + > .../mediatek/mt8186/mt8186-mt6366-common.c | 59 +++++++++++++++++++ > .../mediatek/mt8186/mt8186-mt6366-common.h | 17 ++++++ > 3 files changed, 77 insertions(+) > create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-common.c > create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-common.h > ..snip.. > diff --git a/sound/soc/mediatek/mt8186/mt8186-mt6366-common.c b/sound/soc/mediatek/mt8186/mt8186-mt6366-common.c > new file mode 100644 > index 000000000000..94e1128e8128 > --- /dev/null > +++ b/sound/soc/mediatek/mt8186/mt8186-mt6366-common.c > @@ -0,0 +1,59 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// mt8186-mt6366-common.c > +// -- MT8186 MT6366 ALSA common driver > +// > +// Copyright (c) 2022 MediaTek Inc. > +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com> > +// > +#include <sound/soc.h> > + > +#include "../../codecs/mt6358.h" > +#include "../common/mtk-afe-platform-driver.h" > +#include "mt8186-afe-common.h" > +#include "mt8186-mt6366-common.h" > + > +int mt8186_mt6366_init(struct snd_soc_pcm_runtime *rtd) > +{ > + struct snd_soc_component *cmpnt_afe = > + snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME); > + struct snd_soc_component *cmpnt_codec = > + asoc_rtd_to_codec(rtd, 0)->component; > + struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt_afe); > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > + struct snd_soc_dapm_context *dapm = &rtd->card->dapm; > + int ret; > + > + /* set mtkaif protocol */ > + mt6358_set_mtkaif_protocol(cmpnt_codec, > + MT6358_MTKAIF_PROTOCOL_1); > + afe_priv->mtkaif_protocol = MT6358_MTKAIF_PROTOCOL_1; > + > + ret = snd_soc_dapm_sync(dapm); > + if (ret) { > + dev_info(rtd->dev, "failed to snd_soc_dapm_sync\n"); This should be a dev_err(). > + return ret; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(mt8186_mt6366_init); > + > +int mt8186_mt6366_card_set_be_link(struct snd_soc_card *card, > + struct snd_soc_dai_link *link, > + struct device_node *node, > + char *link_name) > +{ > + int ret; > + > + if (node && strcmp(link->name, link_name) == 0) { > + ret = snd_soc_of_get_dai_link_codecs(card->dev, node, link); > + if (ret < 0) { > + dev_err_probe(card->dev, ret, "get dai link codecs fail\n"); > + return ret; You may at this point just 'return dev_err_probe(...)'. Regards, Angelo
Il 10/06/22 10:27, Jiaxin Yu ha scritto: > Add mt8186 platform and affiliated driver. > > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com> > --- > sound/soc/mediatek/Kconfig | 12 + > sound/soc/mediatek/Makefile | 1 + > sound/soc/mediatek/mt8186/Makefile | 19 + > sound/soc/mediatek/mt8186/mt8186-afe-common.h | 235 ++ > .../soc/mediatek/mt8186/mt8186-afe-control.c | 261 ++ > sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3009 +++++++++++++++++ > 6 files changed, 3537 insertions(+) > create mode 100644 sound/soc/mediatek/mt8186/Makefile > create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-common.h > create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-control.c > create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-pcm.c > ..snip.. > diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-control.c b/sound/soc/mediatek/mt8186/mt8186-afe-control.c > new file mode 100644 > index 000000000000..2d78212c3c3f > --- /dev/null > +++ b/sound/soc/mediatek/mt8186/mt8186-afe-control.c > @@ -0,0 +1,261 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// MediaTek ALSA SoC Audio Control > +// > +// Copyright (c) 2022 MediaTek Inc. > +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com> > + > +#include "mt8186-afe-common.h" > +#include <linux/pm_runtime.h> > + ..snip.. > + > +static unsigned int pcm_rate_transform(struct device *dev, > + unsigned int rate) > +{ > + switch (rate) { > + case 8000: > + return MTK_AFE_PCM_RATE_8K; > + case 16000: > + return MTK_AFE_PCM_RATE_16K; > + case 32000: > + return MTK_AFE_PCM_RATE_32K; > + case 48000: > + return MTK_AFE_PCM_RATE_48K; > + default: > + dev_info(dev, "%s(), rate %u invalid, use %d!!!\n", > + __func__, > + rate, MTK_AFE_PCM_RATE_32K); > + return MTK_AFE_PCM_RATE_32K; This return statement should go outside of the switch, and that print should be a dev_err() instead. > + } > +} > + > +unsigned int mt8186_tdm_relatch_rate_transform(struct device *dev, > + unsigned int rate) > +{ > + switch (rate) { > + case 8000: > + return MTK_AFE_TDM_RELATCH_RATE_8K; > + case 11025: > + return MTK_AFE_TDM_RELATCH_RATE_11K; > + case 12000: > + return MTK_AFE_TDM_RELATCH_RATE_12K; > + case 16000: > + return MTK_AFE_TDM_RELATCH_RATE_16K; > + case 22050: > + return MTK_AFE_TDM_RELATCH_RATE_22K; > + case 24000: > + return MTK_AFE_TDM_RELATCH_RATE_24K; > + case 32000: > + return MTK_AFE_TDM_RELATCH_RATE_32K; > + case 44100: > + return MTK_AFE_TDM_RELATCH_RATE_44K; > + case 48000: > + return MTK_AFE_TDM_RELATCH_RATE_48K; > + case 88200: > + return MTK_AFE_TDM_RELATCH_RATE_88K; > + case 96000: > + return MTK_AFE_TDM_RELATCH_RATE_96K; > + case 176400: > + return MTK_AFE_TDM_RELATCH_RATE_176K; > + case 192000: > + return MTK_AFE_TDM_RELATCH_RATE_192K; > + case 352800: > + return MTK_AFE_TDM_RELATCH_RATE_352K; > + case 384000: > + return MTK_AFE_TDM_RELATCH_RATE_384K; > + default: > + dev_info(dev, "%s(), rate %u invalid, use %d!!!\n", > + __func__, > + rate, MTK_AFE_TDM_RELATCH_RATE_48K); > + return MTK_AFE_TDM_RELATCH_RATE_48K; Same here. > + } > +} > + > +unsigned int mt8186_rate_transform(struct device *dev, > + unsigned int rate, int aud_blk) > +{ > + switch (aud_blk) { > + case MT8186_DAI_PCM: > + return pcm_rate_transform(dev, rate); > + case MT8186_DAI_TDM_IN: > + return tdm_rate_transform(dev, rate); > + default: > + return mt8186_general_rate_transform(dev, rate); > + } > +} > + > +int mt8186_dai_set_priv(struct mtk_base_afe *afe, int id, > + int priv_size, const void *priv_data) > +{ > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > + void *temp_data; > + > + temp_data = devm_kzalloc(afe->dev, > + priv_size, > + GFP_KERNEL); > + if (!temp_data) > + return -ENOMEM; > + > + if (priv_data) > + memcpy(temp_data, priv_data, priv_size); > + > + afe_priv->dai_priv[id] = temp_data; > + > + return 0; > +} > diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-pcm.c b/sound/soc/mediatek/mt8186/mt8186-afe-pcm.c > new file mode 100644 > index 000000000000..aaba8627d9e1 > --- /dev/null > +++ b/sound/soc/mediatek/mt8186/mt8186-afe-pcm.c > @@ -0,0 +1,3009 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// Mediatek ALSA SoC AFE platform driver for 8186 > +// > +// Copyright (c) 2022 MediaTek Inc. > +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com> > + > +#include <linux/delay.h> > +#include <linux/dma-mapping.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/pm_runtime.h> > +#include <linux/reset.h> > +#include <sound/soc.h> > + > +#include "../common/mtk-afe-platform-driver.h" > +#include "../common/mtk-afe-fe-dai.h" > + > +#include "mt8186-afe-common.h" > +#include "mt8186-afe-clk.h" > +#include "mt8186-afe-gpio.h" > +#include "mt8186-interconnection.h" > + ..snip.. > + > +static int mt8186_fe_hw_free(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + int ret; > + > + ret = mtk_afe_fe_hw_free(substream, dai); > + if (ret) > + goto exit; > + > + /* wait for some platform related operation */ This comment says that we should wait for "some platform related operation": it's not describing what kind of operation we would wait for... and we are not waiting for anything at all and just returning. Are you implementing this wait mechanism? ...otherwise, there's no need for that goto at all. > +exit: > + return ret; > +} > + > +static int mt8186_fe_trigger(struct snd_pcm_substream *substream, int cmd, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + struct snd_pcm_runtime * const runtime = substream->runtime; > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > + int id = asoc_rtd_to_cpu(rtd, 0)->id; > + struct mtk_base_afe_memif *memif = &afe->memif[id]; > + int irq_id = memif->irq_usage; > + struct mtk_base_afe_irq *irqs = &afe->irqs[irq_id]; > + const struct mtk_base_irq_data *irq_data = irqs->irq_data; > + unsigned int counter = runtime->period_size; You're assigning a value to 'counter' here and you may or may not use it, because if we look down there... > + unsigned int rate = runtime->rate; > + int fs; > + int ret; > + > + dev_dbg(afe->dev, "%s(), %s cmd %d, irq_id %d\n", > + __func__, memif->data->name, cmd, irq_id); > + > + switch (cmd) { > + case SNDRV_PCM_TRIGGER_START: > + case SNDRV_PCM_TRIGGER_RESUME: > + ret = mtk_memif_set_enable(afe, id); > + if (ret) { > + dev_err(afe->dev, "%s(), error, id %d, memif enable, ret %d\n", > + __func__, id, ret); > + return ret; > + } > + > + /* > + * for small latency record > + * ul memif need read some data before irq enable > + */ > + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE && > + ((runtime->period_size * 1000) / rate <= 10)) > + udelay(300); > + > + /* set irq counter */ > + if (afe_priv->irq_cnt[id] > 0) > + counter = afe_priv->irq_cnt[id]; ...you're reassigning the counter if irq_cnt > 0, so you can as well avoid a reassignment if you simply do it like if (afe_priv->irq_cnt[id] > 0) counter = afe_priv->irq_cnt[id]; else counter = runtime->period_size; > + > + regmap_update_bits(afe->regmap, irq_data->irq_cnt_reg, > + irq_data->irq_cnt_maskbit > + << irq_data->irq_cnt_shift, > + counter << irq_data->irq_cnt_shift); > + > + /* set irq fs */ > + fs = afe->irq_fs(substream, runtime->rate); > + if (fs < 0) > + return -EINVAL; > + > + regmap_update_bits(afe->regmap, irq_data->irq_fs_reg, > + irq_data->irq_fs_maskbit > + << irq_data->irq_fs_shift, > + fs << irq_data->irq_fs_shift); > + > + /* enable interrupt */ > + if (runtime->stop_threshold != ~(0U)) > + regmap_update_bits(afe->regmap, > + irq_data->irq_en_reg, > + 1 << irq_data->irq_en_shift, > + 1 << irq_data->irq_en_shift); > + return 0; > + case SNDRV_PCM_TRIGGER_STOP: > + case SNDRV_PCM_TRIGGER_SUSPEND: > + if (afe_priv->xrun_assert[id] > 0) { > + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { > + int avail = snd_pcm_capture_avail(runtime); > + > + if (avail >= runtime->buffer_size) { > + dev_err(afe->dev, "%s(), id %d, xrun assert\n", > + __func__, id); You're printing an error here, but not returning an error state related to this one at the end of this case... why? (if this is intended, add a comment in the code explaining the reason) > + } > + } > + } > + > + ret = mtk_memif_set_disable(afe, id); > + if (ret) > + dev_err(afe->dev, "%s(), error, id %d, memif enable, ret %d\n", > + __func__, id, ret); > + > + /* disable interrupt */ > + if (runtime->stop_threshold != ~(0U)) > + regmap_update_bits(afe->regmap, > + irq_data->irq_en_reg, > + 1 << irq_data->irq_en_shift, > + 0 << irq_data->irq_en_shift); > + > + /* clear pending IRQ */ > + regmap_write(afe->regmap, irq_data->irq_clr_reg, > + 1 << irq_data->irq_clr_shift); > + return ret; > + default: > + return -EINVAL; > + } > +} > + ..snip.. > + > +static int mt8186_irq_cnt1_set(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *cmpnt = snd_soc_kcontrol_component(kcontrol); > + struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt); > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > + int memif_num = MT8186_PRIMARY_MEMIF; > + struct mtk_base_afe_memif *memif = &afe->memif[memif_num]; > + int irq_id = memif->irq_usage; > + int irq_cnt = afe_priv->irq_cnt[memif_num]; > + > + dev_dbg(afe->dev, "%s(), irq_id %d, irq_cnt = %d, value = %ld\n", > + __func__, irq_id, irq_cnt, ucontrol->value.integer.value[0]); > + > + if (irq_cnt == ucontrol->value.integer.value[0]) > + return 0; > + > + irq_cnt = ucontrol->value.integer.value[0]; > + afe_priv->irq_cnt[memif_num] = irq_cnt; > + > + if (pm_runtime_status_suspended(afe->dev) || irq_id < 0) { > + dev_info(afe->dev, "%s(), suspended || irq_id %d, not set\n", > + __func__, irq_id); This should probably be a dev_dbg, as that's not supposed to normally happen. Besides, since the expected flow for this function is to write to registers, I think that for readability purposes it's best to invert this conditional: if (!pm_runtime_status_suspended(afe->dev) && irq_id >= 0) { ..... } else { dev_dbg(.....) } > + } else { > + struct mtk_base_afe_irq *irqs = &afe->irqs[irq_id]; > + const struct mtk_base_irq_data *irq_data = irqs->irq_data; > + > + regmap_update_bits(afe->regmap, irq_data->irq_cnt_reg, > + irq_data->irq_cnt_maskbit > + << irq_data->irq_cnt_shift, > + irq_cnt << irq_data->irq_cnt_shift); > + } > + > + return 1; > +} > + > +static int mt8186_irq_cnt2_get(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *cmpnt = snd_soc_kcontrol_component(kcontrol); > + struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt); > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > + > + ucontrol->value.integer.value[0] = > + afe_priv->irq_cnt[MT8186_RECORD_MEMIF]; > + > + return 0; > +} > + > +static int mt8186_irq_cnt2_set(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *cmpnt = snd_soc_kcontrol_component(kcontrol); > + struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt); > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > + int memif_num = MT8186_RECORD_MEMIF; > + struct mtk_base_afe_memif *memif = &afe->memif[memif_num]; > + int irq_id = memif->irq_usage; > + int irq_cnt = afe_priv->irq_cnt[memif_num]; > + > + dev_dbg(afe->dev, "%s(), irq_id %d, irq_cnt = %d, value = %ld\n", > + __func__, irq_id, irq_cnt, ucontrol->value.integer.value[0]); > + > + if (irq_cnt == ucontrol->value.integer.value[0]) > + return 0; > + > + irq_cnt = ucontrol->value.integer.value[0]; > + afe_priv->irq_cnt[memif_num] = irq_cnt; > + > + if (pm_runtime_status_suspended(afe->dev) || irq_id < 0) { > + dev_info(afe->dev, "%s(), suspended || irq_id %d, not set\n", > + __func__, irq_id); same here. > + } else { > + struct mtk_base_afe_irq *irqs = &afe->irqs[irq_id]; > + const struct mtk_base_irq_data *irq_data = irqs->irq_data; > + > + regmap_update_bits(afe->regmap, irq_data->irq_cnt_reg, > + irq_data->irq_cnt_maskbit > + << irq_data->irq_cnt_shift, > + irq_cnt << irq_data->irq_cnt_shift); > + } > + > + return 1; > +} > + ..snip.. > + > +static int mt8186_afe_pcm_dev_probe(struct platform_device *pdev) > +{ > + struct mtk_base_afe *afe; > + struct mt8186_afe_private *afe_priv; > + struct resource *res; > + struct reset_control *rstc; > + struct device *dev = &pdev->dev; > + int i, ret, irq_id; > + > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(34)); > + if (ret) > + return ret; > + > + afe = devm_kzalloc(dev, sizeof(*afe), GFP_KERNEL); > + if (!afe) > + return -ENOMEM; > + platform_set_drvdata(pdev, afe); > + > + afe->platform_priv = devm_kzalloc(dev, sizeof(*afe_priv), GFP_KERNEL); > + if (!afe->platform_priv) > + return -ENOMEM; > + > + afe_priv = afe->platform_priv; > + afe->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + afe->base_addr = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(afe->base_addr)) > + return PTR_ERR(afe->base_addr); > + > + /* init audio related clock */ > + ret = mt8186_init_clock(afe); > + if (ret) { > + dev_err(dev, "init clock error, ret %d\n", ret); > + return ret; > + } > + > + /* init memif */ > + afe->memif_32bit_supported = 0; > + afe->memif_size = MT8186_MEMIF_NUM; > + afe->memif = devm_kcalloc(dev, afe->memif_size, sizeof(*afe->memif), > + GFP_KERNEL); > + > + if (!afe->memif) > + return -ENOMEM; > + > + for (i = 0; i < afe->memif_size; i++) { > + afe->memif[i].data = &memif_data[i]; > + afe->memif[i].irq_usage = memif_irq_usage[i]; > + afe->memif[i].const_irq = 1; > + } > + > + mutex_init(&afe->irq_alloc_lock); /* needed when dynamic irq */ > + > + /* init irq */ > + afe->irqs_size = MT8186_IRQ_NUM; > + afe->irqs = devm_kcalloc(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]; > + > + /* request irq */ > + irq_id = platform_get_irq(pdev, 0); > + if (irq_id <= 0) > + return dev_err_probe(dev, irq_id < 0 ? irq_id : -ENXIO, > + "no irq found"); > + > + ret = devm_request_irq(dev, irq_id, mt8186_afe_irq_handler, > + IRQF_TRIGGER_NONE, > + "Afe_ISR_Handle", (void *)afe); > + if (ret) > + return dev_err_probe(dev, ret, "could not request_irq for Afe_ISR_Handle\n"); > + > + ret = enable_irq_wake(irq_id); > + if (ret < 0) > + return dev_err_probe(dev, ret, "enable_irq_wake %d\n", irq_id); > + > + /* 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) > + return dev_err_probe(dev, ret, "dai register i %d fail\n", i); > + } > + > + /* init dai_driver and component_driver */ > + ret = mtk_afe_combine_sub_dai(afe); > + if (ret) > + return dev_err_probe(dev, ret, "mtk_afe_combine_sub_dai fail\n"); > + > + /* reset controller to reset audio regs before regmap cache */ > + rstc = devm_reset_control_get_exclusive(dev, "audiosys"); > + if (IS_ERR(rstc)) > + return dev_err_probe(dev, PTR_ERR(rstc), "could not get audiosys reset\n"); > + > + ret = reset_control_reset(rstc); > + if (ret) > + return dev_err_probe(dev, ret, "failed to trigger audio reset\n"); > + > + /* enable clock for regcache get default value from hw */ > + afe_priv->pm_runtime_bypass_reg_ctl = true; > + pm_runtime_enable(dev); What about.... ret = devm_pm_runtime_enable(dev); if (ret) return ret; ret = pm_runtime_resume_and_get(dev); if (ret) return dev_err_probe(dev, ret, "failed to resume device\n"); > + ret = pm_runtime_get_sync(dev); > + if (ret) { > + dev_err(dev, "failed to resume device: %d\n", ret); > + goto err_pm_disable; > + } > + > + afe->regmap = devm_regmap_init_mmio(dev, afe->base_addr, > + &mt8186_afe_regmap_config); > + if (IS_ERR(afe->regmap)) { > + ret = PTR_ERR(afe->regmap); > + goto err_pm_disable; > + } > + > + /* others */ > + afe->mtk_afe_hardware = &mt8186_afe_hardware; > + afe->memif_fs = mt8186_memif_fs; > + afe->irq_fs = mt8186_irq_fs; > + afe->get_dai_fs = mt8186_get_dai_fs; > + afe->get_memif_pbuf_size = mt8186_get_memif_pbuf_size; > + > + afe->runtime_resume = mt8186_afe_runtime_resume; > + afe->runtime_suspend = mt8186_afe_runtime_suspend; > + > + /* register platform */ > + dev_info(dev, "%s(), devm_snd_soc_register_component\n", __func__); This should be dev_dbg(). > + > + ret = devm_snd_soc_register_component(dev, > + &mt8186_afe_component, > + afe->dai_drivers, > + afe->num_dai_drivers); > + if (ret) { > + dev_err(dev, "err_dai_component\n"); > + goto err_pm_disable; > + } > + > + ret = pm_runtime_put_sync(dev); > + if (ret) { > + pm_runtime_get_noresume(dev); > + dev_err(dev, "failed to suspend device: %d\n", ret); > + goto err_pm_disable; > + } > + afe_priv->pm_runtime_bypass_reg_ctl = false; > + > + regcache_cache_only(afe->regmap, true); > + regcache_mark_dirty(afe->regmap); > + > + return 0; > + > +err_pm_disable: > + pm_runtime_put_noidle(dev); > + pm_runtime_set_suspended(dev); > + pm_runtime_disable(dev); ...and then, since we're using devm_pm_runtime_enable, this call to pm_runtime_disable() can be removed. > + > + return ret; > +} > + > +static int mt8186_afe_pcm_dev_remove(struct platform_device *pdev) > +{ > + struct mtk_base_afe *afe = platform_get_drvdata(pdev); > + > + pm_runtime_disable(&pdev->dev); Here too. Regards, Angelo
On Fri, 2022-06-10 at 12:02 +0200, AngeloGioacchino Del Regno wrote: > Il 10/06/22 10:27, Jiaxin Yu ha scritto: > > Add mt8186 platform and affiliated driver. > > > > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com> > > --- > > sound/soc/mediatek/Kconfig | 12 + > > sound/soc/mediatek/Makefile | 1 + > > sound/soc/mediatek/mt8186/Makefile | 19 + > > sound/soc/mediatek/mt8186/mt8186-afe-common.h | 235 ++ > > .../soc/mediatek/mt8186/mt8186-afe-control.c | 261 ++ > > sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3009 > > +++++++++++++++++ > > 6 files changed, 3537 insertions(+) > > create mode 100644 sound/soc/mediatek/mt8186/Makefile > > create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-common.h > > create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-control.c > > create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-pcm.c ..snip.. > > diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-pcm.c > > b/sound/soc/mediatek/mt8186/mt8186-afe-pcm.c > > new file mode 100644 > > index 000000000000..aaba8627d9e1 > > --- /dev/null > > +++ b/sound/soc/mediatek/mt8186/mt8186-afe-pcm.c > > @@ -0,0 +1,3009 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// > > +// Mediatek ALSA SoC AFE platform driver for 8186 > > +// > > +// Copyright (c) 2022 MediaTek Inc. > > +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com> > > + > > +#include <linux/delay.h> > > +#include <linux/dma-mapping.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/reset.h> > > +#include <sound/soc.h> > > + > > +#include "../common/mtk-afe-platform-driver.h" > > +#include "../common/mtk-afe-fe-dai.h" > > + > > +#include "mt8186-afe-common.h" > > +#include "mt8186-afe-clk.h" > > +#include "mt8186-afe-gpio.h" > > +#include "mt8186-interconnection.h" > > + > > ..snip.. > > > + > > +static int mt8186_fe_hw_free(struct snd_pcm_substream *substream, > > + struct snd_soc_dai *dai) > > +{ > > + int ret; > > + > > + ret = mtk_afe_fe_hw_free(substream, dai); > > + if (ret) > > + goto exit; > > + > > + /* wait for some platform related operation */ > > This comment says that we should wait for "some platform related > operation": > it's not describing what kind of operation we would wait for... and > we are > not waiting for anything at all and just returning. > > Are you implementing this wait mechanism? > ...otherwise, there's no need for that goto at all. > This is a reserved place for development, but there is no need to do anything in the end. So we can remove this line. > > +exit: > > + return ret; > > +} > > + > > +static int mt8186_fe_trigger(struct snd_pcm_substream *substream, > > int cmd, > > + struct snd_soc_dai *dai) > > +{ > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > + struct snd_pcm_runtime * const runtime = substream->runtime; > > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > > + int id = asoc_rtd_to_cpu(rtd, 0)->id; > > + struct mtk_base_afe_memif *memif = &afe->memif[id]; > > + int irq_id = memif->irq_usage; > > + struct mtk_base_afe_irq *irqs = &afe->irqs[irq_id]; > > + const struct mtk_base_irq_data *irq_data = irqs->irq_data; > > + unsigned int counter = runtime->period_size; > > You're assigning a value to 'counter' here and you may or may not use > it, > because if we look down there... > > > + unsigned int rate = runtime->rate; > > + int fs; > > + int ret; > > + > > + dev_dbg(afe->dev, "%s(), %s cmd %d, irq_id %d\n", > > + __func__, memif->data->name, cmd, irq_id); > > + > > + switch (cmd) { > > + case SNDRV_PCM_TRIGGER_START: > > + case SNDRV_PCM_TRIGGER_RESUME: > > + ret = mtk_memif_set_enable(afe, id); > > + if (ret) { > > + dev_err(afe->dev, "%s(), error, id %d, memif > > enable, ret %d\n", > > + __func__, id, ret); > > + return ret; > > + } > > + > > + /* > > + * for small latency record > > + * ul memif need read some data before irq enable > > + */ > > + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE && > > + ((runtime->period_size * 1000) / rate <= 10)) > > + udelay(300); > > + > > + /* set irq counter */ > > + if (afe_priv->irq_cnt[id] > 0) > > + counter = afe_priv->irq_cnt[id]; > > ...you're reassigning the counter if irq_cnt > 0, so you can as well > avoid a > reassignment if you simply do it like > > if (afe_priv->irq_cnt[id] > 0) > counter = afe_priv->irq_cnt[id]; > else > counter = runtime->period_size; > Agree with you. > > + > > + regmap_update_bits(afe->regmap, irq_data->irq_cnt_reg, > > + irq_data->irq_cnt_maskbit > > + << irq_data->irq_cnt_shift, > > + counter << irq_data->irq_cnt_shift); > > + > > + /* set irq fs */ > > + fs = afe->irq_fs(substream, runtime->rate); > > + if (fs < 0) > > + return -EINVAL; > > + > > + regmap_update_bits(afe->regmap, irq_data->irq_fs_reg, > > + irq_data->irq_fs_maskbit > > + << irq_data->irq_fs_shift, > > + fs << irq_data->irq_fs_shift); > > + > > + /* enable interrupt */ > > + if (runtime->stop_threshold != ~(0U)) > > + regmap_update_bits(afe->regmap, > > + irq_data->irq_en_reg, > > + 1 << irq_data->irq_en_shift, > > + 1 << irq_data- > > >irq_en_shift); > > + return 0; > > + case SNDRV_PCM_TRIGGER_STOP: > > + case SNDRV_PCM_TRIGGER_SUSPEND: > > + if (afe_priv->xrun_assert[id] > 0) { > > + if (substream->stream == > > SNDRV_PCM_STREAM_CAPTURE) { > > + int avail = > > snd_pcm_capture_avail(runtime); > > + > > + if (avail >= runtime->buffer_size) { > > + dev_err(afe->dev, "%s(), id %d, > > xrun assert\n", > > + __func__, id); > > You're printing an error here, but not returning an error state > related to > this one at the end of this case... why? > > (if this is intended, add a comment in the code explaining the > reason) > Here is a debug log about xrun when capture. There is no processing here because ALSA will help with it. We just want to know the id information. > > + } > > + } > > + } > + > > + ret = mtk_memif_set_disable(afe, id); > > + if (ret) > > + dev_err(afe->dev, "%s(), error, id %d, memif > > enable, ret %d\n", > > + __func__, id, ret); > > + > > + /* disable interrupt */ > > + if (runtime->stop_threshold != ~(0U)) > > + regmap_update_bits(afe->regmap, > > + irq_data->irq_en_reg, > > + 1 << irq_data->irq_en_shift, > > + 0 << irq_data- > > >irq_en_shift); > > + > > + /* clear pending IRQ */ > > + regmap_write(afe->regmap, irq_data->irq_clr_reg, > > + 1 << irq_data->irq_clr_shift); > > + return ret; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > ..snip.. > > > + > > +static int mt8186_irq_cnt1_set(struct snd_kcontrol *kcontrol, > > + struct snd_ctl_elem_value *ucontrol) > > +{ > > + struct snd_soc_component *cmpnt = > > snd_soc_kcontrol_component(kcontrol); > > + struct mtk_base_afe *afe = > > snd_soc_component_get_drvdata(cmpnt); > > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > > + int memif_num = MT8186_PRIMARY_MEMIF; > > + struct mtk_base_afe_memif *memif = &afe->memif[memif_num]; > > + int irq_id = memif->irq_usage; > > + int irq_cnt = afe_priv->irq_cnt[memif_num]; > > + > > + dev_dbg(afe->dev, "%s(), irq_id %d, irq_cnt = %d, value = > > %ld\n", > > + __func__, irq_id, irq_cnt, ucontrol- > > >value.integer.value[0]); > > + > > + if (irq_cnt == ucontrol->value.integer.value[0]) > > + return 0; > > + > > + irq_cnt = ucontrol->value.integer.value[0]; > > + afe_priv->irq_cnt[memif_num] = irq_cnt; > > + > > + if (pm_runtime_status_suspended(afe->dev) || irq_id < 0) { > > + dev_info(afe->dev, "%s(), suspended || irq_id %d, not > > set\n", > > + __func__, irq_id); > > This should probably be a dev_dbg, as that's not supposed to normally > happen. > Besides, since the expected flow for this function is to write to > registers, > I think that for readability purposes it's best to invert this > conditional: > > if (!pm_runtime_status_suspended(afe->dev) && irq_id >= 0) { > ..... > } else { > dev_dbg(.....) > } > Got it. > > + } else { > > + struct mtk_base_afe_irq *irqs = &afe->irqs[irq_id]; > > + const struct mtk_base_irq_data *irq_data = irqs- > > >irq_data; > > + > > + regmap_update_bits(afe->regmap, irq_data->irq_cnt_reg, > > + irq_data->irq_cnt_maskbit > > + << irq_data->irq_cnt_shift, > > + irq_cnt << irq_data->irq_cnt_shift); > > + } > > + > > + return 1; > > +} > > + > > +static int mt8186_irq_cnt2_get(struct snd_kcontrol *kcontrol, > > + struct snd_ctl_elem_value *ucontrol) > > +{ > > + struct snd_soc_component *cmpnt = > > snd_soc_kcontrol_component(kcontrol); > > + struct mtk_base_afe *afe = > > snd_soc_component_get_drvdata(cmpnt); > > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > > + > > + ucontrol->value.integer.value[0] = > > + afe_priv->irq_cnt[MT8186_RECORD_MEMIF]; > > + > > + return 0; > > +} > > + > > +static int mt8186_irq_cnt2_set(struct snd_kcontrol *kcontrol, > > + struct snd_ctl_elem_value *ucontrol) > > +{ > > + struct snd_soc_component *cmpnt = > > snd_soc_kcontrol_component(kcontrol); > > + struct mtk_base_afe *afe = > > snd_soc_component_get_drvdata(cmpnt); > > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > > + int memif_num = MT8186_RECORD_MEMIF; > > + struct mtk_base_afe_memif *memif = &afe->memif[memif_num]; > > + int irq_id = memif->irq_usage; > > + int irq_cnt = afe_priv->irq_cnt[memif_num]; > > + > > + dev_dbg(afe->dev, "%s(), irq_id %d, irq_cnt = %d, value = > > %ld\n", > > + __func__, irq_id, irq_cnt, ucontrol- > > >value.integer.value[0]); > > + > > + if (irq_cnt == ucontrol->value.integer.value[0]) > > + return 0; > > + > > + irq_cnt = ucontrol->value.integer.value[0]; > > + afe_priv->irq_cnt[memif_num] = irq_cnt; > > + > > + if (pm_runtime_status_suspended(afe->dev) || irq_id < 0) { > > + dev_info(afe->dev, "%s(), suspended || irq_id %d, not > > set\n", > > + __func__, irq_id); > > same here. > Got it. > > +static int mt8186_afe_pcm_dev_probe(struct platform_device *pdev) > > +{ > > + struct mtk_base_afe *afe; > > + struct mt8186_afe_private *afe_priv; > > + struct resource *res; > > + struct reset_control *rstc; > > + struct device *dev = &pdev->dev; > > + int i, ret, irq_id; > > + > > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(34)); > > + if (ret) > > + return ret; > > + > > + afe = devm_kzalloc(dev, sizeof(*afe), GFP_KERNEL); > > + if (!afe) > > + return -ENOMEM; > > + platform_set_drvdata(pdev, afe); > > + > > + afe->platform_priv = devm_kzalloc(dev, sizeof(*afe_priv), > > GFP_KERNEL); > > + if (!afe->platform_priv) > > + return -ENOMEM; > > + > > + afe_priv = afe->platform_priv; > > + afe->dev = &pdev->dev; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + afe->base_addr = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(afe->base_addr)) > > + return PTR_ERR(afe->base_addr); > > + > > + /* init audio related clock */ > > + ret = mt8186_init_clock(afe); > > + if (ret) { > > + dev_err(dev, "init clock error, ret %d\n", ret); > > + return ret; > > + } > > + > > + /* init memif */ > > + afe->memif_32bit_supported = 0; > > + afe->memif_size = MT8186_MEMIF_NUM; > > + afe->memif = devm_kcalloc(dev, afe->memif_size, sizeof(*afe- > > >memif), > > + GFP_KERNEL); > > + > > + if (!afe->memif) > > + return -ENOMEM; > > + > > + for (i = 0; i < afe->memif_size; i++) { > > + afe->memif[i].data = &memif_data[i]; > > + afe->memif[i].irq_usage = memif_irq_usage[i]; > > + afe->memif[i].const_irq = 1; > > + } > > + > > + mutex_init(&afe->irq_alloc_lock); /* needed when dynamic irq > > */ > > + > > + /* init irq */ > > + afe->irqs_size = MT8186_IRQ_NUM; > > + afe->irqs = devm_kcalloc(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]; > > + > > + /* request irq */ > > + irq_id = platform_get_irq(pdev, 0); > > + if (irq_id <= 0) > > + return dev_err_probe(dev, irq_id < 0 ? irq_id : -ENXIO, > > + "no irq found"); > > + > > + ret = devm_request_irq(dev, irq_id, mt8186_afe_irq_handler, > > + IRQF_TRIGGER_NONE, > > + "Afe_ISR_Handle", (void *)afe); > > + if (ret) > > + return dev_err_probe(dev, ret, "could not request_irq > > for Afe_ISR_Handle\n"); > > + > > + ret = enable_irq_wake(irq_id); > > + if (ret < 0) > > + return dev_err_probe(dev, ret, "enable_irq_wake %d\n", > > irq_id); > > + > > + /* 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) > > + return dev_err_probe(dev, ret, "dai register i > > %d fail\n", i); > > + } > > + > > + /* init dai_driver and component_driver */ > > + ret = mtk_afe_combine_sub_dai(afe); > > + if (ret) > > + return dev_err_probe(dev, ret, "mtk_afe_combine_sub_dai > > fail\n"); > > + > > + /* reset controller to reset audio regs before regmap cache */ > > + rstc = devm_reset_control_get_exclusive(dev, "audiosys"); > > + if (IS_ERR(rstc)) > > + return dev_err_probe(dev, PTR_ERR(rstc), "could not get > > audiosys reset\n"); > > + > > + ret = reset_control_reset(rstc); > > + if (ret) > > + return dev_err_probe(dev, ret, "failed to trigger audio > > reset\n"); > > + > > + /* enable clock for regcache get default value from hw */ > > + afe_priv->pm_runtime_bypass_reg_ctl = true; > > > > + pm_runtime_enable(dev); > > What about.... > > ret = devm_pm_runtime_enable(dev); > if (ret) > return ret; > > ret = pm_runtime_resume_and_get(dev); > if (ret) > return dev_err_probe(dev, ret, "failed to resume > device\n"); > Yes, pm_runime_resume_and_get() will do error when pm_runtime_get_sync(). > > + ret = pm_runtime_get_sync(dev); > > + if (ret) { > > + dev_err(dev, "failed to resume device: %d\n", ret); > > + goto err_pm_disable; > > + } > > + > > + afe->regmap = devm_regmap_init_mmio(dev, afe->base_addr, > > + &mt8186_afe_regmap_config); > > + if (IS_ERR(afe->regmap)) { > > + ret = PTR_ERR(afe->regmap); > > + goto err_pm_disable; > > + } > > + > > + /* others */ > > + afe->mtk_afe_hardware = &mt8186_afe_hardware; > > + afe->memif_fs = mt8186_memif_fs; > > + afe->irq_fs = mt8186_irq_fs; > > + afe->get_dai_fs = mt8186_get_dai_fs; > > + afe->get_memif_pbuf_size = mt8186_get_memif_pbuf_size; > > + > > + afe->runtime_resume = mt8186_afe_runtime_resume; > > + afe->runtime_suspend = mt8186_afe_runtime_suspend; > > + > > + /* register platform */ > > + dev_info(dev, "%s(), devm_snd_soc_register_component\n", > > __func__); > > This should be dev_dbg(). > Got it. > > + > > + ret = devm_snd_soc_register_component(dev, > > + &mt8186_afe_component, > > + afe->dai_drivers, > > + afe->num_dai_drivers); > > + if (ret) { > > + dev_err(dev, "err_dai_component\n"); > > + goto err_pm_disable; > > + } > > + > > + ret = pm_runtime_put_sync(dev); > > + if (ret) { > > + pm_runtime_get_noresume(dev); > > + dev_err(dev, "failed to suspend device: %d\n", ret); > > + goto err_pm_disable; > > + } > > + afe_priv->pm_runtime_bypass_reg_ctl = false; > > + > > + regcache_cache_only(afe->regmap, true); > > + regcache_mark_dirty(afe->regmap); > > + > > + return 0; > > + > > +err_pm_disable: > > + pm_runtime_put_noidle(dev); > > + pm_runtime_set_suspended(dev); > > + pm_runtime_disable(dev); > > ...and then, since we're using devm_pm_runtime_enable, this call to > pm_runtime_disable() can be removed. > Yes, we can remove this 4 lines if we use devm_pm_runtime_enable() and pm_runime_resume_and_get(). > > + > > + return ret; > > +} > > + > > +static int mt8186_afe_pcm_dev_remove(struct platform_device *pdev) > > +{ > > + struct mtk_base_afe *afe = platform_get_drvdata(pdev); > > + > > + pm_runtime_disable(&pdev->dev); > > Here too. > Got it. > > Regards, > Angelo
This series of patches adds support for Mediatek AFE of MT8186 Soc. Patches are based on broonie tree "for-next" branch. Changes since v6: - The Makefile and Kconfig updates are moved into the driver patches so that can keep independence of each patch. Changes since v5: - fix build error about function snd_soc_card_jack_new that modified by: Link: https://lore.kernel.org/r/20220408041114.6024-1-akihiko.odaki@gmail.com - some have been accepted, details are in the link below: Link: https://lore.kernel.org/all/165459931885.399031.2621579592368573898.b4-ty@kernel.org/ Changes since v4: - [v5 07/20] - remove unsusd controls - [v5 09/20] - correct indent error - [v5 10/20] - [v5 13/20] - [v5 14/20] - fix the return value if the value is different from the previous value in mixer controls - [v5 17/20] - [v5 19/20] - correct the compatible name with '_' instead of '-' - [v5 18/20] - [v5 20/20] - correct the yaml after 'pip3 install dtschema --upgrade' Changes since v3: - [v4 09/18] - remove DEBUG_COEFF debugging code - [v4 10/18] - simplify the logic of the code - [v4 13/18] - split out the MT6366 bits into mt8186-mt6366-commom.c - fix build error of "error: 'struct dev_pm_info' has no member named 'runtime_error'" - fix bug of adda dai driver - add route for pcm interface channel 2. - [v4 15/18] - [v4 17/18] - commonize the configuration of the codec - move MT6366 common bits into mt8186-mt6366-common.c Changes since v2: - add a new compatible string "mediatek,mt6366-sound" - modify the log level for simplicity - use dev_err_probe(...) instead of dev_err(...) in dev probe() - optimized the logic of some code - use BIT() and GENMASK() macros to descript the registers Thanks for AngeloGioacchino's careful reviews. Changes since v1: [v2 01/17] - add a new ID to the existing mt6358 codec driver [v2 03/17] - modify log level in DAPM events - use standard numeric control with name ending in Switch - return 1 when the value changed in mixer control's .get callback [v2 05/17] - ending in Switch to the standard on/off controls - change to "HW Gain 1 Volume" and "HW Gain 2 Volume" [v2 09/17] - return an error in the default case rather than just picking one of the behaviours when do .set_fmt - use the new defines that are _PROVIDER_MASK, _DAIFMT_CBP_CFP and _DAIFMT_CBC_CFC [v2 10/17] [v2 11/17] - the clock and gpio are aplit out into separate patches The source file's GPL comment use c++ style, and the header fils's GPL comment use c style. We have added "Switch" after the names of all the controls that just are simple on/off. Jiaxin Yu (8): dt-bindings: mediatek: mt6358: add new compatible for using mt6366 ASoC: mediatek: mt8186: add platform driver ASoC: mediatek: mt8186: add mt8186-mt6366 common driver dt-bindings: mediatek: mt8186: add audio afe document ASoC: mediatek: mt8186: add machine driver with mt6366, da7219 and max98357 dt-bindings: mediatek: mt8186: add mt8186-mt6366-da7219-max98357 document ASoC: mediatek: mt8186: add machine driver with mt6366, rt1019 and rt5682s dt-bindings: mediatek: mt8186: add mt8186-mt6366-rt1019-rt5682s document .../devicetree/bindings/sound/mt6358.txt | 4 +- .../bindings/sound/mt8186-afe-pcm.yaml | 175 + .../sound/mt8186-mt6366-da7219-max98357.yaml | 75 + .../sound/mt8186-mt6366-rt1019-rt5682s.yaml | 75 + sound/soc/mediatek/Kconfig | 44 + sound/soc/mediatek/Makefile | 1 + sound/soc/mediatek/mt8186/Makefile | 22 + sound/soc/mediatek/mt8186/mt8186-afe-common.h | 235 ++ .../soc/mediatek/mt8186/mt8186-afe-control.c | 261 ++ sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3009 +++++++++++++++++ .../mediatek/mt8186/mt8186-mt6366-common.c | 59 + .../mediatek/mt8186/mt8186-mt6366-common.h | 17 + .../mt8186/mt8186-mt6366-da7219-max98357.c | 1002 ++++++ .../mt8186/mt8186-mt6366-rt1019-rt5682s.c | 978 ++++++ 14 files changed, 5956 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/sound/mt8186-afe-pcm.yaml create mode 100644 Documentation/devicetree/bindings/sound/mt8186-mt6366-da7219-max98357.yaml create mode 100644 Documentation/devicetree/bindings/sound/mt8186-mt6366-rt1019-rt5682s.yaml create mode 100644 sound/soc/mediatek/mt8186/Makefile create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-common.h create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-control.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-pcm.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-common.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-common.h create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-rt1019-rt5682s.c