Re: [alsa-devel] [PATCH 12/15] drm/edid: Add API to help find connection type

From: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: alsa-devel@alsa-project.org, patches.audio@intel.com,
	lgirdwood@gmail.com, dri-devel@lists.freedesktop.org,
	Vinod Koul <vinod.koul@intel.com>,
	broonie@kernel.org, Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [alsa-devel] [PATCH 12/15] drm/edid: Add API to help find connection type
Date: Thu, 3 Dec 2015 21:38:24 +0530	[thread overview]
Message-ID: <20151203160824.GA26240@subhransu-desktop> (raw)
In-Reply-To: <20151202170701.GB14019@ulmo>

On Wed, Dec 02, 2015 at 06:07:01PM +0100, Thierry Reding wrote:
> On Wed, Dec 02, 2015 at 11:53:02AM +0200, Jani Nikula wrote:
> > On Tue, 01 Dec 2015, "Subhransu S. Prusty" <subhransu.s.prusty@intel.com> wrote:
> > > To fill the audio infoframe it is required to identify the connection type
> > > as DP or HDMI. So parse the required bits in ELD to find the connection
> > > type.
> > >
> > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  include/drm/drm_edid.h | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > > index 2af9769..c7595a5 100644
> > > --- a/include/drm/drm_edid.h
> > > +++ b/include/drm/drm_edid.h
> > > @@ -403,6 +403,16 @@ static inline int drm_eld_size(const uint8_t *eld)
> > >  	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
> > >  }
> > >  
> > > +/**
> > > + * drm_eld_get_conn_type - Get device type hdmi/dp connected
> > > + * @eld: pointer to an eld memory structure
> > > + */
> > > +static inline int drm_eld_get_conn_type(const uint8_t *eld)
> > > +{
> > > +	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK) >>
> > > +		DRM_ELD_CONN_TYPE_SHIFT;
> > > +}
> > 
> > I'm not sure how much this helps when the caller still needs to
> > magically know what the return value means...  Indeed the next patch
> > with /* 0 is hdmi and 1 is DP */ and "conn_type == 0" is a bit ugly.
> > 
> > How about just not shifting the return value, and using
> > DRM_ELD_CONN_TYPE_HDMI and DRM_ELD_CONN_TYPE_DP in the caller? Bonus
> > points for referencing those in the kernel-doc above.
> 
> We already have a similar function for detecting HDMI vs. DVI (see the
> drm_detect_hdmi_monitor()), so perhaps adhering to that convention might
> be preferable. This could be:
> 
> 	static inline bool drm_eld_detect_dp(const u8 *eld)
> 	{
> 		u8 type = eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK;
> 
> 		return type == DRM_ELD_CONN_TYPE_DP;
> 	}

With this approach it needs two APIs to be added for HDMI or DP
detection. So I prefer what Jani suggested and caller compares
whether it is HDMI/DP connection type. Will updae the kernel doc
for the same as well.

> 
> Thierry

> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

-- 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel