Re: [PATCH v4 08/32] ASoC: qcom: qdsp6: Introduce USB AFE port to q6dsp

[Wesley Cheng]

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