Re: [PATCH v4 2/5] drm/i915: Add support for audio driver notifications

From: "Anand, Jerome" <jerome.anand@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>
Cc: "tiwai@suse.de" <tiwai@suse.de>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"Ughreja, Rakesh A" <rakesh.a.ughreja@intel.com>
Subject: Re: [PATCH v4 2/5] drm/i915: Add support for audio driver notifications
Date: Tue, 24 Jan 2017 08:42:20 +0000	[thread overview]
Message-ID: <F6867404AABACB4D8EC371BF802A4B7C149608D9@fmsmsx101.amr.corp.intel.com> (raw)
In-Reply-To: <87bmuxdjs5.fsf@intel.com>

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Tuesday, January 24, 2017 12:31 PM
> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; Anand,
> Jerome <jerome.anand@intel.com>; intel-gfx@lists.freedesktop.org; alsa-
> devel@alsa-project.org
> Cc: tiwai@suse.de; broonie@kernel.org; Ughreja, Rakesh A
> <rakesh.a.ughreja@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v4 2/5] drm/i915: Add support for audio driver
> notifications
> 
> On Mon, 23 Jan 2017, Pierre-Louis Bossart <pierre-
> louis.bossart@linux.intel.com> wrote:
> >>>  #include <drm/drmP.h>
> >>> @@ -630,6 +631,10 @@ void intel_audio_codec_enable(struct
> intel_encoder *intel_encoder,
> >>>  	if (acomp && acomp->audio_ops && acomp->audio_ops-
> >pin_eld_notify)
> >>>  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> >audio_ptr,
> >>>  						 (int) port, (int) pipe);
> >>> +
> >>> +	if (HAS_LPE_AUDIO(dev_priv))
> >>> +		intel_lpe_audio_notify(dev_priv, connector->eld, port,
> >>> +			crtc_state->port_clock);
> >>
> >> Seems unnecessary to check for HAS_LPE_AUDIO (which you'll change to
> >> dev_priv->lpe_audio.platdev, right ;) both in the caller and callee.
> >> Pick one.
> >
> > If we test inside of the function, it'd mean an unconditional jump to
> > test a feature that exists on only two platforms out of the dozen or
> > so that this i915 driver handles. No objection to do the change but is
> > this really desired?
> 
> *shrug* no big deal to check in both.
> 

Thanks for the ack Jani.
It was added based on one review comment. I'll remove it here and have the check only inside the API

> BR,
> Jani.
> 
> >
> >
> 
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx