TPA6130A2 driver: kernel panic after unload; inability to re-load #10
Comments
|
On a related note, the chip here is actually a TPA6130A2, not a TPA613A2. The former supports I2C, while the latter does not 🙂 |
|
Just a general question: what do we expect to happen when we unload a driver? Should removing the module uninitialize the physical hardware? Probably not, but just something to think about as I don't have a good feel for what is typical. |
|
I think at a minimum it should mute the audio channels. For power
considerations, maybe set the sample rate to 48 KHz if it was running
faster, say 192 KHz for example.
Ross
…On Wed, Feb 21, 2024 at 9:25 AM Trevor Vannoy ***@***.***> wrote:
Just a general question: what do we expect to happen when we unload a
driver? Should removing the module uninitialize the physical hardware?
Probably not, but just something to think about as I don't have a good feel
for what is typical.
—
Reply to this email directly, view it on GitHub
<#10 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABNDUXAYOKN3ZG4IEIVLAQDYUYNZPAVCNFSM6AAAAABDSGF54GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJXGE4TGNZXGI>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
Muting (and otherwise resetting) the hardware on unload makes sense, but I could also see situations where that'd be undesirable. In particular, this would force us to have the HPS running in order to use the system; shutting down would unload the modules and reset the hardware. Personally, I think it'd be neat if we at least had the option to shut down the HPS and leave the remaining audio hardware fully functional. This is absolutely something we can handle later, though, or not at all. If we think the option to skip resetting hardware would be useful, I think the standard way to implement that would be as a kernel module parameter (resource, resource), which could default to "reset" but allow the user to specify "no reset" if they so desire. Using |
|
Anytime there are significant changes in an audio path, it should be muted
to avoid pops and potentially loud volume changes (i.e. hearing safety). I
would argue that unloading a driver is such a case. If we are not using
the HPS (i.e. a pure fabric play) then one would have a dedicated SPI and
I2C controller in the fabric and you would still mute when
connections/reconnects happen. The point of using SoC FPGAs is to
partition the application into the hardware and software parts that best
map to the HPS or fabric. Thus in terms of the book, the underlying
assumption is to always use both the HPS and fabric. Thus, while one could
shut down the HPS, I'm not interested in that option since we have it and
should be using it.
I'm not envisioning a case where you would unload the ad1939.ko driver and
still want audio running.
…On Thu, Feb 22, 2024 at 3:05 PM Lucas Ritzdorf ***@***.***> wrote:
Muting (and otherwise resetting) the hardware on unload makes sense, but I
could also see situations where that'd be undesirable. In particular, this
would force us to have the HPS running in order to use the system; shutting
down would unload the modules and reset the hardware.
Personally, I think it'd be neat if we at least had the option to shut
down the HPS and leave the remaining audio hardware fully functional. This
is absolutely something we can handle later, though, or not at all.
If we think the option to skip resetting hardware would be useful, I think
the standard way to implement that would be as a kernel module parameter (
resource
<https://girishjoshi.io/post/create-linux-kernel-module-with-parameters/>,
resource
<https://devarea.com/linux-kernel-development-kernel-module-parameters/>),
which could default to "reset" but allow the user to specify "no reset" if
they so desire. Using insmod, for example, that would look something like insmod
ad1939.ko unload_reset=false.
—
Reply to this email directly, view it on GitHub
<#10 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABNDUXAZXSDGACKYV2BGLATYU66JVAVCNFSM6AAAAABDSGF54GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQGM4TGNZTGU>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Let's not complicate things for the sake of neat or nice-to-have-but-unnecessary feature creep. The goal should be making the code both functional and easy to learn from. Unnecessary complexity in the code is likely to result in extra complexity during the learning process. Keep it simple (but not the Arch Linux meaning of "simple"). |
|
Good point! Also, if the textbook assumes constant HW/SW interoperation in order for the system to properly run, that makes a lot more sense. |
|
Currently working on a very basic driver which integrates properly with the device tree, based on Trevor's work with the AD1939. Once that's done, I can open a separate PR to handle mute-on-unload functionality, if desired. |
This issue is mostly for "housekeeping" purposes, so that the problem described here is known about.
Issues with the existing TPA6130A2 headphone amplifier driver are two-fold:
If someone (@tvannoy or myself, time allowing) are up for the task, this is probably best resolved by rewriting the TPA6130A2 driver in the style of Trevor's recent AD1939 update, since the latter a) does not exhibit this issue, and b) is much cleaner overall. No particular pressure at the moment, though!
The text was updated successfully, but these errors were encountered: