On Wed, May 09, 2012 at 09:55:54AM +0200, Ola Lilja wrote:
On 05/08/2012 05:40 PM, Mark Brown wrote:
On Tue, May 08, 2012 at 03:56:40PM +0200, Ola Lilja wrote:
- if (SND_SOC_DAPM_EVENT_ON(event))
return clk_enable(w->priv);
- else {
clk_disable(w->priv);
return 0;
Coding style - you need more { }. This also all needs to be conditionally complied for the many platforms that don't provide the clock API.
if (clk_enable) return clk_enable(w->priv); else return <some error>;
?
That's not going to help with platforms that don't provide the clock API if that's what you're asking. clk_enable() just won't be a symbol.
- case snd_soc_dapm_clock_supply:
w->priv = (w->shift) ? clk_get_sys(w->name, NULL) :
clk_get(dapm->dev, w->name);
I don't think supporting clk_get_sys() is a particularly good idea here... Also, I think Liam was wanting to add per-user data to the widget rather than reuse the priv pointer.
OK, so how can I solve the fact that we have one clock that is gotten with clk_get_sys()?
You should be using clkdev to map the clock to the device.
The priv-pointer was reused for the delay in the regulator-supply, so I assumed that it was ok here as well. How should I do it so that it is OK with you, then?
Add a struct clk * to the widget.
+int snd_soc_dapm_get_power_status(struct snd_soc_dapm_context *dapm,
const char *pin)
This appears to be *nothing* to do with the rest of the patch! It's also not clear what it's for.
It is for the debugging in our driver, using it to get the status of our clocks so that we can make sure that the correct clocks are enabled/disabled at certain points that we find useful. I named it get_power_status instead of get_clock_status, just because I thought you would complain that get_clock_status was using the ->power, which is not specific to just clock-widgets.
As discussed elsewhere this seems like a bad idea, but even if it's a good idea this should have been split out - it's not a part of introducing a new widget type.