| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 20 Nov 2013 15:47:13 -0600
From: delicious quinoa <delicious.quinoa@...il.com>
To: Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Cc: linux-kernel <linux-kernel@...r.kernel.org>,
linux-gpio@...r.kernel.org,
Linus Walleij <linus.walleij@...ricsson.com>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Grant Likely <grant.likely@...retlab.ca>,
Rob Herring <rob.herring@...xeda.com>,
Steffen Trumtrar <s.trumtrar@...gutronix.de>,
Jamie Iles <jamie@...ieiles.com>,
Heiko Stuebner <heiko@...ech.de>, Alan Tull <atull@...era.com>,
Dinh Nguyen <dinguyen@...era.com>,
Yves Vandervennet <rocket.yvanderv@...il.com>
Subject: Re: [PATCH 1/1] gpio: add a driver for the Synopsys DesignWare APB
GPIO block
On Thu, Nov 7, 2013 at 6:33 AM, Sebastian Hesselbarth
<sebastian.hesselbarth@...il.com> wrote:
> On 11/06/13 23:49, Alan Tull wrote:
>>
>> From: Jamie Iles <jamie@...ieiles.com>
>>
>> The Synopsys DesignWare block is used in some ARM devices (picoxcell)
>> and can be configured to provide multiple banks of GPIO pins.
>>
>> Signed-off-by: Alan Tull <atull@...era.com>
>>
>> v6: - (atull) squash the set of patches
>
>
> Much better to review, thanks.
>
> [...]
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
>> b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
>> new file mode 100644
>> index 0000000..f7e2076
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
>> @@ -0,0 +1,57 @@
>> +* Synopsys DesignWare APB GPIO controller
>> +
>> +Required properties:
>> +- compatible : Should be "snps,dw-apb-gpio"
>> +- reg : Address and length of the register set for the device
>> +
>> +The GPIO controller has a configurable number of banks, each of which are
>> +represented as child nodes with the following properties:
>
>
Hi Sebastian,
I'm about to send out v7. Briefly:
I was able to use irq_chip_generic and support multiple interrupts.
I wasn't able to get rid of the 'struct dwapb_gpio_port'.
I incorporated all the other.cleanup suggestions.
I tested on my hardware which has one interrupt line from each
ip's block. Would appreciate someone testing on hardware that
has one interrupt line per gpio line. I still added the linear domain,
but went back to some of Jamie's original code for irq chip generic.
Still have the child nodes in the device tree.
Alan
> still, s/bank/port/g
>
>> +Required properties:
>> +- compatible : "snps,dw-apb-gpio-bank"
>
>
> should be "snps,dw-apb-gpio-port" then. If someone badly needs
> "snps,dw-apb-gpio-bank", I suggest to mark it as DEPRECATED.
>
>> +- gpio-controller : Marks the device node as a gpio controller.
>> +- #gpio-cells : Should be two. The first cell is the pin number and
>> + the second cell is used to specify optional parameters (currently
>> + unused).
>> +- reg : The integer bank index of the bank, a single cell.
>> +- nr-gpios : The number of pins in the bank, a single cell.
>
>
> Missed that one, must have vendor-specific prefix, i.e. snps,nr-gpios.
> Also, if you leave this property as required, there is no way for those
> IP with CONFIG registers to not use it. Please, make it optional.
I have made it optional, defaulting to 32 if the binding isn't present.
It appears that the CONFIG_ registers don't work the way the spec says
they should on my hardware. So I would like to leave reading these to
be a minor patch for someone to do in the future.
>
>> +Optional properties:
>> +- interrupt-controller : The first bank may be configured to be an
>> interrupt
>> +controller.
>
>
> s/bank/port/
Got this one and the others.
>
>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>> +interrupt. Shall be set to 2. The first cell defines the interrupt
>> number,
>> +the second encodes the triger flags encoded as described in
>> +Documentation/devicetree/bindings/interrupts.txt
>> +- interrupt-parent : The parent interrupt controller.
>> +- interrupts : The interrupts to the parent controller raised when GPIOs
>> +generate the interrupts.
>
>
> Right, here you correctly say that there may be more than just one
> upstream irq.
>
>> +Example:
>> +
>> +gpio: gpio@...00 {
>> + compatible = "snps,dw-apb-gpio";
>> + reg = <0x20000 0x1000>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + banka: gpio-controller@0 {
>> + compatible = "snps,dw-apb-gpio-bank";
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + nr-gpio = <8>;
>> + reg = <0>;
>> + interrupt-controller;
>> + #interrupt-cells = <2>;
>> + interrupt-parent = <&vic1>;
>> + interrupts = <0 1 2 3 4 5 6 7>;
>> + };
>> +
>> + bankb: gpio-controller@1 {
>> + compatible = "snps,dw-apb-gpio-bank";
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + nr-gpio = <8>;
>> + reg = <1>;
>> + };
>
>
> s/bank/port/
>
>
> BTW, what if we get rid of port child nodes completely and rather
> use:
>
> gpio: gpio-controller@...00 {
> compatible = "snps,dw-apb-gpio";
> reg = <0x20000 0x1000>;
> gpio-controller;
> #gpio-cells = <2>;
> interrupt-controller;
> interrupt-parent = <&vic1>;
> interrupts = <0 1 2 3 4 5 6 7>;
> snps,port-widths = <8 8 0 0>;
> };
>
> The only draw-back compared to child-nodes is, that you'll reference
> gpios with <&gpio 13> instead of <&banka 5>. I have no strong opinion
> about it, so I leave the correct answer to either LinusW or DT
> maintainers.
I left this as-is for now.
>
> [...]
>>
>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
>> new file mode 100644
>> index 0000000..7957dfd
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-dwapb.c
>> @@ -0,0 +1,458 @@
>> +/*
>> + * Copyright (c) 2011 Jamie Iles
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * All enquiries to support@...ochip.com
>> + */
>> +#include <linux/basic_mmio_gpio.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/ioport.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>> +
>> +#define GPIO_SWPORTA_DR 0x00
>> +#define GPIO_SWPORTA_DDR 0x04
>> +#define GPIO_SWPORTB_DR 0x0c
>> +#define GPIO_SWPORTB_DDR 0x10
>> +#define GPIO_SWPORTC_DR 0x18
>> +#define GPIO_SWPORTC_DDR 0x1c
>> +#define GPIO_SWPORTD_DR 0x24
>> +#define GPIO_SWPORTD_DDR 0x28
>> +#define GPIO_INTEN 0x30
>> +#define GPIO_INTMASK 0x34
>> +#define GPIO_INTTYPE_LEVEL 0x38
>> +#define GPIO_INT_POLARITY 0x3c
>> +#define GPIO_INTSTATUS 0x40
>> +#define GPIO_PORTA_EOI 0x4c
>> +#define GPIO_EXT_PORTA 0x50
>> +#define GPIO_EXT_PORTB 0x54
>> +#define GPIO_EXT_PORTC 0x58
>> +#define GPIO_EXT_PORTD 0x5c
>> +
>> +struct dwapb_gpio;
>> +
>> +struct dwapb_gpio_port {
>> + struct bgpio_chip bgc;
>> + bool is_registered;
>
>
> if you make *ports an array of 4 below, you can get rid
> of that awkward is_registered. On top of that, with only
> struct bgpio_chip and the pointer back to dwapb_gpio you
> can get rid of struct dwapb_gpio_port completely.
>
>> + struct dwapb_gpio *gpio;
>> +};
>> +
>> +struct dwapb_gpio {
>> + struct device *dev;
>> + void __iomem *regs;
>> + struct dwapb_gpio_port *ports;
>
>
> #define DWAPB_MAX_PORTS 4
>
> struct bgpio_chip port_bgc[DWAPB_MAXPORTS];
Have a catch-22 here:
If port_bgc isn't an aray of pointers, then I need 'is_registered'.
On the other hand, if it is an array of pointers, then that makes
dwapb_gpio_to_irq difficult because it is using container_of. So
I may be stuck with 'struct dwapb_gpio_port'
>
>> + unsigned int nr_ports;
>> + struct irq_domain *domain;
>> + int hwirq;
>
>
> As said earlier, hwirq is actually virq. As you'll possibly have
> to request more than one upstream irq and irqdomain API requires you
> to dispose the mapping before removing, I suggest to remove any
> upstream reference from the above struct and use irq_find_mapping()
> get the virq to dispose (example below).
>
Got this implemented in v7.
>> +};
>> +
>> +static unsigned int dwapb_gpio_nr_ports(struct device_node *of_node)
>> +{
>> + unsigned int nr_ports = 0;
>> + struct device_node *np;
>> +
>> + for_each_child_of_node(of_node, np)
>> + ++nr_ports;
>> +
>> + return nr_ports;
>> +}
>> +
>> +static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
>> +{
>> + struct bgpio_chip *bgc = to_bgpio_chip(gc);
>> + struct dwapb_gpio_port *port = container_of(bgc, struct
>> + dwapb_gpio_port, bgc);
>> + struct dwapb_gpio *gpio = port->gpio;
>> +
>> + return irq_create_mapping(gpio->domain, offset);
>> +}
>> +
>> +static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int
>> offs)
>> +{
>> + u32 v = readl(gpio->regs + GPIO_INT_POLARITY);
>> +
>> + if (gpio_get_value(gpio->ports[0].bgc.gc.base + offs))
>> + v &= ~BIT(offs);
>> + else
>> + v |= BIT(offs);
>> +
>> + writel(v, gpio->regs + GPIO_INT_POLARITY);
>> +}
>> +
>> +static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
>> +{
>> + struct dwapb_gpio *gpio = irq_get_handler_data(irq);
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + u32 irq_status = readl(gpio->regs + GPIO_INTSTATUS);
>> +
>> + while (irq_status) {
>> + int irqoffset = fls(irq_status) - 1;
>> + int gpio_irq = irq_linear_revmap(gpio->domain, irqoffset);
>> + u32 irq_type = irq_get_trigger_type(gpio_irq);
>> +
>> + generic_handle_irq(gpio_irq);
>> + irq_status &= ~(1 << irqoffset);
>
>
> BIT(irqoffset) as above?
Got this and all others like it. I hope.
>> +
>> + if ((irq_type & IRQ_TYPE_SENSE_MASK) ==
>> IRQ_TYPE_EDGE_BOTH)
>> + dwapb_toggle_trigger(gpio, irqoffset);
>> + }
>
>
> If you use irq_chip_generic, the whole handler can be written as:
>
> static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
> {
> struct irq_domain *d = irq_get_handler_data(irq);
> struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, irq);
I think irq_get_domain_generic_chip needs hwirq here.
> u32 irqstatus = readl_relaxed(gc->reg_base + GPIO_INTSTATUS) &
> gc->mask_cache;
I got the readl_relaxed.
>
> while (irqstatus) {
> u32 hwirq = fls(irqstatus) - 1;
> generic_handle_irq(irq_find_mapping(d, gc->irq_base +
> hwirq));
> irqstatus &= ~BIT(hwirq);
>
> if ((irq_get_trigger_type(irq) & IRQ_TYPE_SENSE_MASK)
> == IRQ_TYPE_EDGE_BOTH)
> dwapb_toggle_trigger(gpio, hwirq);
> }
> }
>
>> +
>> + if (chip->irq_eoi)
>> + chip->irq_eoi(irq_desc_get_irq_data(desc));
>> +}
>> +
>> +static void dwapb_irq_enable(struct irq_data *d)
>> +{
>> + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
>> + struct bgpio_chip *bgc = &gpio->ports[0].bgc;
>> + unsigned long flags;
>> + u32 val;
>> +
>> + spin_lock_irqsave(&bgc->lock, flags);
>> + val = readl(gpio->regs + GPIO_INTEN);
>> + val |= 1 << d->hwirq;
>
>
> BIT(d->hwirq) ?
>
>> + writel(val, gpio->regs + GPIO_INTEN);
>> + spin_unlock_irqrestore(&bgc->lock, flags);
>> +}
>> +
>> +static void dwapb_irq_disable(struct irq_data *d)
>> +{
>> + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
>> + struct bgpio_chip *bgc = &gpio->ports[0].bgc;
>> + unsigned long flags;
>> + u32 val;
>> +
>> + spin_lock_irqsave(&bgc->lock, flags);
>> + val = readl(gpio->regs + GPIO_INTEN);
>> + val &= ~(1 << d->hwirq);
>
>
> ditto.
>
>> + writel(val, gpio->regs + GPIO_INTEN);
>> + spin_unlock_irqrestore(&bgc->lock, flags);
>> +}
>> +
>> +static int dwapb_irq_set_type(struct irq_data *d, u32 type)
>> +{
>> + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
>> + struct bgpio_chip *bgc = &gpio->ports[0].bgc;
>> + int bit = d->hwirq;
>> + unsigned long level, polarity, flags;
>> +
>> + if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
>> + IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
>> + return -EINVAL;
>> +
>> + spin_lock_irqsave(&bgc->lock, flags);
>> + level = readl(gpio->regs + GPIO_INTTYPE_LEVEL);
>> + polarity = readl(gpio->regs + GPIO_INT_POLARITY);
>> +
>> + switch (type) {
>> + case IRQ_TYPE_EDGE_BOTH:
>> + level |= (1 << bit);
>> + dwapb_toggle_trigger(gpio, bit);
>> + break;
>> + case IRQ_TYPE_EDGE_RISING:
>> + level |= (1 << bit);
>> + polarity |= (1 << bit);
>> + break;
>> + case IRQ_TYPE_EDGE_FALLING:
>> + level |= (1 << bit);
>> + polarity &= ~(1 << bit);
>> + break;
>> + case IRQ_TYPE_LEVEL_HIGH:
>> + level &= ~(1 << bit);
>> + polarity |= (1 << bit);
>> + break;
>> + case IRQ_TYPE_LEVEL_LOW:
>> + level &= ~(1 << bit);
>> + polarity &= ~(1 << bit);
>
>
> ditto for all above and below.
Yep
>
>> + break;
>> + }
>> +
>> + writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
>> + writel(polarity, gpio->regs + GPIO_INT_POLARITY);
>> + spin_unlock_irqrestore(&bgc->lock, flags);
>> +
>> + return 0;
>> +}
>> +
>> +static void dwapb_irq_ack(struct irq_data *d)
>> +{
>> + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
>> + struct bgpio_chip *bgc = &gpio->ports[0].bgc;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&bgc->lock, flags);
>> + writel(1 << d->hwirq, gpio->regs + GPIO_PORTA_EOI);
>> + spin_unlock_irqrestore(&bgc->lock, flags);
>> +}
>> +
>> +static void dwapb_irq_mask(struct irq_data *d)
>> +{
>> + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
>> + struct bgpio_chip *bgc = &gpio->ports[0].bgc;
>> + unsigned long value, flags;
>> +
>> + spin_lock_irqsave(&bgc->lock, flags);
>> + value = readl(gpio->regs + GPIO_INTMASK);
>> + value |= 1 << d->hwirq;
>> + writel(value, gpio->regs + GPIO_INTMASK);
>> + spin_unlock_irqrestore(&bgc->lock, flags);
>> +}
>> +
>> +static void dwapb_irq_unmask(struct irq_data *d)
>> +{
>> + struct dwapb_gpio *gpio = irq_data_get_irq_chip_data(d);
>> + struct bgpio_chip *bgc = &gpio->ports[0].bgc;
>> + unsigned long value, flags;
>> +
>> + spin_lock_irqsave(&bgc->lock, flags);
>> + value = readl(gpio->regs + GPIO_INTMASK);
>> + value &= ~(1 << d->hwirq);
>> + writel(value, gpio->regs + GPIO_INTMASK);
>> + spin_unlock_irqrestore(&bgc->lock, flags);
>> +}
>> +
>> +static struct irq_chip dwapb_irq_chip = {
>> + .name = "gpio-dwapb",
>> + .irq_ack = dwapb_irq_ack,
>> + .irq_mask = dwapb_irq_mask,
>> + .irq_unmask = dwapb_irq_unmask,
>> + .irq_set_type = dwapb_irq_set_type,
>> + .irq_enable = dwapb_irq_enable,
>> + .irq_disable = dwapb_irq_disable,
>> +};
>
>
> If you use irq_chip_generic, that should allow you to get rid of
> ack/mask/unmask callbacks above. If you need an example, look at
> drivers/irqchip/irq-orion.c.
Good, got it.
>
>> +static int dwapb_gpio_irq_map(struct irq_domain *domain,
>> + unsigned int irq,
>> + irq_hw_number_t hwirq)
>> +{
>> + struct dwapb_gpio *gpio = domain->host_data;
>> +
>> + irq_set_chip_data(irq, gpio);
>> + irq_set_chip_and_handler(irq, &dwapb_irq_chip, handle_level_irq);
>> + irq_set_irq_type(irq, IRQ_TYPE_NONE);
>> + irq_set_handler_data(irq, gpio);
>> +
>> + return 0;
>> +}
>> +
>> +static struct irq_domain_ops dwapb_gpio_irq_ops = {
>> + .map = dwapb_gpio_irq_map,
>> + .xlate = irq_domain_xlate_twocell,
>> +};
>> +
>> +static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>> + struct dwapb_gpio_port *port)
>> +{
>> + struct gpio_chip *gc = &port->bgc.gc;
>> + struct device_node *node = gc->of_node;
>> + unsigned int ngpio = gc->ngpio;
>> + int reg;
>> +
>> + if (of_get_property(node, "interrupts", ®) == NULL)
>> + return;
>> +
>> + gpio->hwirq = irq_of_parse_and_map(node, 0);
>
>
> Loop over all "interrupts" passed and register them to
> dwapb_irq_handler, as said before, there can be more than
> one upstream irq. And better use devm_request_irq, all interrupts
> have been converted to resources by platform bus already.
v7 adds looping over the interrupts.
devm_request_irq and irq_chip_generic don't seem to be compatible with
chained handler types which aren't irq_handler_t.
>
>> + if (gpio->hwirq == NO_IRQ)
>> + return;
>> +
>> + gpio->domain = irq_domain_add_linear(node, ngpio,
>> &dwapb_gpio_irq_ops,
>> + gpio);
>> + if (!gpio->domain) {
>> + irq_dispose_mapping(gpio->hwirq);
>> + return;
>> + }
>> +
>> + irq_set_handler_data(gpio->hwirq, gpio);
>> + irq_set_chained_handler(gpio->hwirq, dwapb_irq_handler);
>> + port->bgc.gc.to_irq = dwapb_gpio_to_irq;
>> +}
>> +
>> +static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>> + struct device_node *port_np,
>> + unsigned int offs)
>> +{
>> + struct dwapb_gpio_port *port;
>> + u32 port_idx, ngpio;
>> + void __iomem *dat, *set, *dirout;
>> + int err;
>> +
>> + if (of_property_read_u32(port_np, "reg", &port_idx)) {
>> + dev_err(gpio->dev, "missing port index for %s\n",
>> + port_np->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + if (port_idx > 3) {
>> + dev_err(gpio->dev, "invalid port index for %s\n",
>> + port_np->full_name);
>> + return -EINVAL;
>> + }
>
>
> join the two above to
>
> if (of_property_read_u32(port_np, "reg", &port_idx) ||
> port_idx >= DWAPB_MAX_PORTS) {
> dev_err(.., "missing/invalid port index ...", ...)
> return -EINVAL;
> }
Got it.
>
>> + port = &gpio->ports[offs];
>
>
> if you devm_kzalloc each gpio_chip locally here and assign it
> after successful gpiochip_add, you have your is_registered
> back implicitly.
>
>> + port->gpio = gpio;
>> +
>> + if (of_property_read_u32(port_np, "nr-gpios", &ngpio)) {
>> + dev_err(gpio->dev, "failed to get number of gpios for
>> %s\n",
>> + port_np->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + dat = gpio->regs + GPIO_EXT_PORTA +
>> + (port_idx * (GPIO_EXT_PORTB - GPIO_EXT_PORTA));
>
>
> #define GPIO_EXT_PORT_SIZE 0xc
OK, I will create 3 macros since the SIZE isn' the same for all three.
>
> dat = gpio->reg + GPIO_EXT_PORTA + (port_idx * GPIO_EXT_PORT_SIZE);
>
>> + set = gpio->regs + GPIO_SWPORTA_DR +
>> + (port_idx * (GPIO_SWPORTB_DR - GPIO_SWPORTA_DR));
>
>
> ditto.
>
>> + dirout = gpio->regs + GPIO_SWPORTA_DDR +
>> + (port_idx * (GPIO_SWPORTB_DDR - GPIO_SWPORTA_DDR));
>
>
> ditto.
>
>> + err = bgpio_init(&port->bgc, gpio->dev, 4, dat, set, NULL, dirout,
>> + NULL, false);
>> + if (err) {
>> + dev_err(gpio->dev, "failed to init gpio chip for %s\n",
>> + port_np->full_name);
>> + return err;
>> + }
>> +
>> + port->bgc.gc.ngpio = ngpio;
>> + port->bgc.gc.of_node = port_np;
>> +
>> + /*
>> + * Only port A can provide interrupts in all configurations of the
>> IP.
>> + */
>> + if (port_idx == 0 &&
>> + of_get_property(port_np, "interrupt-controller", NULL))
>
>
> of_property_read_bool(port_np, "interrupt-controller")
>
>> + dwapb_configure_irqs(gpio, port);
>> +
>> + err = gpiochip_add(&port->bgc.gc);
>> + if (err)
>> + dev_err(gpio->dev, "failed to register gpiochip for %s\n",
>> + port_np->full_name);
>> + else
>> + port->is_registered = true;
>> +
>> + return err;
>> +}
>> +
>> +static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
>> +{
>> + unsigned int m;
>> +
>> + for (m = 0; m < gpio->nr_ports; ++m)
>> + if (gpio->ports[m].is_registered)
>> + WARN_ON(gpiochip_remove(&gpio->ports[m].bgc.gc));
>
>
> if (gpio->port_bgc[m])
> ...
>
>> +}
>> +
>> +static int dwapb_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res;
>> + struct dwapb_gpio *gpio;
>> + struct device_node *np;
>> + int err;
>> + unsigned int offs = 0;
>> +
>> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
>> + if (!gpio)
>> + return -ENOMEM;
>> + gpio->dev = &pdev->dev;
>> +
>> + gpio->nr_ports = dwapb_gpio_nr_ports(pdev->dev.of_node);
>> + if (!gpio->nr_ports) {
>> + err = -EINVAL;
>> + goto out_err;
>> + }
>> + gpio->ports = devm_kzalloc(&pdev->dev, gpio->nr_ports *
>> + sizeof(*gpio->ports), GFP_KERNEL);
>> + if (!gpio->ports) {
>> + err = -ENOMEM;
>> + goto out_err;
>> + }
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + gpio->regs = devm_ioremap_resource(&pdev->dev, res);
>> + if (!gpio->regs) {
>> + err = -ENOMEM;
>
>
> if (!gpio->regs)
> return PTR_ERR(gpio->regs);
>
> Jamie already mentioned it.
Cool, got it.
>
>> + goto out_free_ports;
>> + }
>> +
>> + for_each_child_of_node(pdev->dev.of_node, np) {
>> + err = dwapb_gpio_add_port(gpio, np, offs++);
>> + if (err)
>> + goto out_unregister;
>> + }
>> + platform_set_drvdata(pdev, gpio);
>> +
>> + return 0;
>> +
>> +out_unregister:
>> + dwapb_gpio_unregister(gpio);
>> +
>> + if (gpio->domain) {
>> + irq_dispose_mapping(gpio->hwirq);
>> + irq_domain_remove(gpio->domain);
>
>
> pseudo-code:
>
> for hwirq in #gpios-porta loop
> irq_dispose_mapping(
> irq_find_mapping(domain,gc->irq_base + hwirq))
> end loop
> irq_domain_remove(domain)
>
>> + }
>> +
>> +out_free_ports:
>> + devm_kfree(&pdev->dev, gpio->ports);
>> +
>> +out_err:
>> + devm_kfree(&pdev->dev, gpio);
>
>
> Still, remove devm_kfree above.
>
>> + return err;
>> +}
>> +
>> +static int dwapb_gpio_remove(struct platform_device *pdev)
>> +{
>> + struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
>> +
>> + dwapb_gpio_unregister(gpio);
>> + if (gpio->domain) {
>> + irq_dispose_mapping(gpio->hwirq);
>> + irq_domain_remove(gpio->domain);
>> + }
>> + devm_kfree(&pdev->dev, gpio->ports);
>> + devm_kfree(&pdev->dev, gpio);
>
>
> ditto.
>
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id dwapb_of_match[] = {
>> + { .compatible = "snps,dw-apb-gpio" },
>> + { /* Sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, dwapb_of_match);
>> +
>> +static struct platform_driver dwapb_gpio_driver = {
>> + .driver = {
>> + .name = "gpio-dwapb",
>> + .owner = THIS_MODULE,
>> + .of_match_table = of_match_ptr(dwapb_of_match),
>> + },
>> + .probe = dwapb_gpio_probe,
>> + .remove = dwapb_gpio_remove,
>> +};
>> +
>> +static int __init dwapb_gpio_init(void)
>> +{
>> + return platform_driver_register(&dwapb_gpio_driver);
>> +}
>> +postcore_initcall(dwapb_gpio_init);
>> +
>> +static void __exit dwapb_gpio_exit(void)
>> +{
>> + platform_driver_unregister(&dwapb_gpio_driver);
>> +}
>> +module_exit(dwapb_gpio_exit);
>
>
> Jamie already mentioned to use module_platform_driver(), of course
> this will remove the postcore_initcall and move gpio init to some
> later point. I know GPIO is important, but nevertheless I suggest
> to remove it. Deferred probing will sort out initialization order.
>
> Sebastian
>
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Jamie Iles");
>> +MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver");
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists