On 05/04/2015 07:04 AM, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Signed-off-by: Fu Wei fu.wei@linaro.org
Like others have said, you need to fix your patch descriptions.
Also, this patch should be 3/6. That is, before patch #4.
arch/arm64/kernel/acpi.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..833d21e 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -23,6 +23,7 @@ #include <linux/irqdomain.h> #include <linux/memblock.h> #include <linux/of_fdt.h> +#include <linux/platform_device.h> #include <linux/smp.h>
#include <asm/cputype.h> @@ -343,3 +344,156 @@ void __init acpi_gic_init(void)
early_acpi_os_unmap_memory((char *)table, tbl_size); }
+static void __init *acpi_gtdt_subtable(struct acpi_table_gtdt *gtdt) +{
- if (!gtdt->platform_timer_count)
return NULL;- return ((void *)gtdt + (gtdt->platform_timer_offset));
+}
+static int __init acpi_gtdt_set_resource_irq_flags(u32 timer_flags) +{
- int flags;
- switch (timer_flags &
(ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY)) {- case (ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY):
flags = IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ;pr_debug("GTDT: sbsa-gwdt irq flags is low edge");break;- case ACPI_GTDT_INTERRUPT_MODE:
flags = IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ;pr_debug("GTDT: sbsa-gwdt irq flags is high edge");break;- case ACPI_GTDT_INTERRUPT_POLARITY:
flags = IORESOURCE_IRQ_LOWLEVEL | IORESOURCE_IRQ;pr_debug("GTDT: sbsa-gwdt irq flags is low level");break;- default:
flags = IORESOURCE_IRQ_HIGHLEVEL | IORESOURCE_IRQ;pr_debug("GTDT: sbsa-gwdt irq flags is high level");- }
- pr_debug("GTDT: sbsa-gwdt irq flags:%d\n", flags);
- return flags;
Blank line before the 'return'
+}
+static int __init acpi_gtdt_import_sbsa_gwdt(struct acpi_gtdt_header *table,
int index)+{
- struct acpi_gtdt_watchdog *watchdog;
- int irq, gsi;
- int err;
- u32 flags;
- void *rf_base_phy, *cf_base_phy;
- struct platform_device *pdev;
- struct resource *res;
- watchdog = container_of(table, struct acpi_gtdt_watchdog, header);
- /* get SBSA Generic Watchdog device information from GTDT */
- rf_base_phy = (void *)watchdog->refresh_frame_address;
- cf_base_phy = (void *)watchdog->control_frame_address;
- irq = watchdog->timer_interrupt;
- flags = watchdog->timer_flags;
- pr_info("GTDT: found a device @ 0x%p/0x%p irq:%d flags:%x\n",
rf_base_phy, cf_base_phy, irq, flags);- if (!rf_base_phy || !cf_base_phy || !irq) {
pr_err("GTDT: fail to get the device info.\n");return -EINVAL;- }
- gsi = map_generic_timer_interrupt(irq, flags);
How about using acpi_register_gsi()? That way, you won't need to export map_generic_timer_interrupt().
- pdev = platform_device_alloc("sbsa-gwdt", index);
- if (!pdev)
return -ENOMEM;- res = kcalloc(3, sizeof(*res), GFP_KERNEL);
- if (!res)
goto err_free_device;- res[0].start = (resource_size_t)rf_base_phy;
- res[0].end = (resource_size_t)(rf_base_phy + SZ_64K - 1);
- res[0].name = "refresh_frame";
- res[0].flags = IORESOURCE_MEM;
- res[1].start = (resource_size_t)cf_base_phy;
- res[1].end = (resource_size_t)(cf_base_phy + SZ_64K - 1);
- res[1].name = "control_frame";
- res[1].flags = IORESOURCE_MEM;
- res[2].start = res[2].end = gsi;
- res[2].name = "ws0";
- res[2].flags = acpi_gtdt_set_resource_irq_flags(flags);
- err = platform_device_add_resources(pdev, res, 3);
- if (err)
goto err_free_res;- err = platform_device_add(pdev);
- if (err)
goto err_free_res;- return 0;
+err_free_res:
- kfree(res);
+err_free_device:
- platform_device_put(pdev);
- return err;
+}
+/* Initialize SBSA generic Watchdog platform device info from GTDT */ +static int __init acpi_gtdt_sbsa_gwdt_init(struct acpi_table_header *table) +{
- struct acpi_table_gtdt *gtdt;
- int i, timer_count, gwdt_count = 0;
- struct acpi_gtdt_header *header;
- void __iomem *gtdt_subtable;
- if (table->revision < 2) {
pr_warn("GTDT: Revision:%d doesn't support SBSA GWDT.\n",table->revision);return -ENODEV;- }
- gtdt = container_of(table, struct acpi_table_gtdt, header);
- timer_count = gtdt->platform_timer_count;
- gtdt_subtable = acpi_gtdt_subtable(gtdt);
Just merge the contents of acpi_gtdt_subtable() into here. No one else will call acpi_gtdt_subtable().
- if (!gtdt_subtable) {
pr_warn("GTDT: No Platform Timer structures!\n");return -EINVAL;- }
- for (i = 0; i < timer_count; i++) {
header = (struct acpi_gtdt_header *)gtdt_subtable;if (header->type == ACPI_GTDT_TYPE_WATCHDOG) {if (acpi_gtdt_import_sbsa_gwdt(header, i))pr_err("GTDT: adding sbsa-gwdt%d ""platform device fail! subtable: %d\n",
Please no exclamation marks in messages. Try to make your messages like normal English. How about this:
pr_err("GTDT: failed trying to create platform sbsa-gwdt%d\n", gwdt_count);
However, I'm not crazy about gwdt_count because you never pass that value to acpi_gtdt_import_sbsa_gwdt(). Instead you pass 'i', which is different value.
gwdt_count, i);gwdt_count++;}gtdt_subtable += header->length;
You should add a safety check on the value of gtdt_subtable to make sure it doesn't go past the end of the buffer. timer_count might be corrupted.
- }
- return 0;
+}
+/* Initialize the SBSA Generic Watchdog presented in GTDT */ +static int __init acpi_gtdt_init(void) +{
- if (acpi_disabled)
return 0;- return acpi_table_parse(ACPI_SIG_GTDT, acpi_gtdt_sbsa_gwdt_init);
+}
I think instead of creating this function, you should move it into arch_timer_acpi_init().