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

[Wesley Cheng <quic_wcheng@xxxxxxxxxxx>]

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