diff mbox series

[RFC,v2,07/22] ASoC: Add SOC USB APIs for adding an USB backend

Message ID 20230126031424.14582-8-quic_wcheng@quicinc.com
State New
Headers show
Series Introduce QC USB SND audio offloading support | expand

Commit Message

Wesley Cheng Jan. 26, 2023, 3:14 a.m. UTC
Some platforms may want to register its USB port to be handled by the ASoC
framework.  Audio playback/capture support is also handled entirely by the
vendor ASoC drivers.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 include/sound/soc-usb.h |  33 +++++++
 sound/soc/Makefile      |   2 +-
 sound/soc/soc-usb.c     | 202 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 236 insertions(+), 1 deletion(-)
 create mode 100644 include/sound/soc-usb.h
 create mode 100644 sound/soc/soc-usb.c

Comments

Pierre-Louis Bossart Jan. 26, 2023, 3:32 p.m. UTC | #1
On 1/25/23 21:14, Wesley Cheng wrote:
> Some platforms may want to register its USB port to be handled by the ASoC
> framework.  Audio playback/capture support is also handled entirely by the
> vendor ASoC drivers.

Can you clarify what you mean by 'port'?

> +/**
> + * snd_soc_usb_add_port() - Add a USB backend port
> + * @dev: USB backend device
> + * @connection_cb: connection status callback
> + *
> + * Register a USB backend device to the SND USB SOC framework.  Memory is
> + * allocated as part of the USB backend device.
> + *
> + */
> +struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev,
> +			int (*connection_cb)(struct snd_soc_usb *usb, int card_idx,
> +			int 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;
> +
> +	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);

Can a backend have more than one ports?

Is there any relationship between port and USB endpoint, and if yes
where is this determined?

> +
> +/**
> + * 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.
> + *
> + */
> +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;
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_usb_remove_port);

What happens if the ASoC driver probes/initialize AFTER the USB device
is plugged?

Likewise, can the ASoC driver be removed 'safely' with a fallback to
normal non-offloaded operation happening on remove?

> +
> +/**
> + * 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 USB backend selects, which device to enable offloading on.
> + *
> + */
> +int snd_soc_usb_connect(struct device *usbdev, int card_idx)
> +{
> +	struct snd_soc_usb *ctx;
> +
> +	if (!usbdev)
> +		return -ENODEV;
> +
> +	ctx = snd_soc_find_usb_ctx(usbdev);
> +	if (!ctx)
> +		return -ENODEV;
> +
> +	if (ctx->connection_status_cb)
> +		ctx->connection_status_cb(ctx, card_idx, 1);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_usb_connect);
> +
> +/**
> + * snd_soc_usb_connect() - Notification of USB device connection
> + * @usbdev: USB bus device
> + *
> + * Notify of a new USB SND device disconnection to the USB backend.
> + *
> + */
> +int snd_soc_usb_disconnect(struct device *usbdev)
> +{
> +	struct snd_soc_usb *ctx;
> +
> +	if (!usbdev)
> +		return -ENODEV;
> +
> +	ctx = snd_soc_find_usb_ctx(usbdev);
> +	if (!ctx)
> +		return -ENODEV;
> +
> +	if (ctx->connection_status_cb)
> +		ctx->connection_status_cb(ctx, -1, 0);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_usb_disconnect);

Similar concern on connect/disconnect, does this assume any specific
order for the driver probe?
Greg Kroah-Hartman Jan. 28, 2023, 1:26 p.m. UTC | #2
On Wed, Jan 25, 2023 at 07:14:09PM -0800, Wesley Cheng wrote:
> diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h
> new file mode 100644
> index 000000000000..ec422a8a834f
> --- /dev/null
> +++ b/include/sound/soc-usb.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.

It is now 2023 :)

> + */
> +
> +#ifndef __LINUX_SND_SOC_USB_H
> +#define __LINUX_SND_SOC_USB_H
> +
> +/**
> + * struct snd_soc_usb
> + * @component - Reference to DAPM component
> + * @connection_status_cb - callback to notify connection events
> + * @priv_data - vendor data

You do not document all items in the structure so you will get build
warnings :(

And what exactly is "vendor data"?  You use that term in a few places in
this series, there is no such thing as a "vendor" in the kernel.  This
could be a device or driver specific data, but not a "vendor".

> --- /dev/null
> +++ b/sound/soc/soc-usb.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#include <linux/of.h>
> +#include <linux/usb.h>
> +#include <sound/soc.h>
> +#include <sound/soc-usb.h>
> +#include "../usb/card.h"
> +
> +static DEFINE_MUTEX(ctx_mutex);
> +static LIST_HEAD(usb_ctx_list);

What is this a list of?  Why a list?  This should be dynamic and tied to
the device itself somehow, not a separate list you have to walk.

> +
> +#define for_each_usb_ctx(ctx)			\
> +	list_for_each_entry(ctx, &usb_ctx_list, list)

No need for a #define like this, just spell it out.


> +
> +static struct device_node *snd_soc_find_phandle(struct device *dev)
> +{
> +	struct device_node *node;
> +
> +	node = of_parse_phandle(dev->of_node, "usb-soc-be", 0);
> +	if (!node)
> +		return ERR_PTR(-ENODEV);
> +
> +	return node;
> +}
> +
> +static struct snd_soc_usb *snd_soc_find_usb_ctx(struct device *dev)
> +{
> +	struct device_node *node;
> +	struct snd_soc_usb *ctx = NULL;
> +
> +	node = snd_soc_find_phandle(dev);
> +	if (IS_ERR(node))
> +		return NULL;
> +
> +	mutex_lock(&ctx_mutex);
> +	for_each_usb_ctx(ctx) {
> +		if (ctx->dev->of_node == node) {
> +			of_node_put(node);
> +			mutex_unlock(&ctx_mutex);
> +			return ctx;
> +		}
> +	}
> +	of_node_put(node);
> +	mutex_unlock(&ctx_mutex);
> +
> +	return NULL;
> +}
> +
> +/**
> + * snd_soc_usb_get_priv_data() - Retrieve private data stored
> + * @usbdev: USB bus sysdev
> + *
> + * Fetch the private data stored in the USB SND SOC structure.  This is
> + * intended to be called by the USB offloading class driver, in order to
> + * attain parameters about the USB backend device.
> + *
> + */
> +void *snd_soc_usb_get_priv_data(struct device *usbdev)
> +{
> +	struct snd_soc_usb *ctx;
> +
> +	if (!usbdev)
> +		return NULL;

How could usbdev ever be NULL?

> +
> +	ctx = snd_soc_find_usb_ctx(usbdev);
> +
> +	return ctx ? ctx->priv_data : NULL;
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_usb_get_priv_data);
> +
> +/**
> + * snd_soc_usb_set_priv_data() - Set private data stored
> + * @dev: USB backend device
> + * @priv: private data to store
> + *
> + * Save data describing the USB backend device parameters.  This is intended
> + * to be called by the ASoC USB backend driver.
> + *
> + */
> +void snd_soc_usb_set_priv_data(struct device *dev, void *priv)
> +{
> +	struct snd_soc_usb *ctx;


Why does this function take a "struct device" but the get function take
a USB device?

> +
> +	mutex_lock(&ctx_mutex);
> +	for_each_usb_ctx(ctx) {
> +		if (dev->of_node == ctx->dev->of_node) {
> +			ctx->priv_data = priv;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&ctx_mutex);
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_usb_set_priv_data);
> +
> +/**
> + * snd_soc_usb_add_port() - Add a USB backend port
> + * @dev: USB backend device
> + * @connection_cb: connection status callback
> + *
> + * Register a USB backend device to the SND USB SOC framework.  Memory is
> + * allocated as part of the USB backend device.
> + *
> + */
> +struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev,
> +			int (*connection_cb)(struct snd_soc_usb *usb, int card_idx,
> +			int 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;
> +
> +	mutex_lock(&ctx_mutex);
> +	list_add_tail(&usb->list, &usb_ctx_list);
> +	mutex_unlock(&ctx_mutex);

Again, why a list?


> +
> +	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.
> + *
> + */
> +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;
> +}
> +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 USB backend selects, which device to enable offloading on.
> + *
> + */
> +int snd_soc_usb_connect(struct device *usbdev, int card_idx)
> +{
> +	struct snd_soc_usb *ctx;
> +
> +	if (!usbdev)
> +		return -ENODEV;
> +
> +	ctx = snd_soc_find_usb_ctx(usbdev);
> +	if (!ctx)
> +		return -ENODEV;
> +
> +	if (ctx->connection_status_cb)
> +		ctx->connection_status_cb(ctx, card_idx, 1);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_usb_connect);
> +
> +/**
> + * snd_soc_usb_connect() - Notification of USB device connection
> + * @usbdev: USB bus device
> + *
> + * Notify of a new USB SND device disconnection to the USB backend.
> + *
> + */
> +int snd_soc_usb_disconnect(struct device *usbdev)
> +{
> +	struct snd_soc_usb *ctx;
> +
> +	if (!usbdev)
> +		return -ENODEV;
> +
> +	ctx = snd_soc_find_usb_ctx(usbdev);
> +	if (!ctx)
> +		return -ENODEV;
> +
> +	if (ctx->connection_status_cb)
> +		ctx->connection_status_cb(ctx, -1, 0);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_usb_disconnect);

Meta-comment, why are all of these in the sound directory?  They are
only operating on USB devices, nothing else.  So why here?

thanks,

greg k-h
Zhou Furong Jan. 29, 2023, 6:54 a.m. UTC | #3
>> +void *snd_soc_usb_get_priv_data(struct device *usbdev)
>> +{
>> +	struct snd_soc_usb *ctx;
>> +
>> +	if (!usbdev)
>> +		return NULL;
> 
> How could usbdev ever be NULL?
The method is exported to public, valid check should be reasonable
as someone may call it by mistake

> 
>> +
>> +	ctx = snd_soc_find_usb_ctx(usbdev);
>> +
>> +	return ctx ? ctx->priv_data : NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_get_priv_data);
>> +
Greg Kroah-Hartman Jan. 29, 2023, 7:09 a.m. UTC | #4
On Sun, Jan 29, 2023 at 02:54:43PM +0800, Zhou Furong wrote:
> 
> 
> > > +void *snd_soc_usb_get_priv_data(struct device *usbdev)
> > > +{
> > > +	struct snd_soc_usb *ctx;
> > > +
> > > +	if (!usbdev)
> > > +		return NULL;
> > 
> > How could usbdev ever be NULL?
> The method is exported to public, valid check should be reasonable
> as someone may call it by mistake

We do not protect the kernel from itself like this, no need to check
things that should never happen.  If the caller gets it wrong, their
code will break :)

thanks,

greg k-h
Zhou Furong Jan. 30, 2023, 8:34 a.m. UTC | #5
On 2023/1/29 15:09, Greg KH wrote:
> On Sun, Jan 29, 2023 at 02:54:43PM +0800, Zhou Furong wrote:
>>
>>
>>>> +void *snd_soc_usb_get_priv_data(struct device *usbdev)
>>>> +{
>>>> +	struct snd_soc_usb *ctx;
>>>> +
>>>> +	if (!usbdev)
>>>> +		return NULL;
>>>
>>> How could usbdev ever be NULL?
>> The method is exported to public, valid check should be reasonable
>> as someone may call it by mistake
> 
> We do not protect the kernel from itself like this, no need to check
> things that should never happen.  If the caller gets it wrong, their
> code will break :)
> 
> thanks,
> 
> greg k-h

Thank you Greg!

This has been confused me for long time when I found Linux kernel don't 
check input even for public method.
Greg Kroah-Hartman Jan. 30, 2023, 9:27 a.m. UTC | #6
On Mon, Jan 30, 2023 at 04:34:58PM +0800, Zhou Furong wrote:
> 
> 
> On 2023/1/29 15:09, Greg KH wrote:
> > On Sun, Jan 29, 2023 at 02:54:43PM +0800, Zhou Furong wrote:
> > > 
> > > 
> > > > > +void *snd_soc_usb_get_priv_data(struct device *usbdev)
> > > > > +{
> > > > > +	struct snd_soc_usb *ctx;
> > > > > +
> > > > > +	if (!usbdev)
> > > > > +		return NULL;
> > > > 
> > > > How could usbdev ever be NULL?
> > > The method is exported to public, valid check should be reasonable
> > > as someone may call it by mistake
> > 
> > We do not protect the kernel from itself like this, no need to check
> > things that should never happen.  If the caller gets it wrong, their
> > code will break :)
> > 
> > thanks,
> > 
> > greg k-h
> 
> Thank you Greg!
> 
> This has been confused me for long time when I found Linux kernel don't
> check input even for public method.

That is because we control all callers of internal kernel apis,
otherwise we would have nothing but checks all over the place that did
nothing in the end.

thanks,

greg k-h
Wesley Cheng Jan. 30, 2023, 10:36 p.m. UTC | #7
Hi Pierre,

On 1/26/2023 7:32 AM, Pierre-Louis Bossart wrote:
> 
> 
> On 1/25/23 21:14, Wesley Cheng wrote:
>> Some platforms may want to register its USB port to be handled by the ASoC
>> framework.  Audio playback/capture support is also handled entirely by the
>> vendor ASoC drivers.
> 
> Can you clarify what you mean by 'port'?
> 

Ideally, port is intended to represent how many USB audio devices the 
audio DSP can support.

>> +/**
>> + * snd_soc_usb_add_port() - Add a USB backend port
>> + * @dev: USB backend device
>> + * @connection_cb: connection status callback
>> + *
>> + * Register a USB backend device to the SND USB SOC framework.  Memory is
>> + * allocated as part of the USB backend device.
>> + *
>> + */
>> +struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev,
>> +			int (*connection_cb)(struct snd_soc_usb *usb, int card_idx,
>> +			int 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;
>> +
>> +	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);
> 
> Can a backend have more than one ports?
> 

The intended model is one BE per usb device.

> Is there any relationship between port and USB endpoint, and if yes
> where is this determined?
> 

Might need some more clarification on this question.  I mean, whichever 
port the USB device is connected to will be the USB endpoint(s) being 
utilized.

Maybe the confusion is in the "port" label itself?  You can think of 
port meaning the same thing as a udev. (struct usb_device)

>> +
>> +/**
>> + * 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.
>> + *
>> + */
>> +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;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_remove_port);
> 
> What happens if the ASoC driver probes/initialize AFTER the USB device
> is plugged?
> 

Good point, that's one thing I was seeing how we could address 
initially, but never got around to adding it.  Currently the code would 
basically not enable the DAPM pins/path for that backend device. (when 
we probe the USB backend, we disable the USB_RX_BE pin)

If the USB SND driver gets the connection before the USB BE is up, then 
q6usb_alsa_connection_cb() would not be called, which sets the USB_RX_BE 
pin state (to enable).  Then when the Q6USB backend is probed, the 
USB_RX_BE pin state would be set to disabled.

Will see if I can maybe cache the connection state somewhere and pass it 
along when the USB BE is up.

> Likewise, can the ASoC driver be removed 'safely' with a fallback to
> normal non-offloaded operation happening on remove?
> 

As of now, there is no automatic fallback for that scenario.  If I 
compile all our Q6 dai drivers as modules as well as the platform card, 
then we won't be able to individually remove ASoC component modules 
since they are being used by the platform soundcard device.

The only way to remove the USB backend driver is first to remove the 
platform sound card, which will tear down the current audio session and 
remove the sound card that was created.


>> +
>> +/**
>> + * 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 USB backend selects, which device to enable offloading on.
>> + *
>> + */
>> +int snd_soc_usb_connect(struct device *usbdev, int card_idx)
>> +{
>> +	struct snd_soc_usb *ctx;
>> +
>> +	if (!usbdev)
>> +		return -ENODEV;
>> +
>> +	ctx = snd_soc_find_usb_ctx(usbdev);
>> +	if (!ctx)
>> +		return -ENODEV;
>> +
>> +	if (ctx->connection_status_cb)
>> +		ctx->connection_status_cb(ctx, card_idx, 1);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_connect);
>> +
>> +/**
>> + * snd_soc_usb_connect() - Notification of USB device connection
>> + * @usbdev: USB bus device
>> + *
>> + * Notify of a new USB SND device disconnection to the USB backend.
>> + *
>> + */
>> +int snd_soc_usb_disconnect(struct device *usbdev)
>> +{
>> +	struct snd_soc_usb *ctx;
>> +
>> +	if (!usbdev)
>> +		return -ENODEV;
>> +
>> +	ctx = snd_soc_find_usb_ctx(usbdev);
>> +	if (!ctx)
>> +		return -ENODEV;
>> +
>> +	if (ctx->connection_status_cb)
>> +		ctx->connection_status_cb(ctx, -1, 0);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_disconnect);
> 
> Similar concern on connect/disconnect, does this assume any specific
> order for the driver probe?

I think the above explanation clarifies the order which is currently 
going to cause us to potentially miss a device connection.

Thanks
Wesley Cheng
Wesley Cheng Feb. 10, 2023, 10:46 p.m. UTC | #8
Hi Greg,

On 1/28/2023 5:26 AM, Greg KH wrote:
> On Wed, Jan 25, 2023 at 07:14:09PM -0800, Wesley Cheng wrote:
>> diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h
>> new file mode 100644
>> index 000000000000..ec422a8a834f
>> --- /dev/null
>> +++ b/include/sound/soc-usb.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> 
> It is now 2023 :)
> 

Sorry for the delayed response.  Will change the year for all the 
patches...time flies.

>> + */
>> +
>> +#ifndef __LINUX_SND_SOC_USB_H
>> +#define __LINUX_SND_SOC_USB_H
>> +
>> +/**
>> + * struct snd_soc_usb
>> + * @component - Reference to DAPM component
>> + * @connection_status_cb - callback to notify connection events
>> + * @priv_data - vendor data
> 
> You do not document all items in the structure so you will get build
> warnings :(
> 
> And what exactly is "vendor data"?  You use that term in a few places in
> this series, there is no such thing as a "vendor" in the kernel.  This
> could be a device or driver specific data, but not a "vendor".
> 

The term vendor data can be renamed to something else.  It essentially 
signifies that each platform can potentially have a different 
implementation of how these callbacks behave.  It makes sense to rename 
it into driver specific data, since it depends on how the offload driver 
is added.

>> --- /dev/null
>> +++ b/sound/soc/soc-usb.c
>> @@ -0,0 +1,202 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +#include <linux/of.h>
>> +#include <linux/usb.h>
>> +#include <sound/soc.h>
>> +#include <sound/soc-usb.h>
>> +#include "../usb/card.h"
>> +
>> +static DEFINE_MUTEX(ctx_mutex);
>> +static LIST_HEAD(usb_ctx_list);
> 
> What is this a list of?  Why a list?  This should be dynamic and tied to
> the device itself somehow, not a separate list you have to walk.
> 

This is a list of USB backends that have been registered.  At the moment 
we only have one USB backend, as the audio DSP only supports playback on 
a single device through the offload path (on our chipset).  Potentially, 
if there are other platforms that can support multiple, they can 
register several USB backends to control each offload path accordingly.

It was difficult to tie the "struct snd_soc_usb" into a device, because 
of how different the device lifetime is for the USB audio device (udev) 
and the USB backend (one is dynamically created/freed based on USB 
device plugged into the port, the other exists until the backend is 
removed), and the fact that communication has to happen both ways.  This 
warrented a need to have this structure exist as a separate entity, 
hence the reason why I went with a list.

>> +
>> +#define for_each_usb_ctx(ctx)			\
>> +	list_for_each_entry(ctx, &usb_ctx_list, list)
> 
> No need for a #define like this, just spell it out.
> 
> 

Sure.

>> +
>> +static struct device_node *snd_soc_find_phandle(struct device *dev)
>> +{
>> +	struct device_node *node;
>> +
>> +	node = of_parse_phandle(dev->of_node, "usb-soc-be", 0);
>> +	if (!node)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	return node;
>> +}
>> +
>> +static struct snd_soc_usb *snd_soc_find_usb_ctx(struct device *dev)
>> +{
>> +	struct device_node *node;
>> +	struct snd_soc_usb *ctx = NULL;
>> +
>> +	node = snd_soc_find_phandle(dev);
>> +	if (IS_ERR(node))
>> +		return NULL;
>> +
>> +	mutex_lock(&ctx_mutex);
>> +	for_each_usb_ctx(ctx) {
>> +		if (ctx->dev->of_node == node) {
>> +			of_node_put(node);
>> +			mutex_unlock(&ctx_mutex);
>> +			return ctx;
>> +		}
>> +	}
>> +	of_node_put(node);
>> +	mutex_unlock(&ctx_mutex);
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * snd_soc_usb_get_priv_data() - Retrieve private data stored
>> + * @usbdev: USB bus sysdev
>> + *
>> + * Fetch the private data stored in the USB SND SOC structure.  This is
>> + * intended to be called by the USB offloading class driver, in order to
>> + * attain parameters about the USB backend device.
>> + *
>> + */
>> +void *snd_soc_usb_get_priv_data(struct device *usbdev)
>> +{
>> +	struct snd_soc_usb *ctx;
>> +
>> +	if (!usbdev)
>> +		return NULL;
> 
> How could usbdev ever be NULL?
> 
>> +
>> +	ctx = snd_soc_find_usb_ctx(usbdev);
>> +
>> +	return ctx ? ctx->priv_data : NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_get_priv_data);
>> +
>> +/**
>> + * snd_soc_usb_set_priv_data() - Set private data stored
>> + * @dev: USB backend device
>> + * @priv: private data to store
>> + *
>> + * Save data describing the USB backend device parameters.  This is intended
>> + * to be called by the ASoC USB backend driver.
>> + *
>> + */
>> +void snd_soc_usb_set_priv_data(struct device *dev, void *priv)
>> +{
>> +	struct snd_soc_usb *ctx;
> 
> 
> Why does this function take a "struct device" but the get function take
> a USB device?
> 

I can modify that.  It was done this way with the intention to not have 
the USB SND offload driver worry about finding the USB backend device. 
But in a way, it does indirectly place a design where the USB SND 
offload is the consumer of the platform data, and the USB backend is the 
source.

>> +
>> +	mutex_lock(&ctx_mutex);
>> +	for_each_usb_ctx(ctx) {
>> +		if (dev->of_node == ctx->dev->of_node) {
>> +			ctx->priv_data = priv;
>> +			break;
>> +		}
>> +	}
>> +	mutex_unlock(&ctx_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_set_priv_data);
>> +
>> +/**
>> + * snd_soc_usb_add_port() - Add a USB backend port
>> + * @dev: USB backend device
>> + * @connection_cb: connection status callback
>> + *
>> + * Register a USB backend device to the SND USB SOC framework.  Memory is
>> + * allocated as part of the USB backend device.
>> + *
>> + */
>> +struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev,
>> +			int (*connection_cb)(struct snd_soc_usb *usb, int card_idx,
>> +			int 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;
>> +
>> +	mutex_lock(&ctx_mutex);
>> +	list_add_tail(&usb->list, &usb_ctx_list);
>> +	mutex_unlock(&ctx_mutex);
> 
> Again, why a list?
> 
> 
>> +
>> +	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.
>> + *
>> + */
>> +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;
>> +}
>> +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 USB backend selects, which device to enable offloading on.
>> + *
>> + */
>> +int snd_soc_usb_connect(struct device *usbdev, int card_idx)
>> +{
>> +	struct snd_soc_usb *ctx;
>> +
>> +	if (!usbdev)
>> +		return -ENODEV;
>> +
>> +	ctx = snd_soc_find_usb_ctx(usbdev);
>> +	if (!ctx)
>> +		return -ENODEV;
>> +
>> +	if (ctx->connection_status_cb)
>> +		ctx->connection_status_cb(ctx, card_idx, 1);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_connect);
>> +
>> +/**
>> + * snd_soc_usb_connect() - Notification of USB device connection
>> + * @usbdev: USB bus device
>> + *
>> + * Notify of a new USB SND device disconnection to the USB backend.
>> + *
>> + */
>> +int snd_soc_usb_disconnect(struct device *usbdev)
>> +{
>> +	struct snd_soc_usb *ctx;
>> +
>> +	if (!usbdev)
>> +		return -ENODEV;
>> +
>> +	ctx = snd_soc_find_usb_ctx(usbdev);
>> +	if (!ctx)
>> +		return -ENODEV;
>> +
>> +	if (ctx->connection_status_cb)
>> +		ctx->connection_status_cb(ctx, -1, 0);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_disconnect);
> 
> Meta-comment, why are all of these in the sound directory?  They are
> only operating on USB devices, nothing else.  So why here?
> 

The USB SND exists in the sound directory as well as the ASoC framework. 
  The main goal of this is to abstract USB SND to components that ASoC 
requires, versus having to expose all of USB SND.

Thanks
Wesley Cheng
diff mbox series

Patch

diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h
new file mode 100644
index 000000000000..ec422a8a834f
--- /dev/null
+++ b/include/sound/soc-usb.h
@@ -0,0 +1,33 @@ 
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef __LINUX_SND_SOC_USB_H
+#define __LINUX_SND_SOC_USB_H
+
+/**
+ * struct snd_soc_usb
+ * @component - Reference to DAPM component
+ * @connection_status_cb - callback to notify connection events
+ * @priv_data - vendor data
+ **/
+struct snd_soc_usb {
+	struct list_head list;
+	struct device *dev;
+	struct snd_soc_component *component;
+	int (*connection_status_cb)(struct snd_soc_usb *usb, int card_idx,
+				int connected);
+	void *priv_data;
+};
+
+int snd_soc_usb_connect(struct device *usbdev, int card_idx);
+int snd_soc_usb_disconnect(struct device *usbdev);
+void snd_soc_usb_set_priv_data(struct device *dev, void *priv);
+void *snd_soc_usb_get_priv_data(struct device *usbdev);
+
+struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev,
+			int (*connection_cb)(struct snd_soc_usb *usb, int card_idx,
+			int connected));
+int snd_soc_usb_remove_port(struct device *dev);
+#endif
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 507eaed1d6a1..3305ceb59d84 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -1,5 +1,5 @@ 
 # SPDX-License-Identifier: GPL-2.0
-snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-utils.o soc-dai.o soc-component.o
+snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-usb.o soc-utils.o soc-dai.o soc-component.o
 snd-soc-core-objs += soc-pcm.o soc-devres.o soc-ops.o soc-link.o soc-card.o
 snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
 
diff --git a/sound/soc/soc-usb.c b/sound/soc/soc-usb.c
new file mode 100644
index 000000000000..bfce6c9609e1
--- /dev/null
+++ b/sound/soc/soc-usb.c
@@ -0,0 +1,202 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+#include <linux/of.h>
+#include <linux/usb.h>
+#include <sound/soc.h>
+#include <sound/soc-usb.h>
+#include "../usb/card.h"
+
+static DEFINE_MUTEX(ctx_mutex);
+static LIST_HEAD(usb_ctx_list);
+
+#define for_each_usb_ctx(ctx)			\
+	list_for_each_entry(ctx, &usb_ctx_list, list)
+
+static struct device_node *snd_soc_find_phandle(struct device *dev)
+{
+	struct device_node *node;
+
+	node = of_parse_phandle(dev->of_node, "usb-soc-be", 0);
+	if (!node)
+		return ERR_PTR(-ENODEV);
+
+	return node;
+}
+
+static struct snd_soc_usb *snd_soc_find_usb_ctx(struct device *dev)
+{
+	struct device_node *node;
+	struct snd_soc_usb *ctx = NULL;
+
+	node = snd_soc_find_phandle(dev);
+	if (IS_ERR(node))
+		return NULL;
+
+	mutex_lock(&ctx_mutex);
+	for_each_usb_ctx(ctx) {
+		if (ctx->dev->of_node == node) {
+			of_node_put(node);
+			mutex_unlock(&ctx_mutex);
+			return ctx;
+		}
+	}
+	of_node_put(node);
+	mutex_unlock(&ctx_mutex);
+
+	return NULL;
+}
+
+/**
+ * snd_soc_usb_get_priv_data() - Retrieve private data stored
+ * @usbdev: USB bus sysdev
+ *
+ * Fetch the private data stored in the USB SND SOC structure.  This is
+ * intended to be called by the USB offloading class driver, in order to
+ * attain parameters about the USB backend device.
+ *
+ */
+void *snd_soc_usb_get_priv_data(struct device *usbdev)
+{
+	struct snd_soc_usb *ctx;
+
+	if (!usbdev)
+		return NULL;
+
+	ctx = snd_soc_find_usb_ctx(usbdev);
+
+	return ctx ? ctx->priv_data : NULL;
+}
+EXPORT_SYMBOL_GPL(snd_soc_usb_get_priv_data);
+
+/**
+ * snd_soc_usb_set_priv_data() - Set private data stored
+ * @dev: USB backend device
+ * @priv: private data to store
+ *
+ * Save data describing the USB backend device parameters.  This is intended
+ * to be called by the ASoC USB backend driver.
+ *
+ */
+void snd_soc_usb_set_priv_data(struct device *dev, void *priv)
+{
+	struct snd_soc_usb *ctx;
+
+	mutex_lock(&ctx_mutex);
+	for_each_usb_ctx(ctx) {
+		if (dev->of_node == ctx->dev->of_node) {
+			ctx->priv_data = priv;
+			break;
+		}
+	}
+	mutex_unlock(&ctx_mutex);
+}
+EXPORT_SYMBOL_GPL(snd_soc_usb_set_priv_data);
+
+/**
+ * snd_soc_usb_add_port() - Add a USB backend port
+ * @dev: USB backend device
+ * @connection_cb: connection status callback
+ *
+ * Register a USB backend device to the SND USB SOC framework.  Memory is
+ * allocated as part of the USB backend device.
+ *
+ */
+struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev,
+			int (*connection_cb)(struct snd_soc_usb *usb, int card_idx,
+			int 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;
+
+	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.
+ *
+ */
+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;
+}
+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 USB backend selects, which device to enable offloading on.
+ *
+ */
+int snd_soc_usb_connect(struct device *usbdev, int card_idx)
+{
+	struct snd_soc_usb *ctx;
+
+	if (!usbdev)
+		return -ENODEV;
+
+	ctx = snd_soc_find_usb_ctx(usbdev);
+	if (!ctx)
+		return -ENODEV;
+
+	if (ctx->connection_status_cb)
+		ctx->connection_status_cb(ctx, card_idx, 1);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_usb_connect);
+
+/**
+ * snd_soc_usb_connect() - Notification of USB device connection
+ * @usbdev: USB bus device
+ *
+ * Notify of a new USB SND device disconnection to the USB backend.
+ *
+ */
+int snd_soc_usb_disconnect(struct device *usbdev)
+{
+	struct snd_soc_usb *ctx;
+
+	if (!usbdev)
+		return -ENODEV;
+
+	ctx = snd_soc_find_usb_ctx(usbdev);
+	if (!ctx)
+		return -ENODEV;
+
+	if (ctx->connection_status_cb)
+		ctx->connection_status_cb(ctx, -1, 0);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_usb_disconnect);