[alsa-devel] [PATCH 0/4] ALSA: hdac: fixes and support for hda codecs
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(-)
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);
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;
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)) /*
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);
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
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
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
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
participants (3)
-
Koul, Vinod -
Takashi Iwai -
Vinod Koul