lkml.org 
[lkml]   [2014]   [Nov]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v3 0/3] Support PMIC operation region for CrystalCove and XPower
Date
On Tuesday, November 25, 2014 07:52:31 PM Aaron Lu wrote:
> On 11/25/2014 09:47 AM, Rafael J. Wysocki wrote:
> > On Friday, November 21, 2014 03:11:48 PM Aaron Lu wrote:
> >> v3:
> >> Only some function/variable name changes, no functiona changes:
> >> - Replace the dptf/DPTF word originate from the BIOS ACPI table with more
> >> meaningful word thermal/THERMAL in all places;
> >> - Eliminate the soc part in various structure and function names to make
> >> them shorter:
> >> intel_soc_pmic_opregion -> intel_pmic_opregion
> >> intel_soc_pmic_pmop_handler -> intel_pmic_pmop_handler
> >> intel_soc_pmic_install_opregion_handler -> intel_pmic_install_opregion_handler
> >> etc.
> >>
> >>
> >> v2:
> >> Place PMIC operation files under drivers/acpi/pmic instead of
> >> drivers/acpi/pmic_opregion as suggested by Rafael;
> >> Rename PMIC operation files to make them shorter as suggested by Rafael.
> >>
> >> v1:
> >> On Intel Baytrail-T and Baytrail-T-CR platforms, there are two customized
> >> ACPI operation regions defined for the Power Management Integrated Circuit
> >> device, one is for power resource handling and one is for thermal: sensor
> >> temperature reporting, trip point setting, etc. There are different PMIC
> >> chips used on those platforms and though each has the same two operation
> >> regions and functionality, their implementation is different so every PMIC
> >> will need a driver. But since their functionality is similar, some common
> >> code is abstracted into the intel_soc_pmic_opregion.c.
> >>
> >> The last version is posted here:
> >> https://lkml.org/lkml/2014/9/8/801
> >>
> >> Changes since then:
> >> 1 Move to drivers/acpi as discussed on the above thread;
> >> 2 Added support for XPower AXP288 PMIC operation region support;
> >> 3 Since operation region handler can not be removed(at the moment at least),
> >> use bool for the two operation region driver configs instead of tristate;
> >> Another reason to do this is that, with Mika's MFD ACPI support patch, all
> >> those MFD cell devices created will have the same modalias as their parent's
> >> so it doesn't make much sense to compile these drivers into modules.
> >>
> >> Patch 1 applies on top of Rafael's pm-next branch, and then patch 2 and
> >> patch 3 needs merge of Lee's mfd/ib-mfd-iio-3.19 branch where the PMIC
> >> driver XPower AXP288 and iio driver axp288_adc is located.
> >>
> >>
> >> Aaron Lu (3):
> >> ACPI / PMIC: support PMIC operation region for CrystalCove
> >> ACPI / PMIC: support PMIC operation region for XPower AXP288
> >> ACPI / PMIC: AXP288: support virtual GPIO in ACPI table
> >
> > OK
> >
> > I've pulled the Lee's 'mfd/ib-mfd-iio-3.19' branch and applied your updated
> > three on top of that. Please check the bleeding-edge branch of linux-pm.git
> > for the result.
>
> Thanks, and a fix patch is here:
>
> From: Aaron Lu <aaron.lu@intel.com>
> Date: Tue, 25 Nov 2014 14:35:38 +0800
> Subject: [PATCH] ACPI / PMIC: Make it possible to build PMIC driver as a module
>
> This can solve a problem that when axp288_adc driver is built as a
> module and the PMIC driver is builtin, following error would ocur:

I would prefer that to be sloved by requiring axp288_adc to be built in
if the PMIC stuff is selected. Otherwise we may need to deal with some
nasty module load ordering dependencies.

> drivers/built-in.o: In function `intel_xpower_pmic_get_raw_temp':
> intel_pmic_xpower.c:(.text+0xdfaa7): undefined reference to `iio_channel_get'
> intel_pmic_xpower.c:(.text+0xdfb24): undefined reference to `iio_read_channel_raw'
> intel_pmic_xpower.c:(.text+0xdfb4e): undefined reference to `iio_channel_release'
>
> Also, with the fix commit: 52870786ff5d ("ACPI: Use ACPI companion to
> match only the first physical device"), the MFD cell device will have
> its own platform modalias instead of its parent's ACPI modalias, this
> made it possible for the module to be autoloaded.
>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
> drivers/acpi/Kconfig | 6 +++---
> drivers/acpi/pmic/intel_pmic_crc.c | 12 +++++++++++-
> drivers/acpi/pmic/intel_pmic_xpower.c | 12 +++++++++++-
> 3 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 227f0692cbff..f9459ba4ce59 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -394,7 +394,7 @@ config ACPI_EXTLOG
> tracepoint which carries that information to userspace.
>
> menuconfig PMIC_OPREGION
> - bool "PMIC (Power Management Integrated Circuit) operation region support"
> + tristate "PMIC (Power Management Integrated Circuit) operation region support"
> help
> Select this option to enable support for ACPI operation
> region of the PMIC chip. The operation region can be used
> @@ -403,13 +403,13 @@ menuconfig PMIC_OPREGION
>
> if PMIC_OPREGION
> config CRC_PMIC_OPREGION
> - bool "ACPI operation region support for CrystalCove PMIC"
> + tristate "ACPI operation region support for CrystalCove PMIC"
> depends on INTEL_SOC_PMIC
> help
> This config adds ACPI operation region support for CrystalCove PMIC.
>
> config XPOWER_PMIC_OPREGION
> - bool "ACPI operation region support for XPower AXP288 PMIC"
> + tristate "ACPI operation region support for XPower AXP288 PMIC"
> depends on AXP288_ADC
> help
> This config adds ACPI operation region support for XPower AXP288 PMIC.
> diff --git a/drivers/acpi/pmic/intel_pmic_crc.c b/drivers/acpi/pmic/intel_pmic_crc.c
> index 8955e5b41195..8a193381b5ee 100644
> --- a/drivers/acpi/pmic/intel_pmic_crc.c
> +++ b/drivers/acpi/pmic/intel_pmic_crc.c
> @@ -194,13 +194,23 @@ static int intel_crc_pmic_opregion_probe(struct platform_device *pdev)
> &intel_crc_pmic_opregion_data);
> }
>
> +#define DRV_NAME "crystal_cove_region"

This name is just horrible, BTW.

> +
> +static struct platform_device_id crc_opregion_id_table[] = {
> + { .name = DRV_NAME },
> + {},
> +};
> +
> static struct platform_driver intel_crc_pmic_opregion_driver = {
> .probe = intel_crc_pmic_opregion_probe,
> + .id_table = crc_opregion_id_table,
> .driver = {
> - .name = "crystal_cove_region",
> + .name = DRV_NAME,
> },
> };
>
> +MODULE_DEVICE_TABLE(platform, crc_opregion_id_table);
> +
> static int __init intel_crc_pmic_opregion_driver_init(void)
> {
> return platform_driver_register(&intel_crc_pmic_opregion_driver);
> diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
> index 9ec57ebd81c9..4debcbbd6285 100644
> --- a/drivers/acpi/pmic/intel_pmic_xpower.c
> +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
> @@ -251,13 +251,23 @@ static int intel_xpower_pmic_opregion_probe(struct platform_device *pdev)
> return result;
> }
>
> +#define DRV_NAME "axp288_opregion"

Same here.

The vast majority of people who will see those names have no idea what an
"opregion" is and "region" alone is just meaningless.

> +
> +static struct platform_device_id xpower_opregion_id_table[] = {
> + { .name = DRV_NAME },
> + {},
> +};
> +
> static struct platform_driver intel_xpower_pmic_opregion_driver = {
> .probe = intel_xpower_pmic_opregion_probe,
> + .id_table = xpower_opregion_id_table,
> .driver = {
> - .name = "axp288_opregion",
> + .name = DRV_NAME,
> },
> };
>
> +MODULE_DEVICE_TABLE(platform, xpower_opregion_id_table);
> +
> static int __init intel_xpower_pmic_opregion_driver_init(void)
> {
> return platform_driver_register(&intel_xpower_pmic_opregion_driver);
>

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


\
 
 \ /
  Last update: 2014-11-25 22:01    [W:0.088 / U:1.444 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site