This is the driver for ACPI attached GPIO controller in AMD Platforms. This driver is basically a port of "drivers/pinctrl/pinctrl-amd.c" driver in Linux. This patch has the basic functions of GPIO, leaving interrupt related support.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Not yet. HEAD is in freeze for 12.0 release anyway, so it can't be committed at the moment. I'll look into it next week.
Comment Actions
Hi Gonzo,
Sorry to trouble again. Did you get a chance to look at this? From the release schedule, seems FreeBSD 12.0 is scheduled for release in Nov 2018. By any chance, could this patch be taken before 12.0 release?
| sys/dev/amdgpio/amdgpio.c | ||
|---|---|---|
| 165 | This line is redundant. The value is overwritten by the next line. | |
| 187 | This one is also redundant. | |
| 363 | sc_bank_prefix is assigned but never used. I think since there is just one bank it's better to remove this field altogether. | |
| sys/dev/amdgpio/amdgpio.h | ||
| 117 | All static variables should be moved to C file from the header. There is no reason to keep it in .h file as far as I can see. | |
Comment Actions
No worries, sorry for the delay. Unfortunately, this driver can't make it to 12.0 since the HEAD branch is in code freeze starting from August 24th. During the code freeze, only bugfixes are allowed to be committed with explicit approval by the release engineering team. Technically new features should be avoided even during the code slush but it's less strictly enforced comparing to the code freeze. Details about the release process can be found here: https://www.freebsd.org/releases/12.0R/schedule.html
Once the code is cleaned-up and code freeze is over I'll commit it to the HEAD (which will be 13-CURRENT at that point) and MFC to older branches.
Comment Actions
- Removed static keywords from pin grouping definitions in H file.
- Moved static struct "resource_spec" to C file.
- Removed unwanted static declaration of probe, attach and detach in H file.
- Removed redundant file in C file
Comment Actions
Hi Gonzo,
Please find attached the new revision for this patch. Also, please look at the inline comments and let me know if you have any feedback.
One question, I have used tabs for spacing in H file. When I look at the file in vim, things are aligned. But here(in phabricator), I see misalignment in macros definition. Is this normal with phabricator?
Thanks.
| sys/dev/amdgpio/amdgpio.c | ||
|---|---|---|
| 363 | This driver has 4 banks (please see sc_nbanks above). There is some plan to implement a "show" routine later for this driver, which shows the pin details bank wise. That's why I have the definitions for sc_banks and sc_bank_prefix. But, if you insist this being removed for now (since it's not used now), I will remove it. We need to implement functions to configure gpio pins as interrupt line. For that, we need the INTRNG framework support for amd64 platforms. Since, it's not available now, we haven't done it now. Once this is implemented, we will have the "show" routine as well. | |
| sys/dev/amdgpio/amdgpio.h | ||
| 117 | Originally intent is to keep functions and pin group structure definitions seperately in C and H files respectively. So, removed the static keywords from the header file. But, a) Removed the unwanted static declaration of probe, attach and detach function in H file and made appropriate changes in C file (in attach routine, not calling detach in case of failure) b) Moved "resource_spec" definition alone to C file (as done in most of the drivers) | |
Comment Actions
Thanks for taking time to review the first patch and clarifying on the release process.
Comment Actions
One question, I have used tabs for spacing in H file. When I look at the file in vim, things are aligned. But here(in phabricator), I see misalignment in macros definition. Is this normal with phabricator?
I think sometimes phabricator misaligns things but in this case, there are mixes of spaces/tabs in header file that violate style(9). I'll post comments separately. Thanks for pointing it out.
| sys/dev/amdgpio/amdgpio.h | ||
|---|---|---|
| 117 | I misunderstood the intention then, sorry. My line of thinking was: if it's a static variable and used only in one compilation unit, it's better to be moved to C file because it's naturally belongs there - like a resources definition and internal methods declarations. It wasn't the "static" keyword I objected to, it was the location of the data itself. But if you want to keep pins information in the header file as a kind of constant for code readability purpose then the original usage of the "static" keyword was correct and my comment was wrong. Please add "static" back to kernzp_pins, kernzp_groups, and *_pins constants. | |
| sys/dev/amdgpio/amdgpio.h | ||
|---|---|---|
| 29 | style(9) requires tab character after the #define. There are several instances of #define<space> in this file | |
| 42 | Values of AMD_GPIO_PINS_BANK0..AMD_GPIO_PINS_BANK4 are aligned using mix of spaces and tabs, should be just tabs | |
| 61 | Values in this section are aligned only by spaces, should be tabs. | |
| 112 | fields indented using four spaces, should be single tab instead. | |
| 259 | There is a tab character after every "}," in this array. There should be no trailing spaces. There are also several instances of trailing spaces/tabs in C file too | |
Comment Actions
To be honest, the current code freeze seems quite... flexible. As in, there's certainly some new stuff coming in, and from my understanding adding a new driver (as opposed to modifying something that's already there) is rather unlikely to break something. Maybe just send an email to re@ and see what they say?
Comment Actions
- Added the static keywords to those mentioned structures in H file, as the intent is code readability
- Cleared the unwanted whitespaces/tabs and followed style(9) format for indentation in both C and H file.
Comment Actions
Hi Olexsandr,
Addressed all your comments. Also, taken care of the whitespace and tabs formatting. Please see if anything against the style guidelines still. Thanks for your time again.
Comment Actions
Hi Rajesh,
The code looks fine now. One thing though, it's disconnected from the build. You might want to take a look at sys/modules/Makefile and search for _bytgpio string to see how you can make it x86_64 only
Comment Actions
Sorry for delays with answers
To be honest, the current code freeze seems quite... flexible. As in, there's certainly some new stuff coming in, and from my understanding adding a new driver (as opposed to modifying something that's already there) is rather unlikely to break something. Maybe just send an email to re@ and see what they say?
Yes, and IMHO this flexibility is not a very good thing (speaking from personal experience as a part-time RE at $WORK). I don't want to submit more new feature changes to re@ that is strictly necessary and so far I can't find pro arguments for this driver. As far as I understand this driver is going to be used as a building block for other components that are going to use it either as a control method for peripherals or as an interrupt source. If I am wrong and there are readily available immediate use cases, for instance, AMD-based embedded platforms where GPIO can be used from the userland or AMD-based laptops where gpioctl can be used directly to enable certain functionality that is otherwise unavailable I'll bring the patch up to re@ for the consideration. Otherwise, I'd rather wait for the code thaw and submit it to -HEAD with further MFCs to -12 and -11, it will be available in 12.1 in a year or so.
Rajesh, if you're familiar with the solutions available on the market could you share such use cases so I could argue for the inclusion of the driver?
Thanks
Comment Actions
Hi Oleksandr,
Sorry about the delayed response.
Addressed your comments regarding build in "sys/modules/Makefile" and "sys/conf/files.amd64". With these changes, tried compiling the kernel with and without "gpio" and "amdgpio" in the custom kernel config file. When "amdgpio" is added as part of kernel config, faced some compilation issue because of the "ofw" related header file inclusions. So, removed that, as that is unnecessary here. With all these changes, things goes fine.
Also, made the following minor changes. Sorry about having these after all the review. But I feel it's worth it.
- Added a check for "Output" capability of the pin "pin_set" and "pin_toggle" routines in order to make writing to "Input" pin Invalid.
- Made minor change to "pin name" setting in "amdgpio_attach" to have appropriate bank number in the name.
Comment Actions
Hi Oleksandr,
Typical use cases we have come across so far is to use it as a interrupt source for few devices and LED functions. For now, we have just enabled the driver for basic IO operations. It would be good if we have this included in 12.0, but if it's really not good to have new feature/driver sent to @re at this point, we can still manage with MFC to stable/12 and stable/11.
Comment Actions
This is the only change left I believe
| sys/modules/Makefile | ||
|---|---|---|
| 742 ↗ | (On Diff #48898) | The list of the conditional modules is sorted alphabetically, this line should go right after the .if ${MACHINE_CPUARCH} == "amd64" |
Comment Actions
Hi Oleksandr,
Addressed the comment about alphabetical order in "sys/modules/Makefile". Seems, "sys/conf/files.amd64" also needs the modules to be listed in alphabetical order. Corrected that as well.