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

[Ola Lilja <ola.o.lilja@stericsson.com>]

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>;

?

> 
> 
>> +	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()?
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?

> 
>> + /**
>> + * snd_soc_dapm_get_power_status - get widget power status
>> + * @dapm: DAPM context
>> + * @pin: Widget name
>> + *
>> + * Get widget power status - Enabled or disabled.
>> + *
>> + * Returns -1 for failure.
>> + * Returns 0 if disabled.
>> + * Returns 1 if enabled.
>> + */
>> +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.