[PATCH v1 1/4] ASoC: Intel: bytcr_rt5651: Get platform data via dev_get_platdata()

[PATCH v1 1/4] ASoC: Intel: bytcr_rt5651: Get platform data via dev_get_platdata()

[Andy Shevchenko]

Access to platform data via dev_get_platdata() getter to make code cleaner.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 00c347b52863..2335cca17cc8 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -894,9 +894,10 @@ struct acpi_chan_package {   /* ACPICA seems to require 64 bit integers */
 
 static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	static const char * const mic_name[] = { "dmic", "in1", "in2", "in12" };
+	struct snd_soc_acpi_mach *mach = dev_get_platdata(dev);
 	struct byt_rt5651_private *priv;
-	struct snd_soc_acpi_mach *mach;
 	const char *platform_name;
 	struct acpi_device *adev;
 	struct device *codec_dev;
@@ -912,8 +913,6 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 
 	/* register the soc card */
 	byt_rt5651_card.dev = &pdev->dev;
-
-	mach = byt_rt5651_card.dev->platform_data;
 	snd_soc_card_set_drvdata(&byt_rt5651_card, priv);
 
 	/* fix index of codec dai */
-- 
2.33.0

[PATCH v1 1/4] ASoC: Intel: bytcr_rt5651: Get platform data via dev_get_platdata()

[Andy Shevchenko]

Access to platform data via dev_get_platdata() getter to make code cleaner.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 00c347b52863..2335cca17cc8 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -894,9 +894,10 @@ struct acpi_chan_package {   /* ACPICA seems to require 64 bit integers */
 
 static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	static const char * const mic_name[] = { "dmic", "in1", "in2", "in12" };
+	struct snd_soc_acpi_mach *mach = dev_get_platdata(dev);
 	struct byt_rt5651_private *priv;
-	struct snd_soc_acpi_mach *mach;
 	const char *platform_name;
 	struct acpi_device *adev;
 	struct device *codec_dev;
@@ -912,8 +913,6 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 
 	/* register the soc card */
 	byt_rt5651_card.dev = &pdev->dev;
-
-	mach = byt_rt5651_card.dev->platform_data;
 	snd_soc_card_set_drvdata(&byt_rt5651_card, priv);
 
 	/* fix index of codec dai */
-- 
2.33.0

[PATCH v1 2/4] ASoC: Intel: bytcr_rt5651: Use temporary variable for struct device

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 42 ++++++++++++---------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 2335cca17cc8..36f63516f0cb 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -907,12 +907,12 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	int dai_index = 0;
 	int i;
 
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
 	/* register the soc card */
-	byt_rt5651_card.dev = &pdev->dev;
+	byt_rt5651_card.dev = dev;
 	snd_soc_card_set_drvdata(&byt_rt5651_card, priv);
 
 	/* fix index of codec dai */
@@ -932,7 +932,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 		put_device(&adev->dev);
 		byt_rt5651_dais[dai_index].codecs->name = byt_rt5651_codec_name;
 	} else {
-		dev_err(&pdev->dev, "Error cannot find '%s' dev\n", mach->id);
+		dev_err(dev, "Error cannot find '%s' dev\n", mach->id);
 		return -ENXIO;
 	}
 
@@ -980,13 +980,13 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 							       &pkg_ctx);
 		if (pkg_found) {
 			if (chan_package.aif_value == 1) {
-				dev_info(&pdev->dev, "BIOS Routing: AIF1 connected\n");
+				dev_info(dev, "BIOS Routing: AIF1 connected\n");
 				byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF1;
 			} else  if (chan_package.aif_value == 2) {
-				dev_info(&pdev->dev, "BIOS Routing: AIF2 connected\n");
+				dev_info(dev, "BIOS Routing: AIF2 connected\n");
 				byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF2;
 			} else {
-				dev_info(&pdev->dev, "BIOS Routing isn't valid, ignored\n");
+				dev_info(dev, "BIOS Routing isn't valid, ignored\n");
 				pkg_found = false;
 			}
 		}
@@ -1001,7 +1001,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	dmi_check_system(byt_rt5651_quirk_table);
 
 	if (quirk_override != -1) {
-		dev_info(&pdev->dev, "Overriding quirk 0x%lx => 0x%x\n",
+		dev_info(dev, "Overriding quirk 0x%lx => 0x%x\n",
 			 byt_rt5651_quirk, quirk_override);
 		byt_rt5651_quirk = quirk_override;
 	}
@@ -1017,8 +1017,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 
 	if (byt_rt5651_gpios) {
 		devm_acpi_dev_add_driver_gpios(codec_dev, byt_rt5651_gpios);
-		priv->ext_amp_gpio = devm_fwnode_gpiod_get(&pdev->dev,
-							   codec_dev->fwnode,
+		priv->ext_amp_gpio = devm_fwnode_gpiod_get(dev, codec_dev->fwnode,
 							   "ext-amp-enable",
 							   GPIOD_OUT_LOW,
 							   "speaker-amp");
@@ -1029,15 +1028,13 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 				priv->ext_amp_gpio = NULL;
 				break;
 			default:
-				dev_err(&pdev->dev, "Failed to get ext-amp-enable GPIO: %d\n",
-					ret_val);
+				dev_err(dev, "Failed to get ext-amp-enable GPIO: %d\n", ret_val);
 				fallthrough;
 			case -EPROBE_DEFER:
 				goto err;
 			}
 		}
-		priv->hp_detect = devm_fwnode_gpiod_get(&pdev->dev,
-							codec_dev->fwnode,
+		priv->hp_detect = devm_fwnode_gpiod_get(dev, codec_dev->fwnode,
 							"hp-detect",
 							GPIOD_IN,
 							"hp-detect");
@@ -1048,8 +1045,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 				priv->hp_detect = NULL;
 				break;
 			default:
-				dev_err(&pdev->dev, "Failed to get hp-detect GPIO: %d\n",
-					ret_val);
+				dev_err(dev, "Failed to get hp-detect GPIO: %d\n", ret_val);
 				fallthrough;
 			case -EPROBE_DEFER:
 				goto err;
@@ -1057,7 +1053,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 		}
 	}
 
-	log_quirks(&pdev->dev);
+	log_quirks(dev);
 
 	if ((byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) ||
 	    (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2))
@@ -1068,10 +1064,10 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 		byt_rt5651_dais[dai_index].cpus->dai_name = "ssp0-port";
 
 	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
-		priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
+		priv->mclk = devm_clk_get(dev, "pmc_plt_clk_3");
 		if (IS_ERR(priv->mclk)) {
 			ret_val = PTR_ERR(priv->mclk);
-			dev_err(&pdev->dev,
+			dev_err(dev,
 				"Failed to get MCLK from pmc_plt_clk_3: %d\n",
 				ret_val);
 			/*
@@ -1111,7 +1107,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	if (ret_val)
 		goto err;
 
-	sof_parent = snd_soc_acpi_sof_parent(&pdev->dev);
+	sof_parent = snd_soc_acpi_sof_parent(dev);
 
 	/* set card and driver name */
 	if (sof_parent) {
@@ -1124,13 +1120,11 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 
 	/* set pm ops */
 	if (sof_parent)
-		pdev->dev.driver->pm = &snd_soc_pm_ops;
-
-	ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5651_card);
+		dev->driver->pm = &snd_soc_pm_ops;
 
+	ret_val = devm_snd_soc_register_card(dev, &byt_rt5651_card);
 	if (ret_val) {
-		dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n",
-			ret_val);
+		dev_err(dev, "devm_snd_soc_register_card failed %d\n", ret_val);
 		goto err;
 	}
 	platform_set_drvdata(pdev, &byt_rt5651_card);
-- 
2.33.0

[PATCH v1 2/4] ASoC: Intel: bytcr_rt5651: Use temporary variable for struct device

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 42 ++++++++++++---------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 2335cca17cc8..36f63516f0cb 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -907,12 +907,12 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	int dai_index = 0;
 	int i;
 
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
 	/* register the soc card */
-	byt_rt5651_card.dev = &pdev->dev;
+	byt_rt5651_card.dev = dev;
 	snd_soc_card_set_drvdata(&byt_rt5651_card, priv);
 
 	/* fix index of codec dai */
@@ -932,7 +932,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 		put_device(&adev->dev);
 		byt_rt5651_dais[dai_index].codecs->name = byt_rt5651_codec_name;
 	} else {
-		dev_err(&pdev->dev, "Error cannot find '%s' dev\n", mach->id);
+		dev_err(dev, "Error cannot find '%s' dev\n", mach->id);
 		return -ENXIO;
 	}
 
@@ -980,13 +980,13 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 							       &pkg_ctx);
 		if (pkg_found) {
 			if (chan_package.aif_value == 1) {
-				dev_info(&pdev->dev, "BIOS Routing: AIF1 connected\n");
+				dev_info(dev, "BIOS Routing: AIF1 connected\n");
 				byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF1;
 			} else  if (chan_package.aif_value == 2) {
-				dev_info(&pdev->dev, "BIOS Routing: AIF2 connected\n");
+				dev_info(dev, "BIOS Routing: AIF2 connected\n");
 				byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF2;
 			} else {
-				dev_info(&pdev->dev, "BIOS Routing isn't valid, ignored\n");
+				dev_info(dev, "BIOS Routing isn't valid, ignored\n");
 				pkg_found = false;
 			}
 		}
@@ -1001,7 +1001,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	dmi_check_system(byt_rt5651_quirk_table);
 
 	if (quirk_override != -1) {
-		dev_info(&pdev->dev, "Overriding quirk 0x%lx => 0x%x\n",
+		dev_info(dev, "Overriding quirk 0x%lx => 0x%x\n",
 			 byt_rt5651_quirk, quirk_override);
 		byt_rt5651_quirk = quirk_override;
 	}
@@ -1017,8 +1017,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 
 	if (byt_rt5651_gpios) {
 		devm_acpi_dev_add_driver_gpios(codec_dev, byt_rt5651_gpios);
-		priv->ext_amp_gpio = devm_fwnode_gpiod_get(&pdev->dev,
-							   codec_dev->fwnode,
+		priv->ext_amp_gpio = devm_fwnode_gpiod_get(dev, codec_dev->fwnode,
 							   "ext-amp-enable",
 							   GPIOD_OUT_LOW,
 							   "speaker-amp");
@@ -1029,15 +1028,13 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 				priv->ext_amp_gpio = NULL;
 				break;
 			default:
-				dev_err(&pdev->dev, "Failed to get ext-amp-enable GPIO: %d\n",
-					ret_val);
+				dev_err(dev, "Failed to get ext-amp-enable GPIO: %d\n", ret_val);
 				fallthrough;
 			case -EPROBE_DEFER:
 				goto err;
 			}
 		}
-		priv->hp_detect = devm_fwnode_gpiod_get(&pdev->dev,
-							codec_dev->fwnode,
+		priv->hp_detect = devm_fwnode_gpiod_get(dev, codec_dev->fwnode,
 							"hp-detect",
 							GPIOD_IN,
 							"hp-detect");
@@ -1048,8 +1045,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 				priv->hp_detect = NULL;
 				break;
 			default:
-				dev_err(&pdev->dev, "Failed to get hp-detect GPIO: %d\n",
-					ret_val);
+				dev_err(dev, "Failed to get hp-detect GPIO: %d\n", ret_val);
 				fallthrough;
 			case -EPROBE_DEFER:
 				goto err;
@@ -1057,7 +1053,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 		}
 	}
 
-	log_quirks(&pdev->dev);
+	log_quirks(dev);
 
 	if ((byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) ||
 	    (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2))
@@ -1068,10 +1064,10 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 		byt_rt5651_dais[dai_index].cpus->dai_name = "ssp0-port";
 
 	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
-		priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
+		priv->mclk = devm_clk_get(dev, "pmc_plt_clk_3");
 		if (IS_ERR(priv->mclk)) {
 			ret_val = PTR_ERR(priv->mclk);
-			dev_err(&pdev->dev,
+			dev_err(dev,
 				"Failed to get MCLK from pmc_plt_clk_3: %d\n",
 				ret_val);
 			/*
@@ -1111,7 +1107,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	if (ret_val)
 		goto err;
 
-	sof_parent = snd_soc_acpi_sof_parent(&pdev->dev);
+	sof_parent = snd_soc_acpi_sof_parent(dev);
 
 	/* set card and driver name */
 	if (sof_parent) {
@@ -1124,13 +1120,11 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 
 	/* set pm ops */
 	if (sof_parent)
-		pdev->dev.driver->pm = &snd_soc_pm_ops;
-
-	ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5651_card);
+		dev->driver->pm = &snd_soc_pm_ops;
 
+	ret_val = devm_snd_soc_register_card(dev, &byt_rt5651_card);
 	if (ret_val) {
-		dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n",
-			ret_val);
+		dev_err(dev, "devm_snd_soc_register_card failed %d\n", ret_val);
 		goto err;
 	}
 	platform_set_drvdata(pdev, &byt_rt5651_card);
-- 
2.33.0

[PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Andy Shevchenko]

The devm_clk_get_optional() helper returns NULL when devm_clk_get()
returns -ENOENT. This makes things slightly cleaner. The added benefit
is mostly cosmetic.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 69 ++++++++++++---------------
 1 file changed, 30 insertions(+), 39 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 36f63516f0cb..28c561302e69 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -188,13 +188,10 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 	}
 
 	if (SND_SOC_DAPM_EVENT_ON(event)) {
-		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
-			ret = clk_prepare_enable(priv->mclk);
-			if (ret < 0) {
-				dev_err(card->dev,
-					"could not configure MCLK state");
-				return ret;
-			}
+		ret = clk_prepare_enable(priv->mclk);
+		if (ret < 0) {
+			dev_err(card->dev, "could not configure MCLK state");
+			return ret;
 		}
 		ret = byt_rt5651_prepare_and_enable_pll1(codec_dai, 48000, 50);
 	} else {
@@ -207,8 +204,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 					     48000 * 512,
 					     SND_SOC_CLOCK_IN);
 		if (!ret)
-			if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN)
-				clk_disable_unprepare(priv->mclk);
+			clk_disable_unprepare(priv->mclk);
 	}
 
 	if (ret < 0) {
@@ -629,29 +625,25 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
 		return ret;
 	}
 
-	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
-		/*
-		 * The firmware might enable the clock at
-		 * boot (this information may or may not
-		 * be reflected in the enable clock register).
-		 * To change the rate we must disable the clock
-		 * first to cover these cases. Due to common
-		 * clock framework restrictions that do not allow
-		 * to disable a clock that has not been enabled,
-		 * we need to enable the clock first.
-		 */
-		ret = clk_prepare_enable(priv->mclk);
-		if (!ret)
-			clk_disable_unprepare(priv->mclk);
+	/*
+	 * The firmware might enable the clock at boot (this information
+	 * may or may not be reflected in the enable clock register).
+	 * To change the rate we must disable the clock first to cover
+	 * these cases. Due to common clock framework restrictions that
+	 * do not allow to disable a clock that has not been enabled,
+	 * we need to enable the clock first.
+	 */
+	ret = clk_prepare_enable(priv->mclk);
+	if (!ret)
+		clk_disable_unprepare(priv->mclk);
 
-		if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
-			ret = clk_set_rate(priv->mclk, 25000000);
-		else
-			ret = clk_set_rate(priv->mclk, 19200000);
+	if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
+		ret = clk_set_rate(priv->mclk, 25000000);
+	else
+		ret = clk_set_rate(priv->mclk, 19200000);
 
-		if (ret)
-			dev_err(card->dev, "unable to set MCLK rate\n");
-	}
+	if (ret)
+		dev_err(card->dev, "unable to set MCLK rate\n");
 
 	report = 0;
 	if (BYT_RT5651_JDSRC(byt_rt5651_quirk))
@@ -1064,21 +1056,20 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 		byt_rt5651_dais[dai_index].cpus->dai_name = "ssp0-port";
 
 	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
-		priv->mclk = devm_clk_get(dev, "pmc_plt_clk_3");
+		priv->mclk = devm_clk_get_optional(dev, "pmc_plt_clk_3");
 		if (IS_ERR(priv->mclk)) {
 			ret_val = PTR_ERR(priv->mclk);
 			dev_err(dev,
 				"Failed to get MCLK from pmc_plt_clk_3: %d\n",
 				ret_val);
-			/*
-			 * Fall back to bit clock usage for -ENOENT (clock not
-			 * available likely due to missing dependencies), bail
-			 * for all other errors, including -EPROBE_DEFER
-			 */
-			if (ret_val != -ENOENT)
-				goto err;
-			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
+			goto err;
 		}
+		/*
+		 * Fall back to bit clock usage when clock is not
+		 * available likely due to missing dependencies.
+		 */
+		if (!priv->mclk)
+			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
 	}
 
 	snprintf(byt_rt5651_components, sizeof(byt_rt5651_components),
-- 
2.33.0

[PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Andy Shevchenko]

The devm_clk_get_optional() helper returns NULL when devm_clk_get()
returns -ENOENT. This makes things slightly cleaner. The added benefit
is mostly cosmetic.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 69 ++++++++++++---------------
 1 file changed, 30 insertions(+), 39 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 36f63516f0cb..28c561302e69 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -188,13 +188,10 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 	}
 
 	if (SND_SOC_DAPM_EVENT_ON(event)) {
-		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
-			ret = clk_prepare_enable(priv->mclk);
-			if (ret < 0) {
-				dev_err(card->dev,
-					"could not configure MCLK state");
-				return ret;
-			}
+		ret = clk_prepare_enable(priv->mclk);
+		if (ret < 0) {
+			dev_err(card->dev, "could not configure MCLK state");
+			return ret;
 		}
 		ret = byt_rt5651_prepare_and_enable_pll1(codec_dai, 48000, 50);
 	} else {
@@ -207,8 +204,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 					     48000 * 512,
 					     SND_SOC_CLOCK_IN);
 		if (!ret)
-			if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN)
-				clk_disable_unprepare(priv->mclk);
+			clk_disable_unprepare(priv->mclk);
 	}
 
 	if (ret < 0) {
@@ -629,29 +625,25 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
 		return ret;
 	}
 
-	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
-		/*
-		 * The firmware might enable the clock at
-		 * boot (this information may or may not
-		 * be reflected in the enable clock register).
-		 * To change the rate we must disable the clock
-		 * first to cover these cases. Due to common
-		 * clock framework restrictions that do not allow
-		 * to disable a clock that has not been enabled,
-		 * we need to enable the clock first.
-		 */
-		ret = clk_prepare_enable(priv->mclk);
-		if (!ret)
-			clk_disable_unprepare(priv->mclk);
+	/*
+	 * The firmware might enable the clock at boot (this information
+	 * may or may not be reflected in the enable clock register).
+	 * To change the rate we must disable the clock first to cover
+	 * these cases. Due to common clock framework restrictions that
+	 * do not allow to disable a clock that has not been enabled,
+	 * we need to enable the clock first.
+	 */
+	ret = clk_prepare_enable(priv->mclk);
+	if (!ret)
+		clk_disable_unprepare(priv->mclk);
 
-		if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
-			ret = clk_set_rate(priv->mclk, 25000000);
-		else
-			ret = clk_set_rate(priv->mclk, 19200000);
+	if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
+		ret = clk_set_rate(priv->mclk, 25000000);
+	else
+		ret = clk_set_rate(priv->mclk, 19200000);
 
-		if (ret)
-			dev_err(card->dev, "unable to set MCLK rate\n");
-	}
+	if (ret)
+		dev_err(card->dev, "unable to set MCLK rate\n");
 
 	report = 0;
 	if (BYT_RT5651_JDSRC(byt_rt5651_quirk))
@@ -1064,21 +1056,20 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 		byt_rt5651_dais[dai_index].cpus->dai_name = "ssp0-port";
 
 	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
-		priv->mclk = devm_clk_get(dev, "pmc_plt_clk_3");
+		priv->mclk = devm_clk_get_optional(dev, "pmc_plt_clk_3");
 		if (IS_ERR(priv->mclk)) {
 			ret_val = PTR_ERR(priv->mclk);
 			dev_err(dev,
 				"Failed to get MCLK from pmc_plt_clk_3: %d\n",
 				ret_val);
-			/*
-			 * Fall back to bit clock usage for -ENOENT (clock not
-			 * available likely due to missing dependencies), bail
-			 * for all other errors, including -EPROBE_DEFER
-			 */
-			if (ret_val != -ENOENT)
-				goto err;
-			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
+			goto err;
 		}
+		/*
+		 * Fall back to bit clock usage when clock is not
+		 * available likely due to missing dependencies.
+		 */
+		if (!priv->mclk)
+			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
 	}
 
 	snprintf(byt_rt5651_components, sizeof(byt_rt5651_components),
-- 
2.33.0

[PATCH v1 4/4] ASoC: Intel: bytcr_rt5651: Utilize dev_err_probe() to avoid log saturation

[Andy Shevchenko]

dev_err_probe() avoids printing into log when the deferred probe is invoked.
This is possible when clock provider is pending to appear.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 28c561302e69..5e9c53dadbc7 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -1058,10 +1058,8 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
 		priv->mclk = devm_clk_get_optional(dev, "pmc_plt_clk_3");
 		if (IS_ERR(priv->mclk)) {
-			ret_val = PTR_ERR(priv->mclk);
-			dev_err(dev,
-				"Failed to get MCLK from pmc_plt_clk_3: %d\n",
-				ret_val);
+			ret_val = dev_err_probe(dev, PTR_ERR(priv->mclk),
+						"Failed to get MCLK from pmc_plt_clk_3\n");
 			goto err;
 		}
 		/*
-- 
2.33.0

[PATCH v1 4/4] ASoC: Intel: bytcr_rt5651: Utilize dev_err_probe() to avoid log saturation

[Andy Shevchenko]

dev_err_probe() avoids printing into log when the deferred probe is invoked.
This is possible when clock provider is pending to appear.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 28c561302e69..5e9c53dadbc7 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -1058,10 +1058,8 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
 		priv->mclk = devm_clk_get_optional(dev, "pmc_plt_clk_3");
 		if (IS_ERR(priv->mclk)) {
-			ret_val = PTR_ERR(priv->mclk);
-			dev_err(dev,
-				"Failed to get MCLK from pmc_plt_clk_3: %d\n",
-				ret_val);
+			ret_val = dev_err_probe(dev, PTR_ERR(priv->mclk),
+						"Failed to get MCLK from pmc_plt_clk_3\n");
 			goto err;
 		}
 		/*
-- 
2.33.0

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Pierre-Louis Bossart]

On 10/6/21 10:04 AM, Andy Shevchenko wrote:
> The devm_clk_get_optional() helper returns NULL when devm_clk_get()
> returns -ENOENT. This makes things slightly cleaner. The added benefit
> is mostly cosmetic.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  sound/soc/intel/boards/bytcr_rt5651.c | 69 ++++++++++++---------------
>  1 file changed, 30 insertions(+), 39 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
> index 36f63516f0cb..28c561302e69 100644
> --- a/sound/soc/intel/boards/bytcr_rt5651.c
> +++ b/sound/soc/intel/boards/bytcr_rt5651.c
> @@ -188,13 +188,10 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
>  	}
>  
>  	if (SND_SOC_DAPM_EVENT_ON(event)) {
> -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
> -			ret = clk_prepare_enable(priv->mclk);
> -			if (ret < 0) {
> -				dev_err(card->dev,
> -					"could not configure MCLK state");
> -				return ret;
> -			}
> +		ret = clk_prepare_enable(priv->mclk);
> +		if (ret < 0) {
> +			dev_err(card->dev, "could not configure MCLK state");
> +			return ret;
>  		}

I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
see below it was designed as a fall-back mode. We don't want to return
an error when we know the clock is not present/desired.


>  		ret = byt_rt5651_prepare_and_enable_pll1(codec_dai, 48000, 50);
>  	} else {
> @@ -207,8 +204,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
>  					     48000 * 512,
>  					     SND_SOC_CLOCK_IN);
>  		if (!ret)
> -			if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN)
> -				clk_disable_unprepare(priv->mclk);
> +			clk_disable_unprepare(priv->mclk);
>  	}
>  
>  	if (ret < 0) {
> @@ -629,29 +625,25 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
>  		return ret;
>  	}
>  
> -	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
> -		/*
> -		 * The firmware might enable the clock at
> -		 * boot (this information may or may not
> -		 * be reflected in the enable clock register).
> -		 * To change the rate we must disable the clock
> -		 * first to cover these cases. Due to common
> -		 * clock framework restrictions that do not allow
> -		 * to disable a clock that has not been enabled,
> -		 * we need to enable the clock first.
> -		 */
> -		ret = clk_prepare_enable(priv->mclk);
> -		if (!ret)
> -			clk_disable_unprepare(priv->mclk);
> +	/*
> +	 * The firmware might enable the clock at boot (this information
> +	 * may or may not be reflected in the enable clock register).
> +	 * To change the rate we must disable the clock first to cover
> +	 * these cases. Due to common clock framework restrictions that
> +	 * do not allow to disable a clock that has not been enabled,
> +	 * we need to enable the clock first.
> +	 */
> +	ret = clk_prepare_enable(priv->mclk);
> +	if (!ret)
> +		clk_disable_unprepare(priv->mclk);
>  
> -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
> -			ret = clk_set_rate(priv->mclk, 25000000);
> -		else
> -			ret = clk_set_rate(priv->mclk, 19200000);
> +	if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
> +		ret = clk_set_rate(priv->mclk, 25000000);
> +	else
> +		ret = clk_set_rate(priv->mclk, 19200000);
>  
> -		if (ret)
> -			dev_err(card->dev, "unable to set MCLK rate\n");
> -	}
> +	if (ret)
> +		dev_err(card->dev, "unable to set MCLK rate\n");

same here, why was the quirk removed?

>  
>  	report = 0;
>  	if (BYT_RT5651_JDSRC(byt_rt5651_quirk))
> @@ -1064,21 +1056,20 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>  		byt_rt5651_dais[dai_index].cpus->dai_name = "ssp0-port";
>  
>  	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
> -		priv->mclk = devm_clk_get(dev, "pmc_plt_clk_3");
> +		priv->mclk = devm_clk_get_optional(dev, "pmc_plt_clk_3");
>  		if (IS_ERR(priv->mclk)) {
>  			ret_val = PTR_ERR(priv->mclk);
>  			dev_err(dev,
>  				"Failed to get MCLK from pmc_plt_clk_3: %d\n",
>  				ret_val);
> -			/*
> -			 * Fall back to bit clock usage for -ENOENT (clock not
> -			 * available likely due to missing dependencies), bail
> -			 * for all other errors, including -EPROBE_DEFER
> -			 */
> -			if (ret_val != -ENOENT)
> -				goto err;
> -			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
> +			goto err;
>  		}
> +		/*
> +		 * Fall back to bit clock usage when clock is not
> +		 * available likely due to missing dependencies.
> +		 */
> +		if (!priv->mclk)
> +			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;

that part in the probe looks fine, but the changes above are controversial.

>  	}
>  
>  	snprintf(byt_rt5651_components, sizeof(byt_rt5651_components),
> 

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Pierre-Louis Bossart]

On 10/6/21 10:04 AM, Andy Shevchenko wrote:
> The devm_clk_get_optional() helper returns NULL when devm_clk_get()
> returns -ENOENT. This makes things slightly cleaner. The added benefit
> is mostly cosmetic.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  sound/soc/intel/boards/bytcr_rt5651.c | 69 ++++++++++++---------------
>  1 file changed, 30 insertions(+), 39 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
> index 36f63516f0cb..28c561302e69 100644
> --- a/sound/soc/intel/boards/bytcr_rt5651.c
> +++ b/sound/soc/intel/boards/bytcr_rt5651.c
> @@ -188,13 +188,10 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
>  	}
>  
>  	if (SND_SOC_DAPM_EVENT_ON(event)) {
> -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
> -			ret = clk_prepare_enable(priv->mclk);
> -			if (ret < 0) {
> -				dev_err(card->dev,
> -					"could not configure MCLK state");
> -				return ret;
> -			}
> +		ret = clk_prepare_enable(priv->mclk);
> +		if (ret < 0) {
> +			dev_err(card->dev, "could not configure MCLK state");
> +			return ret;
>  		}

I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
see below it was designed as a fall-back mode. We don't want to return
an error when we know the clock is not present/desired.


>  		ret = byt_rt5651_prepare_and_enable_pll1(codec_dai, 48000, 50);
>  	} else {
> @@ -207,8 +204,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
>  					     48000 * 512,
>  					     SND_SOC_CLOCK_IN);
>  		if (!ret)
> -			if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN)
> -				clk_disable_unprepare(priv->mclk);
> +			clk_disable_unprepare(priv->mclk);
>  	}
>  
>  	if (ret < 0) {
> @@ -629,29 +625,25 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
>  		return ret;
>  	}
>  
> -	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
> -		/*
> -		 * The firmware might enable the clock at
> -		 * boot (this information may or may not
> -		 * be reflected in the enable clock register).
> -		 * To change the rate we must disable the clock
> -		 * first to cover these cases. Due to common
> -		 * clock framework restrictions that do not allow
> -		 * to disable a clock that has not been enabled,
> -		 * we need to enable the clock first.
> -		 */
> -		ret = clk_prepare_enable(priv->mclk);
> -		if (!ret)
> -			clk_disable_unprepare(priv->mclk);
> +	/*
> +	 * The firmware might enable the clock at boot (this information
> +	 * may or may not be reflected in the enable clock register).
> +	 * To change the rate we must disable the clock first to cover
> +	 * these cases. Due to common clock framework restrictions that
> +	 * do not allow to disable a clock that has not been enabled,
> +	 * we need to enable the clock first.
> +	 */
> +	ret = clk_prepare_enable(priv->mclk);
> +	if (!ret)
> +		clk_disable_unprepare(priv->mclk);
>  
> -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
> -			ret = clk_set_rate(priv->mclk, 25000000);
> -		else
> -			ret = clk_set_rate(priv->mclk, 19200000);
> +	if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
> +		ret = clk_set_rate(priv->mclk, 25000000);
> +	else
> +		ret = clk_set_rate(priv->mclk, 19200000);
>  
> -		if (ret)
> -			dev_err(card->dev, "unable to set MCLK rate\n");
> -	}
> +	if (ret)
> +		dev_err(card->dev, "unable to set MCLK rate\n");

same here, why was the quirk removed?

>  
>  	report = 0;
>  	if (BYT_RT5651_JDSRC(byt_rt5651_quirk))
> @@ -1064,21 +1056,20 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>  		byt_rt5651_dais[dai_index].cpus->dai_name = "ssp0-port";
>  
>  	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
> -		priv->mclk = devm_clk_get(dev, "pmc_plt_clk_3");
> +		priv->mclk = devm_clk_get_optional(dev, "pmc_plt_clk_3");
>  		if (IS_ERR(priv->mclk)) {
>  			ret_val = PTR_ERR(priv->mclk);
>  			dev_err(dev,
>  				"Failed to get MCLK from pmc_plt_clk_3: %d\n",
>  				ret_val);
> -			/*
> -			 * Fall back to bit clock usage for -ENOENT (clock not
> -			 * available likely due to missing dependencies), bail
> -			 * for all other errors, including -EPROBE_DEFER
> -			 */
> -			if (ret_val != -ENOENT)
> -				goto err;
> -			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
> +			goto err;
>  		}
> +		/*
> +		 * Fall back to bit clock usage when clock is not
> +		 * available likely due to missing dependencies.
> +		 */
> +		if (!priv->mclk)
> +			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;

that part in the probe looks fine, but the changes above are controversial.

>  	}
>  
>  	snprintf(byt_rt5651_components, sizeof(byt_rt5651_components),
> 

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Andy Shevchenko]

On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
> On 10/6/21 10:04 AM, Andy Shevchenko wrote:
> > The devm_clk_get_optional() helper returns NULL when devm_clk_get()
> > returns -ENOENT. This makes things slightly cleaner. The added benefit
> > is mostly cosmetic.

...

> >  	if (SND_SOC_DAPM_EVENT_ON(event)) {
> > -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
> > -			ret = clk_prepare_enable(priv->mclk);
> > -			if (ret < 0) {
> > -				dev_err(card->dev,
> > -					"could not configure MCLK state");
> > -				return ret;
> > -			}
> > +		ret = clk_prepare_enable(priv->mclk);
> > +		if (ret < 0) {
> > +			dev_err(card->dev, "could not configure MCLK state");
> > +			return ret;
> >  		}
> 
> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
> see below it was designed as a fall-back mode. We don't want to return
> an error when we know the clock is not present/desired.

Why should we do a unneeded test? When we switch to the optional, there
will be no error from these CCF APIs. Besides that it drops indentation
level and makes code neat.

...

> same here, why was the quirk removed?

Same answer.

...

> that part in the probe looks fine, but the changes above are controversial.

I didn't get. How controversial? Why? The whole point of _optional is to get
rid of unneeded checks (since they are _anyway_ be called).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Andy Shevchenko]

On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
> On 10/6/21 10:04 AM, Andy Shevchenko wrote:
> > The devm_clk_get_optional() helper returns NULL when devm_clk_get()
> > returns -ENOENT. This makes things slightly cleaner. The added benefit
> > is mostly cosmetic.

...

> >  	if (SND_SOC_DAPM_EVENT_ON(event)) {
> > -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
> > -			ret = clk_prepare_enable(priv->mclk);
> > -			if (ret < 0) {
> > -				dev_err(card->dev,
> > -					"could not configure MCLK state");
> > -				return ret;
> > -			}
> > +		ret = clk_prepare_enable(priv->mclk);
> > +		if (ret < 0) {
> > +			dev_err(card->dev, "could not configure MCLK state");
> > +			return ret;
> >  		}
> 
> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
> see below it was designed as a fall-back mode. We don't want to return
> an error when we know the clock is not present/desired.

Why should we do a unneeded test? When we switch to the optional, there
will be no error from these CCF APIs. Besides that it drops indentation
level and makes code neat.

...

> same here, why was the quirk removed?

Same answer.

...

> that part in the probe looks fine, but the changes above are controversial.

I didn't get. How controversial? Why? The whole point of _optional is to get
rid of unneeded checks (since they are _anyway_ be called).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Pierre-Louis Bossart]

On 10/6/21 11:23 AM, Andy Shevchenko wrote:
> On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
>> On 10/6/21 10:04 AM, Andy Shevchenko wrote:
>>> The devm_clk_get_optional() helper returns NULL when devm_clk_get()
>>> returns -ENOENT. This makes things slightly cleaner. The added benefit
>>> is mostly cosmetic.
> 
> ...
> 
>>>  	if (SND_SOC_DAPM_EVENT_ON(event)) {
>>> -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
>>> -			ret = clk_prepare_enable(priv->mclk);
>>> -			if (ret < 0) {
>>> -				dev_err(card->dev,
>>> -					"could not configure MCLK state");
>>> -				return ret;
>>> -			}
>>> +		ret = clk_prepare_enable(priv->mclk);
>>> +		if (ret < 0) {
>>> +			dev_err(card->dev, "could not configure MCLK state");
>>> +			return ret;
>>>  		}
>>
>> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
>> see below it was designed as a fall-back mode. We don't want to return
>> an error when we know the clock is not present/desired.
> 
> Why should we do a unneeded test? When we switch to the optional, there
> will be no error from these CCF APIs. Besides that it drops indentation
> level and makes code neat.

By looking at this code only one cannot really visualize that it's a
no-op. I personally prefer to see explicit intent rather than have to
dig hundreds of lines below what this clock is optional.

I am also not even sure that in real products this clock is actually
optional, the default is to make use of it:

#define BYT_RT5651_DEFAULT_QUIRKS	(BYT_RT5651_MCLK_EN | \

and the only platform without this clock is "Minnowboard Max B3" -
probably not used by anyone. I fried mine a long time ago.

We'd need to Hans to comment on this since he's really the only one
maintaining this code.

> ...
> 
>> same here, why was the quirk removed?
> 
> Same answer.
> 
> ...
> 
>> that part in the probe looks fine, but the changes above are controversial.
> 
> I didn't get. How controversial? Why? The whole point of _optional is to get
> rid of unneeded checks (since they are _anyway_ be called).

It's inconsistent since you kept the following part but no longer use it:

+		/*
+		 * Fall back to bit clock usage when clock is not
+		 * available likely due to missing dependencies.
+		 */
+		if (!priv->mclk)
+			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Pierre-Louis Bossart]

On 10/6/21 11:23 AM, Andy Shevchenko wrote:
> On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
>> On 10/6/21 10:04 AM, Andy Shevchenko wrote:
>>> The devm_clk_get_optional() helper returns NULL when devm_clk_get()
>>> returns -ENOENT. This makes things slightly cleaner. The added benefit
>>> is mostly cosmetic.
> 
> ...
> 
>>>  	if (SND_SOC_DAPM_EVENT_ON(event)) {
>>> -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
>>> -			ret = clk_prepare_enable(priv->mclk);
>>> -			if (ret < 0) {
>>> -				dev_err(card->dev,
>>> -					"could not configure MCLK state");
>>> -				return ret;
>>> -			}
>>> +		ret = clk_prepare_enable(priv->mclk);
>>> +		if (ret < 0) {
>>> +			dev_err(card->dev, "could not configure MCLK state");
>>> +			return ret;
>>>  		}
>>
>> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
>> see below it was designed as a fall-back mode. We don't want to return
>> an error when we know the clock is not present/desired.
> 
> Why should we do a unneeded test? When we switch to the optional, there
> will be no error from these CCF APIs. Besides that it drops indentation
> level and makes code neat.

By looking at this code only one cannot really visualize that it's a
no-op. I personally prefer to see explicit intent rather than have to
dig hundreds of lines below what this clock is optional.

I am also not even sure that in real products this clock is actually
optional, the default is to make use of it:

#define BYT_RT5651_DEFAULT_QUIRKS	(BYT_RT5651_MCLK_EN | \

and the only platform without this clock is "Minnowboard Max B3" -
probably not used by anyone. I fried mine a long time ago.

We'd need to Hans to comment on this since he's really the only one
maintaining this code.

> ...
> 
>> same here, why was the quirk removed?
> 
> Same answer.
> 
> ...
> 
>> that part in the probe looks fine, but the changes above are controversial.
> 
> I didn't get. How controversial? Why? The whole point of _optional is to get
> rid of unneeded checks (since they are _anyway_ be called).

It's inconsistent since you kept the following part but no longer use it:

+		/*
+		 * Fall back to bit clock usage when clock is not
+		 * available likely due to missing dependencies.
+		 */
+		if (!priv->mclk)
+			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Andy Shevchenko]

On Wed, Oct 06, 2021 at 11:37:24AM -0500, Pierre-Louis Bossart wrote:
> On 10/6/21 11:23 AM, Andy Shevchenko wrote:
> > On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
> >> On 10/6/21 10:04 AM, Andy Shevchenko wrote:

...

> >>>  	if (SND_SOC_DAPM_EVENT_ON(event)) {
> >>> -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
> >>> -			ret = clk_prepare_enable(priv->mclk);
> >>> -			if (ret < 0) {
> >>> -				dev_err(card->dev,
> >>> -					"could not configure MCLK state");
> >>> -				return ret;
> >>> -			}
> >>> +		ret = clk_prepare_enable(priv->mclk);
> >>> +		if (ret < 0) {
> >>> +			dev_err(card->dev, "could not configure MCLK state");
> >>> +			return ret;
> >>>  		}
> >>
> >> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
> >> see below it was designed as a fall-back mode. We don't want to return
> >> an error when we know the clock is not present/desired.
> > 
> > Why should we do a unneeded test? When we switch to the optional, there
> > will be no error from these CCF APIs. Besides that it drops indentation
> > level and makes code neat.
> 
> By looking at this code only one cannot really visualize that it's a
> no-op. I personally prefer to see explicit intent rather than have to
> dig hundreds of lines below what this clock is optional.
> 
> I am also not even sure that in real products this clock is actually
> optional,

The code tells that it's optional. If it's not the case, the code has
to be fixed accordingly.

> the default is to make use of it:
> 
> #define BYT_RT5651_DEFAULT_QUIRKS	(BYT_RT5651_MCLK_EN | \
> 
> and the only platform without this clock is "Minnowboard Max B3" -
> probably not used by anyone. I fried mine a long time ago.
> 
> We'd need to Hans to comment on this since he's really the only one
> maintaining this code.

Additional pair of eyes and a brain is always good to have!

...

> >> that part in the probe looks fine, but the changes above are controversial.
> > 
> > I didn't get. How controversial? Why? The whole point of _optional is to get
> > rid of unneeded checks (since they are _anyway_ be called).
> 
> It's inconsistent since you kept the following part but no longer use it:

It's used in some cases, but not with CLK APIs where it's redundant.

Otherwise I would remove it completely (actually it's possible to replace
it with the !priv->mclk test.

> +		/*
> +		 * Fall back to bit clock usage when clock is not
> +		 * available likely due to missing dependencies.
> +		 */
> +		if (!priv->mclk)
> +			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Andy Shevchenko]

On Wed, Oct 06, 2021 at 11:37:24AM -0500, Pierre-Louis Bossart wrote:
> On 10/6/21 11:23 AM, Andy Shevchenko wrote:
> > On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
> >> On 10/6/21 10:04 AM, Andy Shevchenko wrote:

...

> >>>  	if (SND_SOC_DAPM_EVENT_ON(event)) {
> >>> -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
> >>> -			ret = clk_prepare_enable(priv->mclk);
> >>> -			if (ret < 0) {
> >>> -				dev_err(card->dev,
> >>> -					"could not configure MCLK state");
> >>> -				return ret;
> >>> -			}
> >>> +		ret = clk_prepare_enable(priv->mclk);
> >>> +		if (ret < 0) {
> >>> +			dev_err(card->dev, "could not configure MCLK state");
> >>> +			return ret;
> >>>  		}
> >>
> >> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
> >> see below it was designed as a fall-back mode. We don't want to return
> >> an error when we know the clock is not present/desired.
> > 
> > Why should we do a unneeded test? When we switch to the optional, there
> > will be no error from these CCF APIs. Besides that it drops indentation
> > level and makes code neat.
> 
> By looking at this code only one cannot really visualize that it's a
> no-op. I personally prefer to see explicit intent rather than have to
> dig hundreds of lines below what this clock is optional.
> 
> I am also not even sure that in real products this clock is actually
> optional,

The code tells that it's optional. If it's not the case, the code has
to be fixed accordingly.

> the default is to make use of it:
> 
> #define BYT_RT5651_DEFAULT_QUIRKS	(BYT_RT5651_MCLK_EN | \
> 
> and the only platform without this clock is "Minnowboard Max B3" -
> probably not used by anyone. I fried mine a long time ago.
> 
> We'd need to Hans to comment on this since he's really the only one
> maintaining this code.

Additional pair of eyes and a brain is always good to have!

...

> >> that part in the probe looks fine, but the changes above are controversial.
> > 
> > I didn't get. How controversial? Why? The whole point of _optional is to get
> > rid of unneeded checks (since they are _anyway_ be called).
> 
> It's inconsistent since you kept the following part but no longer use it:

It's used in some cases, but not with CLK APIs where it's redundant.

Otherwise I would remove it completely (actually it's possible to replace
it with the !priv->mclk test.

> +		/*
> +		 * Fall back to bit clock usage when clock is not
> +		 * available likely due to missing dependencies.
> +		 */
> +		if (!priv->mclk)
> +			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

On Wed, Oct 06, 2021 at 07:50:45PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 06, 2021 at 11:37:24AM -0500, Pierre-Louis Bossart wrote:

> > By looking at this code only one cannot really visualize that it's a
> > no-op. I personally prefer to see explicit intent rather than have to
> > dig hundreds of lines below what this clock is optional.

> > I am also not even sure that in real products this clock is actually
> > optional,

> The code tells that it's optional. If it's not the case, the code has
> to be fixed accordingly.

AIUI with the clock API the idiomatic thing is that any optionality is
handled at the point where the clock is acquired - if the clock is
optional you end up with NULL which in the clock API is a dummy clock
and ignored.  The rest of the code then doesn't need to worry about any
of this stuff and the handling can only be in one place.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

On Wed, Oct 06, 2021 at 07:50:45PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 06, 2021 at 11:37:24AM -0500, Pierre-Louis Bossart wrote:

> > By looking at this code only one cannot really visualize that it's a
> > no-op. I personally prefer to see explicit intent rather than have to
> > dig hundreds of lines below what this clock is optional.

> > I am also not even sure that in real products this clock is actually
> > optional,

> The code tells that it's optional. If it's not the case, the code has
> to be fixed accordingly.

AIUI with the clock API the idiomatic thing is that any optionality is
handled at the point where the clock is acquired - if the clock is
optional you end up with NULL which in the clock API is a dummy clock
and ignored.  The rest of the code then doesn't need to worry about any
of this stuff and the handling can only be in one place.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Hans de Goede]

Hi,

On 10/6/21 6:37 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 10/6/21 11:23 AM, Andy Shevchenko wrote:
>> On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
>>> On 10/6/21 10:04 AM, Andy Shevchenko wrote:
>>>> The devm_clk_get_optional() helper returns NULL when devm_clk_get()
>>>> returns -ENOENT. This makes things slightly cleaner. The added benefit
>>>> is mostly cosmetic.
>>
>> ...
>>
>>>>  	if (SND_SOC_DAPM_EVENT_ON(event)) {
>>>> -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
>>>> -			ret = clk_prepare_enable(priv->mclk);
>>>> -			if (ret < 0) {
>>>> -				dev_err(card->dev,
>>>> -					"could not configure MCLK state");
>>>> -				return ret;
>>>> -			}
>>>> +		ret = clk_prepare_enable(priv->mclk);
>>>> +		if (ret < 0) {
>>>> +			dev_err(card->dev, "could not configure MCLK state");
>>>> +			return ret;
>>>>  		}
>>>
>>> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
>>> see below it was designed as a fall-back mode. We don't want to return
>>> an error when we know the clock is not present/desired.
>>
>> Why should we do a unneeded test? When we switch to the optional, there
>> will be no error from these CCF APIs. Besides that it drops indentation
>> level and makes code neat.
> 
> By looking at this code only one cannot really visualize that it's a
> no-op. I personally prefer to see explicit intent rather than have to
> dig hundreds of lines below what this clock is optional.
> 
> I am also not even sure that in real products this clock is actually
> optional, the default is to make use of it:
> 
> #define BYT_RT5651_DEFAULT_QUIRKS	(BYT_RT5651_MCLK_EN | \
> 
> and the only platform without this clock is "Minnowboard Max B3" -
> probably not used by anyone. I fried mine a long time ago.
> 
> We'd need to Hans to comment on this since he's really the only one
> maintaining this code.

So as Mark wrote in his later reply:

"AIUI with the clock API the idiomatic thing is that any optionality is
handled at the point where the clock is acquired - if the clock is
optional you end up with NULL which in the clock API is a dummy clock
and ignored.  The rest of the code then doesn't need to worry about any
of this stuff and the handling can only be in one place."

Combined with there pretty much always actually being an mclk I believe
that this patch from Andy results in a nice cleanup so I'm in favor with
this. And the other cleanups also look sensible to me

I would like to run a small smoke-test with both the series to make
sure nothing regresses (should be fine but better safe then sorry).

Andy I believe that there is going to be a v2 to address a couple
of nitpicks, right ?

Note for testing I would prefer a full v2 series, even if some
patches don't change. And I assume the same applies to Mark for
applying this.

Sending partial series with only changed patches on the v2
send turns things into a puzzle, which is not ideal IMHO.

Regards,

Hans





> 
>> ...
>>
>>> same here, why was the quirk removed?
>>
>> Same answer.
>>
>> ...
>>
>>> that part in the probe looks fine, but the changes above are controversial.
>>
>> I didn't get. How controversial? Why? The whole point of _optional is to get
>> rid of unneeded checks (since they are _anyway_ be called).
> 
> It's inconsistent since you kept the following part but no longer use it:
> 
> +		/*
> +		 * Fall back to bit clock usage when clock is not
> +		 * available likely due to missing dependencies.
> +		 */
> +		if (!priv->mclk)
> +			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
> 
> 
> 

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Hans de Goede]

Hi,

On 10/6/21 6:37 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 10/6/21 11:23 AM, Andy Shevchenko wrote:
>> On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
>>> On 10/6/21 10:04 AM, Andy Shevchenko wrote:
>>>> The devm_clk_get_optional() helper returns NULL when devm_clk_get()
>>>> returns -ENOENT. This makes things slightly cleaner. The added benefit
>>>> is mostly cosmetic.
>>
>> ...
>>
>>>>  	if (SND_SOC_DAPM_EVENT_ON(event)) {
>>>> -		if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
>>>> -			ret = clk_prepare_enable(priv->mclk);
>>>> -			if (ret < 0) {
>>>> -				dev_err(card->dev,
>>>> -					"could not configure MCLK state");
>>>> -				return ret;
>>>> -			}
>>>> +		ret = clk_prepare_enable(priv->mclk);
>>>> +		if (ret < 0) {
>>>> +			dev_err(card->dev, "could not configure MCLK state");
>>>> +			return ret;
>>>>  		}
>>>
>>> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
>>> see below it was designed as a fall-back mode. We don't want to return
>>> an error when we know the clock is not present/desired.
>>
>> Why should we do a unneeded test? When we switch to the optional, there
>> will be no error from these CCF APIs. Besides that it drops indentation
>> level and makes code neat.
> 
> By looking at this code only one cannot really visualize that it's a
> no-op. I personally prefer to see explicit intent rather than have to
> dig hundreds of lines below what this clock is optional.
> 
> I am also not even sure that in real products this clock is actually
> optional, the default is to make use of it:
> 
> #define BYT_RT5651_DEFAULT_QUIRKS	(BYT_RT5651_MCLK_EN | \
> 
> and the only platform without this clock is "Minnowboard Max B3" -
> probably not used by anyone. I fried mine a long time ago.
> 
> We'd need to Hans to comment on this since he's really the only one
> maintaining this code.

So as Mark wrote in his later reply:

"AIUI with the clock API the idiomatic thing is that any optionality is
handled at the point where the clock is acquired - if the clock is
optional you end up with NULL which in the clock API is a dummy clock
and ignored.  The rest of the code then doesn't need to worry about any
of this stuff and the handling can only be in one place."

Combined with there pretty much always actually being an mclk I believe
that this patch from Andy results in a nice cleanup so I'm in favor with
this. And the other cleanups also look sensible to me

I would like to run a small smoke-test with both the series to make
sure nothing regresses (should be fine but better safe then sorry).

Andy I believe that there is going to be a v2 to address a couple
of nitpicks, right ?

Note for testing I would prefer a full v2 series, even if some
patches don't change. And I assume the same applies to Mark for
applying this.

Sending partial series with only changed patches on the v2
send turns things into a puzzle, which is not ideal IMHO.

Regards,

Hans





> 
>> ...
>>
>>> same here, why was the quirk removed?
>>
>> Same answer.
>>
>> ...
>>
>>> that part in the probe looks fine, but the changes above are controversial.
>>
>> I didn't get. How controversial? Why? The whole point of _optional is to get
>> rid of unneeded checks (since they are _anyway_ be called).
> 
> It's inconsistent since you kept the following part but no longer use it:
> 
> +		/*
> +		 * Fall back to bit clock usage when clock is not
> +		 * available likely due to missing dependencies.
> +		 */
> +		if (!priv->mclk)
> +			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
> 
> 
> 

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Pierre-Louis Bossart]

>> We'd need to Hans to comment on this since he's really the only one
>> maintaining this code.
> 
> So as Mark wrote in his later reply:
> 
> "AIUI with the clock API the idiomatic thing is that any optionality is
> handled at the point where the clock is acquired - if the clock is
> optional you end up with NULL which in the clock API is a dummy clock
> and ignored.  The rest of the code then doesn't need to worry about any
> of this stuff and the handling can only be in one place."
> 
> Combined with there pretty much always actually being an mclk I believe
> that this patch from Andy results in a nice cleanup so I'm in favor with
> this. And the other cleanups also look sensible to me

No sustained objection on my side.

Thanks Andy for starting this thread.

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Pierre-Louis Bossart]

>> We'd need to Hans to comment on this since he's really the only one
>> maintaining this code.
> 
> So as Mark wrote in his later reply:
> 
> "AIUI with the clock API the idiomatic thing is that any optionality is
> handled at the point where the clock is acquired - if the clock is
> optional you end up with NULL which in the clock API is a dummy clock
> and ignored.  The rest of the code then doesn't need to worry about any
> of this stuff and the handling can only be in one place."
> 
> Combined with there pretty much always actually being an mclk I believe
> that this patch from Andy results in a nice cleanup so I'm in favor with
> this. And the other cleanups also look sensible to me

No sustained objection on my side.

Thanks Andy for starting this thread.

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Andy Shevchenko]

On Wed, Oct 6, 2021 at 8:12 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 10/6/21 6:37 PM, Pierre-Louis Bossart wrote:
> > On 10/6/21 11:23 AM, Andy Shevchenko wrote:
> >> On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
> >>> On 10/6/21 10:04 AM, Andy Shevchenko wrote:

...

> >>> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
> >>> see below it was designed as a fall-back mode. We don't want to return
> >>> an error when we know the clock is not present/desired.
> >>
> >> Why should we do a unneeded test? When we switch to the optional, there
> >> will be no error from these CCF APIs. Besides that it drops indentation
> >> level and makes code neat.
> >
> > By looking at this code only one cannot really visualize that it's a
> > no-op. I personally prefer to see explicit intent rather than have to
> > dig hundreds of lines below what this clock is optional.
> >
> > I am also not even sure that in real products this clock is actually
> > optional, the default is to make use of it:
> >
> > #define BYT_RT5651_DEFAULT_QUIRKS     (BYT_RT5651_MCLK_EN | \
> >
> > and the only platform without this clock is "Minnowboard Max B3" -
> > probably not used by anyone. I fried mine a long time ago.
> >
> > We'd need to Hans to comment on this since he's really the only one
> > maintaining this code.
>
> So as Mark wrote in his later reply:
>
> "AIUI with the clock API the idiomatic thing is that any optionality is
> handled at the point where the clock is acquired - if the clock is
> optional you end up with NULL which in the clock API is a dummy clock
> and ignored.  The rest of the code then doesn't need to worry about any
> of this stuff and the handling can only be in one place."
>
> Combined with there pretty much always actually being an mclk I believe
> that this patch from Andy results in a nice cleanup so I'm in favor with
> this. And the other cleanups also look sensible to me

Thanks!

> I would like to run a small smoke-test with both the series to make
> sure nothing regresses (should be fine but better safe then sorry).

Thanks ahead!

> Andy I believe that there is going to be a v2 to address a couple
> of nitpicks, right ?

Right.

> Note for testing I would prefer a full v2 series, even if some
> patches don't change. And I assume the same applies to Mark for
> applying this.
>
> Sending partial series with only changed patches on the v2
> send turns things into a puzzle, which is not ideal IMHO.

I'll do it tomorrow.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/4] ASoC: Intel: bytcr_rt5651: use devm_clk_get_optional() for mclk

[Andy Shevchenko]

On Wed, Oct 6, 2021 at 8:12 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 10/6/21 6:37 PM, Pierre-Louis Bossart wrote:
> > On 10/6/21 11:23 AM, Andy Shevchenko wrote:
> >> On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
> >>> On 10/6/21 10:04 AM, Andy Shevchenko wrote:

...

> >>> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
> >>> see below it was designed as a fall-back mode. We don't want to return
> >>> an error when we know the clock is not present/desired.
> >>
> >> Why should we do a unneeded test? When we switch to the optional, there
> >> will be no error from these CCF APIs. Besides that it drops indentation
> >> level and makes code neat.
> >
> > By looking at this code only one cannot really visualize that it's a
> > no-op. I personally prefer to see explicit intent rather than have to
> > dig hundreds of lines below what this clock is optional.
> >
> > I am also not even sure that in real products this clock is actually
> > optional, the default is to make use of it:
> >
> > #define BYT_RT5651_DEFAULT_QUIRKS     (BYT_RT5651_MCLK_EN | \
> >
> > and the only platform without this clock is "Minnowboard Max B3" -
> > probably not used by anyone. I fried mine a long time ago.
> >
> > We'd need to Hans to comment on this since he's really the only one
> > maintaining this code.
>
> So as Mark wrote in his later reply:
>
> "AIUI with the clock API the idiomatic thing is that any optionality is
> handled at the point where the clock is acquired - if the clock is
> optional you end up with NULL which in the clock API is a dummy clock
> and ignored.  The rest of the code then doesn't need to worry about any
> of this stuff and the handling can only be in one place."
>
> Combined with there pretty much always actually being an mclk I believe
> that this patch from Andy results in a nice cleanup so I'm in favor with
> this. And the other cleanups also look sensible to me

Thanks!

> I would like to run a small smoke-test with both the series to make
> sure nothing regresses (should be fine but better safe then sorry).

Thanks ahead!

> Andy I believe that there is going to be a v2 to address a couple
> of nitpicks, right ?

Right.

> Note for testing I would prefer a full v2 series, even if some
> patches don't change. And I assume the same applies to Mark for
> applying this.
>
> Sending partial series with only changed patches on the v2
> send turns things into a puzzle, which is not ideal IMHO.

I'll do it tomorrow.


-- 
With Best Regards,
Andy Shevchenko