[v5,1/3] gpio: designware: convert device node to fwnode
diff mbox

Message ID 1457077474-124064-2-git-send-email-qiujiang@huawei.com
State New
Headers show

Commit Message

qiujiang March 4, 2016, 7:44 a.m. UTC
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(-)

Comments

Alan Tull March 10, 2016, 7:09 p.m. UTC | #1
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
Andy Shevchenko March 10, 2016, 8:27 p.m. UTC | #2
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.
qiujiang March 11, 2016, 12:44 a.m. UTC | #3
在 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
qiujiang March 11, 2016, 12:48 a.m. UTC | #4
在 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
qiujiang March 15, 2016, 12:56 p.m. UTC | #5
在 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
Linus Walleij March 22, 2016, 10:38 a.m. UTC | #6
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
Mika Westerberg March 22, 2016, 3:55 p.m. UTC | #7
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
qiujiang March 23, 2016, 11:41 a.m. UTC | #8
在 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
Alan Tull March 23, 2016, 4:20 p.m. UTC | #9
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
qiujiang March 24, 2016, 1:24 a.m. UTC | #10
在 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

Patch
diff mbox

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;