Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <eb1c679b-f50b-1f20-c7c8-da3f4857bec1@linux.intel.com>
Date:   Tue, 25 Jul 2023 10:27:00 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.13.0
Subject: Re: [PATCH v4 08/32] ASoC: qcom: qdsp6: Introduce USB AFE port to
 q6dsp
Content-Language: en-US
To:     Wesley Cheng <quic_wcheng@quicinc.com>, agross@kernel.org,
        andersson@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        catalin.marinas@arm.com, will@kernel.org, mathias.nyman@intel.com,
        gregkh@linuxfoundation.org, lgirdwood@gmail.com,
        broonie@kernel.org, perex@perex.cz, tiwai@suse.com,
        srinivas.kandagatla@linaro.org, bgoswami@quicinc.com,
        Thinh.Nguyen@synopsys.com
Cc:     linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-usb@vger.kernel.org, alsa-devel@alsa-project.org,
        quic_jackp@quicinc.com, oneukum@suse.com, albertccwang@google.com,
        o-takashi@sakamocchi.jp
References: <20230725023416.11205-1-quic_wcheng@quicinc.com>
 <20230725023416.11205-9-quic_wcheng@quicinc.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <20230725023416.11205-9-quic_wcheng@quicinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


> +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?


> +/* 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.


> +	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 ...

> +	}
> +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
??