On Wed, 26 Jul 2023 00:59:57 +0200, Wesley Cheng wrote:
+static int enable_audio_stream(struct snd_usb_substream *subs,
snd_pcm_format_t pcm_format,unsigned int channels, unsigned int cur_rate,int datainterval)+{
... this implementation, I wonder whether it'd be better to modify and export snd_usb_hw_params() snd snd_usb_hw_free() to fit with qcom driver. Then you can avoid lots of open code.
I think the problem is that snd_usb_hw_params assumes that we've already done a PCM open on the PCM device created by USB SND. However, with the offload path, we don't reference the USB PCM device, but the one created by the platform sound card. Hence, I don't have access to the snd_pcm_substream.
I attempted to derive snd_pcm_substream from snd_usb_substream, but since PCM open isn't run, it doesn't provide a valid structure.
What do you think about adding a wrapper to snd_usb_hw_params? Have a version that will take in snd_usb_substream, and another that is registered to hw_params().
Yes, that's what I had in mind, too.
In general, if you see a direct use of chip->mutex, it can be often done better in a different form. The use of an internal lock or such from an external driver is always fragile and error-prone.
Also, the current open-code misses the potential race against the disconnection during the operation. In snd-usb-audio, it protects with snd_usb_lock_shutdown() and snd_usb_unlock_shutdown() pairs.
I agree...I think then the best approach would be something like the above, ie:
int snd_usb_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *hw_params) { struct snd_usb_substream *subs = substream->runtime->private_data;
snd_usb_ep_attach(subs, hw_params); ...
int snd_usb_ep_attach(...) { //implementation of current code in snd_usb_hw_params() } EXPORT_SYMBOL(snd_usb_ep_attach);
Yes, exactly something like that ;)
thanks,
Takashi