Skip to content
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

Add support for merus-amp soundcard and ma120x0p codec #3483

Merged
merged 1 commit into from
Mar 6, 2020
Merged

Add support for merus-amp soundcard and ma120x0p codec #3483

merged 1 commit into from
Mar 6, 2020

Conversation

AMuszkat
Copy link

This patch add support for merus-amp soundcard
and its codec ma120x0p. The code has been tested on a Raspberry pi ZW, pi 3B+ and Raspberry pi 4.
The soundcard consists in a Class-D amplifier HAT with a built-in boost converter.

@pelwell
Copy link
Contributor

pelwell commented Feb 24, 2020

I can't give a detailed review yet, but so far:

  1. It needs a README entry for the overlay.
  2. There are debug printks left that should be deleted.
  3. The commit message needs a Signed-off-by line.

@pelwell
Copy link
Contributor

pelwell commented Feb 25, 2020

The one in arch/arm/boot/dts/overlays.

@pelwell
Copy link
Contributor

pelwell commented Feb 25, 2020

  1. Unless register definitions are shared between multiple source files, Linux drivers are expected to put them at the start of the source file that uses them. In your case this means that the content of ma120x0p.h (or at least those bits that are used) needs to go in ma120x0p.c, and the pointless inclusion of ma120x0p.h in merus-amp.c needs to be deleted.
  2. The merus-amp driver looks like it could be replaced with an instance of the simple-audio-card driver - search the existing overlays for "simple-audio-card" for examples of its use.

@AMuszkat
Copy link
Author

makes sense, will try it and get back asap.

@pelwell
Copy link
Contributor

pelwell commented Feb 29, 2020

At first glance that's looking much better. I'll run a few checks and get back to you.

@pelwell
Copy link
Contributor

pelwell commented Mar 1, 2020

In general I'm reasonably happy with this PR. I'd prefer the codec to be upstreamed so we don't have to maintain it, but this wouldn't be the first downstream codec.

The changes I'd like to see are:

  1. the addition of the "interrupt" parameter to the README entry (or the removal of the parameter if it isn't useful), and
  2. the errors and style warnings from checkpatch fixed (I'm not bothered about the warnings asking for documentation of compatible strings, etc.):
    # First squash the commits together
    $ git rebase -i HEAD~2
    # In the editor, change the instruction for the second commit to "squash" or "fixup" - squash
    # lets you merge the two commit messages, fixup just deletes the second comment - then
    # save and exit.
    
    # Now check the commit
    $ git format-patch HEAD~1
    $ scripts/checkpatch.pl 0001-*
    # (assuming there is no other file name beginning 0001-)
    
    # Fix the errors/warnings and repeat the format-patch and checkpatch until only
    # the documentation-like warnings appear.
    
    # Now push the new version for review - you'll need the -f option
    $ git push -f your-remote-name HEAD:rpi-4.19.y
    # (substituting the real name of your remote)

@pelwell
Copy link
Contributor

pelwell commented Mar 6, 2020

I see you've pushed an update - GitHub doesn't send out notifications for that, so it's always a good idea to add a comment.

That's looking much better, but checkpatch still has a few complaints (I've deleted the ones I don't care about):

ERROR: do not set execute permissions for source files
#51: FILE: arch/arm/boot/dts/overlays/merus-amp-overlay.dts

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#56: FILE: arch/arm/boot/dts/overlays/merus-amp-overlay.dts:1:
+// Definitions for Infineon Merus-Amp

WARNING: please write a paragraph that describes the config symbol fully
#180: FILE: sound/soc/codecs/Kconfig:635:
+config SND_SOC_MA120X0P

ERROR: trailing whitespace
#1484: FILE: sound/soc/codecs/ma120x0p.c:1269:
+^I^Idev_err(&i2c->dev, $

ERROR: trailing whitespace
#1509: FILE: sound/soc/codecs/ma120x0p.c:1294:
+^I^Iif (ret != 0) $

total: 3 errors, 11 warnings, 1538 lines checked

@AMuszkat
Copy link
Author

AMuszkat commented Mar 6, 2020

Hi Phil,

Except for the error #51 I can not see anything wrong with the rest of them. Içm not sure why they are being checked as errors/warnings.

On the other hand, on error #56 I can not find any overlay file with SPDX-License Identifier on the first line, which I find it extremely strange too.

Any clues on how I can proceed?

@pelwell
Copy link
Contributor

pelwell commented Mar 6, 2020

Except for the error #51 I can not see anything wrong with the rest of them. I'm not sure why they are being checked as errors/warnings.

  1. chmod -x arch/arm/boot/dts/overlays/merus-amp-overlay.dts
  2. Some overlays do have them - I suggest:
    // SPDX-License-Identifier: GPL-2.0-only
  3. checkpatch seems to want a minimum of 4 lines, excluding the boiler plate. You can ignore this one.
  4. You have a trailing space ( ) on the end of line 1269.
  5. Ditto on line 1294.

correct checkpatch warnings and errors

Signed-off-by: AMuszkat <ariel.muszkat@gmail.com>
@AMuszkat
Copy link
Author

AMuszkat commented Mar 6, 2020

Done and checked! should I push it again?

@pelwell
Copy link
Contributor

pelwell commented Mar 6, 2020

Yes please, then I'll merge it if nothing else shows up.

@AMuszkat
Copy link
Author

AMuszkat commented Mar 6, 2020

Done

@pelwell pelwell merged commit 6a5bfd7 into raspberrypi:rpi-4.19.y Mar 6, 2020
@pelwell
Copy link
Contributor

pelwell commented Mar 6, 2020

Somewhere along the line we seem to have lost your original commit message. I can't change it now without a complete reversion, but if you post something suitable here I'll make sure it is applied to the newer kernels.

@AMuszkat
Copy link
Author

AMuszkat commented Mar 6, 2020

I'm actually seeing it in the rpi-4.19.y branch:
"Add support for merus-amp soundcard and ma120x0p codec"

@pelwell
Copy link
Contributor

pelwell commented Mar 6, 2020

Yes, but the body says:

correct checkpatch warnings and errors

@AMuszkat
Copy link
Author

AMuszkat commented Mar 6, 2020

ha ok! no problem for me if "Add support for merus-amp soundcard and ma120x0p codec" appears on the new kernels.

@pelwell
Copy link
Contributor

pelwell commented Mar 6, 2020

Thanks - will do.

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Mar 6, 2020
kernel: Add support for merus-amp soundcard and ma120x0p codec
See: raspberrypi/linux#3483

kernel: gpio-ir-overlay: add parameter to configure signal polarity
See: raspberrypi/linux#3490

kernel: overlays: sc16ic750-i2c: Fix xtal parameter
See: raspberrypi/linux#3156

kernel: configs: Add KVM support to arm64 bcm2711_defconfig
See: raspberrypi/linux#3035

firmware: Support Isp stats and params

firmware: arm_loader: Make EMMC2 dma-ranges patch more tolerant

firmware: bootromfs: Delete unwanted assert

firmware: usb_eth: Increase timeouts for TFTP requests and retransmit ACK
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Mar 6, 2020
kernel: Add support for merus-amp soundcard and ma120x0p codec
See: raspberrypi/linux#3483

kernel: gpio-ir-overlay: add parameter to configure signal polarity
See: raspberrypi/linux#3490

kernel: overlays: sc16ic750-i2c: Fix xtal parameter
See: raspberrypi/linux#3156

kernel: configs: Add KVM support to arm64 bcm2711_defconfig
See: raspberrypi/linux#3035

firmware: Support Isp stats and params

firmware: arm_loader: Make EMMC2 dma-ranges patch more tolerant

firmware: bootromfs: Delete unwanted assert

firmware: usb_eth: Increase timeouts for TFTP requests and retransmit ACK
@AMuszkat
Copy link
Author

AMuszkat commented May 7, 2020

Hi Phil,

Is there any target date for the next Raspbian update? The current one is still in 4.19.97 kernel and we would like to have support fot this sound card.

Best,
Ariel

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.

2 participants