gpiolib: split character device into gpiolib-cdev
Kent Gibson
reviewerPATCH v2 1/1 gpiolib: split character device into gpiolib-cdev
Split the cdev specific functionality out of gpiolib.c and into gpiolib-cdev.c. This improves the readability and maintainability of both the cdev and core gpiolib code.
---
While this patch is complete and ready for review, I don't expect it to be applied as is. There are a few cdev patches pending merge into gpio/devel that are sure to conflict, and it makes more sense to rebase this on top of them than vice versa. But I thought it would be worthwhile to get it out for review so it can be ready to be rebased when the time is right.
Also, this is a naive split. That is, if applied as is, it will lose the line history of the cdev code. This is not what I intend, and I understand can be avoided by encouraging git to remember the history with a few moves, but I'm unsure how the maintainers would prefer that to be done.
Bart,
As this was your idea, I've taken the liberty of adding the Suggested-by. I hope that is ok.
Changes in v2:
- rebased to latest gpio/devel and added base-commit to placate the build bot. The comments above still apply, as there are still a couple of commits in gpio/fixes that will conflict.
Kent.
drivers/gpio/Makefile | 1 + drivers/gpio/gpiolib-cdev.c | 1148 +++++++++++++++++++++++++++++++++++ drivers/gpio/gpiolib-cdev.h | 11 + drivers/gpio/gpiolib.c | 1112 +-------------------------------- 4 files changed, 1164 insertions(+), 1108 deletions(-) create mode 100644 drivers/gpio/gpiolib-cdev.c create mode 100644 drivers/gpio/gpiolib-cdev.h
drivers/gpio/Makefile +1 -0
@@ -7,6 +7,7 @@ obj-$(CONFIG_GPIOLIB) += gpiolib.o obj-$(CONFIG_GPIOLIB) += gpiolib-devres.o obj-$(CONFIG_GPIOLIB) += gpiolib-legacy.o obj-$(CONFIG_GPIOLIB) += gpiolib-devprop.o +obj-$(CONFIG_GPIOLIB) += gpiolib-cdev.o obj-$(CONFIG_OF_GPIO) += gpiolib-of.o obj-$(CONFIG_GPIO_SYSFS) += gpiolib-sysfs.o obj-$(CONFIG_GPIO_ACPI) += gpiolib-acpi.o
drivers/gpio/gpiolib-cdev.c new file +1148 -0
1,148 lines not on this page · raw message
drivers/gpio/gpiolib-cdev.h new file +11 -0
@@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef GPIOLIB_CDEV_H +#define GPIOLIB_CDEV_H + +#include <linux/device.h> + +int gpiolib_cdev_register (struct gpio_device *gdev,dev_t devt); +void gpiolib_cdev_unregister (struct gpio_device *gdev); + +#endif /* GPIOLIB_CDEV_H */
drivers/gpio/gpiolib.c +4 -1108
1,153 lines not on this page · raw message
base-commit: a8173820f441ab3e2a45c4bb66b70da9a57a349e
Signature · 1 line
2.26.2
wt., 2 cze 2020 o 16:11 Kent Gibson <[email protected]> napisał(a):
Quoted 26 lines
Split the cdev specific functionality out of
gpiolib.cand intogpiolib-cdev.c. This improves the readability and maintainability of both the cdev and core gpiolib code.Suggested-by Bartosz Golaszewski <[email protected]>Signed-off-by Kent Gibson <[email protected]>---
While this patch is complete and ready for review, I don't expect it to be applied as is. There are a few cdev patches pending merge into gpio/devel that are sure to conflict, and it makes more sense to rebase this on top of them than vice versa. But I thought it would be worthwhile to get it out for review so it can be ready to be rebased when the time is right.Also, this is a naive split. That is, if applied as is, it will lose the line history of the cdev code. This is not what I intend, and I understand can be avoided by encouraging git to remember the history with a few moves, but I'm unsure how the maintainers would prefer that to be done.
Bart,
As this was your idea, I've taken the liberty of adding the Suggested-by. I hope that is ok.
Quoted 6 lines
Changes in v2:
- rebased to latest gpio/devel and added base-commit to placate the build bot. The comments above still apply, as there are still a couple of commits in gpio/fixes that will conflict.Kent.
Thanks for doing this Kent!
This looks mostly good, see a single comment below.
Linus: do you think we can get this in for v5.8? Maybe apply this as the last patch before your PR?
Quoted 49 lines
drivers/gpio/Makefile | 1 + drivers/gpio/gpiolib-cdev.c | 1148 +++++++++++++++++++++++++++++++++++ drivers/gpio/gpiolib-cdev.h | 11 + drivers/gpio/gpiolib.c | 1112 +-------------------------------- 4 files changed, 1164 insertions(+), 1108 deletions(-) create mode 100644 drivers/gpio/gpiolib-cdev.c create mode 100644 drivers/gpio/gpiolib-cdev.hdrivers/gpio/Makefile +1
-0@@ -7,6 +7,7 @@ obj-$(CONFIG_GPIOLIB) += gpiolib.o obj-$(CONFIG_GPIOLIB) += gpiolib-devres.o obj-$(CONFIG_GPIOLIB) += gpiolib-legacy.o obj-$(CONFIG_GPIOLIB) += gpiolib-devprop.o +obj-$(CONFIG_GPIOLIB) += gpiolib-cdev.o obj-$(CONFIG_OF_GPIO) += gpiolib-of.o obj-$(CONFIG_GPIO_SYSFS) += gpiolib-sysfs.o obj-$(CONFIG_GPIO_ACPI) += gpiolib-acpi.odrivers/gpio/gpiolib-cdev.c new file +22
-0@@ -0,0 +1,1148 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/bitmap.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/interrupt.h> +#include <linux/irqreturn.h> +#include <linux/spinlock.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/gpio/driver.h> +#include <linux/pinctrl/consumer.h> +#include <linux/cdev.h> +#include <linux/uaccess.h> +#include <linux/compat.h> +#include <linux/anon_inodes.h> +#include <linux/file.h> +#include <linux/kfifo.h> +#include <linux/poll.h> +#include <linux/timekeeping.h> +#include <uapi/linux/gpio.h>
drivers/gpio/gpiolib-cdev.c line 32
Quoted 10 lines
drivers/gpio/gpiolib-cdev.c new file +10
-0+ + +#include "gpiolib.h" + +/* Implementation infrastructure for GPIO interfaces. +* +* The GPIO programming interface allows for inlining speed-critical +* get/set operations for common cases, so that access to SOC-integrated +* GPIOs can sometimes cost only an instruction or two per bit. +*/
Is this comment relevant for the character device?
Bartosz
Kent Gibson
reviewerQuoted 94 lines · Bartosz Golaszewski
wt., 2 cze 2020 o 16:11 Kent Gibson <[email protected]> napisał(a):
Quoted 26 lines
Split the cdev specific functionality out of
gpiolib.cand intogpiolib-cdev.c. This improves the readability and maintainability of both the cdev and core gpiolib code.Suggested-by Bartosz Golaszewski <[email protected]>Signed-off-by Kent Gibson <[email protected]>---
While this patch is complete and ready for review, I don't expect it to be applied as is. There are a few cdev patches pending merge into gpio/devel that are sure to conflict, and it makes more sense to rebase this on top of them than vice versa. But I thought it would be worthwhile to get it out for review so it can be ready to be rebased when the time is right.Also, this is a naive split. That is, if applied as is, it will lose the line history of the cdev code. This is not what I intend, and I understand can be avoided by encouraging git to remember the history with a few moves, but I'm unsure how the maintainers would prefer that to be done.
Bart,
As this was your idea, I've taken the liberty of adding the Suggested-by. I hope that is ok.Quoted 6 lines
Changes in v2:
- rebased to latest gpio/devel and added base-commit to placate the build bot. The comments above still apply, as there are still a couple of commits in gpio/fixes that will conflict.Kent.
Thanks for doing this Kent!
This looks mostly good, see a single comment below.
Linus: do you think we can get this in for v5.8? Maybe apply this as the last patch before your PR?
Quoted 53 lines
drivers/gpio/Makefile | 1 + drivers/gpio/gpiolib-cdev.c | 1148 +++++++++++++++++++++++++++++++++++ drivers/gpio/gpiolib-cdev.h | 11 + drivers/gpio/gpiolib.c | 1112 +-------------------------------- 4 files changed, 1164 insertions(+), 1108 deletions(-) create mode 100644 drivers/gpio/gpiolib-cdev.c create mode 100644 drivers/gpio/gpiolib-cdev.hdrivers/gpio/Makefile +1
-0@@ -7,6 +7,7 @@ obj-$(CONFIG_GPIOLIB) += gpiolib.o obj-$(CONFIG_GPIOLIB) += gpiolib-devres.o obj-$(CONFIG_GPIOLIB) += gpiolib-legacy.o obj-$(CONFIG_GPIOLIB) += gpiolib-devprop.o +obj-$(CONFIG_GPIOLIB) += gpiolib-cdev.o obj-$(CONFIG_OF_GPIO) += gpiolib-of.o obj-$(CONFIG_GPIO_SYSFS) += gpiolib-sysfs.o obj-$(CONFIG_GPIO_ACPI) += gpiolib-acpi.odrivers/gpio/gpiolib-cdev.c new file +26
-0@@ -0,0 +1,1148 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/bitmap.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/interrupt.h> +#include <linux/irqreturn.h> +#include <linux/spinlock.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/gpio/driver.h> +#include <linux/pinctrl/consumer.h> +#include <linux/cdev.h> +#include <linux/uaccess.h> +#include <linux/compat.h> +#include <linux/anon_inodes.h> +#include <linux/file.h> +#include <linux/kfifo.h> +#include <linux/poll.h> +#include <linux/timekeeping.h> +#include <uapi/linux/gpio.h> + + +#include "gpiolib.h" +
Quoted 9 lines
drivers/gpio/gpiolib-cdev.c line 32
Quoted 6 lines
drivers/gpio/gpiolib-cdev.c new file +6
-0+/* Implementation infrastructure for GPIO interfaces. +* +* The GPIO programming interface allows for inlining speed-critical +* get/set operations for common cases, so that access to SOC-integrated +* GPIOs can sometimes cost only an instruction or two per bit. +*/ Is this comment relevant for the character device?
True - that comment should stay in gpiolib, and gpiolib-cdev should get one of it's own.
Any suggestions on how to maintain line history?
I know you can trick git by moving the original file into two new ones, then moving one of those back to the old name, but not sure if that is what you would want to see in a patch.
Cheers,
Kent.
czw., 4 cze 2020 o 16:18 Kent Gibson <[email protected]> napisał(a):
Quoted 5 lines
Quoted 4 lines
Is this comment relevant for the character device?
Quoted 8 lines
True - that comment should stay in gpiolib, and gpiolib-cdev should get one of it's own.
Any suggestions on how to maintain line history?
I know you can trick git by moving the original file into two new ones, then moving one of those back to the old name, but not sure if that is what you would want to see in a patch.
People don't seem to care much about this in the kernel. Files get moved and git blame results get lost all the time. Don't stress about it.
Bart
Kent Gibson
reviewerQuoted 15 lines · Bartosz Golaszewski
czw., 4 cze 2020 o 16:18 Kent Gibson <[email protected]> napisał(a):
Quoted 5 lines
Quoted 4 lines
Is this comment relevant for the character device?
Quoted 8 lines
True - that comment should stay in gpiolib, and gpiolib-cdev should get one of it's own.
Any suggestions on how to maintain line history?
I know you can trick git by moving the original file into two new ones, then moving one of those back to the old name, but not sure if that is what you would want to see in a patch.
Quoted 4 lines
People don't seem to care much about this in the kernel. Files get moved and git blame results get lost all the time. Don't stress about it.
Oh, ok. I guess that explains why I couldn't find any examples of how to do it in a patch - I had thought I wasn't looking hard enough.
There are still a couple of commits in gpio/fixes that will conflict - are those getting merged into gpio/devel any time soon?
Or is there another branch I should rebase onto?
Cheers,
Kent.
pt., 5 cze 2020 o 01:30 Kent Gibson <[email protected]> napisał(a):
Quoted 17 lines · Bartosz Golaszewski
Quoted 15 lines
czw., 4 cze 2020 o 16:18 Kent Gibson <[email protected]> napisał(a):
Quoted 13 lines
Quoted 4 lines
Is this comment relevant for the character device?
True - that comment should stay in gpiolib, and gpiolib-cdev should get one of it's own.
Any suggestions on how to maintain line history?
I know you can trick git by moving the original file into two new ones, then moving one of those back to the old name, but not sure if that is what you would want to see in a patch.
Quoted 8 lines
Quoted 4 lines
People don't seem to care much about this in the kernel. Files get moved and git blame results get lost all the time. Don't stress about it.
Oh, ok. I guess that explains why I couldn't find any examples of how to do it in a patch - I had thought I wasn't looking hard enough.
It's not like this information is lost: once you see (with git blame) that a file has been moved, you can check out the revision from before the move and see the file's history. Rinse and repeat if it was moved more than once.
Quoted 4 lines
There are still a couple of commits in gpio/fixes that will conflict - are those getting merged into gpio/devel any time soon?
Or is there another branch I should rebase onto?
Linus W already sent out his main PR to Linus T. Sometimes he does additional PRs near the end of the merge window so maybe we can get it in this way?
Bart
Quoted 6 lines · Kent Gibson
Split the cdev specific functionality out of
gpiolib.cand intogpiolib-cdev.c. This improves the readability and maintainability of both the cdev and core gpiolib code.Suggested-by Bartosz Golaszewski <[email protected]>Signed-off-by Kent Gibson <[email protected]>
Patch applied as early v5.9 development.
I will include it into linux-next after v5.8-rc1 is out.
Yours,
Linus Walleij
śr., 10 cze 2020 o 11:13 Linus Walleij <[email protected]> napisał(a):
Quoted 7 lines · Kent Gibson
Quoted 5 lines
Split the cdev specific functionality out of
gpiolib.cand intogpiolib-cdev.c. This improves the readability and maintainability of both the cdev and core gpiolib code.
Quoted 7 lines
Quoted 2 lines
Suggested-by Bartosz Golaszewski <[email protected]>Signed-off-by Kent Gibson <[email protected]>Patch applied as early v5.9 development.
I will include it into linux-next after v5.8-rc1 is out.
Hi Linus,
FYI there's a v3 already on the list with a minor fix.
Bart