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.
Details
Details
- Reviewers
avg
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
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. | |
| 128 | What purpose does this character device serve? | |
| 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. | |
| 434 | Redundant spaces within "[]". | |
| 518 | redundant blank line | |
| 529 | redundant blank line | |
| 534 | redundant blank line | |
| 540 | redundant blank line | |
Comment Actions
Author update his patch and addressed most of the phabricator comments and update it for supporting APU1,2 and 3 boards.
Comment Actions
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. | |
| 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. | |
Comment Actions
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. | |
| 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. x = f();
if (x) {
...
}is easier to read and takes less visual space than x = f();
if (x) {
...
} | |
Comment Actions
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.
Comment Actions
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 ?
Comment Actions
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.