| Message ID | 1612939421-19900-1-git-send-email-spujar@nvidia.com |
|---|---|
| Headers | show |
| Series | Use clocks property in a device node | expand |
On Wed, 10 Feb 2021 12:13:38 +0530, Sameer Pujar wrote: > It is recommended to not specifiy clocks property in an endpoint subnode. > This series moves clocks to device node. > > However after moving the clocks to device node, the audio playback or > capture fails. The specified clock is not actually getting enabled and > hence the failure is seen. There seems to be a bug in simple-card-utils.c > where clock handle is not assigned when parsing clocks from device node. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/3] ASoC: simple-card-utils: Fix device module clock commit: 1e30f642cf2939bbdac82ea0dd3071232670b5ab [2/3] Revert "ASoC: audio-graph-card: Add clocks property to endpoint node" commit: 0be0f142b8323378df6358c36dd15494134f5b94 [3/3] arm64: tegra: Move clocks from RT5658 endpoint to device node (no commit info) All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi Sameer > diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c > index bc0b62e..0754d70 100644 > --- a/sound/soc/generic/simple-card-utils.c > +++ b/sound/soc/generic/simple-card-utils.c > @@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev, > * or device's module clock. > */ > clk = devm_get_clk_from_child(dev, node, NULL); > - if (!IS_ERR(clk)) { > - simple_dai->sysclk = clk_get_rate(clk); > + if (IS_ERR(clk)) > + clk = devm_get_clk_from_child(dev, dlc->of_node, NULL); > > + if (!IS_ERR(clk)) { > simple_dai->clk = clk; > - } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) { > + simple_dai->sysclk = clk_get_rate(clk); > + } else if (!of_property_read_u32(node, "system-clock-frequency", > + &val)) { > simple_dai->sysclk = val; > - } else { > - clk = devm_get_clk_from_child(dev, dlc->of_node, NULL); > - if (!IS_ERR(clk)) > - simple_dai->sysclk = clk_get_rate(clk); > } The comment is indicating that that the clock parsing order, but this patch exchanges it. This comment also should be updated, I think. /* * Parse dai->sysclk come from "clocks = <&xxx>" * (if system has common clock) * or "system-clock-frequency = <xxx>" * or device's module clock. */ asoc_simple_set_clk_rate() will be called if it has simple_dai->clk. CPU or Codec component clock rate will be exchanged by this patch, I think. I'm not sure the effect of this patch to existing boards. And also, this patch has too many unneeded exchange, thus it was difficult to read for me. I think it can be simply like this ? It is understandable what it want to do. diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c index 8c423afb9d2e..d441890de4dc 100644 --- a/sound/soc/generic/simple-card-utils.c +++ b/sound/soc/generic/simple-card-utils.c @@ -168,16 +168,14 @@ int asoc_simple_parse_clk(struct device *dev, * or device's module clock. */ clk = devm_get_clk_from_child(dev, node, NULL); + if (IS_ERR(clk)) + clk = devm_get_clk_from_child(dev, dlc->of_node, NULL); + if (!IS_ERR(clk)) { simple_dai->sysclk = clk_get_rate(clk); - simple_dai->clk = clk; } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) { simple_dai->sysclk = val; - } else { - clk = devm_get_clk_from_child(dev, dlc->of_node, NULL); - if (!IS_ERR(clk)) - simple_dai->sysclk = clk_get_rate(clk); } if (of_property_read_bool(node, "system-clock-direction-out")) Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Morimoto-san, On 2/12/2021 5:14 AM, Kuninori Morimoto wrote: >> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c >> index bc0b62e..0754d70 100644 >> --- a/sound/soc/generic/simple-card-utils.c >> +++ b/sound/soc/generic/simple-card-utils.c >> @@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev, >> * or device's module clock. >> */ >> clk = devm_get_clk_from_child(dev, node, NULL); >> - if (!IS_ERR(clk)) { >> - simple_dai->sysclk = clk_get_rate(clk); >> + if (IS_ERR(clk)) >> + clk = devm_get_clk_from_child(dev, dlc->of_node, NULL); >> >> + if (!IS_ERR(clk)) { >> simple_dai->clk = clk; >> - } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) { >> + simple_dai->sysclk = clk_get_rate(clk); >> + } else if (!of_property_read_u32(node, "system-clock-frequency", >> + &val)) { >> simple_dai->sysclk = val; >> - } else { >> - clk = devm_get_clk_from_child(dev, dlc->of_node, NULL); >> - if (!IS_ERR(clk)) >> - simple_dai->sysclk = clk_get_rate(clk); >> } > The comment is indicating that that the clock parsing order, > but this patch exchanges it. > This comment also should be updated, I think. > > /* > * Parse dai->sysclk come from "clocks = <&xxx>" > * (if system has common clock) > * or "system-clock-frequency = <xxx>" > * or device's module clock. > */ Yes, this can be rephrased now. > asoc_simple_set_clk_rate() will be called if it has simple_dai->clk. > CPU or Codec component clock rate will be exchanged by this patch, I think. > I'm not sure the effect of this patch to existing boards. If CPU or Codec node does not specifiy "mclk-fs" factor, asoc_simple_set_clk_rate() won't be called. So I don't think there would be any effect w.r.t clock rate. With this patch clocks would get enabled/disabled. > > And also, this patch has too many unneeded exchange, > thus it was difficult to read for me. > I think it can be simply like this ? > It is understandable what it want to do. I think the patch does exactly the same thing as what you are suggesting below. Am I missing anything? > > diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c > index 8c423afb9d2e..d441890de4dc 100644 > --- a/sound/soc/generic/simple-card-utils.c > +++ b/sound/soc/generic/simple-card-utils.c > @@ -168,16 +168,14 @@ int asoc_simple_parse_clk(struct device *dev, > * or device's module clock. > */ > clk = devm_get_clk_from_child(dev, node, NULL); > + if (IS_ERR(clk)) > + clk = devm_get_clk_from_child(dev, dlc->of_node, NULL); > + > if (!IS_ERR(clk)) { > simple_dai->sysclk = clk_get_rate(clk); > - > simple_dai->clk = clk; > } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) { > simple_dai->sysclk = val; > - } else { > - clk = devm_get_clk_from_child(dev, dlc->of_node, NULL); > - if (!IS_ERR(clk)) > - simple_dai->sysclk = clk_get_rate(clk); > } > > if (of_property_read_bool(node, "system-clock-direction-out"))
Hi Sameer > > /* > > * Parse dai->sysclk come from "clocks = <&xxx>" > > * (if system has common clock) > > * or "system-clock-frequency = <xxx>" > > * or device's module clock. > > */ > > Yes, this can be rephrased now. Thanks. It is not a big-deal. no streass :) > > And also, this patch has too many unneeded exchange, > > thus it was difficult to read for me. > > I think it can be simply like this ? > > It is understandable what it want to do. > > I think the patch does exactly the same thing as what you are > suggesting below. Am I missing anything? Yes, it is 100% same, but is simple patch. I wanted to tell was it is easy to read/understand. Your patch is already applied, so nothing we can do now ;) Thank you for your help !! Best regards --- Kuninori Morimoto