| Message ID | 20211006150451.16561-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a8627df5491e00e23d4f2e648ff796adbfa23cc5 |
| Headers | show |
| Series | [v1,1/4] ASoC: Intel: bytcr_rt5651: Get platform data via dev_get_platdata() | expand |
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), >
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.
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),
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(-)