Re: [PATCH v24 29/34] ALSA: usb-audio: qcom: Add USB offload route kcontrol

Wesley Cheng
2024-08-20T17:38:08Z

Hi Pierre,

On 8/19/2024 11:39 PM, Pierre-Louis Bossart wrote:
>>>> +/**
>>>> + * snd_usb_offload_create_ctl() - Add USB offload bounded mixer
>>>> + * @chip - USB SND chip device
>>>> + *
>>>> + * Creates a sound control for a USB audio device, so that applications can
>>>> + * query for if there is an available USB audio offload path, and which
>>>> + * card is managing it.
>>>> + */
>>>> +int snd_usb_offload_create_ctl(struct snd_usb_audio *chip)
>>>> +{
>>>> + struct usb_device *udev = chip->dev;
>>>> + struct snd_kcontrol_new *chip_kctl;
>>>> + struct snd_usb_stream *as;
>>>> + char ctl_name[37];
>>>> + int ret;
>>>> +
>>>> + list_for_each_entry(as, &chip->pcm_list, list) {
>>>> + chip_kctl = &snd_usb_offload_mapped_ctl;
>>>> + chip_kctl->count = 1;
>>>> + /*
>>>> + * Store the associated USB SND card number and PCM index for
>>>> + * the kctl.
>>>> + */
>>>> + chip_kctl->private_value = as->pcm_index |
>>>> + chip->card->number << 16;
>>>> + sprintf(ctl_name, "USB Offload Playback Route PCM#%d",
>>>> + as->pcm_index);
>>>> + chip_kctl->name = ctl_name;
>>>> + ret = snd_ctl_add(chip->card, snd_ctl_new1(chip_kctl,
>>>> + udev->bus->sysdev));
>>>> + if (ret < 0)
>>>> + break;
>>>> + }
>>>> +
>>>> + return ret;
>> Hi Pierre,
>>> None of this looks Qualcomm-specific, shouldn't this be part of the
>>> soc_usb framework instead of being added in the qcom/ stuff?
>> Started working on this particular comment, and there are some things that needs to be considered if we moved this into SOC USB:
>>
>> 1.  We do save the reference to the USB BE DAI link within the USB DT node, which can be fetched/referenced based on sysdev.  However, I'm not sure if everyone would potentially follow that way.
>>
>> 2.  I tried a few implementations of adding a new SOC USB API, and the argument list was a bit long, because I didn't want to directly reference the usb_chip.
>>
>> Sorry for the delay, but I wanted to give a good stab at implementing this before bringing up the implications.  It is possible, but definitely not as clean as how we have it now IMO.
> My comment was only referring to the location of the code, it's now in
> sound/usb/qcom/mixer_usb_offload.c but does not contain anything
> specific to Qualcomm. I was not asking for any encapsulation inside of
> soc-usb, I was only suggesting a move of the code to a shared helper
> library so that this code can be reused as is and not duplicated if the
> QCOM parts are not compiled in.

Ah, great, thanks for the clarification.  Let me take a look with that perspective.

Thanks

Wesley Cheng