LED heartbeat trigger

[apritzel]

So why this new module? There is already a DT facility to achieve exactly this: (from Documentation/devicetree/bindings/leds/common.txt)

gpio-leds {
        compatible = "gpio-leds";

        system-status {
                label = "Status";
                linux,default-trigger = "heartbeat";
                gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
        };
};

This gives you even loadavg derived frequency. The only thing you have to figure out now is how to describe the LED properly, arch/arm/boot/dts/sun8i-h3-nanopi.dtsi should give an example.

[longsleep]

This project was created as example of a very simple Kernel module which utilizes gpio with a kernel thread as part of a small educational project as a showcase how easy it is to make a out-of-tree kernel module from scratch.

[longsleep]

Thinking of that, i should probably add a hint that the behavior of this module can be replicated with gpio-leds standard module with heartbeat trigger.

[apritzel]

Well, if it is an educational project, it should better be teaching the right things ;-) So apart from minor stylistic issues (CamelCase identifiers or C++ comments) and a lack of error checking (what if the GPIO is already assigned?) there is an architectural issue: this code looks like a platform driver, but it shouldn' be. It does not drive any hardware (as it relies on the existing GPIO framework), nor does it provide any services. In this respect the DT connection is really wrong: The DT node is only described by its compatible string, but doesn't contain any other properties, not even the actual GPIO to use. Instead this is provided as a module parameter, which sounds a bit awkward.

So if the purpose of this code is to provide an example of a kernel module utilizing threads, I'd remove all of the DT bits, as it is really misleading and does not provide any benefit. And frankly it makes the module much harder to use, as one has to introduce a DT node in the first place. Also note that such a task (mostly waiting) could be probably better solved by using queue_delayed_work().

[longsleep]

Seems like that i get educated by this project as well :) - very much appreciated.

I agree that the platform driver code sends the wrong message. The module started without it and then mutated to showcase a little DT integration. I will think about it and either remove it or split it up. Originally i wanted to add DT node support for the GPIO pin but ran out of time ...

[longsleep]

Btw, do you know a good .editorconfig example for Linux Kernel code. The camelCase and C++ comments could be easily detected and linted in any compatible editor. I will look into that too.

[apritzel]

Re: coding style> I am not aware of any kernel specific editor checks, most people just use a C syntax highlighting and know the rest by heart ;-). However scripts/checkpatch.pl checks against the official coding style rules and is quite elaborate, call it with -f to check a file instead of a patch: total: 1 errors, 10 warnings, 112 lines checked And usually some kind of review finds the remaining issues not covered by checkpatch.

If you are looking for a DT example, I found drivers/reset/reset-sunxi.c simple enough to showcase a DT enabled platform driver. There is some rework underway to make it even easier. But much of DT related code parts are nowadays handled in higher layers (of_address_to_resource(), for instance), so isn't very educational anymore (in terms of: look, here is parses the "reg" property).