From: evv42 Subject: Re: awacs(4) - fix sound issues for imac g3 To: joshua stein , "tech@openbsd.org" Date: Thu, 15 Feb 2024 13:43:44 +0000 On Thursday, 18 January 2024 at 16:32, joshua stein 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 > > #include > > #include > > +#include /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 #include #include +#include #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); } }