Received-SPF: pass (google.com: domain of linux-kernel+bounces-54976-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 06 Feb 2024 13:57:46 +0100
Message-ID: <871q9pwy0l.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Wesley Cheng <quic_wcheng@quicinc.com>
Cc: <srinivas.kandagatla@linaro.org>,
	<mathias.nyman@intel.com>,
	<perex@perex.cz>,
	<conor+dt@kernel.org>,
	<corbet@lwn.net>,
	<lgirdwood@gmail.com>,
	<andersson@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>,
	<gregkh@linuxfoundation.org>,
	<Thinh.Nguyen@synopsys.com>,
	<broonie@kernel.org>,
	<bgoswami@quicinc.com>,
	<tiwai@suse.com>,
	<robh+dt@kernel.org>,
	<konrad.dybcio@linaro.org>,
	<linux-kernel@vger.kernel.org>,
	<devicetree@vger.kernel.org>,
	<linux-sound@vger.kernel.org>,
	<linux-usb@vger.kernel.org>,
	<linux-arm-msm@vger.kernel.org>,
	<linux-doc@vger.kernel.org>,
	<alsa-devel@alsa-project.org>
Subject: Re: [PATCH v13 48/53] ALSA: usb-audio: mixer: Add USB offloading mixer control
In-Reply-To: <20240203023645.31105-49-quic_wcheng@quicinc.com>
References: <20240203023645.31105-1-quic_wcheng@quicinc.com>
	<20240203023645.31105-49-quic_wcheng@quicinc.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII

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@quicinc.com>

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.

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.

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


thanks,

Takashi