[PATCH V2 00/14] Cleanup before adding Scarlett Gen 3 support
Hi Takashi,
Here is version 2 of a set of patches which is some cleanup of the Scarlett Gen 2 mixer driver in preparation for adding Scarlett Gen 3 support.
One review comment I got (from Hin-Tak) was:
40+ patches is a lot, for modifying just one file. I would collapse it all into one and break it up again to under 10, maybe, broadly into "functionally-equivalent re-org", "small isolated bug fixes", "additional functions, not yet used", "hooking up those new functions", etc?
I'm not sure that I agree with that comment -- I tried to follow the Documentation/process/submitting-patches.rst advice of "Separate each logical change into a separate patch" for easy review of the individual pieces, but perhaps I went too far in that direction?
Please let me know if I should combine some of these patches together.
After this set of patches I have a second set of patches almost ready. They add a few features to the Gen 2 support and update the code ready to add Gen 3 support:
ALSA: usb-audio: scarlett2: Add usb_tx/rx functions ALSA: usb-audio: scarlett2: Update initialisation sequence ALSA: usb-audio: scarlett2: Fix 6i6 Gen 2 line out descriptions ALSA: usb-audio: scarlett2: Always enable interrupt polling ALSA: usb-audio: scarlett2: Add "Sync Status" control ALSA: usb-audio: scarlett2: Merge common line in capture strings ALSA: usb-audio: scarlett2: Reformat scarlett2_config_items[] ALSA: usb-audio: scarlett2: Improve device info lookup ALSA: usb-audio: scarlett2: Move info lookup out of init function ALSA: usb-audio: scarlett2: Remove repeated device info comments ALSA: usb-audio: scarlett2: Add scarlett2_vol_ctl_write() helper ALSA: usb-audio: scarlett2: Add mute support ALSA: usb-audio: scarlett2: Allow arbitrary ordering of mux entries ALSA: usb-audio: scarlett2: Split struct scarlett2_ports ALSA: usb-audio: scarlett2: Fix Level Meter control
The final set of patches adds Gen 3 mixer support and then adds support for the new Gen 3 features:
ALSA: usb-audio: scarlett2: Add Gen 3 mixer support ALSA: usb-audio: scarlett2: Add support for "input-other" notify ALSA: usb-audio: scarlett2: Add Gen 3 MSD mode switch ALSA: usb-audio: scarlett2: Move get config above set config ALSA: usb-audio: scarlett2: Allow bit-level access to config ALSA: usb-audio: scarlett2: Add support for Solo and 2i2 Gen 3 ALSA: usb-audio: scarlett2: Add "air" switch support ALSA: usb-audio: scarlett2: Add phantom power switch support ALSA: usb-audio: scarlett2: Add direct monitor support ALSA: usb-audio: scarlett2: Label 18i8 Gen 3 line outputs correctly ALSA: usb-audio: scarlett2: Split up sw_hw_enum_ctl_put() ALSA: usb-audio: scarlett2: Add sw_hw_ctls and mux_ctls ALSA: usb-audio: scarlett2: Update mux controls to allow updates ALSA: usb-audio: scarlett2: Add speaker switching support ALSA: usb-audio: scarlett2: Update get_config to do endian conversion ALSA: usb-audio: scarlett2: Add support for the talkback feature
If you'd like to see the commits so far all at once, please check this branch:
https://github.com/geoffreybennett/scarlett-gen2/commits/scarlett-gen3-for-n...
Thanks, Geoffrey.
Geoffrey D. Bennett (14): ALSA: usb-audio: scarlett2: Remove incorrect S/PDIF comment ALSA: usb-audio: scarlett2: Fix 18i8 Gen 2 PCM Input count ALSA: usb-audio: scarlett2: Coding style improvements ALSA: usb-audio: scarlett2: Remove unused/useless code ALSA: usb-audio: scarlett2: Remove interrupt debug message ALSA: usb-audio: scarlett2: Remove redundant info->button_count ALSA: usb-audio: scarlett2: Rename buttons/interrupts/vol ALSA: usb-audio: scarlett2: Rename struct scarlett2_mixer_data ALSA: usb-audio: scarlett2: Add temp variable for consistency ALSA: usb-audio: scarlett2: Fix data_mutex lock ALSA: usb-audio: scarlett2: Fix scarlett2_*_ctl_put() return values ALSA: usb-audio: scarlett2: Fix union usage in mixer control callbacks ALSA: usb-audio: scarlett2: Don't copy struct scarlett2_config ALSA: usb-audio: scarlett2: Remove hard-coded USB #defines
sound/usb/mixer_scarlett_gen2.c | 425 +++++++++++++++++--------------- 1 file changed, 224 insertions(+), 201 deletions(-)
base-commit: f8fbcdfb0665de60997d9746809e1704ed782bbc
On Sun, 20 Jun 2021 18:46:15 +0200, Geoffrey D. Bennett wrote:
Hi Takashi,
Here is version 2 of a set of patches which is some cleanup of the Scarlett Gen 2 mixer driver in preparation for adding Scarlett Gen 3 support.
One review comment I got (from Hin-Tak) was:
40+ patches is a lot, for modifying just one file. I would collapse it all into one and break it up again to under 10, maybe, broadly into "functionally-equivalent re-org", "small isolated bug fixes", "additional functions, not yet used", "hooking up those new functions", etc?
I'm not sure that I agree with that comment -- I tried to follow the Documentation/process/submitting-patches.rst advice of "Separate each logical change into a separate patch" for easy review of the individual pieces, but perhaps I went too far in that direction?
Please let me know if I should combine some of these patches together.
The split is fine as long as it's done logically, so I took as is.
But, one thing that can be improved at the next time is to sort out fix patches. e.g. you had patches for fixing the mixer field type (int vs enum) and a patch to correct the locking; those are rather independent from the cleanup series and should be applied for the stable backports, too. I didn't add stable at this time because I wasn't sure whether applicable and that's no severe issue, but the process can be better.
Note that the merge window may be closed in this week, so if you want the stuff to be merged, please submit now.
thanks,
Takashi
On Mon, 21 Jun 2021 08:43:51 +0200, Takashi Iwai wrote:
On Sun, 20 Jun 2021 18:46:15 +0200, Geoffrey D. Bennett wrote:
Hi Takashi,
Here is version 2 of a set of patches which is some cleanup of the Scarlett Gen 2 mixer driver in preparation for adding Scarlett Gen 3 support.
One review comment I got (from Hin-Tak) was:
40+ patches is a lot, for modifying just one file. I would collapse it all into one and break it up again to under 10, maybe, broadly into "functionally-equivalent re-org", "small isolated bug fixes", "additional functions, not yet used", "hooking up those new functions", etc?
I'm not sure that I agree with that comment -- I tried to follow the Documentation/process/submitting-patches.rst advice of "Separate each logical change into a separate patch" for easy review of the individual pieces, but perhaps I went too far in that direction?
Please let me know if I should combine some of these patches together.
The split is fine as long as it's done logically, so I took as is.
But, one thing that can be improved at the next time is to sort out fix patches. e.g. you had patches for fixing the mixer field type (int vs enum) and a patch to correct the locking; those are rather independent from the cleanup series and should be applied for the stable backports, too. I didn't add stable at this time because I wasn't sure whether applicable and that's no severe issue, but the process can be better.
Note that the merge window may be closed in this week, so if you want the stuff to be merged, please submit now.
Oh, one more thing: please use the mail thread for a patch set at the next time!
Takashi
On Mon, Jun 21, 2021 at 08:43:51AM +0200, Takashi Iwai wrote:
On Sun, 20 Jun 2021 18:46:15 +0200, Geoffrey D. Bennett wrote:
Hi Takashi,
Here is version 2 of a set of patches which is some cleanup of the Scarlett Gen 2 mixer driver in preparation for adding Scarlett Gen 3 support.
One review comment I got (from Hin-Tak) was:
40+ patches is a lot, for modifying just one file. I would collapse it all into one and break it up again to under 10, maybe, broadly into "functionally-equivalent re-org", "small isolated bug fixes", "additional functions, not yet used", "hooking up those new functions", etc?
I'm not sure that I agree with that comment -- I tried to follow the Documentation/process/submitting-patches.rst advice of "Separate each logical change into a separate patch" for easy review of the individual pieces, but perhaps I went too far in that direction?
Please let me know if I should combine some of these patches together.
The split is fine as long as it's done logically, so I took as is.
But, one thing that can be improved at the next time is to sort out fix patches. e.g. you had patches for fixing the mixer field type (int vs enum) and a patch to correct the locking; those are rather independent from the cleanup series and should be applied for the stable backports, too. I didn't add stable at this time because I wasn't sure whether applicable and that's no severe issue, but the process can be better.
I did not notice any actual bug from using the wrong field type, and nobody reported the locking problem with Gen 2, so I think those are low priority.
The two patches I submitted before for "Read all configuration at init time" are a higher priority for stable as they fix an actual problem that users are encountering: since our last discussion I had another report from a user; they were wondering why their headphones stopped working after changing an unrelated control.
Note that the merge window may be closed in this week, so if you want the stuff to be merged, please submit now.
Thanks, I will submit very soon.
Oh, one more thing: please use the mail thread for a patch set at the next time!
Sorry about that. Right after I sent I noticed that I forgot --thread=shallow.
Thanks, Geoffrey.
participants (2)
-
Geoffrey D. Bennett -
Takashi Iwai