Re: [PATCH] gpio: amdptpci: add support for AMD PT PCI controller
From: Linus Walleij
Date: Wed Oct 07 2026 - 07:46:07 EST
Hi Szuying,
thanks for your patch!
On Mon, Oct 5, 2026 at 5:54 AM Szuying Chen <Chloe_Chen@xxxxxxxxxxxxxx> wrote:
> This commit implements the new gpio-amdptpci driver to support the
> GPIO functionality on AMD PT PCI platforms.
>
> Signed-off-by: Szuying Chen <Chloe_Chen@xxxxxxxxxxxxxx>
> Co-authored-by: YD Tseng <Yd_Tseng@xxxxxxxxxxxxxx>
(...)
> +#define PT27_REG_DIR 0x0000
> +#define PT27_REG_IN 0x0004
> +#define PT27_REG_OUT 0x0008
> +#define PT27_REG_INT_EN 0x000C
> +#define PT27_REG_INT_LVL 0x0010
> +#define PT27_REG_INT_MODE0 0x0014
> +#define PT27_REG_INT_MODE1 0x0018
> +#define PT27_REG_INT_STAT 0x001C
> +#define PT27_REG_INT_MASK 0x0020
> +#define PT27_REG_INT_CTL 0x0024
Compare this to drivers/gpio/gpio-amdpt.c:
/* PCI-E MMIO register offsets */
#define PT_DIRECTION_REG 0x00
#define PT_INPUTDATA_REG 0x04
#define PT_OUTPUTDATA_REG 0x08
#define PT_CLOCKRATE_REG 0x0C
#define PT_SYNC_REG 0x28
So this hardware appears to be related?
I can see that there is a lot of modifications to it though, and that it
may warrant a new driver. But this needs to be motivated in the
commit message.
The fact that one is an ACPI-probed MMIO driver and the other is
a PCI driver doesn't really matter for code sharing though, I need
a better motivation if the drivers should not share code.
> +static inline u32 pt27_rd32(struct pt27_gpio *c, u32 off)
> +{
> + return ioread32(c->base + off);
> +}
> +
> +static inline void pt27_wr32(struct pt27_gpio *c, u32 off, u32 v)
> +{
> + iowrite32(v, c->base + off);
> +}
> +
> +static inline u16 pt27_rd16(struct pt27_gpio *c, u32 off)
> +{
> + return ioread16(c->base + off);
> +}
> +
> +static inline void pt27_wr16(struct pt27_gpio *c, u32 off, u16 v)
> +{
> + iowrite16(v, c->base + off);
> +}
> +
> +static inline u8 pt27_rd8(struct pt27_gpio *c, u32 off)
> +{
> + return ioread8(c->base + off);
> +}
> +
> +static inline void pt27_wr8(struct pt27_gpio *c, u32 off, u8 v)
> +{
> + iowrite8(v, c->base + off);
> +}
What is the point of all this indirection? Just use the ioread/write
accessors directly in the code.
> +static void pt27_runtime_put_autosuspend(struct pt27_gpio *chip)
> +{
> + pm_runtime_mark_last_busy(chip->dev);
> + pm_runtime_put_autosuspend(chip->dev);
> +
> +}
Runtime PM already has a helper function like this, use it.
> +static void pt27_save_regs(struct pt27_gpio *chip)
> +{
> + chip->saved.dir = pt27_rd32(chip, PT27_REG_DIR);
> + chip->saved.out = pt27_rd32(chip, PT27_REG_OUT);
> + chip->saved.int_en = pt27_rd32(chip, PT27_REG_INT_EN);
> + chip->saved.int_lvl = pt27_rd32(chip, PT27_REG_INT_LVL);
> + chip->saved.int_mode0 = pt27_rd32(chip, PT27_REG_INT_MODE0);
> + chip->saved.int_mode1 = pt27_rd16(chip, PT27_REG_INT_MODE1);
> + chip->saved.int_mask = pt27_rd32(chip, PT27_REG_INT_MASK);
> + chip->saved.int_ctl = pt27_rd8(chip, PT27_REG_INT_CTL);
> + chip->saved.vendor0 = pt27_rd32(chip, PT27_REG_VENDOR0);
> + chip->saved.vendor1 = pt27_rd32(chip, PT27_REG_VENDOR1);
> + chip->saved.wake_en = pt27_rd16(chip, PT27_REG_WAKE_EN);
> + chip->saved.wake_lvl = pt27_rd16(chip, PT27_REG_WAKE_LVL);
> + chip->saved.wake_mode = pt27_rd16(chip, PT27_REG_WAKE_MODE);
> + chip->saved_valid = true;
> +
> +}
> +
> +static void pt27_restore_regs(struct pt27_gpio *chip)
> +{
> + pt27_wr32(chip, PT27_REG_INT_EN, 0);
> + pt27_wr32(chip, PT27_REG_INT_MASK, PT27_GPIO_MASK);
> +
> + pt27_wr32(chip, PT27_REG_DIR, chip->saved.dir);
> + pt27_wr32(chip, PT27_REG_OUT, chip->saved.out);
> + pt27_wr32(chip, PT27_REG_INT_LVL, chip->saved.int_lvl);
> + pt27_wr32(chip, PT27_REG_INT_MODE0, chip->saved.int_mode0);
> + pt27_wr16(chip, PT27_REG_INT_MODE1, chip->saved.int_mode1);
> + pt27_wr8(chip, PT27_REG_INT_CTL, chip->saved.int_ctl);
> + pt27_wr32(chip, PT27_REG_VENDOR0, chip->saved.vendor0);
> + pt27_wr32(chip, PT27_REG_VENDOR1, chip->saved.vendor1);
> +
> + pt27_wr16(chip, PT27_REG_WAKE_LVL, chip->saved.wake_lvl);
> + pt27_wr16(chip, PT27_REG_WAKE_MODE, chip->saved.wake_mode);
> + pt27_wr16(chip, PT27_REG_WAKE_EN, chip->saved.wake_en);
> +
> + pt27_wr32(chip, PT27_REG_INT_STAT, PT27_GPIO_MASK);
> + pt27_wr16(chip, PT27_REG_WAKE_STAT, PT27_WAKE_MASK);
> +
> + pt27_wr32(chip, PT27_REG_INT_EN, chip->saved.int_en);
> + pt27_wr32(chip, PT27_REG_INT_MASK, chip->saved.int_mask);
> +
> +}
If you're gonna do this what about using regmap which already has
an internal register cache, knows how to restore them and can deal
with the different word sizes and all?
> +static int pt27_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
(...)
> + val &= ~BIT(offset); /* 0 = input */
(...)
> +static int pt27_gpio_direction_output(struct gpio_chip *gc, unsigned int offset,
> + int value)
> +{
> + if (value)
> + val |= BIT(offset);
> + else
> + val &= ~BIT(offset);
(...)
> +static int pt27_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
(...)
> + return (val & BIT(offset)) ? GPIO_LINE_DIRECTION_OUT
> + : GPIO_LINE_DIRECTION_IN;
(...)
> +static int pt27_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> + dir = pt27_rd32(chip, PT27_REG_DIR);
> + val = pt27_rd32(chip, (dir & BIT(offset)) ? PT27_REG_OUT : PT27_REG_IN);
(...)
> +static int pt27_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
> + val = pt27_rd32(chip, PT27_REG_OUT);
> + if (value)
> + val |= BIT(offset);
> + else
> + val &= ~BIT(offset);
(...)
Just use select GPIO_GENERIC and follow the pattern of the
other drivers using this. Something like:
struct gpio_generic_chip_config config;
config = (struct gpio_generic_chip_config) {
.dev = dev,
.sz = 4,
.dat = g->base + PT27_REG_IN,
.set = g->base + PT27_REG_OUT,
.dirout = g->base + PT27_REG_DIR,
.flags = GPIO_GENERIC_READ_OUTPUT_REG_SET,
};
If you *absolutely* need this runtime PM around all accessors, then
implement generic runtime PM handling to the gpio-mmio.c library and
add a new GPIO_GENERIC_RUNTIME_PM flag to
<linux/gpio/generic.h> as a separate patch.
> +static bool pt27_irq_regs_accessible(struct pt27_gpio *chip)
> +{
> +
> + return READ_ONCE(chip->irq_bus_active) ||
> + READ_ONCE(chip->irq_pm_held);
> +}
Looks like second-guessing the state of runtime PM?
This can't be right. Make sure the code only access
registers when runtime PM is resumed.
For example whenever the irqchip .enable() callback
is called, get runtime PM, put it on .disable().
> +static void pt27_irq_unmask(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct pt27_gpio *chip = gpiochip_get_data(gc);
> + irq_hw_number_t hwirq = irqd_to_hwirq(d);
> + unsigned long flags;
> + u32 val;
> +
> + if (!pt27_irq_regs_accessible(chip)) {
> + dev_err_ratelimited(chip->dev,
> + "%s: cannot unmask GPIO%lu; PT27 is not in D0\n",
> + __func__, hwirq);
> + return;
> + }
This looks wrong as per above reasoning.
> + chip->enabled_irq_mask |= BIT(hwirq);
I don't see why you would track this.
> +static void pt27_irq_bus_lock(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct pt27_gpio *chip = gpiochip_get_data(gc);
> + int ret;
> +
> + mutex_lock(&chip->irq_bus_lock);
> +
> + ret = pm_runtime_resume_and_get(chip->dev);
> + if (ret < 0) {
> + WRITE_ONCE(chip->irq_bus_active, false);
> + dev_err_ratelimited(chip->dev,
> + "%s: cannot resume for IRQ configuration: %d\n",
> + __func__, ret);
> + return;
> + }
> +
> + WRITE_ONCE(chip->irq_bus_active, true);
Why is this tracked locally in the driver? Surely the
frameworks (runtime PM or irqchip) track this.
> +}
Isn't this solving the issue I pointed out above? If yes,
why is the workaround still there?
I will probably find more on further reviews but these things
are a good starting point for reworking the driver.
Yours,
Linus Walleij