| Message ID | 0a21e3f14742e9adcf29361f7f2867199cd0dd4a.1716967982.git.matthias.schiffer@ew.tq-group.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio-tqmx86 fixes | expand |
On Wed, May 29, 2024 at 09:45:16AM +0200, Matthias Schiffer wrote: > Simplify a lot of code in the driver by introducing helpers for the > common RMW pattern. No tqmx86_gpio_update_bits() function with builtin > locking is added, as it would become redundant with the following fixes, > which further consolidate interrupt configuration register setup. > > No functional change intended. > > Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller") > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > --- > drivers/gpio/gpio-tqmx86.c | 40 ++++++++++++++++++++++---------------- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c > index 613ab9ef2e744..7a851e1730dd1 100644 > --- a/drivers/gpio/gpio-tqmx86.c > +++ b/drivers/gpio/gpio-tqmx86.c > @@ -54,6 +54,17 @@ static void tqmx86_gpio_write(struct tqmx86_gpio_data *gd, unsigned int reg, > iowrite8(val, gd->io_base + reg); > } > > +static void _tqmx86_gpio_update_bits(struct tqmx86_gpio_data *gd, > + unsigned int reg, u8 mask, u8 val) Why the _ prefix? This is a local function, it is static, so you don't have name space issues. Functions starting with _ are those you should not call without a good reason, there is generally a version without the _ prefix which is the real function to use. So i would drop the _. This is also not a fix. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html and stick to the rules described there. I don't know how the GPIO tree works, but for netdev, about a week after fixes are merged, they appear in net-next. So you can then build on top of them for development work. Andrew
On Wed, 2024-05-29 at 14:19 +0200, Andrew Lunn wrote: > > On Wed, May 29, 2024 at 09:45:16AM +0200, Matthias Schiffer wrote: > > Simplify a lot of code in the driver by introducing helpers for the > > common RMW pattern. No tqmx86_gpio_update_bits() function with builtin > > locking is added, as it would become redundant with the following fixes, > > which further consolidate interrupt configuration register setup. > > > > No functional change intended. > > > > Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller") > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > --- > > drivers/gpio/gpio-tqmx86.c | 40 ++++++++++++++++++++++---------------- > > 1 file changed, 23 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c > > index 613ab9ef2e744..7a851e1730dd1 100644 > > --- a/drivers/gpio/gpio-tqmx86.c > > +++ b/drivers/gpio/gpio-tqmx86.c > > @@ -54,6 +54,17 @@ static void tqmx86_gpio_write(struct tqmx86_gpio_data *gd, unsigned int reg, > > iowrite8(val, gd->io_base + reg); > > } > > > > +static void _tqmx86_gpio_update_bits(struct tqmx86_gpio_data *gd, > > + unsigned int reg, u8 mask, u8 val) > > Why the _ prefix? This is a local function, it is static, so you don't > have name space issues. Functions starting with _ are those you should > not call without a good reason, there is generally a version without > the _ prefix which is the real function to use. So i would drop the _. My intention was to mark functions that need to be called while holding the spinlock with a _ prefix. Should I just remove the prefix and add a comment instead? Matthias > > This is also not a fix. Please read: > > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > > and stick to the rules described there. > > I don't know how the GPIO tree works, but for netdev, about a week > after fixes are merged, they appear in net-next. So you can then build > on top of them for development work. > > Andrew
> My intention was to mark functions that need to be called while holding the spinlock with a _ > prefix. Should I just remove the prefix and add a comment instead? Yes. You could also add sparse markup of the locks, or add an assert_spin_locked(lock); Andrew
diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c index 613ab9ef2e744..7a851e1730dd1 100644 --- a/drivers/gpio/gpio-tqmx86.c +++ b/drivers/gpio/gpio-tqmx86.c @@ -54,6 +54,17 @@ static void tqmx86_gpio_write(struct tqmx86_gpio_data *gd, unsigned int reg, iowrite8(val, gd->io_base + reg); } +static void _tqmx86_gpio_update_bits(struct tqmx86_gpio_data *gd, + unsigned int reg, u8 mask, u8 val) +{ + u8 tmp = tqmx86_gpio_read(gd, reg); + + tmp &= ~mask; + tmp |= val & mask; + + tqmx86_gpio_write(gd, reg, tmp); +} + static int tqmx86_gpio_get(struct gpio_chip *chip, unsigned int offset) { struct tqmx86_gpio_data *gpio = gpiochip_get_data(chip); @@ -110,15 +121,13 @@ static void tqmx86_gpio_irq_mask(struct irq_data *data) struct tqmx86_gpio_data *gpio = gpiochip_get_data( irq_data_get_irq_chip_data(data)); unsigned long flags; - u8 gpiic, mask; + u8 mask; mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS); - raw_spin_lock_irqsave(&gpio->spinlock, flags); - gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC); - gpiic &= ~mask; - tqmx86_gpio_write(gpio, TQMX86_GPIIC, gpiic); + _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, 0); raw_spin_unlock_irqrestore(&gpio->spinlock, flags); + gpiochip_disable_irq(&gpio->chip, irqd_to_hwirq(data)); } @@ -128,16 +137,14 @@ static void tqmx86_gpio_irq_unmask(struct irq_data *data) struct tqmx86_gpio_data *gpio = gpiochip_get_data( irq_data_get_irq_chip_data(data)); unsigned long flags; - u8 gpiic, mask; - - mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS); + u8 mask, val; gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(data)); + + mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS); + val = gpio->irq_type[offset] << (offset * TQMX86_GPII_BITS); raw_spin_lock_irqsave(&gpio->spinlock, flags); - gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC); - gpiic &= ~mask; - gpiic |= gpio->irq_type[offset] << (offset * TQMX86_GPII_BITS); - tqmx86_gpio_write(gpio, TQMX86_GPIIC, gpiic); + _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val); raw_spin_unlock_irqrestore(&gpio->spinlock, flags); } @@ -148,7 +155,7 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type) unsigned int offset = (data->hwirq - TQMX86_NGPO); unsigned int edge_type = type & IRQF_TRIGGER_MASK; unsigned long flags; - u8 new_type, gpiic; + u8 new_type, mask, val; switch (edge_type) { case IRQ_TYPE_EDGE_RISING: @@ -166,11 +173,10 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type) gpio->irq_type[offset] = new_type; + mask = TQMX86_GPII_MASK << (offset * TQMX86_GPII_BITS); + val = new_type << (offset * TQMX86_GPII_BITS); raw_spin_lock_irqsave(&gpio->spinlock, flags); - gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC); - gpiic &= ~((TQMX86_GPII_MASK) << (offset * TQMX86_GPII_BITS)); - gpiic |= new_type << (offset * TQMX86_GPII_BITS); - tqmx86_gpio_write(gpio, TQMX86_GPIIC, gpiic); + _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val); raw_spin_unlock_irqrestore(&gpio->spinlock, flags); return 0;
Simplify a lot of code in the driver by introducing helpers for the common RMW pattern. No tqmx86_gpio_update_bits() function with builtin locking is added, as it would become redundant with the following fixes, which further consolidate interrupt configuration register setup. No functional change intended. Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller") Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- drivers/gpio/gpio-tqmx86.c | 40 ++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 17 deletions(-)