Re: [PATCH v3 0/3] Support PMIC operation region for CrystalCove and XPower

[Aaron Lu]

On 11/26/2014 04:41 AM, Rafael J. Wysocki wrote:
> 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@xxxxxxxxx>
>> 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.

Good point, and I saw you have solved this problem in the bleeding-edge
branch, thanks!

> 
>>  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@xxxxxxxxx>
>> Signed-off-by: Aaron Lu <aaron.lu@xxxxxxxxx>
>> ---
>>  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.

Heh. I tried to follow the pattern in CrystalCove's MFD driver, where
all its cell devices are named crystal_cove_XXX, and since the platform
bus' match is done by comparing the device's name and the name field of
the driver's id table entry, they have to be the same. The problem is,
the name field of the platform_device_id structure has a size limit of
20, I can't even put the 'op' before the 'region' there. Perhaps time to
enlarge that size limit?

Thanks,
Aaron

> 
>> +
>> +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);
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/