> -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] > Sent: Monday, January 23, 2017 4:24 PM > To: Anand, Jerome <jerome.anand@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx > Cc: tiwai@xxxxxxx; broonie@xxxxxxxxxx; Ughreja, Rakesh A > <rakesh.a.ughreja@xxxxxxxxx> > Subject: Re: [PATCH v4 1/5] drm/i915: setup bridge for HDMI LPE > audio driver > > On Sat, 21 Jan 2017, Jerome Anand <jerome.anand@xxxxxxxxx> wrote: > > Enable support for HDMI LPE audio mode on Baytrail and Cherrytrail > > when HDaudio controller is not detected > > > > Setup minimum required resources during i915_driver_load: > > 1. Create a platform device to share MMIO/IRQ resources 2. Make the > > platform device child of i915 device for runtime PM. > > 3. Create IRQ chip to forward HDMI LPE audio irqs. > > > > HDMI LPE audio driver (a standalone sound driver) probes the LPE audio > > device and creates a new sound card. > > > > Signed-off-by: Pierre-Louis Bossart > > <pierre-louis.bossart@xxxxxxxxxxxxxxx> > > Signed-off-by: Jerome Anand <jerome.anand@xxxxxxxxx> > > Some comments inline. This is not a detailed technical review, more a high > level glance at interfacing with the rest of i915. > Sure - thanks for the quick review Jani > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c index 4d22b4b..70d728b 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1131,7 +1131,8 @@ static void i915_driver_register(struct > drm_i915_private *dev_priv) > > if (IS_GEN5(dev_priv)) > > intel_gpu_ips_init(dev_priv); > > > > - i915_audio_component_init(dev_priv); > > + if (intel_lpe_audio_init(dev_priv) < 0) > > + i915_audio_component_init(dev_priv); > > I'd like this abstracted behind a single intel_audio_init() in intel_audio.c, > which would do the right thing. > Ok - I'll create two new API's in intel_audio.c > > > > /* > > * Some ports require correctly set-up hpd registers for detection > > to @@ -1149,7 +1150,10 @@ static void i915_driver_register(struct > drm_i915_private *dev_priv) > > */ > > static void i915_driver_unregister(struct drm_i915_private *dev_priv) > > { > > - i915_audio_component_cleanup(dev_priv); > > + if (HAS_LPE_AUDIO(dev_priv)) > > + intel_lpe_audio_teardown(dev_priv); > > + else > > + i915_audio_component_cleanup(dev_priv); > > Same here for cleanup. > OK > > > > intel_gpu_ips_teardown(); > > acpi_video_unregister(); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h index f66eeede..cc7033a 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2449,6 +2449,12 @@ struct drm_i915_private { > > /* Used to save the pipe-to-encoder mapping for audio */ > > struct intel_encoder *av_enc_map[I915_MAX_PIPES]; > > > > + /* necessary resource sharing with HDMI LPE audio driver. */ > > + struct { > > + struct platform_device *platdev; > > + int irq; > > + } lpe_audio; > > + > > I think the av_enc_map array is misplaced within dev_priv, and these should > really be closer to the audio_component etc. members. But could be cleaned > up later too. > Will cleanup later. > > /* > > * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your > patch > > * will be rejected. Instead look for a better place. > > @@ -2848,6 +2854,8 @@ intel_info(const struct drm_i915_private > > *dev_priv) > > > > #define HAS_POOLED_EU(dev_priv) ((dev_priv)->info.has_pooled_eu) > > > > +#define HAS_LPE_AUDIO(dev_priv) ((dev_priv)->lpe_audio.platdev != > > +NULL) > > Please note that *all* of our HAS_ macros check for static features of the > hardware, not dynamic state of the driver. Let's keep it that way. > OK - will remove it from here. > If you abstract the init/cleanup in intel_audio.c like I suggested, I think you > can just check for dev_priv->lpe_audio.platdev directly, and get rid of > HAS_LPE_AUDIO() altogether. Then it'll all be within intel_audio.c or > intel_lpe_audio.c, and it's pretty obvious what it means. > OK > > +/** > > + * intel_lpe_audio_detect() - check & setup lpe audio if present > > + * @dev_priv: the i915 drm device private data > > + * > > + * Detect if lpe audio is present > > + * > > + * Return: true if lpe audio present else Return = false */ bool > > +intel_lpe_audio_detect(struct drm_i915_private *dev_priv) > > Could this not be static? > OK - wanted it to be as part of the interface. Not used anywhere now, so will make it static > > +/** > > + * intel_lpe_audio_setup() - setup the bridge between HDMI LPE Audio > > + * driver and i915 > > + * @dev_priv: the i915 drm device private data > > + * > > + * set up the minimum required resources for the bridge: irq chip, > > + * platform resource and platform device. i915 device is set as > > +parent > > + * of the new platform device. > > + * > > + * Return: 0 if successful. non-zero if allocation/initialization > > +fails */ int intel_lpe_audio_setup(struct drm_i915_private > > +*dev_priv) > > Could this not be static? > OK > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
