On 2016-05-11 17:29, Mark Brown wrote:
> On Tue, May 10, 2016 at 05:06:37PM +0200, Peter Rosin wrote:
>
>> + if (master) {
>> + switch (max9860->pclk_rate) {
>> + case 12000000:
>> + sysclk = MAX9860_FREQ_12MHZ;
>> + break;
>> + case 13000000:
>> + sysclk = MAX9860_FREQ_13MHZ;
>> + break;
>> + case 19200000:
>> + sysclk = MAX9860_FREQ_19_2MHZ;
>> + break;
>> + }
>
> What if we have another PCLK rate?
In that case the sysclk variable will remain cleared (0) and the
code that follows will trigger the PLL section with the N divider
for clock master mode (that mode is always used in clock slave mode).
>> +#ifdef CONFIG_PM
>> +static int max9860_suspend(struct device *dev)
>> +{
>> + struct max9860_priv *max9860 = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + ret = regmap_update_bits(max9860->regmap, MAX9860_SYSCLK,
>> + MAX9860_PSCLK, MAX9860_PSCLK_OFF);
>> + if (ret) {
>> + dev_err(dev, "Failed to disable clock: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int max9860_resume(struct device *dev)
>> +{
>> + struct max9860_priv *max9860 = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + regcache_cache_only(max9860->regmap, false);
>> + ret = regcache_sync(max9860->regmap);
>
> We didn't go into cache only mode on suspend? I'd also expect to see
> the regulators disabled over suspend and some system PM ops.
Ooops, that is a leftover, and I think it can be removed. However, your
comment suggests that I have misunderstood the workings of
SND_SOC_DAPM_REGULATOR_SUPPLY. I thought dapm would take care of the
regulators (and the clocks for SND_SOC_DAPM_CLOCK_SUPPLY) so that
disabling regulators over suspend was handled by the asoc core?
>> +static int max9860_mclk_rate(struct device *dev, unsigned long *mclk_rate)
>> +{
>> + struct clk *mclk = clk_get(dev, "mclk");
>
> Request resources on probe, not at some random point in driver
> execution. That will mean probe deferral works properly and that we
> don't get broken devices instantiated in userspace.
This function is only called during probe, but yes, it needs to
do probe deferral. I'll fix that for the next version.
>> + ret = clk_prepare_enable(mclk);
>> + if (ret) {
>> + dev_err(dev, "Failed to enable MCLK: %d\n", ret);
>> + clk_put(mclk);
>> + return ret;
>> + }
>> +
>> + *mclk_rate = clk_get_rate(mclk);
>> +
>> + clk_disable_unprepare(mclk);
>
> This is definitely confused too. Enabling the clock to read the
> programmed frequency is at best odd, and obviously if we do get the rate
> this will ensure that MCLK is disabled which probably isn't ideal.
This is the same situation as for the regulators, I thought dapm
handled it and would prep/enable clocks when they were needed?
>> +err_pm:
>> + pm_runtime_disable(dev);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(max9860_probe);
>
> I've no idea why this is exported...
Me neither. I'll kill that export for the next round.
I'll wait for further input on the regulator/clock interaction with dapm
before I send a v2.
Thanks,
Peter
