Message ID | 20200623040107.22270-10-warthog618@gmail.com |
---|---|
State | New |
Headers | show |
Series | gpio: cdev: add uAPI V2 | expand |
wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): > > Rename priv to gcdev to improve readability. > > The name "priv" indicates that the object is pointed to by > file->private_data, not what the object is actually is. > It is always used to point to a struct gpio_chardev_data so renaming > it to gcdev seemed as good as anything, and certainly clearer than "priv". > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > Ugh now it's gcdev and gdev everywhere and it doesn't really make it more readable. Maybe chardev_data or cdev_data? Bart
On Wed, Jun 24, 2020 at 04:04:09PM +0200, Bartosz Golaszewski wrote: > wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > Rename priv to gcdev to improve readability. > > > > The name "priv" indicates that the object is pointed to by > > file->private_data, not what the object is actually is. > > It is always used to point to a struct gpio_chardev_data so renaming > > it to gcdev seemed as good as anything, and certainly clearer than "priv". > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > > > Ugh now it's gcdev and gdev everywhere and it doesn't really make it > more readable. Maybe chardev_data or cdev_data? > Agreed, it isn't ideal visually, but is at least more unique than priv. Linus was going for short names recently (e.g. gc for gpiochip), so I was going for something short. And I try avoid names ending in _data or _state or similar where they don't really add anything. Would chardev or gchardev work for you? Cheers, Kent.
śr., 24 cze 2020 o 16:19 Kent Gibson <warthog618@gmail.com> napisał(a): > > On Wed, Jun 24, 2020 at 04:04:09PM +0200, Bartosz Golaszewski wrote: > > wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > Rename priv to gcdev to improve readability. > > > > > > The name "priv" indicates that the object is pointed to by > > > file->private_data, not what the object is actually is. > > > It is always used to point to a struct gpio_chardev_data so renaming > > > it to gcdev seemed as good as anything, and certainly clearer than "priv". > > > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > > > > > > Ugh now it's gcdev and gdev everywhere and it doesn't really make it > > more readable. Maybe chardev_data or cdev_data? > > > > Agreed, it isn't ideal visually, but is at least more unique than priv. > Linus was going for short names recently (e.g. gc for gpiochip), so I was > going for something short. > > And I try avoid names ending in _data or _state or similar where they > don't really add anything. > > Would chardev or gchardev work for you? > Yes, chardev is fine. Even cdev is fine for me: gdev vs gcdev is confusing but gdev vs cdev looks better IMO. Bart
On Wed, Jun 24, 2020 at 04:20:49PM +0200, Bartosz Golaszewski wrote: > śr., 24 cze 2020 o 16:19 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > On Wed, Jun 24, 2020 at 04:04:09PM +0200, Bartosz Golaszewski wrote: > > > wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > Rename priv to gcdev to improve readability. > > > > > > > > The name "priv" indicates that the object is pointed to by > > > > file->private_data, not what the object is actually is. > > > > It is always used to point to a struct gpio_chardev_data so renaming > > > > it to gcdev seemed as good as anything, and certainly clearer than "priv". > > > > > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > > > > > > > > > Ugh now it's gcdev and gdev everywhere and it doesn't really make it > > > more readable. Maybe chardev_data or cdev_data? > > > > > > > Agreed, it isn't ideal visually, but is at least more unique than priv. > > Linus was going for short names recently (e.g. gc for gpiochip), so I was > > going for something short. > > > > And I try avoid names ending in _data or _state or similar where they > > don't really add anything. > > > > Would chardev or gchardev work for you? > > > > Yes, chardev is fine. Even cdev is fine for me: gdev vs gcdev is > confusing but gdev vs cdev looks better IMO. > OK, I was avoiding cdev to try to make the name more likely to be globally unique, hence the leading "g". To try to keep it short, how about attacking it from the other end and reducing it to gcd? That would also be in keeping with the naming convention I use in later patches, e.g. glv for gpioline_values. So gcd for gpio_chardev_data. Hmmm, or maybe gcdd? Why is it that naming things is always the hardest part ;-)? Cheers, Kent.
On Thu, Jun 25, 2020 at 1:16 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Jun 24, 2020 at 04:20:49PM +0200, Bartosz Golaszewski wrote: > > śr., 24 cze 2020 o 16:19 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > On Wed, Jun 24, 2020 at 04:04:09PM +0200, Bartosz Golaszewski wrote: > > > > wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > > > > > Rename priv to gcdev to improve readability. > > > > > > > > > > The name "priv" indicates that the object is pointed to by > > > > > file->private_data, not what the object is actually is. > > > > > It is always used to point to a struct gpio_chardev_data so renaming > > > > > it to gcdev seemed as good as anything, and certainly clearer than "priv". > > > > > > > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > > > > > > > > > > > > Ugh now it's gcdev and gdev everywhere and it doesn't really make it > > > > more readable. Maybe chardev_data or cdev_data? > > > > > > > > > > Agreed, it isn't ideal visually, but is at least more unique than priv. > > > Linus was going for short names recently (e.g. gc for gpiochip), so I was > > > going for something short. > > > > > > And I try avoid names ending in _data or _state or similar where they > > > don't really add anything. > > > > > > Would chardev or gchardev work for you? > > > > > > > Yes, chardev is fine. Even cdev is fine for me: gdev vs gcdev is > > confusing but gdev vs cdev looks better IMO. > > > > OK, I was avoiding cdev to try to make the name more likely to be > globally unique, hence the leading "g". > > To try to keep it short, how about attacking it from the other end and > reducing it to gcd? > That would also be in keeping with the naming convention I use in later > patches, e.g. glv for gpioline_values. > So gcd for gpio_chardev_data. Hmmm, or maybe gcdd? > > Why is it that naming things is always the hardest part ;-)? > I prefer cdev here but it's just a personal preference. If anyone has a better idea I'm happy to switch to it. Bart
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 1e8e0a0a9b51..5f5b715ed7f7 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -828,8 +828,8 @@ struct gpio_chardev_data { */ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { - struct gpio_chardev_data *priv = file->private_data; - struct gpio_device *gdev = priv->gdev; + struct gpio_chardev_data *gcdev = file->private_data; + struct gpio_device *gdev = gcdev->gdev; struct gpio_chip *gc = gdev->chip; void __user *ip = (void __user *)arg; struct gpio_desc *desc; @@ -889,7 +889,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) hwgpio = gpio_chip_hwgpio(desc); - if (test_bit(hwgpio, priv->watched_lines)) + if (test_bit(hwgpio, gcdev->watched_lines)) return -EBUSY; gpio_desc_to_lineinfo(desc, &lineinfo); @@ -897,7 +897,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) return -EFAULT; - set_bit(hwgpio, priv->watched_lines); + set_bit(hwgpio, gcdev->watched_lines); return 0; } else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) { if (copy_from_user(&offset, ip, sizeof(offset))) @@ -909,10 +909,10 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) hwgpio = gpio_chip_hwgpio(desc); - if (!test_bit(hwgpio, priv->watched_lines)) + if (!test_bit(hwgpio, gcdev->watched_lines)) return -EBUSY; - clear_bit(hwgpio, priv->watched_lines); + clear_bit(hwgpio, gcdev->watched_lines); return 0; } return -EINVAL; @@ -935,12 +935,12 @@ to_gpio_chardev_data(struct notifier_block *nb) static int lineinfo_changed_notify(struct notifier_block *nb, unsigned long action, void *data) { - struct gpio_chardev_data *priv = to_gpio_chardev_data(nb); + struct gpio_chardev_data *gcdev = to_gpio_chardev_data(nb); struct gpioline_info_changed chg; struct gpio_desc *desc = data; int ret; - if (!test_bit(gpio_chip_hwgpio(desc), priv->watched_lines)) + if (!test_bit(gpio_chip_hwgpio(desc), gcdev->watched_lines)) return NOTIFY_DONE; memset(&chg, 0, sizeof(chg)); @@ -949,9 +949,9 @@ static int lineinfo_changed_notify(struct notifier_block *nb, chg.timestamp = ktime_get_ns(); gpio_desc_to_lineinfo(desc, &chg.info); - ret = kfifo_in_spinlocked(&priv->events, &chg, 1, &priv->wait.lock); + ret = kfifo_in_spinlocked(&gcdev->events, &chg, 1, &gcdev->wait.lock); if (ret) - wake_up_poll(&priv->wait, EPOLLIN); + wake_up_poll(&gcdev->wait, EPOLLIN); else pr_debug_ratelimited("lineinfo event FIFO is full - event dropped\n"); @@ -961,13 +961,13 @@ static int lineinfo_changed_notify(struct notifier_block *nb, static __poll_t lineinfo_watch_poll(struct file *file, struct poll_table_struct *pollt) { - struct gpio_chardev_data *priv = file->private_data; + struct gpio_chardev_data *gcdev = file->private_data; __poll_t events = 0; - poll_wait(file, &priv->wait, pollt); + poll_wait(file, &gcdev->wait, pollt); - if (!kfifo_is_empty_spinlocked_noirqsave(&priv->events, - &priv->wait.lock)) + if (!kfifo_is_empty_spinlocked_noirqsave(&gcdev->events, + &gcdev->wait.lock)) events = EPOLLIN | EPOLLRDNORM; return events; @@ -976,7 +976,7 @@ static __poll_t lineinfo_watch_poll(struct file *file, static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, size_t count, loff_t *off) { - struct gpio_chardev_data *priv = file->private_data; + struct gpio_chardev_data *gcdev = file->private_data; struct gpioline_info_changed event; ssize_t bytes_read = 0; int ret; @@ -985,28 +985,28 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf, return -EINVAL; do { - spin_lock(&priv->wait.lock); - if (kfifo_is_empty(&priv->events)) { + spin_lock(&gcdev->wait.lock); + if (kfifo_is_empty(&gcdev->events)) { if (bytes_read) { - spin_unlock(&priv->wait.lock); + spin_unlock(&gcdev->wait.lock); return bytes_read; } if (file->f_flags & O_NONBLOCK) { - spin_unlock(&priv->wait.lock); + spin_unlock(&gcdev->wait.lock); return -EAGAIN; } - ret = wait_event_interruptible_locked(priv->wait, - !kfifo_is_empty(&priv->events)); + ret = wait_event_interruptible_locked(gcdev->wait, + !kfifo_is_empty(&gcdev->events)); if (ret) { - spin_unlock(&priv->wait.lock); + spin_unlock(&gcdev->wait.lock); return ret; } } - ret = kfifo_out(&priv->events, &event, 1); - spin_unlock(&priv->wait.lock); + ret = kfifo_out(&gcdev->events, &event, 1); + spin_unlock(&gcdev->wait.lock); if (ret != 1) { ret = -EIO; break; @@ -1031,33 +1031,33 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file) { struct gpio_device *gdev = container_of(inode->i_cdev, struct gpio_device, chrdev); - struct gpio_chardev_data *priv; + struct gpio_chardev_data *gcdev; int ret = -ENOMEM; /* Fail on open if the backing gpiochip is gone */ if (!gdev->chip) return -ENODEV; - priv = kzalloc(sizeof(*priv), GFP_KERNEL); - if (!priv) + gcdev = kzalloc(sizeof(*gcdev), GFP_KERNEL); + if (!gcdev) return -ENOMEM; - priv->watched_lines = bitmap_zalloc(gdev->chip->ngpio, GFP_KERNEL); - if (!priv->watched_lines) - goto out_free_priv; + gcdev->watched_lines = bitmap_zalloc(gdev->chip->ngpio, GFP_KERNEL); + if (!gcdev->watched_lines) + goto out_free_gcdev; - init_waitqueue_head(&priv->wait); - INIT_KFIFO(priv->events); - priv->gdev = gdev; + init_waitqueue_head(&gcdev->wait); + INIT_KFIFO(gcdev->events); + gcdev->gdev = gdev; - priv->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify; + gcdev->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify; ret = atomic_notifier_chain_register(&gdev->notifier, - &priv->lineinfo_changed_nb); + &gcdev->lineinfo_changed_nb); if (ret) goto out_free_bitmap; get_device(&gdev->dev); - file->private_data = priv; + file->private_data = gcdev; ret = nonseekable_open(inode, file); if (ret) @@ -1067,11 +1067,11 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file) out_unregister_notifier: atomic_notifier_chain_unregister(&gdev->notifier, - &priv->lineinfo_changed_nb); + &gcdev->lineinfo_changed_nb); out_free_bitmap: - bitmap_free(priv->watched_lines); -out_free_priv: - kfree(priv); + bitmap_free(gcdev->watched_lines); +out_free_gcdev: + kfree(gcdev); return ret; } @@ -1083,14 +1083,14 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file) */ static int gpio_chrdev_release(struct inode *inode, struct file *file) { - struct gpio_chardev_data *priv = file->private_data; - struct gpio_device *gdev = priv->gdev; + struct gpio_chardev_data *gcdev = file->private_data; + struct gpio_device *gdev = gcdev->gdev; - bitmap_free(priv->watched_lines); + bitmap_free(gcdev->watched_lines); atomic_notifier_chain_unregister(&gdev->notifier, - &priv->lineinfo_changed_nb); + &gcdev->lineinfo_changed_nb); put_device(&gdev->dev); - kfree(priv); + kfree(gcdev); return 0; }
Rename priv to gcdev to improve readability. The name "priv" indicates that the object is pointed to by file->private_data, not what the object is actually is. It is always used to point to a struct gpio_chardev_data so renaming it to gcdev seemed as good as anything, and certainly clearer than "priv". Signed-off-by: Kent Gibson <warthog618@gmail.com> --- drivers/gpio/gpiolib-cdev.c | 90 ++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 45 deletions(-)