Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933485AbbBBNEP (ORCPT ); Mon, 2 Feb 2015 08:04:15 -0500 Received: from mga09.intel.com ([134.134.136.24]:60556 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933452AbbBBNEF (ORCPT ); Mon, 2 Feb 2015 08:04:05 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,506,1418112000"; d="scan'208";a="671470993" Date: Mon, 2 Feb 2015 15:03:57 +0200 From: "mika.westerberg@linux.intel.com" To: Ken Xue Cc: "Rafael J. Wysocki" , "andy.shevchenko@gmail.com" , "linux-acpi@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH V2] acpi:apd:add AMD ACPI2Platform device support for x86 system. Message-ID: <20150202130356.GS22740@lahna.fi.intel.com> References: <1422870652.2528.6.camel@kxue-X58A-UD3R> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1422870652.2528.6.camel@kxue-X58A-UD3R> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9448 Lines: 335 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 > 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 > --- > 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 > + * Wu, Jeff > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/