All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Geoffrey D. Bennett" <g@b4.vu>
To: Takashi Iwai <tiwai@suse.de>
Cc: Daniel Sales <daniel.sales.z@gmail.com>,
	alsa-devel@alsa-project.org,
	Vladimir Sadovnikov <sadko4u@gmail.com>,
	Markus Schroetter <project.m.schroetter@gmail.com>,
	Alex Fellows <alex.fellows@gmail.com>
Subject: Re: [PATCH 0/2] ALSA: usb-audio: scarlett2: Read all configuration at init time
Date: Tue, 8 Jun 2021 05:21:16 +0930	[thread overview]
Message-ID: <20210607195116.GA182875@m.b4.vu> (raw)
In-Reply-To: <s5h1r9ef895.wl-tiwai@suse.de>

On Mon, Jun 07, 2021 at 09:23:34AM +0200, Takashi Iwai wrote:
> On Sun, 06 Jun 2021 16:16:44 +0200,
> Geoffrey D. Bennett wrote:
> > 
> > These two patches add support for reading the mixer volumes and mux
> > configuration from the hardware when the driver is initialising.
> > 
> > Previously the ALSA volume controls were initialised to zero and the
> > mux configuration set to a fixed default instead of being initialised
> > to match the hardware state.
> > 
> > The ALSA controls for the Scarlett Gen 2 interfaces should now always
> > be in sync with the hardware. Thanks to Vladimir Sadovnikov for
> > figuring out how to do this.
> > 
> > Takashi, if these pass your review, I believe that they are
> > appropriate for:
> > #Cc: stable@vger.kernel.org
> 
> Well, in general, having a proper fixed value for the initial mixer
> value is the right thing, which is a part of the driver's role.
> Though, in snd-usb-audio, we don't set up the initial values just
> because of laziness; since the topology in USB audio is variable per
> device and often hard to parse correctly, it's difficult to determine
> the suitable initial values, hence we leave untouched.  So, in that
> sense, setting the zero isn't wrong, rather safer, per se.
> 
> However, Scarlett 2 seems to want to be different; it has already some
> initialization code to read the existing configs.  So this change
> sounds more or less acceptable.  But it's questionable whether it's
> really for stable as a "fix".

For the Scarlett devices, the sensible (but not all good)
initialisation options are:

1. Don't load configuration from hardware so the ALSA controls show
default values not in sync with hardware. This is a bad user
experience.

2. Reset hardware configuration to hard-coded defaults so ALSA
controls will be in sync with hardware. This is a really bad user
experience as it is common for the device to be configured using the
proprietary vendor-supplied software and then connected to Linux with
the expectation that the configuration is kept.

3. Load configuration from hardware so ALSA controls are in sync with
hardware. This is a good user experience.

Before these two patches, it was option (1) for the mix/mux controls
and option (3) for the remainder of the controls. I would call this
definitely not sensible behaviour.

I got many queries as to the apparently strange workings of the driver
due to this; certainly many users besides myself considered the
original behaviour a bug so it does fit the rule "It must fix a real
bug that bothers people" from
Documentation/process/stable-kernel-rules.rst

You might consider that it does not meet the "It must be obviously
correct and tested" rule though, and I could agree with that. Perhaps
leave it out from stable for now and revisit later after we get more
test results? Is there a particular amount of time or number of test
success reports that you would consider sufficient?

> In anyway, please fix the bug ktest bot spotted, the missing endian
> conversions and resubmit.

Thanks, I have resubmitted.

Regards,
Geoffrey.

      parent reply	other threads:[~2021-06-07 19:52 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-06 14:16 [PATCH 0/2] ALSA: usb-audio: scarlett2: Read all configuration at init time Geoffrey D. Bennett
2021-06-06 14:16 ` [PATCH 1/2] ALSA: usb-audio: scarlett2: Read mixer volumes " Geoffrey D. Bennett
2021-06-06 14:32   ` Markus
2021-06-07  5:23   ` kernel test robot
2021-06-07  5:23     ` kernel test robot
2021-06-06 14:17 ` [PATCH 2/2] ALSA: usb-audio: scarlett2: Read mux " Geoffrey D. Bennett
2021-06-06 14:33   ` Markus
2021-06-07  7:23 ` [PATCH 0/2] ALSA: usb-audio: scarlett2: Read all configuration " Takashi Iwai
     [not found]   ` <3c7a458a-a5cd-08e4-a462-293c5bf633ec@gmail.com>
2021-06-07 15:12     ` Takashi Iwai
2021-06-07 17:18       ` Markus Schroetter
2021-06-07 19:51   ` Geoffrey D. Bennett [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210607195116.GA182875@m.b4.vu \
    --to=g@b4.vu \
    --cc=alex.fellows@gmail.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=daniel.sales.z@gmail.com \
    --cc=project.m.schroetter@gmail.com \
    --cc=sadko4u@gmail.com \
    --cc=tiwai@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.