[PATCH 00/15] ASoC: hdac_hdmi: Add DP & notification support

[PATCH 00/15] ASoC: hdac_hdmi: Add DP & notification support

[Subhransu S. Prusty]

This patch series adds DP audio and hotplug notification
support.

On Skylake two DP ports are available and to enable DP on both
ports all pins need to be enabled.

There is a special vendor widget which need to be programmed to
enable all pins and converters. This series adds hotplug
notification, read/set constraint based on ELD, enable all
pin/cvts, DP1.2, programs the audio infoframe for DP. There is
a one to one mapping between converter and stream, so the dais
are created based on the no of streams supported on hdmi codec.
Even though cvts can be mapped dynamically to the streams,
currently it is statically mapped as simultaneous playback on
both DP and HDMI is not supported as of now.

Pin muxes and controls are created dynamically to map converter
to pin widget. So at run time specific pin is mapped to the dai
based on the control selected (based on the display type DP/HDMI
connected).

Finally the DP audio infoframe programming is added to support
the DP feature. 

Also with hotplug notification support, ELD is read and
capabilities are set for rate, formats and channels. drm_eld
sound/core framework is updated to limit the formats based on
ELD.

There are few fixes one fixing the static checker warning and
other one not to fail if no connection list is found for a pin
widget.


Pls note, the 12th patch is adding a small macro for getting
connection type in drm header, we have CCed drm folks on that and
this one. Pls ack so that we can have this series merged thru
sound trees

Jeeja KP (1):
  ASoC: hdac_hdmi: Add codec suspend/resume handler

Ramesh Babu (1):
  ASoC: hdac_hdmi: Keep display active while enumerating codec

Subhransu S. Prusty (13):
  ASoC: hdac_hdmi: Fix to check num nodes correctly
  ASoC: hdac_hdmi: Fix to warn instead of err for no connected nids
  ASoC: hdac_hdmi - Use list to add pins and converters
  ALSA: hda - Add helper to read eld data
  ASoC: hdac_hdmi: Add hotplug notification and read eld
  ALSA: pcm: Add DRM helper to set constraint for format
  ASoC: hdac_hdmi: Apply constraints based on ELD
  ASoC: hdac_hdmi: Enable DP1.2 and all converters/pins
  ASoC: hdac_hdmi - create dais based on number of streams
  ASoC: hdac_hdmi: Create widget/route based on nodes enumerated
  ASoC: hdac_hdmi: Assign pin for stream based on dapm connection
  drm/edid: Add API to help find connection type
  ASoC: hdac_hdmi: Add infoframe support for dp audio

 include/drm/drm_edid.h       |  10 +
 include/sound/hdaudio.h      |   3 +
 sound/core/pcm_drm_eld.c     |  42 +-
 sound/hda/Makefile           |   2 +-
 sound/hda/hdac_eld.c         |  95 +++++
 sound/soc/codecs/Kconfig     |   1 +
 sound/soc/codecs/hdac_hdmi.c | 908 ++++++++++++++++++++++++++++++++++++-------
 7 files changed, 908 insertions(+), 153 deletions(-)
 create mode 100644 sound/hda/hdac_eld.c

-- 
1.9.1

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

[Subhransu S. Prusty]

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;
+}
+
 struct edid *drm_do_get_edid(struct drm_connector *connector,
 	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
 			      size_t len),
-- 
1.9.1

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

[Jani Nikula]

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.

BR,
Jani.


> +
>  struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
>  			      size_t len),

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2115 bytes --]

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;
	}

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Subhransu S. Prusty]

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

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

[Jani Nikula]

On Thu, 03 Dec 2015, "Subhransu S. Prusty" <subhransu.s.prusty@intel.com> wrote:
> 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.

I presume Thierry means you'd assume HDMI if drm_eld_detect_dp() returns
false.

I'm fine with either approach.

BR,
Jani.

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3503 bytes --]

On Thu, Dec 03, 2015 at 01:09:16PM +0200, Jani Nikula wrote:
> On Thu, 03 Dec 2015, "Subhransu S. Prusty" <subhransu.s.prusty@intel.com> wrote:
> > 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.
> 
> I presume Thierry means you'd assume HDMI if drm_eld_detect_dp() returns
> false.

Yes, that's what I was implying. This is probably highly subjective, but
I personally find boolean return values much easier to deal with because
of the limited set of values. With drm_eld_get_conn_type() you'd need to
explicitly check == DRM_ELD_CONN_TYPE_HDMI and == DRM_ELD_CONN_TYPE_DP
and then still have special code to reject all other values. Unless of
course if you consider != DRM_ELD_CONN_TYPE_DP as being HDMI, in which
case a boolean is much more concise.

But like I said, this is just my opinion, and I don't feel strongly
enough to object to the current patch.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Subhransu S. Prusty]

On Thu, Dec 03, 2015 at 12:21:42PM +0100, Thierry Reding wrote:
> On Thu, Dec 03, 2015 at 01:09:16PM +0200, Jani Nikula wrote:
> > On Thu, 03 Dec 2015, "Subhransu S. Prusty" <subhransu.s.prusty@intel.com> wrote:
> > > 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.
> > 
> > I presume Thierry means you'd assume HDMI if drm_eld_detect_dp() returns
> > false.
> 
> Yes, that's what I was implying. This is probably highly subjective, but
> I personally find boolean return values much easier to deal with because
> of the limited set of values. With drm_eld_get_conn_type() you'd need to
> explicitly check == DRM_ELD_CONN_TYPE_HDMI and == DRM_ELD_CONN_TYPE_DP
> and then still have special code to reject all other values. Unless of

I don't know what does the second bit mean in the connection type. So was
just planning to reject anything other that DP/HDMI. If that bit doesn't
carry any information, then yes I would also prefer returning a boolean.

> course if you consider != DRM_ELD_CONN_TYPE_DP as being HDMI, in which
> case a boolean is much more concise.
> 
> But like I said, this is just my opinion, and I don't feel strongly
> enough to object to the current patch.
> 
> Thierry



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

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

[Subhransu S. Prusty]

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.

Sure, will update and submit again.

Regards,
Subhransu
> 
> BR,
> Jani.
> 
> 
> > +
> >  struct edid *drm_do_get_edid(struct drm_connector *connector,
> >  	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
> >  			      size_t len),
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

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