Re: [PATCH v25 00/33] Introduce QC USB SND audio offloading support

[Wesley Cheng <quic_wcheng@xxxxxxxxxxx>]

Hi Pierre,

On 8/26/2024 2:28 AM, Pierre-Louis Bossart wrote:
>> Changelog
>> --------------------------------------------
>> Changes in v25:
>> - Cleanups on typos mentioned within the xHCI layers
>> - Modified the xHCI interrupter search if clients specify interrupter index
>> - Moved mixer_usb_offload into its own module, so that other vendor offload USB
>> modules can utilize it also.
>> - Added support for USB audio devices that may have multiple PCM streams, as
>> previous implementation only assumed a single PCM device.  SOC USB will be
>> able to handle an array of PCM indexes supported by the USB audio device.
>> - Added some additional checks in the QC USB offload driver to check that device
>> has at least one playback stream before allowing to bind
>> - Reordered DT bindings to fix the error found by Rob's bot.  The patch that
>> added USB_RX was after the example was updated.
>> - Updated comments within SOC USB to clarify terminology and to keep it consistent
>> - Added SND_USB_JACK type for notifying of USB device audio connections
> I went through the code and didn't find anything that looked like a
> major blocker. There are still a number of cosmetic things you'd want to
> fix such as using checkpatch.pl --strict --codespell to look for obvious
> style issues and typos, see selection below. git am also complains about
> EOF lines.

Thanks for the consistent reviews.  Will fix the checkpatch errors and mis-spells.  I didn't have codespell added so fixed that and resolved the typos.

Thanks

Wesley Cheng

> Overall this is starting to look good and ready for other reviewers to
> look at.
>
>
>
> WARNING: 'reaquire' may be misspelled - perhaps 'reacquire'?
> #54: FILE: drivers/usb/host/xhci-ring.c:3037:
> + * for non OS owned interrupter event ring. It may drop and reaquire
> xhci->lock
>                                                              ^^^^^^^^
> WARNING: 'compliation' may be misspelled - perhaps 'compilation'?
> #16:
> module compliation added by Wesley Cheng to complete original concept code
>        ^^^^^^^^^^^
> CHECK: Prefer kzalloc(sizeof(*sgt)...) over kzalloc(sizeof(struct
> sg_table)...)
> #105: FILE: drivers/usb/host/xhci-sideband.c:35:
> +	sgt = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
>
> CHECK: struct mutex definition without comment
> #557: FILE: include/linux/usb/xhci-sideband.h:35:
> +	struct mutex			mutex;
>
> WARNING: 'straightfoward' may be misspelled - perhaps 'straightforward'?
> #22:
> straightfoward, as the ASoC components have direct references to the ASoC
> ^^^^^^^^^^^^^^
> CHECK: Unnecessary parentheses around 'card == sdev->card_idx'
> #142: FILE: sound/soc/qcom/qdsp6/q6usb.c:217:
> +	if ((card == sdev->card_idx) &&
> +		(pcm == sdev->ppcm_idx[sdev->num_playback - 1])) {
>
> CHECK: Unnecessary parentheses around 'pcm ==
> sdev->ppcm_idx[sdev->num_playback - 1]'
> #142: FILE: sound/soc/qcom/qdsp6/q6usb.c:217:
> +	if ((card == sdev->card_idx) &&
> +		(pcm == sdev->ppcm_idx[sdev->num_playback - 1])) {
>
> WARNING: 'seqeunces' may be misspelled - perhaps 'sequences'?
> #8:
> seqeunces.  This allows for platform USB SND modules to properly initialize
> ^^^^^^^^^
>
> WARNING: 'exisiting' may be misspelled - perhaps 'existing'?
> #12:
> exisiting parameters.
> ^^^^^^^^^
>
> CHECK: Please use a blank line after function/struct/union/enum declarations
> #1020: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:98:
> +};
> +#define QMI_UAUDIO_STREAM_REQ_MSG_V01_MAX_MSG_LEN 46
>
> CHECK: Please use a blank line after function/struct/union/enum declarations
> #1054: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:132:
> +};
> +#define QMI_UAUDIO_STREAM_RESP_MSG_V01_MAX_MSG_LEN 202
>
> CHECK: Please use a blank line after function/struct/union/enum declarations
> #1081: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:159:
> +};
> +#define QMI_UAUDIO_STREAM_IND_MSG_V01_MAX_MSG_LEN 181
>
> CHECK: Macro argument 'n' may be better as '(n)' to avoid precedence issues
> #100: FILE: sound/usb/mixer_usb_offload.c:19:
> +#define PCM_IDX(n)  (n & 0xffff)
>
> CHECK: Macro argument 'n' may be better as '(n)' to avoid precedence issues
> #101: FILE: sound/usb/mixer_usb_offload.c:20:
> +#define CARD_IDX(n) (n >> 16)
>