Re: [PATCH 4/5] ALSA: hdac: Add support to enable SPIB for hdac ext stream

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 8/21/15 11:06 AM, Vinod Koul wrote:
> From: Jeeja KP <jeeja.kp@intel.com>
>
> The drivers need to set the spib and maxfifios values, so add
> these new APIs snd_hdac_ext_stream_set_spib() and
> snd_hdac_ext_stream_set_spbmaxfifo() APIs
>
> For these APIs we also need to have spib and fifos pointer, so
> add these to hdac_ext_stream and initialize them at stream init
>
> Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>   include/sound/hda_register.h    |  4 +++
>   include/sound/hdaudio_ext.h     | 10 ++++++++
>   sound/hda/ext/hdac_ext_stream.c | 54 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 68 insertions(+)
>
> diff --git a/include/sound/hda_register.h b/include/sound/hda_register.h
> index ae995e523ff8..2ae8812d7b1a 100644
> --- a/include/sound/hda_register.h
> +++ b/include/sound/hda_register.h
> @@ -160,6 +160,10 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 };
>   #define AZX_SPB_BASE			0x08
>   /* Interval used to calculate the iterating register offset */
>   #define AZX_SPB_INTERVAL		0x08
> +/* SPIB base */
> +#define AZX_SPB_SPIB			0x00
> +/* SPIB MAXFIFO base*/
> +#define AZX_SPB_MAXFIFO			0x04
>
>   /* registers of Global Time Synchronization Capability Structure */
>   #define AZX_GTS_CAP_ID			0x1
> diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
> index 160160d9bffc..9385c99a6504 100644
> --- a/include/sound/hdaudio_ext.h
> +++ b/include/sound/hdaudio_ext.h
> @@ -63,6 +63,8 @@ enum hdac_ext_stream_type {
>    * @hstream: hdac_stream
>    * @pphc_addr: processing pipe host stream pointer
>    * @pplc_addr: processing pipe link stream pointer
> + * @spib_addr: software position in buffers stream pointer
> + * @fifo_addr: software position Max fifos stream pointer
>    * @decoupled: stream host and link is decoupled
>    * @link_locked: link is locked
>    * @link_prepared: link is prepared
> @@ -74,6 +76,9 @@ struct hdac_ext_stream {
>   	void __iomem *pphc_addr;
>   	void __iomem *pplc_addr;
>
> +	void __iomem *spib_addr;
> +	void __iomem *fifo_addr;
> +
>   	bool decoupled:1;
>   	bool link_locked:1;
>   	bool link_prepared;
> @@ -100,6 +105,11 @@ void snd_hdac_ext_stream_decouple(struct hdac_ext_bus *bus,
>   				struct hdac_ext_stream *azx_dev, bool decouple);
>   void snd_hdac_ext_stop_streams(struct hdac_ext_bus *sbus);
>
> +int snd_hdac_ext_stream_set_spib(struct hdac_ext_bus *ebus,
> +				 struct hdac_ext_stream *stream, u32 value);
> +int snd_hdac_ext_stream_set_spbmaxfifo(struct hdac_ext_bus *ebus,
> +				 struct hdac_ext_stream *stream);
> +
>   void snd_hdac_ext_link_stream_start(struct hdac_ext_stream *hstream);
>   void snd_hdac_ext_link_stream_clear(struct hdac_ext_stream *hstream);
>   void snd_hdac_ext_link_stream_reset(struct hdac_ext_stream *hstream);
> diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c
> index b649625f43a4..a4f6bbe5da8f 100644
> --- a/sound/hda/ext/hdac_ext_stream.c
> +++ b/sound/hda/ext/hdac_ext_stream.c
> @@ -49,6 +49,16 @@ void snd_hdac_ext_stream_init(struct hdac_ext_bus *ebus,
>   				AZX_PPLC_INTERVAL * idx;
>   	}
>
> +	if (ebus->spbcap) {
> +		stream->spib_addr = ebus->spbcap + AZX_SPB_BASE +
> +					AZX_SPB_INTERVAL * idx +
> +					AZX_SPB_SPIB;
> +
> +		stream->fifo_addr = ebus->spbcap + AZX_SPB_BASE +
> +					AZX_SPB_INTERVAL * idx +
> +					AZX_SPB_MAXFIFO;
> +	}
> +
>   	stream->decoupled = false;
>   	snd_hdac_stream_init(bus, &stream->hstream, idx, direction, tag);
>   }
> @@ -435,6 +445,50 @@ void snd_hdac_ext_stream_spbcap_enable(struct hdac_ext_bus *ebus,
>   EXPORT_SYMBOL_GPL(snd_hdac_ext_stream_spbcap_enable);
>
>   /**
> + * snd_hdac_ext_stream_set_spib - sets the spib value of a stream
> + * @ebus: HD-audio ext core bus
> + * @stream: hdac_ext_stream
> + * @value: spib value to set
> + */
> +int snd_hdac_ext_stream_set_spib(struct hdac_ext_bus *ebus,
> +				 struct hdac_ext_stream *stream, u32 value)
> +{
> +	struct hdac_bus *bus = &ebus->bus;
> +
> +	if (!ebus->spbcap) {
> +		dev_err(bus->dev, "Address of SPB capability is NULL");
> +		return -EINVAL;
> +	}
> +
> +	writel(value, stream->spib_addr);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_ext_stream_set_spib);
> +
> +/**
> + * snd_hdac_ext_stream_set_spbmaxfifo - sets the spib value of a stream

copy-paste issue, wrong comment

> + * @ebus: HD-audio ext core bus
> + * @stream: hdac_ext_stream
> + *
> + * Return maxfifo for the stream
> + */
> +int snd_hdac_ext_stream_set_spbmaxfifo(struct hdac_ext_bus *ebus,
> +				 struct hdac_ext_stream *stream)
> +{
> +	struct hdac_bus *bus = &ebus->bus;
> +
> +	if (!ebus->spbcap) {
> +		dev_err(bus->dev, "Address of SPB capability is NULL");
> +		return -EINVAL;
> +	}
> +
> +	return readl(stream->fifo_addr);
> +}

weird function name. a set_spbmaxfifo that reads a value? Shouldn't it 
be a get?

> +EXPORT_SYMBOL_GPL(snd_hdac_ext_stream_set_spbmaxfifo);
> +
> +
> +/**
>    * snd_hdac_ext_stop_streams - stop all stream if running
>    * @ebus: HD-audio ext core bus
>    */
>