[PATCH v3 2/3] ACPI / PMIC: support PMIC operation region for XPower AXP288

[Aaron Lu]

The Baytrail-T-CR platform firmware has defined two customized operation
regions for PMIC chip Dollar Cove XPower - one is for power resource
handling and one is for thermal just like the CrystalCove one. This patch
adds support for them on top of the common PMIC opregion region code.

Signed-off-by: Aaron Lu <***@intel.com>
Acked-by: Lee Jones <***@linaro.org> for the MFD part
---
drivers/acpi/Kconfig | 6 +
drivers/acpi/Makefile | 1 +
drivers/acpi/pmic/intel_pmic_xpower.c | 277 ++++++++++++++++++++++++++++++++++
drivers/mfd/axp20x.c | 3 +
4 files changed, 287 insertions(+)
create mode 100644 drivers/acpi/pmic/intel_pmic_xpower.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 3e5f2056f946..227f0692cbff 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -408,6 +408,12 @@ config CRC_PMIC_OPREGION
help
This config adds ACPI operation region support for CrystalCove PMIC.

+config XPOWER_PMIC_OPREGION
+ bool "ACPI operation region support for XPower AXP288 PMIC"
+ depends on AXP288_ADC
+ help
+ This config adds ACPI operation region support for XPower AXP288 PMIC.
+
endif

endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index f5938399ac14..f74317cc1ca9 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -91,3 +91,4 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o

obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o
obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
+obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
new file mode 100644
index 000000000000..6c4d6ce0cff1
--- /dev/null
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -0,0 +1,277 @@
+/*
+ * intel_pmic_xpower.c - XPower AXP288 PMIC operation region driver
+ *
+ * Copyright (C) 2014 Intel Corporation. All rights reserved.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/mfd/axp20x.h>
+#include <linux/regmap.h>
+#include <linux/platform_device.h>
+#include <linux/iio/consumer.h>
+#include "intel_pmic.h"
+
+#define XPOWER_GPADC_LOW 0x5b
+
+static struct pmic_pwr_table pwr_table[] = {
+ {
+ .address = 0x00,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x05,
+ }
+ }, /* ALD1, 0x13, bit5 */
+ {
+ .address = 0x04,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x06,
+ }
+ }, /* ALD2, 0x13, bit6 */
+ {
+ .address = 0x08,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x07,
+ }
+ }, /* ALD3, 0x13, bit7 */
+ {
+ .address = 0x0c,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x03,
+ }
+ }, /* DLD1, 0x12, bit3 */
+ {
+ .address = 0x10,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x04,
+ }
+ }, /* DLD2, 0x12, bit4 */
+ {
+ .address = 0x14,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x05,
+ }
+ }, /* DLD3, 0x12, bit5 */
+ {
+ .address = 0x18,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x06,
+ }
+ }, /* DLD4, 0x12, bit6 */
+ {
+ .address = 0x1c,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x00,
+ }
+ }, /* ELD1, 0x12, bit0 */
+ {
+ .address = 0x20,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x01,
+ }
+ }, /* ELD2, 0x12, bit1 */
+ {
+ .address = 0x24,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x02,
+ }
+ }, /* ELD3, 0x12, bit2 */
+ {
+ .address = 0x28,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x02,
+ }
+ }, /* FLD1, 0x13, bit2 */
+ {
+ .address = 0x2c,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x03,
+ }
+ }, /* FLD2, 0x13, bit3 */
+ {
+ .address = 0x30,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x04,
+ }
+ }, /* FLD3, 0x13, bit4 */
+ {
+ .address = 0x38,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x03,
+ }
+ }, /* BUC1, 0x10, bit3 */
+ {
+ .address = 0x3c,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x06,
+ }
+ }, /* BUC2, 0x10, bit6 */
+ {
+ .address = 0x40,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x05,
+ }
+ }, /* BUC3, 0x10, bit5 */
+ {
+ .address = 0x44,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x04,
+ }
+ }, /* BUC4, 0x10, bit4 */
+ {
+ .address = 0x48,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x01,
+ }
+ }, /* BUC5, 0x10, bit1 */
+ {
+ .address = 0x4c,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x00
+ }
+ }, /* BUC6, 0x10, bit0 */
+};
+
+/* TMP0 - TMP5 are the same, all from GPADC */
+static struct pmic_thermal_table thermal_table[] = {
+ {
+ .address = 0x00,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x0c,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x18,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x24,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x30,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x3c,
+ .reg = XPOWER_GPADC_LOW
+ },
+};
+
+static int intel_xpower_pmic_get_power(struct regmap *regmap,
+ struct pmic_pwr_reg *preg, u64 *value)
+{
+ int data;
+
+ if (regmap_read(regmap, preg->reg, &data))
+ return -EIO;
+
+ *value = (data & BIT(preg->bit)) ? 1 : 0;
+ return 0;
+}
+
+static int intel_xpower_pmic_update_power(struct regmap *regmap,
+ struct pmic_pwr_reg *preg, bool on)
+{
+ int data;
+
+ if (regmap_read(regmap, preg->reg, &data))
+ return -EIO;
+
+ if (on)
+ data |= BIT(preg->bit);
+ else
+ data &= ~BIT(preg->bit);
+
+ if (regmap_write(regmap, preg->reg, data))
+ return -EIO;
+
+ return 0;
+}
+
+/*
+ * We could get the sensor value by manipulating the HW regs here, but since
+ * the axp288 IIO driver may also access the same regs at the same time, the
+ * APIs provided by IIO subsystem are used here instead to avoid problems. As
+ * a result, the two passed in params are of no actual use.
+ */
+static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
+{
+ struct iio_channel *gpadc_chan;
+ int ret, val;
+
+ gpadc_chan = iio_channel_get(NULL, "axp288-system-temp");
+ if (IS_ERR_OR_NULL(gpadc_chan))
+ return -EACCES;
+
+ ret = iio_read_channel_raw(gpadc_chan, &val);
+ if (ret < 0)
+ val = ret;
+
+ iio_channel_release(gpadc_chan);
+ return val;
+}
+
+static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
+ .get_power = intel_xpower_pmic_get_power,
+ .update_power = intel_xpower_pmic_update_power,
+ .get_raw_temp = intel_xpower_pmic_get_raw_temp,
+ .pwr_table = pwr_table,
+ .pwr_table_count = ARRAY_SIZE(pwr_table),
+ .thermal_table = thermal_table,
+ .thermal_table_count = ARRAY_SIZE(thermal_table),
+};
+
+
+static int intel_xpower_pmic_opregion_probe(struct platform_device *pdev)
+{
+ struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+ return intel_pmic_install_opregion_handler(&pdev->dev,
+ ACPI_HANDLE(pdev->dev.parent), axp20x->regmap,
+ &intel_xpower_pmic_opregion_data);
+}
+
+static struct platform_driver intel_xpower_pmic_opregion_driver = {
+ .probe = intel_xpower_pmic_opregion_probe,
+ .driver = {
+ .name = "axp288_opregion",
+ },
+};
+
+static int __init intel_xpower_pmic_opregion_driver_init(void)
+{
+ return platform_driver_register(&intel_xpower_pmic_opregion_driver);
+}
+module_init(intel_xpower_pmic_opregion_driver_init);
+
+MODULE_DESCRIPTION("XPower AXP288 ACPI operation region driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index acbb4c3bf98c..daf3c8d33b4d 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -354,6 +354,9 @@ static struct mfd_cell axp288_cells[] = {
.num_resources = ARRAY_SIZE(axp288_battery_resources),
.resources = axp288_battery_resources,
},
+ {
+ .name = "axp288_opregion",
+ },
};

static struct axp20x_dev *axp20x_pm_power_off;

[Aaron Lu]

The same virtual GPIO strategy is also used for the AXP288 PMIC in that
various control methods that are used to do power rail handling and
sensor reading/setting will touch GPIO fields defined under the PMIC
device. The GPIO fileds are only defined by the ACPI code while the
actual hardware doesn't really have a GPIO controller, but to make those
control method execution succeed, we have to install a GPIO handler for
the PMIC device handle. Since we do not need the virtual GPIO strategy,
we can simply do nothing in that handler.

Signed-off-by: Aaron Lu <***@intel.com>
---
drivers/acpi/pmic/intel_pmic_xpower.c | 27 +++++++++++++++++++++++----
1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index 6c4d6ce0cff1..480c41c36444 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -251,13 +251,32 @@ static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
.thermal_table_count = ARRAY_SIZE(thermal_table),
};

+static acpi_status intel_xpower_pmic_gpio_handler(u32 function,
+ acpi_physical_address address, u32 bit_width, u64 *value,
+ void *handler_context, void *region_context)
+{
+ return AE_OK;
+}

static int intel_xpower_pmic_opregion_probe(struct platform_device *pdev)
{
- struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
- return intel_pmic_install_opregion_handler(&pdev->dev,
- ACPI_HANDLE(pdev->dev.parent), axp20x->regmap,
- &intel_xpower_pmic_opregion_data);
+ struct device *parent = pdev->dev.parent;
+ struct axp20x_dev *axp20x = dev_get_drvdata(parent);
+ acpi_status status;
+ int result;
+
+ result = intel_pmic_install_opregion_handler(&pdev->dev,
+ ACPI_HANDLE(parent), axp20x->regmap,
+ &intel_xpower_pmic_opregion_data);
+ if (!result) {
+ status = acpi_install_address_space_handler(
+ ACPI_HANDLE(parent), ACPI_ADR_SPACE_GPIO,
+ intel_xpower_pmic_gpio_handler, NULL, NULL);
+ if (ACPI_FAILURE(status))
+ result = -ENODEV;
+ }
+
+ return result;
}

static struct platform_driver intel_xpower_pmic_opregion_driver = {

[Rafael J. Wysocki]

Post by Aaron Lu
The same virtual GPIO strategy is also used for the AXP288 PMIC in that
various control methods that are used to do power rail handling and
sensor reading/setting will touch GPIO fields defined under the PMIC
device. The GPIO fileds are only defined by the ACPI code while the
actual hardware doesn't really have a GPIO controller, but to make those
control method execution succeed, we have to install a GPIO handler for
the PMIC device handle. Since we do not need the virtual GPIO strategy,
we can simply do nothing in that handler.
---
drivers/acpi/pmic/intel_pmic_xpower.c | 27 +++++++++++++++++++++++----
1 file changed, 23 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index 6c4d6ce0cff1..480c41c36444 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -251,13 +251,32 @@ static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
.thermal_table_count = ARRAY_SIZE(thermal_table),
};
+static acpi_status intel_xpower_pmic_gpio_handler(u32 function,
+ acpi_physical_address address, u32 bit_width, u64 *value,
+ void *handler_context, void *region_context)
+{
+ return AE_OK;
+}
static int intel_xpower_pmic_opregion_probe(struct platform_device *pdev)
{
- struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
- return intel_pmic_install_opregion_handler(&pdev->dev,
- ACPI_HANDLE(pdev->dev.parent), axp20x->regmap,
- &intel_xpower_pmic_opregion_data);
+ struct device *parent = pdev->dev.parent;
+ struct axp20x_dev *axp20x = dev_get_drvdata(parent);
+ acpi_status status;
+ int result;
+
+ result = intel_pmic_install_opregion_handler(&pdev->dev,
+ ACPI_HANDLE(parent), axp20x->regmap,
+ &intel_xpower_pmic_opregion_data);
+ if (!result) {
+ status = acpi_install_address_space_handler(
+ ACPI_HANDLE(parent), ACPI_ADR_SPACE_GPIO,
+ intel_xpower_pmic_gpio_handler, NULL, NULL);

So here we have a problem, because we can't unregister the opregion handler
registered above if this fails. Not nice.

Post by Aaron Lu
+ if (ACPI_FAILURE(status))
+ result = -ENODEV;
+ }
+
+ return result;
}
static struct platform_driver intel_xpower_pmic_opregion_driver = {

[Aaron Lu]

Post by Rafael J. Wysocki

Post by Aaron Lu
The same virtual GPIO strategy is also used for the AXP288 PMIC in that
various control methods that are used to do power rail handling and
sensor reading/setting will touch GPIO fields defined under the PMIC
device. The GPIO fileds are only defined by the ACPI code while the
actual hardware doesn't really have a GPIO controller, but to make those
control method execution succeed, we have to install a GPIO handler for
the PMIC device handle. Since we do not need the virtual GPIO strategy,
we can simply do nothing in that handler.
---
drivers/acpi/pmic/intel_pmic_xpower.c | 27 +++++++++++++++++++++++----
1 file changed, 23 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index 6c4d6ce0cff1..480c41c36444 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -251,13 +251,32 @@ static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
.thermal_table_count = ARRAY_SIZE(thermal_table),
};
+static acpi_status intel_xpower_pmic_gpio_handler(u32 function,
+ acpi_physical_address address, u32 bit_width, u64 *value,
+ void *handler_context, void *region_context)
+{
+ return AE_OK;
+}
static int intel_xpower_pmic_opregion_probe(struct platform_device *pdev)
{
- struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
- return intel_pmic_install_opregion_handler(&pdev->dev,
- ACPI_HANDLE(pdev->dev.parent), axp20x->regmap,
- &intel_xpower_pmic_opregion_data);
+ struct device *parent = pdev->dev.parent;
+ struct axp20x_dev *axp20x = dev_get_drvdata(parent);
+ acpi_status status;
+ int result;
+
+ result = intel_pmic_install_opregion_handler(&pdev->dev,
+ ACPI_HANDLE(parent), axp20x->regmap,
+ &intel_xpower_pmic_opregion_data);
+ if (!result) {
+ status = acpi_install_address_space_handler(
+ ACPI_HANDLE(parent), ACPI_ADR_SPACE_GPIO,
+ intel_xpower_pmic_gpio_handler, NULL, NULL);

So here we have a problem, because we can't unregister the opregion handler
registered above if this fails. Not nice.

I'll add a remove_opregion_handler call if the above install failed, the
chance the remove_opregion_handler will trigger a problem during init time
is pretty low.

If that is not desired, I can install the operation region handler for
the virtual GPIO first and then the real PMIC operation region handler,
the cost of leaving a virtual GPIO operation region handler is essential
zero I think.

Thanks,
Aaron

Post by Rafael J. Wysocki

Post by Aaron Lu
+ if (ACPI_FAILURE(status))
+ result = -ENODEV;
+ }
+
+ return result;
}
static struct platform_driver intel_xpower_pmic_opregion_driver = {

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

[Rafael J. Wysocki]

Post by Rafael J. Wysocki

Post by Aaron Lu
+ if (!result) {
+ status = acpi_install_address_space_handler(
+ ACPI_HANDLE(parent), ACPI_ADR_SPACE_GPIO,
+ intel_xpower_pmic_gpio_handler, NULL, NULL);

So here we have a problem, because we can't unregister the opregion handler
registered above if this fails. Not nice.

I'm not correct in the cover letter, the actual problem with operation
The acpi_remove_address_space_handler doesn't wait for the current
running handler to return, so if we call
acpi_remove_address_space_handler in a module's exit function, the
handler's memory will be freed and the running handler will access to
a no longer valid memory place.
So as long as we can make sure the handler will not go away from the
memory, we are safe.

This only means that address space handlers cannot be removed from kernel
modules.

Lv was trying to add a wrapper for that some time ago, maybe it's a good
idea to revive that?

Date: Mon, 20 Oct 2014 17:06:20 +0800
Subject: [PATCH 3/3] ACPI / PMIC: AXP288: support virtual GPIO in ACPI table
The same virtual GPIO strategy is also used for the AXP288 PMIC in that
various control methods that are used to do power rail handling and
sensor reading/setting will touch GPIO fields defined under the PMIC
device. The GPIO fileds are only defined by the ACPI code while the
actual hardware doesn't really have a GPIO controller, but to make those
control method execution succeed, we have to install a GPIO handler for
the PMIC device handle. Since we do not need the virtual GPIO strategy,
we can simply do nothing in that handler.
---
drivers/acpi/pmic/intel_pmic_xpower.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index 795ca073db08..9ec57ebd81c9 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -220,13 +220,35 @@ static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
.thermal_table_count = ARRAY_SIZE(thermal_table),
};
+static acpi_status intel_xpower_pmic_gpio_handler(u32 function,
+ acpi_physical_address address, u32 bit_width, u64 *value,
+ void *handler_context, void *region_context)
+{
+ return AE_OK;
+}
static int intel_xpower_pmic_opregion_probe(struct platform_device *pdev)
{
- struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
- return intel_pmic_install_opregion_handler(&pdev->dev,
- ACPI_HANDLE(pdev->dev.parent), axp20x->regmap,
- &intel_xpower_pmic_opregion_data);
+ struct device *parent = pdev->dev.parent;
+ struct axp20x_dev *axp20x = dev_get_drvdata(parent);
+ acpi_status status;
+ int result;
+
+ status = acpi_install_address_space_handler(ACPI_HANDLE(parent),
+ ACPI_ADR_SPACE_GPIO, intel_xpower_pmic_gpio_handler,
+ NULL, NULL);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ result = intel_pmic_install_opregion_handler(&pdev->dev,
+ ACPI_HANDLE(parent), axp20x->regmap,
+ &intel_xpower_pmic_opregion_data);
+ if (result)
+ acpi_remove_address_space_handler(ACPI_HANDLE(parent),
+ ACPI_ADR_SPACE_GPIO,
+ intel_xpower_pmic_gpio_handler);
+
+ return result;
}
static struct platform_driver intel_xpower_pmic_opregion_driver = {

[Zheng, Lv]

Hi,

Sent: Monday, November 24, 2014 11:20 PM
Lv
Subject: Re: [PATCH v3 updated 3/3] ACPI / PMIC: AXP288: support virtual GPIO in ACPI table

Post by Rafael J. Wysocki

Post by Aaron Lu
+ if (!result) {
+ status = acpi_install_address_space_handler(
+ ACPI_HANDLE(parent), ACPI_ADR_SPACE_GPIO,
+ intel_xpower_pmic_gpio_handler, NULL, NULL);

So here we have a problem, because we can't unregister the opregion handler
registered above if this fails. Not nice.

I'm not correct in the cover letter, the actual problem with operation
The acpi_remove_address_space_handler doesn't wait for the current
running handler to return, so if we call
acpi_remove_address_space_handler in a module's exit function, the
handler's memory will be freed and the running handler will access to
a no longer valid memory place.
So as long as we can make sure the handler will not go away from the
memory, we are safe.

This only means that address space handlers cannot be removed from kernel
modules.
Lv was trying to add a wrapper for that some time ago, maybe it's a good
idea to revive that?

I may fix the issue in ACPICA, but it looks like that it will cost time to discuss.
So I also think it is better to take the wrapper now.
I'll hand the patch to Aaron to let him do a test on it before posting it again.

Thanks and best regards
-Lv

Date: Mon, 20 Oct 2014 17:06:20 +0800
Subject: [PATCH 3/3] ACPI / PMIC: AXP288: support virtual GPIO in ACPI table
The same virtual GPIO strategy is also used for the AXP288 PMIC in that
various control methods that are used to do power rail handling and
sensor reading/setting will touch GPIO fields defined under the PMIC
device. The GPIO fileds are only defined by the ACPI code while the
actual hardware doesn't really have a GPIO controller, but to make those
control method execution succeed, we have to install a GPIO handler for
the PMIC device handle. Since we do not need the virtual GPIO strategy,
we can simply do nothing in that handler.
---
drivers/acpi/pmic/intel_pmic_xpower.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index 795ca073db08..9ec57ebd81c9 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -220,13 +220,35 @@ static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
.thermal_table_count = ARRAY_SIZE(thermal_table),
};
+static acpi_status intel_xpower_pmic_gpio_handler(u32 function,
+ acpi_physical_address address, u32 bit_width, u64 *value,
+ void *handler_context, void *region_context)
+{
+ return AE_OK;
+}
static int intel_xpower_pmic_opregion_probe(struct platform_device *pdev)
{
- struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
- return intel_pmic_install_opregion_handler(&pdev->dev,
- ACPI_HANDLE(pdev->dev.parent), axp20x->regmap,
- &intel_xpower_pmic_opregion_data);
+ struct device *parent = pdev->dev.parent;
+ struct axp20x_dev *axp20x = dev_get_drvdata(parent);
+ acpi_status status;
+ int result;
+
+ status = acpi_install_address_space_handler(ACPI_HANDLE(parent),
+ ACPI_ADR_SPACE_GPIO, intel_xpower_pmic_gpio_handler,
+ NULL, NULL);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ result = intel_pmic_install_opregion_handler(&pdev->dev,
+ ACPI_HANDLE(parent), axp20x->regmap,
+ &intel_xpower_pmic_opregion_data);
+ if (result)
+ acpi_remove_address_space_handler(ACPI_HANDLE(parent),
+ ACPI_ADR_SPACE_GPIO,
+ intel_xpower_pmic_gpio_handler);
+
+ return result;
}
static struct platform_driver intel_xpower_pmic_opregion_driver = {

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

N�����r��y����b�X��ǧv�^�)޺{.n�+����{����zX����ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z��w���?�����&�)ߢf

[Aaron Lu]

Post by Rafael J. Wysocki

Post by Rafael J. Wysocki

Post by Aaron Lu
+ if (!result) {
+ status = acpi_install_address_space_handler(
+ ACPI_HANDLE(parent), ACPI_ADR_SPACE_GPIO,
+ intel_xpower_pmic_gpio_handler, NULL, NULL);

So here we have a problem, because we can't unregister the opregion handler
registered above if this fails. Not nice.

I'm not correct in the cover letter, the actual problem with operation
The acpi_remove_address_space_handler doesn't wait for the current
running handler to return, so if we call
acpi_remove_address_space_handler in a module's exit function, the
handler's memory will be freed and the running handler will access to
a no longer valid memory place.
So as long as we can make sure the handler will not go away from the
memory, we are safe.

This only means that address space handlers cannot be removed from kernel
modules.

If the module can not be unloaded(no module exit call), then we should
be safe.

Post by Rafael J. Wysocki
Lv was trying to add a wrapper for that some time ago, maybe it's a good
idea to revive that?

Is it this one? If it is, I'll test it and then add the unload
functionality to the PMIC drivers.

From: Lv Zheng <***@intel.com>
Date: Tue, 25 Nov 2014 15:42:44 +0800
Subject: [PATCH] ACPICA: Events: Add invocation protection for operation region callbacks.

This patch adds invocation protection around operation region callbacks to
offer a module safe environment for OSPM provided address space handlers.

It is likely that OSPM where ModuleDevice is supported will implement
specific address space handlers in the modules. Thus the unloading of
the handlers' owner modules may lead to program crash around the invocation
if the handler callbacks are invoked without protection. Since the
handler callbacks are invoked inside of ACPICA, it is better to implement
invocation protection inside of ACPICA.
As address space handlers are expected to be executed in parallel, it is
not a good choice to implement protection using locks. This patch uses
reference counting based protection mechanism. When OSPM calls
acpi_remove_address_space_handler(), the function waits until all invocations
exit to ensure no invocation can happen after the unloading of the modules.

Note that this might be a workaround and not tested, better solution could
be implemented to tune the design of the namespace objects. Lv Zheng.

Signed-off-by: Lv Zheng <***@intel.com>
---
drivers/acpi/acpica/acevents.h | 9 ++++
drivers/acpi/acpica/acobject.h | 1 +
drivers/acpi/acpica/evhandler.c | 117 ++++++++++++++++++++++++++++++++++++++++
drivers/acpi/acpica/evregion.c | 11 +++-
drivers/acpi/acpica/evxfregn.c | 9 ++++
include/acpi/actypes.h | 2 +
6 files changed, 147 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpica/acevents.h b/drivers/acpi/acpica/acevents.h
index 7a7811a9fc26..3020ac4ab7a8 100644
--- a/drivers/acpi/acpica/acevents.h
+++ b/drivers/acpi/acpica/acevents.h
@@ -175,6 +175,15 @@ acpi_ev_install_space_handler(struct acpi_namespace_node *node,
acpi_adr_space_handler handler,
acpi_adr_space_setup setup, void *context);

+acpi_status
+acpi_ev_get_space_handler(union acpi_operand_object *handler_desc);
+
+void acpi_ev_put_space_handler(union acpi_operand_object *handler_desc);
+
+void acpi_ev_flush_space_handler(union acpi_operand_object *handler_desc);
+
+s16 acpi_ev_space_handler_count(union acpi_operand_object *handler_desc);
+
/*
* evregion - Operation region support
*/
diff --git a/drivers/acpi/acpica/acobject.h b/drivers/acpi/acpica/acobject.h
index 8abb393dafab..a719d9733e6b 100644
--- a/drivers/acpi/acpica/acobject.h
+++ b/drivers/acpi/acpica/acobject.h
@@ -304,6 +304,7 @@ struct acpi_object_notify_handler {

struct acpi_object_addr_handler {
ACPI_OBJECT_COMMON_HEADER u8 space_id;
+ s16 invocation_count;
u8 handler_flags;
acpi_adr_space_handler handler;
struct acpi_namespace_node *node; /* Parent device */
diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c
index 78ac29351c9e..b27cc8e0507f 100644
--- a/drivers/acpi/acpica/evhandler.c
+++ b/drivers/acpi/acpica/evhandler.c
@@ -499,6 +499,7 @@ acpi_ev_install_space_handler(struct acpi_namespace_node * node,
handler_obj->address_space.space_id = (u8)space_id;
handler_obj->address_space.handler_flags = flags;
handler_obj->address_space.region_list = NULL;
+ handler_obj->address_space.invocation_count = 0;
handler_obj->address_space.node = node;
handler_obj->address_space.handler = handler;
handler_obj->address_space.context = context;
@@ -534,3 +535,119 @@ acpi_ev_install_space_handler(struct acpi_namespace_node * node,
unlock_and_exit:
return_ACPI_STATUS(status);
}
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_ev_flush_space_handler
+ *
+ * PARAMETERS: handler_desc - Address space object
+ * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
+ *
+ * RETURN: None.
+ *
+ * DESCRIPTION: Flush the reference of the given address space handler object.
+ *
+ ******************************************************************************/
+
+void acpi_ev_flush_space_handler(union acpi_operand_object *handler_desc)
+{
+ acpi_cpu_flags lock_flags;
+
+ ACPI_FUNCTION_TRACE_PTR(acpi_ev_flush_space_handler, handler_desc);
+
+ if (handler_desc && handler_desc->address_space.invocation_count >= 0) {
+ lock_flags =
+ acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
+ handler_desc->address_space.invocation_count |= ACPI_INT16_MIN;
+ acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
+ }
+
+ return_VOID;
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_ev_get_space_handler
+ *
+ * PARAMETERS: handler_desc - Address space object
+ * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
+ *
+ * RETURN: Status.
+ *
+ * DESCRIPTION: Acquire an reference of the given address space handler object.
+ *
+ ******************************************************************************/
+
+acpi_status acpi_ev_get_space_handler(union acpi_operand_object *handler_desc)
+{
+ acpi_cpu_flags lock_flags;
+
+ ACPI_FUNCTION_TRACE_PTR(acpi_ev_get_space_handler, handler_desc);
+
+ if (handler_desc && handler_desc->address_space.invocation_count >= 0) {
+ lock_flags =
+ acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
+ handler_desc->address_space.invocation_count++;
+ acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
+ return_ACPI_STATUS(AE_OK);
+ }
+
+ return_ACPI_STATUS(AE_ERROR);
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_ev_put_space_handler
+ *
+ * PARAMETERS: handler_desc - Address space object
+ * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
+ *
+ * RETURN: None.
+ *
+ * DESCRIPTION: Release an reference of the given address space handler object.
+ *
+ ******************************************************************************/
+
+void acpi_ev_put_space_handler(union acpi_operand_object *handler_desc)
+{
+ acpi_cpu_flags lock_flags;
+
+ ACPI_FUNCTION_TRACE_PTR(acpi_ev_put_space_handler, handler_desc);
+
+ if (handler_desc) {
+ lock_flags =
+ acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
+ handler_desc->address_space.invocation_count--;
+ acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
+ }
+
+ return_VOID;
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_ev_space_handler_count
+ *
+ * PARAMETERS: handler_desc - Address space object
+ * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
+ *
+ * RETURN: Invocation count of the handler.
+ *
+ * DESCRIPTION: Get the reference of the given address space handler object.
+ *
+ ******************************************************************************/
+
+s16 acpi_ev_space_handler_count(union acpi_operand_object *handler_desc)
+{
+ s16 count = 0;
+ acpi_cpu_flags lock_flags;
+
+ if (handler_desc) {
+ lock_flags =
+ acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
+ count = handler_desc->address_space.invocation_count;
+ acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
+ }
+
+ return (count);
+}
diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c
index 8eb8575e8c16..dcdd779257d0 100644
--- a/drivers/acpi/acpica/evregion.c
+++ b/drivers/acpi/acpica/evregion.c
@@ -165,6 +165,10 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
return_ACPI_STATUS(AE_NOT_EXIST);
}

+ status = acpi_ev_get_space_handler(handler_desc);
+ if (ACPI_FAILURE(status)) {
+ return_ACPI_STATUS(AE_NOT_EXIST);
+ }
context = handler_desc->address_space.context;

/*
@@ -185,7 +189,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
region_obj,
acpi_ut_get_region_name(region_obj->region.
space_id)));
- return_ACPI_STATUS(AE_NOT_EXIST);
+ status = AE_NOT_EXIST;
+ goto error_exit;
}

/*
@@ -210,7 +215,7 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
acpi_ut_get_region_name(region_obj->
region.
space_id)));
- return_ACPI_STATUS(status);
+ goto error_exit;
}

/* Region initialization may have been completed by region_setup */
@@ -306,6 +311,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
acpi_ex_enter_interpreter();
}

+error_exit:
+ acpi_ev_put_space_handler(handler_desc);
return_ACPI_STATUS(status);
}

diff --git a/drivers/acpi/acpica/evxfregn.c b/drivers/acpi/acpica/evxfregn.c
index 2d6f187939c7..c8c8538aae40 100644
--- a/drivers/acpi/acpica/evxfregn.c
+++ b/drivers/acpi/acpica/evxfregn.c
@@ -266,6 +266,15 @@ acpi_remove_address_space_handler(acpi_handle device,

*last_obj_ptr = handler_obj->address_space.next;

+ /* Wait for handlers to exit */
+
+ acpi_ev_flush_space_handler(handler_obj);
+ while (acpi_ev_space_handler_count(handler_obj) != ACPI_INT16_MIN) {
+ (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
+ acpi_os_sleep((u64)10);
+ (void)acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
+ }
+
/* Now we can delete the handler object */

acpi_ut_remove_reference(handler_obj);
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index 7000e66f768e..a341a9a8157f 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -65,6 +65,8 @@
#define ACPI_UINT16_MAX (u16)(~((u16) 0)) /* 0xFFFF */
#define ACPI_UINT32_MAX (u32)(~((u32) 0)) /* 0xFFFFFFFF */
#define ACPI_UINT64_MAX (u64)(~((u64) 0)) /* 0xFFFFFFFFFFFFFFFF */
+#define ACPI_INT16_MAX ((s16)(ACPI_UINT16_MAX>>1))
+#define ACPI_INT16_MIN ((s16)(-ACPI_INT16_MAX - 1))
#define ACPI_ASCII_MAX 0x7F

/*

[Rafael J. Wysocki]

Post by Aaron Lu

Post by Rafael J. Wysocki

Post by Rafael J. Wysocki

Post by Aaron Lu
+ if (!result) {
+ status = acpi_install_address_space_handler(
+ ACPI_HANDLE(parent), ACPI_ADR_SPACE_GPIO,
+ intel_xpower_pmic_gpio_handler, NULL, NULL);

So here we have a problem, because we can't unregister the opregion handler
registered above if this fails. Not nice.

I'm not correct in the cover letter, the actual problem with operation
The acpi_remove_address_space_handler doesn't wait for the current
running handler to return, so if we call
acpi_remove_address_space_handler in a module's exit function, the
handler's memory will be freed and the running handler will access to
a no longer valid memory place.
So as long as we can make sure the handler will not go away from the
memory, we are safe.

This only means that address space handlers cannot be removed from kernel
modules.

If the module can not be unloaded(no module exit call), then we should
be safe.

Post by Rafael J. Wysocki
Lv was trying to add a wrapper for that some time ago, maybe it's a good
idea to revive that?

Is it this one? If it is, I'll test it and then add the unload
functionality to the PMIC drivers.

Well, you need to wait for the patch below to be applied to upstream ACPICA
and transfered to Linux from there.

Post by Aaron Lu
Date: Tue, 25 Nov 2014 15:42:44 +0800
Subject: [PATCH] ACPICA: Events: Add invocation protection for operation region callbacks.
This patch adds invocation protection around operation region callbacks to
offer a module safe environment for OSPM provided address space handlers.
It is likely that OSPM where ModuleDevice is supported will implement
specific address space handlers in the modules. Thus the unloading of
the handlers' owner modules may lead to program crash around the invocation
if the handler callbacks are invoked without protection. Since the
handler callbacks are invoked inside of ACPICA, it is better to implement
invocation protection inside of ACPICA.
As address space handlers are expected to be executed in parallel, it is
not a good choice to implement protection using locks. This patch uses
reference counting based protection mechanism. When OSPM calls
acpi_remove_address_space_handler(), the function waits until all invocations
exit to ensure no invocation can happen after the unloading of the modules.
Note that this might be a workaround and not tested, better solution could
be implemented to tune the design of the namespace objects. Lv Zheng.
---
drivers/acpi/acpica/acevents.h | 9 ++++
drivers/acpi/acpica/acobject.h | 1 +
drivers/acpi/acpica/evhandler.c | 117 ++++++++++++++++++++++++++++++++++++++++
drivers/acpi/acpica/evregion.c | 11 +++-
drivers/acpi/acpica/evxfregn.c | 9 ++++
include/acpi/actypes.h | 2 +
6 files changed, 147 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/acpica/acevents.h b/drivers/acpi/acpica/acevents.h
index 7a7811a9fc26..3020ac4ab7a8 100644
--- a/drivers/acpi/acpica/acevents.h
+++ b/drivers/acpi/acpica/acevents.h
@@ -175,6 +175,15 @@ acpi_ev_install_space_handler(struct acpi_namespace_node *node,
acpi_adr_space_handler handler,
acpi_adr_space_setup setup, void *context);
+acpi_status
+acpi_ev_get_space_handler(union acpi_operand_object *handler_desc);
+
+void acpi_ev_put_space_handler(union acpi_operand_object *handler_desc);
+
+void acpi_ev_flush_space_handler(union acpi_operand_object *handler_desc);
+
+s16 acpi_ev_space_handler_count(union acpi_operand_object *handler_desc);
+
/*
* evregion - Operation region support
*/
diff --git a/drivers/acpi/acpica/acobject.h b/drivers/acpi/acpica/acobject.h
index 8abb393dafab..a719d9733e6b 100644
--- a/drivers/acpi/acpica/acobject.h
+++ b/drivers/acpi/acpica/acobject.h
@@ -304,6 +304,7 @@ struct acpi_object_notify_handler {
struct acpi_object_addr_handler {
ACPI_OBJECT_COMMON_HEADER u8 space_id;
+ s16 invocation_count;
u8 handler_flags;
acpi_adr_space_handler handler;
struct acpi_namespace_node *node; /* Parent device */
diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c
index 78ac29351c9e..b27cc8e0507f 100644
--- a/drivers/acpi/acpica/evhandler.c
+++ b/drivers/acpi/acpica/evhandler.c
@@ -499,6 +499,7 @@ acpi_ev_install_space_handler(struct acpi_namespace_node * node,
handler_obj->address_space.space_id = (u8)space_id;
handler_obj->address_space.handler_flags = flags;
handler_obj->address_space.region_list = NULL;
+ handler_obj->address_space.invocation_count = 0;
handler_obj->address_space.node = node;
handler_obj->address_space.handler = handler;
handler_obj->address_space.context = context;
@@ -534,3 +535,119 @@ acpi_ev_install_space_handler(struct acpi_namespace_node * node,
return_ACPI_STATUS(status);
}
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_ev_flush_space_handler
+ *
+ * PARAMETERS: handler_desc - Address space object
+ * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
+ *
+ * RETURN: None.
+ *
+ * DESCRIPTION: Flush the reference of the given address space handler object.
+ *
+ ******************************************************************************/
+
+void acpi_ev_flush_space_handler(union acpi_operand_object *handler_desc)
+{
+ acpi_cpu_flags lock_flags;
+
+ ACPI_FUNCTION_TRACE_PTR(acpi_ev_flush_space_handler, handler_desc);
+
+ if (handler_desc && handler_desc->address_space.invocation_count >= 0) {
+ lock_flags =
+ acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
+ handler_desc->address_space.invocation_count |= ACPI_INT16_MIN;
+ acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
+ }
+
+ return_VOID;
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_ev_get_space_handler
+ *
+ * PARAMETERS: handler_desc - Address space object
+ * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
+ *
+ * RETURN: Status.
+ *
+ * DESCRIPTION: Acquire an reference of the given address space handler object.
+ *
+ ******************************************************************************/
+
+acpi_status acpi_ev_get_space_handler(union acpi_operand_object *handler_desc)
+{
+ acpi_cpu_flags lock_flags;
+
+ ACPI_FUNCTION_TRACE_PTR(acpi_ev_get_space_handler, handler_desc);
+
+ if (handler_desc && handler_desc->address_space.invocation_count >= 0) {
+ lock_flags =
+ acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
+ handler_desc->address_space.invocation_count++;
+ acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
+ return_ACPI_STATUS(AE_OK);
+ }
+
+ return_ACPI_STATUS(AE_ERROR);
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_ev_put_space_handler
+ *
+ * PARAMETERS: handler_desc - Address space object
+ * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
+ *
+ * RETURN: None.
+ *
+ * DESCRIPTION: Release an reference of the given address space handler object.
+ *
+ ******************************************************************************/
+
+void acpi_ev_put_space_handler(union acpi_operand_object *handler_desc)
+{
+ acpi_cpu_flags lock_flags;
+
+ ACPI_FUNCTION_TRACE_PTR(acpi_ev_put_space_handler, handler_desc);
+
+ if (handler_desc) {
+ lock_flags =
+ acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
+ handler_desc->address_space.invocation_count--;
+ acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
+ }
+
+ return_VOID;
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_ev_space_handler_count
+ *
+ * PARAMETERS: handler_desc - Address space object
+ * (ACPI_TYPE_LOCAL_ADDRESS_HANDLER)
+ *
+ * RETURN: Invocation count of the handler.
+ *
+ * DESCRIPTION: Get the reference of the given address space handler object.
+ *
+ ******************************************************************************/
+
+s16 acpi_ev_space_handler_count(union acpi_operand_object *handler_desc)
+{
+ s16 count = 0;
+ acpi_cpu_flags lock_flags;
+
+ if (handler_desc) {
+ lock_flags =
+ acpi_os_acquire_lock(acpi_gbl_reference_count_lock);
+ count = handler_desc->address_space.invocation_count;
+ acpi_os_release_lock(acpi_gbl_reference_count_lock, lock_flags);
+ }
+
+ return (count);
+}
diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c
index 8eb8575e8c16..dcdd779257d0 100644
--- a/drivers/acpi/acpica/evregion.c
+++ b/drivers/acpi/acpica/evregion.c
@@ -165,6 +165,10 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
return_ACPI_STATUS(AE_NOT_EXIST);
}
+ status = acpi_ev_get_space_handler(handler_desc);
+ if (ACPI_FAILURE(status)) {
+ return_ACPI_STATUS(AE_NOT_EXIST);
+ }
context = handler_desc->address_space.context;
/*
@@ -185,7 +189,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
region_obj,
acpi_ut_get_region_name(region_obj->region.
space_id)));
- return_ACPI_STATUS(AE_NOT_EXIST);
+ status = AE_NOT_EXIST;
+ goto error_exit;
}
/*
@@ -210,7 +215,7 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
acpi_ut_get_region_name(region_obj->
region.
space_id)));
- return_ACPI_STATUS(status);
+ goto error_exit;
}
/* Region initialization may have been completed by region_setup */
@@ -306,6 +311,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
acpi_ex_enter_interpreter();
}
+ acpi_ev_put_space_handler(handler_desc);
return_ACPI_STATUS(status);
}
diff --git a/drivers/acpi/acpica/evxfregn.c b/drivers/acpi/acpica/evxfregn.c
index 2d6f187939c7..c8c8538aae40 100644
--- a/drivers/acpi/acpica/evxfregn.c
+++ b/drivers/acpi/acpica/evxfregn.c
@@ -266,6 +266,15 @@ acpi_remove_address_space_handler(acpi_handle device,
*last_obj_ptr = handler_obj->address_space.next;
+ /* Wait for handlers to exit */
+
+ acpi_ev_flush_space_handler(handler_obj);
+ while (acpi_ev_space_handler_count(handler_obj) != ACPI_INT16_MIN) {
+ (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
+ acpi_os_sleep((u64)10);
+ (void)acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
+ }
+
/* Now we can delete the handler object */
acpi_ut_remove_reference(handler_obj);
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index 7000e66f768e..a341a9a8157f 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -65,6 +65,8 @@
#define ACPI_UINT16_MAX (u16)(~((u16) 0)) /* 0xFFFF */
#define ACPI_UINT32_MAX (u32)(~((u32) 0)) /* 0xFFFFFFFF */
#define ACPI_UINT64_MAX (u64)(~((u64) 0)) /* 0xFFFFFFFFFFFFFFFF */
+#define ACPI_INT16_MAX ((s16)(ACPI_UINT16_MAX>>1))
+#define ACPI_INT16_MIN ((s16)(-ACPI_INT16_MAX - 1))
#define ACPI_ASCII_MAX 0x7F
/*

[Rafael J. Wysocki]

Post by Aaron Lu
The Baytrail-T-CR platform firmware has defined two customized operation
regions for PMIC chip Dollar Cove XPower - one is for power resource
handling and one is for thermal just like the CrystalCove one. This patch
adds support for them on top of the common PMIC opregion region code.
---
drivers/acpi/Kconfig | 6 +
drivers/acpi/Makefile | 1 +
drivers/acpi/pmic/intel_pmic_xpower.c | 277 ++++++++++++++++++++++++++++++++++
drivers/mfd/axp20x.c | 3 +
4 files changed, 287 insertions(+)
create mode 100644 drivers/acpi/pmic/intel_pmic_xpower.c
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 3e5f2056f946..227f0692cbff 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -408,6 +408,12 @@ config CRC_PMIC_OPREGION
help
This config adds ACPI operation region support for CrystalCove PMIC.
+config XPOWER_PMIC_OPREGION
+ bool "ACPI operation region support for XPower AXP288 PMIC"
+ depends on AXP288_ADC
+ help
+ This config adds ACPI operation region support for XPower AXP288 PMIC.
+
endif
endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index f5938399ac14..f74317cc1ca9 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -91,3 +91,4 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o
obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
+obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
new file mode 100644
index 000000000000..6c4d6ce0cff1
--- /dev/null
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -0,0 +1,277 @@
+/*
+ * intel_pmic_xpower.c - XPower AXP288 PMIC operation region driver
+ *
+ * Copyright (C) 2014 Intel Corporation. All rights reserved.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/mfd/axp20x.h>
+#include <linux/regmap.h>
+#include <linux/platform_device.h>
+#include <linux/iio/consumer.h>
+#include "intel_pmic.h"
+
+#define XPOWER_GPADC_LOW 0x5b
+
+static struct pmic_pwr_table pwr_table[] = {
+ {
+ .address = 0x00,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x05,
+ }
+ }, /* ALD1, 0x13, bit5 */

It is not necessary to repeat the reg numbers and bits in the comments.

Post by Aaron Lu
+ {
+ .address = 0x04,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x06,
+ }
+ }, /* ALD2, 0x13, bit6 */
+ {
+ .address = 0x08,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x07,
+ }
+ }, /* ALD3, 0x13, bit7 */
+ {
+ .address = 0x0c,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x03,
+ }
+ }, /* DLD1, 0x12, bit3 */
+ {
+ .address = 0x10,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x04,
+ }
+ }, /* DLD2, 0x12, bit4 */
+ {
+ .address = 0x14,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x05,
+ }
+ }, /* DLD3, 0x12, bit5 */
+ {
+ .address = 0x18,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x06,
+ }
+ }, /* DLD4, 0x12, bit6 */
+ {
+ .address = 0x1c,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x00,
+ }
+ }, /* ELD1, 0x12, bit0 */
+ {
+ .address = 0x20,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x01,
+ }
+ }, /* ELD2, 0x12, bit1 */
+ {
+ .address = 0x24,
+ .pwr_reg = {
+ .reg = 0x12,
+ .bit = 0x02,
+ }
+ }, /* ELD3, 0x12, bit2 */
+ {
+ .address = 0x28,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x02,
+ }
+ }, /* FLD1, 0x13, bit2 */
+ {
+ .address = 0x2c,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x03,
+ }
+ }, /* FLD2, 0x13, bit3 */
+ {
+ .address = 0x30,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x04,
+ }
+ }, /* FLD3, 0x13, bit4 */
+ {
+ .address = 0x38,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x03,
+ }
+ }, /* BUC1, 0x10, bit3 */
+ {
+ .address = 0x3c,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x06,
+ }
+ }, /* BUC2, 0x10, bit6 */
+ {
+ .address = 0x40,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x05,
+ }
+ }, /* BUC3, 0x10, bit5 */
+ {
+ .address = 0x44,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x04,
+ }
+ }, /* BUC4, 0x10, bit4 */
+ {
+ .address = 0x48,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x01,
+ }
+ }, /* BUC5, 0x10, bit1 */
+ {
+ .address = 0x4c,
+ .pwr_reg = {
+ .reg = 0x10,
+ .bit = 0x00
+ }
+ }, /* BUC6, 0x10, bit0 */
+};
+
+/* TMP0 - TMP5 are the same, all from GPADC */
+static struct pmic_thermal_table thermal_table[] = {
+ {
+ .address = 0x00,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x0c,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x18,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x24,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x30,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x3c,
+ .reg = XPOWER_GPADC_LOW
+ },
+};
+
+static int intel_xpower_pmic_get_power(struct regmap *regmap,
+ struct pmic_pwr_reg *preg, u64 *value)
+{
+ int data;
+
+ if (regmap_read(regmap, preg->reg, &data))
+ return -EIO;
+
+ *value = (data & BIT(preg->bit)) ? 1 : 0;
+ return 0;
+}
+
+static int intel_xpower_pmic_update_power(struct regmap *regmap,
+ struct pmic_pwr_reg *preg, bool on)
+{
+ int data;
+
+ if (regmap_read(regmap, preg->reg, &data))
+ return -EIO;
+
+ if (on)
+ data |= BIT(preg->bit);
+ else
+ data &= ~BIT(preg->bit);
+
+ if (regmap_write(regmap, preg->reg, data))
+ return -EIO;
+
+ return 0;
+}
+
+/*
+ * We could get the sensor value by manipulating the HW regs here, but since
+ * the axp288 IIO driver may also access the same regs at the same time, the
+ * APIs provided by IIO subsystem are used here instead to avoid problems. As
+ * a result, the two passed in params are of no actual use.
+ */
+static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
+{
+ struct iio_channel *gpadc_chan;
+ int ret, val;
+
+ gpadc_chan = iio_channel_get(NULL, "axp288-system-temp");
+ if (IS_ERR_OR_NULL(gpadc_chan))
+ return -EACCES;
+
+ ret = iio_read_channel_raw(gpadc_chan, &val);
+ if (ret < 0)
+ val = ret;
+
+ iio_channel_release(gpadc_chan);
+ return val;
+}
+
+static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
+ .get_power = intel_xpower_pmic_get_power,
+ .update_power = intel_xpower_pmic_update_power,
+ .get_raw_temp = intel_xpower_pmic_get_raw_temp,
+ .pwr_table = pwr_table,
+ .pwr_table_count = ARRAY_SIZE(pwr_table),
+ .thermal_table = thermal_table,
+ .thermal_table_count = ARRAY_SIZE(thermal_table),
+};
+
+
+static int intel_xpower_pmic_opregion_probe(struct platform_device *pdev)
+{
+ struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+ return intel_pmic_install_opregion_handler(&pdev->dev,
+ ACPI_HANDLE(pdev->dev.parent), axp20x->regmap,
+ &intel_xpower_pmic_opregion_data);
+}
+
+static struct platform_driver intel_xpower_pmic_opregion_driver = {
+ .probe = intel_xpower_pmic_opregion_probe,
+ .driver = {
+ .name = "axp288_opregion",
+ },
+};
+
+static int __init intel_xpower_pmic_opregion_driver_init(void)
+{
+ return platform_driver_register(&intel_xpower_pmic_opregion_driver);
+}
+module_init(intel_xpower_pmic_opregion_driver_init);
+
+MODULE_DESCRIPTION("XPower AXP288 ACPI operation region driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index acbb4c3bf98c..daf3c8d33b4d 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -354,6 +354,9 @@ static struct mfd_cell axp288_cells[] = {
.num_resources = ARRAY_SIZE(axp288_battery_resources),
.resources = axp288_battery_resources,
},
+ {
+ .name = "axp288_opregion",
+ },
};
static struct axp20x_dev *axp20x_pm_power_off;

The rest looks OK to me.

[Aaron Lu]

Post by Rafael J. Wysocki

Post by Aaron Lu
+static struct pmic_pwr_table pwr_table[] = {
+ {
+ .address = 0x00,
+ .pwr_reg = {
+ .reg = 0x13,
+ .bit = 0x05,
+ }
+ }, /* ALD1, 0x13, bit5 */

It is not necessary to repeat the reg numbers and bits in the comments.

I've removed them all, it's only useful when developing the code.
Here is the updated version:

From: Aaron Lu <***@intel.com>
Date: Mon, 20 Oct 2014 17:00:59 +0800
Subject: [PATCH 2/3] ACPI / PMIC: support PMIC operation region for XPower
AXP288

The Baytrail-T-CR platform firmware has defined two customized operation
regions for PMIC chip Dollar Cove XPower - one is for power resource
handling and one is for thermal just like the CrystalCove one. This patch
adds support for them on top of the common PMIC opregion region code.

Signed-off-by: Aaron Lu <***@intel.com>
Acked-by: Lee Jones <***@linaro.org> for the MFD part
---
drivers/acpi/Kconfig | 6 +
drivers/acpi/Makefile | 1 +
drivers/acpi/pmic/intel_pmic_xpower.c | 246 ++++++++++++++++++++++++++++++++++
drivers/mfd/axp20x.c | 3 +
4 files changed, 256 insertions(+)
create mode 100644 drivers/acpi/pmic/intel_pmic_xpower.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 3e5f2056f946..227f0692cbff 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -408,6 +408,12 @@ config CRC_PMIC_OPREGION
help
This config adds ACPI operation region support for CrystalCove PMIC.

+config XPOWER_PMIC_OPREGION
+ bool "ACPI operation region support for XPower AXP288 PMIC"
+ depends on AXP288_ADC
+ help
+ This config adds ACPI operation region support for XPower AXP288 PMIC.
+
endif

endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index f5938399ac14..f74317cc1ca9 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -91,3 +91,4 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o

obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o
obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
+obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
new file mode 100644
index 000000000000..795ca073db08
--- /dev/null
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -0,0 +1,246 @@
+/*
+ * intel_pmic_xpower.c - XPower AXP288 PMIC operation region driver
+ *
+ * Copyright (C) 2014 Intel Corporation. All rights reserved.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/mfd/axp20x.h>
+#include <linux/regmap.h>
+#include <linux/platform_device.h>
+#include <linux/iio/consumer.h>
+#include "intel_pmic.h"
+
+#define XPOWER_GPADC_LOW 0x5b
+
+static struct pmic_table power_table[] = {
+ {
+ .address = 0x00,
+ .reg = 0x13,
+ .bit = 0x05,
+ },
+ {
+ .address = 0x04,
+ .reg = 0x13,
+ .bit = 0x06,
+ },
+ {
+ .address = 0x08,
+ .reg = 0x13,
+ .bit = 0x07,
+ },
+ {
+ .address = 0x0c,
+ .reg = 0x12,
+ .bit = 0x03,
+ },
+ {
+ .address = 0x10,
+ .reg = 0x12,
+ .bit = 0x04,
+ },
+ {
+ .address = 0x14,
+ .reg = 0x12,
+ .bit = 0x05,
+ },
+ {
+ .address = 0x18,
+ .reg = 0x12,
+ .bit = 0x06,
+ },
+ {
+ .address = 0x1c,
+ .reg = 0x12,
+ .bit = 0x00,
+ },
+ {
+ .address = 0x20,
+ .reg = 0x12,
+ .bit = 0x01,
+ },
+ {
+ .address = 0x24,
+ .reg = 0x12,
+ .bit = 0x02,
+ },
+ {
+ .address = 0x28,
+ .reg = 0x13,
+ .bit = 0x02,
+ },
+ {
+ .address = 0x2c,
+ .reg = 0x13,
+ .bit = 0x03,
+ },
+ {
+ .address = 0x30,
+ .reg = 0x13,
+ .bit = 0x04,
+ },
+ {
+ .address = 0x38,
+ .reg = 0x10,
+ .bit = 0x03,
+ },
+ {
+ .address = 0x3c,
+ .reg = 0x10,
+ .bit = 0x06,
+ },
+ {
+ .address = 0x40,
+ .reg = 0x10,
+ .bit = 0x05,
+ },
+ {
+ .address = 0x44,
+ .reg = 0x10,
+ .bit = 0x04,
+ },
+ {
+ .address = 0x48,
+ .reg = 0x10,
+ .bit = 0x01,
+ },
+ {
+ .address = 0x4c,
+ .reg = 0x10,
+ .bit = 0x00
+ },
+};
+
+/* TMP0 - TMP5 are the same, all from GPADC */
+static struct pmic_table thermal_table[] = {
+ {
+ .address = 0x00,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x0c,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x18,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x24,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x30,
+ .reg = XPOWER_GPADC_LOW
+ },
+ {
+ .address = 0x3c,
+ .reg = XPOWER_GPADC_LOW
+ },
+};
+
+static int intel_xpower_pmic_get_power(struct regmap *regmap, int reg,
+ int bit, u64 *value)
+{
+ int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ *value = (data & BIT(bit)) ? 1 : 0;
+ return 0;
+}
+
+static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
+ int bit, bool on)
+{
+ int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ if (on)
+ data |= BIT(bit);
+ else
+ data &= ~BIT(bit);
+
+ if (regmap_write(regmap, reg, data))
+ return -EIO;
+
+ return 0;
+}
+
+/**
+ * intel_xpower_pmic_get_raw_temp(): Get raw temperature reading from the PMIC
+ *
+ * @regmap: regmap of the PMIC device
+ * @reg: register to get the reading
+ *
+ * We could get the sensor value by manipulating the HW regs here, but since
+ * the axp288 IIO driver may also access the same regs at the same time, the
+ * APIs provided by IIO subsystem are used here instead to avoid problems. As
+ * a result, the two passed in params are of no actual use.
+ *
+ * Return a positive value on success, errno on failure.
+ */
+static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
+{
+ struct iio_channel *gpadc_chan;
+ int ret, val;
+
+ gpadc_chan = iio_channel_get(NULL, "axp288-system-temp");
+ if (IS_ERR_OR_NULL(gpadc_chan))
+ return -EACCES;
+
+ ret = iio_read_channel_raw(gpadc_chan, &val);
+ if (ret < 0)
+ val = ret;
+
+ iio_channel_release(gpadc_chan);
+ return val;
+}
+
+static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
+ .get_power = intel_xpower_pmic_get_power,
+ .update_power = intel_xpower_pmic_update_power,
+ .get_raw_temp = intel_xpower_pmic_get_raw_temp,
+ .power_table = power_table,
+ .power_table_count = ARRAY_SIZE(power_table),
+ .thermal_table = thermal_table,
+ .thermal_table_count = ARRAY_SIZE(thermal_table),
+};
+
+
+static int intel_xpower_pmic_opregion_probe(struct platform_device *pdev)
+{
+ struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+ return intel_pmic_install_opregion_handler(&pdev->dev,
+ ACPI_HANDLE(pdev->dev.parent), axp20x->regmap,
+ &intel_xpower_pmic_opregion_data);
+}
+
+static struct platform_driver intel_xpower_pmic_opregion_driver = {
+ .probe = intel_xpower_pmic_opregion_probe,
+ .driver = {
+ .name = "axp288_opregion",
+ },
+};
+
+static int __init intel_xpower_pmic_opregion_driver_init(void)
+{
+ return platform_driver_register(&intel_xpower_pmic_opregion_driver);
+}
+module_init(intel_xpower_pmic_opregion_driver_init);
+
+MODULE_DESCRIPTION("XPower AXP288 ACPI operation region driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index acbb4c3bf98c..daf3c8d33b4d 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -354,6 +354,9 @@ static struct mfd_cell axp288_cells[] = {
.num_resources = ARRAY_SIZE(axp288_battery_resources),
.resources = axp288_battery_resources,
},
+ {
+ .name = "axp288_opregion",
+ },
};

static struct axp20x_dev *axp20x_pm_power_off;

[Aaron Lu]

The Baytrail-T platform firmware has defined two customized operation
regions for PMIC chip Crystal Cove - one is for power resource handling
and one is for thermal: sensor temperature reporting, trip point setting,
etc. This patch adds support for them on top of the existing Crystal Cove
PMIC driver.
The reason to split code into a separate file intel_pmic.c is that there
are more PMIC drivers with ACPI operation region support coming and we can
re-use those code. The intel_pmic_opregion_data structure is created also
for this purpose: when we need to support a new PMIC's operation region,
we just need to fill those callbacks and the two register mapping tables.

Thanks for resending, looks better to me.
Some nitpicking below.

Thaks for taking a look at them, some response below.

---
drivers/acpi/Kconfig | 17 ++
drivers/acpi/Makefile | 3 +
drivers/acpi/pmic/intel_pmic.c | 339 +++++++++++++++++++++++++++++++++++++
drivers/acpi/pmic/intel_pmic.h | 34 ++++
drivers/acpi/pmic/intel_pmic_crc.c | 216 +++++++++++++++++++++++
drivers/mfd/intel_soc_pmic_crc.c | 3 +
6 files changed, 612 insertions(+)
create mode 100644 drivers/acpi/pmic/intel_pmic.c
create mode 100644 drivers/acpi/pmic/intel_pmic.h
create mode 100644 drivers/acpi/pmic/intel_pmic_crc.c
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 79078b8f5697..3e5f2056f946 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -393,4 +393,21 @@ config ACPI_EXTLOG
driver adds support for that functionality with corresponding
tracepoint which carries that information to userspace.
+menuconfig PMIC_OPREGION
+ bool "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
+ to control power rails and sensor reading/writing on the
+ PMIC chip.
+
+if PMIC_OPREGION
+config CRC_PMIC_OPREGION

If that is the only possible choice for PMIC_OPREGION, it should be selected
automatically. Alternatively, PMIC_OPREGION should be selected automatically
if CRC_PMIC_OPREGION is set.

It is not the only possible choice, currently we have two(see patch 2/3):
CRC_PMIC_OPREGION and XPOWER_PMIC_OPREGION. I would assume this is a
increasing list with more and more PMIC opregion support added. I can
use select for PMIC_OPREGION for all those PMIC operation region drivers,
but it seems easier to use a "if PMIC_OPREGION ... endif" between them.
Please let me know if this is OK?

+ bool "ACPI operation region support for CrystalCove PMIC"
+ depends on INTEL_SOC_PMIC
+ help
+ This config adds ACPI operation region support for CrystalCove PMIC.
+
+endif
+
endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 6d11522f0e48..f5938399ac14 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -88,3 +88,6 @@ obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
obj-$(CONFIG_ACPI_APEI) += apei/
obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
+
+obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o
+obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
new file mode 100644
index 000000000000..5dbc0fb4d536
--- /dev/null
+++ b/drivers/acpi/pmic/intel_pmic.c
@@ -0,0 +1,339 @@
+/*
+ * intel_pmic.c - Intel PMIC operation region driver
+ *
+ * Copyright (C) 2014 Intel Corporation. All rights reserved.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/regmap.h>
+#include "intel_pmic.h"
+
+#define PMIC_PMOP_OPREGION_ID 0x8d
+#define PMIC_THERMAL_OPREGION_ID 0x8c
+
+struct acpi_lpat {
+ int temp;
+ int raw;
+};
+
+struct intel_pmic_opregion {
+ struct mutex lock;
+ struct acpi_lpat *lpat;
+ int lpat_count;
+ struct regmap *regmap;
+ struct intel_pmic_opregion_data *data;
+};
+
+static struct pmic_pwr_reg *
+pmic_get_pwr_reg(int address, struct pmic_pwr_table *table, int count)
+{
+ int i;
+
+ for (i = 0; i < count; i++) {
+ if (table[i].address == address)
+ return &table[i].pwr_reg;
+ }
+ return NULL;
+}
+
+static int
+pmic_get_thermal_reg(int address, struct pmic_thermal_table *table, int count)
+{
+ int i;
+
+ for (i = 0; i < count; i++) {
+ if (table[i].address == address)
+ return table[i].reg;
+ }
+ return -ENOENT;
+}

This is slightly inconsistent. While pmic_get_pwr_reg() returns a pointer
to struct pmic_pwr_reg, this one returns an int.
I see that this is because the definitions of struct pmic_thermal_table
and struct pmic_pwr_table are inconsistent, but is that really necessary?
You could define
struct pmic_table {
int address; /* operation region address */
int reg; /* corresponding PMIC register */
int bit; /* control bit for power */
};
and use it for both power and thermal. [The latter will not use the bit field,
but is that really a problem?]
It looks like some code duplication might be reduced this way.

Yes.

Besides, "power" looks better than "pwr", especially that you use "thermal"
instead of "thrm" (for example).

OK.

+
+/* Return temperature from raw value through LPAT table */
+static int raw_to_temp(struct acpi_lpat *lpat, int count, int raw)
+{
+ int i, delta_temp, delta_raw, temp;
+
+ for (i = 0; i < count - 1; i++) {
+ if ((raw >= lpat[i].raw && raw <= lpat[i+1].raw) ||
+ (raw <= lpat[i].raw && raw >= lpat[i+1].raw))
+ break;
+ }
+
+ if (i == count - 1)
+ return -ENOENT;
+
+ delta_temp = lpat[i+1].temp - lpat[i].temp;
+ delta_raw = lpat[i+1].raw - lpat[i].raw;
+ temp = lpat[i].temp + (raw - lpat[i].raw) * delta_temp / delta_raw;
+
+ return temp;
+}
+
+/* Return raw value from temperature through LPAT table */
+static int temp_to_raw(struct acpi_lpat *lpat, int count, int temp)
+{
+ int i, delta_temp, delta_raw, raw;
+
+ for (i = 0; i < count - 1; i++) {
+ if (temp >= lpat[i].temp && temp <= lpat[i+1].temp)
+ break;
+ }
+
+ if (i == count - 1)
+ return -ENOENT;
+
+ delta_temp = lpat[i+1].temp - lpat[i].temp;
+ delta_raw = lpat[i+1].raw - lpat[i].raw;
+ raw = lpat[i].raw + (temp - lpat[i].temp) * delta_raw / delta_temp;
+
+ return raw;
+}
+
+static void
+pmic_thermal_lpat(struct intel_pmic_opregion *opregion, acpi_handle handle,
+ struct device *dev)
+{
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ union acpi_object *obj_p, *obj_e;
+ int *lpat, i;
+ acpi_status status;
+
+ status = acpi_evaluate_object(handle, "LPAT", NULL, &buffer);
+ if (ACPI_FAILURE(status))
+ return;
+
+ obj_p = (union acpi_object *)buffer.pointer;
+ if (!obj_p || (obj_p->type != ACPI_TYPE_PACKAGE) ||
+ (obj_p->package.count % 2) || (obj_p->package.count < 4))
+ goto out;
+
+ lpat = devm_kmalloc(dev, sizeof(*lpat) * obj_p->package.count,
+ GFP_KERNEL);

This looks fishy.
Of course, sizeof(*lpat) is the same as sizeof(int), but is more obfuscated
and you're allocating memory for an array of integers.

OK.

+ if (!lpat)
+ goto out;
+
+ for (i = 0; i < obj_p->package.count; i++) {
+ obj_e = &obj_p->package.elements[i];
+ if (obj_e->type != ACPI_TYPE_INTEGER)

lpat[] has to be freed here.

Oh right.

+ goto out;
+ lpat[i] = obj_e->integer.value;

Here, integer.value is generally u64, so I'd use an explicit cast to s64 before
casting that to int. Otherwise it looks like you've forgotten about possible
overflows, which I assume is not the case.

OK.

+ }
+
+ opregion->lpat = (struct acpi_lpat *)lpat;
+ opregion->lpat_count = obj_p->package.count / 2;
+
+ kfree(buffer.pointer);
+}
+
+static acpi_status
+intel_pmic_pmop_handler(u32 function, acpi_physical_address address,
+ u32 bits, u64 *value64, void *handler_context,
+ void *region_context)
+{
+ struct intel_pmic_opregion *opregion = region_context;
+ struct regmap *regmap = opregion->regmap;
+ struct intel_pmic_opregion_data *d = opregion->data;
+ struct pmic_pwr_reg *preg;
+ int result;
+
+ if (bits != 32 || !value64)
+ return AE_BAD_PARAMETER;
+
+ if (function == ACPI_WRITE && !(*value64 == 0 || *value64 == 1))
+ return AE_BAD_PARAMETER;
+
+ preg = pmic_get_pwr_reg(address, d->pwr_table, d->pwr_table_count);
+ if (!preg)
+ return AE_BAD_PARAMETER;
+
+ mutex_lock(&opregion->lock);
+
+ if (function == ACPI_READ)
+ result = d->get_power(regmap, preg, value64);
+ else
+ result = d->update_power(regmap, preg, *value64 == 1);

I'd write that as
retult = function == ACPI_READ ?
d->update_power(regmap, preg, *value64 == 1);
which will be consistent with the "return" statement below.

OK.

+
+ mutex_unlock(&opregion->lock);
+
+ return result ? AE_ERROR : AE_OK;
+}
+
+static acpi_status pmic_read_temp(struct intel_pmic_opregion *opregion,
+ int reg, u64 *value)
+{
+ int raw_temp, temp;
+
+ if (!opregion->data->get_raw_temp)
+ return AE_BAD_PARAMETER;
+
+ raw_temp = opregion->data->get_raw_temp(opregion->regmap, reg);
+ if (raw_temp < 0)
+ return AE_ERROR;
+
+ if (!opregion->lpat) {
+ *value = raw_temp;
+ return AE_OK;
+ }
+
+ temp = raw_to_temp(opregion->lpat, opregion->lpat_count, raw_temp);
+ if (temp < 0)
+ return AE_ERROR;
+
+ *value = temp;
+ return AE_OK;
+}
+
+static acpi_status pmic_thermal_temp(struct intel_pmic_opregion *opregion,
+ int reg, u32 function, u64 *value)
+{
+ if (function != ACPI_READ)
+ return AE_BAD_PARAMETER;
+
+ return pmic_read_temp(opregion, reg, value);

What about
return function == ACPI_READ ?
pmic_read_temp(opregion, reg, value) : AE_BAD_PARAMETER;
?

OK.

+}
+
+static acpi_status pmic_thermal_aux(struct intel_pmic_opregion *opregion,
+ int reg, u32 function, u64 *value)
+{
+ int raw_temp;
+
+ if (function == ACPI_READ)
+ return pmic_read_temp(opregion, reg, value);
+
+ if (!opregion->data->update_aux)
+ return AE_BAD_PARAMETER;
+
+ if (opregion->lpat) {
+ raw_temp = temp_to_raw(opregion->lpat, opregion->lpat_count,
+ *value);
+ if (raw_temp < 0)
+ return AE_ERROR;
+ } else {
+ raw_temp = *value;
+ }
+
+ return opregion->data->update_aux(opregion->regmap, reg, raw_temp) ?
+ AE_ERROR : AE_OK;

You seem to be casting all error codes into AE_ERROR here. Should the function
simply return int and pass the original error code to the caller instead?

You mean pass the original error code to intel_pmic_thermal_handler?
Yes, I can do that. But since there isn't a 1-1 mapping between the
standard error code and ACPICA error values, I'm afriad I'll need to
cast them into AE_ERROR in intel_pmic_thermal_handler before return.

+}
+
+static acpi_status pmic_thermal_pen(struct intel_pmic_opregion *opregion,
+ int reg, u32 function, u64 *value)
+{
+ struct intel_pmic_opregion_data *d = opregion->data;
+ struct regmap *regmap = opregion->regmap;
+
+ if (!d->get_policy || !d->update_policy)
+ return AE_BAD_PARAMETER;
+
+ if (function == ACPI_READ)
+ return d->get_policy(regmap, reg, value) ? AE_ERROR : AE_OK;
+
+ if (*value != 0 || *value != 1)
+ return AE_BAD_PARAMETER;
+
+ return d->update_policy(regmap, reg, *value) ? AE_ERROR : AE_OK;

Well, same here.

+}
+
+static bool pmic_thermal_is_temp(int address)
+{
+ return (address <= 0x3c) && !(address % 12);
+}
+
+static bool pmic_thermal_is_aux(int address)
+{
+ return (address >= 4 && address <= 0x40 && !((address - 4) % 12)) ||
+ (address >= 8 && address <= 0x44 && !((address - 8) % 12));
+}
+
+static bool pmic_thermal_is_pen(int address)
+{
+ return address >= 0x48 && address <= 0x5c;
+}
+
+static acpi_status
+intel_pmic_thermal_handler(u32 function, acpi_physical_address address,
+ u32 bits, u64 *value64, void *handler_context,
+ void *region_context)
+{
+ struct intel_pmic_opregion *opregion = region_context;
+ int reg;
+ int result;
+
+ if (bits != 32 || !value64)
+ return AE_BAD_PARAMETER;
+
+ reg = pmic_get_thermal_reg(address, opregion->data->thermal_table,
+ opregion->data->thermal_table_count);
+ if (!reg)
+ return AE_BAD_PARAMETER;
+
+ mutex_lock(&opregion->lock);
+
+ result = AE_BAD_PARAMETER;
+ if (pmic_thermal_is_temp(address))
+ result = pmic_thermal_temp(opregion, reg, function, value64);
+ else if (pmic_thermal_is_aux(address))
+ result = pmic_thermal_aux(opregion, reg, function, value64);
+ else if (pmic_thermal_is_pen(address))
+ result = pmic_thermal_pen(opregion, reg, function, value64);
+
+ mutex_unlock(&opregion->lock);
+
+ return result;
+}
+
+int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
+ struct regmap *regmap,
+ struct intel_pmic_opregion_data *d)
+{
+ acpi_status status;
+ struct intel_pmic_opregion *opregion;
+
+ if (!dev || !regmap || !d)
+ return -EINVAL;
+
+ if (!handle)
+ return -ENODEV;
+
+ opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL);
+ if (!opregion)
+ return -ENOMEM;
+
+ mutex_init(&opregion->lock);
+ opregion->regmap = regmap;
+ pmic_thermal_lpat(opregion, handle, dev);
+
+ status = acpi_install_address_space_handler(handle,
+ PMIC_PMOP_OPREGION_ID,
+ intel_pmic_pmop_handler,
+ NULL, opregion);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;

And you return a int from here.

I prefer to use int whenever possible, i.e. if the function is not
returning a value to a ACPICA function, I'll use int as the return value
instead of acpi_status.

Would it make sense for the majority of functions in this file to return ints
rather than acpi_status values?

Yes, I think I can do that. Then I just need to do one cast in the
operation region handler function for those error return values.

+
+ status = acpi_install_address_space_handler(handle,
+ PMIC_THERMAL_OPREGION_ID,
+ intel_pmic_thermal_handler,
+ NULL, opregion);
+ if (ACPI_FAILURE(status)) {
+ acpi_remove_address_space_handler(handle, PMIC_PMOP_OPREGION_ID,
+ intel_pmic_pmop_handler);
+ return -ENODEV;
+ }
+
+ opregion->data = d;

I guess the opregion will never be removed, right?

Once installed properly, it will not be removed.

+ return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmic_install_opregion_handler);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h
new file mode 100644
index 000000000000..18b9bb80f8b6
--- /dev/null
+++ b/drivers/acpi/pmic/intel_pmic.h
@@ -0,0 +1,34 @@
+#ifndef __INTEL_PMIC_H
+#define __INTEL_PMIC_H
+
+struct pmic_pwr_reg {
+ int reg; /* corresponding PMIC register */
+ int bit; /* control bit for power */
+};
+
+struct pmic_pwr_table {
+ int address; /* operation region address */
+ struct pmic_pwr_reg pwr_reg;
+};
+
+struct pmic_thermal_table {
+ int address; /* operation region address */
+ int reg; /* corresponding thermal register */
+};
+
+struct intel_pmic_opregion_data {
+ int (*get_power)(struct regmap *r, struct pmic_pwr_reg *preg, u64 *value);
+ int (*update_power)(struct regmap *r, struct pmic_pwr_reg *preg, bool on);
+ int (*get_raw_temp)(struct regmap *r, int reg);
+ int (*update_aux)(struct regmap *r, int reg, int raw_temp);
+ int (*get_policy)(struct regmap *r, int reg, u64 *value);
+ int (*update_policy)(struct regmap *r, int reg, int enable);
+ struct pmic_pwr_table *pwr_table;
+ int pwr_table_count;
+ struct pmic_thermal_table *thermal_table;
+ int thermal_table_count;
+};
+
+int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle, struct regmap *regmap, struct intel_pmic_opregion_data *d);
+
+#endif
diff --git a/drivers/acpi/pmic/intel_pmic_crc.c b/drivers/acpi/pmic/intel_pmic_crc.c
new file mode 100644
index 000000000000..7629f16d1526
--- /dev/null
+++ b/drivers/acpi/pmic/intel_pmic_crc.c
@@ -0,0 +1,216 @@
+/*
+ * intel_pmic_crc.c - Intel CrystalCove PMIC operation region driver
+ *
+ * Copyright (C) 2014 Intel Corporation. All rights reserved.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/regmap.h>
+#include <linux/platform_device.h>
+#include "intel_pmic.h"
+
+#define PWR_SOURCE_SELECT BIT(1)
+
+#define PMIC_A0LOCK_REG 0xc5
+
+static struct pmic_pwr_table pwr_table[] = {
+ {
+ .address = 0x24,
+ .pwr_reg = {
+ .reg = 0x66,
+ .bit = 0x00,
+ },
+ }, /* X285 -> V2P85SX, camara */
+ {
+ .address = 0x48,
+ .pwr_reg = {
+ .reg = 0x5d,
+ .bit = 0x00,
+ },
+ }, /* V18X -> V1P8SX, eMMC/camara/audio */
+};
+
+static struct pmic_thermal_table thermal_table[] = {
+ {
+ .address = 0x00,
+ .reg = 0x75
+ }, /* TMP0 -> SYS0_THRM_RSLT_L */
+ {
+ .address = 0x04,
+ .reg = 0x95
+ }, /* AX00 -> SYS0_THRMALRT0_L */
+ {
+ .address = 0x08,
+ .reg = 0x97
+ }, /* AX01 -> SYS0_THRMALRT1_L */
+ {
+ .address = 0x0c,
+ .reg = 0x77
+ }, /* TMP1 -> SYS1_THRM_RSLT_L */
+ {
+ .address = 0x10,
+ .reg = 0x9a
+ }, /* AX10 -> SYS1_THRMALRT0_L */
+ {
+ .address = 0x14,
+ .reg = 0x9c
+ }, /* AX11 -> SYS1_THRMALRT1_L */
+ {
+ .address = 0x18,
+ .reg = 0x79
+ }, /* TMP2 -> SYS2_THRM_RSLT_L */
+ {
+ .address = 0x1c,
+ .reg = 0x9f
+ }, /* AX20 -> SYS2_THRMALRT0_L */
+ {
+ .address = 0x20,
+ .reg = 0xa1
+ }, /* AX21 -> SYS2_THRMALRT1_L */
+ {
+ .address = 0x48,
+ .reg = 0x94
+ }, /* PEN0 -> SYS0_THRMALRT0_H */
+ {
+ .address = 0x4c,
+ .reg = 0x99
+ }, /* PEN1 -> SYS1_THRMALRT1_H */
+ {
+ .address = 0x50,
+ .reg = 0x9e
+ }, /* PEN2 -> SYS2_THRMALRT2_H */
+};
+
+static int intel_crc_pmic_get_power(struct regmap *regmap,
+ struct pmic_pwr_reg *preg, u64 *value)
+{
+ int data;
+
+ if (regmap_read(regmap, preg->reg, &data))
+ return -EIO;
+
+ *value = (data & PWR_SOURCE_SELECT) && (data & BIT(preg->bit)) ? 1 : 0;
+ return 0;
+}
+
+static int intel_crc_pmic_update_power(struct regmap *regmap,
+ struct pmic_pwr_reg *preg, bool on)
+{
+ int data;
+
+ if (regmap_read(regmap, preg->reg, &data))
+ return -EIO;
+
+ if (on) {
+ data |= PWR_SOURCE_SELECT | BIT(preg->bit);
+ } else {
+ data &= ~BIT(preg->bit);
+ data |= PWR_SOURCE_SELECT;
+ }
+
+ if (regmap_write(regmap, preg->reg, data))
+ return -EIO;
+ return 0;
+}
+
+/* Raw temperature value is 10bits: 8bits in reg and 2bits in reg-1 bit0,1 */

Proper kerneldoc, please. Here and elsewhere where it makes sense.
All functions that aren't static need to have kerneldoc comments.

Will do that.

+static int intel_crc_pmic_get_raw_temp(struct regmap *regmap, int reg)
+{
+ int temp_l, temp_h;
+
+ if (regmap_read(regmap, reg, &temp_l) ||
+ regmap_read(regmap, reg - 1, &temp_h))
+ return -EIO;
+
+ return (temp_l | ((temp_h & 0x3) << 8));

At least one paren is not necessary here.

+}
+
+static int
+intel_crc_pmic_update_aux(struct regmap *regmap, int reg, int raw)
+{
+ if (regmap_write(regmap, reg, raw) ||
+ regmap_update_bits(regmap, reg - 1, 0x3, raw >> 8))
+ return -EIO;
+
+ return 0;

What about
return regmap_write(regmap, reg, raw) ||
regmap_update_bits(regmap, reg - 1, 0x3, raw >> 8) ? -EIO : 0;

OK.

+}
+
+static int
+intel_crc_pmic_get_policy(struct regmap *regmap, int reg, u64 *value)
+{
+ int pen;
+
+ if (regmap_read(regmap, reg, &pen))
+ return -EIO;
+ *value = pen >> 7;
+ return 0;
+}
+
+static int intel_crc_pmic_update_policy(struct regmap *regmap,
+ int reg, int enable)
+{
+ int alert0;
+
+ /* Update to policy enable bit requires unlocking a0lock */
+ if (regmap_read(regmap, PMIC_A0LOCK_REG, &alert0))
+ return -EIO;

Empty line here?

OK.

Thanks a lot for the review.

-Aaron

+ if (regmap_update_bits(regmap, PMIC_A0LOCK_REG, 0x01, 0))
+ return -EIO;
+
+ if (regmap_update_bits(regmap, reg, 0x80, enable << 7))
+ return -EIO;
+
+ /* restore alert0 */
+ if (regmap_write(regmap, PMIC_A0LOCK_REG, alert0))
+ return -EIO;
+
+ return 0;
+}
+
+static struct intel_pmic_opregion_data intel_crc_pmic_opregion_data = {
+ .get_power = intel_crc_pmic_get_power,
+ .update_power = intel_crc_pmic_update_power,
+ .get_raw_temp = intel_crc_pmic_get_raw_temp,
+ .update_aux = intel_crc_pmic_update_aux,
+ .get_policy = intel_crc_pmic_get_policy,
+ .update_policy = intel_crc_pmic_update_policy,
+ .pwr_table = pwr_table,
+ .pwr_table_count= ARRAY_SIZE(pwr_table),
+ .thermal_table = thermal_table,
+ .thermal_table_count = ARRAY_SIZE(thermal_table),
+};
+
+static int intel_crc_pmic_opregion_probe(struct platform_device *pdev)
+{
+ struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
+ return intel_pmic_install_opregion_handler(&pdev->dev,
+ ACPI_HANDLE(pdev->dev.parent), pmic->regmap,
+ &intel_crc_pmic_opregion_data);
+}
+
+static struct platform_driver intel_crc_pmic_opregion_driver = {
+ .probe = intel_crc_pmic_opregion_probe,
+ .driver = {
+ .name = "crystal_cove_region",
+ },
+};
+
+static int __init intel_crc_pmic_opregion_driver_init(void)
+{
+ return platform_driver_register(&intel_crc_pmic_opregion_driver);
+}
+module_init(intel_crc_pmic_opregion_driver_init);
+
+MODULE_DESCRIPTION("CrystalCove ACPI opration region driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index 7107cab832e6..48845d636bba 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -106,6 +106,9 @@ static struct mfd_cell crystal_cove_dev[] = {
.num_resources = ARRAY_SIZE(gpio_resources),
.resources = gpio_resources,
},
+ {
+ .name = "crystal_cove_region",
+ },
};
static struct regmap_config crystal_cove_regmap_config = {

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

[Aaron Lu]

The Baytrail-T platform firmware has defined two customized operation
regions for PMIC chip Crystal Cove - one is for power resource handling
and one is for thermal: sensor temperature reporting, trip point setting,
etc. This patch adds support for them on top of the existing Crystal Cove
PMIC driver.
The reason to split code into a separate file intel_pmic.c is that there
are more PMIC drivers with ACPI operation region support coming and we can
re-use those code. The intel_pmic_opregion_data structure is created also
for this purpose: when we need to support a new PMIC's operation region,
we just need to fill those callbacks and the two register mapping tables.

Thanks for resending, looks better to me.
Some nitpicking below.

Updated patch here:

From: Aaron Lu <***@intel.com>
Date: Mon, 20 Oct 2014 16:38:37 +0800
Subject: [PATCH 1/3] ACPI / PMIC: support PMIC operation region for
CrystalCove

The Baytrail-T platform firmware has defined two customized operation
regions for PMIC chip Crystal Cove - one is for power resource handling
and one is for thermal: sensor temperature reporting, trip point setting,
etc. This patch adds support for them on top of the existing Crystal Cove
PMIC driver.

The reason to split code into a separate file intel_pmic.c is that there
are more PMIC drivers with ACPI operation region support coming and we can
re-use those code. The intel_pmic_opregion_data structure is created also
for this purpose: when we need to support a new PMIC's operation region,
we just need to fill those callbacks and the two register mapping tables.

Signed-off-by: Aaron Lu <***@intel.com>
Acked-by: Lee Jones <***@linaro.org> for the MFD part
---
drivers/acpi/Kconfig | 17 ++
drivers/acpi/Makefile | 3 +
drivers/acpi/pmic/intel_pmic.c | 354 +++++++++++++++++++++++++++++++++++++
drivers/acpi/pmic/intel_pmic.h | 25 +++
drivers/acpi/pmic/intel_pmic_crc.c | 211 ++++++++++++++++++++++
drivers/mfd/intel_soc_pmic_crc.c | 3 +
6 files changed, 613 insertions(+)
create mode 100644 drivers/acpi/pmic/intel_pmic.c
create mode 100644 drivers/acpi/pmic/intel_pmic.h
create mode 100644 drivers/acpi/pmic/intel_pmic_crc.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 79078b8f5697..3e5f2056f946 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -393,4 +393,21 @@ config ACPI_EXTLOG
driver adds support for that functionality with corresponding
tracepoint which carries that information to userspace.

+menuconfig PMIC_OPREGION
+ bool "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
+ to control power rails and sensor reading/writing on the
+ PMIC chip.
+
+if PMIC_OPREGION
+config CRC_PMIC_OPREGION
+ bool "ACPI operation region support for CrystalCove PMIC"
+ depends on INTEL_SOC_PMIC
+ help
+ This config adds ACPI operation region support for CrystalCove PMIC.
+
+endif
+
endif # ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 6d11522f0e48..f5938399ac14 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -88,3 +88,6 @@ obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
obj-$(CONFIG_ACPI_APEI) += apei/

obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
+
+obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o
+obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
new file mode 100644
index 000000000000..8da794664c7f
--- /dev/null
+++ b/drivers/acpi/pmic/intel_pmic.c
@@ -0,0 +1,354 @@
+/*
+ * intel_pmic.c - Intel PMIC operation region driver
+ *
+ * Copyright (C) 2014 Intel Corporation. All rights reserved.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/regmap.h>
+#include "intel_pmic.h"
+
+#define PMIC_POWER_OPREGION_ID 0x8d
+#define PMIC_THERMAL_OPREGION_ID 0x8c
+
+struct acpi_lpat {
+ int temp;
+ int raw;
+};
+
+struct intel_pmic_opregion {
+ struct mutex lock;
+ struct acpi_lpat *lpat;
+ int lpat_count;
+ struct regmap *regmap;
+ struct intel_pmic_opregion_data *data;
+};
+
+static int pmic_get_reg_bit(int address, struct pmic_table *table,
+ int count, int *reg, int *bit)
+{
+ int i;
+
+ for (i = 0; i < count; i++) {
+ if (table[i].address == address) {
+ *reg = table[i].reg;
+ if (bit)
+ *bit = table[i].bit;
+ return 0;
+ }
+ }
+ return -ENOENT;
+}
+
+/**
+ * raw_to_temp(): Return temperature from raw value through LPAT table
+ *
+ * @lpat: the temperature_raw mapping table
+ * @count: the count of the above mapping table
+ * @raw: the raw value, used as a key to get the temerature from the
+ * above mapping table
+ *
+ * A positive value will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+static int raw_to_temp(struct acpi_lpat *lpat, int count, int raw)
+{
+ int i, delta_temp, delta_raw, temp;
+
+ for (i = 0; i < count - 1; i++) {
+ if ((raw >= lpat[i].raw && raw <= lpat[i+1].raw) ||
+ (raw <= lpat[i].raw && raw >= lpat[i+1].raw))
+ break;
+ }
+
+ if (i == count - 1)
+ return -ENOENT;
+
+ delta_temp = lpat[i+1].temp - lpat[i].temp;
+ delta_raw = lpat[i+1].raw - lpat[i].raw;
+ temp = lpat[i].temp + (raw - lpat[i].raw) * delta_temp / delta_raw;
+
+ return temp;
+}
+
+/**
+ * temp_to_raw(): Return raw value from temperature through LPAT table
+ *
+ * @lpat: the temperature_raw mapping table
+ * @count: the count of the above mapping table
+ * @temp: the temperature, used as a key to get the raw value from the
+ * above mapping table
+ *
+ * A positive value will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+static int temp_to_raw(struct acpi_lpat *lpat, int count, int temp)
+{
+ int i, delta_temp, delta_raw, raw;
+
+ for (i = 0; i < count - 1; i++) {
+ if (temp >= lpat[i].temp && temp <= lpat[i+1].temp)
+ break;
+ }
+
+ if (i == count - 1)
+ return -ENOENT;
+
+ delta_temp = lpat[i+1].temp - lpat[i].temp;
+ delta_raw = lpat[i+1].raw - lpat[i].raw;
+ raw = lpat[i].raw + (temp - lpat[i].temp) * delta_raw / delta_temp;
+
+ return raw;
+}
+
+static void pmic_thermal_lpat(struct intel_pmic_opregion *opregion,
+ acpi_handle handle, struct device *dev)
+{
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ union acpi_object *obj_p, *obj_e;
+ int *lpat, i;
+ acpi_status status;
+
+ status = acpi_evaluate_object(handle, "LPAT", NULL, &buffer);
+ if (ACPI_FAILURE(status))
+ return;
+
+ obj_p = (union acpi_object *)buffer.pointer;
+ if (!obj_p || (obj_p->type != ACPI_TYPE_PACKAGE) ||
+ (obj_p->package.count % 2) || (obj_p->package.count < 4))
+ goto out;
+
+ lpat = devm_kmalloc(dev, sizeof(int) * obj_p->package.count,
+ GFP_KERNEL);
+ if (!lpat)
+ goto out;
+
+ for (i = 0; i < obj_p->package.count; i++) {
+ obj_e = &obj_p->package.elements[i];
+ if (obj_e->type != ACPI_TYPE_INTEGER) {
+ devm_kfree(dev, lpat);
+ goto out;
+ }
+ lpat[i] = (s64)obj_e->integer.value;
+ }
+
+ opregion->lpat = (struct acpi_lpat *)lpat;
+ opregion->lpat_count = obj_p->package.count / 2;
+
+out:
+ kfree(buffer.pointer);
+}
+
+static acpi_status intel_pmic_power_handler(u32 function,
+ acpi_physical_address address, u32 bits, u64 *value64,
+ void *handler_context, void *region_context)
+{
+ struct intel_pmic_opregion *opregion = region_context;
+ struct regmap *regmap = opregion->regmap;
+ struct intel_pmic_opregion_data *d = opregion->data;
+ int reg, bit, result;
+
+ if (bits != 32 || !value64)
+ return AE_BAD_PARAMETER;
+
+ if (function == ACPI_WRITE && !(*value64 == 0 || *value64 == 1))
+ return AE_BAD_PARAMETER;
+
+ result = pmic_get_reg_bit(address, d->power_table,
+ d->power_table_count, &reg, &bit);
+ if (result == -ENOENT)
+ return AE_BAD_PARAMETER;
+
+ mutex_lock(&opregion->lock);
+
+ result = function == ACPI_READ ?
+ d->get_power(regmap, reg, bit, value64) :
+ d->update_power(regmap, reg, bit, *value64 == 1);
+
+ mutex_unlock(&opregion->lock);
+
+ return result ? AE_ERROR : AE_OK;
+}
+
+static int pmic_read_temp(struct intel_pmic_opregion *opregion,
+ int reg, u64 *value)
+{
+ int raw_temp, temp;
+
+ if (!opregion->data->get_raw_temp)
+ return -ENXIO;
+
+ raw_temp = opregion->data->get_raw_temp(opregion->regmap, reg);
+ if (raw_temp < 0)
+ return raw_temp;
+
+ if (!opregion->lpat) {
+ *value = raw_temp;
+ return 0;
+ }
+
+ temp = raw_to_temp(opregion->lpat, opregion->lpat_count, raw_temp);
+ if (temp < 0)
+ return temp;
+
+ *value = temp;
+ return 0;
+}
+
+static int pmic_thermal_temp(struct intel_pmic_opregion *opregion, int reg,
+ u32 function, u64 *value)
+{
+ return function == ACPI_READ ?
+ pmic_read_temp(opregion, reg, value) : -EINVAL;
+}
+
+static int pmic_thermal_aux(struct intel_pmic_opregion *opregion, int reg,
+ u32 function, u64 *value)
+{
+ int raw_temp;
+
+ if (function == ACPI_READ)
+ return pmic_read_temp(opregion, reg, value);
+
+ if (!opregion->data->update_aux)
+ return -ENXIO;
+
+ if (opregion->lpat) {
+ raw_temp = temp_to_raw(opregion->lpat, opregion->lpat_count,
+ *value);
+ if (raw_temp < 0)
+ return raw_temp;
+ } else {
+ raw_temp = *value;
+ }
+
+ return opregion->data->update_aux(opregion->regmap, reg, raw_temp);
+}
+
+static int pmic_thermal_pen(struct intel_pmic_opregion *opregion, int reg,
+ u32 function, u64 *value)
+{
+ struct intel_pmic_opregion_data *d = opregion->data;
+ struct regmap *regmap = opregion->regmap;
+
+ if (!d->get_policy || !d->update_policy)
+ return -ENXIO;
+
+ if (function == ACPI_READ)
+ return d->get_policy(regmap, reg, value);
+
+ if (*value != 0 || *value != 1)
+ return -EINVAL;
+
+ return d->update_policy(regmap, reg, *value);
+}
+
+static bool pmic_thermal_is_temp(int address)
+{
+ return (address <= 0x3c) && !(address % 12);
+}
+
+static bool pmic_thermal_is_aux(int address)
+{
+ return (address >= 4 && address <= 0x40 && !((address - 4) % 12)) ||
+ (address >= 8 && address <= 0x44 && !((address - 8) % 12));
+}
+
+static bool pmic_thermal_is_pen(int address)
+{
+ return address >= 0x48 && address <= 0x5c;
+}
+
+static acpi_status intel_pmic_thermal_handler(u32 function,
+ acpi_physical_address address, u32 bits, u64 *value64,
+ void *handler_context, void *region_context)
+{
+ struct intel_pmic_opregion *opregion = region_context;
+ struct intel_pmic_opregion_data *d = opregion->data;
+ int reg, result;
+
+ if (bits != 32 || !value64)
+ return AE_BAD_PARAMETER;
+
+ result = pmic_get_reg_bit(address, d->thermal_table,
+ d->thermal_table_count, &reg, NULL);
+ if (result == -ENOENT)
+ return AE_BAD_PARAMETER;
+
+ mutex_lock(&opregion->lock);
+
+ if (pmic_thermal_is_temp(address))
+ result = pmic_thermal_temp(opregion, reg, function, value64);
+ else if (pmic_thermal_is_aux(address))
+ result = pmic_thermal_aux(opregion, reg, function, value64);
+ else if (pmic_thermal_is_pen(address))
+ result = pmic_thermal_pen(opregion, reg, function, value64);
+ else
+ result = -EINVAL;
+
+ mutex_unlock(&opregion->lock);
+
+ if (result < 0) {
+ if (result == -EINVAL)
+ return AE_BAD_PARAMETER;
+ else
+ return AE_ERROR;
+ }
+
+ return AE_OK;
+}
+
+int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
+ struct regmap *regmap,
+ struct intel_pmic_opregion_data *d)
+{
+ acpi_status status;
+ struct intel_pmic_opregion *opregion;
+
+ if (!dev || !regmap || !d)
+ return -EINVAL;
+
+ if (!handle)
+ return -ENODEV;
+
+ opregion = devm_kzalloc(dev, sizeof(*opregion), GFP_KERNEL);
+ if (!opregion)
+ return -ENOMEM;
+
+ mutex_init(&opregion->lock);
+ opregion->regmap = regmap;
+ pmic_thermal_lpat(opregion, handle, dev);
+
+ status = acpi_install_address_space_handler(handle,
+ PMIC_POWER_OPREGION_ID,
+ intel_pmic_power_handler,
+ NULL, opregion);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ status = acpi_install_address_space_handler(handle,
+ PMIC_THERMAL_OPREGION_ID,
+ intel_pmic_thermal_handler,
+ NULL, opregion);
+ if (ACPI_FAILURE(status)) {
+ acpi_remove_address_space_handler(handle, PMIC_POWER_OPREGION_ID,
+ intel_pmic_power_handler);
+ return -ENODEV;
+ }
+
+ opregion->data = d;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmic_install_opregion_handler);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h
new file mode 100644
index 000000000000..d4e90af8f0dd
--- /dev/null
+++ b/drivers/acpi/pmic/intel_pmic.h
@@ -0,0 +1,25 @@
+#ifndef __INTEL_PMIC_H
+#define __INTEL_PMIC_H
+
+struct pmic_table {
+ int address; /* operation region address */
+ int reg; /* corresponding thermal register */
+ int bit; /* control bit for power */
+};
+
+struct intel_pmic_opregion_data {
+ int (*get_power)(struct regmap *r, int reg, int bit, u64 *value);
+ int (*update_power)(struct regmap *r, int reg, int bit, bool on);
+ int (*get_raw_temp)(struct regmap *r, int reg);
+ int (*update_aux)(struct regmap *r, int reg, int raw_temp);
+ int (*get_policy)(struct regmap *r, int reg, u64 *value);
+ int (*update_policy)(struct regmap *r, int reg, int enable);
+ struct pmic_table *power_table;
+ int power_table_count;
+ struct pmic_table *thermal_table;
+ int thermal_table_count;
+};
+
+int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle, struct regmap *regmap, struct intel_pmic_opregion_data *d);
+
+#endif
diff --git a/drivers/acpi/pmic/intel_pmic_crc.c b/drivers/acpi/pmic/intel_pmic_crc.c
new file mode 100644
index 000000000000..8955e5b41195
--- /dev/null
+++ b/drivers/acpi/pmic/intel_pmic_crc.c
@@ -0,0 +1,211 @@
+/*
+ * intel_pmic_crc.c - Intel CrystalCove PMIC operation region driver
+ *
+ * Copyright (C) 2014 Intel Corporation. All rights reserved.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/regmap.h>
+#include <linux/platform_device.h>
+#include "intel_pmic.h"
+
+#define PWR_SOURCE_SELECT BIT(1)
+
+#define PMIC_A0LOCK_REG 0xc5
+
+static struct pmic_table power_table[] = {
+ {
+ .address = 0x24,
+ .reg = 0x66,
+ .bit = 0x00,
+ },
+ {
+ .address = 0x48,
+ .reg = 0x5d,
+ .bit = 0x00,
+ },
+};
+
+static struct pmic_table thermal_table[] = {
+ {
+ .address = 0x00,
+ .reg = 0x75
+ },
+ {
+ .address = 0x04,
+ .reg = 0x95
+ },
+ {
+ .address = 0x08,
+ .reg = 0x97
+ },
+ {
+ .address = 0x0c,
+ .reg = 0x77
+ },
+ {
+ .address = 0x10,
+ .reg = 0x9a
+ },
+ {
+ .address = 0x14,
+ .reg = 0x9c
+ },
+ {
+ .address = 0x18,
+ .reg = 0x79
+ },
+ {
+ .address = 0x1c,
+ .reg = 0x9f
+ },
+ {
+ .address = 0x20,
+ .reg = 0xa1
+ },
+ {
+ .address = 0x48,
+ .reg = 0x94
+ },
+ {
+ .address = 0x4c,
+ .reg = 0x99
+ },
+ {
+ .address = 0x50,
+ .reg = 0x9e
+ },
+};
+
+static int intel_crc_pmic_get_power(struct regmap *regmap, int reg,
+ int bit, u64 *value)
+{
+ int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ *value = (data & PWR_SOURCE_SELECT) && (data & BIT(bit)) ? 1 : 0;
+ return 0;
+}
+
+static int intel_crc_pmic_update_power(struct regmap *regmap, int reg,
+ int bit, bool on)
+{
+ int data;
+
+ if (regmap_read(regmap, reg, &data))
+ return -EIO;
+
+ if (on) {
+ data |= PWR_SOURCE_SELECT | BIT(bit);
+ } else {
+ data &= ~BIT(bit);
+ data |= PWR_SOURCE_SELECT;
+ }
+
+ if (regmap_write(regmap, reg, data))
+ return -EIO;
+ return 0;
+}
+
+static int intel_crc_pmic_get_raw_temp(struct regmap *regmap, int reg)
+{
+ int temp_l, temp_h;
+
+ /*
+ * Raw temperature value is 10bits: 8bits in reg
+ * and 2bits in reg-1: bit0,1
+ */
+ if (regmap_read(regmap, reg, &temp_l) ||
+ regmap_read(regmap, reg - 1, &temp_h))
+ return -EIO;
+
+ return temp_l | (temp_h & 0x3) << 8;
+}
+
+static int intel_crc_pmic_update_aux(struct regmap *regmap, int reg, int raw)
+{
+ return regmap_write(regmap, reg, raw) ||
+ regmap_update_bits(regmap, reg - 1, 0x3, raw >> 8) ? -EIO : 0;
+}
+
+static int intel_crc_pmic_get_policy(struct regmap *regmap, int reg, u64 *value)
+{
+ int pen;
+
+ if (regmap_read(regmap, reg, &pen))
+ return -EIO;
+ *value = pen >> 7;
+ return 0;
+}
+
+static int intel_crc_pmic_update_policy(struct regmap *regmap,
+ int reg, int enable)
+{
+ int alert0;
+
+ /* Update to policy enable bit requires unlocking a0lock */
+ if (regmap_read(regmap, PMIC_A0LOCK_REG, &alert0))
+ return -EIO;
+
+ if (regmap_update_bits(regmap, PMIC_A0LOCK_REG, 0x01, 0))
+ return -EIO;
+
+ if (regmap_update_bits(regmap, reg, 0x80, enable << 7))
+ return -EIO;
+
+ /* restore alert0 */
+ if (regmap_write(regmap, PMIC_A0LOCK_REG, alert0))
+ return -EIO;
+
+ return 0;
+}
+
+static struct intel_pmic_opregion_data intel_crc_pmic_opregion_data = {
+ .get_power = intel_crc_pmic_get_power,
+ .update_power = intel_crc_pmic_update_power,
+ .get_raw_temp = intel_crc_pmic_get_raw_temp,
+ .update_aux = intel_crc_pmic_update_aux,
+ .get_policy = intel_crc_pmic_get_policy,
+ .update_policy = intel_crc_pmic_update_policy,
+ .power_table = power_table,
+ .power_table_count= ARRAY_SIZE(power_table),
+ .thermal_table = thermal_table,
+ .thermal_table_count = ARRAY_SIZE(thermal_table),
+};
+
+static int intel_crc_pmic_opregion_probe(struct platform_device *pdev)
+{
+ struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
+ return intel_pmic_install_opregion_handler(&pdev->dev,
+ ACPI_HANDLE(pdev->dev.parent), pmic->regmap,
+ &intel_crc_pmic_opregion_data);
+}
+
+static struct platform_driver intel_crc_pmic_opregion_driver = {
+ .probe = intel_crc_pmic_opregion_probe,
+ .driver = {
+ .name = "crystal_cove_region",
+ },
+};
+
+static int __init intel_crc_pmic_opregion_driver_init(void)
+{
+ return platform_driver_register(&intel_crc_pmic_opregion_driver);
+}
+module_init(intel_crc_pmic_opregion_driver_init);
+
+MODULE_DESCRIPTION("CrystalCove ACPI opration region driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index 7107cab832e6..48845d636bba 100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -106,6 +106,9 @@ static struct mfd_cell crystal_cove_dev[] = {
.num_resources = ARRAY_SIZE(gpio_resources),
.resources = gpio_resources,
},
+ {
+ .name = "crystal_cove_region",
+ },
};

static struct regmap_config crystal_cove_regmap_config = {

[Rafael J. Wysocki]

- 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
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.
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.
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.
https://lkml.org/lkml/2014/9/8/801
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.
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.

[Aaron Lu]

Post by Rafael J. Wysocki

- 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
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.
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.
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.
https://lkml.org/lkml/2014/9/8/801
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.
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 <***@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:
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 <***@intel.com>
Signed-off-by: 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"
+
+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"
+
+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);

[Rafael J. Wysocki]

Post by Aaron Lu

Post by Rafael J. Wysocki

- 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
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.
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.
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.
https://lkml.org/lkml/2014/9/8/801
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.
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.

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

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.

Post by Aaron Lu
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.
---
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.

Post by Aaron Lu
+
+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.

Post by Aaron Lu
+
+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);

[Aaron Lu]

Post by Rafael J. Wysocki

Post by Aaron Lu

Post by Rafael J. Wysocki

- 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
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.
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.
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.
https://lkml.org/lkml/2014/9/8/801
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.
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.

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

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!

Post by Rafael J. Wysocki

Post by Aaron Lu
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.
---
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

Post by Rafael J. Wysocki

Post by Aaron Lu
+
+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.

Post by Aaron Lu
+
+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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Aaron Lu]

Post by Aaron Lu

Post by Rafael J. Wysocki

Post by Aaron Lu

Post by Rafael J. Wysocki

- 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
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.
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.
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.
https://lkml.org/lkml/2014/9/8/801
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.
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.

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

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!

Post by Rafael J. Wysocki

Post by Aaron Lu
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.
---
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?

Or we can simply use 'acpi' instead of 'region' or 'opregion' here, to
mean that this cell device is for ACPI purpose. How does this sound?


Thanks,
Aaron

Post by Aaron Lu

Post by Rafael J. Wysocki

Post by Aaron Lu
+
+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.

Post by Aaron Lu
+
+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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/