[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6372908e-7dbd-c36f-bfa4-ad3c342e5ce8@quicinc.com>
Date: Mon, 7 Aug 2023 16:39:24 -0700
From: Wesley Cheng <quic_wcheng@...cinc.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
<agross@...nel.org>, <andersson@...nel.org>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<catalin.marinas@....com>, <will@...nel.org>,
<mathias.nyman@...el.com>, <gregkh@...uxfoundation.org>,
<lgirdwood@...il.com>, <broonie@...nel.org>, <perex@...ex.cz>,
<tiwai@...e.com>, <srinivas.kandagatla@...aro.org>,
<bgoswami@...cinc.com>, <Thinh.Nguyen@...opsys.com>
CC: <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-usb@...r.kernel.org>, <alsa-devel@...a-project.org>,
<quic_jackp@...cinc.com>, <oneukum@...e.com>,
<albertccwang@...gle.com>, <o-takashi@...amocchi.jp>
Subject: Re: [PATCH v4 08/32] ASoC: qcom: qdsp6: Introduce USB AFE port to
q6dsp
Hi Pierre,
On 7/25/2023 1:27 AM, Pierre-Louis Bossart wrote:
>
>> +static const struct snd_soc_dai_ops q6usb_ops = {
>> + .prepare = q6afe_dai_prepare,
>> + .hw_params = q6usb_hw_params,
>> + .shutdown = q6afe_dai_shutdown,
>
> it's a bit odd to see a .shutdown without a .startup?
>
> Is this intentional and should a comment be added?
>
>
That is correct, because the Q6AFE port start command needs to also pass
in the PCM params, so we wait for the hw_params() callback before
actually starting the port. I will add a comment.
>> +/* device token of actual end USB aduio device */
>
> audio
>
>> + u32 dev_token;
>> +/* endianness of this interface */
>> + u32 endian;
>> +/* service interval */
>> + u32 service_interval;
>> +} __packed;
>> +
>> +/**
>> + * struct afe_param_id_usb_audio_dev_params
>> + * @cfg_minor_version: Minor version used for tracking USB audio device
>> + * configuration.
>> + * Supported values:
>> + * AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG
>> + * @dev_token: device token of actual end USB aduio device
>
> audio. please run a spell-checker.
>
>
Will fix the typos.
>> + svc_int.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG;
>> + svc_int.svc_interval = pcfg->usb_cfg.service_interval;
>> + ret = q6afe_port_set_param_v2(port, &svc_int,
>> + AFE_PARAM_ID_USB_AUDIO_SVC_INTERVAL,
>> + AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(svc_int));
>> + if (ret) {
>> + dev_err(port->afe->dev, "%s: AFE device param cmd svc_interval failed %d\n",
>> + __func__, ret);
>> + ret = -EINVAL;
>
> why do you override the return value?
>
>> + goto exit;
>
> not necessary, this is a jump to the next line. Looks like copy-paste ...
>
Thanks, will fix.
>> + }
>> +exit:
>> + return ret;
>> +}
>> +
>> +/**
>> + * q6afe_usb_port_prepare() - Prepare usb afe port.
>> + *
>> + * @port: Instance of afe port
>> + * @cfg: USB configuration for the afe port
>> + *
>> + */
>> +void q6afe_usb_port_prepare(struct q6afe_port *port,
>> + struct q6afe_usb_cfg *cfg)
>> +{
>> + union afe_port_config *pcfg = &port->port_cfg;
>> +
>> + pcfg->usb_cfg.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG;
>> + pcfg->usb_cfg.sample_rate = cfg->sample_rate;
>> + pcfg->usb_cfg.num_channels = cfg->num_channels;
>> + pcfg->usb_cfg.bit_width = cfg->bit_width;
>> +
>> + afe_port_send_usb_dev_param(port, cfg);
>> +}
>> +EXPORT_SYMBOL_GPL(q6afe_usb_port_prepare);
>> +
>> /**
>> * q6afe_hdmi_port_prepare() - Prepare hdmi afe port.
>> *
>> @@ -1611,7 +1791,10 @@ struct q6afe_port *q6afe_port_get_from_id(struct device *dev, int id)
>> break;
>> case AFE_PORT_ID_WSA_CODEC_DMA_RX_0 ... AFE_PORT_ID_RX_CODEC_DMA_RX_7:
>> cfg_type = AFE_PARAM_ID_CODEC_DMA_CONFIG;
>> - break;
>> + break;
>> + case AFE_PORT_ID_USB_RX:
>> + cfg_type = AFE_PARAM_ID_USB_AUDIO_CONFIG;
>> + break;
>> default:
>> dev_err(dev, "Invalid port id 0x%x\n", port_id);
>> return ERR_PTR(-EINVAL);
>> diff --git a/sound/soc/qcom/qdsp6/q6afe.h b/sound/soc/qcom/qdsp6/q6afe.h
>> index 30fd77e2f458..e098a3e15135 100644
>> --- a/sound/soc/qcom/qdsp6/q6afe.h
>> +++ b/sound/soc/qcom/qdsp6/q6afe.h
>> @@ -5,7 +5,7 @@
>>
>> #include <dt-bindings/sound/qcom,q6afe.h>
>>
>> -#define AFE_PORT_MAX 129
>> +#define AFE_PORT_MAX 130
>>
>> #define MSM_AFE_PORT_TYPE_RX 0
>> #define MSM_AFE_PORT_TYPE_TX 1
>> @@ -205,6 +205,47 @@ struct q6afe_cdc_dma_cfg {
>> u16 active_channels_mask;
>> };
>>
>> +/**
>> + * struct q6afe_usb_cfg
>> + * @cfg_minor_version: Minor version used for tracking USB audio device
>> + * configuration.
>> + * Supported values:
>> + * AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG
>> + * @sample_rate: Sampling rate of the port
>> + * Supported values:
>> + * AFE_PORT_SAMPLE_RATE_8K
>> + * AFE_PORT_SAMPLE_RATE_11025
>> + * AFE_PORT_SAMPLE_RATE_12K
>> + * AFE_PORT_SAMPLE_RATE_16K
>> + * AFE_PORT_SAMPLE_RATE_22050
>> + * AFE_PORT_SAMPLE_RATE_24K
>> + * AFE_PORT_SAMPLE_RATE_32K
>> + * AFE_PORT_SAMPLE_RATE_44P1K
>> + * AFE_PORT_SAMPLE_RATE_48K
>> + * AFE_PORT_SAMPLE_RATE_96K
>> + * AFE_PORT_SAMPLE_RATE_192K
>> + * @bit_width: Bit width of the sample.
>> + * Supported values: 16, 24
>> + * @num_channels: Number of channels
>> + * Supported values: 1, 2
>> + * @data_format: Data format supported by the USB
>> + * Supported values: 0
>> + * @reserved: this field must be 0
>> + * @dev_token: device token of actual end USB audio device
>> + * @endian: endianness of this interface
>> + * @service_interval: service interval
>> + **/
>> +struct q6afe_usb_cfg {
>> + u32 cfg_minor_version;
>> + u32 sample_rate;
>> + u16 bit_width;
>> + u16 num_channels;
>> + u16 data_format;
>> + u16 reserved;
>> + u32 dev_token;
>> + u32 endian;
>> + u32 service_interval;
>> +};
>
> this definition looks exactly the same as
> struct afe_param_id_usb_cfg
> ??
>
>
I'll remove some of the params that we aren't utilizing.
Thanks
Wesley Cheng
Powered by blists - more mailing lists