-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add fixed voltage regulators to audio overlays using pcm512x, wm8804 and adau1977 codecs #1479
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 fixed voltage regulators to audio overlays using pcm512x, wm8804 and adau1977 codecs #1479
Conversation
76874ae
to
facc9a4
Compare
I think better way writes two overlay pcm512x-regulator-overlay, wm8804-regulator-overlay and them included in codec overlays. |
facc9a4
to
cc3ec07
Compare
@clivem you also need to reference the regulators in the codec DT nodes. Note that the supply prroperties are required, not optional, as per the devicetree binding documentation. See for example So it can be argued that the current overlays are broken and need fixing anyways. BTW: you don't need to define multiple regulators, one per voltage is enough (even a single one for all voltages would do to make DT and the regulator core happy). eg something like this:
|
cc3ec07
to
2f5ef46
Compare
@clivem Thanks - this could be a useful resource. @HiassofT Thanks also for your input. Both - it's a tremendous help having guidance from specialists in the field. @kukabu No, I think this is the correct approach, because we don't want all users to have to change the name of the overlay they use in order to continue using their soundcard just because we've enabled regulators. However, this PR is something we will keep around until the day that regulators are enabled, at which point we'll merge it. |
2f5ef46
to
8e3ebe6
Compare
@HiassofT Matthias, is that OK? EDIT: By which I mean, is it done in an upstream compatible way? (I know it works. Tested on HB digi and IQ pcm512x. "fixed" regulator module auto-loaded, no complaints from drivers, and music playing. ;)) |
8e3ebe6
to
13eca40
Compare
I think it'd be best to move the 3.3v regulator definition to bcm2708_common.dtsi - or some common file that's included by all RPi boards. The RPis have a fixed 3.3V regulator on board and most of the system and addon cards are running off that, so the regulator should be part of the board definition. Not sure under which root node it should be anchored ("/regulators", "/motherboard" maybe?) but I'm pretty sure there's some convention for that. Ping @pelwell I think you are more familiar with the conventions used in the (RPi) devicetree structure than I am |
Under |
c0e0305
to
0c057a2
Compare
I like it. It's as neat as it's going to get, and without checking each of the codecs and their regulator requirements I don't see anything wrong with it. |
0c057a2
to
d182f19
Compare
Thanks, looks fine to me, too! While we're at it I think it might make sense to register a 5V fixed regulator as well - just to be prepared if some HAT or onboard component is going to require this. |
d182f19
to
ad2a187
Compare
Do we want to add a def for 1V8 as well while we are at it? ISTR, the chap trying to merge that Fe-Pi, had a 1V8 def for his LDO reg. On second thoughts, might be better for everything other than the 3.3V and 5V, to be kept in the overlays. ;) |
The 1V8 reg in that codes seems to be a "real" regulator that'll be switched on/off as needed - that needs to be properly setup in the specific DT overlay/driver. Only 5V and 3V3 fixed rails are available on the RPi board, so it's sufficient to add these to the DT. |
I didn't actually look at it again, just had it in the back of my mind from way back when, (sgtl5000 on Wandboard), regulator was 1V8. Also had it in mind that DVDD can be either 1V8 or 3V3 for PCM512x. |
I've just done a quick test with the patch applied and ...
.... on B, B+, Zero, 2B, 3B and various HB and IQ boards attached, that use the regs. All looks good. |
ad2a187
to
a3e1e01
Compare
@pelwell Of course, up to you, but having tested this patch with and without enabling regulator framework, I cannot find a reason for not wanting to apply it now, even if you don't intend to enable regulator framework now. |
I think it would make more sense to apply with the regulator framework enabled so if something breaks we notice that immediately, not a few months later when we may have forgotten about this. @pelwell @popcornmix are there plans to revive firmware-next to test new features? This PR would be a good candidate IMHO. @clivem Otherwise a PR on the 4.6 tree might make sense, that one is used by Milhouse for his LibreELEC builds so it'll get some test coverage. |
Re: BRANCH=next I've created a "next" branch in the kernel tree so that it can be an alternative branch using the same kernel version, rather than having to be the next version, but we were waiting for a candidate implementation of the ARM-side clock manager that works with I2S audio before making the first release. However, I've got no problem cleaning the pipes with this small patch. |
a3e1e01
to
966e458
Compare
ec92b04
to
694ed6c
Compare
87853d1
to
a7edc09
Compare
NB. Comment out the regulator defs for the moment. If raspberrypi#1479 ever gets accepted, they can be un-commented.
NB. Comment out the regulator defs for the moment. If raspberrypi#1479 ever gets accepted, they can be un-commented.
NB. Comment out the regulator defs for the moment. If raspberrypi#1479 ever gets accepted, they can be un-commented.
NB. Comment out the regulator defs for the moment. If raspberrypi#1479 ever gets accepted, they can be un-commented.
c826aa6
to
a653ae0
Compare
a653ae0
to
4ba442a
Compare
Will be used if CONFIG_REGULATOR and CONFIG_REGULATOR_FIXED_VOLTAGE are enabled, otherwise ignored. Version 3, putting the 3v3 reg into bcm2708_common.dtsi. Version 4, adding a def for a 5V fixed regulator. Version 5, fix-up whitespace in adau1977-adc-overlay.dts and move i2s fragment to fragment@0. Version 6, rename boomberry -> justboom. Version 7, uncomment reg defs in iqudio-digi-wm8804-audio overlay. Version 8, add regs to hifiberry-digi-pro overlay. Patch has now been tested, (with and without regulator framework enabled), with Pi Zero, A+, B, B+, 2B, 3B and various PCM512x and wm8804 boards. Signed-off-by: DigitalDreamtime <clive.messer@digitaldreamtime.co.uk>
4ba442a
to
48f4f3d
Compare
Phil/Dom, feel free to close straight away if you want without pulling. But if you ever do enable REGULATOR, you might want to have these reg defs already in the overlays affected.
This stops the codecs using regs, (specifically pcm512x and wm8804), from refusing to load because they cant find the necessary regs.