[PATCH 00/29] staging: bcm2835-audio: Cleanups and fixes

[PATCH 00/29] staging: bcm2835-audio: Cleanups and fixes

[Takashi Iwai]

Hi,

since I had an opportunity to play with RPi3B+ recently, I took a look
at the existing bcm2835-audio driver code and was amused very much :)

So here is the result, a cleanup and fix patch series.

Most of the patches are trivial cleanups, just brushing up, removing
many redundant and buggy codes, as well as code simplifications.

A big functional change is that now it uses non-atomic PCM ops, so
that we can kill the ugly workqueue usages.  Also, the resource
management was simplified.

As a result, we can get rid of ca 1000 lines.  Not too bad.


Takashi

===

Takashi Iwai (29):
  staging: bcm2835-audio: Clean up mutex locks
  staging: bcm2835-audio: Remove redundant spdif stream ctls
  staging: bcm2835-audio: Clean up include files in bcm2835-ctl.c
  staging: bcm2835-audio: Remove redundant substream mask checks
  staging: bcm2835-audio: Fix mute controls, volume handling cleanup
  staging: bcm2835-audio: Remove redundant function calls
  staging: bcm2835-audio: Remove superfluous open flag
  staging: bcm2835-audio: Drop useless running flag and check
  staging: bcm2835-audio: Fix incorrect draining handling
  staging: bcm2835-audio: Kill unused spinlock
  staging: bcm2835-audio: Use PCM runtime values instead
  staging: bcm2835-audio: Drop unnecessary pcm indirect setup
  staging: bcm2835-audio: Drop useless NULL check
  staging: bcm2835-audio: Propagate parameter setup error
  staging: bcm2835-audio: Drop debug messages in bcm2835-pcm.c
  staging: bcm2835-audio: Drop superfluous mutex lock during prepare
  staging: bcm2835-audio: Add 10ms period constraint
  staging: bcm2835-audio: Make single vchi handle
  staging: bcm2835-audio: Code refactoring of vchiq accessor codes
  staging: bcm2835-audio: Operate non-atomic PCM ops
  staging: bcm2835-audio: Use card->private_data
  staging: bcm2835-audio: Use standard error print helpers
  staging: bcm2835-audio: Remove unnecessary header file includes
  staging: bcm2835-audio: Move module parameter description
  staging: bcm2835-audio: Use coherent device buffers
  staging: bcm2835-audio: Set SNDRV_PCM_INFO_SYNC_APPLPTR
  staging: bcm2835-audio: Simplify PCM creation helpers
  staging: bcm2835-audio: Simplify kctl creation helpers
  staging: bcm2835-audio: Simplify card object management

 .../vc04_services/bcm2835-audio/bcm2835-ctl.c | 235 ++---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 338 ++-----
 .../bcm2835-audio/bcm2835-vchiq.c             | 893 +++++-------------
 .../vc04_services/bcm2835-audio/bcm2835.c     | 222 ++---
 .../vc04_services/bcm2835-audio/bcm2835.h     |  85 +-
 5 files changed, 413 insertions(+), 1360 deletions(-)

-- 
2.18.0

[PATCH 01/29] staging: bcm2835-audio: Clean up mutex locks

[Takashi Iwai]

snd-bcm2835 driver takes the lock with mutex_lock_interruptible() in
all places, which don't make sense.  Replace them with the simple
mutex_lock().

Also taking a mutex lock right after creating it for each PCM object
is nonsense, too.  It cannot be racy at that point.  We can get rid of
it.

Last but not least, initializing chip->audio_mutex at each place is
error-prone.  Initialize properly at creating the chip object in
snd_bcm2835_create() instead.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-ctl.c | 18 +++----
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 33 ++-----------
 .../bcm2835-audio/bcm2835-vchiq.c             | 47 ++++---------------
 .../vc04_services/bcm2835-audio/bcm2835.c     |  1 +
 4 files changed, 20 insertions(+), 79 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
index ec468d5719b1..04ea3ec7f64f 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
@@ -77,8 +77,7 @@ static int snd_bcm2835_ctl_get(struct snd_kcontrol *kcontrol,
 {
 	struct bcm2835_chip *chip = snd_kcontrol_chip(kcontrol);
 
-	if (mutex_lock_interruptible(&chip->audio_mutex))
-		return -EINTR;
+	mutex_lock(&chip->audio_mutex);
 
 	BUG_ON(!chip && !(chip->avail_substreams & AVAIL_SUBSTREAMS_MASK));
 
@@ -99,8 +98,7 @@ static int snd_bcm2835_ctl_put(struct snd_kcontrol *kcontrol,
 	struct bcm2835_chip *chip = snd_kcontrol_chip(kcontrol);
 	int changed = 0;
 
-	if (mutex_lock_interruptible(&chip->audio_mutex))
-		return -EINTR;
+	mutex_lock(&chip->audio_mutex);
 
 	if (kcontrol->private_value == PCM_PLAYBACK_VOLUME) {
 		audio_info("Volume change attempted.. volume = %d new_volume = %d\n", chip->volume, (int)ucontrol->value.integer.value[0]);
@@ -187,8 +185,7 @@ static int snd_bcm2835_spdif_default_get(struct snd_kcontrol *kcontrol,
 	struct bcm2835_chip *chip = snd_kcontrol_chip(kcontrol);
 	int i;
 
-	if (mutex_lock_interruptible(&chip->audio_mutex))
-		return -EINTR;
+	mutex_lock(&chip->audio_mutex);
 
 	for (i = 0; i < 4; i++)
 		ucontrol->value.iec958.status[i] =
@@ -205,8 +202,7 @@ static int snd_bcm2835_spdif_default_put(struct snd_kcontrol *kcontrol,
 	unsigned int val = 0;
 	int i, change;
 
-	if (mutex_lock_interruptible(&chip->audio_mutex))
-		return -EINTR;
+	mutex_lock(&chip->audio_mutex);
 
 	for (i = 0; i < 4; i++)
 		val |= (unsigned int)ucontrol->value.iec958.status[i] << (i * 8);
@@ -251,8 +247,7 @@ static int snd_bcm2835_spdif_stream_get(struct snd_kcontrol *kcontrol,
 	struct bcm2835_chip *chip = snd_kcontrol_chip(kcontrol);
 	int i;
 
-	if (mutex_lock_interruptible(&chip->audio_mutex))
-		return -EINTR;
+	mutex_lock(&chip->audio_mutex);
 
 	for (i = 0; i < 4; i++)
 		ucontrol->value.iec958.status[i] =
@@ -269,8 +264,7 @@ static int snd_bcm2835_spdif_stream_put(struct snd_kcontrol *kcontrol,
 	unsigned int val = 0;
 	int i, change;
 
-	if (mutex_lock_interruptible(&chip->audio_mutex))
-		return -EINTR;
+	mutex_lock(&chip->audio_mutex);
 
 	for (i = 0; i < 4; i++)
 		val |= (unsigned int)ucontrol->value.iec958.status[i] << (i * 8);
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 8359cf881bef..f2d8b17d0cfe 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -99,10 +99,7 @@ static int snd_bcm2835_playback_open_generic(
 	int idx;
 	int err;
 
-	if (mutex_lock_interruptible(&chip->audio_mutex)) {
-		audio_error("Interrupted whilst waiting for lock\n");
-		return -EINTR;
-	}
+	mutex_lock(&chip->audio_mutex);
 	audio_info("Alsa open (%d)\n", substream->number);
 	idx = substream->number;
 
@@ -194,10 +191,7 @@ static int snd_bcm2835_playback_close(struct snd_pcm_substream *substream)
 	struct bcm2835_alsa_stream *alsa_stream;
 
 	chip = snd_pcm_substream_chip(substream);
-	if (mutex_lock_interruptible(&chip->audio_mutex)) {
-		audio_error("Interrupted whilst waiting for lock\n");
-		return -EINTR;
-	}
+	mutex_lock(&chip->audio_mutex);
 	runtime = substream->runtime;
 	alsa_stream = runtime->private_data;
 
@@ -274,8 +268,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 	int channels;
 	int err;
 
-	if (mutex_lock_interruptible(&chip->audio_mutex))
-		return -EINTR;
+	mutex_lock(&chip->audio_mutex);
 
 	/* notify the vchiq that it should enter spdif passthrough mode by
 	 * setting channels=0 (see
@@ -449,14 +442,9 @@ int snd_bcm2835_new_pcm(struct bcm2835_chip *chip, u32 numchannels)
 	struct snd_pcm *pcm;
 	int err;
 
-	mutex_init(&chip->audio_mutex);
-	if (mutex_lock_interruptible(&chip->audio_mutex)) {
-		audio_error("Interrupted whilst waiting for lock\n");
-		return -EINTR;
-	}
 	err = snd_pcm_new(chip->card, "bcm2835 ALSA", 0, numchannels, 0, &pcm);
 	if (err < 0)
-		goto out;
+		return err;
 	pcm->private_data = chip;
 	strcpy(pcm->name, "bcm2835 ALSA");
 	chip->pcm = pcm;
@@ -474,9 +462,6 @@ int snd_bcm2835_new_pcm(struct bcm2835_chip *chip, u32 numchannels)
 					      snd_bcm2835_playback_hw.buffer_bytes_max,
 					      snd_bcm2835_playback_hw.buffer_bytes_max);
 
-out:
-	mutex_unlock(&chip->audio_mutex);
-
 	return 0;
 }
 
@@ -485,13 +470,9 @@ int snd_bcm2835_new_spdif_pcm(struct bcm2835_chip *chip)
 	struct snd_pcm *pcm;
 	int err;
 
-	if (mutex_lock_interruptible(&chip->audio_mutex)) {
-		audio_error("Interrupted whilst waiting for lock\n");
-		return -EINTR;
-	}
 	err = snd_pcm_new(chip->card, "bcm2835 ALSA", 1, 1, 0, &pcm);
 	if (err < 0)
-		goto out;
+		return err;
 
 	pcm->private_data = chip;
 	strcpy(pcm->name, "bcm2835 IEC958/HDMI");
@@ -504,8 +485,6 @@ int snd_bcm2835_new_spdif_pcm(struct bcm2835_chip *chip)
 	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_CONTINUOUS,
 		snd_dma_continuous_data(GFP_KERNEL),
 		snd_bcm2835_playback_spdif_hw.buffer_bytes_max, snd_bcm2835_playback_spdif_hw.buffer_bytes_max);
-out:
-	mutex_unlock(&chip->audio_mutex);
 
 	return 0;
 }
@@ -518,8 +497,6 @@ int snd_bcm2835_new_simple_pcm(struct bcm2835_chip *chip,
 	struct snd_pcm *pcm;
 	int err;
 
-	mutex_init(&chip->audio_mutex);
-
 	err = snd_pcm_new(chip->card, name, 0, numchannels,
 			  0, &pcm);
 	if (err)
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 868e2d6aaf1b..bec361aff4fe 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -319,11 +319,7 @@ static int vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
 	}
 
 	LOG_DBG(" .. about to lock (%d)\n", instance->num_connections);
-	if (mutex_lock_interruptible(&instance->vchi_mutex)) {
-		LOG_DBG("Interrupted whilst waiting for lock on (%d)\n",
-			instance->num_connections);
-		return -EINTR;
-	}
+	mutex_lock(&instance->vchi_mutex);
 
 	/* Close all VCHI service connections */
 	for (i = 0; i < instance->num_connections; i++) {
@@ -434,11 +430,7 @@ int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
 	instance = alsa_stream->instance;
 	LOG_DBG(" instance (%p)\n", instance);
 
-	if (mutex_lock_interruptible(&instance->vchi_mutex)) {
-		LOG_DBG("Interrupted whilst waiting for lock on (%d)\n", instance->num_connections);
-		ret = -EINTR;
-		goto free_wq;
-	}
+	mutex_lock(&instance->vchi_mutex);
 	vchi_service_use(instance->vchi_handle[0]);
 
 	m.type = VC_AUDIO_MSG_TYPE_OPEN;
@@ -479,11 +471,7 @@ static int bcm2835_audio_set_ctls_chan(struct bcm2835_alsa_stream *alsa_stream,
 	LOG_INFO(" Setting ALSA dest(%d), volume(%d)\n",
 		 chip->dest, chip->volume);
 
-	if (mutex_lock_interruptible(&instance->vchi_mutex)) {
-		LOG_DBG("Interrupted whilst waiting for lock on (%d)\n",
-			instance->num_connections);
-		return -EINTR;
-	}
+	mutex_lock(&instance->vchi_mutex);
 	vchi_service_use(instance->vchi_handle[0]);
 
 	instance->result = -1;
@@ -569,10 +557,7 @@ int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 		return -EINVAL;
 	}
 
-	if (mutex_lock_interruptible(&instance->vchi_mutex)) {
-		LOG_DBG("Interrupted whilst waiting for lock on (%d)\n", instance->num_connections);
-		return -EINTR;
-	}
+	mutex_lock(&instance->vchi_mutex);
 	vchi_service_use(instance->vchi_handle[0]);
 
 	instance->result = -1;
@@ -629,11 +614,7 @@ static int bcm2835_audio_start_worker(struct bcm2835_alsa_stream *alsa_stream)
 	int status;
 	int ret;
 
-	if (mutex_lock_interruptible(&instance->vchi_mutex)) {
-		LOG_DBG("Interrupted whilst waiting for lock on (%d)\n",
-			instance->num_connections);
-		return -EINTR;
-	}
+	mutex_lock(&instance->vchi_mutex);
 	vchi_service_use(instance->vchi_handle[0]);
 
 	m.type = VC_AUDIO_MSG_TYPE_START;
@@ -665,11 +646,7 @@ static int bcm2835_audio_stop_worker(struct bcm2835_alsa_stream *alsa_stream)
 	int status;
 	int ret;
 
-	if (mutex_lock_interruptible(&instance->vchi_mutex)) {
-		LOG_DBG("Interrupted whilst waiting for lock on (%d)\n",
-			instance->num_connections);
-		return -EINTR;
-	}
+	mutex_lock(&instance->vchi_mutex);
 	vchi_service_use(instance->vchi_handle[0]);
 
 	m.type = VC_AUDIO_MSG_TYPE_STOP;
@@ -704,11 +681,7 @@ int bcm2835_audio_close(struct bcm2835_alsa_stream *alsa_stream)
 
 	my_workqueue_quit(alsa_stream);
 
-	if (mutex_lock_interruptible(&instance->vchi_mutex)) {
-		LOG_DBG("Interrupted whilst waiting for lock on (%d)\n",
-			instance->num_connections);
-		return -EINTR;
-	}
+	mutex_lock(&instance->vchi_mutex);
 	vchi_service_use(instance->vchi_handle[0]);
 
 	m.type = VC_AUDIO_MSG_TYPE_CLOSE;
@@ -761,11 +734,7 @@ static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
 
 	LOG_INFO(" Writing %d bytes from %p\n", count, src);
 
-	if (mutex_lock_interruptible(&instance->vchi_mutex)) {
-		LOG_DBG("Interrupted whilst waiting for lock on (%d)\n",
-			instance->num_connections);
-		return -EINTR;
-	}
+	mutex_lock(&instance->vchi_mutex);
 	vchi_service_use(instance->vchi_handle[0]);
 
 	if (instance->peer_version == 0 &&
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index da0fa34501fa..fa04f6bc9858 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -149,6 +149,7 @@ static int snd_bcm2835_create(struct snd_card *card,
 		return -ENOMEM;
 
 	chip->card = card;
+	mutex_init(&chip->audio_mutex);
 
 	chip->vchi_ctx = devres_find(card->dev->parent,
 				     bcm2835_devm_free_vchi_ctx, NULL, NULL);
-- 
2.18.0

[PATCH 02/29] staging: bcm2835-audio: Remove redundant spdif stream ctls

[Takashi Iwai]

The "IEC958 Playback Stream" control does basically the very same
thing as "IEC958 Playback Default" redundantly.  The former should
have been stream-specific and restored after closing the stream, but
we don't do in that way.

Since it's nothing but confusion, remove this fake.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-ctl.c | 51 -------------------
 1 file changed, 51 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
index 04ea3ec7f64f..9020887e1ada 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
@@ -233,48 +233,6 @@ static int snd_bcm2835_spdif_mask_get(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
-static int snd_bcm2835_spdif_stream_info(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_info *uinfo)
-{
-	uinfo->type = SNDRV_CTL_ELEM_TYPE_IEC958;
-	uinfo->count = 1;
-	return 0;
-}
-
-static int snd_bcm2835_spdif_stream_get(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
-{
-	struct bcm2835_chip *chip = snd_kcontrol_chip(kcontrol);
-	int i;
-
-	mutex_lock(&chip->audio_mutex);
-
-	for (i = 0; i < 4; i++)
-		ucontrol->value.iec958.status[i] =
-		(chip->spdif_status >> (i * 8)) & 0xff;
-
-	mutex_unlock(&chip->audio_mutex);
-	return 0;
-}
-
-static int snd_bcm2835_spdif_stream_put(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
-{
-	struct bcm2835_chip *chip = snd_kcontrol_chip(kcontrol);
-	unsigned int val = 0;
-	int i, change;
-
-	mutex_lock(&chip->audio_mutex);
-
-	for (i = 0; i < 4; i++)
-		val |= (unsigned int)ucontrol->value.iec958.status[i] << (i * 8);
-	change = val != chip->spdif_status;
-	chip->spdif_status = val;
-
-	mutex_unlock(&chip->audio_mutex);
-	return change;
-}
-
 static struct snd_kcontrol_new snd_bcm2835_spdif[] = {
 	{
 		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
@@ -290,15 +248,6 @@ static struct snd_kcontrol_new snd_bcm2835_spdif[] = {
 		.info = snd_bcm2835_spdif_mask_info,
 		.get = snd_bcm2835_spdif_mask_get,
 	},
-	{
-		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
-		SNDRV_CTL_ELEM_ACCESS_INACTIVE,
-		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
-		.name = SNDRV_CTL_NAME_IEC958("", PLAYBACK, PCM_STREAM),
-		.info = snd_bcm2835_spdif_stream_info,
-		.get = snd_bcm2835_spdif_stream_get,
-		.put = snd_bcm2835_spdif_stream_put,
-	},
 };
 
 int snd_bcm2835_new_ctl(struct bcm2835_chip *chip)
-- 
2.18.0

[PATCH 03/29] staging: bcm2835-audio: Clean up include files in bcm2835-ctl.c

[Takashi Iwai]

Only a few of them are really needed.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-ctl.c     | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
index 9020887e1ada..1c5a87580978 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
@@ -1,23 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright 2011 Broadcom Corporation.  All rights reserved. */
 
-#include <linux/platform_device.h>
-#include <linux/init.h>
-#include <linux/io.h>
-#include <linux/jiffies.h>
-#include <linux/slab.h>
-#include <linux/time.h>
-#include <linux/wait.h>
-#include <linux/delay.h>
-#include <linux/moduleparam.h>
-#include <linux/sched.h>
-
 #include <sound/core.h>
 #include <sound/control.h>
-#include <sound/pcm.h>
-#include <sound/pcm_params.h>
-#include <sound/rawmidi.h>
-#include <sound/initval.h>
 #include <sound/tlv.h>
 #include <sound/asoundef.h>
 
-- 
2.18.0

[PATCH 04/29] staging: bcm2835-audio: Remove redundant substream mask checks

[Takashi Iwai]

The avail_substreams bit mask is checked for the possible racy
accesses, but this cannot happen in practice; i.e. the assignment and
the check are superfluous.

Let's rip them off.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-ctl.c   |  2 --
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c   |  8 --------
 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c | 17 +++++++----------
 .../vc04_services/bcm2835-audio/bcm2835.c       |  5 +----
 .../vc04_services/bcm2835-audio/bcm2835.h       |  2 --
 5 files changed, 8 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
index 1c5a87580978..d2f0f609f737 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
@@ -64,8 +64,6 @@ static int snd_bcm2835_ctl_get(struct snd_kcontrol *kcontrol,
 
 	mutex_lock(&chip->audio_mutex);
 
-	BUG_ON(!chip && !(chip->avail_substreams & AVAIL_SUBSTREAMS_MASK));
-
 	if (kcontrol->private_value == PCM_PLAYBACK_VOLUME)
 		ucontrol->value.integer.value[0] = chip2alsa(chip->volume);
 	else if (kcontrol->private_value == PCM_PLAYBACK_MUTE)
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index f2d8b17d0cfe..0be185350f33 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -118,14 +118,6 @@ static int snd_bcm2835_playback_open_generic(
 		goto out;
 	}
 
-	/* Check if we are ready */
-	if (!(chip->avail_substreams & (1 << idx))) {
-		/* We are not ready yet */
-		audio_error("substream(%d) device is not ready yet\n", idx);
-		err = -EAGAIN;
-		goto out;
-	}
-
 	alsa_stream = kzalloc(sizeof(*alsa_stream), GFP_KERNEL);
 	if (!alsa_stream) {
 		err = -ENOMEM;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index bec361aff4fe..942a38942c29 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -523,16 +523,13 @@ int bcm2835_audio_set_ctls(struct bcm2835_chip *chip)
 
 	/* change ctls for all substreams */
 	for (i = 0; i < MAX_SUBSTREAMS; i++) {
-		if (chip->avail_substreams & (1 << i)) {
-			if (!chip->alsa_stream[i]) {
-				LOG_DBG(" No ALSA stream available?! %i:%p (%x)\n", i, chip->alsa_stream[i], chip->avail_substreams);
-				ret = 0;
-			} else if (bcm2835_audio_set_ctls_chan(chip->alsa_stream[i], chip) != 0) {
-				LOG_ERR("Couldn't set the controls for stream %d\n", i);
-				ret = -1;
-			} else {
-				LOG_DBG(" Controls set for stream %d\n", i);
-			}
+		if (!chip->alsa_stream[i])
+			continue;
+		if (bcm2835_audio_set_ctls_chan(chip->alsa_stream[i], chip) != 0) {
+			LOG_ERR("Couldn't set the controls for stream %d\n", i);
+			ret = -1;
+		} else {
+			LOG_DBG(" Controls set for stream %d\n", i);
 		}
 	}
 	return ret;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index fa04f6bc9858..6876a5eadc07 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -280,7 +280,7 @@ static int snd_add_child_device(struct device *device,
 	struct snd_card *card;
 	struct device *child;
 	struct bcm2835_chip *chip;
-	int err, i;
+	int err;
 
 	child = snd_create_device(device, &audio_driver->driver,
 				  audio_driver->driver.name);
@@ -325,9 +325,6 @@ static int snd_add_child_device(struct device *device,
 		return err;
 	}
 
-	for (i = 0; i < numchans; i++)
-		chip->avail_substreams |= (1 << i);
-
 	err = snd_card_register(card);
 	if (err) {
 		dev_err(child, "Failed to register card, error %d\n", err);
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index 5dc427240a1d..c0e7df4914ed 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -98,8 +98,6 @@ struct bcm2835_chip {
 	struct snd_card *card;
 	struct snd_pcm *pcm;
 	struct snd_pcm *pcm_spdif;
-	/* Bitmat for valid reg_base and irq numbers */
-	unsigned int avail_substreams;
 	struct device *dev;
 	struct bcm2835_alsa_stream *alsa_stream[MAX_SUBSTREAMS];
 
-- 
2.18.0

[PATCH 05/29] staging: bcm2835-audio: Fix mute controls, volume handling cleanup

[Takashi Iwai]

In the current code, the mute control is dealt in a special manner,
modifying the current volume and saving the old volume, etc.  This is
inconsistent (e.g. change the volume while muted, then unmute), and
way too complex.

Also, the whole volume handling code has conversion between ALSA
volume and raw volume values, which can lead to another
inconsistency and complexity.

This patch simplifies these points:
- The ALSA volume value is saved in chip->volume
- volume->mute saves the mute state
- The mute state is evaluated only when the actual volume is passed to
  the hardware, bcm2835_audio_set_ctls()

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-ctl.c | 84 +++++++------------
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c |  6 +-
 .../bcm2835-audio/bcm2835-vchiq.c             | 32 ++-----
 .../vc04_services/bcm2835-audio/bcm2835.h     |  5 +-
 4 files changed, 45 insertions(+), 82 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
index d2f0f609f737..e17b72f21a9d 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
@@ -12,6 +12,21 @@
 #define CTRL_VOL_MAX 400
 #define CTRL_VOL_MIN -10239 /* originally -10240 */
 
+static int bcm2835_audio_set_chip_ctls(struct bcm2835_chip *chip)
+{
+	int i, err = 0;
+
+	/* change ctls for all substreams */
+	for (i = 0; i < MAX_SUBSTREAMS; i++) {
+		if (chip->alsa_stream[i]) {
+			err = bcm2835_audio_set_ctls(chip->alsa_stream[i]);
+			if (err < 0)
+				break;
+		}
+	}
+	return err;
+}
+
 static int snd_bcm2835_ctl_info(struct snd_kcontrol *kcontrol,
 				struct snd_ctl_elem_info *uinfo)
 {
@@ -34,29 +49,6 @@ static int snd_bcm2835_ctl_info(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
-/* toggles mute on or off depending on the value of nmute, and returns
- * 1 if the mute value was changed, otherwise 0
- */
-static int toggle_mute(struct bcm2835_chip *chip, int nmute)
-{
-	/* if settings are ok, just return 0 */
-	if (chip->mute == nmute)
-		return 0;
-
-	/* if the sound is muted then we need to unmute */
-	if (chip->mute == CTRL_VOL_MUTE) {
-		chip->volume = chip->old_volume; /* copy the old volume back */
-		audio_info("Unmuting, old_volume = %d, volume = %d ...\n", chip->old_volume, chip->volume);
-	} else /* otherwise we mute */ {
-		chip->old_volume = chip->volume;
-		chip->volume = 26214; /* set volume to minimum level AKA mute */
-		audio_info("Muting, old_volume = %d, volume = %d ...\n", chip->old_volume, chip->volume);
-	}
-
-	chip->mute = nmute;
-	return 1;
-}
-
 static int snd_bcm2835_ctl_get(struct snd_kcontrol *kcontrol,
 			       struct snd_ctl_elem_value *ucontrol)
 {
@@ -65,7 +57,7 @@ static int snd_bcm2835_ctl_get(struct snd_kcontrol *kcontrol,
 	mutex_lock(&chip->audio_mutex);
 
 	if (kcontrol->private_value == PCM_PLAYBACK_VOLUME)
-		ucontrol->value.integer.value[0] = chip2alsa(chip->volume);
+		ucontrol->value.integer.value[0] = chip->volume;
 	else if (kcontrol->private_value == PCM_PLAYBACK_MUTE)
 		ucontrol->value.integer.value[0] = chip->mute;
 	else if (kcontrol->private_value == PCM_PLAYBACK_DEVICE)
@@ -79,38 +71,26 @@ static int snd_bcm2835_ctl_put(struct snd_kcontrol *kcontrol,
 				struct snd_ctl_elem_value *ucontrol)
 {
 	struct bcm2835_chip *chip = snd_kcontrol_chip(kcontrol);
+	int val, *valp;
 	int changed = 0;
 
-	mutex_lock(&chip->audio_mutex);
-
-	if (kcontrol->private_value == PCM_PLAYBACK_VOLUME) {
-		audio_info("Volume change attempted.. volume = %d new_volume = %d\n", chip->volume, (int)ucontrol->value.integer.value[0]);
-		if (chip->mute == CTRL_VOL_MUTE) {
-			/* changed = toggle_mute(chip, CTRL_VOL_UNMUTE); */
-			changed = 1; /* should return 0 to signify no change but the mixer takes this as the opposite sign (no idea why) */
-			goto unlock;
-		}
-		if (changed || (ucontrol->value.integer.value[0] != chip2alsa(chip->volume))) {
-			chip->volume = alsa2chip(ucontrol->value.integer.value[0]);
-			changed = 1;
-		}
-
-	} else if (kcontrol->private_value == PCM_PLAYBACK_MUTE) {
-		/* Now implemented */
-		audio_info(" Mute attempted\n");
-		changed = toggle_mute(chip, ucontrol->value.integer.value[0]);
+	if (kcontrol->private_value == PCM_PLAYBACK_VOLUME)
+		valp = &chip->volume;
+	else if (kcontrol->private_value == PCM_PLAYBACK_MUTE)
+		valp = &chip->mute;
+	else if (kcontrol->private_value == PCM_PLAYBACK_DEVICE)
+		valp = &chip->dest;
+	else
+		return -EINVAL;
 
-	} else if (kcontrol->private_value == PCM_PLAYBACK_DEVICE) {
-		if (ucontrol->value.integer.value[0] != chip->dest) {
-			chip->dest = ucontrol->value.integer.value[0];
-			changed = 1;
-		}
+	val = ucontrol->value.integer.value[0];
+	mutex_lock(&chip->audio_mutex);
+	if (val != *valp) {
+		*valp = val;
+		changed = 1;
+		if (bcm2835_audio_set_chip_ctls(chip))
+			dev_err(chip->card->dev, "Failed to set ALSA controls..\n");
 	}
-
-	if (changed && bcm2835_audio_set_ctls(chip))
-		dev_err(chip->card->dev, "Failed to set ALSA controls..\n");
-
-unlock:
 	mutex_unlock(&chip->audio_mutex);
 	return changed;
 }
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 0be185350f33..9a79d2267df4 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -280,7 +280,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 	bcm2835_audio_setup(alsa_stream);
 
 	/* in preparation of the stream, set the controls (volume level) of the stream */
-	bcm2835_audio_set_ctls(alsa_stream->chip);
+	bcm2835_audio_set_ctls(alsa_stream);
 
 	memset(&alsa_stream->pcm_indirect, 0, sizeof(alsa_stream->pcm_indirect));
 
@@ -441,7 +441,7 @@ int snd_bcm2835_new_pcm(struct bcm2835_chip *chip, u32 numchannels)
 	strcpy(pcm->name, "bcm2835 ALSA");
 	chip->pcm = pcm;
 	chip->dest = AUDIO_DEST_AUTO;
-	chip->volume = alsa2chip(0);
+	chip->volume = 0;
 	chip->mute = CTRL_VOL_UNMUTE; /*disable mute on startup */
 	/* set operators */
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
@@ -498,7 +498,7 @@ int snd_bcm2835_new_simple_pcm(struct bcm2835_chip *chip,
 	strcpy(pcm->name, name);
 	chip->pcm = pcm;
 	chip->dest = route;
-	chip->volume = alsa2chip(0);
+	chip->volume = 0;
 	chip->mute = CTRL_VOL_UNMUTE;
 
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 942a38942c29..8684dc1d0b41 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -460,11 +460,11 @@ int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
 	return ret;
 }
 
-static int bcm2835_audio_set_ctls_chan(struct bcm2835_alsa_stream *alsa_stream,
-				       struct bcm2835_chip *chip)
+int bcm2835_audio_set_ctls(struct bcm2835_alsa_stream *alsa_stream)
 {
 	struct vc_audio_msg m;
 	struct bcm2835_audio_instance *instance = alsa_stream->instance;
+	struct bcm2835_chip *chip = alsa_stream->chip;
 	int status;
 	int ret;
 
@@ -478,7 +478,10 @@ static int bcm2835_audio_set_ctls_chan(struct bcm2835_alsa_stream *alsa_stream,
 
 	m.type = VC_AUDIO_MSG_TYPE_CONTROL;
 	m.u.control.dest = chip->dest;
-	m.u.control.volume = chip->volume;
+	if (!chip->mute)
+		m.u.control.volume = CHIP_MIN_VOLUME;
+	else
+		m.u.control.volume = alsa2chip(chip->volume);
 
 	/* Create the message available completion */
 	init_completion(&instance->msg_avail_comp);
@@ -514,27 +517,6 @@ static int bcm2835_audio_set_ctls_chan(struct bcm2835_alsa_stream *alsa_stream,
 	return ret;
 }
 
-int bcm2835_audio_set_ctls(struct bcm2835_chip *chip)
-{
-	int i;
-	int ret = 0;
-
-	LOG_DBG(" Setting ALSA dest(%d), volume(%d)\n", chip->dest, chip->volume);
-
-	/* change ctls for all substreams */
-	for (i = 0; i < MAX_SUBSTREAMS; i++) {
-		if (!chip->alsa_stream[i])
-			continue;
-		if (bcm2835_audio_set_ctls_chan(chip->alsa_stream[i], chip) != 0) {
-			LOG_ERR("Couldn't set the controls for stream %d\n", i);
-			ret = -1;
-		} else {
-			LOG_DBG(" Controls set for stream %d\n", i);
-		}
-	}
-	return ret;
-}
-
 int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 			     unsigned int channels, unsigned int samplerate,
 			     unsigned int bps)
@@ -548,7 +530,7 @@ int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 		 channels, samplerate, bps);
 
 	/* resend ctls - alsa_stream may not have been open when first send */
-	ret = bcm2835_audio_set_ctls_chan(alsa_stream, alsa_stream->chip);
+	ret = bcm2835_audio_set_ctls(alsa_stream);
 	if (ret) {
 		LOG_ERR(" Alsa controls not supported\n");
 		return -EINVAL;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index c0e7df4914ed..8ee25a837b08 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -74,6 +74,8 @@ enum {
 // convert chip to alsa volume
 #define chip2alsa(vol) -(((vol) * 100) >> 8)
 
+#define CHIP_MIN_VOLUME		26214 /* minimum level aka mute */
+
 /* Some constants for values .. */
 enum snd_bcm2835_route {
 	AUDIO_DEST_AUTO = 0,
@@ -102,7 +104,6 @@ struct bcm2835_chip {
 	struct bcm2835_alsa_stream *alsa_stream[MAX_SUBSTREAMS];
 
 	int volume;
-	int old_volume; /* stores the volume value whist muted */
 	int dest;
 	int mute;
 
@@ -160,7 +161,7 @@ int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 int bcm2835_audio_setup(struct bcm2835_alsa_stream *alsa_stream);
 int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream);
 int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream);
-int bcm2835_audio_set_ctls(struct bcm2835_chip *chip);
+int bcm2835_audio_set_ctls(struct bcm2835_alsa_stream *alsa_stream);
 int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 			unsigned int count,
 			void *src);
-- 
2.18.0

[PATCH 06/29] staging: bcm2835-audio: Remove redundant function calls

[Takashi Iwai]

bcm2835_audio_setup(), bcm2835_audio_flush_buffers() and
bcm2835_audio_flush_playback_buffers() functions do implement
nothing.

Also, bcm2835_audio_set_ctls() is already called inside
bcm2835_audio_set_params(), so the later call is superfluous.

This patch removes these superfluous implementations.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c |  5 -----
 .../bcm2835-audio/bcm2835-vchiq.c             | 21 -------------------
 .../vc04_services/bcm2835-audio/bcm2835.h     |  3 ---
 3 files changed, 29 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 9a79d2267df4..a3ab5bfea08a 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -277,11 +277,6 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 	if (err < 0)
 		audio_error(" error setting hw params\n");
 
-	bcm2835_audio_setup(alsa_stream);
-
-	/* in preparation of the stream, set the controls (volume level) of the stream */
-	bcm2835_audio_set_ctls(alsa_stream);
-
 	memset(&alsa_stream->pcm_indirect, 0, sizeof(alsa_stream->pcm_indirect));
 
 	alsa_stream->pcm_indirect.hw_buffer_size =
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 8684dc1d0b41..488e676e25e1 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -580,12 +580,6 @@ int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 	return ret;
 }
 
-int bcm2835_audio_setup(struct bcm2835_alsa_stream *alsa_stream)
-{
-
-	return 0;
-}
-
 static int bcm2835_audio_start_worker(struct bcm2835_alsa_stream *alsa_stream)
 {
 	struct vc_audio_msg m;
@@ -774,21 +768,6 @@ static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
 	return ret;
 }
 
-/**
- * Returns all buffers from arm->vc
- */
-void bcm2835_audio_flush_buffers(struct bcm2835_alsa_stream *alsa_stream)
-{
-}
-
-/**
- * Forces VC to flush(drop) its filled playback buffers and
- * return them the us. (VC->ARM)
- */
-void bcm2835_audio_flush_playback_buffers(struct bcm2835_alsa_stream *alsa_stream)
-{
-}
-
 unsigned int bcm2835_audio_retrieve_buffers(struct bcm2835_alsa_stream *alsa_stream)
 {
 	unsigned int count = atomic_read(&alsa_stream->retrieved);
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index 8ee25a837b08..d2441916960d 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -158,7 +158,6 @@ int bcm2835_audio_close(struct bcm2835_alsa_stream *alsa_stream);
 int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 			     unsigned int channels, unsigned int samplerate,
 			     unsigned int bps);
-int bcm2835_audio_setup(struct bcm2835_alsa_stream *alsa_stream);
 int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream);
 int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream);
 int bcm2835_audio_set_ctls(struct bcm2835_alsa_stream *alsa_stream);
@@ -167,7 +166,5 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 			void *src);
 void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream);
 unsigned int bcm2835_audio_retrieve_buffers(struct bcm2835_alsa_stream *alsa_stream);
-void bcm2835_audio_flush_buffers(struct bcm2835_alsa_stream *alsa_stream);
-void bcm2835_audio_flush_playback_buffers(struct bcm2835_alsa_stream *alsa_stream);
 
 #endif /* __SOUND_ARM_BCM2835_H */
-- 
2.18.0

[PATCH 07/29] staging: bcm2835-audio: Remove superfluous open flag

[Takashi Iwai]

All the alsa_stream->open flag checks in the current code are
redundant, and they cannot be racy.  For the code simplification,
let's remove the flag and its check.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../staging/vc04_services/bcm2835-audio/bcm2835-pcm.c    | 9 ++-------
 drivers/staging/vc04_services/bcm2835-audio/bcm2835.h    | 1 -
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index a3ab5bfea08a..2c2b6b70df63 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -57,8 +57,7 @@ void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream)
 	audio_info("alsa_stream=%p substream=%p\n", alsa_stream,
 		alsa_stream ? alsa_stream->substream : 0);
 
-	if (alsa_stream->open)
-		consumed = bcm2835_audio_retrieve_buffers(alsa_stream);
+	consumed = bcm2835_audio_retrieve_buffers(alsa_stream);
 
 	/* We get called only if playback was triggered, So, the number of buffers we retrieve in
 	 * each iteration are the buffers that have been played out already
@@ -154,7 +153,6 @@ static int snd_bcm2835_playback_open_generic(
 	chip->alsa_stream[idx] = alsa_stream;
 
 	chip->opened |= (1 << idx);
-	alsa_stream->open = 1;
 	alsa_stream->draining = 1;
 
 out:
@@ -205,10 +203,7 @@ static int snd_bcm2835_playback_close(struct snd_pcm_substream *substream)
 	alsa_stream->period_size = 0;
 	alsa_stream->buffer_size = 0;
 
-	if (alsa_stream->open) {
-		alsa_stream->open = 0;
-		bcm2835_audio_close(alsa_stream);
-	}
+	bcm2835_audio_close(alsa_stream);
 	if (alsa_stream->chip)
 		alsa_stream->chip->alsa_stream[alsa_stream->idx] = NULL;
 	/*
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index d2441916960d..79363260ae34 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -121,7 +121,6 @@ struct bcm2835_alsa_stream {
 
 	spinlock_t lock;
 
-	int open;
 	int running;
 	int draining;
 
-- 
2.18.0

[PATCH 08/29] staging: bcm2835-audio: Drop useless running flag and check

[Takashi Iwai]

The running flag of alsa_stream is basically useless.  The running
state is strictly controlled in ALSA PCM core side, hence the check in
PCM trigger and close callbacks are superfluous.

Also, the prefill ack at trigger start became superfluous nowadays
with the ALSA PCM core update.

Let's rip them off.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 46 ++++---------------
 .../vc04_services/bcm2835-audio/bcm2835.h     |  1 -
 2 files changed, 8 insertions(+), 39 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 2c2b6b70df63..b4b9e90131bf 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -187,19 +187,6 @@ static int snd_bcm2835_playback_close(struct snd_pcm_substream *substream)
 
 	audio_info("Alsa close\n");
 
-	/*
-	 * Call stop if it's still running. This happens when app
-	 * is force killed and we don't get a stop trigger.
-	 */
-	if (alsa_stream->running) {
-		int err;
-
-		err = bcm2835_audio_stop(alsa_stream);
-		alsa_stream->running = 0;
-		if (err)
-			audio_error(" Failed to STOP alsa device\n");
-	}
-
 	alsa_stream->period_size = 0;
 	alsa_stream->buffer_size = 0;
 
@@ -324,27 +311,13 @@ static int snd_bcm2835_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-		audio_debug("bcm2835_AUDIO_TRIGGER_START running=%d\n",
-			alsa_stream->running);
-		if (!alsa_stream->running) {
-			err = bcm2835_audio_start(alsa_stream);
-			if (!err) {
-				alsa_stream->pcm_indirect.hw_io =
-					alsa_stream->pcm_indirect.hw_data =
-					bytes_to_frames(runtime,
-					alsa_stream->pos);
-				substream->ops->ack(substream);
-				alsa_stream->running = 1;
-				alsa_stream->draining = 1;
-			} else {
-				audio_error(" Failed to START alsa device (%d)\n", err);
-			}
-		}
+		err = bcm2835_audio_start(alsa_stream);
+		if (!err)
+			alsa_stream->draining = 1;
+		else
+			audio_error(" Failed to START alsa device (%d)\n", err);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
-		audio_debug
-			("bcm2835_AUDIO_TRIGGER_STOP running=%d draining=%d\n",
-			alsa_stream->running, runtime->status->state == SNDRV_PCM_STATE_DRAINING);
 		if (runtime->status->state == SNDRV_PCM_STATE_DRAINING) {
 			audio_info("DRAINING\n");
 			alsa_stream->draining = 1;
@@ -352,12 +325,9 @@ static int snd_bcm2835_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 			audio_info("DROPPING\n");
 			alsa_stream->draining = 0;
 		}
-		if (alsa_stream->running) {
-			err = bcm2835_audio_stop(alsa_stream);
-			if (err != 0)
-				audio_error(" Failed to STOP alsa device (%d)\n", err);
-			alsa_stream->running = 0;
-		}
+		err = bcm2835_audio_stop(alsa_stream);
+		if (err != 0)
+			audio_error(" Failed to STOP alsa device (%d)\n", err);
 		break;
 	default:
 		err = -EINVAL;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index 79363260ae34..2a9f3d6c10dc 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -121,7 +121,6 @@ struct bcm2835_alsa_stream {
 
 	spinlock_t lock;
 
-	int running;
 	int draining;
 
 	int channels;
-- 
2.18.0

[PATCH 09/29] staging: bcm2835-audio: Fix incorrect draining handling

[Takashi Iwai]

The handling of SNDRV_PCM_TRIGGER_STOP at the trigger callback is
incorrect: when the STOP is issued, the driver is supposed to drop the
stream immediately.  Meanwhile bcm2835 driver checks the DRAINING
state and tries to issue some different command.

This patch straightens things a bit, dropping the incorrect state
checks.  The draining behavior would be still not perfect at this
point, but will be improved in a later patch.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c  | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index b4b9e90131bf..00c2abab4bba 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -153,7 +153,6 @@ static int snd_bcm2835_playback_open_generic(
 	chip->alsa_stream[idx] = alsa_stream;
 
 	chip->opened |= (1 << idx);
-	alsa_stream->draining = 1;
 
 out:
 	mutex_unlock(&chip->audio_mutex);
@@ -268,6 +267,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 	alsa_stream->buffer_size = snd_pcm_lib_buffer_bytes(substream);
 	alsa_stream->period_size = snd_pcm_lib_period_bytes(substream);
 	alsa_stream->pos = 0;
+	alsa_stream->draining = false;
 
 	audio_debug("buffer_size=%d, period_size=%d pos=%d frame_bits=%d\n",
 		alsa_stream->buffer_size, alsa_stream->period_size,
@@ -312,21 +312,15 @@ static int snd_bcm2835_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 		err = bcm2835_audio_start(alsa_stream);
-		if (!err)
-			alsa_stream->draining = 1;
-		else
+		if (err)
 			audio_error(" Failed to START alsa device (%d)\n", err);
 		break;
+	case SNDRV_PCM_TRIGGER_DRAIN:
+		alsa_stream->draining = true;
+		break;
 	case SNDRV_PCM_TRIGGER_STOP:
-		if (runtime->status->state == SNDRV_PCM_STATE_DRAINING) {
-			audio_info("DRAINING\n");
-			alsa_stream->draining = 1;
-		} else {
-			audio_info("DROPPING\n");
-			alsa_stream->draining = 0;
-		}
 		err = bcm2835_audio_stop(alsa_stream);
-		if (err != 0)
+		if (err)
 			audio_error(" Failed to STOP alsa device (%d)\n", err);
 		break;
 	default:
-- 
2.18.0

[PATCH 10/29] staging: bcm2835-audio: Kill unused spinlock

[Takashi Iwai]

The alsa_stream->lock is never used.  Kill it.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 2 --
 drivers/staging/vc04_services/bcm2835-audio/bcm2835.h     | 2 --
 2 files changed, 4 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 00c2abab4bba..fc1b345d206c 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -128,8 +128,6 @@ static int snd_bcm2835_playback_open_generic(
 	alsa_stream->substream = substream;
 	alsa_stream->idx = idx;
 
-	spin_lock_init(&alsa_stream->lock);
-
 	err = bcm2835_audio_open(alsa_stream);
 	if (err) {
 		kfree(alsa_stream);
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index 2a9f3d6c10dc..20f5ff1649e6 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -119,8 +119,6 @@ struct bcm2835_alsa_stream {
 	struct snd_pcm_substream *substream;
 	struct snd_pcm_indirect pcm_indirect;
 
-	spinlock_t lock;
-
 	int draining;
 
 	int channels;
-- 
2.18.0

[PATCH 11/29] staging: bcm2835-audio: Use PCM runtime values instead

[Takashi Iwai]

Some fields in alsa_stream are the values we keep already in PCM
runtime object, hence they are redundant.  Use the standard PCM
runtime values instead of the private copies.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 23 ++++---------------
 .../vc04_services/bcm2835-audio/bcm2835.h     |  4 ----
 2 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index fc1b345d206c..a3605505cc20 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -206,22 +206,7 @@ static int snd_bcm2835_playback_close(struct snd_pcm_substream *substream)
 static int snd_bcm2835_pcm_hw_params(struct snd_pcm_substream *substream,
 	struct snd_pcm_hw_params *params)
 {
-	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct bcm2835_alsa_stream *alsa_stream = runtime->private_data;
-	int err;
-
-	err = snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(params));
-	if (err < 0) {
-		audio_error
-			(" pcm_lib_malloc failed to allocated pages for buffers\n");
-		return err;
-	}
-
-	alsa_stream->channels = params_channels(params);
-	alsa_stream->params_rate = params_rate(params);
-	alsa_stream->pcm_format_width = snd_pcm_format_width(params_format(params));
-
-	return err;
+	return snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(params));
 }
 
 /* hw_free callback */
@@ -248,11 +233,11 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 	if (chip->spdif_status & IEC958_AES0_NONAUDIO)
 		channels = 0;
 	else
-		channels = alsa_stream->channels;
+		channels = runtime->channels;
 
 	err = bcm2835_audio_set_params(alsa_stream, channels,
-		alsa_stream->params_rate,
-		alsa_stream->pcm_format_width);
+				       runtime->rate,
+				       snd_pcm_format_width(runtime->format));
 	if (err < 0)
 		audio_error(" error setting hw params\n");
 
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index 20f5ff1649e6..3bf128422a6f 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -121,10 +121,6 @@ struct bcm2835_alsa_stream {
 
 	int draining;
 
-	int channels;
-	int params_rate;
-	int pcm_format_width;
-
 	unsigned int pos;
 	unsigned int buffer_size;
 	unsigned int period_size;
-- 
2.18.0

[PATCH 12/29] staging: bcm2835-audio: Drop unnecessary pcm indirect setup

[Takashi Iwai]

The hw_queue_size of PCM indirect helper doesn't need to be set up if
you use the whole given buffer size.  Drop the useless
initialization, which just confuses readers.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index a3605505cc20..c935c6e99633 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -280,7 +280,6 @@ static int snd_bcm2835_pcm_ack(struct snd_pcm_substream *substream)
 	struct bcm2835_alsa_stream *alsa_stream = runtime->private_data;
 	struct snd_pcm_indirect *pcm_indirect = &alsa_stream->pcm_indirect;
 
-	pcm_indirect->hw_queue_size = runtime->hw.buffer_bytes_max;
 	return snd_pcm_indirect_playback_transfer(substream, pcm_indirect,
 						  snd_bcm2835_pcm_transfer);
 }
-- 
2.18.0

[PATCH 13/29] staging: bcm2835-audio: Drop useless NULL check

[Takashi Iwai]

alsa_stream->chip can be never NULL.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index c935c6e99633..13c61af4e723 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -188,8 +188,7 @@ static int snd_bcm2835_playback_close(struct snd_pcm_substream *substream)
 	alsa_stream->buffer_size = 0;
 
 	bcm2835_audio_close(alsa_stream);
-	if (alsa_stream->chip)
-		alsa_stream->chip->alsa_stream[alsa_stream->idx] = NULL;
+	alsa_stream->chip->alsa_stream[alsa_stream->idx] = NULL;
 	/*
 	 * Do not free up alsa_stream here, it will be freed up by
 	 * runtime->private_free callback we registered in *_open above
-- 
2.18.0

[PATCH 14/29] staging: bcm2835-audio: Propagate parameter setup error

[Takashi Iwai]

When the parameter setup fails, the driver should propagate the error
code instead of silently ignoring it.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 13c61af4e723..41bcaff6358b 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -238,7 +238,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 				       runtime->rate,
 				       snd_pcm_format_width(runtime->format));
 	if (err < 0)
-		audio_error(" error setting hw params\n");
+		goto out;
 
 	memset(&alsa_stream->pcm_indirect, 0, sizeof(alsa_stream->pcm_indirect));
 
@@ -255,8 +255,9 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 		alsa_stream->buffer_size, alsa_stream->period_size,
 		alsa_stream->pos, runtime->frame_bits);
 
+ out:
 	mutex_unlock(&chip->audio_mutex);
-	return 0;
+	return err;
 }
 
 static void snd_bcm2835_pcm_transfer(struct snd_pcm_substream *substream,
-- 
2.18.0

[PATCH 15/29] staging: bcm2835-audio: Drop debug messages in bcm2835-pcm.c

[Takashi Iwai]

These debug messages worsen the code readability a lot while they give
little debuggability (which we already have via tracing, in anyway).

Let's clean them up.  This allows us to reduce the
snd_bcm2835_pcm_lib_ioctl() function to be a direct call of the
snd_pcm_lib_ioctl callback (like most other drivers do), too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 51 +++----------------
 1 file changed, 7 insertions(+), 44 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 41bcaff6358b..1f9c940f1cc3 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -44,9 +44,7 @@ static const struct snd_pcm_hardware snd_bcm2835_playback_spdif_hw = {
 
 static void snd_bcm2835_playback_free(struct snd_pcm_runtime *runtime)
 {
-	audio_info("Freeing up alsa stream here ..\n");
 	kfree(runtime->private_data);
-	runtime->private_data = NULL;
 }
 
 void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream)
@@ -99,7 +97,6 @@ static int snd_bcm2835_playback_open_generic(
 	int err;
 
 	mutex_lock(&chip->audio_mutex);
-	audio_info("Alsa open (%d)\n", substream->number);
 	idx = substream->number;
 
 	if (spdif && chip->opened) {
@@ -182,8 +179,6 @@ static int snd_bcm2835_playback_close(struct snd_pcm_substream *substream)
 	runtime = substream->runtime;
 	alsa_stream = runtime->private_data;
 
-	audio_info("Alsa close\n");
-
 	alsa_stream->period_size = 0;
 	alsa_stream->buffer_size = 0;
 
@@ -251,10 +246,6 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 	alsa_stream->pos = 0;
 	alsa_stream->draining = false;
 
-	audio_debug("buffer_size=%d, period_size=%d pos=%d frame_bits=%d\n",
-		alsa_stream->buffer_size, alsa_stream->period_size,
-		alsa_stream->pos, runtime->frame_bits);
-
  out:
 	mutex_unlock(&chip->audio_mutex);
 	return err;
@@ -266,12 +257,8 @@ static void snd_bcm2835_pcm_transfer(struct snd_pcm_substream *substream,
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct bcm2835_alsa_stream *alsa_stream = runtime->private_data;
 	void *src = (void *) (substream->runtime->dma_area + rec->sw_data);
-	int err;
-
-	err = bcm2835_audio_write(alsa_stream, bytes, src);
-	if (err)
-		audio_error(" Failed to transfer to alsa device (%d)\n", err);
 
+	bcm2835_audio_write(alsa_stream, bytes, src);
 }
 
 static int snd_bcm2835_pcm_ack(struct snd_pcm_substream *substream)
@@ -289,27 +276,18 @@ static int snd_bcm2835_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct bcm2835_alsa_stream *alsa_stream = runtime->private_data;
-	int err = 0;
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-		err = bcm2835_audio_start(alsa_stream);
-		if (err)
-			audio_error(" Failed to START alsa device (%d)\n", err);
-		break;
+		return bcm2835_audio_start(alsa_stream);
 	case SNDRV_PCM_TRIGGER_DRAIN:
 		alsa_stream->draining = true;
-		break;
+		return 0;
 	case SNDRV_PCM_TRIGGER_STOP:
-		err = bcm2835_audio_stop(alsa_stream);
-		if (err)
-			audio_error(" Failed to STOP alsa device (%d)\n", err);
-		break;
+		return bcm2835_audio_stop(alsa_stream);
 	default:
-		err = -EINVAL;
+		return -EINVAL;
 	}
-
-	return err;
 }
 
 /* pointer callback */
@@ -319,31 +297,16 @@ snd_bcm2835_pcm_pointer(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct bcm2835_alsa_stream *alsa_stream = runtime->private_data;
 
-	audio_debug("pcm_pointer... (%d) hwptr=%d appl=%d pos=%d\n", 0,
-		frames_to_bytes(runtime, runtime->status->hw_ptr),
-		frames_to_bytes(runtime, runtime->control->appl_ptr),
-		alsa_stream->pos);
-
 	return snd_pcm_indirect_playback_pointer(substream,
 		&alsa_stream->pcm_indirect,
 		alsa_stream->pos);
 }
 
-static int snd_bcm2835_pcm_lib_ioctl(struct snd_pcm_substream *substream,
-	unsigned int cmd, void *arg)
-{
-	int ret = snd_pcm_lib_ioctl(substream, cmd, arg);
-
-	audio_info(" .. substream=%p, cmd=%d, arg=%p (%x) ret=%d\n", substream,
-		cmd, arg, arg ? *(unsigned int *)arg : 0, ret);
-	return ret;
-}
-
 /* operators */
 static const struct snd_pcm_ops snd_bcm2835_playback_ops = {
 	.open = snd_bcm2835_playback_open,
 	.close = snd_bcm2835_playback_close,
-	.ioctl = snd_bcm2835_pcm_lib_ioctl,
+	.ioctl = snd_pcm_lib_ioctl,
 	.hw_params = snd_bcm2835_pcm_hw_params,
 	.hw_free = snd_bcm2835_pcm_hw_free,
 	.prepare = snd_bcm2835_pcm_prepare,
@@ -355,7 +318,7 @@ static const struct snd_pcm_ops snd_bcm2835_playback_ops = {
 static const struct snd_pcm_ops snd_bcm2835_playback_spdif_ops = {
 	.open = snd_bcm2835_playback_spdif_open,
 	.close = snd_bcm2835_playback_close,
-	.ioctl = snd_bcm2835_pcm_lib_ioctl,
+	.ioctl = snd_pcm_lib_ioctl,
 	.hw_params = snd_bcm2835_pcm_hw_params,
 	.hw_free = snd_bcm2835_pcm_hw_free,
 	.prepare = snd_bcm2835_pcm_prepare,
-- 
2.18.0

[PATCH 16/29] staging: bcm2835-audio: Drop superfluous mutex lock during prepare

[Takashi Iwai]

The chip->audio_mutex is used basically for protecting the opened
stream assignment, and the prepare callback is irrelevant with it.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 1f9c940f1cc3..9659c25b9f9d 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -218,8 +218,6 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 	int channels;
 	int err;
 
-	mutex_lock(&chip->audio_mutex);
-
 	/* notify the vchiq that it should enter spdif passthrough mode by
 	 * setting channels=0 (see
 	 * https://github.com/raspberrypi/linux/issues/528)
@@ -233,7 +231,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 				       runtime->rate,
 				       snd_pcm_format_width(runtime->format));
 	if (err < 0)
-		goto out;
+		return err;
 
 	memset(&alsa_stream->pcm_indirect, 0, sizeof(alsa_stream->pcm_indirect));
 
@@ -246,9 +244,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 	alsa_stream->pos = 0;
 	alsa_stream->draining = false;
 
- out:
-	mutex_unlock(&chip->audio_mutex);
-	return err;
+	return 0;
 }
 
 static void snd_bcm2835_pcm_transfer(struct snd_pcm_substream *substream,
-- 
2.18.0

[PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint

[Takashi Iwai]

It seems that the resolution of vc04 callback is in 10 msec; i.e. the
minimal period size is also 10 msec.

This patch adds the corresponding hw constraint.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 9659c25b9f9d..6d89db6e14e4 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -145,6 +145,11 @@ static int snd_bcm2835_playback_open_generic(
 				   SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
 				   16);
 
+	/* position update is in 10ms order */
+	snd_pcm_hw_constraint_minmax(runtime,
+				     SNDRV_PCM_HW_PARAM_PERIOD_TIME,
+				     10 * 1000, UINT_MAX);
+
 	chip->alsa_stream[idx] = alsa_stream;
 
 	chip->opened |= (1 << idx);
-- 
2.18.0

[PATCH 18/29] staging: bcm2835-audio: Make single vchi handle

[Takashi Iwai]

The bcm2835_audio_instance object contains the array of
VCHI_SERVICE_HANDLE_T, while the code assumes and uses only the first
element explicitly.  Let's reduce to a single vchi handle for
simplifying the code.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../bcm2835-audio/bcm2835-vchiq.c             | 170 ++++++------------
 1 file changed, 58 insertions(+), 112 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 488e676e25e1..be76f97705f4 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -44,8 +44,7 @@
 #endif
 
 struct bcm2835_audio_instance {
-	unsigned int num_connections;
-	VCHI_SERVICE_HANDLE_T vchi_handle[VCHI_MAX_NUM_CONNECTIONS];
+	VCHI_SERVICE_HANDLE_T vchi_handle;
 	struct completion msg_avail_comp;
 	struct mutex vchi_mutex;
 	struct bcm2835_alsa_stream *alsa_stream;
@@ -202,12 +201,12 @@ static void audio_vchi_callback(void *param,
 		BUG();
 		return;
 	}
-	if (!instance->vchi_handle[0]) {
-		LOG_ERR(" .. instance->vchi_handle[0] is null\n");
+	if (!instance->vchi_handle) {
+		LOG_ERR(" .. instance->vchi_handle is null\n");
 		BUG();
 		return;
 	}
-	status = vchi_msg_dequeue(instance->vchi_handle[0],
+	status = vchi_msg_dequeue(instance->vchi_handle,
 				  &m, sizeof(m), &msg_len, VCHI_FLAGS_NONE);
 	if (m.type == VC_AUDIO_MSG_TYPE_RESULT) {
 		LOG_DBG(" .. instance=%p, m.type=VC_AUDIO_MSG_TYPE_RESULT, success=%d\n",
@@ -237,102 +236,61 @@ static void audio_vchi_callback(void *param,
 
 static struct bcm2835_audio_instance *
 vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
-		   VCHI_CONNECTION_T **vchi_connections,
-		   unsigned int num_connections)
+		   VCHI_CONNECTION_T *vchi_connection)
 {
-	unsigned int i;
+	SERVICE_CREATION_T params = {
+		.version		= VCHI_VERSION_EX(VC_AUDIOSERV_VER, VC_AUDIOSERV_MIN_VER),
+		.service_id		= VC_AUDIO_SERVER_NAME,
+		.connection		= vchi_connection,
+		.rx_fifo_size		= 0,
+		.tx_fifo_size		= 0,
+		.callback		= audio_vchi_callback,
+		.want_unaligned_bulk_rx = 1, //TODO: remove VCOS_FALSE
+		.want_unaligned_bulk_tx = 1, //TODO: remove VCOS_FALSE
+		.want_crc		= 0
+	};
 	struct bcm2835_audio_instance *instance;
 	int status;
-	int ret;
-
-	LOG_DBG("%s: start", __func__);
 
-	if (num_connections > VCHI_MAX_NUM_CONNECTIONS) {
-		LOG_ERR("%s: unsupported number of connections %u (max=%u)\n",
-			__func__, num_connections, VCHI_MAX_NUM_CONNECTIONS);
-
-		return ERR_PTR(-EINVAL);
-	}
 	/* Allocate memory for this instance */
 	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
 	if (!instance)
 		return ERR_PTR(-ENOMEM);
 
-	instance->num_connections = num_connections;
-
 	/* Create a lock for exclusive, serialized VCHI connection access */
 	mutex_init(&instance->vchi_mutex);
 	/* Open the VCHI service connections */
-	for (i = 0; i < num_connections; i++) {
-		SERVICE_CREATION_T params = {
-			.version		= VCHI_VERSION_EX(VC_AUDIOSERV_VER, VC_AUDIOSERV_MIN_VER),
-			.service_id		= VC_AUDIO_SERVER_NAME,
-			.connection		= vchi_connections[i],
-			.rx_fifo_size		= 0,
-			.tx_fifo_size		= 0,
-			.callback		= audio_vchi_callback,
-			.callback_param		= instance,
-			.want_unaligned_bulk_rx = 1, //TODO: remove VCOS_FALSE
-			.want_unaligned_bulk_tx = 1, //TODO: remove VCOS_FALSE
-			.want_crc		= 0
-		};
-
-		LOG_DBG("%s: about to open %i\n", __func__, i);
-		status = vchi_service_open(vchi_instance, &params,
-					   &instance->vchi_handle[i]);
-
-		LOG_DBG("%s: opened %i: %p=%d\n", __func__, i, instance->vchi_handle[i], status);
-		if (status) {
-			LOG_ERR("%s: failed to open VCHI service connection (status=%d)\n",
-				__func__, status);
-			ret = -EPERM;
-			goto err_close_services;
-		}
-		/* Finished with the service for now */
-		vchi_service_release(instance->vchi_handle[i]);
-	}
+	params.callback_param = instance,
 
-	LOG_DBG("%s: okay\n", __func__);
-	return instance;
+	status = vchi_service_open(vchi_instance, &params,
+				   &instance->vchi_handle);
 
-err_close_services:
-	for (i = 0; i < instance->num_connections; i++) {
-		LOG_ERR("%s: closing %i: %p\n", __func__, i, instance->vchi_handle[i]);
-		if (instance->vchi_handle[i])
-			vchi_service_close(instance->vchi_handle[i]);
+	if (status) {
+		LOG_ERR("%s: failed to open VCHI service connection (status=%d)\n",
+			__func__, status);
+		kfree(instance);
+		return ERR_PTR(-EPERM);
 	}
 
-	kfree(instance);
-	LOG_ERR("%s: error\n", __func__);
+	/* Finished with the service for now */
+	vchi_service_release(instance->vchi_handle);
 
-	return ERR_PTR(ret);
+	return instance;
 }
 
 static int vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
 {
-	unsigned int i;
-
-	if (!instance) {
-		LOG_ERR("%s: invalid handle %p\n", __func__, instance);
-
-		return -1;
-	}
+	int status;
 
-	LOG_DBG(" .. about to lock (%d)\n", instance->num_connections);
 	mutex_lock(&instance->vchi_mutex);
 
 	/* Close all VCHI service connections */
-	for (i = 0; i < instance->num_connections; i++) {
-		int status;
-
-		LOG_DBG(" .. %i:closing %p\n", i, instance->vchi_handle[i]);
-		vchi_service_use(instance->vchi_handle[i]);
+	vchi_service_use(instance->vchi_handle);
 
-		status = vchi_service_close(instance->vchi_handle[i]);
-		if (status) {
-			LOG_DBG("%s: failed to close VCHI service connection (status=%d)\n",
-				__func__, status);
-		}
+	status = vchi_service_close(instance->vchi_handle);
+	if (status) {
+		LOG_DBG("%s: failed to close VCHI service connection (status=%d)\n",
+			__func__, status);
 	}
 
 	mutex_unlock(&instance->vchi_mutex);
@@ -383,19 +341,9 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 		(struct bcm2835_audio_instance *)alsa_stream->instance;
 	struct bcm2835_vchi_ctx *vhci_ctx = alsa_stream->chip->vchi_ctx;
 
-	LOG_INFO("%s: start\n", __func__);
-	BUG_ON(instance);
-	if (instance) {
-		LOG_ERR("%s: VCHI instance already open (%p)\n",
-			__func__, instance);
-		instance->alsa_stream = alsa_stream;
-		alsa_stream->instance = instance;
-		return 0;
-	}
-
 	/* Initialize an instance of the audio service */
 	instance = vc_vchi_audio_init(vhci_ctx->vchi_instance,
-				      &vhci_ctx->vchi_connection, 1);
+				      vhci_ctx->vchi_connection);
 
 	if (IS_ERR(instance)) {
 		LOG_ERR("%s: failed to initialize audio service\n", __func__);
@@ -407,8 +355,6 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 	instance->alsa_stream = alsa_stream;
 	alsa_stream->instance = instance;
 
-	LOG_DBG(" success !\n");
-
 	return 0;
 }
 
@@ -431,12 +377,12 @@ int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
 	LOG_DBG(" instance (%p)\n", instance);
 
 	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle[0]);
+	vchi_service_use(instance->vchi_handle);
 
 	m.type = VC_AUDIO_MSG_TYPE_OPEN;
 
 	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle[0],
+	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
 					&m, sizeof(m));
 
 	if (status) {
@@ -450,7 +396,7 @@ int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
 	ret = 0;
 
 unlock:
-	vchi_service_release(instance->vchi_handle[0]);
+	vchi_service_release(instance->vchi_handle);
 	mutex_unlock(&instance->vchi_mutex);
 
 free_wq:
@@ -472,7 +418,7 @@ int bcm2835_audio_set_ctls(struct bcm2835_alsa_stream *alsa_stream)
 		 chip->dest, chip->volume);
 
 	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle[0]);
+	vchi_service_use(instance->vchi_handle);
 
 	instance->result = -1;
 
@@ -487,7 +433,7 @@ int bcm2835_audio_set_ctls(struct bcm2835_alsa_stream *alsa_stream)
 	init_completion(&instance->msg_avail_comp);
 
 	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle[0],
+	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
 					&m, sizeof(m));
 
 	if (status) {
@@ -511,7 +457,7 @@ int bcm2835_audio_set_ctls(struct bcm2835_alsa_stream *alsa_stream)
 	ret = 0;
 
 unlock:
-	vchi_service_release(instance->vchi_handle[0]);
+	vchi_service_release(instance->vchi_handle);
 	mutex_unlock(&instance->vchi_mutex);
 
 	return ret;
@@ -537,7 +483,7 @@ int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 	}
 
 	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle[0]);
+	vchi_service_use(instance->vchi_handle);
 
 	instance->result = -1;
 
@@ -550,7 +496,7 @@ int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 	init_completion(&instance->msg_avail_comp);
 
 	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle[0],
+	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
 					&m, sizeof(m));
 
 	if (status) {
@@ -574,7 +520,7 @@ int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 	ret = 0;
 
 unlock:
-	vchi_service_release(instance->vchi_handle[0]);
+	vchi_service_release(instance->vchi_handle);
 	mutex_unlock(&instance->vchi_mutex);
 
 	return ret;
@@ -588,12 +534,12 @@ static int bcm2835_audio_start_worker(struct bcm2835_alsa_stream *alsa_stream)
 	int ret;
 
 	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle[0]);
+	vchi_service_use(instance->vchi_handle);
 
 	m.type = VC_AUDIO_MSG_TYPE_START;
 
 	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle[0],
+	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
 					&m, sizeof(m));
 
 	if (status) {
@@ -607,7 +553,7 @@ static int bcm2835_audio_start_worker(struct bcm2835_alsa_stream *alsa_stream)
 	ret = 0;
 
 unlock:
-	vchi_service_release(instance->vchi_handle[0]);
+	vchi_service_release(instance->vchi_handle);
 	mutex_unlock(&instance->vchi_mutex);
 	return ret;
 }
@@ -620,13 +566,13 @@ static int bcm2835_audio_stop_worker(struct bcm2835_alsa_stream *alsa_stream)
 	int ret;
 
 	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle[0]);
+	vchi_service_use(instance->vchi_handle);
 
 	m.type = VC_AUDIO_MSG_TYPE_STOP;
 	m.u.stop.draining = alsa_stream->draining;
 
 	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle[0],
+	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
 					&m, sizeof(m));
 
 	if (status) {
@@ -640,7 +586,7 @@ static int bcm2835_audio_stop_worker(struct bcm2835_alsa_stream *alsa_stream)
 	ret = 0;
 
 unlock:
-	vchi_service_release(instance->vchi_handle[0]);
+	vchi_service_release(instance->vchi_handle);
 	mutex_unlock(&instance->vchi_mutex);
 	return ret;
 }
@@ -655,7 +601,7 @@ int bcm2835_audio_close(struct bcm2835_alsa_stream *alsa_stream)
 	my_workqueue_quit(alsa_stream);
 
 	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle[0]);
+	vchi_service_use(instance->vchi_handle);
 
 	m.type = VC_AUDIO_MSG_TYPE_CLOSE;
 
@@ -663,7 +609,7 @@ int bcm2835_audio_close(struct bcm2835_alsa_stream *alsa_stream)
 	init_completion(&instance->msg_avail_comp);
 
 	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle[0],
+	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
 					&m, sizeof(m));
 
 	if (status) {
@@ -687,7 +633,7 @@ int bcm2835_audio_close(struct bcm2835_alsa_stream *alsa_stream)
 	ret = 0;
 
 unlock:
-	vchi_service_release(instance->vchi_handle[0]);
+	vchi_service_release(instance->vchi_handle);
 	mutex_unlock(&instance->vchi_mutex);
 
 	/* Stop the audio service */
@@ -708,10 +654,10 @@ static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
 	LOG_INFO(" Writing %d bytes from %p\n", count, src);
 
 	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle[0]);
+	vchi_service_use(instance->vchi_handle);
 
 	if (instance->peer_version == 0 &&
-	    vchi_get_peer_version(instance->vchi_handle[0], &instance->peer_version) == 0)
+	    vchi_get_peer_version(instance->vchi_handle, &instance->peer_version) == 0)
 		LOG_DBG("%s: client version %d connected\n", __func__, instance->peer_version);
 
 	m.type = VC_AUDIO_MSG_TYPE_WRITE;
@@ -723,7 +669,7 @@ static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
 	m.u.write.silence = src == NULL;
 
 	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle[0],
+	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
 					&m, sizeof(m));
 
 	if (status) {
@@ -736,7 +682,7 @@ static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
 	if (!m.u.write.silence) {
 		if (!m.u.write.max_packet) {
 			/* Send the message to the videocore */
-			status = vchi_bulk_queue_transmit(instance->vchi_handle[0],
+			status = vchi_bulk_queue_transmit(instance->vchi_handle,
 							  src, count,
 							  0 * VCHI_FLAGS_BLOCK_UNTIL_QUEUED
 							  +
@@ -746,7 +692,7 @@ static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
 			while (count > 0) {
 				int bytes = min_t(int, m.u.write.max_packet, count);
 
-				status = bcm2835_vchi_msg_queue(instance->vchi_handle[0],
+				status = bcm2835_vchi_msg_queue(instance->vchi_handle,
 								src, bytes);
 				src = (char *)src + bytes;
 				count -= bytes;
@@ -763,7 +709,7 @@ static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
 	ret = 0;
 
 unlock:
-	vchi_service_release(instance->vchi_handle[0]);
+	vchi_service_release(instance->vchi_handle);
 	mutex_unlock(&instance->vchi_mutex);
 	return ret;
 }
-- 
2.18.0

[PATCH 19/29] staging: bcm2835-audio: Code refactoring of vchiq accessor codes

[Takashi Iwai]

This is a cleanup and code refactoring in bcm2835-vchiq.c.

The major code changes are to provide local helpers for easier use of
lock / unlock, and message passing with/without response wait.  This
allows us to reduce lots of open codes.

Also, the max packet is set at opening the stream, not at each time
when the write gets called.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../bcm2835-audio/bcm2835-vchiq.c             | 440 ++++++------------
 1 file changed, 142 insertions(+), 298 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index be76f97705f4..96d3083e8add 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -49,6 +49,7 @@ struct bcm2835_audio_instance {
 	struct mutex vchi_mutex;
 	struct bcm2835_alsa_stream *alsa_stream;
 	int result;
+	unsigned int max_packet;
 	short peer_version;
 };
 
@@ -65,16 +66,68 @@ static int bcm2835_audio_start_worker(struct bcm2835_alsa_stream *alsa_stream);
 static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
 				      unsigned int count, void *src);
 
-// Routine to send a message across a service
+static void bcm2835_audio_lock(struct bcm2835_audio_instance *instance)
+{
+	mutex_lock(&instance->vchi_mutex);
+	vchi_service_use(instance->vchi_handle);
+}
+
+static void bcm2835_audio_unlock(struct bcm2835_audio_instance *instance)
+{
+	vchi_service_release(instance->vchi_handle);
+	mutex_unlock(&instance->vchi_mutex);
+}
+
+static int bcm2835_audio_send_msg_locked(struct bcm2835_audio_instance *instance,
+					 struct vc_audio_msg *m, bool wait)
+{
+	int status;
+
+	if (wait) {
+		instance->result = -1;
+		init_completion(&instance->msg_avail_comp);
+	}
+
+	status = vchi_queue_kernel_message(instance->vchi_handle,
+					   m, sizeof(*m));
+	if (status) {
+		LOG_ERR("vchi message queue failed: %d, msg=%d\n",
+			status, m->type);
+		return -EIO;
+	}
+
+	if (wait) {
+		if (!wait_for_completion_timeout(&instance->msg_avail_comp,
+						 msecs_to_jiffies(10 * 1000))) {
+			LOG_ERR("vchi message timeout, msg=%d\n", m->type);
+			return -ETIMEDOUT;
+		} else if (instance->result) {
+			LOG_ERR("vchi message response error:%d, msg=%d\n",
+				instance->result, m->type);
+			return -EIO;
+		}
+	}
+
+	return 0;
+}
 
-static int
-bcm2835_vchi_msg_queue(VCHI_SERVICE_HANDLE_T handle,
-		       void *data,
-		       unsigned int size)
+static int bcm2835_audio_send_msg(struct bcm2835_audio_instance *instance,
+				  struct vc_audio_msg *m, bool wait)
 {
-	return vchi_queue_kernel_message(handle,
-					 data,
-					 size);
+	int err;
+
+	bcm2835_audio_lock(instance);
+	err = bcm2835_audio_send_msg_locked(instance, m, wait);
+	bcm2835_audio_unlock(instance);
+	return err;
+}
+
+static int bcm2835_audio_send_simple(struct bcm2835_audio_instance *instance,
+				     int type, bool wait)
+{
+	struct vc_audio_msg m = { .type = type };
+
+	return bcm2835_audio_send_msg(instance, &m, wait);
 }
 
 static const u32 BCM2835_AUDIO_WRITE_COOKIE1 = ('B' << 24 | 'C' << 16 |
@@ -283,10 +336,9 @@ static int vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
 	int status;
 
 	mutex_lock(&instance->vchi_mutex);
-
-	/* Close all VCHI service connections */
 	vchi_service_use(instance->vchi_handle);
 
+	/* Close all VCHI service connections */
 	status = vchi_service_close(instance->vchi_handle);
 	if (status) {
 		LOG_DBG("%s: failed to close VCHI service connection (status=%d)\n",
@@ -345,12 +397,8 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 	instance = vc_vchi_audio_init(vhci_ctx->vchi_instance,
 				      vhci_ctx->vchi_connection);
 
-	if (IS_ERR(instance)) {
-		LOG_ERR("%s: failed to initialize audio service\n", __func__);
-
-		/* vchi_instance is retained for use the next time. */
+	if (IS_ERR(instance))
 		return PTR_ERR(instance);
-	}
 
 	instance->alsa_stream = alsa_stream;
 	alsa_stream->instance = instance;
@@ -361,66 +409,44 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
 {
 	struct bcm2835_audio_instance *instance;
-	struct vc_audio_msg m;
-	int status;
-	int ret;
+	int err;
 
 	alsa_stream->my_wq = alloc_workqueue("my_queue", WQ_HIGHPRI, 1);
 	if (!alsa_stream->my_wq)
 		return -ENOMEM;
 
-	ret = bcm2835_audio_open_connection(alsa_stream);
-	if (ret)
+	err = bcm2835_audio_open_connection(alsa_stream);
+	if (err < 0)
 		goto free_wq;
 
 	instance = alsa_stream->instance;
-	LOG_DBG(" instance (%p)\n", instance);
-
-	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle);
-
-	m.type = VC_AUDIO_MSG_TYPE_OPEN;
-
-	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
-					&m, sizeof(m));
-
-	if (status) {
-		LOG_ERR("%s: failed on vchi_msg_queue (status=%d)\n",
-			__func__, status);
-
-		ret = -1;
-		goto unlock;
-	}
 
-	ret = 0;
+	err = bcm2835_audio_send_simple(instance, VC_AUDIO_MSG_TYPE_OPEN,
+					false);
+	if (err < 0)
+		goto deinit;
 
-unlock:
-	vchi_service_release(instance->vchi_handle);
-	mutex_unlock(&instance->vchi_mutex);
+	bcm2835_audio_lock(instance);
+	vchi_get_peer_version(instance->vchi_handle, &instance->peer_version);
+	bcm2835_audio_unlock(instance);
+	if (instance->peer_version < 2 || force_bulk)
+		instance->max_packet = 0; /* bulk transfer */
+	else
+		instance->max_packet = 4000;
 
-free_wq:
-	if (ret)
-		destroy_workqueue(alsa_stream->my_wq);
+	return 0;
 
-	return ret;
+ deinit:
+	vc_vchi_audio_deinit(instance);
+ free_wq:
+	destroy_workqueue(alsa_stream->my_wq);
+	return err;
 }
 
 int bcm2835_audio_set_ctls(struct bcm2835_alsa_stream *alsa_stream)
 {
-	struct vc_audio_msg m;
-	struct bcm2835_audio_instance *instance = alsa_stream->instance;
 	struct bcm2835_chip *chip = alsa_stream->chip;
-	int status;
-	int ret;
-
-	LOG_INFO(" Setting ALSA dest(%d), volume(%d)\n",
-		 chip->dest, chip->volume);
-
-	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle);
-
-	instance->result = -1;
+	struct vc_audio_msg m = {};
 
 	m.type = VC_AUDIO_MSG_TYPE_CONTROL;
 	m.u.control.dest = chip->dest;
@@ -429,289 +455,107 @@ int bcm2835_audio_set_ctls(struct bcm2835_alsa_stream *alsa_stream)
 	else
 		m.u.control.volume = alsa2chip(chip->volume);
 
-	/* Create the message available completion */
-	init_completion(&instance->msg_avail_comp);
-
-	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
-					&m, sizeof(m));
-
-	if (status) {
-		LOG_ERR("%s: failed on vchi_msg_queue (status=%d)\n",
-			__func__, status);
-
-		ret = -1;
-		goto unlock;
-	}
-
-	/* We are expecting a reply from the videocore */
-	wait_for_completion(&instance->msg_avail_comp);
-
-	if (instance->result) {
-		LOG_ERR("%s: result=%d\n", __func__, instance->result);
-
-		ret = -1;
-		goto unlock;
-	}
-
-	ret = 0;
-
-unlock:
-	vchi_service_release(instance->vchi_handle);
-	mutex_unlock(&instance->vchi_mutex);
-
-	return ret;
+	return bcm2835_audio_send_msg(alsa_stream->instance, &m, true);
 }
 
 int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 			     unsigned int channels, unsigned int samplerate,
 			     unsigned int bps)
 {
-	struct vc_audio_msg m;
-	struct bcm2835_audio_instance *instance = alsa_stream->instance;
-	int status;
-	int ret;
-
-	LOG_INFO(" Setting ALSA channels(%d), samplerate(%d), bits-per-sample(%d)\n",
-		 channels, samplerate, bps);
+	struct vc_audio_msg m = {
+		 .type = VC_AUDIO_MSG_TYPE_CONFIG,
+		 .u.config.channels = channels,
+		 .u.config.samplerate = samplerate,
+		 .u.config.bps = bps,
+	};
+	int err;
 
 	/* resend ctls - alsa_stream may not have been open when first send */
-	ret = bcm2835_audio_set_ctls(alsa_stream);
-	if (ret) {
-		LOG_ERR(" Alsa controls not supported\n");
-		return -EINVAL;
-	}
-
-	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle);
-
-	instance->result = -1;
-
-	m.type = VC_AUDIO_MSG_TYPE_CONFIG;
-	m.u.config.channels = channels;
-	m.u.config.samplerate = samplerate;
-	m.u.config.bps = bps;
-
-	/* Create the message available completion */
-	init_completion(&instance->msg_avail_comp);
-
-	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
-					&m, sizeof(m));
-
-	if (status) {
-		LOG_ERR("%s: failed on vchi_msg_queue (status=%d)\n",
-			__func__, status);
-
-		ret = -1;
-		goto unlock;
-	}
-
-	/* We are expecting a reply from the videocore */
-	wait_for_completion(&instance->msg_avail_comp);
-
-	if (instance->result) {
-		LOG_ERR("%s: result=%d", __func__, instance->result);
-
-		ret = -1;
-		goto unlock;
-	}
-
-	ret = 0;
-
-unlock:
-	vchi_service_release(instance->vchi_handle);
-	mutex_unlock(&instance->vchi_mutex);
+	err = bcm2835_audio_set_ctls(alsa_stream);
+	if (err)
+		return err;
 
-	return ret;
+	return bcm2835_audio_send_msg(alsa_stream->instance, &m, true);
 }
 
 static int bcm2835_audio_start_worker(struct bcm2835_alsa_stream *alsa_stream)
 {
-	struct vc_audio_msg m;
-	struct bcm2835_audio_instance *instance = alsa_stream->instance;
-	int status;
-	int ret;
-
-	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle);
-
-	m.type = VC_AUDIO_MSG_TYPE_START;
-
-	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
-					&m, sizeof(m));
-
-	if (status) {
-		LOG_ERR("%s: failed on vchi_msg_queue (status=%d)\n",
-			__func__, status);
-
-		ret = -1;
-		goto unlock;
-	}
-
-	ret = 0;
-
-unlock:
-	vchi_service_release(instance->vchi_handle);
-	mutex_unlock(&instance->vchi_mutex);
-	return ret;
+	return bcm2835_audio_send_simple(alsa_stream->instance,
+					 VC_AUDIO_MSG_TYPE_START, false);
 }
 
 static int bcm2835_audio_stop_worker(struct bcm2835_alsa_stream *alsa_stream)
 {
-	struct vc_audio_msg m;
-	struct bcm2835_audio_instance *instance = alsa_stream->instance;
-	int status;
-	int ret;
-
-	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle);
-
-	m.type = VC_AUDIO_MSG_TYPE_STOP;
-	m.u.stop.draining = alsa_stream->draining;
-
-	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
-					&m, sizeof(m));
-
-	if (status) {
-		LOG_ERR("%s: failed on vchi_msg_queue (status=%d)\n",
-			__func__, status);
-
-		ret = -1;
-		goto unlock;
-	}
-
-	ret = 0;
-
-unlock:
-	vchi_service_release(instance->vchi_handle);
-	mutex_unlock(&instance->vchi_mutex);
-	return ret;
+	return bcm2835_audio_send_simple(alsa_stream->instance,
+					 VC_AUDIO_MSG_TYPE_STOP, false);
 }
 
 int bcm2835_audio_close(struct bcm2835_alsa_stream *alsa_stream)
 {
-	struct vc_audio_msg m;
 	struct bcm2835_audio_instance *instance = alsa_stream->instance;
-	int status;
-	int ret;
+	int err;
 
 	my_workqueue_quit(alsa_stream);
 
-	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle);
-
-	m.type = VC_AUDIO_MSG_TYPE_CLOSE;
-
-	/* Create the message available completion */
-	init_completion(&instance->msg_avail_comp);
-
-	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
-					&m, sizeof(m));
-
-	if (status) {
-		LOG_ERR("%s: failed on vchi_msg_queue (status=%d)\n",
-			__func__, status);
-		ret = -1;
-		goto unlock;
-	}
-
-	/* We are expecting a reply from the videocore */
-	wait_for_completion(&instance->msg_avail_comp);
-
-	if (instance->result) {
-		LOG_ERR("%s: failed result (result=%d)\n",
-			__func__, instance->result);
-
-		ret = -1;
-		goto unlock;
-	}
-
-	ret = 0;
-
-unlock:
-	vchi_service_release(instance->vchi_handle);
-	mutex_unlock(&instance->vchi_mutex);
+	err = bcm2835_audio_send_simple(alsa_stream->instance,
+					VC_AUDIO_MSG_TYPE_CLOSE, true);
 
 	/* Stop the audio service */
 	vc_vchi_audio_deinit(instance);
 	alsa_stream->instance = NULL;
 
-	return ret;
+	return err;
 }
 
 static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
-				      unsigned int count, void *src)
+				      unsigned int size, void *src)
 {
-	struct vc_audio_msg m;
 	struct bcm2835_audio_instance *instance = alsa_stream->instance;
-	int status;
-	int ret;
-
-	LOG_INFO(" Writing %d bytes from %p\n", count, src);
-
-	mutex_lock(&instance->vchi_mutex);
-	vchi_service_use(instance->vchi_handle);
-
-	if (instance->peer_version == 0 &&
-	    vchi_get_peer_version(instance->vchi_handle, &instance->peer_version) == 0)
-		LOG_DBG("%s: client version %d connected\n", __func__, instance->peer_version);
+	struct vc_audio_msg m = {
+		.type = VC_AUDIO_MSG_TYPE_WRITE,
+		.u.write.count = size,
+		.u.write.max_packet = instance->max_packet,
+		.u.write.cookie1 = BCM2835_AUDIO_WRITE_COOKIE1,
+		.u.write.cookie2 = BCM2835_AUDIO_WRITE_COOKIE2,
+	};
+	unsigned int count;
+	int err, status;
 
-	m.type = VC_AUDIO_MSG_TYPE_WRITE;
-	m.u.write.count = count;
-	// old version uses bulk, new version uses control
-	m.u.write.max_packet = instance->peer_version < 2 || force_bulk ? 0 : 4000;
-	m.u.write.cookie1 = BCM2835_AUDIO_WRITE_COOKIE1;
-	m.u.write.cookie2 = BCM2835_AUDIO_WRITE_COOKIE2;
-	m.u.write.silence = src == NULL;
+	if (!size)
+		return 0;
 
-	/* Send the message to the videocore */
-	status = bcm2835_vchi_msg_queue(instance->vchi_handle,
-					&m, sizeof(m));
+	bcm2835_audio_lock(instance);
+	err = bcm2835_audio_send_msg_locked(instance, &m, false);
+	if (err < 0)
+		goto unlock;
 
-	if (status) {
-		LOG_ERR("%s: failed on vchi_msg_queue (status=%d)\n",
-			__func__, status);
+	count = size;
+	if (!instance->max_packet) {
+		/* Send the message to the videocore */
+		status = vchi_bulk_queue_transmit(instance->vchi_handle,
+						  src, count,
+						  VCHI_FLAGS_BLOCK_UNTIL_DATA_READ,
+						  NULL);
+	} else {
+		while (count > 0) {
+			int bytes = min(instance->max_packet, count);
 
-		ret = -1;
-		goto unlock;
-	}
-	if (!m.u.write.silence) {
-		if (!m.u.write.max_packet) {
-			/* Send the message to the videocore */
-			status = vchi_bulk_queue_transmit(instance->vchi_handle,
-							  src, count,
-							  0 * VCHI_FLAGS_BLOCK_UNTIL_QUEUED
-							  +
-							  1 * VCHI_FLAGS_BLOCK_UNTIL_DATA_READ,
-							  NULL);
-		} else {
-			while (count > 0) {
-				int bytes = min_t(int, m.u.write.max_packet, count);
-
-				status = bcm2835_vchi_msg_queue(instance->vchi_handle,
-								src, bytes);
-				src = (char *)src + bytes;
-				count -= bytes;
-			}
+			status = vchi_queue_kernel_message(instance->vchi_handle,
+							   src, bytes);
+			src += bytes;
+			count -= bytes;
 		}
-		if (status) {
-			LOG_ERR("%s: failed on vchi_bulk_queue_transmit (status=%d)\n",
-				__func__, status);
+	}
 
-			ret = -1;
-			goto unlock;
-		}
+	if (status) {
+		LOG_ERR("failed on %d bytes transfer (status=%d)\n",
+			size, status);
+		err = -EIO;
 	}
-	ret = 0;
 
-unlock:
-	vchi_service_release(instance->vchi_handle);
-	mutex_unlock(&instance->vchi_mutex);
-	return ret;
+ unlock:
+	bcm2835_audio_unlock(instance);
+	return err;
 }
 
 unsigned int bcm2835_audio_retrieve_buffers(struct bcm2835_alsa_stream *alsa_stream)
-- 
2.18.0

[PATCH 20/29] staging: bcm2835-audio: Operate non-atomic PCM ops

[Takashi Iwai]

This is the most significant part in the patch series.

The bcm2835-audio driver used to queue the commands to vc04 core via
workqueue, but basically the whole accesses to vc04 core are done in
the sleepable context, including the callback calls.  In such a case,
rewriting the code using non-atomic PCM ops will simplify the logic a
lot.

This patch does it: all workqueue are gone and each former-work
implementation is now directly called from PCM ops like trigger and
write transfer.

Along with it, the DMA position updater, bcm2835_playback_fifo(), was
also rewritten to use a simpler logic.  Now it handles the XRUN and
draining properly by calling snd_pcm_stop() conditionally.

The current position is kept in atomic_t value so that it can be read
concurrently from the pointer callback.

Also, the bcm2835_audio_instance object is allocated at the beginning
of bcm2835_audio_open().  This makes the resource management clearer.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c |  74 +++---
 .../bcm2835-audio/bcm2835-vchiq.c             | 244 +++---------------
 .../vc04_services/bcm2835-audio/bcm2835.h     |   9 +-
 3 files changed, 82 insertions(+), 245 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 6d89db6e14e4..38969b5dfb57 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -11,7 +11,8 @@
 /* hardware definition */
 static const struct snd_pcm_hardware snd_bcm2835_playback_hw = {
 	.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
-	SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID),
+		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
+		 SNDRV_PCM_INFO_DRAIN_TRIGGER),
 	.formats = SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE,
 	.rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_8000_48000,
 	.rate_min = 8000,
@@ -27,7 +28,8 @@ static const struct snd_pcm_hardware snd_bcm2835_playback_hw = {
 
 static const struct snd_pcm_hardware snd_bcm2835_playback_spdif_hw = {
 	.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
-	SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID),
+		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
+		 SNDRV_PCM_INFO_DRAIN_TRIGGER),
 	.formats = SNDRV_PCM_FMTBIT_S16_LE,
 	.rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_44100 |
 	SNDRV_PCM_RATE_48000,
@@ -47,42 +49,34 @@ static void snd_bcm2835_playback_free(struct snd_pcm_runtime *runtime)
 	kfree(runtime->private_data);
 }
 
-void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream)
+void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream,
+			   unsigned int bytes)
 {
-	unsigned int consumed = 0;
-	int new_period = 0;
-
-	audio_info("alsa_stream=%p substream=%p\n", alsa_stream,
-		alsa_stream ? alsa_stream->substream : 0);
-
-	consumed = bcm2835_audio_retrieve_buffers(alsa_stream);
-
-	/* We get called only if playback was triggered, So, the number of buffers we retrieve in
-	 * each iteration are the buffers that have been played out already
-	 */
-
-	if (alsa_stream->period_size) {
-		if ((alsa_stream->pos / alsa_stream->period_size) !=
-			((alsa_stream->pos + consumed) / alsa_stream->period_size))
-			new_period = 1;
-	}
-	audio_debug("updating pos cur: %d + %d max:%d period_bytes:%d, hw_ptr: %d new_period:%d\n",
-		alsa_stream->pos,
-		consumed,
-		alsa_stream->buffer_size,
-		(int) (alsa_stream->period_size * alsa_stream->substream->runtime->periods),
-		frames_to_bytes(alsa_stream->substream->runtime, alsa_stream->substream->runtime->status->hw_ptr),
-		new_period);
-	if (alsa_stream->buffer_size) {
-		alsa_stream->pos += consumed & ~(1 << 30);
-		alsa_stream->pos %= alsa_stream->buffer_size;
+	struct snd_pcm_substream *substream = alsa_stream->substream;
+	unsigned int pos;
+
+	if (!alsa_stream->period_size)
+		return;
+
+	if (bytes >= alsa_stream->buffer_size) {
+		snd_pcm_stream_lock(substream);
+		snd_pcm_stop(substream,
+			     alsa_stream->draining ?
+			     SNDRV_PCM_STATE_SETUP :
+			     SNDRV_PCM_STATE_XRUN);
+		snd_pcm_stream_unlock(substream);
+		return;
 	}
 
-	if (alsa_stream->substream) {
-		if (new_period)
-			snd_pcm_period_elapsed(alsa_stream->substream);
-	} else {
-		audio_warning(" unexpected NULL substream\n");
+	pos = atomic_read(&alsa_stream->pos);
+	pos += bytes;
+	pos %= alsa_stream->buffer_size;
+	atomic_set(&alsa_stream->pos, pos);
+
+	alsa_stream->period_offset += bytes;
+	if (alsa_stream->period_offset >= alsa_stream->period_size) {
+		alsa_stream->period_offset %= alsa_stream->period_size;
+		snd_pcm_period_elapsed(substream);
 	}
 }
 
@@ -246,7 +240,8 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 
 	alsa_stream->buffer_size = snd_pcm_lib_buffer_bytes(substream);
 	alsa_stream->period_size = snd_pcm_lib_period_bytes(substream);
-	alsa_stream->pos = 0;
+	atomic_set(&alsa_stream->pos, 0);
+	alsa_stream->period_offset = 0;
 	alsa_stream->draining = false;
 
 	return 0;
@@ -283,7 +278,7 @@ static int snd_bcm2835_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 		return bcm2835_audio_start(alsa_stream);
 	case SNDRV_PCM_TRIGGER_DRAIN:
 		alsa_stream->draining = true;
-		return 0;
+		return bcm2835_audio_drain(alsa_stream);
 	case SNDRV_PCM_TRIGGER_STOP:
 		return bcm2835_audio_stop(alsa_stream);
 	default:
@@ -300,7 +295,7 @@ snd_bcm2835_pcm_pointer(struct snd_pcm_substream *substream)
 
 	return snd_pcm_indirect_playback_pointer(substream,
 		&alsa_stream->pcm_indirect,
-		alsa_stream->pos);
+		atomic_read(&alsa_stream->pos));
 }
 
 /* operators */
@@ -338,6 +333,7 @@ int snd_bcm2835_new_pcm(struct bcm2835_chip *chip, u32 numchannels)
 	if (err < 0)
 		return err;
 	pcm->private_data = chip;
+	pcm->nonatomic = true;
 	strcpy(pcm->name, "bcm2835 ALSA");
 	chip->pcm = pcm;
 	chip->dest = AUDIO_DEST_AUTO;
@@ -367,6 +363,7 @@ int snd_bcm2835_new_spdif_pcm(struct bcm2835_chip *chip)
 		return err;
 
 	pcm->private_data = chip;
+	pcm->nonatomic = true;
 	strcpy(pcm->name, "bcm2835 IEC958/HDMI");
 	chip->pcm_spdif = pcm;
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
@@ -395,6 +392,7 @@ int snd_bcm2835_new_simple_pcm(struct bcm2835_chip *chip,
 		return err;
 
 	pcm->private_data = chip;
+	pcm->nonatomic = true;
 	strcpy(pcm->name, name);
 	chip->pcm = pcm;
 	chip->dest = route;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 96d3083e8add..d7e2718e050f 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -26,10 +26,6 @@
 
 /* ---- Private Constants and Types ------------------------------------------ */
 
-#define BCM2835_AUDIO_STOP           0
-#define BCM2835_AUDIO_START          1
-#define BCM2835_AUDIO_WRITE          2
-
 /* Logging macros (for remapping to other logging mechanisms, i.e., printf) */
 #ifdef AUDIO_DEBUG_ENABLE
 #define LOG_ERR(fmt, arg...)   pr_err("%s:%d " fmt, __func__, __LINE__, ##arg)
@@ -55,17 +51,6 @@ struct bcm2835_audio_instance {
 
 static bool force_bulk;
 
-/* ---- Private Variables ---------------------------------------------------- */
-
-/* ---- Private Function Prototypes ------------------------------------------ */
-
-/* ---- Private Functions ---------------------------------------------------- */
-
-static int bcm2835_audio_stop_worker(struct bcm2835_alsa_stream *alsa_stream);
-static int bcm2835_audio_start_worker(struct bcm2835_alsa_stream *alsa_stream);
-static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
-				      unsigned int count, void *src);
-
 static void bcm2835_audio_lock(struct bcm2835_audio_instance *instance)
 {
 	mutex_lock(&instance->vchi_mutex);
@@ -135,108 +120,6 @@ static const u32 BCM2835_AUDIO_WRITE_COOKIE1 = ('B' << 24 | 'C' << 16 |
 static const u32 BCM2835_AUDIO_WRITE_COOKIE2 = ('D' << 24 | 'A' << 16 |
 						'T' << 8  | 'A');
 
-struct bcm2835_audio_work {
-	struct work_struct my_work;
-	struct bcm2835_alsa_stream *alsa_stream;
-	int cmd;
-	void *src;
-	unsigned int count;
-};
-
-static void my_wq_function(struct work_struct *work)
-{
-	struct bcm2835_audio_work *w =
-		container_of(work, struct bcm2835_audio_work, my_work);
-	int ret = -9;
-
-	switch (w->cmd) {
-	case BCM2835_AUDIO_START:
-		ret = bcm2835_audio_start_worker(w->alsa_stream);
-		break;
-	case BCM2835_AUDIO_STOP:
-		ret = bcm2835_audio_stop_worker(w->alsa_stream);
-		break;
-	case BCM2835_AUDIO_WRITE:
-		ret = bcm2835_audio_write_worker(w->alsa_stream, w->count,
-						 w->src);
-		break;
-	default:
-		LOG_ERR(" Unexpected work: %p:%d\n", w->alsa_stream, w->cmd);
-		break;
-	}
-	kfree((void *)work);
-}
-
-int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
-{
-	struct bcm2835_audio_work *work;
-
-	work = kmalloc(sizeof(*work), GFP_ATOMIC);
-	/*--- Queue some work (item 1) ---*/
-	if (!work) {
-		LOG_ERR(" .. Error: NULL work kmalloc\n");
-		return -ENOMEM;
-	}
-	INIT_WORK(&work->my_work, my_wq_function);
-	work->alsa_stream = alsa_stream;
-	work->cmd = BCM2835_AUDIO_START;
-	if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
-		kfree(work);
-		return -EBUSY;
-	}
-	return 0;
-}
-
-int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
-{
-	struct bcm2835_audio_work *work;
-
-	work = kmalloc(sizeof(*work), GFP_ATOMIC);
-	/*--- Queue some work (item 1) ---*/
-	if (!work) {
-		LOG_ERR(" .. Error: NULL work kmalloc\n");
-		return -ENOMEM;
-	}
-	INIT_WORK(&work->my_work, my_wq_function);
-	work->alsa_stream = alsa_stream;
-	work->cmd = BCM2835_AUDIO_STOP;
-	if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
-		kfree(work);
-		return -EBUSY;
-	}
-	return 0;
-}
-
-int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
-			unsigned int count, void *src)
-{
-	struct bcm2835_audio_work *work;
-
-	work = kmalloc(sizeof(*work), GFP_ATOMIC);
-	/*--- Queue some work (item 1) ---*/
-	if (!work) {
-		LOG_ERR(" .. Error: NULL work kmalloc\n");
-		return -ENOMEM;
-	}
-	INIT_WORK(&work->my_work, my_wq_function);
-	work->alsa_stream = alsa_stream;
-	work->cmd = BCM2835_AUDIO_WRITE;
-	work->src = src;
-	work->count = count;
-	if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
-		kfree(work);
-		return -EBUSY;
-	}
-	return 0;
-}
-
-static void my_workqueue_quit(struct bcm2835_alsa_stream *alsa_stream)
-{
-	flush_workqueue(alsa_stream->my_wq);
-	destroy_workqueue(alsa_stream->my_wq);
-	alsa_stream->my_wq = NULL;
-}
-
 static void audio_vchi_callback(void *param,
 				const VCHI_CALLBACK_REASON_T reason,
 				void *msg_handle)
@@ -249,47 +132,27 @@ static void audio_vchi_callback(void *param,
 	if (reason != VCHI_CALLBACK_MSG_AVAILABLE)
 		return;
 
-	if (!instance) {
-		LOG_ERR(" .. instance is null\n");
-		BUG();
-		return;
-	}
-	if (!instance->vchi_handle) {
-		LOG_ERR(" .. instance->vchi_handle is null\n");
-		BUG();
-		return;
-	}
 	status = vchi_msg_dequeue(instance->vchi_handle,
 				  &m, sizeof(m), &msg_len, VCHI_FLAGS_NONE);
 	if (m.type == VC_AUDIO_MSG_TYPE_RESULT) {
-		LOG_DBG(" .. instance=%p, m.type=VC_AUDIO_MSG_TYPE_RESULT, success=%d\n",
-			instance, m.u.result.success);
 		instance->result = m.u.result.success;
 		complete(&instance->msg_avail_comp);
 	} else if (m.type == VC_AUDIO_MSG_TYPE_COMPLETE) {
-		struct bcm2835_alsa_stream *alsa_stream = instance->alsa_stream;
-
-		LOG_DBG(" .. instance=%p, m.type=VC_AUDIO_MSG_TYPE_COMPLETE, complete=%d\n",
-			instance, m.u.complete.count);
 		if (m.u.complete.cookie1 != BCM2835_AUDIO_WRITE_COOKIE1 ||
 		    m.u.complete.cookie2 != BCM2835_AUDIO_WRITE_COOKIE2)
-			LOG_ERR(" .. response is corrupt\n");
-		else if (alsa_stream) {
-			atomic_add(m.u.complete.count,
-				   &alsa_stream->retrieved);
-			bcm2835_playback_fifo(alsa_stream);
-		} else {
-			LOG_ERR(" .. unexpected alsa_stream=%p\n",
-				alsa_stream);
-		}
+			LOG_ERR("invalid cookie\n");
+		else
+			bcm2835_playback_fifo(instance->alsa_stream,
+					      m.u.complete.count);
 	} else {
-		LOG_ERR(" .. unexpected m.type=%d\n", m.type);
+		LOG_ERR("unexpected callback type=%d\n", m.type);
 	}
 }
 
-static struct bcm2835_audio_instance *
+static int
 vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
-		   VCHI_CONNECTION_T *vchi_connection)
+		   VCHI_CONNECTION_T *vchi_connection,
+		   struct bcm2835_audio_instance *instance)
 {
 	SERVICE_CREATION_T params = {
 		.version		= VCHI_VERSION_EX(VC_AUDIOSERV_VER, VC_AUDIOSERV_MIN_VER),
@@ -298,23 +161,14 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 		.rx_fifo_size		= 0,
 		.tx_fifo_size		= 0,
 		.callback		= audio_vchi_callback,
+		.callback_param		= instance,
 		.want_unaligned_bulk_rx = 1, //TODO: remove VCOS_FALSE
 		.want_unaligned_bulk_tx = 1, //TODO: remove VCOS_FALSE
 		.want_crc		= 0
 	};
-	struct bcm2835_audio_instance *instance;
 	int status;
 
-	/* Allocate memory for this instance */
-	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
-	if (!instance)
-		return ERR_PTR(-ENOMEM);
-
-	/* Create a lock for exclusive, serialized VCHI connection access */
-	mutex_init(&instance->vchi_mutex);
 	/* Open the VCHI service connections */
-	params.callback_param = instance,
-
 	status = vchi_service_open(vchi_instance, &params,
 				   &instance->vchi_handle);
 
@@ -322,16 +176,16 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 		LOG_ERR("%s: failed to open VCHI service connection (status=%d)\n",
 			__func__, status);
 		kfree(instance);
-		return ERR_PTR(-EPERM);
+		return -EPERM;
 	}
 
 	/* Finished with the service for now */
 	vchi_service_release(instance->vchi_handle);
 
-	return instance;
+	return 0;
 }
 
-static int vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
+static void vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
 {
 	int status;
 
@@ -346,10 +200,6 @@ static int vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
 	}
 
 	mutex_unlock(&instance->vchi_mutex);
-
-	kfree(instance);
-
-	return 0;
 }
 
 int bcm2835_new_vchi_ctx(struct bcm2835_vchi_ctx *vchi_ctx)
@@ -387,39 +237,25 @@ void bcm2835_free_vchi_ctx(struct bcm2835_vchi_ctx *vchi_ctx)
 	vchi_ctx->vchi_instance = NULL;
 }
 
-static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream)
-{
-	struct bcm2835_audio_instance *instance =
-		(struct bcm2835_audio_instance *)alsa_stream->instance;
-	struct bcm2835_vchi_ctx *vhci_ctx = alsa_stream->chip->vchi_ctx;
-
-	/* Initialize an instance of the audio service */
-	instance = vc_vchi_audio_init(vhci_ctx->vchi_instance,
-				      vhci_ctx->vchi_connection);
-
-	if (IS_ERR(instance))
-		return PTR_ERR(instance);
-
-	instance->alsa_stream = alsa_stream;
-	alsa_stream->instance = instance;
-
-	return 0;
-}
-
 int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
 {
+	struct bcm2835_vchi_ctx *vchi_ctx = alsa_stream->chip->vchi_ctx;
 	struct bcm2835_audio_instance *instance;
 	int err;
 
-	alsa_stream->my_wq = alloc_workqueue("my_queue", WQ_HIGHPRI, 1);
-	if (!alsa_stream->my_wq)
+	/* Allocate memory for this instance */
+	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
+	if (!instance)
 		return -ENOMEM;
+	mutex_init(&instance->vchi_mutex);
+	instance->alsa_stream = alsa_stream;
+	alsa_stream->instance = instance;
 
-	err = bcm2835_audio_open_connection(alsa_stream);
+	err = vc_vchi_audio_init(vchi_ctx->vchi_instance,
+				 vchi_ctx->vchi_connection,
+				 instance);
 	if (err < 0)
-		goto free_wq;
-
-	instance = alsa_stream->instance;
+		goto free_instance;
 
 	err = bcm2835_audio_send_simple(instance, VC_AUDIO_MSG_TYPE_OPEN,
 					false);
@@ -438,8 +274,9 @@ int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
 
  deinit:
 	vc_vchi_audio_deinit(instance);
- free_wq:
-	destroy_workqueue(alsa_stream->my_wq);
+ free_instance:
+	alsa_stream->instance = NULL;
+	kfree(instance);
 	return err;
 }
 
@@ -478,37 +315,46 @@ int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 	return bcm2835_audio_send_msg(alsa_stream->instance, &m, true);
 }
 
-static int bcm2835_audio_start_worker(struct bcm2835_alsa_stream *alsa_stream)
+int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
 {
 	return bcm2835_audio_send_simple(alsa_stream->instance,
 					 VC_AUDIO_MSG_TYPE_START, false);
 }
 
-static int bcm2835_audio_stop_worker(struct bcm2835_alsa_stream *alsa_stream)
+int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 {
 	return bcm2835_audio_send_simple(alsa_stream->instance,
 					 VC_AUDIO_MSG_TYPE_STOP, false);
 }
 
+int bcm2835_audio_drain(struct bcm2835_alsa_stream *alsa_stream)
+{
+	struct vc_audio_msg m = {
+		.type = VC_AUDIO_MSG_TYPE_STOP,
+		.u.stop.draining = 1,
+	};
+
+	return bcm2835_audio_send_msg(alsa_stream->instance, &m, false);
+}
+
 int bcm2835_audio_close(struct bcm2835_alsa_stream *alsa_stream)
 {
 	struct bcm2835_audio_instance *instance = alsa_stream->instance;
 	int err;
 
-	my_workqueue_quit(alsa_stream);
-
 	err = bcm2835_audio_send_simple(alsa_stream->instance,
 					VC_AUDIO_MSG_TYPE_CLOSE, true);
 
 	/* Stop the audio service */
 	vc_vchi_audio_deinit(instance);
 	alsa_stream->instance = NULL;
+	kfree(instance);
 
 	return err;
 }
 
-static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
-				      unsigned int size, void *src)
+int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
+			unsigned int size, void *src)
 {
 	struct bcm2835_audio_instance *instance = alsa_stream->instance;
 	struct vc_audio_msg m = {
@@ -558,13 +404,5 @@ static int bcm2835_audio_write_worker(struct bcm2835_alsa_stream *alsa_stream,
 	return err;
 }
 
-unsigned int bcm2835_audio_retrieve_buffers(struct bcm2835_alsa_stream *alsa_stream)
-{
-	unsigned int count = atomic_read(&alsa_stream->retrieved);
-
-	atomic_sub(count, &alsa_stream->retrieved);
-	return count;
-}
-
 module_param(force_bulk, bool, 0444);
 MODULE_PARM_DESC(force_bulk, "Force use of vchiq bulk for audio");
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index 3bf128422a6f..a3c181613374 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -121,13 +121,12 @@ struct bcm2835_alsa_stream {
 
 	int draining;
 
-	unsigned int pos;
+	atomic_t pos;
+	unsigned int period_offset;
 	unsigned int buffer_size;
 	unsigned int period_size;
 
-	atomic_t retrieved;
 	struct bcm2835_audio_instance *instance;
-	struct workqueue_struct *my_wq;
 	int idx;
 };
 
@@ -152,11 +151,13 @@ int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
 			     unsigned int bps);
 int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream);
 int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream);
+int bcm2835_audio_drain(struct bcm2835_alsa_stream *alsa_stream);
 int bcm2835_audio_set_ctls(struct bcm2835_alsa_stream *alsa_stream);
 int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 			unsigned int count,
 			void *src);
-void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream);
+void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream,
+			   unsigned int size);
 unsigned int bcm2835_audio_retrieve_buffers(struct bcm2835_alsa_stream *alsa_stream);
 
 #endif /* __SOUND_ARM_BCM2835_H */
-- 
2.18.0

[PATCH 21/29] staging: bcm2835-audio: Use card->private_data

[Takashi Iwai]

Instead of allocating a separate snd_device object, let snd_card_new()
allocate the private resource.  This simplifies the code.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835.c     | 91 +++----------------
 1 file changed, 13 insertions(+), 78 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 6876a5eadc07..55e7fbc3ec44 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -86,9 +86,6 @@ static int bcm2835_devm_add_vchi_ctx(struct device *dev)
 
 static void snd_bcm2835_release(struct device *dev)
 {
-	struct bcm2835_chip *chip = dev_get_drvdata(dev);
-
-	kfree(chip);
 }
 
 static struct device *
@@ -117,69 +114,6 @@ snd_create_device(struct device *parent,
 	return device;
 }
 
-/* component-destructor
- * (see "Management of Cards and Components")
- */
-static int snd_bcm2835_dev_free(struct snd_device *device)
-{
-	struct bcm2835_chip *chip = device->device_data;
-	struct snd_card *card = chip->card;
-
-	snd_device_free(card, chip);
-
-	return 0;
-}
-
-/* chip-specific constructor
- * (see "Management of Cards and Components")
- */
-static int snd_bcm2835_create(struct snd_card *card,
-			      struct bcm2835_chip **rchip)
-{
-	struct bcm2835_chip *chip;
-	int err;
-	static struct snd_device_ops ops = {
-		.dev_free = snd_bcm2835_dev_free,
-	};
-
-	*rchip = NULL;
-
-	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
-	if (!chip)
-		return -ENOMEM;
-
-	chip->card = card;
-	mutex_init(&chip->audio_mutex);
-
-	chip->vchi_ctx = devres_find(card->dev->parent,
-				     bcm2835_devm_free_vchi_ctx, NULL, NULL);
-	if (!chip->vchi_ctx) {
-		kfree(chip);
-		return -ENODEV;
-	}
-
-	err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
-	if (err) {
-		kfree(chip);
-		return err;
-	}
-
-	*rchip = chip;
-	return 0;
-}
-
-static struct snd_card *snd_bcm2835_card_new(struct device *dev)
-{
-	struct snd_card *card;
-	int ret;
-
-	ret = snd_card_new(dev, -1, NULL, THIS_MODULE, 0, &card);
-	if (ret)
-		return ERR_PTR(ret);
-
-	return card;
-}
-
 typedef int (*bcm2835_audio_newpcm_func)(struct bcm2835_chip *chip,
 					 const char *name,
 					 enum snd_bcm2835_route route,
@@ -292,25 +226,26 @@ static int snd_add_child_device(struct device *device,
 		return PTR_ERR(child);
 	}
 
-	card = snd_bcm2835_card_new(child);
-	if (IS_ERR(card)) {
+	err = snd_card_new(child, -1, NULL, THIS_MODULE, sizeof(*chip), &card);
+	if (err < 0) {
 		dev_err(child, "Failed to create card");
-		return PTR_ERR(card);
+		return err;
 	}
 
-	snd_card_set_dev(card, child);
+	chip = card->private_data;
+	chip->card = card;
+	chip->dev = child;
+	mutex_init(&chip->audio_mutex);
+
+	chip->vchi_ctx = devres_find(device,
+				     bcm2835_devm_free_vchi_ctx, NULL, NULL);
+	if (!chip->vchi_ctx)
+		return -ENODEV;
+
 	strcpy(card->driver, audio_driver->driver.name);
 	strcpy(card->shortname, audio_driver->shortname);
 	strcpy(card->longname, audio_driver->longname);
 
-	err = snd_bcm2835_create(card, &chip);
-	if (err) {
-		dev_err(child, "Failed to create chip, error %d\n", err);
-		return err;
-	}
-
-	chip->dev = child;
-
 	err = audio_driver->newpcm(chip, audio_driver->shortname,
 		audio_driver->route,
 		numchans);
-- 
2.18.0

[PATCH 22/29] staging: bcm2835-audio: Use standard error print helpers

[Takashi Iwai]

For making the whole code more consistent, replace the home-made debug
print macros with the standard dev_err() & co.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c |  4 +-
 .../bcm2835-audio/bcm2835-vchiq.c             | 52 ++++++++-----------
 .../vc04_services/bcm2835-audio/bcm2835.c     |  2 +-
 .../vc04_services/bcm2835-audio/bcm2835.h     | 43 +--------------
 4 files changed, 27 insertions(+), 74 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 38969b5dfb57..d2373e4a4d53 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -101,8 +101,8 @@ static int snd_bcm2835_playback_open_generic(
 		goto out;
 	}
 	if (idx >= MAX_SUBSTREAMS) {
-		audio_error
-			("substream(%d) device doesn't exist max(%d) substreams allowed\n",
+		dev_err(chip->dev,
+			"substream(%d) device doesn't exist max(%d) substreams allowed\n",
 			idx, MAX_SUBSTREAMS);
 		err = -ENODEV;
 		goto out;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index d7e2718e050f..7fff5c63e33f 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -26,20 +26,8 @@
 
 /* ---- Private Constants and Types ------------------------------------------ */
 
-/* Logging macros (for remapping to other logging mechanisms, i.e., printf) */
-#ifdef AUDIO_DEBUG_ENABLE
-#define LOG_ERR(fmt, arg...)   pr_err("%s:%d " fmt, __func__, __LINE__, ##arg)
-#define LOG_WARN(fmt, arg...)  pr_info("%s:%d " fmt, __func__, __LINE__, ##arg)
-#define LOG_INFO(fmt, arg...)  pr_info("%s:%d " fmt, __func__, __LINE__, ##arg)
-#define LOG_DBG(fmt, arg...)   pr_info("%s:%d " fmt, __func__, __LINE__, ##arg)
-#else
-#define LOG_ERR(fmt, arg...)   pr_err("%s:%d " fmt, __func__, __LINE__, ##arg)
-#define LOG_WARN(fmt, arg...)	 no_printk(fmt, ##arg)
-#define LOG_INFO(fmt, arg...)	 no_printk(fmt, ##arg)
-#define LOG_DBG(fmt, arg...)	 no_printk(fmt, ##arg)
-#endif
-
 struct bcm2835_audio_instance {
+	struct device *dev;
 	VCHI_SERVICE_HANDLE_T vchi_handle;
 	struct completion msg_avail_comp;
 	struct mutex vchi_mutex;
@@ -76,7 +64,8 @@ static int bcm2835_audio_send_msg_locked(struct bcm2835_audio_instance *instance
 	status = vchi_queue_kernel_message(instance->vchi_handle,
 					   m, sizeof(*m));
 	if (status) {
-		LOG_ERR("vchi message queue failed: %d, msg=%d\n",
+		dev_err(instance->dev,
+			"vchi message queue failed: %d, msg=%d\n",
 			status, m->type);
 		return -EIO;
 	}
@@ -84,10 +73,12 @@ static int bcm2835_audio_send_msg_locked(struct bcm2835_audio_instance *instance
 	if (wait) {
 		if (!wait_for_completion_timeout(&instance->msg_avail_comp,
 						 msecs_to_jiffies(10 * 1000))) {
-			LOG_ERR("vchi message timeout, msg=%d\n", m->type);
+			dev_err(instance->dev,
+				"vchi message timeout, msg=%d\n", m->type);
 			return -ETIMEDOUT;
 		} else if (instance->result) {
-			LOG_ERR("vchi message response error:%d, msg=%d\n",
+			dev_err(instance->dev,
+				"vchi message response error:%d, msg=%d\n",
 				instance->result, m->type);
 			return -EIO;
 		}
@@ -140,12 +131,12 @@ static void audio_vchi_callback(void *param,
 	} else if (m.type == VC_AUDIO_MSG_TYPE_COMPLETE) {
 		if (m.u.complete.cookie1 != BCM2835_AUDIO_WRITE_COOKIE1 ||
 		    m.u.complete.cookie2 != BCM2835_AUDIO_WRITE_COOKIE2)
-			LOG_ERR("invalid cookie\n");
+			dev_err(instance->dev, "invalid cookie\n");
 		else
 			bcm2835_playback_fifo(instance->alsa_stream,
 					      m.u.complete.count);
 	} else {
-		LOG_ERR("unexpected callback type=%d\n", m.type);
+		dev_err(instance->dev, "unexpected callback type=%d\n", m.type);
 	}
 }
 
@@ -173,8 +164,9 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 				   &instance->vchi_handle);
 
 	if (status) {
-		LOG_ERR("%s: failed to open VCHI service connection (status=%d)\n",
-			__func__, status);
+		dev_err(instance->dev,
+			"failed to open VCHI service connection (status=%d)\n",
+			status);
 		kfree(instance);
 		return -EPERM;
 	}
@@ -195,30 +187,30 @@ static void vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)
 	/* Close all VCHI service connections */
 	status = vchi_service_close(instance->vchi_handle);
 	if (status) {
-		LOG_DBG("%s: failed to close VCHI service connection (status=%d)\n",
-			__func__, status);
+		dev_err(instance->dev,
+			"failed to close VCHI service connection (status=%d)\n",
+			status);
 	}
 
 	mutex_unlock(&instance->vchi_mutex);
 }
 
-int bcm2835_new_vchi_ctx(struct bcm2835_vchi_ctx *vchi_ctx)
+int bcm2835_new_vchi_ctx(struct device *dev, struct bcm2835_vchi_ctx *vchi_ctx)
 {
 	int ret;
 
 	/* Initialize and create a VCHI connection */
 	ret = vchi_initialise(&vchi_ctx->vchi_instance);
 	if (ret) {
-		LOG_ERR("%s: failed to initialise VCHI instance (ret=%d)\n",
-			__func__, ret);
-
+		dev_err(dev, "failed to initialise VCHI instance (ret=%d)\n",
+			ret);
 		return -EIO;
 	}
 
 	ret = vchi_connect(NULL, 0, vchi_ctx->vchi_instance);
 	if (ret) {
-		LOG_ERR("%s: failed to connect VCHI instance (ret=%d)\n",
-			__func__, ret);
+		dev_dbg(dev, "failed to connect VCHI instance (ret=%d)\n",
+			ret);
 
 		kfree(vchi_ctx->vchi_instance);
 		vchi_ctx->vchi_instance = NULL;
@@ -248,6 +240,7 @@ int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream)
 	if (!instance)
 		return -ENOMEM;
 	mutex_init(&instance->vchi_mutex);
+	instance->dev = alsa_stream->chip->dev;
 	instance->alsa_stream = alsa_stream;
 	alsa_stream->instance = instance;
 
@@ -394,7 +387,8 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 	}
 
 	if (status) {
-		LOG_ERR("failed on %d bytes transfer (status=%d)\n",
+		dev_err(instance->dev,
+			"failed on %d bytes transfer (status=%d)\n",
 			size, status);
 		err = -EIO;
 	}
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 55e7fbc3ec44..8a87c33b1ea9 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -73,7 +73,7 @@ static int bcm2835_devm_add_vchi_ctx(struct device *dev)
 
 	memset(vchi_ctx, 0, sizeof(*vchi_ctx));
 
-	ret = bcm2835_new_vchi_ctx(vchi_ctx);
+	ret = bcm2835_new_vchi_ctx(dev, vchi_ctx);
 	if (ret) {
 		devres_free(vchi_ctx);
 		return ret;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index a3c181613374..319c3e5dfbe4 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -17,47 +17,6 @@
 
 #include "interface/vchi/vchi.h"
 
-/*
- * #define AUDIO_DEBUG_ENABLE
- * #define AUDIO_VERBOSE_DEBUG_ENABLE
- */
-
-/* Debug macros */
-
-#ifdef AUDIO_DEBUG_ENABLE
-#ifdef AUDIO_VERBOSE_DEBUG_ENABLE
-
-#define audio_debug(fmt, arg...) \
-	pr_info("%s:%d " fmt, __func__, __LINE__, ##arg)
-
-#define audio_info(fmt, arg...) \
-	pr_info("%s:%d " fmt, __func__, __LINE__, ##arg)
-
-#else
-
-#define audio_debug(fmt, arg...)
-
-#define audio_info(fmt, arg...)
-
-#endif /* AUDIO_VERBOSE_DEBUG_ENABLE */
-
-#else
-
-#define audio_debug(fmt, arg...)
-
-#define audio_info(fmt, arg...)
-
-#endif /* AUDIO_DEBUG_ENABLE */
-
-#define audio_error(fmt, arg...) \
-	pr_err("%s:%d " fmt, __func__, __LINE__, ##arg)
-
-#define audio_warning(fmt, arg...) \
-	pr_warn("%s:%d " fmt, __func__, __LINE__, ##arg)
-
-#define audio_alert(fmt, arg...) \
-	pr_alert("%s:%d " fmt, __func__, __LINE__, ##arg)
-
 #define MAX_SUBSTREAMS   (8)
 #define AVAIL_SUBSTREAMS_MASK  (0xff)
 
@@ -141,7 +100,7 @@ int snd_bcm2835_new_simple_pcm(struct bcm2835_chip *chip,
 int snd_bcm2835_new_hdmi_ctl(struct bcm2835_chip *chip);
 int snd_bcm2835_new_headphones_ctl(struct bcm2835_chip *chip);
 
-int bcm2835_new_vchi_ctx(struct bcm2835_vchi_ctx *vchi_ctx);
+int bcm2835_new_vchi_ctx(struct device *dev, struct bcm2835_vchi_ctx *vchi_ctx);
 void bcm2835_free_vchi_ctx(struct bcm2835_vchi_ctx *vchi_ctx);
 
 int bcm2835_audio_open(struct bcm2835_alsa_stream *alsa_stream);
-- 
2.18.0

[PATCH 23/29] staging: bcm2835-audio: Remove unnecessary header file includes

[Takashi Iwai]

Yet a few header files are included unnecessarily.  Drop them.

Also remove trivial comments.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../bcm2835-audio/bcm2835-vchiq.c             | 19 -------------------
 .../vc04_services/bcm2835-audio/bcm2835.h     |  6 ------
 2 files changed, 25 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 7fff5c63e33f..1d756f467eb8 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -1,31 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright 2011 Broadcom Corporation.  All rights reserved. */
 
-#include <linux/device.h>
-#include <sound/core.h>
-#include <sound/initval.h>
-#include <sound/pcm.h>
-#include <linux/io.h>
-#include <linux/interrupt.h>
-#include <linux/fs.h>
-#include <linux/file.h>
-#include <linux/mm.h>
-#include <linux/syscalls.h>
-#include <linux/uaccess.h>
 #include <linux/slab.h>
-#include <linux/delay.h>
-#include <linux/atomic.h>
 #include <linux/module.h>
 #include <linux/completion.h>
-
 #include "bcm2835.h"
-
-/* ---- Include Files -------------------------------------------------------- */
-
 #include "vc_vchi_audioserv_defs.h"
 
-/* ---- Private Constants and Types ------------------------------------------ */
-
 struct bcm2835_audio_instance {
 	struct device *dev;
 	VCHI_SERVICE_HANDLE_T vchi_handle;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index 319c3e5dfbe4..4e41069dc22a 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -5,16 +5,10 @@
 #define __SOUND_ARM_BCM2835_H
 
 #include <linux/device.h>
-#include <linux/list.h>
-#include <linux/interrupt.h>
 #include <linux/wait.h>
 #include <sound/core.h>
-#include <sound/initval.h>
 #include <sound/pcm.h>
-#include <sound/pcm_params.h>
 #include <sound/pcm-indirect.h>
-#include <linux/workqueue.h>
-
 #include "interface/vchi/vchi.h"
 
 #define MAX_SUBSTREAMS   (8)
-- 
2.18.0

[PATCH 24/29] staging: bcm2835-audio: Move module parameter description

[Takashi Iwai]

For more consistency, move the module parameter description right
after its variable definition.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 1d756f467eb8..0bdaea1fdd77 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -19,6 +19,8 @@ struct bcm2835_audio_instance {
 };
 
 static bool force_bulk;
+module_param(force_bulk, bool, 0444);
+MODULE_PARM_DESC(force_bulk, "Force use of vchiq bulk for audio");
 
 static void bcm2835_audio_lock(struct bcm2835_audio_instance *instance)
 {
@@ -378,6 +380,3 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 	bcm2835_audio_unlock(instance);
 	return err;
 }
-
-module_param(force_bulk, bool, 0444);
-MODULE_PARM_DESC(force_bulk, "Force use of vchiq bulk for audio");
-- 
2.18.0

[PATCH 25/29] staging: bcm2835-audio: Use coherent device buffers

[Takashi Iwai]

The memory access to the pages allocated with
SNDRV_DMA_TYPE_CONTINUOUS are basically non-coherent, and it becomes a
problem when a process accesses via mmap.

For the more consistent access, use the device coherent memory, just
by replacing the call pattern in the allocator helpers.

The only point we need to be careful for is the device object passed
there; since bcm2835-audio driver creates fake devices and each card
is created on top of that, we need to pass its parent device as the
real device object.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c        | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index d2373e4a4d53..8b8e286587cb 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -345,8 +345,8 @@ int snd_bcm2835_new_pcm(struct bcm2835_chip *chip, u32 numchannels)
 
 	/* pre-allocation of buffers */
 	/* NOTE: this may fail */
-	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_CONTINUOUS,
-					      snd_dma_continuous_data(GFP_KERNEL),
+	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
+					      chip->card->dev->parent,
 					      snd_bcm2835_playback_hw.buffer_bytes_max,
 					      snd_bcm2835_playback_hw.buffer_bytes_max);
 
@@ -371,8 +371,8 @@ int snd_bcm2835_new_spdif_pcm(struct bcm2835_chip *chip)
 
 	/* pre-allocation of buffers */
 	/* NOTE: this may fail */
-	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_CONTINUOUS,
-		snd_dma_continuous_data(GFP_KERNEL),
+	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
+		chip->card->dev->parent,
 		snd_bcm2835_playback_spdif_hw.buffer_bytes_max, snd_bcm2835_playback_spdif_hw.buffer_bytes_max);
 
 	return 0;
@@ -404,8 +404,8 @@ int snd_bcm2835_new_simple_pcm(struct bcm2835_chip *chip,
 
 	snd_pcm_lib_preallocate_pages_for_all(
 		pcm,
-		SNDRV_DMA_TYPE_CONTINUOUS,
-		snd_dma_continuous_data(GFP_KERNEL),
+		SNDRV_DMA_TYPE_DEV,
+		chip->card->dev->parent,
 		snd_bcm2835_playback_hw.buffer_bytes_max,
 		snd_bcm2835_playback_hw.buffer_bytes_max);
 
-- 
2.18.0

[PATCH 26/29] staging: bcm2835-audio: Set SNDRV_PCM_INFO_SYNC_APPLPTR

[Takashi Iwai]

The recent ALSA PCM core supports the SNDRV_PCM_INFO_SYNC_APPLPTR flag
indicating that the driver needs the ack call at each appl_ptr
update.  This is requirement for the indirect PCM implementations like
bcm2835-audio driver, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 8b8e286587cb..5ddb8ee93cb2 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -12,7 +12,7 @@
 static const struct snd_pcm_hardware snd_bcm2835_playback_hw = {
 	.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
 		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
-		 SNDRV_PCM_INFO_DRAIN_TRIGGER),
+		 SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR),
 	.formats = SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE,
 	.rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_8000_48000,
 	.rate_min = 8000,
@@ -29,7 +29,7 @@ static const struct snd_pcm_hardware snd_bcm2835_playback_hw = {
 static const struct snd_pcm_hardware snd_bcm2835_playback_spdif_hw = {
 	.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
 		 SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
-		 SNDRV_PCM_INFO_DRAIN_TRIGGER),
+		 SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR),
 	.formats = SNDRV_PCM_FMTBIT_S16_LE,
 	.rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_44100 |
 	SNDRV_PCM_RATE_48000,
-- 
2.18.0

[PATCH 27/29] staging: bcm2835-audio: Simplify PCM creation helpers

[Takashi Iwai]

All three functions to create PCM objects are fairly resemble, and can
be unified to a single common helper.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 87 ++++---------------
 .../vc04_services/bcm2835-audio/bcm2835.c     | 17 +++-
 .../vc04_services/bcm2835-audio/bcm2835.h     |  9 +-
 3 files changed, 32 insertions(+), 81 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 5ddb8ee93cb2..98480d97cc2e 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -324,91 +324,36 @@ static const struct snd_pcm_ops snd_bcm2835_playback_spdif_ops = {
 };
 
 /* create a pcm device */
-int snd_bcm2835_new_pcm(struct bcm2835_chip *chip, u32 numchannels)
+int snd_bcm2835_new_pcm(struct bcm2835_chip *chip, const char *name,
+			int idx, enum snd_bcm2835_route route,
+			u32 numchannels, bool spdif)
 {
 	struct snd_pcm *pcm;
 	int err;
 
-	err = snd_pcm_new(chip->card, "bcm2835 ALSA", 0, numchannels, 0, &pcm);
-	if (err < 0)
-		return err;
-	pcm->private_data = chip;
-	pcm->nonatomic = true;
-	strcpy(pcm->name, "bcm2835 ALSA");
-	chip->pcm = pcm;
-	chip->dest = AUDIO_DEST_AUTO;
-	chip->volume = 0;
-	chip->mute = CTRL_VOL_UNMUTE; /*disable mute on startup */
-	/* set operators */
-	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
-			&snd_bcm2835_playback_ops);
-
-	/* pre-allocation of buffers */
-	/* NOTE: this may fail */
-	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
-					      chip->card->dev->parent,
-					      snd_bcm2835_playback_hw.buffer_bytes_max,
-					      snd_bcm2835_playback_hw.buffer_bytes_max);
-
-	return 0;
-}
-
-int snd_bcm2835_new_spdif_pcm(struct bcm2835_chip *chip)
-{
-	struct snd_pcm *pcm;
-	int err;
-
-	err = snd_pcm_new(chip->card, "bcm2835 ALSA", 1, 1, 0, &pcm);
-	if (err < 0)
-		return err;
-
-	pcm->private_data = chip;
-	pcm->nonatomic = true;
-	strcpy(pcm->name, "bcm2835 IEC958/HDMI");
-	chip->pcm_spdif = pcm;
-	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
-			&snd_bcm2835_playback_spdif_ops);
-
-	/* pre-allocation of buffers */
-	/* NOTE: this may fail */
-	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
-		chip->card->dev->parent,
-		snd_bcm2835_playback_spdif_hw.buffer_bytes_max, snd_bcm2835_playback_spdif_hw.buffer_bytes_max);
-
-	return 0;
-}
-
-int snd_bcm2835_new_simple_pcm(struct bcm2835_chip *chip,
-			       const char *name,
-			       enum snd_bcm2835_route route,
-			       u32 numchannels)
-{
-	struct snd_pcm *pcm;
-	int err;
-
-	err = snd_pcm_new(chip->card, name, 0, numchannels,
-			  0, &pcm);
+	err = snd_pcm_new(chip->card, name, idx, numchannels, 0, &pcm);
 	if (err)
 		return err;
 
 	pcm->private_data = chip;
 	pcm->nonatomic = true;
 	strcpy(pcm->name, name);
-	chip->pcm = pcm;
-	chip->dest = route;
-	chip->volume = 0;
-	chip->mute = CTRL_VOL_UNMUTE;
+	if (!spdif) {
+		chip->dest = route;
+		chip->volume = 0;
+		chip->mute = CTRL_VOL_UNMUTE;
+	}
 
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
+			spdif ? &snd_bcm2835_playback_spdif_ops :
 			&snd_bcm2835_playback_ops);
 
-	snd_pcm_lib_preallocate_pages_for_all(
-		pcm,
-		SNDRV_DMA_TYPE_DEV,
-		chip->card->dev->parent,
-		snd_bcm2835_playback_hw.buffer_bytes_max,
-		snd_bcm2835_playback_hw.buffer_bytes_max);
+	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
+		chip->card->dev->parent, 128 * 1024, 128 * 1024);
 
+	if (spdif)
+		chip->pcm_spdif = pcm;
+	else
+		chip->pcm = pcm;
 	return 0;
 }
-
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 8a87c33b1ea9..2869f310086f 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -138,17 +138,26 @@ static int bcm2835_audio_alsa_newpcm(struct bcm2835_chip *chip,
 {
 	int err;
 
-	err = snd_bcm2835_new_pcm(chip, numchannels - 1);
+	err = snd_bcm2835_new_pcm(chip, "bcm2835 ALSA", 0, AUDIO_DEST_AUTO,
+				  numchannels - 1, false);
 	if (err)
 		return err;
 
-	err = snd_bcm2835_new_spdif_pcm(chip);
+	err = snd_bcm2835_new_pcm(chip, "bcm2835 IEC958/HDMI", 1, 0, 1, true);
 	if (err)
 		return err;
 
 	return 0;
 }
 
+static int bcm2835_audio_simple_newpcm(struct bcm2835_chip *chip,
+				       const char *name,
+				       enum snd_bcm2835_route route,
+				       u32 numchannels)
+{
+	return snd_bcm2835_new_pcm(chip, name, 0, route, numchannels, false);
+}
+
 static struct bcm2835_audio_driver bcm2835_audio_alsa = {
 	.driver = {
 		.name = "bcm2835_alsa",
@@ -169,7 +178,7 @@ static struct bcm2835_audio_driver bcm2835_audio_hdmi = {
 	.shortname = "bcm2835 HDMI",
 	.longname  = "bcm2835 HDMI",
 	.minchannels = 1,
-	.newpcm = snd_bcm2835_new_simple_pcm,
+	.newpcm = bcm2835_audio_simple_newpcm,
 	.newctl = snd_bcm2835_new_hdmi_ctl,
 	.route = AUDIO_DEST_HDMI
 };
@@ -182,7 +191,7 @@ static struct bcm2835_audio_driver bcm2835_audio_headphones = {
 	.shortname = "bcm2835 Headphones",
 	.longname  = "bcm2835 Headphones",
 	.minchannels = 1,
-	.newpcm = snd_bcm2835_new_simple_pcm,
+	.newpcm = bcm2835_audio_simple_newpcm,
 	.newctl = snd_bcm2835_new_headphones_ctl,
 	.route = AUDIO_DEST_HEADPHONES
 };
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index 4e41069dc22a..e13435d1c205 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -84,12 +84,9 @@ struct bcm2835_alsa_stream {
 };
 
 int snd_bcm2835_new_ctl(struct bcm2835_chip *chip);
-int snd_bcm2835_new_pcm(struct bcm2835_chip *chip, u32 numchannels);
-int snd_bcm2835_new_spdif_pcm(struct bcm2835_chip *chip);
-int snd_bcm2835_new_simple_pcm(struct bcm2835_chip *chip,
-			       const char *name,
-			       enum snd_bcm2835_route route,
-			       u32 numchannels);
+int snd_bcm2835_new_pcm(struct bcm2835_chip *chip, const char *name,
+			int idx, enum snd_bcm2835_route route,
+			u32 numchannels, bool spdif);
 
 int snd_bcm2835_new_hdmi_ctl(struct bcm2835_chip *chip);
 int snd_bcm2835_new_headphones_ctl(struct bcm2835_chip *chip);
-- 
2.18.0

[PATCH 28/29] staging: bcm2835-audio: Simplify kctl creation helpers

[Takashi Iwai]

Just a minor code refactoring and adding some const prefix.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-ctl.c | 69 +++++++------------
 1 file changed, 25 insertions(+), 44 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
index e17b72f21a9d..a6ec72a5f0be 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
@@ -97,40 +97,34 @@ static int snd_bcm2835_ctl_put(struct snd_kcontrol *kcontrol,
 
 static DECLARE_TLV_DB_SCALE(snd_bcm2835_db_scale, CTRL_VOL_MIN, 1, 1);
 
-static struct snd_kcontrol_new snd_bcm2835_ctl[] = {
+static const struct snd_kcontrol_new snd_bcm2835_ctl[] = {
 	{
 		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
 		.name = "PCM Playback Volume",
-		.index = 0,
 		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ,
 		.private_value = PCM_PLAYBACK_VOLUME,
 		.info = snd_bcm2835_ctl_info,
 		.get = snd_bcm2835_ctl_get,
 		.put = snd_bcm2835_ctl_put,
-		.count = 1,
 		.tlv = {.p = snd_bcm2835_db_scale}
 	},
 	{
 		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
 		.name = "PCM Playback Switch",
-		.index = 0,
 		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
 		.private_value = PCM_PLAYBACK_MUTE,
 		.info = snd_bcm2835_ctl_info,
 		.get = snd_bcm2835_ctl_get,
 		.put = snd_bcm2835_ctl_put,
-		.count = 1,
 	},
 	{
 		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
 		.name = "PCM Playback Route",
-		.index = 0,
 		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
 		.private_value = PCM_PLAYBACK_DEVICE,
 		.info = snd_bcm2835_ctl_info,
 		.get = snd_bcm2835_ctl_get,
 		.put = snd_bcm2835_ctl_put,
-		.count = 1,
 	},
 };
 
@@ -196,7 +190,7 @@ static int snd_bcm2835_spdif_mask_get(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
-static struct snd_kcontrol_new snd_bcm2835_spdif[] = {
+static const struct snd_kcontrol_new snd_bcm2835_spdif[] = {
 	{
 		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
 		.name = SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT),
@@ -213,28 +207,32 @@ static struct snd_kcontrol_new snd_bcm2835_spdif[] = {
 	},
 };
 
-int snd_bcm2835_new_ctl(struct bcm2835_chip *chip)
+static int create_ctls(struct bcm2835_chip *chip, size_t size,
+		       const struct snd_kcontrol_new *kctls)
 {
-	int err;
-	unsigned int idx;
+	int i, err;
 
-	strcpy(chip->card->mixername, "Broadcom Mixer");
-	for (idx = 0; idx < ARRAY_SIZE(snd_bcm2835_ctl); idx++) {
-		err = snd_ctl_add(chip->card,
-				  snd_ctl_new1(&snd_bcm2835_ctl[idx], chip));
-		if (err < 0)
-			return err;
-	}
-	for (idx = 0; idx < ARRAY_SIZE(snd_bcm2835_spdif); idx++) {
-		err = snd_ctl_add(chip->card,
-				  snd_ctl_new1(&snd_bcm2835_spdif[idx], chip));
+	for (i = 0; i < size; i++) {
+		err = snd_ctl_add(chip->card, snd_ctl_new1(&kctls[i], chip));
 		if (err < 0)
 			return err;
 	}
 	return 0;
 }
 
-static struct snd_kcontrol_new snd_bcm2835_headphones_ctl[] = {
+int snd_bcm2835_new_ctl(struct bcm2835_chip *chip)
+{
+	int err;
+
+	strcpy(chip->card->mixername, "Broadcom Mixer");
+	err = create_ctls(chip, ARRAY_SIZE(snd_bcm2835_ctl), snd_bcm2835_ctl);
+	if (err < 0)
+		return err;
+	return create_ctls(chip, ARRAY_SIZE(snd_bcm2835_spdif),
+			   snd_bcm2835_spdif);
+}
+
+static const struct snd_kcontrol_new snd_bcm2835_headphones_ctl[] = {
 	{
 		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
 		.name = "Headphone Playback Volume",
@@ -263,21 +261,12 @@ static struct snd_kcontrol_new snd_bcm2835_headphones_ctl[] = {
 
 int snd_bcm2835_new_headphones_ctl(struct bcm2835_chip *chip)
 {
-	int err;
-	unsigned int idx;
-
 	strcpy(chip->card->mixername, "Broadcom Mixer");
-	for (idx = 0; idx < ARRAY_SIZE(snd_bcm2835_headphones_ctl); idx++) {
-		err = snd_ctl_add(chip->card,
-				  snd_ctl_new1(&snd_bcm2835_headphones_ctl[idx],
-					       chip));
-		if (err)
-			return err;
-	}
-	return 0;
+	return create_ctls(chip, ARRAY_SIZE(snd_bcm2835_headphones_ctl),
+			   snd_bcm2835_headphones_ctl);
 }
 
-static struct snd_kcontrol_new snd_bcm2835_hdmi[] = {
+static const struct snd_kcontrol_new snd_bcm2835_hdmi[] = {
 	{
 		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
 		.name = "HDMI Playback Volume",
@@ -306,16 +295,8 @@ static struct snd_kcontrol_new snd_bcm2835_hdmi[] = {
 
 int snd_bcm2835_new_hdmi_ctl(struct bcm2835_chip *chip)
 {
-	int err;
-	unsigned int idx;
-
 	strcpy(chip->card->mixername, "Broadcom Mixer");
-	for (idx = 0; idx < ARRAY_SIZE(snd_bcm2835_hdmi); idx++) {
-		err = snd_ctl_add(chip->card,
-				  snd_ctl_new1(&snd_bcm2835_hdmi[idx], chip));
-		if (err)
-			return err;
-	}
-	return 0;
+	return create_ctls(chip, ARRAY_SIZE(snd_bcm2835_hdmi),
+			   snd_bcm2835_hdmi);
 }
 
-- 
2.18.0

[PATCH 29/29] staging: bcm2835-audio: Simplify card object management

[Takashi Iwai]

Instead of creating a dummy child device to manage the card object,
just use devm stuff directly for releasing with snd_card_free().
This results in a lot of code reduction.

Since the dummy child devices are gone, the device object to be passed
to the memory allocator needs to be adjusted as well.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c |   2 +-
 .../vc04_services/bcm2835-audio/bcm2835.c     | 120 +++++-------------
 2 files changed, 33 insertions(+), 89 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 98480d97cc2e..e66da11af5cf 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -349,7 +349,7 @@ int snd_bcm2835_new_pcm(struct bcm2835_chip *chip, const char *name,
 			&snd_bcm2835_playback_ops);
 
 	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
-		chip->card->dev->parent, 128 * 1024, 128 * 1024);
+		chip->card->dev, 128 * 1024, 128 * 1024);
 
 	if (spdif)
 		chip->pcm_spdif = pcm;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 2869f310086f..87d56ab1ffa0 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -22,38 +22,6 @@ module_param(enable_compat_alsa, bool, 0444);
 MODULE_PARM_DESC(enable_compat_alsa,
 		 "Enables ALSA compatibility virtual audio device");
 
-static void snd_devm_unregister_child(struct device *dev, void *res)
-{
-	struct device *childdev = *(struct device **)res;
-	struct bcm2835_chip *chip = dev_get_drvdata(childdev);
-	struct snd_card *card = chip->card;
-
-	snd_card_free(card);
-
-	device_unregister(childdev);
-}
-
-static int snd_devm_add_child(struct device *dev, struct device *child)
-{
-	struct device **dr;
-	int ret;
-
-	dr = devres_alloc(snd_devm_unregister_child, sizeof(*dr), GFP_KERNEL);
-	if (!dr)
-		return -ENOMEM;
-
-	ret = device_add(child);
-	if (ret) {
-		devres_free(dr);
-		return ret;
-	}
-
-	*dr = child;
-	devres_add(dev, dr);
-
-	return 0;
-}
-
 static void bcm2835_devm_free_vchi_ctx(struct device *dev, void *res)
 {
 	struct bcm2835_vchi_ctx *vchi_ctx = res;
@@ -84,36 +52,6 @@ static int bcm2835_devm_add_vchi_ctx(struct device *dev)
 	return 0;
 }
 
-static void snd_bcm2835_release(struct device *dev)
-{
-}
-
-static struct device *
-snd_create_device(struct device *parent,
-		  struct device_driver *driver,
-		  const char *name)
-{
-	struct device *device;
-	int ret;
-
-	device = devm_kzalloc(parent, sizeof(*device), GFP_KERNEL);
-	if (!device)
-		return ERR_PTR(-ENOMEM);
-
-	device_initialize(device);
-	device->parent = parent;
-	device->driver = driver;
-	device->release = snd_bcm2835_release;
-
-	dev_set_name(device, "%s", name);
-
-	ret = snd_devm_add_child(parent, device);
-	if (ret)
-		return ERR_PTR(ret);
-
-	return device;
-}
-
 typedef int (*bcm2835_audio_newpcm_func)(struct bcm2835_chip *chip,
 					 const char *name,
 					 enum snd_bcm2835_route route,
@@ -216,40 +154,36 @@ static struct bcm2835_audio_drivers children_devices[] = {
 	},
 };
 
-static int snd_add_child_device(struct device *device,
+static void bcm2835_card_free(void *data)
+{
+	snd_card_free(data);
+}
+
+static int snd_add_child_device(struct device *dev,
 				struct bcm2835_audio_driver *audio_driver,
 				u32 numchans)
 {
 	struct snd_card *card;
-	struct device *child;
 	struct bcm2835_chip *chip;
 	int err;
 
-	child = snd_create_device(device, &audio_driver->driver,
-				  audio_driver->driver.name);
-	if (IS_ERR(child)) {
-		dev_err(device,
-			"Unable to create child device %p, error %ld",
-			audio_driver->driver.name,
-			PTR_ERR(child));
-		return PTR_ERR(child);
-	}
-
-	err = snd_card_new(child, -1, NULL, THIS_MODULE, sizeof(*chip), &card);
+	err = snd_card_new(dev, -1, NULL, THIS_MODULE, sizeof(*chip), &card);
 	if (err < 0) {
-		dev_err(child, "Failed to create card");
+		dev_err(dev, "Failed to create card");
 		return err;
 	}
 
 	chip = card->private_data;
 	chip->card = card;
-	chip->dev = child;
+	chip->dev = dev;
 	mutex_init(&chip->audio_mutex);
 
-	chip->vchi_ctx = devres_find(device,
+	chip->vchi_ctx = devres_find(dev,
 				     bcm2835_devm_free_vchi_ctx, NULL, NULL);
-	if (!chip->vchi_ctx)
-		return -ENODEV;
+	if (!chip->vchi_ctx) {
+		err = -ENODEV;
+		goto error;
+	}
 
 	strcpy(card->driver, audio_driver->driver.name);
 	strcpy(card->shortname, audio_driver->shortname);
@@ -259,26 +193,36 @@ static int snd_add_child_device(struct device *device,
 		audio_driver->route,
 		numchans);
 	if (err) {
-		dev_err(child, "Failed to create pcm, error %d\n", err);
-		return err;
+		dev_err(dev, "Failed to create pcm, error %d\n", err);
+		goto error;
 	}
 
 	err = audio_driver->newctl(chip);
 	if (err) {
-		dev_err(child, "Failed to create controls, error %d\n", err);
-		return err;
+		dev_err(dev, "Failed to create controls, error %d\n", err);
+		goto error;
 	}
 
 	err = snd_card_register(card);
 	if (err) {
-		dev_err(child, "Failed to register card, error %d\n", err);
-		return err;
+		dev_err(dev, "Failed to register card, error %d\n", err);
+		goto error;
 	}
 
-	dev_set_drvdata(child, chip);
-	dev_info(child, "card created with %d channels\n", numchans);
+	dev_set_drvdata(dev, chip);
 
+	err = devm_add_action(dev, bcm2835_card_free, card);
+	if (err < 0) {
+		dev_err(dev, "Failed to add devm action, err %d\n", err);
+		goto error;
+	}
+
+	dev_info(dev, "card created with %d channels\n", numchans);
 	return 0;
+
+ error:
+	snd_card_free(card);
+	return err;
 }
 
 static int snd_add_child_devices(struct device *device, u32 numchans)
-- 
2.18.0

Re: [PATCH 00/29] staging: bcm2835-audio: Cleanups and fixes

[Stefan Wahren]

Hi Takashi,

> Takashi Iwai <tiwai@suse.de> hat am 4. September 2018 um 17:58 geschrieben:
> 
> 
> Hi,
> 
> since I had an opportunity to play with RPi3B+ recently, I took a look
> at the existing bcm2835-audio driver code and was amused very much :)
> 
> So here is the result, a cleanup and fix patch series.
> 
> Most of the patches are trivial cleanups, just brushing up, removing
> many redundant and buggy codes, as well as code simplifications.
> 
> A big functional change is that now it uses non-atomic PCM ops, so
> that we can kill the ugly workqueue usages.  Also, the resource
> management was simplified.

first of all, thank you very much for this series.

Eric has no time as maintainer, so i will try to give you some feedback (beware of very little audio driver knowledge).

I functionally tested your patch series on a Raspberry Pi 1 B (bcm2835_defconfig), so this whole series is at least:

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

Unfortunately there is still an corruption issue with underlying vchiq and multi_v7_defconfig, so no wider tests.

I don't know if you tested this series on a Raspberry Pi. Maybe you have some specific scenarios, which should be tested.

As a reviewer i have some suggestions, but only trivia. I don't know if it's a problem that this series hasn't been send to devel@driverdev.osuosl.org

Stefan

Re: [PATCH 03/29] staging: bcm2835-audio: Clean up include files in bcm2835-ctl.c

[Stefan Wahren]

Hi,

> Takashi Iwai <tiwai@suse.de> hat am 4. September 2018 um 17:58 geschrieben:
> 
> 
> Only a few of them are really needed.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  .../vc04_services/bcm2835-audio/bcm2835-ctl.c     | 15 ---------------

I think this patch and patch #23 could be merge in case a new series is required.

Stefan

Re: [PATCH 16/29] staging: bcm2835-audio: Drop superfluous mutex lock during prepare

[Stefan Wahren]

Hi,

> Takashi Iwai <tiwai@suse.de> hat am 4. September 2018 um 17:58 geschrieben:
> 
> 
> The chip->audio_mutex is used basically for protecting the opened
> stream assignment, and the prepare callback is irrelevant with it.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> index 1f9c940f1cc3..9659c25b9f9d 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> @@ -218,8 +218,6 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
>  	int channels;
>  	int err;
>  
> -	mutex_lock(&chip->audio_mutex);
> -
>  	/* notify the vchiq that it should enter spdif passthrough mode by
>  	 * setting channels=0 (see
>  	 * https://github.com/raspberrypi/linux/issues/528)
> @@ -233,7 +231,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
>  				       runtime->rate,
>  				       snd_pcm_format_width(runtime->format));
>  	if (err < 0)
> -		goto out;
> +		return err;
>  
>  	memset(&alsa_stream->pcm_indirect, 0, sizeof(alsa_stream->pcm_indirect));
>  
> @@ -246,9 +244,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
>  	alsa_stream->pos = 0;
>  	alsa_stream->draining = false;
>  
> - out:
> -	mutex_unlock(&chip->audio_mutex);
> -	return err;
> +	return 0;

looks like your are removing code which has been added in patch #14 ?

>  }
>  
>  static void snd_bcm2835_pcm_transfer(struct snd_pcm_substream *substream,
> -- 
> 2.18.0
>

Re: [PATCH 16/29] staging: bcm2835-audio: Drop superfluous mutex lock during prepare

[Takashi Iwai]

On Sat, 08 Sep 2018 15:40:41 +0200,
Stefan Wahren wrote:
> 
> Hi,
> 
> > Takashi Iwai <tiwai@suse.de> hat am 4. September 2018 um 17:58 geschrieben:
> > 
> > 
> > The chip->audio_mutex is used basically for protecting the opened
> > stream assignment, and the prepare callback is irrelevant with it.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> > index 1f9c940f1cc3..9659c25b9f9d 100644
> > --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> > +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> > @@ -218,8 +218,6 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
> >  	int channels;
> >  	int err;
> >  
> > -	mutex_lock(&chip->audio_mutex);
> > -
> >  	/* notify the vchiq that it should enter spdif passthrough mode by
> >  	 * setting channels=0 (see
> >  	 * https://github.com/raspberrypi/linux/issues/528)
> > @@ -233,7 +231,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
> >  				       runtime->rate,
> >  				       snd_pcm_format_width(runtime->format));
> >  	if (err < 0)
> > -		goto out;
> > +		return err;
> >  
> >  	memset(&alsa_stream->pcm_indirect, 0, sizeof(alsa_stream->pcm_indirect));
> >  
> > @@ -246,9 +244,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
> >  	alsa_stream->pos = 0;
> >  	alsa_stream->draining = false;
> >  
> > - out:
> > -	mutex_unlock(&chip->audio_mutex);
> > -	return err;
> > +	return 0;
> 
> looks like your are removing code which has been added in patch #14 ?

No, the patch #14 corrects the missing error path.  Without patch 14,
the error from prepare is ignored.  And this patch gets rid of the
superfluous mutex lock.


thanks,

Takashi

Re: [PATCH 03/29] staging: bcm2835-audio: Clean up include files in bcm2835-ctl.c

[Takashi Iwai]

On Sat, 08 Sep 2018 15:25:14 +0200,
Stefan Wahren wrote:
> 
> Hi,
> 
> > Takashi Iwai <tiwai@suse.de> hat am 4. September 2018 um 17:58 geschrieben:
> > 
> > 
> > Only a few of them are really needed.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  .../vc04_services/bcm2835-audio/bcm2835-ctl.c     | 15 ---------------
> 
> I think this patch and patch #23 could be merge in case a new series is required.

OK, will do.


thanks,

Takashi

Re: [PATCH 00/29] staging: bcm2835-audio: Cleanups and fixes

[Takashi Iwai]

On Sat, 08 Sep 2018 15:18:10 +0200,
Stefan Wahren wrote:
> 
> Hi Takashi,
> 
> > Takashi Iwai <tiwai@suse.de> hat am 4. September 2018 um 17:58 geschrieben:
> > 
> > 
> > Hi,
> > 
> > since I had an opportunity to play with RPi3B+ recently, I took a look
> > at the existing bcm2835-audio driver code and was amused very much :)
> > 
> > So here is the result, a cleanup and fix patch series.
> > 
> > Most of the patches are trivial cleanups, just brushing up, removing
> > many redundant and buggy codes, as well as code simplifications.
> > 
> > A big functional change is that now it uses non-atomic PCM ops, so
> > that we can kill the ugly workqueue usages.  Also, the resource
> > management was simplified.
> 
> first of all, thank you very much for this series.
> 
> Eric has no time as maintainer, so i will try to give you some feedback (beware of very little audio driver knowledge).
> 
> I functionally tested your patch series on a Raspberry Pi 1 B (bcm2835_defconfig), so this whole series is at least:
> 
> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

OK, thanks, I'll put to my series in case of resubmission.
Meanwhile I'll keep the series in topic/vc04 branch of sound.git
tree.

> Unfortunately there is still an corruption issue with underlying vchiq and multi_v7_defconfig, so no wider tests.

What is this corruption issue?

> I don't know if you tested this series on a Raspberry Pi. Maybe you have some specific scenarios, which should be tested.

I have only a RPi3B+, and that's all what I've tested.
It'd be great if the patch series could be tested in a wider range of
models, of course.

The patches are only about cleanups.  They corrected the bad usages of
audio APIs and its design, but basically I haven't touched the basic
functionality intentionally at all.  So the behavior should be kept as
before.

(Actually it'd be better to revisit the design later, especially about
the multi-cards option and the PCM route mixer control, but I left as
is for compatibility reason for now.)

> As a reviewer i have some suggestions, but only trivia. I don't know if it's a problem that this series hasn't been send to devel@driverdev.osuosl.org

No, it's just because that address isn't found in MAINTAINERS file.
If it should go through it, please correct the entry at first :)


Thanks!

Takashi

Re: [PATCH 00/29] staging: bcm2835-audio: Cleanups and fixes

[Stefan Wahren]

Hi,

> Takashi Iwai <tiwai@suse.de> hat am 8. September 2018 um 18:21 geschrieben:
> 
> 
> On Sat, 08 Sep 2018 15:18:10 +0200,
> Stefan Wahren wrote:
> > 
> > Hi Takashi,
> > 
> > > Takashi Iwai <tiwai@suse.de> hat am 4. September 2018 um 17:58 geschrieben:
> > > 
> > > 
> > > Hi,
> > > 
> > > since I had an opportunity to play with RPi3B+ recently, I took a look
> > > at the existing bcm2835-audio driver code and was amused very much :)
> > > 
> > > So here is the result, a cleanup and fix patch series.
> > > 
> > > Most of the patches are trivial cleanups, just brushing up, removing
> > > many redundant and buggy codes, as well as code simplifications.
> > > 
> > > A big functional change is that now it uses non-atomic PCM ops, so
> > > that we can kill the ugly workqueue usages.  Also, the resource
> > > management was simplified.
> > 
> > first of all, thank you very much for this series.
> > 
> > Eric has no time as maintainer, so i will try to give you some feedback (beware of very little audio driver knowledge).
> > 
> > I functionally tested your patch series on a Raspberry Pi 1 B (bcm2835_defconfig), so this whole series is at least:
> > 
> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> 
> OK, thanks, I'll put to my series in case of resubmission.
> Meanwhile I'll keep the series in topic/vc04 branch of sound.git
> tree.

does it mean this series should go through your tree instead of Greg's?

> 
> > Unfortunately there is still an corruption issue with underlying vchiq and multi_v7_defconfig, so no wider tests.
> 
> What is this corruption issue?

Actual there are two of them.

First one are incompatibilities of older VC4 firmware with commit 14dd37fc7b65 ("staging: vc04_services: Remove cache-line-size property (v3)"). There is a pull request for the Foundation kernel which hasn't been upstreamed yet [1].

The second is documented here [2].

[1] - https://github.com/raspberrypi/linux/pull/2666
[2] - https://github.com/lategoodbye/rpi-zero/issues/23

> 
> > I don't know if you tested this series on a Raspberry Pi. Maybe you have some specific scenarios, which should be tested.
> 
> I have only a RPi3B+, and that's all what I've tested.

Great, just of curiosity which config did you use?

> It'd be great if the patch series could be tested in a wider range of
> models, of course.
> 
> The patches are only about cleanups.  They corrected the bad usages of
> audio APIs and its design, but basically I haven't touched the basic
> functionality intentionally at all.  So the behavior should be kept as
> before.
> 
> (Actually it'd be better to revisit the design later, especially about
> the multi-cards option and the PCM route mixer control, but I left as
> is for compatibility reason for now.)
> 
> > As a reviewer i have some suggestions, but only trivia. I don't know if it's a problem that this series hasn't been send to devel@driverdev.osuosl.org
> 
> No, it's just because that address isn't found in MAINTAINERS file.
> If it should go through it, please correct the entry at first :)

No, this is the mailing list for all staging driver. It is reported by get_maintainers.pl

Stefan

> 
> 
> Thanks!
> 
> Takashi

Re: [PATCH 00/29] staging: bcm2835-audio: Cleanups and fixes

[Takashi Iwai]

On Sat, 08 Sep 2018 19:00:34 +0200,
Stefan Wahren wrote:
> 
> Hi,
> 
> > Takashi Iwai <tiwai@suse.de> hat am 8. September 2018 um 18:21 geschrieben:
> > 
> > 
> > On Sat, 08 Sep 2018 15:18:10 +0200,
> > Stefan Wahren wrote:
> > > 
> > > Hi Takashi,
> > > 
> > > > Takashi Iwai <tiwai@suse.de> hat am 4. September 2018 um 17:58 geschrieben:
> > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > since I had an opportunity to play with RPi3B+ recently, I took a look
> > > > at the existing bcm2835-audio driver code and was amused very much :)
> > > > 
> > > > So here is the result, a cleanup and fix patch series.
> > > > 
> > > > Most of the patches are trivial cleanups, just brushing up, removing
> > > > many redundant and buggy codes, as well as code simplifications.
> > > > 
> > > > A big functional change is that now it uses non-atomic PCM ops, so
> > > > that we can kill the ugly workqueue usages.  Also, the resource
> > > > management was simplified.
> > > 
> > > first of all, thank you very much for this series.
> > > 
> > > Eric has no time as maintainer, so i will try to give you some feedback (beware of very little audio driver knowledge).
> > > 
> > > I functionally tested your patch series on a Raspberry Pi 1 B (bcm2835_defconfig), so this whole series is at least:
> > > 
> > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> > 
> > OK, thanks, I'll put to my series in case of resubmission.
> > Meanwhile I'll keep the series in topic/vc04 branch of sound.git
> > tree.
> 
> does it mean this series should go through your tree instead of Greg's?

I don't mind either way.  I just wanted to expose the latest patchset
in git tree in case anyone needing more reviews.
The merge route can be decided later.


> > > Unfortunately there is still an corruption issue with underlying vchiq and multi_v7_defconfig, so no wider tests.
> > 
> > What is this corruption issue?
> 
> Actual there are two of them.
> 
> First one are incompatibilities of older VC4 firmware with commit 14dd37fc7b65 ("staging: vc04_services: Remove cache-line-size property (v3)"). There is a pull request for the Foundation kernel which hasn't been upstreamed yet [1].
> 
> The second is documented here [2].
> 
> [1] - https://github.com/raspberrypi/linux/pull/2666
> [2] - https://github.com/lategoodbye/rpi-zero/issues/23

OK, thanks.


> > > I don't know if you tested this series on a Raspberry Pi. Maybe you have some specific scenarios, which should be tested.
> > 
> > I have only a RPi3B+, and that's all what I've tested.
> 
> Great, just of curiosity which config did you use?

It's SLE / openSUSE.  The kernel is almost vanilla 4.18.5 with my
patches.

> > It'd be great if the patch series could be tested in a wider range of
> > models, of course.
> > 
> > The patches are only about cleanups.  They corrected the bad usages of
> > audio APIs and its design, but basically I haven't touched the basic
> > functionality intentionally at all.  So the behavior should be kept as
> > before.
> > 
> > (Actually it'd be better to revisit the design later, especially about
> > the multi-cards option and the PCM route mixer control, but I left as
> > is for compatibility reason for now.)
> > 
> > > As a reviewer i have some suggestions, but only trivia. I don't know if it's a problem that this series hasn't been send to devel@driverdev.osuosl.org
> > 
> > No, it's just because that address isn't found in MAINTAINERS file.
> > If it should go through it, please correct the entry at first :)
> 
> No, this is the mailing list for all staging driver. It is reported by get_maintainers.pl

Ah, I see.


thanks,

Takashi

Re: [PATCH 00/29] staging: bcm2835-audio: Cleanups and fixes

[Greg Kroah-Hartman]

On Sat, Sep 08, 2018 at 06:21:55PM +0200, Takashi Iwai wrote:
> On Sat, 08 Sep 2018 15:18:10 +0200,
> Stefan Wahren wrote:
> > 
> > Hi Takashi,
> > 
> > > Takashi Iwai <tiwai@suse.de> hat am 4. September 2018 um 17:58 geschrieben:
> > > 
> > > 
> > > Hi,
> > > 
> > > since I had an opportunity to play with RPi3B+ recently, I took a look
> > > at the existing bcm2835-audio driver code and was amused very much :)
> > > 
> > > So here is the result, a cleanup and fix patch series.
> > > 
> > > Most of the patches are trivial cleanups, just brushing up, removing
> > > many redundant and buggy codes, as well as code simplifications.
> > > 
> > > A big functional change is that now it uses non-atomic PCM ops, so
> > > that we can kill the ugly workqueue usages.  Also, the resource
> > > management was simplified.
> > 
> > first of all, thank you very much for this series.
> > 
> > Eric has no time as maintainer, so i will try to give you some feedback (beware of very little audio driver knowledge).
> > 
> > I functionally tested your patch series on a Raspberry Pi 1 B (bcm2835_defconfig), so this whole series is at least:
> > 
> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> 
> OK, thanks, I'll put to my series in case of resubmission.
> Meanwhile I'll keep the series in topic/vc04 branch of sound.git
> tree.

I'll just take this through my tree now, so there aren't merge issues if
there happens to be other cleanups to the driver that show up in the
near future.

thanks,

greg k-h

Re: [PATCH 00/29] staging: bcm2835-audio: Cleanups and fixes

[Takashi Iwai]

On Mon, 10 Sep 2018 11:12:36 +0200,
Greg Kroah-Hartman wrote:
> 
> On Sat, Sep 08, 2018 at 06:21:55PM +0200, Takashi Iwai wrote:
> > On Sat, 08 Sep 2018 15:18:10 +0200,
> > Stefan Wahren wrote:
> > > 
> > > Hi Takashi,
> > > 
> > > > Takashi Iwai <tiwai@suse.de> hat am 4. September 2018 um 17:58 geschrieben:
> > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > since I had an opportunity to play with RPi3B+ recently, I took a look
> > > > at the existing bcm2835-audio driver code and was amused very much :)
> > > > 
> > > > So here is the result, a cleanup and fix patch series.
> > > > 
> > > > Most of the patches are trivial cleanups, just brushing up, removing
> > > > many redundant and buggy codes, as well as code simplifications.
> > > > 
> > > > A big functional change is that now it uses non-atomic PCM ops, so
> > > > that we can kill the ugly workqueue usages.  Also, the resource
> > > > management was simplified.
> > > 
> > > first of all, thank you very much for this series.
> > > 
> > > Eric has no time as maintainer, so i will try to give you some feedback (beware of very little audio driver knowledge).
> > > 
> > > I functionally tested your patch series on a Raspberry Pi 1 B (bcm2835_defconfig), so this whole series is at least:
> > > 
> > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> > 
> > OK, thanks, I'll put to my series in case of resubmission.
> > Meanwhile I'll keep the series in topic/vc04 branch of sound.git
> > tree.
> 
> I'll just take this through my tree now, so there aren't merge issues if
> there happens to be other cleanups to the driver that show up in the
> near future.

OK, thanks!


Takashi

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint

[Stefan Wahren]

Hi Takashi,

Am 04.09.2018 um 17:58 schrieb Takashi Iwai:
> It seems that the resolution of vc04 callback is in 10 msec; i.e. the
> minimal period size is also 10 msec.
>
> This patch adds the corresponding hw constraint.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> index 9659c25b9f9d..6d89db6e14e4 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> @@ -145,6 +145,11 @@ static int snd_bcm2835_playback_open_generic(
>  				   SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
>  				   16);
>  
> +	/* position update is in 10ms order */
> +	snd_pcm_hw_constraint_minmax(runtime,
> +				     SNDRV_PCM_HW_PARAM_PERIOD_TIME,
> +				     10 * 1000, UINT_MAX);
> +
>  	chip->alsa_stream[idx] = alsa_stream;
>  
>  	chip->opened |= (1 << idx);

in the Foundation Kernel (Downstream) there is a patch to interpolate
the audio delay. So my questions is, does your patch above makes the
following patch obsolete?

[PATCH] bcm2835: interpolate audio delay

It appears the GPU only sends us a message all 10ms to update
the playback progress. Other than this, the playback position
(what SNDRV_PCM_IOCTL_DELAY will return) is not updated at all.
Userspace will see jitter up to 10ms in the audio position.

Make this a bit nicer for userspace by interpolating the
position using the CPU clock.

I'm not sure if setting snd_pcm_runtime.delay is the right
approach for this. Or if there is maybe an already existing
mechanism for position interpolation in the ALSA core.

I only set SNDRV_PCM_INFO_BATCH because this appears to remove
at least one situation snd_pcm_runtime.delay is used, so I have
to worry less in which place I have to update this field, or
how it interacts with the rest of ALSA.

In the future, it might be nice to use VC_AUDIO_MSG_TYPE_LATENCY.
One problem is that it requires sending a videocore message, and
waiting for a reply, which could make the implementation much
harder due to locking and synchronization requirements.
---
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c       | 13 +++++++++++--
 .../staging/vc04_services/bcm2835-audio/bcm2835.h   |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index 94654c0c7bba5..36165a60fb059 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -22,7 +22,7 @@
 /* hardware definition */
 static const struct snd_pcm_hardware snd_bcm2835_playback_hw = {
 	.info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
-	SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID),
+	SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_BATCH),
 	.formats = SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE,
 	.rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_8000_48000,
 	.rate_min = 8000,
@@ -93,6 +93,8 @@ void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream)
 		alsa_stream->pos %= alsa_stream->buffer_size;
 	}
 
+	alsa_stream->interpolate_start = ktime_get_ns();
+
 	if (alsa_stream->substream) {
 		if (new_period)
 			snd_pcm_period_elapsed(alsa_stream->substream);
@@ -323,6 +325,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
 	alsa_stream->buffer_size = snd_pcm_lib_buffer_bytes(substream);
 	alsa_stream->period_size = snd_pcm_lib_period_bytes(substream);
 	alsa_stream->pos = 0;
+	alsa_stream->interpolate_start = ktime_get_ns();
 
 	audio_debug("buffer_size=%d, period_size=%d pos=%d frame_bits=%d\n",
 		alsa_stream->buffer_size, alsa_stream->period_size,
@@ -415,13 +418,19 @@ snd_bcm2835_pcm_pointer(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct bcm2835_alsa_stream *alsa_stream = runtime->private_data;
-
+	u64 now = ktime_get_ns();
 
 	audio_debug("pcm_pointer... (%d) hwptr=%d appl=%d pos=%d\n", 0,
 		frames_to_bytes(runtime, runtime->status->hw_ptr),
 		frames_to_bytes(runtime, runtime->control->appl_ptr),
 		alsa_stream->pos);
 
+	/* Give userspace better delay reporting by interpolating between GPU
+	 * notifications, assuming audio speed is close enough to the clock
+	 * used for ktime */
+	if (alsa_stream->interpolate_start && alsa_stream->interpolate_start < now)
+		runtime->delay = -(int)div_u64((now - alsa_stream->interpolate_start) * runtime->rate,  1000000000);
+
 	return snd_pcm_indirect_playback_pointer(substream,
 		&alsa_stream->pcm_indirect,
 		alsa_stream->pos);
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
index 379604d3554e8..e64862e1781be 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h
@@ -137,6 +137,7 @@ struct bcm2835_alsa_stream {
 	unsigned int pos;
 	unsigned int buffer_size;
 	unsigned int period_size;
+	u64 interpolate_start;
 
 	atomic_t retrieved;
 	struct bcm2835_audio_instance *instance;

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint

[Takashi Iwai]

On Wed, 19 Sep 2018 11:42:22 +0200,
Stefan Wahren wrote:
> 
> Hi Takashi,
> 
> Am 04.09.2018 um 17:58 schrieb Takashi Iwai:
> > It seems that the resolution of vc04 callback is in 10 msec; i.e. the
> > minimal period size is also 10 msec.
> >
> > This patch adds the corresponding hw constraint.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> > index 9659c25b9f9d..6d89db6e14e4 100644
> > --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> > +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> > @@ -145,6 +145,11 @@ static int snd_bcm2835_playback_open_generic(
> >  				   SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
> >  				   16);
> >  
> > +	/* position update is in 10ms order */
> > +	snd_pcm_hw_constraint_minmax(runtime,
> > +				     SNDRV_PCM_HW_PARAM_PERIOD_TIME,
> > +				     10 * 1000, UINT_MAX);
> > +
> >  	chip->alsa_stream[idx] = alsa_stream;
> >  
> >  	chip->opened |= (1 << idx);
> 
> in the Foundation Kernel (Downstream) there is a patch to interpolate
> the audio delay. So my questions is, does your patch above makes the
> following patch obsolete?

Through a quick glance, no, my patch is orthogonal to this.

My patch adds a PCM hw constraint so that the period size won't go
below 10ms, while the downstream patch provides the additional delay
value that is calculated from the system clock.

> [PATCH] bcm2835: interpolate audio delay
> 
> It appears the GPU only sends us a message all 10ms to update
> the playback progress. Other than this, the playback position
> (what SNDRV_PCM_IOCTL_DELAY will return) is not updated at all.
> Userspace will see jitter up to 10ms in the audio position.
> 
> Make this a bit nicer for userspace by interpolating the
> position using the CPU clock.
> 
> I'm not sure if setting snd_pcm_runtime.delay is the right
> approach for this. Or if there is maybe an already existing
> mechanism for position interpolation in the ALSA core.

That's OK, as long as the computation is accurate enough (at least not
exceed the actual position) and is light-weight.

> I only set SNDRV_PCM_INFO_BATCH because this appears to remove
> at least one situation snd_pcm_runtime.delay is used, so I have
> to worry less in which place I have to update this field, or
> how it interacts with the rest of ALSA.

Actually, this SNDRV_PCM_INFO_BATCH addition should be a separate
patch.  It has nothing to do with the runtime->delay calculation.
(And, this "one situation" is likely called PulseAudio :)

> In the future, it might be nice to use VC_AUDIO_MSG_TYPE_LATENCY.
> One problem is that it requires sending a videocore message, and
> waiting for a reply, which could make the implementation much
> harder due to locking and synchronization requirements.

This can be now easy with my patch series.  By switching to non-atomic
operation, we can issue the vc04 command inside the pointer callback,
too.


thanks,

Takashi

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint

[Stefan Wahren]

Hi,

[add Phil and Mike]

Am 19.09.2018 um 11:52 schrieb Takashi Iwai:
> On Wed, 19 Sep 2018 11:42:22 +0200,
> Stefan Wahren wrote:
>> Hi Takashi,
>>
>> Am 04.09.2018 um 17:58 schrieb Takashi Iwai:
>>> It seems that the resolution of vc04 callback is in 10 msec; i.e. the
>>> minimal period size is also 10 msec.
>>>
>>> This patch adds the corresponding hw constraint.
>>>
>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>> ---
>>>  drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>> index 9659c25b9f9d..6d89db6e14e4 100644
>>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>> @@ -145,6 +145,11 @@ static int snd_bcm2835_playback_open_generic(
>>>  				   SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
>>>  				   16);
>>>  
>>> +	/* position update is in 10ms order */
>>> +	snd_pcm_hw_constraint_minmax(runtime,
>>> +				     SNDRV_PCM_HW_PARAM_PERIOD_TIME,
>>> +				     10 * 1000, UINT_MAX);
>>> +
>>>  	chip->alsa_stream[idx] = alsa_stream;
>>>  
>>>  	chip->opened |= (1 << idx);
>> in the Foundation Kernel (Downstream) there is a patch to interpolate
>> the audio delay. So my questions is, does your patch above makes the
>> following patch obsolete?
> Through a quick glance, no, my patch is orthogonal to this.
>
> My patch adds a PCM hw constraint so that the period size won't go
> below 10ms, while the downstream patch provides the additional delay
> value that is calculated from the system clock.

thanks for your explanation. So your patch must be reverted with
implementation of interpolate audio delay.

>
>> [PATCH] bcm2835: interpolate audio delay
>>
>> It appears the GPU only sends us a message all 10ms to update
>> the playback progress. Other than this, the playback position
>> (what SNDRV_PCM_IOCTL_DELAY will return) is not updated at all.
>> Userspace will see jitter up to 10ms in the audio position.
>>
>> Make this a bit nicer for userspace by interpolating the
>> position using the CPU clock.
>>
>> I'm not sure if setting snd_pcm_runtime.delay is the right
>> approach for this. Or if there is maybe an already existing
>> mechanism for position interpolation in the ALSA core.
> That's OK, as long as the computation is accurate enough (at least not
> exceed the actual position) and is light-weight.
>
>> I only set SNDRV_PCM_INFO_BATCH because this appears to remove
>> at least one situation snd_pcm_runtime.delay is used, so I have
>> to worry less in which place I have to update this field, or
>> how it interacts with the rest of ALSA.
> Actually, this SNDRV_PCM_INFO_BATCH addition should be a separate
> patch.  It has nothing to do with the runtime->delay calculation.
> (And, this "one situation" is likely called PulseAudio :)
>
>> In the future, it might be nice to use VC_AUDIO_MSG_TYPE_LATENCY.
>> One problem is that it requires sending a videocore message, and
>> waiting for a reply, which could make the implementation much
>> harder due to locking and synchronization requirements.
> This can be now easy with my patch series.  By switching to non-atomic
> operation, we can issue the vc04 command inside the pointer callback,
> too.

I think we should try to implement this later.

@Mike: Do you want to write a patch series which upstream "interpolate
audio delay" and addresses Takashi's comments?

I would help you, in case you have questions about setup a Raspberry Pi
with Mainline kernel or patch submission.

Regards
Stefan

>
>
> thanks,
>
> Takashi

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint

[Mike Brady]

Hi Stefan. Thanks for this.

> On 19 Sep 2018, at 13:41, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> 
> Hi,
> 
> [add Phil and Mike]
> 
> Am 19.09.2018 um 11:52 schrieb Takashi Iwai:
>> On Wed, 19 Sep 2018 11:42:22 +0200,
>> Stefan Wahren wrote:
>>> Hi Takashi,
>>> 
>>> Am 04.09.2018 um 17:58 schrieb Takashi Iwai:
>>>> It seems that the resolution of vc04 callback is in 10 msec; i.e. the
>>>> minimal period size is also 10 msec.
>>>> 
>>>> This patch adds the corresponding hw constraint.
>>>> 
>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>>> ---
>>>> drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +++++
>>>> 1 file changed, 5 insertions(+)
>>>> 
>>>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>>> index 9659c25b9f9d..6d89db6e14e4 100644
>>>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>>> @@ -145,6 +145,11 @@ static int snd_bcm2835_playback_open_generic(
>>>> 				   SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
>>>> 				   16);
>>>> 
>>>> +	/* position update is in 10ms order */
>>>> +	snd_pcm_hw_constraint_minmax(runtime,
>>>> +				     SNDRV_PCM_HW_PARAM_PERIOD_TIME,
>>>> +				     10 * 1000, UINT_MAX);
>>>> +
>>>> 	chip->alsa_stream[idx] = alsa_stream;
>>>> 
>>>> 	chip->opened |= (1 << idx);
>>> in the Foundation Kernel (Downstream) there is a patch to interpolate
>>> the audio delay. So my questions is, does your patch above makes the
>>> following patch obsolete?
>> Through a quick glance, no, my patch is orthogonal to this.
>> 
>> My patch adds a PCM hw constraint so that the period size won't go
>> below 10ms, while the downstream patch provides the additional delay
>> value that is calculated from the system clock.
> 
> thanks for your explanation. So your patch must be reverted with
> implementation of interpolate audio delay.
> 
>> 
>>> [PATCH] bcm2835: interpolate audio delay
>>> 
>>> It appears the GPU only sends us a message all 10ms to update
>>> the playback progress. Other than this, the playback position
>>> (what SNDRV_PCM_IOCTL_DELAY will return) is not updated at all.
>>> Userspace will see jitter up to 10ms in the audio position.
>>> 
>>> Make this a bit nicer for userspace by interpolating the
>>> position using the CPU clock.
>>> 
>>> I'm not sure if setting snd_pcm_runtime.delay is the right
>>> approach for this. Or if there is maybe an already existing
>>> mechanism for position interpolation in the ALSA core.
>> That's OK, as long as the computation is accurate enough (at least not
>> exceed the actual position) and is light-weight.
>> 
>>> I only set SNDRV_PCM_INFO_BATCH because this appears to remove
>>> at least one situation snd_pcm_runtime.delay is used, so I have
>>> to worry less in which place I have to update this field, or
>>> how it interacts with the rest of ALSA.
>> Actually, this SNDRV_PCM_INFO_BATCH addition should be a separate
>> patch.  It has nothing to do with the runtime->delay calculation.
>> (And, this "one situation" is likely called PulseAudio :)
>> 
>>> In the future, it might be nice to use VC_AUDIO_MSG_TYPE_LATENCY.
>>> One problem is that it requires sending a videocore message, and
>>> waiting for a reply, which could make the implementation much
>>> harder due to locking and synchronization requirements.
>> This can be now easy with my patch series.  By switching to non-atomic
>> operation, we can issue the vc04 command inside the pointer callback,
>> too.
> 
> I think we should try to implement this later.
> 
> @Mike: Do you want to write a patch series which upstream "interpolate
> audio delay" and addresses Takashi's comments?
> 
> I would help you, in case you have questions about setup a Raspberry Pi
> with Mainline kernel or patch submission.

Yeah, sure. I might need some of the handholding alright. Can you point me at any documentation please?
Regards
Mike

> 
> Regards
> Stefan
> 
>> 
>> 
>> thanks,
>> 
>> Takashi
> 
> 

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint

[Takashi Iwai]

On Wed, 19 Sep 2018 14:41:28 +0200,
Stefan Wahren wrote:
> 
> Hi,
> 
> [add Phil and Mike]
> 
> Am 19.09.2018 um 11:52 schrieb Takashi Iwai:
> > On Wed, 19 Sep 2018 11:42:22 +0200,
> > Stefan Wahren wrote:
> >> Hi Takashi,
> >>
> >> Am 04.09.2018 um 17:58 schrieb Takashi Iwai:
> >>> It seems that the resolution of vc04 callback is in 10 msec; i.e. the
> >>> minimal period size is also 10 msec.
> >>>
> >>> This patch adds the corresponding hw constraint.
> >>>
> >>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>> ---
> >>>  drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> >>> index 9659c25b9f9d..6d89db6e14e4 100644
> >>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> >>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
> >>> @@ -145,6 +145,11 @@ static int snd_bcm2835_playback_open_generic(
> >>>  				   SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
> >>>  				   16);
> >>>  
> >>> +	/* position update is in 10ms order */
> >>> +	snd_pcm_hw_constraint_minmax(runtime,
> >>> +				     SNDRV_PCM_HW_PARAM_PERIOD_TIME,
> >>> +				     10 * 1000, UINT_MAX);
> >>> +
> >>>  	chip->alsa_stream[idx] = alsa_stream;
> >>>  
> >>>  	chip->opened |= (1 << idx);
> >> in the Foundation Kernel (Downstream) there is a patch to interpolate
> >> the audio delay. So my questions is, does your patch above makes the
> >> following patch obsolete?
> > Through a quick glance, no, my patch is orthogonal to this.
> >
> > My patch adds a PCM hw constraint so that the period size won't go
> > below 10ms, while the downstream patch provides the additional delay
> > value that is calculated from the system clock.
> 
> thanks for your explanation. So your patch must be reverted with
> implementation of interpolate audio delay.

No, no.
Both can be applied as is.  They have *nothing to do* with each
other.


> >> [PATCH] bcm2835: interpolate audio delay
> >>
> >> It appears the GPU only sends us a message all 10ms to update
> >> the playback progress. Other than this, the playback position
> >> (what SNDRV_PCM_IOCTL_DELAY will return) is not updated at all.
> >> Userspace will see jitter up to 10ms in the audio position.
> >>
> >> Make this a bit nicer for userspace by interpolating the
> >> position using the CPU clock.
> >>
> >> I'm not sure if setting snd_pcm_runtime.delay is the right
> >> approach for this. Or if there is maybe an already existing
> >> mechanism for position interpolation in the ALSA core.
> > That's OK, as long as the computation is accurate enough (at least not
> > exceed the actual position) and is light-weight.
> >
> >> I only set SNDRV_PCM_INFO_BATCH because this appears to remove
> >> at least one situation snd_pcm_runtime.delay is used, so I have
> >> to worry less in which place I have to update this field, or
> >> how it interacts with the rest of ALSA.
> > Actually, this SNDRV_PCM_INFO_BATCH addition should be a separate
> > patch.  It has nothing to do with the runtime->delay calculation.
> > (And, this "one situation" is likely called PulseAudio :)
> >
> >> In the future, it might be nice to use VC_AUDIO_MSG_TYPE_LATENCY.
> >> One problem is that it requires sending a videocore message, and
> >> waiting for a reply, which could make the implementation much
> >> harder due to locking and synchronization requirements.
> > This can be now easy with my patch series.  By switching to non-atomic
> > operation, we can issue the vc04 command inside the pointer callback,
> > too.
> 
> I think we should try to implement this later.
> 
> @Mike: Do you want to write a patch series which upstream "interpolate
> audio delay" and addresses Takashi's comments?
> 
> I would help you, in case you have questions about setup a Raspberry Pi
> with Mainline kernel or patch submission.

Well, the question is who really wants this.  The value given by that
patch is nothing but some estimation and might be even incorrect.

PulseAudio won't need it any longer when you set the BATCH flag.
Then it'll switch from tsched mode to the old mode, and the delay
value would be almost irrelevant.


Takashi

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint [Resend in plain text...]

[Mike Brady]

Hi there. Apologies for the delay. The issue here is not the 10ms period constrain -- it’s the possible addition of code to interpolate the playback position between GPU-driven updates. The intention is to give userland a jitter-free view of the playback position.

> On 19 Sep 2018, at 19:39, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Wed, 19 Sep 2018 14:41:28 +0200,
> Stefan Wahren wrote:
>> 
>> Hi,
>> 
>> [add Phil and Mike]
>> 
>> Am 19.09.2018 um 11:52 schrieb Takashi Iwai:
>>> On Wed, 19 Sep 2018 11:42:22 +0200,
>>> Stefan Wahren wrote:
>>>> Hi Takashi,
>>>> 
>>>> Am 04.09.2018 um 17:58 schrieb Takashi Iwai:
>>>>> It seems that the resolution of vc04 callback is in 10 msec; i.e. the
>>>>> minimal period size is also 10 msec.
>>>>> 
>>>>> This patch adds the corresponding hw constraint.
>>>>> 
>>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>>>> ---
>>>>> drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +++++
>>>>> 1 file changed, 5 insertions(+)
>>>>> 
>>>>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>>>> index 9659c25b9f9d..6d89db6e14e4 100644
>>>>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>>>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
>>>>> @@ -145,6 +145,11 @@ static int snd_bcm2835_playback_open_generic(
>>>>> 				   SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
>>>>> 				   16);
>>>>> 
>>>>> +	/* position update is in 10ms order */
>>>>> +	snd_pcm_hw_constraint_minmax(runtime,
>>>>> +				     SNDRV_PCM_HW_PARAM_PERIOD_TIME,
>>>>> +				     10 * 1000, UINT_MAX);
>>>>> +
>>>>> 	chip->alsa_stream[idx] = alsa_stream;
>>>>> 
>>>>> 	chip->opened |= (1 << idx);
>>>> in the Foundation Kernel (Downstream) there is a patch to interpolate
>>>> the audio delay. So my questions is, does your patch above makes the
>>>> following patch obsolete?
>>> Through a quick glance, no, my patch is orthogonal to this.
>>> 
>>> My patch adds a PCM hw constraint so that the period size won't go
>>> below 10ms, while the downstream patch provides the additional delay
>>> value that is calculated from the system clock.
>> 
>> thanks for your explanation. So your patch must be reverted with
>> implementation of interpolate audio delay.
> 
> No, no.
> Both can be applied as is.  They have *nothing to do* with each
> other.

Agreed. The patches address different issues.

> [PATCH] bcm2835: interpolate audio delay
>>>> 
>>>> It appears the GPU only sends us a message all 10ms to update
>>>> the playback progress. Other than this, the playback position
>>>> (what SNDRV_PCM_IOCTL_DELAY will return) is not updated at all.
>>>> Userspace will see jitter up to 10ms in the audio position.
>>>> 
>>>> Make this a bit nicer for userspace by interpolating the
>>>> position using the CPU clock.
>>>> 
>>>> I'm not sure if setting snd_pcm_runtime.delay is the right
>>>> approach for this. Or if there is maybe an already existing
>>>> mechanism for position interpolation in the ALSA core.
>>> That's OK, as long as the computation is accurate enough (at least not
>>> exceed the actual position) and is light-weight.
>>> 
>>>> I only set SNDRV_PCM_INFO_BATCH because this appears to remove
>>>> at least one situation snd_pcm_runtime.delay is used, so I have
>>>> to worry less in which place I have to update this field, or
>>>> how it interacts with the rest of ALSA.
>>> Actually, this SNDRV_PCM_INFO_BATCH addition should be a separate
>>> patch.  It has nothing to do with the runtime->delay calculation.
>>> (And, this "one situation" is likely called PulseAudio :)
>>> 
>>>> In the future, it might be nice to use VC_AUDIO_MSG_TYPE_LATENCY.
>>>> One problem is that it requires sending a videocore message, and
>>>> waiting for a reply, which could make the implementation much
>>>> harder due to locking and synchronization requirements.
>>> This can be now easy with my patch series.  By switching to non-atomic
>>> operation, we can issue the vc04 command inside the pointer callback,
>>> too.
>> 
>> I think we should try to implement this later.
>> 
>> @Mike: Do you want to write a patch series which upstream "interpolate
>> audio delay" and addresses Takashi's comments?
>> 
>> I would help you, in case you have questions about setup a Raspberry Pi
>> with Mainline kernel or patch submission.
> 
> Well, the question is who really wants this.  The value given by that
> patch is nothing but some estimation and might be even incorrect.
> 
> PulseAudio won't need it any longer when you set the BATCH flag.
> Then it'll switch from tsched mode to the old mode, and the delay
> value would be almost irrelevant.

Well, two answers. First, Shairport Sync (https://github.com/mikebrady/shairport-sync) needs it — whenever a packet of audio frames is about to be added to the output queue (at approximately 7.9 millisecond intervals), the delay is checked to try to maintain sync to within a few milliseconds. The BCM2835 audio device is the only one I have yet come across with so much jitter. Whatever other drivers do, the delay they report doesn’t suffer from anything like this level of jitter.

The second answer is that the veracity of the ALSA documentation depends on it — any application using the ALSA system for synchronisation will rely on this being an accurate reflection of the situation. AFAIK there is really no workaround it if the application is confined to “safe” ALSA (http://0pointer.de/blog/projects/guide-to-sound-apis).

On LMKL.org, Takashi wrote:

> Date	Wed, 19 Sep 2018 11:52:33 +0200
> From	Takashi Iwai <>
> Subject	Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint

> [snip]

> That's OK, as long as the computation is accurate enough (at least not
> exceed the actual position) and is light-weight.

> [snip]

The overhead is small -- an extra ktime_get() every time a GPU message
is sent -- and another call and a few calculations whenever the delay
is sought from userland.

At 48,000 frames per second, i.e. approximately 20 microseconds per frame, it would take a clock inaccuracy of roughly
20 microseconds in 10 milliseconds -- 2,000 parts per million — to result in an inaccurate estimate.
Crystal or resonator-based clocks typically have an inaccuracy of 10s to 100s of parts per million.

Finally, to see the effect of the absence and presence of this interpolation, please have a look at this: https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016, where a downstream version of this fix was being discussed.

Best wishes
Mike Brady

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint [Resend in plain text...]

[Takashi Iwai]

On Tue, 09 Oct 2018 15:18:15 +0200,
Mike Brady wrote:
> 
> >> @Mike: Do you want to write a patch series which upstream "interpolate
> >> audio delay" and addresses Takashi's comments?
> >> 
> >> I would help you, in case you have questions about setup a Raspberry Pi
> >> with Mainline kernel or patch submission.
> > 
> > Well, the question is who really wants this.  The value given by that
> > patch is nothing but some estimation and might be even incorrect.
> > 
> > PulseAudio won't need it any longer when you set the BATCH flag.
> > Then it'll switch from tsched mode to the old mode, and the delay
> > value would be almost irrelevant.
> 
> Well, two answers. First, Shairport Sync
> (https://github.com/mikebrady/shairport-sync) needs it — whenever a
> packet of audio frames is about to be added to the output queue (at
> approximately 7.9 millisecond intervals), the delay is checked to
> try to maintain sync to within a few milliseconds. The BCM2835 audio
> device is the only one I have yet come across with so much
> jitter. Whatever other drivers do, the delay they report doesn’t
> suffer from anything like this level of jitter.

OK, if there is another application using that delay value, it's worth
to consider providing a fine-grained value.

> The second answer is that the veracity of the ALSA documentation
> depends on it — any application using the ALSA system for
> synchronisation will rely on this being an accurate reflection of
> the situation. AFAIK there is really no workaround it if the
> application is confined to “safe” ALSA
> (http://0pointer.de/blog/projects/guide-to-sound-apis).

> On LMKL.org, Takashi wrote:
> 
> > Date	Wed, 19 Sep 2018 11:52:33 +0200
> > From	Takashi Iwai <>
> > Subject	Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint
> 
> > [snip]
> 
> > That's OK, as long as the computation is accurate enough (at least not
> > exceed the actual position) and is light-weight.
> 
> > [snip]
> 
> The overhead is small -- an extra ktime_get() every time a GPU message
> is sent -- and another call and a few calculations whenever the delay
> is sought from userland.
> 
> At 48,000 frames per second, i.e. approximately 20 microseconds per
> frame, it would take a clock inaccuracy of roughly
> 20 microseconds in 10 milliseconds -- 2,000 parts per million — to
> result in an inaccurate estimate. 
> Crystal or resonator-based clocks typically have an inaccuracy of
> 10s to 100s of parts per million.
> 
> Finally, to see the effect of the absence and presence of this
> interpolation, please have a look at this:
> https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016,
> where a downstream version of this fix was being discussed.

I'm not opposing to the usage of delay value.  The attribute is
provided exactly for such a purpose.  It's a good thing (tm).

The potential problem is, however, rather the implementation: it's
using a system timer for interpolation, which is known to drift from
the actual clocks.  Though, one may say that in such a use case, we
may ignore the drift since the interpolation is so narrow.

But another question is whether it should be implemented in each
driver level.  The time-stamping is basically a PCM core
functionality, and nothing specific to the hardware, especially when
it's referring to the system timer.

e.g. you can think in a different way, too: we may put a timestamp at
each hwptr update, and pass it as-is, instead of updating the
timestamp at each position query.  This will effectively gives the
accurate position-timestamp pair, and user-space may interpolate as it
likes, too.


In anyway, if *this* kind of feature needs to be merged, it's
definitely to be discussed with the upstream.  So, if you're going to
merge that sort of path, please keep Cc to alsa-devel ML.


thanks,

Takashi

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint [Resend in plain text...]

[Mike Brady]

Hi Takashi.

> On 9 Oct 2018, at 14:44, Takashi Iwai <tiwai@suse.de> wrote:
> 
> On Tue, 09 Oct 2018 15:18:15 +0200,
> Mike Brady wrote:
>> 
>>>> @Mike: Do you want to write a patch series which upstream "interpolate
>>>> audio delay" and addresses Takashi's comments?
>>>> 
>>>> I would help you, in case you have questions about setup a Raspberry Pi
>>>> with Mainline kernel or patch submission.
>>> 
>>> Well, the question is who really wants this.  The value given by that
>>> patch is nothing but some estimation and might be even incorrect.
>>> 
>>> PulseAudio won't need it any longer when you set the BATCH flag.
>>> Then it'll switch from tsched mode to the old mode, and the delay
>>> value would be almost irrelevant.
>> 
>> Well, two answers. First, Shairport Sync
>> (https://github.com/mikebrady/shairport-sync) needs it — whenever a
>> packet of audio frames is about to be added to the output queue (at
>> approximately 7.9 millisecond intervals), the delay is checked to
>> try to maintain sync to within a few milliseconds. The BCM2835 audio
>> device is the only one I have yet come across with so much
>> jitter. Whatever other drivers do, the delay they report doesn’t
>> suffer from anything like this level of jitter.
> 
> OK, if there is another application using that delay value, it's worth
> to consider providing a fine-grained value.
> 
>> The second answer is that the veracity of the ALSA documentation
>> depends on it — any application using the ALSA system for
>> synchronisation will rely on this being an accurate reflection of
>> the situation. AFAIK there is really no workaround it if the
>> application is confined to “safe” ALSA
>> (http://0pointer.de/blog/projects/guide-to-sound-apis).
> 
>> On LMKL.org, Takashi wrote:
>> 
>>> Date	Wed, 19 Sep 2018 11:52:33 +0200
>>> From	Takashi Iwai <>
>>> Subject	Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint
>> 
>>> [snip]
>> 
>>> That's OK, as long as the computation is accurate enough (at least not
>>> exceed the actual position) and is light-weight.
>> 
>>> [snip]
>> 
>> The overhead is small -- an extra ktime_get() every time a GPU message
>> is sent -- and another call and a few calculations whenever the delay
>> is sought from userland.
>> 
>> At 48,000 frames per second, i.e. approximately 20 microseconds per
>> frame, it would take a clock inaccuracy of roughly
>> 20 microseconds in 10 milliseconds -- 2,000 parts per million — to
>> result in an inaccurate estimate. 
>> Crystal or resonator-based clocks typically have an inaccuracy of
>> 10s to 100s of parts per million.
>> 
>> Finally, to see the effect of the absence and presence of this
>> interpolation, please have a look at this:
>> https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016,
>> where a downstream version of this fix was being discussed.
> 
> I'm not opposing to the usage of delay value.  The attribute is
> provided exactly for such a purpose.  It's a good thing (tm).
> 
> The potential problem is, however, rather the implementation: it's
> using a system timer for interpolation, which is known to drift from
> the actual clocks.  Though, one may say that in such a use case, we
> may ignore the drift since the interpolation is so narrow.

Yes, that was my thought. I guess another thing in its favour is that this audio device will always
be in partnership with a processor as part of an SoC, so it will always be likely to have a reasonably
accurate clock.

> But another question is whether it should be implemented in each
> driver level.  The time-stamping is basically a PCM core
> functionality, and nothing specific to the hardware, especially when
> it's referring to the system timer.

That’s a fair point. I don’t know what is done in other drivers, but can only report that with one possible exception,
the DACs used with Shairport Sync by many end users report well-behaved delay figures, certainly to within two microseconds. I’m afraid I don’t know how they do it.

> e.g. you can think in a different way, too: we may put a timestamp at
> each hwptr update, and pass it as-is, instead of updating the
> timestamp at each position query.  This will effectively gives the
> accurate position-timestamp pair, and user-space may interpolate as it
> likes, too.

That’s not a bad idea, and I might take it up on the alsa-devel mailing list, as you suggest.

> In anyway, if *this* kind of feature needs to be merged, it's
> definitely to be discussed with the upstream.  So, if you're going to
> merge that sort of path, please keep Cc to alsa-devel ML.

In the meantime, would you think that the balance of convenience lies with this interpolation scheme? (Finally, I have a patch ready….)
Regards
Mike

> 
> thanks,
> 
> Takashi

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint [Resend in plain text...]

[Takashi Iwai]

On Tue, 09 Oct 2018 17:28:36 +0200,
Mike Brady wrote:
> 
> Hi Takashi.
> 
> > On 9 Oct 2018, at 14:44, Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > On Tue, 09 Oct 2018 15:18:15 +0200,
> > Mike Brady wrote:
> >> 
> >>>> @Mike: Do you want to write a patch series which upstream "interpolate
> >>>> audio delay" and addresses Takashi's comments?
> >>>> 
> >>>> I would help you, in case you have questions about setup a Raspberry Pi
> >>>> with Mainline kernel or patch submission.
> >>> 
> >>> Well, the question is who really wants this.  The value given by that
> >>> patch is nothing but some estimation and might be even incorrect.
> >>> 
> >>> PulseAudio won't need it any longer when you set the BATCH flag.
> >>> Then it'll switch from tsched mode to the old mode, and the delay
> >>> value would be almost irrelevant.
> >> 
> >> Well, two answers. First, Shairport Sync
> >> (https://github.com/mikebrady/shairport-sync) needs it — whenever a
> >> packet of audio frames is about to be added to the output queue (at
> >> approximately 7.9 millisecond intervals), the delay is checked to
> >> try to maintain sync to within a few milliseconds. The BCM2835 audio
> >> device is the only one I have yet come across with so much
> >> jitter. Whatever other drivers do, the delay they report doesn’t
> >> suffer from anything like this level of jitter.
> > 
> > OK, if there is another application using that delay value, it's worth
> > to consider providing a fine-grained value.
> > 
> >> The second answer is that the veracity of the ALSA documentation
> >> depends on it — any application using the ALSA system for
> >> synchronisation will rely on this being an accurate reflection of
> >> the situation. AFAIK there is really no workaround it if the
> >> application is confined to “safe” ALSA
> >> (http://0pointer.de/blog/projects/guide-to-sound-apis).
> > 
> >> On LMKL.org, Takashi wrote:
> >> 
> >>> Date	Wed, 19 Sep 2018 11:52:33 +0200
> >>> From	Takashi Iwai <>
> >>> Subject	Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint
> >> 
> >>> [snip]
> >> 
> >>> That's OK, as long as the computation is accurate enough (at least not
> >>> exceed the actual position) and is light-weight.
> >> 
> >>> [snip]
> >> 
> >> The overhead is small -- an extra ktime_get() every time a GPU message
> >> is sent -- and another call and a few calculations whenever the delay
> >> is sought from userland.
> >> 
> >> At 48,000 frames per second, i.e. approximately 20 microseconds per
> >> frame, it would take a clock inaccuracy of roughly
> >> 20 microseconds in 10 milliseconds -- 2,000 parts per million — to
> >> result in an inaccurate estimate. 
> >> Crystal or resonator-based clocks typically have an inaccuracy of
> >> 10s to 100s of parts per million.
> >> 
> >> Finally, to see the effect of the absence and presence of this
> >> interpolation, please have a look at this:
> >> https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016,
> >> where a downstream version of this fix was being discussed.
> > 
> > I'm not opposing to the usage of delay value.  The attribute is
> > provided exactly for such a purpose.  It's a good thing (tm).
> > 
> > The potential problem is, however, rather the implementation: it's
> > using a system timer for interpolation, which is known to drift from
> > the actual clocks.  Though, one may say that in such a use case, we
> > may ignore the drift since the interpolation is so narrow.
> 
> Yes, that was my thought. I guess another thing in its favour is that this audio device will always
> be in partnership with a processor as part of an SoC, so it will always be likely to have a reasonably
> accurate clock.
> 
> > But another question is whether it should be implemented in each
> > driver level.  The time-stamping is basically a PCM core
> > functionality, and nothing specific to the hardware, especially when
> > it's referring to the system timer.
> 
> That’s a fair point. I don’t know what is done in other drivers, but can only report that with one possible exception,
> the DACs used with Shairport Sync by many end users report well-behaved delay figures, certainly to within two microseconds. I’m afraid I don’t know how they do it.
> 
> > e.g. you can think in a different way, too: we may put a timestamp at
> > each hwptr update, and pass it as-is, instead of updating the
> > timestamp at each position query.  This will effectively gives the
> > accurate position-timestamp pair, and user-space may interpolate as it
> > likes, too.
> 
> That’s not a bad idea, and I might take it up on the alsa-devel mailing list, as you suggest.
> 
> > In anyway, if *this* kind of feature needs to be merged, it's
> > definitely to be discussed with the upstream.  So, if you're going to
> > merge that sort of path, please keep Cc to alsa-devel ML.
> 
> In the meantime, would you think that the balance of convenience lies with this interpolation scheme? (Finally, I have a patch ready….)

I find it's not too bad, at least as a starting point.
It doesn't break anything dramatically from the existing API POV.

We may brush up things (or extend the API) later, and may move it up
to the core code.  But, again, as a kick off, it's good to start with
a local minimal change.


thanks,

Takashi

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint [Resend in plain text...]

[Mike Brady]

Hi Takashi. Just testing out the updated bcm2835 audio driver — it seems that it will underflow at somewhere above about 4410 and below 5120 frames, whereas the present driver is happy down to at least 2000 frames — I haven’t tried lower than about 1700.

Is this change meant to happen?

Regards
Mike


> On 9 Oct 2018, at 16:28, Mike Brady <mikebrady@eircom.net> wrote:
> 
> Hi Takashi.
> 
>> On 9 Oct 2018, at 14:44, Takashi Iwai <tiwai@suse.de> wrote:
>> 
>> On Tue, 09 Oct 2018 15:18:15 +0200,
>> Mike Brady wrote:
>>> 
>>>>> @Mike: Do you want to write a patch series which upstream "interpolate
>>>>> audio delay" and addresses Takashi's comments?
>>>>> 
>>>>> I would help you, in case you have questions about setup a Raspberry Pi
>>>>> with Mainline kernel or patch submission.
>>>> 
>>>> Well, the question is who really wants this.  The value given by that
>>>> patch is nothing but some estimation and might be even incorrect.
>>>> 
>>>> PulseAudio won't need it any longer when you set the BATCH flag.
>>>> Then it'll switch from tsched mode to the old mode, and the delay
>>>> value would be almost irrelevant.
>>> 
>>> Well, two answers. First, Shairport Sync
>>> (https://github.com/mikebrady/shairport-sync) needs it — whenever a
>>> packet of audio frames is about to be added to the output queue (at
>>> approximately 7.9 millisecond intervals), the delay is checked to
>>> try to maintain sync to within a few milliseconds. The BCM2835 audio
>>> device is the only one I have yet come across with so much
>>> jitter. Whatever other drivers do, the delay they report doesn’t
>>> suffer from anything like this level of jitter.
>> 
>> OK, if there is another application using that delay value, it's worth
>> to consider providing a fine-grained value.
>> 
>>> The second answer is that the veracity of the ALSA documentation
>>> depends on it — any application using the ALSA system for
>>> synchronisation will rely on this being an accurate reflection of
>>> the situation. AFAIK there is really no workaround it if the
>>> application is confined to “safe” ALSA
>>> (http://0pointer.de/blog/projects/guide-to-sound-apis).
>> 
>>> On LMKL.org, Takashi wrote:
>>> 
>>>> Date	Wed, 19 Sep 2018 11:52:33 +0200
>>>> From	Takashi Iwai <>
>>>> Subject	Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint
>>> 
>>>> [snip]
>>> 
>>>> That's OK, as long as the computation is accurate enough (at least not
>>>> exceed the actual position) and is light-weight.
>>> 
>>>> [snip]
>>> 
>>> The overhead is small -- an extra ktime_get() every time a GPU message
>>> is sent -- and another call and a few calculations whenever the delay
>>> is sought from userland.
>>> 
>>> At 48,000 frames per second, i.e. approximately 20 microseconds per
>>> frame, it would take a clock inaccuracy of roughly
>>> 20 microseconds in 10 milliseconds -- 2,000 parts per million — to
>>> result in an inaccurate estimate. 
>>> Crystal or resonator-based clocks typically have an inaccuracy of
>>> 10s to 100s of parts per million.
>>> 
>>> Finally, to see the effect of the absence and presence of this
>>> interpolation, please have a look at this:
>>> https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016,
>>> where a downstream version of this fix was being discussed.
>> 
>> I'm not opposing to the usage of delay value.  The attribute is
>> provided exactly for such a purpose.  It's a good thing (tm).
>> 
>> The potential problem is, however, rather the implementation: it's
>> using a system timer for interpolation, which is known to drift from
>> the actual clocks.  Though, one may say that in such a use case, we
>> may ignore the drift since the interpolation is so narrow.
> 
> Yes, that was my thought. I guess another thing in its favour is that this audio device will always
> be in partnership with a processor as part of an SoC, so it will always be likely to have a reasonably
> accurate clock.
> 
>> But another question is whether it should be implemented in each
>> driver level.  The time-stamping is basically a PCM core
>> functionality, and nothing specific to the hardware, especially when
>> it's referring to the system timer.
> 
> That’s a fair point. I don’t know what is done in other drivers, but can only report that with one possible exception,
> the DACs used with Shairport Sync by many end users report well-behaved delay figures, certainly to within two microseconds. I’m afraid I don’t know how they do it.
> 
>> e.g. you can think in a different way, too: we may put a timestamp at
>> each hwptr update, and pass it as-is, instead of updating the
>> timestamp at each position query.  This will effectively gives the
>> accurate position-timestamp pair, and user-space may interpolate as it
>> likes, too.
> 
> That’s not a bad idea, and I might take it up on the alsa-devel mailing list, as you suggest.
> 
>> In anyway, if *this* kind of feature needs to be merged, it's
>> definitely to be discussed with the upstream.  So, if you're going to
>> merge that sort of path, please keep Cc to alsa-devel ML.
> 
> In the meantime, would you think that the balance of convenience lies with this interpolation scheme? (Finally, I have a patch ready….)
> Regards
> Mike
> 
>> 
>> thanks,
>> 
>> Takashi
> 

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint [Resend in plain text...]

[Stefan Wahren]

Hi Mike,

Am 11.10.2018 um 14:53 schrieb Mike Brady:
> Hi Takashi. Just testing out the updated bcm2835 audio driver — it seems that it will underflow at somewhere above about 4410 and below 5120 frames, whereas the present driver is happy down to at least 2000 frames — I haven’t tried lower than about 1700.

in order to avoid confusion please describe a behavior relative to
commit hashes and trees.

Are you able to bisect to a specific commit?

> Is this change meant to happen?

Takashi's series has been applied to next since a month.

Thanks
Stefan

>
> Regards
> Mike
>
>

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint [Resend in plain text...]

[Mike Brady]

Hi Takashi. My apologies — t turns out I was wrong. My measurements were systematically wrong due to integer truncation going from 64 bit to 32 bit representation.

Apologies
Mike

> On 11 Oct 2018, at 13:53, Mike Brady <mikebrady@eircom.net> wrote:
> 
> Hi Takashi. Just testing out the updated bcm2835 audio driver — it seems that it will underflow at somewhere above about 4410 and below 5120 frames, whereas the present driver is happy down to at least 2000 frames — I haven’t tried lower than about 1700.
> 
> Is this change meant to happen?
> 
> Regards
> Mike
> 
> 
>> On 9 Oct 2018, at 16:28, Mike Brady <mikebrady@eircom.net> wrote:
>> 
>> Hi Takashi.
>> 
>>> On 9 Oct 2018, at 14:44, Takashi Iwai <tiwai@suse.de> wrote:
>>> 
>>> On Tue, 09 Oct 2018 15:18:15 +0200,
>>> Mike Brady wrote:
>>>> 
>>>>>> @Mike: Do you want to write a patch series which upstream "interpolate
>>>>>> audio delay" and addresses Takashi's comments?
>>>>>> 
>>>>>> I would help you, in case you have questions about setup a Raspberry Pi
>>>>>> with Mainline kernel or patch submission.
>>>>> 
>>>>> Well, the question is who really wants this.  The value given by that
>>>>> patch is nothing but some estimation and might be even incorrect.
>>>>> 
>>>>> PulseAudio won't need it any longer when you set the BATCH flag.
>>>>> Then it'll switch from tsched mode to the old mode, and the delay
>>>>> value would be almost irrelevant.
>>>> 
>>>> Well, two answers. First, Shairport Sync
>>>> (https://github.com/mikebrady/shairport-sync) needs it — whenever a
>>>> packet of audio frames is about to be added to the output queue (at
>>>> approximately 7.9 millisecond intervals), the delay is checked to
>>>> try to maintain sync to within a few milliseconds. The BCM2835 audio
>>>> device is the only one I have yet come across with so much
>>>> jitter. Whatever other drivers do, the delay they report doesn’t
>>>> suffer from anything like this level of jitter.
>>> 
>>> OK, if there is another application using that delay value, it's worth
>>> to consider providing a fine-grained value.
>>> 
>>>> The second answer is that the veracity of the ALSA documentation
>>>> depends on it — any application using the ALSA system for
>>>> synchronisation will rely on this being an accurate reflection of
>>>> the situation. AFAIK there is really no workaround it if the
>>>> application is confined to “safe” ALSA
>>>> (http://0pointer.de/blog/projects/guide-to-sound-apis).
>>> 
>>>> On LMKL.org, Takashi wrote:
>>>> 
>>>>> Date	Wed, 19 Sep 2018 11:52:33 +0200
>>>>> From	Takashi Iwai <>
>>>>> Subject	Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint
>>>> 
>>>>> [snip]
>>>> 
>>>>> That's OK, as long as the computation is accurate enough (at least not
>>>>> exceed the actual position) and is light-weight.
>>>> 
>>>>> [snip]
>>>> 
>>>> The overhead is small -- an extra ktime_get() every time a GPU message
>>>> is sent -- and another call and a few calculations whenever the delay
>>>> is sought from userland.
>>>> 
>>>> At 48,000 frames per second, i.e. approximately 20 microseconds per
>>>> frame, it would take a clock inaccuracy of roughly
>>>> 20 microseconds in 10 milliseconds -- 2,000 parts per million — to
>>>> result in an inaccurate estimate. 
>>>> Crystal or resonator-based clocks typically have an inaccuracy of
>>>> 10s to 100s of parts per million.
>>>> 
>>>> Finally, to see the effect of the absence and presence of this
>>>> interpolation, please have a look at this:
>>>> https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016,
>>>> where a downstream version of this fix was being discussed.
>>> 
>>> I'm not opposing to the usage of delay value.  The attribute is
>>> provided exactly for such a purpose.  It's a good thing (tm).
>>> 
>>> The potential problem is, however, rather the implementation: it's
>>> using a system timer for interpolation, which is known to drift from
>>> the actual clocks.  Though, one may say that in such a use case, we
>>> may ignore the drift since the interpolation is so narrow.
>> 
>> Yes, that was my thought. I guess another thing in its favour is that this audio device will always
>> be in partnership with a processor as part of an SoC, so it will always be likely to have a reasonably
>> accurate clock.
>> 
>>> But another question is whether it should be implemented in each
>>> driver level.  The time-stamping is basically a PCM core
>>> functionality, and nothing specific to the hardware, especially when
>>> it's referring to the system timer.
>> 
>> That’s a fair point. I don’t know what is done in other drivers, but can only report that with one possible exception,
>> the DACs used with Shairport Sync by many end users report well-behaved delay figures, certainly to within two microseconds. I’m afraid I don’t know how they do it.
>> 
>>> e.g. you can think in a different way, too: we may put a timestamp at
>>> each hwptr update, and pass it as-is, instead of updating the
>>> timestamp at each position query.  This will effectively gives the
>>> accurate position-timestamp pair, and user-space may interpolate as it
>>> likes, too.
>> 
>> That’s not a bad idea, and I might take it up on the alsa-devel mailing list, as you suggest.
>> 
>>> In anyway, if *this* kind of feature needs to be merged, it's
>>> definitely to be discussed with the upstream.  So, if you're going to
>>> merge that sort of path, please keep Cc to alsa-devel ML.
>> 
>> In the meantime, would you think that the balance of convenience lies with this interpolation scheme? (Finally, I have a patch ready….)
>> Regards
>> Mike
>> 
>>> 
>>> thanks,
>>> 
>>> Takashi
>> 
> 

Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint [Resend in plain text...]

[Takashi Iwai]

On Sat, 13 Oct 2018 17:00:32 +0200,
Mike Brady wrote:
> 
> Hi Takashi. My apologies — t turns out I was wrong. My measurements were systematically wrong due to integer truncation going from 64 bit to 32 bit representation.

That relieved me ;)  I thought of starting checking in the next week,
as nothing obvious came to my mind.

In anyways thank you for your testing!


Takashi

> 
> Apologies
> Mike
> 
> > On 11 Oct 2018, at 13:53, Mike Brady <mikebrady@eircom.net> wrote:
> > 
> > Hi Takashi. Just testing out the updated bcm2835 audio driver — it seems that it will underflow at somewhere above about 4410 and below 5120 frames, whereas the present driver is happy down to at least 2000 frames — I haven’t tried lower than about 1700.
> > 
> > Is this change meant to happen?
> > 
> > Regards
> > Mike
> > 
> > 
> >> On 9 Oct 2018, at 16:28, Mike Brady <mikebrady@eircom.net> wrote:
> >> 
> >> Hi Takashi.
> >> 
> >>> On 9 Oct 2018, at 14:44, Takashi Iwai <tiwai@suse.de> wrote:
> >>> 
> >>> On Tue, 09 Oct 2018 15:18:15 +0200,
> >>> Mike Brady wrote:
> >>>> 
> >>>>>> @Mike: Do you want to write a patch series which upstream "interpolate
> >>>>>> audio delay" and addresses Takashi's comments?
> >>>>>> 
> >>>>>> I would help you, in case you have questions about setup a Raspberry Pi
> >>>>>> with Mainline kernel or patch submission.
> >>>>> 
> >>>>> Well, the question is who really wants this.  The value given by that
> >>>>> patch is nothing but some estimation and might be even incorrect.
> >>>>> 
> >>>>> PulseAudio won't need it any longer when you set the BATCH flag.
> >>>>> Then it'll switch from tsched mode to the old mode, and the delay
> >>>>> value would be almost irrelevant.
> >>>> 
> >>>> Well, two answers. First, Shairport Sync
> >>>> (https://github.com/mikebrady/shairport-sync) needs it — whenever a
> >>>> packet of audio frames is about to be added to the output queue (at
> >>>> approximately 7.9 millisecond intervals), the delay is checked to
> >>>> try to maintain sync to within a few milliseconds. The BCM2835 audio
> >>>> device is the only one I have yet come across with so much
> >>>> jitter. Whatever other drivers do, the delay they report doesn’t
> >>>> suffer from anything like this level of jitter.
> >>> 
> >>> OK, if there is another application using that delay value, it's worth
> >>> to consider providing a fine-grained value.
> >>> 
> >>>> The second answer is that the veracity of the ALSA documentation
> >>>> depends on it — any application using the ALSA system for
> >>>> synchronisation will rely on this being an accurate reflection of
> >>>> the situation. AFAIK there is really no workaround it if the
> >>>> application is confined to “safe” ALSA
> >>>> (http://0pointer.de/blog/projects/guide-to-sound-apis).
> >>> 
> >>>> On LMKL.org, Takashi wrote:
> >>>> 
> >>>>> Date	Wed, 19 Sep 2018 11:52:33 +0200
> >>>>> From	Takashi Iwai <>
> >>>>> Subject	Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint
> >>>> 
> >>>>> [snip]
> >>>> 
> >>>>> That's OK, as long as the computation is accurate enough (at least not
> >>>>> exceed the actual position) and is light-weight.
> >>>> 
> >>>>> [snip]
> >>>> 
> >>>> The overhead is small -- an extra ktime_get() every time a GPU message
> >>>> is sent -- and another call and a few calculations whenever the delay
> >>>> is sought from userland.
> >>>> 
> >>>> At 48,000 frames per second, i.e. approximately 20 microseconds per
> >>>> frame, it would take a clock inaccuracy of roughly
> >>>> 20 microseconds in 10 milliseconds -- 2,000 parts per million — to
> >>>> result in an inaccurate estimate. 
> >>>> Crystal or resonator-based clocks typically have an inaccuracy of
> >>>> 10s to 100s of parts per million.
> >>>> 
> >>>> Finally, to see the effect of the absence and presence of this
> >>>> interpolation, please have a look at this:
> >>>> https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016,
> >>>> where a downstream version of this fix was being discussed.
> >>> 
> >>> I'm not opposing to the usage of delay value.  The attribute is
> >>> provided exactly for such a purpose.  It's a good thing (tm).
> >>> 
> >>> The potential problem is, however, rather the implementation: it's
> >>> using a system timer for interpolation, which is known to drift from
> >>> the actual clocks.  Though, one may say that in such a use case, we
> >>> may ignore the drift since the interpolation is so narrow.
> >> 
> >> Yes, that was my thought. I guess another thing in its favour is that this audio device will always
> >> be in partnership with a processor as part of an SoC, so it will always be likely to have a reasonably
> >> accurate clock.
> >> 
> >>> But another question is whether it should be implemented in each
> >>> driver level.  The time-stamping is basically a PCM core
> >>> functionality, and nothing specific to the hardware, especially when
> >>> it's referring to the system timer.
> >> 
> >> That’s a fair point. I don’t know what is done in other drivers, but can only report that with one possible exception,
> >> the DACs used with Shairport Sync by many end users report well-behaved delay figures, certainly to within two microseconds. I’m afraid I don’t know how they do it.
> >> 
> >>> e.g. you can think in a different way, too: we may put a timestamp at
> >>> each hwptr update, and pass it as-is, instead of updating the
> >>> timestamp at each position query.  This will effectively gives the
> >>> accurate position-timestamp pair, and user-space may interpolate as it
> >>> likes, too.
> >> 
> >> That’s not a bad idea, and I might take it up on the alsa-devel mailing list, as you suggest.
> >> 
> >>> In anyway, if *this* kind of feature needs to be merged, it's
> >>> definitely to be discussed with the upstream.  So, if you're going to
> >>> merge that sort of path, please keep Cc to alsa-devel ML.
> >> 
> >> In the meantime, would you think that the balance of convenience lies with this interpolation scheme? (Finally, I have a patch ready….)
> >> Regards
> >> Mike
> >> 
> >>> 
> >>> thanks,
> >>> 
> >>> Takashi
> >> 
> > 
>