| Message ID | 20190405203558.19160-3-rajatja@google.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
| Series | [v3,1/3] platform/x86: intel_pmc_core: Convert to a platform_driver | expand |
On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <rajatja@google.com> wrote: > > Add code to instantiate the pmc_core platform device and thus attach to > the driver, if the ACPI device for the same ("INT33A1") is not present > in a system where it should be. This was discussed here: > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1966991.html > > +#include <linux/acpi.h> > +#include <linux/platform_device.h> > + > +#include <asm/cpu_device_id.h> > +#include <asm/intel-family.h> > + > +static struct platform_device pmc_core_device = { > + .name = "pmc_core", > +}; > + > +static int dummy; > +/* > + * INTEL_CPU_FAM6 macro won't take NULL for driver_data (uses &driver_data), > + * thus provide a dummy driver_data. > + */ You may use pmc_core_device instead, right? > +static const struct x86_cpu_id intel_pmc_core_ids[] = { > + INTEL_CPU_FAM6(SKYLAKE_MOBILE, dummy), > + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, dummy), > + INTEL_CPU_FAM6(KABYLAKE_MOBILE, dummy), > + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, dummy), > + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, dummy), > + INTEL_CPU_FAM6(ICELAKE_MOBILE, dummy), > + {} > +}; > +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids); > + > +static int __init pmc_core_platform_init(void) > +{ > + int ret; > + > + /* Skip creating the platform device if ACPI already has a device */ > + if (acpi_dev_present("INT33A1", NULL, -1) || > + !x86_match_cpu(intel_pmc_core_ids)) > + return -ENODEV; Split it to one conditional per line. > + > + ret = platform_device_register(&pmc_core_device); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static void __exit pmc_core_platform_exit(void) > +{ > + if (!acpi_dev_present("INT33A1", NULL, -1) && > + x86_match_cpu(intel_pmc_core_ids)) Redundant check. > + platform_device_unregister(&pmc_core_device); > +}
On Fri, Apr 05, 2019 at 01:35:58PM -0700, Rajat Jain wrote: > Add code to instantiate the pmc_core platform device and thus attach to > the driver, if the ACPI device for the same ("INT33A1") is not present > in a system where it should be. This was discussed here: > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1966991.html > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > v3: (first version of *this* patch -to go with rest of v3 patchset) > v2: (does not exist) > v1: (does not exist) > > drivers/platform/x86/Makefile | 2 +- > .../platform/x86/intel_pmc_core_plat_init.c | 64 +++++++++++++++++++ > 2 files changed, 65 insertions(+), 1 deletion(-) > create mode 100644 drivers/platform/x86/intel_pmc_core_plat_init.c Can we call this intel_pmc_core_plat_drv.c instead? > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 86cb76677bc8..83d0bb9a14bb 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o > obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \ > intel_telemetry_pltdrv.o \ > intel_telemetry_debugfs.o > -obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o > +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_init.o ditto > obj-$(CONFIG_PMC_ATOM) += pmc_atom.o > obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o > obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o > diff --git a/drivers/platform/x86/intel_pmc_core_plat_init.c b/drivers/platform/x86/intel_pmc_core_plat_init.c > new file mode 100644 > index 000000000000..45be89e7586c > --- /dev/null > +++ b/drivers/platform/x86/intel_pmc_core_plat_init.c > @@ -0,0 +1,64 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* > + * Intel PMC Core platform init > + * Copyright (c) 2019, Google Inc. I'd like to consult Andy and Darren if we should also have Intel Copyright too? > + * Author - Rajat Jain > + * > + * This code instantiates platform devices for intel_pmc_core driver, only > + * on supported platforms that may not have the ACPI devices in the ACPI tables. > + * No new platforms should be added here, because we expect that new platforms > + * should all have the ACPI device, which is the preferred way of enumeration. > + */ > + > +#include <linux/acpi.h> > +#include <linux/platform_device.h> > + > +#include <asm/cpu_device_id.h> > +#include <asm/intel-family.h> > + > +static struct platform_device pmc_core_device = { > + .name = "pmc_core", > +}; > + > +static int dummy; > +/* > + * INTEL_CPU_FAM6 macro won't take NULL for driver_data (uses &driver_data), > + * thus provide a dummy driver_data. > + */ > +static const struct x86_cpu_id intel_pmc_core_ids[] = { > + INTEL_CPU_FAM6(SKYLAKE_MOBILE, dummy), > + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, dummy), > + INTEL_CPU_FAM6(KABYLAKE_MOBILE, dummy), > + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, dummy), > + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, dummy), > + INTEL_CPU_FAM6(ICELAKE_MOBILE, dummy), > + {} > +}; > +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids); > + > +static int __init pmc_core_platform_init(void) > +{ > + int ret; > + > + /* Skip creating the platform device if ACPI already has a device */ > + if (acpi_dev_present("INT33A1", NULL, -1) || > + !x86_match_cpu(intel_pmc_core_ids)) > + return -ENODEV; > + > + ret = platform_device_register(&pmc_core_device); > + if (ret) > + return ret; we can just return platform_devie_register (...) and avoid extra lines and variable. > + > + return 0; > +} > + > +static void __exit pmc_core_platform_exit(void) > +{ > + if (!acpi_dev_present("INT33A1", NULL, -1) && > + x86_match_cpu(intel_pmc_core_ids)) > + platform_device_unregister(&pmc_core_device); > +} > + > +module_init(pmc_core_platform_init); > +module_exit(pmc_core_platform_exit); > -- > 2.21.0.392.gf8f6787159e-goog >
On Mon, Apr 8, 2019 at 11:15 AM Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> wrote: > > On Fri, Apr 05, 2019 at 01:35:58PM -0700, Rajat Jain wrote: > > Add code to instantiate the pmc_core platform device and thus attach to > > the driver, if the ACPI device for the same ("INT33A1") is not present > > in a system where it should be. This was discussed here: > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1966991.html > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > --- > > v3: (first version of *this* patch -to go with rest of v3 patchset) > > v2: (does not exist) > > v1: (does not exist) > > > > drivers/platform/x86/Makefile | 2 +- > > .../platform/x86/intel_pmc_core_plat_init.c | 64 +++++++++++++++++++ > > 2 files changed, 65 insertions(+), 1 deletion(-) > > create mode 100644 drivers/platform/x86/intel_pmc_core_plat_init.c > > Can we call this intel_pmc_core_plat_drv.c instead? Sure. > > > > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > > index 86cb76677bc8..83d0bb9a14bb 100644 > > --- a/drivers/platform/x86/Makefile > > +++ b/drivers/platform/x86/Makefile > > @@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o > > obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \ > > intel_telemetry_pltdrv.o \ > > intel_telemetry_debugfs.o > > -obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o > > +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_init.o > > ditto Sure. > > > obj-$(CONFIG_PMC_ATOM) += pmc_atom.o > > obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o > > obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o > > diff --git a/drivers/platform/x86/intel_pmc_core_plat_init.c b/drivers/platform/x86/intel_pmc_core_plat_init.c > > new file mode 100644 > > index 000000000000..45be89e7586c > > --- /dev/null > > +++ b/drivers/platform/x86/intel_pmc_core_plat_init.c > > @@ -0,0 +1,64 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +/* > > + * Intel PMC Core platform init > > + * Copyright (c) 2019, Google Inc. > > I'd like to consult Andy and Darren if we should also have Intel Copyright > too? > > > + * Author - Rajat Jain > > + * > > + * This code instantiates platform devices for intel_pmc_core driver, only > > + * on supported platforms that may not have the ACPI devices in the ACPI tables. > > + * No new platforms should be added here, because we expect that new platforms > > + * should all have the ACPI device, which is the preferred way of enumeration. > > + */ > > + > > +#include <linux/acpi.h> > > +#include <linux/platform_device.h> > > + > > +#include <asm/cpu_device_id.h> > > +#include <asm/intel-family.h> > > + > > +static struct platform_device pmc_core_device = { > > + .name = "pmc_core", > > +}; > > + > > +static int dummy; > > +/* > > + * INTEL_CPU_FAM6 macro won't take NULL for driver_data (uses &driver_data), > > + * thus provide a dummy driver_data. > > + */ > > +static const struct x86_cpu_id intel_pmc_core_ids[] = { > > + INTEL_CPU_FAM6(SKYLAKE_MOBILE, dummy), > > + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, dummy), > > + INTEL_CPU_FAM6(KABYLAKE_MOBILE, dummy), > > + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, dummy), > > + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, dummy), > > + INTEL_CPU_FAM6(ICELAKE_MOBILE, dummy), > > + {} > > +}; > > +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids); > > + > > +static int __init pmc_core_platform_init(void) > > +{ > > + int ret; > > + > > + /* Skip creating the platform device if ACPI already has a device */ > > + if (acpi_dev_present("INT33A1", NULL, -1) || > > + !x86_match_cpu(intel_pmc_core_ids)) > > + return -ENODEV; > > + > > + ret = platform_device_register(&pmc_core_device); > > + if (ret) > > + return ret; > > we can just return platform_devie_register (...) and avoid extra lines and > variable. Sure. > > > + > > + return 0; > > +} > > + > > +static void __exit pmc_core_platform_exit(void) > > +{ > > + if (!acpi_dev_present("INT33A1", NULL, -1) && > > + x86_match_cpu(intel_pmc_core_ids)) > > + platform_device_unregister(&pmc_core_device); > > +} > > + > > +module_init(pmc_core_platform_init); > > +module_exit(pmc_core_platform_exit); > > -- > > 2.21.0.392.gf8f6787159e-goog > > > > -- > Best Regards, > Rajneesh
On Mon, Apr 8, 2019 at 10:07 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <rajatja@google.com> wrote: > > > > Add code to instantiate the pmc_core platform device and thus attach to > > the driver, if the ACPI device for the same ("INT33A1") is not present > > in a system where it should be. This was discussed here: > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1966991.html > > > > > > > +#include <linux/acpi.h> > > +#include <linux/platform_device.h> > > + > > +#include <asm/cpu_device_id.h> > > +#include <asm/intel-family.h> > > + > > +static struct platform_device pmc_core_device = { > > + .name = "pmc_core", > > +}; > > + > > > +static int dummy; > > +/* > > + * INTEL_CPU_FAM6 macro won't take NULL for driver_data (uses &driver_data), > > + * thus provide a dummy driver_data. > > + */ > > You may use pmc_core_device instead, right? sure, will do. > > > +static const struct x86_cpu_id intel_pmc_core_ids[] = { > > + INTEL_CPU_FAM6(SKYLAKE_MOBILE, dummy), > > + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, dummy), > > + INTEL_CPU_FAM6(KABYLAKE_MOBILE, dummy), > > + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, dummy), > > + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, dummy), > > + INTEL_CPU_FAM6(ICELAKE_MOBILE, dummy), > > + {} > > +}; > > +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids); > > + > > +static int __init pmc_core_platform_init(void) > > +{ > > + int ret; > > + > > + /* Skip creating the platform device if ACPI already has a device */ > > + if (acpi_dev_present("INT33A1", NULL, -1) || > > + !x86_match_cpu(intel_pmc_core_ids)) > > + return -ENODEV; > > Split it to one conditional per line. sure, will do. > > > + > > + ret = platform_device_register(&pmc_core_device); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +static void __exit pmc_core_platform_exit(void) > > +{ > > > + if (!acpi_dev_present("INT33A1", NULL, -1) && > > + x86_match_cpu(intel_pmc_core_ids)) > > Redundant check. will fix. > > > + platform_device_unregister(&pmc_core_device); > > +} > > -- > With Best Regards, > Andy Shevchenko
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 86cb76677bc8..83d0bb9a14bb 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -89,7 +89,7 @@ obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU) += intel_bxtwc_tmu.o obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \ intel_telemetry_pltdrv.o \ intel_telemetry_debugfs.o -obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o +obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o intel_pmc_core_plat_init.o obj-$(CONFIG_PMC_ATOM) += pmc_atom.o obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o diff --git a/drivers/platform/x86/intel_pmc_core_plat_init.c b/drivers/platform/x86/intel_pmc_core_plat_init.c new file mode 100644 index 000000000000..45be89e7586c --- /dev/null +++ b/drivers/platform/x86/intel_pmc_core_plat_init.c @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Intel PMC Core platform init + * Copyright (c) 2019, Google Inc. + * Author - Rajat Jain + * + * This code instantiates platform devices for intel_pmc_core driver, only + * on supported platforms that may not have the ACPI devices in the ACPI tables. + * No new platforms should be added here, because we expect that new platforms + * should all have the ACPI device, which is the preferred way of enumeration. + */ + +#include <linux/acpi.h> +#include <linux/platform_device.h> + +#include <asm/cpu_device_id.h> +#include <asm/intel-family.h> + +static struct platform_device pmc_core_device = { + .name = "pmc_core", +}; + +static int dummy; +/* + * INTEL_CPU_FAM6 macro won't take NULL for driver_data (uses &driver_data), + * thus provide a dummy driver_data. + */ +static const struct x86_cpu_id intel_pmc_core_ids[] = { + INTEL_CPU_FAM6(SKYLAKE_MOBILE, dummy), + INTEL_CPU_FAM6(SKYLAKE_DESKTOP, dummy), + INTEL_CPU_FAM6(KABYLAKE_MOBILE, dummy), + INTEL_CPU_FAM6(KABYLAKE_DESKTOP, dummy), + INTEL_CPU_FAM6(CANNONLAKE_MOBILE, dummy), + INTEL_CPU_FAM6(ICELAKE_MOBILE, dummy), + {} +}; +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids); + +static int __init pmc_core_platform_init(void) +{ + int ret; + + /* Skip creating the platform device if ACPI already has a device */ + if (acpi_dev_present("INT33A1", NULL, -1) || + !x86_match_cpu(intel_pmc_core_ids)) + return -ENODEV; + + ret = platform_device_register(&pmc_core_device); + if (ret) + return ret; + + return 0; +} + +static void __exit pmc_core_platform_exit(void) +{ + if (!acpi_dev_present("INT33A1", NULL, -1) && + x86_match_cpu(intel_pmc_core_ids)) + platform_device_unregister(&pmc_core_device); +} + +module_init(pmc_core_platform_init); +module_exit(pmc_core_platform_exit);
Add code to instantiate the pmc_core platform device and thus attach to the driver, if the ACPI device for the same ("INT33A1") is not present in a system where it should be. This was discussed here: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1966991.html Signed-off-by: Rajat Jain <rajatja@google.com> --- v3: (first version of *this* patch -to go with rest of v3 patchset) v2: (does not exist) v1: (does not exist) drivers/platform/x86/Makefile | 2 +- .../platform/x86/intel_pmc_core_plat_init.c | 64 +++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 drivers/platform/x86/intel_pmc_core_plat_init.c