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
