* [v7 0/2] gpio: aspeed: Add SGPIO driver @ 2019-07-31 20:01 Hongwei Zhang 2019-07-31 20:01 ` [v7 1/2] dt-bindings: gpio: aspeed: Add SGPIO support Hongwei Zhang ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Hongwei Zhang @ 2019-07-31 20:01 UTC (permalink / raw) To: Andrew Jeffery, Linus Walleij, linux-gpio Cc: Hongwei Zhang, Joel Stanley, devicetree, linux-aspeed, Bartosz Golaszewski, Rob Herring, Mark Rutland, linux-kernel, linux-arm-kernel Hello, This short series introduce dt-binding document and a driver for the Aspeed AST2500 SGPIO controller. Please review. [v7]: Changes between v6 and v7: - fix missing variable 'reg' assign issue in aspeed_sgpio_set() - v6 feedback updates [v6]: Changes between v5 and v6: - fix a bug in aspeed_sgpio_dir_out() - v5 feedback updates, some comments cleanup The related SGPM pinmux dt-binding document, dts, and pinctrl driver updates have been accepted and merged: _http://patchwork.ozlabs.org/patch/1110210/ Hongwei Zhang (2): dt-bindings: gpio: aspeed: Add SGPIO support gpio: aspeed: Add SGPIO driver .../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 +++ drivers/gpio/sgpio-aspeed.c | 530 +++++++++++++++++++++ 2 files changed, 585 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt create mode 100644 drivers/gpio/sgpio-aspeed.c -- 2.7.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [v7 1/2] dt-bindings: gpio: aspeed: Add SGPIO support 2019-07-31 20:01 [v7 0/2] gpio: aspeed: Add SGPIO driver Hongwei Zhang @ 2019-07-31 20:01 ` Hongwei Zhang 2019-08-14 7:59 ` Linus Walleij 2019-07-31 20:01 ` [v7 2/2] gpio: aspeed: Add SGPIO driver Hongwei Zhang 2019-08-20 21:35 ` Hongwei Zhang 2 siblings, 1 reply; 8+ messages in thread From: Hongwei Zhang @ 2019-07-31 20:01 UTC (permalink / raw) To: Andrew Jeffery, Linus Walleij, Joel Stanley, devicetree Cc: Hongwei Zhang, Rob Herring, Bartosz Golaszewski, linux-aspeed, linux-kernel, linux-arm-kernel, linux-gpio Add bindings to support SGPIO on AST2400 or AST2500. Signed-off-by: Hongwei Zhang <hongweiz@ami.com> Reviewed-by: Andrew Jeffery <andrew@aj.id.au> --- .../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt new file mode 100644 index 0000000..8545bbc --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt @@ -0,0 +1,55 @@ +Aspeed SGPIO controller Device Tree Bindings +------------------------------------------- + +This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80 full +featured Serial GPIOs. Each of the Serial GPIO pins can be programmed to +support the following options: +- Support interrupt option for each input port and various interrupt + sensitivity option (level-high, level-low, edge-high, edge-low) +- Support reset tolerance option for each output port +- Directly connected to APB bus and its shift clock is from APB bus clock + divided by a programmable value. +- Co-work with external signal-chained TTL components (74LV165/74LV595) + + +Required properties: + +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio" + +- #gpio-cells : Should be two + - First cell is the GPIO line number + - Second cell is used to specify optional + parameters (unused) + +- reg : Address and length of the register set for the device +- gpio-controller : Marks the device node as a GPIO controller +- interrupts : Interrupt specifier (see interrupt bindings for + details) + +- interrupt-controller : Mark the GPIO controller as an interrupt-controller + +- ngpios : number of GPIO pins to serialise. + (should be multiple of 8, up to 80 pins) + +- clocks : A phandle to the APB clock for SGPM clock division + +- bus-frequency : SGPM CLK frequency + + +The sgpio and interrupt properties are further described in their respective bindings documentation: + +- Documentation/devicetree/bindings/gpio/gpio.txt +- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt + + Example: + sgpio: sgpio@1e780200 { + #gpio-cells = <2>; + compatible = "aspeed,ast2500-sgpio"; + gpio-controller; + interrupts = <40>; + reg = <0x1e780200 0x0100>; + clocks = <&syscon ASPEED_CLK_APB>; + interrupt-controller; + ngpios = <8>; + bus-frequency = <12000000>; + }; -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [v7 1/2] dt-bindings: gpio: aspeed: Add SGPIO support 2019-07-31 20:01 ` [v7 1/2] dt-bindings: gpio: aspeed: Add SGPIO support Hongwei Zhang @ 2019-08-14 7:59 ` Linus Walleij 0 siblings, 0 replies; 8+ messages in thread From: Linus Walleij @ 2019-08-14 7:59 UTC (permalink / raw) To: Hongwei Zhang Cc: Andrew Jeffery, Joel Stanley, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rob Herring, Bartosz Golaszewski, linux-aspeed, linux-kernel, Linux ARM, open list:GPIO SUBSYSTEM On Wed, Jul 31, 2019 at 10:01 PM Hongwei Zhang <hongweiz@ami.com> wrote: > Add bindings to support SGPIO on AST2400 or AST2500. > > Signed-off-by: Hongwei Zhang <hongweiz@ami.com> > Reviewed-by: Andrew Jeffery <andrew@aj.id.au> OK timeout for further DT binding review. I adjusted a bunch of things like whitespace and referencing other files when applying. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 8+ messages in thread
* [v7 2/2] gpio: aspeed: Add SGPIO driver 2019-07-31 20:01 [v7 0/2] gpio: aspeed: Add SGPIO driver Hongwei Zhang 2019-07-31 20:01 ` [v7 1/2] dt-bindings: gpio: aspeed: Add SGPIO support Hongwei Zhang @ 2019-07-31 20:01 ` Hongwei Zhang 2019-08-01 2:59 ` Andrew Jeffery ` (2 more replies) 2019-08-20 21:35 ` Hongwei Zhang 2 siblings, 3 replies; 8+ messages in thread From: Hongwei Zhang @ 2019-07-31 20:01 UTC (permalink / raw) To: Andrew Jeffery, Linus Walleij, linux-gpio Cc: Hongwei Zhang, Joel Stanley, linux-aspeed, Bartosz Golaszewski, linux-kernel, linux-arm-kernel Add SGPIO driver support for Aspeed AST2500 SoC. Signed-off-by: Hongwei Zhang <hongweiz@ami.com> Reviewed-by: Andrew Jeffery <andrew@aj.id.au> --- drivers/gpio/sgpio-aspeed.c | 530 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 530 insertions(+) create mode 100644 drivers/gpio/sgpio-aspeed.c diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c new file mode 100644 index 0000000..752efea --- /dev/null +++ b/drivers/gpio/sgpio-aspeed.c @@ -0,0 +1,530 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright 2019 American Megatrends International LLC. + * + * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in> + */ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#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/platform_device.h> +#include <linux/spinlock.h> +#include <linux/string.h> + +#define MAX_NR_SGPIO 80 + +#define ASPEED_SGPIO_CTRL 0x54 + +#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6) +#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16) +#define ASPEED_SGPIO_ENABLE BIT(0) + +struct aspeed_sgpio { + struct gpio_chip chip; + struct clk *pclk; + spinlock_t lock; + void __iomem *base; + uint32_t dir_in[3]; + int irq; +}; + +struct aspeed_sgpio_bank { + uint16_t val_regs; + uint16_t rdata_reg; + uint16_t irq_regs; + const char names[4][3]; +}; + +/* + * Note: The "value" register returns the input value when the GPIO is + * configured as an input. + * + * The "rdata" register returns the output value when the GPIO is + * configured as an output. + */ +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = { + { + .val_regs = 0x0000, + .rdata_reg = 0x0070, + .irq_regs = 0x0004, + .names = { "A", "B", "C", "D" }, + }, + { + .val_regs = 0x001C, + .rdata_reg = 0x0074, + .irq_regs = 0x0020, + .names = { "E", "F", "G", "H" }, + }, + { + .val_regs = 0x0038, + .rdata_reg = 0x0078, + .irq_regs = 0x003C, + .names = { "I", "J" }, + }, +}; + +enum aspeed_sgpio_reg { + reg_val, + reg_rdata, + reg_irq_enable, + reg_irq_type0, + reg_irq_type1, + reg_irq_type2, + reg_irq_status, +}; + +#define GPIO_VAL_VALUE 0x00 +#define GPIO_IRQ_ENABLE 0x00 +#define GPIO_IRQ_TYPE0 0x04 +#define GPIO_IRQ_TYPE1 0x08 +#define GPIO_IRQ_TYPE2 0x0C +#define GPIO_IRQ_STATUS 0x10 + +static void __iomem *bank_reg(struct aspeed_sgpio *gpio, + const struct aspeed_sgpio_bank *bank, + const enum aspeed_sgpio_reg reg) +{ + switch (reg) { + case reg_val: + return gpio->base + bank->val_regs + GPIO_VAL_VALUE; + case reg_rdata: + return gpio->base + bank->rdata_reg; + case reg_irq_enable: + return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE; + case reg_irq_type0: + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0; + case reg_irq_type1: + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1; + case reg_irq_type2: + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2; + case reg_irq_status: + return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS; + default: + /* acturally if code runs to here, it's an error case */ + BUG_ON(1); + } +} + +#define GPIO_BANK(x) ((x) >> 5) +#define GPIO_OFFSET(x) ((x) & 0x1f) +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x)) + +static const struct aspeed_sgpio_bank *to_bank(unsigned int offset) +{ + unsigned int bank = GPIO_BANK(offset); + + WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks)); + return &aspeed_sgpio_banks[bank]; +} + +static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset) +{ + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); + const struct aspeed_sgpio_bank *bank = to_bank(offset); + unsigned long flags; + enum aspeed_sgpio_reg reg; + bool is_input; + int rc = 0; + + spin_lock_irqsave(&gpio->lock, flags); + + is_input = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset); + reg = is_input ? reg_val : reg_rdata; + rc = !!(ioread32(bank_reg(gpio, bank, reg)) & GPIO_BIT(offset)); + + spin_unlock_irqrestore(&gpio->lock, flags); + + return rc; +} + +static void __aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val) +{ + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); + const struct aspeed_sgpio_bank *bank = to_bank(offset); + void __iomem *addr; + u32 reg = 0; + + addr = bank_reg(gpio, bank, reg_val); + reg = ioread32(addr); + + if (val) + reg |= GPIO_BIT(offset); + else + reg &= ~GPIO_BIT(offset); + + iowrite32(reg, addr); +} + +static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val) +{ + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); + unsigned long flags; + + spin_lock_irqsave(&gpio->lock, flags); + + __aspeed_sgpio_set(gc, offset, val); + + spin_unlock_irqrestore(&gpio->lock, flags); +} + +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int offset) +{ + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); + unsigned long flags; + + spin_lock_irqsave(&gpio->lock, flags); + gpio->dir_in[GPIO_BANK(offset)] |= GPIO_BIT(offset); + spin_unlock_irqrestore(&gpio->lock, flags); + + return 0; +} + +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val) +{ + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); + unsigned long flags; + + spin_lock_irqsave(&gpio->lock, flags); + + gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset); + __aspeed_sgpio_set(gc, offset, val); + + spin_unlock_irqrestore(&gpio->lock, flags); + + return 0; +} + +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset) +{ + int dir_status; + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); + unsigned long flags; + + spin_lock_irqsave(&gpio->lock, flags); + dir_status = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset); + spin_unlock_irqrestore(&gpio->lock, flags); + + return dir_status; + +} + +static void irqd_to_aspeed_sgpio_data(struct irq_data *d, + struct aspeed_sgpio **gpio, + const struct aspeed_sgpio_bank **bank, + u32 *bit, int *offset) +{ + struct aspeed_sgpio *internal; + + *offset = irqd_to_hwirq(d); + internal = irq_data_get_irq_chip_data(d); + WARN_ON(!internal); + + *gpio = internal; + *bank = to_bank(*offset); + *bit = GPIO_BIT(*offset); +} + +static void aspeed_sgpio_irq_ack(struct irq_data *d) +{ + const struct aspeed_sgpio_bank *bank; + struct aspeed_sgpio *gpio; + unsigned long flags; + void __iomem *status_addr; + int offset; + u32 bit; + + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset); + + status_addr = bank_reg(gpio, bank, reg_irq_status); + + spin_lock_irqsave(&gpio->lock, flags); + + iowrite32(bit, status_addr); + + spin_unlock_irqrestore(&gpio->lock, flags); +} + +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set) +{ + const struct aspeed_sgpio_bank *bank; + struct aspeed_sgpio *gpio; + unsigned long flags; + u32 reg, bit; + void __iomem *addr; + int offset; + + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset); + addr = bank_reg(gpio, bank, reg_irq_enable); + + spin_lock_irqsave(&gpio->lock, flags); + + reg = ioread32(addr); + if (set) + reg |= bit; + else + reg &= ~bit; + + iowrite32(reg, addr); + + spin_unlock_irqrestore(&gpio->lock, flags); +} + +static void aspeed_sgpio_irq_mask(struct irq_data *d) +{ + aspeed_sgpio_irq_set_mask(d, false); +} + +static void aspeed_sgpio_irq_unmask(struct irq_data *d) +{ + aspeed_sgpio_irq_set_mask(d, true); +} + +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type) +{ + u32 type0 = 0; + u32 type1 = 0; + u32 type2 = 0; + u32 bit, reg; + const struct aspeed_sgpio_bank *bank; + irq_flow_handler_t handler; + struct aspeed_sgpio *gpio; + unsigned long flags; + void __iomem *addr; + int offset; + + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset); + + switch (type & IRQ_TYPE_SENSE_MASK) { + case IRQ_TYPE_EDGE_BOTH: + type2 |= bit; + /* fall through */ + case IRQ_TYPE_EDGE_RISING: + type0 |= bit; + /* fall through */ + case IRQ_TYPE_EDGE_FALLING: + handler = handle_edge_irq; + break; + case IRQ_TYPE_LEVEL_HIGH: + type0 |= bit; + /* fall through */ + case IRQ_TYPE_LEVEL_LOW: + type1 |= bit; + handler = handle_level_irq; + break; + default: + return -EINVAL; + } + + spin_lock_irqsave(&gpio->lock, flags); + + addr = bank_reg(gpio, bank, reg_irq_type0); + reg = ioread32(addr); + reg = (reg & ~bit) | type0; + iowrite32(reg, addr); + + addr = bank_reg(gpio, bank, reg_irq_type1); + reg = ioread32(addr); + reg = (reg & ~bit) | type1; + iowrite32(reg, addr); + + addr = bank_reg(gpio, bank, reg_irq_type2); + reg = ioread32(addr); + reg = (reg & ~bit) | type2; + iowrite32(reg, addr); + + spin_unlock_irqrestore(&gpio->lock, flags); + + irq_set_handler_locked(d, handler); + + return 0; +} + +static void aspeed_sgpio_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); + struct aspeed_sgpio *data = gpiochip_get_data(gc); + unsigned int i, p, girq; + unsigned long reg; + + chained_irq_enter(ic, desc); + + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) { + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i]; + + reg = ioread32(bank_reg(data, bank, reg_irq_status)); + + for_each_set_bit(p, ®, 32) { + girq = irq_find_mapping(gc->irq.domain, i * 32 + p); + generic_handle_irq(girq); + } + + } + + chained_irq_exit(ic, desc); +} + +static struct irq_chip aspeed_sgpio_irqchip = { + .name = "aspeed-sgpio", + .irq_ack = aspeed_sgpio_irq_ack, + .irq_mask = aspeed_sgpio_irq_mask, + .irq_unmask = aspeed_sgpio_irq_unmask, + .irq_set_type = aspeed_sgpio_set_type, +}; + +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio, + struct platform_device *pdev) +{ + int rc, i; + const struct aspeed_sgpio_bank *bank; + + rc = platform_get_irq(pdev, 0); + if (rc < 0) + return rc; + + gpio->irq = rc; + + /* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */ + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) { + bank = &aspeed_sgpio_banks[i]; + /* disable irq enable bits */ + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable)); + /* clear status bits */ + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status)); + } + + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip, + 0, handle_bad_irq, IRQ_TYPE_NONE); + if (rc) { + dev_info(&pdev->dev, "Could not add irqchip\n"); + return rc; + } + + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip, + gpio->irq, aspeed_sgpio_irq_handler); + + /* set IRQ settings and Enable Interrupt */ + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) { + bank = &aspeed_sgpio_banks[i]; + /* set falling or level-low irq */ + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0)); + /* trigger type is edge */ + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1)); + /* dual edge trigger mode. */ + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2)); + /* enable irq */ + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable)); + } + + return 0; +} + +static const struct of_device_id aspeed_sgpio_of_table[] = { + { .compatible = "aspeed,ast2400-sgpio" }, + { .compatible = "aspeed,ast2500-sgpio" }, + {} +}; +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table); + +static int __init aspeed_sgpio_probe(struct platform_device *pdev) +{ + struct aspeed_sgpio *gpio; + u32 nr_gpios, sgpio_freq, sgpio_clk_div; + int rc; + unsigned long apb_freq; + + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); + if (!gpio) + return -ENOMEM; + + gpio->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(gpio->base)) + return PTR_ERR(gpio->base); + + rc = of_property_read_u32(pdev->dev.of_node, "ngpios", &nr_gpios); + if (rc < 0) { + dev_err(&pdev->dev, "Could not read ngpios property\n"); + return -EINVAL; + } else if (nr_gpios > MAX_NR_SGPIO) { + dev_err(&pdev->dev, "Number of GPIOs exceeds the maximum of %d: %d\n", + MAX_NR_SGPIO, nr_gpios); + return -EINVAL; + } + + rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq); + if (rc < 0) { + dev_err(&pdev->dev, "Could not read bus-frequency property\n"); + return -EINVAL; + } + + gpio->pclk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(gpio->pclk)) { + dev_err(&pdev->dev, "devm_clk_get failed\n"); + return PTR_ERR(gpio->pclk); + } + + apb_freq = clk_get_rate(gpio->pclk); + + /* + * From the datasheet, + * SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1) + * period = 2 * (GPIO254[31:16] + 1) / PCLK + * frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK) + * frequency = PCLK / (2 * (GPIO254[31:16] + 1)) + * frequency * 2 * (GPIO254[31:16] + 1) = PCLK + * GPIO254[31:16] = PCLK / (frequency * 2) - 1 + */ + if (sgpio_freq == 0) + return -EINVAL; + + sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1; + + if (sgpio_clk_div > (1 << 16) - 1) + return -EINVAL; + + iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) | + FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) | + ASPEED_SGPIO_ENABLE, + gpio->base + ASPEED_SGPIO_CTRL); + + spin_lock_init(&gpio->lock); + + gpio->chip.parent = &pdev->dev; + gpio->chip.ngpio = nr_gpios; + gpio->chip.direction_input = aspeed_sgpio_dir_in; + gpio->chip.direction_output = aspeed_sgpio_dir_out; + gpio->chip.get_direction = aspeed_sgpio_get_direction; + gpio->chip.request = NULL; + gpio->chip.free = NULL; + gpio->chip.get = aspeed_sgpio_get; + gpio->chip.set = aspeed_sgpio_set; + gpio->chip.set_config = NULL; + gpio->chip.label = dev_name(&pdev->dev); + gpio->chip.base = -1; + + /* set all SGPIO pins as input (1). */ + memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in)); + + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio); + if (rc < 0) + return rc; + + return aspeed_sgpio_setup_irqs(gpio, pdev); +} + +static struct platform_driver aspeed_sgpio_driver = { + .driver = { + .name = KBUILD_MODNAME, + .of_match_table = aspeed_sgpio_of_table, + }, +}; + +module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe); +MODULE_DESCRIPTION("Aspeed Serial GPIO Driver"); +MODULE_LICENSE("GPL"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [v7 2/2] gpio: aspeed: Add SGPIO driver 2019-07-31 20:01 ` [v7 2/2] gpio: aspeed: Add SGPIO driver Hongwei Zhang @ 2019-08-01 2:59 ` Andrew Jeffery 2019-08-14 8:09 ` Linus Walleij 2019-08-21 14:25 ` Hongwei Zhang 2 siblings, 0 replies; 8+ messages in thread From: Andrew Jeffery @ 2019-08-01 2:59 UTC (permalink / raw) To: Hongwei Zhang, Linus Walleij, linux-gpio Cc: Joel Stanley, linux-aspeed, Bartosz Golaszewski, linux-kernel, linux-arm-kernel On Thu, 1 Aug 2019, at 05:32, Hongwei Zhang wrote: > Add SGPIO driver support for Aspeed AST2500 SoC. > > Signed-off-by: Hongwei Zhang <hongweiz@ami.com> > Reviewed-by: Andrew Jeffery <andrew@aj.id.au> Adding my Reviewed-by tag is a bit keen, I only gave it for the bindings on v6. However, having looked over the patch you've addressed all of the issues I've found, so you can keep it. Andrew > --- > drivers/gpio/sgpio-aspeed.c | 530 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 530 insertions(+) > create mode 100644 drivers/gpio/sgpio-aspeed.c > > diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c > new file mode 100644 > index 0000000..752efea > --- /dev/null > +++ b/drivers/gpio/sgpio-aspeed.c > @@ -0,0 +1,530 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright 2019 American Megatrends International LLC. > + * > + * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in> > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#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/platform_device.h> > +#include <linux/spinlock.h> > +#include <linux/string.h> > + > +#define MAX_NR_SGPIO 80 > + > +#define ASPEED_SGPIO_CTRL 0x54 > + > +#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6) > +#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16) > +#define ASPEED_SGPIO_ENABLE BIT(0) > + > +struct aspeed_sgpio { > + struct gpio_chip chip; > + struct clk *pclk; > + spinlock_t lock; > + void __iomem *base; > + uint32_t dir_in[3]; > + int irq; > +}; > + > +struct aspeed_sgpio_bank { > + uint16_t val_regs; > + uint16_t rdata_reg; > + uint16_t irq_regs; > + const char names[4][3]; > +}; > + > +/* > + * Note: The "value" register returns the input value when the GPIO is > + * configured as an input. > + * > + * The "rdata" register returns the output value when the GPIO is > + * configured as an output. > + */ > +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = { > + { > + .val_regs = 0x0000, > + .rdata_reg = 0x0070, > + .irq_regs = 0x0004, > + .names = { "A", "B", "C", "D" }, > + }, > + { > + .val_regs = 0x001C, > + .rdata_reg = 0x0074, > + .irq_regs = 0x0020, > + .names = { "E", "F", "G", "H" }, > + }, > + { > + .val_regs = 0x0038, > + .rdata_reg = 0x0078, > + .irq_regs = 0x003C, > + .names = { "I", "J" }, > + }, > +}; > + > +enum aspeed_sgpio_reg { > + reg_val, > + reg_rdata, > + reg_irq_enable, > + reg_irq_type0, > + reg_irq_type1, > + reg_irq_type2, > + reg_irq_status, > +}; > + > +#define GPIO_VAL_VALUE 0x00 > +#define GPIO_IRQ_ENABLE 0x00 > +#define GPIO_IRQ_TYPE0 0x04 > +#define GPIO_IRQ_TYPE1 0x08 > +#define GPIO_IRQ_TYPE2 0x0C > +#define GPIO_IRQ_STATUS 0x10 > + > +static void __iomem *bank_reg(struct aspeed_sgpio *gpio, > + const struct aspeed_sgpio_bank *bank, > + const enum aspeed_sgpio_reg reg) > +{ > + switch (reg) { > + case reg_val: > + return gpio->base + bank->val_regs + GPIO_VAL_VALUE; > + case reg_rdata: > + return gpio->base + bank->rdata_reg; > + case reg_irq_enable: > + return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE; > + case reg_irq_type0: > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0; > + case reg_irq_type1: > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1; > + case reg_irq_type2: > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2; > + case reg_irq_status: > + return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS; > + default: > + /* acturally if code runs to here, it's an error case */ > + BUG_ON(1); > + } > +} > + > +#define GPIO_BANK(x) ((x) >> 5) > +#define GPIO_OFFSET(x) ((x) & 0x1f) > +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x)) > + > +static const struct aspeed_sgpio_bank *to_bank(unsigned int offset) > +{ > + unsigned int bank = GPIO_BANK(offset); > + > + WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks)); > + return &aspeed_sgpio_banks[bank]; > +} > + > +static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset) > +{ > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); > + const struct aspeed_sgpio_bank *bank = to_bank(offset); > + unsigned long flags; > + enum aspeed_sgpio_reg reg; > + bool is_input; > + int rc = 0; > + > + spin_lock_irqsave(&gpio->lock, flags); > + > + is_input = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset); > + reg = is_input ? reg_val : reg_rdata; > + rc = !!(ioread32(bank_reg(gpio, bank, reg)) & GPIO_BIT(offset)); > + > + spin_unlock_irqrestore(&gpio->lock, flags); > + > + return rc; > +} > + > +static void __aspeed_sgpio_set(struct gpio_chip *gc, unsigned int > offset, int val) > +{ > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); > + const struct aspeed_sgpio_bank *bank = to_bank(offset); > + void __iomem *addr; > + u32 reg = 0; > + > + addr = bank_reg(gpio, bank, reg_val); > + reg = ioread32(addr); > + > + if (val) > + reg |= GPIO_BIT(offset); > + else > + reg &= ~GPIO_BIT(offset); > + > + iowrite32(reg, addr); > +} > + > +static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int > offset, int val) > +{ > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); > + unsigned long flags; > + > + spin_lock_irqsave(&gpio->lock, flags); > + > + __aspeed_sgpio_set(gc, offset, val); > + > + spin_unlock_irqrestore(&gpio->lock, flags); > +} > + > +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int > offset) > +{ > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); > + unsigned long flags; > + > + spin_lock_irqsave(&gpio->lock, flags); > + gpio->dir_in[GPIO_BANK(offset)] |= GPIO_BIT(offset); > + spin_unlock_irqrestore(&gpio->lock, flags); > + > + return 0; > +} > + > +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int > offset, int val) > +{ > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); > + unsigned long flags; > + > + spin_lock_irqsave(&gpio->lock, flags); > + > + gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset); > + __aspeed_sgpio_set(gc, offset, val); > + > + spin_unlock_irqrestore(&gpio->lock, flags); > + > + return 0; > +} > + > +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned > int offset) > +{ > + int dir_status; > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc); > + unsigned long flags; > + > + spin_lock_irqsave(&gpio->lock, flags); > + dir_status = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset); > + spin_unlock_irqrestore(&gpio->lock, flags); > + > + return dir_status; > + > +} > + > +static void irqd_to_aspeed_sgpio_data(struct irq_data *d, > + struct aspeed_sgpio **gpio, > + const struct aspeed_sgpio_bank **bank, > + u32 *bit, int *offset) > +{ > + struct aspeed_sgpio *internal; > + > + *offset = irqd_to_hwirq(d); > + internal = irq_data_get_irq_chip_data(d); > + WARN_ON(!internal); > + > + *gpio = internal; > + *bank = to_bank(*offset); > + *bit = GPIO_BIT(*offset); > +} > + > +static void aspeed_sgpio_irq_ack(struct irq_data *d) > +{ > + const struct aspeed_sgpio_bank *bank; > + struct aspeed_sgpio *gpio; > + unsigned long flags; > + void __iomem *status_addr; > + int offset; > + u32 bit; > + > + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset); > + > + status_addr = bank_reg(gpio, bank, reg_irq_status); > + > + spin_lock_irqsave(&gpio->lock, flags); > + > + iowrite32(bit, status_addr); > + > + spin_unlock_irqrestore(&gpio->lock, flags); > +} > + > +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set) > +{ > + const struct aspeed_sgpio_bank *bank; > + struct aspeed_sgpio *gpio; > + unsigned long flags; > + u32 reg, bit; > + void __iomem *addr; > + int offset; > + > + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset); > + addr = bank_reg(gpio, bank, reg_irq_enable); > + > + spin_lock_irqsave(&gpio->lock, flags); > + > + reg = ioread32(addr); > + if (set) > + reg |= bit; > + else > + reg &= ~bit; > + > + iowrite32(reg, addr); > + > + spin_unlock_irqrestore(&gpio->lock, flags); > +} > + > +static void aspeed_sgpio_irq_mask(struct irq_data *d) > +{ > + aspeed_sgpio_irq_set_mask(d, false); > +} > + > +static void aspeed_sgpio_irq_unmask(struct irq_data *d) > +{ > + aspeed_sgpio_irq_set_mask(d, true); > +} > + > +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type) > +{ > + u32 type0 = 0; > + u32 type1 = 0; > + u32 type2 = 0; > + u32 bit, reg; > + const struct aspeed_sgpio_bank *bank; > + irq_flow_handler_t handler; > + struct aspeed_sgpio *gpio; > + unsigned long flags; > + void __iomem *addr; > + int offset; > + > + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset); > + > + switch (type & IRQ_TYPE_SENSE_MASK) { > + case IRQ_TYPE_EDGE_BOTH: > + type2 |= bit; > + /* fall through */ > + case IRQ_TYPE_EDGE_RISING: > + type0 |= bit; > + /* fall through */ > + case IRQ_TYPE_EDGE_FALLING: > + handler = handle_edge_irq; > + break; > + case IRQ_TYPE_LEVEL_HIGH: > + type0 |= bit; > + /* fall through */ > + case IRQ_TYPE_LEVEL_LOW: > + type1 |= bit; > + handler = handle_level_irq; > + break; > + default: > + return -EINVAL; > + } > + > + spin_lock_irqsave(&gpio->lock, flags); > + > + addr = bank_reg(gpio, bank, reg_irq_type0); > + reg = ioread32(addr); > + reg = (reg & ~bit) | type0; > + iowrite32(reg, addr); > + > + addr = bank_reg(gpio, bank, reg_irq_type1); > + reg = ioread32(addr); > + reg = (reg & ~bit) | type1; > + iowrite32(reg, addr); > + > + addr = bank_reg(gpio, bank, reg_irq_type2); > + reg = ioread32(addr); > + reg = (reg & ~bit) | type2; > + iowrite32(reg, addr); > + > + spin_unlock_irqrestore(&gpio->lock, flags); > + > + irq_set_handler_locked(d, handler); > + > + return 0; > +} > + > +static void aspeed_sgpio_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); > + struct aspeed_sgpio *data = gpiochip_get_data(gc); > + unsigned int i, p, girq; > + unsigned long reg; > + > + chained_irq_enter(ic, desc); > + > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) { > + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i]; > + > + reg = ioread32(bank_reg(data, bank, reg_irq_status)); > + > + for_each_set_bit(p, ®, 32) { > + girq = irq_find_mapping(gc->irq.domain, i * 32 + p); > + generic_handle_irq(girq); > + } > + > + } > + > + chained_irq_exit(ic, desc); > +} > + > +static struct irq_chip aspeed_sgpio_irqchip = { > + .name = "aspeed-sgpio", > + .irq_ack = aspeed_sgpio_irq_ack, > + .irq_mask = aspeed_sgpio_irq_mask, > + .irq_unmask = aspeed_sgpio_irq_unmask, > + .irq_set_type = aspeed_sgpio_set_type, > +}; > + > +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio, > + struct platform_device *pdev) > +{ > + int rc, i; > + const struct aspeed_sgpio_bank *bank; > + > + rc = platform_get_irq(pdev, 0); > + if (rc < 0) > + return rc; > + > + gpio->irq = rc; > + > + /* Disable IRQ and clear Interrupt status registers for all SPGIO > Pins. */ > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) { > + bank = &aspeed_sgpio_banks[i]; > + /* disable irq enable bits */ > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable)); > + /* clear status bits */ > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status)); > + } > + > + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip, > + 0, handle_bad_irq, IRQ_TYPE_NONE); > + if (rc) { > + dev_info(&pdev->dev, "Could not add irqchip\n"); > + return rc; > + } > + > + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip, > + gpio->irq, aspeed_sgpio_irq_handler); > + > + /* set IRQ settings and Enable Interrupt */ > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) { > + bank = &aspeed_sgpio_banks[i]; > + /* set falling or level-low irq */ > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0)); > + /* trigger type is edge */ > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1)); > + /* dual edge trigger mode. */ > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2)); > + /* enable irq */ > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable)); > + } > + > + return 0; > +} > + > +static const struct of_device_id aspeed_sgpio_of_table[] = { > + { .compatible = "aspeed,ast2400-sgpio" }, > + { .compatible = "aspeed,ast2500-sgpio" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table); > + > +static int __init aspeed_sgpio_probe(struct platform_device *pdev) > +{ > + struct aspeed_sgpio *gpio; > + u32 nr_gpios, sgpio_freq, sgpio_clk_div; > + int rc; > + unsigned long apb_freq; > + > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); > + if (!gpio) > + return -ENOMEM; > + > + gpio->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(gpio->base)) > + return PTR_ERR(gpio->base); > + > + rc = of_property_read_u32(pdev->dev.of_node, "ngpios", &nr_gpios); > + if (rc < 0) { > + dev_err(&pdev->dev, "Could not read ngpios property\n"); > + return -EINVAL; > + } else if (nr_gpios > MAX_NR_SGPIO) { > + dev_err(&pdev->dev, "Number of GPIOs exceeds the maximum of %d: > %d\n", > + MAX_NR_SGPIO, nr_gpios); > + return -EINVAL; > + } > + > + rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", > &sgpio_freq); > + if (rc < 0) { > + dev_err(&pdev->dev, "Could not read bus-frequency property\n"); > + return -EINVAL; > + } > + > + gpio->pclk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(gpio->pclk)) { > + dev_err(&pdev->dev, "devm_clk_get failed\n"); > + return PTR_ERR(gpio->pclk); > + } > + > + apb_freq = clk_get_rate(gpio->pclk); > + > + /* > + * From the datasheet, > + * SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1) > + * period = 2 * (GPIO254[31:16] + 1) / PCLK > + * frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK) > + * frequency = PCLK / (2 * (GPIO254[31:16] + 1)) > + * frequency * 2 * (GPIO254[31:16] + 1) = PCLK > + * GPIO254[31:16] = PCLK / (frequency * 2) - 1 > + */ > + if (sgpio_freq == 0) > + return -EINVAL; > + > + sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1; > + > + if (sgpio_clk_div > (1 << 16) - 1) > + return -EINVAL; > + > + iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) | > + FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) | > + ASPEED_SGPIO_ENABLE, > + gpio->base + ASPEED_SGPIO_CTRL); > + > + spin_lock_init(&gpio->lock); > + > + gpio->chip.parent = &pdev->dev; > + gpio->chip.ngpio = nr_gpios; > + gpio->chip.direction_input = aspeed_sgpio_dir_in; > + gpio->chip.direction_output = aspeed_sgpio_dir_out; > + gpio->chip.get_direction = aspeed_sgpio_get_direction; > + gpio->chip.request = NULL; > + gpio->chip.free = NULL; > + gpio->chip.get = aspeed_sgpio_get; > + gpio->chip.set = aspeed_sgpio_set; > + gpio->chip.set_config = NULL; > + gpio->chip.label = dev_name(&pdev->dev); > + gpio->chip.base = -1; > + > + /* set all SGPIO pins as input (1). */ > + memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in)); > + > + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio); > + if (rc < 0) > + return rc; > + > + return aspeed_sgpio_setup_irqs(gpio, pdev); > +} > + > +static struct platform_driver aspeed_sgpio_driver = { > + .driver = { > + .name = KBUILD_MODNAME, > + .of_match_table = aspeed_sgpio_of_table, > + }, > +}; > + > +module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe); > +MODULE_DESCRIPTION("Aspeed Serial GPIO Driver"); > +MODULE_LICENSE("GPL"); > -- > 2.7.4 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [v7 2/2] gpio: aspeed: Add SGPIO driver 2019-07-31 20:01 ` [v7 2/2] gpio: aspeed: Add SGPIO driver Hongwei Zhang 2019-08-01 2:59 ` Andrew Jeffery @ 2019-08-14 8:09 ` Linus Walleij 2019-08-21 14:25 ` Hongwei Zhang 2 siblings, 0 replies; 8+ messages in thread From: Linus Walleij @ 2019-08-14 8:09 UTC (permalink / raw) To: Hongwei Zhang Cc: Andrew Jeffery, open list:GPIO SUBSYSTEM, Joel Stanley, linux-aspeed, Bartosz Golaszewski, linux-kernel, Linux ARM Hi Hongwei, thanks for your patch! I have now merged the bindings so you only need to respin this patch. On Wed, Jul 31, 2019 at 10:02 PM Hongwei Zhang <hongweiz@ami.com> wrote: > Add SGPIO driver support for Aspeed AST2500 SoC. > > Signed-off-by: Hongwei Zhang <hongweiz@ami.com> > Reviewed-by: Andrew Jeffery <andrew@aj.id.au> I guess I need to go with this, there are some minor things I still want to be fixed: > +static void __aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val) I don't like __underscore_functions because their semantic is ambiguous. Rename this something like aspeed_sgpio_commit() or whatever best fits the actual use. > +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio, > + struct platform_device *pdev) > +{ (...) > + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip, > + 0, handle_bad_irq, IRQ_TYPE_NONE); (...) > + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip, > + gpio->irq, aspeed_sgpio_irq_handler); We do not set up chained irqchips like this anymore, sorry. I am currently rewriting all existing chained drivers to pass an initialized irqchip when registering the whole gpio chip. See drivers/gpio/TODO. Here are examples: https://lore.kernel.org/linux-gpio/20190811080539.15647-1-linus.walleij@linaro.org/ https://lore.kernel.org/linux-gpio/20190812132554.18313-1-linus.walleij@linaro.org/ > + /* set all SGPIO pins as input (1). */ > + memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in)); Do the irqchip set-up here, before adding the gpio_chip. > + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio); > + if (rc < 0) > + return rc; > + > + return aspeed_sgpio_setup_irqs(gpio, pdev); Yours, Linus Walleij ^ permalink raw reply [flat|nested] 8+ messages in thread
* [v7 2/2] gpio: aspeed: Add SGPIO driver 2019-07-31 20:01 ` [v7 2/2] gpio: aspeed: Add SGPIO driver Hongwei Zhang 2019-08-01 2:59 ` Andrew Jeffery 2019-08-14 8:09 ` Linus Walleij @ 2019-08-21 14:25 ` Hongwei Zhang 2 siblings, 0 replies; 8+ messages in thread From: Hongwei Zhang @ 2019-08-21 14:25 UTC (permalink / raw) To: Andrew Jeffery, Linus Walleij, linux-gpio Cc: Hongwei Zhang, Joel Stanley, linux-aspeed, Bartosz Golaszewski, linux-kernel, linux-arm-kernel Hello Linus, Thanks for your review! I just submitted v8 to the list, please help to review it again. Since you have already merged the dt-binding document [v7 1/2], and I don't have your update to this file, so to avoid confusion, I only include the driver code in v8. Regards, --Hongwei P.S. sorry previous response used wrong In-Reply-To ID, so resent it again. > From: Linus Walleij <linus.walleij@linaro.org> > Sent: Wednesday, August 14, 2019 4:09 AM > To: Hongwei Zhang > Cc: Andrew Jeffery; open list:GPIO SUBSYSTEM; Joel Stanley; linux-aspeed; Bartosz Golaszewski; > linux-kernel@vger.kernel.org; Linux ARM > Subject: Re: [v7 2/2] gpio: aspeed: Add SGPIO driver > > Hi Hongwei, > > thanks for your patch! > > I have now merged the bindings so you only need to respin this patch. > > On Wed, Jul 31, 2019 at 10:02 PM Hongwei Zhang <hongweiz@ami.com> wrote: > > > Add SGPIO driver support for Aspeed AST2500 SoC. > > > > Signed-off-by: Hongwei Zhang <hongweiz@ami.com> > > Reviewed-by: Andrew Jeffery <andrew@aj.id.au> > > I guess I need to go with this, there are some minor things I still want to be fixed: > > > +static void __aspeed_sgpio_set(struct gpio_chip *gc, unsigned int > > +offset, int val) > > I don't like __underscore_functions because their semantic is ambiguous. > done, please see v8. > Rename this something like aspeed_sgpio_commit() or whatever best fits the actual use. > > > +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio, > > + struct platform_device *pdev) { > (...) > > + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip, > > + 0, handle_bad_irq, IRQ_TYPE_NONE); > (...) > > + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip, > > + gpio->irq, > > + aspeed_sgpio_irq_handler); > > We do not set up chained irqchips like this anymore, sorry. > > I am currently rewriting all existing chained drivers to pass an initialized irqchip when registering the > whole gpio chip. > See drivers/gpio/TODO. > > Here are examples: > https://lore.kernel.org/linux-gpio/20190811080539.15647-1-linus.walleij@linaro.org/ > https://lore.kernel.org/linux-gpio/20190812132554.18313-1-linus.walleij@linaro.org/ > done, please see v8. > > + /* set all SGPIO pins as input (1). */ > > + memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in)); > > Do the irqchip set-up here, before adding the gpio_chip. > > > + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio); > > + if (rc < 0) > > + return rc; > > + > > + return aspeed_sgpio_setup_irqs(gpio, pdev); > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 8+ messages in thread
* [v7 2/2] gpio: aspeed: Add SGPIO driver 2019-07-31 20:01 [v7 0/2] gpio: aspeed: Add SGPIO driver Hongwei Zhang 2019-07-31 20:01 ` [v7 1/2] dt-bindings: gpio: aspeed: Add SGPIO support Hongwei Zhang 2019-07-31 20:01 ` [v7 2/2] gpio: aspeed: Add SGPIO driver Hongwei Zhang @ 2019-08-20 21:35 ` Hongwei Zhang 2 siblings, 0 replies; 8+ messages in thread From: Hongwei Zhang @ 2019-08-20 21:35 UTC (permalink / raw) To: Andrew Jeffery, Linus Walleij, linux-gpio Cc: Hongwei Zhang, Joel Stanley, linux-aspeed, Bartosz Golaszewski, linux-kernel, linux-arm-kernel Hello Linus, Thanks for your review! I just submitted v8 to the list, please help to review it again. Since you have already merged the dt-binding document [v7 1/2], and I don't have your update to this file, so to avoid confusion, I only include the driver code in v8. Regards, --Hongwei > From: Linus Walleij <linus.walleij@linaro.org> > Sent: Wednesday, August 14, 2019 4:09 AM > To: Hongwei Zhang > Cc: Andrew Jeffery; open list:GPIO SUBSYSTEM; Joel Stanley; linux-aspeed; Bartosz Golaszewski; > linux-kernel@vger.kernel.org; Linux ARM > Subject: Re: [v7 2/2] gpio: aspeed: Add SGPIO driver > > Hi Hongwei, > > thanks for your patch! > > I have now merged the bindings so you only need to respin this patch. > > On Wed, Jul 31, 2019 at 10:02 PM Hongwei Zhang <hongweiz@ami.com> wrote: > > > Add SGPIO driver support for Aspeed AST2500 SoC. > > > > Signed-off-by: Hongwei Zhang <hongweiz@ami.com> > > Reviewed-by: Andrew Jeffery <andrew@aj.id.au> > > I guess I need to go with this, there are some minor things I still want to be fixed: > > > +static void __aspeed_sgpio_set(struct gpio_chip *gc, unsigned int > > +offset, int val) > > I don't like __underscore_functions because their semantic is ambiguous. > done, please see v8. > Rename this something like aspeed_sgpio_commit() or whatever best fits the actual use. > > > +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio, > > + struct platform_device *pdev) { > (...) > > + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip, > > + 0, handle_bad_irq, IRQ_TYPE_NONE); > (...) > > + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip, > > + gpio->irq, > > + aspeed_sgpio_irq_handler); > > We do not set up chained irqchips like this anymore, sorry. > > I am currently rewriting all existing chained drivers to pass an initialized irqchip when registering the > whole gpio chip. > See drivers/gpio/TODO. > > Here are examples: > https://lore.kernel.org/linux-gpio/20190811080539.15647-1-linus.walleij@linaro.org/ > https://lore.kernel.org/linux-gpio/20190812132554.18313-1-linus.walleij@linaro.org/ > done, please see v8. > > + /* set all SGPIO pins as input (1). */ > > + memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in)); > > Do the irqchip set-up here, before adding the gpio_chip. > > > + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio); > > + if (rc < 0) > > + return rc; > > + > > + return aspeed_sgpio_setup_irqs(gpio, pdev); > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-08-21 14:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-31 20:01 [v7 0/2] gpio: aspeed: Add SGPIO driver Hongwei Zhang 2019-07-31 20:01 ` [v7 1/2] dt-bindings: gpio: aspeed: Add SGPIO support Hongwei Zhang 2019-08-14 7:59 ` Linus Walleij 2019-07-31 20:01 ` [v7 2/2] gpio: aspeed: Add SGPIO driver Hongwei Zhang 2019-08-01 2:59 ` Andrew Jeffery 2019-08-14 8:09 ` Linus Walleij 2019-08-21 14:25 ` Hongwei Zhang 2019-08-20 21:35 ` Hongwei Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).