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. -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
