| Message ID | 20221208033148.21866-1-trevor.wu@mediatek.com |
|---|---|
| Headers | show |
| Series | ASoC: mediatek: Add support for MT8188 SoC | expand |
Il 08/12/22 04:31, Trevor Wu ha scritto: > Add mt8188 adda dai driver support. > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > --- > I don't add Reviewed-by tag because one new header file is included > in the patch to resolve compiling issue found by kernel test robot. > --- > sound/soc/mediatek/mt8188/mt8188-dai-adda.c | 632 ++++++++++++++++++++ > 1 file changed, 632 insertions(+) > create mode 100644 sound/soc/mediatek/mt8188/mt8188-dai-adda.c
Il 08/12/22 04:31, Trevor Wu ha scritto: > Add mt8188 audio cg clock control. Audio clock gates are registered to CCF > for reference count and clock parent management. > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 08/12/22 04:31, Trevor Wu ha scritto: > Add header files for register definition and structure. > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 08/12/22 04:31, Trevor Wu ha scritto: > SMC call is required to communicate with ATF for some secure operations, > so we add SMC ops IDs and SMC CMD ID to common header. > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 08/12/22 04:31, Trevor Wu ha scritto: > Add mt8188 etdm dai driver support. > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> > --- > I don't add Reviewed-by tag because one new header file is included > in the patch to resolve compiling issue found by kernel test robot. > Additionally, I re-layout the code for better understanding of the > follow-up patch. > --- > sound/soc/mediatek/mt8188/mt8188-dai-etdm.c | 2591 +++++++++++++++++++ > 1 file changed, 2591 insertions(+) > create mode 100644 sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > > diff --git a/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c b/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > new file mode 100644 > index 000000000000..c653fa5e3f85 > --- /dev/null > +++ b/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c ..snip.. > + > +static void mt8188_dai_etdm_parse_of(struct mtk_base_afe *afe) > +{ > + const struct device_node *of_node = afe->dev->of_node; > + struct mt8188_afe_private *afe_priv = afe->platform_priv; > + struct mtk_dai_etdm_priv *etdm_data; > + int i, j; > + char prop[48]; > + u8 disable_chn[MT8188_ETDM_MAX_CHANNELS]; > + int max_chn = MT8188_ETDM_MAX_CHANNELS; > + u32 sel; > + int ret; > + int dai_id; > + unsigned int sync_id; > + struct { > + const char *name; > + const unsigned int sync_id; > + } of_afe_etdms[MT8188_AFE_IO_ETDM_NUM] = { > + {"etdm-in1", ETDM_SYNC_FROM_IN1}, > + {"etdm-in2", ETDM_SYNC_FROM_IN2}, > + {"etdm-out1", ETDM_SYNC_FROM_OUT1}, > + {"etdm-out2", ETDM_SYNC_FROM_OUT2}, > + {"etdm-out3", ETDM_SYNC_FROM_OUT3}, > + }; > + > + for (i = 0; i < MT8188_AFE_IO_ETDM_NUM; i++) { > + dai_id = ETDM_TO_DAI_ID(i); > + etdm_data = afe_priv->dai_priv[dai_id]; > + > + ret = snprintf(prop, sizeof(prop), > + "mediatek,%s-mclk-always-on-rate", > + of_afe_etdms[i].name); > + if (ret < 0) { > + dev_info(afe->dev, "%s snprintf err=%d\n", Is this property optional? If yes, this dev_info() must be a dev_dbg(), otherwise, this must be a dev_err(). Please fix all prints to use the right message level. > + __func__, ret); > + return; Also, is it possible to specify this property only on selected eTDMs? As it is right now, if anyone wants to specify this only on, for example, etdm-out1 and out2, that won't work. In that case, you should replace that return with a `continue`. P.S.: I'm raising this question because you're not forcing "all or nothing" in commit [10/12] where you introduce the bindings for this driver, so I suppose that returning (hence stopping to parse) is a mistake. > + } > + ret = of_property_read_u32(of_node, prop, &sel); > + if (ret == 0) { > + etdm_data->mclk_dir = SND_SOC_CLOCK_OUT; > + if (mtk_dai_etdm_cal_mclk(afe, sel, dai_id)) > + dev_info(afe->dev, "%s unsupported mclk %uHz\n", > + __func__, sel); > + } > + Regards, Angelo
Il 08/12/22 04:31, Trevor Wu ha scritto: > Add mt8188 pcmif dai driver support > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 08/12/22 04:31, Trevor Wu ha scritto: > Add audio clock wrapper and audio tuner control. > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On Tue, 2022-12-13 at 11:45 +0100, AngeloGioacchino Del Regno wrote: > Il 08/12/22 04:31, Trevor Wu ha scritto: > > Add mt8188 etdm dai driver support. > > > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> > > --- > > I don't add Reviewed-by tag because one new header file is included > > in the patch to resolve compiling issue found by kernel test robot. > > Additionally, I re-layout the code for better understanding of the > > follow-up patch. > > --- > > sound/soc/mediatek/mt8188/mt8188-dai-etdm.c | 2591 > > +++++++++++++++++++ > > 1 file changed, 2591 insertions(+) > > create mode 100644 sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > > > > diff --git a/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > > b/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > > new file mode 100644 > > index 000000000000..c653fa5e3f85 > > --- /dev/null > > +++ b/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > > ..snip.. > > > + > > +static void mt8188_dai_etdm_parse_of(struct mtk_base_afe *afe) > > +{ > > + const struct device_node *of_node = afe->dev->of_node; > > + struct mt8188_afe_private *afe_priv = afe->platform_priv; > > + struct mtk_dai_etdm_priv *etdm_data; > > + int i, j; > > + char prop[48]; > > + u8 disable_chn[MT8188_ETDM_MAX_CHANNELS]; > > + int max_chn = MT8188_ETDM_MAX_CHANNELS; > > + u32 sel; > > + int ret; > > + int dai_id; > > + unsigned int sync_id; > > + struct { > > + const char *name; > > + const unsigned int sync_id; > > + } of_afe_etdms[MT8188_AFE_IO_ETDM_NUM] = { > > + {"etdm-in1", ETDM_SYNC_FROM_IN1}, > > + {"etdm-in2", ETDM_SYNC_FROM_IN2}, > > + {"etdm-out1", ETDM_SYNC_FROM_OUT1}, > > + {"etdm-out2", ETDM_SYNC_FROM_OUT2}, > > + {"etdm-out3", ETDM_SYNC_FROM_OUT3}, > > + }; > > + > > + for (i = 0; i < MT8188_AFE_IO_ETDM_NUM; i++) { > > + dai_id = ETDM_TO_DAI_ID(i); > > + etdm_data = afe_priv->dai_priv[dai_id]; > > + > > + ret = snprintf(prop, sizeof(prop), > > + "mediatek,%s-mclk-always-on-rate", > > + of_afe_etdms[i].name); > > + if (ret < 0) { > > + dev_info(afe->dev, "%s snprintf err=%d\n", > > Is this property optional? If yes, this dev_info() must be a > dev_dbg(), > otherwise, this must be a dev_err(). > > Please fix all prints to use the right message level. Hi Angelo, Yes, this property is optional, so I don't use dev_err here. dev_dbg is hard to find the problem. I will make use of dev_err instead of dev_info in V4. > > > + __func__, ret); > > + return; > > Also, is it possible to specify this property only on selected eTDMs? > > As it is right now, if anyone wants to specify this only on, for > example, > etdm-out1 and out2, that won't work. > In that case, you should replace that return with a `continue`. > > P.S.: I'm raising this question because you're not forcing "all or > nothing" > in commit [10/12] where you introduce the bindings for this > driver, > so I suppose that returning (hence stopping to parse) is a > mistake. > It is OK to specify only etdm-out1 and out2 on. This is just an simple error handling for snprintf to silence coverity. Because it shouldn't happen, I don't return an error value to make probe failed. Thanks, Trevor > > + } > > + ret = of_property_read_u32(of_node, prop, &sel); > > + if (ret == 0) { > > + etdm_data->mclk_dir = SND_SOC_CLOCK_OUT; > > + if (mtk_dai_etdm_cal_mclk(afe, sel, dai_id)) > > + dev_info(afe->dev, "%s unsupported mclk > > %uHz\n", > > + __func__, sel); > > + } > > + > > > Regards, > Angelo >