Offer to upstream 2 downstream drivers

[10ne1]

Hi

I'm running the mainline kernel + device tree with two of your downstream drivers which I've forward-ported, bcm2835-virtgpio and iqaudio-dac. You can view my WIP branch which contains some minor cleanups, DT bindings & a dwc2 fix I've already sent to linux-usb.

I don't want to continously rebase these OOT drivers onto mainline & also check your downstream branches for changes to integrate, so the most obvious path for me is to just upstream them and let the mainline master branch be the integration point after that.

The virtgpio one is fairly simple with the mbox property firmware driver already upstream and only the iqaudio one is a little harder because of a nasty bug I've unearthed via a mmapped hrtimer-based audio player (playhrt) which I need to fix (bug is also present in your tree btw).

1. Are you ok with me upstreaming?
2. Do you know of any showstopper issues (I couldn't find previous mailing list driver submisions for these)?
3. Any suggestions?

[stschake]

I think the reason it hasn't been upstreamed is simply that upstream frowns upon drivers that are little more than a wrapper around a proprietary mailbox interface where the "real functionality" is implemented. You can try, but that's pretty much the same reason you won't ever see firmware KMS upstream or the rpi-ft5406 touchscreen "driver".

[10ne1]

Indeed, I missed that mainline has no other drivers like this "virual" mailbox led driver. Oh well, at least it's a simple standalone wrapper which doesn't see much development downstream (I'm talking about the virtual driver not the firmware it calls into) so it's not that big of a pain to maintain OOT. I'll focus on the iqaudio driver instead. Thanks!

[lategoodbye]

As BCM2835 maintainer i'm happy about all upstreaming efforts. I think the moderated mailing list linux-rpi-kernel is a better place to discuss such topics. Maybe you didn't noticed but the GPIO expander driver has already been merged for 4.17.

[popcornmix]

From our point of view we welcome any upstreaming effort and if upstream contains usable versions of downstream drivers we'll gladly switch to use the upstream versions. I can't say if the drivers will be accepted, but if @lategoodbye thinks it's worth pursuing then it's worth trying.

[10ne1]

Thank you @lategoodbye & @popcornmix , especially for the iqaudio code review. The feedback is very good and sensible and I'll implement the suggestions before sending a patch on the mailing list (I'm subscribed now).

However, as I said before, I still need to fix a nasty lock issue in the iquaudio driver before sending the patch. If you're curious, I've put the dmesg output here. Once that's sorted out I'll send a v2 patch.

[lategoodbye]

Hm, i'm not a audio/alsa expert, but i think you should concentrate more on the sleeping in the wrong context. Maybe the lock issue is only a inherited error. Don't wait to long in order to create the perfect code. It's better to send a RFC version of the patch to alsa-devel. Btw always run scripts/checkpatch.pl against your patches before submission.

[HiassofT]

I thought I fixed the lockdep warning a long time ago, but it looks like it slipped through. You can just use REGCACHE_FLAT instead of REGCACHE_RBTREE in bcm2835-i2s, it's a small register range and it's not sparse, so an RB tree doesn't make much sense.

I wouldn't bother trying to upstream the iqaudio dac driver, except for limiting the playback gain it doesn't do much. Instead just use the generic audio graph card driver and configure it via DT like this: https://gist.github.com/HiassofT/19546a6fa9bd146652211405438e3ff0

Upstream DT doesn't have 3.3V regulators exposed, you can either add them to the DT or just drop them from the overlay - in that case dummy regulators will be used, which is no big deal as the 3.3V rail will always be on.

[10ne1]

@lategoodbye Yes, I'm focusing on the first stack trace. I started digging from my userspace hrtimer player which mmaps the DAC card memory and then in a while(1) monotonic nanosleep loop reads from an input pipe directly in the mmaped region & commits it, resulting in some straightforward syscalls like these.

As can be seen at the end of the gist and from the kernel log trace, something happens in one of the snd_pcm ioctls triggering the BUG_ON() and the pipe read encounters an underrun (not sure yet how these two events might be connected) which closes the player prematurely. I'll dig further in the kernel to understand what's happening the next few days, time permitting.

The funny thing is this happens only once on the first playhrt invocation after boot, then everything looks and behaves normal (no tasks stuck in untinterruptible sleep etc). This is how I've been listening to music for the past 6 months: let the first player invocation crash then ignore the kernel stack traces :) Still do until I manage to figure out a fix.

@HiassofT Wow. Thank you! I'll definitely try both your suggestions. I've already added the 3v3 regulator for the pcm5122 i2c1 control interface in my upstream device tree, just didn't knew about that generic ASOC driver. If I could use that I'd have one less downstream thing to worry about.

[HiassofT]

IIRC the problem with regcache usage in the bcm2835-i2s driver is that there's no regmap access during probe() so the first regmap access, which allocates regcache memory, happens when the sound device is first accessed - but that's in a call chain with a spinlock held and irqs disabled in the alsa layer, hence the lockdep warning. Outputting the lockdep warning takes some time, especially when you have a serial console enabled, hence you'll also get an underrun error.

Switching to flat regcache is an easy fix, but probably it's better to recheck the I2S driver use of regcache.

I did a quick test with my iquadio dac on upstream kernel (used today's next) and uploaded an updated overlay https://gist.github.com/HiassofT/3570c680d8fa3c048e10f04fd41eb10b

Upstream dts claims gpio 4 by default (not really sure why, this looks a bit meh to me), so I addeed a fragment to remove it in the alt0 pinctrl claim and free it for use as AMP control output in the soundcard driver.

[10ne1]

@HiassofT Without your help it would have taken me many hours to figure out these things on my own, assuming I could in the first place. Again thank you, very awesome explanation, big thumbs up :+1:

The graph driver works very well with the provided bindings. Call me crazy but the audio quality improves significantly compared to the iqaudio driver. I'm assuming it's because it doesn't apply gain processing on the stream and that is also why the db level increased significantly (doesn't bother me).

The REGCACHE_FLAT fix also works, I get no more stack traces. I'm not very familiar with the bcm2835-i2s code but I'll try to have a look maybe I can come up with something sensible.