Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <c57ddcf6-81c3-6be4-a07e-0dbe7e8aa8f2@quicinc.com>
Date:   Wed, 18 Oct 2023 17:10:14 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v9 26/34] ASoC: qcom: qdsp6: q6afe: Split USB AFE
 dev_token param into separate API
Content-Language: en-US
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.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-27-quic_wcheng@quicinc.com>
 <c6a003eb-213d-4456-bc6a-e07c08c57396@linux.intel.com>
From:   Wesley Cheng <quic_wcheng@quicinc.com>
In-Reply-To: <c6a003eb-213d-4456-bc6a-e07c08c57396@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Pierre,

On 10/17/2023 3:39 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 10/17/23 15:01, Wesley Cheng wrote:
>> The Q6USB backend can carry information about the available USB SND cards
>> and PCM devices discovered on the USB bus.  The dev_token field is used by
>> the audio DSP to notify the USB offload driver of which card and PCM index
>> to enable playback on.  Separate this into a dedicated API, so the USB
>> backend can set the dev_token accordingly.  The audio DSP does not utilize
>> this information until the AFE port start command is sent, which is done
>> during the PCM prepare phase.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   sound/soc/qcom/qdsp6/q6afe.c | 49 +++++++++++++++++++++++++-----------
>>   sound/soc/qcom/qdsp6/q6afe.h |  1 +
>>   2 files changed, 36 insertions(+), 14 deletions(-)
>>
>> diff --git a/sound/soc/qcom/qdsp6/q6afe.c b/sound/soc/qcom/qdsp6/q6afe.c
>> index 72c4e6fe20c4..f09a756246f8 100644
>> --- a/sound/soc/qcom/qdsp6/q6afe.c
>> +++ b/sound/soc/qcom/qdsp6/q6afe.c
>> @@ -1394,10 +1394,42 @@ void q6afe_tdm_port_prepare(struct q6afe_port *port,
>>   }
>>   EXPORT_SYMBOL_GPL(q6afe_tdm_port_prepare);
>>   
>> -static int afe_port_send_usb_dev_param(struct q6afe_port *port, struct q6afe_usb_cfg *cfg)
>> +/**
>> + * afe_port_send_usb_dev_param() - Send USB dev token
>> + *
>> + * @port: Instance of afe port
>> + * @cardidx: USB SND card index to reference
>> + * @pcmidx: USB SND PCM device index to reference
>> + *
>> + * The USB dev token carries information about which USB SND card instance and
>> + * PCM device to execute the offload on.  This information is carried through
>> + * to the stream enable QMI request, which is handled by the offload class
>> + * driver.  The information is parsed to determine which USB device to query
>> + * the required resources for.
>> + */
>> +int afe_port_send_usb_dev_param(struct q6afe_port *port, int cardidx, int pcmidx)
>>   {
>> -	union afe_port_config *pcfg = &port->port_cfg;
>>   	struct afe_param_id_usb_audio_dev_params usb_dev;
>> +	int ret;
>> +
>> +	memset(&usb_dev, 0, sizeof(usb_dev));
>> +
>> +	usb_dev.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG;
>> +	usb_dev.dev_token = (cardidx << 16) | (pcmidx << 8);
>> +	ret = q6afe_port_set_param_v2(port, &usb_dev,
>> +				AFE_PARAM_ID_USB_AUDIO_DEV_PARAMS,
>> +				AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(usb_dev));
>> +	if (ret)
>> +		dev_err(port->afe->dev, "%s: AFE device param cmd failed %d\n",
>> +			__func__, ret);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(afe_port_send_usb_dev_param);
>> +
>> +static int afe_port_send_usb_params(struct q6afe_port *port, struct q6afe_usb_cfg *cfg)
>> +{
>> +	union afe_port_config *pcfg = &port->port_cfg;
>>   	struct afe_param_id_usb_audio_dev_lpcm_fmt lpcm_fmt;
>>   	struct afe_param_id_usb_audio_svc_interval svc_int;
>>   	int ret = 0;
>> @@ -1408,20 +1440,9 @@ static int afe_port_send_usb_dev_param(struct q6afe_port *port, struct q6afe_usb
>>   		goto exit;
>>   	}
>>   
>> -	memset(&usb_dev, 0, sizeof(usb_dev));
>>   	memset(&lpcm_fmt, 0, sizeof(lpcm_fmt));
>>   	memset(&svc_int, 0, sizeof(svc_int));
>>   
>> -	usb_dev.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG;
>> -	ret = q6afe_port_set_param_v2(port, &usb_dev,
>> -				      AFE_PARAM_ID_USB_AUDIO_DEV_PARAMS,
>> -				      AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(usb_dev));
>> -	if (ret) {
>> -		dev_err(port->afe->dev, "%s: AFE device param cmd failed %d\n",
>> -			__func__, ret);
>> -		goto exit;
>> -	}
>> -
> 
> this feels like a questionable patch split. Why not introduce the new
> helper earlier and avoid adding code then modifying the same code?
> 

Let me see if I can squash this with the change that adds the USB AFE port.

Thanks
Wesley Cheng