Re: [PATCH v3 1/1] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk

From: Takashi Iwai <tiwai@suse.de>
To: Olivia Mackintosh <livvy@base.nu>
Cc: alsa-devel@alsa-project.org, fabian@lesniak-it.de
Subject: Re: [PATCH v3 1/1] ALSA: usb-audio: Add DJM750 to Pioneer mixer quirk
Date: Thu, 04 Feb 2021 22:33:02 +0100	[thread overview]
Message-ID: <s5h35yb5xip.wl-tiwai@suse.de> (raw)
In-Reply-To: <20210204193906.20716-2-livvy@base.nu>

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