| Message ID | 20130326190513.1671a3be@armhf |
|---|---|
| State | New |
| Headers | show |
On 03/26/2013 07:05 PM, Jean-Francois Moine wrote: > In the function kirkwood_set_rate, in case of a non dco supported rate > and no external clock, the clock source was set to an undefined value. > This patch just displays a message without changing the clock source. > > Signed-off-by: Jean-Francois Moine<moinejf@free.fr> > --- > sound/soc/kirkwood/kirkwood-i2s.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c > index c74c890..afca1ec 100644 > --- a/sound/soc/kirkwood/kirkwood-i2s.c > +++ b/sound/soc/kirkwood/kirkwood-i2s.c > @@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai, > clk_set_rate(priv->extclk, 256 * rate); > > clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK; > + } else { > + dev_err(dai->dev, "%s: no clock\n", __func__); > + return; > } > writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL); > } NACK. Having no clock at all should be catched during _probe. Moreover, not having the internal clock enabled will lead to system hang due to clock gating. You should rather pass an optional (DT-only) extclk phandle on the second clocks property. Sebastian P.S. as this is alsa stuff you should have some alsa maintainers on your Cc list. Please run ./scripts/get_maintainer.pl on your patches next time. It will give you a list of people you have to to Cc.
On Tue, 26 Mar 2013 20:41:40 +0100 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > On 03/26/2013 07:05 PM, Jean-Francois Moine wrote: > > In the function kirkwood_set_rate, in case of a non dco supported rate > > and no external clock, the clock source was set to an undefined value. > > This patch just displays a message without changing the clock source. > > > > Signed-off-by: Jean-Francois Moine<moinejf@free.fr> > > --- > > sound/soc/kirkwood/kirkwood-i2s.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c > > index c74c890..afca1ec 100644 > > --- a/sound/soc/kirkwood/kirkwood-i2s.c > > +++ b/sound/soc/kirkwood/kirkwood-i2s.c > > @@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai, > > clk_set_rate(priv->extclk, 256 * rate); > > > > clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK; > > + } else { > > + dev_err(dai->dev, "%s: no clock\n", __func__); > > + return; > > } > > writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL); > > } > > NACK. > > Having no clock at all should be catched during _probe. Moreover, > not having the internal clock enabled will lead to system hang due to > clock gating. You should rather pass an optional (DT-only) extclk phandle > on the second clocks property. > > Sebastian > > P.S. as this is alsa stuff you should have some alsa maintainers on your > Cc list. Please run ./scripts/get_maintainer.pl on your patches next time. > It will give you a list of people you have to to Cc. It is a compilation error: the variable clks_ctrl is not initialized. The sequence has been created by the commit 363589bf110aa0352a2031 "ASoC: kirkwood-i2s: add support for external clock rates" Russell is in the Cc list.
On 03/26/2013 09:05 PM, Jean-Francois Moine wrote: > On Tue, 26 Mar 2013 20:41:40 +0100 > Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com> wrote: > >> On 03/26/2013 07:05 PM, Jean-Francois Moine wrote: >>> In the function kirkwood_set_rate, in case of a non dco supported rate >>> and no external clock, the clock source was set to an undefined value. >>> This patch just displays a message without changing the clock source. >>> >>> Signed-off-by: Jean-Francois Moine<moinejf@free.fr> >>> --- >>> sound/soc/kirkwood/kirkwood-i2s.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c >>> index c74c890..afca1ec 100644 >>> --- a/sound/soc/kirkwood/kirkwood-i2s.c >>> +++ b/sound/soc/kirkwood/kirkwood-i2s.c >>> @@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai, >>> clk_set_rate(priv->extclk, 256 * rate); >>> >>> clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK; >>> + } else { >>> + dev_err(dai->dev, "%s: no clock\n", __func__); >>> + return; >>> } >>> writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL); >>> } >> >> NACK. >> >> Having no clock at all should be catched during _probe. Moreover, >> not having the internal clock enabled will lead to system hang due to >> clock gating. You should rather pass an optional (DT-only) extclk phandle >> on the second clocks property. >> >> Sebastian >> >> P.S. as this is alsa stuff you should have some alsa maintainers on your >> Cc list. Please run ./scripts/get_maintainer.pl on your patches next time. >> It will give you a list of people you have to to Cc. > > It is a compilation error: the variable clks_ctrl is not initialized. If it is about non-initialized clks_ctrl, I suggest to remove it completely and clone the writel to both if and else branch. > The sequence has been created by the commit 363589bf110aa0352a2031 > "ASoC: kirkwood-i2s: add support for external clock rates" > Russell is in the Cc list. Russell was the commit author and also should be added because of his ARM maintainer status. But kirkwood-i2s is alsa and needs some sound maintainers to be Cc'd: $ ./scripts/get_maintainer.pl -f sound/soc/kirkwood/kirkwood-i2s.c Liam Girdwood <lgirdwood@gmail.com> (supporter:SOUND - SOC LAYER...) Mark Brown <broonie@opensource.wolfsonmicro.com> (supporter:SOUND - SOC LAYER...,commit_signer:9/11=82%) Jaroslav Kysela <perex@perex.cz> (maintainer:SOUND) Takashi Iwai <tiwai@suse.de> (maintainer:SOUND) Russell King <rmk+kernel@arm.linux.org.uk> (commit_signer:6/11=55%) Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:2/11=18%) Andrew Lumm <andrew@lunn.ch> (commit_signer:2/11=18%) Thierry Reding <thierry.reding@avionic-design.de> (commit_signer:1/11=9%) alsa-devel@alsa-project.org (moderated list:SOUND - SOC LAYER...) linux-kernel@vger.kernel.org (open list) Sebastian
On 03/26/2013 09:05 PM, Jean-Francois Moine wrote: > On Tue, 26 Mar 2013 20:41:40 +0100 > Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com> wrote: > >> On 03/26/2013 07:05 PM, Jean-Francois Moine wrote: >>> In the function kirkwood_set_rate, in case of a non dco supported rate >>> and no external clock, the clock source was set to an undefined value. >>> This patch just displays a message without changing the clock source. >>> >>> Signed-off-by: Jean-Francois Moine<moinejf@free.fr> >>> --- >>> sound/soc/kirkwood/kirkwood-i2s.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c >>> index c74c890..afca1ec 100644 >>> --- a/sound/soc/kirkwood/kirkwood-i2s.c >>> +++ b/sound/soc/kirkwood/kirkwood-i2s.c >>> @@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai, >>> clk_set_rate(priv->extclk, 256 * rate); >>> >>> clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK; >>> + } else { >>> + dev_err(dai->dev, "%s: no clock\n", __func__); >>> + return; >>> } >>> writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL); >>> } >> >> NACK. >> >> Having no clock at all should be catched during _probe. Moreover, >> not having the internal clock enabled will lead to system hang due to >> clock gating. You should rather pass an optional (DT-only) extclk phandle >> on the second clocks property. Jean-Francois, I had a close look at the code and your patch. From a driver point-of-view kirkwood_set_rate should never been called with an unsupported rate (see KIRKWOOD_I2S_RATES) when no extclk is available. With extclk available, the else-if branch will be taken on rates != KIRKWOOD_I2S_RATES. Actually, your added else branch will never be taken at all. I suggest (again) to remove clks_ctrl and move the writel inside if and else-if branch to cure the compiler warning. Sebastian
On Tue, 26 Mar 2013 21:39:40 +0100 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > On 03/26/2013 09:05 PM, Jean-Francois Moine wrote: > > On Tue, 26 Mar 2013 20:41:40 +0100 > > Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com> wrote: > > > >> On 03/26/2013 07:05 PM, Jean-Francois Moine wrote: > >>> In the function kirkwood_set_rate, in case of a non dco supported rate > >>> and no external clock, the clock source was set to an undefined value. > >>> This patch just displays a message without changing the clock source. > >>> > >>> Signed-off-by: Jean-Francois Moine<moinejf@free.fr> > >>> --- > >>> sound/soc/kirkwood/kirkwood-i2s.c | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c > >>> index c74c890..afca1ec 100644 > >>> --- a/sound/soc/kirkwood/kirkwood-i2s.c > >>> +++ b/sound/soc/kirkwood/kirkwood-i2s.c > >>> @@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai, > >>> clk_set_rate(priv->extclk, 256 * rate); > >>> > >>> clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK; > >>> + } else { > >>> + dev_err(dai->dev, "%s: no clock\n", __func__); > >>> + return; > >>> } > >>> writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL); > >>> } > >> > >> NACK. > >> > >> Having no clock at all should be catched during _probe. Moreover, > >> not having the internal clock enabled will lead to system hang due to > >> clock gating. You should rather pass an optional (DT-only) extclk phandle > >> on the second clocks property. > > Jean-Francois, > > I had a close look at the code and your patch. From a driver > point-of-view kirkwood_set_rate should never been called with > an unsupported rate (see KIRKWOOD_I2S_RATES) when no extclk > is available. With extclk available, the else-if branch will > be taken on rates != KIRKWOOD_I2S_RATES. Actually, your added > else branch will never be taken at all. > > I suggest (again) to remove clks_ctrl and move the writel inside > if and else-if branch to cure the compiler warning. > > Sebastian That's what there was in the original patch from Rabeeh, but maybe it is the opportunity to use WARN_ON. Russell?
On Wed, Mar 27, 2013 at 08:31:57AM +0100, Jean-Francois Moine wrote: > On Tue, 26 Mar 2013 21:39:40 +0100 > Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > > I suggest (again) to remove clks_ctrl and move the writel inside > > if and else-if branch to cure the compiler warning. > > > > Sebastian > > That's what there was in the original patch from Rabeeh, but maybe it > is the opportunity to use WARN_ON. Russell? Sebastian is correct in that such a path should _never_ be reached because ALSA will reject anything but 44.1, 48 or 96kHz rates if we don't have an extclk. However, I disagree with Sebastian's solution - doing that is likely to generate more code because gcc tends not to optimise: if (foo) { writel(some_value, register); } else { writel(some_other_value, register); } very well. It will generate two separate writel()s when in fact, it could just do: if (foo) { val = some_value; } else { val = some_other_value; } writel(val, register); One solution to this would be to just get rid of "if (!IS_ERR(priv->extclk))" entirely, so that the "else" clause always assumes that if we hit that, there will be an external clock. If it does, the clk API gets to deal with being passed an error pointer for a clock, and we switch to extclk mode. Either that'll cause a nice backtrace from the kernel (which is good in this case) or audio will just not work. Remember, though - if there isn't an extclk set, then we should never get there in the first place. If it makes people feel happier, also put a WARN_ON(IS_ERR(priv->extclk)) there to guarantee a backtrace if it happens.
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index c74c890..afca1ec 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai, clk_set_rate(priv->extclk, 256 * rate); clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK; + } else { + dev_err(dai->dev, "%s: no clock\n", __func__); + return; } writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL); }
In the function kirkwood_set_rate, in case of a non dco supported rate and no external clock, the clock source was set to an undefined value. This patch just displays a message without changing the clock source. Signed-off-by: Jean-Francois Moine <moinejf@free.fr> --- sound/soc/kirkwood/kirkwood-i2s.c | 3 +++ 1 file changed, 3 insertions(+)