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

[Takashi Iwai <tiwai@suse.de>]

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