On Mon, Mar 14, 2016 at 05:21:54PM +0200, Ville Syrjälä wrote: > On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: > > On 3/11/16 1:09 PM, Ville Syrjälä wrote: > > > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > >> Thanks for the review Ville > > >> > > >> [snip] > > >> > > >>> Kinda hard to see where everything gets used due to the way the patches > > >>> are split up. > > >> > > >> Yes, it's far from great... > > >> > > >>> At least the hotplug/mode change events are not needed. We only have the > > >>> two points where i915 should inform the audio driver about this stuff, > > >>> and those are the intel_audio_code_enable/disable(). For that we > > >>> already have the .pin_eld_notify() hook. > > >>> > > >>> The interrupt stuff should mostly vanish from i915 with the subdevice > > >>> approach. As in i915 would just call the interrupt handler of the audio > > >>> driver based on the LPE bits in IIR, and the audio driver can then do > > >>> whatever it wants based on its own status register. > > >> > > >> Are you saying that the subdevice would provide a read/write interface > > >> for the audio driver to look at display registers, and the i915 driver > > >> would only provide a notification interface (EDID and interrupts) to the > > >> audio driver? > > > > > > The audio driver would simply ioremap the appropriate range of > > > registers itself. > > > > > >> If yes, would there be two component framework links, one between > > >> i915/audio driver and one between subdevice/audio driver. > > > > > > Yeah sort of. i915 registers the platform device for the audio, the > > > audio driver can then bind to the device via the platform driver .probe > > > callback. It can then register with the audio component stuff at some > > > point to get the relevant notifications on the display state. When > > > i915 gets unloaded we remove the platform device, at which point the > > > audio driver's platform driver .remove() callback gets invoked and > > > it should unregister/cleanup everything. > > > > we don't want to expose this interface when HDAudio is present and > > enabled so we would need to add a test for this. > > Also it looks like you want the creation of the platform device done in > > i915, we were thinking of doing this as part of the audio drivers but in > > the end this looks equivalent. In both cases we would end-up ignoring > > the HAD022A8 HID and not use acpi for this extension > > Well, if you have a device you can hang off from then i915 should need > to register it I suppose. Though that would make the interrupt > forwarding perhaps less nice. There's also the suspend/resume order > dependency to deal with if there's no parent/child relationship between > the devices. Oh and I suppose you'd end up ioremapping part of the pci dev2 mmio bar to get at the registers, which would look a bit funkly at least. > > > > > > I just tried to frob around with the VED code a bit, and got it to load > > > at least. It's not quite happy about reloading i915 while the ipvr > > > driver was loaded though. Not sure what's going on there, but I do > > > think this approach should work. So the VED patch could serve as a > > > decent enough model to follow. > > > > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
