| Message ID | 20220316060139.6211-1-trevor.wu@mediatek.com |
|---|---|
| Headers | show |
| Series | ASoC: mediatek: Add support for MT8195 sound card with max98390 and rt5682 | expand |
Hi, I didn't review too many details because I found the patch is not easy to review. Please consider to not reorder symbols if it can. If it is still hard to generate reasonable chunks or the reorders are necessary, it could put some refactor patches prior to the "merge". On Wed, Mar 16, 2022 at 02:01:35PM +0800, Trevor Wu wrote: > diff --git a/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1019-rt5682.c b/sound/soc/mediatek/mt8195/mt8195-mt6359.c [...] > #include <linux/input.h> > #include <linux/module.h> > +#include <linux/of_device.h> > #include <linux/pm_runtime.h> > #include <sound/jack.h> > #include <sound/pcm_params.h> > #include <sound/rt5682.h> > -#include <sound/sof.h> Why does it remove the header? > +struct mt8195_mt6359_priv { > + struct snd_soc_jack headset_jack; > + struct snd_soc_jack dp_jack; > + struct snd_soc_jack hdmi_jack; > + struct clk *i2so1_mclk; > +}; > + > +struct mt8195_card_data { > + const char *name; > + unsigned long quirk; > +}; > + > +struct sof_conn_stream { > + const char *normal_link; > + const char *sof_link; > + const char *sof_dma; > + int stream_dir; > +}; [...] > -struct sof_conn_stream { > - const char *normal_link; > - const char *sof_link; > - const char *sof_dma; > - int stream_dir; > -}; > - > -struct mt8195_mt6359_rt1019_rt5682_priv { > - struct snd_soc_jack headset_jack; > - struct snd_soc_jack dp_jack; > - struct snd_soc_jack hdmi_jack; > - struct clk *i2so1_mclk; > -}; The effective operation here: rename from mt8195_mt6359_rt1019_rt5682_priv to mt8195_mt6359_priv. However, it somehow reorders the code. As a result, the change looks like more complicated than just a "merge" operation. > -static const struct snd_soc_dapm_route mt8195_mt6359_rt1019_rt5682_routes[] = { > - /* speaker */ > - { "Speakers", NULL, "Speaker" }, > +static const struct snd_kcontrol_new mt8195_mt6359_controls[] = { > + SOC_DAPM_PIN_SWITCH("Headphone Jack"), > + SOC_DAPM_PIN_SWITCH("Headset Mic"), > +}; > + > +static const struct snd_soc_dapm_route mt8195_mt6359_routes[] = { > /* headset */ > { "Headphone Jack", NULL, "HPOL" }, > { "Headphone Jack", NULL, "HPOR" }, > @@ -80,55 +94,31 @@ static const struct snd_soc_dapm_route mt8195_mt6359_rt1019_rt5682_routes[] = { > {"I021", NULL, SOF_DMA_DL3}, > }; > > -static const struct snd_kcontrol_new mt8195_mt6359_rt1019_rt5682_controls[] = { > - SOC_DAPM_PIN_SWITCH("Speakers"), > - SOC_DAPM_PIN_SWITCH("Headphone Jack"), > - SOC_DAPM_PIN_SWITCH("Headset Mic"), > +static const struct snd_soc_dapm_widget mt8195_dual_speaker_widgets[] = { > + SND_SOC_DAPM_SPK("Left Speaker", NULL), > + SND_SOC_DAPM_SPK("Right Speaker", NULL), > }; > > -static int mt8195_rt5682_etdm_hw_params(struct snd_pcm_substream *substream, > - struct snd_pcm_hw_params *params) > -{ [...] > +static const struct snd_kcontrol_new mt8195_dual_speaker_controls[] = { > + SOC_DAPM_PIN_SWITCH("Left Speaker"), > + SOC_DAPM_PIN_SWITCH("Right Speaker"), > +}; Ditto. I would expect it only renames and adds something. However, if you look at the block and the following, it looks like changed a lot. > @@ -143,20 +133,20 @@ static int mt8195_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd) > struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt_afe); > struct mt8195_afe_private *afe_priv = afe->platform_priv; > struct mtkaif_param *param = &afe_priv->mtkaif_params; > - int phase; > - unsigned int monitor; > - int mtkaif_calibration_num_phase; > + int chosen_phase_1, chosen_phase_2, chosen_phase_3; > + int prev_cycle_1, prev_cycle_2, prev_cycle_3; > int test_done_1, test_done_2, test_done_3; > int cycle_1, cycle_2, cycle_3; > - int prev_cycle_1, prev_cycle_2, prev_cycle_3; > - int chosen_phase_1, chosen_phase_2, chosen_phase_3; > - int counter; > - bool mtkaif_calibration_ok; > int mtkaif_chosen_phase[MT8195_MTKAIF_MISO_NUM]; > int mtkaif_phase_cycle[MT8195_MTKAIF_MISO_NUM]; > + int mtkaif_calibration_num_phase; > + bool mtkaif_calibration_ok; > + unsigned int monitor; > + int counter; > + int phase; > int i; The reorder of variable declaration is irrelevant to the patch. Drop them. If it has good reason to do so, send another patch for the purpose. > @@ -513,7 +446,7 @@ static int mt8195_playback_startup(struct snd_pcm_substream *substream) > return 0; > } > > -static const struct snd_soc_ops mt8195_playback_ops = { > +const struct snd_soc_ops mt8195_playback_ops = { > .startup = mt8195_playback_startup, Why does it remove the `static`? > +static int mt8195_mt6359_dev_probe(struct platform_device *pdev) > { [...] > + match = of_match_device(pdev->dev.driver->of_match_table, &pdev->dev); > + if (!match || !match->data) > + return -EINVAL; > + > + card_data = (struct mt8195_card_data *)match->data; Use of_device_get_match_data(). > -static const struct dev_pm_ops mt8195_mt6359_rt1019_rt5682_pm_ops = { > +const struct dev_pm_ops mt8195_mt6359_pm_ops = { > .poweroff = snd_soc_poweroff, > .restore = snd_soc_resume, > }; Why does it remove the `static`?
On Thu, 2022-03-17 at 10:24 +0800, Tzung-Bi Shih wrote: > Hi, > I didn't review too many details because I found the patch is not > easy to > review. Please consider to not reorder symbols if it can. If it is > still > hard to generate reasonable chunks or the reorders are necessary, it > could > put some refactor patches prior to the "merge". Hi Tzung-Bi, Thanks for your suggestion. Originally, I try to delete the old machine drivers and create a new one, so the layout is reordered and some functions are copied from mt8195-mt6359-rt1011-rt5682.c. But the git patch becomes a diff with mt8195-mt6359-rt1019-rt5682.c. I can split the one into two patches in v3, one is "merge" and another one is "revise". I hope it can make the review easier. > > On Wed, Mar 16, 2022 at 02:01:35PM +0800, Trevor Wu wrote: > > diff --git a/sound/soc/mediatek/mt8195/mt8195-mt6359-rt1019- > > rt5682.c b/sound/soc/mediatek/mt8195/mt8195-mt6359.c > > [...] > > #include <linux/input.h> > > #include <linux/module.h> > > +#include <linux/of_device.h> > > #include <linux/pm_runtime.h> > > #include <sound/jack.h> > > #include <sound/pcm_params.h> > > #include <sound/rt5682.h> > > -#include <sound/sof.h> > > Why does it remove the header? It seems that the header is redundant, because the driver works on my platform. But I will double confirm it. > > > +struct mt8195_mt6359_priv { > > + struct snd_soc_jack headset_jack; > > + struct snd_soc_jack dp_jack; > > + struct snd_soc_jack hdmi_jack; > > + struct clk *i2so1_mclk; > > +}; > > + > > +struct mt8195_card_data { > > + const char *name; > > + unsigned long quirk; > > +}; > > + > > +struct sof_conn_stream { > > + const char *normal_link; > > + const char *sof_link; > > + const char *sof_dma; > > + int stream_dir; > > +}; > > [...] > > -struct sof_conn_stream { > > - const char *normal_link; > > - const char *sof_link; > > - const char *sof_dma; > > - int stream_dir; > > -}; > > - > > -struct mt8195_mt6359_rt1019_rt5682_priv { > > - struct snd_soc_jack headset_jack; > > - struct snd_soc_jack dp_jack; > > - struct snd_soc_jack hdmi_jack; > > - struct clk *i2so1_mclk; > > -}; > > The effective operation here: rename from > mt8195_mt6359_rt1019_rt5682_priv > to mt8195_mt6359_priv. However, it somehow reorders the code. As a > result, > the change looks like more complicated than just a "merge" operation. > > > -static const struct snd_soc_dapm_route > > mt8195_mt6359_rt1019_rt5682_routes[] = { > > - /* speaker */ > > - { "Speakers", NULL, "Speaker" }, > > +static const struct snd_kcontrol_new mt8195_mt6359_controls[] = { > > + SOC_DAPM_PIN_SWITCH("Headphone Jack"), > > + SOC_DAPM_PIN_SWITCH("Headset Mic"), > > +}; > > + > > +static const struct snd_soc_dapm_route mt8195_mt6359_routes[] = { > > /* headset */ > > { "Headphone Jack", NULL, "HPOL" }, > > { "Headphone Jack", NULL, "HPOR" }, > > @@ -80,55 +94,31 @@ static const struct snd_soc_dapm_route > > mt8195_mt6359_rt1019_rt5682_routes[] = { > > {"I021", NULL, SOF_DMA_DL3}, > > }; > > > > -static const struct snd_kcontrol_new > > mt8195_mt6359_rt1019_rt5682_controls[] = { > > - SOC_DAPM_PIN_SWITCH("Speakers"), > > - SOC_DAPM_PIN_SWITCH("Headphone Jack"), > > - SOC_DAPM_PIN_SWITCH("Headset Mic"), > > +static const struct snd_soc_dapm_widget > > mt8195_dual_speaker_widgets[] = { > > + SND_SOC_DAPM_SPK("Left Speaker", NULL), > > + SND_SOC_DAPM_SPK("Right Speaker", NULL), > > }; > > > > -static int mt8195_rt5682_etdm_hw_params(struct snd_pcm_substream > > *substream, > > - struct snd_pcm_hw_params > > *params) > > -{ > > [...] > > +static const struct snd_kcontrol_new > > mt8195_dual_speaker_controls[] = { > > + SOC_DAPM_PIN_SWITCH("Left Speaker"), > > + SOC_DAPM_PIN_SWITCH("Right Speaker"), > > +}; > > Ditto. I would expect it only renames and adds something. However, > if you > look at the block and the following, it looks like changed a lot. > > > @@ -143,20 +133,20 @@ static int > > mt8195_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd) > > struct mtk_base_afe *afe = > > snd_soc_component_get_drvdata(cmpnt_afe); > > struct mt8195_afe_private *afe_priv = afe->platform_priv; > > struct mtkaif_param *param = &afe_priv->mtkaif_params; > > - int phase; > > - unsigned int monitor; > > - int mtkaif_calibration_num_phase; > > + int chosen_phase_1, chosen_phase_2, chosen_phase_3; > > + int prev_cycle_1, prev_cycle_2, prev_cycle_3; > > int test_done_1, test_done_2, test_done_3; > > int cycle_1, cycle_2, cycle_3; > > - int prev_cycle_1, prev_cycle_2, prev_cycle_3; > > - int chosen_phase_1, chosen_phase_2, chosen_phase_3; > > - int counter; > > - bool mtkaif_calibration_ok; > > int mtkaif_chosen_phase[MT8195_MTKAIF_MISO_NUM]; > > int mtkaif_phase_cycle[MT8195_MTKAIF_MISO_NUM]; > > + int mtkaif_calibration_num_phase; > > + bool mtkaif_calibration_ok; > > + unsigned int monitor; > > + int counter; > > + int phase; > > int i; > > The reorder of variable declaration is irrelevant to the patch. Drop > them. > If it has good reason to do so, send another patch for the purpose. This function is copied from mt8195-mt6359-rt1011-rt5682.c, because this is the latest version of mt8195_mt6359_mtkaif_calibration(). The reordering is suggested by the reviewer. > > > @@ -513,7 +446,7 @@ static int mt8195_playback_startup(struct > > snd_pcm_substream *substream) > > return 0; > > } > > > > -static const struct snd_soc_ops mt8195_playback_ops = { > > +const struct snd_soc_ops mt8195_playback_ops = { > > .startup = mt8195_playback_startup, > > Why does it remove the `static`? Sorry, I will add it in v3. > > > +static int mt8195_mt6359_dev_probe(struct platform_device *pdev) > > { > > [...] > > + match = of_match_device(pdev->dev.driver->of_match_table, > > &pdev->dev); > > + if (!match || !match->data) > > + return -EINVAL; > > + > > + card_data = (struct mt8195_card_data *)match->data; > > Use of_device_get_match_data(). OK. > > > -static const struct dev_pm_ops mt8195_mt6359_rt1019_rt5682_pm_ops > > = { > > +const struct dev_pm_ops mt8195_mt6359_pm_ops = { > > .poweroff = snd_soc_poweroff, > > .restore = snd_soc_resume, > > }; > > Why does it remove the `static`? I will add it in v3. Thanks, Trevor
This series of patches adds support for mt8195 board with mt6359, max98390 and rt5682. To prevent from copy-paste components, mt8195 machine drivers and dt-bindings are merged in the patch. Patches are based on broonie tree "for-next" branch. Changes since v1: - remove merged patches about reset controller - propose a common machine driver instead of machine driver common code - propose a common dt-bindings for mt8195 sound card Trevor Wu (5): ASoC: mediatek: mt8195: merge machine driver ASoC: dt-bindings: mediatek: mt8195: merge mt8195 machine yaml ASoC: mediatek: mt8195: rename card controls ASoC: mediatek: mt8195: add machine support for max98390 and rt5682 ASoC: dt-bindings: mediatek: mt8195: support mt8195-mt6359-max98390-rt5682 .../sound/mt8195-mt6359-rt1011-rt5682.yaml | 51 - ...-rt1019-rt5682.yaml => mt8195-mt6359.yaml} | 9 +- sound/soc/mediatek/Kconfig | 26 +- sound/soc/mediatek/mt8195/Makefile | 3 +- .../mt8195/mt8195-mt6359-rt1011-rt5682.c | 1198 ----------------- ...mt6359-rt1019-rt5682.c => mt8195-mt6359.c} | 984 +++++++++----- 6 files changed, 634 insertions(+), 1637 deletions(-) delete mode 100644 Documentation/devicetree/bindings/sound/mt8195-mt6359-rt1011-rt5682.yaml rename Documentation/devicetree/bindings/sound/{mt8195-mt6359-rt1019-rt5682.yaml => mt8195-mt6359.yaml} (84%) delete mode 100644 sound/soc/mediatek/mt8195/mt8195-mt6359-rt1011-rt5682.c rename sound/soc/mediatek/mt8195/{mt8195-mt6359-rt1019-rt5682.c => mt8195-mt6359.c} (76%)