Re: wskbd_set_mixervolume
On Thu, Feb 10, 2022 at 04:53:59PM +0100, Anton Lindqvist wrote: > On Wed, Feb 09, 2022 at 09:13:58AM +0100, Alexandre Ratchov wrote: > > On Tue, Feb 08, 2022 at 06:59:39PM +0100, Anton Lindqvist wrote: > > > On Tue, Feb 08, 2022 at 07:32:38AM +0100, Alexandre Ratchov wrote: > > > > On Mon, Feb 07, 2022 at 06:55:21PM +0100, Anton Lindqvist wrote: > > > > > On Mon, Feb 07, 2022 at 11:21:43AM +0100, Alexandre Ratchov wrote: > > > > > > On Sun, Feb 06, 2022 at 08:40:35AM +0100, Anton Lindqvist wrote: > > > > > > > > > > > > > > Polished diff. I'm omitting a necessary refactoring commit making > > > > > > > audio_attach_mi() accept a new cookie argument. > > > > > > > > > > > > > > > > > > > I'm not sure to understand the need for the wskbd_audio structure. > > > > > > Couldn't we just store the cookie in audio and wskbd softc > > > > > > structures, > > > > > > then pass it in the wskbd_set_mixervolume_sc() calls ? > > > > > > > > > > Due to the device caching the data must be stored in either the audio > > > > > or > > > > > wskbd softc and I don't want to expose the softc structures so I ended > > > > > up introducing wskbd_audio. Dropping the caching would probably make > > > > > it > > > > > possible to only pass down the cookie to wskbd_set_mixervolume_sc() > > > > > and > > > > > always do the audio device lookup. > > > > > > > > > > Is anyone in favor of this approach? I achieves the expected behavior > > > > > in > > > > > my opinion. > > > > > > > > IMHO, handling the volume keys this way won't work in the general > > > > case. But for the short term we've no other options, have we? > > > > > > > > AFAICS, you're fixing a concrete use-case by tweaking what already > > > > exists, this won't make things more broken than they already are. I'm > > > > OK with it. > > > > > > Here's the complete diff including adding a cookie argument to > > > audio_attach_mi(). > > > > Is the caching necessary? device_lookup() seems cheap and there are at > > most two devices in most cases. Keeping the code minimal especially on > > rare and non-performace-critical code-paths would be nice. > > > > If you choose to drop the caching, this would allow to just add a a > > new "cookie" argument to wskbd_set_mixervolume(), similarly to what > > you did for audio_attach_mi() > > Sure, I ended up adding a new function after all since the > wskbd_set_mixervolume() prototype is not present in any header at this > point. > ok ratchov
Re: wskbd_set_mixervolume
On Wed, Feb 09, 2022 at 09:13:58AM +0100, Alexandre Ratchov wrote:
> On Tue, Feb 08, 2022 at 06:59:39PM +0100, Anton Lindqvist wrote:
> > On Tue, Feb 08, 2022 at 07:32:38AM +0100, Alexandre Ratchov wrote:
> > > On Mon, Feb 07, 2022 at 06:55:21PM +0100, Anton Lindqvist wrote:
> > > > On Mon, Feb 07, 2022 at 11:21:43AM +0100, Alexandre Ratchov wrote:
> > > > > On Sun, Feb 06, 2022 at 08:40:35AM +0100, Anton Lindqvist wrote:
> > > > > >
> > > > > > Polished diff. I'm omitting a necessary refactoring commit making
> > > > > > audio_attach_mi() accept a new cookie argument.
> > > > > >
> > > > >
> > > > > I'm not sure to understand the need for the wskbd_audio structure.
> > > > > Couldn't we just store the cookie in audio and wskbd softc structures,
> > > > > then pass it in the wskbd_set_mixervolume_sc() calls ?
> > > >
> > > > Due to the device caching the data must be stored in either the audio or
> > > > wskbd softc and I don't want to expose the softc structures so I ended
> > > > up introducing wskbd_audio. Dropping the caching would probably make it
> > > > possible to only pass down the cookie to wskbd_set_mixervolume_sc() and
> > > > always do the audio device lookup.
> > > >
> > > > Is anyone in favor of this approach? I achieves the expected behavior in
> > > > my opinion.
> > >
> > > IMHO, handling the volume keys this way won't work in the general
> > > case. But for the short term we've no other options, have we?
> > >
> > > AFAICS, you're fixing a concrete use-case by tweaking what already
> > > exists, this won't make things more broken than they already are. I'm
> > > OK with it.
> >
> > Here's the complete diff including adding a cookie argument to
> > audio_attach_mi().
>
> Is the caching necessary? device_lookup() seems cheap and there are at
> most two devices in most cases. Keeping the code minimal especially on
> rare and non-performace-critical code-paths would be nice.
>
> If you choose to drop the caching, this would allow to just add a a
> new "cookie" argument to wskbd_set_mixervolume(), similarly to what
> you did for audio_attach_mi()
Sure, I ended up adding a new function after all since the
wskbd_set_mixervolume() prototype is not present in any header at this
point.
diff --git sys/arch/hppa/gsc/harmony.c sys/arch/hppa/gsc/harmony.c
index 033a3d2c356..d5748fac4b1 100644
--- sys/arch/hppa/gsc/harmony.c
+++ sys/arch/hppa/gsc/harmony.c
@@ -249,7 +249,7 @@ harmony_attach(parent, self, aux)
if ((rev & CS4215_REV_VER) >= CS4215_REV_VER_E)
sc->sc_hasulinear8 = 1;
- audio_attach_mi(&harmony_sa_hw_if, sc, &sc->sc_dv);
+ audio_attach_mi(&harmony_sa_hw_if, sc, NULL, &sc->sc_dv);
timeout_set(&sc->sc_acc_tmo, harmony_acc_tmo, sc);
sc->sc_acc_num = 0xa5a5a5a5;
diff --git sys/arch/luna88k/cbus/nec86.c sys/arch/luna88k/cbus/nec86.c
index b6516cc4888..dc4e2069ddd 100644
--- sys/arch/luna88k/cbus/nec86.c
+++ sys/arch/luna88k/cbus/nec86.c
@@ -237,7 +237,7 @@ nec86_attachsubr(struct nec86_softc *sc)
if (sc->sc_attached == 0) {
printf(": %s\n", boardname[ysc->model]);
- audio_attach_mi(&nec86_hw_if, ysc, &ysc->sc_dev);
+ audio_attach_mi(&nec86_hw_if, ysc, NULL, &ysc->sc_dev);
sc->sc_attached = 1;
}
}
diff --git sys/arch/macppc/dev/aoa.c sys/arch/macppc/dev/aoa.c
index a2ca8e10a95..638da4b1669 100644
--- sys/arch/macppc/dev/aoa.c
+++ sys/arch/macppc/dev/aoa.c
@@ -134,7 +134,7 @@ aoa_defer(struct device *dev)
{
struct aoa_softc *sc = (struct aoa_softc *)dev;
- audio_attach_mi(&aoa_hw_if, sc, &sc->sc_dev);
+ audio_attach_mi(&aoa_hw_if, sc, NULL, &sc->sc_dev);
deq_reset(sc);
}
diff --git sys/arch/macppc/dev/awacs.c sys/arch/macppc/dev/awacs.c
index 8ea7d4869a0..adcafcf79fb 100644
--- sys/arch/macppc/dev/awacs.c
+++ sys/arch/macppc/dev/awacs.c
@@ -340,7 +340,7 @@ awacs_attach(struct device *parent, struct device *self,
void *aux)
awacs_halt_input(sc);
printf("\n");
- audio_attach_mi(&awacs_hw_if, sc, &sc->sc_dev);
+ audio_attach_mi(&awacs_hw_if, sc, NULL, &sc->sc_dev);
}
u_int
diff --git sys/arch/macppc/dev/daca.c sys/arch/macppc/dev/daca.c
index 070b839c99a..91b078dd254 100644
--- sys/arch/macppc/dev/daca.c
+++ sys/arch/macppc/dev/daca.c
@@ -154,7 +154,7 @@ daca_defer(struct device *dev)
/* XXX If i2c ha
Re: wskbd_set_mixervolume
On Tue, Feb 08, 2022 at 06:59:39PM +0100, Anton Lindqvist wrote: > On Tue, Feb 08, 2022 at 07:32:38AM +0100, Alexandre Ratchov wrote: > > On Mon, Feb 07, 2022 at 06:55:21PM +0100, Anton Lindqvist wrote: > > > On Mon, Feb 07, 2022 at 11:21:43AM +0100, Alexandre Ratchov wrote: > > > > On Sun, Feb 06, 2022 at 08:40:35AM +0100, Anton Lindqvist wrote: > > > > > > > > > > Polished diff. I'm omitting a necessary refactoring commit making > > > > > audio_attach_mi() accept a new cookie argument. > > > > > > > > > > > > > I'm not sure to understand the need for the wskbd_audio structure. > > > > Couldn't we just store the cookie in audio and wskbd softc structures, > > > > then pass it in the wskbd_set_mixervolume_sc() calls ? > > > > > > Due to the device caching the data must be stored in either the audio or > > > wskbd softc and I don't want to expose the softc structures so I ended > > > up introducing wskbd_audio. Dropping the caching would probably make it > > > possible to only pass down the cookie to wskbd_set_mixervolume_sc() and > > > always do the audio device lookup. > > > > > > Is anyone in favor of this approach? I achieves the expected behavior in > > > my opinion. > > > > IMHO, handling the volume keys this way won't work in the general > > case. But for the short term we've no other options, have we? > > > > AFAICS, you're fixing a concrete use-case by tweaking what already > > exists, this won't make things more broken than they already are. I'm > > OK with it. > > Here's the complete diff including adding a cookie argument to > audio_attach_mi(). Is the caching necessary? device_lookup() seems cheap and there are at most two devices in most cases. Keeping the code minimal especially on rare and non-performace-critical code-paths would be nice. If you choose to drop the caching, this would allow to just add a a new "cookie" argument to wskbd_set_mixervolume(), similarly to what you did for audio_attach_mi()
Re: wskbd_set_mixervolume
do? */
- audio_attach_mi(&snapper_hw_if, sc, &sc->sc_dev);
+ audio_attach_mi(&snapper_hw_if, sc, NULL, &sc->sc_dev);
/* kiic_setmode(sc->sc_i2c, I2C_STDSUBMODE); */
snapper_init(sc);
diff --git sys/arch/macppc/dev/tumbler.c sys/arch/macppc/dev/tumbler.c
index 6c522a10585..4e2b1fc321a 100644
--- sys/arch/macppc/dev/tumbler.c
+++ sys/arch/macppc/dev/tumbler.c
@@ -299,7 +299,7 @@ tumbler_defer(struct device *dev)
/* XXX If i2c has failed to attach, what should we do? */
- audio_attach_mi(&tumbler_hw_if, sc, &sc->sc_dev);
+ audio_attach_mi(&tumbler_hw_if, sc, NULL, &sc->sc_dev);
tumbler_init(sc);
}
diff --git sys/arch/sparc64/dev/ce4231.c sys/arch/sparc64/dev/ce4231.c
index 5e97b0fb228..970d759ba3e 100644
--- sys/arch/sparc64/dev/ce4231.c
+++ sys/arch/sparc64/dev/ce4231.c
@@ -268,7 +268,7 @@ ce4231_attach(struct device *parent, struct device *self,
void *aux)
printf(": nvaddrs %d\n", ea->ea_nvaddrs);
- audio_attach_mi(&ce4231_sa_hw_if, sc, &sc->sc_dev);
+ audio_attach_mi(&ce4231_sa_hw_if, sc, NULL, &sc->sc_dev);
/* Enable mode 2. */
ce4231_write(sc, SP_MISC_INFO, ce4231_read(sc, SP_MISC_INFO) | MODE2);
diff --git sys/dev/audio.c sys/dev/audio.c
index ec52ee6ef01..f2b40637771 100644
--- sys/dev/audio.c
+++ sys/dev/audio.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include/* struct wskbd_audio */
#include "audio.h"
#include "wskbd.h"
@@ -96,6 +97,8 @@ struct wskbd_vol
#define WSKBD_MUTE_DISABLE 2
#define WSKBD_MUTE_ENABLE 3
};
+
+int wskbd_set_mixervolume_unit(int, long, long);
#endif
/*
@@ -112,6 +115,7 @@ struct mixer_ev {
struct audio_softc {
struct device dev;
struct audio_hw_if *ops;/* driver funcs */
+ void *cookie; /* wskbd cookie */
void *arg; /* first arg to driver funcs */
int mode; /* bitmask of AUMODE_* */
int quiesce;/* device suspended */
@@ -1236,6 +1240,7 @@ audio_attach(struct device *parent, struct device *self,
void *aux)
}
#endif
sc->ops = ops;
+ sc->cookie = sa->cookie;
sc->arg = arg;
#if NWSKBD > 0
@@ -1467,13 +1472,14 @@ audio_submatch(struct device *parent, void *match, void
*aux)
}
struct device *
-audio_attach_mi(struct audio_hw_if *ops, void *arg, struct device *dev)
+audio_attach_mi(struct audio_hw_if *ops, void *arg, void *cookie, struct
device *dev)
{
struct audio_attach_args aa;
aa.type = AUDIODEV_TYPE_AUDIO;
aa.hwif = ops;
aa.hdl = arg;
+ aa.cookie = cookie;
/*
* attach this driver to the caller (hardware driver), this
@@ -2452,10 +2458,6 @@ wskbd_mixer_init(struct audio_softc *sc)
};
int i;
- if (sc->dev.dv_unit != 0) {
- DPRINTF("%s: not configuring wskbd keys\n", DEVNAME(sc));
- return;
- }
for (i = 0; i < sizeof(spkr_names) / sizeof(spkr_names[0]); i++) {
if (wskbd_initvol(sc, &sc->spkr,
spkr_names[i].cn, spkr_names[i].dn))
@@ -2566,13 +2568,55 @@ wskbd_set_mixermute(long mute, long out)
return 0;
}
+int
+wskbd_set_mixervolume_sc(struct wskbd_audio *audio, long dir, long out)
+{
+ if (audio->dev != NULL) {
+ if ((audio->dev->dv_flags & DVF_ACTIVE) == 0) {
+ /* Audio device gone, fallback to audio0. */
+ device_unref(audio->dev);
+ audio->dev = NULL;
+ audio->unit = 0;
+ }
+ } else if (audio->cookie != NULL) {
+ void *cookie;
+ int i;
+
+ cookie = audio->cookie;
+ audio->cookie = NULL;
+ for (i = 0; i < audio_cd.cd_ndevs; i++) {
+ struct audio_softc *sc;
+
+ sc = (struct audio_softc *)device_lookup(&audio_cd, i);
+ if (sc == NULL)
+ continue;
+ if (sc->cookie != cookie) {
+ device_unref(&sc->dev);
+ continue;
+ }
+
+ audio->dev = (struct device *)sc;
+ audio->unit = i;
+ break;
+ }
+ }
+
+ return wskbd_set_mixervolume_unit(audio->unit, dir, out);
+}
+
int
wskbd_set_mixervolume(long dir, long out)
+{
+ return wskbd_set_mixervolume_unit(0, dir, out);
+}
+
+int
+wskbd_set_mixervolume_unit(int unit, long dir, long out)
{
struct audio_softc *sc;
struct wskbd_vol *vol;
- sc = (struct audio_softc *
Re: wskbd_set_mixervolume
On Mon, Feb 07, 2022 at 06:55:21PM +0100, Anton Lindqvist wrote: > On Mon, Feb 07, 2022 at 11:21:43AM +0100, Alexandre Ratchov wrote: > > On Sun, Feb 06, 2022 at 08:40:35AM +0100, Anton Lindqvist wrote: > > > > > > Polished diff. I'm omitting a necessary refactoring commit making > > > audio_attach_mi() accept a new cookie argument. > > > > > > > I'm not sure to understand the need for the wskbd_audio structure. > > Couldn't we just store the cookie in audio and wskbd softc structures, > > then pass it in the wskbd_set_mixervolume_sc() calls ? > > Due to the device caching the data must be stored in either the audio or > wskbd softc and I don't want to expose the softc structures so I ended > up introducing wskbd_audio. Dropping the caching would probably make it > possible to only pass down the cookie to wskbd_set_mixervolume_sc() and > always do the audio device lookup. > > Is anyone in favor of this approach? I achieves the expected behavior in > my opinion. IMHO, handling the volume keys this way won't work in the general case. But for the short term we've no other options, have we? AFAICS, you're fixing a concrete use-case by tweaking what already exists, this won't make things more broken than they already are. I'm OK with it.
Re: wskbd_set_mixervolume
On Mon, Feb 07, 2022 at 06:58:40PM +0100, Anton Lindqvist wrote:
> On Mon, Feb 07, 2022 at 11:43:43AM +0100, Alexandre Ratchov wrote:
> > On Sat, Feb 05, 2022 at 06:30:43PM +0100, Claudio Jeker wrote:
> > > On Sat, Feb 05, 2022 at 12:28:08PM +0100, Mark Kettenis wrote:
> > > > > Date: Sat, 5 Feb 2022 09:29:42 +0100
> > > > > From: Anton Lindqvist
> > > > >
> > > > > Hi,
> > > > > I recently got a USB headset with physical volume buttons, handled by
> > > > > ucc(4). However, after enabling the device in sndiod the volume
> > > > > buttons
> > > > > does not cause the volume to change. Turns out wskbd_set_mixervolume()
> > > > > is only propagating volume changes to first attached audio device. Is
> > > > > their any good not to consider all attached audio devices?
> > > >
> > > > I think this is tricky. The mixer values of different audio devices
> > > > may start out differently and may have different granularity and
> > > > probably operate on a different scale. This may lead to situations
> > > > where as you turn the volume up and down, the relative output volume
> > > > between devices changes considerably. I also think that your
> > > > implementation will unmute all audio devices as soon as you touch the
> > > > volume control buttons, which is probably not desirable.
> > > >
> > > > Thinking about other ways to do this, we could:
> > > >
> > > > - Add a knob that allows the user to control which audio device is
> > > > controlled by the volume control buttons. The choice could include
> > > > "none" and "all" as well as the individual devices.
> > > >
> > > > - Add infrastructure to bind specific keyboards to specific audio
> > > > devices, a bit like how we support binding specific wskbd devices to
> > > > specific wsdisplay devices.
> > > >
> > > > The first suggestion is probably relatively easy to achieve. The
> > > > implementation of the latter would defenitely need more thought and
> > > > discussion.
> > > >
> > > > The "none" choice above would (partly) solve another issue where
> > > > userland applications see the key presses and act upon them even
> > > > though the kernel already did the volume adjustment.
> > >
> > > There is a 3rd option of passing the information to sndiod and let it do
> > > the volume scaling.
> > >
> >
> > Here's a small WIP program to do so. It also adds a key to cycle
> > through audio devices.
> >
> > https://github.com/ratchov/sndiokeys
> >
> > By default it uses Ctrl+Alt+{plus,minus,0} because multimedia keys are
> > already used.
> >
> > Anton, I've no headphones with buttons to test, does this work for
> > yours?
>
> Interesting! I cannot get to run under X, it fails in XGetKeyboardMapping:
>
> (gdb) r
> Starting program: /tmp/sndiokeys/sndiokeys
> X Error of failed request: BadAccess (attempt to access private resource
> denied)
> Major opcode of failed request: 33 (X_GrabKey)
> Serial number of failed request: 12
> Current serial number in output stream: 76
>
> Breakpoint 1, _libc_exit (status=1) at /home/src2/lib/libc/stdlib/exit.c:54
> 54 /home/src2/lib/libc/stdlib/exit.c: No such file or directory.
> (gdb) bt
> #0 _libc_exit (status=1) at /home/src2/lib/libc/stdlib/exit.c:54
> #1 0x0cfba76512b1 in _XDefaultError () from /usr/X11R6/lib/libX11.so.17.1
> #2 0x0cfba76514a9 in _XError () from /usr/X11R6/lib/libX11.so.17.1
> #3 0x0cfba764d47a in handle_response () from
> /usr/X11R6/lib/libX11.so.17.1
> #4 0x0cfba764dd50 in _XReply () from /usr/X11R6/lib/libX11.so.17.1
> #5 0x0cfba762d8ba in XGetKeyboardMapping () from
> /usr/X11R6/lib/libX11.so.17.1
> #6 0x0cf94a4778f7 in grab_keys () at sndiokeys.c:318
> #7 0x0cf94a477279 in main (argc=0, argv=0x7f7dd760) at
> sndiokeys.c:529
>
>
One of the keys is already used (aka grabbed) by another X
program. You've to either stop the other program or start sndiokeys
before it.
Meanwhile, I'll try to add error handling and print a meaningful error
message.
Re: wskbd_set_mixervolume
On Mon, Feb 07, 2022 at 11:43:43AM +0100, Alexandre Ratchov wrote:
> On Sat, Feb 05, 2022 at 06:30:43PM +0100, Claudio Jeker wrote:
> > On Sat, Feb 05, 2022 at 12:28:08PM +0100, Mark Kettenis wrote:
> > > > Date: Sat, 5 Feb 2022 09:29:42 +0100
> > > > From: Anton Lindqvist
> > > >
> > > > Hi,
> > > > I recently got a USB headset with physical volume buttons, handled by
> > > > ucc(4). However, after enabling the device in sndiod the volume buttons
> > > > does not cause the volume to change. Turns out wskbd_set_mixervolume()
> > > > is only propagating volume changes to first attached audio device. Is
> > > > their any good not to consider all attached audio devices?
> > >
> > > I think this is tricky. The mixer values of different audio devices
> > > may start out differently and may have different granularity and
> > > probably operate on a different scale. This may lead to situations
> > > where as you turn the volume up and down, the relative output volume
> > > between devices changes considerably. I also think that your
> > > implementation will unmute all audio devices as soon as you touch the
> > > volume control buttons, which is probably not desirable.
> > >
> > > Thinking about other ways to do this, we could:
> > >
> > > - Add a knob that allows the user to control which audio device is
> > > controlled by the volume control buttons. The choice could include
> > > "none" and "all" as well as the individual devices.
> > >
> > > - Add infrastructure to bind specific keyboards to specific audio
> > > devices, a bit like how we support binding specific wskbd devices to
> > > specific wsdisplay devices.
> > >
> > > The first suggestion is probably relatively easy to achieve. The
> > > implementation of the latter would defenitely need more thought and
> > > discussion.
> > >
> > > The "none" choice above would (partly) solve another issue where
> > > userland applications see the key presses and act upon them even
> > > though the kernel already did the volume adjustment.
> >
> > There is a 3rd option of passing the information to sndiod and let it do
> > the volume scaling.
> >
>
> Here's a small WIP program to do so. It also adds a key to cycle
> through audio devices.
>
> https://github.com/ratchov/sndiokeys
>
> By default it uses Ctrl+Alt+{plus,minus,0} because multimedia keys are
> already used.
>
> Anton, I've no headphones with buttons to test, does this work for
> yours?
Interesting! I cannot get to run under X, it fails in XGetKeyboardMapping:
(gdb) r
Starting program: /tmp/sndiokeys/sndiokeys
X Error of failed request: BadAccess (attempt to access private resource
denied)
Major opcode of failed request: 33 (X_GrabKey)
Serial number of failed request: 12
Current serial number in output stream: 76
Breakpoint 1, _libc_exit (status=1) at /home/src2/lib/libc/stdlib/exit.c:54
54 /home/src2/lib/libc/stdlib/exit.c: No such file or directory.
(gdb) bt
#0 _libc_exit (status=1) at /home/src2/lib/libc/stdlib/exit.c:54
#1 0x0cfba76512b1 in _XDefaultError () from /usr/X11R6/lib/libX11.so.17.1
#2 0x0cfba76514a9 in _XError () from /usr/X11R6/lib/libX11.so.17.1
#3 0x0cfba764d47a in handle_response () from /usr/X11R6/lib/libX11.so.17.1
#4 0x0cfba764dd50 in _XReply () from /usr/X11R6/lib/libX11.so.17.1
#5 0x0cfba762d8ba in XGetKeyboardMapping () from
/usr/X11R6/lib/libX11.so.17.1
#6 0x0cf94a4778f7 in grab_keys () at sndiokeys.c:318
#7 0x0cf94a477279 in main (argc=0, argv=0x7f7dd760) at sndiokeys.c:529
Re: wskbd_set_mixervolume
On Mon, Feb 07, 2022 at 11:21:43AM +0100, Alexandre Ratchov wrote: > On Sun, Feb 06, 2022 at 08:40:35AM +0100, Anton Lindqvist wrote: > > > > Polished diff. I'm omitting a necessary refactoring commit making > > audio_attach_mi() accept a new cookie argument. > > > > I'm not sure to understand the need for the wskbd_audio structure. > Couldn't we just store the cookie in audio and wskbd softc structures, > then pass it in the wskbd_set_mixervolume_sc() calls ? Due to the device caching the data must be stored in either the audio or wskbd softc and I don't want to expose the softc structures so I ended up introducing wskbd_audio. Dropping the caching would probably make it possible to only pass down the cookie to wskbd_set_mixervolume_sc() and always do the audio device lookup. Is anyone in favor of this approach? I achieves the expected behavior in my opinion.
Re: wskbd_set_mixervolume
On Sat, Feb 05, 2022 at 06:30:43PM +0100, Claudio Jeker wrote:
> On Sat, Feb 05, 2022 at 12:28:08PM +0100, Mark Kettenis wrote:
> > > Date: Sat, 5 Feb 2022 09:29:42 +0100
> > > From: Anton Lindqvist
> > >
> > > Hi,
> > > I recently got a USB headset with physical volume buttons, handled by
> > > ucc(4). However, after enabling the device in sndiod the volume buttons
> > > does not cause the volume to change. Turns out wskbd_set_mixervolume()
> > > is only propagating volume changes to first attached audio device. Is
> > > their any good not to consider all attached audio devices?
> >
> > I think this is tricky. The mixer values of different audio devices
> > may start out differently and may have different granularity and
> > probably operate on a different scale. This may lead to situations
> > where as you turn the volume up and down, the relative output volume
> > between devices changes considerably. I also think that your
> > implementation will unmute all audio devices as soon as you touch the
> > volume control buttons, which is probably not desirable.
> >
> > Thinking about other ways to do this, we could:
> >
> > - Add a knob that allows the user to control which audio device is
> > controlled by the volume control buttons. The choice could include
> > "none" and "all" as well as the individual devices.
> >
> > - Add infrastructure to bind specific keyboards to specific audio
> > devices, a bit like how we support binding specific wskbd devices to
> > specific wsdisplay devices.
> >
> > The first suggestion is probably relatively easy to achieve. The
> > implementation of the latter would defenitely need more thought and
> > discussion.
> >
> > The "none" choice above would (partly) solve another issue where
> > userland applications see the key presses and act upon them even
> > though the kernel already did the volume adjustment.
>
> There is a 3rd option of passing the information to sndiod and let it do
> the volume scaling.
>
Here's a small WIP program to do so. It also adds a key to cycle
through audio devices.
https://github.com/ratchov/sndiokeys
By default it uses Ctrl+Alt+{plus,minus,0} because multimedia keys are
already used.
Anton, I've no headphones with buttons to test, does this work for
yours?
Re: wskbd_set_mixervolume
On Sun, Feb 06, 2022 at 08:40:35AM +0100, Anton Lindqvist wrote:
>
> Polished diff. I'm omitting a necessary refactoring commit making
> audio_attach_mi() accept a new cookie argument.
>
I'm not sure to understand the need for the wskbd_audio structure.
Couldn't we just store the cookie in audio and wskbd softc structures,
then pass it in the wskbd_set_mixervolume_sc() calls ?
> diff --git sys/dev/audio.c sys/dev/audio.c
> index 64696025a50..f2b40637771 100644
> --- sys/dev/audio.c
> +++ sys/dev/audio.c
> @@ -28,6 +28,7 @@
> #include
> #include
> #include
> +#include /* struct wskbd_audio */
> #include "audio.h"
> #include "wskbd.h"
>
> @@ -96,6 +97,8 @@ struct wskbd_vol
> #define WSKBD_MUTE_DISABLE 2
> #define WSKBD_MUTE_ENABLE3
> };
> +
> +int wskbd_set_mixervolume_unit(int, long, long);
> #endif
>
> /*
> @@ -2455,10 +2458,6 @@ wskbd_mixer_init(struct audio_softc *sc)
> };
> int i;
>
> - if (sc->dev.dv_unit != 0) {
> - DPRINTF("%s: not configuring wskbd keys\n", DEVNAME(sc));
> - return;
> - }
> for (i = 0; i < sizeof(spkr_names) / sizeof(spkr_names[0]); i++) {
> if (wskbd_initvol(sc, &sc->spkr,
> spkr_names[i].cn, spkr_names[i].dn))
> @@ -2569,13 +2568,55 @@ wskbd_set_mixermute(long mute, long out)
> return 0;
> }
>
> +int
> +wskbd_set_mixervolume_sc(struct wskbd_audio *audio, long dir, long out)
> +{
> + if (audio->dev != NULL) {
> + if ((audio->dev->dv_flags & DVF_ACTIVE) == 0) {
> + /* Audio device gone, fallback to audio0. */
> + device_unref(audio->dev);
> + audio->dev = NULL;
> + audio->unit = 0;
> + }
> + } else if (audio->cookie != NULL) {
> + void *cookie;
> + int i;
> +
> + cookie = audio->cookie;
> + audio->cookie = NULL;
> + for (i = 0; i < audio_cd.cd_ndevs; i++) {
> + struct audio_softc *sc;
> +
> + sc = (struct audio_softc *)device_lookup(&audio_cd, i);
> + if (sc == NULL)
> + continue;
> + if (sc->cookie != cookie) {
> + device_unref(&sc->dev);
> + continue;
> + }
> +
> + audio->dev = (struct device *)sc;
> + audio->unit = i;
> + break;
> + }
> + }
> +
> + return wskbd_set_mixervolume_unit(audio->unit, dir, out);
> +}
> +
> int
> wskbd_set_mixervolume(long dir, long out)
> +{
> + return wskbd_set_mixervolume_unit(0, dir, out);
> +}
> +
> +int
> +wskbd_set_mixervolume_unit(int unit, long dir, long out)
> {
> struct audio_softc *sc;
> struct wskbd_vol *vol;
>
> - sc = (struct audio_softc *)device_lookup(&audio_cd, 0);
> + sc = (struct audio_softc *)device_lookup(&audio_cd, unit);
> if (sc == NULL)
> return ENODEV;
> vol = out ? &sc->spkr : &sc->mic;
> diff --git sys/dev/usb/uaudio.c sys/dev/usb/uaudio.c
> index 0a19d512a5c..d86019311e0 100644
> --- sys/dev/usb/uaudio.c
> +++ sys/dev/usb/uaudio.c
> @@ -3841,7 +3841,7 @@ uaudio_attach(struct device *parent, struct device
> *self, void *aux)
> /* print a nice uaudio attach line */
> uaudio_print(sc);
>
> - audio_attach_mi(&uaudio_hw_if, sc, NULL, &sc->dev);
> + audio_attach_mi(&uaudio_hw_if, sc, arg->cookie, &sc->dev);
> }
>
> int
> diff --git sys/dev/usb/ucc.c sys/dev/usb/ucc.c
> index f23f32990bb..705a31b327b 100644
> --- sys/dev/usb/ucc.c
> +++ sys/dev/usb/ucc.c
> @@ -104,7 +104,7 @@ void ucc_attach(struct device *, struct device *,
> void *);
> int ucc_detach(struct device *, int);
> void ucc_intr(struct uhidev *, void *, u_int);
>
> -void ucc_attach_wskbd(struct ucc_softc *);
> +void ucc_attach_wskbd(struct ucc_softc *, void *);
> int ucc_enable(void *, int);
> void ucc_set_leds(void *, int);
> int ucc_ioctl(void *, u_long, caddr_t, int, struct proc *);
> @@ -680,7 +680,7 @@ ucc_attach(struct device *parent, struct device *self,
> void *aux)
>
> /* Cannot load an empty map. */
> if (sc->sc_maplen > 0)
> - ucc_attach_wskbd(sc);
> + ucc_attach_wskbd(sc, uha->uaa->cookie);
> }
>
> int
>
Re: wskbd_set_mixervolume
On Sat, Feb 05, 2022 at 06:30:43PM +0100, Claudio Jeker wrote: > On Sat, Feb 05, 2022 at 12:28:08PM +0100, Mark Kettenis wrote: > > > Date: Sat, 5 Feb 2022 09:29:42 +0100 > > > From: Anton Lindqvist > > > > > > Hi, > > > I recently got a USB headset with physical volume buttons, handled by > > > ucc(4). However, after enabling the device in sndiod the volume buttons > > > does not cause the volume to change. Turns out wskbd_set_mixervolume() > > > is only propagating volume changes to first attached audio device. Is > > > their any good not to consider all attached audio devices? > > > > I think this is tricky. The mixer values of different audio devices > > may start out differently and may have different granularity and > > probably operate on a different scale. This may lead to situations > > where as you turn the volume up and down, the relative output volume > > between devices changes considerably. I also think that your > > implementation will unmute all audio devices as soon as you touch the > > volume control buttons, which is probably not desirable. > > > > Thinking about other ways to do this, we could: > > > > - Add a knob that allows the user to control which audio device is > > controlled by the volume control buttons. The choice could include > > "none" and "all" as well as the individual devices. > > > > - Add infrastructure to bind specific keyboards to specific audio > > devices, a bit like how we support binding specific wskbd devices to > > specific wsdisplay devices. > > > > The first suggestion is probably relatively easy to achieve. The > > implementation of the latter would defenitely need more thought and > > discussion. > > > > The "none" choice above would (partly) solve another issue where > > userland applications see the key presses and act upon them even > > though the kernel already did the volume adjustment. > > There is a 3rd option of passing the information to sndiod and let it do > the volume scaling. > Agreed, this will fix most volume keys-related problems.
Re: wskbd_set_mixervolume
On Sat, Feb 05, 2022 at 09:29:42AM +0100, Anton Lindqvist wrote:
> Hi,
> I recently got a USB headset with physical volume buttons, handled by
> ucc(4). However, after enabling the device in sndiod the volume buttons
> does not cause the volume to change. Turns out wskbd_set_mixervolume()
> is only propagating volume changes to first attached audio device. Is
> their any good not to consider all attached audio devices?
This is a hack to make pc keyboard volume keys mimic volume keys of
old laptops, which used to simply control internal speaker/headphones
level. Hence the use of the first device (in most cases the internal).
> The diff below gives me the desired behavior by propagating volume
> changes to all attached audio devices.
>
Your diff is correct.
However it's based on a broken hack, so it won't work as expected:
Currently volume keys change the volume but also propagate to X as
XF86XK_Audio{Raise,Lower}Volume events which in turn may change the
volume a second time in a different way, possibly of different
device. To increase further the confusion, X keys auto-repeat, while
volume key changes through wskbd_set_mixervolume() don't.
For instance, in one terminal start "sndioctl -m" to monitor volume
changes then play something with ports/mplayer and observe volume
changes:
$ sndioctl -m
...
output[0].level=0.322 # changed
output[1].level=0.322 # changed
app/mplayer0.level=0.567# changed
output[0].level=0.353 # changed
output[1].level=0.353 # changed
app/mplayer0.level=0.583# changed
IMHO, adding a sysctl or wsconsctl knob, as kettenis@ suggested, would
make your keys "work", and allow to start experimenting with proper
user-space solution.
Re: wskbd_set_mixervolume
On Sat, Feb 05, 2022 at 02:46:53PM +0100, Anton Lindqvist wrote:
> On Sat, Feb 05, 2022 at 12:28:08PM +0100, Mark Kettenis wrote:
> > > Date: Sat, 5 Feb 2022 09:29:42 +0100
> > > From: Anton Lindqvist
> > >
> > > Hi,
> > > I recently got a USB headset with physical volume buttons, handled by
> > > ucc(4). However, after enabling the device in sndiod the volume buttons
> > > does not cause the volume to change. Turns out wskbd_set_mixervolume()
> > > is only propagating volume changes to first attached audio device. Is
> > > their any good not to consider all attached audio devices?
> >
> > I think this is tricky. The mixer values of different audio devices
> > may start out differently and may have different granularity and
> > probably operate on a different scale. This may lead to situations
> > where as you turn the volume up and down, the relative output volume
> > between devices changes considerably. I also think that your
> > implementation will unmute all audio devices as soon as you touch the
> > volume control buttons, which is probably not desirable.
> >
> > Thinking about other ways to do this, we could:
> >
> > - Add a knob that allows the user to control which audio device is
> > controlled by the volume control buttons. The choice could include
> > "none" and "all" as well as the individual devices.
> >
> > - Add infrastructure to bind specific keyboards to specific audio
> > devices, a bit like how we support binding specific wskbd devices to
> > specific wsdisplay devices.
> >
> > The first suggestion is probably relatively easy to achieve. The
> > implementation of the latter would defenitely need more thought and
> > discussion.
> >
> > The "none" choice above would (partly) solve another issue where
> > userland applications see the key presses and act upon them even
> > though the kernel already did the volume adjustment.
> >
> > Cheers,
> >
> > Mark
>
> Thanks for the input. Another approach would be to correlate the audio
> and wskbd device. Here's a proof of concept diff that propagates a
> "cookie" from the common point of attachment, being uhub for my
> particular device. The same cookie is later used to find the matching
> audio device while handling volume keys. The found audio device could
> probably be cached inside `struct wskbd_softc' if it grabs a device
> reference. I'm taking quite a few shortcuts here to avoid changing the
> prototypes of commonly used functions.
Polished diff. I'm omitting a necessary refactoring commit making
audio_attach_mi() accept a new cookie argument.
diff --git sys/dev/audio.c sys/dev/audio.c
index 64696025a50..f2b40637771 100644
--- sys/dev/audio.c
+++ sys/dev/audio.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include/* struct wskbd_audio */
#include "audio.h"
#include "wskbd.h"
@@ -96,6 +97,8 @@ struct wskbd_vol
#define WSKBD_MUTE_DISABLE 2
#define WSKBD_MUTE_ENABLE 3
};
+
+int wskbd_set_mixervolume_unit(int, long, long);
#endif
/*
@@ -2455,10 +2458,6 @@ wskbd_mixer_init(struct audio_softc *sc)
};
int i;
- if (sc->dev.dv_unit != 0) {
- DPRINTF("%s: not configuring wskbd keys\n", DEVNAME(sc));
- return;
- }
for (i = 0; i < sizeof(spkr_names) / sizeof(spkr_names[0]); i++) {
if (wskbd_initvol(sc, &sc->spkr,
spkr_names[i].cn, spkr_names[i].dn))
@@ -2569,13 +2568,55 @@ wskbd_set_mixermute(long mute, long out)
return 0;
}
+int
+wskbd_set_mixervolume_sc(struct wskbd_audio *audio, long dir, long out)
+{
+ if (audio->dev != NULL) {
+ if ((audio->dev->dv_flags & DVF_ACTIVE) == 0) {
+ /* Audio device gone, fallback to audio0. */
+ device_unref(audio->dev);
+ audio->dev = NULL;
+ audio->unit = 0;
+ }
+ } else if (audio->cookie != NULL) {
+ void *cookie;
+ int i;
+
+ cookie = audio->cookie;
+ audio->cookie = NULL;
+ for (i = 0; i < audio_cd.cd_ndevs; i++) {
+ struct audio_softc *sc;
+
+ sc = (struct audio_softc *)device_lookup(&audio_cd, i);
+ if (sc == NULL)
+ continue;
+ if (sc->cookie != cookie) {
+ device_unref(&sc->dev);
+ continue;
+ }
Re: wskbd_set_mixervolume
On Sat, Feb 05, 2022 at 12:28:08PM +0100, Mark Kettenis wrote:
> > Date: Sat, 5 Feb 2022 09:29:42 +0100
> > From: Anton Lindqvist
> >
> > Hi,
> > I recently got a USB headset with physical volume buttons, handled by
> > ucc(4). However, after enabling the device in sndiod the volume buttons
> > does not cause the volume to change. Turns out wskbd_set_mixervolume()
> > is only propagating volume changes to first attached audio device. Is
> > their any good not to consider all attached audio devices?
>
> I think this is tricky. The mixer values of different audio devices
> may start out differently and may have different granularity and
> probably operate on a different scale. This may lead to situations
> where as you turn the volume up and down, the relative output volume
> between devices changes considerably. I also think that your
> implementation will unmute all audio devices as soon as you touch the
> volume control buttons, which is probably not desirable.
>
> Thinking about other ways to do this, we could:
>
> - Add a knob that allows the user to control which audio device is
> controlled by the volume control buttons. The choice could include
> "none" and "all" as well as the individual devices.
>
> - Add infrastructure to bind specific keyboards to specific audio
> devices, a bit like how we support binding specific wskbd devices to
> specific wsdisplay devices.
>
> The first suggestion is probably relatively easy to achieve. The
> implementation of the latter would defenitely need more thought and
> discussion.
>
> The "none" choice above would (partly) solve another issue where
> userland applications see the key presses and act upon them even
> though the kernel already did the volume adjustment.
There is a 3rd option of passing the information to sndiod and let it do
the volume scaling.
> Cheers,
>
> Mark
>
> > The diff below gives me the desired behavior by propagating volume
> > changes to all attached audio devices.
> >
> > diff --git sys/dev/audio.c sys/dev/audio.c
> > index ec52ee6ef01..ca19557d39e 100644
> > --- sys/dev/audio.c
> > +++ sys/dev/audio.c
> > @@ -2452,10 +2452,6 @@ wskbd_mixer_init(struct audio_softc *sc)
> > };
> > int i;
> >
> > - if (sc->dev.dv_unit != 0) {
> > - DPRINTF("%s: not configuring wskbd keys\n", DEVNAME(sc));
> > - return;
> > - }
> > for (i = 0; i < sizeof(spkr_names) / sizeof(spkr_names[0]); i++) {
> > if (wskbd_initvol(sc, &sc->spkr,
> > spkr_names[i].cn, spkr_names[i].dn))
> > @@ -2569,19 +2565,26 @@ wskbd_set_mixermute(long mute, long out)
> > int
> > wskbd_set_mixervolume(long dir, long out)
> > {
> > - struct audio_softc *sc;
> > - struct wskbd_vol *vol;
> > + int error = ENODEV;
> > + int minor;
> >
> > - sc = (struct audio_softc *)device_lookup(&audio_cd, 0);
> > - if (sc == NULL)
> > - return ENODEV;
> > - vol = out ? &sc->spkr : &sc->mic;
> > - if (dir == 0)
> > - vol->mute_pending ^= WSKBD_MUTE_TOGGLE;
> > - else
> > - vol->val_pending += dir;
> > - if (!task_add(systq, &sc->wskbd_task))
> > - device_unref(&sc->dev);
> > - return 0;
> > + for (minor = 0; minor < audio_cd.cd_ndevs; minor++) {
> > + struct audio_softc *sc;
> > + struct wskbd_vol *vol;
> > +
> > + sc = (struct audio_softc *)device_lookup(&audio_cd, minor);
> > + if (sc == NULL)
> > + continue;
> > + vol = out ? &sc->spkr : &sc->mic;
> > + if (dir == 0)
> > + vol->mute_pending ^= WSKBD_MUTE_TOGGLE;
> > + else
> > + vol->val_pending += dir;
> > + if (!task_add(systq, &sc->wskbd_task))
> > + device_unref(&sc->dev);
> > + error = 0;
> > + }
> > +
> > + return error;
> > }
> > #endif /* NWSKBD > 0 */
> >
> >
>
--
:wq Claudio
Re: wskbd_set_mixervolume
On Sat, Feb 05, 2022 at 12:28:08PM +0100, Mark Kettenis wrote:
> > Date: Sat, 5 Feb 2022 09:29:42 +0100
> > From: Anton Lindqvist
> >
> > Hi,
> > I recently got a USB headset with physical volume buttons, handled by
> > ucc(4). However, after enabling the device in sndiod the volume buttons
> > does not cause the volume to change. Turns out wskbd_set_mixervolume()
> > is only propagating volume changes to first attached audio device. Is
> > their any good not to consider all attached audio devices?
>
> I think this is tricky. The mixer values of different audio devices
> may start out differently and may have different granularity and
> probably operate on a different scale. This may lead to situations
> where as you turn the volume up and down, the relative output volume
> between devices changes considerably. I also think that your
> implementation will unmute all audio devices as soon as you touch the
> volume control buttons, which is probably not desirable.
>
> Thinking about other ways to do this, we could:
>
> - Add a knob that allows the user to control which audio device is
> controlled by the volume control buttons. The choice could include
> "none" and "all" as well as the individual devices.
>
> - Add infrastructure to bind specific keyboards to specific audio
> devices, a bit like how we support binding specific wskbd devices to
> specific wsdisplay devices.
>
> The first suggestion is probably relatively easy to achieve. The
> implementation of the latter would defenitely need more thought and
> discussion.
>
> The "none" choice above would (partly) solve another issue where
> userland applications see the key presses and act upon them even
> though the kernel already did the volume adjustment.
>
> Cheers,
>
> Mark
Thanks for the input. Another approach would be to correlate the audio
and wskbd device. Here's a proof of concept diff that propagates a
"cookie" from the common point of attachment, being uhub for my
particular device. The same cookie is later used to find the matching
audio device while handling volume keys. The found audio device could
probably be cached inside `struct wskbd_softc' if it grabs a device
reference. I'm taking quite a few shortcuts here to avoid changing the
prototypes of commonly used functions.
diff --git sys/dev/audio.c sys/dev/audio.c
index ec52ee6ef01..f412e497edd 100644
--- sys/dev/audio.c
+++ sys/dev/audio.c
@@ -112,6 +112,7 @@ struct mixer_ev {
struct audio_softc {
struct device dev;
struct audio_hw_if *ops;/* driver funcs */
+ void *cookie; /* XXX */
void *arg; /* first arg to driver funcs */
int mode; /* bitmask of AUMODE_* */
int quiesce;/* device suspended */
@@ -1236,6 +1237,7 @@ audio_attach(struct device *parent, struct device *self,
void *aux)
}
#endif
sc->ops = ops;
+ sc->cookie = sa->cookie;
sc->arg = arg;
#if NWSKBD > 0
@@ -1474,6 +1476,24 @@ audio_attach_mi(struct audio_hw_if *ops, void *arg,
struct device *dev)
aa.type = AUDIODEV_TYPE_AUDIO;
aa.hwif = ops;
aa.hdl = arg;
+ aa.cookie = NULL;
+
+ /*
+* attach this driver to the caller (hardware driver), this
+* checks the kernel config and possibly calls audio_attach()
+*/
+ return config_found_sm(dev, &aa, audioprint, audio_submatch);
+}
+
+struct device *
+audio_attach_mi1(struct audio_hw_if *ops, void *arg, struct device *dev, void
*cookie)
+{
+ struct audio_attach_args aa;
+
+ aa.type = AUDIODEV_TYPE_AUDIO;
+ aa.hwif = ops;
+ aa.hdl = arg;
+ aa.cookie = cookie;
/*
* attach this driver to the caller (hardware driver), this
@@ -2452,10 +2472,6 @@ wskbd_mixer_init(struct audio_softc *sc)
};
int i;
- if (sc->dev.dv_unit != 0) {
- DPRINTF("%s: not configuring wskbd keys\n", DEVNAME(sc));
- return;
- }
for (i = 0; i < sizeof(spkr_names) / sizeof(spkr_names[0]); i++) {
if (wskbd_initvol(sc, &sc->spkr,
spkr_names[i].cn, spkr_names[i].dn))
@@ -2566,6 +2582,37 @@ wskbd_set_mixermute(long mute, long out)
return 0;
}
+int
+wskbd_set_mixervolume1(void *cookie, long dir, long out)
+{
+ int minor;
+
+ for (minor = 0; minor < audio_cd.cd_ndevs; minor++) {
+ struct audio_softc *sc;
+ struct wskbd_vol *vol;
+
+ sc = (struct audio_softc *)device_lookup(&audio_cd, minor);
+ if (sc == NULL)
+ continue;
+ if (sc->cookie != coo
Re: wskbd_set_mixervolume
> Date: Sat, 5 Feb 2022 09:29:42 +0100
> From: Anton Lindqvist
>
> Hi,
> I recently got a USB headset with physical volume buttons, handled by
> ucc(4). However, after enabling the device in sndiod the volume buttons
> does not cause the volume to change. Turns out wskbd_set_mixervolume()
> is only propagating volume changes to first attached audio device. Is
> their any good not to consider all attached audio devices?
I think this is tricky. The mixer values of different audio devices
may start out differently and may have different granularity and
probably operate on a different scale. This may lead to situations
where as you turn the volume up and down, the relative output volume
between devices changes considerably. I also think that your
implementation will unmute all audio devices as soon as you touch the
volume control buttons, which is probably not desirable.
Thinking about other ways to do this, we could:
- Add a knob that allows the user to control which audio device is
controlled by the volume control buttons. The choice could include
"none" and "all" as well as the individual devices.
- Add infrastructure to bind specific keyboards to specific audio
devices, a bit like how we support binding specific wskbd devices to
specific wsdisplay devices.
The first suggestion is probably relatively easy to achieve. The
implementation of the latter would defenitely need more thought and
discussion.
The "none" choice above would (partly) solve another issue where
userland applications see the key presses and act upon them even
though the kernel already did the volume adjustment.
Cheers,
Mark
> The diff below gives me the desired behavior by propagating volume
> changes to all attached audio devices.
>
> diff --git sys/dev/audio.c sys/dev/audio.c
> index ec52ee6ef01..ca19557d39e 100644
> --- sys/dev/audio.c
> +++ sys/dev/audio.c
> @@ -2452,10 +2452,6 @@ wskbd_mixer_init(struct audio_softc *sc)
> };
> int i;
>
> - if (sc->dev.dv_unit != 0) {
> - DPRINTF("%s: not configuring wskbd keys\n", DEVNAME(sc));
> - return;
> - }
> for (i = 0; i < sizeof(spkr_names) / sizeof(spkr_names[0]); i++) {
> if (wskbd_initvol(sc, &sc->spkr,
> spkr_names[i].cn, spkr_names[i].dn))
> @@ -2569,19 +2565,26 @@ wskbd_set_mixermute(long mute, long out)
> int
> wskbd_set_mixervolume(long dir, long out)
> {
> - struct audio_softc *sc;
> - struct wskbd_vol *vol;
> + int error = ENODEV;
> + int minor;
>
> - sc = (struct audio_softc *)device_lookup(&audio_cd, 0);
> - if (sc == NULL)
> - return ENODEV;
> - vol = out ? &sc->spkr : &sc->mic;
> - if (dir == 0)
> - vol->mute_pending ^= WSKBD_MUTE_TOGGLE;
> - else
> - vol->val_pending += dir;
> - if (!task_add(systq, &sc->wskbd_task))
> - device_unref(&sc->dev);
> - return 0;
> + for (minor = 0; minor < audio_cd.cd_ndevs; minor++) {
> + struct audio_softc *sc;
> + struct wskbd_vol *vol;
> +
> + sc = (struct audio_softc *)device_lookup(&audio_cd, minor);
> + if (sc == NULL)
> + continue;
> + vol = out ? &sc->spkr : &sc->mic;
> + if (dir == 0)
> + vol->mute_pending ^= WSKBD_MUTE_TOGGLE;
> + else
> + vol->val_pending += dir;
> + if (!task_add(systq, &sc->wskbd_task))
> + device_unref(&sc->dev);
> + error = 0;
> + }
> +
> + return error;
> }
> #endif /* NWSKBD > 0 */
>
>
wskbd_set_mixervolume
Hi,
I recently got a USB headset with physical volume buttons, handled by
ucc(4). However, after enabling the device in sndiod the volume buttons
does not cause the volume to change. Turns out wskbd_set_mixervolume()
is only propagating volume changes to first attached audio device. Is
their any good not to consider all attached audio devices?
The diff below gives me the desired behavior by propagating volume
changes to all attached audio devices.
diff --git sys/dev/audio.c sys/dev/audio.c
index ec52ee6ef01..ca19557d39e 100644
--- sys/dev/audio.c
+++ sys/dev/audio.c
@@ -2452,10 +2452,6 @@ wskbd_mixer_init(struct audio_softc *sc)
};
int i;
- if (sc->dev.dv_unit != 0) {
- DPRINTF("%s: not configuring wskbd keys\n", DEVNAME(sc));
- return;
- }
for (i = 0; i < sizeof(spkr_names) / sizeof(spkr_names[0]); i++) {
if (wskbd_initvol(sc, &sc->spkr,
spkr_names[i].cn, spkr_names[i].dn))
@@ -2569,19 +2565,26 @@ wskbd_set_mixermute(long mute, long out)
int
wskbd_set_mixervolume(long dir, long out)
{
- struct audio_softc *sc;
- struct wskbd_vol *vol;
+ int error = ENODEV;
+ int minor;
- sc = (struct audio_softc *)device_lookup(&audio_cd, 0);
- if (sc == NULL)
- return ENODEV;
- vol = out ? &sc->spkr : &sc->mic;
- if (dir == 0)
- vol->mute_pending ^= WSKBD_MUTE_TOGGLE;
- else
- vol->val_pending += dir;
- if (!task_add(systq, &sc->wskbd_task))
- device_unref(&sc->dev);
- return 0;
+ for (minor = 0; minor < audio_cd.cd_ndevs; minor++) {
+ struct audio_softc *sc;
+ struct wskbd_vol *vol;
+
+ sc = (struct audio_softc *)device_lookup(&audio_cd, minor);
+ if (sc == NULL)
+ continue;
+ vol = out ? &sc->spkr : &sc->mic;
+ if (dir == 0)
+ vol->mute_pending ^= WSKBD_MUTE_TOGGLE;
+ else
+ vol->val_pending += dir;
+ if (!task_add(systq, &sc->wskbd_task))
+ device_unref(&sc->dev);
+ error = 0;
+ }
+
+ return error;
}
#endif /* NWSKBD > 0 */
