Hi Andrew,
Thanks for your suggestion. As I understand it, you’re suggesting that this driver should share the
common parts with aspeed-gpio.c, correct?
However, I don’t think that’s necessary. You can treat it as a new GPIO controller because the
register layout is quite different from aspeed-gpio.c.
If I try to make it common, the driver could become too complex, potentially requiring a separate
gpio-aspeed-common.c and necessitating changes to the existing aspeed-gpio.c
Maybe the discussion of merging aspeed-gpio.c and this driver can be postponed until after this one
is accepted?
For Others, please see the reply inline.
Best regards,
Billy Tsai
> > In the 7th generation of the SoC from Aspeed, the control logic of the
> > GPIO controller has been updated to support per-pin control. Each pin now
> > has its own 32-bit register, allowing for individual control of the pin’s
> > value, direction, interrupt type, and other settings.
> >
> > Signed-off-by: Billy Tsai <billy_tsai@xxxxxxxxxxxxxx>
> > ---
> > drivers/gpio/Kconfig | 7 +
> > drivers/gpio/Makefile | 1 +
> > drivers/gpio/gpio-aspeed-g7.c | 831 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 839 insertions(+)
> > create mode 100644 drivers/gpio/gpio-aspeed-g7.c
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 58f43bcced7c..93f237429b92 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -172,6 +172,13 @@ config GPIO_ASPEED
> > help
> > Say Y here to support Aspeed AST2400 and AST2500 GPIO controllers.
> >
> > +config GPIO_ASPEED_G7
> > + tristate "Aspeed G7 GPIO support"
> > + depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
> > + select GPIOLIB_IRQCHIP
> > + help
> > + Say Y here to support Aspeed AST2700 GPIO controllers.
> > +
> > config GPIO_ASPEED_SGPIO
> > bool "Aspeed SGPIO support"
> > depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 64dd6d9d730d..e830291761ee 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -34,6 +34,7 @@ obj-$(CONFIG_GPIO_AMD_FCH) += gpio-amd-fch.o
> > obj-$(CONFIG_GPIO_AMDPT) += gpio-amdpt.o
> > obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
> > obj-$(CONFIG_GPIO_ASPEED) += gpio-aspeed.o
> > +obj-$(CONFIG_GPIO_ASPEED_G7) += gpio-aspeed-g7.o
> > obj-$(CONFIG_GPIO_ASPEED_SGPIO) += gpio-aspeed-sgpio.o
> > obj-$(CONFIG_GPIO_ATH79) += gpio-ath79.o
> > obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o
> > diff --git a/drivers/gpio/gpio-aspeed-g7.c b/drivers/gpio/gpio-aspeed-g7.c
> > new file mode 100644
> > index 000000000000..dbca097de6ea
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-aspeed-g7.c
> > @@ -0,0 +1,831 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2024 Aspeed Technology Inc.
> > + *
> > + * Billy Tsai <billy_tsai@xxxxxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/gpio/aspeed.h>
> I think you should either drop this include or rework the existing
> implementations so the coprocessor handshake works correctly.
The coprocessor handshake will be implemented later, so I will remove the related include for now.
> > +#include <linux/gpio/driver.h>
> > +#include <linux/hashtable.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/string.h>
> > +
> > +#include <asm/div64.h>
> > +
> > +#define GPIO_G7_IRQ_STS_BASE 0x100
> > +#define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4)
> > +#define GPIO_G7_CTRL_REG_BASE 0x180
> > +#define GPIO_G7_CTRL_REG_OFFSET(x) (GPIO_G7_CTRL_REG_BASE + (x) * 0x4)
> > +#define GPIO_G7_OUT_DATA BIT(0)
> > +#define GPIO_G7_DIR BIT(1)
> > +#define GPIO_G7_IRQ_EN BIT(2)
> > +#define GPIO_G7_IRQ_TYPE0 BIT(3)
> > +#define GPIO_G7_IRQ_TYPE1 BIT(4)
> > +#define GPIO_G7_IRQ_TYPE2 BIT(5)
> > +#define GPIO_G7_RST_TOLERANCE BIT(6)
> > +#define GPIO_G7_DEBOUNCE_SEL GENMASK(8, 7)
> > +#define GPIO_G7_INPUT_MASK BIT(9)
> > +#define GPIO_G7_IRQ_STS BIT(12)
> > +#define GPIO_G7_IN_DATA BIT(13)
> Can you please add `CTRL` into these field macro names so it's clear
> they relate to the control register?
Okay.
> > +/*
> > + * The configuration of the following registers should be determined
> > + * outside of the GPIO driver.
> Where though?
The usage of the following registers hasn’t been implemented yet, so I will remove them for now.
> > + */
> > +#define GPIO_G7_PRIVILEGE_W_REG_BASE 0x810
> > +#define GPIO_G7_PRIVILEGE_W_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_W_REG_BASE + ((x) >> 2) * 0x4)
> > +#define GPIO_G7_PRIVILEGE_R_REG_BASE 0x910
> > +#define GPIO_G7_PRIVILEGE_R_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_R_REG_BASE + ((x) >> 2) * 0x4)
> > +#define GPIO_G7_IRQ_TARGET_REG_BASE 0xA10
> > +#define GPIO_G7_IRQ_TARGET_REG_OFFSET(x) (GPIO_G7_IRQ_TARGET_REG_BASE + ((x) >> 2) * 0x4)
> > +#define GPIO_G7_IRQ_TO_INTC2_18 BIT(0)
> > +#define GPIO_G7_IRQ_TO_INTC2_19 BIT(1)
> > +#define GPIO_G7_IRQ_TO_INTC2_20 BIT(2)
> > +#define GPIO_G7_IRQ_TO_SIO BIT(3)
> > +#define GPIO_G7_IRQ_TARGET_RESET_TOLERANCE BIT(6)
> > +#define GPIO_G7_IRQ_TARGET_W_PROTECT BIT(7)
> > +
> > +static inline u32 field_get(u32 _mask, u32 _val)
> > +{
> > + return (((_val) & (_mask)) >> (ffs(_mask) - 1));
> > +}
> > +
> > +static inline u32 field_prep(u32 _mask, u32 _val)
> > +{
> > + return (((_val) << (ffs(_mask) - 1)) & (_mask));
> > +}
> So linux/bitfield.h provides a lot of APIs along these lines, perhaps
> use them instead?
I will use FIELD_GET and FIELD_PREP to replace them.
> > +
> > +static inline void ast_write_bits(void __iomem *addr, u32 mask, u32 val)
> > +{
> > + iowrite32((ioread32(addr) & ~(mask)) | field_prep(mask, val), addr);
> > +}
> > +
> > +static inline void ast_clr_bits(void __iomem *addr, u32 mask)
> > +{
> > + iowrite32((ioread32(addr) & ~(mask)), addr);
> > +}
> > +
> > +struct aspeed_bank_props {
> > + unsigned int bank;
> > + u32 input;
> > + u32 output;
> > +};
> > +
> > +struct aspeed_gpio_g7_config {
> > + unsigned int nr_gpios;
> > + const struct aspeed_bank_props *props;
> > +};
> > +
> > +/*
> > + * @offset_timer: Maps an offset to an @timer_users index, or zero if disabled
> > + * @timer_users: Tracks the number of users for each timer
> > + *
> > + * The @timer_users has four elements but the first element is unused. This is
> > + * to simplify accounting and indexing, as a zero value in @offset_timer
> > + * represents disabled debouncing for the GPIO. Any other value for an element
> > + * of @offset_timer is used as an index into @timer_users. This behaviour of
> > + * the zero value aligns with the behaviour of zero built from the timer
> > + * configuration registers (i.e. debouncing is disabled).
> > + */
> > +struct aspeed_gpio_g7 {
> > + struct gpio_chip chip;
> > + struct device *dev;
> > + raw_spinlock_t lock;
> > + void __iomem *base;
> > + int irq;
> > + const struct aspeed_gpio_g7_config *config;
> > +
> > + u8 *offset_timer;
> > + unsigned int timer_users[4];
> > + struct clk *clk;
> > +
> > + u32 *dcache;
> > +};
> > +
> > +/*
> > + * Note: The "value" register returns the input value sampled on the
> > + * line even when the GPIO is configured as an output. Since
> > + * that input goes through synchronizers, writing, then reading
> > + * back may not return the written value right away.
> > + *
> > + * The "rdata" register returns the content of the write latch
> > + * and thus can be used to read back what was last written
> > + * reliably.
> > + */
> > +
> > +static const int debounce_timers[4] = { 0x00, 0x04, 0x00, 0x08 };
> This is all largely copy/pasted from gpio-aspeed.c. Can we split it out
> and share the definitions?
Do you mean moving them into the common header file?
The structure is fine, but I’m unsure about the debounce_timers.
It’s a static array, so I don’t think it needs to be shared with gpio-aspeed.c.
> > +
> > +#define GPIO_BANK(x) ((x) >> 5)
> > +#define GPIO_OFFSET(x) ((x) & 0x1f)
> > +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
> > +
> > +static inline bool is_bank_props_sentinel(const struct aspeed_bank_props *props)
> > +{
> > + return !(props->input || props->output);
> > +}
> > +
> > +static inline const struct aspeed_bank_props *find_bank_props(struct aspeed_gpio_g7 *gpio,
> > + unsigned int offset)
> > +{
> > + const struct aspeed_bank_props *props = gpio->config->props;
> > +
> > + while (!is_bank_props_sentinel(props)) {
> > + if (props->bank == GPIO_BANK(offset))
> > + return props;
> > + props++;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static inline bool have_gpio(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> > +{
> > + const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
> > +
> > + if (offset > gpio->chip.ngpio)
> > + return false;
> > +
> > + return (!props || ((props->input | props->output) & GPIO_BIT(offset)));
> > +}
> > +
> > +static inline bool have_input(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> > +{
> > + const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
> > +
> > + return !props || (props->input & GPIO_BIT(offset));
> > +}
> > +
> > +#define have_irq(g, o) have_input((g), (o))
> > +#define have_debounce(g, o) have_input((g), (o))
> > +
> > +static inline bool have_output(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> > +{
> > + const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
> > +
> > + return !props || (props->output & GPIO_BIT(offset));
> > +}
> > +
> This is all common as well.
Moving them into the common header file?
> > +static int aspeed_gpio_g7_get(struct gpio_chip *gc, unsigned int offset)
> > +{
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > + void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > + return !!(field_get(GPIO_G7_IN_DATA, ioread32(addr)));
> > +}
> > +
> > +static void __aspeed_gpio_g7_set(struct gpio_chip *gc, unsigned int offset, int val)
> > +{
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > + void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> The rest of the implementation of this function is broadly the same as
> in gpio-aspeed.c. The main difference is accounting for the address to
> access and the bit to whack. If we define some callbacks that replace
> GPIO_BANK()/to_bank() and GPIO_BIT() that can account for the
> differences in register layout, perhaps this could be common?
> The trade-off is some complexity vs copy-paste, but there does seem to
> be an awful lot of the latter with only minor changes so far.
Do you mean I need to create a gpio-aspeed-common.c, define the necessary common APIs,
and have gpio-aspeed.c and this driver hook into those APIs?
> > + u32 reg;
> > +
> > + reg = gpio->dcache[GPIO_BANK(offset)];
> > +
> > + if (val)
> > + reg |= GPIO_BIT(offset);
> > + else
> > + reg &= ~GPIO_BIT(offset);
> > + gpio->dcache[GPIO_BANK(offset)] = reg;
> > +
> > + ast_write_bits(addr, GPIO_G7_OUT_DATA, val);
> > +}
> > +
> > +static void aspeed_gpio_g7_set(struct gpio_chip *gc, unsigned int offset, int val)
> > +{
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > + unsigned long flags;
> > +
> > + raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + __aspeed_gpio_g7_set(gc, offset, val);
> > +
> > + raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +}
> > +
> > +static int aspeed_gpio_g7_dir_in(struct gpio_chip *gc, unsigned int offset)
> > +{
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > + void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > + unsigned long flags;
> > +
> > + if (!have_input(gpio, offset))
> > + return -EOPNOTSUPP;
> > +
> > + raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + ast_clr_bits(addr, GPIO_G7_DIR);
> > +
> > + raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int aspeed_gpio_g7_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
> > +{
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > + void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > + unsigned long flags;
> > +
> > + if (!have_output(gpio, offset))
> > + return -EOPNOTSUPP;
> > +
> > + raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + __aspeed_gpio_g7_set(gc, offset, val);
> > + ast_write_bits(addr, GPIO_G7_DIR, 1);
> > +
> > + raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int aspeed_gpio_g7_get_direction(struct gpio_chip *gc, unsigned int offset)
> > +{
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > + void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > + unsigned long flags;
> > + u32 val;
> > +
> > + if (!have_input(gpio, offset))
> > + return GPIO_LINE_DIRECTION_OUT;
> > +
> > + if (!have_output(gpio, offset))
> > + return GPIO_LINE_DIRECTION_IN;
> > +
> > + raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + val = ioread32(addr) & GPIO_G7_DIR;
> > +
> > + raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> > +}
> On top of handling the differences in the register layout as I
> mentioned above, the main difference in these get/set implementations
> is dropping the calls through the coprocessor handshake APIs. If you
> stub out the implementation of the coprocessor APIs I think it's likely
> these can be common. To do that you would need to make them use
> callbacks into the SoC-specific driver. To stub out the implementation
> you could leave the callback pointer as NULL for now.
Same as above?
> > +
> > +static inline int irqd_to_aspeed_gpio_g7_data(struct irq_data *d, struct aspeed_gpio_g7 **gpio,
> > + int *offset)
> > +{
> > + struct aspeed_gpio_g7 *internal;
> > +
> > + *offset = irqd_to_hwirq(d);
> > +
> > + internal = irq_data_get_irq_chip_data(d);
> > +
> > + /* This might be a bit of a questionable place to check */
> > + if (!have_irq(internal, *offset))
> > + return -EOPNOTSUPP;
> > +
> > + *gpio = internal;
> > +
> > + return 0;
> > +}
> You do have different data-types here (struct aspeed_gpio_g7), but
> possibly with appropriate struct definitions and use of container_of()
> by the caller, this could be common too?
> > +
> > +static void aspeed_gpio_g7_irq_ack(struct irq_data *d)
> > +{
> > + struct aspeed_gpio_g7 *gpio;
> > + unsigned long flags;
> > + void __iomem *addr;
> > + int rc, offset;
> > +
> > + rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
> > + if (rc)
> > + return;
> > +
> > + addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > + raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + ast_write_bits(addr, GPIO_G7_IRQ_STS, 1);
> > +
> > + raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +}
> > +
> > +static void aspeed_gpio_g7_irq_set_mask(struct irq_data *d, bool set)
> > +{
> > + struct aspeed_gpio_g7 *gpio;
> > + unsigned long flags;
> > + void __iomem *addr;
> > + int rc, offset;
> > +
> > + rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
> > + if (rc)
> > + return;
> > +
> > + addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > + /* Unmasking the IRQ */
> > + if (set)
> > + gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(d));
> > +
> > + raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + if (set)
> > + ast_write_bits(addr, GPIO_G7_IRQ_EN, 1);
> > + else
> > + ast_clr_bits(addr, GPIO_G7_IRQ_EN);
> > +
> > + raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + /* Masking the IRQ */
> > + if (!set)
> > + gpiochip_disable_irq(&gpio->chip, irqd_to_hwirq(d));
> > +}
> > +
> > +static void aspeed_gpio_g7_irq_mask(struct irq_data *d)
> > +{
> > + aspeed_gpio_g7_irq_set_mask(d, false);
> > +}
> > +
> > +static void aspeed_gpio_g7_irq_unmask(struct irq_data *d)
> > +{
> > + aspeed_gpio_g7_irq_set_mask(d, true);
> > +}
> > +
> > +static int aspeed_gpio_g7_set_type(struct irq_data *d, unsigned int type)
> > +{
> > + u32 type0 = 0;
> > + u32 type1 = 0;
> > + u32 type2 = 0;
> > + irq_flow_handler_t handler;
> > + struct aspeed_gpio_g7 *gpio;
> > + unsigned long flags;
> > + void __iomem *addr;
> > + int rc, offset;
> > +
> > + rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
> > + if (rc)
> > + return -EINVAL;
> > + addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > + switch (type & IRQ_TYPE_SENSE_MASK) {
> > + case IRQ_TYPE_EDGE_BOTH:
> > + type2 = 1;
> > + fallthrough;
> > + case IRQ_TYPE_EDGE_RISING:
> > + type0 = 1;
> > + fallthrough;
> > + case IRQ_TYPE_EDGE_FALLING:
> > + handler = handle_edge_irq;
> > + break;
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + type0 |= 1;
> > + fallthrough;
> > + case IRQ_TYPE_LEVEL_LOW:
> > + type1 |= 1;
> > + handler = handle_level_irq;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + ast_write_bits(addr, GPIO_G7_IRQ_TYPE2, type2);
> > + ast_write_bits(addr, GPIO_G7_IRQ_TYPE1, type1);
> > + ast_write_bits(addr, GPIO_G7_IRQ_TYPE0, type0);
> Can we write them as a field in the one call? They're all in the same
> register, which was not true in the previous controller register
> layout.
Okay.
> > +
> > + raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + irq_set_handler_locked(d, handler);
> > +
> > + return 0;
> > +}
> > +
> > +static void aspeed_gpio_g7_irq_handler(struct irq_desc *desc)
> > +{
> > + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> > + struct irq_chip *ic = irq_desc_get_chip(desc);
> > + unsigned int i, p, banks;
> > + unsigned long reg;
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > + void __iomem *addr;
> > +
> > + chained_irq_enter(ic, desc);
> > +
> > + banks = DIV_ROUND_UP(gpio->chip.ngpio, 32);
> > + for (i = 0; i < banks; i++) {
> > + addr = gpio->base + GPIO_G7_IRQ_STS_OFFSET(i);
> > +
> > + reg = ioread32(addr);
> > +
> > + for_each_set_bit(p, ®, 32)
> > + generic_handle_domain_irq(gc->irq.domain, i * 32 + p);
> > + }
> > +
> > + chained_irq_exit(ic, desc);
> > +}
> The only thing that's different for the IRQ status handling is the
> spread of the registers in the layout. In terms of the bits in each
> bank's IRQ status register, the layout is the same. By implementing the
> means to locate the status register as a callback this code could be
> common between the drivers.
> > +
> > +static void aspeed_init_irq_valid_mask(struct gpio_chip *gc, unsigned long *valid_mask,
> > + unsigned int ngpios)
> > +{
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > + const struct aspeed_bank_props *props = gpio->config->props;
> > +
> > + while (!is_bank_props_sentinel(props)) {
> > + unsigned int offset;
> > + const unsigned long input = props->input;
> > +
> > + /* Pretty crummy approach, but similar to GPIO core */
> > + for_each_clear_bit(offset, &input, 32) {
> > + unsigned int i = props->bank * 32 + offset;
> > +
> > + if (i >= gpio->chip.ngpio)
> > + break;
> > +
> > + clear_bit(i, valid_mask);
> > + }
> > +
> > + props++;
> > + }
> > +}
> This is the same except for the change to use `struct aspeed_gpio_g7`?
> Can we make this common?
> > +
> > +static int aspeed_gpio_g7_reset_tolerance(struct gpio_chip *chip, unsigned int offset, bool enable)
> > +{
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> > + unsigned long flags;
> > + void __iomem *addr;
> > +
> > + addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > + raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + if (enable)
> > + ast_write_bits(addr, GPIO_G7_RST_TOLERANCE, 1);
> > + else
> > + ast_clr_bits(addr, GPIO_G7_RST_TOLERANCE);
> > +
> > + raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int aspeed_gpio_g7_request(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + if (!have_gpio(gpiochip_get_data(chip), offset))
> > + return -ENODEV;
> > +
> > + return pinctrl_gpio_request(chip->base + offset);
> pinctrl_gpio_request() takes the chip and the offset value separately.
> It looks like you've developed this patch against an older kernel tree?
I will fix it.
> > +}
> > +
> > +static void aspeed_gpio_g7_free(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + pinctrl_gpio_free(chip->base + offset);
> Same as above for pinctrl_gpio_free().
I will fix it.
> > +}
> > +
> > +static int usecs_to_cycles(struct aspeed_gpio_g7 *gpio, unsigned long usecs, u32 *cycles)
> > +{
> > + u64 rate;
> > + u64 n;
> > + u32 r;
> > +
> > + rate = clk_get_rate(gpio->clk);
> > + if (!rate)
> > + return -EOPNOTSUPP;
> > +
> > + n = rate * usecs;
> > + r = do_div(n, 1000000);
> > +
> > + if (n >= U32_MAX)
> > + return -ERANGE;
> > +
> > + /* At least as long as the requested time */
> > + *cycles = n + (!!r);
> > +
> > + return 0;
> > +}
> > +
> > +/* Call under gpio->lock */
> > +static int register_allocated_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset,
> > + unsigned int timer)
> > +{
> > + if (WARN(gpio->offset_timer[offset] != 0, "Offset %d already allocated timer %d\n", offset,
> > + gpio->offset_timer[offset]))
> > + return -EINVAL;
> > +
> > + if (WARN(gpio->timer_users[timer] == UINT_MAX, "Timer user count would overflow\n"))
> > + return -EPERM;
> > +
> > + gpio->offset_timer[offset] = timer;
> > + gpio->timer_users[timer]++;
> > +
> > + return 0;
> > +}
> > +
> > +/* Call under gpio->lock */
> > +static int unregister_allocated_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> > +{
> > + if (WARN(gpio->offset_timer[offset] == 0, "No timer allocated to offset %d\n", offset))
> > + return -EINVAL;
> > +
> > + if (WARN(gpio->timer_users[gpio->offset_timer[offset]] == 0,
> > + "No users recorded for timer %d\n", gpio->offset_timer[offset]))
> > + return -EINVAL;
> > +
> > + gpio->timer_users[gpio->offset_timer[offset]]--;
> > + gpio->offset_timer[offset] = 0;
> > +
> > + return 0;
> > +}
> > +
> > +/* Call under gpio->lock */
> > +static inline bool timer_allocation_registered(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> > +{
> > + return gpio->offset_timer[offset] > 0;
> > +}
> The above functions have all been copy/pasted, can we make them common?
> > +
> > +static void configure_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset, unsigned int timer)
> > +{
> > + void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > + /* Note: Debounce timer isn't under control of the command
> > + * source registers, so no need to sync with the coprocessor
> > + */
> > + ast_write_bits(addr, GPIO_G7_DEBOUNCE_SEL, timer);
> > +}
> > +
> > +static int enable_debounce(struct gpio_chip *chip, unsigned int offset, unsigned long usecs)
> > +{
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> > + u32 requested_cycles;
> > + unsigned long flags;
> > + int rc;
> > + int i;
> > +
> > + if (!gpio->clk)
> > + return -EINVAL;
> > +
> > + rc = usecs_to_cycles(gpio, usecs, &requested_cycles);
> > + if (rc < 0) {
> > + dev_warn(chip->parent, "Failed to convert %luus to cycles at %luHz: %d\n", usecs,
> > + clk_get_rate(gpio->clk), rc);
> > + return rc;
> > + }
> > +
> > + raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + if (timer_allocation_registered(gpio, offset)) {
> > + rc = unregister_allocated_timer(gpio, offset);
> > + if (rc < 0)
> > + goto out;
> > + }
> > +
> > + /* Try to find a timer already configured for the debounce period */
> > + for (i = 1; i < ARRAY_SIZE(debounce_timers); i++) {
> > + u32 cycles;
> > +
> > + cycles = ioread32(gpio->base + debounce_timers[i]);
> > + if (requested_cycles == cycles)
> > + break;
> > + }
> > +
> > + if (i == ARRAY_SIZE(debounce_timers)) {
> > + int j;
> > +
> > + /*
> > + * As there are no timers configured for the requested debounce
> > + * period, find an unused timer instead
> > + */
> > + for (j = 1; j < ARRAY_SIZE(gpio->timer_users); j++) {
> > + if (gpio->timer_users[j] == 0)
> > + break;
> > + }
> > +
> > + if (j == ARRAY_SIZE(gpio->timer_users)) {
> > + dev_warn(chip->parent,
> > + "Debounce timers exhausted, cannot debounce for period %luus\n",
> > + usecs);
> > +
> > + rc = -EPERM;
> > +
> > + /*
> > + * We already adjusted the accounting to remove @offset
> > + * as a user of its previous timer, so also configure
> > + * the hardware so @offset has timers disabled for
> > + * consistency.
> > + */
> > + configure_timer(gpio, offset, 0);
> > + goto out;
> > + }
> > +
> > + i = j;
> > +
> > + iowrite32(requested_cycles, gpio->base + debounce_timers[i]);
> > + }
> > +
> > + if (WARN(i == 0, "Cannot register index of disabled timer\n")) {
> > + rc = -EINVAL;
> > + goto out;
> > + }
> > +
> > + register_allocated_timer(gpio, offset, i);
> > + configure_timer(gpio, offset, i);
> > +
> > +out:
> > + raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + return rc;
> > +}
> > +
> > +static int disable_debounce(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> > + unsigned long flags;
> > + int rc;
> > +
> > + raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + rc = unregister_allocated_timer(gpio, offset);
> > + if (!rc)
> > + configure_timer(gpio, offset, 0);
> > +
> > + raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + return rc;
> > +}
> > +
> > +static int set_debounce(struct gpio_chip *chip, unsigned int offset, unsigned long usecs)
> > +{
> > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> > +
> > + if (!have_debounce(gpio, offset))
> > + return -EOPNOTSUPP;
> > +
> > + if (usecs)
> > + return enable_debounce(chip, offset, usecs);
> > +
> > + return disable_debounce(chip, offset);
> > +}
> These are all copy/pasted, save for changing to `struct
> aspeed_gpio_g7`. Can we make them common?
> > +
> > +static int aspeed_gpio_g7_set_config(struct gpio_chip *chip, unsigned int offset,
> > + unsigned long config)
> > +{
> > + unsigned long param = pinconf_to_config_param(config);
> > + u32 arg = pinconf_to_config_argument(config);
> > +
> > + if (param == PIN_CONFIG_INPUT_DEBOUNCE)
> > + return set_debounce(chip, offset, arg);
> > + else if (param == PIN_CONFIG_BIAS_DISABLE || param == PIN_CONFIG_BIAS_PULL_DOWN ||
> > + param == PIN_CONFIG_DRIVE_STRENGTH)
> > + return pinctrl_gpio_set_config(offset, config);
> > + else if (param == PIN_CONFIG_DRIVE_OPEN_DRAIN || param == PIN_CONFIG_DRIVE_OPEN_SOURCE)
> > + /* Return -EOPNOTSUPP to trigger emulation, as per datasheet */
> > + return -EOPNOTSUPP;
> > + else if (param == PIN_CONFIG_PERSIST_STATE)
> > + return aspeed_gpio_g7_reset_tolerance(chip, offset, arg);
> > +
> > + return -EOPNOTSUPP;
> > +}
> This is copy/paste, save for the call to
> aspeed_gpio_g7_reset_tolerance(). Can we make it common?
