Re: [PATCH 03/11] ASoC: fsl: add sgtl5000 clock support for imx-sgtl5000
On Wed, May 02, 2012 at 08:12:24PM +0800, Shawn Guo wrote: > On Wed, May 02, 2012 at 12:41:22PM +0100, Mark Brown wrote: > > The major problem here is that we can't rely on having the clock API > > present at all on most platforms so as soon as we start adding clock > > support into things we have to do a performance with HAVE_CLK which is > > at best annoying. > Is this something that the series below tries to solve? > http://thread.gmane.org/gmane.linux.ports.arm.kernel/163652 Well, it helps building but that's only part of the issue - we've still got registration problems and if we were using the API in anger we would need things like clk_set_rate() to actually work. It's not just a case of simply enabling and disabling clocks (which can be stubbed out pretty effectively), audio generally needs to manage the clock rates too. signature.asc Description: Digital signature
Re: [PATCH 03/11] ASoC: fsl: add sgtl5000 clock support for imx-sgtl5000
On Wed, May 02, 2012 at 12:41:22PM +0100, Mark Brown wrote: > We generally try to keep the clock API out of CODEC drivers due to the > issues with the clock API that the generic implementation is intended to > help us solve. > > The major problem here is that we can't rely on having the clock API > present at all on most platforms so as soon as we start adding clock > support into things we have to do a performance with HAVE_CLK which is > at best annoying. > Is this something that the series below tries to solve? http://thread.gmane.org/gmane.linux.ports.arm.kernel/163652 Regards, Shawn > There's also the fact that even if the clock API is available we've no > way to register a clock consistently and since the audio clock tree > often needs management off-SoC for at least some components when the > clock API is in use you end up having to handle clock trees that go into > and out of the clock API. > > The upshot of this is that while we really should be using the clock API > for clocks the API as it stands is somewhat difficult to deploy in cross > platform code. What I'd suggest is connecting the clock to the CODEC in > the device tree but actually doing the clk_get() in the machine driver. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/11] ASoC: fsl: add sgtl5000 clock support for imx-sgtl5000
On Wed, May 02, 2012 at 06:50:24PM +0800, Richard Zhao wrote: > On Tue, May 01, 2012 at 09:44:48PM +0800, Shawn Guo wrote: > > > + data->codec_clk = clk_get(&codec_dev->dev, NULL); > > It's a clock of sgtl5000 codec. I feel it makes more sense to have > > sgtl5000 device driver than machine driver to manage this clock. > The ASoC machine driver handled the clock settings, I just followed the > same way, using snd_soc_dai_set_sysclk to set codec clk. But what you > said is reasonable. I also find it's impossible to use devm_get_clk. > Mark, do you suggest get clk in ASoC machine driver or in codec driver? We generally try to keep the clock API out of CODEC drivers due to the issues with the clock API that the generic implementation is intended to help us solve. The major problem here is that we can't rely on having the clock API present at all on most platforms so as soon as we start adding clock support into things we have to do a performance with HAVE_CLK which is at best annoying. There's also the fact that even if the clock API is available we've no way to register a clock consistently and since the audio clock tree often needs management off-SoC for at least some components when the clock API is in use you end up having to handle clock trees that go into and out of the clock API. The upshot of this is that while we really should be using the clock API for clocks the API as it stands is somewhat difficult to deploy in cross platform code. What I'd suggest is connecting the clock to the CODEC in the device tree but actually doing the clk_get() in the machine driver. signature.asc Description: Digital signature
Re: [PATCH 03/11] ASoC: fsl: add sgtl5000 clock support for imx-sgtl5000
On Tue, May 01, 2012 at 09:44:48PM +0800, Shawn Guo wrote:
> On Fri, Apr 27, 2012 at 03:02:57PM +0800, Richard Zhao wrote:
> > From: Richard Zhao
> >
> > It tries to clk_get the clock. And if it failed, it assumes the clock
> > by default enabled.
> >
> > Signed-off-by: Richard Zhao
> > ---
> > sound/soc/fsl/imx-sgtl5000.c | 40
> >
> > 1 files changed, 32 insertions(+), 8 deletions(-)
> >
> > diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c
> > index 73b935e..3a729ca 100644
> > --- a/sound/soc/fsl/imx-sgtl5000.c
> > +++ b/sound/soc/fsl/imx-sgtl5000.c
> > @@ -13,6 +13,8 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> > #include
> >
> > #include "../codecs/sgtl5000.h"
> > @@ -25,6 +27,7 @@ struct imx_sgtl5000_data {
> > struct snd_soc_card card;
> > char codec_dai_name[DAI_NAME_SIZE];
> > char platform_name[DAI_NAME_SIZE];
> > + struct clk *codec_clk;
> > unsigned int clk_frequency;
> > };
> >
> > @@ -58,6 +61,7 @@ static int __devinit imx_sgtl5000_probe(struct
> > platform_device *pdev)
> > struct device_node *np = pdev->dev.of_node;
> > struct device_node *ssi_np, *codec_np;
> > struct platform_device *ssi_pdev;
> > + struct i2c_client *codec_dev;
> > struct imx_sgtl5000_data *data;
> > int int_port, ext_port;
> > int ret;
> > @@ -113,6 +117,11 @@ static int __devinit imx_sgtl5000_probe(struct
> > platform_device *pdev)
> > ret = -EINVAL;
> > goto fail;
> > }
> > + codec_dev = of_find_i2c_device_by_node(codec_np);
> > + if (!codec_dev) {
> > + dev_err(&pdev->dev, "failed to find codec platform device\n");
> > + return -EINVAL;
> > + }
>
> What if the codec is accessed via SPI bus on some machines?
>
> >
> > data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > if (!data) {
> > @@ -120,11 +129,20 @@ static int __devinit imx_sgtl5000_probe(struct
> > platform_device *pdev)
> > goto fail;
> > }
> >
> > - ret = of_property_read_u32(codec_np, "clock-frequency",
> > - &data->clk_frequency);
> > - if (ret) {
> > - dev_err(&pdev->dev, "clock-frequency missing or invalid\n");
> > - goto fail;
> > + data->codec_clk = clk_get(&codec_dev->dev, NULL);
>
> It's a clock of sgtl5000 codec. I feel it makes more sense to have
> sgtl5000 device driver than machine driver to manage this clock.
The ASoC machine driver handled the clock settings, I just followed the
same way, using snd_soc_dai_set_sysclk to set codec clk. But what you
said is reasonable. I also find it's impossible to use devm_get_clk.
Mark, do you suggest get clk in ASoC machine driver or in codec driver?
Thanks
Richard
>
> Regards,
> Shawn
>
> > + if (IS_ERR(data->codec_clk)) {
> > + /* assuming clock enabled by default */
> > + data->codec_clk = NULL;
> > + ret = of_property_read_u32(codec_np, "clock-frequency",
> > + &data->clk_frequency);
> > + if (ret) {
> > + dev_err(&codec_dev->dev,
> > + "clock-frequency missing or invalid\n");
> > + goto fail;
> > + }
> > + } else {
> > + data->clk_frequency = clk_get_rate(data->codec_clk);
> > + clk_prepare_enable(data->codec_clk);
> > }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/11] ASoC: fsl: add sgtl5000 clock support for imx-sgtl5000
On Fri, Apr 27, 2012 at 03:02:57PM +0800, Richard Zhao wrote:
> From: Richard Zhao
>
> It tries to clk_get the clock. And if it failed, it assumes the clock
> by default enabled.
>
> Signed-off-by: Richard Zhao
> ---
> sound/soc/fsl/imx-sgtl5000.c | 40
> 1 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c
> index 73b935e..3a729ca 100644
> --- a/sound/soc/fsl/imx-sgtl5000.c
> +++ b/sound/soc/fsl/imx-sgtl5000.c
> @@ -13,6 +13,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
>
> #include "../codecs/sgtl5000.h"
> @@ -25,6 +27,7 @@ struct imx_sgtl5000_data {
> struct snd_soc_card card;
> char codec_dai_name[DAI_NAME_SIZE];
> char platform_name[DAI_NAME_SIZE];
> + struct clk *codec_clk;
> unsigned int clk_frequency;
> };
>
> @@ -58,6 +61,7 @@ static int __devinit imx_sgtl5000_probe(struct
> platform_device *pdev)
> struct device_node *np = pdev->dev.of_node;
> struct device_node *ssi_np, *codec_np;
> struct platform_device *ssi_pdev;
> + struct i2c_client *codec_dev;
> struct imx_sgtl5000_data *data;
> int int_port, ext_port;
> int ret;
> @@ -113,6 +117,11 @@ static int __devinit imx_sgtl5000_probe(struct
> platform_device *pdev)
> ret = -EINVAL;
> goto fail;
> }
> + codec_dev = of_find_i2c_device_by_node(codec_np);
> + if (!codec_dev) {
> + dev_err(&pdev->dev, "failed to find codec platform device\n");
> + return -EINVAL;
> + }
What if the codec is accessed via SPI bus on some machines?
>
> data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> if (!data) {
> @@ -120,11 +129,20 @@ static int __devinit imx_sgtl5000_probe(struct
> platform_device *pdev)
> goto fail;
> }
>
> - ret = of_property_read_u32(codec_np, "clock-frequency",
> -&data->clk_frequency);
> - if (ret) {
> - dev_err(&pdev->dev, "clock-frequency missing or invalid\n");
> - goto fail;
> + data->codec_clk = clk_get(&codec_dev->dev, NULL);
It's a clock of sgtl5000 codec. I feel it makes more sense to have
sgtl5000 device driver than machine driver to manage this clock.
Regards,
Shawn
> + if (IS_ERR(data->codec_clk)) {
> + /* assuming clock enabled by default */
> + data->codec_clk = NULL;
> + ret = of_property_read_u32(codec_np, "clock-frequency",
> + &data->clk_frequency);
> + if (ret) {
> + dev_err(&codec_dev->dev,
> + "clock-frequency missing or invalid\n");
> + goto fail;
> + }
> + } else {
> + data->clk_frequency = clk_get_rate(data->codec_clk);
> + clk_prepare_enable(data->codec_clk);
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/11] ASoC: fsl: add sgtl5000 clock support for imx-sgtl5000
On Fri, Apr 27, 2012 at 03:02:57PM +0800, Richard Zhao wrote: > From: Richard Zhao > > It tries to clk_get the clock. And if it failed, it assumes the clock > by default enabled. Applied, thanks. signature.asc Description: Digital signature
[PATCH 03/11] ASoC: fsl: add sgtl5000 clock support for imx-sgtl5000
From: Richard Zhao
It tries to clk_get the clock. And if it failed, it assumes the clock
by default enabled.
Signed-off-by: Richard Zhao
---
sound/soc/fsl/imx-sgtl5000.c | 40
1 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c
index 73b935e..3a729ca 100644
--- a/sound/soc/fsl/imx-sgtl5000.c
+++ b/sound/soc/fsl/imx-sgtl5000.c
@@ -13,6 +13,8 @@
#include
#include
#include
+#include
+#include
#include
#include "../codecs/sgtl5000.h"
@@ -25,6 +27,7 @@ struct imx_sgtl5000_data {
struct snd_soc_card card;
char codec_dai_name[DAI_NAME_SIZE];
char platform_name[DAI_NAME_SIZE];
+ struct clk *codec_clk;
unsigned int clk_frequency;
};
@@ -58,6 +61,7 @@ static int __devinit imx_sgtl5000_probe(struct
platform_device *pdev)
struct device_node *np = pdev->dev.of_node;
struct device_node *ssi_np, *codec_np;
struct platform_device *ssi_pdev;
+ struct i2c_client *codec_dev;
struct imx_sgtl5000_data *data;
int int_port, ext_port;
int ret;
@@ -113,6 +117,11 @@ static int __devinit imx_sgtl5000_probe(struct
platform_device *pdev)
ret = -EINVAL;
goto fail;
}
+ codec_dev = of_find_i2c_device_by_node(codec_np);
+ if (!codec_dev) {
+ dev_err(&pdev->dev, "failed to find codec platform device\n");
+ return -EINVAL;
+ }
data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
if (!data) {
@@ -120,11 +129,20 @@ static int __devinit imx_sgtl5000_probe(struct
platform_device *pdev)
goto fail;
}
- ret = of_property_read_u32(codec_np, "clock-frequency",
- &data->clk_frequency);
- if (ret) {
- dev_err(&pdev->dev, "clock-frequency missing or invalid\n");
- goto fail;
+ data->codec_clk = clk_get(&codec_dev->dev, NULL);
+ if (IS_ERR(data->codec_clk)) {
+ /* assuming clock enabled by default */
+ data->codec_clk = NULL;
+ ret = of_property_read_u32(codec_np, "clock-frequency",
+ &data->clk_frequency);
+ if (ret) {
+ dev_err(&codec_dev->dev,
+ "clock-frequency missing or invalid\n");
+ goto fail;
+ }
+ } else {
+ data->clk_frequency = clk_get_rate(data->codec_clk);
+ clk_prepare_enable(data->codec_clk);
}
data->dai.name = "HiFi";
@@ -140,10 +158,10 @@ static int __devinit imx_sgtl5000_probe(struct
platform_device *pdev)
data->card.dev = &pdev->dev;
ret = snd_soc_of_parse_card_name(&data->card, "model");
if (ret)
- goto fail;
+ goto clk_fail;
ret = snd_soc_of_parse_audio_routing(&data->card, "audio-routing");
if (ret)
- goto fail;
+ goto clk_fail;
data->card.num_links = 1;
data->card.dai_link = &data->dai;
data->card.dapm_widgets = imx_sgtl5000_dapm_widgets;
@@ -152,10 +170,12 @@ static int __devinit imx_sgtl5000_probe(struct
platform_device *pdev)
ret = snd_soc_register_card(&data->card);
if (ret) {
dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
- goto fail;
+ goto clk_fail;
}
platform_set_drvdata(pdev, data);
+clk_fail:
+ clk_put(data->codec_clk);
fail:
if (ssi_np)
of_node_put(ssi_np);
@@ -169,6 +189,10 @@ static int __devexit imx_sgtl5000_remove(struct
platform_device *pdev)
{
struct imx_sgtl5000_data *data = platform_get_drvdata(pdev);
+ if (data->codec_clk) {
+ clk_disable_unprepare(data->codec_clk);
+ clk_put(data->codec_clk);
+ }
snd_soc_unregister_card(&data->card);
return 0;
--
1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/11] ASoC: fsl: add sgtl5000 clock support for imx-sgtl5000
From: Richard Zhao
It tries to clk_get the clock. And if it failed, it assumes the clock
by default enabled.
Signed-off-by: Richard Zhao
---
sound/soc/fsl/imx-sgtl5000.c | 40
1 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c
index 73b935e..3a729ca 100644
--- a/sound/soc/fsl/imx-sgtl5000.c
+++ b/sound/soc/fsl/imx-sgtl5000.c
@@ -13,6 +13,8 @@
#include
#include
#include
+#include
+#include
#include
#include "../codecs/sgtl5000.h"
@@ -25,6 +27,7 @@ struct imx_sgtl5000_data {
struct snd_soc_card card;
char codec_dai_name[DAI_NAME_SIZE];
char platform_name[DAI_NAME_SIZE];
+ struct clk *codec_clk;
unsigned int clk_frequency;
};
@@ -58,6 +61,7 @@ static int __devinit imx_sgtl5000_probe(struct
platform_device *pdev)
struct device_node *np = pdev->dev.of_node;
struct device_node *ssi_np, *codec_np;
struct platform_device *ssi_pdev;
+ struct i2c_client *codec_dev;
struct imx_sgtl5000_data *data;
int int_port, ext_port;
int ret;
@@ -113,6 +117,11 @@ static int __devinit imx_sgtl5000_probe(struct
platform_device *pdev)
ret = -EINVAL;
goto fail;
}
+ codec_dev = of_find_i2c_device_by_node(codec_np);
+ if (!codec_dev) {
+ dev_err(&pdev->dev, "failed to find codec platform device\n");
+ return -EINVAL;
+ }
data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
if (!data) {
@@ -120,11 +129,20 @@ static int __devinit imx_sgtl5000_probe(struct
platform_device *pdev)
goto fail;
}
- ret = of_property_read_u32(codec_np, "clock-frequency",
- &data->clk_frequency);
- if (ret) {
- dev_err(&pdev->dev, "clock-frequency missing or invalid\n");
- goto fail;
+ data->codec_clk = clk_get(&codec_dev->dev, NULL);
+ if (IS_ERR(data->codec_clk)) {
+ /* assuming clock enabled by default */
+ data->codec_clk = NULL;
+ ret = of_property_read_u32(codec_np, "clock-frequency",
+ &data->clk_frequency);
+ if (ret) {
+ dev_err(&codec_dev->dev,
+ "clock-frequency missing or invalid\n");
+ goto fail;
+ }
+ } else {
+ data->clk_frequency = clk_get_rate(data->codec_clk);
+ clk_prepare_enable(data->codec_clk);
}
data->dai.name = "HiFi";
@@ -140,10 +158,10 @@ static int __devinit imx_sgtl5000_probe(struct
platform_device *pdev)
data->card.dev = &pdev->dev;
ret = snd_soc_of_parse_card_name(&data->card, "model");
if (ret)
- goto fail;
+ goto clk_fail;
ret = snd_soc_of_parse_audio_routing(&data->card, "audio-routing");
if (ret)
- goto fail;
+ goto clk_fail;
data->card.num_links = 1;
data->card.dai_link = &data->dai;
data->card.dapm_widgets = imx_sgtl5000_dapm_widgets;
@@ -152,10 +170,12 @@ static int __devinit imx_sgtl5000_probe(struct
platform_device *pdev)
ret = snd_soc_register_card(&data->card);
if (ret) {
dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
- goto fail;
+ goto clk_fail;
}
platform_set_drvdata(pdev, data);
+clk_fail:
+ clk_put(data->codec_clk);
fail:
if (ssi_np)
of_node_put(ssi_np);
@@ -169,6 +189,10 @@ static int __devexit imx_sgtl5000_remove(struct
platform_device *pdev)
{
struct imx_sgtl5000_data *data = platform_get_drvdata(pdev);
+ if (data->codec_clk) {
+ clk_disable_unprepare(data->codec_clk);
+ clk_put(data->codec_clk);
+ }
snd_soc_unregister_card(&data->card);
return 0;
--
1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
