Hi Mark,
On 9/27/2023 7:48 AM, Mark Brown wrote:
On Thu, Sep 21, 2023 at 02:48:16PM -0700, Wesley Cheng wrote:
+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);
Very nitpicky but this function possibly wants a _usb_ in the name, not that it *super* matters with it being static. Or it could just be inlined into the only user and not worry about the naming at all.
Thanks for the review! Sure, let me reshuffle things around a bit and just get rid of this function entirely and inline it to the API below.
+/**
- snd_soc_usb_get_priv_data() - Retrieve private data stored
- @dev: device reference
- Fetch the private data stored in the USB SND SOC structure.
- */
+void *snd_soc_usb_get_priv_data(struct device *dev) +{
- struct snd_soc_usb *ctx;
- ctx = snd_soc_find_usb_ctx(dev);
- if (!ctx) {
/* Check if backend device */mutex_lock(&ctx_mutex);list_for_each_entry(ctx, &usb_ctx_list, list) {if (dev->of_node == ctx->dev->of_node) {mutex_unlock(&ctx_mutex);goto out;}}mutex_unlock(&ctx_mutex);ctx = NULL;- }
This seems a lot more expensive than I'd expect for a get_priv_data operation, usually it's just a container_of() or other constant time pulling out of a pointer rather than a linked list walk - the sort of thing that people put at the start of functions and do all the time. If we need this I think it needs a name that's more clearly tied to the use case.
I didn't actually find the user of this though?
So the end user of this would be the qc_audio_offload driver, within prepare_qmi_response(). This is to fetch some information about the DPCM backend during the stream enable request.
Previously, I limited the # of snd_soc_usb ports to be registered to one, but that would affect the scalability of this layer. Hence, adding a list instead increased the complexity. Will rename this accordingly.
Thanks Wesley Cheng