[v4,03/14] ASoC: hdac_hdmi: Use list to add pins and converters

Commit Message

Subhransu S. Prusty Dec. 9, 2015, 4:16 p.m. UTC

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 <[e-mail]>
Signed-off-by: Vinod Koul <[e-mail]>
---
 sound/soc/codecs/hdac_hdmi.c | 154 +++++++++++++++++++++++++++++--------------
 1 file changed, 106 insertions(+), 48 deletions(-)

Comments

Mark Brown Jan. 8, 2016, 1:32 p.m. UTC | #1

On Wed, Dec 09, 2015 at 09:46:10PM +0530, Subhransu S. Prusty wrote:

> 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.

This sounds like it's supposed to be supporting multiple outputs but...

> +	/*
> +	 * 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);

...it looks like we still only support one random pin and one random
convertor?  How do we know if we got the right ones?  I *think* this
mostly ends up doing the same thing as the previous version but if
that's the case why are we doing it?

As I've said before one of the reasons it sometimes takes a long time to
review these things is that there's a lot of big patch serieses getting
sent which aren't that well explained.

> -	snd_hdac_codec_write(&edev->hdac, pin_nid, 0,
> +	snd_hdac_codec_write(&edev->hdac, pin->nid, 0,

Huge portions of this patch are a simple refactoring like this, it'd be
good to have split those out as a separate patch so the actual content
is more visible.

Vinod Koul Jan. 8, 2016, 1:54 p.m. UTC | #2

On Fri, Jan 08, 2016 at 01:32:51PM +0000, Mark Brown wrote:
> On Wed, Dec 09, 2015 at 09:46:10PM +0530, Subhransu S. Prusty wrote:
> 
> > 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.
> 
> This sounds like it's supposed to be supporting multiple outputs but...
> 
> > +	/*
> > +	 * 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);
> 
> ...it looks like we still only support one random pin and one random
> convertor?  How do we know if we got the right ones?  I *think* this
> mostly ends up doing the same thing as the previous version but if
> that's the case why are we doing it?

Hi Mark,

This is the start of conversion to have multiple outputs supported. This
patch only converts to use list so that multiple pins can be used

Actual code to add support for three skl pins is in this series as explained
in the cover letter

> 
> As I've said before one of the reasons it sometimes takes a long time to
> review these things is that there's a lot of big patch serieses getting
> sent which aren't that well explained.
> 
> > -	snd_hdac_codec_write(&edev->hdac, pin_nid, 0,
> > +	snd_hdac_codec_write(&edev->hdac, pin->nid, 0,
> 
> Huge portions of this patch are a simple refactoring like this, it'd be
> good to have split those out as a separate patch so the actual content
> is more visible.

Yes this patch is only refactoring to use list for pins

Thanks

Mark Brown Jan. 8, 2016, 2:04 p.m. UTC | #3

On Fri, Jan 08, 2016 at 07:24:46PM +0530, Vinod Koul wrote:
> On Fri, Jan 08, 2016 at 01:32:51PM +0000, Mark Brown wrote:
> > On Wed, Dec 09, 2015 at 09:46:10PM +0530, Subhransu S. Prusty wrote:

> > > 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.

> > This sounds like it's supposed to be supporting multiple outputs but...

....

> > ...it looks like we still only support one random pin and one random
> > convertor?  How do we know if we got the right ones?  I *think* this
> > mostly ends up doing the same thing as the previous version but if
> > that's the case why are we doing it?

> This is the start of conversion to have multiple outputs supported. This
> patch only converts to use list so that multiple pins can be used

> Actual code to add support for three skl pins is in this series as explained
> in the cover letter

This is the sort of thing that needs to be in the patch changelogs.  I
know that at some point we want to get to having multiple outputs but
right now I'm reviewing this individual patch and I need to be able to
tell if it does what it's supposed to be doing.  Please do work on the
quality of your changelogs, as I keep saying difficulty in following
what the code is supposed to be doing is a constant problem with the
Skylake patches.

> > As I've said before one of the reasons it sometimes takes a long time to
> > review these things is that there's a lot of big patch serieses getting
> > sent which aren't that well explained.

> > > -	snd_hdac_codec_write(&edev->hdac, pin_nid, 0,
> > > +	snd_hdac_codec_write(&edev->hdac, pin->nid, 0,

> > Huge portions of this patch are a simple refactoring like this, it'd be
> > good to have split those out as a separate patch so the actual content
> > is more visible.

> Yes this patch is only refactoring to use list for pins

It looks like there's some other stuff going on too.  The changelog
certainly seems to suggest there is.

Patch

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 429fa14..b206d5e 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->dai_id = 0;
+	dai_map->pin = pin;
 
-	dai_map->cvt.nid = cvt_nid;
+	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 = kzalloc(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 = kzalloc(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, num_nodes;
 	struct hdac_device *hdac = &edev->hdac;
 	struct hdac_hdmi_priv *hdmi = edev->private_data;
-	int cvt_nid = 0, pin_nid = 0;
+	int ret;
 
 	num_nodes = snd_hdac_get_sub_nodes(hdac, hdac->afg, &nid);
 	if (!nid || num_nodes <= 0) {
@@ -438,29 +483,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)
@@ -544,6 +585,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;
@@ -555,8 +599,22 @@  static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
 
 static int hdac_hdmi_dev_remove(struct hdac_ext_device *edev)
 {
+	struct hdac_hdmi_priv *hdmi = edev->private_data;
+	struct hdac_hdmi_pin *pin, *pin_next;
+	struct hdac_hdmi_cvt *cvt, *cvt_next;
+
 	snd_soc_unregister_codec(&edev->hdac.dev);
 
+	list_for_each_entry_safe(cvt, cvt_next, &hdmi->cvt_list, head) {
+		list_del(&cvt->head);
+		kfree(cvt);
+	}
+
+	list_for_each_entry_safe(pin, pin_next, &hdmi->pin_list, head) {
+		list_del(&pin->head);
+		kfree(pin);
+	}
+
 	return 0;
 }