2018-10-16 15:06:57

by Nicolas Saenz Julienne

[permalink] [raw]
Subject: [PATCH 0/9] staging: bcm2835-audio: Cleanups and upgrades

Hi,

I just received a RPi3B+ and decided to have a go at fixing stuff in
the audio driver:

The first half of the patchset are just random fixes found while reading
the code.

The second half provides devicetree bindings for bcm2835-audio and fixes
the probe sequence so it fails gracefully if VCHIQ isn't there (as per
TODO).

The series was developed on top of linux-next and tested on a RPi2B and
RPi3B+. Sadly I don't have an HDMI monitor with sound so I only tested
the mini jack output.

Thanks,
Nicolas

===

Nicolas Saenz Julienne (9):
staging: bcm2835-audio: unify FOURCC command definitions
staging: bcm2835-audio: don't initialize memory twice
staging: bcm2835-audio: reorder variable declarations & remove trivial
comments
staging: bcm2835-audio: use anonymous union in struct vc_audio_msg
staging: bcm2835-audio: more generic probe function name
ASoC: dt-bindings: bcm2835-rpi: add onboard audio bindings
ARM: dts: bcm2835-rpi: add onboard audio device
staging: vchiq_arm: add function to check if probe was successful
staging: bcm2835-audio: check for VCHIQ during probe

.../bindings/sound/brcm,bcm2835-audio.txt | 15 +++++++
arch/arm/boot/dts/bcm2835-rpi.dtsi | 5 +++
.../vc04_services/bcm2835-audio/bcm2835-pcm.c | 10 +----
.../bcm2835-audio/bcm2835-vchiq.c | 39 ++++++++-----------
.../vc04_services/bcm2835-audio/bcm2835.c | 30 ++++++++------
.../bcm2835-audio/vc_vchi_audioserv_defs.h | 6 ++-
.../vc04_services/interface/vchi/vchi.h | 3 ++
.../interface/vchiq_arm/vchiq_2835_arm.c | 2 +
.../interface/vchiq_arm/vchiq_arm.c | 16 ++++++++
.../interface/vchiq_arm/vchiq_arm.h | 1 +
.../interface/vchiq_arm/vchiq_if.h | 4 ++
.../interface/vchiq_arm/vchiq_shim.c | 7 ++++
12 files changed, 95 insertions(+), 43 deletions(-)
create mode 100644 Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt

--
2.19.1



2018-10-16 15:05:04

by Nicolas Saenz Julienne

[permalink] [raw]
Subject: [PATCH 9/9] staging: bcm2835-audio: check for VCHIQ during probe

The audio data is sent to rpi's VideoCore IV trough VCHIQ. We make sure
it's available and that we fail gracefully in case it isn't there.

Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
drivers/staging/vc04_services/bcm2835-audio/bcm2835.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 039565311d10..f4b19a4a974b 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -294,9 +294,19 @@ static int snd_add_child_devices(struct device *device, u32 numchans)
static int snd_bcm2835_alsa_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
+ struct device_node *fw_node;
u32 numchans;
int err;

+ fw_node = of_find_compatible_node(NULL, NULL, "brcm,bcm2835-vchiq");
+ if (!fw_node) {
+ dev_err(&pdev->dev, "no vchiq firmware node\n");
+ return -ENODEV;
+ }
+
+ if (!vchi_ready(fw_node))
+ return -EPROBE_DEFER;
+
err = of_property_read_u32(dev->of_node, "brcm,pwm-channels",
&numchans);
if (err) {
--
2.19.1


2018-10-16 15:05:10

by Nicolas Saenz Julienne

[permalink] [raw]
Subject: [PATCH 8/9] staging: vchiq_arm: add function to check if probe was successful

Devices depending on VCHIQ need to double check it's initialization
process was successful. This patch adds a helper function to do so.

Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
.../staging/vc04_services/interface/vchi/vchi.h | 3 +++
.../interface/vchiq_arm/vchiq_2835_arm.c | 2 ++
.../interface/vchiq_arm/vchiq_arm.c | 16 ++++++++++++++++
.../interface/vchiq_arm/vchiq_arm.h | 1 +
.../vc04_services/interface/vchiq_arm/vchiq_if.h | 4 ++++
.../interface/vchiq_arm/vchiq_shim.c | 7 +++++++
6 files changed, 33 insertions(+)

diff --git a/drivers/staging/vc04_services/interface/vchi/vchi.h b/drivers/staging/vc04_services/interface/vchi/vchi.h
index 01381904775d..acf01352135f 100644
--- a/drivers/staging/vc04_services/interface/vchi/vchi.h
+++ b/drivers/staging/vc04_services/interface/vchi/vchi.h
@@ -113,6 +113,9 @@ extern uint32_t vchi_current_time(VCHI_INSTANCE_T instance_handle);
/******************************************************************************
Global service API
*****************************************************************************/
+// Routine to check if vchi is ready
+extern bool vchi_ready(struct device_node *firmware_node);
+
// Routine to create a named service
extern int32_t vchi_service_create(VCHI_INSTANCE_T instance_handle,
SERVICE_CREATION_T *setup,
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 83d740feab96..31dd8a303a20 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -194,6 +194,8 @@ int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
}

g_dev = dev;
+ drvdata->ready = 1;
+
vchiq_log_info(vchiq_arm_log_level,
"vchiq_init - done (slots %pK, phys %pad)",
vchiq_slot_zero, &slot_phys);
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index ea789376de0f..2690e751d1a5 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3682,6 +3682,22 @@ static int vchiq_remove(struct platform_device *pdev)
return 0;
}

+bool vchiq_ready(struct device_node *firmware_node)
+{
+ struct platform_device *pdev = of_find_device_by_node(firmware_node);
+ struct vchiq_drvdata *drvdata;
+
+ if (!pdev)
+ return false;
+
+ drvdata = platform_get_drvdata(pdev);
+ if (!drvdata)
+ return false;
+
+ return drvdata->ready;
+}
+EXPORT_SYMBOL_GPL(vchiq_ready);
+
static struct platform_driver vchiq_driver = {
.driver = {
.name = "bcm2835_vchiq",
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 2f3ebc99cbcf..8215904d219b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -126,6 +126,7 @@ typedef struct vchiq_arm_state_struct {
struct vchiq_drvdata {
const unsigned int cache_line_size;
struct rpi_firmware *fw;
+ unsigned int ready:1;
};

extern int vchiq_arm_log_level;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
index e4109a83e628..54ba822f38ff 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
@@ -34,6 +34,8 @@
#ifndef VCHIQ_IF_H
#define VCHIQ_IF_H

+#include <linux/of.h>
+
#include "interface/vchi/vchi_mh.h"

#define VCHIQ_SERVICE_HANDLE_INVALID 0
@@ -179,4 +181,6 @@ extern VCHIQ_STATUS_T vchiq_dump_phys_mem(VCHIQ_SERVICE_HANDLE_T service,
extern VCHIQ_STATUS_T vchiq_get_peer_version(VCHIQ_SERVICE_HANDLE_T handle,
short *peer_version);

+extern bool vchiq_ready(struct device_node *firmware_node);
+
#endif /* VCHIQ_IF_H */
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
index c3223fcdaf87..69fdface29fd 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
@@ -32,6 +32,7 @@
*/
#include <linux/module.h>
#include <linux/types.h>
+#include <linux/of.h>

#include "interface/vchi/vchi.h"
#include "vchiq.h"
@@ -50,6 +51,12 @@ struct shim_service {
void *callback_param;
};

+bool vchi_ready(struct device_node *firmware_node)
+{
+ return vchiq_ready(firmware_node);
+}
+EXPORT_SYMBOL(vchi_ready);
+
/***********************************************************
* Name: vchi_msg_peek
*
--
2.19.1


2018-10-16 15:05:14

by Nicolas Saenz Julienne

[permalink] [raw]
Subject: [PATCH 3/9] staging: bcm2835-audio: reorder variable declarations & remove trivial comments

When it comes to declaring variables it's preferred, when possible, to
use an inverted tree organization scheme.

Also, removes some comments that were useless.

Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
.../vc04_services/bcm2835-audio/bcm2835-pcm.c | 10 ++--------
.../vc04_services/bcm2835-audio/bcm2835-vchiq.c | 4 ++--
.../staging/vc04_services/bcm2835-audio/bcm2835.c | 14 +++++++-------
3 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index e66da11af5cf..98b6977bdce7 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -164,14 +164,11 @@ static int snd_bcm2835_playback_spdif_open(struct snd_pcm_substream *substream)
return snd_bcm2835_playback_open_generic(substream, 1);
}

-/* close callback */
static int snd_bcm2835_playback_close(struct snd_pcm_substream *substream)
{
- /* the hardware-specific codes will be here */
-
- struct bcm2835_chip *chip;
- struct snd_pcm_runtime *runtime;
struct bcm2835_alsa_stream *alsa_stream;
+ struct snd_pcm_runtime *runtime;
+ struct bcm2835_chip *chip;

chip = snd_pcm_substream_chip(substream);
mutex_lock(&chip->audio_mutex);
@@ -195,20 +192,17 @@ static int snd_bcm2835_playback_close(struct snd_pcm_substream *substream)
return 0;
}

-/* hw_params callback */
static int snd_bcm2835_pcm_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
{
return snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(params));
}

-/* hw_free callback */
static int snd_bcm2835_pcm_hw_free(struct snd_pcm_substream *substream)
{
return snd_pcm_lib_free_pages(substream);
}

-/* prepare callback */
static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream)
{
struct bcm2835_chip *chip = snd_pcm_substream_chip(substream);
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index aca7008e1921..932ef12ac5d2 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -94,9 +94,9 @@ static void audio_vchi_callback(void *param,
void *msg_handle)
{
struct bcm2835_audio_instance *instance = param;
- int status;
- int msg_len;
struct vc_audio_msg m;
+ int msg_len;
+ int status;

if (reason != VCHI_CALLBACK_MSG_AVAILABLE)
return;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 0efae7068fef..6ee8334dfc81 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -161,8 +161,8 @@ static int snd_add_child_device(struct device *dev,
struct bcm2835_audio_driver *audio_driver,
u32 numchans)
{
- struct snd_card *card;
struct bcm2835_chip *chip;
+ struct snd_card *card;
int err;

err = snd_card_new(dev, -1, NULL, THIS_MODULE, sizeof(*chip), &card);
@@ -225,12 +225,12 @@ static int snd_add_child_device(struct device *dev,

static int snd_add_child_devices(struct device *device, u32 numchans)
{
- int i;
- int count_devices = 0;
- int minchannels = 0;
- int extrachannels = 0;
int extrachannels_per_driver = 0;
int extrachannels_remainder = 0;
+ int count_devices = 0;
+ int extrachannels = 0;
+ int minchannels = 0;
+ int i;

for (i = 0; i < ARRAY_SIZE(children_devices); i++)
if (*children_devices[i].is_enabled)
@@ -258,9 +258,9 @@ static int snd_add_child_devices(struct device *device, u32 numchans)
extrachannels_remainder);

for (i = 0; i < ARRAY_SIZE(children_devices); i++) {
- int err;
- int numchannels_this_device;
struct bcm2835_audio_driver *audio_driver;
+ int numchannels_this_device;
+ int err;

if (!*children_devices[i].is_enabled)
continue;
--
2.19.1


2018-10-16 15:05:31

by Nicolas Saenz Julienne

[permalink] [raw]
Subject: [PATCH 7/9] ARM: dts: bcm2835-rpi: add onboard audio device

The audio device interfaces with VCHIQ to send audio through the rpi's
Headphones & HDMI.

Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
arch/arm/boot/dts/bcm2835-rpi.dtsi | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index cb2d6d78a7fb..6cc580d17862 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -35,6 +35,11 @@
reg = <0x7e00b840 0xf>;
interrupts = <0 2>;
};
+
+ audio: audio {
+ compatible = "brcm,bcm2835-audio";
+ brcm,pwm-channels = <8>;
+ };
};
};

--
2.19.1


2018-10-16 15:05:40

by Nicolas Saenz Julienne

[permalink] [raw]
Subject: [PATCH 5/9] staging: bcm2835-audio: more generic probe function name

There will only be one probe function, there is no use for appendig
"_dt" the end of the name.

Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
drivers/staging/vc04_services/bcm2835-audio/bcm2835.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 6ee8334dfc81..039565311d10 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -291,7 +291,7 @@ static int snd_add_child_devices(struct device *device, u32 numchans)
return 0;
}

-static int snd_bcm2835_alsa_probe_dt(struct platform_device *pdev)
+static int snd_bcm2835_alsa_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
u32 numchans;
@@ -344,7 +344,7 @@ static const struct of_device_id snd_bcm2835_of_match_table[] = {
MODULE_DEVICE_TABLE(of, snd_bcm2835_of_match_table);

static struct platform_driver bcm2835_alsa0_driver = {
- .probe = snd_bcm2835_alsa_probe_dt,
+ .probe = snd_bcm2835_alsa_probe,
#ifdef CONFIG_PM
.suspend = snd_bcm2835_alsa_suspend,
.resume = snd_bcm2835_alsa_resume,
--
2.19.1


2018-10-16 15:06:48

by Nicolas Saenz Julienne

[permalink] [raw]
Subject: [PATCH 1/9] staging: bcm2835-audio: unify FOURCC command definitions

The device communicates with the audio core using FOURCC codes. The
driver was generating them using different macros/expressions. We now
use the same macro to create them and centralize all the definitions.

Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
.../vc04_services/bcm2835-audio/bcm2835-vchiq.c | 13 ++++---------
.../bcm2835-audio/vc_vchi_audioserv_defs.h | 4 +++-
2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 781754f36da7..aca7008e1921 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -89,11 +89,6 @@ static int bcm2835_audio_send_simple(struct bcm2835_audio_instance *instance,
return bcm2835_audio_send_msg(instance, &m, wait);
}

-static const u32 BCM2835_AUDIO_WRITE_COOKIE1 = ('B' << 24 | 'C' << 16 |
- 'M' << 8 | 'A');
-static const u32 BCM2835_AUDIO_WRITE_COOKIE2 = ('D' << 24 | 'A' << 16 |
- 'T' << 8 | 'A');
-
static void audio_vchi_callback(void *param,
const VCHI_CALLBACK_REASON_T reason,
void *msg_handle)
@@ -112,8 +107,8 @@ static void audio_vchi_callback(void *param,
instance->result = m.u.result.success;
complete(&instance->msg_avail_comp);
} 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)
+ if (m.u.complete.cookie1 != VC_AUDIO_WRITE_COOKIE1 ||
+ m.u.complete.cookie2 != VC_AUDIO_WRITE_COOKIE2)
dev_err(instance->dev, "invalid cookie\n");
else
bcm2835_playback_fifo(instance->alsa_stream,
@@ -329,8 +324,8 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
.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,
+ .u.write.cookie1 = VC_AUDIO_WRITE_COOKIE1,
+ .u.write.cookie2 = VC_AUDIO_WRITE_COOKIE2,
};
unsigned int count;
int err, status;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/vc_vchi_audioserv_defs.h b/drivers/staging/vc04_services/bcm2835-audio/vc_vchi_audioserv_defs.h
index 1a7f0884ac9c..dc62875cfdca 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/vc_vchi_audioserv_defs.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/vc_vchi_audioserv_defs.h
@@ -7,8 +7,10 @@
#define VC_AUDIOSERV_MIN_VER 1
#define VC_AUDIOSERV_VER 2

-/* FourCC code used for VCHI connection */
+/* FourCC codes used for VCHI communication */
#define VC_AUDIO_SERVER_NAME MAKE_FOURCC("AUDS")
+#define VC_AUDIO_WRITE_COOKIE1 MAKE_FOURCC("BCMA")
+#define VC_AUDIO_WRITE_COOKIE2 MAKE_FOURCC("DATA")

/*
* List of screens that are currently supported
--
2.19.1


2018-10-16 15:06:49

by Nicolas Saenz Julienne

[permalink] [raw]
Subject: [PATCH 4/9] staging: bcm2835-audio: use anonymous union in struct vc_audio_msg

In this case explicitly naming the union doesn't help overall code
comprehension and clutters it.

Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
.../bcm2835-audio/bcm2835-vchiq.c | 30 +++++++++----------
.../bcm2835-audio/vc_vchi_audioserv_defs.h | 2 +-
2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
index 932ef12ac5d2..0db412fd7c55 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
@@ -104,15 +104,15 @@ static void audio_vchi_callback(void *param,
status = vchi_msg_dequeue(instance->vchi_handle,
&m, sizeof(m), &msg_len, VCHI_FLAGS_NONE);
if (m.type == VC_AUDIO_MSG_TYPE_RESULT) {
- instance->result = m.u.result.success;
+ instance->result = m.result.success;
complete(&instance->msg_avail_comp);
} else if (m.type == VC_AUDIO_MSG_TYPE_COMPLETE) {
- if (m.u.complete.cookie1 != VC_AUDIO_WRITE_COOKIE1 ||
- m.u.complete.cookie2 != VC_AUDIO_WRITE_COOKIE2)
+ if (m.complete.cookie1 != VC_AUDIO_WRITE_COOKIE1 ||
+ m.complete.cookie2 != VC_AUDIO_WRITE_COOKIE2)
dev_err(instance->dev, "invalid cookie\n");
else
bcm2835_playback_fifo(instance->alsa_stream,
- m.u.complete.count);
+ m.complete.count);
} else {
dev_err(instance->dev, "unexpected callback type=%d\n", m.type);
}
@@ -249,11 +249,11 @@ int bcm2835_audio_set_ctls(struct bcm2835_alsa_stream *alsa_stream)
struct vc_audio_msg m = {};

m.type = VC_AUDIO_MSG_TYPE_CONTROL;
- m.u.control.dest = chip->dest;
+ m.control.dest = chip->dest;
if (!chip->mute)
- m.u.control.volume = CHIP_MIN_VOLUME;
+ m.control.volume = CHIP_MIN_VOLUME;
else
- m.u.control.volume = alsa2chip(chip->volume);
+ m.control.volume = alsa2chip(chip->volume);

return bcm2835_audio_send_msg(alsa_stream->instance, &m, true);
}
@@ -264,9 +264,9 @@ int bcm2835_audio_set_params(struct bcm2835_alsa_stream *alsa_stream,
{
struct vc_audio_msg m = {
.type = VC_AUDIO_MSG_TYPE_CONFIG,
- .u.config.channels = channels,
- .u.config.samplerate = samplerate,
- .u.config.bps = bps,
+ .config.channels = channels,
+ .config.samplerate = samplerate,
+ .config.bps = bps,
};
int err;

@@ -294,7 +294,7 @@ 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,
+ .stop.draining = 1,
};

return bcm2835_audio_send_msg(alsa_stream->instance, &m, false);
@@ -322,10 +322,10 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
struct bcm2835_audio_instance *instance = alsa_stream->instance;
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 = VC_AUDIO_WRITE_COOKIE1,
- .u.write.cookie2 = VC_AUDIO_WRITE_COOKIE2,
+ .write.count = size,
+ .write.max_packet = instance->max_packet,
+ .write.cookie1 = VC_AUDIO_WRITE_COOKIE1,
+ .write.cookie2 = VC_AUDIO_WRITE_COOKIE2,
};
unsigned int count;
int err, status;
diff --git a/drivers/staging/vc04_services/bcm2835-audio/vc_vchi_audioserv_defs.h b/drivers/staging/vc04_services/bcm2835-audio/vc_vchi_audioserv_defs.h
index dc62875cfdca..d6401e914ac9 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/vc_vchi_audioserv_defs.h
+++ b/drivers/staging/vc04_services/bcm2835-audio/vc_vchi_audioserv_defs.h
@@ -93,7 +93,7 @@ struct vc_audio_msg {
struct vc_audio_write write;
struct vc_audio_result result;
struct vc_audio_complete complete;
- } u;
+ };
};

#endif /* _VC_AUDIO_DEFS_H_ */
--
2.19.1


2018-10-16 15:06:53

by Nicolas Saenz Julienne

[permalink] [raw]
Subject: [PATCH 2/9] staging: bcm2835-audio: don't initialize memory twice

The memory is being allocated with devres_alloc(), wich ultimately uses
__GFP_ZERO to call kmalloc. We don't need to zero the memory area again
in bcm2835-audio.

Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
drivers/staging/vc04_services/bcm2835-audio/bcm2835.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 87d56ab1ffa0..0efae7068fef 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -39,8 +39,6 @@ static int bcm2835_devm_add_vchi_ctx(struct device *dev)
if (!vchi_ctx)
return -ENOMEM;

- memset(vchi_ctx, 0, sizeof(*vchi_ctx));
-
ret = bcm2835_new_vchi_ctx(dev, vchi_ctx);
if (ret) {
devres_free(vchi_ctx);
--
2.19.1


2018-10-16 15:07:44

by Nicolas Saenz Julienne

[permalink] [raw]
Subject: [PATCH 6/9] ASoC: dt-bindings: bcm2835-rpi: add onboard audio bindings

Adds a device tree binding file for Raspberry pi's Headphones and HDMI
audio output devices.

Based on raspberry's downstream kernel implementation:
https://github.com/raspberrypi/linux/blob/rpi-4.14.y/arch/arm/boot/dts/bcm2708-rpi.dtsi

Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
.../bindings/sound/brcm,bcm2835-audio.txt | 15 +++++++++++++++
1 file changed, 15 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt

diff --git a/Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt b/Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt
new file mode 100644
index 000000000000..ee6fa085aaa9
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt
@@ -0,0 +1,15 @@
+Broadcom BCM283x audio device
+
+Required properties:
+
+- compatible: Should be "brcm,bcm2835-audio"
+- brcm,pwm-channels: number of PWM channels, they are behind RPi's Video Core
+ IV, not actual Linux PWM devices.
+
+Example:
+
+audio: audio {
+ compatible = "brcm,bcm2835-audio";
+ brcm,pwm-channels = <8>;
+};
+
--
2.19.1


2018-10-16 15:48:06

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH 0/9] staging: bcm2835-audio: Cleanups and upgrades

On Tue, 16 Oct 2018 17:02:19 +0200,
Nicolas Saenz Julienne wrote:
>
> Hi,
>
> I just received a RPi3B+ and decided to have a go at fixing stuff in
> the audio driver:
>
> The first half of the patchset are just random fixes found while reading
> the code.
>
> The second half provides devicetree bindings for bcm2835-audio and fixes
> the probe sequence so it fails gracefully if VCHIQ isn't there (as per
> TODO).
>
> The series was developed on top of linux-next and tested on a RPi2B and
> RPi3B+. Sadly I don't have an HDMI monitor with sound so I only tested
> the mini jack output.

All patches look good to me:
Reviewed-by: Takashi Iwai <[email protected]>


thanks,

Takashi

>
> Thanks,
> Nicolas
>
> ===
>
> Nicolas Saenz Julienne (9):
> staging: bcm2835-audio: unify FOURCC command definitions
> staging: bcm2835-audio: don't initialize memory twice
> staging: bcm2835-audio: reorder variable declarations & remove trivial
> comments
> staging: bcm2835-audio: use anonymous union in struct vc_audio_msg
> staging: bcm2835-audio: more generic probe function name
> ASoC: dt-bindings: bcm2835-rpi: add onboard audio bindings
> ARM: dts: bcm2835-rpi: add onboard audio device
> staging: vchiq_arm: add function to check if probe was successful
> staging: bcm2835-audio: check for VCHIQ during probe
>
> .../bindings/sound/brcm,bcm2835-audio.txt | 15 +++++++
> arch/arm/boot/dts/bcm2835-rpi.dtsi | 5 +++
> .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 10 +----
> .../bcm2835-audio/bcm2835-vchiq.c | 39 ++++++++-----------
> .../vc04_services/bcm2835-audio/bcm2835.c | 30 ++++++++------
> .../bcm2835-audio/vc_vchi_audioserv_defs.h | 6 ++-
> .../vc04_services/interface/vchi/vchi.h | 3 ++
> .../interface/vchiq_arm/vchiq_2835_arm.c | 2 +
> .../interface/vchiq_arm/vchiq_arm.c | 16 ++++++++
> .../interface/vchiq_arm/vchiq_arm.h | 1 +
> .../interface/vchiq_arm/vchiq_if.h | 4 ++
> .../interface/vchiq_arm/vchiq_shim.c | 7 ++++
> 12 files changed, 95 insertions(+), 43 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt
>
> --
> 2.19.1
>

2018-10-16 15:50:33

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH 5/9] staging: bcm2835-audio: more generic probe function name

On Tue, 16 Oct 2018 17:02:24 +0200,
Nicolas Saenz Julienne wrote:
>
> There will only be one probe function, there is no use for appendig
> "_dt" the end of the name.
>
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> ---
> drivers/staging/vc04_services/bcm2835-audio/bcm2835.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> index 6ee8334dfc81..039565311d10 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> @@ -291,7 +291,7 @@ static int snd_add_child_devices(struct device *device, u32 numchans)
> return 0;
> }
>
> -static int snd_bcm2835_alsa_probe_dt(struct platform_device *pdev)
> +static int snd_bcm2835_alsa_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> u32 numchans;
> @@ -344,7 +344,7 @@ static const struct of_device_id snd_bcm2835_of_match_table[] = {
> MODULE_DEVICE_TABLE(of, snd_bcm2835_of_match_table);
>
> static struct platform_driver bcm2835_alsa0_driver = {

Actually now I wonder why this "0" here...

It has nothing to do with the patch, but it's a good opportunity to
clean it up, too.


thanks,

Takashi

2018-10-16 15:57:54

by Stefan Wahren

[permalink] [raw]
Subject: Re: [PATCH 6/9] ASoC: dt-bindings: bcm2835-rpi: add onboard audio bindings

Hi Nicolas,

Am 16.10.2018 um 17:02 schrieb Nicolas Saenz Julienne:
> Adds a device tree binding file for Raspberry pi's Headphones and HDMI
> audio output devices.
>
> Based on raspberry's downstream kernel implementation:
> https://github.com/raspberrypi/linux/blob/rpi-4.14.y/arch/arm/boot/dts/bcm2708-rpi.dtsi
>
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> ---
> .../bindings/sound/brcm,bcm2835-audio.txt | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt b/Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt
> new file mode 100644
> index 000000000000..ee6fa085aaa9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt
> @@ -0,0 +1,15 @@
> +Broadcom BCM283x audio device
> +
> +Required properties:
> +
> +- compatible: Should be "brcm,bcm2835-audio"
> +- brcm,pwm-channels: number of PWM channels, they are behind RPi's Video Core
> + IV, not actual Linux PWM devices.
> +
> +Example:
> +
> +audio: audio {
> + compatible = "brcm,bcm2835-audio";
> + brcm,pwm-channels = <8>;
> +};
> +

i apologize but it seems to me that the TODO mentioned in the cover
letter isn't update to date anymore.

Phil Elwell posted an important bugfix for vchiq before [1], but only
the driver part has been applied yet. After applying the DT changes i'm
not sure if it still works.

AFAIK the audio driver uses VCHIQ as a software interface and the
binding doesn't describe the real hardware.

Since the camera driver will be registered as a platform device [2], i
prefer this way for the audio driver, too.

I'm actually working on this here [3] (currently only compile tested).

Stefan

[1] - https://patchwork.ozlabs.org/cover/970434/
[2] - https://lore.kernel.org/patchwork/patch/904411/
[3] - https://github.com/anholt/linux/commits/bcm2835-audio

2018-10-16 16:19:03

by Eric Anholt

[permalink] [raw]
Subject: Re: [PATCH 7/9] ARM: dts: bcm2835-rpi: add onboard audio device

Nicolas Saenz Julienne <[email protected]> writes:

> The audio device interfaces with VCHIQ to send audio through the rpi's
> Headphones & HDMI.
>
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> ---
> arch/arm/boot/dts/bcm2835-rpi.dtsi | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
> index cb2d6d78a7fb..6cc580d17862 100644
> --- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
> +++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
> @@ -35,6 +35,11 @@
> reg = <0x7e00b840 0xf>;
> interrupts = <0 2>;
> };
> +
> + audio: audio {
> + compatible = "brcm,bcm2835-audio";
> + brcm,pwm-channels = <8>;
> + };
> };
> };

Assuming the DT binding gets acked,

Acked-by: Eric Anholt <[email protected]>

Attachments:
signature.asc (847.00 B)

2018-10-16 16:49:09

by Nicolas Saenz Julienne

[permalink] [raw]
Subject: Re: [PATCH 6/9] ASoC: dt-bindings: bcm2835-rpi: add onboard audio bindings

Hi Stefan, thanks for the review,

On Tue, 2018-10-16 at 17:56 +0200, Stefan Wahren wrote:
> Hi Nicolas,
>
> Am 16.10.2018 um 17:02 schrieb Nicolas Saenz Julienne:
> > Adds a device tree binding file for Raspberry pi's Headphones and
> > HDMI
> > audio output devices.
> >
> > Based on raspberry's downstream kernel implementation:
> >

https://github.com/raspberrypi/linux/blob/rpi-4.14.y/arch/arm/boot/dts/bcm2708-rpi.dtsi
> >
> > Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> > ---
> > .../bindings/sound/brcm,bcm2835-audio.txt | 15
> > +++++++++++++++
> > 1 file changed, 15 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt
> >
> > diff --git a/Documentation/devicetree/bindings/sound/brcm,bcm2835-
> > audio.txt b/Documentation/devicetree/bindings/sound/brcm,bcm2835-
> > audio.txt
> > new file mode 100644
> > index 000000000000..ee6fa085aaa9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/brcm,bcm2835-
> > audio.txt
> > @@ -0,0 +1,15 @@
> > +Broadcom BCM283x audio device
> > +
> > +Required properties:
> > +
> > +- compatible: Should be "brcm,bcm2835-audio"
> > +- brcm,pwm-channels: number of PWM channels, they are behind RPi's
> > Video Core
> > + IV, not actual Linux PWM devices.
> > +
> > +Example:
> > +
> > +audio: audio {
> > + compatible = "brcm,bcm2835-audio";
> > + brcm,pwm-channels = <8>;
> > +};
> > +
>
> i apologize but it seems to me that the TODO mentioned in the cover
> letter isn't update to date anymore.
>
> Phil Elwell posted an important bugfix for vchiq before [1], but only
> the driver part has been applied yet. After applying the DT changes
> i'm
> not sure if it still works.
>
> AFAIK the audio driver uses VCHIQ as a software interface and the
> binding doesn't describe the real hardware.
>
> Since the camera driver will be registered as a platform device [2],
> i
> prefer this way for the audio driver, too.
>
> I'm actually working on this here [3] (currently only compile
> tested).
>

Fair enough. I wasn't feeling too good about the bindings myself. I
only went with them because I saw something similar was happening
between "raspberrypi,bcm2835-power" and "raspberrypi,bcm2835-firmware".
I'll be glad to review & test your patches whenever they are ready.

This makes commits 6 to 9 useless. Do you want me to send a v2? I can
throw in an extra change Takashi suggested and update the TODO file.

Regards,
Nicolas

Attachments:
signature.asc (499.00 B)
This is a digitally signed message part

2018-10-16 19:28:29

by Stefan Wahren

[permalink] [raw]
Subject: Re: [PATCH 6/9] ASoC: dt-bindings: bcm2835-rpi: add onboard audio bindings

Hi Nicolas,

> Nicolas Saenz Julienne <[email protected]> hat am 16. Oktober 2018 um 18:48 geschrieben:
>
>
> Hi Stefan, thanks for the review,
>
> On Tue, 2018-10-16 at 17:56 +0200, Stefan Wahren wrote:
> > Hi Nicolas,
> >
> > Am 16.10.2018 um 17:02 schrieb Nicolas Saenz Julienne:
> > > Adds a device tree binding file for Raspberry pi's Headphones and
> > > HDMI
> > > audio output devices.
> > >
> > > Based on raspberry's downstream kernel implementation:
> > >
>
> https://github.com/raspberrypi/linux/blob/rpi-4.14.y/arch/arm/boot/dts/bcm2708-rpi.dtsi
> > >
> > > Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> > > ---
> > > .../bindings/sound/brcm,bcm2835-audio.txt | 15
> > > +++++++++++++++
> > > 1 file changed, 15 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/sound/brcm,bcm2835-audio.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/sound/brcm,bcm2835-
> > > audio.txt b/Documentation/devicetree/bindings/sound/brcm,bcm2835-
> > > audio.txt
> > > new file mode 100644
> > > index 000000000000..ee6fa085aaa9
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/sound/brcm,bcm2835-
> > > audio.txt
> > > @@ -0,0 +1,15 @@
> > > +Broadcom BCM283x audio device
> > > +
> > > +Required properties:
> > > +
> > > +- compatible: Should be "brcm,bcm2835-audio"
> > > +- brcm,pwm-channels: number of PWM channels, they are behind RPi's
> > > Video Core
> > > + IV, not actual Linux PWM devices.
> > > +
> > > +Example:
> > > +
> > > +audio: audio {
> > > + compatible = "brcm,bcm2835-audio";
> > > + brcm,pwm-channels = <8>;
> > > +};
> > > +
> >
> > i apologize but it seems to me that the TODO mentioned in the cover
> > letter isn't update to date anymore.
> >
> > Phil Elwell posted an important bugfix for vchiq before [1], but only
> > the driver part has been applied yet. After applying the DT changes
> > i'm
> > not sure if it still works.
> >
> > AFAIK the audio driver uses VCHIQ as a software interface and the
> > binding doesn't describe the real hardware.
> >
> > Since the camera driver will be registered as a platform device [2],
> > i
> > prefer this way for the audio driver, too.
> >
> > I'm actually working on this here [3] (currently only compile
> > tested).
> >
>
> Fair enough. I wasn't feeling too good about the bindings myself. I
> only went with them because I saw something similar was happening
> between "raspberrypi,bcm2835-power" and "raspberrypi,bcm2835-firmware".
> I'll be glad to review & test your patches whenever they are ready.

i will inform you.

>
> This makes commits 6 to 9 useless. Do you want me to send a v2? I can
> throw in an extra change Takashi suggested and update the TODO file.

You can add my

Acked-by: Stefan Wahren <[email protected]>

to patches 1 to 5. Please drop 6 to 9 from the series and add the other suggestions.

Btw please add [email protected] to CC for V2

Thanks
Stefan

>
> Regards,
> Nicolas