mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Christian Ruppert <arc@gmx.li>
To: Will Porter <mrwillporter@gmail.com>
Cc: Takashi Iwai <tiwai@suse.com>, Jaroslav Kysela <perex@perex.cz>,
	linux-sound@vger.kernel.org, linux-kernel@vger.kernel.org,
	Neil Andrews <neil@androos.io>
Subject: Re: [PATCH 0/5] ALSA: usb-audio: Audient iD14 MkI vendor controls
Date: Wed, 28 Oct 2026 00:21:51 +0100	[thread overview]
Message-ID: <auEyD23PT_kpJsrJ@khmer.arcnet> (raw)
In-Reply-To: <CAMZ8TExJThTMZVvbDMVdtXSB-edGAsAnmKYtAtM1CCw0pDvNGw@mail.gmail.com>

Hi Will,

On Mon, Oct 26, 2026 at 04:44:50PM -0500, Will Porter wrote:
> On Mon, Oct 26, 2026 at 07:36:24PM +0100, Christian Ruppert wrote:
> > I took a look at this question and it seems like we can share the majority
> > of code in the EVO driver by complementing it with the following features:
> > - variable width read/write
> > - caching of write-only values
> > - enum data type
> 
> Thanks for looking into this so closely, and for the fixes. I like
> the idea of one driver, and I'd like to find out whether the iD line fits.
> 
> Your TLV in patch 4/4 is the same one I use for the iD14 matrix: same
> unit 0x3C, s16 in 1/256 dB, -128 to +6 dB. That made me wonder whether
> the iD14 matrix could use your MIXER type almost as it is.

Agreed. If my understanding is correct, this type could be reused without
any modifications. The others would have to be newly defined due to
different unit IDs/control selectors. BTW: the mixer matrix seems to be
identical also for the EVO8 so chances are it's the same for most audient
devices and we could simply use a width and height parameter to generate
it in a loop.

> Its channel
> number is input * 4 + output, so Main L/R and Cue L/R of each input
> are adjacent, like your stereo pairs. Could we expose it as 28
> controls with two values each, rather than my 56 single-value ones?
> An input's level and pan in a mix set its L and R values together,
> so a pair seems a natural fit. Does that look right to you?

This is exactly what the stereo pairs do for the EVO. For example, mic 1
shows up as one stereo pair which can be either recorded on both channels
(paned between L and R) or exclusively on the left one if it's connected
to a stereo mic. Same for mic 2 (which can be used as the right channel
of a stereo mic or an independently paned mic). The same principle applies
for the L and R outputs of the monitor and USB main out where it seems a
bit more quirky but is coherent with the Mics. That said, the existing
table system would also allow for individual channels or grouping them in
"mini-matrices of four" but neither seemed right when looking at them in
alsamixer and so I stuck with one stereo mixer for each (mono) input
channel, similar to most mixing consoles.

> > The only part which seems more problematic is probably FU12 which looks
> > like it requires some custom logic and would still need actual code.
> 
> Agreed. I'd keep FU 12 as its own code.
> 
> A few questions about the rest:
> 
> 1. Read before write. As I understand it, the put handlers in
>    mixer_evo.c read the value first to see if it changed. On the iD14,
>    GET stalls for mic boost, input polarity, optical mode and the iD
>    key function, so a put that reads first would fail there. Talkback
>    source always reads 0, so a read would hide real changes. Could
>    these write-only controls skip GET in both get and put, with get
>    returning the cached value? And could every put still send the
>    value, even when it matches the cache, so that "alsactl restore"
>    reaches a device that was reset?

My view of the WO implementation would be to add three flags: cache, init
and force_write:
- If cache is set, the values will be cached and the channel not marked as
  volatile. No need to read back actual values from USB before a write in
  this case. This could also be used in the EVO for the mixer matrix which
  doesn't have physical controls that can change values while the driver
  doesn't look. Didn't implement it to not complicate the driver with
  unnecessary caching but if we need caching anyway we should use it.
- If init is set, the driver will initialize the control at probe. The EVO
  mixer matrix' initial read returns a wrong value so initialization would
  be great. Also not implemented today because there's no need to complicate
  the driver for a control which must be fiddled with anyway before it is
  useful but if we have the feature we should use it.
- If force_write is set, the driver will always write the value, no matter
  if a cached (or previously read) value is identical. IMHO this is mainly
  required for broken or quirky controls but I'm a bit undecided if it
  would really hurt to simply make this behaviour the default for
  everything.

> 2. The cache. My driver writes a known value at probe (off or none,
>    and for optical mode its measured power-on mode, S/PDIF) and
>    updates the cache only after a full-length write. If the probe
>    write fails, an enum reads "Unknown" and a switch reads off. Would
>    that approach work for you?

This is very much in line with what I was thinking, see above.

> 3. Control entries. The iD14 table uses 16 unit/selector pairs and
>    explicit control names, such as "Main Out L Source Playback Enum".
>    It also needs volumes in 1 dB steps (0..128, stored as s16
>    1/256 dB), a boost switch that writes 0x0a00 for on, and enums
>    that map items to device values, plus an "Other" item that only
>    get reports, for values outside the list. Would it make sense to
>    move the unit, selector and an optional full name into each control
>    entry, and add the new value formats (1 dB volumes, boost, enum)
>    as ctypes? Or would you structure it differently?

This is several questions in one, let me try not to forget an answer ;)
- The intent of the control types was to associate unit/selector pairs
  with data types. I would add a new ctype for each unit/selector pair
  but am open to move unit/selector to the controls and only leave the
  data type definition in ctype (which will then become rather a dtype)
  if it significantly reduces repetition. I'm afraid it might just shift
  repetition to the controls table, though. Btw: I like the unit/selector
  macros, will introduce this also for the EVO.
- I don't have a preference between explicit control names and the
  basename/suffix system the EVO driver currently uses. If you prefer we
  can simply replace the basename with a full name and remove the flags
  field in the controls.
- The boost switch writing 0x0 for 0dB gain and 0x0a00 for 10dB gain looks
  suspiciously like a volume control between 0dB and 10dB * 256steps/dB.
  Does this control only accept these two values or can we simply
  implement it as a standard volume type controlling a mic pre-amp? If
  it's only 0xa00 or 0x0 I would suggest to slightly redefine the boolean
  type to use the max value for true and the min value for false which
  would also be an elegant way to remove the inversion flag.
- We definitely need a new snd_evo_type entry and get/set functions for
  enums, up to you to define those in the most suitable manner for the
  id14.

> 4. Checks. My helper returns -EIO on a short read or write, before
>    it decodes a value or updates the cache. My matrix put also rejects
>    values outside -128..+6 dB, and get clamps what it reads back.
>    Could the shared code keep these checks?

Sure, error management is always good to have. I never encountered any
short reads/writes or out of range reads with the EVO so I didn't add any
logic for this until now. Also, for the controls I checked, the EVO
cleanly rejects out-of-range writes but if the id14 doesn't we should add
those checks as well.

> I don't have a firm view yet on which way is better. If the iD14
> fits the table without many special cases, one driver seems clearly
> worth it. If it needs a lot of per-device branches, two smaller files
> might be easier to read and review. What do you think, now that you
> have seen what the iD14 needs?

I think we can put everything except for FU12 into the tables and the EVO
driver would even benefit from the added features (see above).

> If we do combine them, the order I imagined is: your four fixes first,
> then the rename to Audient, then the generic changes, then the iD14
> tables on top as v2 of my series.

Agreed. Renaming the file first might turn out to be annoying because it's
the one thing which might create rebase conflicts we'll have to deal with
while we work on this but it's probably preferable to keep as much of the
commit history as possible after the rename.

> In that case, would you prefer to do the rename and the generic part
> yourself?

I suppose we can cut one testing loop if I do the part of the generic part
that touches the EVO myself so I can do that, no problem.

> The rename would also touch MAINTAINERS; how would you like
> to handle review of the iD14 parts?

I would definitely prefer to list both of us as maintainers. Practically
all code will be shared and I can't speak for the id14 in any way.

> I can write the iD14 parts and
> test them on my MkI. I don't have an EVO 4, so would you be able to
> test that side? And which base should I work against, sound for-next
> once your fixes are in?

It shouldn't make a huge difference because we're rather independent on
this file. I'll probably prepare everything on 7.3 and then rebase it onto
sound/master to test it there and submit (that's what I did until now, but
for-next is also fine). I suggest I submit an RFC with the generic part
that you can apply locally to work against (and potentially adjust) before
you submit everything in a final series which, if everything goes well, I
only need to retest and tag.

> I'm also not sure how a combined change
> should reach Takashi: as one series, or as two with a stated
> dependency.

  parent reply	other threads:[~2026-10-27 23:22 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-19 23:01 Will Porter
2026-10-19 23:01 ` [PATCH 1/5] ALSA: usb-audio: Give the Audient iD14 MkI its own mixer map Will Porter
2026-10-19 23:01 ` [PATCH 2/5] ALSA: usb-audio: Add Audient iD14 MkI monitor and routing controls Will Porter
2026-10-19 23:01 ` [PATCH 3/5] ALSA: usb-audio: Add Audient iD14 MkI mixer matrix Will Porter
2026-10-19 23:01 ` [PATCH 4/5] ALSA: usb-audio: Add Audient iD14 MkI write-only controls Will Porter
2026-10-19 23:01 ` [PATCH 5/5] ALSA: usb-audio: Replace the Audient iD14 MkI FU 12 control Will Porter
2026-10-20 13:10 ` [PATCH 0/5] ALSA: usb-audio: Audient iD14 MkI vendor controls Takashi Iwai
2026-10-26 18:36 ` Christian Ruppert
2026-10-26 21:44   ` Will Porter
2026-10-27  6:35     ` Takashi Iwai
2026-10-27 23:21     ` Christian Ruppert [this message]
2026-10-28  2:36       ` Will Porter

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=auEyD23PT_kpJsrJ@khmer.arcnet \
    --to=arc@gmx.li \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sound@vger.kernel.org \
    --cc=mrwillporter@gmail.com \
    --cc=neil@androos.io \
    --cc=perex@perex.cz \
    --cc=tiwai@suse.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®