What to do with an Ubnt airCube LED driver?

Hello,

I added a port for the airCube some weeks back. But it is missing a LED driver because the system developers decided that they need a custom controller for the LED. Back when I analyzed the system, I already had a look at the protocol of the controller but didn't write a driver for it. See: https://openwrt.org/toh/ubiquiti/ubiquiti_aircube_isp#led-controller or https://github.com/c-mauderer/openwrt-Ubiquity-Aircube-notes/blob/master/notes/Notes.asciidoc#led-controller for the protocol.

Just today I added a driver that supports at least the brightness settings. It seems that OpenWRT doesn't do any more than switching the LED on or off so I thought that would be enough. The patch can be found here: https://github.com/c-mauderer/openwrt/commit/c7988f23c33c4242e60d0b2b035ce1a597a65d29

Now I need some guidance what to do with the driver:

  • Is a patch that adds a special LED driver to the kernel acceptable for OpenWRT?
  • Or would it be preferable for the driver to be in a package?
  • As third option: Should I try to push the patch into the Linux Kernel and backport it from there to OpenWRT? Do I even have a chance that the Kernel developers accept such a patch?

Best regards

Christian

1 Like

With the caveat that I'm not a core developer...

In my opinion, something is a lot better than nothing, and I see that you've used the ath79 platform, so that's a good choice.

I would suggest that you post it to the openwrt-devel mailing list or open a pull request. You'll get some good feedback and be able to get it into even better shape before considering an upstream submission.

One thing that caught my eye was that it looks like you used six spaces for indentation. Both the OpenWrt team and upstream "insist on" Linux-style formatting, tab indentation in particular. Here's one reference that might make your code not raise eyebrows when it lands

https://www.kernel.org/doc/html/v4.10/process/coding-style.html

(You also might want to spell out "Ubiquiti" in at least the comments in your code.)

The reason why putting it upstream first might could be a better idea is that it would avoid an increased maintenance effort here at OpenWRT. But I'm not sure what the best practices are for that in this project. Therefore I wanted to ask first. Of course, I can do that in a pull-request too.

Where did you find six spaces in the kernel patch? I'm not really familiar with the Linux kernel coding style but I tried to stick on the style surrounding the code. The only location where I found some spaces in other led-drivers was to align broken long lines to the brackets of functions or equal signs so I used them there too. All other indentations should be tabs.

For searching the code that might is a good idea. I'll do that in the kernel patch before submitting it anywhere.

Six spaces was from looking on Github directly. Might be fine in the source. Just something that caught my eye that I think would be worth checking before taking another step.

For me, I think there is a “utility” bar for OpenWrt that is a little lower than those of Linux. The OpenWrt reviewers seem to take “upstream-ability” into consideration. For me, I’d prefer those eyes and suggestions before a “nobody” submits to upstream and I get hammered or ignored.

Although I don't find any white space bugs it was a good hint to take a more detailed look at the kernel patch anyway. I used the Linux ./scripts/checkpatch.pl script and received at least some warnings. I have to add at least the devicetree/bindings documentation and it seems that it is considered good style to put the led in a sub-node in the device tree even if the device can only support one LED. So my patch will need some polishing.

1 Like

Didn't know about that one -- will probably save me some pain soon!

I had contact with the script some time back when I tried to submit a tiny patch to the kernel. Someone else was faster in submitting a slightly better version of the same patch but I learned about that script. So it was a positive result for me anyway.

Regarding the patch: I updated it with a version where checkpatch.pl has less warnings (only "does MAINTAINERS need updating" left) and will try a pull request in OpenWRT to get some feedback.

Updated patch see: https://github.com/c-mauderer/openwrt/commit/a631709d32e46aa85d2dce0b0e5190c67c9e0ff2

1 Like

This topic was automatically closed 10 days after the last reply. New replies are no longer allowed.