Re: [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
On Thu, Mar 12, 2015 at 10:17:11AM +0800, Songjun Wu wrote: > Enable WM8731 to support common clock framework. Applied, thanks. signature.asc Description: Digital signature
Re: [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
On Thu, Mar 12, 2015 at 10:17:11AM +0800, Songjun Wu wrote: > Enable WM8731 to support common clock framework. > > Signed-off-by: Songjun Wu > --- Looks ok to me: Acked-by: Charles Keepax Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
Enable WM8731 to support common clock framework.
Signed-off-by: Songjun Wu
---
sound/soc/codecs/wm8731.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index 098c143..8df1550 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -45,6 +46,7 @@ static const char *wm8731_supply_names[WM8731_NUM_SUPPLIES] =
{
/* codec private data */
struct wm8731_priv {
struct regmap *regmap;
+ struct clk *mclk;
struct regulator_bulk_data supplies[WM8731_NUM_SUPPLIES];
const struct snd_pcm_hw_constraint_list *constraints;
unsigned int sysclk;
@@ -390,6 +392,8 @@ static int wm8731_set_dai_sysclk(struct snd_soc_dai
*codec_dai,
switch (clk_id) {
case WM8731_SYSCLK_XTAL:
case WM8731_SYSCLK_MCLK:
+ if (wm8731->mclk && clk_set_rate(wm8731->mclk, freq))
+ return -EINVAL;
wm8731->sysclk_type = clk_id;
break;
default:
@@ -491,6 +495,8 @@ static int wm8731_set_bias_level(struct snd_soc_codec
*codec,
switch (level) {
case SND_SOC_BIAS_ON:
+ if (wm8731->mclk)
+ clk_prepare_enable(wm8731->mclk);
break;
case SND_SOC_BIAS_PREPARE:
break;
@@ -509,6 +515,8 @@ static int wm8731_set_bias_level(struct snd_soc_codec
*codec,
snd_soc_write(codec, WM8731_PWR, reg | 0x0040);
break;
case SND_SOC_BIAS_OFF:
+ if (wm8731->mclk)
+ clk_disable_unprepare(wm8731->mclk);
snd_soc_write(codec, WM8731_PWR, 0x);
regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies),
wm8731->supplies);
@@ -667,6 +675,19 @@ static int wm8731_spi_probe(struct spi_device *spi)
if (wm8731 == NULL)
return -ENOMEM;
+ wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
+ if (IS_ERR(wm8731->mclk)) {
+ ret = PTR_ERR(wm8731->mclk);
+ if (ret == -ENOENT) {
+ wm8731->mclk = NULL;
+ dev_warn(&spi->dev, "Assuming static MCLK\n");
+ } else {
+ dev_err(&spi->dev, "Failed to get MCLK: %d\n",
+ ret);
+ return ret;
+ }
+ }
+
mutex_init(&wm8731->lock);
wm8731->regmap = devm_regmap_init_spi(spi, &wm8731_regmap);
@@ -718,6 +739,19 @@ static int wm8731_i2c_probe(struct i2c_client *i2c,
if (wm8731 == NULL)
return -ENOMEM;
+ wm8731->mclk = devm_clk_get(&i2c->dev, "mclk");
+ if (IS_ERR(wm8731->mclk)) {
+ ret = PTR_ERR(wm8731->mclk);
+ if (ret == -ENOENT) {
+ wm8731->mclk = NULL;
+ dev_warn(&i2c->dev, "Assuming static MCLK\n");
+ } else {
+ dev_err(&i2c->dev, "Failed to get MCLK: %d\n",
+ ret);
+ return ret;
+ }
+ }
+
mutex_init(&wm8731->lock);
wm8731->regmap = devm_regmap_init_i2c(i2c, &wm8731_regmap);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH] ASoC: wm8731: let codec to manage clock by itself
On 02/16/2015 08:58 PM, Manuel Lauss wrote:
On Mon, Feb 16, 2015 at 11:31 AM, Charles Keepax
wrote:
On Thu, Feb 12, 2015 at 04:23:06PM +0800, Bo Shen wrote:
Hi All,
On 02/05/2015 03:35 PM, Bo Shen wrote:
Let the wm8731 codec to manage clock by itself.
Signed-off-by: Bo Shen
---
sound/soc/codecs/wm8731.c | 28
1 file changed, 28 insertions(+)
Any comments for this patch (aka ping?)
I preferred the idea of having the clock as optional and from the
discussion on the last patch it didn't look too tricky to
me too
OK, I will keep the clock as optional.
achieve. OTOH I think the Atmel system is the only one that uses
both this CODEC and common clock so it doesn't look like this
would cause any problems, but might be nice for this not to be
one more thing for someone to fix up when moving a system to
common clock.
Not quite, there's one MIPS platform I maintain which needs a patch
like the one below (tested):
Do I need to seed the following patch together or you send it?
diff --git a/arch/mips/alchemy/devboards/db1200.c
b/arch/mips/alchemy/devboards/db1200.c
index 8c13675..aa01ab2 100644
--- a/arch/mips/alchemy/devboards/db1200.c
+++ b/arch/mips/alchemy/devboards/db1200.c
@@ -19,6 +19,8 @@
*/
#include
+#include
+#include
#include
#include
#include
@@ -862,6 +864,12 @@ int __init db1200_dev_setup(void)
irq_set_status_flags(DB1200_PC1_INSERT_INT, IRQ_NOAUTOEN);
irq_set_status_flags(DB1200_PC1_EJECT_INT, IRQ_NOAUTOEN);
+/* register the 12MHz crystal for the WM8731 I2S codec */
+c = clk_register_fixed_rate(NULL, "wm8731xtal", NULL,
+CLK_IS_ROOT, 1200);
+if (!IS_ERR(c))
+clk_register_clkdev(c, "mclk", "0-001b");
+
i2c_register_board_info(0, db1200_i2c_devs,
ARRAY_SIZE(db1200_i2c_devs));
spi_register_board_info(db1200_spi_devs,
diff --git a/arch/mips/alchemy/devboards/db1300.c
b/arch/mips/alchemy/devboards/db1300.c
index 1c64fdb..1aa01c8 100644
--- a/arch/mips/alchemy/devboards/db1300.c
+++ b/arch/mips/alchemy/devboards/db1300.c
@@ -5,6 +5,8 @@
*/
#include
+#include
+#include
#include
#include
#include
@@ -799,6 +801,12 @@ int __init db1300_dev_setup(void)
if (platform_driver_register(&db1300_wm97xx_driver))
pr_warn("DB1300: failed to init touch pen irq support!\n");
+/* register the 12MHz crystal for the WM8731 I2S codec */
+c = clk_register_fixed_rate(NULL, "wm8731xtal", NULL,
+CLK_IS_ROOT, 1200);
+if (!IS_ERR(c))
+clk_register_clkdev(c, "mclk", "0-001b");
+
/* Audio PSC clock is supplied by codecs (PSC1, 2) */
__raw_writel(PSC_SEL_CLK_SERCLK,
(void __iomem *)KSEG1ADDR(AU1300_PSC1_PHYS_ADDR) + PSC_SEL_OFFSET);
Best Regards,
Bo Shen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH] ASoC: wm8731: let codec to manage clock by itself
On Mon, Feb 16, 2015 at 11:31 AM, Charles Keepax
wrote:
> On Thu, Feb 12, 2015 at 04:23:06PM +0800, Bo Shen wrote:
>> Hi All,
>>
>> On 02/05/2015 03:35 PM, Bo Shen wrote:
>>> Let the wm8731 codec to manage clock by itself.
>>>
>>> Signed-off-by: Bo Shen
>>> ---
>>>
>>> sound/soc/codecs/wm8731.c | 28
>>> 1 file changed, 28 insertions(+)
>>
>> Any comments for this patch (aka ping?)
>
> I preferred the idea of having the clock as optional and from the
> discussion on the last patch it didn't look too tricky to
me too
> achieve. OTOH I think the Atmel system is the only one that uses
> both this CODEC and common clock so it doesn't look like this
> would cause any problems, but might be nice for this not to be
> one more thing for someone to fix up when moving a system to
> common clock.
Not quite, there's one MIPS platform I maintain which needs a patch
like the one below (tested):
diff --git a/arch/mips/alchemy/devboards/db1200.c
b/arch/mips/alchemy/devboards/db1200.c
index 8c13675..aa01ab2 100644
--- a/arch/mips/alchemy/devboards/db1200.c
+++ b/arch/mips/alchemy/devboards/db1200.c
@@ -19,6 +19,8 @@
*/
#include
+#include
+#include
#include
#include
#include
@@ -862,6 +864,12 @@ int __init db1200_dev_setup(void)
irq_set_status_flags(DB1200_PC1_INSERT_INT, IRQ_NOAUTOEN);
irq_set_status_flags(DB1200_PC1_EJECT_INT, IRQ_NOAUTOEN);
+/* register the 12MHz crystal for the WM8731 I2S codec */
+c = clk_register_fixed_rate(NULL, "wm8731xtal", NULL,
+CLK_IS_ROOT, 1200);
+if (!IS_ERR(c))
+clk_register_clkdev(c, "mclk", "0-001b");
+
i2c_register_board_info(0, db1200_i2c_devs,
ARRAY_SIZE(db1200_i2c_devs));
spi_register_board_info(db1200_spi_devs,
diff --git a/arch/mips/alchemy/devboards/db1300.c
b/arch/mips/alchemy/devboards/db1300.c
index 1c64fdb..1aa01c8 100644
--- a/arch/mips/alchemy/devboards/db1300.c
+++ b/arch/mips/alchemy/devboards/db1300.c
@@ -5,6 +5,8 @@
*/
#include
+#include
+#include
#include
#include
#include
@@ -799,6 +801,12 @@ int __init db1300_dev_setup(void)
if (platform_driver_register(&db1300_wm97xx_driver))
pr_warn("DB1300: failed to init touch pen irq support!\n");
+/* register the 12MHz crystal for the WM8731 I2S codec */
+c = clk_register_fixed_rate(NULL, "wm8731xtal", NULL,
+CLK_IS_ROOT, 1200);
+if (!IS_ERR(c))
+clk_register_clkdev(c, "mclk", "0-001b");
+
/* Audio PSC clock is supplied by codecs (PSC1, 2) */
__raw_writel(PSC_SEL_CLK_SERCLK,
(void __iomem *)KSEG1ADDR(AU1300_PSC1_PHYS_ADDR) + PSC_SEL_OFFSET);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH] ASoC: wm8731: let codec to manage clock by itself
On Thu, Feb 12, 2015 at 04:23:06PM +0800, Bo Shen wrote: > Hi All, > > On 02/05/2015 03:35 PM, Bo Shen wrote: >> Let the wm8731 codec to manage clock by itself. >> >> Signed-off-by: Bo Shen >> --- >> >> sound/soc/codecs/wm8731.c | 28 >> 1 file changed, 28 insertions(+) > > Any comments for this patch (aka ping?) I preferred the idea of having the clock as optional and from the discussion on the last patch it didn't look too tricky to achieve. OTOH I think the Atmel system is the only one that uses both this CODEC and common clock so it doesn't look like this would cause any problems, but might be nice for this not to be one more thing for someone to fix up when moving a system to common clock. Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ASoC: wm8731: let codec to manage clock by itself
Hi All,
On 02/05/2015 03:35 PM, Bo Shen wrote:
Let the wm8731 codec to manage clock by itself.
Signed-off-by: Bo Shen
---
sound/soc/codecs/wm8731.c | 28
1 file changed, 28 insertions(+)
Any comments for this patch (aka ping?)
diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index b115ed8..ecd8424 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -13,6 +13,7 @@
* published by the Free Software Foundation.
*/
+#include
#include
#include
#include
@@ -45,6 +46,7 @@ static const char *wm8731_supply_names[WM8731_NUM_SUPPLIES] =
{
/* codec private data */
struct wm8731_priv {
struct regmap *regmap;
+ struct clk *mclk;
struct regulator_bulk_data supplies[WM8731_NUM_SUPPLIES];
const struct snd_pcm_hw_constraint_list *constraints;
unsigned int sysclk;
@@ -389,6 +391,9 @@ static int wm8731_set_dai_sysclk(struct snd_soc_dai
*codec_dai,
switch (clk_id) {
case WM8731_SYSCLK_XTAL:
case WM8731_SYSCLK_MCLK:
+ if (IS_ENABLED(CONFIG_COMMON_CLK))
+ if (clk_set_rate(wm8731->mclk, freq))
+ return -EINVAL;
wm8731->sysclk_type = clk_id;
break;
default:
@@ -490,6 +495,9 @@ static int wm8731_set_bias_level(struct snd_soc_codec
*codec,
switch (level) {
case SND_SOC_BIAS_ON:
+ if (IS_ENABLED(CONFIG_COMMON_CLK))
+ if (clk_prepare_enable(wm8731->mclk))
+ return -EINVAL;
break;
case SND_SOC_BIAS_PREPARE:
break;
@@ -508,6 +516,8 @@ static int wm8731_set_bias_level(struct snd_soc_codec
*codec,
snd_soc_write(codec, WM8731_PWR, reg | 0x0040);
break;
case SND_SOC_BIAS_OFF:
+ if (IS_ENABLED(CONFIG_COMMON_CLK))
+ clk_disable_unprepare(wm8731->mclk);
snd_soc_write(codec, WM8731_PWR, 0x);
regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies),
wm8731->supplies);
@@ -666,6 +676,15 @@ static int wm8731_spi_probe(struct spi_device *spi)
if (wm8731 == NULL)
return -ENOMEM;
+ if (IS_ENABLED(CONFIG_COMMON_CLK)) {
+ wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
+ if (IS_ERR(wm8731->mclk)) {
+ ret = PTR_ERR(wm8731->mclk);
+ dev_err(&spi->dev, "Failed to get MCLK\n");
+ return ret;
+ }
+ }
+
mutex_init(&wm8731->lock);
wm8731->regmap = devm_regmap_init_spi(spi, &wm8731_regmap);
@@ -717,6 +736,15 @@ static int wm8731_i2c_probe(struct i2c_client *i2c,
if (wm8731 == NULL)
return -ENOMEM;
+ if (IS_ENABLED(CONFIG_COMMON_CLK)) {
+ wm8731->mclk = devm_clk_get(&i2c->dev, "mclk");
+ if (IS_ERR(wm8731->mclk)) {
+ ret = PTR_ERR(wm8731->mclk);
+ dev_err(&i2c->dev, "Failed to get MCLK\n");
+ return ret;
+ }
+ }
+
mutex_init(&wm8731->lock);
wm8731->regmap = devm_regmap_init_i2c(i2c, &wm8731_regmap);
Best Regards,
Bo Shen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ASoC: wm8731: let codec to manage clock by itself
Let the wm8731 codec to manage clock by itself.
Signed-off-by: Bo Shen
---
sound/soc/codecs/wm8731.c | 28
1 file changed, 28 insertions(+)
diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index b115ed8..ecd8424 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -13,6 +13,7 @@
* published by the Free Software Foundation.
*/
+#include
#include
#include
#include
@@ -45,6 +46,7 @@ static const char *wm8731_supply_names[WM8731_NUM_SUPPLIES] =
{
/* codec private data */
struct wm8731_priv {
struct regmap *regmap;
+ struct clk *mclk;
struct regulator_bulk_data supplies[WM8731_NUM_SUPPLIES];
const struct snd_pcm_hw_constraint_list *constraints;
unsigned int sysclk;
@@ -389,6 +391,9 @@ static int wm8731_set_dai_sysclk(struct snd_soc_dai
*codec_dai,
switch (clk_id) {
case WM8731_SYSCLK_XTAL:
case WM8731_SYSCLK_MCLK:
+ if (IS_ENABLED(CONFIG_COMMON_CLK))
+ if (clk_set_rate(wm8731->mclk, freq))
+ return -EINVAL;
wm8731->sysclk_type = clk_id;
break;
default:
@@ -490,6 +495,9 @@ static int wm8731_set_bias_level(struct snd_soc_codec
*codec,
switch (level) {
case SND_SOC_BIAS_ON:
+ if (IS_ENABLED(CONFIG_COMMON_CLK))
+ if (clk_prepare_enable(wm8731->mclk))
+ return -EINVAL;
break;
case SND_SOC_BIAS_PREPARE:
break;
@@ -508,6 +516,8 @@ static int wm8731_set_bias_level(struct snd_soc_codec
*codec,
snd_soc_write(codec, WM8731_PWR, reg | 0x0040);
break;
case SND_SOC_BIAS_OFF:
+ if (IS_ENABLED(CONFIG_COMMON_CLK))
+ clk_disable_unprepare(wm8731->mclk);
snd_soc_write(codec, WM8731_PWR, 0x);
regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies),
wm8731->supplies);
@@ -666,6 +676,15 @@ static int wm8731_spi_probe(struct spi_device *spi)
if (wm8731 == NULL)
return -ENOMEM;
+ if (IS_ENABLED(CONFIG_COMMON_CLK)) {
+ wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
+ if (IS_ERR(wm8731->mclk)) {
+ ret = PTR_ERR(wm8731->mclk);
+ dev_err(&spi->dev, "Failed to get MCLK\n");
+ return ret;
+ }
+ }
+
mutex_init(&wm8731->lock);
wm8731->regmap = devm_regmap_init_spi(spi, &wm8731_regmap);
@@ -717,6 +736,15 @@ static int wm8731_i2c_probe(struct i2c_client *i2c,
if (wm8731 == NULL)
return -ENOMEM;
+ if (IS_ENABLED(CONFIG_COMMON_CLK)) {
+ wm8731->mclk = devm_clk_get(&i2c->dev, "mclk");
+ if (IS_ERR(wm8731->mclk)) {
+ ret = PTR_ERR(wm8731->mclk);
+ dev_err(&i2c->dev, "Failed to get MCLK\n");
+ return ret;
+ }
+ }
+
mutex_init(&wm8731->lock);
wm8731->regmap = devm_regmap_init_i2c(i2c, &wm8731_regmap);
--
2.3.0.rc0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
On Wed, Feb 04, 2015 at 11:45:29AM +0800, Bo Shen wrote: > Do you mean I send my RFC patch as the formal patch, and let other boards > which use the wm8731 to add clk object, am I right? No, we need to keep the boards working so we either need patches adding the fixed clocks for existing boards or we need to have the handling of missing clocks that Manuel suggested. signature.asc Description: Digital signature
Re: [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
Hi Mark,
On 02/04/2015 12:21 AM, Mark Brown wrote:
On Tue, Feb 03, 2015 at 03:40:45PM +0100, Manuel Lauss wrote:
On Tue, Feb 3, 2015 at 1:44 PM, Mark Brown wrote:
+wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
+if (IS_ERR(wm8731->mclk)) {
+wm8731->mclk = NULL;
+dev_warn(&spi->dev, "assuming static MCLK\n");
+}
This is broken for both deferred probe and in the case where the clock
API genuinely returns a NULL clock. Other than that it's the kind of
thing that we've done for some other drivers, though it's not good to
have to do this. Check them for correct behaviour.
Hm, so the only option is to create the simples possible 12MHz clk object?
Well, that's the best option in general. You can get away with just
making sure that -EPROBE_DEFER is handled and that IS_ERR() is used to
check for an invalid clock but if you can define a clock that's even
better (and should be pretty painless), we're going to want to do that
transition at some point.
Do you mean I send my RFC patch as the formal patch, and let other
boards which use the wm8731 to add clk object, am I right?
Best Regards,
Bo Shen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
On 02/03/2015 06:26 PM, Lars-Peter Clausen wrote:
On 02/03/2015 06:17 PM, Russell King - ARM Linux wrote:
On Tue, Feb 03, 2015 at 05:53:48PM +0100, Lars-Peter Clausen wrote:
On 02/03/2015 01:44 PM, Mark Brown wrote:
On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote:
+wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
+if (IS_ERR(wm8731->mclk)) {
+wm8731->mclk = NULL;
+dev_warn(&spi->dev, "assuming static MCLK\n");
+}
This is broken for both deferred probe and in the case where the clock
API genuinely returns a NULL clock. Other than that it's the kind of
thing that we've done for some other drivers, though it's not good to
have to do this. Check them for correct behaviour.
Ideally we'd introduce a {devm_}clk_get_optional(), with the same semantics
as gpiod_get_optional(), which handles the finer details of differentiating
between clock specified, but not yet probed, clock specified, but
incorrectly and no clock specified, so this doesn't have to be done over and
over by each driver.
No, we don't need to. It clk_get() already knows this distinction, and
it appropriately returns -ENOENT vs -EPROBE_DEFER according to whether
there's a clock specified in DT or not.
I know, but it returns a error when no clock is specified (-ENOENT), whereas
gpiod_get_optional()-like semantics mean, it would return no error.
What I wanted to say is that pretty much every user of clk_get() that wants
a optional clock gets the handling wrong. E.g. they check for PTR_ERR(clk)
== -EPROBE_DEFER rather than checking for PTR_ERR(clk) != -ENOENT. Which
causes errors when the clock is specified, but incorrectly specified (e.g.
invalid phandle or specifier) to be silently ignored.
My hope is that having a explicit API for requesting a optional clock might
make it easier for users to gets things right.
If you have coccinelle you can use the following script to find good and bad
users:
@@
expression clk;
@@
clk =
(
devm_clk_get
|
clk_get
)
(...);
<+...
(
*PTR_ERR(clk) == -EPROBE_DEFER
|
*PTR_ERR(clk) != -ENOENT
)
...+>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
On 02/03/2015 06:17 PM, Russell King - ARM Linux wrote:
On Tue, Feb 03, 2015 at 05:53:48PM +0100, Lars-Peter Clausen wrote:
On 02/03/2015 01:44 PM, Mark Brown wrote:
On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote:
+wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
+if (IS_ERR(wm8731->mclk)) {
+wm8731->mclk = NULL;
+dev_warn(&spi->dev, "assuming static MCLK\n");
+}
This is broken for both deferred probe and in the case where the clock
API genuinely returns a NULL clock. Other than that it's the kind of
thing that we've done for some other drivers, though it's not good to
have to do this. Check them for correct behaviour.
Ideally we'd introduce a {devm_}clk_get_optional(), with the same semantics
as gpiod_get_optional(), which handles the finer details of differentiating
between clock specified, but not yet probed, clock specified, but
incorrectly and no clock specified, so this doesn't have to be done over and
over by each driver.
No, we don't need to. It clk_get() already knows this distinction, and
it appropriately returns -ENOENT vs -EPROBE_DEFER according to whether
there's a clock specified in DT or not.
I know, but it returns a error when no clock is specified (-ENOENT), whereas
gpiod_get_optional()-like semantics mean, it would return no error.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
On Tue, Feb 03, 2015 at 05:53:48PM +0100, Lars-Peter Clausen wrote:
> On 02/03/2015 01:44 PM, Mark Brown wrote:
> >On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote:
> >
> >>+wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
> >>+if (IS_ERR(wm8731->mclk)) {
> >>+wm8731->mclk = NULL;
> >>+dev_warn(&spi->dev, "assuming static MCLK\n");
> >>+}
> >
> >This is broken for both deferred probe and in the case where the clock
> >API genuinely returns a NULL clock. Other than that it's the kind of
> >thing that we've done for some other drivers, though it's not good to
> >have to do this. Check them for correct behaviour.
>
> Ideally we'd introduce a {devm_}clk_get_optional(), with the same semantics
> as gpiod_get_optional(), which handles the finer details of differentiating
> between clock specified, but not yet probed, clock specified, but
> incorrectly and no clock specified, so this doesn't have to be done over and
> over by each driver.
No, we don't need to. It clk_get() already knows this distinction, and
it appropriately returns -ENOENT vs -EPROBE_DEFER according to whether
there's a clock specified in DT or not.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
On 02/03/2015 01:44 PM, Mark Brown wrote:
On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote:
+wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
+if (IS_ERR(wm8731->mclk)) {
+wm8731->mclk = NULL;
+dev_warn(&spi->dev, "assuming static MCLK\n");
+}
This is broken for both deferred probe and in the case where the clock
API genuinely returns a NULL clock. Other than that it's the kind of
thing that we've done for some other drivers, though it's not good to
have to do this. Check them for correct behaviour.
Ideally we'd introduce a {devm_}clk_get_optional(), with the same semantics
as gpiod_get_optional(), which handles the finer details of differentiating
between clock specified, but not yet probed, clock specified, but
incorrectly and no clock specified, so this doesn't have to be done over and
over by each driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
On Tue, Feb 03, 2015 at 03:40:45PM +0100, Manuel Lauss wrote:
> On Tue, Feb 3, 2015 at 1:44 PM, Mark Brown wrote:
> >> +wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
> >> +if (IS_ERR(wm8731->mclk)) {
> >> +wm8731->mclk = NULL;
> >> +dev_warn(&spi->dev, "assuming static MCLK\n");
> >> +}
> > This is broken for both deferred probe and in the case where the clock
> > API genuinely returns a NULL clock. Other than that it's the kind of
> > thing that we've done for some other drivers, though it's not good to
> > have to do this. Check them for correct behaviour.
> Hm, so the only option is to create the simples possible 12MHz clk object?
Well, that's the best option in general. You can get away with just
making sure that -EPROBE_DEFER is handled and that IS_ERR() is used to
check for an invalid clock but if you can define a clock that's even
better (and should be pretty painless), we're going to want to do that
transition at some point.
signature.asc
Description: Digital signature
Re: [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
On Tue, Feb 3, 2015 at 1:44 PM, Mark Brown wrote:
> On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote:
>
>> +wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
>> +if (IS_ERR(wm8731->mclk)) {
>> +wm8731->mclk = NULL;
>> +dev_warn(&spi->dev, "assuming static MCLK\n");
>> +}
>
> This is broken for both deferred probe and in the case where the clock
> API genuinely returns a NULL clock. Other than that it's the kind of
> thing that we've done for some other drivers, though it's not good to
> have to do this. Check them for correct behaviour.
Hm, so the only option is to create the simples possible 12MHz clk object?
Manuel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote:
> +wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
> +if (IS_ERR(wm8731->mclk)) {
> +wm8731->mclk = NULL;
> +dev_warn(&spi->dev, "assuming static MCLK\n");
> +}
This is broken for both deferred probe and in the case where the clock
API genuinely returns a NULL clock. Other than that it's the kind of
thing that we've done for some other drivers, though it's not good to
have to do this. Check them for correct behaviour.
The coding style is also not right for the whole patch and there's a
lot of missing error checking.
signature.asc
Description: Digital signature
Re: [RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
On Tue, Feb 3, 2015 at 4:33 AM, Bo Shen wrote:
> Let the wm8731 codec to manage clock by itself.
>
> As all at91 related boards have been switch to CCF framework. So, on
> the at91 related boards which use wm8731 will let it manage the clock,
> or else the board use wm8731 is broken.
>
> However, at the same time the wm8731 codec is used on other boards,
> I am sure this change will broken some boards.
>
> For example: poodle and corgi based on PXA SoC (in default configuration
> file, no one use it). DB1200 board which is a mips based board. So I have
> no idea how to fix them.
>
> So, my suggestion is to add CCF check based on the following patch? Any
> idea or suggestions?
What about the patch below? It makes absence of mclk object non-fatal and
checks if wm8731->mclk is non-NULL before enabling/disabling it. Works on
my DB1200/DB1300 boards:
diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index b115ed8..648b8cd 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -13,6 +13,7 @@
* published by the Free Software Foundation.
*/
+#include
#include
#include
#include
@@ -45,6 +46,7 @@ static const char
*wm8731_supply_names[WM8731_NUM_SUPPLIES] = {
/* codec private data */
struct wm8731_priv {
struct regmap *regmap;
+struct clk *mclk;
struct regulator_bulk_data supplies[WM8731_NUM_SUPPLIES];
const struct snd_pcm_hw_constraint_list *constraints;
unsigned int sysclk;
@@ -389,6 +391,8 @@ static int wm8731_set_dai_sysclk(struct
snd_soc_dai *codec_dai,
switch (clk_id) {
case WM8731_SYSCLK_XTAL:
case WM8731_SYSCLK_MCLK:
+if (wm8731->mclk && clk_set_rate(wm8731->mclk, freq))
+return -EINVAL;
wm8731->sysclk_type = clk_id;
break;
default:
@@ -490,6 +494,8 @@ static int wm8731_set_bias_level(struct
snd_soc_codec *codec,
switch (level) {
case SND_SOC_BIAS_ON:
+if (wm8731->mclk)
+clk_prepare_enable(wm8731->mclk);
break;
case SND_SOC_BIAS_PREPARE:
break;
@@ -508,6 +514,8 @@ static int wm8731_set_bias_level(struct
snd_soc_codec *codec,
snd_soc_write(codec, WM8731_PWR, reg | 0x0040);
break;
case SND_SOC_BIAS_OFF:
+if (wm8731->mclk)
+clk_disable_unprepare(wm8731->mclk);
snd_soc_write(codec, WM8731_PWR, 0x);
regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies),
wm8731->supplies);
@@ -666,6 +674,12 @@ static int wm8731_spi_probe(struct spi_device *spi)
if (wm8731 == NULL)
return -ENOMEM;
+wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
+if (IS_ERR(wm8731->mclk)) {
+wm8731->mclk = NULL;
+dev_warn(&spi->dev, "assuming static MCLK\n");
+}
+
mutex_init(&wm8731->lock);
wm8731->regmap = devm_regmap_init_spi(spi, &wm8731_regmap);
@@ -719,6 +733,12 @@ static int wm8731_i2c_probe(struct i2c_client *i2c,
mutex_init(&wm8731->lock);
+wm8731->mclk = devm_clk_get(&i2c->dev, "mclk");
+if (IS_ERR(wm8731->mclk)) {
+wm8731->mclk = NULL;
+dev_warn(&i2c->dev, "assuming static MCLK\n");
+}
+
wm8731->regmap = devm_regmap_init_i2c(i2c, &wm8731_regmap);
if (IS_ERR(wm8731->regmap)) {
ret = PTR_ERR(wm8731->regmap);
--
Manuel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH] ASoC: wm8731: let codec to manage clock by itself
Let the wm8731 codec to manage clock by itself.
As all at91 related boards have been switch to CCF framework. So, on
the at91 related boards which use wm8731 will let it manage the clock,
or else the board use wm8731 is broken.
However, at the same time the wm8731 codec is used on other boards,
I am sure this change will broken some boards.
For example: poodle and corgi based on PXA SoC (in default configuration
file, no one use it). DB1200 board which is a mips based board. So I have
no idea how to fix them.
So, my suggestion is to add CCF check based on the following patch? Any
idea or suggestions?
Signed-off-by: Bo Shen
---
sound/soc/codecs/wm8731.c | 20
1 file changed, 20 insertions(+)
diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index b9211b4..83f75d66 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -13,6 +13,7 @@
* published by the Free Software Foundation.
*/
+#include
#include
#include
#include
@@ -45,6 +46,7 @@ static const char *wm8731_supply_names[WM8731_NUM_SUPPLIES] =
{
/* codec private data */
struct wm8731_priv {
struct regmap *regmap;
+ struct clk *mclk;
struct regulator_bulk_data supplies[WM8731_NUM_SUPPLIES];
const struct snd_pcm_hw_constraint_list *constraints;
unsigned int sysclk;
@@ -389,6 +391,8 @@ static int wm8731_set_dai_sysclk(struct snd_soc_dai
*codec_dai,
switch (clk_id) {
case WM8731_SYSCLK_XTAL:
case WM8731_SYSCLK_MCLK:
+ if (clk_set_rate(wm8731->mclk, freq))
+ return -EINVAL;
wm8731->sysclk_type = clk_id;
break;
default:
@@ -490,6 +494,7 @@ static int wm8731_set_bias_level(struct snd_soc_codec
*codec,
switch (level) {
case SND_SOC_BIAS_ON:
+ clk_prepare_enable(wm8731->mclk);
break;
case SND_SOC_BIAS_PREPARE:
break;
@@ -508,6 +513,7 @@ static int wm8731_set_bias_level(struct snd_soc_codec
*codec,
snd_soc_write(codec, WM8731_PWR, reg | 0x0040);
break;
case SND_SOC_BIAS_OFF:
+ clk_disable_unprepare(wm8731->mclk);
snd_soc_write(codec, WM8731_PWR, 0x);
regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies),
wm8731->supplies);
@@ -666,6 +672,13 @@ static int wm8731_spi_probe(struct spi_device *spi)
if (wm8731 == NULL)
return -ENOMEM;
+ wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
+ if (IS_ERR(wm8731->mclk)) {
+ ret = PTR_ERR(wm8731->mclk);
+ dev_err(&spi->dev, "Failed to get MCLK\n");
+ return ret;
+ }
+
mutex_init(&wm8731->lock);
wm8731->regmap = devm_regmap_init_spi(spi, &wm8731_regmap);
@@ -717,6 +730,13 @@ static int wm8731_i2c_probe(struct i2c_client *i2c,
if (wm8731 == NULL)
return -ENOMEM;
+ wm8731->mclk = devm_clk_get(&i2c->dev, "mclk");
+ if (IS_ERR(wm8731->mclk)) {
+ ret = PTR_ERR(wm8731->mclk);
+ dev_err(&i2c->dev, "Failed to get MCLK\n");
+ return ret;
+ }
+
wm8731->regmap = devm_regmap_init_i2c(i2c, &wm8731_regmap);
if (IS_ERR(wm8731->regmap)) {
ret = PTR_ERR(wm8731->regmap);
--
2.3.0.rc0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
