Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-02-02 Thread Agustin Vega-Frias 

Hi Rafael,

On 2017-01-31 17:10, Rafael J. Wysocki wrote:

On Wed, Jan 18, 2017 at 5:46 PM, Agustin Vega-Frias
 wrote:

ACPI extended IRQ resources may contain a ResourceSource to specify
an alternate interrupt controller. Introduce acpi_irq_get and use it
to implement ResourceSource/IRQ domain mapping.

The new API is similar to of_irq_get and allows re-initialization
of a platform resource from the ACPI extended IRQ resource, and
provides proper behavior for probe deferral when the domain is not
yet present when called.

Signed-off-by: Agustin Vega-Frias 
---

[...]

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index c4af003..61423d2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, 
unsigned int num)

}

r = platform_get_resource(dev, IORESOURCE_IRQ, num);
+   if (r && r->flags & IORESOURCE_DISABLED && 
ACPI_COMPANION(>dev)) {

+   int ret;
+
+   ret = acpi_irq_get(ACPI_HANDLE(>dev), num, r);
+   if (ret)
+   return ret;
+   }


The code above is a bit convoluted.  It would be better to write it as
something like

if (r && r->flags & IORESOURCE_DISABLED) {
struct acpi_device *adev = ACPI_COMPANION(>dev);

if (adev) {
int ret = acpi_irq_get(adev->handle, num, r);

if (ret)
return ret;
}
}



I changed this to:

r = platform_get_resource(dev, IORESOURCE_IRQ, num);
if (has_acpi_companion(>dev)) {
if (r && r->flags & IORESOURCE_DISABLED) {
int ret;

ret = acpi_irq_get(ACPI_HANDLE(>dev), num, r);
if (ret)
return ret;
}
}

To avoid errors when CONFIG_ACPI is disabled.

Thanks for the ACKs. V12 coming soon.

Agustin


+
/*
 * The resources may pass trigger flags to the irqs that need
 * to be set up. It so happens that the trigger flags for
@@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
memset(>dev.devres_head, 0, 
sizeof(pd->dev.devres_head));

}
 }
-
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 5b36974..03a94cd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1153,4 +1153,14 @@ static inline void acpi_table_upgrade(void) { }
 static inline int parse_spcr(bool earlycon) { return 0; }
 #endif

+#ifdef CONFIG_ACPI_GENERIC_GSI
+int acpi_irq_get(acpi_handle handle, unsigned int index, struct 
resource *res);

+#else
+static inline int acpi_irq_get(acpi_handle handle, unsigned int 
index,

+  struct resource *res)
+{
+   return -EINVAL;
+}
+#endif
+
 #endif /*_LINUX_ACPI_H*/


The rest looks reasonable to me.

Thanks,
Rafael


--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-02-02 Thread Agustin Vega-Frias 

Hi Rafael,

On 2017-01-31 17:10, Rafael J. Wysocki wrote:

On Wed, Jan 18, 2017 at 5:46 PM, Agustin Vega-Frias
 wrote:

ACPI extended IRQ resources may contain a ResourceSource to specify
an alternate interrupt controller. Introduce acpi_irq_get and use it
to implement ResourceSource/IRQ domain mapping.

The new API is similar to of_irq_get and allows re-initialization
of a platform resource from the ACPI extended IRQ resource, and
provides proper behavior for probe deferral when the domain is not
yet present when called.

Signed-off-by: Agustin Vega-Frias 
---

[...]

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index c4af003..61423d2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, 
unsigned int num)

}

r = platform_get_resource(dev, IORESOURCE_IRQ, num);
+   if (r && r->flags & IORESOURCE_DISABLED && 
ACPI_COMPANION(>dev)) {

+   int ret;
+
+   ret = acpi_irq_get(ACPI_HANDLE(>dev), num, r);
+   if (ret)
+   return ret;
+   }


The code above is a bit convoluted.  It would be better to write it as
something like

if (r && r->flags & IORESOURCE_DISABLED) {
struct acpi_device *adev = ACPI_COMPANION(>dev);

if (adev) {
int ret = acpi_irq_get(adev->handle, num, r);

if (ret)
return ret;
}
}



I changed this to:

r = platform_get_resource(dev, IORESOURCE_IRQ, num);
if (has_acpi_companion(>dev)) {
if (r && r->flags & IORESOURCE_DISABLED) {
int ret;

ret = acpi_irq_get(ACPI_HANDLE(>dev), num, r);
if (ret)
return ret;
}
}

To avoid errors when CONFIG_ACPI is disabled.

Thanks for the ACKs. V12 coming soon.

Agustin


+
/*
 * The resources may pass trigger flags to the irqs that need
 * to be set up. It so happens that the trigger flags for
@@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
memset(>dev.devres_head, 0, 
sizeof(pd->dev.devres_head));

}
 }
-
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 5b36974..03a94cd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1153,4 +1153,14 @@ static inline void acpi_table_upgrade(void) { }
 static inline int parse_spcr(bool earlycon) { return 0; }
 #endif

+#ifdef CONFIG_ACPI_GENERIC_GSI
+int acpi_irq_get(acpi_handle handle, unsigned int index, struct 
resource *res);

+#else
+static inline int acpi_irq_get(acpi_handle handle, unsigned int 
index,

+  struct resource *res)
+{
+   return -EINVAL;
+}
+#endif
+
 #endif /*_LINUX_ACPI_H*/


The rest looks reasonable to me.

Thanks,
Rafael


--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-31 Thread Rafael J. Wysocki 
On Wed, Jan 18, 2017 at 5:46 PM, Agustin Vega-Frias
 wrote:
> ACPI extended IRQ resources may contain a ResourceSource to specify
> an alternate interrupt controller. Introduce acpi_irq_get and use it
> to implement ResourceSource/IRQ domain mapping.
>
> The new API is similar to of_irq_get and allows re-initialization
> of a platform resource from the ACPI extended IRQ resource, and
> provides proper behavior for probe deferral when the domain is not
> yet present when called.
>
> Signed-off-by: Agustin Vega-Frias 
> ---
>  drivers/acpi/Makefile   |   2 +-
>  drivers/acpi/gsi.c  |  98 -
>  drivers/acpi/irq.c  | 282 
> 
>  drivers/base/platform.c |   9 +-
>  include/linux/acpi.h|  10 ++
>  5 files changed, 301 insertions(+), 100 deletions(-)
>  delete mode 100644 drivers/acpi/gsi.c
>  create mode 100644 drivers/acpi/irq.c
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 9ed0878..a391bbc 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -55,7 +55,7 @@ acpi-$(CONFIG_DEBUG_FS)   += debugfs.o
>  acpi-$(CONFIG_ACPI_NUMA)   += numa.o
>  acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
>  acpi-y += acpi_lpat.o
> -acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
> +acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
>  acpi-$(CONFIG_ACPI_WATCHDOG)   += acpi_watchdog.o
>
>  # These are (potentially) separate modules
> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
> deleted file mode 100644
> index ee9e0f2..000
> --- a/drivers/acpi/gsi.c
> +++ /dev/null
> @@ -1,98 +0,0 @@
> -/*
> - * ACPI GSI IRQ layer
> - *
> - * Copyright (C) 2015 ARM Ltd.
> - * Author: Lorenzo Pieralisi 
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */
> -#include 
> -#include 
> -#include 
> -#include 
> -
> -enum acpi_irq_model_id acpi_irq_model;
> -
> -static struct fwnode_handle *acpi_gsi_domain_id;
> -
> -/**
> - * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
> - * @gsi: GSI IRQ number to map
> - * @irq: pointer where linux IRQ number is stored
> - *
> - * irq location updated with irq value [>0 on success, 0 on failure]
> - *
> - * Returns: linux IRQ number on success (>0)
> - *  -EINVAL on failure
> - */
> -int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> -{
> -   struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
> -   DOMAIN_BUS_ANY);
> -
> -   *irq = irq_find_mapping(d, gsi);
> -   /*
> -* *irq == 0 means no mapping, that should
> -* be reported as a failure
> -*/
> -   return (*irq > 0) ? *irq : -EINVAL;
> -}
> -EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
> -
> -/**
> - * acpi_register_gsi() - Map a GSI to a linux IRQ number
> - * @dev: device for which IRQ has to be mapped
> - * @gsi: GSI IRQ number
> - * @trigger: trigger type of the GSI number to be mapped
> - * @polarity: polarity of the GSI to be mapped
> - *
> - * Returns: a valid linux IRQ number on success
> - *  -EINVAL on failure
> - */
> -int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
> - int polarity)
> -{
> -   struct irq_fwspec fwspec;
> -
> -   if (WARN_ON(!acpi_gsi_domain_id)) {
> -   pr_warn("GSI: No registered irqchip, giving up\n");
> -   return -EINVAL;
> -   }
> -
> -   fwspec.fwnode = acpi_gsi_domain_id;
> -   fwspec.param[0] = gsi;
> -   fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
> -   fwspec.param_count = 2;
> -
> -   return irq_create_fwspec_mapping();
> -}
> -EXPORT_SYMBOL_GPL(acpi_register_gsi);
> -
> -/**
> - * acpi_unregister_gsi() - Free a GSI<->linux IRQ number mapping
> - * @gsi: GSI IRQ number
> - */
> -void acpi_unregister_gsi(u32 gsi)
> -{
> -   struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
> -   DOMAIN_BUS_ANY);
> -   int irq = irq_find_mapping(d, gsi);
> -
> -   irq_dispose_mapping(irq);
> -}
> -EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
> -
> -/**
> - * acpi_set_irq_model - Setup the GSI irqdomain information
> - * @model: the value assigned to acpi_irq_model
> - * @fwnode: the irq_domain identifier for mapping and looking up
> - *  GSI interrupts
> - */
> -void __init acpi_set_irq_model(enum acpi_irq_model_id model,
> -  struct fwnode_handle *fwnode)
> -{
> -   acpi_irq_model = model;
> -   acpi_gsi_domain_id = fwnode;
> -}
> diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
> new file mode 100644
> index 000..cadc4cdd
> --- /dev/null
> +++ b/drivers/acpi/irq.c
> @@ -0,0 +1,282 @@
> +/*
> + *

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-31 Thread Rafael J. Wysocki 
On Wed, Jan 18, 2017 at 5:46 PM, Agustin Vega-Frias
 wrote:
> ACPI extended IRQ resources may contain a ResourceSource to specify
> an alternate interrupt controller. Introduce acpi_irq_get and use it
> to implement ResourceSource/IRQ domain mapping.
>
> The new API is similar to of_irq_get and allows re-initialization
> of a platform resource from the ACPI extended IRQ resource, and
> provides proper behavior for probe deferral when the domain is not
> yet present when called.
>
> Signed-off-by: Agustin Vega-Frias 
> ---
>  drivers/acpi/Makefile   |   2 +-
>  drivers/acpi/gsi.c  |  98 -
>  drivers/acpi/irq.c  | 282 
> 
>  drivers/base/platform.c |   9 +-
>  include/linux/acpi.h|  10 ++
>  5 files changed, 301 insertions(+), 100 deletions(-)
>  delete mode 100644 drivers/acpi/gsi.c
>  create mode 100644 drivers/acpi/irq.c
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 9ed0878..a391bbc 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -55,7 +55,7 @@ acpi-$(CONFIG_DEBUG_FS)   += debugfs.o
>  acpi-$(CONFIG_ACPI_NUMA)   += numa.o
>  acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
>  acpi-y += acpi_lpat.o
> -acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
> +acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
>  acpi-$(CONFIG_ACPI_WATCHDOG)   += acpi_watchdog.o
>
>  # These are (potentially) separate modules
> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c
> deleted file mode 100644
> index ee9e0f2..000
> --- a/drivers/acpi/gsi.c
> +++ /dev/null
> @@ -1,98 +0,0 @@
> -/*
> - * ACPI GSI IRQ layer
> - *
> - * Copyright (C) 2015 ARM Ltd.
> - * Author: Lorenzo Pieralisi 
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */
> -#include 
> -#include 
> -#include 
> -#include 
> -
> -enum acpi_irq_model_id acpi_irq_model;
> -
> -static struct fwnode_handle *acpi_gsi_domain_id;
> -
> -/**
> - * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
> - * @gsi: GSI IRQ number to map
> - * @irq: pointer where linux IRQ number is stored
> - *
> - * irq location updated with irq value [>0 on success, 0 on failure]
> - *
> - * Returns: linux IRQ number on success (>0)
> - *  -EINVAL on failure
> - */
> -int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
> -{
> -   struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
> -   DOMAIN_BUS_ANY);
> -
> -   *irq = irq_find_mapping(d, gsi);
> -   /*
> -* *irq == 0 means no mapping, that should
> -* be reported as a failure
> -*/
> -   return (*irq > 0) ? *irq : -EINVAL;
> -}
> -EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
> -
> -/**
> - * acpi_register_gsi() - Map a GSI to a linux IRQ number
> - * @dev: device for which IRQ has to be mapped
> - * @gsi: GSI IRQ number
> - * @trigger: trigger type of the GSI number to be mapped
> - * @polarity: polarity of the GSI to be mapped
> - *
> - * Returns: a valid linux IRQ number on success
> - *  -EINVAL on failure
> - */
> -int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
> - int polarity)
> -{
> -   struct irq_fwspec fwspec;
> -
> -   if (WARN_ON(!acpi_gsi_domain_id)) {
> -   pr_warn("GSI: No registered irqchip, giving up\n");
> -   return -EINVAL;
> -   }
> -
> -   fwspec.fwnode = acpi_gsi_domain_id;
> -   fwspec.param[0] = gsi;
> -   fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
> -   fwspec.param_count = 2;
> -
> -   return irq_create_fwspec_mapping();
> -}
> -EXPORT_SYMBOL_GPL(acpi_register_gsi);
> -
> -/**
> - * acpi_unregister_gsi() - Free a GSI<->linux IRQ number mapping
> - * @gsi: GSI IRQ number
> - */
> -void acpi_unregister_gsi(u32 gsi)
> -{
> -   struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
> -   DOMAIN_BUS_ANY);
> -   int irq = irq_find_mapping(d, gsi);
> -
> -   irq_dispose_mapping(irq);
> -}
> -EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
> -
> -/**
> - * acpi_set_irq_model - Setup the GSI irqdomain information
> - * @model: the value assigned to acpi_irq_model
> - * @fwnode: the irq_domain identifier for mapping and looking up
> - *  GSI interrupts
> - */
> -void __init acpi_set_irq_model(enum acpi_irq_model_id model,
> -  struct fwnode_handle *fwnode)
> -{
> -   acpi_irq_model = model;
> -   acpi_gsi_domain_id = fwnode;
> -}
> diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
> new file mode 100644
> index 000..cadc4cdd
> --- /dev/null
> +++ b/drivers/acpi/irq.c
> @@ -0,0 +1,282 @@
> +/*
> + * ACPI GSI IRQ layer
> + *
> + * Copyright (C) 2015 ARM Ltd.
> + * Author:

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-19 Thread Agustin Vega-Frias 

On 2017-01-19 07:36, Lorenzo Pieralisi wrote:

On Wed, Jan 18, 2017 at 09:45:56PM +0200, Andy Shevchenko wrote:

[...]


> +/**
> + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
> + *use it to initialize the given Linux IRQ resource.
> + * @handle ACPI device handle
> + * @index  ACPI IRQ resource index to lookup
> + * @resLinux IRQ resource to initialize

Ah, you missed colons after field names:
* @field1:

> + *
> + * Return:

Next line:  0 on success

> + * -EINVAL if an error occurs
> + * -EPROBE_DEFER if the IRQ lookup/conversion failed
> + */
> +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource 
*res)
> +{

> +   int rc;

Put this last in the definition block.

> +   struct irq_fwspec fwspec;
> +   struct irq_domain *domain;
> +   unsigned long flags;
> +
> +   rc = acpi_irq_parse_one(handle, index, , );
> +   if (rc)
> +   return rc;
> +
> +   domain = irq_find_matching_fwnode(fwspec.fwnode, DOMAIN_BUS_ANY);
> +   if (!domain)
> +   return -EPROBE_DEFER;

Hmm... Could it be other issues here?


That's a good point. Here probing should be deferred only if we know a
driver capable of handling fwspec.fwnode will have a chance to be
probed/initialized eventually right (which basically means it is
compiled in the kernel and we hope it will probed successfully) ? I am
not sure there is an easy way to detect that in ACPI at the moment, I
suspect it is time we added a linker section (or augment the existing
one used for irqchip early MADT parsing - IRQCHIP_ACPI_DECLARE) for 
this
purpose I do not see any other option (apart from leaving devices in 
the

deferred probe list if a driver for the irqchip represented by
fwspec.fwnode is not present in the kernel, which is a bit sloppy, or
resorting to checking Kconfig entries to detect compile time enabled
irqchip drivers).


I'll add the linker section suggested by Lorenzo for the extra check.
We can do that check in acpi_get_irq_source_fwhandle and make the lookup
fail if the device is not listed in the table.

That way we can keep this check as-is.

Thoughts?




> +
> +   rc = irq_create_fwspec_mapping();
> +   if (rc <= 0)
> +   return -EINVAL;
> +

> +   res->start = rc;
> +   res->end = rc;
> +   res->flags = flags;

Perhaps struct resource *r should be a parameter to 
acpi_irq_parse_one().


Yeah but then you would end up with flags initialized in
acpi_irq_parse_one() and the other resource params (ie start, end) 
here,

I do not think it is nicer.


The reason this took this form is that we are trying to get some 
symmetry

with what of_irq_get does.

Thanks,
Agustin



Thanks,
Lorenzo

[...]



With Best Regards,
Andy Shevchenko


--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-19 Thread Agustin Vega-Frias 

On 2017-01-19 07:36, Lorenzo Pieralisi wrote:

On Wed, Jan 18, 2017 at 09:45:56PM +0200, Andy Shevchenko wrote:

[...]


> +/**
> + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
> + *use it to initialize the given Linux IRQ resource.
> + * @handle ACPI device handle
> + * @index  ACPI IRQ resource index to lookup
> + * @resLinux IRQ resource to initialize

Ah, you missed colons after field names:
* @field1:

> + *
> + * Return:

Next line:  0 on success

> + * -EINVAL if an error occurs
> + * -EPROBE_DEFER if the IRQ lookup/conversion failed
> + */
> +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource 
*res)
> +{

> +   int rc;

Put this last in the definition block.

> +   struct irq_fwspec fwspec;
> +   struct irq_domain *domain;
> +   unsigned long flags;
> +
> +   rc = acpi_irq_parse_one(handle, index, , );
> +   if (rc)
> +   return rc;
> +
> +   domain = irq_find_matching_fwnode(fwspec.fwnode, DOMAIN_BUS_ANY);
> +   if (!domain)
> +   return -EPROBE_DEFER;

Hmm... Could it be other issues here?


That's a good point. Here probing should be deferred only if we know a
driver capable of handling fwspec.fwnode will have a chance to be
probed/initialized eventually right (which basically means it is
compiled in the kernel and we hope it will probed successfully) ? I am
not sure there is an easy way to detect that in ACPI at the moment, I
suspect it is time we added a linker section (or augment the existing
one used for irqchip early MADT parsing - IRQCHIP_ACPI_DECLARE) for 
this
purpose I do not see any other option (apart from leaving devices in 
the

deferred probe list if a driver for the irqchip represented by
fwspec.fwnode is not present in the kernel, which is a bit sloppy, or
resorting to checking Kconfig entries to detect compile time enabled
irqchip drivers).


I'll add the linker section suggested by Lorenzo for the extra check.
We can do that check in acpi_get_irq_source_fwhandle and make the lookup
fail if the device is not listed in the table.

That way we can keep this check as-is.

Thoughts?




> +
> +   rc = irq_create_fwspec_mapping();
> +   if (rc <= 0)
> +   return -EINVAL;
> +

> +   res->start = rc;
> +   res->end = rc;
> +   res->flags = flags;

Perhaps struct resource *r should be a parameter to 
acpi_irq_parse_one().


Yeah but then you would end up with flags initialized in
acpi_irq_parse_one() and the other resource params (ie start, end) 
here,

I do not think it is nicer.


The reason this took this form is that we are trying to get some 
symmetry

with what of_irq_get does.

Thanks,
Agustin



Thanks,
Lorenzo

[...]



With Best Regards,
Andy Shevchenko


--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-19 Thread Agustin Vega-Frias 

Hi Andy,

On 2017-01-18 14:45, Andy Shevchenko wrote:

On Wed, Jan 18, 2017 at 9:04 PM, Agustin Vega-Frias
 wrote:

[...]



+ */
+static struct fwnode_handle *
+acpi_get_irq_source_fwhandle(const struct acpi_resource_source 
*source)

+{
+   struct fwnode_handle *result;
+   struct acpi_device *device;
+   acpi_handle handle;
+   acpi_status status;
+
+   if (!source->string_length)
+   return acpi_gsi_domain_id;
+
+   status = acpi_get_handle(NULL, source->string_ptr, );
+   if (ACPI_FAILURE(status)) {


+   pr_warn("Could not find handle for %s\n", 
source->string_ptr);

+   return NULL;
+   }
+
+   device = acpi_bus_get_acpi_device(handle);
+   if (!device) {
+   pr_warn("Could not get device for %s\n", 
source->string_ptr);


I'm not sure both messages have a value.


I'll probably just drop the explicit pr_warns and just use WARN_ON on
the if condition. Is that a reasonable alternative in your view?




+   return NULL;
+   }



[...]


+ */
+static int acpi_irq_parse_one(acpi_handle handle, unsigned int index,
+ struct irq_fwspec *fwspec, unsigned long 
*flags)

+{
+   struct acpi_irq_parse_one_ctx ctx = { -EINVAL, index, flags, 
fwspec };

+   acpi_status status;
+
+   status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+acpi_irq_parse_one_cb, );



+   if (ACPI_FAILURE(status))
+   return -EINVAL;


Shouldn't you have the same in rc? Would be redundant.



Good point, since we always return -EINVAL on failure we can skip
the check since rc will only be updated on success.


+   return ctx.rc;
+}
+


[...]

+   domain = irq_find_matching_fwnode(fwspec.fwnode, 
DOMAIN_BUS_ANY);

+   if (!domain)
+   return -EPROBE_DEFER;


Hmm... Could it be other issues here?


I'll comment on this on Lorenzo's reply.




+
+   rc = irq_create_fwspec_mapping();
+   if (rc <= 0)
+   return -EINVAL;
+



+   res->start = rc;
+   res->end = rc;
+   res->flags = flags;


Perhaps struct resource *r should be a parameter to 
acpi_irq_parse_one().


I'll comment on this on Lorenzo's reply.




+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_irq_get);
+
+/**
  * acpi_set_irq_model - Setup the GSI irqdomain information
  * @model: the value assigned to acpi_irq_model
  * @fwnode: the irq_domain identifier for mapping and looking up


I'll address the other issues on V11.

Thanks for your thorough review,
Agustin



--
With Best Regards,
Andy Shevchenko


--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-19 Thread Agustin Vega-Frias 

Hi Andy,

On 2017-01-18 14:45, Andy Shevchenko wrote:

On Wed, Jan 18, 2017 at 9:04 PM, Agustin Vega-Frias
 wrote:

[...]



+ */
+static struct fwnode_handle *
+acpi_get_irq_source_fwhandle(const struct acpi_resource_source 
*source)

+{
+   struct fwnode_handle *result;
+   struct acpi_device *device;
+   acpi_handle handle;
+   acpi_status status;
+
+   if (!source->string_length)
+   return acpi_gsi_domain_id;
+
+   status = acpi_get_handle(NULL, source->string_ptr, );
+   if (ACPI_FAILURE(status)) {


+   pr_warn("Could not find handle for %s\n", 
source->string_ptr);

+   return NULL;
+   }
+
+   device = acpi_bus_get_acpi_device(handle);
+   if (!device) {
+   pr_warn("Could not get device for %s\n", 
source->string_ptr);


I'm not sure both messages have a value.


I'll probably just drop the explicit pr_warns and just use WARN_ON on
the if condition. Is that a reasonable alternative in your view?




+   return NULL;
+   }



[...]


+ */
+static int acpi_irq_parse_one(acpi_handle handle, unsigned int index,
+ struct irq_fwspec *fwspec, unsigned long 
*flags)

+{
+   struct acpi_irq_parse_one_ctx ctx = { -EINVAL, index, flags, 
fwspec };

+   acpi_status status;
+
+   status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+acpi_irq_parse_one_cb, );



+   if (ACPI_FAILURE(status))
+   return -EINVAL;


Shouldn't you have the same in rc? Would be redundant.



Good point, since we always return -EINVAL on failure we can skip
the check since rc will only be updated on success.


+   return ctx.rc;
+}
+


[...]

+   domain = irq_find_matching_fwnode(fwspec.fwnode, 
DOMAIN_BUS_ANY);

+   if (!domain)
+   return -EPROBE_DEFER;


Hmm... Could it be other issues here?


I'll comment on this on Lorenzo's reply.




+
+   rc = irq_create_fwspec_mapping();
+   if (rc <= 0)
+   return -EINVAL;
+



+   res->start = rc;
+   res->end = rc;
+   res->flags = flags;


Perhaps struct resource *r should be a parameter to 
acpi_irq_parse_one().


I'll comment on this on Lorenzo's reply.




+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_irq_get);
+
+/**
  * acpi_set_irq_model - Setup the GSI irqdomain information
  * @model: the value assigned to acpi_irq_model
  * @fwnode: the irq_domain identifier for mapping and looking up


I'll address the other issues on V11.

Thanks for your thorough review,
Agustin



--
With Best Regards,
Andy Shevchenko


--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-19 Thread Lorenzo Pieralisi 
On Wed, Jan 18, 2017 at 09:45:56PM +0200, Andy Shevchenko wrote:

[...]

> > +/**
> > + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
> > + *use it to initialize the given Linux IRQ resource.
> > + * @handle ACPI device handle
> > + * @index  ACPI IRQ resource index to lookup
> > + * @resLinux IRQ resource to initialize
> 
> Ah, you missed colons after field names:
> * @field1:
> 
> > + *
> > + * Return:
> 
> Next line:  0 on success
> 
> > + * -EINVAL if an error occurs
> > + * -EPROBE_DEFER if the IRQ lookup/conversion failed
> > + */
> > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource 
> > *res)
> > +{
> 
> > +   int rc;
> 
> Put this last in the definition block.
> 
> > +   struct irq_fwspec fwspec;
> > +   struct irq_domain *domain;
> > +   unsigned long flags;
> > +
> > +   rc = acpi_irq_parse_one(handle, index, , );
> > +   if (rc)
> > +   return rc;
> > +
> > +   domain = irq_find_matching_fwnode(fwspec.fwnode, DOMAIN_BUS_ANY);
> > +   if (!domain)
> > +   return -EPROBE_DEFER;
> 
> Hmm... Could it be other issues here?

That's a good point. Here probing should be deferred only if we know a
driver capable of handling fwspec.fwnode will have a chance to be
probed/initialized eventually right (which basically means it is
compiled in the kernel and we hope it will probed successfully) ? I am
not sure there is an easy way to detect that in ACPI at the moment, I
suspect it is time we added a linker section (or augment the existing
one used for irqchip early MADT parsing - IRQCHIP_ACPI_DECLARE) for this
purpose I do not see any other option (apart from leaving devices in the
deferred probe list if a driver for the irqchip represented by
fwspec.fwnode is not present in the kernel, which is a bit sloppy, or
resorting to checking Kconfig entries to detect compile time enabled
irqchip drivers).

> > +
> > +   rc = irq_create_fwspec_mapping();
> > +   if (rc <= 0)
> > +   return -EINVAL;
> > +
> 
> > +   res->start = rc;
> > +   res->end = rc;
> > +   res->flags = flags;
> 
> Perhaps struct resource *r should be a parameter to acpi_irq_parse_one().

Yeah but then you would end up with flags initialized in
acpi_irq_parse_one() and the other resource params (ie start, end) here,
I do not think it is nicer.

Thanks,
Lorenzo

> 
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_irq_get);
> > +
> > +/**
> >   * acpi_set_irq_model - Setup the GSI irqdomain information
> >   * @model: the value assigned to acpi_irq_model
> >   * @fwnode: the irq_domain identifier for mapping and looking up
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index c4af003..61423d2 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, 
> > unsigned int num)
> > }
> >
> > r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> > +   if (r && r->flags & IORESOURCE_DISABLED && 
> > ACPI_COMPANION(>dev)) {
> 
> has_acpi_companion() ?
> 
> > +   int ret;
> > +
> > +   ret = acpi_irq_get(ACPI_HANDLE(>dev), num, r);
> > +   if (ret)
> > +   return ret;
> > +   }
> 
> > @@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
> > memset(>dev.devres_head, 0, 
> > sizeof(pd->dev.devres_head));
> > }
> >  }
> > -
> 
> It doesn't belong here.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-19 Thread Lorenzo Pieralisi 
On Wed, Jan 18, 2017 at 09:45:56PM +0200, Andy Shevchenko wrote:

[...]

> > +/**
> > + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
> > + *use it to initialize the given Linux IRQ resource.
> > + * @handle ACPI device handle
> > + * @index  ACPI IRQ resource index to lookup
> > + * @resLinux IRQ resource to initialize
> 
> Ah, you missed colons after field names:
> * @field1:
> 
> > + *
> > + * Return:
> 
> Next line:  0 on success
> 
> > + * -EINVAL if an error occurs
> > + * -EPROBE_DEFER if the IRQ lookup/conversion failed
> > + */
> > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource 
> > *res)
> > +{
> 
> > +   int rc;
> 
> Put this last in the definition block.
> 
> > +   struct irq_fwspec fwspec;
> > +   struct irq_domain *domain;
> > +   unsigned long flags;
> > +
> > +   rc = acpi_irq_parse_one(handle, index, , );
> > +   if (rc)
> > +   return rc;
> > +
> > +   domain = irq_find_matching_fwnode(fwspec.fwnode, DOMAIN_BUS_ANY);
> > +   if (!domain)
> > +   return -EPROBE_DEFER;
> 
> Hmm... Could it be other issues here?

That's a good point. Here probing should be deferred only if we know a
driver capable of handling fwspec.fwnode will have a chance to be
probed/initialized eventually right (which basically means it is
compiled in the kernel and we hope it will probed successfully) ? I am
not sure there is an easy way to detect that in ACPI at the moment, I
suspect it is time we added a linker section (or augment the existing
one used for irqchip early MADT parsing - IRQCHIP_ACPI_DECLARE) for this
purpose I do not see any other option (apart from leaving devices in the
deferred probe list if a driver for the irqchip represented by
fwspec.fwnode is not present in the kernel, which is a bit sloppy, or
resorting to checking Kconfig entries to detect compile time enabled
irqchip drivers).

> > +
> > +   rc = irq_create_fwspec_mapping();
> > +   if (rc <= 0)
> > +   return -EINVAL;
> > +
> 
> > +   res->start = rc;
> > +   res->end = rc;
> > +   res->flags = flags;
> 
> Perhaps struct resource *r should be a parameter to acpi_irq_parse_one().

Yeah but then you would end up with flags initialized in
acpi_irq_parse_one() and the other resource params (ie start, end) here,
I do not think it is nicer.

Thanks,
Lorenzo

> 
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_irq_get);
> > +
> > +/**
> >   * acpi_set_irq_model - Setup the GSI irqdomain information
> >   * @model: the value assigned to acpi_irq_model
> >   * @fwnode: the irq_domain identifier for mapping and looking up
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index c4af003..61423d2 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, 
> > unsigned int num)
> > }
> >
> > r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> > +   if (r && r->flags & IORESOURCE_DISABLED && 
> > ACPI_COMPANION(>dev)) {
> 
> has_acpi_companion() ?
> 
> > +   int ret;
> > +
> > +   ret = acpi_irq_get(ACPI_HANDLE(>dev), num, r);
> > +   if (ret)
> > +   return ret;
> > +   }
> 
> > @@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
> > memset(>dev.devres_head, 0, 
> > sizeof(pd->dev.devres_head));
> > }
> >  }
> > -
> 
> It doesn't belong here.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-18 Thread Andy Shevchenko 
On Wed, Jan 18, 2017 at 9:04 PM, Agustin Vega-Frias
 wrote:
> ACPI extended IRQ resources may contain a ResourceSource to specify
> an alternate interrupt controller. Introduce acpi_irq_get and use it
> to implement ResourceSource/IRQ domain mapping.
>
> The new API is similar to of_irq_get and allows re-initialization
> of a platform resource from the ACPI extended IRQ resource, and
> provides proper behavior for probe deferral when the domain is not
> yet present when called.

Thanks, looks better now.

>  /**
> + * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given
> + *  acpi_resource_source which is used
> + *  as an IRQ domain id

Please, make short description here and put long one below

> + * @source: acpi_resource_source to use for the lookup
> + *

* Description:
* ...
*

> + * Returns: The appropriate IRQ fwhandle domain id
> + *  NULL on failure

* Return:
* blablabla

> + */
> +static struct fwnode_handle *
> +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
> +{
> +   struct fwnode_handle *result;
> +   struct acpi_device *device;
> +   acpi_handle handle;
> +   acpi_status status;
> +
> +   if (!source->string_length)
> +   return acpi_gsi_domain_id;
> +
> +   status = acpi_get_handle(NULL, source->string_ptr, );
> +   if (ACPI_FAILURE(status)) {

> +   pr_warn("Could not find handle for %s\n", source->string_ptr);
> +   return NULL;
> +   }
> +
> +   device = acpi_bus_get_acpi_device(handle);
> +   if (!device) {
> +   pr_warn("Could not get device for %s\n", source->string_ptr);

I'm not sure both messages have a value.

> +   return NULL;
> +   }

> +/**

You mark as kernel doc, but in fact it's just a comment.

> + * Context for the resource walk used to lookup IRQ resources.
> + */
> +struct acpi_irq_parse_one_ctx {
> +   int rc;
> +   unsigned int index;
> +   unsigned long *res_flags;
> +   struct irq_fwspec *fwspec;
> +};
> +
> +/**
> + * acpi_irq_parse_one_match - Handle a matching IRQ resource

This looks better, but lacks of parameter descriptions.

> + */

> +/**
> + * acpi_irq_parse_one_cb - Handle the given resource
> + * @ares: resource to handle

> + * @context: context for the walk, contains the lookup index and references
> + *   to the flags and fwspec where the result is returned

Make it shorter, ideally into one line

> + *

* Description:

> + * This is called by acpi_walk_resources passing each resource returned by
> + * the _CRS method. We only inspect IRQ resources. Since IRQ resources
> + * might contain multiple interrupts we check if the index is within this
> + * one's interrupt array, otherwise we subtract the current resource IRQ
> + * count from the lookup index to prepare for the next resource.
> + * Once a match is found we call acpi_irq_parse_one_match to populate
> + * the result and end the walk by returning AE_CTRL_TERMINATE.
> + *
> + * Return AE_OK if the walk should continue, AE_CTRL_TERMINATE if a matching

* Return:
* blablabla

> + * IRQ resource was found.
> + */

> +/**
> + * acpi_irq_parse_one - Resolve an interrupt for a device
> + * @handle: the device whose interrupt is to be resolved
> + * @index: index of the interrupt to resolve
> + * @fwspec: structure irq_fwspec filled by this function
> + * @flags: resource flags filled by this function
> + *
> + * This function resolves an interrupt for a device by walking its CRS 
> resources
> + * to find the appropriate ACPI IRQ resource and populating the given 
> structure
> + * which can be used to retrieve a Linux IRQ number.
> + *
> + * Returns the result stored in ctx.rc by the callback, or -EINVAL if the 
> given
> + * index is out of range.

Ditto for Description and Return.

> + */
> +static int acpi_irq_parse_one(acpi_handle handle, unsigned int index,
> + struct irq_fwspec *fwspec, unsigned long *flags)
> +{
> +   struct acpi_irq_parse_one_ctx ctx = { -EINVAL, index, flags, fwspec };
> +   acpi_status status;
> +
> +   status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> +acpi_irq_parse_one_cb, );

> +   if (ACPI_FAILURE(status))
> +   return -EINVAL;

Shouldn't you have the same in rc? Would be redundant.

> +   return ctx.rc;
> +}
> +
> +/**
> + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
> + *use it to initialize the given Linux IRQ resource.
> + * @handle ACPI device handle
> + * @index  ACPI IRQ resource index to lookup
> + * @resLinux IRQ resource to initialize

Ah, you missed colons after field names:
* @field1:

> + *
> + * Return:

Next line:  0 on success

> + * -EINVAL if an error occurs
> + * -EPROBE_DEFER if the IRQ lookup/conversion failed
> + */
>

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-18 Thread Andy Shevchenko 
On Wed, Jan 18, 2017 at 9:04 PM, Agustin Vega-Frias
 wrote:
> ACPI extended IRQ resources may contain a ResourceSource to specify
> an alternate interrupt controller. Introduce acpi_irq_get and use it
> to implement ResourceSource/IRQ domain mapping.
>
> The new API is similar to of_irq_get and allows re-initialization
> of a platform resource from the ACPI extended IRQ resource, and
> provides proper behavior for probe deferral when the domain is not
> yet present when called.

Thanks, looks better now.

>  /**
> + * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given
> + *  acpi_resource_source which is used
> + *  as an IRQ domain id

Please, make short description here and put long one below

> + * @source: acpi_resource_source to use for the lookup
> + *

* Description:
* ...
*

> + * Returns: The appropriate IRQ fwhandle domain id
> + *  NULL on failure

* Return:
* blablabla

> + */
> +static struct fwnode_handle *
> +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
> +{
> +   struct fwnode_handle *result;
> +   struct acpi_device *device;
> +   acpi_handle handle;
> +   acpi_status status;
> +
> +   if (!source->string_length)
> +   return acpi_gsi_domain_id;
> +
> +   status = acpi_get_handle(NULL, source->string_ptr, );
> +   if (ACPI_FAILURE(status)) {

> +   pr_warn("Could not find handle for %s\n", source->string_ptr);
> +   return NULL;
> +   }
> +
> +   device = acpi_bus_get_acpi_device(handle);
> +   if (!device) {
> +   pr_warn("Could not get device for %s\n", source->string_ptr);

I'm not sure both messages have a value.

> +   return NULL;
> +   }

> +/**

You mark as kernel doc, but in fact it's just a comment.

> + * Context for the resource walk used to lookup IRQ resources.
> + */
> +struct acpi_irq_parse_one_ctx {
> +   int rc;
> +   unsigned int index;
> +   unsigned long *res_flags;
> +   struct irq_fwspec *fwspec;
> +};
> +
> +/**
> + * acpi_irq_parse_one_match - Handle a matching IRQ resource

This looks better, but lacks of parameter descriptions.

> + */

> +/**
> + * acpi_irq_parse_one_cb - Handle the given resource
> + * @ares: resource to handle

> + * @context: context for the walk, contains the lookup index and references
> + *   to the flags and fwspec where the result is returned

Make it shorter, ideally into one line

> + *

* Description:

> + * This is called by acpi_walk_resources passing each resource returned by
> + * the _CRS method. We only inspect IRQ resources. Since IRQ resources
> + * might contain multiple interrupts we check if the index is within this
> + * one's interrupt array, otherwise we subtract the current resource IRQ
> + * count from the lookup index to prepare for the next resource.
> + * Once a match is found we call acpi_irq_parse_one_match to populate
> + * the result and end the walk by returning AE_CTRL_TERMINATE.
> + *
> + * Return AE_OK if the walk should continue, AE_CTRL_TERMINATE if a matching

* Return:
* blablabla

> + * IRQ resource was found.
> + */

> +/**
> + * acpi_irq_parse_one - Resolve an interrupt for a device
> + * @handle: the device whose interrupt is to be resolved
> + * @index: index of the interrupt to resolve
> + * @fwspec: structure irq_fwspec filled by this function
> + * @flags: resource flags filled by this function
> + *
> + * This function resolves an interrupt for a device by walking its CRS 
> resources
> + * to find the appropriate ACPI IRQ resource and populating the given 
> structure
> + * which can be used to retrieve a Linux IRQ number.
> + *
> + * Returns the result stored in ctx.rc by the callback, or -EINVAL if the 
> given
> + * index is out of range.

Ditto for Description and Return.

> + */
> +static int acpi_irq_parse_one(acpi_handle handle, unsigned int index,
> + struct irq_fwspec *fwspec, unsigned long *flags)
> +{
> +   struct acpi_irq_parse_one_ctx ctx = { -EINVAL, index, flags, fwspec };
> +   acpi_status status;
> +
> +   status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> +acpi_irq_parse_one_cb, );

> +   if (ACPI_FAILURE(status))
> +   return -EINVAL;

Shouldn't you have the same in rc? Would be redundant.

> +   return ctx.rc;
> +}
> +
> +/**
> + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
> + *use it to initialize the given Linux IRQ resource.
> + * @handle ACPI device handle
> + * @index  ACPI IRQ resource index to lookup
> + * @resLinux IRQ resource to initialize

Ah, you missed colons after field names:
* @field1:

> + *
> + * Return:

Next line:  0 on success

> + * -EINVAL if an error occurs
> + * -EPROBE_DEFER if the IRQ lookup/conversion failed
> + */
> +int

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-18 Thread Agustin Vega-Frias 
ACPI extended IRQ resources may contain a ResourceSource to specify
an alternate interrupt controller. Introduce acpi_irq_get and use it
to implement ResourceSource/IRQ domain mapping.

The new API is similar to of_irq_get and allows re-initialization
of a platform resource from the ACPI extended IRQ resource, and
provides proper behavior for probe deferral when the domain is not
yet present when called.

Signed-off-by: Agustin Vega-Frias 
---
 drivers/acpi/Makefile |   2 +-
 drivers/acpi/{gsi.c => irq.c} | 184 ++
 drivers/base/platform.c   |   9 ++-
 include/linux/acpi.h  |  10 +++
 4 files changed, 203 insertions(+), 2 deletions(-)
 rename drivers/acpi/{gsi.c => irq.c} (32%)

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 9ed0878..a391bbc 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -55,7 +55,7 @@ acpi-$(CONFIG_DEBUG_FS)   += debugfs.o
 acpi-$(CONFIG_ACPI_NUMA)   += numa.o
 acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
 acpi-y += acpi_lpat.o
-acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
+acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
 acpi-$(CONFIG_ACPI_WATCHDOG)   += acpi_watchdog.o

 # These are (potentially) separate modules
diff --git a/drivers/acpi/gsi.c b/drivers/acpi/irq.c
similarity index 32%
rename from drivers/acpi/gsi.c
rename to drivers/acpi/irq.c
index ee9e0f2..cadc4cdd 100644
--- a/drivers/acpi/gsi.c
+++ b/drivers/acpi/irq.c
@@ -85,6 +85,190 @@ void acpi_unregister_gsi(u32 gsi)
 EXPORT_SYMBOL_GPL(acpi_unregister_gsi);

 /**
+ * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given
+ *  acpi_resource_source which is used
+ *  as an IRQ domain id
+ * @source: acpi_resource_source to use for the lookup
+ *
+ * Returns: The appropriate IRQ fwhandle domain id
+ *  NULL on failure
+ */
+static struct fwnode_handle *
+acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
+{
+   struct fwnode_handle *result;
+   struct acpi_device *device;
+   acpi_handle handle;
+   acpi_status status;
+
+   if (!source->string_length)
+   return acpi_gsi_domain_id;
+
+   status = acpi_get_handle(NULL, source->string_ptr, );
+   if (ACPI_FAILURE(status)) {
+   pr_warn("Could not find handle for %s\n", source->string_ptr);
+   return NULL;
+   }
+
+   device = acpi_bus_get_acpi_device(handle);
+   if (!device) {
+   pr_warn("Could not get device for %s\n", source->string_ptr);
+   return NULL;
+   }
+
+   result = >fwnode;
+   acpi_bus_put_acpi_device(device);
+
+   return result;
+}
+
+/**
+ * Context for the resource walk used to lookup IRQ resources.
+ */
+struct acpi_irq_parse_one_ctx {
+   int rc;
+   unsigned int index;
+   unsigned long *res_flags;
+   struct irq_fwspec *fwspec;
+};
+
+/**
+ * acpi_irq_parse_one_match - Handle a matching IRQ resource
+ */
+static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode,
+   u32 hwirq, u8 triggering,
+   u8 polarity, u8 shareable,
+   struct acpi_irq_parse_one_ctx *ctx)
+{
+   ctx->rc = 0;
+   *ctx->res_flags = acpi_dev_irq_flags(triggering, polarity, shareable);
+   ctx->fwspec->fwnode = fwnode;
+   ctx->fwspec->param[0] = hwirq;
+   ctx->fwspec->param[1] = acpi_dev_get_irq_type(triggering, polarity);
+   ctx->fwspec->param_count = 2;
+}
+
+/**
+ * acpi_irq_parse_one_cb - Handle the given resource
+ * @ares: resource to handle
+ * @context: context for the walk, contains the lookup index and references
+ *   to the flags and fwspec where the result is returned
+ *
+ * This is called by acpi_walk_resources passing each resource returned by
+ * the _CRS method. We only inspect IRQ resources. Since IRQ resources
+ * might contain multiple interrupts we check if the index is within this
+ * one's interrupt array, otherwise we subtract the current resource IRQ
+ * count from the lookup index to prepare for the next resource.
+ * Once a match is found we call acpi_irq_parse_one_match to populate
+ * the result and end the walk by returning AE_CTRL_TERMINATE.
+ *
+ * Return AE_OK if the walk should continue, AE_CTRL_TERMINATE if a matching
+ * IRQ resource was found.
+ */
+static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares,
+void *context)
+{
+   struct acpi_irq_parse_one_ctx *ctx = context;
+   struct acpi_resource_irq *irq;
+   struct acpi_resource_extended_irq *eirq;
+   struct fwnode_handle *fwnode;
+
+   switch (ares->type) {
+   case ACPI_RESOURCE_TYPE_IRQ:
+   irq = >data.irq;
+   if (ctx->index >=

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-18 Thread Agustin Vega-Frias 
ACPI extended IRQ resources may contain a ResourceSource to specify
an alternate interrupt controller. Introduce acpi_irq_get and use it
to implement ResourceSource/IRQ domain mapping.

The new API is similar to of_irq_get and allows re-initialization
of a platform resource from the ACPI extended IRQ resource, and
provides proper behavior for probe deferral when the domain is not
yet present when called.

Signed-off-by: Agustin Vega-Frias 
---
 drivers/acpi/Makefile |   2 +-
 drivers/acpi/{gsi.c => irq.c} | 184 ++
 drivers/base/platform.c   |   9 ++-
 include/linux/acpi.h  |  10 +++
 4 files changed, 203 insertions(+), 2 deletions(-)
 rename drivers/acpi/{gsi.c => irq.c} (32%)

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 9ed0878..a391bbc 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -55,7 +55,7 @@ acpi-$(CONFIG_DEBUG_FS)   += debugfs.o
 acpi-$(CONFIG_ACPI_NUMA)   += numa.o
 acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
 acpi-y += acpi_lpat.o
-acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
+acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
 acpi-$(CONFIG_ACPI_WATCHDOG)   += acpi_watchdog.o

 # These are (potentially) separate modules
diff --git a/drivers/acpi/gsi.c b/drivers/acpi/irq.c
similarity index 32%
rename from drivers/acpi/gsi.c
rename to drivers/acpi/irq.c
index ee9e0f2..cadc4cdd 100644
--- a/drivers/acpi/gsi.c
+++ b/drivers/acpi/irq.c
@@ -85,6 +85,190 @@ void acpi_unregister_gsi(u32 gsi)
 EXPORT_SYMBOL_GPL(acpi_unregister_gsi);

 /**
+ * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given
+ *  acpi_resource_source which is used
+ *  as an IRQ domain id
+ * @source: acpi_resource_source to use for the lookup
+ *
+ * Returns: The appropriate IRQ fwhandle domain id
+ *  NULL on failure
+ */
+static struct fwnode_handle *
+acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
+{
+   struct fwnode_handle *result;
+   struct acpi_device *device;
+   acpi_handle handle;
+   acpi_status status;
+
+   if (!source->string_length)
+   return acpi_gsi_domain_id;
+
+   status = acpi_get_handle(NULL, source->string_ptr, );
+   if (ACPI_FAILURE(status)) {
+   pr_warn("Could not find handle for %s\n", source->string_ptr);
+   return NULL;
+   }
+
+   device = acpi_bus_get_acpi_device(handle);
+   if (!device) {
+   pr_warn("Could not get device for %s\n", source->string_ptr);
+   return NULL;
+   }
+
+   result = >fwnode;
+   acpi_bus_put_acpi_device(device);
+
+   return result;
+}
+
+/**
+ * Context for the resource walk used to lookup IRQ resources.
+ */
+struct acpi_irq_parse_one_ctx {
+   int rc;
+   unsigned int index;
+   unsigned long *res_flags;
+   struct irq_fwspec *fwspec;
+};
+
+/**
+ * acpi_irq_parse_one_match - Handle a matching IRQ resource
+ */
+static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode,
+   u32 hwirq, u8 triggering,
+   u8 polarity, u8 shareable,
+   struct acpi_irq_parse_one_ctx *ctx)
+{
+   ctx->rc = 0;
+   *ctx->res_flags = acpi_dev_irq_flags(triggering, polarity, shareable);
+   ctx->fwspec->fwnode = fwnode;
+   ctx->fwspec->param[0] = hwirq;
+   ctx->fwspec->param[1] = acpi_dev_get_irq_type(triggering, polarity);
+   ctx->fwspec->param_count = 2;
+}
+
+/**
+ * acpi_irq_parse_one_cb - Handle the given resource
+ * @ares: resource to handle
+ * @context: context for the walk, contains the lookup index and references
+ *   to the flags and fwspec where the result is returned
+ *
+ * This is called by acpi_walk_resources passing each resource returned by
+ * the _CRS method. We only inspect IRQ resources. Since IRQ resources
+ * might contain multiple interrupts we check if the index is within this
+ * one's interrupt array, otherwise we subtract the current resource IRQ
+ * count from the lookup index to prepare for the next resource.
+ * Once a match is found we call acpi_irq_parse_one_match to populate
+ * the result and end the walk by returning AE_CTRL_TERMINATE.
+ *
+ * Return AE_OK if the walk should continue, AE_CTRL_TERMINATE if a matching
+ * IRQ resource was found.
+ */
+static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares,
+void *context)
+{
+   struct acpi_irq_parse_one_ctx *ctx = context;
+   struct acpi_resource_irq *irq;
+   struct acpi_resource_extended_irq *eirq;
+   struct fwnode_handle *fwnode;
+
+   switch (ares->type) {
+   case ACPI_RESOURCE_TYPE_IRQ:
+   irq = >data.irq;
+   if (ctx->index >= irq->interrupt_count) {
+

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-18 Thread Agustin Vega-Frias 

Hi Andy,

On 2017-01-18 13:00, Andy Shevchenko wrote:

On Wed, Jan 18, 2017 at 6:46 PM, Agustin Vega-Frias
 wrote:

ACPI extended IRQ resources may contain a ResourceSource to specify
an alternate interrupt controller. Introduce acpi_irq_get and use it
to implement ResourceSource/IRQ domain mapping.

The new API is similar to of_irq_get and allows re-initialization
of a platform resource from the ACPI extended IRQ resource, and
provides proper behavior for probe deferral when the domain is not
yet present when called.



Care to use -M -C when prepare this patch?



I did use it, but the similarity index was below the default threshold.
I'll send in a new version of this patch with this fixed.

Thanks,
Agustin


 drivers/acpi/gsi.c  |  98 -
 drivers/acpi/irq.c  | 282 



--
With Best Regards,
Andy Shevchenko


--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-18 Thread Agustin Vega-Frias 

Hi Andy,

On 2017-01-18 13:00, Andy Shevchenko wrote:

On Wed, Jan 18, 2017 at 6:46 PM, Agustin Vega-Frias
 wrote:

ACPI extended IRQ resources may contain a ResourceSource to specify
an alternate interrupt controller. Introduce acpi_irq_get and use it
to implement ResourceSource/IRQ domain mapping.

The new API is similar to of_irq_get and allows re-initialization
of a platform resource from the ACPI extended IRQ resource, and
provides proper behavior for probe deferral when the domain is not
yet present when called.



Care to use -M -C when prepare this patch?



I did use it, but the similarity index was below the default threshold.
I'll send in a new version of this patch with this fixed.

Thanks,
Agustin


 drivers/acpi/gsi.c  |  98 -
 drivers/acpi/irq.c  | 282 



--
With Best Regards,
Andy Shevchenko


--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-18 Thread Andy Shevchenko 
On Wed, Jan 18, 2017 at 6:46 PM, Agustin Vega-Frias
 wrote:
> ACPI extended IRQ resources may contain a ResourceSource to specify
> an alternate interrupt controller. Introduce acpi_irq_get and use it
> to implement ResourceSource/IRQ domain mapping.
>
> The new API is similar to of_irq_get and allows re-initialization
> of a platform resource from the ACPI extended IRQ resource, and
> provides proper behavior for probe deferral when the domain is not
> yet present when called.
>

Care to use -M -C when prepare this patch?

>  drivers/acpi/gsi.c  |  98 -
>  drivers/acpi/irq.c  | 282 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

2017-01-18 Thread Andy Shevchenko 
On Wed, Jan 18, 2017 at 6:46 PM, Agustin Vega-Frias
 wrote:
> ACPI extended IRQ resources may contain a ResourceSource to specify
> an alternate interrupt controller. Introduce acpi_irq_get and use it
> to implement ResourceSource/IRQ domain mapping.
>
> The new API is similar to of_irq_get and allows re-initialization
> of a platform resource from the ACPI extended IRQ resource, and
> provides proper behavior for probe deferral when the domain is not
> yet present when called.
>

Care to use -M -C when prepare this patch?

>  drivers/acpi/gsi.c  |  98 -
>  drivers/acpi/irq.c  | 282 
> 

-- 
With Best Regards,
Andy Shevchenko