linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3] acpi:apd:add AMD ACPI2Platform device support for x86 system.
@ 2015-02-03  1:54 Ken Xue
  2015-02-03 10:06 ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Ken Xue @ 2015-02-03  1:54 UTC (permalink / raw)
  To: Rafael J. Wysocki, andy.shevchenko, mika.westerberg
  Cc: linux-acpi, linux-kernel

This new feature is to interpret AMD specific ACPI device to
platform device such as I2C, UART found on AMD CZ and later chipsets. It
based on example intel LPSS. Now, it can support AMD I2C & UART.

Signed-off-by: Ken Xue <Ken.Xue@amd.com>
---
 arch/x86/Kconfig        |  11 +++
 drivers/acpi/Makefile   |   2 +-
 drivers/acpi/acpi_apd.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |   2 +
 drivers/acpi/scan.c     |   1 +
 5 files changed, 216 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/acpi_apd.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0dc9d01..ddf8d42 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -496,6 +496,17 @@ config X86_INTEL_LPSS
 	  things like clock tree (common clock framework) and pincontrol
 	  which are needed by the LPSS peripheral drivers.
 
+config X86_AMD_PLATFORM_DEVICE
+	bool "AMD ACPI2Platform devices support"
+	depends on ACPI
+	select COMMON_CLK
+	select PINCTRL
+	---help---
+	  Select to interpret AMD specific ACPI device to platform device
+	  such as I2C, UART found on AMD Carrizo and later chipsets. Selecting
+	  this option enables things like clock tree (common clock framework)
+	  and pinctrl.
+
 config IOSF_MBI
 	tristate "Intel SoC IOSF Sideband support for SoC platforms"
 	depends on PCI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index f74317c..0071141 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_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
 acpi-y				+= int340x_thermal.o
diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
new file mode 100644
index 0000000..b27bc1c
--- /dev/null
+++ b/drivers/acpi/acpi_apd.c
@@ -0,0 +1,201 @@
+/*
+ * AMD ACPI support for ACPI2platform device.
+ *
+ * Copyright (c) 2014,2015 AMD Corporation.
+ * Authors: Ken Xue <Ken.Xue@amd.com>
+ *	Wu, Jeff <Jeff.Wu@amd.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/clk-provider.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/clkdev.h>
+#include <linux/acpi.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/pm.h>
+
+#include "internal.h"
+
+ACPI_MODULE_NAME("acpi_apd");
+struct apd_private_data;
+
+/**
+ * device flags of acpi_apd_dev_desc.
+ * ACPI_APD_SYSFS : add device attributes in sysfs
+ * ACPI_APD_PM : attach power domain to device
+ * ACPI_APD_PM_ON : power on device when attach power domain
+ */
+#define ACPI_APD_SYSFS	BIT(0)
+#define ACPI_APD_PM	BIT(1)
+#define ACPI_APD_PM_ON	BIT(2)
+
+/**
+ * struct apd_device_desc - a descriptor for apd device
+ * @flags: device flags like ACPI_APD_SYSFS ACPI_APD_PM ACPI_APD_PM_ON
+ * @fixed_clk_rate: fixed rate input clock source for acpi device;
+ *			0 means no fixed rate input clock source
+ * @setup: a hook routine to set device resource during create platform device
+ *
+ * device description defined as acpi_device_id.driver_data
+ */
+struct apd_device_desc {
+	unsigned int flags;
+	unsigned int fixed_clk_rate;
+	int (*setup)(struct apd_private_data *pdata);
+};
+
+struct apd_private_data {
+	struct clk *clk;
+	struct acpi_device *adev;
+	const struct apd_device_desc *dev_desc;
+};
+
+#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
+#define APD_ADDR(desc)	((unsigned long)&desc)
+
+static int acpi_apd_setup(struct apd_private_data *pdata)
+{
+	const struct apd_device_desc *dev_desc = pdata->dev_desc;
+	struct clk *clk = ERR_PTR(-ENODEV);
+
+	if (dev_desc->fixed_clk_rate) {
+		clk = clk_register_fixed_rate(&pdata->adev->dev,
+					dev_name(&pdata->adev->dev),
+					NULL, CLK_IS_ROOT,
+					dev_desc->fixed_clk_rate);
+		clk_register_clkdev(clk, NULL, dev_name(&pdata->adev->dev));
+		pdata->clk = clk;
+	}
+
+	return 0;
+}
+
+static struct apd_device_desc cz_i2c_desc = {
+	.setup = acpi_apd_setup,
+	.fixed_clk_rate = 133000000,
+};
+
+static struct apd_device_desc cz_uart_desc = {
+	.setup = acpi_apd_setup,
+	.fixed_clk_rate = 48000000,
+};
+
+#else
+
+#define APD_ADDR(desc) (0UL)
+
+#endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
+
+static int acpi_apd_create_device(struct acpi_device *adev,
+				   const struct acpi_device_id *id)
+{
+	const struct apd_device_desc *dev_desc;
+	struct apd_private_data *pdata;
+	struct platform_device *pdev;
+	int ret;
+
+	dev_desc = (struct apd_device_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;
+
+	pdata->adev = adev;
+	pdata->dev_desc = dev_desc;
+
+	if (dev_desc->setup) {
+		ret = dev_desc->setup(pdata);
+		if (ret)
+			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 const struct acpi_device_id acpi_apd_device_ids[] = {
+	/* Generic apd devices */
+	{ "AMD0010", APD_ADDR(cz_i2c_desc) },
+	{ "AMD0020", APD_ADDR(cz_uart_desc) },
+	{ }
+};
+
+#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
+static int acpi_apd_platform_notify(struct notifier_block *nb,
+				     unsigned long action, void *data)
+{
+	struct platform_device *pdev = to_platform_device(data);
+	const struct acpi_device_id *id = NULL;
+	struct apd_private_data *pdata;
+	struct acpi_device *adev;
+	bool power_on;
+	int ret;
+
+	id = acpi_match_device(acpi_apd_device_ids, &pdev->dev);
+	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->dev_desc)
+		return 0;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		if (pdata->dev_desc->flags & ACPI_APD_PM) {
+			power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON);
+			ret = dev_pm_domain_attach(&pdev->dev, power_on);
+
+			if (ret)
+				dev_dbg(&pdev->dev,
+				"failed to attached pm domain (%d)\n", ret);
+		}
+		break;
+	case BUS_NOTIFY_DEL_DEVICE:
+		if (pdata->dev_desc->flags & ACPI_APD_PM) {
+			power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON);
+			dev_pm_domain_detach(&pdev->dev, power_on);
+		}
+		break;
+	}
+
+	return 0;
+}
+
+static struct notifier_block acpi_apd_nb = {
+	.notifier_call = acpi_apd_platform_notify,
+};
+#endif
+
+static struct acpi_scan_handler apd_handler = {
+	.ids = acpi_apd_device_ids,
+	.attach = acpi_apd_create_device,
+};
+
+void __init acpi_apd_init(void)
+{
+#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
+	bus_register_notifier(&platform_bus_type, &acpi_apd_nb);
+#endif
+
+	acpi_scan_add_handler(&apd_handler);
+}
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 163e82f..c24ae9d 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -68,6 +68,8 @@ static inline void acpi_debugfs_init(void) { return; }
 #endif
 void acpi_lpss_init(void);
 
+void acpi_apd_init(void);
+
 acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
 bool acpi_queue_hotplug_work(struct work_struct *work);
 void acpi_device_hotplug(struct acpi_device *adev, u32 src);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index dc4d896..bbca783 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2544,6 +2544,7 @@ int __init acpi_scan_init(void)
 	acpi_pci_link_init();
 	acpi_processor_init();
 	acpi_lpss_init();
+	acpi_apd_init();
 	acpi_cmos_rtc_init();
 	acpi_container_init();
 	acpi_memory_hotplug_init();
-- 
1.9.1




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

* Re: [PATCH V3] acpi:apd:add AMD ACPI2Platform device support for x86 system.
  2015-02-03  1:54 [PATCH V3] acpi:apd:add AMD ACPI2Platform device support for x86 system Ken Xue
@ 2015-02-03 10:06 ` Andy Shevchenko
  2015-02-03 22:07   ` Rafael J. Wysocki
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2015-02-03 10:06 UTC (permalink / raw)
  To: Ken Xue; +Cc: Rafael J. Wysocki, mika.westerberg, linux-acpi, linux-kernel

On Tue, Feb 3, 2015 at 3:54 AM, Ken Xue <Ken.Xue@amd.com> wrote:
> This new feature is to interpret AMD specific ACPI device to
> platform device such as I2C, UART found on AMD CZ and later chipsets. It
> based on example intel LPSS. Now, it can support AMD I2C & UART.
>

Few minor things below.
Be free to fix in depend on what Mika and Rafael confirm.

Anyway,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> ---
>  arch/x86/Kconfig        |  11 +++
>  drivers/acpi/Makefile   |   2 +-
>  drivers/acpi/acpi_apd.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/internal.h |   2 +
>  drivers/acpi/scan.c     |   1 +
>  5 files changed, 216 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/acpi/acpi_apd.c
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 0dc9d01..ddf8d42 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -496,6 +496,17 @@ config X86_INTEL_LPSS
>           things like clock tree (common clock framework) and pincontrol
>           which are needed by the LPSS peripheral drivers.
>
> +config X86_AMD_PLATFORM_DEVICE
> +       bool "AMD ACPI2Platform devices support"
> +       depends on ACPI
> +       select COMMON_CLK
> +       select PINCTRL
> +       ---help---
> +         Select to interpret AMD specific ACPI device to platform device
> +         such as I2C, UART found on AMD Carrizo and later chipsets. Selecting
> +         this option enables things like clock tree (common clock framework)
> +         and pinctrl.

Sounds 'like' is redundant here. It explicitly enables common clock
and pin control frameworks.

> +
>  config IOSF_MBI
>         tristate "Intel SoC IOSF Sideband support for SoC platforms"
>         depends on PCI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index f74317c..0071141 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_lpss.o acpi_apd.o
>  acpi-y                         += acpi_platform.o
>  acpi-y                         += acpi_pnp.o
>  acpi-y                         += int340x_thermal.o
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> new file mode 100644
> index 0000000..b27bc1c
> --- /dev/null
> +++ b/drivers/acpi/acpi_apd.c
> @@ -0,0 +1,201 @@
> +/*
> + * AMD ACPI support for ACPI2platform device.
> + *
> + * Copyright (c) 2014,2015 AMD Corporation.
> + * Authors: Ken Xue <Ken.Xue@amd.com>
> + *     Wu, Jeff <Jeff.Wu@amd.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/clk-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/clkdev.h>
> +#include <linux/acpi.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/pm.h>
> +
> +#include "internal.h"
> +
> +ACPI_MODULE_NAME("acpi_apd");
> +struct apd_private_data;
> +
> +/**

/*, or use description on per flag basis.

> + * device flags of acpi_apd_dev_desc.
> + * ACPI_APD_SYSFS : add device attributes in sysfs
> + * ACPI_APD_PM : attach power domain to device
> + * ACPI_APD_PM_ON : power on device when attach power domain
> + */
> +#define ACPI_APD_SYSFS BIT(0)
> +#define ACPI_APD_PM    BIT(1)
> +#define ACPI_APD_PM_ON BIT(2)
> +
> +/**
> + * struct apd_device_desc - a descriptor for apd device
> + * @flags: device flags like ACPI_APD_SYSFS ACPI_APD_PM ACPI_APD_PM_ON

%ACPI_APD_SYSFS, %ACPI_APD_PM, %ACPI_APD_APM_ON.

Could it be combination? State this explicitly.

> + * @fixed_clk_rate: fixed rate input clock source for acpi device;
> + *                     0 means no fixed rate input clock source
> + * @setup: a hook routine to set device resource during create platform device
> + *
> + * device description defined as acpi_device_id.driver_data

d -> D

> + */
> +struct apd_device_desc {
> +       unsigned int flags;
> +       unsigned int fixed_clk_rate;
> +       int (*setup)(struct apd_private_data *pdata);
> +};
> +
> +struct apd_private_data {
> +       struct clk *clk;
> +       struct acpi_device *adev;
> +       const struct apd_device_desc *dev_desc;
> +};
> +
> +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> +#define APD_ADDR(desc) ((unsigned long)&desc)
> +
> +static int acpi_apd_setup(struct apd_private_data *pdata)
> +{
> +       const struct apd_device_desc *dev_desc = pdata->dev_desc;
> +       struct clk *clk = ERR_PTR(-ENODEV);
> +
> +       if (dev_desc->fixed_clk_rate) {
> +               clk = clk_register_fixed_rate(&pdata->adev->dev,
> +                                       dev_name(&pdata->adev->dev),
> +                                       NULL, CLK_IS_ROOT,
> +                                       dev_desc->fixed_clk_rate);
> +               clk_register_clkdev(clk, NULL, dev_name(&pdata->adev->dev));
> +               pdata->clk = clk;
> +       }

What about
if (!…clk_rate)
 return 0;
…
return 0; ?

And question why we need int to be returned at all?

> +
> +       return 0;
> +}
> +
> +static struct apd_device_desc cz_i2c_desc = {
> +       .setup = acpi_apd_setup,
> +       .fixed_clk_rate = 133000000,
> +};
> +
> +static struct apd_device_desc cz_uart_desc = {
> +       .setup = acpi_apd_setup,
> +       .fixed_clk_rate = 48000000,
> +};
> +
> +#else
> +
> +#define APD_ADDR(desc) (0UL)
> +
> +#endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
> +
> +static int acpi_apd_create_device(struct acpi_device *adev,
> +                                  const struct acpi_device_id *id)
> +{
> +       const struct apd_device_desc *dev_desc;
> +       struct apd_private_data *pdata;
> +       struct platform_device *pdev;
> +       int ret;
> +
> +       dev_desc = (struct apd_device_desc *)id->driver_data;

You may move this assignment to the definition block. And if it
doesn't fit one line you may cast to (void *), though I don't know if
Rafael likes such trick.

> +       if (!dev_desc) {
> +               pdev = acpi_create_platform_device(adev);
> +               return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;

I guess it worth to add a comment what means return code here.

> +       }
> +       pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +       if (!pdata)
> +               return -ENOMEM;
> +
> +       pdata->adev = adev;
> +       pdata->dev_desc = dev_desc;
> +
> +       if (dev_desc->setup) {
> +               ret = dev_desc->setup(pdata);
> +               if (ret)
> +                       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 const struct acpi_device_id acpi_apd_device_ids[] = {
> +       /* Generic apd devices */
> +       { "AMD0010", APD_ADDR(cz_i2c_desc) },
> +       { "AMD0020", APD_ADDR(cz_uart_desc) },
> +       { }
> +};
> +
> +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> +static int acpi_apd_platform_notify(struct notifier_block *nb,
> +                                    unsigned long action, void *data)
> +{
> +       struct platform_device *pdev = to_platform_device(data);
> +       const struct acpi_device_id *id = NULL;

Redundant assignment.

> +       struct apd_private_data *pdata;
> +       struct acpi_device *adev;
> +       bool power_on;
> +       int ret;
> +
> +       id = acpi_match_device(acpi_apd_device_ids, &pdev->dev);
> +       if (!id || !id->driver_data)
> +               return 0;
> +
> +       if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))

Better of course to do like
status = …
if (status) [don't remember how to check status with ACPI API]
 return 0;

> +               return 0;
> +
> +       pdata = acpi_driver_data(adev);
> +       if (!pdata || !pdata->dev_desc)
> +               return 0;
> +
> +       switch (action) {
> +       case BUS_NOTIFY_ADD_DEVICE:
> +               if (pdata->dev_desc->flags & ACPI_APD_PM) {
> +                       power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON);
> +                       ret = dev_pm_domain_attach(&pdev->dev, power_on);
> +
> +                       if (ret)
> +                               dev_dbg(&pdev->dev,
> +                               "failed to attached pm domain (%d)\n", ret);
> +               }
> +               break;
> +       case BUS_NOTIFY_DEL_DEVICE:
> +               if (pdata->dev_desc->flags & ACPI_APD_PM) {
> +                       power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON);
> +                       dev_pm_domain_detach(&pdev->dev, power_on);
> +               }
> +               break;
> +       }
> +
> +       return 0;
> +}
> +
> +static struct notifier_block acpi_apd_nb = {
> +       .notifier_call = acpi_apd_platform_notify,
> +};
> +#endif
> +
> +static struct acpi_scan_handler apd_handler = {
> +       .ids = acpi_apd_device_ids,
> +       .attach = acpi_apd_create_device,
> +};
> +
> +void __init acpi_apd_init(void)
> +{
> +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> +       bus_register_notifier(&platform_bus_type, &acpi_apd_nb);
> +#endif
> +
> +       acpi_scan_add_handler(&apd_handler);
> +}
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 163e82f..c24ae9d 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -68,6 +68,8 @@ static inline void acpi_debugfs_init(void) { return; }
>  #endif
>  void acpi_lpss_init(void);
>
> +void acpi_apd_init(void);
> +
>  acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
>  bool acpi_queue_hotplug_work(struct work_struct *work);
>  void acpi_device_hotplug(struct acpi_device *adev, u32 src);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index dc4d896..bbca783 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2544,6 +2544,7 @@ int __init acpi_scan_init(void)
>         acpi_pci_link_init();
>         acpi_processor_init();
>         acpi_lpss_init();
> +       acpi_apd_init();
>         acpi_cmos_rtc_init();
>         acpi_container_init();
>         acpi_memory_hotplug_init();
> --
> 1.9.1
>
>
>



-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH V3] acpi:apd:add AMD ACPI2Platform device support for x86 system.
  2015-02-03 10:06 ` Andy Shevchenko
@ 2015-02-03 22:07   ` Rafael J. Wysocki
  2015-02-04  3:40     ` Ken Xue
  0 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2015-02-03 22:07 UTC (permalink / raw)
  To: Andy Shevchenko, Ken Xue; +Cc: mika.westerberg, linux-acpi, linux-kernel

On Tuesday, February 03, 2015 12:06:06 PM Andy Shevchenko wrote:
> On Tue, Feb 3, 2015 at 3:54 AM, Ken Xue <Ken.Xue@amd.com> wrote:
> > This new feature is to interpret AMD specific ACPI device to
> > platform device such as I2C, UART found on AMD CZ and later chipsets. It
> > based on example intel LPSS. Now, it can support AMD I2C & UART.
> >
> 
> Few minor things below.
> Be free to fix in depend on what Mika and Rafael confirm.
> 
> Anyway,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks Andy!

Ken, please address the Andy's comments given below, they all make sense to me.

> > Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> > ---
> >  arch/x86/Kconfig        |  11 +++
> >  drivers/acpi/Makefile   |   2 +-
> >  drivers/acpi/acpi_apd.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/acpi/internal.h |   2 +
> >  drivers/acpi/scan.c     |   1 +
> >  5 files changed, 216 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/acpi/acpi_apd.c
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 0dc9d01..ddf8d42 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -496,6 +496,17 @@ config X86_INTEL_LPSS
> >           things like clock tree (common clock framework) and pincontrol
> >           which are needed by the LPSS peripheral drivers.
> >
> > +config X86_AMD_PLATFORM_DEVICE
> > +       bool "AMD ACPI2Platform devices support"
> > +       depends on ACPI
> > +       select COMMON_CLK
> > +       select PINCTRL
> > +       ---help---
> > +         Select to interpret AMD specific ACPI device to platform device
> > +         such as I2C, UART found on AMD Carrizo and later chipsets. Selecting
> > +         this option enables things like clock tree (common clock framework)
> > +         and pinctrl.
> 
> Sounds 'like' is redundant here. It explicitly enables common clock
> and pin control frameworks.
> 
> > +
> >  config IOSF_MBI
> >         tristate "Intel SoC IOSF Sideband support for SoC platforms"
> >         depends on PCI
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index f74317c..0071141 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_lpss.o acpi_apd.o
> >  acpi-y                         += acpi_platform.o
> >  acpi-y                         += acpi_pnp.o
> >  acpi-y                         += int340x_thermal.o
> > diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> > new file mode 100644
> > index 0000000..b27bc1c
> > --- /dev/null
> > +++ b/drivers/acpi/acpi_apd.c
> > @@ -0,0 +1,201 @@
> > +/*
> > + * AMD ACPI support for ACPI2platform device.
> > + *
> > + * Copyright (c) 2014,2015 AMD Corporation.
> > + * Authors: Ken Xue <Ken.Xue@amd.com>
> > + *     Wu, Jeff <Jeff.Wu@amd.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/clk-provider.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/acpi.h>
> > +#include <linux/err.h>
> > +#include <linux/clk.h>
> > +#include <linux/pm.h>
> > +
> > +#include "internal.h"
> > +
> > +ACPI_MODULE_NAME("acpi_apd");
> > +struct apd_private_data;
> > +
> > +/**
> 
> /*, or use description on per flag basis.
> 
> > + * device flags of acpi_apd_dev_desc.
> > + * ACPI_APD_SYSFS : add device attributes in sysfs
> > + * ACPI_APD_PM : attach power domain to device
> > + * ACPI_APD_PM_ON : power on device when attach power domain
> > + */
> > +#define ACPI_APD_SYSFS BIT(0)
> > +#define ACPI_APD_PM    BIT(1)
> > +#define ACPI_APD_PM_ON BIT(2)
> > +
> > +/**
> > + * struct apd_device_desc - a descriptor for apd device
> > + * @flags: device flags like ACPI_APD_SYSFS ACPI_APD_PM ACPI_APD_PM_ON
> 
> %ACPI_APD_SYSFS, %ACPI_APD_PM, %ACPI_APD_APM_ON.
> 
> Could it be combination? State this explicitly.
> 
> > + * @fixed_clk_rate: fixed rate input clock source for acpi device;
> > + *                     0 means no fixed rate input clock source
> > + * @setup: a hook routine to set device resource during create platform device
> > + *
> > + * device description defined as acpi_device_id.driver_data
> 
> d -> D
> 
> > + */
> > +struct apd_device_desc {
> > +       unsigned int flags;
> > +       unsigned int fixed_clk_rate;
> > +       int (*setup)(struct apd_private_data *pdata);
> > +};
> > +
> > +struct apd_private_data {
> > +       struct clk *clk;
> > +       struct acpi_device *adev;
> > +       const struct apd_device_desc *dev_desc;
> > +};
> > +
> > +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> > +#define APD_ADDR(desc) ((unsigned long)&desc)
> > +
> > +static int acpi_apd_setup(struct apd_private_data *pdata)
> > +{
> > +       const struct apd_device_desc *dev_desc = pdata->dev_desc;
> > +       struct clk *clk = ERR_PTR(-ENODEV);
> > +
> > +       if (dev_desc->fixed_clk_rate) {
> > +               clk = clk_register_fixed_rate(&pdata->adev->dev,
> > +                                       dev_name(&pdata->adev->dev),
> > +                                       NULL, CLK_IS_ROOT,
> > +                                       dev_desc->fixed_clk_rate);
> > +               clk_register_clkdev(clk, NULL, dev_name(&pdata->adev->dev));
> > +               pdata->clk = clk;
> > +       }
> 
> What about
> if (!…clk_rate)
>  return 0;
> …
> return 0; ?
> 
> And question why we need int to be returned at all?
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static struct apd_device_desc cz_i2c_desc = {
> > +       .setup = acpi_apd_setup,
> > +       .fixed_clk_rate = 133000000,
> > +};
> > +
> > +static struct apd_device_desc cz_uart_desc = {
> > +       .setup = acpi_apd_setup,
> > +       .fixed_clk_rate = 48000000,
> > +};
> > +
> > +#else
> > +
> > +#define APD_ADDR(desc) (0UL)
> > +
> > +#endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
> > +
> > +static int acpi_apd_create_device(struct acpi_device *adev,
> > +                                  const struct acpi_device_id *id)
> > +{
> > +       const struct apd_device_desc *dev_desc;
> > +       struct apd_private_data *pdata;
> > +       struct platform_device *pdev;
> > +       int ret;
> > +
> > +       dev_desc = (struct apd_device_desc *)id->driver_data;
> 
> You may move this assignment to the definition block. And if it
> doesn't fit one line you may cast to (void *), though I don't know if
> Rafael likes such trick.
> 
> > +       if (!dev_desc) {
> > +               pdev = acpi_create_platform_device(adev);
> > +               return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
> 
> I guess it worth to add a comment what means return code here.
> 
> > +       }
> > +       pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > +       if (!pdata)
> > +               return -ENOMEM;
> > +
> > +       pdata->adev = adev;
> > +       pdata->dev_desc = dev_desc;
> > +
> > +       if (dev_desc->setup) {
> > +               ret = dev_desc->setup(pdata);
> > +               if (ret)
> > +                       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 const struct acpi_device_id acpi_apd_device_ids[] = {
> > +       /* Generic apd devices */
> > +       { "AMD0010", APD_ADDR(cz_i2c_desc) },
> > +       { "AMD0020", APD_ADDR(cz_uart_desc) },
> > +       { }
> > +};
> > +
> > +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> > +static int acpi_apd_platform_notify(struct notifier_block *nb,
> > +                                    unsigned long action, void *data)
> > +{
> > +       struct platform_device *pdev = to_platform_device(data);
> > +       const struct acpi_device_id *id = NULL;
> 
> Redundant assignment.
> 
> > +       struct apd_private_data *pdata;
> > +       struct acpi_device *adev;
> > +       bool power_on;
> > +       int ret;
> > +
> > +       id = acpi_match_device(acpi_apd_device_ids, &pdev->dev);
> > +       if (!id || !id->driver_data)
> > +               return 0;
> > +
> > +       if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
> 
> Better of course to do like
> status = …
> if (status) [don't remember how to check status with ACPI API]
>  return 0;
> 
> > +               return 0;
> > +
> > +       pdata = acpi_driver_data(adev);
> > +       if (!pdata || !pdata->dev_desc)
> > +               return 0;
> > +
> > +       switch (action) {
> > +       case BUS_NOTIFY_ADD_DEVICE:
> > +               if (pdata->dev_desc->flags & ACPI_APD_PM) {
> > +                       power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON);
> > +                       ret = dev_pm_domain_attach(&pdev->dev, power_on);
> > +
> > +                       if (ret)
> > +                               dev_dbg(&pdev->dev,
> > +                               "failed to attached pm domain (%d)\n", ret);
> > +               }
> > +               break;
> > +       case BUS_NOTIFY_DEL_DEVICE:
> > +               if (pdata->dev_desc->flags & ACPI_APD_PM) {
> > +                       power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON);
> > +                       dev_pm_domain_detach(&pdev->dev, power_on);
> > +               }
> > +               break;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static struct notifier_block acpi_apd_nb = {
> > +       .notifier_call = acpi_apd_platform_notify,
> > +};
> > +#endif
> > +
> > +static struct acpi_scan_handler apd_handler = {
> > +       .ids = acpi_apd_device_ids,
> > +       .attach = acpi_apd_create_device,
> > +};
> > +
> > +void __init acpi_apd_init(void)
> > +{
> > +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> > +       bus_register_notifier(&platform_bus_type, &acpi_apd_nb);
> > +#endif
> > +
> > +       acpi_scan_add_handler(&apd_handler);
> > +}
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 163e82f..c24ae9d 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -68,6 +68,8 @@ static inline void acpi_debugfs_init(void) { return; }
> >  #endif
> >  void acpi_lpss_init(void);
> >
> > +void acpi_apd_init(void);
> > +
> >  acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
> >  bool acpi_queue_hotplug_work(struct work_struct *work);
> >  void acpi_device_hotplug(struct acpi_device *adev, u32 src);
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index dc4d896..bbca783 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -2544,6 +2544,7 @@ int __init acpi_scan_init(void)
> >         acpi_pci_link_init();
> >         acpi_processor_init();
> >         acpi_lpss_init();
> > +       acpi_apd_init();
> >         acpi_cmos_rtc_init();
> >         acpi_container_init();
> >         acpi_memory_hotplug_init();
> > --
> > 1.9.1
> >
> >
> >
> 
> 
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

* Re: [PATCH V3] acpi:apd:add AMD ACPI2Platform device support for x86 system.
  2015-02-03 22:07   ` Rafael J. Wysocki
@ 2015-02-04  3:40     ` Ken Xue
  2015-02-05  3:07       ` [PATCH V4] " Ken Xue
  0 siblings, 1 reply; 10+ messages in thread
From: Ken Xue @ 2015-02-04  3:40 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Andy Shevchenko, mika.westerberg, linux-acpi, linux-kernel

On Tue, 2015-02-03 at 23:07 +0100, Rafael J. Wysocki wrote:
> On Tuesday, February 03, 2015 12:06:06 PM Andy Shevchenko wrote:
> > On Tue, Feb 3, 2015 at 3:54 AM, Ken Xue <Ken.Xue@amd.com> wrote:
> > > This new feature is to interpret AMD specific ACPI device to
> > > platform device such as I2C, UART found on AMD CZ and later chipsets. It
> > > based on example intel LPSS. Now, it can support AMD I2C & UART.
> > >
> > 
> > Few minor things below.
> > Be free to fix in depend on what Mika and Rafael confirm.
> > 
> > Anyway,
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Thanks Andy!
> 
> Ken, please address the Andy's comments given below, they all make sense to me.
> 
Thanks for all your comments.

> > > Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> > > ---
> > >  arch/x86/Kconfig        |  11 +++
> > >  drivers/acpi/Makefile   |   2 +-
> > >  drivers/acpi/acpi_apd.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/acpi/internal.h |   2 +
> > >  drivers/acpi/scan.c     |   1 +
> > >  5 files changed, 216 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/acpi/acpi_apd.c
> > >
> > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > index 0dc9d01..ddf8d42 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -496,6 +496,17 @@ config X86_INTEL_LPSS
> > >           things like clock tree (common clock framework) and pincontrol
> > >           which are needed by the LPSS peripheral drivers.
> > >
> > > +config X86_AMD_PLATFORM_DEVICE
> > > +       bool "AMD ACPI2Platform devices support"
> > > +       depends on ACPI
> > > +       select COMMON_CLK
> > > +       select PINCTRL
> > > +       ---help---
> > > +         Select to interpret AMD specific ACPI device to platform device
> > > +         such as I2C, UART found on AMD Carrizo and later chipsets. Selecting
> > > +         this option enables things like clock tree (common clock framework)
> > > +         and pinctrl.
> > 
> > Sounds 'like' is redundant here. It explicitly enables common clock
> > and pin control frameworks.
> > 
I just copy the last sentence from description of X86_INTEL_LPSS. but it
is OK, i will remove it from next release.

> > > +
> > >  config IOSF_MBI
> > >         tristate "Intel SoC IOSF Sideband support for SoC platforms"
> > >         depends on PCI
> > > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > > index f74317c..0071141 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_lpss.o acpi_apd.o
> > >  acpi-y                         += acpi_platform.o
> > >  acpi-y                         += acpi_pnp.o
> > >  acpi-y                         += int340x_thermal.o
> > > diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> > > new file mode 100644
> > > index 0000000..b27bc1c
> > > --- /dev/null
> > > +++ b/drivers/acpi/acpi_apd.c
> > > @@ -0,0 +1,201 @@
> > > +/*
> > > + * AMD ACPI support for ACPI2platform device.
> > > + *
> > > + * Copyright (c) 2014,2015 AMD Corporation.
> > > + * Authors: Ken Xue <Ken.Xue@amd.com>
> > > + *     Wu, Jeff <Jeff.Wu@amd.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/clk-provider.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_domain.h>
> > > +#include <linux/clkdev.h>
> > > +#include <linux/acpi.h>
> > > +#include <linux/err.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/pm.h>
> > > +
> > > +#include "internal.h"
> > > +
> > > +ACPI_MODULE_NAME("acpi_apd");
> > > +struct apd_private_data;
> > > +
> > > +/**
> > 
> > /*, or use description on per flag basis.
> > 
OK.

> > > + * device flags of acpi_apd_dev_desc.
> > > + * ACPI_APD_SYSFS : add device attributes in sysfs
> > > + * ACPI_APD_PM : attach power domain to device
> > > + * ACPI_APD_PM_ON : power on device when attach power domain
> > > + */
> > > +#define ACPI_APD_SYSFS BIT(0)
> > > +#define ACPI_APD_PM    BIT(1)
> > > +#define ACPI_APD_PM_ON BIT(2)
> > > +
> > > +/**
> > > + * struct apd_device_desc - a descriptor for apd device
> > > + * @flags: device flags like ACPI_APD_SYSFS ACPI_APD_PM ACPI_APD_PM_ON
> > 
> > %ACPI_APD_SYSFS, %ACPI_APD_PM, %ACPI_APD_APM_ON.
> > 
> > Could it be combination? State this explicitly.
> > 
OK.

> > > + * @fixed_clk_rate: fixed rate input clock source for acpi device;
> > > + *                     0 means no fixed rate input clock source
> > > + * @setup: a hook routine to set device resource during create platform device
> > > + *
> > > + * device description defined as acpi_device_id.driver_data
> > 
> > d -> D
> > 
OK.

> > > + */
> > > +struct apd_device_desc {
> > > +       unsigned int flags;
> > > +       unsigned int fixed_clk_rate;
> > > +       int (*setup)(struct apd_private_data *pdata);
> > > +};
> > > +
> > > +struct apd_private_data {
> > > +       struct clk *clk;
> > > +       struct acpi_device *adev;
> > > +       const struct apd_device_desc *dev_desc;
> > > +};
> > > +
> > > +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> > > +#define APD_ADDR(desc) ((unsigned long)&desc)
> > > +
> > > +static int acpi_apd_setup(struct apd_private_data *pdata)
> > > +{
> > > +       const struct apd_device_desc *dev_desc = pdata->dev_desc;
> > > +       struct clk *clk = ERR_PTR(-ENODEV);
> > > +
> > > +       if (dev_desc->fixed_clk_rate) {
> > > +               clk = clk_register_fixed_rate(&pdata->adev->dev,
> > > +                                       dev_name(&pdata->adev->dev),
> > > +                                       NULL, CLK_IS_ROOT,
> > > +                                       dev_desc->fixed_clk_rate);
> > > +               clk_register_clkdev(clk, NULL, dev_name(&pdata->adev->dev));
> > > +               pdata->clk = clk;
> > > +       }
> > 
> > What about
> > if (!…clk_rate)
> >  return 0;
> > …
OK.

> > return 0; ?
> > 
> > And question why we need int to be returned at all?
> >
It just provides a possibility to return error. In current use case,
there is no need to return. But i believe it is better to reserve this
design.

>  
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static struct apd_device_desc cz_i2c_desc = {
> > > +       .setup = acpi_apd_setup,
> > > +       .fixed_clk_rate = 133000000,
> > > +};
> > > +
> > > +static struct apd_device_desc cz_uart_desc = {
> > > +       .setup = acpi_apd_setup,
> > > +       .fixed_clk_rate = 48000000,
> > > +};
> > > +
> > > +#else
> > > +
> > > +#define APD_ADDR(desc) (0UL)
> > > +
> > > +#endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
> > > +
> > > +static int acpi_apd_create_device(struct acpi_device *adev,
> > > +                                  const struct acpi_device_id *id)
> > > +{
> > > +       const struct apd_device_desc *dev_desc;
> > > +       struct apd_private_data *pdata;
> > > +       struct platform_device *pdev;
> > > +       int ret;
> > > +
> > > +       dev_desc = (struct apd_device_desc *)id->driver_data;
> > 
> > You may move this assignment to the definition block. And if it
> > doesn't fit one line you may cast to (void *), though I don't know if
> > Rafael likes such trick.
> > 
OK.

> > > +       if (!dev_desc) {
> > > +               pdev = acpi_create_platform_device(adev);
> > > +               return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
> > 
> > I guess it worth to add a comment what means return code here.
> > 
OK.

> > > +       }
> > > +       pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > > +       if (!pdata)
> > > +               return -ENOMEM;
> > > +
> > > +       pdata->adev = adev;
> > > +       pdata->dev_desc = dev_desc;
> > > +
> > > +       if (dev_desc->setup) {
> > > +               ret = dev_desc->setup(pdata);
> > > +               if (ret)
> > > +                       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 const struct acpi_device_id acpi_apd_device_ids[] = {
> > > +       /* Generic apd devices */
> > > +       { "AMD0010", APD_ADDR(cz_i2c_desc) },
> > > +       { "AMD0020", APD_ADDR(cz_uart_desc) },
> > > +       { }
> > > +};
> > > +
> > > +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> > > +static int acpi_apd_platform_notify(struct notifier_block *nb,
> > > +                                    unsigned long action, void *data)
> > > +{
> > > +       struct platform_device *pdev = to_platform_device(data);
> > > +       const struct acpi_device_id *id = NULL;
> > 
> > Redundant assignment.
> > 
OK.

> > > +       struct apd_private_data *pdata;
> > > +       struct acpi_device *adev;
> > > +       bool power_on;
> > > +       int ret;
> > > +
> > > +       id = acpi_match_device(acpi_apd_device_ids, &pdev->dev);
> > > +       if (!id || !id->driver_data)
> > > +               return 0;
> > > +
> > > +       if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
> > 
> > Better of course to do like
> > status = …
> > if (status) [don't remember how to check status with ACPI API]
> >  return 0;
> > 
I use same coding style as LPSS and other examples in current kernel.
I don't think it is really need to be changed.

> > > +               return 0;
> > > +
> > > +       pdata = acpi_driver_data(adev);
> > > +       if (!pdata || !pdata->dev_desc)
> > > +               return 0;
> > > +
> > > +       switch (action) {
> > > +       case BUS_NOTIFY_ADD_DEVICE:
> > > +               if (pdata->dev_desc->flags & ACPI_APD_PM) {
> > > +                       power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON);
> > > +                       ret = dev_pm_domain_attach(&pdev->dev, power_on);
> > > +
> > > +                       if (ret)
> > > +                               dev_dbg(&pdev->dev,
> > > +                               "failed to attached pm domain (%d)\n", ret);
> > > +               }
> > > +               break;
> > > +       case BUS_NOTIFY_DEL_DEVICE:
> > > +               if (pdata->dev_desc->flags & ACPI_APD_PM) {
> > > +                       power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON);
> > > +                       dev_pm_domain_detach(&pdev->dev, power_on);
> > > +               }
> > > +               break;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static struct notifier_block acpi_apd_nb = {
> > > +       .notifier_call = acpi_apd_platform_notify,
> > > +};
> > > +#endif
> > > +
> > > +static struct acpi_scan_handler apd_handler = {
> > > +       .ids = acpi_apd_device_ids,
> > > +       .attach = acpi_apd_create_device,
> > > +};
> > > +
> > > +void __init acpi_apd_init(void)
> > > +{
> > > +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> > > +       bus_register_notifier(&platform_bus_type, &acpi_apd_nb);
> > > +#endif
> > > +
> > > +       acpi_scan_add_handler(&apd_handler);
> > > +}
> > > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > > index 163e82f..c24ae9d 100644
> > > --- a/drivers/acpi/internal.h
> > > +++ b/drivers/acpi/internal.h
> > > @@ -68,6 +68,8 @@ static inline void acpi_debugfs_init(void) { return; }
> > >  #endif
> > >  void acpi_lpss_init(void);
> > >
> > > +void acpi_apd_init(void);
> > > +
> > >  acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
> > >  bool acpi_queue_hotplug_work(struct work_struct *work);
> > >  void acpi_device_hotplug(struct acpi_device *adev, u32 src);
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index dc4d896..bbca783 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -2544,6 +2544,7 @@ int __init acpi_scan_init(void)
> > >         acpi_pci_link_init();
> > >         acpi_processor_init();
> > >         acpi_lpss_init();
> > > +       acpi_apd_init();
> > >         acpi_cmos_rtc_init();
> > >         acpi_container_init();
> > >         acpi_memory_hotplug_init();
> > > --
> > > 1.9.1
> > >
> > >
> > >
> > 
> > 
> > 
> > 
> 



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

* [PATCH V4] acpi:apd:add AMD ACPI2Platform device support for x86 system.
  2015-02-04  3:40     ` Ken Xue
@ 2015-02-05  3:07       ` Ken Xue
  2015-02-05 12:02         ` mika.westerberg
  0 siblings, 1 reply; 10+ messages in thread
From: Ken Xue @ 2015-02-05  3:07 UTC (permalink / raw)
  To: Rafael J. Wysocki, Andy Shevchenko, mika.westerberg
  Cc: linux-acpi, linux-kernel

>From d22789f089ee644413a144633f368f45cb0ac9d8 Mon Sep 17 00:00:00 2001
From: Ken Xue <Ken.Xue@amd.com>
Date: Thu, 5 Feb 2015 11:04:44 +0800
Subject: [PATCH] acpi:apd:add AMD ACPI2Platform device support for x86 system.

This new feature is to interpret AMD specific ACPI device to
platform device such as I2C, UART found on AMD CZ and later chipsets. It
based on example intel LPSS. Now, it can support AMD I2C & UART.
---
 arch/x86/Kconfig        |  11 ++++
 drivers/acpi/Makefile   |   2 +-
 drivers/acpi/acpi_apd.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |   2 +
 drivers/acpi/scan.c     |   1 +
 5 files changed, 165 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/acpi_apd.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0dc9d01..3e15cee 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -496,6 +496,17 @@ config X86_INTEL_LPSS
 	  things like clock tree (common clock framework) and pincontrol
 	  which are needed by the LPSS peripheral drivers.
 
+config X86_AMD_PLATFORM_DEVICE
+	bool "AMD ACPI2Platform devices support"
+	depends on ACPI
+	select COMMON_CLK
+	select PINCTRL
+	---help---
+	  Select to interpret AMD specific ACPI device to platform device
+	  such as I2C, UART, GPIO found on AMD Carrizo and later chipsets.
+	  I2C and UART depend on COMMON_CLK to set clock. GPIO driver is
+	  implemented under PINCTRL subsystem.
+
 config IOSF_MBI
 	tristate "Intel SoC IOSF Sideband support for SoC platforms"
 	depends on PCI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index f74317c..0071141 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_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
 acpi-y				+= int340x_thermal.o
diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
new file mode 100644
index 0000000..3984ea9
--- /dev/null
+++ b/drivers/acpi/acpi_apd.c
@@ -0,0 +1,150 @@
+/*
+ * AMD ACPI support for ACPI2platform device.
+ *
+ * Copyright (c) 2014,2015 AMD Corporation.
+ * Authors: Ken Xue <Ken.Xue@amd.com>
+ *	Wu, Jeff <Jeff.Wu@amd.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/clk-provider.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/clkdev.h>
+#include <linux/acpi.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/pm.h>
+
+#include "internal.h"
+
+ACPI_MODULE_NAME("acpi_apd");
+struct apd_private_data;
+
+/**
+ * ACPI_APD_SYSFS : add device attributes in sysfs
+ * ACPI_APD_PM : attach power domain to device
+ */
+#define ACPI_APD_SYSFS	BIT(0)
+#define ACPI_APD_PM	BIT(1)
+
+/**
+ * struct apd_device_desc - a descriptor for apd device
+ * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM
+ * @fixed_clk_rate: fixed rate input clock source for acpi device;
+ *			0 means no fixed rate input clock source
+ * @setup: a hook routine to set device resource during create platform device
+ *
+ * Device description defined as acpi_device_id.driver_data
+ */
+struct apd_device_desc {
+	unsigned int flags;
+	unsigned int fixed_clk_rate;
+	int (*setup)(struct apd_private_data *pdata);
+};
+
+struct apd_private_data {
+	struct clk *clk;
+	struct acpi_device *adev;
+	const struct apd_device_desc *dev_desc;
+};
+
+#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
+#define APD_ADDR(desc)	((unsigned long)&desc)
+
+static int acpi_apd_setup(struct apd_private_data *pdata)
+{
+	const struct apd_device_desc *dev_desc = pdata->dev_desc;
+	struct clk *clk = ERR_PTR(-ENODEV);
+
+	if (dev_desc->fixed_clk_rate) {
+		clk = clk_register_fixed_rate(&pdata->adev->dev,
+					dev_name(&pdata->adev->dev),
+					NULL, CLK_IS_ROOT,
+					dev_desc->fixed_clk_rate);
+		clk_register_clkdev(clk, NULL, dev_name(&pdata->adev->dev));
+		pdata->clk = clk;
+	}
+
+	return 0;
+}
+
+static struct apd_device_desc cz_i2c_desc = {
+	.setup = acpi_apd_setup,
+	.fixed_clk_rate = 133000000,
+};
+
+static struct apd_device_desc cz_uart_desc = {
+	.setup = acpi_apd_setup,
+	.fixed_clk_rate = 48000000,
+};
+
+#else
+
+#define APD_ADDR(desc) (0UL)
+
+#endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
+
+/**
+* Create platform device during acpi scan attach handle.
+* Return value > 0 on success of creating device.
+*/
+static int acpi_apd_create_device(struct acpi_device *adev,
+				   const struct acpi_device_id *id)
+{
+	const struct apd_device_desc *dev_desc = (void *)id->driver_data;
+	struct apd_private_data *pdata;
+	struct platform_device *pdev;
+	int ret;
+
+	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;
+
+	pdata->adev = adev;
+	pdata->dev_desc = dev_desc;
+
+	if (dev_desc->setup) {
+		ret = dev_desc->setup(pdata);
+		if (ret)
+			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 const struct acpi_device_id acpi_apd_device_ids[] = {
+	/* Generic apd devices */
+	{ "AMD0010", APD_ADDR(cz_i2c_desc) },
+	{ "AMD0020", APD_ADDR(cz_uart_desc) },
+	{ "AMD0030", },
+	{ }
+};
+
+static struct acpi_scan_handler apd_handler = {
+	.ids = acpi_apd_device_ids,
+	.attach = acpi_apd_create_device,
+};
+
+void __init acpi_apd_init(void)
+{
+	acpi_scan_add_handler(&apd_handler);
+}
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 163e82f..c24ae9d 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -68,6 +68,8 @@ static inline void acpi_debugfs_init(void) { return; }
 #endif
 void acpi_lpss_init(void);
 
+void acpi_apd_init(void);
+
 acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
 bool acpi_queue_hotplug_work(struct work_struct *work);
 void acpi_device_hotplug(struct acpi_device *adev, u32 src);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index dc4d896..bbca783 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2544,6 +2544,7 @@ int __init acpi_scan_init(void)
 	acpi_pci_link_init();
 	acpi_processor_init();
 	acpi_lpss_init();
+	acpi_apd_init();
 	acpi_cmos_rtc_init();
 	acpi_container_init();
 	acpi_memory_hotplug_init();
-- 
1.9.1




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

* Re: [PATCH V4] acpi:apd:add AMD ACPI2Platform device support for x86 system.
  2015-02-05  3:07       ` [PATCH V4] " Ken Xue
@ 2015-02-05 12:02         ` mika.westerberg
  2015-02-05 15:09           ` Rafael J. Wysocki
  0 siblings, 1 reply; 10+ messages in thread
From: mika.westerberg @ 2015-02-05 12:02 UTC (permalink / raw)
  To: Ken Xue; +Cc: Rafael J. Wysocki, Andy Shevchenko, linux-acpi, linux-kernel

On Thu, Feb 05, 2015 at 11:07:17AM +0800, Ken Xue wrote:
> >From d22789f089ee644413a144633f368f45cb0ac9d8 Mon Sep 17 00:00:00 2001
> From: Ken Xue <Ken.Xue@amd.com>
> Date: Thu, 5 Feb 2015 11:04:44 +0800
> Subject: [PATCH] acpi:apd:add AMD ACPI2Platform device support for x86 system.
> 
> This new feature is to interpret AMD specific ACPI device to
> platform device such as I2C, UART found on AMD CZ and later chipsets. It
> based on example intel LPSS. Now, it can support AMD I2C & UART.

You are missing Signed-off-by here.

Anyway looks good to me,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

* Re: [PATCH V4] acpi:apd:add AMD ACPI2Platform device support for x86 system.
  2015-02-05 12:02         ` mika.westerberg
@ 2015-02-05 15:09           ` Rafael J. Wysocki
  0 siblings, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2015-02-05 15:09 UTC (permalink / raw)
  To: mika.westerberg, Ken Xue; +Cc: Andy Shevchenko, linux-acpi, linux-kernel

On Thursday, February 05, 2015 02:02:14 PM mika.westerberg@linux.intel.com wrote:
> On Thu, Feb 05, 2015 at 11:07:17AM +0800, Ken Xue wrote:
> > >From d22789f089ee644413a144633f368f45cb0ac9d8 Mon Sep 17 00:00:00 2001
> > From: Ken Xue <Ken.Xue@amd.com>
> > Date: Thu, 5 Feb 2015 11:04:44 +0800
> > Subject: [PATCH] acpi:apd:add AMD ACPI2Platform device support for x86 system.
> > 
> > This new feature is to interpret AMD specific ACPI device to
> > platform device such as I2C, UART found on AMD CZ and later chipsets. It
> > based on example intel LPSS. Now, it can support AMD I2C & UART.
> 
> You are missing Signed-off-by here.
> 
> Anyway looks good to me,
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks!

Ken, can you please resend this one with a sign-off?  I can't apply it
otherwise.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

* Re: [PATCH V4] acpi:apd:add AMD ACPI2Platform device support for x86 system.
  2015-02-06  9:37 ` mika.westerberg
@ 2015-02-08 23:21   ` Rafael J. Wysocki
  0 siblings, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2015-02-08 23:21 UTC (permalink / raw)
  To: mika.westerberg; +Cc: Ken Xue, Andy Shevchenko, linux-acpi, linux-kernel

On Friday, February 06, 2015 11:37:40 AM mika.westerberg@linux.intel.com wrote:
> On Fri, Feb 06, 2015 at 08:34:52AM +0800, Ken Xue wrote:
> > >From 1a6a3a5c0815cb1f52ec0a2b9601edfa9bfebe81 Mon Sep 17 00:00:00 2001
> > From: Ken Xue <Ken.Xue@amd.com>
> > Date: Fri, 6 Feb 2015 08:27:51 +0800
> > Subject: [PATCH] acpi:apd:add AMD ACPI2Platform device support for x86 system.
> > 
> > This new feature is to interpret AMD specific ACPI device to
> > platform device such as I2C, UART, GPIO found on AMD CZ and
> > later chipsets. It based on example intel LPSS. Now, it can
> > support AMD I2C, UART and GPIO.
> > 
> > Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Applied, thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

* Re: [PATCH V4] acpi:apd:add AMD ACPI2Platform device support for x86 system.
  2015-02-06  0:34 Ken Xue
@ 2015-02-06  9:37 ` mika.westerberg
  2015-02-08 23:21   ` Rafael J. Wysocki
  0 siblings, 1 reply; 10+ messages in thread
From: mika.westerberg @ 2015-02-06  9:37 UTC (permalink / raw)
  To: Ken Xue; +Cc: Rafael J. Wysocki, Andy Shevchenko, linux-acpi, linux-kernel

On Fri, Feb 06, 2015 at 08:34:52AM +0800, Ken Xue wrote:
> >From 1a6a3a5c0815cb1f52ec0a2b9601edfa9bfebe81 Mon Sep 17 00:00:00 2001
> From: Ken Xue <Ken.Xue@amd.com>
> Date: Fri, 6 Feb 2015 08:27:51 +0800
> Subject: [PATCH] acpi:apd:add AMD ACPI2Platform device support for x86 system.
> 
> This new feature is to interpret AMD specific ACPI device to
> platform device such as I2C, UART, GPIO found on AMD CZ and
> later chipsets. It based on example intel LPSS. Now, it can
> support AMD I2C, UART and GPIO.
> 
> Signed-off-by: Ken Xue <Ken.Xue@amd.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

* [PATCH V4] acpi:apd:add AMD ACPI2Platform device support for x86 system.
@ 2015-02-06  0:34 Ken Xue
  2015-02-06  9:37 ` mika.westerberg
  0 siblings, 1 reply; 10+ messages in thread
From: Ken Xue @ 2015-02-06  0:34 UTC (permalink / raw)
  To: Rafael J. Wysocki, Andy Shevchenko, mika.westerberg
  Cc: linux-acpi, linux-kernel

>From 1a6a3a5c0815cb1f52ec0a2b9601edfa9bfebe81 Mon Sep 17 00:00:00 2001
From: Ken Xue <Ken.Xue@amd.com>
Date: Fri, 6 Feb 2015 08:27:51 +0800
Subject: [PATCH] acpi:apd:add AMD ACPI2Platform device support for x86 system.

This new feature is to interpret AMD specific ACPI device to
platform device such as I2C, UART, GPIO found on AMD CZ and
later chipsets. It based on example intel LPSS. Now, it can
support AMD I2C, UART and GPIO.

Signed-off-by: Ken Xue <Ken.Xue@amd.com>
---
 arch/x86/Kconfig        |  11 ++++
 drivers/acpi/Makefile   |   2 +-
 drivers/acpi/acpi_apd.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |   2 +
 drivers/acpi/scan.c     |   1 +
 5 files changed, 165 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/acpi_apd.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0dc9d01..3e15cee 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -496,6 +496,17 @@ config X86_INTEL_LPSS
 	  things like clock tree (common clock framework) and pincontrol
 	  which are needed by the LPSS peripheral drivers.
 
+config X86_AMD_PLATFORM_DEVICE
+	bool "AMD ACPI2Platform devices support"
+	depends on ACPI
+	select COMMON_CLK
+	select PINCTRL
+	---help---
+	  Select to interpret AMD specific ACPI device to platform device
+	  such as I2C, UART, GPIO found on AMD Carrizo and later chipsets.
+	  I2C and UART depend on COMMON_CLK to set clock. GPIO driver is
+	  implemented under PINCTRL subsystem.
+
 config IOSF_MBI
 	tristate "Intel SoC IOSF Sideband support for SoC platforms"
 	depends on PCI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index f74317c..0071141 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_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
 acpi-y				+= int340x_thermal.o
diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
new file mode 100644
index 0000000..3984ea9
--- /dev/null
+++ b/drivers/acpi/acpi_apd.c
@@ -0,0 +1,150 @@
+/*
+ * AMD ACPI support for ACPI2platform device.
+ *
+ * Copyright (c) 2014,2015 AMD Corporation.
+ * Authors: Ken Xue <Ken.Xue@amd.com>
+ *	Wu, Jeff <Jeff.Wu@amd.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/clk-provider.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/clkdev.h>
+#include <linux/acpi.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/pm.h>
+
+#include "internal.h"
+
+ACPI_MODULE_NAME("acpi_apd");
+struct apd_private_data;
+
+/**
+ * ACPI_APD_SYSFS : add device attributes in sysfs
+ * ACPI_APD_PM : attach power domain to device
+ */
+#define ACPI_APD_SYSFS	BIT(0)
+#define ACPI_APD_PM	BIT(1)
+
+/**
+ * struct apd_device_desc - a descriptor for apd device
+ * @flags: device flags like %ACPI_APD_SYSFS, %ACPI_APD_PM
+ * @fixed_clk_rate: fixed rate input clock source for acpi device;
+ *			0 means no fixed rate input clock source
+ * @setup: a hook routine to set device resource during create platform device
+ *
+ * Device description defined as acpi_device_id.driver_data
+ */
+struct apd_device_desc {
+	unsigned int flags;
+	unsigned int fixed_clk_rate;
+	int (*setup)(struct apd_private_data *pdata);
+};
+
+struct apd_private_data {
+	struct clk *clk;
+	struct acpi_device *adev;
+	const struct apd_device_desc *dev_desc;
+};
+
+#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
+#define APD_ADDR(desc)	((unsigned long)&desc)
+
+static int acpi_apd_setup(struct apd_private_data *pdata)
+{
+	const struct apd_device_desc *dev_desc = pdata->dev_desc;
+	struct clk *clk = ERR_PTR(-ENODEV);
+
+	if (dev_desc->fixed_clk_rate) {
+		clk = clk_register_fixed_rate(&pdata->adev->dev,
+					dev_name(&pdata->adev->dev),
+					NULL, CLK_IS_ROOT,
+					dev_desc->fixed_clk_rate);
+		clk_register_clkdev(clk, NULL, dev_name(&pdata->adev->dev));
+		pdata->clk = clk;
+	}
+
+	return 0;
+}
+
+static struct apd_device_desc cz_i2c_desc = {
+	.setup = acpi_apd_setup,
+	.fixed_clk_rate = 133000000,
+};
+
+static struct apd_device_desc cz_uart_desc = {
+	.setup = acpi_apd_setup,
+	.fixed_clk_rate = 48000000,
+};
+
+#else
+
+#define APD_ADDR(desc) (0UL)
+
+#endif /* CONFIG_X86_AMD_PLATFORM_DEVICE */
+
+/**
+* Create platform device during acpi scan attach handle.
+* Return value > 0 on success of creating device.
+*/
+static int acpi_apd_create_device(struct acpi_device *adev,
+				   const struct acpi_device_id *id)
+{
+	const struct apd_device_desc *dev_desc = (void *)id->driver_data;
+	struct apd_private_data *pdata;
+	struct platform_device *pdev;
+	int ret;
+
+	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;
+
+	pdata->adev = adev;
+	pdata->dev_desc = dev_desc;
+
+	if (dev_desc->setup) {
+		ret = dev_desc->setup(pdata);
+		if (ret)
+			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 const struct acpi_device_id acpi_apd_device_ids[] = {
+	/* Generic apd devices */
+	{ "AMD0010", APD_ADDR(cz_i2c_desc) },
+	{ "AMD0020", APD_ADDR(cz_uart_desc) },
+	{ "AMD0030", },
+	{ }
+};
+
+static struct acpi_scan_handler apd_handler = {
+	.ids = acpi_apd_device_ids,
+	.attach = acpi_apd_create_device,
+};
+
+void __init acpi_apd_init(void)
+{
+	acpi_scan_add_handler(&apd_handler);
+}
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 163e82f..c24ae9d 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -68,6 +68,8 @@ static inline void acpi_debugfs_init(void) { return; }
 #endif
 void acpi_lpss_init(void);
 
+void acpi_apd_init(void);
+
 acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
 bool acpi_queue_hotplug_work(struct work_struct *work);
 void acpi_device_hotplug(struct acpi_device *adev, u32 src);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index dc4d896..bbca783 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2544,6 +2544,7 @@ int __init acpi_scan_init(void)
 	acpi_pci_link_init();
 	acpi_processor_init();
 	acpi_lpss_init();
+	acpi_apd_init();
 	acpi_cmos_rtc_init();
 	acpi_container_init();
 	acpi_memory_hotplug_init();
-- 
1.9.1




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

end of thread, other threads:[~2015-02-08 22:58 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-03  1:54 [PATCH V3] acpi:apd:add AMD ACPI2Platform device support for x86 system Ken Xue
2015-02-03 10:06 ` Andy Shevchenko
2015-02-03 22:07   ` Rafael J. Wysocki
2015-02-04  3:40     ` Ken Xue
2015-02-05  3:07       ` [PATCH V4] " Ken Xue
2015-02-05 12:02         ` mika.westerberg
2015-02-05 15:09           ` Rafael J. Wysocki
2015-02-06  0:34 Ken Xue
2015-02-06  9:37 ` mika.westerberg
2015-02-08 23:21   ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).