Re: [PATCH] gpiolib: split character device into gpiolib-cdev
Hi Kent,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on v5.7-rc5]
[cannot apply to gpio/for-next linus/master linux/master v5.7-rc7 v5.7-rc6
next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Kent-Gibson/gpiolib-split-character-device-into-gpiolib-cdev/20200528-35
base:2ef96a5bb12be62ef75b5828c0aab838ebb29cb8
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot
cppcheck warnings: (new ones prefixed by >>)
>> drivers/gpio/gpiolib-cdev.c:1035:6: warning: Variable 'ret' is reassigned a
>> value before the old one has been used. [redundantAssignment]
ret = atomic_notifier_chain_register(>notifier,
^
drivers/gpio/gpiolib-cdev.c:1016:0: note: Variable 'ret' is reassigned a
value before the old one has been used.
int ret = -ENOMEM;
^
drivers/gpio/gpiolib-cdev.c:1035:6: note: Variable 'ret' is reassigned a
value before the old one has been used.
ret = atomic_notifier_chain_register(>notifier,
^
vim +/ret +1035 drivers/gpio/gpiolib-cdev.c
1004
1005 /**
1006 * gpio_chrdev_open() - open the chardev for ioctl operations
1007 * @inode: inode for this chardev
1008 * @filp: file struct for storing private data
1009 * Returns 0 on success
1010 */
1011 static int gpio_chrdev_open(struct inode *inode, struct file *filp)
1012 {
1013 struct gpio_device *gdev = container_of(inode->i_cdev,
1014struct gpio_device,
chrdev);
1015 struct gpio_chardev_data *priv;
1016 int ret = -ENOMEM;
1017
1018 /* Fail on open if the backing gpiochip is gone */
1019 if (!gdev->chip)
1020 return -ENODEV;
1021
1022 priv = kzalloc(sizeof(*priv), GFP_KERNEL);
1023 if (!priv)
1024 return -ENOMEM;
1025
1026 priv->watched_lines = bitmap_zalloc(gdev->chip->ngpio,
GFP_KERNEL);
1027 if (!priv->watched_lines)
1028 goto out_free_priv;
1029
1030 init_waitqueue_head(>wait);
1031 INIT_KFIFO(priv->events);
1032 priv->gdev = gdev;
1033
1034 priv->lineinfo_changed_nb.notifier_call =
lineinfo_changed_notify;
> 1035 ret = atomic_notifier_chain_register(>notifier,
1036
>lineinfo_changed_nb);
1037 if (ret)
1038 goto out_free_bitmap;
1039
1040 get_device(>dev);
1041 filp->private_data = priv;
1042
1043 ret = nonseekable_open(inode, filp);
1044 if (ret)
1045 goto out_unregister_notifier;
1046
1047 return ret;
1048
1049 out_unregister_notifier:
1050 atomic_notifier_chain_unregister(>notifier,
1051 >lineinfo_changed_nb);
1052 out_free_bitmap:
1053 bitmap_free(priv->watched_lines);
1054 out_free_priv:
1055 kfree(priv);
1056 return ret;
1057 }
1058
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
Re: [PATCH] gpiolib: split character device into gpiolib-cdev
Hi Kent,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on v5.7-rc5]
[cannot apply to gpio/for-next linus/master linux/master v5.7-rc7 v5.7-rc6
next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Kent-Gibson/gpiolib-split-character-device-into-gpiolib-cdev/20200528-35
base:2ef96a5bb12be62ef75b5828c0aab838ebb29cb8
config: riscv-allyesconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot
All warnings (new ones prefixed by >>, old ones prefixed by <<):
drivers/gpio/gpiolib-cdev.c:1092:5: warning: no previous prototype for
'gpiolib_cdev_register' [-Wmissing-prototypes]
1092 | int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
| ^
drivers/gpio/gpiolib-cdev.c:1110:6: warning: no previous prototype for
'gpiolib_cdev_unregister' [-Wmissing-prototypes]
1110 | void gpiolib_cdev_unregister(struct gpio_device *gdev)
| ^~~
drivers/gpio/gpiolib-cdev.c: In function 'gpio_desc_to_lineinfo':
>> drivers/gpio/gpiolib-cdev.c:779:3: warning: 'strncpy' specified bound 32
>> equals destination size [-Wstringop-truncation]
779 | strncpy(info->name, desc->name, sizeof(info->name));
| ^~~
vim +/strncpy +779 drivers/gpio/gpiolib-cdev.c
769
770 static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
771struct gpioline_info *info)
772 {
773 struct gpio_chip *gc = desc->gdev->chip;
774 unsigned long flags;
775
776 spin_lock_irqsave(_lock, flags);
777
778 if (desc->name) {
> 779 strncpy(info->name, desc->name, sizeof(info->name));
780 info->name[sizeof(info->name) - 1] = '\0';
781 } else {
782 info->name[0] = '\0';
783 }
784
785 if (desc->label) {
786 strncpy(info->consumer, desc->label,
sizeof(info->consumer));
787 info->consumer[sizeof(info->consumer) - 1] = '\0';
788 } else {
789 info->consumer[0] = '\0';
790 }
791
792 /*
793 * Userspace only need to know that the kernel is using this
GPIO so
794 * it can't use it.
795 */
796 info->flags = 0;
797 if (test_bit(FLAG_REQUESTED, >flags) ||
798 test_bit(FLAG_IS_HOGGED, >flags) ||
799 test_bit(FLAG_USED_AS_IRQ, >flags) ||
800 test_bit(FLAG_EXPORT, >flags) ||
801 test_bit(FLAG_SYSFS, >flags) ||
802 !pinctrl_gpio_can_use_line(gc->base + info->line_offset))
803 info->flags |= GPIOLINE_FLAG_KERNEL;
804 if (test_bit(FLAG_IS_OUT, >flags))
805 info->flags |= GPIOLINE_FLAG_IS_OUT;
806 if (test_bit(FLAG_ACTIVE_LOW, >flags))
807 info->flags |= GPIOLINE_FLAG_ACTIVE_LOW;
808 if (test_bit(FLAG_OPEN_DRAIN, >flags))
809 info->flags |= (GPIOLINE_FLAG_OPEN_DRAIN |
810 GPIOLINE_FLAG_IS_OUT);
811 if (test_bit(FLAG_OPEN_SOURCE, >flags))
812 info->flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
813 GPIOLINE_FLAG_IS_OUT);
814 if (test_bit(FLAG_BIAS_DISABLE, >flags))
815 info->flags |= GPIOLINE_FLAG_BIAS_DISABLE;
816 if (test_bit(FLAG_PULL_DOWN, >flags))
817 info->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
818 if (test_bit(FLAG_PULL_UP, >flags))
819 info->flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
820
821 spin_unlock_irqrestore(_lock, flags);
822 }
823
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH] gpiolib: split character device into gpiolib-cdev
Hi Kent,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on v5.7-rc5]
[cannot apply to gpio/for-next linus/master linux/master v5.7-rc7 v5.7-rc6
next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Kent-Gibson/gpiolib-split-character-device-into-gpiolib-cdev/20200528-35
base:2ef96a5bb12be62ef75b5828c0aab838ebb29cb8
config: nios2-allmodconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> drivers/gpio/gpiolib-cdev.c:1092:5: warning: no previous prototype for
>> 'gpiolib_cdev_register' [-Wmissing-prototypes]
1092 | int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
| ^
>> drivers/gpio/gpiolib-cdev.c:1110:6: warning: no previous prototype for
>> 'gpiolib_cdev_unregister' [-Wmissing-prototypes]
1110 | void gpiolib_cdev_unregister(struct gpio_device *gdev)
| ^~~
vim +/gpiolib_cdev_register +1092 drivers/gpio/gpiolib-cdev.c
1091
> 1092 int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
1093 {
1094 int ret;
1095
1096 cdev_init(>chrdev, _fileops);
1097 gdev->chrdev.owner = THIS_MODULE;
1098 gdev->dev.devt = MKDEV(MAJOR(devt), gdev->id);
1099
1100 ret = cdev_device_add(>chrdev, >dev);
1101 if (ret)
1102 return ret;
1103
1104 chip_dbg(gdev->chip, "added GPIO chardev (%d:%d)\n",
1105 MAJOR(devt), gdev->id);
1106
1107 return 0;
1108 }
1109
> 1110 void gpiolib_cdev_unregister(struct gpio_device *gdev)
{
1112 cdev_device_del(>chrdev, >dev);
1113 }
1114
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
[PATCH] 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.
Suggested-by: Bartosz Golaszewski
Signed-off-by: Kent Gibson
---
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.
Kent.
drivers/gpio/Makefile |1 +
drivers/gpio/gpiolib-cdev.c | 1114 +++
drivers/gpio/gpiolib-cdev.h | 11 +
drivers/gpio/gpiolib.c | 1076 +
4 files changed, 1130 insertions(+), 1072 deletions(-)
create mode 100644 drivers/gpio/gpiolib-cdev.c
create mode 100644 drivers/gpio/gpiolib-cdev.h
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 65bf3940e33c..b5b58b624f37 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -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
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
new file mode 100644
index ..6b6fde33acd6
--- /dev/null
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -0,0 +1,1114 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+
+#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.
+ */
+
+
+/*
+ * GPIO line handle management
+ */
+
+/**
+ * struct linehandle_state - contains the state of a userspace handle
+ * @gdev: the GPIO device the handle pertains to
+ * @label: consumer label used to tag descriptors
+ * @descs: the GPIO descriptors held by this handle
+ * @numdescs: the number of descriptors held in the descs array
+ */
+struct linehandle_state {
+ struct gpio_device *gdev;
+ const char *label;
+ struct gpio_desc *descs[GPIOHANDLES_MAX];
+ u32 numdescs;
+};
+
+#define GPIOHANDLE_REQUEST_VALID_FLAGS \
+ (GPIOHANDLE_REQUEST_INPUT | \
+ GPIOHANDLE_REQUEST_OUTPUT | \
+ GPIOHANDLE_REQUEST_ACTIVE_LOW | \
+ GPIOHANDLE_REQUEST_BIAS_PULL_UP | \
+ GPIOHANDLE_REQUEST_BIAS_PULL_DOWN | \
+ GPIOHANDLE_REQUEST_BIAS_DISABLE | \
+ GPIOHANDLE_REQUEST_OPEN_DRAIN | \
+ GPIOHANDLE_REQUEST_OPEN_SOURCE)
+
+static int linehandle_validate_flags(u32 flags)
+{
+ /* Return an error if an unknown flag is set */
+ if (flags & ~GPIOHANDLE_REQUEST_VALID_FLAGS)
+ return -EINVAL;
+
+ /*
+* Do not allow both INPUT & OUTPUT flags to be set as they are
+* contradictory.
+*/
+ if ((flags & GPIOHANDLE_REQUEST_INPUT) &&
+ (flags & GPIOHANDLE_REQUEST_OUTPUT))
+ return -EINVAL;
+
+ /*
+* Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If
+* the hardware actually supports enabling both at the same time the
+* electrical result would be disastrous.
+*/
+ if ((flags & GPIOHANDLE_REQUEST_OPEN_DRAIN) &&
+ (flags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
+ return -EINVAL;
+
+ /* OPEN_DRAIN and OPEN_SOURCE flags only make sense for output mode. */
+ if (!(flags & GPIOHANDLE_REQUEST_OUTPUT) &&
+ ((flags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
+(flags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
+ return -EINVAL;
+
+ /* Bias flags only allowed for input or output mode. */
+ if (!((flags & GPIOHANDLE_REQUEST_INPUT) ||
+ (flags & GPIOHANDLE_REQUEST_OUTPUT)) &&
+ ((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
+(flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
+
