[Sound-open-firmware] [PATCH 4/5] host: rename host_pos_read to host_pos_local and update comments

[Jie, Yang]

> -----Original Message-----
> From: Liam Girdwood [mailto:liam.r.girdwood at linux.intel.com]
> Sent: Wednesday, February 15, 2017 6:49 PM
> To: Keyon Jie <yang.jie at linux.intel.com>
> Cc: sound-open-firmware at alsa-project.org; Zhang, Keqiao
> <keqiao.zhang at intel.com>; Jie, Yang <yang.jie at intel.com>; Ingalsuo, Seppo
> <seppo.ingalsuo at intel.com>
> Subject: Re: [Sound-open-firmware] [PATCH 4/5] host: rename host_pos_read
> to host_pos_local and update comments
> 
> On Wed, 2017-02-15 at 18:07 +0800, Keyon Jie wrote:
> > We are using host_pos_read not only for playback, for capture, we use
> > it as the host side buffer write pointer, so, here rename it to
> > host_pos_local and add comments to explain that.
> >
> > Signed-off-by: Keyon Jie <yang.jie at linux.intel.com>
> > ---
> >  src/audio/host.c | 48
> > ++++++++++++++++++++++++------------------------
> >  1 file changed, 24 insertions(+), 24 deletions(-)
> >
> > diff --git a/src/audio/host.c b/src/audio/host.c index
> > 61d2b62..f814603 100644
> > --- a/src/audio/host.c
> > +++ b/src/audio/host.c
> > @@ -71,11 +71,11 @@ struct host_data {
> >  	struct hc_buf host;
> >  	struct hc_buf local;
> >  	uint32_t host_size;
> > -	volatile uint32_t *host_pos;    /* read pos, update to mailbox for host
> side */
> > -	uint32_t host_pos_read;         /* host buffer read pos in bytes */
> > -	uint32_t host_period_bytes;	/* host period size in bytes */
> > -	uint32_t host_period_pos;	/* position in current host perid */
> > -	uint32_t host_app_pos;        /* host buffer app write pos, points to
> mailbox */
> > +	volatile uint32_t *host_pos;    /* read/write pos, update to mailbox for
> host side */
> > +	uint32_t host_pos_local;         /* the real host buffer local read/write
> pos in bytes */
> > +	uint32_t host_period_bytes;	/* host period size(host_pos update
> period) in bytes */
> > +	uint32_t host_period_pos;	/* position in current host period */
> > +	uint32_t host_app_pos;        /* host buffer app write/read pos, udpate
> via IPC */
> 
> Why dont we just rename to use r_ptr and w_ptr naming, that makes it very
> simple to understand. This way the copy() always updates read and write ptrs
> without worrying about naming.

Um, this means we may need more codes to judge the stream direction, e.g.
if(stream->direction == playback)
	*host_pos = r_ptr;
else
	*host_pos = w_ptr;

but let me think of that further and try to figure out the modification, to see
if it can make life easier really.

Thanks,
~Keyon

> 
> >  	uint32_t host_avail;   /* host buffer available size */
> >  	uint32_t host_free;    /* host buffer free size */
> >  	/* pointers set during params to host or local above */ @@ -91,24
> > +91,24 @@ struct host_data {
> >
> >  static inline void host_update_buffer_produce(struct host_data *hd)
> > {
> > -	if (hd->host_pos_read < hd->host_app_pos)
> > -		hd->host_avail = hd->host_app_pos - hd->host_pos_read;
> > -	else if (hd->host_pos_read == hd->host_app_pos)
> > +	if (hd->host_pos_local < hd->host_app_pos)
> > +		hd->host_avail = hd->host_app_pos - hd->host_pos_local;
> > +	else if (hd->host_pos_local == hd->host_app_pos)
> >  		hd->host_avail = hd->host_size; /* full */
> >  	else
> > -		hd->host_avail = hd->host_size -hd->host_pos_read +
> > +		hd->host_avail = hd->host_size -hd->host_pos_local +
> >  			hd->host_app_pos;
> >  	hd->host_free = hd->host_size - hd->host_avail;  }
> >
> >  static inline void host_update_buffer_consume(struct host_data *hd)
> > {
> > -	if (hd->host_pos_read < hd->host_app_pos)
> > -		hd->host_avail = hd->host_app_pos - hd->host_pos_read;
> > -	else if (hd->host_pos_read == hd->host_app_pos)
> > +	if (hd->host_pos_local < hd->host_app_pos)
> > +		hd->host_avail = hd->host_app_pos - hd->host_pos_local;
> > +	else if (hd->host_pos_local == hd->host_app_pos)
> >  		hd->host_avail = 0; /* empty */
> >  	else
> > -		hd->host_avail = hd->host_size -hd->host_pos_read +
> > +		hd->host_avail = hd->host_size -hd->host_pos_local +
> >  			hd->host_app_pos;
> >  	hd->host_free = hd->host_size - hd->host_avail;  } @@ -163,11
> > +163,11 @@ static void host_dma_cb_playback(struct comp_dev *dev,
> >  	comp_update_buffer_produce(hd->dma_buffer);
> >
> >  	/* new local period, update host buffer position blks */
> > -	hd->host_pos_read += local_elem->size;
> > +	hd->host_pos_local += local_elem->size;
> >
> >  	/* buffer overlap ? */
> > -	if (hd->host_pos_read >= hd->host_size)
> > -		hd->host_pos_read = 0;
> > +	if (hd->host_pos_local >= hd->host_size)
> > +		hd->host_pos_local = 0;
> >  	host_update_buffer_consume(hd);
> >
> >  	/* send IPC message to driver if needed */ @@ -178,7 +178,7 @@
> > static void host_dma_cb_playback(struct comp_dev *dev,
> >  		if (hd->host_avail) {
> >  			/* update for host side */
> >  			if (hd->host_pos) {
> > -				*hd->host_pos = hd->host_pos_read;
> > +				*hd->host_pos = hd->host_pos_local;
> >  				ipc_stream_send_notification(dev, &hd->cp);
> >  			}
> >  		}
> > @@ -281,13 +281,13 @@ static void host_dma_cb_capture(struct comp_dev
> > *dev,  #endif
> >
> >  	/* new local period, update host buffer position blks */
> > -	hd->host_pos_read += local_elem->size;
> > +	hd->host_pos_local += local_elem->size;
> >
> >  	/* buffer overlap ? */
> > -	if (hd->host_pos_read >= hd->host_size)
> > -		hd->host_pos_read = 0;
> > +	if (hd->host_pos_local >= hd->host_size)
> > +		hd->host_pos_local = 0;
> >  	if (hd->host_pos)
> > -		*hd->host_pos = hd->host_pos_read;
> > +		*hd->host_pos = hd->host_pos_local;
> >
> >  	/* recalc available buffer space */
> >  	comp_update_buffer_consume(hd->dma_buffer);
> > @@ -381,7 +381,7 @@ static uint32_t host_finish_work(void *data, uint32_t
> udelay)
> >  		trace_comp("hFw");
> >  		/* update for host side */
> >  		if (hd->host_pos) {
> > -			*hd->host_pos = hd->host_pos_read;
> > +			*hd->host_pos = hd->host_pos_local;
> >  			/* send the last notification to host */
> >  			ipc_stream_send_notification(dev, &hd->cp);
> >  		}
> > @@ -632,7 +632,7 @@ static int host_prepare(struct comp_dev *dev)
> >
> >  	if (hd->host_pos)
> >  		*hd->host_pos = 0;
> > -	hd->host_pos_read = 0;
> > +	hd->host_pos_local = 0;
> >  	hd->host_period_pos = 0;
> >  	hd->host_period_bytes =
> >  		hd->params.period_frames * hd->params.frame_size; @@ -
> 674,7 +674,7
> > @@ static int host_pointer_reset(struct comp_dev *dev)
> >  	if (hd->host_pos)
> >  		*hd->host_pos = 0;
> >  	hd->host_app_pos = 0;
> > -	hd->host_pos_read = 0;
> > +	hd->host_pos_local = 0;
> >  	hd->host_period_pos = 0;
> >
> >  	host_update_buffer_consume(hd);
>