diff mbox series

[09/22] gpiolib: cdev: rename priv to gcdev

Message ID 20200623040107.22270-10-warthog618@gmail.com
State New
Headers show
Series gpio: cdev: add uAPI V2 | expand

Commit Message

Kent Gibson June 23, 2020, 4 a.m. UTC
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(-)

Comments

Bartosz Golaszewski June 24, 2020, 2:04 p.m. UTC | #1
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
Kent Gibson June 24, 2020, 2:19 p.m. UTC | #2
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.
Bartosz Golaszewski June 24, 2020, 2:20 p.m. UTC | #3
ś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
Kent Gibson June 24, 2020, 11:16 p.m. UTC | #4
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.
Bartosz Golaszewski June 25, 2020, 10:26 a.m. UTC | #5
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 mbox series

Patch

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;
 }