[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <2133dfd6-40f4-41cb-85ea-63fd9467a75f@quicinc.com>
Date: Wed, 28 Aug 2024 12:00:14 -0700
From: Wesley Cheng <quic_wcheng@...cinc.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
<srinivas.kandagatla@...aro.org>, <mathias.nyman@...el.com>,
<perex@...ex.cz>, <conor+dt@...nel.org>, <dmitry.torokhov@...il.com>,
<corbet@....net>, <broonie@...nel.org>, <lgirdwood@...il.com>,
<tiwai@...e.com>, <krzk+dt@...nel.org>, <Thinh.Nguyen@...opsys.com>,
<bgoswami@...cinc.com>, <robh@...nel.org>,
<gregkh@...uxfoundation.org>
CC: <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-sound@...r.kernel.org>, <linux-usb@...r.kernel.org>,
<linux-input@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-doc@...r.kernel.org>, <alsa-devel@...a-project.org>
Subject: Re: [PATCH v25 00/33] Introduce QC USB SND audio offloading support
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)
>
Powered by blists - more mailing lists