Re: [RFC PATCH v3] ALSA: usb-audio: Add support for TASCAM US-144MKII

[Takashi Iwai]

On Sun, 03 Aug 2025 22:34:09 +0200,
Šerif Rami wrote:
> 
> Hi,
> 
> Apologies for the invalid patches and the noise from my earlier attempts.
> I have corrected my workflow and am now submitting this properly.
> 
> This patch introduces a new, dedicated driver for the TASCAM US-144MKII.
> It replaces the partial support that previously existed
> in the `us122l` driver, which now no longer targets this device.
> The new driver implements ALSA compatible duplex and RawMIDI implementation of proprietary MIDI protocol.
> 
> This v3 patch has been tested as follows:
> The code has been checked against the kernel's coding style guidelines.
> It compiles cleanly without any warnings or errors.
> I have performed a full kernel build against the latest master branch, installed it, and successfully verified that the module loads and functions correctly with the hardware.
> 
> Any feedback is greatly appreciated.

Thanks for the patch.  The code looks almost good and you can submit
as a proper patch at the next submission.
A few points from my side:

- The sysfs file should be dropped as Greg suggested

- Try to use guard() for spinlocks.  (I know other USB audio drivers
  don't do that yet, but I already have a bit patch set to convert
  those.)

  Of course, there are cases where guard() and scoped_guard() don't
  fit well (e.g. the loop with temporary unlock/re-locking), but in
  most cases, you can use it well.  If it doesn't fit, it's a good
  chance to take a look at your code again and reconsider whether the
  code flow can be changed better.

- Similarly, try to use __free(kfree) for temporary buffers.

- snd_pcm_lib_preallocate_*() can be replaced with
  snd_pcm_set_managed_buffer().  Then you can drop
  snd_pcm_lib_malloc_pages() and snd_pcm_lib_free_pages() calls from
  hw_params and hw_free callbacks, too.

- Most of enum info callbacks can be simplified with
  snd_ctl_enum_info() helper function.

- It's a bit big code and it'd be great if you can split the patches
  in a logical manner.  But it'd be OK-ish to have a single patch if
  it's not easy, too.


thanks,

Takashi