Add BoomBerry support and support for mcp7940x family of RTC

[shawaj]

Add full support for BoomBerry audio cards.

On the RTC, the rtc-ds1307 driver specifies both the mcp7941x and the mcp7940x RTC families but only one is listed in overlays area. Thought it made sense to add explicit support for both.

(P.S. apologies in advance for number of commits - working in the browser due to lack of normal computer ATM - but I think with a new feature on GH you can squash and merge easily anyway from your side?)

[clivem]

Aaron, I'm going to refrain from saying anything negative...... Who is making/selling these Boomberry DAC HAT's? Are they just straight copies of HifiBerry Digi+ and DAC+?

[clivem]

I've been waiting for @pelwell to give you a clip around the ear for combining DAC and RTC in one pull request..... LOL

Aaron, do you have a donkey in this derby? Can't see any BoomBerry boards in your web shop? Can't seem to get anything back from Google either? Are they HiFiBerry DAC+ and Digi clones? Would appear so from driver. (I'm not sure the fustercluck machine driver is the way to go, Digi WM8804 and DAC PCM5122 from one machine driver, and duplication of existing code, anyway.)

This is what I do for Geekroo Digi. It gets the Geekroo name in alcacap output, which is usually all the clone manufacturer is concerned about, it not saying HiFiBerry...

clivem@2bfd82b

I'd be tempted to do the same for this BoomBerry Digi and something similar for the BoomBerry DAC board, with hifiberry_dacplus machine driver, assuming it is a clone?

[clivem]

I forgotten DanielM's github monicker. Need to check he is happy with overriding/setting those names from dt, in his machine driver.....

[shawaj]

@clivem not just straight copies of HiFiBerry no, although obviously using some of the same chips. Apart from chips they are a different design with different features.

Idea of single driver is ease of use for end users really.

Hopefully @pelwell will forgive me for my poor pull request ettiquette :-)

[clivem]

Apart from chips they are a different design with different features.

Where can I read about these features, or better yet, buy these boards? Is anyone actually using them out in the wild? Where are they being sold?

Idea of single driver is ease of use for end users really.

Which might be a valid line of thought, but for the fact that the dt is loading the module and the users point of entry is "dtoverlay=...." in config.txt, and you have a separate dt file for dac and digi, right?
Who gives a digerydoo which "named" modules are loaded? Not the user, as long as a module is loaded via dt and his hardware actually works, in my experience. ;)

[shawaj]

@clivem - not being sold yet, so nowhere to read about the features just yet.

About the driver - there are two separate dt files yes, but I still think that it makes more sense this way from a user perspective than to have a larger number of separate drivers. And also having more than one driver adds to extra config in terms of supporting the full range of cards in music player distributions etc (as well as a more confusing array of options for the user to choose from in those distributions). Think it makes more sense from all perspectives to have a single driver for these to support the entire range.

[clivem]

@shawaj If you don't want to confuse the users, then why not just use the existing drivers, that are already supported by the music player distros? "dtoverlay=hifiberry_digi" and "dtoverlay=hifiberry-dacplus". I cant see that new machine driver is adding anything that wasn't there already. Maybe I didn't look closely enough at the machine driver code you submitted, for the extra features. (I though it was a cut and paste job, from the existing berry_digi and berry_dacplus, into a single new file.)

[shawaj]

@clivem - to me, that seems far more confusing from an end user perspective

[clivem]

I'm obviously missing something here. That you can support a card using an existing driver and overlay, already in use by the distros, without adding one line of code or dt config, but you choose not to, confuses the hell out of me.....

[shawaj]

Isn't that exactly what is already done with iqaudio / hifiberry etc? If going down your route it would make far more sense to have a generic chip driver and everyone use that. That doesn't seem to be the general consensus, based on what already exists in the distros...

Unless I'm missing something here too, I am not really seeing your issue with this.

[clivem]

@shawaj I'll duck out of this conversation now, as I sure don't have a horse in this race, and hopefully Phil or Dom will explain why it would be OK to have as many dt configs as you want, but from a code maintainability point of view, unless a new card adds something that cannot be supported by an existing soc machine driver, it does not make sense to duplicate code that is already there.

[shawaj]

No worries Clive - thanks for your input. I see where you are coming from but I don't agree with what you're saying for a number of reasons, not least because of what already exists within the distros (among other reasons).

[shawaj]

@pelwell think I have now fixed the mess I created with the multiple commits. Let me know if that now looks ok

[clivem]

rpi-4.4.y is current branch now. I'm not sure why you have added non-dt initialisation code. Should be dt only now! I still have an issue that there is no new code in that machine driver, just duplication of existing code, but at least HiFiBerry DanielM now gets credit......

[pelwell]

1. What happened to separating the two drivers again? It will be much clearer, and easier for us in the future if we try to rationalise the current drivers.
2. Remove all changes to bcm2708.c and bcm2709.c - that has been replaced by Device Tree now.
3. I can still see three other commits - did the rebase not work?

When that looks OK either you or I can cherry-pick to 4.4.

For other readers: the current replication of code is less than ideal, but I don't think Aaron should be expected to fix the mess. As long as the code is clear, up-to-date and includes suitable attributions I'm prepared to accept it.

[shawaj]

1. sorry think I misunderstood what you were saying here about separating the drivers - can you confirm you want one for DAC/Amp, one for digi rather than all in one?

2. will do that, no problem

3. will remove those tomorrow

Thanks very much

[shawaj]

@pelwell this is all done now :-)

[pelwell]

You're almost there. You missed a few niceties about the overlay README format (80-column max, two blank lines between, etc.) and the overlays (the RTC had no parameter to enable it, there was a stray semicolon, and the frag0 label has to be added), but otherwise it is plausible. Probably as a result of merging then separating the drivers the startup and shutdown functions moved w.r.t. the hifiberry original, so I moved them back to aid comparison.

Can you tell me why you removed the assignment to the .dev field of the snd_soc_card structure? (This isn't a rhetorical question - you may have a good reason for it, but you may not).

My fixups (excluding the .dev issue) can be found here: https://gist.github.com/pelwell/2d88bd7875b9066437aaf6887e0cd4b0

When you have read and understood my changes, tested that it all compiles (it wouldn't have done before) and works, then update the PR. You can use git rebase -i HEAD~4 to copy the fixups below the relevant patches and change the "pick"s to "fixup"s (for the fixup commits only).

[shawaj]

@pelwell fixed all those issues now.

About the .dev issue, are you referring to this line - snd_rpi_boomberry_dac.dev = &pdev->dev; ?

[pelwell]

Yes I am.

[shawaj]

it is on lines 128 (dac) and 180 (digi)

[pelwell]

Ah yes - meld had grouped it in with another difference so it wasn't obvious.

I see that the whitespace on the digi startup/shutdown functions has changed from tabs to spaces, and that there are a number of other whitespace issues - trailing whitespace and DOS line-endings on the .dts files. git show HEAD^ will highlight the errors. If you are making a change, it would be nice if you could move the initialisation of the .dev fields back where they were to aid any future merge.

[shawaj]

strange about the whitespace errors as I am working on debian 8.

anyway, all fixed now for you (including moving the .dev fields)

[shawaj]

@pelwell - thanks for merging. what did you want to do about the cherry-pick to 4.4.y?

Also, meant to say, if you do have an idea of what you are wanting to do in terms of "fixing the mess" in terms of code replication etc. we wouldn't mind helping with that if that is at all beneficial.

[pelwell]

I've just done the cherry-pick to 4.4.y.

As for rationalising the number of near-duplicate drivers, the answer has to be to merge them all into a generic/superset driver, using the compatible strings to enable any per-board features. By keeping the code as similar as possible you have made this easier. @clivem is in the process of doing that for the IQaudIO driver, so either that code or something based on it would probably be part of the effort.

Unless you want to have a go at making some of the changes yourself, the main thing you can do is co-operate come the time.

[clivem]

@pelwell Can you apply this to rpi-4.4.y, on top of the cherry pick from rpi-4.1.y.

rpi-kernel-audio-0017-sound-soc-bcm-boomberry-dac-card.txt

[pelwell]

@shawaj Clive appears to be correct - the snd_soc_limit_volume call takes a struct snd_soc_card * as the first parameter, not a struct snd_soc_codec *. Why did you change that from the hifiberry_dacplus code?

[clivem]

In 4.1.y ...
int snd_soc_limit_volume(struct snd_soc_codec *codec, const char *name, int max);

In 4.4.y ...
int snd_soc_limit_volume(struct snd_soc_card *card, const char *name, int max);

Use codec ref in 4.1.y and card ref in 4.4.y.....

[pelwell]

Pushed.

[shawaj]

I am seeing the same in 4.1.y at line 171 as what we have - https://github.com/raspberrypi/linux/blob/rpi-4.1.y/sound/soc/bcm/hifiberry_dacplus.c

Is this just a difference for 4.4.y?

[pelwell]

It is - the ALSA API changed with 4.4.

[shawaj]

oh sorry, only just saw comments above!