Re: [PATCH v13 48/53] ALSA: usb-audio: mixer: Add USB offloading mixer control

[Wesley Cheng <quic_wcheng@xxxxxxxxxxx>]

Hi Takashi,

On 2/6/2024 4:57 AM, Takashi Iwai wrote:
> On Sat, 03 Feb 2024 03:36:40 +0100,
> Wesley Cheng wrote:
> > In order to allow userspace/applications know about USB offloading status,
> > expose a sound kcontrol that fetches information about which sound card
> > index is associated with the ASoC platform card supporting offloading.  In
> > the USB audio offloading framework, the ASoC BE DAI link is the entity
> > responsible for registering to the SOC USB layer.  SOC USB will expose more
> > details about the current offloading status, which includes the USB sound
> > card and USB PCM device indexes currently being used.
> >
> > Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
>
> The concept is understandable, but the control element name ("SNDUSB
> OFFLD playback available") looks non-intrusive and non-conformant.
> Use a bit more understandable name instead.
>
> This provides a card number where the offload driver is bound, and the
> name should indicate something about that.

Hmmm, does USB sound have a naming convention that it usually follows 
for mixer/control interfaces?

For something that is more closely related, how about:
"USB offload capable card"

> Also, about the implementation:
>
> > +static int
> > +snd_usb_offload_create_mixer(struct usb_mixer_interface *mixer,
> > +		       const struct snd_kcontrol_new *new_kctl)
> > +{
> > +	struct snd_kcontrol *kctl;
> > +	struct usb_mixer_elem_info *elem;
> > +
> > +	elem = kzalloc(sizeof(struct usb_mixer_elem_info), GFP_KERNEL);
> > +	if (!elem)
> > +		return -ENOMEM;
> > +
> > +	elem->head.mixer = mixer;
> > +	elem->val_type = USB_MIXER_S32;
> > +	elem->control = 0;
> > +	elem->head.id = 0;
> > +	elem->channels = 1;
> > +
> > +	kctl = snd_ctl_new1(new_kctl, elem);
> > +	if (!kctl) {
> > +		kfree(elem);
> > +		return -ENOMEM;
> > +	}
> > +	kctl->private_free = snd_usb_mixer_elem_free;
> > +
> > +	return snd_usb_mixer_add_control(&elem->head, kctl);
>
> This control has almost little to do with the standard USB interface,
> and it'll be much simpler if you create a raw control element.
> Pass the bus or the sysdev to private_data, and that's all you need in
> the get callback.

Sure, I'll remove the need to register over the SND USB mixer API, and 
just use the core SND control APIs.

> Also, don't forget to set the proper access bits (it's read-only).

Thanks for pointing this out, will fix.

Thanks
Wesley Cheng

> thanks,
>
> Takashi