| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 17 Mar 2022 12:17:30 +0800
From: Trevor Wu <trevor.wu@...iatek.com>
To: Tzung-Bi Shih <tzungbi@...nel.org>
CC: <broonie@...nel.org>, <tiwai@...e.com>, <robh+dt@...nel.org>,
<matthias.bgg@...il.com>, <devicetree@...r.kernel.org>,
<alsa-devel@...a-project.org>, <linux-kernel@...r.kernel.org>,
<linux-mediatek@...ts.infradead.org>, <yc.hung@...iatek.com>,
<aaronyu@...gle.com>, <linux-arm-kernel@...ts.infradead.org>,
<angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v2 1/5] ASoC: mediatek: mt8195: merge machine driver
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
Powered by blists - more mailing lists