TPA6130A2 driver: kernel panic after unload; inability to re-load

[LRitzdorf]

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:

• The driver cannot be unloaded and re-loaded into the kernel. Relevant kernel messages (generated upon re-registration) are:

  Failed to register I2C driver
  Error: Driver 'Audio Logic tpa613a2 Driver' is already registered, aborting...
  platform_driver_register returned -16

• Unloading the driver (even without attempting to re-load it later) causes a kernel panic the next time the system is powered off.

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!

[LRitzdorf]

On a related note, the chip here is actually a TPA6130A2, not a TPA613A2. The former supports I2C, while the latter does not 🙂

[tvannoy]

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.

[rksnider]

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 https://github.com/ADSD-SoC-FPGA/Code/issues/10#issuecomment-1957193772, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABNDUXAYOKN3ZG4IEIVLAQDYUYNZPAVCNFSM6AAAAABDSGF54GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJXGE4TGNZXGI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[LRitzdorf]

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 insmod, for example, that would look something like insmod ad1939.ko unload_reset=false.

[rksnider]

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 https://github.com/ADSD-SoC-FPGA/Code/issues/10#issuecomment-1960393735, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABNDUXAZXSDGACKYV2BGLATYU66JVAVCNFSM6AAAAABDSGF54GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQGM4TGNZTGU . You are receiving this because you commented.Message ID: @.***>

[tvannoy]

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").

[LRitzdorf]

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.

[LRitzdorf]

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.