apuled(4): add support for LEDs on PC Engines APU boards

[olivier]

Summary

PR 189772 includes a cool drivers for PC Engines APU 1 and 2.
Here is a phab review for adding more reviewer to this PR.

[avg]

Besides the style issues I have another concern.
Now we would have at least two drivers that can concurrently access AMD PMIO registers. Given that the access is through in an index-data pair of registers, there is a possibility of a conflict. We need to think how to protect those registers from conflicting accesses.

sys/modules/apuled/apuled.c

59

You can include that header file here if that helps to avoid duplicate definitions.
See intpm.c for an example.

128

What purpose does this character device serve?
I thought that hooking into led(4) infrastructure would be sufficient.

155

General suggestion: if you move these module and driver definitions to the end of file, then everything wouldcome in its natural order and you wouldn't have to declare prototypes for the methods.

157

It's possible to use bool here if that would make the code clearer.

166

The return value of a function call can not be assigned to, so the usual argument of using this weird order of comparison operands does not apply here :-)

254

New line after break feels redundant.
There are a few more instances of this elsewhere.

434

Redundant spaces within "[]".
There are several more cases like this.

518

redundant blank line

529

redundant blank line

534

redundant blank line

540

redundant blank line

[olivier]

Author update his patch and addressed most of the phabricator comments and update it for supporting APU1,2 and 3 boards.

[olivier]

The updated code should answer to all remarks.
About the sys/modules/Makefile diff: I've synced my FreeBSD src tree to the last revision which updated this file, and when I've updated this review it had included by mistake the change.

sys/modules/apuled/apuled.c

59

It was not an header file, but a .c : typo fixed on the comment.

[lab_gta.com]

sys/modules/apuled/apuled.c

59

Header file no longer exists. It was combined with associated c module.

128

The APU boards also support a mode switch on the front of the case. This device can be used to get current status of switch.

157

This needs to be an int. Return value is used to determine what type of APU board was found.

254

Removed extra line breaks.

434

Removed redundant spaces around all "[]".

518

Removed redundant blank line.

529

Removed redundant blank line.

540

Removed redundant blank line.

[avg]

Please address a few minor style comments. Please consider using amd_chipset.h.
I am still somewhat concerned about possible clashes on concurrent accesses to the PMIO space.

sys/modules/apuled/apuled.c

59

What I meant was sys/dev/amdsbwd/amd_chipset.h which is alive and well.

154

In the FreeBSD style we do not place a space after an opening parenthesis or before a closing one.
Please remove the extra spaces here and in other function signatures.

157

okay

185

Too many empty lines in this function's body and in those of the next few functions. I do not see any code that really has to be emphasized by new lines.
E.g.

x = f();
if (x) {
  ...
}

is easier to read and takes less visual space than

x = f();

if (x) {
  ...
}

[avg]

Is there still an interest in getting this moving forward?
I think that it should be relatively easy to address my comments and get this into a committable shape.

[olivier]

In D9876#280057, @avg wrote:

Is there still an interest in getting this moving forward?
I think that it should be relatively easy to address my comments and get this into a committable shape.

As APU2 user, I'm very interested by getting this moving forward… but I'm not a C developer then I can't address your comments: Peraphs original author (@lab_gta.com) can do it ?

[rb_gid.co.uk]

In D9876#280057, @avg wrote:

Is there still an interest in getting this moving forward?
I think that it should be relatively easy to address my comments and get this into a committable shape.

We're using this code in production., it would be helpful to have it committed.

[Arne.Ehrlich_groknet.de]

Tested this on an APU3 Board. Works great;
but i wonder if it would be smarte to write a gpiobus(4) driver instead and support the other remaining GPIO pins as well.

Addition: the APU2/3 GPIOs are on an other IO controller, and would probably require a different driver, independent of this one.

[olivier]

Updating the patch to latest -head.