Re: [PATCH 6/9] ARM: DaVinci: ASoC: Adds McASP clock support for TI DM646X processor

Re: [PATCH 6/9] ARM: DaVinci: ASoC: Adds McASP clock support for TI DM646X processor

[Medisetty, Naresh]

> On Mon, Mar 16, 2009 at 08:05:25AM -0400, Naresh Medisetty wrote:
>
> > +     CLK("soc-audio.0", NULL, &mcasp0_clk),
> > +     CLK("soc-audio.1", NULL, &mcasp1_clk),
>
> Assuming this is a shorthand for defining two platform devices for two
> ASoC cards I'd suggest that you instead move towards registering clocks
> for the DAIs and probing them as platform devices - take a look at how
> the PXA AC97 driver now does this for an example.  The soc-audio device
> will hopefully get removed at some point and you can't guarantee that
> the user won't want to put the two DAIs into a single sound card.

Yes, This is a shorthand for defining two platform devices for two
ASoC cards.

I will re-work on this and submit a patch

Re: [PATCH 6/9] ARM: DaVinci: ASoC: Adds McASP clock support for TI DM646X processor

[Kevin Hilman]

"Medisetty, Naresh" <naresh@ti.com> writes:

>> On Mon, Mar 16, 2009 at 08:05:25AM -0400, Naresh Medisetty wrote:
>>
>> > +     CLK("soc-audio.0", NULL, &mcasp0_clk),
>> > +     CLK("soc-audio.1", NULL, &mcasp1_clk),
>>
>> Assuming this is a shorthand for defining two platform devices for two
>> ASoC cards I'd suggest that you instead move towards registering clocks
>> for the DAIs and probing them as platform devices - take a look at how
>> the PXA AC97 driver now does this for an example.  The soc-audio device
>> will hopefully get removed at some point and you can't guarantee that
>> the user won't want to put the two DAIs into a single sound card.
>
> Yes, This is a shorthand for defining two platform devices for two
> ASoC cards.
>

Naresh,

When you move to platform device/data way of handling the clocks
(which I would prefer as well) then these clock definitions can 
use the physical clock name:


    CLK(NULL, "mcasp0" &mcasp0_clk),
    CLK(NULL, "mcasp1", &mcasp1_clk),

The platform code can do a clk_get(<physcial name>) and pass that
clk in with the platform data.

Kevin

Re: [PATCH 6/9] ARM: DaVinci: ASoC: Adds McASP clock support for TI DM646X processor

[Mark Brown]

On Thu, Mar 19, 2009 at 10:38:03AM -0700, Kevin Hilman wrote:

>     CLK(NULL, "mcasp0" &mcasp0_clk),
>     CLK(NULL, "mcasp1", &mcasp1_clk),

Exactly.

> The platform code can do a clk_get(<physcial name>) and pass that
> clk in with the platform data.

There should be no need to do this - the McASP DAI driver can just be a
regular platform device, probe as normal and register the DAI with the
ASoC core once it has done so.  This is, in fact, the preferred method
for doing this though support for htis is recent so only one CPU DAI
driver does it so far.

Re: [PATCH 6/9] ARM: DaVinci: ASoC: Adds McASP clock support for TI DM646X processor

[Kevin Hilman]

Mark Brown <broonie@sirena.org.uk> writes:

> On Thu, Mar 19, 2009 at 10:38:03AM -0700, Kevin Hilman wrote:
>
>>     CLK(NULL, "mcasp0" &mcasp0_clk),
>>     CLK(NULL, "mcasp1", &mcasp1_clk),
>
> Exactly.
>
>> The platform code can do a clk_get(<physcial name>) and pass that
>> clk in with the platform data.
>
> There should be no need to do this - the McASP DAI driver can just be a
> regular platform device, probe as normal and register the DAI with the
> ASoC core once it has done so.  This is, in fact, the preferred method
> for doing this though support for htis is recent so only one CPU DAI
> driver does it so far.

Sure, but what I don't want in that DAI driver is conditional code
because of different clock names on different chips.  For example,
some devices may only have one clk, so the hardware name is actualy
"mcasp" instead of "mcasp0" etc.

IOW, there will always be conditional code, I would prefer it not to
be in sound/soc/* and rather see it in the chip/board specific init
code.

Kevin

Re: [PATCH 6/9] ARM: DaVinci: ASoC: Adds McASP clock support for TI DM646X processor

[David Brownell]

On Thursday 19 March 2009, Kevin Hilman wrote:
> Sure, but what I don't want in that DAI driver is conditional code
> because of different clock names on different chips.  For example,
> some devices may only have one clk, so the hardware name is actualy
> "mcasp" instead of "mcasp0" etc.

Agreed that the clock setup should be in the arch/arm/mach-davinci
part of the tree ... but, for the record, I wouldn't find it at all
strange to see all such clocks named "mcasp0" instead of having some
boards use "mcasp" if there's only one.  :)

[PATCH 6/9] ARM: DaVinci: ASoC: Adds McASP clock support for TI DM646X processor

[Naresh Medisetty]

Adds McASP clock support for TI DM646X processor

Signed-off-by: Naresh Medisetty <naresh@ti.com>
---
This patch applies to the DaVinci git tree available at http://git.kernel.org/?p=linux/kernel/git/khilman/linux-davinci.git;a=commit;h=486afa37130356662213cc1a2199a285b4fd72af.

 arch/arm/mach-davinci/dm646x.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/dm646x.c b/arch/arm/mach-davinci/dm646x.c
index af040cf..0aee774 100644
--- a/arch/arm/mach-davinci/dm646x.c
+++ b/arch/arm/mach-davinci/dm646x.c
@@ -185,6 +185,18 @@ static struct clk gpio_clk = {
 	.lpsc = DM646X_LPSC_GPIO,
 };
 
+static struct clk mcasp0_clk = {
+	.name = "mcasp0",
+	.parent = &pll1_sysclk3,
+	.lpsc = DM646X_LPSC_McASP0,
+};
+
+static struct clk mcasp1_clk = {
+	.name = "mcasp0",
+	.parent = &pll1_sysclk3,
+	.lpsc = DM646X_LPSC_McASP1,
+};
+
 static struct clk aemif_clk = {
 	.name = "aemif",
 	.parent = &pll1_sysclk3,
@@ -253,6 +265,8 @@ struct davinci_clk dm646x_clks[] = {
 	CLK(NULL, "uart2", &uart2_clk),
 	CLK("i2c_davinci.1", NULL, &i2c_clk),
 	CLK(NULL, "gpio", &gpio_clk),
+	CLK("soc-audio.0", NULL, &mcasp0_clk),
+	CLK("soc-audio.1", NULL, &mcasp1_clk),
 	CLK(NULL, "aemif", &aemif_clk),
 	CLK("davinci_emac.1", NULL, &emac_clk),
 	CLK(NULL, "pwm0", &pwm0_clk),
-- 
1.5.6

Re: [PATCH 6/9] ARM: DaVinci: ASoC: Adds McASP clock support for TI DM646X processor

[Mark Brown]

On Mon, Mar 16, 2009 at 08:05:25AM -0400, Naresh Medisetty wrote:

> +	CLK("soc-audio.0", NULL, &mcasp0_clk),
> +	CLK("soc-audio.1", NULL, &mcasp1_clk),

Assuming this is a shorthand for defining two platform devices for two
ASoC cards I'd suggest that you instead move towards registering clocks
for the DAIs and probing them as platform devices - take a look at how
the PXA AC97 driver now does this for an example.  The soc-audio device
will hopefully get removed at some point and you can't guarantee that
the user won't want to put the two DAIs into a single sound card.