Hi,

On 12/7/07, Michael Lorenz <macallan@netbsd.org> wrote:
> On Dec 7, 2007, at 09:06, Marco Trillo wrote:
> > On 12/7/07, Michael Lorenz <macallan@netbsd.org> wrote:
> >> On Dec 6, 2007, at 17:45, Marco Trillo wrote:
> >>> The problem is that the sound-objects parser uses these callbacks
> >>> to inform
> >>> the driver that a device has changed its state (for example when
> >>> updating the
> >>> state as a result of a port change interrupt). So the driver
> >>> registers a callback
> >>> for each device it cares about. This callback is called when the
> >>> device changes
> >>> its state.
> >>
> >> So, it registers an interrupt handler? I don't know, my gut feeling
> >> is that the parser should just parse the property, nothing else. But
> >> that's just a gut feeling.
> >
> > It registers an interrupt handler if the property contains references
> > to extint-gpio detect nodes. In the case of the screamer, the awacs
> > driver calls the aoa_cintr() handler from its port change interrupt
> > handler (these type of detects have a 'model' attribute with the value
> > 'InSenseBitsDetect' which differences them of the gpio detects which
> > have the value 'GPIODetect' or 'GenericGPIODetect').
> >
> > Since the property contains all the necessary masks/match values, the
> > intention was for the 'aoa' module to take care of all of this.
>
> That might Just Work with i2s but awacs is rather different here, the
> status interrupt can signal a few more things than just gpio changes.
> We don't use any of that just yet but we might want to at some point.

The current code should already handle that:

awacs_status_intr(void *cookie)
{
 	[...]
	reg = awacs_read_reg(sc, AWACS_SOUND_CTRL);
	if (reg & AWACS_PORTCHG) {
		if (sc->sc_aoa)
			aoa_cintr(sc->sc_aoa);
		else
			awacs_select_output(sc, awacs_check_output(sc));
			
		/* clear the interrupt */
		awacs_write_reg(sc, AWACS_SOUND_CTRL,
				sc->sc_soundctl | AWACS_PORTCHG);
	}
	[...]
}

So if the interrupt is not AWACS_PORTCHG, the aoa_cintr() function
should not be called.

> > By the way, my original plan of making the codec drivers
> > machine-independent files in sys/dev/i2c/ is starting to being botched
> > by the 'daca' setups requiring some configuration for the output
> > device selection: instead of using gpio mutes like the other codecs,
> > they use a control in the codec to switch the polarity of the output
> > depending on what device is being used, the internal mono speaker or
> > headphones.
>
> One more reason to let the codec drivers fiddle with gpios.

Yes, the 'i2s' driver still has to deal with gpios anyway because it
needs the 'amp-mute', 'headphone-mute' and other mute gpios which are
not provided in the sound-objects. On the other hand, the i2c codec
drivers should not need to deal with the gpio stuff, instead they
(actually only daca) implement a method to 'configure the output
device'.

Talking about the 'i2s' driver, there is some code in it that looks like this:

out32rb(sc->sc_baseaddr + KEYLARGO_FCR1, x);

However, 'sc_baseaddr' is the physical address of the mac-io space
(which should be 0x80000000 in the KeyLargo). I think this is working
because the mac-io space is being mapped by machdep.c using the BAT
facility:

oea_batinit(0x80000000, BAT_BL_256M, 0xf0000000, BAT_BL_256M,
    0x90000000, BAT_BL_256M, 0xa0000000, BAT_BL_256M,
    0xb0000000, BAT_BL_256M, 0);

(Well, that's in netbsd-4, I don't know where that code moved to in
-current, but the snapper driver in -current still uses the physical
address so I assume that this BAT mapping is still being done.)

However I think that BAT mapping will not work on the G5, since it
does not provide the BAT facility.
What do you think of making obio(4) map the FCR space and provide
abstraction functions to read and write to the registers:

obio_write_fcr(OBIO_FCR1, x);

Another thing related to the codecs... some codecs provide an 'analog
power down' or 'low power' feature; the current i2s code sets a
timeout of 20 seconds since the device is closed and then calls the
poweroff function of the codec.

However I don't think that this policy should be implemented by the
i2s driver, but with the current audio(4) infrastructure is the only
possible way (since the audio_hw_if::powerstate member alone is
unusable, since for example we don't want to power on the codec each
time a mixer value is changed, and it does not implement any sort of
timeout).
Maybe the 'jmcneill-pm' branch provides a better framework in audio(4)
for implementing this.

> >>> Using properties would work to see the list of available devices,
> >>> but will not allow
> >>> the parser to take care of the device state changes (but I may be
> >>> missing something).
> >>
> >> Shouldn't individual drivers take care of that?
> >
> > Maybe instead of making the 'aoa' module take care of the interrupt,
> > the driver can register the handler based on the data from the
> > property, read the sense bits and then pass the sense bits to an aoa
> > function.
> >
> > This way may be cleaner especially for the awacs driver.
>
> Couldn't agree more.

I'm still not very convinced; while this would work fine for awacs
which does only have a single source of the 'sense bits', gpio-based
detects can have multiple addresses to read. In the current code all
of the process is centralized in a single place in aoa.c.

> >>>>> At the moment I've tested it with the awacs driver in a screamer-
> >>>>> based
> >>>>> G4 (which is also a plain screamer with only an internal speaker
> >>>>> and a
> >>>>> headphones port). The good thing about the sound-objects
> >>>>> property is
> >>>>> that it eliminates the need to hardcode different GPIO wirings for
> >>>>> different machines.
> >>>>
> >>>> Neither the PB3400c nor the beige G3 have sound-objects but both
> >>>> need
> >>>> special wiring for their gpios ( it's reversed on the powerbook and
> >>>> uses a different pin on the G3 as you probably saw in awacs.c )
> >>>
> >>> Yeah, I forgot about these especial cases. Well, at least we don't
> >>> need more
> >>> special cases for some G4s or some other screamer-based models.
> >>
> >> I wouldn't bet on it. Actually I'm pretty sure some more *Books will
> >> need special attention.
> >
> > Hmm... with Apple's inconsistency it will not be a surprise. But
> > looking at the Darwin's screamer driver the only special case I can
> > see is the 'init operation' I commented (the programmable outputs).
> > This apparently only affects G4's.
>
> The new world powerbooks will likely have something i2s-based or a
> Screamer, the older ones on the other hand might be weirder and
> likely have additional inputs. On the other hand that's probably not
> much to worry about until one of us actually gets his hands on such
> hardware. One of these days I'll probably figure out how to talk to
> the 3400c's modem just for the hell of it ( there's a 2nd ohare mac-
> io, its interrupt controller cascaded to the onboard ohare. I suspect
> one of its serial ports is wired to the modem but it appears to need
> some fiddling in some register somewhere to power up ) - I bet the
> modem's audio output is wired somehow to the onboard awacs even
> though it's not a screamer.

Yeah, and it probably uses the loopthrough feature of the awacs so the
modem sounds can be heared from the standard outputs.

>
> >>> I can try hardcoding irq 58 for daca-based machines... and if the
> >>> testing succeeds, then fine.
> >>
> >> Hmm, the headphone detect gpio probably doesn't have much to do with
> >> the codec, more with the mac-io model? My iBook has:
> >> model                   4141504c 2c4b6579 6c617267 6f00....
> >> "AAPL,Keylargo"
> >> probably too generic to be useful.
> >
> > Yeah... the tress I know that have this 'extint-gpio12' stuff are the
> > PowerBook2,1 and the PowerBook2,2 daca-based iBook G3's.
> > But of course the parser should handle this automatically, so no
> > special cases are required -- except for this 'irq 58' thing.
>
> *nod*

So let's hardcode irq 58. If it doesn't work then we can try with irq
61, and if that still doesn't work ... either don't implement
automatic switch at all or use some sort of polling.
I see that the awacs driver in -current has an 'awacs_thread' that
seems to be related to this. Maybe a solution like that would be
feasible for daca.

>
> > The 'onyx' and 'topaz' codecs are used in newer G5-based models. There
> > also seem to be machines with two codecs, for example PowerMac G5s
> > which have a TAS3004 for analog output and a 'topaz' for digital
> > output.
>
> Yeah, the DMA machinery looks like it supports at least 4 channels
> even on awacs models - probably for video IO on the beige G3 and AV
> models?

The problem is that the current i2s driver assumes only one codec.
Well, at least the TAS3004 analog output should work on the G5s.  If
someone with a G5 is interested, support for the 'onyx' codec in
analog mode shouldn't be difficult to write either. The digital stuff
may require some twiddling with the i2s driver...

Greetings,
Marco.