On Tue, Mar 15, 2016 at 03:35:45PM +0200, Ville Syrjälä wrote: > On Tue, Mar 15, 2016 at 09:39:48AM +0100, Daniel Vetter wrote: > > On Mon, Mar 14, 2016 at 12:27:51PM -0500, Pierre-Louis Bossart wrote: > > > On 3/14/16 10:30 AM, Ville Syrjälä wrote: > > > >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 understand the benefits of a parent/child device/subdevice model. What I > > > don't see is whether we need the component framework at all here? > > > It was used in the case of HDaudio since both i915 and HDaudio controllers > > > get probed at different times, here the HDMI interface is just a bunch of > > > registers/DMA from the same hardware so the whole master/client interface > > > exposed by the component framework to 'bind' independent drivers is a bit > > > overkill? > > > > I haven't read the patches, but using component.c when you instead can > > model it with parent/child smells like abuse. Component.c is meant for > > when you have multiple devices all over the device tree that in aggregate > > constitute the overall logical driver. This doesn't seem to be the case > > here. > > We still need the eld notify hooks so that i915 can inform the audio > driver about the state of the display. Whether that's best done via > the component stuff or something else I don't know. Hm right. I guess we could stuff it into the platform data stuff maybe instead, so the driver could get at the i915/snd interface directly? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
