Re: [PATCH v28 30/32] ALSA: usb-audio: Add USB offload route kcontrol

[Wesley Cheng <quic_wcheng@xxxxxxxxxxx>]

Hi Pierre,

On 9/25/2024 7:54 AM, Pierre-Louis Bossart wrote:
>
>
>> +static int
>> +snd_usb_offload_route_get(struct snd_kcontrol *kcontrol,
>> +			  struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct device *sysdev = snd_kcontrol_chip(kcontrol);
>> +	int ret;
>> +
>> +	ret = snd_soc_usb_update_offload_route(sysdev,
>> +					       CARD_IDX(kcontrol->private_value),
>> +					       PCM_IDX(kcontrol->private_value),
>> +					       SNDRV_PCM_STREAM_PLAYBACK,
>> +					       ucontrol->value.integer.value);
>> +	if (ret < 0) {
>> +		ucontrol->value.integer.value[0] = -1;
>> +		ucontrol->value.integer.value[1] = -1;
>> +	}
> well this invalidates again what I understood from the last patch and
> goes back to what I understood initially: the error code is never
> returned to higher levels - when offload is not supported the kcontrol
> values are encoded to the -1 magic value.
Yes, higher levels won't get an error code when they try to fetch for the kcontrol value, and if say...there is no callback to update the offload route then the -1 values are passed back.  I don't think we would want to return an error code.  We just want to communicate the current mapping of the offload path.
>> +	return 0;
> and this begs the question if this helper should return a void value.
This is the registered callback for the .get() call for the kcontrol, so it has to follow the definition below:
    typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol)
>> +}
>> +
>> +static int snd_usb_offload_route_info(struct snd_kcontrol *kcontrol,
>> +				      struct snd_ctl_elem_info *uinfo)
>> +{
>> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
>> +	uinfo->count = 2;
>> +	uinfo->value.integer.min = -1;
>> +	/* Arbitrary max value, as there is no 'limit' on number of PCM devices */
>> +	uinfo->value.integer.max = 0xff;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct snd_kcontrol_new snd_usb_offload_mapped_ctl = {
>> +	.iface = SNDRV_CTL_ELEM_IFACE_CARD,
>> +	.access = SNDRV_CTL_ELEM_ACCESS_READ,
>> +	.info = snd_usb_offload_route_info,
>> +	.get = snd_usb_offload_route_get,
>> +};
>> +
>> +/**
>> + * 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_substream *subs;
>> +	struct snd_usb_stream *as;
>> +	char ctl_name[37];
> that's quite a magic value.

Ah, will fix this.  Should be 34 ("USB Offload Playback Route PCM#" [31] + max pcm index[3]).  From past discussions, technically there isn't an upper limit defined for PCM devices, but the above snd_usb_offload_route_info() has it set to 0xff, so I'll be consistent here as well and assume that if we have more than 255 PCM devices for one device, then we won't create further kcontrols. (probably still overkill, but who knows what USB audio devices are out there)

Thanks

Wesley Cheng


>> +	int ret;
>> +
>> +	list_for_each_entry(as, &chip->pcm_list, list) {
>> +		subs = &as->substream[SNDRV_PCM_STREAM_PLAYBACK];
>> +		if (!subs->ep_num)
>> +			continue;
>> +
>> +		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;
>> +}