[PATCH v1 1/2] gpio: pch: Use BIT() and GENMASK() where it's appropriate

[PATCH v1 1/2] gpio: pch: Use BIT() and GENMASK() where it's appropriate

[Andy Shevchenko]

Use BIT() and GENMASK() where it's appropriate.
At the same time drop it where it's not appropriate.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pch.c | 43 +++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
index 3f3d9a94b709..0b5aea0b1e8a 100644
--- a/drivers/gpio/gpio-pch.c
+++ b/drivers/gpio/gpio-pch.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (C) 2011 LAPIS Semiconductor Co., Ltd.
  */
+#include <linux/bits.h>
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -11,11 +12,11 @@
 #include <linux/slab.h>
 
 #define PCH_EDGE_FALLING	0
-#define PCH_EDGE_RISING		BIT(0)
-#define PCH_LEVEL_L		BIT(1)
-#define PCH_LEVEL_H		(BIT(0) | BIT(1))
+#define PCH_EDGE_RISING		1
+#define PCH_LEVEL_L		2
+#define PCH_LEVEL_H		3
 #define PCH_EDGE_BOTH		BIT(2)
-#define PCH_IM_MASK		(BIT(0) | BIT(1) | BIT(2))
+#define PCH_IM_MASK		GENMASK(2, 0)
 
 #define PCH_IRQ_BASE		24
 
@@ -103,9 +104,9 @@ static void pch_gpio_set(struct gpio_chip *gpio, unsigned nr, int val)
 	spin_lock_irqsave(&chip->spinlock, flags);
 	reg_val = ioread32(&chip->reg->po);
 	if (val)
-		reg_val |= (1 << nr);
+		reg_val |= BIT(nr);
 	else
-		reg_val &= ~(1 << nr);
+		reg_val &= ~BIT(nr);
 
 	iowrite32(reg_val, &chip->reg->po);
 	spin_unlock_irqrestore(&chip->spinlock, flags);
@@ -115,7 +116,7 @@ static int pch_gpio_get(struct gpio_chip *gpio, unsigned nr)
 {
 	struct pch_gpio *chip =	gpiochip_get_data(gpio);
 
-	return (ioread32(&chip->reg->pi) >> nr) & 1;
+	return !!(ioread32(&chip->reg->pi) & BIT(nr));
 }
 
 static int pch_gpio_direction_output(struct gpio_chip *gpio, unsigned nr,
@@ -130,13 +131,14 @@ static int pch_gpio_direction_output(struct gpio_chip *gpio, unsigned nr,
 
 	reg_val = ioread32(&chip->reg->po);
 	if (val)
-		reg_val |= (1 << nr);
+		reg_val |= BIT(nr);
 	else
-		reg_val &= ~(1 << nr);
+		reg_val &= ~BIT(nr);
 	iowrite32(reg_val, &chip->reg->po);
 
-	pm = ioread32(&chip->reg->pm) & ((1 << gpio_pins[chip->ioh]) - 1);
-	pm |= (1 << nr);
+	pm = ioread32(&chip->reg->pm);
+	pm &= BIT(gpio_pins[chip->ioh]) - 1;
+	pm |= BIT(nr);
 	iowrite32(pm, &chip->reg->pm);
 
 	spin_unlock_irqrestore(&chip->spinlock, flags);
@@ -151,8 +153,9 @@ static int pch_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
 	unsigned long flags;
 
 	spin_lock_irqsave(&chip->spinlock, flags);
-	pm = ioread32(&chip->reg->pm) & ((1 << gpio_pins[chip->ioh]) - 1);
-	pm &= ~(1 << nr);
+	pm = ioread32(&chip->reg->pm);
+	pm &= BIT(gpio_pins[chip->ioh]) - 1;
+	pm &= ~BIT(nr);
 	iowrite32(pm, &chip->reg->pm);
 	spin_unlock_irqrestore(&chip->spinlock, flags);
 
@@ -277,7 +280,7 @@ static void pch_irq_unmask(struct irq_data *d)
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	struct pch_gpio *chip = gc->private;
 
-	iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->imaskclr);
+	iowrite32(BIT(d->irq - chip->irq_base), &chip->reg->imaskclr);
 }
 
 static void pch_irq_mask(struct irq_data *d)
@@ -285,7 +288,7 @@ static void pch_irq_mask(struct irq_data *d)
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	struct pch_gpio *chip = gc->private;
 
-	iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->imask);
+	iowrite32(BIT(d->irq - chip->irq_base), &chip->reg->imask);
 }
 
 static void pch_irq_ack(struct irq_data *d)
@@ -293,7 +296,7 @@ static void pch_irq_ack(struct irq_data *d)
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	struct pch_gpio *chip = gc->private;
 
-	iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->iclr);
+	iowrite32(BIT(d->irq - chip->irq_base), &chip->reg->iclr);
 }
 
 static irqreturn_t pch_gpio_handler(int irq, void *dev_id)
@@ -344,7 +347,6 @@ static int pch_gpio_probe(struct pci_dev *pdev,
 	s32 ret;
 	struct pch_gpio *chip;
 	int irq_base;
-	u32 msk;
 
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (chip == NULL)
@@ -357,7 +359,7 @@ static int pch_gpio_probe(struct pci_dev *pdev,
 		return ret;
 	}
 
-	ret = pcim_iomap_regions(pdev, 1 << 1, KBUILD_MODNAME);
+	ret = pcim_iomap_regions(pdev, BIT(1), KBUILD_MODNAME);
 	if (ret) {
 		dev_err(&pdev->dev, "pci_request_regions FAILED-%d", ret);
 		return ret;
@@ -393,9 +395,8 @@ static int pch_gpio_probe(struct pci_dev *pdev,
 	chip->irq_base = irq_base;
 
 	/* Mask all interrupts, but enable them */
-	msk = (1 << gpio_pins[chip->ioh]) - 1;
-	iowrite32(msk, &chip->reg->imask);
-	iowrite32(msk, &chip->reg->ien);
+	iowrite32(BIT(gpio_pins[chip->ioh]) - 1, &chip->reg->imask);
+	iowrite32(BIT(gpio_pins[chip->ioh]) - 1, &chip->reg->ien);
 
 	ret = devm_request_irq(&pdev->dev, pdev->irq, pch_gpio_handler,
 			       IRQF_SHARED, KBUILD_MODNAME, chip);
-- 
2.25.1

[PATCH v1 2/2] gpio: pch: Get rid of unneeded variable in IRQ handler

[Andy Shevchenko]

There is no need to have an additional variable in IRQ handler. We may simple
rely on the fact of having non-zero register value we read from the hardware.

While here, drop repetitive messages in time critical function.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pch.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
index 0b5aea0b1e8a..a0b12bc766db 100644
--- a/drivers/gpio/gpio-pch.c
+++ b/drivers/gpio/gpio-pch.c
@@ -303,14 +303,15 @@ static irqreturn_t pch_gpio_handler(int irq, void *dev_id)
 {
 	struct pch_gpio *chip = dev_id;
 	unsigned long reg_val = ioread32(&chip->reg->istatus);
-	int i, ret = IRQ_NONE;
+	int i;
 
-	for_each_set_bit(i, &reg_val, gpio_pins[chip->ioh]) {
-		dev_dbg(chip->dev, "[%d]:irq=%d  status=0x%lx\n", i, irq, reg_val);
+	dev_dbg(chip->dev, "irq=%d  status=0x%lx\n", irq, reg_val);
+
+	reg_val &= BIT(gpio_pins[chip->ioh]) - 1;
+	for_each_set_bit(i, &reg_val, gpio_pins[chip->ioh])
 		generic_handle_irq(chip->irq_base + i);
-		ret = IRQ_HANDLED;
-	}
-	return ret;
+
+	return IRQ_RETVAL(reg_val);
 }
 
 static int pch_gpio_alloc_generic_chip(struct pch_gpio *chip,
-- 
2.25.1

Re: [PATCH v1 1/2] gpio: pch: Use BIT() and GENMASK() where it's appropriate

[Bartosz Golaszewski]

czw., 2 kwi 2020 o 22:19 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> Use BIT() and GENMASK() where it's appropriate.
> At the same time drop it where it's not appropriate.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpio-pch.c | 43 +++++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
> index 3f3d9a94b709..0b5aea0b1e8a 100644
> --- a/drivers/gpio/gpio-pch.c
> +++ b/drivers/gpio/gpio-pch.c
> @@ -2,6 +2,7 @@
>  /*
>   * Copyright (C) 2011 LAPIS Semiconductor Co., Ltd.
>   */
> +#include <linux/bits.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> @@ -11,11 +12,11 @@
>  #include <linux/slab.h>
>
>  #define PCH_EDGE_FALLING       0
> -#define PCH_EDGE_RISING                BIT(0)
> -#define PCH_LEVEL_L            BIT(1)
> -#define PCH_LEVEL_H            (BIT(0) | BIT(1))
> +#define PCH_EDGE_RISING                1
> +#define PCH_LEVEL_L            2
> +#define PCH_LEVEL_H            3

If these define bitmask values for some fields in registers, then I'd
suggest to write it as hex numbers. I find it much more readable this
way.

Bart

Re: [PATCH v1 1/2] gpio: pch: Use BIT() and GENMASK() where it's appropriate

[Andy Shevchenko]

On Mon, Apr 06, 2020 at 09:23:30AM +0200, Bartosz Golaszewski wrote:
> czw., 2 kwi 2020 o 22:19 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> >
> > Use BIT() and GENMASK() where it's appropriate.
> > At the same time drop it where it's not appropriate.

Thanks for review, my comments below.

...

> >  #define PCH_EDGE_FALLING       0
> > -#define PCH_EDGE_RISING                BIT(0)
> > -#define PCH_LEVEL_L            BIT(1)
> > -#define PCH_LEVEL_H            (BIT(0) | BIT(1))
> > +#define PCH_EDGE_RISING                1
> > +#define PCH_LEVEL_L            2
> > +#define PCH_LEVEL_H            3
> 
> If these define bitmask values for some fields in registers, then I'd
> suggest to write it as hex numbers. I find it much more readable this
> way.

You meant
 0x0
 0x1
 0x2
 0x3
?

But what the benefit comes out of it? There are sliding 3 bits (3 bits
per each GPIO line), so this numbers in hex, in my opinion, will add
a confusion: "Are they always in position 2..0 or not?"

That said, I'm not against the change, but I would like to be sure
what is the benefit.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] gpio: pch: Use BIT() and GENMASK() where it's appropriate

[Bartosz Golaszewski]

czw., 9 kwi 2020 o 17:09 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Mon, Apr 06, 2020 at 09:23:30AM +0200, Bartosz Golaszewski wrote:
> > czw., 2 kwi 2020 o 22:19 Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> napisał(a):
> > >
> > > Use BIT() and GENMASK() where it's appropriate.
> > > At the same time drop it where it's not appropriate.
>
> Thanks for review, my comments below.
>
> ...
>
> > >  #define PCH_EDGE_FALLING       0
> > > -#define PCH_EDGE_RISING                BIT(0)
> > > -#define PCH_LEVEL_L            BIT(1)
> > > -#define PCH_LEVEL_H            (BIT(0) | BIT(1))
> > > +#define PCH_EDGE_RISING                1
> > > +#define PCH_LEVEL_L            2
> > > +#define PCH_LEVEL_H            3
> >
> > If these define bitmask values for some fields in registers, then I'd
> > suggest to write it as hex numbers. I find it much more readable this
> > way.
>
> You meant
>  0x0
>  0x1
>  0x2
>  0x3
> ?
>
> But what the benefit comes out of it? There are sliding 3 bits (3 bits
> per each GPIO line), so this numbers in hex, in my opinion, will add
> a confusion: "Are they always in position 2..0 or not?"
>
> That said, I'm not against the change, but I would like to be sure
> what is the benefit.
>

Frankly this is just my personal preference. I think it's consistent
with the majority of codebase in the kernel but I won't block this
patch for that reason. Feel free to leave it like it is if you prefer
it.

Bart

Re: [PATCH v1 1/2] gpio: pch: Use BIT() and GENMASK() where it's appropriate

[Andy Shevchenko]

On Wed, Apr 15, 2020 at 01:17:41PM +0200, Bartosz Golaszewski wrote:
> czw., 9 kwi 2020 o 17:09 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> > On Mon, Apr 06, 2020 at 09:23:30AM +0200, Bartosz Golaszewski wrote:
> > > czw., 2 kwi 2020 o 22:19 Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> napisał(a):

...

> > That said, I'm not against the change, but I would like to be sure
> > what is the benefit.
> >
> 
> Frankly this is just my personal preference. I think it's consistent
> with the majority of codebase in the kernel but I won't block this
> patch for that reason. Feel free to leave it like it is if you prefer
> it.

Thank you, I prefer to see plain (decimal) numbers there.

I sent v2 since it has more patches and one more BIT removal here.

-- 
With Best Regards,
Andy Shevchenko