| Message ID | 20200402201859.35832-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] gpio: pch: Use BIT() and GENMASK() where it's appropriate | expand |
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
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.
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
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.
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);
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(-)