Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <8eb90a7a-8649-4a31-9997-d970915510bf@linux.intel.com>
Date:   Tue, 17 Oct 2023 16:48:06 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Subject: Re: [PATCH v9 07/34] ASoC: Add SOC USB APIs for adding an USB backend
To:     Wesley Cheng <quic_wcheng@quicinc.com>, mathias.nyman@intel.com,
        gregkh@linuxfoundation.org, lgirdwood@gmail.com,
        broonie@kernel.org, perex@perex.cz, tiwai@suse.com,
        agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        conor+dt@kernel.org, srinivas.kandagatla@linaro.org,
        bgoswami@quicinc.com, Thinh.Nguyen@synopsys.com
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        alsa-devel@alsa-project.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org
References: <20231017200109.11407-1-quic_wcheng@quicinc.com>
 <20231017200109.11407-8-quic_wcheng@quicinc.com>
Content-Language: en-US
In-Reply-To: <20231017200109.11407-8-quic_wcheng@quicinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/17/23 15:00, Wesley Cheng wrote:
> Some platforms may have support for offloading USB audio devices to a
> dedicated audio DSP.  Introduce a set of APIs that allow for management of
> USB sound card and PCM devices enumerated by the USB SND class driver.
> This allows for the ASoC components to be aware of what USB devices are

USB devices or USB endpoints? or both?

> available for offloading.

> +/**
> + * struct snd_soc_usb_device
> + * @card_idx - sound card index associated with USB device
> + * @chip_idx - USB sound chip array index
> + * @num_playback - number of playback streams
> + * @num_capture - number of capture streams

presumably excluding explicit feedback streams?

> + **/
> +struct snd_soc_usb_device {
> +	int card_idx;
> +	int chip_idx;
> +	int num_playback;
> +	int num_capture;
> +};
> +
> +/**
> + * struct snd_soc_usb
> + * @list - list head for SND SOC struct list
> + * @dev - USB backend device reference
> + * @component - reference to ASoC component
> + * @connection_status_cb - callback to notify connection events
> + * @priv_data - driver data
> + **/
> +struct snd_soc_usb {
> +	struct list_head list;
> +	struct device *dev;

usbdev for consistency with the API below?

> +	struct snd_soc_component *component;

could you use component only and infer the device from component->dev?

> +	int (*connection_status_cb)(struct snd_soc_usb *usb,
> +			struct snd_soc_usb_device *sdev, bool connected);
> +	void *priv_data;
> +};
> +
> +int snd_soc_usb_connect(struct device *usbdev, struct snd_soc_usb_device *sdev);
> +int snd_soc_usb_disconnect(struct device *usbdev, struct snd_soc_usb_device *sdev);
> +void *snd_soc_usb_get_priv_data(struct device *usbdev);
> +
> +struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev, void *priv,

struct device *usbdev for consistency ?

> +			int (*connection_cb)(struct snd_soc_usb *usb,
> +			struct snd_soc_usb_device *sdev, bool connected));
> +int snd_soc_usb_remove_port(struct device *dev);

struct device *usbdev for consistency ?


> +struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev, void *priv,
> +			int (*connection_cb)(struct snd_soc_usb *usb,
> +			struct snd_soc_usb_device *sdev, bool connected))> +{
> +	struct snd_soc_usb *usb;
> +
> +	usb = devm_kzalloc(dev, sizeof(*usb), GFP_KERNEL);
> +	if (!usb)
> +		return ERR_PTR(-ENOMEM);
> +
> +	usb->connection_status_cb = connection_cb;
> +	usb->dev = dev;
> +	usb->priv_data = priv;
> +
> +	mutex_lock(&ctx_mutex);
> +	list_add_tail(&usb->list, &usb_ctx_list);
> +	mutex_unlock(&ctx_mutex);
> +
> +	return usb;
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_usb_add_port);
> +
> +/**
> + * snd_soc_usb_remove_port() - Remove a USB backend port
> + * @dev: USB backend device
> + *
> + * Remove a USB backend device from USB SND SOC.  Memory is freed when USB
> + * backend is removed.

when the USB backend driver is unbound?

> + *
> + */
> +int snd_soc_usb_remove_port(struct device *dev)
> +{
> +	struct snd_soc_usb *ctx, *tmp;
> +
> +	mutex_lock(&ctx_mutex);
> +	list_for_each_entry_safe(ctx, tmp, &usb_ctx_list, list) {
> +		if (ctx->dev == dev) {
> +			list_del(&ctx->list);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&ctx_mutex);
> +
> +	return 0;

can this return void to align with the current trend?

> +}
> +EXPORT_SYMBOL_GPL(snd_soc_usb_remove_port);
> +
> +/**
> + * snd_soc_usb_connect() - Notification of USB device connection
> + * @usbdev: USB bus device
> + * @card_idx: USB SND card instance
> + *
> + * Notify of a new USB SND device connection.  The card_idx can be used to
> + * handle how the DPCM backend selects, which device to enable USB offloading
> + * on.

card_idx is not used below, and I don't see how this relates to a
notification?

> + *
> + */
> +int snd_soc_usb_connect(struct device *usbdev, struct snd_soc_usb_device *sdev)
> +{
> +	struct snd_soc_usb *ctx;
> +	struct device_node *node;
> +
> +	if (!usbdev)
> +		return -ENODEV;
> +
> +	node = snd_soc_find_phandle(usbdev);
> +	if (IS_ERR(node))
> +		return -ENODEV;
> +
> +	ctx = snd_soc_find_usb_ctx(node);
> +	of_node_put(node);
> +	if (!ctx)
> +		return -ENODEV;
> +
> +	if (ctx->connection_status_cb)
> +		ctx->connection_status_cb(ctx, sdev, true);
> +
> +	return 0;
> +}