Re: [PATCH] ALSA: usb-audio: Add sampling rates support for Mbox3

[Takashi Iwai]

On Sun, 28 Apr 2024 02:57:29 +0200,
ManuLinares wrote:
> 
> This adds support for all sample rates supported by the hardware,
> Digidesign Mbox 3 supports: {44100, 48000, 88200, 96000}
> 
> Fixes syncing clock issues that presented as pops. To test this, without
> this patch playing 440hz tone produces pops.
> 
> Clock is now synced between playback and capture interfaces so no more
> latency drift issue when using pipewire pro-profile.
> (https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/3900)
> 
> Signed-off-by: ManuLinares <mbarriolinares@xxxxxxxxx>
(snip)
> -	dev_dbg(&dev->dev, "device initialised!\n");
> +	dev_dbg(&dev->dev, "MBOX3: device initialised!\n");
>  
>  	err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
>  		&dev->descriptor, sizeof(dev->descriptor));
>  	config = dev->actconfig;
> -	if (err < 0)
> -		dev_dbg(&dev->dev, "error usb_get_descriptor: %d\n", err);
> +	if (err < 0) 

You put a tailing sparce superfluously.

> +		dev_dbg(&dev->dev, "MBOX3: error usb_get_descriptor: %d\n", err);
>  
>  	err = usb_reset_configuration(dev);
> -	if (err < 0)
> -		dev_dbg(&dev->dev, "error usb_reset_configuration: %d\n", err);
> -	dev_dbg(&dev->dev, "mbox3_boot: new boot length = %d\n",
> +	if (err < 0) 

Ditto.  Try to run checkpatch.pl at the next time; it should warn you
such errors.

(snip)
> +static void mbox3_set_format_quirk(struct snd_usb_substream *subs,
> +									const struct audioformat *fmt)
> +{
> +	// Set rate only for one interface
> +	//u8 iface = subs->data_endpoint->iface;
> +	//if (iface != 2) return;
> +
> +	u8 buffer[4] = {0};
> +	u32 new_rate = subs->data_endpoint->cur_rate;

Let's to be classic: try to put the variable definitions at the begin
of the function.

(snip)
> +	__le32 set_rate = cpu_to_le32(new_rate);

Ditto.

(snip)
> +	// Check whether the change was successful
> +	buffer[0] = 0; buffer[1] = 0; buffer[2] = 0; buffer[3] = 0;

You can use memset().  (Or if it were a __le32 variable, it can be
simply zero assignment.)

> +	snd_usb_ctl_msg(subs->dev,
> +					usb_sndctrlpipe(subs->dev, 0),
> +					0x01, 0x21 | USB_DIR_IN, 0x0100, 0x8101, &buffer, 4);
> +	//set_rate = *(int *)buffer;
> +	set_rate = le32_to_cpu(*(u32 *)buffer);

Strictly speaking, this won't work always as no alignment is
guaranteed for char[4].  If you need to pass/receive a 4-byte integer,
better to use a __le32 variable instead.  For a one-byte temporary
buffer, you can use another variable additionally.

Could you resubmit with those corrections?


thanks,

Takashi