[PATCH V2] acpi:apd:add AMD ACPI2Platform device support for x86 system.

[PATCH V2] acpi:apd:add AMD ACPI2Platform device support for x86 system.

[Ken Xue]

>From b9654ecbfaebde00aee746a024eec9fe8de24b97 Mon Sep 17 00:00:00 2001
From: Ken Xue <Ken.Xue@amd.com>
Date: Mon, 2 Feb 2015 17:32:24 +0800
Subject: [PATCH] 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 | 208 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |   2 +
 drivers/acpi/scan.c     |   1 +
 5 files changed, 223 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..b875ef6
--- /dev/null
+++ b/drivers/acpi/acpi_apd.c
@@ -0,0 +1,208 @@
+/*
+ * 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;
+	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)
+{
+	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)
+{
+	struct apd_private_data *pdata;
+	struct apd_device_desc *dev_desc;
+	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 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);
+	struct apd_private_data *pdata;
+	const struct acpi_device_id *id = NULL;
+	struct acpi_device *adev;
+	int ret;
+
+	id = acpi_match_device(acpi_apd_device_ids, &pdev->dev);
+	if (!id || !id->driver_data)
+		return 0;
+
+	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) {
+			if (pdata->dev_desc->flags & ACPI_APD_PM_ON)
+				ret = dev_pm_domain_attach(&pdev->dev, true);
+			else
+				ret = dev_pm_domain_attach(&pdev->dev, false);
+
+			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) {
+			if (pdata->dev_desc->flags & ACPI_APD_PM_ON)
+				dev_pm_domain_detach(&pdev->dev, true);
+			else
+				dev_pm_domain_detach(&pdev->dev, false);
+		}
+		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

Re: [PATCH V2] acpi:apd:add AMD ACPI2Platform device support for x86 system.

[mika.westerberg]

On Mon, Feb 02, 2015 at 05:50:52PM +0800, Ken Xue wrote:
> >From b9654ecbfaebde00aee746a024eec9fe8de24b97 Mon Sep 17 00:00:00 2001
> From: Ken Xue <Ken.Xue@amd.com>
> Date: Mon, 2 Feb 2015 17:32:24 +0800
> Subject: [PATCH] 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.

Looks good to me. There are few smallish issues still, see below.

> Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> ---
>  arch/x86/Kconfig        |  11 +++
>  drivers/acpi/Makefile   |   2 +-
>  drivers/acpi/acpi_apd.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/internal.h |   2 +
>  drivers/acpi/scan.c     |   1 +
>  5 files changed, 223 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..b875ef6
> --- /dev/null
> +++ b/drivers/acpi/acpi_apd.c
> @@ -0,0 +1,208 @@
> +/*
> + * 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.
> + */

Empty line here.

> +#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"
> +
> +

Delete this empty line.

> +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;
> +	struct apd_device_desc *dev_desc;
> +};
> +
> +

Ditto.

> +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> +#define APD_ADDR(desc)	((unsigned long)&desc)
> +
> +static int acpi_apd_setup(struct apd_private_data *pdata)
> +{
> +	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 = {

const?

> +	.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)
> +{
> +	struct apd_private_data *pdata;
> +	struct apd_device_desc *dev_desc;
> +	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 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);
> +	struct apd_private_data *pdata;
> +	const struct acpi_device_id *id = NULL;
> +	struct acpi_device *adev;
> +	int ret;
> +
> +	id = acpi_match_device(acpi_apd_device_ids, &pdev->dev);
> +	if (!id || !id->driver_data)
> +		return 0;
> +
> +	if (!id || !id->driver_data)
> +		return 0;

Doubled check, remove.

> +
> +	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) {
> +			if (pdata->dev_desc->flags & ACPI_APD_PM_ON)
> +				ret = dev_pm_domain_attach(&pdev->dev, true);
> +			else
> +				ret = dev_pm_domain_attach(&pdev->dev, false);

How about:

	power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON)
	ret = dev_pm_domain_attach(&pdev->dev, power_on);

?

Furthermore I think this is not needed at all. If you check
platform_drv_probe() it calls dev_pm_domain_attach() already.

> +
> +			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) {
> +			if (pdata->dev_desc->flags & ACPI_APD_PM_ON)
> +				dev_pm_domain_detach(&pdev->dev, true);
> +			else
> +				dev_pm_domain_detach(&pdev->dev, false);

Ditto.

> +		}
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct notifier_block acpi_apd_nb = {

const?

> +	.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
> 
> 

Re: [PATCH V2] acpi:apd:add AMD ACPI2Platform device support for x86 system.

[Ken Xue]

On Mon, 2015-02-02 at 15:03 +0200, mika.westerberg@linux.intel.com
wrote:
> On Mon, Feb 02, 2015 at 05:50:52PM +0800, Ken Xue wrote:
> > >From b9654ecbfaebde00aee746a024eec9fe8de24b97 Mon Sep 17 00:00:00 2001
> > From: Ken Xue <Ken.Xue@amd.com>
> > Date: Mon, 2 Feb 2015 17:32:24 +0800
> > Subject: [PATCH] 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.
> 
> Looks good to me. There are few smallish issues still, see below.
> 
...

> > +	switch (action) {
> > +	case BUS_NOTIFY_ADD_DEVICE:
> > +		if (pdata->dev_desc->flags & ACPI_APD_PM) {
> > +			if (pdata->dev_desc->flags & ACPI_APD_PM_ON)
> > +				ret = dev_pm_domain_attach(&pdev->dev, true);
> > +			else
> > +				ret = dev_pm_domain_attach(&pdev->dev, false);
> 
> How about:
> 
> 	power_on = !!(pdata->dev_desc->flags & ACPI_APD_PM_ON)
> 	ret = dev_pm_domain_attach(&pdev->dev, power_on);
> 
> ?
good 

> 
> Furthermore I think this is not needed at all. If you check
> platform_drv_probe() it calls dev_pm_domain_attach() already.
> 
as you said, platform_drv_probe calls dev_pm_domain_attach(). but
platform_drv_probe just is a default probe routine. Not all platform
device drivers use this probe routine. so, codes here may be still
necessary.

Re: [PATCH V2] acpi:apd:add AMD ACPI2Platform device support for x86 system.

[mika.westerberg]

On Tue, Feb 03, 2015 at 09:04:55AM +0800, Ken Xue wrote:
> as you said, platform_drv_probe calls dev_pm_domain_attach(). but
> platform_drv_probe just is a default probe routine. Not all platform
> device drivers use this probe routine. so, codes here may be still
> necessary.

Are you saying that for platform devices there is some other path to get
a driver probed, other than platform_drv_probe()? Can you point me to
it?

Re: [PATCH V2] acpi:apd:add AMD ACPI2Platform device support for x86 system.

[Ken Xue]

On Tue, 2015-02-03 at 11:53 +0200, mika.westerberg@linux.intel.com
wrote:
> On Tue, Feb 03, 2015 at 09:04:55AM +0800, Ken Xue wrote:
> > as you said, platform_drv_probe calls dev_pm_domain_attach(). but
> > platform_drv_probe just is a default probe routine. Not all platform
> > device drivers use this probe routine. so, codes here may be still
> > necessary.
> 
> Are you saying that for platform devices there is some other path to get
> a driver probed, other than platform_drv_probe()? Can you point me to
> it?
>From the codes, i can see there is a possibility that drv->driver.probe
may not be set in __platform_driver_register. But i really can not point
out a use case that platform device driver without probe.
so, it is safe to remove "dev_pm_domain_attach" in
"acpi_apd_platform_notify". right?

Re: [PATCH V2] acpi:apd:add AMD ACPI2Platform device support for x86 system.

[mika.westerberg]

On Tue, Feb 03, 2015 at 05:55:25PM +0800, Ken Xue wrote:
> On Tue, 2015-02-03 at 11:53 +0200, mika.westerberg@linux.intel.com
> wrote:
> > On Tue, Feb 03, 2015 at 09:04:55AM +0800, Ken Xue wrote:
> > > as you said, platform_drv_probe calls dev_pm_domain_attach(). but
> > > platform_drv_probe just is a default probe routine. Not all platform
> > > device drivers use this probe routine. so, codes here may be still
> > > necessary.
> > 
> > Are you saying that for platform devices there is some other path to get
> > a driver probed, other than platform_drv_probe()? Can you point me to
> > it?
> >From the codes, i can see there is a possibility that drv->driver.probe
> may not be set in __platform_driver_register. But i really can not point
> out a use case that platform device driver without probe.
> so, it is safe to remove "dev_pm_domain_attach" in
> "acpi_apd_platform_notify". right?

In that case, I think so, yes.