| Message ID | 1247521841-21358-4-git-send-email-khilman@deeprootsystems.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Mark Brown <broonie@opensource.wolfsonmicro.com> writes: > On Mon, Jul 13, 2009 at 02:50:41PM -0700, Kevin Hilman wrote: > >> - dev->clk = clk_get(&pdev->dev, pdata->clk_name); >> + dev->clk = pdata->clk; > > I've applied the first two but as I said in response to your previous > mail this doesn't seem like the way the clock API should be used. I can > apply this if required but it'd seem better to fix things up to use the > clock API abstraction here to cut down on the amount of cross tree > issues in the future if possible. There are two solutions to this. 1) this proposal 2) go back to the original one where we used: dev->clk = clk_get(&pdev->dev, NULL); and simply rely on the fact that the platform setup code sets up the clkdev nodes to have the "soc-audio" string in them instead of the physical clock names. This was how it currently is in mainline, but when the dm6467 changes were merged the change was made to pass clock names in platform data, which is not acceptable on the ARM upstream side. Personally, I prefer (2), but was under the impression that it was not liked on the ASoC side. I'll gladly switch things back to using (2) if that's OK on ASoC side. Kevin
Mark Brown <broonie@opensource.wolfsonmicro.com> writes: > On Mon, Jul 13, 2009 at 03:12:09PM -0700, Kevin Hilman wrote: > >> There are two solutions to this. > > No, there's at least three. > >> 1) this proposal > >> 2) go back to the original one where we used: > >> dev->clk = clk_get(&pdev->dev, NULL); > >> and simply rely on the fact that the platform setup code sets up the >> clkdev nodes to have the "soc-audio" string in them instead of the >> physical clock names. > > The other option is that the clocks are associated with the DAI device > for the platform device for the DAI and the clk_get() is done on the DAI > platform device with a fixed clock name. The ASoC code is almost all of > the way there, it's already got the struct device right and just needs > the clock name making fixed. Do you have these queued up someplace? or can you describe in mor detail where the clk_get should be placed? Being somewhat ignorant of ASoC internals, I'm not sure what you mean by this proposal. I don't see any DAI setup done in platform code. >> This was how it currently is in mainline, but when the dm6467 changes >> were merged the change was made to pass clock names in platform data, >> which is not acceptable on the ARM upstream side. > > My impression from looking at the patches had been that this was just > how the DaVinci architecture code operated - the clock additions in the > patches certainly didn't look out of place and there weren't any review > comments from the architecture side. Indeed, the lack of review was my fault. I let these ones through without arguing my original opinions strong enough. >> Personally, I prefer (2), but was under the impression that it was not >> liked on the ASoC side. > > It's not liked on the ASoC side because we have a perfectly good device > model way of getting the clock to the device so we can just use the > clock API as normal. Nothing should be using the soc-audio device to > probe - doing so in the DaVinci drivers would be a regression. I'm not sure what you mean by "using the clock API as normal." grepping in sound/soc for clk_get, all the users of clk_get have hard-coded clock names, which is exactly what we do not want for DaVinci. The problem we have on DaVinci is trying to use the same driver across multiple SoCs that may have different clock names. Or have multiple instances of the driver with different clock names on the same SoC. The platform init at (usually the board file) has to decide which clock to use and setup platform_data and/or clkdev nodes accordingly, and this is usually before the 'struct device' Kevin
On Monday 13 July 2009, Kevin Hilman wrote: > grepping in sound/soc for clk_get, all the users of clk_get have > hard-coded clock names, which is exactly what we do not want for > DaVinci. > > The problem we have on DaVinci is trying to use the same driver across > multiple SoCs that may have different clock names. Â Or have multiple > instances of the driver with different clock names on the same SoC. I've lost something here ... c = clk_get(device, "logical_name"); should work regardless of the physical name used by the clock(s) on a given platform, yes? The last time I looked at ASoc, the problem I recall was that the device wasn't a standard platform device; it had to be an ASoC-created thing. And that's where the problems came from: there was no hook make sure the lookup above returned the right clock, since the device was unknown to the clock tree. Either that should help clarify something, or else it should sow more confusion. I hope the former. ;)
Mark Brown <broonie@opensource.wolfsonmicro.com> writes: > On Mon, Jul 13, 2009 at 04:10:21PM -0700, Kevin Hilman wrote: >> Mark Brown <broonie@opensource.wolfsonmicro.com> writes: > >> > The other option is that the clocks are associated with the DAI device >> > for the platform device for the DAI and the clk_get() is done on the DAI >> > platform device with a fixed clock name. The ASoC code is almost all of >> > the way there, it's already got the struct device right and just needs >> > the clock name making fixed. > >> Do you have these queued up someplace? or can you describe in mor >> detail where the clk_get should be placed? > > git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git for-2.6.32 > > which should also be turning up in -next ATM. The driver should look > like a normal platform device driver, doing resource grabbing on probe() > and then registering with the ASoC core once that's happened. > >> Being somewhat ignorant of ASoC internals, I'm not sure what you mean >> by this proposal. I don't see any DAI setup done in platform code. > > There *should* be devices with names "davinci-asp" and "davinci-mcasp" > being set up. If there aren't the audio will never come up. Somehow I missed seeing these. Using these devices, we can simply rely on the 'struct device' and have no need to pass in the clock names. The arch/arm/mach-davinci/* code can setup the clkdev nodes with the "davinci-asp" and "davinci-mcasp" names and then there would be no need to pass the clock names. Patches coming soon... Kevin
diff --git a/sound/soc/davinci/davinci-i2s.c b/sound/soc/davinci/davinci-i2s.c index f7330e1..ddc047e 100644 --- a/sound/soc/davinci/davinci-i2s.c +++ b/sound/soc/davinci/davinci-i2s.c @@ -528,7 +528,7 @@ static int davinci_i2s_probe(struct platform_device *pdev) goto err_release_region; } - dev->clk = clk_get(&pdev->dev, pdata->clk_name); + dev->clk = pdata->clk; if (IS_ERR(dev->clk)) { ret = -ENODEV; goto err_free_mem; diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index b27aab6..3b095f9 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -764,7 +764,7 @@ static int davinci_mcasp_probe(struct platform_device *pdev) } pdata = pdev->dev.platform_data; - dev->clk = clk_get(&pdev->dev, pdata->clk_name); + dev->clk = pdata->clk; if (IS_ERR(dev->clk)) { ret = -ENODEV; goto err_release_region;
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> --- sound/soc/davinci/davinci-i2s.c | 2 +- sound/soc/davinci/davinci-mcasp.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)