| Message ID | 1444054191-17030-5-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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);