Download raw body.
awacs(4) - fix sound issues for imac g3
On Thursday, 18 January 2024 at 16:32, joshua stein <jcs@jcs.org> wrote:
> On Tue, 05 Dec 2023 at 13:27:56 +0000, evv42 wrote:
>
> > Hi,
> >
> > The following patch fixes multiple issues regarding the awacs driver
> > for the three revisions of imac g3 (PowerMac 2,1, 2,2 and 4,1), in
> > particular headphones detection and integrated speakers not working.
> >
> > This patch introduces hardware detection via Open Firmware, which
> > enables that support, and would allow better support for other
> > hardware that also have issues.
> >
> > It should not affect other hardware, and has only been
> > tested on the PowerMac2,2 revision.
> >
> > evv42
>
>
> Hi, some minor suggestions below:
>
> > diff --git sys/arch/macppc/dev/awacs.c sys/arch/macppc/dev/awacs.c
> > index 335dc26ae8..eda09142e4 100644
> > --- sys/arch/macppc/dev/awacs.c
> > +++ sys/arch/macppc/dev/awacs.c
> > @@ -38,6 +38,7 @@
> > #include <machine/bus.h>
> > #include <machine/autoconf.h>
> > #include <macppc/dev/dbdma.h>
> > +#include <dev/ofw/openfirm.h>/For hardware detection/
>
>
> I don't think that comment is needed.
>
> > #ifdef AWACS_DEBUG
> > # define DPRINTF printf
> > @@ -195,7 +196,11 @@ const struct audio_hw_if awacs_hw_if = {
> > /* cc1 /
> > #define AWACS_MUTE_SPEAKER 0x00000080
> > #define AWACS_MUTE_HEADPHONE 0x00000200
> > -
> > +/
> > + *iMacs that use this driver have a speaker amp power down feature,
> > + * triggered by this bit in cc1.
> > + */
> > +#define AWACS_MUTE_SPEAKER_IMAC 0x00000800
> > const struct awacs_speed_tab {
>
>
> Minor spacing nits: add space between * and i, add a newline after
> the #define.
>
> All of the comments should have spacing added to be consistent with
> style(9): change /comment/ to /* comment */
>
> > int rate;
> > u_int32_t bits;
> > @@ -210,6 +215,22 @@ const struct awacs_speed_tab {
> > { 44100, AWACS_RATE_44100 },
> > };
> >
> > +int awacs_hardware;
> > +int awacs_headphone_mask;
>
>
> Those should probably go in the awacs_softc to make them per-device
> and not global.
>
> > +
> > +/add hardware as needed/
> > +#define HW_IS_OTHER 0
> > +#define HW_IS_IMAC1 1/*iMac (Late 1999) */
> > +#define HW_IS_IMAC2 2/iMac (Summer 2000) and iMac (Early 2001, Summer 2001)/
> > +
> > +/Codecs status headphones mask/
> > +
> > +#define AWACS_STATUS_HP_CONN 8/headphone connector on back/
> > +
> > +#define AWACS_STATUS_HP_RCONN_IMAC 4/right connector on front/
> > +#define AWACS_STATUS_HP_LCONN_IMAC 2/left connector on front/
> > +#define AWACS_STATUS_HP_SCONN_IMAC 1 /connector on the right side/
> > +
>
>
> Add lots of spacing there.
>
> > int
> > awacs_match(struct device *parent, void *match, void *aux)
> > {
> > @@ -240,9 +261,31 @@ awacs_attach(struct device *parent, struct device *self, void *aux)
> > {
> > struct awacs_softc *sc = (struct awacs_softc *)self;
> > struct confargs *ca = aux;
> > + char name[64], *t = NULL;
> > + int len, node, tlen;
> > int cirq, oirq, iirq;
> > int cirq_type, oirq_type, iirq_type;
> >
> > + /detect hardware via OF(code from macppc/mainbus.c)/
> > + node = OF_peer(0);
> > + len = OF_getprop(node, "model", name, sizeof(name));
> > + if (len > 1) {
> > + name[len] = '\0';
> > + tlen = strlen(name)+1;
> > + if ((t = malloc(tlen, M_DEVBUF, M_NOWAIT)) != NULL)
> > + strlcpy(t, name, tlen);
> > + }
>
>
> You're not bailing out of the malloc fails and then continue to
> check against t below. Though you don't need t at all, just use
> name since it's already a string.
>
> > + /t is a 64-char truncated string thar contains the Open Firmware model/
> > + if (!strcmp(t,"PowerMac2,1"))awacs_hardware = HW_IS_IMAC1;
> > + else if (!strcmp(t,"PowerMac2,2") || !strcmp(t,"PowerMac4,1"))
> > + awacs_hardware = HW_IS_IMAC2;
> > + else awacs_hardware = HW_IS_OTHER;
>
>
> Add spacing there, wrap it as in style(9):
>
> if (strcmp(t, "PowerMac2,1"))
> awacs_hardware = HW_IS_IMAC1;
> else if ...
>
> > + /set headphone mask/
> > + if (awacs_hardware == HW_IS_IMAC1 || awacs_hardware == HW_IS_IMAC2)
> > + awacs_headphone_mask = AWACS_STATUS_HP_LCONN_IMAC |
> > + AWACS_STATUS_HP_RCONN_IMAC | AWACS_STATUS_HP_SCONN_IMAC;
> > + else awacs_headphone_mask = AWACS_STATUS_HP_CONN;
> > +
>
>
> Add a newline after else.
>
> > ca->ca_reg[0] += ca->ca_baseaddr;
> > ca->ca_reg[2] += ca->ca_baseaddr;
> > ca->ca_reg[4] += ca->ca_baseaddr;
> > @@ -289,7 +332,9 @@ awacs_attach(struct device *parent, struct device *self, void *aux)
> > sc->sc_codecctl2 = AWACS_CODEC_ADDR2 | AWACS_CODEC_EMSEL0;
> > sc->sc_codecctl4 = AWACS_CODEC_ADDR4 | AWACS_CODEC_EMSEL0;
> >
> > - sc->sc_codecctl0 |= AWACS_INPUT_CD | AWACS_DEFAULT_CD_GAIN;
> > + /don't set CD in on imacs, or else the outputs will be very noisy/
> > + if (awacs_hardware != HW_IS_IMAC1 && awacs_hardware != HW_IS_IMAC2)
> > + sc->sc_codecctl0 |= AWACS_INPUT_CD | AWACS_DEFAULT_CD_GAIN;
> > awacs_write_codec(sc, sc->sc_codecctl0);
> >
> > /* Set initial volume[s] */
> > @@ -302,12 +347,22 @@ awacs_attach(struct device *parent, struct device *self, void *aux)
> > awacs_write_codec(sc, sc->sc_codecctl1);
> >
> > /* check for headphone present */
> > - if (awacs_read_reg(sc, AWACS_CODEC_STATUS) & 0x8) {
> > + if (awacs_read_reg(sc, AWACS_CODEC_STATUS) & awacs_headphone_mask) {
>
>
> Move awacs_headphone_mask to the softc
>
> > /* default output to speakers /
> > printf(" headphones");
> > sc->sc_output_mask = 1 << 1;
> > - sc->sc_codecctl1 &= ~AWACS_MUTE_HEADPHONE;
> > - sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER;
> > + / front headphones on iMacs are wired to the speakers output /
> > + if((awacs_hardware != HW_IS_IMAC2 && awacs_hardware != HW_IS_IMAC1) ||
> > + ((awacs_hardware == HW_IS_IMAC2 || awacs_hardware == HW_IS_IMAC1) &&
> > + (awacs_read_reg(sc, AWACS_CODEC_STATUS) & AWACS_STATUS_HP_SCONN_IMAC))
> > + ){
> > + sc->sc_codecctl1 &= ~AWACS_MUTE_HEADPHONE;
> > + sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER;
> > + }
> > + if (awacs_hardware == HW_IS_IMAC1)
> > + sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER_IMAC;
> > + else if (awacs_hardware == HW_IS_IMAC2)
> > + sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER_IMAC;
> > awacs_write_codec(sc, sc->sc_codecctl1);
> > } else {
> > / default output to speakers */
> > @@ -315,13 +370,18 @@ awacs_attach(struct device *parent, struct device *self, void *aux)
> > sc->sc_output_mask = 1 << 0;
> > sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER;
> > sc->sc_codecctl1 |= AWACS_MUTE_HEADPHONE;
> > + if (awacs_hardware == HW_IS_IMAC1)
> > + sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER_IMAC;
> > + else if (awacs_hardware == HW_IS_IMAC2)
> > + sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER_IMAC;
> > awacs_write_codec(sc, sc->sc_codecctl1);
> > }
> >
> > /* default input from CD */
> > sc->sc_record_source = 1 << 0;
> > sc->sc_codecctl0 &= ~AWACS_INPUT_MASK;
> > - sc->sc_codecctl0 |= AWACS_INPUT_CD;
> > + if (awacs_hardware != HW_IS_IMAC1 && awacs_hardware != HW_IS_IMAC2)
> > + sc->sc_codecctl0 |= AWACS_INPUT_CD;
> > awacs_write_codec(sc, sc->sc_codecctl0);
> >
> > /* Enable interrupts and looping mode. */
> > @@ -372,17 +432,33 @@ awacs_intr(void *v)
> > printf("status = %x\n", awacs_read_reg(sc, AWACS_CODEC_STATUS));
> > #endif
> >
> > - if (awacs_read_reg(sc, AWACS_CODEC_STATUS) & 0x8) {
> > + if (awacs_read_reg(sc, AWACS_CODEC_STATUS) & awacs_headphone_mask) {
> > /* default output to speakers /
> > sc->sc_output_mask = 1 << 1;
> > - sc->sc_codecctl1 &= ~AWACS_MUTE_HEADPHONE;
> > - sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER;
> > + / front headphones on iMacs are wired to the speakers output */
> > + if((awacs_hardware != HW_IS_IMAC2 &&
> > + awacs_hardware != HW_IS_IMAC1) ||
> > + ((awacs_hardware == HW_IS_IMAC2 ||
> > + awacs_hardware == HW_IS_IMAC1) &&
> > + (awacs_read_reg(sc, AWACS_CODEC_STATUS) &
> > + AWACS_STATUS_HP_SCONN_IMAC))){
>
>
> The indenting is hard to follow there. Each wrapped line should be
> indented with 4 spaces as in style(9).
>
> > + sc->sc_codecctl1 &= ~AWACS_MUTE_HEADPHONE;
> > + sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER;
> > + }
> > + if (awacs_hardware == HW_IS_IMAC1)
> > + sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER_IMAC;
> > + else if (awacs_hardware == HW_IS_IMAC2)
> > + sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER_IMAC;
> > awacs_write_codec(sc, sc->sc_codecctl1);
> > } else {
> > /* default output to speakers */
> > sc->sc_output_mask = 1 << 0;
> > sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER;
> > sc->sc_codecctl1 |= AWACS_MUTE_HEADPHONE;
> > + if (awacs_hardware == HW_IS_IMAC1)
> > + sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER_IMAC;
> > + else if (awacs_hardware == HW_IS_IMAC2)
> > + sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER_IMAC;
> > awacs_write_codec(sc, sc->sc_codecctl1);
> > }
> > }
Hi, here is an updated diff that hopefully addresses your comments.
evv42
diff --git sys/arch/macppc/dev/awacs.c sys/arch/macppc/dev/awacs.c
index 335dc26ae8..aa05d91999 100644
--- sys/arch/macppc/dev/awacs.c
+++ sys/arch/macppc/dev/awacs.c
@@ -38,6 +38,7 @@
#include <machine/bus.h>
#include <machine/autoconf.h>
#include <macppc/dev/dbdma.h>
+#include <dev/ofw/openfirm.h>
#ifdef AWACS_DEBUG
# define DPRINTF printf
@@ -70,6 +71,9 @@ struct awacs_softc {
u_int sc_record_source; /* recording source mask */
u_int sc_output_mask; /* output source mask */
+ int sc_awacs_hardware;
+ int sc_awacs_headphone_mask;
+
char *sc_reg;
u_int sc_codecctl0;
u_int sc_codecctl1;
@@ -195,6 +199,11 @@ const struct audio_hw_if awacs_hw_if = {
/* cc1 */
#define AWACS_MUTE_SPEAKER 0x00000080
#define AWACS_MUTE_HEADPHONE 0x00000200
+/*
+ * iMacs that use this driver have a speaker amp power down feature,
+ * triggered by this bit in cc1.
+ */
+#define AWACS_MUTE_SPEAKER_IMAC 0x00000800
const struct awacs_speed_tab {
int rate;
@@ -210,6 +219,25 @@ const struct awacs_speed_tab {
{ 44100, AWACS_RATE_44100 },
};
+/* add hardware as needed */
+#define HW_IS_OTHER 0
+/* iMac (Late 1999) */
+#define HW_IS_IMAC1 1
+/* iMac (Summer 2000) and iMac (Early 2001, Summer 2001) */
+#define HW_IS_IMAC2 2
+
+/* Codecs status headphones mask */
+
+/* headphone connector on back */
+#define AWACS_STATUS_HP_CONN 8
+
+/* right connector on front */
+#define AWACS_STATUS_HP_RCONN_IMAC 4
+/* left connector on front */
+#define AWACS_STATUS_HP_LCONN_IMAC 2
+/* connector on the right side */
+#define AWACS_STATUS_HP_SCONN_IMAC 1
+
int
awacs_match(struct device *parent, void *match, void *aux)
{
@@ -240,9 +268,33 @@ awacs_attach(struct device *parent, struct device *self, void *aux)
{
struct awacs_softc *sc = (struct awacs_softc *)self;
struct confargs *ca = aux;
+ char name[64];
+ int len, node;
int cirq, oirq, iirq;
int cirq_type, oirq_type, iirq_type;
+ /* detect hardware via OF */
+ node = OF_peer(0);
+ len = OF_getprop(node, "model", name, sizeof(name));
+ if (len > 1) {
+ name[len] = '\0';
+ }
+ /* name is a 64-char truncated string thar contains the Open Firmware model */
+ if (!strcmp(name,"PowerMac2,1"))
+ sc->sc_awacs_hardware = HW_IS_IMAC1;
+ else if (!strcmp(name,"PowerMac2,2") ||
+ !strcmp(name,"PowerMac4,1"))
+ sc->sc_awacs_hardware = HW_IS_IMAC2;
+ else
+ sc->sc_awacs_hardware = HW_IS_OTHER;
+ /* set headphone mask */
+ if (sc->sc_awacs_hardware == HW_IS_IMAC1 ||
+ sc->sc_awacs_hardware == HW_IS_IMAC2)
+ sc->sc_awacs_headphone_mask = AWACS_STATUS_HP_LCONN_IMAC |
+ AWACS_STATUS_HP_RCONN_IMAC | AWACS_STATUS_HP_SCONN_IMAC;
+ else
+ sc->sc_awacs_headphone_mask = AWACS_STATUS_HP_CONN;
+
ca->ca_reg[0] += ca->ca_baseaddr;
ca->ca_reg[2] += ca->ca_baseaddr;
ca->ca_reg[4] += ca->ca_baseaddr;
@@ -289,7 +341,10 @@ awacs_attach(struct device *parent, struct device *self, void *aux)
sc->sc_codecctl2 = AWACS_CODEC_ADDR2 | AWACS_CODEC_EMSEL0;
sc->sc_codecctl4 = AWACS_CODEC_ADDR4 | AWACS_CODEC_EMSEL0;
- sc->sc_codecctl0 |= AWACS_INPUT_CD | AWACS_DEFAULT_CD_GAIN;
+ /* don't set CD in on imacs, or else the outputs will be very noisy */
+ if (sc->sc_awacs_hardware != HW_IS_IMAC1 &&
+ sc->sc_awacs_hardware != HW_IS_IMAC2)
+ sc->sc_codecctl0 |= AWACS_INPUT_CD | AWACS_DEFAULT_CD_GAIN;
awacs_write_codec(sc, sc->sc_codecctl0);
/* Set initial volume[s] */
@@ -302,12 +357,25 @@ awacs_attach(struct device *parent, struct device *self, void *aux)
awacs_write_codec(sc, sc->sc_codecctl1);
/* check for headphone present */
- if (awacs_read_reg(sc, AWACS_CODEC_STATUS) & 0x8) {
+ if (awacs_read_reg(sc, AWACS_CODEC_STATUS) & sc->sc_awacs_headphone_mask) {
/* default output to speakers */
printf(" headphones");
sc->sc_output_mask = 1 << 1;
- sc->sc_codecctl1 &= ~AWACS_MUTE_HEADPHONE;
- sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER;
+ /* front headphones on iMacs are wired to the speakers output */
+ if((sc->sc_awacs_hardware != HW_IS_IMAC2 &&
+ sc->sc_awacs_hardware != HW_IS_IMAC1) ||
+ ((sc->sc_awacs_hardware == HW_IS_IMAC2 ||
+ sc->sc_awacs_hardware == HW_IS_IMAC1) &&
+ (awacs_read_reg(sc, AWACS_CODEC_STATUS) &
+ AWACS_STATUS_HP_SCONN_IMAC))
+ ){
+ sc->sc_codecctl1 &= ~AWACS_MUTE_HEADPHONE;
+ sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER;
+ }
+ if (sc->sc_awacs_hardware == HW_IS_IMAC1)
+ sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER_IMAC;
+ else if (sc->sc_awacs_hardware == HW_IS_IMAC2)
+ sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER_IMAC;
awacs_write_codec(sc, sc->sc_codecctl1);
} else {
/* default output to speakers */
@@ -315,13 +383,19 @@ awacs_attach(struct device *parent, struct device *self, void *aux)
sc->sc_output_mask = 1 << 0;
sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER;
sc->sc_codecctl1 |= AWACS_MUTE_HEADPHONE;
+ if (sc->sc_awacs_hardware == HW_IS_IMAC1)
+ sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER_IMAC;
+ else if (sc->sc_awacs_hardware == HW_IS_IMAC2)
+ sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER_IMAC;
awacs_write_codec(sc, sc->sc_codecctl1);
}
/* default input from CD */
sc->sc_record_source = 1 << 0;
sc->sc_codecctl0 &= ~AWACS_INPUT_MASK;
- sc->sc_codecctl0 |= AWACS_INPUT_CD;
+ if (sc->sc_awacs_hardware != HW_IS_IMAC1 &&
+ sc->sc_awacs_hardware != HW_IS_IMAC2)
+ sc->sc_codecctl0 |= AWACS_INPUT_CD;
awacs_write_codec(sc, sc->sc_codecctl0);
/* Enable interrupts and looping mode. */
@@ -372,17 +446,33 @@ awacs_intr(void *v)
printf("status = %x\n", awacs_read_reg(sc, AWACS_CODEC_STATUS));
#endif
- if (awacs_read_reg(sc, AWACS_CODEC_STATUS) & 0x8) {
+ if (awacs_read_reg(sc, AWACS_CODEC_STATUS) & sc->sc_awacs_headphone_mask) {
/* default output to speakers */
sc->sc_output_mask = 1 << 1;
- sc->sc_codecctl1 &= ~AWACS_MUTE_HEADPHONE;
- sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER;
+ /* front headphones on iMacs are wired to the speakers output */
+ if((sc->sc_awacs_hardware != HW_IS_IMAC2 &&
+ sc->sc_awacs_hardware != HW_IS_IMAC1) ||
+ ((sc->sc_awacs_hardware == HW_IS_IMAC2 ||
+ sc->sc_awacs_hardware == HW_IS_IMAC1) &&
+ (awacs_read_reg(sc, AWACS_CODEC_STATUS) &
+ AWACS_STATUS_HP_SCONN_IMAC))){
+ sc->sc_codecctl1 &= ~AWACS_MUTE_HEADPHONE;
+ sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER;
+ }
+ if (sc->sc_awacs_hardware == HW_IS_IMAC1)
+ sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER_IMAC;
+ else if (sc->sc_awacs_hardware == HW_IS_IMAC2)
+ sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER_IMAC;
awacs_write_codec(sc, sc->sc_codecctl1);
} else {
/* default output to speakers */
sc->sc_output_mask = 1 << 0;
sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER;
sc->sc_codecctl1 |= AWACS_MUTE_HEADPHONE;
+ if (sc->sc_awacs_hardware == HW_IS_IMAC1)
+ sc->sc_codecctl1 &= ~AWACS_MUTE_SPEAKER_IMAC;
+ else if (sc->sc_awacs_hardware == HW_IS_IMAC2)
+ sc->sc_codecctl1 |= AWACS_MUTE_SPEAKER_IMAC;
awacs_write_codec(sc, sc->sc_codecctl1);
}
}
awacs(4) - fix sound issues for imac g3