[alsa-devel] asoc: s3c24xx+uda1380 - some questions
Hi, I'm trying to develop asoc driver for hp ipaq rx1950 (s3c24xx + uda1380).
1st question: where I can enable/disable uda1380 codec power? I need to control codec power for powersave reasons.
I tried to add custom power function call to uda1380_set_bias_level in uda1380.c (pointer to power function is stored in codec->private_data), as it done for uda134x codec, but after enable-disable-enable cycle (for example, after resuming from suspend) driver cannot communicate with codec anymore (i2c bus is always busy), only reloading i2c driver helps.
I suspect, power(0) is called in inappropriate moment, when i2c-transfer still is not finished.
2nd question: for some reason, driver does not produce any sound during first aplay invocation, only second and later aplay invocation driver produces sound. What can be the reason of this problem?
Regards Vasily
On Tue, Jan 27, 2009 at 03:19:39PM +0200, Vasily Khoruzhick wrote:
I'm trying to develop asoc driver for hp ipaq rx1950 (s3c24xx + uda1380).
1st question: where I can enable/disable uda1380 codec power? I need to control codec power for powersave reasons.
CCing in Philipp Zabel who wrote the UDA1380 driver. Assuming you mean control to cut the power to the chip the most standard way of doing it would be with the regulator API but if the chip isn't being used much in new designs it might not be worth it.
I tried to add custom power function call to uda1380_set_bias_level in uda1380.c (pointer to power function is stored in codec->private_data), as it done for uda134x codec, but after enable-disable-enable cycle (for example, after resuming from suspend) driver cannot communicate with codec anymore (i2c bus is always busy), only reloading i2c driver helps.
The UDA134x is a bit of a special case since it needs the machine driver to provide bitbanging functions for the L3 interface.
2nd question: for some reason, driver does not produce any sound during first aplay invocation, only second and later aplay invocation driver produces sound. What can be the reason of this problem?
That sounds like something in the teardown path should be done on startup as well. Can't think of any obvious gotchas there, though I'd be looking at the codec and machine drivers since the CPU driver for the S3C24xx is used in quite a few designs.
On Tuesday 27 January 2009 17:00:41 Mark Brown wrote:
CCing in Philipp Zabel who wrote the UDA1380 driver. Assuming you mean control to cut the power to the chip the most standard way of doing it would be with the regulator API but if the chip isn't being used much in new designs it might not be worth it.
Uh-oh, I didn't thought that it will be so complicated to decide when turn one gpio line to 0 or 1 :) Btw, I'm thinking about using own write- and read- wrappers for uda1380, in this way I can check on each read/write whether codec power is enabled.
That sounds like something in the teardown path should be done on startup as well. Can't think of any obvious gotchas there, though I'd be looking at the codec and machine drivers since the CPU driver for the S3C24xx is used in quite a few designs.
Sorry, but what is "teardown path"?
Regards Vasily
On Tue, Jan 27, 2009 at 05:19:53PM +0200, Vasily Khoruzhick wrote:
Uh-oh, I didn't thought that it will be so complicated to decide when turn one gpio line to 0 or 1 :) Btw, I'm thinking about using own write- and read- wrappers for uda1380, in this way I can check on each read/write whether codec power is enabled.
Are you really cutting all power to the codec or just the analogue supplies? Normally the digital is powered off a separate supply shared with other bits of the system.
That sounds like something in the teardown path should be done on startup as well. Can't think of any obvious gotchas there, though I'd be looking at the codec and machine drivers since the CPU driver for the S3C24xx is used in quite a few designs.
Sorry, but what is "teardown path"?
When playback is stopped.
On Tuesday 27 January 2009 17:25:57 Mark Brown wrote:
On Tue, Jan 27, 2009 at 05:19:53PM +0200, Vasily Khoruzhick wrote:
Uh-oh, I didn't thought that it will be so complicated to decide when turn one gpio line to 0 or 1 :) Btw, I'm thinking about using own write- and read- wrappers for uda1380, in this way I can check on each read/write whether codec power is enabled.
Are you really cutting all power to the codec or just the analogue supplies? Normally the digital is powered off a separate supply shared with other bits of the system.
Yep, I'm really cutting all power to the codec. AFAIR it saves ~10-15mA of current consumption.
Regards Vasily
On Tue, Jan 27, 2009 at 4:49 PM, Vasily Khoruzhick anarsoul@gmail.com wrote:
On Tuesday 27 January 2009 17:25:57 Mark Brown wrote:
On Tue, Jan 27, 2009 at 05:19:53PM +0200, Vasily Khoruzhick wrote:
Uh-oh, I didn't thought that it will be so complicated to decide when turn one gpio line to 0 or 1 :) Btw, I'm thinking about using own write- and read- wrappers for uda1380, in this way I can check on each read/write whether codec power is enabled.
Are you really cutting all power to the codec or just the analogue supplies? Normally the digital is powered off a separate supply shared with other bits of the system.
Yep, I'm really cutting all power to the codec. AFAIR it saves ~10-15mA of current consumption.
Could you show the code? On magician I'm just powering up uda1380 when the driver is loaded, but I wouldn't mind saving some power :) There are two GPIOs involved - one connected to the uda1380's RESET line and one to control the power. The last one doesn't have anything to do with uda1380, really. It's an external power switch in a PMIC (or CPLD, on magican) so it would be nice if this code could live in the machine specific drivers.
regards Philipp
On Tuesday 27 January 2009 18:06:47 pHilipp Zabel wrote:
Could you show the code? On magician I'm just powering up uda1380 when the driver is loaded, but I wouldn't mind saving some power :) There are two GPIOs involved - one connected to the uda1380's RESET line and one to control the power. The last one doesn't have anything to do with uda1380, really. It's an external power switch in a PMIC (or CPLD, on magican) so it would be nice if this code could live in the machine specific drivers.
regards Philipp
Yep, of course, here's machine driver (originally created by Denis Grigoriev) and modified codec driver.
Btw, on rx1950 there're 4 gpio pins involved: GPJ0 controls codec power GPA1 controls amplifier power GPG12 is jack sense pin, it's value depends whether headphone jack is inserted or not, and GPD0 is connected to uda1380 reset.
On Tue, Jan 27, 2009 at 5:22 PM, Vasily Khoruzhick anarsoul@gmail.com wrote:
On Tuesday 27 January 2009 18:06:47 pHilipp Zabel wrote:
Could you show the code? On magician I'm just powering up uda1380 when the driver is loaded, but I wouldn't mind saving some power :) There are two GPIOs involved - one connected to the uda1380's RESET line and one to control the power. The last one doesn't have anything to do with uda1380, really. It's an external power switch in a PMIC (or CPLD, on magican) so it would be nice if this code could live in the machine specific drivers.
regards Philipp
Yep, of course, here's machine driver (originally created by Denis Grigoriev) and modified codec driver.
Btw, on rx1950 there're 4 gpio pins involved: GPJ0 controls codec power GPA1 controls amplifier power GPG12 is jack sense pin, it's value depends whether headphone jack is inserted or not, and GPD0 is connected to uda1380 reset.
The uda1380 data sheet says that the reset time should be at least 1 µs - could that be a problem?
regards Philipp
On Tuesday 27 January 2009 22:32:11 Vasily Khoruzhick wrote:
On 27 January 2009 21:49:01 pHilipp Zabel wrote:
The uda1380 data sheet says that the reset time should be at least 1 µs - could that be a problem?
regards Philipp
Nope, added mdelay(10) in reset sequence, but it didn't help.
Regards Vasily
Here's patch that fixes 2nd bug I've mentioned (there's a sound only on second and later aplay). It's bug in uda1380 driver (probably, just a typo), driver switches to WSPLL in uda1380_pcm_prepare even if SYSCLK was chosen (uda1380_pcm_prepare modifies UDA1380_CLK register before flushing reg cache, but doesn't restore its value later)
One more question: it seems that my rx1950 clocked in a way that I can't get precise divisor for 44100 and 22050 rates, but uda1380 driver propose them (look UDA1380_RATES define and struct snd_soc_dai uda1380_dai[]. How to exclude all rates except 16000 and 48000? Should I declare my own snd_soc_dai and copy necessary members from uda1380's one?
Regards Vasily
On Tue, Feb 03, 2009 at 01:46:32AM +0200, Vasily Khoruzhick wrote:
and later aplay). It's bug in uda1380 driver (probably, just a typo), driver switches to WSPLL in uda1380_pcm_prepare even if SYSCLK was chosen (uda1380_pcm_prepare modifies UDA1380_CLK register before flushing reg cache, but doesn't restore its value later)
Your explanation here makes sense but...
One more question: it seems that my rx1950 clocked in a way that I can't get precise divisor for 44100 and 22050 rates, but uda1380 driver propose them (look UDA1380_RATES define and struct snd_soc_dai uda1380_dai[]. How to exclude all rates except 16000 and 48000? Should I declare my own snd_soc_dai and copy necessary members from uda1380's one?
Set up additional constraints in your machine driver - see how drivers like wm8903 enforce symmetric configurations for playback and record for an example.
/* FIXME enable DAC_CLK */
- uda1380_write(codec, UDA1380_CLK, clk | R00_DAC_CLK);
- uda1380_write(codec, UDA1380_CLK, clk);
..are you sure this fix won't break existing users? Based on your explanation above (which should *really* go into the commit) I'd expect this to be conditional on something. It looks like what you really want to do here is clean up the FIXMEs :)
On Tuesday 03 February 2009 13:41:37 Mark Brown wrote:
One more question: it seems that my rx1950 clocked in a way that I can't get precise divisor for 44100 and 22050 rates, but uda1380 driver propose them (look UDA1380_RATES define and struct snd_soc_dai uda1380_dai[]. How to exclude all rates except 16000 and 48000? Should I declare my own snd_soc_dai and copy necessary members from uda1380's one?
Set up additional constraints in your machine driver - see how drivers like wm8903 enforce symmetric configurations for playback and record for an example.
Cool, thanks :)
/* FIXME enable DAC_CLK */
- uda1380_write(codec, UDA1380_CLK, clk | R00_DAC_CLK);
- uda1380_write(codec, UDA1380_CLK, clk);
..are you sure this fix won't break existing users? Based on your explanation above (which should *really* go into the commit) I'd expect this to be conditional on something. It looks like what you really want to do here is clean up the FIXMEs :)
Yep, I'm pretty sure. It will restore WSPLL bit if it was set before. It only changes behavior of driver if SYSCLK was chosen. It seems that FIXMEs can be removed, I can resubmit patch if you want.
Btw, Philipp, what do you think about it?
Regards Vasily
On Tue, Feb 03, 2009 at 01:57:12PM +0200, Vasily Khoruzhick wrote:
Yep, I'm pretty sure. It will restore WSPLL bit if it was set before. It only changes behavior of driver if SYSCLK was chosen. It seems that FIXMEs can be removed, I can resubmit patch if you want.
Please do. You should at least include some text in the patch description explaining what you're doing.
On Tuesday 03 February 2009 14:05:08 Mark Brown wrote:
Please do. You should at least include some text in the patch description explaining what you're doing.
One more version of patch, I've fixed patch description, but didn't removed FIXME's, just fixed one comment.
Btw, it would be nice to hear Philipp's opinion :)
Regards Vasily
2009/2/3 Vasily Khoruzhick anarsoul@gmail.com:
On Tuesday 03 February 2009 14:05:08 Mark Brown wrote:
Please do. You should at least include some text in the patch description explaining what you're doing.
One more version of patch, I've fixed patch description, but didn't removed FIXME's, just fixed one comment.
Btw, it would be nice to hear Philipp's opinion :)
Yes, that patch is correct - thanks!
The FIXMEs are there because on magician flushing the interpolator registers only seems to succeed when it is clocked by SYSCLK (see also the comment in uda1380_mute). I think this might be because at the time I flush the registers (in uda1380_pcm_prepare), the SSP link is not yet running.
regards Philipp
On Tue, Jan 27, 2009 at 06:22:25PM +0200, Vasily Khoruzhick wrote:
Btw, on rx1950 there're 4 gpio pins involved: GPJ0 controls codec power GPA1 controls amplifier power GPG12 is jack sense pin, it's value depends whether headphone jack is inserted
You might want to have a look at the jack reporting API that was recently added, it should save you a bit of code:
http://git.kernel.org/?p=linux/kernel/git/broonie/sound-2.6.git;a=commit;h=8...
It'll need a little helper for GPIOs adding to the core but that should be reusable.
down(&rx1950_power_mutex); printk("%s: enable == %d\n", __func__, enable); if (enable) { if (s3c2410_gpio_getpin(S3C2440_GPJ0)) goto done; //spin_lock(&cmpl_lock); //waiting_for_completion = 1; //spin_unlock(&cmpl_lock); s3c2410_gpio_setpin(S3C2410_GPD0, 0); s3c2410_gpio_setpin(S3C2440_GPJ0, 0); s3c2410_gpio_setpin(S3C2440_GPJ0, 1);
/* Wait for EINT20 irq to ensure uda1380 is powered */ //printk("%s: waiting for compeltion...\n", __func__); //wait_for_completion(&rx1950_sound_completion); //printk("%s: completed\n", __func__); //printk("%s: GPG12: %d\n", __func__, // s3c2410_gpio_getpin(S3C2410_GPG12)); mdelay(50); s3c2410_gpio_setpin(S3C2410_GPD0, 1); s3c2410_gpio_setpin(S3C2410_GPD0, 0);
Hrm. The completion makes this look more like it could use a full blown regulator - it's certainly a pattern that will get reused often enough.
/* configure some gpios */ s3c2410_gpio_cfgpin(S3C2410_GPD0, S3C2410_GPIO_OUTPUT); s3c2410_gpio_cfgpin(S3C2410_GPG12, S3C2410_GPIO_IRQ); s3c2410_gpio_cfgpin(S3C2440_GPJ0, S3C2410_GPIO_OUTPUT);
The S3C24xx now supports gpiolib so you should be able to use that - the platform specific GPIO API will probably get killed at some point.
On 27 January 2009 22:00:52 Mark Brown wrote:
On Tue, Jan 27, 2009 at 06:22:25PM +0200, Vasily Khoruzhick wrote:
Btw, on rx1950 there're 4 gpio pins involved: GPJ0 controls codec power GPA1 controls amplifier power GPG12 is jack sense pin, it's value depends whether headphone jack is inserted
You might want to have a look at the jack reporting API that was recently added, it should save you a bit of code:
http://git.kernel.org/?p=linux/kernel/git/broonie/sound-2.6.git;a=commit;h =8a2cd6180f8fa00111843c2f4a4f4361995358e0
It'll need a little helper for GPIOs adding to the core but that should be reusable.
Ok, thanks for advice
Hrm. The completion makes this look more like it could use a full blown regulator - it's certainly a pattern that will get reused often enough.
It's commented out, I don't use completions anymore.
/* configure some gpios */ s3c2410_gpio_cfgpin(S3C2410_GPD0, S3C2410_GPIO_OUTPUT); s3c2410_gpio_cfgpin(S3C2410_GPG12, S3C2410_GPIO_IRQ); s3c2410_gpio_cfgpin(S3C2440_GPJ0, S3C2410_GPIO_OUTPUT);
The S3C24xx now supports gpiolib so you should be able to use that - the platform specific GPIO API will probably get killed at some point.
Yep, but s3c24xx gpiolib doesn't support all gpio pins, especially port J :)
Regards Vasily
On Tue, Jan 27, 2009 at 10:14:36PM +0200, Vasily Khoruzhick wrote:
On 27 January 2009 22:00:52 Mark Brown wrote:
Hrm. The completion makes this look more like it could use a full blown regulator - it's certainly a pattern that will get reused often enough.
It's commented out, I don't use completions anymore.
Yes, I assumed that was just temporary while you were working on this though.
participants (3)
-
Mark Brown -
pHilipp Zabel -
Vasily Khoruzhick