From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C4D6B2F517; Tue, 17 Oct 2023 23:23:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="HH+mHOFD" Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.120]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5F6CFF5; Tue, 17 Oct 2023 16:23:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697584998; x=1729120998; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=5h5iCT8ktCVTN33vs8t2+sxyBfUL3ZcnowzRbDOneZw=; b=HH+mHOFDDEKP2b+kEZRC4gsWYEeHbbryMoEPDMFqWoXrKVncD2w0B23c mm34TxOvl9WaY1R6o4knHBa8iTpZIh6M6GR4IVBkLgxP2zrjBfMYfxfQk eSwNmNknNBGeWnWNcmrMT9nokWBaqqvGa59kU1X9mv9xFjUbg0TArzAAV JOMMNQ1IupZ7ofpgRKbUOHZBMq6dihAcjq2EzOXejPTEDi3mcmwSAfFM4 KiXwk/SQ+eNFSOG/Lnk2/I6rkYvAQPuUgl8fCzlQIckZMbGeiZs86V5We fs9OZEUaPUe2AM8lN26f3Uxz8uDhxaLfAPhWT2yDW85WM/L21gHROmXp6 g==; X-IronPort-AV: E=McAfee;i="6600,9927,10866"; a="384778071" X-IronPort-AV: E=Sophos;i="6.03,233,1694761200"; d="scan'208";a="384778071" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Oct 2023 16:23:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10866"; a="826637455" X-IronPort-AV: E=Sophos;i="6.03,233,1694761200"; d="scan'208";a="826637455" Received: from asprado-mobl2.amr.corp.intel.com (HELO [10.212.55.179]) ([10.212.55.179]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Oct 2023 16:23:13 -0700 Message-ID: <7af39ea4-f120-43a2-b023-fa281ff05966@linux.intel.com> Date: Tue, 17 Oct 2023 16:46:24 -0500 Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v9 11/34] ASoC: qcom: qdsp6: Add USB backend ASoC driver for Q6 Content-Language: en-US To: Wesley Cheng , 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-12-quic_wcheng@quicinc.com> From: Pierre-Louis Bossart In-Reply-To: <20231017200109.11407-12-quic_wcheng@quicinc.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net > +#include > +#include > +#include > +#include alphabetical order? > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "q6dsp-lpass-ports.h" > +#include "q6afe.h" > + > +#define SID_MASK 0xF Prefix? e.g. Q6_USB_SID_MASK? > + > +struct q6usb_port_data { > + struct q6afe_usb_cfg usb_cfg; > + struct snd_soc_usb *usb; > + struct q6usb_offload priv; > + int active_idx; index of what? > +}; > + > +static const struct snd_soc_dapm_widget q6usb_dai_widgets[] = { > + SND_SOC_DAPM_HP("USB_RX_BE", NULL), > +}; > + > +static const struct snd_soc_dapm_route q6usb_dapm_routes[] = { > + {"USB Playback", NULL, "USB_RX_BE"}, > +}; > + > +static int q6usb_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, > + struct snd_soc_dai *dai) > +{ > + return 0; > +} > + > +static const struct snd_soc_dai_ops q6usb_ops = { > + .hw_params = q6usb_hw_params, > +}; > + > +static struct snd_soc_dai_driver q6usb_be_dais[] = { > + { > + .playback = { > + .stream_name = "USB BE RX", > + .rates = SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 | > + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | > + SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | > + SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 | > + SNDRV_PCM_RATE_192000, > + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE | > + SNDRV_PCM_FMTBIT_U16_LE | SNDRV_PCM_FMTBIT_U16_BE | > + SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE | > + SNDRV_PCM_FMTBIT_U24_LE | SNDRV_PCM_FMTBIT_U24_BE, > + .channels_min = 1, > + .channels_max = 2, > + .rate_max = 192000, > + .rate_min = 8000, > + }, > + .id = USB_RX, > + .name = "USB_RX_BE", > + .ops = &q6usb_ops, > + }, > +}; This is a bit confusing. In patch 9/34 you have a static struct snd_soc_dai_driver q6dsp_audio_fe_dais[] = { but this was not described anywhere in your cover letter. or maybe this referred to the 'Q6AFE "cpu"', in which case you have inconsistent naming between q6dsp and q6afe? > + > +static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *component, > + const struct of_phandle_args *args, > + const char **dai_name) > +{ > + int id = args->args[0]; > + int ret = -EINVAL; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(q6usb_be_dais); i++) { > + if (q6usb_be_dais[i].id == id) { > + *dai_name = q6usb_be_dais[i].name; > + ret = 0; > + break; > + } > + } > + > + return ret; > +} > + > +static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, > + struct snd_soc_usb_device *sdev, bool connected) > +{ > + struct q6usb_port_data *data; > + > + if (!usb->component) > + return -ENODEV; > + > + data = dev_get_drvdata(usb->component->dev); > + > + if (connected) { > + /* We only track the latest USB headset plugged in */ > + data->active_idx = sdev->card_idx; > + } > + > + return 0; > +} > + > +static int q6usb_component_probe(struct snd_soc_component *component) > +{ > + struct q6usb_port_data *data = dev_get_drvdata(component->dev); > + > + data->usb = snd_soc_usb_add_port(component->dev, &data->priv, q6usb_alsa_connection_cb); There is a conceptual problem here wrt. resource allocation. snd_soc_usb_add_port() uses devm_kzalloc(). This cannot be used in a component probe, this has to be used in a real driver probe. > + if (IS_ERR(data->usb)) { > + dev_err(component->dev, "failed to add usb port\n"); > + return -ENODEV; > + } > + > + data->usb->component = component; > + > + return 0; > +} > + > +static void q6usb_component_remove(struct snd_soc_component *component) > +{ > + snd_soc_usb_remove_port(component->dev); and here as well, this should free the "struct snd_soc_usb *usb" allocated in the probe. relying on devm_ is not quite right. Or if you use devm_, you have to call it from the platform driver .probe. > +static struct platform_driver q6usb_dai_platform_driver = { > + .driver = { > + .name = "q6usb-dai", > + .of_match_table = of_match_ptr(q6usb_dai_device_id), > + }, > + .probe = q6usb_dai_dev_probe, > + /* > + * Remove not required as resources are cleaned up as part of > + * component removal. Others are device managed resources. > + */ > +}; > +module_platform_driver(q6usb_dai_platform_driver); > + > +MODULE_DESCRIPTION("Q6 USB backend dai driver"); > +MODULE_LICENSE("GPL");