All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] acpi:soc: merge common codes for creating platform device
@ 2014-12-03 12:33 ` Ken Xue
  0 siblings, 0 replies; 6+ messages in thread
From: Ken Xue @ 2014-12-03 12:33 UTC (permalink / raw)
  To: mika.westerberg, rjw; +Cc: linux-acpi, linux-kernel, Ken Xue

This patch is supposed to deliver some common codes for AMD APD and
INTEL LPSS. It can help to convert some specific acpi devices to be
platform devices.

Signed-off-by: Ken Xue <Ken.Xue@amd.com>
---
 drivers/acpi/Makefile   |   2 +-
 drivers/acpi/acpi_soc.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/acpi_soc.h |  90 +++++++++++++++++++++
 3 files changed, 302 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/acpi_soc.c
 create mode 100644 drivers/acpi/acpi_soc.h

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index c3b2fcb..ae3397d 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -40,7 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
 acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o
-acpi-y				+= acpi_lpss.o
+acpi-y				+= acpi_soc.o acpi_lpss.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
 acpi-y				+= int340x_thermal.o
diff --git a/drivers/acpi/acpi_soc.c b/drivers/acpi/acpi_soc.c
new file mode 100644
index 0000000..25089a0
--- /dev/null
+++ b/drivers/acpi/acpi_soc.c
@@ -0,0 +1,211 @@
+/*
+ * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
+ *
+ * Copyright (C) 2015, Intel Corporation & AMD Corporation
+ * Authors: Ken Xue <Ken.Xue@amd.com>
+ *		Mika Westerberg <mika.westerberg@linux.intel.com>
+ *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * 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/err.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/delay.h>
+#include <linux/pm_domain.h>
+
+#include "internal.h"
+#include "acpi_soc.h"
+
+ACPI_MODULE_NAME("acpi_soc");
+
+/* A list for all acpi soc device */
+static LIST_HEAD(a_soc_list);
+
+static int is_memory(struct acpi_resource *res, void *not_used)
+{
+	struct resource r;
+
+	return !acpi_dev_resource_memory(res, &r);
+}
+
+static int acpi_soc_create_device(struct acpi_device *adev,
+				   const struct acpi_device_id *id)
+{
+	struct acpi_soc_dev_desc *dev_desc;
+	struct acpi_soc_dev_private_data *pdata;
+	struct resource_list_entry *rentry;
+	struct list_head resource_list;
+	struct platform_device *pdev;
+	int ret;
+
+	dev_desc = (struct acpi_soc_dev_desc *)id->driver_data;
+	if (!dev_desc) {
+		pdev = acpi_create_platform_device(adev);
+		return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
+	}
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&resource_list);
+	ret = acpi_dev_get_resources(adev, &resource_list, is_memory, NULL);
+	if (ret < 0)
+		goto err_out;
+
+	list_for_each_entry(rentry, &resource_list, node)
+		if (resource_type(&rentry->res) == IORESOURCE_MEM) {
+			if (dev_desc->mem_size_override)
+				pdata->mmio_size = dev_desc->mem_size_override;
+			else
+				pdata->mmio_size = resource_size(&rentry->res);
+			pdata->mmio_base = ioremap(rentry->res.start,
+						   pdata->mmio_size);
+			break;
+		}
+
+	acpi_dev_free_resource_list(&resource_list);
+
+	pdata->adev = adev;
+	pdata->dev_desc = dev_desc;
+
+	if (dev_desc->setup)
+		dev_desc->setup(pdata);
+
+	/*
+	 * This works around a known issue in ACPI tables where acpi soc devices
+	 * have _PS0 and _PS3 without _PSC (and no power resources), so
+	 * acpi_bus_init_power() will assume that the BIOS has put them into D0.
+	 */
+	ret = acpi_device_fix_up_power(adev);
+	if (ret) {
+		/* Skip the device, but continue the namespace scan. */
+		ret = 0;
+		goto err_out;
+	}
+
+	adev->driver_data = pdata;
+	pdev = acpi_create_platform_device(adev);
+	if (!IS_ERR_OR_NULL(pdev))
+		return 1;
+
+	ret = PTR_ERR(pdev);
+	adev->driver_data = NULL;
+
+ err_out:
+	kfree(pdata);
+	return ret;
+}
+
+static int acpi_soc_platform_notify(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	struct platform_device *pdev = to_platform_device(data);
+	struct acpi_soc_dev_private_data *pdata;
+	struct acpi_device *adev;
+	struct acpi_soc *a_soc_entry;
+	const struct acpi_device_id *id = NULL;
+
+	list_for_each_entry(a_soc_entry, &a_soc_list, list) {
+		id = acpi_match_device(a_soc_entry->ids, &pdev->dev);
+		if (!id)
+			break;
+	}
+
+	if (!id || !id->driver_data)
+		return 0;
+
+	if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
+		return 0;
+
+	pdata = acpi_driver_data(adev);
+	if (!pdata || !pdata->mmio_base)
+		return 0;
+
+	switch (action) {
+	case BUS_NOTIFY_BOUND_DRIVER:
+		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
+			if (a_soc_entry->pm_domain)
+				pdev->dev.pm_domain = a_soc_entry->pm_domain;
+			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
+					dev_pm_domain_attach(&pdev->dev, true);
+			else
+					dev_pm_domain_attach(&pdev->dev, false);
+		}
+		break;
+	case BUS_NOTIFY_UNBOUND_DRIVER:
+		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
+			if (a_soc_entry->pm_domain)
+				pdev->dev.pm_domain = a_soc_entry->pm_domain;
+			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
+					dev_pm_domain_detach(&pdev->dev, true);
+			else
+					dev_pm_domain_detach(&pdev->dev, false);
+		}
+		break;
+	case BUS_NOTIFY_ADD_DEVICE:
+		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
+			&& a_soc_entry->pm_domain)
+			return sysfs_create_group(&pdev->dev.kobj,
+						  a_soc_entry->attr_group);
+	case BUS_NOTIFY_DEL_DEVICE:
+		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
+			&& a_soc_entry->pm_domain)
+			sysfs_remove_group(&pdev->dev.kobj,
+			a_soc_entry->attr_group);
+		break;
+	}
+
+	return 0;
+}
+
+static struct notifier_block acpi_soc_nb = {
+	.notifier_call = acpi_soc_platform_notify,
+};
+
+static void acpi_soc_bind(struct device *dev)
+{
+	struct acpi_soc_dev_private_data *pdata;
+
+	pdata = acpi_driver_data(ACPI_COMPANION(dev));
+
+	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->bind)
+		return;
+
+	pdata->dev_desc->bind(pdata);
+}
+
+static void acpi_soc_unbind(struct device *dev)
+{
+	struct acpi_soc_dev_private_data *pdata;
+
+	pdata = acpi_driver_data(ACPI_COMPANION(dev));
+
+	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->unbind)
+		return;
+
+	pdata->dev_desc->unbind(pdata);
+}
+
+void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler)
+{
+	struct acpi_scan_handler *acpi_soc_handler;
+
+	INIT_LIST_HEAD(&a_soc->list);
+	list_add(&a_soc->list, &a_soc_list);
+
+	acpi_soc_handler = kzalloc(sizeof(*acpi_soc_handler), GFP_KERNEL);
+	acpi_soc_handler->ids = a_soc->ids;
+	if (!disable_scan_handler) {
+		acpi_soc_handler->attach = acpi_soc_create_device;
+		acpi_soc_handler->bind = acpi_soc_bind;
+		acpi_soc_handler->unbind = acpi_soc_unbind;
+	}
+	acpi_scan_add_handler(acpi_soc_handler);
+	bus_register_notifier(&platform_bus_type, &acpi_soc_nb);
+}
diff --git a/drivers/acpi/acpi_soc.h b/drivers/acpi/acpi_soc.h
new file mode 100644
index 0000000..cc270a5
--- /dev/null
+++ b/drivers/acpi/acpi_soc.h
@@ -0,0 +1,90 @@
+/*
+ * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
+ *
+ * Copyright (C) 2015, Intel Corporation & AMD Corporation
+ * Authors: Ken Xue <Ken.Xue@amd.com>
+ *		Mika Westerberg <mika.westerberg@linux.intel.com>
+ *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * 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.
+ */
+#ifndef _ACPI_SOC_H
+#define _ACPI_SOC_H
+
+#include <linux/acpi.h>
+#include <linux/clk.h>
+#include <linux/pm.h>
+
+/* Flags */
+#define ACPI_SOC_SYSFS	BIT(0)
+#define ACPI_SOC_PM		BIT(1)
+#define ACPI_SOC_PM_ON	BIT(2)
+
+struct acpi_soc_dev_private_data;
+
+/**
+ * struct acpi_soc - acpi soc
+ * @list: list head
+ * @ids: all acpi device ids for acpi soc
+ * @pm_domain: power domain for all acpi device;can be NULL
+ * @attr_group: attribute group for sysfs support of acpi soc;can be NULL
+ */
+struct acpi_soc {
+	struct list_head list;
+	struct acpi_device_id	*ids;
+	struct dev_pm_domain	*pm_domain;
+	struct attribute_group	*attr_group;
+};
+
+/**
+ * struct acpi_soc_dev_desc - a descriptor for acpi device
+ * @flags: some device feature flags
+ * @clk: clock device
+ * @fixed_clk_rate: fixed rate input clock source for acpi device;
+ *				0 means no fixed rate input clock source
+ * @mem_size_override: a workaround for override device memsize;
+ *				0 means no needs for this WA
+ * @setup: a hook routine to set device resource during create platform device
+ * @bind: a hook of acpi_scan_handler.bind
+ * @unbind: a hook of acpi_scan_handler.unbind
+ *
+ *device description defined as acpi_device_id.driver_data
+ */
+struct acpi_soc_dev_desc {
+	unsigned int flags;
+	struct clk *clk;
+	unsigned int fixed_clk_rate;
+	size_t mem_size_override;
+	int (*setup)(struct acpi_soc_dev_private_data *pdata);
+	void (*bind)(struct acpi_soc_dev_private_data *pdata);
+	void (*unbind)(struct acpi_soc_dev_private_data *pdata);
+};
+
+/**
+ * struct acpi_soc_dev_private_data - acpi device private data
+ * @mmio_base: virtual memory base addr of the device
+ * @mmio_size: device memory size
+ * @dev_desc: device description
+ * @adev: apci device
+ */
+struct acpi_soc_dev_private_data {
+	void __iomem *mmio_base;
+	resource_size_t mmio_size;
+
+	struct acpi_soc_dev_desc *dev_desc;
+	struct acpi_device *adev;
+};
+
+/**
+ * register_acpi_soc - register a new acpi soc
+ * @a_soc: acpi soc
+ * @disable_scan_handler: true means remove default scan handle
+ *			false means use default scan handle
+ *
+ * register a new acpi soc into asoc_list and install default scan handle.
+ */
+void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler);
+
+#endif
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 1/2] acpi:soc: merge common codes for creating platform device
@ 2014-12-03 12:33 ` Ken Xue
  0 siblings, 0 replies; 6+ messages in thread
From: Ken Xue @ 2014-12-03 12:33 UTC (permalink / raw)
  To: mika.westerberg, rjw; +Cc: linux-acpi, linux-kernel, Ken Xue

This patch is supposed to deliver some common codes for AMD APD and
INTEL LPSS. It can help to convert some specific acpi devices to be
platform devices.

Signed-off-by: Ken Xue <Ken.Xue@amd.com>
---
 drivers/acpi/Makefile   |   2 +-
 drivers/acpi/acpi_soc.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/acpi_soc.h |  90 +++++++++++++++++++++
 3 files changed, 302 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/acpi_soc.c
 create mode 100644 drivers/acpi/acpi_soc.h

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index c3b2fcb..ae3397d 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -40,7 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
 acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o
-acpi-y				+= acpi_lpss.o
+acpi-y				+= acpi_soc.o acpi_lpss.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
 acpi-y				+= int340x_thermal.o
diff --git a/drivers/acpi/acpi_soc.c b/drivers/acpi/acpi_soc.c
new file mode 100644
index 0000000..25089a0
--- /dev/null
+++ b/drivers/acpi/acpi_soc.c
@@ -0,0 +1,211 @@
+/*
+ * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
+ *
+ * Copyright (C) 2015, Intel Corporation & AMD Corporation
+ * Authors: Ken Xue <Ken.Xue@amd.com>
+ *		Mika Westerberg <mika.westerberg@linux.intel.com>
+ *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * 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/err.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/delay.h>
+#include <linux/pm_domain.h>
+
+#include "internal.h"
+#include "acpi_soc.h"
+
+ACPI_MODULE_NAME("acpi_soc");
+
+/* A list for all acpi soc device */
+static LIST_HEAD(a_soc_list);
+
+static int is_memory(struct acpi_resource *res, void *not_used)
+{
+	struct resource r;
+
+	return !acpi_dev_resource_memory(res, &r);
+}
+
+static int acpi_soc_create_device(struct acpi_device *adev,
+				   const struct acpi_device_id *id)
+{
+	struct acpi_soc_dev_desc *dev_desc;
+	struct acpi_soc_dev_private_data *pdata;
+	struct resource_list_entry *rentry;
+	struct list_head resource_list;
+	struct platform_device *pdev;
+	int ret;
+
+	dev_desc = (struct acpi_soc_dev_desc *)id->driver_data;
+	if (!dev_desc) {
+		pdev = acpi_create_platform_device(adev);
+		return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
+	}
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&resource_list);
+	ret = acpi_dev_get_resources(adev, &resource_list, is_memory, NULL);
+	if (ret < 0)
+		goto err_out;
+
+	list_for_each_entry(rentry, &resource_list, node)
+		if (resource_type(&rentry->res) == IORESOURCE_MEM) {
+			if (dev_desc->mem_size_override)
+				pdata->mmio_size = dev_desc->mem_size_override;
+			else
+				pdata->mmio_size = resource_size(&rentry->res);
+			pdata->mmio_base = ioremap(rentry->res.start,
+						   pdata->mmio_size);
+			break;
+		}
+
+	acpi_dev_free_resource_list(&resource_list);
+
+	pdata->adev = adev;
+	pdata->dev_desc = dev_desc;
+
+	if (dev_desc->setup)
+		dev_desc->setup(pdata);
+
+	/*
+	 * This works around a known issue in ACPI tables where acpi soc devices
+	 * have _PS0 and _PS3 without _PSC (and no power resources), so
+	 * acpi_bus_init_power() will assume that the BIOS has put them into D0.
+	 */
+	ret = acpi_device_fix_up_power(adev);
+	if (ret) {
+		/* Skip the device, but continue the namespace scan. */
+		ret = 0;
+		goto err_out;
+	}
+
+	adev->driver_data = pdata;
+	pdev = acpi_create_platform_device(adev);
+	if (!IS_ERR_OR_NULL(pdev))
+		return 1;
+
+	ret = PTR_ERR(pdev);
+	adev->driver_data = NULL;
+
+ err_out:
+	kfree(pdata);
+	return ret;
+}
+
+static int acpi_soc_platform_notify(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	struct platform_device *pdev = to_platform_device(data);
+	struct acpi_soc_dev_private_data *pdata;
+	struct acpi_device *adev;
+	struct acpi_soc *a_soc_entry;
+	const struct acpi_device_id *id = NULL;
+
+	list_for_each_entry(a_soc_entry, &a_soc_list, list) {
+		id = acpi_match_device(a_soc_entry->ids, &pdev->dev);
+		if (!id)
+			break;
+	}
+
+	if (!id || !id->driver_data)
+		return 0;
+
+	if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
+		return 0;
+
+	pdata = acpi_driver_data(adev);
+	if (!pdata || !pdata->mmio_base)
+		return 0;
+
+	switch (action) {
+	case BUS_NOTIFY_BOUND_DRIVER:
+		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
+			if (a_soc_entry->pm_domain)
+				pdev->dev.pm_domain = a_soc_entry->pm_domain;
+			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
+					dev_pm_domain_attach(&pdev->dev, true);
+			else
+					dev_pm_domain_attach(&pdev->dev, false);
+		}
+		break;
+	case BUS_NOTIFY_UNBOUND_DRIVER:
+		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
+			if (a_soc_entry->pm_domain)
+				pdev->dev.pm_domain = a_soc_entry->pm_domain;
+			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
+					dev_pm_domain_detach(&pdev->dev, true);
+			else
+					dev_pm_domain_detach(&pdev->dev, false);
+		}
+		break;
+	case BUS_NOTIFY_ADD_DEVICE:
+		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
+			&& a_soc_entry->pm_domain)
+			return sysfs_create_group(&pdev->dev.kobj,
+						  a_soc_entry->attr_group);
+	case BUS_NOTIFY_DEL_DEVICE:
+		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
+			&& a_soc_entry->pm_domain)
+			sysfs_remove_group(&pdev->dev.kobj,
+			a_soc_entry->attr_group);
+		break;
+	}
+
+	return 0;
+}
+
+static struct notifier_block acpi_soc_nb = {
+	.notifier_call = acpi_soc_platform_notify,
+};
+
+static void acpi_soc_bind(struct device *dev)
+{
+	struct acpi_soc_dev_private_data *pdata;
+
+	pdata = acpi_driver_data(ACPI_COMPANION(dev));
+
+	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->bind)
+		return;
+
+	pdata->dev_desc->bind(pdata);
+}
+
+static void acpi_soc_unbind(struct device *dev)
+{
+	struct acpi_soc_dev_private_data *pdata;
+
+	pdata = acpi_driver_data(ACPI_COMPANION(dev));
+
+	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->unbind)
+		return;
+
+	pdata->dev_desc->unbind(pdata);
+}
+
+void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler)
+{
+	struct acpi_scan_handler *acpi_soc_handler;
+
+	INIT_LIST_HEAD(&a_soc->list);
+	list_add(&a_soc->list, &a_soc_list);
+
+	acpi_soc_handler = kzalloc(sizeof(*acpi_soc_handler), GFP_KERNEL);
+	acpi_soc_handler->ids = a_soc->ids;
+	if (!disable_scan_handler) {
+		acpi_soc_handler->attach = acpi_soc_create_device;
+		acpi_soc_handler->bind = acpi_soc_bind;
+		acpi_soc_handler->unbind = acpi_soc_unbind;
+	}
+	acpi_scan_add_handler(acpi_soc_handler);
+	bus_register_notifier(&platform_bus_type, &acpi_soc_nb);
+}
diff --git a/drivers/acpi/acpi_soc.h b/drivers/acpi/acpi_soc.h
new file mode 100644
index 0000000..cc270a5
--- /dev/null
+++ b/drivers/acpi/acpi_soc.h
@@ -0,0 +1,90 @@
+/*
+ * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
+ *
+ * Copyright (C) 2015, Intel Corporation & AMD Corporation
+ * Authors: Ken Xue <Ken.Xue@amd.com>
+ *		Mika Westerberg <mika.westerberg@linux.intel.com>
+ *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * 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.
+ */
+#ifndef _ACPI_SOC_H
+#define _ACPI_SOC_H
+
+#include <linux/acpi.h>
+#include <linux/clk.h>
+#include <linux/pm.h>
+
+/* Flags */
+#define ACPI_SOC_SYSFS	BIT(0)
+#define ACPI_SOC_PM		BIT(1)
+#define ACPI_SOC_PM_ON	BIT(2)
+
+struct acpi_soc_dev_private_data;
+
+/**
+ * struct acpi_soc - acpi soc
+ * @list: list head
+ * @ids: all acpi device ids for acpi soc
+ * @pm_domain: power domain for all acpi device;can be NULL
+ * @attr_group: attribute group for sysfs support of acpi soc;can be NULL
+ */
+struct acpi_soc {
+	struct list_head list;
+	struct acpi_device_id	*ids;
+	struct dev_pm_domain	*pm_domain;
+	struct attribute_group	*attr_group;
+};
+
+/**
+ * struct acpi_soc_dev_desc - a descriptor for acpi device
+ * @flags: some device feature flags
+ * @clk: clock device
+ * @fixed_clk_rate: fixed rate input clock source for acpi device;
+ *				0 means no fixed rate input clock source
+ * @mem_size_override: a workaround for override device memsize;
+ *				0 means no needs for this WA
+ * @setup: a hook routine to set device resource during create platform device
+ * @bind: a hook of acpi_scan_handler.bind
+ * @unbind: a hook of acpi_scan_handler.unbind
+ *
+ *device description defined as acpi_device_id.driver_data
+ */
+struct acpi_soc_dev_desc {
+	unsigned int flags;
+	struct clk *clk;
+	unsigned int fixed_clk_rate;
+	size_t mem_size_override;
+	int (*setup)(struct acpi_soc_dev_private_data *pdata);
+	void (*bind)(struct acpi_soc_dev_private_data *pdata);
+	void (*unbind)(struct acpi_soc_dev_private_data *pdata);
+};
+
+/**
+ * struct acpi_soc_dev_private_data - acpi device private data
+ * @mmio_base: virtual memory base addr of the device
+ * @mmio_size: device memory size
+ * @dev_desc: device description
+ * @adev: apci device
+ */
+struct acpi_soc_dev_private_data {
+	void __iomem *mmio_base;
+	resource_size_t mmio_size;
+
+	struct acpi_soc_dev_desc *dev_desc;
+	struct acpi_device *adev;
+};
+
+/**
+ * register_acpi_soc - register a new acpi soc
+ * @a_soc: acpi soc
+ * @disable_scan_handler: true means remove default scan handle
+ *			false means use default scan handle
+ *
+ * register a new acpi soc into asoc_list and install default scan handle.
+ */
+void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler);
+
+#endif
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] acpi:soc: merge common codes for creating platform device
  2014-12-03 12:33 ` Ken Xue
  (?)
@ 2014-12-04 13:04 ` Mika Westerberg
  2014-12-05  6:34     ` Ken Xue
  -1 siblings, 1 reply; 6+ messages in thread
From: Mika Westerberg @ 2014-12-04 13:04 UTC (permalink / raw)
  To: Ken Xue; +Cc: rjw, linux-acpi, linux-kernel

On Wed, Dec 03, 2014 at 08:33:37PM +0800, Ken Xue wrote:
> This patch is supposed to deliver some common codes for AMD APD and
> INTEL LPSS. It can help to convert some specific acpi devices to be
> platform devices.
> 
> Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> ---
>  drivers/acpi/Makefile   |   2 +-
>  drivers/acpi/acpi_soc.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/acpi_soc.h |  90 +++++++++++++++++++++
>  3 files changed, 302 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/acpi/acpi_soc.c
>  create mode 100644 drivers/acpi/acpi_soc.h
> 
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index c3b2fcb..ae3397d 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -40,7 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
>  acpi-y				+= ec.o
>  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
>  acpi-y				+= pci_root.o pci_link.o pci_irq.o
> -acpi-y				+= acpi_lpss.o
> +acpi-y				+= acpi_soc.o acpi_lpss.o

I think you need to convert acpi_lpss.c to use this new acpi_soc.c.

>  acpi-y				+= acpi_platform.o
>  acpi-y				+= acpi_pnp.o
>  acpi-y				+= int340x_thermal.o
> diff --git a/drivers/acpi/acpi_soc.c b/drivers/acpi/acpi_soc.c
> new file mode 100644
> index 0000000..25089a0
> --- /dev/null
> +++ b/drivers/acpi/acpi_soc.c
> @@ -0,0 +1,211 @@
> +/*
> + * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
> + *
> + * Copyright (C) 2015, Intel Corporation & AMD Corporation
> + * Authors: Ken Xue <Ken.Xue@amd.com>
> + *		Mika Westerberg <mika.westerberg@linux.intel.com>
> + *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> + *
> + * 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/err.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/delay.h>
> +#include <linux/pm_domain.h>
> +
> +#include "internal.h"
> +#include "acpi_soc.h"
> +
> +ACPI_MODULE_NAME("acpi_soc");
> +
> +/* A list for all acpi soc device */
> +static LIST_HEAD(a_soc_list);
> +
> +static int is_memory(struct acpi_resource *res, void *not_used)
> +{
> +	struct resource r;
> +
> +	return !acpi_dev_resource_memory(res, &r);
> +}
> +
> +static int acpi_soc_create_device(struct acpi_device *adev,
> +				   const struct acpi_device_id *id)
> +{
> +	struct acpi_soc_dev_desc *dev_desc;
> +	struct acpi_soc_dev_private_data *pdata;
> +	struct resource_list_entry *rentry;
> +	struct list_head resource_list;
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	dev_desc = (struct acpi_soc_dev_desc *)id->driver_data;
> +	if (!dev_desc) {
> +		pdev = acpi_create_platform_device(adev);
> +		return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
> +	}
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&resource_list);
> +	ret = acpi_dev_get_resources(adev, &resource_list, is_memory, NULL);
> +	if (ret < 0)
> +		goto err_out;
> +
> +	list_for_each_entry(rentry, &resource_list, node)
> +		if (resource_type(&rentry->res) == IORESOURCE_MEM) {
> +			if (dev_desc->mem_size_override)
> +				pdata->mmio_size = dev_desc->mem_size_override;
> +			else
> +				pdata->mmio_size = resource_size(&rentry->res);
> +			pdata->mmio_base = ioremap(rentry->res.start,
> +						   pdata->mmio_size);
> +			break;
> +		}
> +
> +	acpi_dev_free_resource_list(&resource_list);
> +
> +	pdata->adev = adev;
> +	pdata->dev_desc = dev_desc;
> +
> +	if (dev_desc->setup)
> +		dev_desc->setup(pdata);
> +
> +	/*
> +	 * This works around a known issue in ACPI tables where acpi soc devices
> +	 * have _PS0 and _PS3 without _PSC (and no power resources), so
> +	 * acpi_bus_init_power() will assume that the BIOS has put them into D0.
> +	 */
> +	ret = acpi_device_fix_up_power(adev);
> +	if (ret) {
> +		/* Skip the device, but continue the namespace scan. */
> +		ret = 0;
> +		goto err_out;
> +	}
> +
> +	adev->driver_data = pdata;
> +	pdev = acpi_create_platform_device(adev);
> +	if (!IS_ERR_OR_NULL(pdev))
> +		return 1;
> +
> +	ret = PTR_ERR(pdev);
> +	adev->driver_data = NULL;
> +
> + err_out:
> +	kfree(pdata);
> +	return ret;
> +}
> +
> +static int acpi_soc_platform_notify(struct notifier_block *nb,
> +				     unsigned long action, void *data)
> +{
> +	struct platform_device *pdev = to_platform_device(data);
> +	struct acpi_soc_dev_private_data *pdata;
> +	struct acpi_device *adev;
> +	struct acpi_soc *a_soc_entry;
> +	const struct acpi_device_id *id = NULL;
> +
> +	list_for_each_entry(a_soc_entry, &a_soc_list, list) {
> +		id = acpi_match_device(a_soc_entry->ids, &pdev->dev);
> +		if (!id)
> +			break;
> +	}
> +
> +	if (!id || !id->driver_data)
> +		return 0;
> +
> +	if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
> +		return 0;
> +
> +	pdata = acpi_driver_data(adev);
> +	if (!pdata || !pdata->mmio_base)
> +		return 0;
> +
> +	switch (action) {
> +	case BUS_NOTIFY_BOUND_DRIVER:
> +		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
> +			if (a_soc_entry->pm_domain)
> +				pdev->dev.pm_domain = a_soc_entry->pm_domain;
> +			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
> +					dev_pm_domain_attach(&pdev->dev, true);
> +			else
> +					dev_pm_domain_attach(&pdev->dev, false);
> +		}
> +		break;
> +	case BUS_NOTIFY_UNBOUND_DRIVER:
> +		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
> +			if (a_soc_entry->pm_domain)
> +				pdev->dev.pm_domain = a_soc_entry->pm_domain;
> +			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
> +					dev_pm_domain_detach(&pdev->dev, true);
> +			else
> +					dev_pm_domain_detach(&pdev->dev, false);
> +		}
> +		break;
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
> +			&& a_soc_entry->pm_domain)
> +			return sysfs_create_group(&pdev->dev.kobj,
> +						  a_soc_entry->attr_group);
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
> +			&& a_soc_entry->pm_domain)
> +			sysfs_remove_group(&pdev->dev.kobj,
> +			a_soc_entry->attr_group);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct notifier_block acpi_soc_nb = {
> +	.notifier_call = acpi_soc_platform_notify,
> +};
> +
> +static void acpi_soc_bind(struct device *dev)
> +{
> +	struct acpi_soc_dev_private_data *pdata;
> +
> +	pdata = acpi_driver_data(ACPI_COMPANION(dev));
> +
> +	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->bind)
> +		return;
> +
> +	pdata->dev_desc->bind(pdata);
> +}
> +
> +static void acpi_soc_unbind(struct device *dev)
> +{
> +	struct acpi_soc_dev_private_data *pdata;
> +
> +	pdata = acpi_driver_data(ACPI_COMPANION(dev));
> +
> +	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->unbind)
> +		return;
> +
> +	pdata->dev_desc->unbind(pdata);
> +}
> +
> +void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler)

Would it work better if we pass acpi_scan_handler as a parameter to this
function?

Then if it is NULL, it means that it will not register scan handler at
all.

And then provide the default handler in acpi_soc.h so that you can do

	register_acpi_soc(&soc, default_acpi_scan_handler);

or

	register_acpi_soc(&soc, NULL);

depending the case. But also support passing custom scan handler.

> +{
> +	struct acpi_scan_handler *acpi_soc_handler;
> +
> +	INIT_LIST_HEAD(&a_soc->list);
> +	list_add(&a_soc->list, &a_soc_list);
> +
> +	acpi_soc_handler = kzalloc(sizeof(*acpi_soc_handler), GFP_KERNEL);
> +	acpi_soc_handler->ids = a_soc->ids;
> +	if (!disable_scan_handler) {
> +		acpi_soc_handler->attach = acpi_soc_create_device;
> +		acpi_soc_handler->bind = acpi_soc_bind;
> +		acpi_soc_handler->unbind = acpi_soc_unbind;
> +	}
> +	acpi_scan_add_handler(acpi_soc_handler);
> +	bus_register_notifier(&platform_bus_type, &acpi_soc_nb);
> +}
> diff --git a/drivers/acpi/acpi_soc.h b/drivers/acpi/acpi_soc.h
> new file mode 100644
> index 0000000..cc270a5
> --- /dev/null
> +++ b/drivers/acpi/acpi_soc.h
> @@ -0,0 +1,90 @@
> +/*
> + * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
> + *
> + * Copyright (C) 2015, Intel Corporation & AMD Corporation
> + * Authors: Ken Xue <Ken.Xue@amd.com>
> + *		Mika Westerberg <mika.westerberg@linux.intel.com>
> + *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> + *
> + * 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.
> + */
> +#ifndef _ACPI_SOC_H
> +#define _ACPI_SOC_H
> +
> +#include <linux/acpi.h>
> +#include <linux/clk.h>
> +#include <linux/pm.h>
> +
> +/* Flags */
> +#define ACPI_SOC_SYSFS	BIT(0)
> +#define ACPI_SOC_PM		BIT(1)
> +#define ACPI_SOC_PM_ON	BIT(2)

Would be nice to document what those mean.

> +
> +struct acpi_soc_dev_private_data;
> +
> +/**
> + * struct acpi_soc - acpi soc
> + * @list: list head
> + * @ids: all acpi device ids for acpi soc
> + * @pm_domain: power domain for all acpi device;can be NULL
> + * @attr_group: attribute group for sysfs support of acpi soc;can be NULL
> + */
> +struct acpi_soc {
> +	struct list_head list;
> +	struct acpi_device_id	*ids;
> +	struct dev_pm_domain	*pm_domain;
> +	struct attribute_group	*attr_group;

These are tab aligned but list is using spaces. Please be consistent.

> +};
> +
> +/**
> + * struct acpi_soc_dev_desc - a descriptor for acpi device
> + * @flags: some device feature flags

Please a bit better documentation than "some device feature flags".

> + * @clk: clock device
> + * @fixed_clk_rate: fixed rate input clock source for acpi device;
> + *				0 means no fixed rate input clock source
> + * @mem_size_override: a workaround for override device memsize;
> + *				0 means no needs for this WA
> + * @setup: a hook routine to set device resource during create platform device
> + * @bind: a hook of acpi_scan_handler.bind
> + * @unbind: a hook of acpi_scan_handler.unbind
> + *
> + *device description defined as acpi_device_id.driver_data

Missing space.

> + */
> +struct acpi_soc_dev_desc {
> +	unsigned int flags;
> +	struct clk *clk;
> +	unsigned int fixed_clk_rate;
> +	size_t mem_size_override;
> +	int (*setup)(struct acpi_soc_dev_private_data *pdata);
> +	void (*bind)(struct acpi_soc_dev_private_data *pdata);
> +	void (*unbind)(struct acpi_soc_dev_private_data *pdata);
> +};
> +
> +/**
> + * struct acpi_soc_dev_private_data - acpi device private data
> + * @mmio_base: virtual memory base addr of the device
> + * @mmio_size: device memory size
> + * @dev_desc: device description
> + * @adev: apci device

apci -> acpi

> + */
> +struct acpi_soc_dev_private_data {
> +	void __iomem *mmio_base;
> +	resource_size_t mmio_size;
> +
> +	struct acpi_soc_dev_desc *dev_desc;
> +	struct acpi_device *adev;
> +};
> +
> +/**
> + * register_acpi_soc - register a new acpi soc
> + * @a_soc: acpi soc
> + * @disable_scan_handler: true means remove default scan handle
> + *			false means use default scan handle
> + *
> + * register a new acpi soc into asoc_list and install default scan handle.
> + */

IMHO function kernel-docs belong close to the function implementation,
e.g in this case to acpi_soc.c.

> +void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler);
> +
> +#endif
> -- 
> 1.9.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] acpi:soc: merge common codes for creating platform device
  2014-12-04 13:04 ` Mika Westerberg
@ 2014-12-05  6:34     ` Ken Xue
  0 siblings, 0 replies; 6+ messages in thread
From: Ken Xue @ 2014-12-05  6:34 UTC (permalink / raw)
  To: Mika Westerberg; +Cc: rjw, linux-acpi, linux-kernel

On Thu, 2014-12-04 at 15:04 +0200, Mika Westerberg wrote:
> On Wed, Dec 03, 2014 at 08:33:37PM +0800, Ken Xue wrote:
> > This patch is supposed to deliver some common codes for AMD APD and
> > INTEL LPSS. It can help to convert some specific acpi devices to be
> > platform devices.
> > 
> > Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> > ---
> >  drivers/acpi/Makefile   |   2 +-
> >  drivers/acpi/acpi_soc.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/acpi/acpi_soc.h |  90 +++++++++++++++++++++
> >  3 files changed, 302 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/acpi/acpi_soc.c
> >  create mode 100644 drivers/acpi/acpi_soc.h
> > 
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index c3b2fcb..ae3397d 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -40,7 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
> >  acpi-y				+= ec.o
> >  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
> >  acpi-y				+= pci_root.o pci_link.o pci_irq.o
> > -acpi-y				+= acpi_lpss.o
> > +acpi-y				+= acpi_soc.o acpi_lpss.o
> 
> I think you need to convert acpi_lpss.c to use this new acpi_soc.c.
> 
[ken] I would like to merge lpss. But you know that I am lack of INTEL
platforms to verify codes before submit. And i am a little worried about
i may not cover so much rich features of lpss well. so, can we merge
lpss after current patch? or what's your plan?

> >  acpi-y				+= acpi_platform.o
> >  acpi-y				+= acpi_pnp.o
> >  acpi-y				+= int340x_thermal.o
> > diff --git a/drivers/acpi/acpi_soc.c b/drivers/acpi/acpi_soc.c
> > new file mode 100644
> > index 0000000..25089a0
> > --- /dev/null
> > +++ b/drivers/acpi/acpi_soc.c
> > @@ -0,0 +1,211 @@
> > +/*
> > + * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
> > + *
> > + * Copyright (C) 2015, Intel Corporation & AMD Corporation
> > + * Authors: Ken Xue <Ken.Xue@amd.com>
> > + *		Mika Westerberg <mika.westerberg@linux.intel.com>
> > + *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > + *
> > + * 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/err.h>
> > +#include <linux/io.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/delay.h>
> > +#include <linux/pm_domain.h>
> > +
> > +#include "internal.h"
> > +#include "acpi_soc.h"
> > +
> > +ACPI_MODULE_NAME("acpi_soc");
> > +
> > +/* A list for all acpi soc device */
> > +static LIST_HEAD(a_soc_list);
> > +
> > +static int is_memory(struct acpi_resource *res, void *not_used)
> > +{
> > +	struct resource r;
> > +
> > +	return !acpi_dev_resource_memory(res, &r);
> > +}
> > +
> > +static int acpi_soc_create_device(struct acpi_device *adev,
> > +				   const struct acpi_device_id *id)
> > +{
> > +	struct acpi_soc_dev_desc *dev_desc;
> > +	struct acpi_soc_dev_private_data *pdata;
> > +	struct resource_list_entry *rentry;
> > +	struct list_head resource_list;
> > +	struct platform_device *pdev;
> > +	int ret;
> > +
> > +	dev_desc = (struct acpi_soc_dev_desc *)id->driver_data;
> > +	if (!dev_desc) {
> > +		pdev = acpi_create_platform_device(adev);
> > +		return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
> > +	}
> > +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > +	if (!pdata)
> > +		return -ENOMEM;
> > +
> > +	INIT_LIST_HEAD(&resource_list);
> > +	ret = acpi_dev_get_resources(adev, &resource_list, is_memory, NULL);
> > +	if (ret < 0)
> > +		goto err_out;
> > +
> > +	list_for_each_entry(rentry, &resource_list, node)
> > +		if (resource_type(&rentry->res) == IORESOURCE_MEM) {
> > +			if (dev_desc->mem_size_override)
> > +				pdata->mmio_size = dev_desc->mem_size_override;
> > +			else
> > +				pdata->mmio_size = resource_size(&rentry->res);
> > +			pdata->mmio_base = ioremap(rentry->res.start,
> > +						   pdata->mmio_size);
> > +			break;
> > +		}
> > +
> > +	acpi_dev_free_resource_list(&resource_list);
> > +
> > +	pdata->adev = adev;
> > +	pdata->dev_desc = dev_desc;
> > +
> > +	if (dev_desc->setup)
> > +		dev_desc->setup(pdata);
> > +
> > +	/*
> > +	 * This works around a known issue in ACPI tables where acpi soc devices
> > +	 * have _PS0 and _PS3 without _PSC (and no power resources), so
> > +	 * acpi_bus_init_power() will assume that the BIOS has put them into D0.
> > +	 */
> > +	ret = acpi_device_fix_up_power(adev);
> > +	if (ret) {
> > +		/* Skip the device, but continue the namespace scan. */
> > +		ret = 0;
> > +		goto err_out;
> > +	}
> > +
> > +	adev->driver_data = pdata;
> > +	pdev = acpi_create_platform_device(adev);
> > +	if (!IS_ERR_OR_NULL(pdev))
> > +		return 1;
> > +
> > +	ret = PTR_ERR(pdev);
> > +	adev->driver_data = NULL;
> > +
> > + err_out:
> > +	kfree(pdata);
> > +	return ret;
> > +}
> > +
> > +static int acpi_soc_platform_notify(struct notifier_block *nb,
> > +				     unsigned long action, void *data)
> > +{
> > +	struct platform_device *pdev = to_platform_device(data);
> > +	struct acpi_soc_dev_private_data *pdata;
> > +	struct acpi_device *adev;
> > +	struct acpi_soc *a_soc_entry;
> > +	const struct acpi_device_id *id = NULL;
> > +
> > +	list_for_each_entry(a_soc_entry, &a_soc_list, list) {
> > +		id = acpi_match_device(a_soc_entry->ids, &pdev->dev);
> > +		if (!id)
> > +			break;
> > +	}
> > +
> > +	if (!id || !id->driver_data)
> > +		return 0;
> > +
> > +	if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
> > +		return 0;
> > +
> > +	pdata = acpi_driver_data(adev);
> > +	if (!pdata || !pdata->mmio_base)
> > +		return 0;
> > +
> > +	switch (action) {
> > +	case BUS_NOTIFY_BOUND_DRIVER:
> > +		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
> > +			if (a_soc_entry->pm_domain)
> > +				pdev->dev.pm_domain = a_soc_entry->pm_domain;
> > +			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
> > +					dev_pm_domain_attach(&pdev->dev, true);
> > +			else
> > +					dev_pm_domain_attach(&pdev->dev, false);
> > +		}
> > +		break;
> > +	case BUS_NOTIFY_UNBOUND_DRIVER:
> > +		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
> > +			if (a_soc_entry->pm_domain)
> > +				pdev->dev.pm_domain = a_soc_entry->pm_domain;
> > +			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
> > +					dev_pm_domain_detach(&pdev->dev, true);
> > +			else
> > +					dev_pm_domain_detach(&pdev->dev, false);
> > +		}
> > +		break;
> > +	case BUS_NOTIFY_ADD_DEVICE:
> > +		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
> > +			&& a_soc_entry->pm_domain)
> > +			return sysfs_create_group(&pdev->dev.kobj,
> > +						  a_soc_entry->attr_group);
> > +	case BUS_NOTIFY_DEL_DEVICE:
> > +		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
> > +			&& a_soc_entry->pm_domain)
> > +			sysfs_remove_group(&pdev->dev.kobj,
> > +			a_soc_entry->attr_group);
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct notifier_block acpi_soc_nb = {
> > +	.notifier_call = acpi_soc_platform_notify,
> > +};
> > +
> > +static void acpi_soc_bind(struct device *dev)
> > +{
> > +	struct acpi_soc_dev_private_data *pdata;
> > +
> > +	pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > +
> > +	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->bind)
> > +		return;
> > +
> > +	pdata->dev_desc->bind(pdata);
> > +}
> > +
> > +static void acpi_soc_unbind(struct device *dev)
> > +{
> > +	struct acpi_soc_dev_private_data *pdata;
> > +
> > +	pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > +
> > +	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->unbind)
> > +		return;
> > +
> > +	pdata->dev_desc->unbind(pdata);
> > +}
> > +
> > +void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler)
> 
> Would it work better if we pass acpi_scan_handler as a parameter to this
> function?
> 
> Then if it is NULL, it means that it will not register scan handler at
> all.
> 
> And then provide the default handler in acpi_soc.h so that you can do
> 
> 	register_acpi_soc(&soc, default_acpi_scan_handler);
> 
> or
> 
> 	register_acpi_soc(&soc, NULL);
> 
> depending the case. But also support passing custom scan handler.
> 
[Ken] I add some hooks for acpi_scan_handler in acpi_soc_dev_desc like
"setup" "bind" "unbind" in case of different implementation of platform.
your approach is other good way to same target. Right?

> > +{
> > +	struct acpi_scan_handler *acpi_soc_handler;
> > +
> > +	INIT_LIST_HEAD(&a_soc->list);
> > +	list_add(&a_soc->list, &a_soc_list);
> > +
> > +	acpi_soc_handler = kzalloc(sizeof(*acpi_soc_handler), GFP_KERNEL);
> > +	acpi_soc_handler->ids = a_soc->ids;
> > +	if (!disable_scan_handler) {
> > +		acpi_soc_handler->attach = acpi_soc_create_device;
> > +		acpi_soc_handler->bind = acpi_soc_bind;
> > +		acpi_soc_handler->unbind = acpi_soc_unbind;
> > +	}
> > +	acpi_scan_add_handler(acpi_soc_handler);
> > +	bus_register_notifier(&platform_bus_type, &acpi_soc_nb);
> > +}
> > diff --git a/drivers/acpi/acpi_soc.h b/drivers/acpi/acpi_soc.h
> > new file mode 100644
> > index 0000000..cc270a5
> > --- /dev/null
> > +++ b/drivers/acpi/acpi_soc.h
> > @@ -0,0 +1,90 @@
> > +/*
> > + * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
> > + *
> > + * Copyright (C) 2015, Intel Corporation & AMD Corporation
> > + * Authors: Ken Xue <Ken.Xue@amd.com>
> > + *		Mika Westerberg <mika.westerberg@linux.intel.com>
> > + *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > + *
> > + * 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.
> > + */
> > +#ifndef _ACPI_SOC_H
> > +#define _ACPI_SOC_H
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/clk.h>
> > +#include <linux/pm.h>
> > +
> > +/* Flags */
> > +#define ACPI_SOC_SYSFS	BIT(0)
> > +#define ACPI_SOC_PM		BIT(1)
> > +#define ACPI_SOC_PM_ON	BIT(2)
> 
> Would be nice to document what those mean.
> 
[Ken] sure. i will add more description.

> > +
> > +struct acpi_soc_dev_private_data;
> > +
> > +/**
> > + * struct acpi_soc - acpi soc
> > + * @list: list head
> > + * @ids: all acpi device ids for acpi soc
> > + * @pm_domain: power domain for all acpi device;can be NULL
> > + * @attr_group: attribute group for sysfs support of acpi soc;can be NULL
> > + */
> > +struct acpi_soc {
> > +	struct list_head list;
> > +	struct acpi_device_id	*ids;
> > +	struct dev_pm_domain	*pm_domain;
> > +	struct attribute_group	*attr_group;
> 
> These are tab aligned but list is using spaces. Please be consistent.
> 
[Ken] got it. 

> > +};
> > +
> > +/**
> > + * struct acpi_soc_dev_desc - a descriptor for acpi device
> > + * @flags: some device feature flags
> 
[Ken] ok.

> Please a bit better documentation than "some device feature flags".
> 
> > + * @clk: clock device
> > + * @fixed_clk_rate: fixed rate input clock source for acpi device;
> > + *				0 means no fixed rate input clock source
> > + * @mem_size_override: a workaround for override device memsize;
> > + *				0 means no needs for this WA
> > + * @setup: a hook routine to set device resource during create platform device
> > + * @bind: a hook of acpi_scan_handler.bind
> > + * @unbind: a hook of acpi_scan_handler.unbind
> > + *
> > + *device description defined as acpi_device_id.driver_data
> 
> Missing space.
> 
[Ken] got it. 

> > + */
> > +struct acpi_soc_dev_desc {
> > +	unsigned int flags;
> > +	struct clk *clk;
> > +	unsigned int fixed_clk_rate;
> > +	size_t mem_size_override;
> > +	int (*setup)(struct acpi_soc_dev_private_data *pdata);
> > +	void (*bind)(struct acpi_soc_dev_private_data *pdata);
> > +	void (*unbind)(struct acpi_soc_dev_private_data *pdata);
> > +};
> > +
> > +/**
> > + * struct acpi_soc_dev_private_data - acpi device private data
> > + * @mmio_base: virtual memory base addr of the device
> > + * @mmio_size: device memory size
> > + * @dev_desc: device description
> > + * @adev: apci device
> 
> apci -> acpi
> 
[Ken] got it. 

> > + */
> > +struct acpi_soc_dev_private_data {
> > +	void __iomem *mmio_base;
> > +	resource_size_t mmio_size;
> > +
> > +	struct acpi_soc_dev_desc *dev_desc;
> > +	struct acpi_device *adev;
> > +};
> > +
> > +/**
> > + * register_acpi_soc - register a new acpi soc
> > + * @a_soc: acpi soc
> > + * @disable_scan_handler: true means remove default scan handle
> > + *			false means use default scan handle
> > + *
> > + * register a new acpi soc into asoc_list and install default scan handle.
> > + */
> 
> IMHO function kernel-docs belong close to the function implementation,
> e.g in this case to acpi_soc.c.
> 
[Ken] so, you suggest a kernel-doc for acpi_soc? i believe acpi_soc is
only a small extension for platform bus support of acpi.

> > +void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler);
> > +
> > +#endif
> > -- 
> > 1.9.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] acpi:soc: merge common codes for creating platform device
@ 2014-12-05  6:34     ` Ken Xue
  0 siblings, 0 replies; 6+ messages in thread
From: Ken Xue @ 2014-12-05  6:34 UTC (permalink / raw)
  To: Mika Westerberg; +Cc: rjw, linux-acpi, linux-kernel

On Thu, 2014-12-04 at 15:04 +0200, Mika Westerberg wrote:
> On Wed, Dec 03, 2014 at 08:33:37PM +0800, Ken Xue wrote:
> > This patch is supposed to deliver some common codes for AMD APD and
> > INTEL LPSS. It can help to convert some specific acpi devices to be
> > platform devices.
> > 
> > Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> > ---
> >  drivers/acpi/Makefile   |   2 +-
> >  drivers/acpi/acpi_soc.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/acpi/acpi_soc.h |  90 +++++++++++++++++++++
> >  3 files changed, 302 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/acpi/acpi_soc.c
> >  create mode 100644 drivers/acpi/acpi_soc.h
> > 
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index c3b2fcb..ae3397d 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -40,7 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
> >  acpi-y				+= ec.o
> >  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
> >  acpi-y				+= pci_root.o pci_link.o pci_irq.o
> > -acpi-y				+= acpi_lpss.o
> > +acpi-y				+= acpi_soc.o acpi_lpss.o
> 
> I think you need to convert acpi_lpss.c to use this new acpi_soc.c.
> 
[ken] I would like to merge lpss. But you know that I am lack of INTEL
platforms to verify codes before submit. And i am a little worried about
i may not cover so much rich features of lpss well. so, can we merge
lpss after current patch? or what's your plan?

> >  acpi-y				+= acpi_platform.o
> >  acpi-y				+= acpi_pnp.o
> >  acpi-y				+= int340x_thermal.o
> > diff --git a/drivers/acpi/acpi_soc.c b/drivers/acpi/acpi_soc.c
> > new file mode 100644
> > index 0000000..25089a0
> > --- /dev/null
> > +++ b/drivers/acpi/acpi_soc.c
> > @@ -0,0 +1,211 @@
> > +/*
> > + * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
> > + *
> > + * Copyright (C) 2015, Intel Corporation & AMD Corporation
> > + * Authors: Ken Xue <Ken.Xue@amd.com>
> > + *		Mika Westerberg <mika.westerberg@linux.intel.com>
> > + *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > + *
> > + * 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/err.h>
> > +#include <linux/io.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/delay.h>
> > +#include <linux/pm_domain.h>
> > +
> > +#include "internal.h"
> > +#include "acpi_soc.h"
> > +
> > +ACPI_MODULE_NAME("acpi_soc");
> > +
> > +/* A list for all acpi soc device */
> > +static LIST_HEAD(a_soc_list);
> > +
> > +static int is_memory(struct acpi_resource *res, void *not_used)
> > +{
> > +	struct resource r;
> > +
> > +	return !acpi_dev_resource_memory(res, &r);
> > +}
> > +
> > +static int acpi_soc_create_device(struct acpi_device *adev,
> > +				   const struct acpi_device_id *id)
> > +{
> > +	struct acpi_soc_dev_desc *dev_desc;
> > +	struct acpi_soc_dev_private_data *pdata;
> > +	struct resource_list_entry *rentry;
> > +	struct list_head resource_list;
> > +	struct platform_device *pdev;
> > +	int ret;
> > +
> > +	dev_desc = (struct acpi_soc_dev_desc *)id->driver_data;
> > +	if (!dev_desc) {
> > +		pdev = acpi_create_platform_device(adev);
> > +		return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
> > +	}
> > +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > +	if (!pdata)
> > +		return -ENOMEM;
> > +
> > +	INIT_LIST_HEAD(&resource_list);
> > +	ret = acpi_dev_get_resources(adev, &resource_list, is_memory, NULL);
> > +	if (ret < 0)
> > +		goto err_out;
> > +
> > +	list_for_each_entry(rentry, &resource_list, node)
> > +		if (resource_type(&rentry->res) == IORESOURCE_MEM) {
> > +			if (dev_desc->mem_size_override)
> > +				pdata->mmio_size = dev_desc->mem_size_override;
> > +			else
> > +				pdata->mmio_size = resource_size(&rentry->res);
> > +			pdata->mmio_base = ioremap(rentry->res.start,
> > +						   pdata->mmio_size);
> > +			break;
> > +		}
> > +
> > +	acpi_dev_free_resource_list(&resource_list);
> > +
> > +	pdata->adev = adev;
> > +	pdata->dev_desc = dev_desc;
> > +
> > +	if (dev_desc->setup)
> > +		dev_desc->setup(pdata);
> > +
> > +	/*
> > +	 * This works around a known issue in ACPI tables where acpi soc devices
> > +	 * have _PS0 and _PS3 without _PSC (and no power resources), so
> > +	 * acpi_bus_init_power() will assume that the BIOS has put them into D0.
> > +	 */
> > +	ret = acpi_device_fix_up_power(adev);
> > +	if (ret) {
> > +		/* Skip the device, but continue the namespace scan. */
> > +		ret = 0;
> > +		goto err_out;
> > +	}
> > +
> > +	adev->driver_data = pdata;
> > +	pdev = acpi_create_platform_device(adev);
> > +	if (!IS_ERR_OR_NULL(pdev))
> > +		return 1;
> > +
> > +	ret = PTR_ERR(pdev);
> > +	adev->driver_data = NULL;
> > +
> > + err_out:
> > +	kfree(pdata);
> > +	return ret;
> > +}
> > +
> > +static int acpi_soc_platform_notify(struct notifier_block *nb,
> > +				     unsigned long action, void *data)
> > +{
> > +	struct platform_device *pdev = to_platform_device(data);
> > +	struct acpi_soc_dev_private_data *pdata;
> > +	struct acpi_device *adev;
> > +	struct acpi_soc *a_soc_entry;
> > +	const struct acpi_device_id *id = NULL;
> > +
> > +	list_for_each_entry(a_soc_entry, &a_soc_list, list) {
> > +		id = acpi_match_device(a_soc_entry->ids, &pdev->dev);
> > +		if (!id)
> > +			break;
> > +	}
> > +
> > +	if (!id || !id->driver_data)
> > +		return 0;
> > +
> > +	if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
> > +		return 0;
> > +
> > +	pdata = acpi_driver_data(adev);
> > +	if (!pdata || !pdata->mmio_base)
> > +		return 0;
> > +
> > +	switch (action) {
> > +	case BUS_NOTIFY_BOUND_DRIVER:
> > +		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
> > +			if (a_soc_entry->pm_domain)
> > +				pdev->dev.pm_domain = a_soc_entry->pm_domain;
> > +			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
> > +					dev_pm_domain_attach(&pdev->dev, true);
> > +			else
> > +					dev_pm_domain_attach(&pdev->dev, false);
> > +		}
> > +		break;
> > +	case BUS_NOTIFY_UNBOUND_DRIVER:
> > +		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
> > +			if (a_soc_entry->pm_domain)
> > +				pdev->dev.pm_domain = a_soc_entry->pm_domain;
> > +			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
> > +					dev_pm_domain_detach(&pdev->dev, true);
> > +			else
> > +					dev_pm_domain_detach(&pdev->dev, false);
> > +		}
> > +		break;
> > +	case BUS_NOTIFY_ADD_DEVICE:
> > +		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
> > +			&& a_soc_entry->pm_domain)
> > +			return sysfs_create_group(&pdev->dev.kobj,
> > +						  a_soc_entry->attr_group);
> > +	case BUS_NOTIFY_DEL_DEVICE:
> > +		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
> > +			&& a_soc_entry->pm_domain)
> > +			sysfs_remove_group(&pdev->dev.kobj,
> > +			a_soc_entry->attr_group);
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct notifier_block acpi_soc_nb = {
> > +	.notifier_call = acpi_soc_platform_notify,
> > +};
> > +
> > +static void acpi_soc_bind(struct device *dev)
> > +{
> > +	struct acpi_soc_dev_private_data *pdata;
> > +
> > +	pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > +
> > +	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->bind)
> > +		return;
> > +
> > +	pdata->dev_desc->bind(pdata);
> > +}
> > +
> > +static void acpi_soc_unbind(struct device *dev)
> > +{
> > +	struct acpi_soc_dev_private_data *pdata;
> > +
> > +	pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > +
> > +	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->unbind)
> > +		return;
> > +
> > +	pdata->dev_desc->unbind(pdata);
> > +}
> > +
> > +void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler)
> 
> Would it work better if we pass acpi_scan_handler as a parameter to this
> function?
> 
> Then if it is NULL, it means that it will not register scan handler at
> all.
> 
> And then provide the default handler in acpi_soc.h so that you can do
> 
> 	register_acpi_soc(&soc, default_acpi_scan_handler);
> 
> or
> 
> 	register_acpi_soc(&soc, NULL);
> 
> depending the case. But also support passing custom scan handler.
> 
[Ken] I add some hooks for acpi_scan_handler in acpi_soc_dev_desc like
"setup" "bind" "unbind" in case of different implementation of platform.
your approach is other good way to same target. Right?

> > +{
> > +	struct acpi_scan_handler *acpi_soc_handler;
> > +
> > +	INIT_LIST_HEAD(&a_soc->list);
> > +	list_add(&a_soc->list, &a_soc_list);
> > +
> > +	acpi_soc_handler = kzalloc(sizeof(*acpi_soc_handler), GFP_KERNEL);
> > +	acpi_soc_handler->ids = a_soc->ids;
> > +	if (!disable_scan_handler) {
> > +		acpi_soc_handler->attach = acpi_soc_create_device;
> > +		acpi_soc_handler->bind = acpi_soc_bind;
> > +		acpi_soc_handler->unbind = acpi_soc_unbind;
> > +	}
> > +	acpi_scan_add_handler(acpi_soc_handler);
> > +	bus_register_notifier(&platform_bus_type, &acpi_soc_nb);
> > +}
> > diff --git a/drivers/acpi/acpi_soc.h b/drivers/acpi/acpi_soc.h
> > new file mode 100644
> > index 0000000..cc270a5
> > --- /dev/null
> > +++ b/drivers/acpi/acpi_soc.h
> > @@ -0,0 +1,90 @@
> > +/*
> > + * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
> > + *
> > + * Copyright (C) 2015, Intel Corporation & AMD Corporation
> > + * Authors: Ken Xue <Ken.Xue@amd.com>
> > + *		Mika Westerberg <mika.westerberg@linux.intel.com>
> > + *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > + *
> > + * 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.
> > + */
> > +#ifndef _ACPI_SOC_H
> > +#define _ACPI_SOC_H
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/clk.h>
> > +#include <linux/pm.h>
> > +
> > +/* Flags */
> > +#define ACPI_SOC_SYSFS	BIT(0)
> > +#define ACPI_SOC_PM		BIT(1)
> > +#define ACPI_SOC_PM_ON	BIT(2)
> 
> Would be nice to document what those mean.
> 
[Ken] sure. i will add more description.

> > +
> > +struct acpi_soc_dev_private_data;
> > +
> > +/**
> > + * struct acpi_soc - acpi soc
> > + * @list: list head
> > + * @ids: all acpi device ids for acpi soc
> > + * @pm_domain: power domain for all acpi device;can be NULL
> > + * @attr_group: attribute group for sysfs support of acpi soc;can be NULL
> > + */
> > +struct acpi_soc {
> > +	struct list_head list;
> > +	struct acpi_device_id	*ids;
> > +	struct dev_pm_domain	*pm_domain;
> > +	struct attribute_group	*attr_group;
> 
> These are tab aligned but list is using spaces. Please be consistent.
> 
[Ken] got it. 

> > +};
> > +
> > +/**
> > + * struct acpi_soc_dev_desc - a descriptor for acpi device
> > + * @flags: some device feature flags
> 
[Ken] ok.

> Please a bit better documentation than "some device feature flags".
> 
> > + * @clk: clock device
> > + * @fixed_clk_rate: fixed rate input clock source for acpi device;
> > + *				0 means no fixed rate input clock source
> > + * @mem_size_override: a workaround for override device memsize;
> > + *				0 means no needs for this WA
> > + * @setup: a hook routine to set device resource during create platform device
> > + * @bind: a hook of acpi_scan_handler.bind
> > + * @unbind: a hook of acpi_scan_handler.unbind
> > + *
> > + *device description defined as acpi_device_id.driver_data
> 
> Missing space.
> 
[Ken] got it. 

> > + */
> > +struct acpi_soc_dev_desc {
> > +	unsigned int flags;
> > +	struct clk *clk;
> > +	unsigned int fixed_clk_rate;
> > +	size_t mem_size_override;
> > +	int (*setup)(struct acpi_soc_dev_private_data *pdata);
> > +	void (*bind)(struct acpi_soc_dev_private_data *pdata);
> > +	void (*unbind)(struct acpi_soc_dev_private_data *pdata);
> > +};
> > +
> > +/**
> > + * struct acpi_soc_dev_private_data - acpi device private data
> > + * @mmio_base: virtual memory base addr of the device
> > + * @mmio_size: device memory size
> > + * @dev_desc: device description
> > + * @adev: apci device
> 
> apci -> acpi
> 
[Ken] got it. 

> > + */
> > +struct acpi_soc_dev_private_data {
> > +	void __iomem *mmio_base;
> > +	resource_size_t mmio_size;
> > +
> > +	struct acpi_soc_dev_desc *dev_desc;
> > +	struct acpi_device *adev;
> > +};
> > +
> > +/**
> > + * register_acpi_soc - register a new acpi soc
> > + * @a_soc: acpi soc
> > + * @disable_scan_handler: true means remove default scan handle
> > + *			false means use default scan handle
> > + *
> > + * register a new acpi soc into asoc_list and install default scan handle.
> > + */
> 
> IMHO function kernel-docs belong close to the function implementation,
> e.g in this case to acpi_soc.c.
> 
[Ken] so, you suggest a kernel-doc for acpi_soc? i believe acpi_soc is
only a small extension for platform bus support of acpi.

> > +void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler);
> > +
> > +#endif
> > -- 
> > 1.9.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] acpi:soc: merge common codes for creating platform device
  2014-12-05  6:34     ` Ken Xue
  (?)
@ 2014-12-05  8:32     ` Mika Westerberg
  -1 siblings, 0 replies; 6+ messages in thread
From: Mika Westerberg @ 2014-12-05  8:32 UTC (permalink / raw)
  To: Ken Xue; +Cc: rjw, linux-acpi, linux-kernel

On Fri, Dec 05, 2014 at 02:34:46PM +0800, Ken Xue wrote:
> On Thu, 2014-12-04 at 15:04 +0200, Mika Westerberg wrote:
> > On Wed, Dec 03, 2014 at 08:33:37PM +0800, Ken Xue wrote:
> > > This patch is supposed to deliver some common codes for AMD APD and
> > > INTEL LPSS. It can help to convert some specific acpi devices to be
> > > platform devices.
> > > 
> > > Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> > > ---
> > >  drivers/acpi/Makefile   |   2 +-
> > >  drivers/acpi/acpi_soc.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/acpi/acpi_soc.h |  90 +++++++++++++++++++++
> > >  3 files changed, 302 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/acpi/acpi_soc.c
> > >  create mode 100644 drivers/acpi/acpi_soc.h
> > > 
> > > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > > index c3b2fcb..ae3397d 100644
> > > --- a/drivers/acpi/Makefile
> > > +++ b/drivers/acpi/Makefile
> > > @@ -40,7 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
> > >  acpi-y				+= ec.o
> > >  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
> > >  acpi-y				+= pci_root.o pci_link.o pci_irq.o
> > > -acpi-y				+= acpi_lpss.o
> > > +acpi-y				+= acpi_soc.o acpi_lpss.o
> > 
> > I think you need to convert acpi_lpss.c to use this new acpi_soc.c.
> > 
> [ken] I would like to merge lpss. But you know that I am lack of INTEL
> platforms to verify codes before submit. And i am a little worried about
> i may not cover so much rich features of lpss well. so, can we merge
> lpss after current patch? or what's your plan?

I have plenty of Intel machines with LPSS, so I'm more than happy to
test this series on those :-)

> > >  acpi-y				+= acpi_platform.o
> > >  acpi-y				+= acpi_pnp.o
> > >  acpi-y				+= int340x_thermal.o
> > > diff --git a/drivers/acpi/acpi_soc.c b/drivers/acpi/acpi_soc.c
> > > new file mode 100644
> > > index 0000000..25089a0
> > > --- /dev/null
> > > +++ b/drivers/acpi/acpi_soc.c
> > > @@ -0,0 +1,211 @@
> > > +/*
> > > + * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
> > > + *
> > > + * Copyright (C) 2015, Intel Corporation & AMD Corporation
> > > + * Authors: Ken Xue <Ken.Xue@amd.com>
> > > + *		Mika Westerberg <mika.westerberg@linux.intel.com>
> > > + *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > + *
> > > + * 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/err.h>
> > > +#include <linux/io.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/pm_domain.h>
> > > +
> > > +#include "internal.h"
> > > +#include "acpi_soc.h"
> > > +
> > > +ACPI_MODULE_NAME("acpi_soc");
> > > +
> > > +/* A list for all acpi soc device */
> > > +static LIST_HEAD(a_soc_list);
> > > +
> > > +static int is_memory(struct acpi_resource *res, void *not_used)
> > > +{
> > > +	struct resource r;
> > > +
> > > +	return !acpi_dev_resource_memory(res, &r);
> > > +}
> > > +
> > > +static int acpi_soc_create_device(struct acpi_device *adev,
> > > +				   const struct acpi_device_id *id)
> > > +{
> > > +	struct acpi_soc_dev_desc *dev_desc;
> > > +	struct acpi_soc_dev_private_data *pdata;
> > > +	struct resource_list_entry *rentry;
> > > +	struct list_head resource_list;
> > > +	struct platform_device *pdev;
> > > +	int ret;
> > > +
> > > +	dev_desc = (struct acpi_soc_dev_desc *)id->driver_data;
> > > +	if (!dev_desc) {
> > > +		pdev = acpi_create_platform_device(adev);
> > > +		return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
> > > +	}
> > > +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > > +	if (!pdata)
> > > +		return -ENOMEM;
> > > +
> > > +	INIT_LIST_HEAD(&resource_list);
> > > +	ret = acpi_dev_get_resources(adev, &resource_list, is_memory, NULL);
> > > +	if (ret < 0)
> > > +		goto err_out;
> > > +
> > > +	list_for_each_entry(rentry, &resource_list, node)
> > > +		if (resource_type(&rentry->res) == IORESOURCE_MEM) {
> > > +			if (dev_desc->mem_size_override)
> > > +				pdata->mmio_size = dev_desc->mem_size_override;
> > > +			else
> > > +				pdata->mmio_size = resource_size(&rentry->res);
> > > +			pdata->mmio_base = ioremap(rentry->res.start,
> > > +						   pdata->mmio_size);
> > > +			break;
> > > +		}
> > > +
> > > +	acpi_dev_free_resource_list(&resource_list);
> > > +
> > > +	pdata->adev = adev;
> > > +	pdata->dev_desc = dev_desc;
> > > +
> > > +	if (dev_desc->setup)
> > > +		dev_desc->setup(pdata);
> > > +
> > > +	/*
> > > +	 * This works around a known issue in ACPI tables where acpi soc devices
> > > +	 * have _PS0 and _PS3 without _PSC (and no power resources), so
> > > +	 * acpi_bus_init_power() will assume that the BIOS has put them into D0.
> > > +	 */
> > > +	ret = acpi_device_fix_up_power(adev);
> > > +	if (ret) {
> > > +		/* Skip the device, but continue the namespace scan. */
> > > +		ret = 0;
> > > +		goto err_out;
> > > +	}
> > > +
> > > +	adev->driver_data = pdata;
> > > +	pdev = acpi_create_platform_device(adev);
> > > +	if (!IS_ERR_OR_NULL(pdev))
> > > +		return 1;
> > > +
> > > +	ret = PTR_ERR(pdev);
> > > +	adev->driver_data = NULL;
> > > +
> > > + err_out:
> > > +	kfree(pdata);
> > > +	return ret;
> > > +}
> > > +
> > > +static int acpi_soc_platform_notify(struct notifier_block *nb,
> > > +				     unsigned long action, void *data)
> > > +{
> > > +	struct platform_device *pdev = to_platform_device(data);
> > > +	struct acpi_soc_dev_private_data *pdata;
> > > +	struct acpi_device *adev;
> > > +	struct acpi_soc *a_soc_entry;
> > > +	const struct acpi_device_id *id = NULL;
> > > +
> > > +	list_for_each_entry(a_soc_entry, &a_soc_list, list) {
> > > +		id = acpi_match_device(a_soc_entry->ids, &pdev->dev);
> > > +		if (!id)
> > > +			break;
> > > +	}
> > > +
> > > +	if (!id || !id->driver_data)
> > > +		return 0;
> > > +
> > > +	if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
> > > +		return 0;
> > > +
> > > +	pdata = acpi_driver_data(adev);
> > > +	if (!pdata || !pdata->mmio_base)
> > > +		return 0;
> > > +
> > > +	switch (action) {
> > > +	case BUS_NOTIFY_BOUND_DRIVER:
> > > +		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
> > > +			if (a_soc_entry->pm_domain)
> > > +				pdev->dev.pm_domain = a_soc_entry->pm_domain;
> > > +			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
> > > +					dev_pm_domain_attach(&pdev->dev, true);
> > > +			else
> > > +					dev_pm_domain_attach(&pdev->dev, false);
> > > +		}
> > > +		break;
> > > +	case BUS_NOTIFY_UNBOUND_DRIVER:
> > > +		if ((pdata->dev_desc->flags & ACPI_SOC_PM)) {
> > > +			if (a_soc_entry->pm_domain)
> > > +				pdev->dev.pm_domain = a_soc_entry->pm_domain;
> > > +			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
> > > +					dev_pm_domain_detach(&pdev->dev, true);
> > > +			else
> > > +					dev_pm_domain_detach(&pdev->dev, false);
> > > +		}
> > > +		break;
> > > +	case BUS_NOTIFY_ADD_DEVICE:
> > > +		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
> > > +			&& a_soc_entry->pm_domain)
> > > +			return sysfs_create_group(&pdev->dev.kobj,
> > > +						  a_soc_entry->attr_group);
> > > +	case BUS_NOTIFY_DEL_DEVICE:
> > > +		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS)
> > > +			&& a_soc_entry->pm_domain)
> > > +			sysfs_remove_group(&pdev->dev.kobj,
> > > +			a_soc_entry->attr_group);
> > > +		break;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct notifier_block acpi_soc_nb = {
> > > +	.notifier_call = acpi_soc_platform_notify,
> > > +};
> > > +
> > > +static void acpi_soc_bind(struct device *dev)
> > > +{
> > > +	struct acpi_soc_dev_private_data *pdata;
> > > +
> > > +	pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > > +
> > > +	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->bind)
> > > +		return;
> > > +
> > > +	pdata->dev_desc->bind(pdata);
> > > +}
> > > +
> > > +static void acpi_soc_unbind(struct device *dev)
> > > +{
> > > +	struct acpi_soc_dev_private_data *pdata;
> > > +
> > > +	pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > > +
> > > +	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->unbind)
> > > +		return;
> > > +
> > > +	pdata->dev_desc->unbind(pdata);
> > > +}
> > > +
> > > +void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler)
> > 
> > Would it work better if we pass acpi_scan_handler as a parameter to this
> > function?
> > 
> > Then if it is NULL, it means that it will not register scan handler at
> > all.
> > 
> > And then provide the default handler in acpi_soc.h so that you can do
> > 
> > 	register_acpi_soc(&soc, default_acpi_scan_handler);
> > 
> > or
> > 
> > 	register_acpi_soc(&soc, NULL);
> > 
> > depending the case. But also support passing custom scan handler.
> > 
> [Ken] I add some hooks for acpi_scan_handler in acpi_soc_dev_desc like
> "setup" "bind" "unbind" in case of different implementation of platform.
> your approach is other good way to same target. Right?

Yes.

> > > +{
> > > +	struct acpi_scan_handler *acpi_soc_handler;
> > > +
> > > +	INIT_LIST_HEAD(&a_soc->list);
> > > +	list_add(&a_soc->list, &a_soc_list);
> > > +
> > > +	acpi_soc_handler = kzalloc(sizeof(*acpi_soc_handler), GFP_KERNEL);
> > > +	acpi_soc_handler->ids = a_soc->ids;
> > > +	if (!disable_scan_handler) {
> > > +		acpi_soc_handler->attach = acpi_soc_create_device;
> > > +		acpi_soc_handler->bind = acpi_soc_bind;
> > > +		acpi_soc_handler->unbind = acpi_soc_unbind;
> > > +	}
> > > +	acpi_scan_add_handler(acpi_soc_handler);
> > > +	bus_register_notifier(&platform_bus_type, &acpi_soc_nb);
> > > +}
> > > diff --git a/drivers/acpi/acpi_soc.h b/drivers/acpi/acpi_soc.h
> > > new file mode 100644
> > > index 0000000..cc270a5
> > > --- /dev/null
> > > +++ b/drivers/acpi/acpi_soc.h
> > > @@ -0,0 +1,90 @@
> > > +/*
> > > + * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
> > > + *
> > > + * Copyright (C) 2015, Intel Corporation & AMD Corporation
> > > + * Authors: Ken Xue <Ken.Xue@amd.com>
> > > + *		Mika Westerberg <mika.westerberg@linux.intel.com>
> > > + *		Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > + *
> > > + * 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.
> > > + */
> > > +#ifndef _ACPI_SOC_H
> > > +#define _ACPI_SOC_H
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/pm.h>
> > > +
> > > +/* Flags */
> > > +#define ACPI_SOC_SYSFS	BIT(0)
> > > +#define ACPI_SOC_PM		BIT(1)
> > > +#define ACPI_SOC_PM_ON	BIT(2)
> > 
> > Would be nice to document what those mean.
> > 
> [Ken] sure. i will add more description.
> 
> > > +
> > > +struct acpi_soc_dev_private_data;
> > > +
> > > +/**
> > > + * struct acpi_soc - acpi soc
> > > + * @list: list head
> > > + * @ids: all acpi device ids for acpi soc
> > > + * @pm_domain: power domain for all acpi device;can be NULL
> > > + * @attr_group: attribute group for sysfs support of acpi soc;can be NULL
> > > + */
> > > +struct acpi_soc {
> > > +	struct list_head list;
> > > +	struct acpi_device_id	*ids;
> > > +	struct dev_pm_domain	*pm_domain;
> > > +	struct attribute_group	*attr_group;
> > 
> > These are tab aligned but list is using spaces. Please be consistent.
> > 
> [Ken] got it. 
> 
> > > +};
> > > +
> > > +/**
> > > + * struct acpi_soc_dev_desc - a descriptor for acpi device
> > > + * @flags: some device feature flags
> > 
> [Ken] ok.
> 
> > Please a bit better documentation than "some device feature flags".
> > 
> > > + * @clk: clock device
> > > + * @fixed_clk_rate: fixed rate input clock source for acpi device;
> > > + *				0 means no fixed rate input clock source
> > > + * @mem_size_override: a workaround for override device memsize;
> > > + *				0 means no needs for this WA
> > > + * @setup: a hook routine to set device resource during create platform device
> > > + * @bind: a hook of acpi_scan_handler.bind
> > > + * @unbind: a hook of acpi_scan_handler.unbind
> > > + *
> > > + *device description defined as acpi_device_id.driver_data
> > 
> > Missing space.
> > 
> [Ken] got it. 
> 
> > > + */
> > > +struct acpi_soc_dev_desc {
> > > +	unsigned int flags;
> > > +	struct clk *clk;
> > > +	unsigned int fixed_clk_rate;
> > > +	size_t mem_size_override;
> > > +	int (*setup)(struct acpi_soc_dev_private_data *pdata);
> > > +	void (*bind)(struct acpi_soc_dev_private_data *pdata);
> > > +	void (*unbind)(struct acpi_soc_dev_private_data *pdata);
> > > +};
> > > +
> > > +/**
> > > + * struct acpi_soc_dev_private_data - acpi device private data
> > > + * @mmio_base: virtual memory base addr of the device
> > > + * @mmio_size: device memory size
> > > + * @dev_desc: device description
> > > + * @adev: apci device
> > 
> > apci -> acpi
> > 
> [Ken] got it. 
> 
> > > + */
> > > +struct acpi_soc_dev_private_data {
> > > +	void __iomem *mmio_base;
> > > +	resource_size_t mmio_size;
> > > +
> > > +	struct acpi_soc_dev_desc *dev_desc;
> > > +	struct acpi_device *adev;
> > > +};
> > > +
> > > +/**
> > > + * register_acpi_soc - register a new acpi soc
> > > + * @a_soc: acpi soc
> > > + * @disable_scan_handler: true means remove default scan handle
> > > + *			false means use default scan handle
> > > + *
> > > + * register a new acpi soc into asoc_list and install default scan handle.
> > > + */
> > 
> > IMHO function kernel-docs belong close to the function implementation,
> > e.g in this case to acpi_soc.c.
> > 
> [Ken] so, you suggest a kernel-doc for acpi_soc? i believe acpi_soc is
> only a small extension for platform bus support of acpi.

I mean move the kernel doc where the function is defined (acpi_soc.c).
> 
> > > +void register_acpi_soc(struct acpi_soc *a_soc, bool disable_scan_handler);
> > > +
> > > +#endif
> > > -- 
> > > 1.9.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2014-12-05  8:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-03 12:33 [PATCH 1/2] acpi:soc: merge common codes for creating platform device Ken Xue
2014-12-03 12:33 ` Ken Xue
2014-12-04 13:04 ` Mika Westerberg
2014-12-05  6:34   ` Ken Xue
2014-12-05  6:34     ` Ken Xue
2014-12-05  8:32     ` Mika Westerberg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.