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
