[alsa-devel] [PATCH RFC 3/3] uda1380: make driver more powersave-friendly

[Vasily Khoruzhick]

В сообщении от 26 июня 2010 23:45:12 автор Mark Brown написал:
> Please remember to CC Liam on ASoC patches.

Ok, I'll do
> 
> > +	for (reg = UDA1380_MVOL; reg < UDA1380_CACHEREGNUM; reg++)
> > +		set_bit(reg - 0x10, &uda1380_cache_dirty);
> 
> This seems odd, I'd expect the cache to be being marked as clean
> immediately after sync?

Nope, it is not. Only i2c and clock related regs of uda1380 can be modified 
when there's no i2s clock, i.e. mixer regs should be updated right after i2s 
clock was enabled, so we marking mixer-related regs cache as dirty, to make 
sure they'll be updated when possible.
 
> > 		uda1380_write(codec, UDA1380_PM, R02_PON_BIAS | pm);
> 
> Like I said previously you really should look at using DAPM here, this
> should make the code simpler and will let you
> 
> You might also want to consider snd_soc_update_bits().

Hmm, uda134x driver does pretty same things as in my patch... And it seems 
part of your sentence is lost :(
 
> > 		break;
> > 	
> > 	case SND_SOC_BIAS_STANDBY:
> > -		uda1380_write(codec, UDA1380_PM, R02_PON_BIAS);
> > +		if (codec->bias_level == SND_SOC_BIAS_OFF) {
> > +			pdata->set_power(1);
> > +			uda1380_reset(codec);
> 
> The reset here seems unneeded and possibly wasteful if there is no power
> control on the system. It'd seem better to do something like just power up,
> flagging the cache as dirty if there was a callback. I'd strongly expect
> that if you are actually controlling power the device will be in the
> default state anyway.

Ok

> > +			switch (pdata->dac_clk) {
> > +			case UDA1380_DAC_CLK_SYSCLK:
> > +				uda1380_write_reg_cache(codec, UDA1380_CLK, 0);
> > +				break;
> > +			case UDA1380_DAC_CLK_WSPLL:
> > +				uda1380_write_reg_cache(codec, UDA1380_CLK,
> > +					R00_DAC_CLK);
> > +				break;
> > +			}
> 
> Why is this being managed every time the device is enabled? Surely the
> setting can be done once at startup.

Yep, just need to mark cache of this reg as dirty here...
 
> > -	ret = uda1380_reset(codec);
> > -	if (ret < 0) {
> > -		dev_err(codec->dev, "Failed to issue reset\n");
> > -		goto err_reset;
> > -	}
> > -
> 
> The reason for the reset at startup is that we don't know what state the
> device is in when Linux gets control.

It's softreset and it's performed by writing some value to some reg. i2c xfers 
is not possible when codec is not enabled (it is not at this point)

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20100627/e153d408/attachment-0001.sig