| Message ID | 1457077474-124064-2-git-send-email-qiujiang@huawei.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@huawei.com> wrote: > This patch converts device node to fwnode in > dwapb_port_property for designware gpio driver, > so as to provide a unified data structure for DT > and ACPI bindings. > > Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: qiujiang <qiujiang@huawei.com> > --- > drivers/gpio/gpio-dwapb.c | 43 +++++++++++++++----------------- > drivers/mfd/intel_quark_i2c_gpio.c | 2 +- > include/linux/platform_data/gpio-dwapb.h | 2 +- > 3 files changed, 22 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 597de1e..49f6e5d 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -22,6 +22,7 @@ > #include <linux/of_address.h> > #include <linux/of_irq.h> > #include <linux/platform_device.h> > +#include <linux/property.h> > #include <linux/spinlock.h> > #include <linux/platform_data/gpio-dwapb.h> > #include <linux/slab.h> > @@ -290,14 +291,14 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > struct dwapb_port_property *pp) > { > struct gpio_chip *gc = &port->gc; > - struct device_node *node = pp->node; > + struct fwnode_handle *fwnode = pp->fwnode; > struct irq_chip_generic *irq_gc = NULL; > unsigned int hwirq, ngpio = gc->ngpio; > struct irq_chip_type *ct; > int err, i; > > - gpio->domain = irq_domain_add_linear(node, ngpio, > - &irq_generic_chip_ops, gpio); > + gpio->domain = irq_domain_create_linear(fwnode, ngpio, > + &irq_generic_chip_ops, gpio); > if (!gpio->domain) > return; > > @@ -415,7 +416,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > } > > #ifdef CONFIG_OF_GPIO > - port->gc.of_node = pp->node; > + port->gc.of_node = is_of_node(pp->fwnode) ? > + to_of_node(pp->fwnode) : NULL; > #endif > port->gc.ngpio = pp->ngpio; > port->gc.base = pp->gpio_base; > @@ -449,17 +451,13 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) > static struct dwapb_platform_data * > dwapb_gpio_get_pdata_of(struct device *dev) > { > - struct device_node *node, *port_np; > + struct fwnode_handle *fwnode; > struct dwapb_platform_data *pdata; > struct dwapb_port_property *pp; > int nports; > int i; > > - node = dev->of_node; > - if (!IS_ENABLED(CONFIG_OF_GPIO) || !node) > - return ERR_PTR(-ENODEV); > - > - nports = of_get_child_count(node); > + nports = device_get_child_node_count(dev); > if (nports == 0) > return ERR_PTR(-ENODEV); > > @@ -474,21 +472,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) > pdata->nports = nports; > > i = 0; > - for_each_child_of_node(node, port_np) { > + device_for_each_child_node(dev, fwnode) { > pp = &pdata->properties[i++]; > - pp->node = port_np; > + pp->fwnode = fwnode; > > - if (of_property_read_u32(port_np, "reg", &pp->idx) || > + if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) || > pp->idx >= DWAPB_MAX_PORTS) { > - dev_err(dev, "missing/invalid port index for %s\n", > - port_np->full_name); > + dev_err(dev, "missing/invalid port index\n"); > return ERR_PTR(-EINVAL); > } > > - if (of_property_read_u32(port_np, "snps,nr-gpios", > + if (fwnode_property_read_u32(fwnode, "snps,nr-gpios", > &pp->ngpio)) { > - dev_info(dev, "failed to get number of gpios for %s\n", > - port_np->full_name); > + dev_info(dev, "failed to get number of gpios\n"); > pp->ngpio = 32; > } > > @@ -496,18 +492,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) > * Only port A can provide interrupts in all configurations of > * the IP. > */ > - if (pp->idx == 0 && > - of_property_read_bool(port_np, "interrupt-controller")) { > - pp->irq = irq_of_parse_and_map(port_np, 0); > + if (dev->of_node && pp->idx == 0 && > + of_property_read_bool(to_of_node(fwnode), > + "interrupt-controller")) { Hi Qiujiang, Is there a reason to use "of_property_read_bool" here instead of "device_property_read_bool" or similar? Alan > + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); > if (!pp->irq) { > dev_warn(dev, "no irq for bank %s\n", > - port_np->full_name); > + to_of_node(fwnode)->full_name); > } > } > > pp->irq_shared = false; > pp->gpio_base = -1; > - pp->name = port_np->full_name; > + pp->name = to_of_node(fwnode)->full_name; > } > > return pdata; > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c > index 0421374..265bd3c 100644 > --- a/drivers/mfd/intel_quark_i2c_gpio.c > +++ b/drivers/mfd/intel_quark_i2c_gpio.c > @@ -227,7 +227,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell) > return -ENOMEM; > > /* Set the properties for portA */ > - pdata->properties->node = NULL; > + pdata->properties->fwnode = NULL; > pdata->properties->name = "intel-quark-x1000-gpio-portA"; > pdata->properties->idx = 0; > pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO; > diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h > index 28702c8..c5bd1f2 100644 > --- a/include/linux/platform_data/gpio-dwapb.h > +++ b/include/linux/platform_data/gpio-dwapb.h > @@ -15,7 +15,7 @@ > #define GPIO_DW_APB_H > > struct dwapb_port_property { > - struct device_node *node; > + struct fwnode_handle *fwnode; > const char *name; > unsigned int idx; > unsigned int ngpio; > -- > 1.9.1 > -- 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 Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: > On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@huawei.com> wrote: >> This patch converts device node to fwnode in >> dwapb_port_property for designware gpio driver, >> so as to provide a unified data structure for DT >> and ACPI bindings. >> >> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> Signed-off-by: qiujiang <qiujiang@huawei.com> >> @@ -496,18 +492,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) >> * Only port A can provide interrupts in all configurations of >> * the IP. >> */ >> - if (pp->idx == 0 && >> - of_property_read_bool(port_np, "interrupt-controller")) { >> - pp->irq = irq_of_parse_and_map(port_np, 0); >> + if (dev->of_node && pp->idx == 0 && >> + of_property_read_bool(to_of_node(fwnode), >> + "interrupt-controller")) { > > Hi Qiujiang, > > Is there a reason to use "of_property_read_bool" here instead of > "device_property_read_bool" or similar? Yeah, this patch looks unfinished. This should be if (pp->idx == 0 && fwnode_property_read_bool(fwnode, "interrupt-controller")) { > > Alan > >> + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); But here should be common method called which takes fwnode on input. >> if (!pp->irq) { >> dev_warn(dev, "no irq for bank %s\n", >> - port_np->full_name); >> + to_of_node(fwnode)->full_name); >> } >> } >> >> pp->irq_shared = false; >> pp->gpio_base = -1; >> - pp->name = port_np->full_name; >> + pp->name = to_of_node(fwnode)->full_name; Also those two should be device property source agnostic. That's what I tried to tell earlier.
在 2016/3/11 3:09, Alan Tull 写道: > On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@huawei.com> wrote: >> This patch converts device node to fwnode in >> dwapb_port_property for designware gpio driver, >> so as to provide a unified data structure for DT >> and ACPI bindings. >> >> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> Signed-off-by: qiujiang <qiujiang@huawei.com> >> --- >> drivers/gpio/gpio-dwapb.c | 43 +++++++++++++++----------------- >> drivers/mfd/intel_quark_i2c_gpio.c | 2 +- >> include/linux/platform_data/gpio-dwapb.h | 2 +- >> 3 files changed, 22 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c >> index 597de1e..49f6e5d 100644 >> --- a/drivers/gpio/gpio-dwapb.c >> +++ b/drivers/gpio/gpio-dwapb.c >> @@ -22,6 +22,7 @@ >> #include <linux/of_address.h> >> #include <linux/of_irq.h> >> #include <linux/platform_device.h> >> +#include <linux/property.h> >> #include <linux/spinlock.h> >> #include <linux/platform_data/gpio-dwapb.h> >> #include <linux/slab.h> >> @@ -290,14 +291,14 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, >> struct dwapb_port_property *pp) >> { >> struct gpio_chip *gc = &port->gc; >> - struct device_node *node = pp->node; >> + struct fwnode_handle *fwnode = pp->fwnode; >> struct irq_chip_generic *irq_gc = NULL; >> unsigned int hwirq, ngpio = gc->ngpio; >> struct irq_chip_type *ct; >> int err, i; >> >> - gpio->domain = irq_domain_add_linear(node, ngpio, >> - &irq_generic_chip_ops, gpio); >> + gpio->domain = irq_domain_create_linear(fwnode, ngpio, >> + &irq_generic_chip_ops, gpio); >> if (!gpio->domain) >> return; >> >> @@ -415,7 +416,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, >> } >> >> #ifdef CONFIG_OF_GPIO >> - port->gc.of_node = pp->node; >> + port->gc.of_node = is_of_node(pp->fwnode) ? >> + to_of_node(pp->fwnode) : NULL; >> #endif >> port->gc.ngpio = pp->ngpio; >> port->gc.base = pp->gpio_base; >> @@ -449,17 +451,13 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) >> static struct dwapb_platform_data * >> dwapb_gpio_get_pdata_of(struct device *dev) >> { >> - struct device_node *node, *port_np; >> + struct fwnode_handle *fwnode; >> struct dwapb_platform_data *pdata; >> struct dwapb_port_property *pp; >> int nports; >> int i; >> >> - node = dev->of_node; >> - if (!IS_ENABLED(CONFIG_OF_GPIO) || !node) >> - return ERR_PTR(-ENODEV); >> - >> - nports = of_get_child_count(node); >> + nports = device_get_child_node_count(dev); >> if (nports == 0) >> return ERR_PTR(-ENODEV); >> >> @@ -474,21 +472,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) >> pdata->nports = nports; >> >> i = 0; >> - for_each_child_of_node(node, port_np) { >> + device_for_each_child_node(dev, fwnode) { >> pp = &pdata->properties[i++]; >> - pp->node = port_np; >> + pp->fwnode = fwnode; >> >> - if (of_property_read_u32(port_np, "reg", &pp->idx) || >> + if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) || >> pp->idx >= DWAPB_MAX_PORTS) { >> - dev_err(dev, "missing/invalid port index for %s\n", >> - port_np->full_name); >> + dev_err(dev, "missing/invalid port index\n"); >> return ERR_PTR(-EINVAL); >> } >> >> - if (of_property_read_u32(port_np, "snps,nr-gpios", >> + if (fwnode_property_read_u32(fwnode, "snps,nr-gpios", >> &pp->ngpio)) { >> - dev_info(dev, "failed to get number of gpios for %s\n", >> - port_np->full_name); >> + dev_info(dev, "failed to get number of gpios\n"); >> pp->ngpio = 32; >> } >> >> @@ -496,18 +492,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) >> * Only port A can provide interrupts in all configurations of >> * the IP. >> */ >> - if (pp->idx == 0 && >> - of_property_read_bool(port_np, "interrupt-controller")) { >> - pp->irq = irq_of_parse_and_map(port_np, 0); >> + if (dev->of_node && pp->idx == 0 && >> + of_property_read_bool(to_of_node(fwnode), >> + "interrupt-controller")) { > Hi Qiujiang, > > Is there a reason to use "of_property_read_bool" here instead of > "device_property_read_bool" or similar? > > Alan Agree, "to_of_node" should be never used since it coverted to fwnode. I will give a more reasonable solution in the next version. Thanks, Jiang > >> + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); >> if (!pp->irq) { >> dev_warn(dev, "no irq for bank %s\n", >> - port_np->full_name); >> + to_of_node(fwnode)->full_name); >> } >> } >> >> pp->irq_shared = false; >> pp->gpio_base = -1; >> - pp->name = port_np->full_name; >> + pp->name = to_of_node(fwnode)->full_name; >> } >> >> return pdata; >> diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c >> index 0421374..265bd3c 100644 >> --- a/drivers/mfd/intel_quark_i2c_gpio.c >> +++ b/drivers/mfd/intel_quark_i2c_gpio.c >> @@ -227,7 +227,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell) >> return -ENOMEM; >> >> /* Set the properties for portA */ >> - pdata->properties->node = NULL; >> + pdata->properties->fwnode = NULL; >> pdata->properties->name = "intel-quark-x1000-gpio-portA"; >> pdata->properties->idx = 0; >> pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO; >> diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h >> index 28702c8..c5bd1f2 100644 >> --- a/include/linux/platform_data/gpio-dwapb.h >> +++ b/include/linux/platform_data/gpio-dwapb.h >> @@ -15,7 +15,7 @@ >> #define GPIO_DW_APB_H >> >> struct dwapb_port_property { >> - struct device_node *node; >> + struct fwnode_handle *fwnode; >> const char *name; >> unsigned int idx; >> unsigned int ngpio; >> -- >> 1.9.1 >> > . > -- 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
在 2016/3/11 4:27, Andy Shevchenko 写道: > On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: >> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@huawei.com> wrote: >>> This patch converts device node to fwnode in >>> dwapb_port_property for designware gpio driver, >>> so as to provide a unified data structure for DT >>> and ACPI bindings. >>> >>> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>> Signed-off-by: qiujiang <qiujiang@huawei.com> >>> @@ -496,18 +492,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) >>> * Only port A can provide interrupts in all configurations of >>> * the IP. >>> */ >>> - if (pp->idx == 0 && >>> - of_property_read_bool(port_np, "interrupt-controller")) { >>> - pp->irq = irq_of_parse_and_map(port_np, 0); >>> + if (dev->of_node && pp->idx == 0 && >>> + of_property_read_bool(to_of_node(fwnode), >>> + "interrupt-controller")) { >> Hi Qiujiang, >> >> Is there a reason to use "of_property_read_bool" here instead of >> "device_property_read_bool" or similar? > Yeah, this patch looks unfinished. > > This should be > if (pp->idx == 0 && fwnode_property_read_bool(fwnode, > "interrupt-controller")) { Yes, agreed, "to_of_node" will never appear in this patch : ) >> Alan >> >>> + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); > But here should be common method called which takes fwnode on input. Agreed. > >>> if (!pp->irq) { >>> dev_warn(dev, "no irq for bank %s\n", >>> - port_np->full_name); >>> + to_of_node(fwnode)->full_name); >>> } >>> } >>> >>> pp->irq_shared = false; >>> pp->gpio_base = -1; >>> - pp->name = port_np->full_name; >>> + pp->name = to_of_node(fwnode)->full_name; > Also those two should be device property source agnostic. That's what > I tried to tell earlier. Agreed. Thanks, Jiang -- 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
在 2016/3/11 4:27, Andy Shevchenko 写道: > On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: >> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@huawei.com> wrote: >>> This patch converts device node to fwnode in >>> dwapb_port_property for designware gpio driver, >>> so as to provide a unified data structure for DT >>> and ACPI bindings. >>> >>> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>> Signed-off-by: qiujiang <qiujiang@huawei.com> >>> @@ -496,18 +492,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) >>> * Only port A can provide interrupts in all configurations of >>> * the IP. >>> */ >>> - if (pp->idx == 0 && >>> - of_property_read_bool(port_np, "interrupt-controller")) { >>> - pp->irq = irq_of_parse_and_map(port_np, 0); >>> + if (dev->of_node && pp->idx == 0 && >>> + of_property_read_bool(to_of_node(fwnode), >>> + "interrupt-controller")) { >> Hi Qiujiang, >> >> Is there a reason to use "of_property_read_bool" here instead of >> "device_property_read_bool" or similar? > Yeah, this patch looks unfinished. > > This should be > if (pp->idx == 0 && fwnode_property_read_bool(fwnode, > "interrupt-controller")) { >> Alan >> >>> + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); > But here should be common method called which takes fwnode on input. Hi Alan, Andy, There is some trouble to replace the interface irq_of_parse_and_map and take fwnode on input. As far as I know, there are two common APIs to parse interrupt resource: 1) irq_of_parse_and_map() - It is a DT specific API, and have to use of_node as input. 2) platform_get_irq() - It could be used to DT as well as ACPI, It needs platform_ device as input. In this driver, the DTs is defined as this: pc_gpio0: gpio@802e0000 { #address-cells = <1>; #size-cells = <0>; compatible = "snps,dw-apb-gpio"; reg = <0x0 0x802e0000 0x0 0x10000>; porta: gpio-controller@0 { compatible = "snps,dw-apb-gpio-port"; gpio-controller; #gpio-cells = <2>; snps,nr-gpios = <32>; reg = <0>; interrupt-controller; #interrupt-cells = <2>; interrupts = <0 312 4>; }; }; The compatible string, which is "snps,dw-apb-gpio", is only for the parent node. That is mean, there is no platform device for the child node, but the interrupt resource is defined in child node. Is there any other APIs can be used to parse interrupts resource using fwnode as input? Thanks, Jiang >>> if (!pp->irq) { >>> dev_warn(dev, "no irq for bank %s\n", >>> - port_np->full_name); >>> + to_of_node(fwnode)->full_name); >>> } >>> } >>> >>> pp->irq_shared = false; >>> pp->gpio_base = -1; >>> - pp->name = port_np->full_name; >>> + pp->name = to_of_node(fwnode)->full_name; > Also those two should be device property source agnostic. That's what > I tried to tell earlier. > -- 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 Fri, Mar 4, 2016 at 8:44 AM, qiujiang <qiujiang@huawei.com> wrote: > This patch converts device node to fwnode in > dwapb_port_property for designware gpio driver, > so as to provide a unified data structure for DT > and ACPI bindings. > > Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: qiujiang <qiujiang@huawei.com> I need Mika's and maybe also Rafael's ACKs on this before I merge any of the series. > --- a/drivers/mfd/intel_quark_i2c_gpio.c > +++ b/drivers/mfd/intel_quark_i2c_gpio.c > @@ -227,7 +227,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell) > return -ENOMEM; > > /* Set the properties for portA */ > - pdata->properties->node = NULL; > + pdata->properties->fwnode = NULL; And apparently also the MFD maintainer needs to be involved to ACK this, so add Lee Jones on subsequent postings. 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 Tue, Mar 22, 2016 at 11:38:26AM +0100, Linus Walleij wrote: > On Fri, Mar 4, 2016 at 8:44 AM, qiujiang <qiujiang@huawei.com> wrote: > > > This patch converts device node to fwnode in > > dwapb_port_property for designware gpio driver, > > so as to provide a unified data structure for DT > > and ACPI bindings. > > > > Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Signed-off-by: qiujiang <qiujiang@huawei.com> > > I need Mika's and maybe also Rafael's ACKs on this before > I merge any of the series. Hmm, I thought I already acked the ACPI GPIO signaled event patch? It still looks fine to me. -- 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
在 2016/3/11 4:27, Andy Shevchenko 写道: > On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: >> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@huawei.com> wrote: >>> This patch converts device node to fwnode in >>> dwapb_port_property for designware gpio driver, >>> so as to provide a unified data structure for DT >>> and ACPI bindings. >>> >>> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>> Signed-off-by: qiujiang <qiujiang@huawei.com> >>> @@ -496,18 +492,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) >>> * Only port A can provide interrupts in all configurations of >>> * the IP. >>> */ >>> - if (pp->idx == 0 && >>> - of_property_read_bool(port_np, "interrupt-controller")) { >>> - pp->irq = irq_of_parse_and_map(port_np, 0); >>> + if (dev->of_node && pp->idx == 0 && >>> + of_property_read_bool(to_of_node(fwnode), >>> + "interrupt-controller")) { >> Hi Qiujiang, >> >> Is there a reason to use "of_property_read_bool" here instead of >> "device_property_read_bool" or similar? > Yeah, this patch looks unfinished. > > This should be > if (pp->idx == 0 && fwnode_property_read_bool(fwnode, > "interrupt-controller")) { Hi, Alan, Andy, Mika, Many thanks for help me review this patchset. I tried to use a unified interface to parse the interrupts resource in DT and ACPI, but it looks difficult because of the hierarchy device node structure as follow: pc_gpio1: gpio@802f0000 { #address-cells = <1>; #size-cells = <0>; compatible = "snps,dw-apb-gpio"; reg = <0x0 0x802f0000 0x0 0x10000>; porta: gpio-controller@0 { compatible = "snps,dw-apb-gpio-port"; gpio-controller; #gpio-cells = <2>; snps,nr-gpios = <32>; reg = <0>; interrupt-controller; #interrupt-cells = <2>; interrupts = <0 313 4>; }; }; According to the designware gpio databook, each GPIO controller includes 4 ports (porta,b,c,d), only porta can be a interrupt controller. So, I moved the interrupts resource to the parent node from porta in ACPI. Device(GPI0) { Name(_HID, "HISI0181") Name(_ADR, 0) // _ADR: Address Name(_UID, 0) Name (_CRS, ResourceTemplate () { Memory32Fixed (ReadWrite, 0x802e0000, 0x10000) Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive,,,) {344} //moved here }) Device(PRTa) { Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () {"reg",0}, Package () {"snps,nr-gpios",32}, } }) } } That is to say, if GPI0 should be a interrupt controller, the child node PRTa must be present first, then add the interrupt resource to the parent node GPI0 scope. Dose this proposal sounds ok? if yes, we can do that for DT. If not, there can only keep two branches to parse the IRQ resource, and the code looks strange. That would be great if I can get some help from you. Best Regards Jiang >> Alan >> >>> + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); > But here should be common method called which takes fwnode on input. > >>> if (!pp->irq) { >>> dev_warn(dev, "no irq for bank %s\n", >>> - port_np->full_name); >>> + to_of_node(fwnode)->full_name); >>> } >>> } >>> >>> pp->irq_shared = false; >>> pp->gpio_base = -1; >>> - pp->name = port_np->full_name; >>> + pp->name = to_of_node(fwnode)->full_name; > Also those two should be device property source agnostic. That's what > I tried to tell earlier. > -- 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 Wed, Mar 23, 2016 at 6:41 AM, Jiang Qiu <qiujiang@huawei.com> wrote: > 在 2016/3/11 4:27, Andy Shevchenko 写道: >> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: >>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@huawei.com> wrote: >>>> This patch converts device node to fwnode in >>>> dwapb_port_property for designware gpio driver, >>>> so as to provide a unified data structure for DT >>>> and ACPI bindings. >>>> >>>> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>>> Signed-off-by: qiujiang <qiujiang@huawei.com> >>>> @@ -496,18 +492,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) >>>> * Only port A can provide interrupts in all configurations of >>>> * the IP. >>>> */ >>>> - if (pp->idx == 0 && >>>> - of_property_read_bool(port_np, "interrupt-controller")) { >>>> - pp->irq = irq_of_parse_and_map(port_np, 0); >>>> + if (dev->of_node && pp->idx == 0 && >>>> + of_property_read_bool(to_of_node(fwnode), >>>> + "interrupt-controller")) { >>> Hi Qiujiang, >>> >>> Is there a reason to use "of_property_read_bool" here instead of >>> "device_property_read_bool" or similar? >> Yeah, this patch looks unfinished. >> >> This should be >> if (pp->idx == 0 && fwnode_property_read_bool(fwnode, >> "interrupt-controller")) { > Hi, Alan, Andy, Mika, > > Many thanks for help me review this patchset. > > I tried to use a unified interface to parse the interrupts resource in DT and ACPI, > but it looks difficult because of the hierarchy device node structure as follow: > > pc_gpio1: gpio@802f0000 { > #address-cells = <1>; > #size-cells = <0>; > compatible = "snps,dw-apb-gpio"; > reg = <0x0 0x802f0000 0x0 0x10000>; > > porta: gpio-controller@0 { > compatible = "snps,dw-apb-gpio-port"; > gpio-controller; > #gpio-cells = <2>; > snps,nr-gpios = <32>; > reg = <0>; > interrupt-controller; > #interrupt-cells = <2>; > interrupts = <0 313 4>; > }; > }; > > According to the designware gpio databook, each GPIO controller includes 4 ports > (porta,b,c,d), only porta can be a interrupt controller. So, I moved the interrupts > resource to the parent node from porta in ACPI. > > Device(GPI0) { > Name(_HID, "HISI0181") > Name(_ADR, 0) // _ADR: Address > Name(_UID, 0) > > Name (_CRS, ResourceTemplate () { > Memory32Fixed (ReadWrite, 0x802e0000, 0x10000) > Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive,,,) {344} //moved here > }) > > Device(PRTa) { > Name (_DSD, Package () { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package () {"reg",0}, > Package () {"snps,nr-gpios",32}, > } > }) > } > } > > That is to say, if GPI0 should be a interrupt controller, the child node PRTa must be > present first, then add the interrupt resource to the parent node GPI0 scope. > > Dose this proposal sounds ok? if yes, we can do that for DT. If not, there can only > keep two branches to parse the IRQ resource, and the code looks strange. Hi Jiang, Are you suggesting a change for the DT to make it similar to the ACPI case? DT changes create unexpected breakages when people upgrade their kernel even if the change is minor. How bad will the code look if you implement it as the two separate code paths as you suggest? Alan > > That would be great if I can get some help from you. > > Best Regards > Jiang >>> Alan >>> >>>> + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); >> But here should be common method called which takes fwnode on input. >> >>>> if (!pp->irq) { >>>> dev_warn(dev, "no irq for bank %s\n", >>>> - port_np->full_name); >>>> + to_of_node(fwnode)->full_name); >>>> } >>>> } >>>> >>>> pp->irq_shared = false; >>>> pp->gpio_base = -1; >>>> - pp->name = port_np->full_name; >>>> + pp->name = to_of_node(fwnode)->full_name; >> Also those two should be device property source agnostic. That's what >> I tried to tell earlier. >> > > -- 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
在 2016/3/24 0:20, Alan Tull 写道: > On Wed, Mar 23, 2016 at 6:41 AM, Jiang Qiu <qiujiang@huawei.com> wrote: >> 在 2016/3/11 4:27, Andy Shevchenko 写道: >>> On Thu, Mar 10, 2016 at 9:09 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: >>>> On Fri, Mar 4, 2016 at 1:44 AM, qiujiang <qiujiang@huawei.com> wrote: >>>>> This patch converts device node to fwnode in >>>>> dwapb_port_property for designware gpio driver, >>>>> so as to provide a unified data structure for DT >>>>> and ACPI bindings. >>>>> >>>>> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>>>> Signed-off-by: qiujiang <qiujiang@huawei.com> >>>>> @@ -496,18 +492,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) >>>>> * Only port A can provide interrupts in all configurations of >>>>> * the IP. >>>>> */ >>>>> - if (pp->idx == 0 && >>>>> - of_property_read_bool(port_np, "interrupt-controller")) { >>>>> - pp->irq = irq_of_parse_and_map(port_np, 0); >>>>> + if (dev->of_node && pp->idx == 0 && >>>>> + of_property_read_bool(to_of_node(fwnode), >>>>> + "interrupt-controller")) { >>>> Hi Qiujiang, >>>> >>>> Is there a reason to use "of_property_read_bool" here instead of >>>> "device_property_read_bool" or similar? >>> Yeah, this patch looks unfinished. >>> >>> This should be >>> if (pp->idx == 0 && fwnode_property_read_bool(fwnode, >>> "interrupt-controller")) { >> Hi, Alan, Andy, Mika, >> >> Many thanks for help me review this patchset. >> >> I tried to use a unified interface to parse the interrupts resource in DT and ACPI, >> but it looks difficult because of the hierarchy device node structure as follow: >> >> pc_gpio1: gpio@802f0000 { >> #address-cells = <1>; >> #size-cells = <0>; >> compatible = "snps,dw-apb-gpio"; >> reg = <0x0 0x802f0000 0x0 0x10000>; >> >> porta: gpio-controller@0 { >> compatible = "snps,dw-apb-gpio-port"; >> gpio-controller; >> #gpio-cells = <2>; >> snps,nr-gpios = <32>; >> reg = <0>; >> interrupt-controller; >> #interrupt-cells = <2>; >> interrupts = <0 313 4>; >> }; >> }; >> >> According to the designware gpio databook, each GPIO controller includes 4 ports >> (porta,b,c,d), only porta can be a interrupt controller. So, I moved the interrupts >> resource to the parent node from porta in ACPI. >> >> Device(GPI0) { >> Name(_HID, "HISI0181") >> Name(_ADR, 0) // _ADR: Address >> Name(_UID, 0) >> >> Name (_CRS, ResourceTemplate () { >> Memory32Fixed (ReadWrite, 0x802e0000, 0x10000) >> Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive,,,) {344} //moved here >> }) >> >> Device(PRTa) { >> Name (_DSD, Package () { >> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >> Package () { >> Package () {"reg",0}, >> Package () {"snps,nr-gpios",32}, >> } >> }) >> } >> } >> >> That is to say, if GPI0 should be a interrupt controller, the child node PRTa must be >> present first, then add the interrupt resource to the parent node GPI0 scope. >> >> Dose this proposal sounds ok? if yes, we can do that for DT. If not, there can only >> keep two branches to parse the IRQ resource, and the code looks strange. > Hi Jiang, > > Are you suggesting a change for the DT to make it similar to the ACPI > case? DT changes create unexpected breakages when people upgrade > their kernel even if the change is minor. How bad will the code look > if you implement it as the two separate code paths as you suggest? > > Alan Agreed. It would better do not make any change for DT if possible. If keeping the two separate code paths, as presented above, I have to do those check like "if (dev->of_node)" and covert back the fwnode to of_node, so as to parse IRQ resource in DT. Andy think this patch looks unfinished if used to_of_node. Andy, do you think it acceptable if keeping two separate paths for DT and ACPI? >> That would be great if I can get some help from you. >> >> Best Regards >> Jiang >>>> Alan >>>> >>>>> + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); >>> But here should be common method called which takes fwnode on input. >>> >>>>> if (!pp->irq) { >>>>> dev_warn(dev, "no irq for bank %s\n", >>>>> - port_np->full_name); >>>>> + to_of_node(fwnode)->full_name); >>>>> } >>>>> } >>>>> >>>>> pp->irq_shared = false; >>>>> pp->gpio_base = -1; >>>>> - pp->name = port_np->full_name; >>>>> + pp->name = to_of_node(fwnode)->full_name; >>> Also those two should be device property source agnostic. That's what >>> I tried to tell earlier. >>> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > . > -- 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
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 597de1e..49f6e5d 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -22,6 +22,7 @@ #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/platform_device.h> +#include <linux/property.h> #include <linux/spinlock.h> #include <linux/platform_data/gpio-dwapb.h> #include <linux/slab.h> @@ -290,14 +291,14 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, struct dwapb_port_property *pp) { struct gpio_chip *gc = &port->gc; - struct device_node *node = pp->node; + struct fwnode_handle *fwnode = pp->fwnode; struct irq_chip_generic *irq_gc = NULL; unsigned int hwirq, ngpio = gc->ngpio; struct irq_chip_type *ct; int err, i; - gpio->domain = irq_domain_add_linear(node, ngpio, - &irq_generic_chip_ops, gpio); + gpio->domain = irq_domain_create_linear(fwnode, ngpio, + &irq_generic_chip_ops, gpio); if (!gpio->domain) return; @@ -415,7 +416,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, } #ifdef CONFIG_OF_GPIO - port->gc.of_node = pp->node; + port->gc.of_node = is_of_node(pp->fwnode) ? + to_of_node(pp->fwnode) : NULL; #endif port->gc.ngpio = pp->ngpio; port->gc.base = pp->gpio_base; @@ -449,17 +451,13 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) static struct dwapb_platform_data * dwapb_gpio_get_pdata_of(struct device *dev) { - struct device_node *node, *port_np; + struct fwnode_handle *fwnode; struct dwapb_platform_data *pdata; struct dwapb_port_property *pp; int nports; int i; - node = dev->of_node; - if (!IS_ENABLED(CONFIG_OF_GPIO) || !node) - return ERR_PTR(-ENODEV); - - nports = of_get_child_count(node); + nports = device_get_child_node_count(dev); if (nports == 0) return ERR_PTR(-ENODEV); @@ -474,21 +472,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) pdata->nports = nports; i = 0; - for_each_child_of_node(node, port_np) { + device_for_each_child_node(dev, fwnode) { pp = &pdata->properties[i++]; - pp->node = port_np; + pp->fwnode = fwnode; - if (of_property_read_u32(port_np, "reg", &pp->idx) || + if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) || pp->idx >= DWAPB_MAX_PORTS) { - dev_err(dev, "missing/invalid port index for %s\n", - port_np->full_name); + dev_err(dev, "missing/invalid port index\n"); return ERR_PTR(-EINVAL); } - if (of_property_read_u32(port_np, "snps,nr-gpios", + if (fwnode_property_read_u32(fwnode, "snps,nr-gpios", &pp->ngpio)) { - dev_info(dev, "failed to get number of gpios for %s\n", - port_np->full_name); + dev_info(dev, "failed to get number of gpios\n"); pp->ngpio = 32; } @@ -496,18 +492,19 @@ dwapb_gpio_get_pdata_of(struct device *dev) * Only port A can provide interrupts in all configurations of * the IP. */ - if (pp->idx == 0 && - of_property_read_bool(port_np, "interrupt-controller")) { - pp->irq = irq_of_parse_and_map(port_np, 0); + if (dev->of_node && pp->idx == 0 && + of_property_read_bool(to_of_node(fwnode), + "interrupt-controller")) { + pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0); if (!pp->irq) { dev_warn(dev, "no irq for bank %s\n", - port_np->full_name); + to_of_node(fwnode)->full_name); } } pp->irq_shared = false; pp->gpio_base = -1; - pp->name = port_np->full_name; + pp->name = to_of_node(fwnode)->full_name; } return pdata; diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c index 0421374..265bd3c 100644 --- a/drivers/mfd/intel_quark_i2c_gpio.c +++ b/drivers/mfd/intel_quark_i2c_gpio.c @@ -227,7 +227,7 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell) return -ENOMEM; /* Set the properties for portA */ - pdata->properties->node = NULL; + pdata->properties->fwnode = NULL; pdata->properties->name = "intel-quark-x1000-gpio-portA"; pdata->properties->idx = 0; pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO; diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h index 28702c8..c5bd1f2 100644 --- a/include/linux/platform_data/gpio-dwapb.h +++ b/include/linux/platform_data/gpio-dwapb.h @@ -15,7 +15,7 @@ #define GPIO_DW_APB_H struct dwapb_port_property { - struct device_node *node; + struct fwnode_handle *fwnode; const char *name; unsigned int idx; unsigned int ngpio;