| Message ID | 1458959496-57455-2-git-send-email-qiujiang@huawei.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 25, 2016 at 9:31 PM, qiujiang <qiujiang@huawei.com> wrote: > - 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 && > + fwnode_property_read_bool(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; > + if (dev->of_node) > + pp->name = to_of_node(fwnode)->full_name; Hi Jiang, I tested lightly on a CycloneV and it worked fine (with device tree). One suggestion for both patches: you could remove name from struct dwapb_port_property and get rid of pp->name and nobody would miss it. All it is used for is some dev_err's so the device info gets printed anyway. For example (if I leave the irq out of the DT) gpio-dwapb ff708000.gpio: no irq for bank /soc/gpio@ff708000/gpio-controller@0 is redundant. The only additional info here from the name is the port index. That could be added to the messages without having to get the name through the two property/of methods. Alan -- 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 29, 2016 at 7:30 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: > On Fri, Mar 25, 2016 at 9:31 PM, qiujiang <qiujiang@huawei.com> wrote: > >> - 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 && >> + fwnode_property_read_bool(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; >> + if (dev->of_node) >> + pp->name = to_of_node(fwnode)->full_name; > > Hi Jiang, > > I tested lightly on a CycloneV and it worked fine (with device tree). > > One suggestion for both patches: you could remove name from struct > dwapb_port_property and get rid of pp->name and nobody would miss it. > All it is used for is some dev_err's so the device info gets printed > anyway. For example (if I leave the irq out of the DT) > > gpio-dwapb ff708000.gpio: no irq for bank /soc/gpio@ff708000/gpio-controller@0 > > is redundant. The only additional info here from the name is the port > index. That could be added to the messages without having to get the > name through the two property/of methods. > Good suggestion! That'll make patches and code cleaner. Perhaps separate prepended patch?
在 2016/3/30 19:38, Andy Shevchenko 写道: > On Tue, Mar 29, 2016 at 7:30 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: >> On Fri, Mar 25, 2016 at 9:31 PM, qiujiang <qiujiang@huawei.com> wrote: >> >>> - 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 && >>> + fwnode_property_read_bool(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; >>> + if (dev->of_node) >>> + pp->name = to_of_node(fwnode)->full_name; >> >> Hi Jiang, >> >> I tested lightly on a CycloneV and it worked fine (with device tree). >> >> One suggestion for both patches: you could remove name from struct >> dwapb_port_property and get rid of pp->name and nobody would miss it. >> All it is used for is some dev_err's so the device info gets printed >> anyway. For example (if I leave the irq out of the DT) >> >> gpio-dwapb ff708000.gpio: no irq for bank /soc/gpio@ff708000/gpio-controller@0 >> >> is redundant. The only additional info here from the name is the port >> index. That could be added to the messages without having to get the >> name through the two property/of methods. >> > > Good suggestion! That'll make patches and code cleaner. > > Perhaps separate prepended patch? > Hi Alan/Andy, It sounds good, I will follow this suggestion and do a test. But, what's the "separate prepended patch" mean? 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
On Thu, Mar 31, 2016 at 3:51 AM, Jiang Qiu <qiujiang@huawei.com> wrote: > 在 2016/3/30 19:38, Andy Shevchenko 写道: >>> One suggestion for both patches: you could remove name from struct >>> dwapb_port_property and get rid of pp->name and nobody would miss it. >>> All it is used for is some dev_err's so the device info gets printed >>> anyway. For example (if I leave the irq out of the DT) >>> >>> gpio-dwapb ff708000.gpio: no irq for bank /soc/gpio@ff708000/gpio-controller@0 >>> >>> is redundant. The only additional info here from the name is the port >>> index. That could be added to the messages without having to get the >>> name through the two property/of methods. >>> >> >> Good suggestion! That'll make patches and code cleaner. >> >> Perhaps separate prepended patch? >> > Hi Alan/Andy, > > It sounds good, I will follow this suggestion and do a test. But, what's the > "separate prepended patch" mean? Preparatory patch which goes first in the series.
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 597de1e..3498393 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; @@ -447,19 +449,15 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) } static struct dwapb_platform_data * -dwapb_gpio_get_pdata_of(struct device *dev) +dwapb_gpio_get_pdata(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,20 @@ 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 && + fwnode_property_read_bool(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; + if (dev->of_node) + pp->name = to_of_node(fwnode)->full_name; } return pdata; @@ -523,7 +521,7 @@ static int dwapb_gpio_probe(struct platform_device *pdev) struct dwapb_platform_data *pdata = dev_get_platdata(dev); if (!pdata) { - pdata = dwapb_gpio_get_pdata_of(dev); + pdata = dwapb_gpio_get_pdata(dev); if (IS_ERR(pdata)) return PTR_ERR(pdata); } diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c index bdc5e27..958ed26 100644 --- a/drivers/mfd/intel_quark_i2c_gpio.c +++ b/drivers/mfd/intel_quark_i2c_gpio.c @@ -219,7 +219,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;