[PATCH v1 1/4] ASoC: Intel: bytcr_rt5651: Get platform data via dev_get_platdata()
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 */
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);
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),
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);elseret = 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");
/** 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),
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).
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;
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;
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.
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;
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.
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.
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; } /*
participants (5)
-
Andy Shevchenko -
Andy Shevchenko -
Hans de Goede -
Mark Brown -
Pierre-Louis Bossart