On Thu, 04 Feb 2021 20:39:06 +0100, Olivia Mackintosh wrote:
This allows for N different devices to use the pioneer mixer quirk for setting capture/record type and recording level. The impementation has not changed much with the exception of an additional mask on private_value to allow storing of a device index: DEVICE MASK 0xff000000 GROUP_MASK 0x00ff0000 VALUE_MASK 0x0000ffff
There is perhaps room for removing the duplication in the lookup tables (name, wValue, wIndex) and deriving these. See the code header comment to understand how this can be achieved.
Feedback is very much appreciated as I'm not the most proficient C programmer but am learning as I go.
Signed-off-by: Olivia Mackintosh livvy@base.nu
The patch looks almost good, below are just some nitpicking.
--- a/sound/usb/mixer_quirks.c +++ b/sound/usb/mixer_quirks.c @@ -2602,142 +2602,218 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer) return 0; }
No need to add more empty line here.
+struct snd_djm_device {
- char *name;
- const struct snd_djm_ctl *controls;
- const size_t ncontrols;
The const to ncontrols is almost useless.
+struct snd_djm_ctl {
- const char *name;
- const u16 *options;
- const size_t noptions;
- const u16 default_value;
- const u16 wIndex;
Ditto, const for integers are superfluous.
+static char *snd_djm_get_label_caplevel(u16 wvalue)
This should have const instead.
+{
- switch (wvalue) {
- case 0x0000: return "-19dB";
- case 0x0100: return "-15dB";
- case 0x0200: return "-10dB";
- case 0x0300: return "-5dB";
- default: return "\0"; // 'EINVAL'
Let return NULL for the error instead.
+static char *snd_djm_get_label_cap(u16 wvalue)
Use const.
+{
- switch (wvalue & 0x00ff) {
- case SND_DJM_CAP_LINE: return "Control Tone LINE\0";
The trainling '\0' is superfluous. Ditto in other lines.
- default: return "\0"; // 'EINVAL'
Let's use NULL.
+static char *snd_djm_get_label_pb(u16 wvalue)
Use const.
+{
- switch (wvalue & 0x00ff) {
- case SND_DJM_PB_CH1: return "Ch1\0";
- case SND_DJM_PB_CH2: return "Ch2\0";
- case SND_DJM_PB_AUX: return "Aux\0";
- default: return "\0"; // 'EINVAL'
Like the above.
+static char *snd_djm_get_label(u16 wvalue, u16 windex)
Use const.
+{
- switch (windex) {
- case SND_DJM_WINDEX_CAPLVL: return snd_djm_get_label_caplevel(wvalue);
- case SND_DJM_WINDEX_CAP: return snd_djm_get_label_cap(wvalue);
- case SND_DJM_WINDEX_PB: return snd_djm_get_label_pb(wvalue);
- default: return "\0"; // 'EINVAL';
Use NULL.
-static int snd_pioneer_djm_controls_info(struct snd_kcontrol *kctl, struct snd_ctl_elem_info *info) +static int snd_djm_controls_info(struct snd_kcontrol *kctl,
struct snd_ctl_elem_info *info){
- u16 group_index = kctl->private_value >> SND_PIONEER_DJM_GROUP_SHIFT;
- size_t count;
- unsigned long private_value = kctl->private_value;
- u8 device_idx = (private_value & SND_DJM_DEVICE_MASK) >> SND_DJM_DEVICE_SHIFT;
- u8 ctl_idx = (private_value & SND_DJM_GROUP_MASK) >> SND_DJM_GROUP_SHIFT;
- const struct snd_djm_device device = snd_djm_devices[device_idx];
This will end up copying the whole struct on a stack. Instead, const struct snd_djm_device *device = &snd_djm_devices[device_idx]; and access via device->ncontrols
- name = snd_djm_get_label(
ctl->options[info->value.enumerated.item],ctl->wIndex- );
It's better not to put a line-break after the open parenthesis.
name = snd_djm_get_label(ctl->options[info->value.enumerated.item], ctl->wIndex);
- if (strlen(name) == 0) return -EINVAL;
If the functions above return NULL for errors, this can be simplified as: if (!name) return -EINVAL;
-static int snd_pioneer_djm_controls_update(struct usb_mixer_interface *mixer, u16 group, u16 value) +static int snd_djm_controls_update(struct usb_mixer_interface *mixer,
u8 device_idx, u8 group, u16 value){ int err;
- const struct snd_djm_device *device = &snd_djm_devices[device_idx];
Funnily, here you're doing right :)
+static int snd_djm_controls_create(struct usb_mixer_interface *mixer,
const u8 device_idx){ int err, i;
- const struct snd_pioneer_djm_option_group *group;
- u16 value;
- const struct snd_djm_device device = snd_djm_devices[device_idx];
... but here not.
thanks,
Takashi