gpio: pch: Use BIT() and GENMASK() where it's appropriate

PATCH In review Andy Shevchenko 2 participants 7 messages Apr 14, 2020 – Apr 16, 2020

Subsystems (2)

GPIO Intel GPIO drivers

lore

Share & tools

lore

Andy Shevchenko

author

2020-04-14 17:50 UTC #0 lore raw

PATCH v2 1/4 gpio: pch: Use BIT() and GENMASK() where it's appropriate

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

Signed-off-by Andy Shevchenko <[email protected]>

---
v2: update one more macro (all IRQ settings are plain numbers, not bits)

 drivers/gpio/gpio-pch.c | 45 +++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

drivers/gpio/gpio-pch.c +23 ~~-22~~

index 3f3d9a94b709..03eeacdb04fb 100644
@@ -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_BOTH		BIT(2)
-#define PCH_IM_MASK		(BIT(0) | BIT(1) | BIT(2))
+#define PCH_EDGE_RISING		1
+#define PCH_LEVEL_L		2
+#define PCH_LEVEL_H		3
+#define PCH_EDGE_BOTH		4
+#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);

Signature · 1 line

2.25.1

Andy Shevchenko

author

2020-04-14 17:50 UTC #1 lore raw

PATCH v2 2/4 gpio: pch: Get rid of unneeded variable in IRQ handler

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

---
v2: no change

 drivers/gpio/gpio-pch.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/gpio/gpio-pch.c +7 -6

index 03eeacdb04fb..708272db6baf 100644
@@ -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,

Signature · 1 line

2.25.1

Andy Shevchenko

author

2020-04-14 17:50 UTC #2 lore raw

PATCH v2 3/4 gpio: pch: Refactor pch_irq_type() to avoid unnecessary locking

In reply to Andy Shevchenko

When type is not supported there is no need to lock and check.

Signed-off-by Andy Shevchenko <[email protected]>

---
v2: new patch

 drivers/gpio/gpio-pch.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/gpio/gpio-pch.c +5 -6

index 708272db6baf..9c34230f2e84 100644
@@ -229,17 +229,15 @@ static int pch_irq_type(struct irq_data *d, unsigned int type)
 	int ch, irq = d->irq;
 
 	ch = irq - chip->irq_base;
-	if (irq <= chip->irq_base + 7) {
+	if (irq < chip->irq_base + 8) {
 		im_reg = &chip->reg->im0;
-		im_pos = ch;
+		im_pos = ch - 0;
 	} else {
 		im_reg = &chip->reg->im1;
 		im_pos = ch - 8;
 	}
 	dev_dbg(chip->dev, "irq=%d type=%d ch=%d pos=%d\n", irq, type, ch, im_pos);
 
-	spin_lock_irqsave(&chip->spinlock, flags);
-
 	switch (type) {
 	case IRQ_TYPE_EDGE_RISING:
 		val = PCH_EDGE_RISING;

@@ -257,9 +255,11 @@ static int pch_irq_type(struct irq_data *d, unsigned int type)
 		val = PCH_LEVEL_L;
 		break;
 	default:
-		goto unlock;
+		return 0;
 	}
 
+	spin_lock_irqsave(&chip->spinlock, flags);
+
 	/* Set interrupt mode */
 	im = ioread32(im_reg) & ~(PCH_IM_MASK << (im_pos * 4));
 	iowrite32(im | (val << (im_pos * 4)), im_reg);

@@ -270,7 +270,6 @@ static int pch_irq_type(struct irq_data *d, unsigned int type)
 	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
 		irq_set_handler_locked(d, handle_edge_irq);
 
-unlock:
 	spin_unlock_irqrestore(&chip->spinlock, flags);
 	return 0;
 }

Signature · 1 line

2.25.1

Andy Shevchenko

author

2020-04-14 17:50 UTC #3 lore raw

PATCH v2 4/4 gpio: pch: Use in pch_irq_type() macros provided by IRQ core

In reply to Andy Shevchenko

Use in pch_irq_type() the macros provided by IRQ core for IRQ type.

Signed-off-by Andy Shevchenko <[email protected]>

---
v2: new patch

 drivers/gpio/gpio-pch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpio/gpio-pch.c +2 -2

index 9c34230f2e84..e96d28bf43b4 100644
@@ -265,9 +265,9 @@ static int pch_irq_type(struct irq_data *d, unsigned int type)
 	iowrite32(im | (val << (im_pos * 4)), im_reg);
 
 	/* And the handler */
-	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
+	if (type & IRQ_TYPE_LEVEL_MASK)
 		irq_set_handler_locked(d, handle_level_irq);
-	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
+	else if (type & IRQ_TYPE_EDGE_BOTH)
 		irq_set_handler_locked(d, handle_edge_irq);
 
 	spin_unlock_irqrestore(&chip->spinlock, flags);

Signature · 1 line

2.25.1

Linus Walleij

2020-04-16 08:36 UTC #4 lore raw

In reply to Andy Shevchenko

Quoted 6 lines · 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 <[email protected]>
---
v2: update one more macro (all IRQ settings are plain numbers, not bits)

I managed to extract this v2 series with b4 and applied it.

I don't know if you planned to send a pull request for the PCH changes, it was so simple to just use b4 that I tested it on this patch series and it just worked.

Yours,
Linus Walleij

Andy Shevchenko

author

2020-04-16 11:42 UTC #5 lore raw

Quoted 10 lines · Linus Walleij

Quoted 6 lines · 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 <[email protected]>
---
v2: update one more macro (all IRQ settings are plain numbers, not bits)

Quoted 5 lines

I managed to extract this v2 series with b4 and applied it.
I don't know if you planned to send a pull request for the PCH changes, it was so simple to just use b4 that I tested it on this patch series and it just worked.

Thanks!

I would like to have rather Ack and send a PR, since there are more Intel GPIO patches in a bunch.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

2020-04-16 12:21 UTC #6 lore raw

Quoted 13 lines · Andy Shevchenko

Quoted 12 lines · Linus Walleij
Quoted 6 lines · 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 <[email protected]>
---
v2: update one more macro (all IRQ settings are plain numbers, not bits)
I managed to extract this v2 series with b4 and applied it.

Quoted 8 lines

Quoted 3 lines
I don't know if you planned to send a pull request for the PCH changes, it was so simple to just use b4 that I tested it on this patch series and it just worked.
Thanks!
I would like to have rather Ack and send a PR, since there are more Intel GPIO patches in a bunch.

OK I dropped the PCH patches from my branch!

Sorry for missing this.

Yours,
Linus Walleij