[PATCH 0/4] ALSA: hdac: fixes and support for hda codecs

[PATCH 0/4] ALSA: hdac: fixes and support for hda codecs

[Vinod Koul]

We had two fixes found on testing. First one checks if stream is in use or
not before releasing and second fixs wrong stread id map

Then we add structure defination for hdac_ext_dma_params and lastly add
helpers for read/write for codecs

Jeeja KP (1):
  ALSA: hdac: Fix to check if stream not in use in release

Subhransu S. Prusty (3):
  ALSA: hdac: Fix incorrect update of stream id mapping
  ALSA: hdac: structure definition for ext_dma_params
  ALSA: hdac: Add codec read/write and check power state for widgets

 include/sound/hdaudio.h         |  6 ++++++
 include/sound/hdaudio_ext.h     |  6 ++++++
 sound/hda/ext/hdac_ext_stream.c |  9 ++++----
 sound/hda/hdac_device.c         | 47 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+), 5 deletions(-)

-- 
2.4.3

[PATCH 1/4] ALSA: hdac: Fix incorrect update of stream id mapping

[Vinod Koul]

From: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>

Bits in LOSIDV need to be set to map the stream id for specific link.
Fixing this by setting the required bits in the register.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/hda/ext/hdac_ext_stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c
index 33ba77dd32f2..4bcebc8cde26 100644
--- a/sound/hda/ext/hdac_ext_stream.c
+++ b/sound/hda/ext/hdac_ext_stream.c
@@ -227,7 +227,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_link_stream_setup);
 void snd_hdac_ext_link_set_stream_id(struct hdac_ext_link *link,
 				 int stream)
 {
-	snd_hdac_updatew(link->ml_addr, AZX_REG_ML_LOSIDV, (1 << stream), 0);
+	snd_hdac_updatew(link->ml_addr, AZX_REG_ML_LOSIDV, (1 << stream), 1 << stream);
 }
 EXPORT_SYMBOL_GPL(snd_hdac_ext_link_set_stream_id);
 
-- 
2.4.3

[PATCH 2/4] ALSA: hdac: Fix to check if stream not in use in release

[Vinod Koul]

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

if the stream is decoupled and both link and host are used, while
releasing the stream, need to check if link and host stream are
not in use. This patch adds fix to check if the host/link stream
is in used before coupling it back when releasing the stream.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/hda/ext/hdac_ext_stream.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c
index 4bcebc8cde26..cb89ec7c8147 100644
--- a/sound/hda/ext/hdac_ext_stream.c
+++ b/sound/hda/ext/hdac_ext_stream.c
@@ -385,14 +385,13 @@ void snd_hdac_ext_stream_release(struct hdac_ext_stream *stream, int type)
 		break;
 
 	case HDAC_EXT_STREAM_TYPE_HOST:
-		if (stream->decoupled) {
+		if (stream->decoupled && !stream->link_locked)
 			snd_hdac_ext_stream_decouple(ebus, stream, false);
-			snd_hdac_stream_release(&stream->hstream);
-		}
+		snd_hdac_stream_release(&stream->hstream);
 		break;
 
 	case HDAC_EXT_STREAM_TYPE_LINK:
-		if (stream->decoupled)
+		if (stream->decoupled && !stream->hstream.opened)
 			snd_hdac_ext_stream_decouple(ebus, stream, false);
 		spin_lock_irq(&bus->reg_lock);
 		stream->link_locked = 0;
-- 
2.4.3

[PATCH 3/4] ALSA: hdac: structure definition for ext_dma_params

[Vinod Koul]

From: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>

This extends the structure definition of hdac_ext_device and adds definition for
dma_params which will be used with HDA codec.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/sound/hdaudio_ext.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
index 94210dcdb6ea..719edff2a77b 100644
--- a/include/sound/hdaudio_ext.h
+++ b/include/sound/hdaudio_ext.h
@@ -179,9 +179,15 @@ struct hdac_ext_device {
 	/* codec ops */
 	struct hdac_ext_codec_ops ops;
 
+	struct snd_card *card;
+	void *scodec;
 	void *private_data;
 };
 
+struct hdac_ext_dma_params {
+	u32 format;
+	u8 stream_tag;
+};
 #define to_ehdac_device(dev) (container_of((dev), \
 				 struct hdac_ext_device, hdac))
 /*
-- 
2.4.3

[PATCH 4/4] ALSA: hdac: Add codec read/write and check power state for widgets

[Vinod Koul]

From: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>

This adds helpers to read/write the codec. Also adds a helper to check the
power state of widgets.

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/sound/hdaudio.h |  6 ++++++
 sound/hda/hdac_device.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 49bc836fcd84..26e956f4b7c6 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -147,6 +147,12 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid,
 bool snd_hdac_is_supported_format(struct hdac_device *codec, hda_nid_t nid,
 				  unsigned int format);
 
+int snd_hdac_codec_read(struct hdac_device *hdac, hda_nid_t nid,
+			int flags, unsigned int verb, unsigned int parm);
+int snd_hdac_codec_write(struct hdac_device *hdac, hda_nid_t nid,
+			int flags, unsigned int verb, unsigned int parm);
+bool snd_hdac_check_power_state(struct hdac_device *hdac,
+		hda_nid_t nid, unsigned int target_state);
 /**
  * snd_hdac_read_parm - read a codec parameter
  * @codec: the codec object
diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index db96042a497f..24c7a5f6f0f3 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -952,3 +952,50 @@ bool snd_hdac_is_supported_format(struct hdac_device *codec, hda_nid_t nid,
 	return true;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_is_supported_format);
+
+static unsigned int codec_read(struct hdac_device *hdac, hda_nid_t nid,
+			int flags, unsigned int verb, unsigned int parm)
+{
+	unsigned int cmd = snd_hdac_make_cmd(hdac, nid, verb, parm);
+	unsigned int res;
+
+	if (snd_hdac_exec_verb(hdac, cmd, flags, &res))
+		return -1;
+
+	return res;
+}
+
+static int codec_write(struct hdac_device *hdac, hda_nid_t nid,
+			int flags, unsigned int verb, unsigned int parm)
+{
+	unsigned int cmd = snd_hdac_make_cmd(hdac, nid, verb, parm);
+
+	return snd_hdac_exec_verb(hdac, cmd, flags, NULL);
+}
+
+int snd_hdac_codec_read(struct hdac_device *hdac, hda_nid_t nid,
+			int flags, unsigned int verb, unsigned int parm)
+{
+	return codec_read(hdac, nid, flags, verb, parm);
+}
+EXPORT_SYMBOL_GPL(snd_hdac_codec_read);
+
+int snd_hdac_codec_write(struct hdac_device *hdac, hda_nid_t nid,
+			int flags, unsigned int verb, unsigned int parm)
+{
+	return codec_write(hdac, nid, flags, verb, parm);
+}
+EXPORT_SYMBOL_GPL(snd_hdac_codec_write);
+
+bool snd_hdac_check_power_state(struct hdac_device *hdac,
+		hda_nid_t nid, unsigned int target_state)
+{
+	unsigned int state = codec_read(hdac, nid, 0,
+				AC_VERB_GET_POWER_STATE, 0);
+
+	if (state & AC_PWRST_ERROR)
+		return true;
+	state = (state >> 4) & 0x0f;
+	return (state == target_state);
+}
+EXPORT_SYMBOL_GPL(snd_hdac_check_power_state);
-- 
2.4.3

Re: [PATCH 4/4] ALSA: hdac: Add codec read/write and check power state for widgets

[Takashi Iwai]

On Mon, 05 Oct 2015 16:09:51 +0200,
Vinod Koul wrote:
> 
> From: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
> 
> This adds helpers to read/write the codec. Also adds a helper to check the
> power state of widgets.
> 
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  include/sound/hdaudio.h |  6 ++++++
>  sound/hda/hdac_device.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 49bc836fcd84..26e956f4b7c6 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -147,6 +147,12 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid,
>  bool snd_hdac_is_supported_format(struct hdac_device *codec, hda_nid_t nid,
>  				  unsigned int format);
>  
> +int snd_hdac_codec_read(struct hdac_device *hdac, hda_nid_t nid,
> +			int flags, unsigned int verb, unsigned int parm);
> +int snd_hdac_codec_write(struct hdac_device *hdac, hda_nid_t nid,
> +			int flags, unsigned int verb, unsigned int parm);
> +bool snd_hdac_check_power_state(struct hdac_device *hdac,
> +		hda_nid_t nid, unsigned int target_state);
>  /**
>   * snd_hdac_read_parm - read a codec parameter
>   * @codec: the codec object
> diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
> index db96042a497f..24c7a5f6f0f3 100644
> --- a/sound/hda/hdac_device.c
> +++ b/sound/hda/hdac_device.c
> @@ -952,3 +952,50 @@ bool snd_hdac_is_supported_format(struct hdac_device *codec, hda_nid_t nid,
>  	return true;
>  }
>  EXPORT_SYMBOL_GPL(snd_hdac_is_supported_format);
> +
> +static unsigned int codec_read(struct hdac_device *hdac, hda_nid_t nid,
> +			int flags, unsigned int verb, unsigned int parm)
> +{
> +	unsigned int cmd = snd_hdac_make_cmd(hdac, nid, verb, parm);
> +	unsigned int res;
> +
> +	if (snd_hdac_exec_verb(hdac, cmd, flags, &res))
> +		return -1;
> +
> +	return res;
> +}
> +
> +static int codec_write(struct hdac_device *hdac, hda_nid_t nid,
> +			int flags, unsigned int verb, unsigned int parm)
> +{
> +	unsigned int cmd = snd_hdac_make_cmd(hdac, nid, verb, parm);
> +
> +	return snd_hdac_exec_verb(hdac, cmd, flags, NULL);
> +}
> +
> +int snd_hdac_codec_read(struct hdac_device *hdac, hda_nid_t nid,
> +			int flags, unsigned int verb, unsigned int parm)
> +{
> +	return codec_read(hdac, nid, flags, verb, parm);
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_codec_read);

For *every* exported function, you must provide a proper
documentation.  No excuse, as this is even a part of API.

And, you copied these things from sound/pci/hda/, so you should
mention it, and prepare a cleanup patch to use this new one.
Otherwise no one can see a clear merit of this addition.


thanks,

Takashi

> +
> +int snd_hdac_codec_write(struct hdac_device *hdac, hda_nid_t nid,
> +			int flags, unsigned int verb, unsigned int parm)
> +{
> +	return codec_write(hdac, nid, flags, verb, parm);
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_codec_write);
> +
> +bool snd_hdac_check_power_state(struct hdac_device *hdac,
> +		hda_nid_t nid, unsigned int target_state)
> +{
> +	unsigned int state = codec_read(hdac, nid, 0,
> +				AC_VERB_GET_POWER_STATE, 0);
> +
> +	if (state & AC_PWRST_ERROR)
> +		return true;
> +	state = (state >> 4) & 0x0f;
> +	return (state == target_state);
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_check_power_state);
> -- 
> 2.4.3
> 

Re: [PATCH 4/4] ALSA: hdac: Add codec read/write and check power state for widgets

[Koul, Vinod]

On Mon, 2015-10-05 at 17:18 +0200, Takashi Iwai wrote:
> On Mon, 05 Oct 2015 16:09:51 +0200,
> Vinod Koul wrote:
> > 
> > From: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
> > 
> > This adds helpers to read/write the codec. Also adds a helper to check the
> > power state of widgets.
> > 
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> >  include/sound/hdaudio.h |  6 ++++++
> >  sound/hda/hdac_device.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 53 insertions(+)
> > 
> > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> > index 49bc836fcd84..26e956f4b7c6 100644
> > --- a/include/sound/hdaudio.h
> > +++ b/include/sound/hdaudio.h
> > @@ -147,6 +147,12 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec,
> > hda_nid_t nid,
> >  bool snd_hdac_is_supported_format(struct hdac_device *codec, hda_nid_t nid,
> >  				  unsigned int format);
> >  
> > +int snd_hdac_codec_read(struct hdac_device *hdac, hda_nid_t nid,
> > +			int flags, unsigned int verb, unsigned int parm);
> > +int snd_hdac_codec_write(struct hdac_device *hdac, hda_nid_t nid,
> > +			int flags, unsigned int verb, unsigned int parm);
> > +bool snd_hdac_check_power_state(struct hdac_device *hdac,
> > +		hda_nid_t nid, unsigned int target_state);
> >  /**
> >   * snd_hdac_read_parm - read a codec parameter
> >   * @codec: the codec object
> > diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
> > index db96042a497f..24c7a5f6f0f3 100644
> > --- a/sound/hda/hdac_device.c
> > +++ b/sound/hda/hdac_device.c
> > @@ -952,3 +952,50 @@ bool snd_hdac_is_supported_format(struct hdac_device *codec,
> > hda_nid_t nid,
> >  	return true;
> >  }
> >  EXPORT_SYMBOL_GPL(snd_hdac_is_supported_format);
> > +
> > +static unsigned int codec_read(struct hdac_device *hdac, hda_nid_t nid,
> > +			int flags, unsigned int verb, unsigned int parm)
> > +{
> > +	unsigned int cmd = snd_hdac_make_cmd(hdac, nid, verb, parm);
> > +	unsigned int res;
> > +
> > +	if (snd_hdac_exec_verb(hdac, cmd, flags, &res))
> > +		return -1;
> > +
> > +	return res;
> > +}
> > +
> > +static int codec_write(struct hdac_device *hdac, hda_nid_t nid,
> > +			int flags, unsigned int verb, unsigned int parm)
> > +{
> > +	unsigned int cmd = snd_hdac_make_cmd(hdac, nid, verb, parm);
> > +
> > +	return snd_hdac_exec_verb(hdac, cmd, flags, NULL);
> > +}
> > +
> > +int snd_hdac_codec_read(struct hdac_device *hdac, hda_nid_t nid,
> > +			int flags, unsigned int verb, unsigned int parm)
> > +{
> > +	return codec_read(hdac, nid, flags, verb, parm);
> > +}
> > +EXPORT_SYMBOL_GPL(snd_hdac_codec_read);
> 
> For *every* exported function, you must provide a proper
> documentation.  No excuse, as this is even a part of API.

Sure will add that

> And, you copied these things from sound/pci/hda/, so you should
> mention it, and prepare a cleanup patch to use this new one.
> Otherwise no one can see a clear merit of this addition.
Sure, will mention that.
I didn't want to move existing ones without checking with you.
Will start moving them as well

-- 
~Vinod

Re: [PATCH 4/4] ALSA: hdac: Add codec read/write and check power state for widgets

[Takashi Iwai]

On Mon, 05 Oct 2015 17:20:11 +0200,
Koul, Vinod wrote:
> 
> On Mon, 2015-10-05 at 17:18 +0200, Takashi Iwai wrote:
> > On Mon, 05 Oct 2015 16:09:51 +0200,
> > Vinod Koul wrote:
> > > 
> > > From: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
> > > 
> > > This adds helpers to read/write the codec. Also adds a helper to check the
> > > power state of widgets.
> > > 
> > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > ---
> > >  include/sound/hdaudio.h |  6 ++++++
> > >  sound/hda/hdac_device.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 53 insertions(+)
> > > 
> > > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> > > index 49bc836fcd84..26e956f4b7c6 100644
> > > --- a/include/sound/hdaudio.h
> > > +++ b/include/sound/hdaudio.h
> > > @@ -147,6 +147,12 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec,
> > > hda_nid_t nid,
> > >  bool snd_hdac_is_supported_format(struct hdac_device *codec, hda_nid_t nid,
> > >  				  unsigned int format);
> > >  
> > > +int snd_hdac_codec_read(struct hdac_device *hdac, hda_nid_t nid,
> > > +			int flags, unsigned int verb, unsigned int parm);
> > > +int snd_hdac_codec_write(struct hdac_device *hdac, hda_nid_t nid,
> > > +			int flags, unsigned int verb, unsigned int parm);
> > > +bool snd_hdac_check_power_state(struct hdac_device *hdac,
> > > +		hda_nid_t nid, unsigned int target_state);
> > >  /**
> > >   * snd_hdac_read_parm - read a codec parameter
> > >   * @codec: the codec object
> > > diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
> > > index db96042a497f..24c7a5f6f0f3 100644
> > > --- a/sound/hda/hdac_device.c
> > > +++ b/sound/hda/hdac_device.c
> > > @@ -952,3 +952,50 @@ bool snd_hdac_is_supported_format(struct hdac_device *codec,
> > > hda_nid_t nid,
> > >  	return true;
> > >  }
> > >  EXPORT_SYMBOL_GPL(snd_hdac_is_supported_format);
> > > +
> > > +static unsigned int codec_read(struct hdac_device *hdac, hda_nid_t nid,
> > > +			int flags, unsigned int verb, unsigned int parm)
> > > +{
> > > +	unsigned int cmd = snd_hdac_make_cmd(hdac, nid, verb, parm);
> > > +	unsigned int res;
> > > +
> > > +	if (snd_hdac_exec_verb(hdac, cmd, flags, &res))
> > > +		return -1;
> > > +
> > > +	return res;
> > > +}
> > > +
> > > +static int codec_write(struct hdac_device *hdac, hda_nid_t nid,
> > > +			int flags, unsigned int verb, unsigned int parm)
> > > +{
> > > +	unsigned int cmd = snd_hdac_make_cmd(hdac, nid, verb, parm);
> > > +
> > > +	return snd_hdac_exec_verb(hdac, cmd, flags, NULL);
> > > +}
> > > +
> > > +int snd_hdac_codec_read(struct hdac_device *hdac, hda_nid_t nid,
> > > +			int flags, unsigned int verb, unsigned int parm)
> > > +{
> > > +	return codec_read(hdac, nid, flags, verb, parm);
> > > +}
> > > +EXPORT_SYMBOL_GPL(snd_hdac_codec_read);
> > 
> > For *every* exported function, you must provide a proper
> > documentation.  No excuse, as this is even a part of API.
> 
> Sure will add that
> 
> > And, you copied these things from sound/pci/hda/, so you should
> > mention it, and prepare a cleanup patch to use this new one.
> > Otherwise no one can see a clear merit of this addition.
> Sure, will mention that.
> I didn't want to move existing ones without checking with you.
> Will start moving them as well

This is no problem for me, a code reduction is rather always welcome.
Maybe just aliasing in hda_codec.c or hda_local.h would be enough,
something like:

static inline int snd_hda_codec_read(....)
{
	return snd_hdac_codec_read(&codec->core, .....);
}


Takashi

Re: [PATCH 0/4] ALSA: hdac: fixes and support for hda codecs

[Takashi Iwai]

On Mon, 05 Oct 2015 16:09:47 +0200,
Vinod Koul wrote:
> 
> We had two fixes found on testing. First one checks if stream is in use or
> not before releasing and second fixs wrong stread id map
> 
> Then we add structure defination for hdac_ext_dma_params and lastly add
> helpers for read/write for codecs
> 
> Jeeja KP (1):
>   ALSA: hdac: Fix to check if stream not in use in release
> 
> Subhransu S. Prusty (3):
>   ALSA: hdac: Fix incorrect update of stream id mapping
>   ALSA: hdac: structure definition for ext_dma_params
>   ALSA: hdac: Add codec read/write and check power state for widgets

I applied first two patches now as they are clear fixes.

The patch 3 is postponed, since I'd like to see together with its
consumers, so please include in the next patchset.
The patch 4 is rejected as already mentioned.


thanks,

Takashi

> 
>  include/sound/hdaudio.h         |  6 ++++++
>  include/sound/hdaudio_ext.h     |  6 ++++++
>  sound/hda/ext/hdac_ext_stream.c |  9 ++++----
>  sound/hda/hdac_device.c         | 47 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 63 insertions(+), 5 deletions(-)
> 
> -- 
> 2.4.3
> 
> 

Re: [PATCH 0/4] ALSA: hdac: fixes and support for hda codecs

[Koul, Vinod]

On Mon, 2015-10-05 at 17:21 +0200, Takashi Iwai wrote:
> The patch 3 is postponed, since I'd like to see together with its
> consumers, so please include in the next patchset.

Okay I can send that with driver update later on.

-- 
~Vinod