FS#11130 - Rewrite ascodec_as3514.c to use interrupts

Attached to Project: Rockbox
Opened by Tobias Diedrich (ranma) - Friday, 19 March 2010, 18:53 GMT
Last edited by Tobias Diedrich (ranma) - Tuesday, 23 March 2010, 09:06 GMT
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To Tobias Diedrich (ranma)
Operating System Sansa c200
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


This patch rewrites ascodec_as3514.c to use interrupts.
It gets rid of the ascodec mutex and instead introduces a mutex in adc-as3514.c.
Requests are handled using a simple request queue, and allows asynchronous request to be started from interrupt context.
The (20+sizeof(struct wakeup)) bytes request struct is either allocated on the stack (synchronous read/write) or has to be allocated statically by the caller (asynchronous read).

The asynchronous write is used in system-as3525.c (which is run with interrupts disabled).
asynchronous reads are used by another pending patch of mine implementing AUDIO_IRQ (int 9) handling for usb_plug detection without needing a thread.
This task depends upon

Closed by  Tobias Diedrich (ranma)
Tuesday, 23 March 2010, 09:06 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed
Comment by Rafaël Carré (funman) - Friday, 19 March 2010, 19:08 GMT
Good work!

You changed ascodec_lock() into a adc specific lock in adc-as3514.c, but this file is also used by e200v1/c200v1 : doesn't it cause a problem for those devices?
Comment by Tobias Diedrich (ranma) - Friday, 19 March 2010, 20:23 GMT
Good question. I think you're right. Updated patch attached.
Comment by Rafaël Carré (funman) - Saturday, 20 March 2010, 15:41 GMT
Tested on Clip+ : OK
Comment by Michael Chicoine (mc2739) - Saturday, 20 March 2010, 22:14 GMT
Tested on e260v2 with r25262:

When I attach the USB cable while in the Rockbox menu I intermittently get a black screen lockup that requires a long poweroff (10 secs) to recover.

All other functions appear normal.
Comment by Tobias Diedrich (ranma) - Sunday, 21 March 2010, 05:22 GMT
> Michael Chicoine (mc2739)
Only with this patch? Or with 11130+11131 applied?
Since 11130 doesn't change the usb detection I wonder why it should cause a lockup on usb plugin...
Hmm, backlight is controlled using as3514 on e200 though and system_reboot() calls _backlight_off() (why? If it reboots the backlight will be turned back on very soon anyway...). So if system_reboot() is called with interrupts disabled it would hang. Though it's should be called from the usb thread, which means interrupts should be enabled and I don't see why suddenly at that point the interrupt wouldn't fire if it has been working fine that far...
Comment by Tobias Diedrich (ranma) - Sunday, 21 March 2010, 07:36 GMT
> Michael Chicoine (mc2739)
Can you try this?
It will fall back to polling if irqs are disabled.
Comment by Michael Chicoine (mc2739) - Sunday, 21 March 2010, 14:19 GMT
I tested this patch by itself first, and then tested 11131 along with this patch. The lockup occurred in both cases.

So far, with minimal testing, the new patch looks good. I have not had any lockups, yet. I will continue testing throughout the day. I will also test it with 11131 also applied.
Comment by Michael Chicoine (mc2739) - Monday, 22 March 2010, 11:31 GMT
After further testing, I have not had any lockups with the latest patch. No other problems noted.