Re: [PATCH 03/11] ASoC: core: Add widget SND_SOC_DAPM_CLOCK_SUPPLY

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2142 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]