This is the first series of improvements to the tqmx86 GPIO driver,
which fixes some long-standing bugs - in particular, IRQ_TYPE_EDGE_BOTH
can never have worked correctly.
Other patches in the series are code cleanup, which is included as it
makes the actual fixes much nicer. I have included the same Fixes tag in
all commits, as they will need to be cherry-picked together.
A second series with new features (changing GPIO directions, support
more GPIOs on SMARC TQMx86 modules) will be submitted when the fixes
have been reviewed and merged.
Gregor Herburger (1):
gpio: tqmx86: fix typo in Kconfig label
Matthias Schiffer (7):
gpio: tqmx86: introduce shadow register for GPIO output value
gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match
regmap API
gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper
gpio: tqmx86: add macros for interrupt configuration
gpio: tqmx86: store IRQ triggers without offsetting index
gpio: tqmx86: store IRQ trigger type and unmask status separately
gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type
drivers/gpio/Kconfig | 2 +-
drivers/gpio/gpio-tqmx86.c | 151 ++++++++++++++++++++++++++-----------
2 files changed, 106 insertions(+), 47 deletions(-)
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
irq_set_type() should not implicitly unmask the IRQ.
All accesses to the interrupt configuration register are moved to a new
helper _tqmx86_gpio_irq_config(). We also introduce the new rule that
accessing irq_type must happen while locked, which will become
significant for fixing EDGE_BOTH handling.
Fixes: b868db94a6a7 ("gpio: tqmx86: Add GPIO from for this IO controller")
Signed-off-by: Matthias Schiffer <[email protected]>
---
drivers/gpio/gpio-tqmx86.c | 41 +++++++++++++++++++++++---------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 4b37cc3bdd455..c957be3341774 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -34,6 +34,7 @@
#define TQMX86_INT_TRIG_RISING 0x2
#define TQMX86_INT_TRIG_BOTH 0x3
#define TQMX86_INT_TRIG_MASK 0x3
+#define TQMX86_INT_UNMASKED BIT(2)
#define TQMX86_GPII_CONFIG(i, v) ((v) << (2 * (i)))
#define TQMX86_GPII_MASK(i) TQMX86_GPII_CONFIG(i, TQMX86_INT_TRIG_MASK)
@@ -42,6 +43,7 @@ struct tqmx86_gpio_data {
struct gpio_chip chip;
void __iomem *io_base;
int irq;
+ /* Lock must be held for accessing output and irq_type fields */
raw_spinlock_t spinlock;
DECLARE_BITMAP(output, TQMX86_NGPIO);
u8 irq_type[TQMX86_NGPIO];
@@ -119,17 +121,28 @@ static int tqmx86_gpio_get_direction(struct gpio_chip *chip,
return GPIO_LINE_DIRECTION_OUT;
}
+static void _tqmx86_gpio_irq_config(struct tqmx86_gpio_data *gpio, int hwirq)
+{
+ unsigned int offset = hwirq - TQMX86_NGPO;
+ u8 type = TQMX86_INT_TRIG_NONE, mask, val;
+
+ if (gpio->irq_type[hwirq] & TQMX86_INT_UNMASKED)
+ type = gpio->irq_type[hwirq] & TQMX86_INT_TRIG_MASK;
+
+ mask = TQMX86_GPII_MASK(offset);
+ val = TQMX86_GPII_CONFIG(offset, type);
+ _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
+}
+
static void tqmx86_gpio_irq_mask(struct irq_data *data)
{
- unsigned int offset = (data->hwirq - TQMX86_NGPO);
struct tqmx86_gpio_data *gpio = gpiochip_get_data(
irq_data_get_irq_chip_data(data));
unsigned long flags;
- u8 mask;
- mask = TQMX86_GPII_MASK(offset);
raw_spin_lock_irqsave(&gpio->spinlock, flags);
- _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, 0);
+ gpio->irq_type[data->hwirq] &= ~TQMX86_INT_UNMASKED;
+ _tqmx86_gpio_irq_config(gpio, data->hwirq);
raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
gpiochip_disable_irq(&gpio->chip, irqd_to_hwirq(data));
@@ -137,18 +150,15 @@ static void tqmx86_gpio_irq_mask(struct irq_data *data)
static void tqmx86_gpio_irq_unmask(struct irq_data *data)
{
- unsigned int offset = (data->hwirq - TQMX86_NGPO);
struct tqmx86_gpio_data *gpio = gpiochip_get_data(
irq_data_get_irq_chip_data(data));
unsigned long flags;
- u8 mask, val;
gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(data));
- mask = TQMX86_GPII_MASK(offset);
- val = TQMX86_GPII_CONFIG(offset, gpio->irq_type[data->hwirq]);
raw_spin_lock_irqsave(&gpio->spinlock, flags);
- _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
+ gpio->irq_type[data->hwirq] |= TQMX86_INT_UNMASKED;
+ _tqmx86_gpio_irq_config(gpio, data->hwirq);
raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
}
@@ -156,10 +166,9 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
{
struct tqmx86_gpio_data *gpio = gpiochip_get_data(
irq_data_get_irq_chip_data(data));
- unsigned int offset = (data->hwirq - TQMX86_NGPO);
unsigned int edge_type = type & IRQF_TRIGGER_MASK;
unsigned long flags;
- u8 new_type, mask, val;
+ u8 new_type;
switch (edge_type) {
case IRQ_TYPE_EDGE_RISING:
@@ -175,12 +184,12 @@ static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
return -EINVAL; /* not supported */
}
- gpio->irq_type[data->hwirq] = new_type;
-
- mask = TQMX86_GPII_MASK(offset);
- val = TQMX86_GPII_CONFIG(offset, new_type);
raw_spin_lock_irqsave(&gpio->spinlock, flags);
- _tqmx86_gpio_update_bits(gpio, TQMX86_GPIIC, mask, val);
+
+ gpio->irq_type[data->hwirq] &= ~TQMX86_INT_TRIG_MASK;
+ gpio->irq_type[data->hwirq] |= new_type;
+ _tqmx86_gpio_irq_config(gpio, data->hwirq);
+
raw_spin_unlock_irqrestore(&gpio->spinlock, flags);
return 0;
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
On Wed, May 29, 2024 at 9:46 AM Matthias Schiffer
<[email protected]> wrote:
>
> This is the first series of improvements to the tqmx86 GPIO driver,
> which fixes some long-standing bugs - in particular, IRQ_TYPE_EDGE_BOTH
> can never have worked correctly.
>
> Other patches in the series are code cleanup, which is included as it
> makes the actual fixes much nicer. I have included the same Fixes tag in
> all commits, as they will need to be cherry-picked together.
>
> A second series with new features (changing GPIO directions, support
> more GPIOs on SMARC TQMx86 modules) will be submitted when the fixes
> have been reviewed and merged.
>
> Gregor Herburger (1):
> gpio: tqmx86: fix typo in Kconfig label
>
> Matthias Schiffer (7):
> gpio: tqmx86: introduce shadow register for GPIO output value
> gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match
> regmap API
> gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper
> gpio: tqmx86: add macros for interrupt configuration
> gpio: tqmx86: store IRQ triggers without offsetting index
> gpio: tqmx86: store IRQ trigger type and unmask status separately
> gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type
>
> drivers/gpio/Kconfig | 2 +-
> drivers/gpio/gpio-tqmx86.c | 151 ++++++++++++++++++++++++++-----------
> 2 files changed, 106 insertions(+), 47 deletions(-)
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> https://www.tq-group.com/
>
Hi Matthias!
Not all patches in this series are fixes (as in: warrant being sent
upstream outside of the merge window). Please split the series into
two with the first one containing actual fixes to real bugs and the
second for any refactoring and improvements.
Bart
On Wed, 2024-05-29 at 14:08 +0200, Bartosz Golaszewski wrote:
> On Wed, May 29, 2024 at 9:46 AM Matthias Schiffer
> <[email protected]> wrote:
> >
> > This is the first series of improvements to the tqmx86 GPIO driver,
> > which fixes some long-standing bugs - in particular, IRQ_TYPE_EDGE_BOTH
> > can never have worked correctly.
> >
> > Other patches in the series are code cleanup, which is included as it
> > makes the actual fixes much nicer. I have included the same Fixes tag in
> > all commits, as they will need to be cherry-picked together.
> >
> > A second series with new features (changing GPIO directions, support
> > more GPIOs on SMARC TQMx86 modules) will be submitted when the fixes
> > have been reviewed and merged.
> >
> > Gregor Herburger (1):
> > gpio: tqmx86: fix typo in Kconfig label
> >
> > Matthias Schiffer (7):
> > gpio: tqmx86: introduce shadow register for GPIO output value
> > gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match
> > regmap API
> > gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper
> > gpio: tqmx86: add macros for interrupt configuration
> > gpio: tqmx86: store IRQ triggers without offsetting index
> > gpio: tqmx86: store IRQ trigger type and unmask status separately
> > gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type
> >
> > drivers/gpio/Kconfig | 2 +-
> > drivers/gpio/gpio-tqmx86.c | 151 ++++++++++++++++++++++++++-----------
> > 2 files changed, 106 insertions(+), 47 deletions(-)
> >
> > --
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > https://www.tq-group.com/
> >
>
> Hi Matthias!
>
> Not all patches in this series are fixes (as in: warrant being sent
> upstream outside of the merge window). Please split the series into
> two with the first one containing actual fixes to real bugs and the
> second for any refactoring and improvements.
>
> Bart
Hi Bartosz,
as explained in the cover letter, I've found that the fixes become much nicer to implement (and to
review) if they are based on the refactoring. I can leave out _tqmx86_gpio_update_bits for now, but
removing "add macros for interrupt configuration" and "store IRQ triggers without offsetting index"
does make the actual fixes "store IRQ trigger type and unmask status separately" and "fix broken
IRQ_TYPE_EDGE_BOTH interrupt type" somewhat uglier.
That being said, you're the maintainer, and I will structure this series in any way you prefer. I
can remove the mentioned refactoring, even though it makes the fixes less pleasant. Another option
would be that I can submit just the refactoring for -next for now, and leave the fixes for a future
series. Let me know how you want to proceed.
Thanks,
Matthias
On Wed, May 29, 2024 at 2:55 PM Matthias Schiffer
<[email protected]> wrote:
>
> On Wed, 2024-05-29 at 14:08 +0200, Bartosz Golaszewski wrote:
> > On Wed, May 29, 2024 at 9:46 AM Matthias Schiffer
> > <[email protected]> wrote:
> > >
> > > This is the first series of improvements to the tqmx86 GPIO driver,
> > > which fixes some long-standing bugs - in particular, IRQ_TYPE_EDGE_BOTH
> > > can never have worked correctly.
> > >
> > > Other patches in the series are code cleanup, which is included as it
> > > makes the actual fixes much nicer. I have included the same Fixes tag in
> > > all commits, as they will need to be cherry-picked together.
> > >
> > > A second series with new features (changing GPIO directions, support
> > > more GPIOs on SMARC TQMx86 modules) will be submitted when the fixes
> > > have been reviewed and merged.
> > >
> > > Gregor Herburger (1):
> > > gpio: tqmx86: fix typo in Kconfig label
> > >
> > > Matthias Schiffer (7):
> > > gpio: tqmx86: introduce shadow register for GPIO output value
> > > gpio: tqmx86: change tqmx86_gpio_write() order of arguments to match
> > > regmap API
> > > gpio: tqmx86: introduce _tqmx86_gpio_update_bits() helper
> > > gpio: tqmx86: add macros for interrupt configuration
> > > gpio: tqmx86: store IRQ triggers without offsetting index
> > > gpio: tqmx86: store IRQ trigger type and unmask status separately
> > > gpio: tqmx86: fix broken IRQ_TYPE_EDGE_BOTH interrupt type
> > >
> > > drivers/gpio/Kconfig | 2 +-
> > > drivers/gpio/gpio-tqmx86.c | 151 ++++++++++++++++++++++++++-----------
> > > 2 files changed, 106 insertions(+), 47 deletions(-)
> > >
> > > --
> > > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > > Amtsgericht München, HRB 105018
> > > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > > https://www.tq-group.com/
> > >
> >
> > Hi Matthias!
> >
> > Not all patches in this series are fixes (as in: warrant being sent
> > upstream outside of the merge window). Please split the series into
> > two with the first one containing actual fixes to real bugs and the
> > second for any refactoring and improvements.
> >
> > Bart
>
>
> Hi Bartosz,
>
> as explained in the cover letter, I've found that the fixes become much nicer to implement (and to
> review) if they are based on the refactoring. I can leave out _tqmx86_gpio_update_bits for now, but
> removing "add macros for interrupt configuration" and "store IRQ triggers without offsetting index"
> does make the actual fixes "store IRQ trigger type and unmask status separately" and "fix broken
> IRQ_TYPE_EDGE_BOTH interrupt type" somewhat uglier.
>
> That being said, you're the maintainer, and I will structure this series in any way you prefer. I
> can remove the mentioned refactoring, even though it makes the fixes less pleasant. Another option
> would be that I can submit just the refactoring for -next for now, and leave the fixes for a future
> series. Let me know how you want to proceed.
>
> Thanks,
> Matthias
The question is: do you want these fixes to be backported into stable
branches? Because if not then it's true that the ordering doesn't
matter. But if you do, then it makes more sense to put fixes first,
send them to Torvalds, get them backported and then add refactoring
changes on top.
Bart