Re: [PATCH v2 1/5] ASoC: mediatek: mt8195: merge machine driver

[Tzung-Bi Shih]

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`?