On Wed, Dec 23, 2015 at 4:19 PM, Aleksey Makarov
[off-list ref] wrote:

Plural form implies more then one author which makes sense after what
you did to the code.

+ *
+ * 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 <linux/acpi.h>
+#include <linux/amba/bus.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include "internal.h"
+
+static const struct acpi_device_id amba_id_list[] = {
+       {"ARMH0011", 0}, /* PL011 SBSA Uart */
+       {"ARMH0061", 0}, /* PL061 GPIO Device */
+       {"", 0},
+};
+
+static struct clk *amba_dummy_clk;

You may move this into the function, it will be still global, but
hidden from the namespace.

+
+static void amba_register_dummy_clk(void)
+{
+       struct clk *clk;
+
+       /* If clock already registered */
+       if (amba_dummy_clk)
+               return;
+
+       clk = clk_register_fixed_rate(NULL, "apb_pclk", NULL, CLK_IS_ROOT, 0);
+       clk_register_clkdev(clk, "apb_pclk", NULL);
+
+       amba_dummy_clk = clk;
+}
+
+static int amba_handler_attach(struct acpi_device *adev,
+                               const struct acpi_device_id *id)
+{
+       struct amba_device *dev = NULL;

Redundant assignment.

+       struct acpi_device *acpi_parent;
+       struct resource_entry *rentry;
+       struct list_head resource_list;
+       bool address_found = false;
+       int ret, irq_no = 0;

Better to split, they are not about the same by meaning.

+
+       /* If the ACPI node already has a physical device attached, skip it. */
+       if (adev->physical_node_count)
+               return 0;
+
+       dev = amba_device_alloc(NULL, 0, 0);
+       if (!dev) {
+               dev_err(&adev->dev, "%s(): amba_device_alloc() failed\n",
+                       __func__);
+               return -ENOMEM;
+       }
+
+       INIT_LIST_HEAD(&resource_list);
+       ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
+       if (ret < 0)
+               goto err_free;
+
+       list_for_each_entry(rentry, &resource_list, node) {
+               switch (resource_type(rentry->res)) {
+               case IORESOURCE_MEM:
+                       if (!address_found) {
+                               dev->res = *rentry->res;
+                               address_found = true;
+                       }
+                       break;
+               case IORESOURCE_IRQ:
+                       if (irq_no < AMBA_NR_IRQS)
+                               dev->irq[irq_no++] = rentry->res->start;
+                       break;
+               default:
+                       dev_warn(&adev->dev, "Invalid resource\n");

Better to use break here as well.

+               }
+       }
+
+       acpi_dev_free_resource_list(&resource_list);
+
+       /*
+        * If the ACPI node has a parent and that parent has a physical device
+        * attached to it, that physical device should be the parent of the
+        * platform device we are about to create.
+        */
+       dev->dev.parent = NULL;
+       acpi_parent = adev->parent;
+       if (acpi_parent) {
+               struct acpi_device_physical_node *entry;
+               struct list_head *list;
+
+               mutex_lock(&acpi_parent->physical_node_lock);
+               list = &acpi_parent->physical_node_list;
+               if (!list_empty(list)) {
+                       entry = list_first_entry(list,
+                                       struct acpi_device_physical_node,
+                                       node);

There are couple of users already of the similar.
I would suggest for now to create local helper which we might easily
to move to one of drivers/acpi/ header.

static inline struct acpi_device_physical_node
*to_physical_node(struct acpi_dev *adev)
{
  if (list_empty(adev->physical_node_list))
    return NULL;
  return list_first_entry(adev->physical_node_list, struct
acpi_device_physical_node, node);
}

Rafael, what do you think?

+                       dev->dev.parent = entry->dev;
+               }
+               mutex_unlock(&acpi_parent->physical_node_lock);
+       }
+
+       dev_set_name(&dev->dev, "%s", dev_name(&adev->dev));
+       ACPI_COMPANION_SET(&dev->dev, adev);
+
+       ret = amba_device_add(dev, &iomem_resource);
+       if (ret) {
+               dev_err(&adev->dev, "%s(): amba_device_add() failed (%d)\n",
+                      __func__, ret);
+               goto err_free;
+       }
+

Might be worth to comment out what 1 means here.

 void acpi_int340x_thermal_init(void);

Hmm? Looks like we need specific header where drivers can put their
stuff and share with other internal ACPI modules.

+#ifdef CONFIG_ARM_AMBA
+void acpi_amba_init(void);
+#else
+static inline void acpi_amba_init(void) {}
+#endif

Same.

 int acpi_sysfs_init(void);
 void acpi_container_init(void);
 void acpi_memory_hotplug_init(void);
--
2.6.4

-- 
With Best Regards,
Andy Shevchenko