[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 01/15] ASoC: hdac_hdmi: Fix to check num nodes correctly

[Subhransu S. Prusty]

This patch fixes below static checker warning.
	sound/soc/codecs/hdac_hdmi.c:416 hdac_hdmi_parse_and_map_nid()
	warn: unsigned 'hdac->num_nodes' is never less than zero.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 205f2c2..65596b9 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -415,7 +415,7 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
 	int cvt_nid = 0, pin_nid = 0;
 
 	hdac->num_nodes = snd_hdac_get_sub_nodes(hdac, hdac->afg, &nid);
-	if (!nid || hdac->num_nodes < 0) {
+	if (!nid || hdac->num_nodes <= 0) {
 		dev_warn(&hdac->dev, "HDMI: failed to get afg sub nodes\n");
 		return -EINVAL;
 	}
-- 
1.9.1

Re: [PATCH 01/15] ASoC: hdac_hdmi: Fix to check num nodes correctly

[Takashi Iwai]

On Tue, 01 Dec 2015 18:46:57 +0100,
Subhransu S. Prusty wrote:
> 
> This patch fixes below static checker warning.
> 	sound/soc/codecs/hdac_hdmi.c:416 hdac_hdmi_parse_and_map_nid()
> 	warn: unsigned 'hdac->num_nodes' is never less than zero.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  sound/soc/codecs/hdac_hdmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 205f2c2..65596b9 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -415,7 +415,7 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
>  	int cvt_nid = 0, pin_nid = 0;
>  
>  	hdac->num_nodes = snd_hdac_get_sub_nodes(hdac, hdac->afg, &nid);
> -	if (!nid || hdac->num_nodes < 0) {
> +	if (!nid || hdac->num_nodes <= 0) {

Checking zero is good, but checking negative is wrong here for
unsigned int.


Takashi

>  		dev_warn(&hdac->dev, "HDMI: failed to get afg sub nodes\n");
>  		return -EINVAL;
>  	}
> -- 
> 1.9.1
> 

Re: [PATCH 01/15] ASoC: hdac_hdmi: Fix to check num nodes correctly

[Subhransu S. Prusty]

On Tue, Dec 01, 2015 at 01:27:13PM +0100, Takashi Iwai wrote:
> On Tue, 01 Dec 2015 18:46:57 +0100,
> Subhransu S. Prusty wrote:
> > 
> > This patch fixes below static checker warning.
> > 	sound/soc/codecs/hdac_hdmi.c:416 hdac_hdmi_parse_and_map_nid()
> > 	warn: unsigned 'hdac->num_nodes' is never less than zero.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> >  sound/soc/codecs/hdac_hdmi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> > index 205f2c2..65596b9 100644
> > --- a/sound/soc/codecs/hdac_hdmi.c
> > +++ b/sound/soc/codecs/hdac_hdmi.c
> > @@ -415,7 +415,7 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
> >  	int cvt_nid = 0, pin_nid = 0;
> >  
> >  	hdac->num_nodes = snd_hdac_get_sub_nodes(hdac, hdac->afg, &nid);
> > -	if (!nid || hdac->num_nodes < 0) {
> > +	if (!nid || hdac->num_nodes <= 0) {
> 
> Checking zero is good, but checking negative is wrong here for
> unsigned int.

Thanks for pointing. Will update.
> 
> 
> Takashi
> 
> >  		dev_warn(&hdac->dev, "HDMI: failed to get afg sub nodes\n");
> >  		return -EINVAL;
> >  	}
> > -- 
> > 1.9.1
> > 

-- 

[PATCH 02/15] ASoC: hdac_hdmi: Fix to warn instead of err for no connected nids

[Subhransu S. Prusty]

It is possible that some pin widget may return with no converter
connected. So don't throw error if none are found to be connected.
Instead print a warning and continue.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 65596b9..3b2fb87 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -316,10 +316,12 @@ static int hdac_hdmi_query_pin_connlist(struct hdac_ext_device *hdac,
 
 	pin->num_mux_nids = snd_hdac_get_connections(&hdac->hdac, pin->nid,
 			pin->mux_nids, HDA_MAX_CONNECTIONS);
-	if (pin->num_mux_nids == 0) {
-		dev_err(&hdac->hdac.dev, "No connections found\n");
-		return -ENODEV;
-	}
+	if (pin->num_mux_nids == 0)
+		dev_warn(&hdac->hdac.dev, "No connections found for pin: %d\n",
+								pin->nid);
+
+	dev_dbg(&hdac->hdac.dev, "num_mux_nids %d for pin: %d\n",
+			pin->num_mux_nids, pin->nid);
 
 	return pin->num_mux_nids;
 }
-- 
1.9.1

Applied "ASoC: hdac_hdmi: Fix to warn instead of err for no connected nids" to the asoc tree

[Mark Brown]

The patch

   ASoC: hdac_hdmi: Fix to warn instead of err for no connected nids

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 5307246015bceb2758f1eee078c6bdc8545ac91f Mon Sep 17 00:00:00 2001
From: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
Date: Wed, 9 Dec 2015 21:46:09 +0530
Subject: [PATCH] ASoC: hdac_hdmi: Fix to warn instead of err for no connected
 nids

It is possible that some pin widget may return with no converter
connected. So don't throw error if none are found to be connected.
Instead print a warning and continue.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/hdac_hdmi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index e6dc4cd037d3..41117e130ce0 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -316,10 +316,12 @@ static int hdac_hdmi_query_pin_connlist(struct hdac_ext_device *hdac,
 
 	pin->num_mux_nids = snd_hdac_get_connections(&hdac->hdac, pin->nid,
 			pin->mux_nids, HDA_MAX_CONNECTIONS);
-	if (pin->num_mux_nids == 0) {
-		dev_err(&hdac->hdac.dev, "No connections found\n");
-		return -ENODEV;
-	}
+	if (pin->num_mux_nids == 0)
+		dev_warn(&hdac->hdac.dev, "No connections found for pin: %d\n",
+								pin->nid);
+
+	dev_dbg(&hdac->hdac.dev, "num_mux_nids %d for pin: %d\n",
+			pin->num_mux_nids, pin->nid);
 
 	return pin->num_mux_nids;
 }
-- 
2.7.0.rc3

[PATCH 03/15] ASoC: hdac_hdmi - Use list to add pins and converters

[Subhransu S. Prusty]

Future platforms may have a different set of pins/converters.
So use lists to add pins and converters based on enumeration.

Also it may be required to connect any converter to any pin
dynamically as per different use cases (for example DP is
connected to pin 6 on skylake board). So this will help in
dynamically select and route.

Fix the dai map as well to use the pin/cvt from list. Not
enabling all dai maps for now.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 140 ++++++++++++++++++++++++++++---------------
 1 file changed, 92 insertions(+), 48 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 3b2fb87..0869855 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -43,11 +43,13 @@ struct hdac_hdmi_cvt_params {
 };
 
 struct hdac_hdmi_cvt {
+	struct list_head head;
 	hda_nid_t nid;
 	struct hdac_hdmi_cvt_params params;
 };
 
 struct hdac_hdmi_pin {
+	struct list_head head;
 	hda_nid_t nid;
 	int num_mux_nids;
 	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
@@ -55,14 +57,16 @@ struct hdac_hdmi_pin {
 
 struct hdac_hdmi_dai_pin_map {
 	int dai_id;
-	struct hdac_hdmi_pin pin;
-	struct hdac_hdmi_cvt cvt;
+	struct hdac_hdmi_pin *pin;
+	struct hdac_hdmi_cvt *cvt;
 };
 
 struct hdac_hdmi_priv {
-	hda_nid_t pin_nid[3];
-	hda_nid_t cvt_nid[3];
 	struct hdac_hdmi_dai_pin_map dai_map[3];
+	struct list_head pin_list;
+	struct list_head cvt_list;
+	int num_pin;
+	int num_cvt;
 };
 
 static inline struct hdac_ext_device *to_hda_ext_device(struct device *dev)
@@ -149,13 +153,15 @@ static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev,
 		struct hdac_hdmi_dai_pin_map *dai_map, unsigned int pwr_state)
 {
 	/* Power up pin widget */
-	if (!snd_hdac_check_power_state(&edev->hdac, dai_map->pin.nid, pwr_state))
-		snd_hdac_codec_write(&edev->hdac, dai_map->pin.nid, 0,
+	if (!snd_hdac_check_power_state(&edev->hdac, dai_map->pin->nid,
+						pwr_state))
+		snd_hdac_codec_write(&edev->hdac, dai_map->pin->nid, 0,
 			AC_VERB_SET_POWER_STATE, pwr_state);
 
 	/* Power up converter */
-	if (!snd_hdac_check_power_state(&edev->hdac, dai_map->cvt.nid, pwr_state))
-		snd_hdac_codec_write(&edev->hdac, dai_map->cvt.nid, 0,
+	if (!snd_hdac_check_power_state(&edev->hdac, dai_map->cvt->nid,
+						pwr_state))
+		snd_hdac_codec_write(&edev->hdac, dai_map->cvt->nid, 0,
 			AC_VERB_SET_POWER_STATE, pwr_state);
 }
 
@@ -179,13 +185,13 @@ static int hdac_hdmi_playback_prepare(struct snd_pcm_substream *substream,
 	dev_dbg(&hdac->hdac.dev, "stream tag from cpu dai %d format in cvt 0x%x\n",
 			dd->stream_tag,	dd->format);
 
-	ret = hdac_hdmi_setup_audio_infoframe(hdac, dai_map->cvt.nid,
-						dai_map->pin.nid);
+	ret = hdac_hdmi_setup_audio_infoframe(hdac, dai_map->cvt->nid,
+						dai_map->pin->nid);
 	if (ret < 0)
 		return ret;
 
-	return hdac_hdmi_setup_stream(hdac, dai_map->cvt.nid, dai_map->pin.nid,
-					dd->stream_tag, dd->format);
+	return hdac_hdmi_setup_stream(hdac, dai_map->cvt->nid,
+			dai_map->pin->nid, dd->stream_tag, dd->format);
 }
 
 static int hdac_hdmi_set_hw_params(struct snd_pcm_substream *substream,
@@ -221,9 +227,9 @@ static int hdac_hdmi_playback_cleanup(struct snd_pcm_substream *substream,
 
 	dai_map = &hdmi->dai_map[dai->id];
 
-	snd_hdac_codec_write(&edev->hdac, dai_map->cvt.nid, 0,
+	snd_hdac_codec_write(&edev->hdac, dai_map->cvt->nid, 0,
 				AC_VERB_SET_CHANNEL_STREAMID, 0);
-	snd_hdac_codec_write(&edev->hdac, dai_map->cvt.nid, 0,
+	snd_hdac_codec_write(&edev->hdac, dai_map->cvt->nid, 0,
 				AC_VERB_SET_STREAM_FORMAT, 0);
 
 	dd = (struct hdac_ext_dma_params *)snd_soc_dai_get_dma_data(dai, substream);
@@ -249,7 +255,7 @@ static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
 
 	dai_map = &hdmi->dai_map[dai->id];
 
-	val = snd_hdac_codec_read(&hdac->hdac, dai_map->pin.nid, 0,
+	val = snd_hdac_codec_read(&hdac->hdac, dai_map->pin->nid, 0,
 					AC_VERB_GET_PIN_SENSE, 0);
 	dev_info(&hdac->hdac.dev, "Val for AC_VERB_GET_PIN_SENSE: %x\n", val);
 
@@ -260,7 +266,7 @@ static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
 
 	hdac_hdmi_set_power_state(hdac, dai_map, AC_PWRST_D0);
 
-	snd_hdac_codec_write(&hdac->hdac, dai_map->pin.nid, 0,
+	snd_hdac_codec_write(&hdac->hdac, dai_map->pin->nid, 0,
 			AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_UNMUTE);
 
 	snd_pcm_hw_constraint_step(substream->runtime, 0,
@@ -280,7 +286,7 @@ static void hdac_hdmi_pcm_close(struct snd_pcm_substream *substream,
 
 	hdac_hdmi_set_power_state(hdac, dai_map, AC_PWRST_D3);
 
-	snd_hdac_codec_write(&hdac->hdac, dai_map->pin.nid, 0,
+	snd_hdac_codec_write(&hdac->hdac, dai_map->pin->nid, 0,
 			AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_MUTE);
 }
 
@@ -368,40 +374,79 @@ static void create_fill_widget_route_map(struct snd_soc_dapm_context *dapm,
 	snd_soc_dapm_add_routes(dapm, route, ARRAY_SIZE(route));
 }
 
-static int hdac_hdmi_init_dai_map(struct hdac_ext_device *edev,
-			struct hdac_hdmi_dai_pin_map *dai_map,
-			hda_nid_t pin_nid, hda_nid_t cvt_nid, int dai_id)
+static int hdac_hdmi_init_dai_map(struct hdac_ext_device *edev)
 {
-	int ret;
+	struct hdac_hdmi_priv *hdmi = edev->private_data;
+	struct hdac_hdmi_dai_pin_map *dai_map = &hdmi->dai_map[0];
+	struct hdac_hdmi_cvt *cvt;
+	struct hdac_hdmi_pin *pin;
 
-	dai_map->dai_id = dai_id;
-	dai_map->pin.nid = pin_nid;
+	if (list_empty(&hdmi->cvt_list) || list_empty(&hdmi->pin_list))
+		return -EINVAL;
 
-	ret = hdac_hdmi_query_pin_connlist(edev, &dai_map->pin);
-	if (ret < 0) {
-		dev_err(&edev->hdac.dev,
-			"Error querying connection list: %d\n", ret);
-		return ret;
-	}
+	/*
+	 * Currently on board only 1 pin and 1 converter is enabled for
+	 * simplification, more will be added eventually
+	 * So using fixed map for dai_id:pin:cvt
+	 */
+	cvt = list_first_entry(&hdmi->cvt_list, struct hdac_hdmi_cvt, head);
+	pin = list_first_entry(&hdmi->pin_list, struct hdac_hdmi_pin, head);
 
-	dai_map->cvt.nid = cvt_nid;
+	dai_map->dai_id = 0;
+	dai_map->pin = pin;
+
+	dai_map->cvt = cvt;
 
 	/* Enable out path for this pin widget */
-	snd_hdac_codec_write(&edev->hdac, pin_nid, 0,
+	snd_hdac_codec_write(&edev->hdac, pin->nid, 0,
 			AC_VERB_SET_PIN_WIDGET_CONTROL, PIN_OUT);
 
 	/* Enable transmission */
-	snd_hdac_codec_write(&edev->hdac, cvt_nid, 0,
+	snd_hdac_codec_write(&edev->hdac, cvt->nid, 0,
 			AC_VERB_SET_DIGI_CONVERT_1, 1);
 
 	/* Category Code (CC) to zero */
-	snd_hdac_codec_write(&edev->hdac, cvt_nid, 0,
+	snd_hdac_codec_write(&edev->hdac, cvt->nid, 0,
 			AC_VERB_SET_DIGI_CONVERT_2, 0);
 
-	snd_hdac_codec_write(&edev->hdac, pin_nid, 0,
+	snd_hdac_codec_write(&edev->hdac, pin->nid, 0,
 			AC_VERB_SET_CONNECT_SEL, 0);
 
-	return hdac_hdmi_query_cvt_params(&edev->hdac, &dai_map->cvt);
+	return 0;
+}
+
+static int hdac_hdmi_add_cvt(struct hdac_ext_device *edev, hda_nid_t nid)
+{
+	struct hdac_hdmi_priv *hdmi = edev->private_data;
+	struct hdac_hdmi_cvt *cvt;
+
+	cvt = devm_kzalloc(&edev->hdac.dev, sizeof(*cvt), GFP_KERNEL);
+	if (!cvt)
+		return -ENOMEM;
+
+	cvt->nid = nid;
+
+	list_add_tail(&cvt->head, &hdmi->cvt_list);
+	hdmi->num_cvt++;
+
+	return hdac_hdmi_query_cvt_params(&edev->hdac, cvt);
+}
+
+static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
+{
+	struct hdac_hdmi_priv *hdmi = edev->private_data;
+	struct hdac_hdmi_pin *pin;
+
+	pin = devm_kzalloc(&edev->hdac.dev, sizeof(*pin), GFP_KERNEL);
+	if (!pin)
+		return -ENOMEM;
+
+	pin->nid = nid;
+
+	list_add_tail(&pin->head, &hdmi->pin_list);
+	hdmi->num_pin++;
+
+	return 0;
 }
 
 /*
@@ -414,7 +459,7 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
 	int i;
 	struct hdac_device *hdac = &edev->hdac;
 	struct hdac_hdmi_priv *hdmi = edev->private_data;
-	int cvt_nid = 0, pin_nid = 0;
+	int ret;
 
 	hdac->num_nodes = snd_hdac_get_sub_nodes(hdac, hdac->afg, &nid);
 	if (!nid || hdac->num_nodes <= 0) {
@@ -437,29 +482,25 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
 		switch (type) {
 
 		case AC_WID_AUD_OUT:
-			hdmi->cvt_nid[cvt_nid] = nid;
-			cvt_nid++;
+			ret = hdac_hdmi_add_cvt(edev, nid);
+			if (ret < 0)
+				return ret;
 			break;
 
 		case AC_WID_PIN:
-			hdmi->pin_nid[pin_nid] = nid;
-			pin_nid++;
+			ret = hdac_hdmi_add_pin(edev, nid);
+			if (ret < 0)
+				return ret;
 			break;
 		}
 	}
 
 	hdac->end_nid = nid;
 
-	if (!pin_nid || !cvt_nid)
+	if (!hdmi->num_pin || !hdmi->num_cvt)
 		return -EIO;
 
-	/*
-	 * Currently on board only 1 pin and 1 converter is enabled for
-	 * simplification, more will be added eventually
-	 * So using fixed map for dai_id:pin:cvt
-	 */
-	return hdac_hdmi_init_dai_map(edev, &hdmi->dai_map[0], hdmi->pin_nid[0],
-			hdmi->cvt_nid[0], 0);
+	return hdac_hdmi_init_dai_map(edev);
 }
 
 static int hdmi_codec_probe(struct snd_soc_codec *codec)
@@ -543,6 +584,9 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
 
 	dev_set_drvdata(&codec->dev, edev);
 
+	INIT_LIST_HEAD(&hdmi_priv->pin_list);
+	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
+
 	ret = hdac_hdmi_parse_and_map_nid(edev);
 	if (ret < 0)
 		return ret;
-- 
1.9.1

Re: [PATCH 03/15] ASoC: hdac_hdmi - Use list to add pins and converters

[Takashi Iwai]

On Tue, 01 Dec 2015 18:46:59 +0100,
Subhransu S. Prusty wrote:
> 
> +static int hdac_hdmi_add_cvt(struct hdac_ext_device *edev, hda_nid_t nid)
> +{
> +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> +	struct hdac_hdmi_cvt *cvt;
> +
> +	cvt = devm_kzalloc(&edev->hdac.dev, sizeof(*cvt), GFP_KERNEL);

Be careful about devm_*() usage here.  The devres resources may be
freed before the release callback is called for the assigned device
object.  See drivers/base/core.c:device_release().

For the top-level object, usually it's OK, because the top-level
object itself won't be released but only the driver is detached.
Then devres_release_all() is called in device_release_driver() while
the device itself is still present.

I'm not sure whether this would be actually OK with hdac_device.  You
need to double-check, not only check whether the driver appears to
work, but track all object lifetime properly.


Takashi

> +	if (!cvt)
> +		return -ENOMEM;
> +
> +	cvt->nid = nid;
> +
> +	list_add_tail(&cvt->head, &hdmi->cvt_list);
> +	hdmi->num_cvt++;
> +
> +	return hdac_hdmi_query_cvt_params(&edev->hdac, cvt);
> +}
> +
> +static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
> +{
> +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> +	struct hdac_hdmi_pin *pin;
> +
> +	pin = devm_kzalloc(&edev->hdac.dev, sizeof(*pin), GFP_KERNEL);
> +	if (!pin)
> +		return -ENOMEM;
> +
> +	pin->nid = nid;
> +
> +	list_add_tail(&pin->head, &hdmi->pin_list);
> +	hdmi->num_pin++;
> +
> +	return 0;
>  }
>  
>  /*
> @@ -414,7 +459,7 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
>  	int i;
>  	struct hdac_device *hdac = &edev->hdac;
>  	struct hdac_hdmi_priv *hdmi = edev->private_data;
> -	int cvt_nid = 0, pin_nid = 0;
> +	int ret;
>  
>  	hdac->num_nodes = snd_hdac_get_sub_nodes(hdac, hdac->afg, &nid);
>  	if (!nid || hdac->num_nodes <= 0) {
> @@ -437,29 +482,25 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
>  		switch (type) {
>  
>  		case AC_WID_AUD_OUT:
> -			hdmi->cvt_nid[cvt_nid] = nid;
> -			cvt_nid++;
> +			ret = hdac_hdmi_add_cvt(edev, nid);
> +			if (ret < 0)
> +				return ret;
>  			break;
>  
>  		case AC_WID_PIN:
> -			hdmi->pin_nid[pin_nid] = nid;
> -			pin_nid++;
> +			ret = hdac_hdmi_add_pin(edev, nid);
> +			if (ret < 0)
> +				return ret;
>  			break;
>  		}
>  	}
>  
>  	hdac->end_nid = nid;
>  
> -	if (!pin_nid || !cvt_nid)
> +	if (!hdmi->num_pin || !hdmi->num_cvt)
>  		return -EIO;
>  
> -	/*
> -	 * Currently on board only 1 pin and 1 converter is enabled for
> -	 * simplification, more will be added eventually
> -	 * So using fixed map for dai_id:pin:cvt
> -	 */
> -	return hdac_hdmi_init_dai_map(edev, &hdmi->dai_map[0], hdmi->pin_nid[0],
> -			hdmi->cvt_nid[0], 0);
> +	return hdac_hdmi_init_dai_map(edev);
>  }
>  
>  static int hdmi_codec_probe(struct snd_soc_codec *codec)
> @@ -543,6 +584,9 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
>  
>  	dev_set_drvdata(&codec->dev, edev);
>  
> +	INIT_LIST_HEAD(&hdmi_priv->pin_list);
> +	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
> +
>  	ret = hdac_hdmi_parse_and_map_nid(edev);
>  	if (ret < 0)
>  		return ret;
> -- 
> 1.9.1
> 

Re: [PATCH 03/15] ASoC: hdac_hdmi - Use list to add pins and converters

[Subhransu S. Prusty]

On Tue, Dec 01, 2015 at 01:47:04PM +0100, Takashi Iwai wrote:
> On Tue, 01 Dec 2015 18:46:59 +0100,
> Subhransu S. Prusty wrote:
> > 
> > +static int hdac_hdmi_add_cvt(struct hdac_ext_device *edev, hda_nid_t nid)
> > +{
> > +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > +	struct hdac_hdmi_cvt *cvt;
> > +
> > +	cvt = devm_kzalloc(&edev->hdac.dev, sizeof(*cvt), GFP_KERNEL);
> 
> Be careful about devm_*() usage here.  The devres resources may be
> freed before the release callback is called for the assigned device
> object.  See drivers/base/core.c:device_release().
> 
> For the top-level object, usually it's OK, because the top-level
> object itself won't be released but only the driver is detached.
> Then devres_release_all() is called in device_release_driver() while
> the device itself is still present.

Not sure if I understood it correctly. But as the devres_release_all is
called after the driver remove and the above resource is not expected to
be used outside the scope of driver. So should't deleting the list entries
be good enough during driver remove?

> 
> I'm not sure whether this would be actually OK with hdac_device.  You
> need to double-check, not only check whether the driver appears to
> work, but track all object lifetime properly.

Will testing driver load/unload be helpful here with CONFIG_DEBUG_DEVRES
enabled?

Regards,
Subhransu
> 
> 
> Takashi
> 
> > +	if (!cvt)
> > +		return -ENOMEM;
> > +
> > +	cvt->nid = nid;
> > +
> > +	list_add_tail(&cvt->head, &hdmi->cvt_list);
> > +	hdmi->num_cvt++;
> > +
> > +	return hdac_hdmi_query_cvt_params(&edev->hdac, cvt);
> > +}
> > +
> > +static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
> > +{
> > +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > +	struct hdac_hdmi_pin *pin;
> > +
> > +	pin = devm_kzalloc(&edev->hdac.dev, sizeof(*pin), GFP_KERNEL);
> > +	if (!pin)
> > +		return -ENOMEM;
> > +
> > +	pin->nid = nid;
> > +
> > +	list_add_tail(&pin->head, &hdmi->pin_list);
> > +	hdmi->num_pin++;
> > +
> > +	return 0;
> >  }
> >  
> >  /*
> > @@ -414,7 +459,7 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
> >  	int i;
> >  	struct hdac_device *hdac = &edev->hdac;
> >  	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > -	int cvt_nid = 0, pin_nid = 0;
> > +	int ret;
> >  
> >  	hdac->num_nodes = snd_hdac_get_sub_nodes(hdac, hdac->afg, &nid);
> >  	if (!nid || hdac->num_nodes <= 0) {
> > @@ -437,29 +482,25 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
> >  		switch (type) {
> >  
> >  		case AC_WID_AUD_OUT:
> > -			hdmi->cvt_nid[cvt_nid] = nid;
> > -			cvt_nid++;
> > +			ret = hdac_hdmi_add_cvt(edev, nid);
> > +			if (ret < 0)
> > +				return ret;
> >  			break;
> >  
> >  		case AC_WID_PIN:
> > -			hdmi->pin_nid[pin_nid] = nid;
> > -			pin_nid++;
> > +			ret = hdac_hdmi_add_pin(edev, nid);
> > +			if (ret < 0)
> > +				return ret;
> >  			break;
> >  		}
> >  	}
> >  
> >  	hdac->end_nid = nid;
> >  
> > -	if (!pin_nid || !cvt_nid)
> > +	if (!hdmi->num_pin || !hdmi->num_cvt)
> >  		return -EIO;
> >  
> > -	/*
> > -	 * Currently on board only 1 pin and 1 converter is enabled for
> > -	 * simplification, more will be added eventually
> > -	 * So using fixed map for dai_id:pin:cvt
> > -	 */
> > -	return hdac_hdmi_init_dai_map(edev, &hdmi->dai_map[0], hdmi->pin_nid[0],
> > -			hdmi->cvt_nid[0], 0);
> > +	return hdac_hdmi_init_dai_map(edev);
> >  }
> >  
> >  static int hdmi_codec_probe(struct snd_soc_codec *codec)
> > @@ -543,6 +584,9 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
> >  
> >  	dev_set_drvdata(&codec->dev, edev);
> >  
> > +	INIT_LIST_HEAD(&hdmi_priv->pin_list);
> > +	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
> > +
> >  	ret = hdac_hdmi_parse_and_map_nid(edev);
> >  	if (ret < 0)
> >  		return ret;
> > -- 
> > 1.9.1
> > 

-- 

Re: [PATCH 03/15] ASoC: hdac_hdmi - Use list to add pins and converters

[Takashi Iwai]

On Tue, 01 Dec 2015 23:50:50 +0100,
Subhransu S. Prusty wrote:
> 
> On Tue, Dec 01, 2015 at 01:47:04PM +0100, Takashi Iwai wrote:
> > On Tue, 01 Dec 2015 18:46:59 +0100,
> > Subhransu S. Prusty wrote:
> > > 
> > > +static int hdac_hdmi_add_cvt(struct hdac_ext_device *edev, hda_nid_t nid)
> > > +{
> > > +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > > +	struct hdac_hdmi_cvt *cvt;
> > > +
> > > +	cvt = devm_kzalloc(&edev->hdac.dev, sizeof(*cvt), GFP_KERNEL);
> > 
> > Be careful about devm_*() usage here.  The devres resources may be
> > freed before the release callback is called for the assigned device
> > object.  See drivers/base/core.c:device_release().
> > 
> > For the top-level object, usually it's OK, because the top-level
> > object itself won't be released but only the driver is detached.
> > Then devres_release_all() is called in device_release_driver() while
> > the device itself is still present.
> 
> Not sure if I understood it correctly. But as the devres_release_all is
> called after the driver remove and the above resource is not expected to
> be used outside the scope of driver.

devres_release_all() is called *before* calling the device's release
callback, which is usually triggered by put_device().  So it depends
on how you call the device removal whether it's safe or not.

> So should't deleting the list entries
> be good enough during driver remove?

It depends on how the allocated data is accessed.  I guess it won't be
a problem in this case, but in general, you have to be careful about
devm_*() usage.  It can't be used always blindly.

> > I'm not sure whether this would be actually OK with hdac_device.  You
> > need to double-check, not only check whether the driver appears to
> > work, but track all object lifetime properly.
> 
> Will testing driver load/unload be helpful here with CONFIG_DEBUG_DEVRES
> enabled?

Maybe.  But at best you should read the code and follow the flow
closely by yourself.


Takashi

> 
> Regards,
> Subhransu
> > 
> > 
> > Takashi
> > 
> > > +	if (!cvt)
> > > +		return -ENOMEM;
> > > +
> > > +	cvt->nid = nid;
> > > +
> > > +	list_add_tail(&cvt->head, &hdmi->cvt_list);
> > > +	hdmi->num_cvt++;
> > > +
> > > +	return hdac_hdmi_query_cvt_params(&edev->hdac, cvt);
> > > +}
> > > +
> > > +static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
> > > +{
> > > +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > > +	struct hdac_hdmi_pin *pin;
> > > +
> > > +	pin = devm_kzalloc(&edev->hdac.dev, sizeof(*pin), GFP_KERNEL);
> > > +	if (!pin)
> > > +		return -ENOMEM;
> > > +
> > > +	pin->nid = nid;
> > > +
> > > +	list_add_tail(&pin->head, &hdmi->pin_list);
> > > +	hdmi->num_pin++;
> > > +
> > > +	return 0;
> > >  }
> > >  
> > >  /*
> > > @@ -414,7 +459,7 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
> > >  	int i;
> > >  	struct hdac_device *hdac = &edev->hdac;
> > >  	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > > -	int cvt_nid = 0, pin_nid = 0;
> > > +	int ret;
> > >  
> > >  	hdac->num_nodes = snd_hdac_get_sub_nodes(hdac, hdac->afg, &nid);
> > >  	if (!nid || hdac->num_nodes <= 0) {
> > > @@ -437,29 +482,25 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
> > >  		switch (type) {
> > >  
> > >  		case AC_WID_AUD_OUT:
> > > -			hdmi->cvt_nid[cvt_nid] = nid;
> > > -			cvt_nid++;
> > > +			ret = hdac_hdmi_add_cvt(edev, nid);
> > > +			if (ret < 0)
> > > +				return ret;
> > >  			break;
> > >  
> > >  		case AC_WID_PIN:
> > > -			hdmi->pin_nid[pin_nid] = nid;
> > > -			pin_nid++;
> > > +			ret = hdac_hdmi_add_pin(edev, nid);
> > > +			if (ret < 0)
> > > +				return ret;
> > >  			break;
> > >  		}
> > >  	}
> > >  
> > >  	hdac->end_nid = nid;
> > >  
> > > -	if (!pin_nid || !cvt_nid)
> > > +	if (!hdmi->num_pin || !hdmi->num_cvt)
> > >  		return -EIO;
> > >  
> > > -	/*
> > > -	 * Currently on board only 1 pin and 1 converter is enabled for
> > > -	 * simplification, more will be added eventually
> > > -	 * So using fixed map for dai_id:pin:cvt
> > > -	 */
> > > -	return hdac_hdmi_init_dai_map(edev, &hdmi->dai_map[0], hdmi->pin_nid[0],
> > > -			hdmi->cvt_nid[0], 0);
> > > +	return hdac_hdmi_init_dai_map(edev);
> > >  }
> > >  
> > >  static int hdmi_codec_probe(struct snd_soc_codec *codec)
> > > @@ -543,6 +584,9 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
> > >  
> > >  	dev_set_drvdata(&codec->dev, edev);
> > >  
> > > +	INIT_LIST_HEAD(&hdmi_priv->pin_list);
> > > +	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
> > > +
> > >  	ret = hdac_hdmi_parse_and_map_nid(edev);
> > >  	if (ret < 0)
> > >  		return ret;
> > > -- 
> > > 1.9.1
> > > 
> 
> -- 
> 

Re: [PATCH 03/15] ASoC: hdac_hdmi - Use list to add pins and converters

[Takashi Iwai]

On Tue, 01 Dec 2015 19:52:14 +0100,
Takashi Iwai wrote:
> 
> On Tue, 01 Dec 2015 23:50:50 +0100,
> Subhransu S. Prusty wrote:
> > 
> > On Tue, Dec 01, 2015 at 01:47:04PM +0100, Takashi Iwai wrote:
> > > On Tue, 01 Dec 2015 18:46:59 +0100,
> > > Subhransu S. Prusty wrote:
> > > > 
> > > > +static int hdac_hdmi_add_cvt(struct hdac_ext_device *edev, hda_nid_t nid)
> > > > +{
> > > > +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > > > +	struct hdac_hdmi_cvt *cvt;
> > > > +
> > > > +	cvt = devm_kzalloc(&edev->hdac.dev, sizeof(*cvt), GFP_KERNEL);
> > > 
> > > Be careful about devm_*() usage here.  The devres resources may be
> > > freed before the release callback is called for the assigned device
> > > object.  See drivers/base/core.c:device_release().
> > > 
> > > For the top-level object, usually it's OK, because the top-level
> > > object itself won't be released but only the driver is detached.
> > > Then devres_release_all() is called in device_release_driver() while
> > > the device itself is still present.
> > 
> > Not sure if I understood it correctly. But as the devres_release_all is
> > called after the driver remove and the above resource is not expected to
> > be used outside the scope of driver.
> 
> devres_release_all() is called *before* calling the device's release
> callback, which is usually triggered by put_device().  So it depends
> on how you call the device removal whether it's safe or not.
> 
> > So should't deleting the list entries
> > be good enough during driver remove?
> 
> It depends on how the allocated data is accessed.  I guess it won't be
> a problem in this case, but in general, you have to be careful about
> devm_*() usage.  It can't be used always blindly.

To make clear my intention: the code used in this patch appears OK, as
far as I see.  But, for example, if this code would be moved to HDA
core layer, then it might not work -- there you'll have to release the
resource in another way.  A driver-bound resource shall be released
with the driver's unbinding, but a resource bound with the device
isn't necessarily released there but first at device_release().


Takashi

Re: [PATCH 03/15] ASoC: hdac_hdmi - Use list to add pins and converters

[Subhransu S. Prusty]

On Wed, Dec 02, 2015 at 10:31:07AM +0100, Takashi Iwai wrote:
> On Tue, 01 Dec 2015 19:52:14 +0100,
> Takashi Iwai wrote:
> > 
> > On Tue, 01 Dec 2015 23:50:50 +0100,
> > Subhransu S. Prusty wrote:
> > > 
> > > On Tue, Dec 01, 2015 at 01:47:04PM +0100, Takashi Iwai wrote:
> > > > On Tue, 01 Dec 2015 18:46:59 +0100,
> > > > Subhransu S. Prusty wrote:
> > > > > 
> > > > > +static int hdac_hdmi_add_cvt(struct hdac_ext_device *edev, hda_nid_t nid)
> > > > > +{
> > > > > +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > > > > +	struct hdac_hdmi_cvt *cvt;
> > > > > +
> > > > > +	cvt = devm_kzalloc(&edev->hdac.dev, sizeof(*cvt), GFP_KERNEL);
> > > > 
> > > > Be careful about devm_*() usage here.  The devres resources may be
> > > > freed before the release callback is called for the assigned device
> > > > object.  See drivers/base/core.c:device_release().
> > > > 
> > > > For the top-level object, usually it's OK, because the top-level
> > > > object itself won't be released but only the driver is detached.
> > > > Then devres_release_all() is called in device_release_driver() while
> > > > the device itself is still present.
> > > 
> > > Not sure if I understood it correctly. But as the devres_release_all is
> > > called after the driver remove and the above resource is not expected to
> > > be used outside the scope of driver.
> > 
> > devres_release_all() is called *before* calling the device's release
> > callback, which is usually triggered by put_device().  So it depends
> > on how you call the device removal whether it's safe or not.
> > 
> > > So should't deleting the list entries
> > > be good enough during driver remove?
> > 
> > It depends on how the allocated data is accessed.  I guess it won't be
> > a problem in this case, but in general, you have to be careful about
> > devm_*() usage.  It can't be used always blindly.
> 
> To make clear my intention: the code used in this patch appears OK, as
> far as I see.  But, for example, if this code would be moved to HDA
> core layer, then it might not work -- there you'll have to release the
> resource in another way.  A driver-bound resource shall be released
> with the driver's unbinding, but a resource bound with the device
> isn't necessarily released there but first at device_release().

It's better then I will remove the devm_xxx and will manage the resources in
the driver itself.

Regards,
Subhransu
> 
> 
> Takashi

-- 

[PATCH 04/15] ALSA: hda - Add helper to read eld data

[Subhransu S. Prusty]

The eld reading APIs are required for ASoC skylake hdmi
driver as well. So moving these definition to core. Once
component ops for reading ELD is available will mark this
as deprecated.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/sound/hdaudio.h |  3 ++
 sound/hda/Makefile      |  2 +-
 sound/hda/hdac_eld.c    | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 sound/hda/hdac_eld.c

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index e2b712c..1170f21 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -455,6 +455,9 @@ void snd_hdac_stream_sync(struct hdac_stream *azx_dev, bool start,
 			  unsigned int streams);
 void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
 				      unsigned int streams);
+/* HDMI helpers to query eld from codec */
+int snd_hdac_get_eld(struct hdac_device *codec, hda_nid_t nid,
+		unsigned char *buf, int *eld_size);
 /*
  * macros for easy use
  */
diff --git a/sound/hda/Makefile b/sound/hda/Makefile
index 7e999c9..e0a2b25 100644
--- a/sound/hda/Makefile
+++ b/sound/hda/Makefile
@@ -1,5 +1,5 @@
 snd-hda-core-objs := hda_bus_type.o hdac_bus.o hdac_device.o hdac_sysfs.o \
-	hdac_regmap.o hdac_controller.o hdac_stream.o array.o
+	hdac_regmap.o hdac_controller.o hdac_stream.o array.o hdac_eld.o
 
 snd-hda-core-objs += trace.o
 CFLAGS_trace.o := -I$(src)
diff --git a/sound/hda/hdac_eld.c b/sound/hda/hdac_eld.c
new file mode 100644
index 0000000..9126e0f
--- /dev/null
+++ b/sound/hda/hdac_eld.c
@@ -0,0 +1,95 @@
+/*
+ * HDMI Eld routines
+ */
+
+#include <sound/hdaudio.h>
+
+#define ELD_FIXED_BYTES	20
+#define ELD_MAX_SIZE    256
+
+static unsigned int snd_hdac_get_eld_data(struct hdac_device *codec,
+				hda_nid_t nid, int byte_index)
+{
+	unsigned int val;
+
+	val = snd_hdac_codec_read(codec, nid, 0,
+					AC_VERB_GET_HDMI_ELDD, byte_index);
+
+	dev_dbg(&codec->dev, "HDMI: ELD data byte %d: 0x%x\n", byte_index, val);
+
+	return val;
+}
+
+static int snd_hdac_get_eld_size(struct hdac_device *codec, hda_nid_t nid)
+{
+	return snd_hdac_codec_read(codec, nid, 0, AC_VERB_GET_HDMI_DIP_SIZE,
+						 AC_DIPSIZE_ELD_BUF);
+}
+
+/**
+ * snd_hdac_get_eld - Query eld from sink
+ * @codec - device to query
+ * @nid - codec node to query
+ * @buf - Buffer to fill eld data
+ * @eld_size - Size of eld queried
+ *
+ * Returns zero on success or a negative error code
+ *
+ * This function queries the eld size and eld data and fills in the buffer
+ * passed by user
+ */
+int snd_hdac_get_eld(struct hdac_device *codec, hda_nid_t nid,
+		     unsigned char *buf, int *eld_size)
+{
+	int i;
+	int ret = 0;
+	int size;
+
+	/*
+	 * ELD size is initialized to zero in caller function. If no errors and
+	 * ELD is valid, actual eld_size is assigned.
+	 */
+
+	size = snd_hdac_get_eld_size(codec, nid);
+	if (size == 0) {
+		/* wfg: workaround for ASUS P5E-VM HDMI board */
+		dev_info(&codec->dev, "HDMI: ELD buf size is 0, force 128\n");
+		size = 128;
+	}
+	if (size < ELD_FIXED_BYTES || size > ELD_MAX_SIZE) {
+		dev_info(&codec->dev, "HDMI: invalid ELD buf size %d\n", size);
+		return -ERANGE;
+	}
+
+	/* set ELD buffer */
+	for (i = 0; i < size; i++) {
+		unsigned int val = snd_hdac_get_eld_data(codec, nid, i);
+		/*
+		 * Graphics driver might be writing to ELD buffer right now.
+		 * Just abort. The caller will repoll after a while.
+		 */
+		if (!(val & AC_ELDD_ELD_VALID)) {
+			dev_info(&codec->dev, "HDMI: invalid ELD data byte %d\n", i);
+			ret = -EINVAL;
+			goto error;
+		}
+		val &= AC_ELDD_ELD_DATA;
+		/*
+		 * The first byte cannot be zero. This can happen on some DVI
+		 * connections. Some Intel chips may also need some 250ms delay
+		 * to return non-zero ELD data, even when the graphics driver
+		 * correctly writes ELD content before setting ELD_valid bit.
+		 */
+		if (!val && !i) {
+			dev_dbg(&codec->dev, "HDMI: 0 ELD data\n");
+			ret = -EINVAL;
+			goto error;
+		}
+		buf[i] = val;
+	}
+
+	*eld_size = size;
+error:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_get_eld);
-- 
1.9.1

Re: [PATCH 04/15] ALSA: hda - Add helper to read eld data

[Takashi Iwai]

On Tue, 01 Dec 2015 18:47:00 +0100,
Subhransu S. Prusty wrote:
> 
> The eld reading APIs are required for ASoC skylake hdmi
> driver as well. So moving these definition to core. Once
> component ops for reading ELD is available will mark this
> as deprecated.
> 
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>

I'd like rather to merge this from my tree, not from asoc tree.
Otherwise it makes hard to use this in the legacy HDA driver.

Or make this local in asoc, and clean up later as a common helper.


thanks,

Takashi


> ---
>  include/sound/hdaudio.h |  3 ++
>  sound/hda/Makefile      |  2 +-
>  sound/hda/hdac_eld.c    | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 99 insertions(+), 1 deletion(-)
>  create mode 100644 sound/hda/hdac_eld.c
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index e2b712c..1170f21 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -455,6 +455,9 @@ void snd_hdac_stream_sync(struct hdac_stream *azx_dev, bool start,
>  			  unsigned int streams);
>  void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
>  				      unsigned int streams);
> +/* HDMI helpers to query eld from codec */
> +int snd_hdac_get_eld(struct hdac_device *codec, hda_nid_t nid,
> +		unsigned char *buf, int *eld_size);
>  /*
>   * macros for easy use
>   */
> diff --git a/sound/hda/Makefile b/sound/hda/Makefile
> index 7e999c9..e0a2b25 100644
> --- a/sound/hda/Makefile
> +++ b/sound/hda/Makefile
> @@ -1,5 +1,5 @@
>  snd-hda-core-objs := hda_bus_type.o hdac_bus.o hdac_device.o hdac_sysfs.o \
> -	hdac_regmap.o hdac_controller.o hdac_stream.o array.o
> +	hdac_regmap.o hdac_controller.o hdac_stream.o array.o hdac_eld.o
>  
>  snd-hda-core-objs += trace.o
>  CFLAGS_trace.o := -I$(src)
> diff --git a/sound/hda/hdac_eld.c b/sound/hda/hdac_eld.c
> new file mode 100644
> index 0000000..9126e0f
> --- /dev/null
> +++ b/sound/hda/hdac_eld.c
> @@ -0,0 +1,95 @@
> +/*
> + * HDMI Eld routines
> + */
> +
> +#include <sound/hdaudio.h>
> +
> +#define ELD_FIXED_BYTES	20
> +#define ELD_MAX_SIZE    256
> +
> +static unsigned int snd_hdac_get_eld_data(struct hdac_device *codec,
> +				hda_nid_t nid, int byte_index)
> +{
> +	unsigned int val;
> +
> +	val = snd_hdac_codec_read(codec, nid, 0,
> +					AC_VERB_GET_HDMI_ELDD, byte_index);
> +
> +	dev_dbg(&codec->dev, "HDMI: ELD data byte %d: 0x%x\n", byte_index, val);
> +
> +	return val;
> +}
> +
> +static int snd_hdac_get_eld_size(struct hdac_device *codec, hda_nid_t nid)
> +{
> +	return snd_hdac_codec_read(codec, nid, 0, AC_VERB_GET_HDMI_DIP_SIZE,
> +						 AC_DIPSIZE_ELD_BUF);
> +}
> +
> +/**
> + * snd_hdac_get_eld - Query eld from sink
> + * @codec - device to query
> + * @nid - codec node to query
> + * @buf - Buffer to fill eld data
> + * @eld_size - Size of eld queried
> + *
> + * Returns zero on success or a negative error code
> + *
> + * This function queries the eld size and eld data and fills in the buffer
> + * passed by user
> + */
> +int snd_hdac_get_eld(struct hdac_device *codec, hda_nid_t nid,
> +		     unsigned char *buf, int *eld_size)
> +{
> +	int i;
> +	int ret = 0;
> +	int size;
> +
> +	/*
> +	 * ELD size is initialized to zero in caller function. If no errors and
> +	 * ELD is valid, actual eld_size is assigned.
> +	 */
> +
> +	size = snd_hdac_get_eld_size(codec, nid);
> +	if (size == 0) {
> +		/* wfg: workaround for ASUS P5E-VM HDMI board */
> +		dev_info(&codec->dev, "HDMI: ELD buf size is 0, force 128\n");
> +		size = 128;
> +	}
> +	if (size < ELD_FIXED_BYTES || size > ELD_MAX_SIZE) {
> +		dev_info(&codec->dev, "HDMI: invalid ELD buf size %d\n", size);
> +		return -ERANGE;
> +	}
> +
> +	/* set ELD buffer */
> +	for (i = 0; i < size; i++) {
> +		unsigned int val = snd_hdac_get_eld_data(codec, nid, i);
> +		/*
> +		 * Graphics driver might be writing to ELD buffer right now.
> +		 * Just abort. The caller will repoll after a while.
> +		 */
> +		if (!(val & AC_ELDD_ELD_VALID)) {
> +			dev_info(&codec->dev, "HDMI: invalid ELD data byte %d\n", i);
> +			ret = -EINVAL;
> +			goto error;
> +		}
> +		val &= AC_ELDD_ELD_DATA;
> +		/*
> +		 * The first byte cannot be zero. This can happen on some DVI
> +		 * connections. Some Intel chips may also need some 250ms delay
> +		 * to return non-zero ELD data, even when the graphics driver
> +		 * correctly writes ELD content before setting ELD_valid bit.
> +		 */
> +		if (!val && !i) {
> +			dev_dbg(&codec->dev, "HDMI: 0 ELD data\n");
> +			ret = -EINVAL;
> +			goto error;
> +		}
> +		buf[i] = val;
> +	}
> +
> +	*eld_size = size;
> +error:
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_get_eld);
> -- 
> 1.9.1
> 

Re: [PATCH 04/15] ALSA: hda - Add helper to read eld data

[Subhransu S. Prusty]

On Tue, Dec 01, 2015 at 01:39:19PM +0100, Takashi Iwai wrote:
> On Tue, 01 Dec 2015 18:47:00 +0100,
> Subhransu S. Prusty wrote:
> > 
> > The eld reading APIs are required for ASoC skylake hdmi
> > driver as well. So moving these definition to core. Once
> > component ops for reading ELD is available will mark this
> > as deprecated.
> > 
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> 
> I'd like rather to merge this from my tree, not from asoc tree.
> Otherwise it makes hard to use this in the legacy HDA driver.

I have not updated the callers here and not planning to change for now as
it can be removed I guess once the component framework is available to
read eld. What do you say?

Regards,
Subhransu
> 
> Or make this local in asoc, and clean up later as a common helper.
> 
> 
> thanks,
> 
> Takashi
> 
> 

-- 

Re: [PATCH 04/15] ALSA: hda - Add helper to read eld data

[Takashi Iwai]

On Tue, 01 Dec 2015 20:12:14 +0100,
Subhransu S. Prusty wrote:
> 
> On Tue, Dec 01, 2015 at 01:39:19PM +0100, Takashi Iwai wrote:
> > On Tue, 01 Dec 2015 18:47:00 +0100,
> > Subhransu S. Prusty wrote:
> > > 
> > > The eld reading APIs are required for ASoC skylake hdmi
> > > driver as well. So moving these definition to core. Once
> > > component ops for reading ELD is available will mark this
> > > as deprecated.
> > > 
> > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > 
> > I'd like rather to merge this from my tree, not from asoc tree.
> > Otherwise it makes hard to use this in the legacy HDA driver.
> 
> I have not updated the callers here and not planning to change for now as
> it can be removed I guess once the component framework is available to
> read eld. What do you say?

OK, then you also don't have to put the stuff into the common place,
either, as this stuff won't be used any longer in near future :)


Takashi

> 
> Regards,
> Subhransu
> > 
> > Or make this local in asoc, and clean up later as a common helper.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > 
> 
> -- 
> 

[PATCH 05/15] ASoC: hdac_hdmi: Add hotplug notification and read eld

[Subhransu S. Prusty]

This patch uses i915 component framework to register for hotplug
notification. And once it identifies valid pin sense and valid eld,
reads the eld into the corresponding pin map buffer. For now it
directly sends the verbs and reads the eld. Later this will use
the i915 framework to populate ELD buffer once available.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 125 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 118 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 0869855..203c99f 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -34,6 +34,9 @@
 
 #define HDA_MAX_CONNECTIONS     32
 
+#define ELD_MAX_SIZE    256
+#define ELD_FIXED_BYTES	20
+
 struct hdac_hdmi_cvt_params {
 	unsigned int channels_min;
 	unsigned int channels_max;
@@ -48,11 +51,22 @@ struct hdac_hdmi_cvt {
 	struct hdac_hdmi_cvt_params params;
 };
 
+struct hdmi_eld {
+	bool	monitor_present;
+	bool	eld_valid;
+	int	eld_size;
+	char    eld_buffer[ELD_MAX_SIZE];
+};
+
 struct hdac_hdmi_pin {
 	struct list_head head;
 	hda_nid_t nid;
 	int num_mux_nids;
 	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
+	struct hdmi_eld eld;
+	struct hdac_ext_device *edev;
+	int repoll_count;
+	struct delayed_work work;
 };
 
 struct hdac_hdmi_dai_pin_map {
@@ -246,7 +260,6 @@ static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
 	struct hdac_ext_device *hdac = snd_soc_dai_get_drvdata(dai);
 	struct hdac_hdmi_priv *hdmi = hdac->private_data;
 	struct hdac_hdmi_dai_pin_map *dai_map;
-	int val;
 
 	if (dai->id > 0) {
 		dev_err(&hdac->hdac.dev, "Only one dai supported as of now\n");
@@ -255,12 +268,11 @@ static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
 
 	dai_map = &hdmi->dai_map[dai->id];
 
-	val = snd_hdac_codec_read(&hdac->hdac, dai_map->pin->nid, 0,
-					AC_VERB_GET_PIN_SENSE, 0);
-	dev_info(&hdac->hdac.dev, "Val for AC_VERB_GET_PIN_SENSE: %x\n", val);
-
-	if ((!(val & AC_PINSENSE_PRESENCE)) || (!(val & AC_PINSENSE_ELDV))) {
-		dev_err(&hdac->hdac.dev, "Monitor presence invalid with val: %x\n", val);
+	if ((!dai_map->pin->eld.monitor_present) ||
+		(!dai_map->pin->eld.eld_valid)) {
+		dev_err(&hdac->hdac.dev, "Failed: montior present? %d eld valid?: %d\n",
+				dai_map->pin->eld.monitor_present,
+				dai_map->pin->eld.eld_valid);
 		return -ENODEV;
 	}
 
@@ -432,6 +444,68 @@ static int hdac_hdmi_add_cvt(struct hdac_ext_device *edev, hda_nid_t nid)
 	return hdac_hdmi_query_cvt_params(&edev->hdac, cvt);
 }
 
+static void hdac_hdmi_present_sense(struct hdac_hdmi_pin *pin, int repoll)
+{
+	struct hdac_ext_device *edev = pin->edev;
+	int val;
+
+	if (!edev)
+		return;
+
+	pin->repoll_count = repoll;
+
+	pm_runtime_get_sync(&edev->hdac.dev);
+	val = snd_hdac_codec_read(&edev->hdac, pin->nid, 0,
+					AC_VERB_GET_PIN_SENSE, 0);
+
+	dev_dbg(&edev->hdac.dev, "Pin sense val %x for pin: %d\n",
+			val, pin->nid);
+	pin->eld.monitor_present = !!(val & AC_PINSENSE_PRESENCE);
+	pin->eld.eld_valid = !!(val & AC_PINSENSE_ELDV);
+
+	if (!pin->eld.monitor_present || !pin->eld.eld_valid) {
+		dev_info(&edev->hdac.dev, "%s: disconnect or eld_invalid\n",
+				__func__);
+		goto put_hdac_device;
+	}
+
+	if (pin->eld.monitor_present && pin->eld.eld_valid) {
+		/* TODO: Use i915 cmpnt framework when available */
+		if (snd_hdac_get_eld(&edev->hdac, pin->nid,
+				pin->eld.eld_buffer,
+				&pin->eld.eld_size) == 0) {
+			print_hex_dump_bytes("Eld: ", DUMP_PREFIX_OFFSET,
+					pin->eld.eld_buffer, pin->eld.eld_size);
+		} else {
+			dev_err(&edev->hdac.dev, "ELD invalid\n");
+			pin->eld.monitor_present = false;
+			pin->eld.eld_valid = false;
+		}
+
+	}
+
+	/*
+	 * Sometimes the pin_sense may present invalid monitor
+	 * present and eld_valid. If eld data is not valid loop, few
+	 * more times to get correct pin sense and valid eld.
+	 */
+	if ((!pin->eld.monitor_present || !pin->eld.eld_valid) && repoll)
+		schedule_delayed_work(&pin->work, msecs_to_jiffies(300));
+
+put_hdac_device:
+	pm_runtime_put_sync(&edev->hdac.dev);
+}
+static void hdac_hdmi_repoll_eld(struct work_struct *work)
+{
+	struct hdac_hdmi_pin *pin =
+		container_of(to_delayed_work(work), struct hdac_hdmi_pin, work);
+
+	/* picked from legacy */
+	if (pin->repoll_count++ > 6)
+		pin->repoll_count = 0;
+
+	hdac_hdmi_present_sense(pin, pin->repoll_count);
+}
 static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
 {
 	struct hdac_hdmi_priv *hdmi = edev->private_data;
@@ -446,6 +520,9 @@ static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
 	list_add_tail(&pin->head, &hdmi->pin_list);
 	hdmi->num_pin++;
 
+	pin->edev = edev;
+	INIT_DELAYED_WORK(&pin->work, hdac_hdmi_repoll_eld);
+
 	return 0;
 }
 
@@ -503,17 +580,50 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
 	return hdac_hdmi_init_dai_map(edev);
 }
 
+static void hdac_hdmi_eld_notify_cb(void *aptr, int port)
+{
+	struct hdac_ext_device *edev = aptr;
+	struct hdac_hdmi_priv *hdmi = edev->private_data;
+	struct hdac_hdmi_pin *pin;
+	/* Don't know how this mapping is derived */
+	hda_nid_t pin_nid = port + 0x04;
+
+	dev_dbg(&edev->hdac.dev, "%s: for pin: %d\n", __func__, pin_nid);
+
+	list_for_each_entry(pin, &hdmi->pin_list, head) {
+		if (pin->nid == pin_nid)
+			hdac_hdmi_present_sense(pin, 1);
+	}
+}
+
+static struct i915_audio_component_audio_ops aops = {
+	.pin_eld_notify	= hdac_hdmi_eld_notify_cb,
+};
+
 static int hdmi_codec_probe(struct snd_soc_codec *codec)
 {
 	struct hdac_ext_device *edev = snd_soc_codec_get_drvdata(codec);
 	struct hdac_hdmi_priv *hdmi = edev->private_data;
 	struct snd_soc_dapm_context *dapm =
 		snd_soc_component_get_dapm(&codec->component);
+	struct hdac_hdmi_pin *pin;
+	int ret;
 
 	edev->scodec = codec;
 
 	create_fill_widget_route_map(dapm, &hdmi->dai_map[0]);
 
+	aops.audio_ptr = edev;
+	ret = snd_hdac_i915_register_notifier(&aops);
+	if (ret < 0) {
+		dev_err(&edev->hdac.dev, "notifier register failed: err: %d\n",
+				ret);
+		return ret;
+	}
+
+	list_for_each_entry(pin, &hdmi->pin_list, head)
+		hdac_hdmi_eld_notify_cb(edev, (pin->nid - 0x04));
+
 	/* Imp: Store the card pointer in hda_codec */
 	edev->card = dapm->card->snd_card;
 
@@ -644,6 +754,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
 	if (!bus)
 		return 0;
 
+
 	err = snd_hdac_display_power(bus, true);
 	if (err < 0) {
 		dev_err(bus->dev, "Cannot turn on display power on i915\n");
-- 
1.9.1

Re: [PATCH 05/15] ASoC: hdac_hdmi: Add hotplug notification and read eld

[Takashi Iwai]

On Tue, 01 Dec 2015 18:47:01 +0100,
Subhransu S. Prusty wrote:
> 
> This patch uses i915 component framework to register for hotplug
> notification. And once it identifies valid pin sense and valid eld,
> reads the eld into the corresponding pin map buffer. For now it
> directly sends the verbs and reads the eld. Later this will use
> the i915 framework to populate ELD buffer once available.

This will have the same problem as we had in patch_hdmi.c.
When a sound driver goes to sleep before i915 and i915 triggers the
eld_notify for audio disablement, it can be screwed up.

See the commit 8ae743e82f0b86f3b860c27fc2c8f574cf959fd0
    ALSA: hda - Skip ELD notification during system suspend
in for-linus branch of sound git tree.


Takashi

> 
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  sound/soc/codecs/hdac_hdmi.c | 125 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 118 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 0869855..203c99f 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -34,6 +34,9 @@
>  
>  #define HDA_MAX_CONNECTIONS     32
>  
> +#define ELD_MAX_SIZE    256
> +#define ELD_FIXED_BYTES	20
> +
>  struct hdac_hdmi_cvt_params {
>  	unsigned int channels_min;
>  	unsigned int channels_max;
> @@ -48,11 +51,22 @@ struct hdac_hdmi_cvt {
>  	struct hdac_hdmi_cvt_params params;
>  };
>  
> +struct hdmi_eld {
> +	bool	monitor_present;
> +	bool	eld_valid;
> +	int	eld_size;
> +	char    eld_buffer[ELD_MAX_SIZE];
> +};
> +
>  struct hdac_hdmi_pin {
>  	struct list_head head;
>  	hda_nid_t nid;
>  	int num_mux_nids;
>  	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
> +	struct hdmi_eld eld;
> +	struct hdac_ext_device *edev;
> +	int repoll_count;
> +	struct delayed_work work;
>  };
>  
>  struct hdac_hdmi_dai_pin_map {
> @@ -246,7 +260,6 @@ static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
>  	struct hdac_ext_device *hdac = snd_soc_dai_get_drvdata(dai);
>  	struct hdac_hdmi_priv *hdmi = hdac->private_data;
>  	struct hdac_hdmi_dai_pin_map *dai_map;
> -	int val;
>  
>  	if (dai->id > 0) {
>  		dev_err(&hdac->hdac.dev, "Only one dai supported as of now\n");
> @@ -255,12 +268,11 @@ static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
>  
>  	dai_map = &hdmi->dai_map[dai->id];
>  
> -	val = snd_hdac_codec_read(&hdac->hdac, dai_map->pin->nid, 0,
> -					AC_VERB_GET_PIN_SENSE, 0);
> -	dev_info(&hdac->hdac.dev, "Val for AC_VERB_GET_PIN_SENSE: %x\n", val);
> -
> -	if ((!(val & AC_PINSENSE_PRESENCE)) || (!(val & AC_PINSENSE_ELDV))) {
> -		dev_err(&hdac->hdac.dev, "Monitor presence invalid with val: %x\n", val);
> +	if ((!dai_map->pin->eld.monitor_present) ||
> +		(!dai_map->pin->eld.eld_valid)) {
> +		dev_err(&hdac->hdac.dev, "Failed: montior present? %d eld valid?: %d\n",
> +				dai_map->pin->eld.monitor_present,
> +				dai_map->pin->eld.eld_valid);
>  		return -ENODEV;
>  	}
>  
> @@ -432,6 +444,68 @@ static int hdac_hdmi_add_cvt(struct hdac_ext_device *edev, hda_nid_t nid)
>  	return hdac_hdmi_query_cvt_params(&edev->hdac, cvt);
>  }
>  
> +static void hdac_hdmi_present_sense(struct hdac_hdmi_pin *pin, int repoll)
> +{
> +	struct hdac_ext_device *edev = pin->edev;
> +	int val;
> +
> +	if (!edev)
> +		return;
> +
> +	pin->repoll_count = repoll;
> +
> +	pm_runtime_get_sync(&edev->hdac.dev);
> +	val = snd_hdac_codec_read(&edev->hdac, pin->nid, 0,
> +					AC_VERB_GET_PIN_SENSE, 0);
> +
> +	dev_dbg(&edev->hdac.dev, "Pin sense val %x for pin: %d\n",
> +			val, pin->nid);
> +	pin->eld.monitor_present = !!(val & AC_PINSENSE_PRESENCE);
> +	pin->eld.eld_valid = !!(val & AC_PINSENSE_ELDV);
> +
> +	if (!pin->eld.monitor_present || !pin->eld.eld_valid) {
> +		dev_info(&edev->hdac.dev, "%s: disconnect or eld_invalid\n",
> +				__func__);
> +		goto put_hdac_device;
> +	}
> +
> +	if (pin->eld.monitor_present && pin->eld.eld_valid) {
> +		/* TODO: Use i915 cmpnt framework when available */
> +		if (snd_hdac_get_eld(&edev->hdac, pin->nid,
> +				pin->eld.eld_buffer,
> +				&pin->eld.eld_size) == 0) {
> +			print_hex_dump_bytes("Eld: ", DUMP_PREFIX_OFFSET,
> +					pin->eld.eld_buffer, pin->eld.eld_size);
> +		} else {
> +			dev_err(&edev->hdac.dev, "ELD invalid\n");
> +			pin->eld.monitor_present = false;
> +			pin->eld.eld_valid = false;
> +		}
> +
> +	}
> +
> +	/*
> +	 * Sometimes the pin_sense may present invalid monitor
> +	 * present and eld_valid. If eld data is not valid loop, few
> +	 * more times to get correct pin sense and valid eld.
> +	 */
> +	if ((!pin->eld.monitor_present || !pin->eld.eld_valid) && repoll)
> +		schedule_delayed_work(&pin->work, msecs_to_jiffies(300));
> +
> +put_hdac_device:
> +	pm_runtime_put_sync(&edev->hdac.dev);
> +}
> +static void hdac_hdmi_repoll_eld(struct work_struct *work)
> +{
> +	struct hdac_hdmi_pin *pin =
> +		container_of(to_delayed_work(work), struct hdac_hdmi_pin, work);
> +
> +	/* picked from legacy */
> +	if (pin->repoll_count++ > 6)
> +		pin->repoll_count = 0;
> +
> +	hdac_hdmi_present_sense(pin, pin->repoll_count);
> +}
>  static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
>  {
>  	struct hdac_hdmi_priv *hdmi = edev->private_data;
> @@ -446,6 +520,9 @@ static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
>  	list_add_tail(&pin->head, &hdmi->pin_list);
>  	hdmi->num_pin++;
>  
> +	pin->edev = edev;
> +	INIT_DELAYED_WORK(&pin->work, hdac_hdmi_repoll_eld);
> +
>  	return 0;
>  }
>  
> @@ -503,17 +580,50 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
>  	return hdac_hdmi_init_dai_map(edev);
>  }
>  
> +static void hdac_hdmi_eld_notify_cb(void *aptr, int port)
> +{
> +	struct hdac_ext_device *edev = aptr;
> +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> +	struct hdac_hdmi_pin *pin;
> +	/* Don't know how this mapping is derived */
> +	hda_nid_t pin_nid = port + 0x04;
> +
> +	dev_dbg(&edev->hdac.dev, "%s: for pin: %d\n", __func__, pin_nid);
> +
> +	list_for_each_entry(pin, &hdmi->pin_list, head) {
> +		if (pin->nid == pin_nid)
> +			hdac_hdmi_present_sense(pin, 1);
> +	}
> +}
> +
> +static struct i915_audio_component_audio_ops aops = {
> +	.pin_eld_notify	= hdac_hdmi_eld_notify_cb,
> +};
> +
>  static int hdmi_codec_probe(struct snd_soc_codec *codec)
>  {
>  	struct hdac_ext_device *edev = snd_soc_codec_get_drvdata(codec);
>  	struct hdac_hdmi_priv *hdmi = edev->private_data;
>  	struct snd_soc_dapm_context *dapm =
>  		snd_soc_component_get_dapm(&codec->component);
> +	struct hdac_hdmi_pin *pin;
> +	int ret;
>  
>  	edev->scodec = codec;
>  
>  	create_fill_widget_route_map(dapm, &hdmi->dai_map[0]);
>  
> +	aops.audio_ptr = edev;
> +	ret = snd_hdac_i915_register_notifier(&aops);
> +	if (ret < 0) {
> +		dev_err(&edev->hdac.dev, "notifier register failed: err: %d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	list_for_each_entry(pin, &hdmi->pin_list, head)
> +		hdac_hdmi_eld_notify_cb(edev, (pin->nid - 0x04));
> +
>  	/* Imp: Store the card pointer in hda_codec */
>  	edev->card = dapm->card->snd_card;
>  
> @@ -644,6 +754,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
>  	if (!bus)
>  		return 0;
>  
> +
>  	err = snd_hdac_display_power(bus, true);
>  	if (err < 0) {
>  		dev_err(bus->dev, "Cannot turn on display power on i915\n");
> -- 
> 1.9.1
> 

Re: [PATCH 05/15] ASoC: hdac_hdmi: Add hotplug notification and read eld

[Subhransu S. Prusty]

On Tue, Dec 01, 2015 at 01:37:37PM +0100, Takashi Iwai wrote:
> On Tue, 01 Dec 2015 18:47:01 +0100,
> Subhransu S. Prusty wrote:
> > 
> > This patch uses i915 component framework to register for hotplug
> > notification. And once it identifies valid pin sense and valid eld,
> > reads the eld into the corresponding pin map buffer. For now it
> > directly sends the verbs and reads the eld. Later this will use
> > the i915 framework to populate ELD buffer once available.
> 
> This will have the same problem as we had in patch_hdmi.c.
> When a sound driver goes to sleep before i915 and i915 triggers the
> eld_notify for audio disablement, it can be screwed up.
> 
> See the commit 8ae743e82f0b86f3b860c27fc2c8f574cf959fd0
>     ALSA: hda - Skip ELD notification during system suspend
> in for-linus branch of sound git tree.

Yes. Will add this fix in the driver.
> 
> 
> Takashi
> 
> > 
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> >  sound/soc/codecs/hdac_hdmi.c | 125 ++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 118 insertions(+), 7 deletions(-)
> > 
> > diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> > index 0869855..203c99f 100644
> > --- a/sound/soc/codecs/hdac_hdmi.c
> > +++ b/sound/soc/codecs/hdac_hdmi.c
> > @@ -34,6 +34,9 @@
> >  
> >  #define HDA_MAX_CONNECTIONS     32
> >  
> > +#define ELD_MAX_SIZE    256
> > +#define ELD_FIXED_BYTES	20
> > +
> >  struct hdac_hdmi_cvt_params {
> >  	unsigned int channels_min;
> >  	unsigned int channels_max;
> > @@ -48,11 +51,22 @@ struct hdac_hdmi_cvt {
> >  	struct hdac_hdmi_cvt_params params;
> >  };
> >  
> > +struct hdmi_eld {
> > +	bool	monitor_present;
> > +	bool	eld_valid;
> > +	int	eld_size;
> > +	char    eld_buffer[ELD_MAX_SIZE];
> > +};
> > +
> >  struct hdac_hdmi_pin {
> >  	struct list_head head;
> >  	hda_nid_t nid;
> >  	int num_mux_nids;
> >  	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
> > +	struct hdmi_eld eld;
> > +	struct hdac_ext_device *edev;
> > +	int repoll_count;
> > +	struct delayed_work work;
> >  };
> >  
> >  struct hdac_hdmi_dai_pin_map {
> > @@ -246,7 +260,6 @@ static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
> >  	struct hdac_ext_device *hdac = snd_soc_dai_get_drvdata(dai);
> >  	struct hdac_hdmi_priv *hdmi = hdac->private_data;
> >  	struct hdac_hdmi_dai_pin_map *dai_map;
> > -	int val;
> >  
> >  	if (dai->id > 0) {
> >  		dev_err(&hdac->hdac.dev, "Only one dai supported as of now\n");
> > @@ -255,12 +268,11 @@ static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
> >  
> >  	dai_map = &hdmi->dai_map[dai->id];
> >  
> > -	val = snd_hdac_codec_read(&hdac->hdac, dai_map->pin->nid, 0,
> > -					AC_VERB_GET_PIN_SENSE, 0);
> > -	dev_info(&hdac->hdac.dev, "Val for AC_VERB_GET_PIN_SENSE: %x\n", val);
> > -
> > -	if ((!(val & AC_PINSENSE_PRESENCE)) || (!(val & AC_PINSENSE_ELDV))) {
> > -		dev_err(&hdac->hdac.dev, "Monitor presence invalid with val: %x\n", val);
> > +	if ((!dai_map->pin->eld.monitor_present) ||
> > +		(!dai_map->pin->eld.eld_valid)) {
> > +		dev_err(&hdac->hdac.dev, "Failed: montior present? %d eld valid?: %d\n",
> > +				dai_map->pin->eld.monitor_present,
> > +				dai_map->pin->eld.eld_valid);
> >  		return -ENODEV;
> >  	}
> >  
> > @@ -432,6 +444,68 @@ static int hdac_hdmi_add_cvt(struct hdac_ext_device *edev, hda_nid_t nid)
> >  	return hdac_hdmi_query_cvt_params(&edev->hdac, cvt);
> >  }
> >  
> > +static void hdac_hdmi_present_sense(struct hdac_hdmi_pin *pin, int repoll)
> > +{
> > +	struct hdac_ext_device *edev = pin->edev;
> > +	int val;
> > +
> > +	if (!edev)
> > +		return;
> > +
> > +	pin->repoll_count = repoll;
> > +
> > +	pm_runtime_get_sync(&edev->hdac.dev);
> > +	val = snd_hdac_codec_read(&edev->hdac, pin->nid, 0,
> > +					AC_VERB_GET_PIN_SENSE, 0);
> > +
> > +	dev_dbg(&edev->hdac.dev, "Pin sense val %x for pin: %d\n",
> > +			val, pin->nid);
> > +	pin->eld.monitor_present = !!(val & AC_PINSENSE_PRESENCE);
> > +	pin->eld.eld_valid = !!(val & AC_PINSENSE_ELDV);
> > +
> > +	if (!pin->eld.monitor_present || !pin->eld.eld_valid) {
> > +		dev_info(&edev->hdac.dev, "%s: disconnect or eld_invalid\n",
> > +				__func__);
> > +		goto put_hdac_device;
> > +	}
> > +
> > +	if (pin->eld.monitor_present && pin->eld.eld_valid) {
> > +		/* TODO: Use i915 cmpnt framework when available */
> > +		if (snd_hdac_get_eld(&edev->hdac, pin->nid,
> > +				pin->eld.eld_buffer,
> > +				&pin->eld.eld_size) == 0) {
> > +			print_hex_dump_bytes("Eld: ", DUMP_PREFIX_OFFSET,
> > +					pin->eld.eld_buffer, pin->eld.eld_size);
> > +		} else {
> > +			dev_err(&edev->hdac.dev, "ELD invalid\n");
> > +			pin->eld.monitor_present = false;
> > +			pin->eld.eld_valid = false;
> > +		}
> > +
> > +	}
> > +
> > +	/*
> > +	 * Sometimes the pin_sense may present invalid monitor
> > +	 * present and eld_valid. If eld data is not valid loop, few
> > +	 * more times to get correct pin sense and valid eld.
> > +	 */
> > +	if ((!pin->eld.monitor_present || !pin->eld.eld_valid) && repoll)
> > +		schedule_delayed_work(&pin->work, msecs_to_jiffies(300));
> > +
> > +put_hdac_device:
> > +	pm_runtime_put_sync(&edev->hdac.dev);
> > +}
> > +static void hdac_hdmi_repoll_eld(struct work_struct *work)
> > +{
> > +	struct hdac_hdmi_pin *pin =
> > +		container_of(to_delayed_work(work), struct hdac_hdmi_pin, work);
> > +
> > +	/* picked from legacy */
> > +	if (pin->repoll_count++ > 6)
> > +		pin->repoll_count = 0;
> > +
> > +	hdac_hdmi_present_sense(pin, pin->repoll_count);
> > +}
> >  static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
> >  {
> >  	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > @@ -446,6 +520,9 @@ static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
> >  	list_add_tail(&pin->head, &hdmi->pin_list);
> >  	hdmi->num_pin++;
> >  
> > +	pin->edev = edev;
> > +	INIT_DELAYED_WORK(&pin->work, hdac_hdmi_repoll_eld);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -503,17 +580,50 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
> >  	return hdac_hdmi_init_dai_map(edev);
> >  }
> >  
> > +static void hdac_hdmi_eld_notify_cb(void *aptr, int port)
> > +{
> > +	struct hdac_ext_device *edev = aptr;
> > +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > +	struct hdac_hdmi_pin *pin;
> > +	/* Don't know how this mapping is derived */
> > +	hda_nid_t pin_nid = port + 0x04;
> > +
> > +	dev_dbg(&edev->hdac.dev, "%s: for pin: %d\n", __func__, pin_nid);
> > +
> > +	list_for_each_entry(pin, &hdmi->pin_list, head) {
> > +		if (pin->nid == pin_nid)
> > +			hdac_hdmi_present_sense(pin, 1);
> > +	}
> > +}
> > +
> > +static struct i915_audio_component_audio_ops aops = {
> > +	.pin_eld_notify	= hdac_hdmi_eld_notify_cb,
> > +};
> > +
> >  static int hdmi_codec_probe(struct snd_soc_codec *codec)
> >  {
> >  	struct hdac_ext_device *edev = snd_soc_codec_get_drvdata(codec);
> >  	struct hdac_hdmi_priv *hdmi = edev->private_data;
> >  	struct snd_soc_dapm_context *dapm =
> >  		snd_soc_component_get_dapm(&codec->component);
> > +	struct hdac_hdmi_pin *pin;
> > +	int ret;
> >  
> >  	edev->scodec = codec;
> >  
> >  	create_fill_widget_route_map(dapm, &hdmi->dai_map[0]);
> >  
> > +	aops.audio_ptr = edev;
> > +	ret = snd_hdac_i915_register_notifier(&aops);
> > +	if (ret < 0) {
> > +		dev_err(&edev->hdac.dev, "notifier register failed: err: %d\n",
> > +				ret);
> > +		return ret;
> > +	}
> > +
> > +	list_for_each_entry(pin, &hdmi->pin_list, head)
> > +		hdac_hdmi_eld_notify_cb(edev, (pin->nid - 0x04));
> > +
> >  	/* Imp: Store the card pointer in hda_codec */
> >  	edev->card = dapm->card->snd_card;
> >  
> > @@ -644,6 +754,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
> >  	if (!bus)
> >  		return 0;
> >  
> > +
> >  	err = snd_hdac_display_power(bus, true);
> >  	if (err < 0) {
> >  		dev_err(bus->dev, "Cannot turn on display power on i915\n");
> > -- 
> > 1.9.1
> > 

-- 

[PATCH 06/15] ALSA: pcm: Add DRM helper to set constraint for format

[Subhransu S. Prusty]

Setting the constraint format based on ELD was missing bit in
the sound/core pcm drm. Added with this patch.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/core/pcm_drm_eld.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/sound/core/pcm_drm_eld.c b/sound/core/pcm_drm_eld.c
index e70379f..0c5653e 100644
--- a/sound/core/pcm_drm_eld.c
+++ b/sound/core/pcm_drm_eld.c
@@ -20,11 +20,49 @@ static const unsigned int eld_rates[] = {
 	192000,
 };
 
+static unsigned int sad_format(const u8 *sad)
+{
+	return ((sad[0] >> 0x3) & 0x1f);
+}
+
+static unsigned int sad_sample_bits_lpcm(const u8 *sad)
+{
+	return (sad[2] & 7);
+}
+
 static unsigned int sad_max_channels(const u8 *sad)
 {
 	return 1 + (sad[0] & 7);
 }
 
+static int eld_limit_formats(struct snd_pcm_runtime *runtime, void *eld)
+{
+	u64 formats = SNDRV_PCM_FMTBIT_S16_LE;
+	int i;
+	const u8 *sad, *eld_buf = eld;
+
+	sad = drm_eld_sad(eld_buf);
+	if (!sad)
+		goto format_constraint;
+
+	for (i = drm_eld_sad_count(eld_buf); i > 0; i--, sad += 3) {
+		if (sad_format(sad) == 1) { /* AUDIO_CODING_TYPE_LPCM */
+
+			/* 20 bit */
+			if (sad_sample_bits_lpcm(sad) & 0x2)
+				formats |= SNDRV_PCM_FMTBIT_S32_LE;
+
+			/* 24 bit */
+			if (sad_sample_bits_lpcm(sad) & 0x4)
+				formats |= SNDRV_PCM_FMTBIT_S24_LE;
+		}
+	}
+
+format_constraint:
+	return snd_pcm_hw_constraint_mask64(runtime, SNDRV_PCM_HW_PARAM_FORMAT,
+				formats);
+}
+
 static int eld_limit_rates(struct snd_pcm_hw_params *params,
 			   struct snd_pcm_hw_rule *rule)
 {
@@ -93,7 +131,9 @@ int snd_pcm_hw_constraint_eld(struct snd_pcm_runtime *runtime, void *eld)
 	ret = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
 				  eld_limit_channels, eld,
 				  SNDRV_PCM_HW_PARAM_RATE, -1);
+	if (ret < 0)
+		return ret;
 
-	return ret;
+	return eld_limit_formats(runtime, eld);
 }
 EXPORT_SYMBOL_GPL(snd_pcm_hw_constraint_eld);
-- 
1.9.1

Re: [PATCH 06/15] ALSA: pcm: Add DRM helper to set constraint for format

[Takashi Iwai]

On Tue, 01 Dec 2015 18:47:02 +0100,
Subhransu S. Prusty wrote:
> 
> +static int eld_limit_formats(struct snd_pcm_runtime *runtime, void *eld)
> +{
> +	u64 formats = SNDRV_PCM_FMTBIT_S16_LE;
> +	int i;
> +	const u8 *sad, *eld_buf = eld;
> +
> +	sad = drm_eld_sad(eld_buf);
> +	if (!sad)
> +		goto format_constraint;
> +
> +	for (i = drm_eld_sad_count(eld_buf); i > 0; i--, sad += 3) {
> +		if (sad_format(sad) == 1) { /* AUDIO_CODING_TYPE_LPCM */
> +
> +			/* 20 bit */
> +			if (sad_sample_bits_lpcm(sad) & 0x2)
> +				formats |= SNDRV_PCM_FMTBIT_S32_LE;
> +
> +			/* 24 bit */
> +			if (sad_sample_bits_lpcm(sad) & 0x4)
> +				formats |= SNDRV_PCM_FMTBIT_S24_LE;

Does it work correctly with HD-audio...?


Takashi

Re: [PATCH 06/15] ALSA: pcm: Add DRM helper to set constraint for format

[Subhransu S. Prusty]

On Tue, Dec 01, 2015 at 01:55:03PM +0100, Takashi Iwai wrote:
> On Tue, 01 Dec 2015 18:47:02 +0100,
> Subhransu S. Prusty wrote:
> > 
> > +static int eld_limit_formats(struct snd_pcm_runtime *runtime, void *eld)
> > +{
> > +	u64 formats = SNDRV_PCM_FMTBIT_S16_LE;
> > +	int i;
> > +	const u8 *sad, *eld_buf = eld;
> > +
> > +	sad = drm_eld_sad(eld_buf);
> > +	if (!sad)
> > +		goto format_constraint;
> > +
> > +	for (i = drm_eld_sad_count(eld_buf); i > 0; i--, sad += 3) {
> > +		if (sad_format(sad) == 1) { /* AUDIO_CODING_TYPE_LPCM */
> > +
> > +			/* 20 bit */
> > +			if (sad_sample_bits_lpcm(sad) & 0x2)
> > +				formats |= SNDRV_PCM_FMTBIT_S32_LE;
> > +
> > +			/* 24 bit */
> > +			if (sad_sample_bits_lpcm(sad) & 0x4)
> > +				formats |= SNDRV_PCM_FMTBIT_S24_LE;
> 
> Does it work correctly with HD-audio...?

It works correctly. Do you suspect something wrong?
> 
> 
> Takashi

-- 

Re: [PATCH 06/15] ALSA: pcm: Add DRM helper to set constraint for format

[Takashi Iwai]

On Wed, 02 Dec 2015 13:27:21 +0100,
Subhransu S. Prusty wrote:
> 
> On Tue, Dec 01, 2015 at 01:55:03PM +0100, Takashi Iwai wrote:
> > On Tue, 01 Dec 2015 18:47:02 +0100,
> > Subhransu S. Prusty wrote:
> > > 
> > > +static int eld_limit_formats(struct snd_pcm_runtime *runtime, void *eld)
> > > +{
> > > +	u64 formats = SNDRV_PCM_FMTBIT_S16_LE;
> > > +	int i;
> > > +	const u8 *sad, *eld_buf = eld;
> > > +
> > > +	sad = drm_eld_sad(eld_buf);
> > > +	if (!sad)
> > > +		goto format_constraint;
> > > +
> > > +	for (i = drm_eld_sad_count(eld_buf); i > 0; i--, sad += 3) {
> > > +		if (sad_format(sad) == 1) { /* AUDIO_CODING_TYPE_LPCM */
> > > +
> > > +			/* 20 bit */
> > > +			if (sad_sample_bits_lpcm(sad) & 0x2)
> > > +				formats |= SNDRV_PCM_FMTBIT_S32_LE;
> > > +
> > > +			/* 24 bit */
> > > +			if (sad_sample_bits_lpcm(sad) & 0x4)
> > > +				formats |= SNDRV_PCM_FMTBIT_S24_LE;
> > 
> > Does it work correctly with HD-audio...?
> 
> It works correctly. Do you suspect something wrong?

Traditionally, 24bit is supported by S32 format on HDA controller, so
I wonder it.


Takashi

[PATCH 07/15] ASoC: hdac_hdmi: Apply constraints based on ELD

[Subhransu S. Prusty]

Uses the drm eld core framework to apply rate, channel and
format constraint.

Even though the channel constraint is based on ELD, infoframe
is set with stereo only. Multichannel support will be added
later.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/Kconfig     | 1 +
 sound/soc/codecs/hdac_hdmi.c | 7 +++----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 8bba374..3c26df3 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -472,6 +472,7 @@ config SND_SOC_GTM601
 config SND_SOC_HDAC_HDMI
 	tristate
 	select SND_HDA_EXT_CORE
+	select SND_PCM_ELD
 	select HDMI
 
 config SND_SOC_ICS43432
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 203c99f..55db686 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -26,6 +26,7 @@
 #include <sound/soc.h>
 #include <sound/hdaudio_ext.h>
 #include <sound/hda_i915.h>
+#include <sound/pcm_drm_eld.h>
 #include "../../hda/local.h"
 
 #define AMP_OUT_MUTE		0xb080
@@ -281,10 +282,8 @@ static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
 	snd_hdac_codec_write(&hdac->hdac, dai_map->pin->nid, 0,
 			AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_UNMUTE);
 
-	snd_pcm_hw_constraint_step(substream->runtime, 0,
-				   SNDRV_PCM_HW_PARAM_CHANNELS, 2);
-
-	return 0;
+	return snd_pcm_hw_constraint_eld(substream->runtime,
+			dai_map->pin->eld.eld_buffer);
 }
 
 static void hdac_hdmi_pcm_close(struct snd_pcm_substream *substream,
-- 
1.9.1

[PATCH 08/15] ASoC: hdac_hdmi: Enable DP1.2 and all converters/pins

[Subhransu S. Prusty]

By default only one converter and pin widget are enabled. A vendor
widget required to be configured to enable all the widgets of the
codec.

As we are enabling the DP support enable the DP1.2 feature as well.

The changes below are copied from patch_hdmi.c

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 55db686..a9e14a0 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -525,6 +525,46 @@ static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
 	return 0;
 }
 
+#define INTEL_VENDOR_NID 0x08
+#define INTEL_GET_VENDOR_VERB 0xf81
+#define INTEL_SET_VENDOR_VERB 0x781
+#define INTEL_EN_DP12			0x02 /* enable DP 1.2 features */
+#define INTEL_EN_ALL_PIN_CVTS	0x01 /* enable 2nd & 3rd pins and convertors */
+
+static void hdac_hdmi_skl_enable_all_pins(struct hdac_device *hdac)
+{
+	unsigned int vendor_param;
+
+	vendor_param = snd_hdac_codec_read(hdac, INTEL_VENDOR_NID, 0,
+				INTEL_GET_VENDOR_VERB, 0);
+	if (vendor_param == -1 || vendor_param & INTEL_EN_ALL_PIN_CVTS)
+		return;
+
+	vendor_param |= INTEL_EN_ALL_PIN_CVTS;
+	vendor_param = snd_hdac_codec_read(hdac, INTEL_VENDOR_NID, 0,
+				INTEL_SET_VENDOR_VERB, vendor_param);
+	if (vendor_param == -1)
+		return;
+}
+
+static void hdac_hdmi_skl_enable_dp12(struct hdac_device *hdac)
+{
+	unsigned int vendor_param;
+
+	vendor_param = snd_hdac_codec_read(hdac, INTEL_VENDOR_NID, 0,
+				INTEL_GET_VENDOR_VERB, 0);
+	if (vendor_param == -1 || vendor_param & INTEL_EN_DP12)
+		return;
+
+	/* enable DP1.2 mode */
+	vendor_param |= INTEL_EN_DP12;
+	vendor_param = snd_hdac_codec_read(hdac, INTEL_VENDOR_NID, 0,
+				INTEL_SET_VENDOR_VERB, vendor_param);
+	if (vendor_param == -1)
+		return;
+
+}
+
 /*
  * Parse all nodes and store the cvt/pin nids in array
  * Add one time initialization for pin and cvt widgets
@@ -537,6 +577,9 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
 	struct hdac_hdmi_priv *hdmi = edev->private_data;
 	int ret;
 
+	hdac_hdmi_skl_enable_all_pins(hdac);
+	hdac_hdmi_skl_enable_dp12(hdac);
+
 	hdac->num_nodes = snd_hdac_get_sub_nodes(hdac, hdac->afg, &nid);
 	if (!nid || hdac->num_nodes <= 0) {
 		dev_warn(&hdac->dev, "HDMI: failed to get afg sub nodes\n");
-- 
1.9.1

[PATCH 09/15] ASoC: hdac_hdmi - create dais based on number of streams

[Subhransu S. Prusty]

A stream is mapped to a converter. So based on the converters
queried, dais are created.

The streams can be dynamically routed to any converter. For
now it is mapped statically. The dynamic mapping of stream
to converter will be added when required.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 124 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 92 insertions(+), 32 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index a9e14a0..3c4254c 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -29,6 +29,8 @@
 #include <sound/pcm_drm_eld.h>
 #include "../../hda/local.h"
 
+#define NAME_SIZE	32
+
 #define AMP_OUT_MUTE		0xb080
 #define AMP_OUT_UNMUTE		0xb000
 #define PIN_OUT			(AC_PINCTL_OUT_EN)
@@ -565,11 +567,84 @@ static void hdac_hdmi_skl_enable_dp12(struct hdac_device *hdac)
 
 }
 
+static struct snd_soc_dai_ops hdmi_dai_ops = {
+	.startup = hdac_hdmi_pcm_open,
+	.shutdown = hdac_hdmi_pcm_close,
+	.hw_params = hdac_hdmi_set_hw_params,
+	.prepare = hdac_hdmi_playback_prepare,
+	.hw_free = hdac_hdmi_playback_cleanup,
+};
+
+static int hdac_hdmi_create_dais(struct hdac_device *hdac,
+		struct snd_soc_dai_driver **dais,
+		struct hdac_hdmi_priv *hdmi, int num_dais)
+{
+	struct snd_soc_dai_driver *hdmi_dais;
+	struct hdac_hdmi_cvt *cvt;
+	char name[NAME_SIZE], dai_name[NAME_SIZE];
+	int i = 0, j;
+	u32 rates, bps;
+	unsigned int rate_max = 384000, rate_min = 8000;
+	u64 formats;
+	int ret;
+	static unsigned int rate_pcm[] = {
+		8000, 11025, 16000, 22050, 32000, 44100, 48000, 88200,
+		96000, 176400, 192000, 384000
+	};
+
+	hdmi_dais = devm_kzalloc(&hdac->dev, (sizeof(*hdmi_dais) * num_dais),
+			GFP_KERNEL);
+	if (!hdmi_dais)
+		return -ENOMEM;
+
+	list_for_each_entry(cvt, &hdmi->cvt_list, head) {
+		ret = snd_hdac_query_supported_pcm(hdac, cvt->nid, &rates,
+			&formats, &bps);
+		if (ret)
+			return ret;
+		for (j = 0; j < ARRAY_SIZE(rate_pcm); j++) {
+			if (rates & (1 << j)) {
+				rate_min = rate_pcm[j];
+				break;
+			}
+		}
+
+		for (j = ARRAY_SIZE(rate_pcm) - 1; j >= 0; j--) {
+			if (rates & (1 << j)) {
+				rate_max = rate_pcm[j];
+				break;
+			}
+		}
+
+		sprintf(dai_name, "intel-hdmi-hifi%d", i+1);
+		hdmi_dais[i].name = devm_kstrdup(&hdac->dev, dai_name,
+						GFP_KERNEL);
+
+		snprintf(name, sizeof(name), "hifi%d", i+1);
+		hdmi_dais[i].playback.stream_name =
+				devm_kstrdup(&hdac->dev, name, GFP_KERNEL);
+		hdmi_dais[i].playback.formats = formats;
+		hdmi_dais[i].playback.rates = rates;
+		hdmi_dais[i].playback.rate_max = rate_max;
+		hdmi_dais[i].playback.rate_min = rate_min;
+		hdmi_dais[i].playback.channels_min = 2;
+		hdmi_dais[i].playback.channels_max = 2;
+		hdmi_dais[i].ops = &hdmi_dai_ops;
+
+		i++;
+	}
+
+	*dais = hdmi_dais;
+
+	return 0;
+}
+
 /*
  * Parse all nodes and store the cvt/pin nids in array
  * Add one time initialization for pin and cvt widgets
  */
-static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
+static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev,
+		struct snd_soc_dai_driver **dais, int *num_dais)
 {
 	hda_nid_t nid;
 	int i;
@@ -619,6 +694,15 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
 	if (!hdmi->num_pin || !hdmi->num_cvt)
 		return -EIO;
 
+	ret = hdac_hdmi_create_dais(hdac, dais, hdmi, hdmi->num_cvt);
+	if (ret) {
+		dev_err(&hdac->dev, "Failed to create dais with err: %d\n",
+							ret);
+		return ret;
+	}
+
+	*num_dais = hdmi->num_cvt;
+
 	return hdac_hdmi_init_dai_map(edev);
 }
 
@@ -694,38 +778,12 @@ static struct snd_soc_codec_driver hdmi_hda_codec = {
 	.idle_bias_off	= true,
 };
 
-static struct snd_soc_dai_ops hdmi_dai_ops = {
-	.startup = hdac_hdmi_pcm_open,
-	.shutdown = hdac_hdmi_pcm_close,
-	.hw_params = hdac_hdmi_set_hw_params,
-	.prepare = hdac_hdmi_playback_prepare,
-	.hw_free = hdac_hdmi_playback_cleanup,
-};
-
-static struct snd_soc_dai_driver hdmi_dais[] = {
-	{	.name = "intel-hdmi-hif1",
-		.playback = {
-			.stream_name = "hif1",
-			.channels_min = 2,
-			.channels_max = 2,
-			.rates = SNDRV_PCM_RATE_32000 |
-				SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
-				SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000 |
-				SNDRV_PCM_RATE_176400 | SNDRV_PCM_RATE_192000,
-			.formats = SNDRV_PCM_FMTBIT_S16_LE |
-				SNDRV_PCM_FMTBIT_S20_3LE |
-				SNDRV_PCM_FMTBIT_S24_LE |
-				SNDRV_PCM_FMTBIT_S32_LE,
-
-		},
-		.ops = &hdmi_dai_ops,
-	},
-};
-
 static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
 {
 	struct hdac_device *codec = &edev->hdac;
 	struct hdac_hdmi_priv *hdmi_priv;
+	struct snd_soc_dai_driver *hdmi_dais = NULL;
+	int num_dais = 0;
 	int ret = 0;
 
 	hdmi_priv = devm_kzalloc(&codec->dev, sizeof(*hdmi_priv), GFP_KERNEL);
@@ -739,13 +797,15 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
 	INIT_LIST_HEAD(&hdmi_priv->pin_list);
 	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
 
-	ret = hdac_hdmi_parse_and_map_nid(edev);
-	if (ret < 0)
+	ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais);
+	if (ret < 0) {
+		dev_err(&codec->dev, "Failed in parse and map nid with err: %d\n", ret);
 		return ret;
+	}
 
 	/* ASoC specific initialization */
 	return snd_soc_register_codec(&codec->dev, &hdmi_hda_codec,
-			hdmi_dais, ARRAY_SIZE(hdmi_dais));
+			hdmi_dais, num_dais);
 }
 
 static int hdac_hdmi_dev_remove(struct hdac_ext_device *edev)
-- 
1.9.1

[PATCH 10/15] ASoC: hdac_hdmi: Create widget/route based on nodes enumerated

[Subhransu S. Prusty]

Instead of direct mapping between converter and pin, Muxes are
added between them to support any converter connection to any pin.

As the possible mux inputs can only be identified during runtime,
all possible routes are created to connect all converters to all
pin muxes. The user should enable appropriate mux inputs to enable
correct port.

In the process to support the above changes widget/route fill APIs
are updated to take all required parameters.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 224 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 202 insertions(+), 22 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 3c4254c..888d02e 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -345,46 +345,224 @@ static int hdac_hdmi_query_pin_connlist(struct hdac_ext_device *hdac,
 	return pin->num_mux_nids;
 }
 
-static void hdac_hdmi_fill_widget_info(struct snd_soc_dapm_widget *w,
-				enum snd_soc_dapm_type id,
-				const char *wname, const char *stream)
+static void hdac_hdmi_fill_widget_info(struct device *dev,
+				struct snd_soc_dapm_widget *w,
+				enum snd_soc_dapm_type id, void *priv,
+				const char *wname, const char *stream,
+				struct snd_kcontrol_new *wc, int numkc)
 {
 	w->id = id;
-	w->name = wname;
+	w->name = devm_kstrdup(dev, wname, GFP_KERNEL);
 	w->sname = stream;
 	w->reg = SND_SOC_NOPM;
 	w->shift = 0;
-	w->kcontrol_news = NULL;
-	w->num_kcontrols = 0;
-	w->priv = NULL;
+	w->kcontrol_news = wc;
+	w->num_kcontrols = numkc;
+	w->priv = priv;
 }
 
 static void hdac_hdmi_fill_route(struct snd_soc_dapm_route *route,
-		const char *sink, const char *control, const char *src)
+		const char *sink, const char *control, const char *src,
+		int (*handler)(struct snd_soc_dapm_widget *sr,
+			struct snd_soc_dapm_widget *snk))
 {
 	route->sink = sink;
 	route->source = src;
 	route->control = control;
-	route->connected = NULL;
+	route->connected = handler;
 }
 
-static void create_fill_widget_route_map(struct snd_soc_dapm_context *dapm,
-					struct hdac_hdmi_dai_pin_map *dai_map)
+/*
+ * Ideally the Mux inputs should be based on the num_muxs enumerated, but
+ * the display driver seem to be programming the connection list for the pin
+ * widget runtime.
+ *
+ * So programming all the possible inputs for the mux, the user has to take
+ * care of selecting the right one and leaving all other inputs selected to
+ * "NONE"
+ */
+static int hdac_hdmi_create_pin_muxs(struct hdac_ext_device *edev,
+				struct hdac_hdmi_pin *pin,
+				struct snd_soc_dapm_widget *widget,
+				const char *widget_name)
 {
-	struct snd_soc_dapm_route route[1];
-	struct snd_soc_dapm_widget widgets[2] = { {0} };
+	struct hdac_hdmi_priv *hdmi = edev->private_data;
+	struct snd_kcontrol_new *kc;
+	struct hdac_hdmi_cvt *cvt;
+	struct soc_enum *se;
+	char kc_name[NAME_SIZE];
+	char mux_items[NAME_SIZE];
+	/* To hold inputs to the Pin mux */
+	char *items[HDA_MAX_CONNECTIONS];
+	int i = 0;
+	int num_items = hdmi->num_cvt + 1;
+
+	kc = devm_kzalloc(&edev->hdac.dev, sizeof(*kc), GFP_KERNEL);
+	if (!kc)
+		return -ENOMEM;
+
+	se = devm_kzalloc(&edev->hdac.dev, sizeof(*se), GFP_KERNEL);
+	if (!se)
+		return -ENOMEM;
+
+	sprintf(kc_name, "Pin %d Input", pin->nid);
+	kc->name = devm_kstrdup(&edev->hdac.dev, kc_name, GFP_KERNEL);
+	kc->private_value = (long)se;
+	kc->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	kc->access = 0;
+	kc->info = snd_soc_info_enum_double;
+	kc->put = snd_soc_dapm_put_enum_double;
+	kc->get = snd_soc_dapm_get_enum_double;
 
-	memset(&route, 0, sizeof(route));
+	se->reg = SND_SOC_NOPM;
 
-	hdac_hdmi_fill_widget_info(&widgets[0], snd_soc_dapm_output,
-			"hif1 Output", NULL);
-	hdac_hdmi_fill_widget_info(&widgets[1], snd_soc_dapm_aif_in,
-			"Coverter 1", "hif1");
+	/* enum texts: ["NONE", "cvt #", "cvt #", ...] */
+	se->items = num_items;
+	se->mask = roundup_pow_of_two(se->items) - 1;
+
+	sprintf(mux_items, "NONE");
+	items[i] = devm_kstrdup(&edev->hdac.dev, mux_items, GFP_KERNEL);
+
+	list_for_each_entry(cvt, &hdmi->cvt_list, head) {
+		i++;
+		sprintf(mux_items, "cvt %d", cvt->nid);
+		items[i] = devm_kstrdup(&edev->hdac.dev, mux_items, GFP_KERNEL);
+	}
+
+	se->texts = devm_kmemdup(&edev->hdac.dev, items,
+			(num_items  * sizeof(char *)), GFP_KERNEL);
+
+	hdac_hdmi_fill_widget_info(&edev->hdac.dev, widget, snd_soc_dapm_mux,
+				&pin->nid, widget_name, NULL, kc, 1);
+
+	return 0;
+}
 
-	hdac_hdmi_fill_route(&route[0], "hif1 Output", NULL, "Coverter 1");
+/* Add cvt <- input <- mux route map */
+static void hdac_hdmi_add_pinmux_cvt_route(struct hdac_ext_device *edev,
+		struct snd_soc_dapm_widget *widgets,
+		struct snd_soc_dapm_route *route, int rindex)
+{
+	struct hdac_hdmi_priv *hdmi = edev->private_data;
+	const struct snd_kcontrol_new *kc;
+	struct soc_enum *se;
+	int mux_index = hdmi->num_cvt + hdmi->num_pin;
+	int i, j;
+
+	for (i = 0; i < hdmi->num_pin; i++) {
+		kc = widgets[mux_index].kcontrol_news;
+		se = (struct soc_enum *)kc->private_value;
+		for (j = 0; j < hdmi->num_cvt; j++) {
+			hdac_hdmi_fill_route(&route[rindex],
+					widgets[mux_index].name,
+					se->texts[j + 1],
+					widgets[j].name, NULL);
+
+			rindex++;
+		}
+
+		mux_index++;
+	}
+}
+
+/*
+ * Widgets are added in the below sequence
+ *	Converter widgets for num converters enumerated
+ *	Pin widgets for num pins enumerated
+ *	Pin mux widgets to represent connenction list of pin widget
+ *
+ * Total widgets elements = num_cvt + num_pin + num_pin;
+ *
+ * Routes are added as below:
+ *	pin mux -> pin (based on num_pins)
+ *	cvt -> "Input sel control" -> pin_mux
+ *
+ * Total route elements:
+ *	num_pins + (pin_muxes * num_cvt)
+ */
+static int create_fill_widget_route_map(struct snd_soc_dapm_context *dapm)
+{
+	struct snd_soc_dapm_widget *widgets;
+	struct snd_soc_dapm_route *route;
+	struct hdac_ext_device *edev = to_hda_ext_device(dapm->dev);
+	struct hdac_hdmi_priv *hdmi = edev->private_data;
+	struct snd_soc_dai_driver *dai_drv = dapm->component->dai_drv;
+	char widget_name[NAME_SIZE];
+	struct hdac_hdmi_cvt *cvt;
+	struct hdac_hdmi_pin *pin;
+	int i = 0;
+	int ret;
+	int num_routes = 0;
+
+	if (list_empty(&hdmi->cvt_list) || list_empty(&hdmi->pin_list))
+		return -EINVAL;
+
+	widgets = devm_kzalloc(dapm->dev,
+			(sizeof(*widgets) * ((2 * hdmi->num_pin) + hdmi->num_cvt)),
+			GFP_KERNEL);
+	if (!widgets)
+		return -ENOMEM;
+
+	/* DAPM widgets to represent each converter widget */
+	list_for_each_entry(cvt, &hdmi->cvt_list, head) {
+		sprintf(widget_name, "Converter %d", cvt->nid);
+		hdac_hdmi_fill_widget_info(dapm->dev, &widgets[i],
+			snd_soc_dapm_aif_in, &cvt->nid,
+			widget_name, dai_drv[i].playback.stream_name, NULL, 0);
+		i++;
+	}
+
+	list_for_each_entry(pin, &hdmi->pin_list, head) {
+		sprintf(widget_name, "hif%d Output", pin->nid);
+		hdac_hdmi_fill_widget_info(dapm->dev, &widgets[i],
+				snd_soc_dapm_output, &pin->nid,
+				widget_name, NULL, NULL, 0);
+		i++;
+	}
+
+	/* DAPM widgets to represent the connection list to pin widget */
+	list_for_each_entry(pin, &hdmi->pin_list, head) {
+		sprintf(widget_name, "Pin %d Mux", pin->nid);
+		ret = hdac_hdmi_create_pin_muxs(edev, pin, &widgets[i],
+							widget_name);
+		if (ret < 0)
+			return ret;
+		i++;
+
+		/* For cvt to pin_mux mapping */
+		num_routes += hdmi->num_cvt;
+
+		/* For pin_mux to pin mapping */
+		num_routes++;
+	}
+
+	route = devm_kzalloc(dapm->dev, (sizeof(*route) * num_routes),
+							GFP_KERNEL);
+	if (!route)
+		return -ENOMEM;
+
+	i = 0;
+	/* Add pin <- NULL <- mux route map */
+	list_for_each_entry(pin, &hdmi->pin_list, head) {
+		int sink_index = i + hdmi->num_cvt;
+		int src_index = sink_index + hdmi->num_pin;
+
+		hdac_hdmi_fill_route(&route[i], widgets[sink_index].name,
+				NULL, widgets[src_index].name,
+				NULL);
+		i++;
+
+	}
+
+	hdac_hdmi_add_pinmux_cvt_route(edev, widgets, route, i);
+
+	snd_soc_dapm_new_controls(dapm, widgets,
+		((2 * hdmi->num_pin) + hdmi->num_cvt));
+	snd_soc_dapm_add_routes(dapm, route, num_routes);
+	snd_soc_dapm_new_widgets(dapm->card);
+
+	return 0;
 
-	snd_soc_dapm_new_controls(dapm, widgets, ARRAY_SIZE(widgets));
-	snd_soc_dapm_add_routes(dapm, route, ARRAY_SIZE(route));
 }
 
 static int hdac_hdmi_init_dai_map(struct hdac_ext_device *edev)
@@ -737,7 +915,9 @@ static int hdmi_codec_probe(struct snd_soc_codec *codec)
 
 	edev->scodec = codec;
 
-	create_fill_widget_route_map(dapm, &hdmi->dai_map[0]);
+	ret = create_fill_widget_route_map(dapm);
+	if (ret < 0)
+		return ret;
 
 	aops.audio_ptr = edev;
 	ret = snd_hdac_i915_register_notifier(&aops);
-- 
1.9.1

[PATCH 11/15] ASoC: hdac_hdmi: Assign pin for stream based on dapm connection

[Subhransu S. Prusty]

Now that we have all the widgets enumerated we can route the stream
to any pin widget based on Mux connection. So map the pin to stream
accordingly.

Also seems the connection list to the pin widgets are not static
and is updated runtime based on type of display attached to the port.
This looks to be a hardware behavior.

So querying of the connection list is removed from pin initialization
and used in selecting the pin for a dai map.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 208 +++++++++++++++++++++++++++++++------------
 1 file changed, 151 insertions(+), 57 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 888d02e..4cb4262 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -37,6 +37,8 @@
 
 #define HDA_MAX_CONNECTIONS     32
 
+#define HDA_MAX_CVTS		3
+
 #define ELD_MAX_SIZE    256
 #define ELD_FIXED_BYTES	20
 
@@ -79,7 +81,7 @@ struct hdac_hdmi_dai_pin_map {
 };
 
 struct hdac_hdmi_priv {
-	struct hdac_hdmi_dai_pin_map dai_map[3];
+	struct hdac_hdmi_dai_pin_map dai_map[HDA_MAX_CVTS];
 	struct list_head pin_list;
 	struct list_head cvt_list;
 	int num_pin;
@@ -257,12 +259,126 @@ static int hdac_hdmi_playback_cleanup(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int hdac_hdmi_enable_pin(struct hdac_ext_device *hdac,
+		struct hdac_hdmi_dai_pin_map *dai_map)
+{
+	int mux_idx;
+	struct hdac_hdmi_pin *pin = dai_map->pin;
+
+	for (mux_idx = 0; mux_idx < pin->num_mux_nids; mux_idx++) {
+		if (pin->mux_nids[mux_idx] == dai_map->cvt->nid) {
+			snd_hdac_codec_write(&hdac->hdac, pin->nid, 0,
+					AC_VERB_SET_CONNECT_SEL, mux_idx);
+			break;
+		}
+	}
+
+	if (mux_idx == pin->num_mux_nids)
+		return -EIO;
+
+	/* Enable out path for this pin widget */
+	snd_hdac_codec_write(&hdac->hdac, pin->nid, 0,
+			AC_VERB_SET_PIN_WIDGET_CONTROL, PIN_OUT);
+
+	hdac_hdmi_set_power_state(hdac, dai_map, AC_PWRST_D0);
+
+	snd_hdac_codec_write(&hdac->hdac, pin->nid, 0,
+			AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_UNMUTE);
+
+	return 0;
+}
+
+static int hdac_hdmi_query_pin_connlist(struct hdac_ext_device *hdac,
+					struct hdac_hdmi_pin *pin)
+{
+	if (!(get_wcaps(&hdac->hdac, pin->nid) & AC_WCAP_CONN_LIST)) {
+		dev_warn(&hdac->hdac.dev,
+			"HDMI: pin %d wcaps %#x does not support connection list\n",
+			pin->nid, get_wcaps(&hdac->hdac, pin->nid));
+		return -EINVAL;
+	}
+
+	pin->num_mux_nids = snd_hdac_get_connections(&hdac->hdac, pin->nid,
+			pin->mux_nids, HDA_MAX_CONNECTIONS);
+	if (pin->num_mux_nids == 0)
+		dev_warn(&hdac->hdac.dev, "No connections found for pin: %d\n", pin->nid);
+
+	dev_dbg(&hdac->hdac.dev, "num_mux_nids %d for pin: %d\n",
+			pin->num_mux_nids, pin->nid);
+
+	return pin->num_mux_nids;
+}
+
+static inline struct hdac_hdmi_pin *hdac_hdmi_get_pin(
+			struct hdac_ext_device *edev,
+			struct snd_soc_dapm_path *p,
+			struct hdac_hdmi_cvt *cvt)
+{
+	struct hdac_hdmi_priv *hdmi = edev->private_data;
+	struct hdac_hdmi_pin *pin;
+	hda_nid_t *nid;
+	int ret, i;
+
+	nid = (hda_nid_t *)p->sink->priv;
+	list_for_each_entry(pin, &hdmi->pin_list, head) {
+		if (pin->nid == *nid) {
+			ret = hdac_hdmi_query_pin_connlist(edev, pin);
+			if (ret < 0)
+				continue;
+
+			for (i = 0; i < pin->num_mux_nids; i++) {
+				if (pin->mux_nids[i] == cvt->nid)
+					return pin;
+			}
+		}
+	}
+
+	return NULL;
+}
+
+/*
+ * This queries mux widgets in each sink path of the dai widget and returns
+ * a matching pin widget to which the stream may be routed.
+ *
+ * The converter may be input to multiple pin muxes. So each
+ * pin mux (basically each pin widget) is queried to identify if
+ * the converter as one of the input, then the first pin match
+ * is selected for rendering.
+ *
+ * Same stream rendering to multiple pins simultaneously can be done
+ * possibly, but not supported for now.
+ *
+ * So return the first pin connected
+ */
+static struct hdac_hdmi_pin *hdac_hdmi_get_pin_from_daistream(
+			struct hdac_ext_device *edev,
+			struct snd_soc_dapm_widget *strm_w,
+			struct hdac_hdmi_cvt *cvt)
+{
+	struct snd_soc_dapm_path *p;
+
+	snd_soc_dapm_widget_for_each_sink_path(strm_w, p) {
+		if (!p->connect)
+			continue;
+
+		if (strstr(p->sink->name, "Mux"))
+			return hdac_hdmi_get_pin(edev, p, cvt);
+		else
+			return hdac_hdmi_get_pin_from_daistream(edev, p->sink, cvt);
+	}
+
+	return NULL;
+}
+
 static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
 			struct snd_soc_dai *dai)
 {
 	struct hdac_ext_device *hdac = snd_soc_dai_get_drvdata(dai);
 	struct hdac_hdmi_priv *hdmi = hdac->private_data;
 	struct hdac_hdmi_dai_pin_map *dai_map;
+	struct hdac_hdmi_cvt *cvt;
+	struct hdac_hdmi_pin *pin;
+	int ret;
 
 	if (dai->id > 0) {
 		dev_err(&hdac->hdac.dev, "Only one dai supported as of now\n");
@@ -271,18 +387,23 @@ static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
 
 	dai_map = &hdmi->dai_map[dai->id];
 
-	if ((!dai_map->pin->eld.monitor_present) ||
-		(!dai_map->pin->eld.eld_valid)) {
+	cvt = dai_map->cvt;
+	pin = hdac_hdmi_get_pin_from_daistream(hdac, dai->playback_widget, cvt);
+	if (!pin)
+		return -EIO;
+
+	if ((!pin->eld.monitor_present) ||
+		(!pin->eld.eld_valid)) {
 		dev_err(&hdac->hdac.dev, "Failed: montior present? %d eld valid?: %d\n",
-				dai_map->pin->eld.monitor_present,
-				dai_map->pin->eld.eld_valid);
-		return -ENODEV;
+				pin->eld.monitor_present, pin->eld.eld_valid);
+		return -EINVAL;
 	}
 
-	hdac_hdmi_set_power_state(hdac, dai_map, AC_PWRST_D0);
+	dai_map->pin = pin;
 
-	snd_hdac_codec_write(&hdac->hdac, dai_map->pin->nid, 0,
-			AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_UNMUTE);
+	ret = hdac_hdmi_enable_pin(hdac, dai_map);
+	if (ret < 0)
+		return ret;
 
 	return snd_pcm_hw_constraint_eld(substream->runtime,
 			dai_map->pin->eld.eld_buffer);
@@ -301,6 +422,8 @@ static void hdac_hdmi_pcm_close(struct snd_pcm_substream *substream,
 
 	snd_hdac_codec_write(&hdac->hdac, dai_map->pin->nid, 0,
 			AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_MUTE);
+
+	dai_map->pin = NULL;
 }
 
 static int
@@ -323,28 +446,6 @@ hdac_hdmi_query_cvt_params(struct hdac_device *hdac, struct hdac_hdmi_cvt *cvt)
 	return err;
 }
 
-static int hdac_hdmi_query_pin_connlist(struct hdac_ext_device *hdac,
-					struct hdac_hdmi_pin *pin)
-{
-	if (!(get_wcaps(&hdac->hdac, pin->nid) & AC_WCAP_CONN_LIST)) {
-		dev_warn(&hdac->hdac.dev,
-			"HDMI: pin %d wcaps %#x does not support connection list\n",
-			pin->nid, get_wcaps(&hdac->hdac, pin->nid));
-		return -EINVAL;
-	}
-
-	pin->num_mux_nids = snd_hdac_get_connections(&hdac->hdac, pin->nid,
-			pin->mux_nids, HDA_MAX_CONNECTIONS);
-	if (pin->num_mux_nids == 0)
-		dev_warn(&hdac->hdac.dev, "No connections found for pin: %d\n",
-								pin->nid);
-
-	dev_dbg(&hdac->hdac.dev, "num_mux_nids %d for pin: %d\n",
-			pin->num_mux_nids, pin->nid);
-
-	return pin->num_mux_nids;
-}
-
 static void hdac_hdmi_fill_widget_info(struct device *dev,
 				struct snd_soc_dapm_widget *w,
 				enum snd_soc_dapm_type id, void *priv,
@@ -568,40 +669,33 @@ static int create_fill_widget_route_map(struct snd_soc_dapm_context *dapm)
 static int hdac_hdmi_init_dai_map(struct hdac_ext_device *edev)
 {
 	struct hdac_hdmi_priv *hdmi = edev->private_data;
-	struct hdac_hdmi_dai_pin_map *dai_map = &hdmi->dai_map[0];
+	struct hdac_hdmi_dai_pin_map *dai_map;
 	struct hdac_hdmi_cvt *cvt;
-	struct hdac_hdmi_pin *pin;
+	int dai_id = 0;
 
-	if (list_empty(&hdmi->cvt_list) || list_empty(&hdmi->pin_list))
+	if (list_empty(&hdmi->cvt_list))
 		return -EINVAL;
 
-	/*
-	 * Currently on board only 1 pin and 1 converter is enabled for
-	 * simplification, more will be added eventually
-	 * So using fixed map for dai_id:pin:cvt
-	 */
-	cvt = list_first_entry(&hdmi->cvt_list, struct hdac_hdmi_cvt, head);
-	pin = list_first_entry(&hdmi->pin_list, struct hdac_hdmi_pin, head);
-
-	dai_map->dai_id = 0;
-	dai_map->pin = pin;
-
-	dai_map->cvt = cvt;
+	list_for_each_entry(cvt, &hdmi->cvt_list, head) {
+		dai_map = &hdmi->dai_map[dai_id];
+		dai_map->dai_id = dai_id;
+		dai_map->cvt = cvt;
 
-	/* Enable out path for this pin widget */
-	snd_hdac_codec_write(&edev->hdac, pin->nid, 0,
-			AC_VERB_SET_PIN_WIDGET_CONTROL, PIN_OUT);
+		/* Enable transmission */
+		snd_hdac_codec_write(&edev->hdac, cvt->nid, 0,
+				AC_VERB_SET_DIGI_CONVERT_1, 1);
 
-	/* Enable transmission */
-	snd_hdac_codec_write(&edev->hdac, cvt->nid, 0,
-			AC_VERB_SET_DIGI_CONVERT_1, 1);
+		/* Category Code (CC) to zero */
+		snd_hdac_codec_write(&edev->hdac, cvt->nid, 0,
+				AC_VERB_SET_DIGI_CONVERT_2, 0);
 
-	/* Category Code (CC) to zero */
-	snd_hdac_codec_write(&edev->hdac, cvt->nid, 0,
-			AC_VERB_SET_DIGI_CONVERT_2, 0);
+		dai_id++;
 
-	snd_hdac_codec_write(&edev->hdac, pin->nid, 0,
-			AC_VERB_SET_CONNECT_SEL, 0);
+		if (dai_id == HDA_MAX_CVTS) {
+			dev_warn(&edev->hdac.dev, "Max dais supported: %d\n", dai_id);
+			break;
+		}
+	}
 
 	return 0;
 }
-- 
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

[PATCH 13/15] ASoC: hdac_hdmi: Add infoframe support for dp audio

[Subhransu S. Prusty]

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 70 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 60 insertions(+), 10 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 4cb4262..68e310e 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/hdmi.h>
+#include <drm/drm_edid.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <sound/hdaudio_ext.h>
@@ -126,27 +127,70 @@ hdac_hdmi_set_dip_index(struct hdac_ext_device *hdac, hda_nid_t pin_nid,
 				AC_VERB_SET_HDMI_DIP_INDEX, val);
 }
 
+struct dp_audio_infoframe {
+	u8 type; /* 0x84 */
+	u8 len;  /* 0x1b */
+	u8 ver;  /* 0x11 << 2 */
+
+	u8 CC02_CT47;	/* match with HDMI infoframe from this on */
+	u8 SS01_SF24;
+	u8 CXT04;
+	u8 CA;
+	u8 LFEPBL01_LSV36_DM_INH7;
+};
+
 static int hdac_hdmi_setup_audio_infoframe(struct hdac_ext_device *hdac,
 				hda_nid_t cvt_nid, hda_nid_t pin_nid)
 {
 	uint8_t buffer[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AUDIO_INFOFRAME_SIZE];
 	struct hdmi_audio_infoframe frame;
+	struct dp_audio_infoframe dp_ai;
+	struct hdac_hdmi_priv *hdmi = hdac->private_data;
+	struct hdac_hdmi_pin *pin;
 	u8 *dip = (u8 *)&frame;
 	int ret;
-	int i;
+	int i, conn_type;
+	const u8 *eld_buf;
+	int channels = 2;
 
-	hdmi_audio_infoframe_init(&frame);
+	list_for_each_entry(pin, &hdmi->pin_list, head) {
+		if (pin->nid == pin_nid)
+			break;
+	}
+
+	eld_buf = pin->eld.eld_buffer;
+	conn_type = drm_eld_get_conn_type(eld_buf);
 
-	/* Default stereo for now */
-	frame.channels = 2;
+	/* 0 is hdmi and 1 is DP */
+	if (conn_type > 1)
+		return -EIO;
 
 	/* setup channel count */
 	snd_hdac_codec_write(&hdac->hdac, cvt_nid, 0,
-			    AC_VERB_SET_CVT_CHAN_COUNT, frame.channels - 1);
+			    AC_VERB_SET_CVT_CHAN_COUNT, channels - 1);
 
-	ret = hdmi_audio_infoframe_pack(&frame, buffer, sizeof(buffer));
-	if (ret < 0)
-		return ret;
+	if (conn_type == 0) {
+		hdmi_audio_infoframe_init(&frame);
+
+		/* Default stereo for now */
+		frame.channels = channels;
+
+		ret = hdmi_audio_infoframe_pack(&frame, buffer, sizeof(buffer));
+		if (ret < 0)
+			return ret;
+
+		dip = (u8 *)&frame;
+
+	} else {
+		memset(&dp_ai, 0, sizeof(dp_ai));
+		dp_ai.type	= 0x84;
+		dp_ai.len	= 0x1b;
+		dp_ai.ver	= 0x11 << 2;
+		dp_ai.CC02_CT47	= channels - 1;
+		dp_ai.CA	= 0;
+
+		dip = (u8 *)&dip;
+	}
 
 	/* stop infoframe transmission */
 	hdac_hdmi_set_dip_index(hdac, pin_nid, 0x0, 0x0);
@@ -156,9 +200,15 @@ static int hdac_hdmi_setup_audio_infoframe(struct hdac_ext_device *hdac,
 
 	/*  Fill infoframe. Index auto-incremented */
 	hdac_hdmi_set_dip_index(hdac, pin_nid, 0x0, 0x0);
-	for (i = 0; i < sizeof(frame); i++)
-		snd_hdac_codec_write(&hdac->hdac, pin_nid, 0,
+	if (conn_type == 0) {
+		for (i = 0; i < sizeof(frame); i++)
+			snd_hdac_codec_write(&hdac->hdac, pin_nid, 0,
+				AC_VERB_SET_HDMI_DIP_DATA, dip[i]);
+	} else {
+		for (i = 0; i < sizeof(dp_ai); i++)
+			snd_hdac_codec_write(&hdac->hdac, pin_nid, 0,
 				AC_VERB_SET_HDMI_DIP_DATA, dip[i]);
+	}
 
 	/* Start infoframe */
 	hdac_hdmi_set_dip_index(hdac, pin_nid, 0x0, 0x0);
-- 
1.9.1

[PATCH 14/15] ASoC: hdac_hdmi: Add codec suspend/resume handler

[Subhransu S. Prusty]

From: Jeeja KP <jeeja.kp@intel.com>

Need to add enabling of all pins and DP1.2 feature again after
resume.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 68e310e..46aa5cc 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1096,9 +1096,24 @@ static int hdmi_codec_remove(struct snd_soc_codec *codec)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int hdmi_codec_resume(struct snd_soc_codec *codec)
+{
+	struct hdac_ext_device *edev = snd_soc_codec_get_drvdata(codec);
+
+	hdac_hdmi_skl_enable_all_pins(&edev->hdac);
+	hdac_hdmi_skl_enable_dp12(&edev->hdac);
+
+	return 0;
+}
+#else
+#define hdmi_codec_resume NULL
+#endif
+
 static struct snd_soc_codec_driver hdmi_hda_codec = {
 	.probe		= hdmi_codec_probe,
 	.remove		= hdmi_codec_remove,
+	.resume		= hdmi_codec_resume,
 	.idle_bias_off	= true,
 };
 
-- 
1.9.1

[PATCH 15/15] ASoC: hdac_hdmi: Keep display active while enumerating codec

[Subhransu S. Prusty]

From: Ramesh Babu <ramesh.babu@intel.com>

Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 46aa5cc..fa3cfa5 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
 	INIT_LIST_HEAD(&hdmi_priv->pin_list);
 	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
 
+	ret = snd_hdac_display_power(edev->hdac.bus, true);
+	if (ret < 0) {
+		dev_err(&edev->hdac.dev,
+			"Cannot turn on display power on i915 err: %d\n",
+			ret);
+		return ret;
+	}
+
 	ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais);
 	if (ret < 0) {
 		dev_err(&codec->dev, "Failed in parse and map nid with err: %d\n", ret);
-- 
1.9.1

Re: [PATCH 15/15] ASoC: hdac_hdmi: Keep display active while enumerating codec

[Takashi Iwai]

On Tue, 01 Dec 2015 18:47:11 +0100,
Subhransu S. Prusty wrote:
> 
> From: Ramesh Babu <ramesh.babu@intel.com>
> 
> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  sound/soc/codecs/hdac_hdmi.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 46aa5cc..fa3cfa5 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
>  	INIT_LIST_HEAD(&hdmi_priv->pin_list);
>  	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
>  
> +	ret = snd_hdac_display_power(edev->hdac.bus, true);
> +	if (ret < 0) {
> +		dev_err(&edev->hdac.dev,
> +			"Cannot turn on display power on i915 err: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
>  	ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais);
>  	if (ret < 0) {
>  		dev_err(&codec->dev, "Failed in parse and map nid with err: %d\n", ret);
> -- 
> 1.9.1

No counterpart to turn off?


Takashi

Re: [PATCH 15/15] ASoC: hdac_hdmi: Keep display active while enumerating codec

[Subhransu S. Prusty]

On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote:
> On Tue, 01 Dec 2015 18:47:11 +0100,
> Subhransu S. Prusty wrote:
> > 
> > From: Ramesh Babu <ramesh.babu@intel.com>
> > 
> > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> >  sound/soc/codecs/hdac_hdmi.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> > index 46aa5cc..fa3cfa5 100644
> > --- a/sound/soc/codecs/hdac_hdmi.c
> > +++ b/sound/soc/codecs/hdac_hdmi.c
> > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
> >  	INIT_LIST_HEAD(&hdmi_priv->pin_list);
> >  	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
> >  
> > +	ret = snd_hdac_display_power(edev->hdac.bus, true);
> > +	if (ret < 0) {
> > +		dev_err(&edev->hdac.dev,
> > +			"Cannot turn on display power on i915 err: %d\n",
> > +			ret);
> > +		return ret;
> > +	}
> > +
> >  	ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais);
> >  	if (ret < 0) {
> >  		dev_err(&codec->dev, "Failed in parse and map nid with err: %d\n", ret);
> > -- 
> > 1.9.1
> 
> No counterpart to turn off?
turned off in runtime suspend during first explicit suspend call.
> 
> 
> Takashi

-- 

Re: [PATCH 15/15] ASoC: hdac_hdmi: Keep display active while enumerating codec

[Takashi Iwai]

On Tue, 01 Dec 2015 22:48:41 +0100,
Subhransu S. Prusty wrote:
> 
> On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote:
> > On Tue, 01 Dec 2015 18:47:11 +0100,
> > Subhransu S. Prusty wrote:
> > > 
> > > From: Ramesh Babu <ramesh.babu@intel.com>
> > > 
> > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > ---
> > >  sound/soc/codecs/hdac_hdmi.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> > > index 46aa5cc..fa3cfa5 100644
> > > --- a/sound/soc/codecs/hdac_hdmi.c
> > > +++ b/sound/soc/codecs/hdac_hdmi.c
> > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
> > >  	INIT_LIST_HEAD(&hdmi_priv->pin_list);
> > >  	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
> > >  
> > > +	ret = snd_hdac_display_power(edev->hdac.bus, true);
> > > +	if (ret < 0) {
> > > +		dev_err(&edev->hdac.dev,
> > > +			"Cannot turn on display power on i915 err: %d\n",
> > > +			ret);
> > > +		return ret;
> > > +	}
> > > +
> > >  	ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais);
> > >  	if (ret < 0) {
> > >  		dev_err(&codec->dev, "Failed in parse and map nid with err: %d\n", ret);
> > > -- 
> > > 1.9.1
> > 
> > No counterpart to turn off?
> turned off in runtime suspend during first explicit suspend call.

It's a refcount, hence it does matter how many times it's called.
Does it really balance well?  It smells suspicious if you add only a
call to turn on without turning off...


Takashi

Re: [PATCH 15/15] ASoC: hdac_hdmi: Keep display active while enumerating codec

[Babu, Ramesh]

> On Tue, 01 Dec 2015 22:48:41 +0100,
> Subhransu S. Prusty wrote:
> >
> > On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote:
> > > On Tue, 01 Dec 2015 18:47:11 +0100,
> > > Subhransu S. Prusty wrote:
> > > >
> > > > From: Ramesh Babu <ramesh.babu@intel.com>
> > > >
> > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > > ---
> > > >  sound/soc/codecs/hdac_hdmi.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/sound/soc/codecs/hdac_hdmi.c
> b/sound/soc/codecs/hdac_hdmi.c
> > > > index 46aa5cc..fa3cfa5 100644
> > > > --- a/sound/soc/codecs/hdac_hdmi.c
> > > > +++ b/sound/soc/codecs/hdac_hdmi.c
> > > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct
> hdac_ext_device *edev)
> > > >  	INIT_LIST_HEAD(&hdmi_priv->pin_list);
> > > >  	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
> > > >
> > > > +	ret = snd_hdac_display_power(edev->hdac.bus, true);
> > > > +	if (ret < 0) {
> > > > +		dev_err(&edev->hdac.dev,
> > > > +			"Cannot turn on display power on i915 err: %d\n",
> > > > +			ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > >  	ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais);
> > > >  	if (ret < 0) {
> > > >  		dev_err(&codec->dev, "Failed in parse and map nid with err:
> %d\n", ret);
> > > > --
> > > > 1.9.1
> > >
> > > No counterpart to turn off?
> > turned off in runtime suspend during first explicit suspend call.
> 
> It's a refcount, hence it does matter how many times it's called.
> Does it really balance well?  It smells suspicious if you add only a
> call to turn on without turning off...

Display power is turned ON during device probe. When soc codec probe
completes, runtime suspend call is invoked and display power is turned OFF.
So refcount is balanced. 

Re: [PATCH 15/15] ASoC: hdac_hdmi: Keep display active while enumerating codec

[Takashi Iwai]

On Tue, 01 Dec 2015 17:48:43 +0100,
Babu, Ramesh wrote:
> 
> > On Tue, 01 Dec 2015 22:48:41 +0100,
> > Subhransu S. Prusty wrote:
> > >
> > > On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote:
> > > > On Tue, 01 Dec 2015 18:47:11 +0100,
> > > > Subhransu S. Prusty wrote:
> > > > >
> > > > > From: Ramesh Babu <ramesh.babu@intel.com>
> > > > >
> > > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > > > ---
> > > > >  sound/soc/codecs/hdac_hdmi.c | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/sound/soc/codecs/hdac_hdmi.c
> > b/sound/soc/codecs/hdac_hdmi.c
> > > > > index 46aa5cc..fa3cfa5 100644
> > > > > --- a/sound/soc/codecs/hdac_hdmi.c
> > > > > +++ b/sound/soc/codecs/hdac_hdmi.c
> > > > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct
> > hdac_ext_device *edev)
> > > > >  	INIT_LIST_HEAD(&hdmi_priv->pin_list);
> > > > >  	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
> > > > >
> > > > > +	ret = snd_hdac_display_power(edev->hdac.bus, true);
> > > > > +	if (ret < 0) {
> > > > > +		dev_err(&edev->hdac.dev,
> > > > > +			"Cannot turn on display power on i915 err: %d\n",
> > > > > +			ret);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > >  	ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais);
> > > > >  	if (ret < 0) {
> > > > >  		dev_err(&codec->dev, "Failed in parse and map nid with err:
> > %d\n", ret);
> > > > > --
> > > > > 1.9.1
> > > >
> > > > No counterpart to turn off?
> > > turned off in runtime suspend during first explicit suspend call.
> > 
> > It's a refcount, hence it does matter how many times it's called.
> > Does it really balance well?  It smells suspicious if you add only a
> > call to turn on without turning off...
> 
> Display power is turned ON during device probe. When soc codec probe
> completes, runtime suspend call is invoked and display power is turned OFF.
> So refcount is balanced. 

The runtime suspend of the codec, or of the controller?  I haven't
seen the codec suspend/resume doing it in this patchset, so I wonder.

if it's really well balanced, you should add the proper comment in the
code who turns off at when and where.  Otherwise reader will be lost.


Takashi

Re: [PATCH 15/15] ASoC: hdac_hdmi: Keep display active while enumerating codec

[Subhransu S. Prusty]

On Tue, Dec 01, 2015 at 06:08:28PM +0100, Takashi Iwai wrote:
> On Tue, 01 Dec 2015 17:48:43 +0100,
> Babu, Ramesh wrote:
> > 
> > > On Tue, 01 Dec 2015 22:48:41 +0100,
> > > Subhransu S. Prusty wrote:
> > > >
> > > > On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote:
> > > > > On Tue, 01 Dec 2015 18:47:11 +0100,
> > > > > Subhransu S. Prusty wrote:
> > > > > >
> > > > > > From: Ramesh Babu <ramesh.babu@intel.com>
> > > > > >
> > > > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> > > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > > > > ---
> > > > > >  sound/soc/codecs/hdac_hdmi.c | 8 ++++++++
> > > > > >  1 file changed, 8 insertions(+)
> > > > > >
> > > > > > diff --git a/sound/soc/codecs/hdac_hdmi.c
> > > b/sound/soc/codecs/hdac_hdmi.c
> > > > > > index 46aa5cc..fa3cfa5 100644
> > > > > > --- a/sound/soc/codecs/hdac_hdmi.c
> > > > > > +++ b/sound/soc/codecs/hdac_hdmi.c
> > > > > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct
> > > hdac_ext_device *edev)
> > > > > >  	INIT_LIST_HEAD(&hdmi_priv->pin_list);
> > > > > >  	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
> > > > > >
> > > > > > +	ret = snd_hdac_display_power(edev->hdac.bus, true);
> > > > > > +	if (ret < 0) {
> > > > > > +		dev_err(&edev->hdac.dev,
> > > > > > +			"Cannot turn on display power on i915 err: %d\n",
> > > > > > +			ret);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +
> > > > > >  	ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais);
> > > > > >  	if (ret < 0) {
> > > > > >  		dev_err(&codec->dev, "Failed in parse and map nid with err:
> > > %d\n", ret);
> > > > > > --
> > > > > > 1.9.1
> > > > >
> > > > > No counterpart to turn off?
> > > > turned off in runtime suspend during first explicit suspend call.
> > > 
> > > It's a refcount, hence it does matter how many times it's called.
> > > Does it really balance well?  It smells suspicious if you add only a
> > > call to turn on without turning off...
> > 
> > Display power is turned ON during device probe. When soc codec probe
> > completes, runtime suspend call is invoked and display power is turned OFF.
> > So refcount is balanced. 
> 
> The runtime suspend of the codec, or of the controller?  I haven't
> seen the codec suspend/resume doing it in this patchset, so I wonder.

It is turned off in the runtime suspend of the codec. It was added in the
basic driver patch series as part of PM enabling where it enables the
display during runtime resume and disable during runtime suspend. But
when the device runtime PM is first enabled and the device is put to
suspend, only runtime suspend gets called which doesn't actually decrement
the display reference count but throws a warning. With this patch that
gets fixed as well.

> 
> if it's really well balanced, you should add the proper comment in the
> code who turns off at when and where.  Otherwise reader will be lost.

Sure. Will udpate the changelog and add some comments in the code as well.

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

-- 

Re: [PATCH 15/15] ASoC: hdac_hdmi: Keep display active while enumerating codec

[Takashi Iwai]

On Wed, 02 Dec 2015 16:37:36 +0100,
Subhransu S. Prusty wrote:
> 
> On Tue, Dec 01, 2015 at 06:08:28PM +0100, Takashi Iwai wrote:
> > On Tue, 01 Dec 2015 17:48:43 +0100,
> > Babu, Ramesh wrote:
> > > 
> > > > On Tue, 01 Dec 2015 22:48:41 +0100,
> > > > Subhransu S. Prusty wrote:
> > > > >
> > > > > On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote:
> > > > > > On Tue, 01 Dec 2015 18:47:11 +0100,
> > > > > > Subhransu S. Prusty wrote:
> > > > > > >
> > > > > > > From: Ramesh Babu <ramesh.babu@intel.com>
> > > > > > >
> > > > > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> > > > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > > > > > ---
> > > > > > >  sound/soc/codecs/hdac_hdmi.c | 8 ++++++++
> > > > > > >  1 file changed, 8 insertions(+)
> > > > > > >
> > > > > > > diff --git a/sound/soc/codecs/hdac_hdmi.c
> > > > b/sound/soc/codecs/hdac_hdmi.c
> > > > > > > index 46aa5cc..fa3cfa5 100644
> > > > > > > --- a/sound/soc/codecs/hdac_hdmi.c
> > > > > > > +++ b/sound/soc/codecs/hdac_hdmi.c
> > > > > > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct
> > > > hdac_ext_device *edev)
> > > > > > >  	INIT_LIST_HEAD(&hdmi_priv->pin_list);
> > > > > > >  	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
> > > > > > >
> > > > > > > +	ret = snd_hdac_display_power(edev->hdac.bus, true);
> > > > > > > +	if (ret < 0) {
> > > > > > > +		dev_err(&edev->hdac.dev,
> > > > > > > +			"Cannot turn on display power on i915 err: %d\n",
> > > > > > > +			ret);
> > > > > > > +		return ret;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais);
> > > > > > >  	if (ret < 0) {
> > > > > > >  		dev_err(&codec->dev, "Failed in parse and map nid with err:
> > > > %d\n", ret);
> > > > > > > --
> > > > > > > 1.9.1
> > > > > >
> > > > > > No counterpart to turn off?
> > > > > turned off in runtime suspend during first explicit suspend call.
> > > > 
> > > > It's a refcount, hence it does matter how many times it's called.
> > > > Does it really balance well?  It smells suspicious if you add only a
> > > > call to turn on without turning off...
> > > 
> > > Display power is turned ON during device probe. When soc codec probe
> > > completes, runtime suspend call is invoked and display power is turned OFF.
> > > So refcount is balanced. 
> > 
> > The runtime suspend of the codec, or of the controller?  I haven't
> > seen the codec suspend/resume doing it in this patchset, so I wonder.
> 
> It is turned off in the runtime suspend of the codec. It was added in the
> basic driver patch series as part of PM enabling where it enables the
> display during runtime resume and disable during runtime suspend. But
> when the device runtime PM is first enabled and the device is put to
> suspend, only runtime suspend gets called which doesn't actually decrement
> the display reference count but throws a warning. With this patch that
> gets fixed as well.

So, the patch is about fixing the unbalance.  It clarifies my
suspect.  The problem is that $SUBJECT doesn't mention it at all, and 
even the patch had zero changelog text.  Please document more!

Also, if this is a fix, you should put Fixes tag.

BTW, who will turn off the display power at codec removal?


thanks,

Takashi

> > if it's really well balanced, you should add the proper comment in the
> > code who turns off at when and where.  Otherwise reader will be lost.
> 
> Sure. Will udpate the changelog and add some comments in the code as well.
> 
> > 
> > 
> > Takashi
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
> -- 
> 

Re: [PATCH 15/15] ASoC: hdac_hdmi: Keep display active while enumerating codec

[Takashi Iwai]

On Wed, 02 Dec 2015 11:24:53 +0100,
Takashi Iwai wrote:
> 
> BTW, who will turn off the display power at codec removal?

... that is, does the driver keep the display off or does it leave on?

A common practice for avoiding the unbalance is to add the on/off
always as a pair.  I guess it applies here, too.


Takashi

Re: [PATCH 15/15] ASoC: hdac_hdmi: Keep display active while enumerating codec

[Subhransu S. Prusty]

On Wed, Dec 02, 2015 at 11:24:53AM +0100, Takashi Iwai wrote:
> On Wed, 02 Dec 2015 16:37:36 +0100,
> Subhransu S. Prusty wrote:
> > 
> > On Tue, Dec 01, 2015 at 06:08:28PM +0100, Takashi Iwai wrote:
> > > On Tue, 01 Dec 2015 17:48:43 +0100,
> > > Babu, Ramesh wrote:
> > > > 
> > > > > On Tue, 01 Dec 2015 22:48:41 +0100,
> > > > > Subhransu S. Prusty wrote:
> > > > > >
> > > > > > On Tue, Dec 01, 2015 at 01:57:01PM +0100, Takashi Iwai wrote:
> > > > > > > On Tue, 01 Dec 2015 18:47:11 +0100,
> > > > > > > Subhransu S. Prusty wrote:
> > > > > > > >
> > > > > > > > From: Ramesh Babu <ramesh.babu@intel.com>
> > > > > > > >
> > > > > > > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> > > > > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > > > > > > ---
> > > > > > > >  sound/soc/codecs/hdac_hdmi.c | 8 ++++++++
> > > > > > > >  1 file changed, 8 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/sound/soc/codecs/hdac_hdmi.c
> > > > > b/sound/soc/codecs/hdac_hdmi.c
> > > > > > > > index 46aa5cc..fa3cfa5 100644
> > > > > > > > --- a/sound/soc/codecs/hdac_hdmi.c
> > > > > > > > +++ b/sound/soc/codecs/hdac_hdmi.c
> > > > > > > > @@ -1136,6 +1136,14 @@ static int hdac_hdmi_dev_probe(struct
> > > > > hdac_ext_device *edev)
> > > > > > > >  	INIT_LIST_HEAD(&hdmi_priv->pin_list);
> > > > > > > >  	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
> > > > > > > >
> > > > > > > > +	ret = snd_hdac_display_power(edev->hdac.bus, true);
> > > > > > > > +	if (ret < 0) {
> > > > > > > > +		dev_err(&edev->hdac.dev,
> > > > > > > > +			"Cannot turn on display power on i915 err: %d\n",
> > > > > > > > +			ret);
> > > > > > > > +		return ret;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > >  	ret = hdac_hdmi_parse_and_map_nid(edev, &hdmi_dais, &num_dais);
> > > > > > > >  	if (ret < 0) {
> > > > > > > >  		dev_err(&codec->dev, "Failed in parse and map nid with err:
> > > > > %d\n", ret);
> > > > > > > > --
> > > > > > > > 1.9.1
> > > > > > >
> > > > > > > No counterpart to turn off?
> > > > > > turned off in runtime suspend during first explicit suspend call.
> > > > > 
> > > > > It's a refcount, hence it does matter how many times it's called.
> > > > > Does it really balance well?  It smells suspicious if you add only a
> > > > > call to turn on without turning off...
> > > > 
> > > > Display power is turned ON during device probe. When soc codec probe
> > > > completes, runtime suspend call is invoked and display power is turned OFF.
> > > > So refcount is balanced. 
> > > 
> > > The runtime suspend of the codec, or of the controller?  I haven't
> > > seen the codec suspend/resume doing it in this patchset, so I wonder.
> > 
> > It is turned off in the runtime suspend of the codec. It was added in the
> > basic driver patch series as part of PM enabling where it enables the
> > display during runtime resume and disable during runtime suspend. But
> > when the device runtime PM is first enabled and the device is put to
> > suspend, only runtime suspend gets called which doesn't actually decrement
> > the display reference count but throws a warning. With this patch that
> > gets fixed as well.
> 
> So, the patch is about fixing the unbalance.  It clarifies my
> suspect.  The problem is that $SUBJECT doesn't mention it at all, and 
> even the patch had zero changelog text.  Please document more!

Sorry about that. Will add more in the changelog and add comments in the
code as well.

> 
> Also, if this is a fix, you should put Fixes tag.
Sure will do that.
> 
> BTW, who will turn off the display power at codec removal?

The driver keeps the display off. The display power is on in the RTPM resume
when a playbck starts and off in the RTPM suspend callback once the playback
is stopped except for the above scenario where it is turned on during
dev_probe call and put to off during the first explicit call to runtime
suspend.

Regards,
Subhransu

> 
> 
> thanks,
> 
> Takashi
> 
> > > if it's really well balanced, you should add the proper comment in the
> > > code who turns off at when and where.  Otherwise reader will be lost.
> > 
> > Sure. Will udpate the changelog and add some comments in the code as well.
> > 
> > > 
> > > 
> > > Takashi
> > > _______________________________________________
> > > Alsa-devel mailing list
> > > Alsa-devel@alsa-project.org
> > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > 
> > -- 
> > 

--