| Message ID | 1409161588-19417-2-git-send-email-alvin.chen@intel.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Aug 27, 2014 at 7:46 PM, Weike Chen <alvin.chen@intel.com> wrote: > The Synopsys DesignWare APB GPIO driver only supports open firmware devices. > But, like Intel Quark X1000 SOC, which has a single PCI function exporting > a GPIO and an I2C controller, it is a Multifunction device. This patch is > to enable the current Synopsys DesignWare APB GPIO driver to support the > Multifunction device which exports the designware GPIO controller. > > Reviewed-by: Hock Leong Kweh <hock.leong.kweh@intel.com> > Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@intel.com> > Signed-off-by: Weike Chen <alvin.chen@intel.com> Sebastian, Jamie: can either of you take a look at this patch? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014-08-27 10:46:26 [-0700], Weike Chen wrote: > index 9de1515..8250a44 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -136,7 +136,6 @@ config GPIO_DWAPB > tristate "Synopsys DesignWare APB GPIO driver" > select GPIO_GENERIC > select GENERIC_IRQ_CHIP > - depends on OF_GPIO you need either OF_GPIO or PCI > help > Say Y or M here to build support for the Synopsys DesignWare APB > GPIO block. > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index d6618a6..155a64b 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -21,6 +21,7 @@ > #include <linux/of_irq.h> > #include <linux/platform_device.h> > #include <linux/spinlock.h> > +#include <linux/platform_data/gpio-dwapb.h> > > #define GPIO_SWPORTA_DR 0x00 > #define GPIO_SWPORTA_DDR 0x04 > @@ -52,14 +53,15 @@ struct dwapb_gpio_port { > struct bgpio_chip bgc; > bool is_registered; > struct dwapb_gpio *gpio; > + struct dwapb_gpio_port_property *pp; > }; > > struct dwapb_gpio { > struct device *dev; > void __iomem *regs; > struct dwapb_gpio_port *ports; > - unsigned int nr_ports; you could keep this the way it is > struct irq_domain *domain; > + const struct dwapb_gpio_platform_data *pdata; and not making this a member of this struct. I've been going up and down the source and I don't see the need to marry dwapb_gpio to dwapb_gpio_platform_data. That dwapb_gpio_port_property thing has a long name. Could you just set it up, pass it for registration and the free it? You can't free the pdata for the non-OF tree but for the OF case you keep that struct around for no reason. You could safe some memory and use pdata directly for setup. > }; > > static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset) > @@ -207,22 +209,24 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type) > return 0; > } > > +static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) > +{ > + struct irq_desc *desc = irq_to_desc(irq); > + > + dwapb_irq_handler(irq, desc); > + > + return IRQ_HANDLED; I suggest to teach dwapb_irq_handler() to return something that makes you decide whether or not IRQ_HANDLED is the thing to do here. If something goes crazy the core has no way of knowing and shutting you down. Also invoking ->irq_eoi() _before_ your shared was invoked might not smart. What you want is something like static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio, struct irq_chip *chip) { u32 irq_status = readl_relaxed(gpio->regs + GPIO_INTSTATUS); u32 ret = irq_status; while (irq_status) { int hwirq = fls(irq_status) - 1; int gpio_irq = irq_find_mapping(gpio->domain, hwirq); generic_handle_irq(gpio_irq); irq_status &= ~BIT(hwirq); if ((irq_get_trigger_type(gpio_irq) & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH) dwapb_toggle_trigger(gpio, hwirq); } return ret; } 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); _dwapb_irq_handler(gpio, chip); if (chip->irq_eoi) chip->irq_eoi(irq_desc_get_irq_data(desc)); } static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) { struct irq_desc *desc = irq_to_desc(irq); struct irq_chip *chip = irq_desc_get_chip(desc); int worked; worked = dwapb_irq_handler(dev_id, chip); if (worked) return IRQ_HANDLED; else return IRQ_NONE; } How about it? > + > 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; > - struct irq_chip_generic *irq_gc; > + struct device_node *node = port->pp->node; > + struct irq_chip_generic *irq_gc = NULL; > unsigned int hwirq, ngpio = gc->ngpio; > struct irq_chip_type *ct; > - int err, irq, i; > - > - irq = irq_of_parse_and_map(node, 0); > - if (!irq) { > - dev_warn(gpio->dev, "no irq for bank %s\n", > - port->bgc.gc.of_node->full_name); > - return; > - } > + int err, i; > > gpio->domain = irq_domain_add_linear(node, ngpio, > &irq_generic_chip_ops, gpio); > @@ -269,8 +273,24 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH; > irq_gc->chip_types[1].handler = handle_edge_irq; > > - irq_set_chained_handler(irq, dwapb_irq_handler); > - irq_set_handler_data(irq, gpio); > + if (!port->pp->irq_shared) { > + irq_set_chained_handler(port->pp->irq, dwapb_irq_handler); > + } else { > + /* > + * Request a shared IRQ since where MFD would have devices > + * using the same irq pin > + */ > + err = devm_request_irq(gpio->dev, port->pp->irq, > + dwapb_irq_handler_mfd, > + IRQF_SHARED, "gpio-dwapb-mfd", gpio); > + if (err) { > + dev_err(gpio->dev, "error requesting IRQ\n"); > + irq_domain_remove(gpio->domain); > + gpio->domain = NULL; > + return; > + } > + } > + irq_set_handler_data(port->pp->irq, gpio); This does not look like it belongs here. It should only be used together with irq_set_chained_handler() or am I missing here something? > > for (hwirq = 0 ; hwirq < ngpio ; hwirq++) > irq_create_mapping(gpio->domain, hwirq); > @@ -296,57 +316,43 @@ static void dwapb_irq_teardown(struct dwapb_gpio *gpio) > } > > static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > - struct device_node *port_np, > + struct dwapb_gpio_port_property *pp, > 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) || > - port_idx >= DWAPB_MAX_PORTS) { > - dev_err(gpio->dev, "missing/invalid port index for %s\n", > - port_np->full_name); > - return -EINVAL; > - } > - > port = &gpio->ports[offs]; > port->gpio = gpio; > + port->pp = pp; > > - if (of_property_read_u32(port_np, "snps,nr-gpios", &ngpio)) { > - dev_info(gpio->dev, "failed to get number of gpios for %s\n", > - port_np->full_name); > - ngpio = 32; > - } > - > - dat = gpio->regs + GPIO_EXT_PORTA + (port_idx * GPIO_EXT_PORT_SIZE); > - set = gpio->regs + GPIO_SWPORTA_DR + (port_idx * GPIO_SWPORT_DR_SIZE); > + dat = gpio->regs + GPIO_EXT_PORTA + (pp->idx * GPIO_EXT_PORT_SIZE); > + set = gpio->regs + GPIO_SWPORTA_DR + (pp->idx * GPIO_SWPORT_DR_SIZE); > dirout = gpio->regs + GPIO_SWPORTA_DDR + > - (port_idx * GPIO_SWPORT_DDR_SIZE); > + (pp->idx * GPIO_SWPORT_DDR_SIZE); > > 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); > + pp->name); > return err; > } > > - port->bgc.gc.ngpio = ngpio; > - port->bgc.gc.of_node = port_np; > + port->bgc.gc.ngpio = pp->ngpio; > + port->bgc.gc.base = pp->gpio_base; > > /* > * Only port A can provide interrupts in all configurations of the IP. > */ So we had a port_idx check which was refered by the comment. Now we have a comment and no check for it. You could do that loop over that array here and keep that check. > - if (port_idx == 0 && > - of_property_read_bool(port_np, "interrupt-controller")) > + if (pp->irq) > 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); > + pp->name); > else > port->is_registered = true; > > @@ -357,30 +363,115 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) > { > unsigned int m; > > - for (m = 0; m < gpio->nr_ports; ++m) > + for (m = 0; m < gpio->pdata->nports; ++m) > if (gpio->ports[m].is_registered) > gpiochip_remove(&gpio->ports[m].bgc.gc); > } > > +/* > + * Handlers for alternative sources of platform_data > + */ Those abvious comments… > + > +#ifdef CONFIG_OF_GPIO > +/* > + * Translate OpenFirmware node properties into platform_data > + */ … are not really helping > +static struct dwapb_gpio_platform_data * Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > --- a/drivers/gpio/Kconfig > > +++ b/drivers/gpio/Kconfig > > @@ -136,7 +136,6 @@ config GPIO_DWAPB > > tristate "Synopsys DesignWare APB GPIO driver" > > select GPIO_GENERIC > > select GENERIC_IRQ_CHIP > > - depends on OF_GPIO > you need either OF_GPIO or PCI Since we enable this module not only support OF devices, but also support MFD devices, so we remove OF_GPIO dependency. For 'PCI', the original code is also not depended on PCI, and this patch also not, do you think it is necessary? > > > > struct dwapb_gpio { > > struct device *dev; > > void __iomem *regs; > > struct dwapb_gpio_port *ports; > > - unsigned int nr_ports; > you could keep this the way it is It has been moved to 'pdata'. > > struct irq_domain *domain; > > + const struct dwapb_gpio_platform_data *pdata; > > and not making this a member of this struct. I've been going up and down the > source and I don't see the need to marry dwapb_gpio to > dwapb_gpio_platform_data. > That dwapb_gpio_port_property thing has a long name. Could you just set it up, > pass it for registration and the free it? You can't free the pdata for the non-OF > tree but for the OF case you keep that struct around for no reason. > You could safe some memory and use pdata directly for setup. Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from MFD. For OF, 'pdata' is getting from device nodes properties. Why do we have such design? Because it can make the handling more easy for flowing routine. All necessary properties get from 'pdata', never care it is from OF or MFD. And someone are working on replacing OF interface with a firmware agnostic device_property* interface which will work with both OF and ACPI. More information for this design, please contact Darren Hart <dvhart@linux.intel.com>. Darren Hart wrote to me: "Generally speaking, rather than if/else blocks throughout the code checking if it is enumerated via open firmware or as a platform device, a cleaner approach is to check if the pdata is null, and if so, populate the pdata from the open firmware description if present. Then use the pdata throughout the driver. Something to keep in mind is that we are working to replace the of_* interface with a firmware agnostic device_property* interface which will work with both OF and ACPI. Patches to hit LKML this week for the acpi and driver core. Organizing the code as described above will better encapsulate the firmware-config logic and make it easier to update." And you can also refer the code 'driver/input/keyboard/gpio_keys.c', and this patch takes it as an example. > > }; > > > > static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset) > > @@ -207,22 +209,24 @@ static int dwapb_irq_set_type(struct irq_data *d, > u32 type) > > return 0; > > } > > > > +static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) { > > + struct irq_desc *desc = irq_to_desc(irq); > > + > > + dwapb_irq_handler(irq, desc); > > + > > + return IRQ_HANDLED; > > I suggest to teach dwapb_irq_handler() to return something that makes you > decide whether or not IRQ_HANDLED is the thing to do here. If something goes > crazy the core has no way of knowing and shutting you down. > Also invoking ->irq_eoi() _before_ your shared was invoked might not smart. > What you want is something like > > static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio, struct irq_chip > *chip) { > u32 irq_status = readl_relaxed(gpio->regs + GPIO_INTSTATUS); > u32 ret = irq_status; > > while (irq_status) { > int hwirq = fls(irq_status) - 1; > int gpio_irq = irq_find_mapping(gpio->domain, hwirq); > > generic_handle_irq(gpio_irq); > irq_status &= ~BIT(hwirq); > > if ((irq_get_trigger_type(gpio_irq) & > IRQ_TYPE_SENSE_MASK) > == IRQ_TYPE_EDGE_BOTH) > dwapb_toggle_trigger(gpio, hwirq); > } > return ret; > } > > 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); > > _dwapb_irq_handler(gpio, chip); > > if (chip->irq_eoi) > chip->irq_eoi(irq_desc_get_irq_data(desc)); > } > > static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) { > struct irq_desc *desc = irq_to_desc(irq); > struct irq_chip *chip = irq_desc_get_chip(desc); > int worked; > > worked = dwapb_irq_handler(dev_id, chip); > if (worked) > return IRQ_HANDLED; > else > return IRQ_NONE; > } > > How about it? OK, I will improve it as your suggestion. > > + irq_set_handler_data(port->pp->irq, gpio); > > This does not look like it belongs here. It should only be used together with > irq_set_chained_handler() or am I missing here something? This patch reused ' dwapb_irq_handler', it needs the irq handler data. For ' irq_set_handler_data', it just sets the irq data. Why do you think it must be used together with ' irq_set_chained_handler'? > > /* > > * Only port A can provide interrupts in all configurations of the IP. > > */ > So we had a port_idx check which was refered by the comment. Now we have a > comment and no check for it. You could do that loop over that array here and > keep that check. Let me move it to 'dwapb_gpio_get_pdata_of'. > > > > +/* > > + * Handlers for alternative sources of platform_data */ > > Those abvious comments… > > > + > > +#ifdef CONFIG_OF_GPIO > > +/* > > + * Translate OpenFirmware node properties into platform_data */ > … are not really helping Let me remove them. > > +static struct dwapb_gpio_platform_data * > > Sebastian
On 2014-09-04 02:00:03 [+0000], Chen, Alvin wrote: > > > > > --- a/drivers/gpio/Kconfig > > > +++ b/drivers/gpio/Kconfig > > > @@ -136,7 +136,6 @@ config GPIO_DWAPB > > > tristate "Synopsys DesignWare APB GPIO driver" > > > select GPIO_GENERIC > > > select GENERIC_IRQ_CHIP > > > - depends on OF_GPIO > > you need either OF_GPIO or PCI > Since we enable this module not only support OF devices, but also support MFD devices, so we remove OF_GPIO dependency. > For 'PCI', the original code is also not depended on PCI, and this patch also not, do you think it is necessary? if not PCI then you should add something likwe "depends on OF_GPIO || MFD" looking further, you need also HAS_IOMEM for things like devm_ioremap_resource(). Linus, wouldn't it make sense to group this driver and make the block depend on it? > > > struct dwapb_gpio { > > > struct device *dev; > > > void __iomem *regs; > > > struct dwapb_gpio_port *ports; > > > - unsigned int nr_ports; > > you could keep this the way it is > It has been moved to 'pdata'. I saw that. But there is no need keep a pointer to pdata across the whole driver since only need nr_ports in the driver removal part of the whole driver. > > > struct irq_domain *domain; > > > + const struct dwapb_gpio_platform_data *pdata; > > > > and not making this a member of this struct. I've been going up and down the > > source and I don't see the need to marry dwapb_gpio to > > dwapb_gpio_platform_data. > > That dwapb_gpio_port_property thing has a long name. Could you just set it up, > > pass it for registration and the free it? You can't free the pdata for the non-OF > > tree but for the OF case you keep that struct around for no reason. > > You could safe some memory and use pdata directly for setup. > Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from MFD. > For OF, 'pdata' is getting from device nodes properties. Why do we have such design? Because it can make the handling > more easy for flowing routine. All necessary properties get from 'pdata', never care it is from OF or MFD. And someone > are working on replacing OF interface with a firmware agnostic device_property* interface which will work with both OF and ACPI. > More information for this design, please contact Darren Hart <dvhart@linux.intel.com>. Darren Hart wrote to me: > "Generally speaking, rather than if/else blocks throughout the code checking if it is enumerated via open firmware or as a platform device, > a cleaner approach is to check if the pdata is null, and if so, populate the pdata from the open firmware description if present. > Then use the pdata throughout the driver. But isn't it enough to hold on to this pdata thing through the probe function only? After probe is done you could drop that memory in the OF-case, right? > > > + irq_set_handler_data(port->pp->irq, gpio); > > > > This does not look like it belongs here. It should only be used together with > > irq_set_chained_handler() or am I missing here something? > This patch reused ' dwapb_irq_handler', it needs the irq handler data. For ' irq_set_handler_data', it just sets the irq data. > Why do you think it must be used together with ' irq_set_chained_handler'? because you do request_irq(…, driver_data). If you you look close to irq_set_chained_handler() it does not have such a member. Thus it uses irq_set_handler_data() for that same purpose. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Since we enable this module not only support OF devices, but also support > MFD devices, so we remove OF_GPIO dependenc > > For 'PCI', the original code is also not depended on PCI, and this patch also > not, do you think it is necessary? > > if not PCI then you should add something likwe > "depends on OF_GPIO || MFD" > > looking further, you need also HAS_IOMEM for things like > devm_ioremap_resource(). Linus, wouldn't it make sense to group this driver > and make the block depend on it? > I can add "depends on OF_GPIO || INTEL_QUARK_I2C_GPIO_MFD", since now the MFD path only support Intel Quark. Andriy, how do you think? > > > > struct dwapb_gpio { > > > > struct device *dev; > > > > void __iomem *regs; > > > > struct dwapb_gpio_port *ports; > > > > - unsigned int nr_ports; > > > you could keep this the way it is > > It has been moved to 'pdata'. > > I saw that. But there is no need keep a pointer to pdata across the whole driver > since only need nr_ports in the driver removal part of the whole driver. Got your idea. So can I just use a global static pdata pointer instead of adding it as a member of ' struct dwapb_gpio'? Since pdata is used in all 'probe' functions, and make pdata as global static make programming more easy. > > > > > struct irq_domain *domain; > > > > + const struct dwapb_gpio_platform_data *pdata; > > > > > > and not making this a member of this struct. I've been going up and > > > down the source and I don't see the need to marry dwapb_gpio to > > > dwapb_gpio_platform_data. > > > That dwapb_gpio_port_property thing has a long name. Could you just > > > set it up, pass it for registration and the free it? You can't free > > > the pdata for the non-OF tree but for the OF case you keep that struct > around for no reason. > > > You could safe some memory and use pdata directly for setup. > > Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from > MFD. > > For OF, 'pdata' is getting from device nodes properties. Why do we > > have such design? Because it can make the handling more easy for > > flowing routine. All necessary properties get from 'pdata', never care it is > from OF or MFD. And someone are working on replacing OF interface with a > firmware agnostic device_property* interface which will work with both OF and > ACPI. > > More information for this design, please contact Darren Hart > <dvhart@linux.intel.com>. Darren Hart wrote to me: > > "Generally speaking, rather than if/else blocks throughout the code > > checking if it is enumerated via open firmware or as a platform device, a > cleaner approach is to check if the pdata is null, and if so, populate the pdata > from the open firmware description if present. > > Then use the pdata throughout the driver. > > But isn't it enough to hold on to this pdata thing through the probe function > only? After probe is done you could drop that memory in the OF-case, right? OK. If it is OF-case, pdata can be freed in the end of probe. > > > > + irq_set_handler_data(port->pp->irq, gpio); > > > > > > This does not look like it belongs here. It should only be used > > > together with > > > irq_set_chained_handler() or am I missing here something? > > This patch reused ' dwapb_irq_handler', it needs the irq handler data. For ' > irq_set_handler_data', it just sets the irq data. > > Why do you think it must be used together with ' irq_set_chained_handler'? > > because you do request_irq(…, driver_data). If you you look close to > irq_set_chained_handler() it does not have such a member. Thus it uses > irq_set_handler_data() for that same purpose. > OK. I can improve it.
On Thu, 2014-09-04 at 10:38 +0000, Chen, Alvin wrote: > > > > Since we enable this module not only support OF devices, but also support > > MFD devices, so we remove OF_GPIO dependenc > > > For 'PCI', the original code is also not depended on PCI, and this patch also > > not, do you think it is necessary? > > > > if not PCI then you should add something likwe > > "depends on OF_GPIO || MFD" > > > > looking further, you need also HAS_IOMEM for things like > > devm_ioremap_resource(). Linus, wouldn't it make sense to group this driver > > and make the block depend on it? > > > I can add "depends on OF_GPIO || INTEL_QUARK_I2C_GPIO_MFD", since now the MFD path only support Intel Quark. > > Andriy, how do you think? I think we don't need that dependency at all since OF_GPIO has stubs for non-OF case. Am I missing something? GPIOLIB dependency is implied in Kconfig, by the way. P.S. See, for example, leds-gpio.c > > > > > > struct dwapb_gpio { > > > > > struct device *dev; > > > > > void __iomem *regs; > > > > > struct dwapb_gpio_port *ports; > > > > > - unsigned int nr_ports; > > > > you could keep this the way it is > > > It has been moved to 'pdata'. > > > > I saw that. But there is no need keep a pointer to pdata across the whole driver > > since only need nr_ports in the driver removal part of the whole driver. > Got your idea. So can I just use a global static pdata pointer instead of adding it as a member of ' struct dwapb_gpio'? > Since pdata is used in all 'probe' functions, and make pdata as global static make programming more easy. > > > > > > > > struct irq_domain *domain; > > > > > + const struct dwapb_gpio_platform_data *pdata; > > > > > > > > and not making this a member of this struct. I've been going up and > > > > down the source and I don't see the need to marry dwapb_gpio to > > > > dwapb_gpio_platform_data. > > > > That dwapb_gpio_port_property thing has a long name. Could you just > > > > set it up, pass it for registration and the free it? You can't free > > > > the pdata for the non-OF tree but for the OF case you keep that struct > > around for no reason. > > > > You could safe some memory and use pdata directly for setup. > > > Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from > > MFD. > > > For OF, 'pdata' is getting from device nodes properties. Why do we > > > have such design? Because it can make the handling more easy for > > > flowing routine. All necessary properties get from 'pdata', never care it is > > from OF or MFD. And someone are working on replacing OF interface with a > > firmware agnostic device_property* interface which will work with both OF and > > ACPI. > > > More information for this design, please contact Darren Hart > > <dvhart@linux.intel.com>. Darren Hart wrote to me: > > > "Generally speaking, rather than if/else blocks throughout the code > > > checking if it is enumerated via open firmware or as a platform device, a > > cleaner approach is to check if the pdata is null, and if so, populate the pdata > > from the open firmware description if present. > > > Then use the pdata throughout the driver. > > > > But isn't it enough to hold on to this pdata thing through the probe function > > only? After probe is done you could drop that memory in the OF-case, right? > OK. If it is OF-case, pdata can be freed in the end of probe. > > > > > > > + irq_set_handler_data(port->pp->irq, gpio); > > > > > > > > This does not look like it belongs here. It should only be used > > > > together with > > > > irq_set_chained_handler() or am I missing here something? > > > This patch reused ' dwapb_irq_handler', it needs the irq handler data. For ' > > irq_set_handler_data', it just sets the irq data. > > > Why do you think it must be used together with ' irq_set_chained_handler'? > > > > because you do request_irq(…, driver_data). If you you look close to > > irq_set_chained_handler() it does not have such a member. Thus it uses > > irq_set_handler_data() for that same purpose. > > > OK. I can improve it. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 9de1515..8250a44 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -136,7 +136,6 @@ config GPIO_DWAPB tristate "Synopsys DesignWare APB GPIO driver" select GPIO_GENERIC select GENERIC_IRQ_CHIP - depends on OF_GPIO help Say Y or M here to build support for the Synopsys DesignWare APB GPIO block. diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index d6618a6..155a64b 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -21,6 +21,7 @@ #include <linux/of_irq.h> #include <linux/platform_device.h> #include <linux/spinlock.h> +#include <linux/platform_data/gpio-dwapb.h> #define GPIO_SWPORTA_DR 0x00 #define GPIO_SWPORTA_DDR 0x04 @@ -52,14 +53,15 @@ struct dwapb_gpio_port { struct bgpio_chip bgc; bool is_registered; struct dwapb_gpio *gpio; + struct dwapb_gpio_port_property *pp; }; struct dwapb_gpio { struct device *dev; void __iomem *regs; struct dwapb_gpio_port *ports; - unsigned int nr_ports; struct irq_domain *domain; + const struct dwapb_gpio_platform_data *pdata; }; static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset) @@ -207,22 +209,24 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type) return 0; } +static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) +{ + struct irq_desc *desc = irq_to_desc(irq); + + dwapb_irq_handler(irq, desc); + + return IRQ_HANDLED; +} + 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; - struct irq_chip_generic *irq_gc; + struct device_node *node = port->pp->node; + struct irq_chip_generic *irq_gc = NULL; unsigned int hwirq, ngpio = gc->ngpio; struct irq_chip_type *ct; - int err, irq, i; - - irq = irq_of_parse_and_map(node, 0); - if (!irq) { - dev_warn(gpio->dev, "no irq for bank %s\n", - port->bgc.gc.of_node->full_name); - return; - } + int err, i; gpio->domain = irq_domain_add_linear(node, ngpio, &irq_generic_chip_ops, gpio); @@ -269,8 +273,24 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH; irq_gc->chip_types[1].handler = handle_edge_irq; - irq_set_chained_handler(irq, dwapb_irq_handler); - irq_set_handler_data(irq, gpio); + if (!port->pp->irq_shared) { + irq_set_chained_handler(port->pp->irq, dwapb_irq_handler); + } else { + /* + * Request a shared IRQ since where MFD would have devices + * using the same irq pin + */ + err = devm_request_irq(gpio->dev, port->pp->irq, + dwapb_irq_handler_mfd, + IRQF_SHARED, "gpio-dwapb-mfd", gpio); + if (err) { + dev_err(gpio->dev, "error requesting IRQ\n"); + irq_domain_remove(gpio->domain); + gpio->domain = NULL; + return; + } + } + irq_set_handler_data(port->pp->irq, gpio); for (hwirq = 0 ; hwirq < ngpio ; hwirq++) irq_create_mapping(gpio->domain, hwirq); @@ -296,57 +316,43 @@ static void dwapb_irq_teardown(struct dwapb_gpio *gpio) } static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, - struct device_node *port_np, + struct dwapb_gpio_port_property *pp, 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) || - port_idx >= DWAPB_MAX_PORTS) { - dev_err(gpio->dev, "missing/invalid port index for %s\n", - port_np->full_name); - return -EINVAL; - } - port = &gpio->ports[offs]; port->gpio = gpio; + port->pp = pp; - if (of_property_read_u32(port_np, "snps,nr-gpios", &ngpio)) { - dev_info(gpio->dev, "failed to get number of gpios for %s\n", - port_np->full_name); - ngpio = 32; - } - - dat = gpio->regs + GPIO_EXT_PORTA + (port_idx * GPIO_EXT_PORT_SIZE); - set = gpio->regs + GPIO_SWPORTA_DR + (port_idx * GPIO_SWPORT_DR_SIZE); + dat = gpio->regs + GPIO_EXT_PORTA + (pp->idx * GPIO_EXT_PORT_SIZE); + set = gpio->regs + GPIO_SWPORTA_DR + (pp->idx * GPIO_SWPORT_DR_SIZE); dirout = gpio->regs + GPIO_SWPORTA_DDR + - (port_idx * GPIO_SWPORT_DDR_SIZE); + (pp->idx * GPIO_SWPORT_DDR_SIZE); 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); + pp->name); return err; } - port->bgc.gc.ngpio = ngpio; - port->bgc.gc.of_node = port_np; + port->bgc.gc.ngpio = pp->ngpio; + port->bgc.gc.base = pp->gpio_base; /* * Only port A can provide interrupts in all configurations of the IP. */ - if (port_idx == 0 && - of_property_read_bool(port_np, "interrupt-controller")) + if (pp->irq) 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); + pp->name); else port->is_registered = true; @@ -357,30 +363,115 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) { unsigned int m; - for (m = 0; m < gpio->nr_ports; ++m) + for (m = 0; m < gpio->pdata->nports; ++m) if (gpio->ports[m].is_registered) gpiochip_remove(&gpio->ports[m].bgc.gc); } +/* + * Handlers for alternative sources of platform_data + */ + +#ifdef CONFIG_OF_GPIO +/* + * Translate OpenFirmware node properties into platform_data + */ +static struct dwapb_gpio_platform_data * +dwapb_gpio_get_pdata_of(struct device *dev) +{ + struct device_node *node, *port_np; + struct dwapb_gpio_platform_data *pdata; + struct dwapb_gpio_port_property *pp; + int nports; + int i; + + node = dev->of_node; + if (!node) + return ERR_PTR(-ENODEV); + + nports = of_get_child_count(node); + if (nports == 0) + return ERR_PTR(-ENODEV); + + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) + return ERR_PTR(-ENOMEM); + + pdata->properties = devm_kcalloc(dev, nports, sizeof(*pp), GFP_KERNEL); + if (!pdata->properties) + return ERR_PTR(-ENOMEM); + + pdata->nports = nports; + + i = 0; + for_each_child_of_node(node, port_np) { + pp = &pdata->properties[i++]; + pp->node = port_np; + + if (of_property_read_u32(port_np, "reg", &pp->idx) || + pp->idx >= DWAPB_MAX_PORTS) { + dev_err(dev, "missing/invalid port index for %s\n", + port_np->full_name); + return ERR_PTR(-EINVAL); + } + + if (of_property_read_u32(port_np, "snps,nr-gpios", + &pp->ngpio)) { + dev_info(dev, "failed to get number of gpios for %s\n", + port_np->full_name); + pp->ngpio = 32; + } + + if (pp->idx == 0 && + of_property_read_bool(port_np, "interrupt-controller")) { + pp->irq = irq_of_parse_and_map(port_np, 0); + if (!pp->irq) { + dev_warn(dev, "no irq for bank %s\n", + port_np->full_name); + } + } else { + pp->irq = 0; + } + + pp->irq_shared = false; + pp->gpio_base = 0; + pp->name = port_np->full_name; + } + + return pdata; +} +#else +static inline struct dwapb_gpio_platform_data * +dwapb_gpio_get_pdata_of(struct device *dev) +{ + return ERR_PTR(-ENODEV); +} +#endif + static int dwapb_gpio_probe(struct platform_device *pdev) { + int i; struct resource *res; struct dwapb_gpio *gpio; - struct device_node *np; int err; - unsigned int offs = 0; + struct device *dev = &pdev->dev; + const struct dwapb_gpio_platform_data *pdata = dev_get_platdata(dev); + + if (!pdata) { + pdata = dwapb_gpio_get_pdata_of(dev); + if (IS_ERR(pdata)) + return PTR_ERR(pdata); + } + if (!pdata->nports) + return -ENODEV; gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL); if (!gpio) return -ENOMEM; gpio->dev = &pdev->dev; + gpio->pdata = pdata; - gpio->nr_ports = of_get_child_count(pdev->dev.of_node); - if (!gpio->nr_ports) { - err = -EINVAL; - goto out_err; - } - gpio->ports = devm_kzalloc(&pdev->dev, gpio->nr_ports * + gpio->ports = devm_kcalloc(&pdev->dev, pdata->nports, sizeof(*gpio->ports), GFP_KERNEL); if (!gpio->ports) { err = -ENOMEM; @@ -394,8 +485,8 @@ static int dwapb_gpio_probe(struct platform_device *pdev) goto out_err; } - for_each_child_of_node(pdev->dev.of_node, np) { - err = dwapb_gpio_add_port(gpio, np, offs++); + for (i = 0; i < pdata->nports; i++) { + err = dwapb_gpio_add_port(gpio, &pdata->properties[i], i); if (err) goto out_unregister; } diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h new file mode 100644 index 0000000..1f1ce3a --- /dev/null +++ b/include/linux/platform_data/gpio-dwapb.h @@ -0,0 +1,32 @@ +/* + * Copyright(c) 2014 Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + */ + +#ifndef GPIO_DW_APB_H +#define GPIO_DW_APB_H + +struct dwapb_gpio_port_property { + struct device_node *node; + const char *name; + unsigned int idx; + unsigned int ngpio; + unsigned int gpio_base; + unsigned int irq; + bool irq_shared; +}; + +struct dwapb_gpio_platform_data { + struct dwapb_gpio_port_property *properties; + unsigned int nports; +}; + +#endif