gpiolib: split character device into gpiolib-cdev

PATCH Superseded Kent Gibson 2 participants 4 messages May 27, 2020 – Jun 1, 2020

Subsystems (2)

GPIO GPIO uapi

lore

Share & tools

lore

Kent Gibson

author reviewer

2020-05-27 15:41 UTC #0 lore raw

PATCH 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.

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.

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

drivers/gpio/Makefile +1 -0

index 65bf3940e33c..b5b58b624f37 100644
@@ -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 +1114 -0

1,114 lines not on this page · raw message

drivers/gpio/gpiolib-cdev.h new file +11 -0

new file mode 100644
index 000000000000..973578e7ad10
@@ -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 ~~-1072~~

1,117 lines not on this page · raw message

Signature · 1 line

2.26.2

kbuild test robot

build bot

2020-05-28 00:47 UTC #1 lore raw

Thank you for the patch! Perhaps something to improve:

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-000035
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 <[email protected]>

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(&gdev->chrdev, &gpio_fileops);
  1097		gdev->chrdev.owner = THIS_MODULE;
  1098		gdev->dev.devt = MKDEV(MAJOR(devt), gdev->id);
  1099	
  1100		ret = cdev_device_add(&gdev->chrdev, &gdev->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)

1111 {
1112 cdev_device_del(&gdev->chrdev, &gdev->dev);
1113 }
1114

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

kbuild test robot

build bot

2020-05-28 04:56 UTC #2 lore raw

In reply to Kent Gibson

Thank you for the patch! Perhaps something to improve:

Hi Kent,

Thank you for the patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Kent-Gibson/gpiolib-split-character-device-into-gpiolib-cdev/20200528-000035
base:    2ef96a5bb12be62ef75b5828c0aab838ebb29cb8
config: riscv-allyesconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate

Reported-by kbuild test robot <[email protected]>

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,
771 struct gpioline_info *info)
772 {
773 struct gpio_chip *gc = desc->gdev->chip;
774 unsigned long flags;
775
776 spin_lock_irqsave(&gpio_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, &desc->flags) ||
   798		    test_bit(FLAG_IS_HOGGED, &desc->flags) ||
   799		    test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
   800		    test_bit(FLAG_EXPORT, &desc->flags) ||
   801		    test_bit(FLAG_SYSFS, &desc->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, &desc->flags))
805 info->flags |= GPIOLINE_FLAG_IS_OUT;
806 if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
807 info->flags |= GPIOLINE_FLAG_ACTIVE_LOW;
808 if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
809 info->flags |= (GPIOLINE_FLAG_OPEN_DRAIN |
810 GPIOLINE_FLAG_IS_OUT);
811 if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
812 info->flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
813 GPIOLINE_FLAG_IS_OUT);
814 if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
815 info->flags |= GPIOLINE_FLAG_BIAS_DISABLE;
816 if (test_bit(FLAG_PULL_DOWN, &desc->flags))
817 info->flags |= GPIOLINE_FLAG_BIAS_PULL_DOWN;
818 if (test_bit(FLAG_PULL_UP, &desc->flags))
819 info->flags |= GPIOLINE_FLAG_BIAS_PULL_UP;
820
821 spin_unlock_irqrestore(&gpio_lock, flags);
822 }
823

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

kbuild test robot

build bot

2020-06-01 18:33 UTC #3 lore raw

In reply to Kent Gibson

Thank you for the patch! Perhaps something to improve:

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-000035
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 <[email protected]>

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(&gdev->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(&gdev->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,
1014 struct 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(&priv->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(&gdev->notifier,

1036 &priv->lineinfo_changed_nb);
1037 if (ret)
1038 goto out_free_bitmap;
1039
1040 get_device(&gdev->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(&gdev->notifier,
1051 &priv->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-all(a)lists.01.org