Skip to content

Support for Blokas Labs pisound board. #1684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 23, 2016
Merged

Support for Blokas Labs pisound board. #1684

merged 3 commits into from
Oct 23, 2016

Conversation

gtrainavicius
Copy link
Contributor

This pull request integrates the pisound kernel module (https://github.com/BlokasLabs/pisound/) to Raspberry Pi kernel. The out-of-tree module code was successfully tested on multiple kinds of Raspberry Pi hardware by our beta testers.

Also I tested the module integrated to kernel code too, it builds and runs as expected, the code and device tree are exactly the same as in the out-of-tree code, apart from the code being amalgamated into a single source file.

Thank you!

@pelwell
Copy link
Contributor

pelwell commented Oct 17, 2016

I've add a patch to fix the README formatting. scripts/checkpatch.pl complains about a few formatting issues with the driver (C++ comments, line length, brace position and usage etc.), but if you can fix those I'll happily merge it.

It's nice to see MIDI support - it takes me back to 1985 when I first started playing with it.

@gtrainavicius
Copy link
Contributor Author

Great! Didn't notice the requirement for running the scripts/checkpatch.pl, can you point me to any place listing the requirements for kernel patches, for future reference?

I will let you know as soon as I get the warnings fixed.

@popcornmix
Copy link
Collaborator

@gtrainavicius
Copy link
Contributor Author

gtrainavicius commented Oct 22, 2016

I have fixed most of the warnings, only these two remain:

WARNING: DT compatible string "blokaslabs,pisound" appears un-documented -- check ./Documentation/devicetree/bindings/
#656: FILE: sound/soc/bcm/pisound.c:656:
+       { .compatible = "blokaslabs,pisound", },

WARNING: DT compatible string "blokaslabs,pisound-spi" appears un-documented -- check ./Documentation/devicetree/bindings/
#657: FILE: sound/soc/bcm/pisound.c:657:
+       { .compatible = "blokaslabs,pisound-spi", },

total: 0 errors, 2 warnings, 987 lines checked

I am not sure what would be the expected way to document them, also didn't find examples of other sound cards documenting them...

Is solving these warnings optional?

I have also successfully tested the updated code, the changes work as expected.

@pelwell
Copy link
Contributor

pelwell commented Oct 22, 2016

No, you don't need to worry about those. I'll cast an eye over the result when I'm not on a phone, but then I'll almost certainly merge it.

@pelwell pelwell merged commit f2af003 into raspberrypi:rpi-4.4.y Oct 23, 2016
@gtrainavicius
Copy link
Contributor Author

Thank you!

@gtrainavicius gtrainavicius deleted the giedrius/rpi-4-4.y-pisound branch October 23, 2016 09:30
@clivem
Copy link

clivem commented Oct 24, 2016

In file included from include/linux/printk.h:6:0,
                 from include/linux/kernel.h:13,
                 from include/linux/list.h:8,
                 from include/linux/module.h:9,
                 from sound/soc/bcm/pisound.c:22:
sound/soc/bcm/pisound.c: In function 'pisnd_midi_recv_callback':
include/linux/kern_levels.h:4:18: warning: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'uint8_t * {aka unsigned char *}' [-Wformat=]
 #define KERN_SOH "\001"  /* ASCII Start Of Header */
                  ^
include/linux/kern_levels.h:8:20: note: in expansion of macro 'KERN_SOH'
 #define KERN_ALERT KERN_SOH "1" /* action must be taken immediately */
                    ^~~~~~~~
include/linux/printk.h:248:9: note: in expansion of macro 'KERN_ALERT'
  printk(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
         ^~~~~~~~~~
sound/soc/bcm/pisound.c:59:22: note: in expansion of macro 'pr_alert'
 # define printd(...) pr_alert(PISOUND_LOG_PREFIX __VA_ARGS__)
                      ^~~~~~~~
sound/soc/bcm/pisound.c:122:3: note: in expansion of macro 'printd'
   printd("midi recv 0x%02x, res = %d\n", data, res);
   ^~~~~~

@gtrainavicius
Copy link
Contributor Author

Sorry about that... This code is compiled / printed only when DEBUG is defined. I will make a fix for the warning and additionally change the macro used to enable this module's debug info, as it is relevant only for the module developers only.

I will submit a patch ASAP.

@clivem
Copy link

clivem commented Oct 24, 2016

No worries. Just noticed the warning after running a build.
Yes, I do define subdir-ccflags-y := -DDEBUG in sound/soc/Makefile

@gtrainavicius
Copy link
Contributor Author

Alright, here is the fix, let me know if I should create a new pull request instead:

(created the patch file using: git format-patch -1 HEAD --stdout > pisound_warning_fix.patch)

pisound_warning_fix.zip

From 4824a86bda07ac2bbc0dab45dabf52979e221bca Mon Sep 17 00:00:00 2001
From: Giedrius Trainavicius <[email protected]>
Date: Tue, 25 Oct 2016 01:47:20 +0300
Subject: [PATCH] pisound: Fixed a warning on DEBUG builds and changed the
 macro that enables debug level printing from DEBUG to PISOUND_DEBUG

---
 sound/soc/bcm/pisound.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/bcm/pisound.c b/sound/soc/bcm/pisound.c
index b156e57..a3cd089 100644
--- a/sound/soc/bcm/pisound.c
+++ b/sound/soc/bcm/pisound.c
@@ -55,7 +55,7 @@ static void pisnd_midi_uninit(void);

 #define PISOUND_LOG_PREFIX "pisound: "

-#ifdef DEBUG
+#ifdef PISOUND_DEBUG
 #  define printd(...) pr_alert(PISOUND_LOG_PREFIX __VA_ARGS__)
 #else
 #  define printd(...) do {} while (0)
@@ -119,7 +119,7 @@ static void pisnd_midi_recv_callback(void *substream)
    while ((n = pisnd_spi_recv(data, sizeof(data)))) {
        int res = snd_rawmidi_receive(substream, data, n);
        (void)res;
-       printd("midi recv 0x%02x, res = %d\n", data, res);
+       printd("midi recv %u bytes, res = %d\n", n, res);
    }
 }

-- 
2.1.4


@pelwell
Copy link
Contributor

pelwell commented Oct 25, 2016

I've applied that patch (editing the commit message for line length).

@clivem
Copy link

clivem commented Oct 25, 2016

Cool! I'm sure you don't need me to state the obvious, but with the patch, compile warning is resolved! ;)

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Oct 25, 2016
kernel: Support for Blokas Labs pisound board
See: raspberrypi/linux#1684

firmware: Video_decode: Check licenced codecs at component create

firmware: dispmanx: Report transform or display as the display_rotate variable
See: raspberrypi/userland#348

firmware: arm_loader: Don't lose force_turbo when initial_turbo completes
See: #667

firmware: mmal: improvements to mmal_queue code

firmware: arm_dt: Silence system-supplied dtparams

firmware: vc_image: Remove obsolete processor support using _VC_VERSION
firmware: vc_image: Include colourspace in RGB to YUV conversions
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Oct 25, 2016
kernel: Support for Blokas Labs pisound board
See: raspberrypi/linux#1684

firmware: Video_decode: Check licenced codecs at component create

firmware: dispmanx: Report transform or display as the display_rotate variable
See: raspberrypi/userland#348

firmware: arm_loader: Don't lose force_turbo when initial_turbo completes
See: raspberrypi/firmware#667

firmware: mmal: improvements to mmal_queue code

firmware: arm_dt: Silence system-supplied dtparams

firmware: vc_image: Remove obsolete processor support using _VC_VERSION
firmware: vc_image: Include colourspace in RGB to YUV conversions
popcornmix added a commit to raspberrypi/firmware that referenced this pull request Oct 25, 2016
kernel: Support for Blokas Labs pisound board
See: raspberrypi/linux#1684

firmware: Video_decode: Check licenced codecs at component create

firmware: dispmanx: Report transform or display as the display_rotate variable
See: raspberrypi/userland#348

firmware: arm_loader: Don't lose force_turbo when initial_turbo completes
See: #667

firmware: mmal: improvements to mmal_queue code

firmware: arm_dt: Silence system-supplied dtparams

firmware: vc_image: Remove obsolete processor support using _VC_VERSION
firmware: vc_image: Include colourspace in RGB to YUV conversions
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Oct 25, 2016
kernel: Support for Blokas Labs pisound board
See: raspberrypi/linux#1684

firmware: Video_decode: Check licenced codecs at component create

firmware: dispmanx: Report transform or display as the display_rotate variable
See: raspberrypi/userland#348

firmware: arm_loader: Don't lose force_turbo when initial_turbo completes
See: raspberrypi/firmware#667

firmware: mmal: improvements to mmal_queue code

firmware: arm_dt: Silence system-supplied dtparams

firmware: vc_image: Remove obsolete processor support using _VC_VERSION
firmware: vc_image: Include colourspace in RGB to YUV conversions
neuschaefer pushed a commit to neuschaefer/raspi-binary-firmware that referenced this pull request Feb 27, 2017
kernel: Support for Blokas Labs pisound board
See: raspberrypi/linux#1684

firmware: Video_decode: Check licenced codecs at component create

firmware: dispmanx: Report transform or display as the display_rotate variable
See: raspberrypi/userland#348

firmware: arm_loader: Don't lose force_turbo when initial_turbo completes
See: raspberrypi#667

firmware: mmal: improvements to mmal_queue code

firmware: arm_dt: Silence system-supplied dtparams

firmware: vc_image: Remove obsolete processor support using _VC_VERSION
firmware: vc_image: Include colourspace in RGB to YUV conversions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants