Received-SPF: pass (google.com: domain of linux-kernel+bounces-162213-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 29 Apr 2024 14:11:41 +0200
Message-ID: <87jzkgpdvm.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: ManuLinares <mbarriolinares@gmail.com>
Cc: alsa-devel@alsa-project.org,
	Jaroslav Kysela <perex@perex.cz>,
	Takashi Iwai <tiwai@suse.com>,
	dengxiang <dengxiang@nfschina.com>,
	Geraldo Nascimento <geraldogabriel@gmail.com>,
	Max McCarthy <mmccarthy@mcintoshlabs.com>,
	WhaleChang <whalechang@google.com>,
	Lukasz Tyl <ltyl@hem-e.com>,
	Jeremie Knuesel <knuesel@gmail.com>,
	Alexander Tsoy <alexander@tsoy.me>,
	Jussi Laako <jussi@sonarnerd.net>,
	linux-sound@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ALSA: usb-audio: Add sampling rates support for Mbox3
In-Reply-To: <20240428005733.202978-1-mbarriolinares@gmail.com>
References: <20240428005733.202978-1-mbarriolinares@gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII

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@gmail.com>
(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