Support PMIC operation region for CrystalCove and XPower

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 <[email protected]>

Acked-by Lee Jones <[email protected]> for the MFD part

---
 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

drivers/acpi/Kconfig +0 -0

index 79078b8f5697..3e5f2056f946 100644
@@ -393,4 +393,21 @@ config ACPI_EXTLOG
 	  driver adds support for that functionality with corresponding
 	  tracepoint which carries that information to userspace.
 

drivers/acpi/Kconfig +7 -0

+	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

drivers/acpi/Makefile +3 -0

index 6d11522f0e48..f5938399ac14 100644
@@ -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

drivers/acpi/pmic/intel_pmic.c new file +49 -0

new file mode 100644
index 000000000000..5dbc0fb4d536
@@ -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

drivers/acpi/pmic/intel_pmic.c new file +52 -0

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

drivers/acpi/pmic/intel_pmic.c new file +26 -0

+	}
+
+	opregion->lpat = (struct acpi_lpat *)lpat;
+	opregion->lpat_count = obj_p->package.count / 2;
+
+out:
+	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;
+

drivers/acpi/pmic/intel_pmic.c new file +28 -0

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

drivers/acpi/pmic/intel_pmic.c new file +14 -0

+}
+
+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) {

drivers/acpi/pmic/intel_pmic.c new file +8 -0

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

drivers/acpi/pmic/intel_pmic.c new file +67 -0

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

drivers/acpi/pmic/intel_pmic.c new file +5 -0

+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmic_install_opregion_handler);
+
+MODULE_LICENSE("GPL");

drivers/acpi/pmic/intel_pmic.h new file +34 -0

new file mode 100644
index 000000000000..18b9bb80f8b6
@@ -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

drivers/acpi/pmic/intel_pmic_crc.c new file +117 -0

new file mode 100644
index 000000000000..7629f16d1526
@@ -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 {

drivers/acpi/pmic/intel_pmic_crc.c new file +11 -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;

drivers/acpi/pmic/intel_pmic_crc.c new file +49 -0

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

drivers/mfd/intel_soc_pmic_crc.c +3 -0

index 7107cab832e6..48845d636bba 100644
@@ -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 = {

Quoted 12 lines · 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.

Quoted 2 lines

Signed-off-by Aaron Lu <[email protected]>

Acked-by Lee Jones <[email protected]> for the MFD part

Thanks for resending, looks better to me.

Some nitpicking below.

Quoted 21 lines

---
 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

drivers/acpi/Kconfig +0 -0

index 79078b8f5697..3e5f2056f946 100644
@@ -393,4 +393,21 @@ config ACPI_EXTLOG
 	  driver adds support for that functionality with corresponding
 	  tracepoint which carries that information to userspace.
 

drivers/acpi/Kconfig config ACPI_EXTLOG · line 405

Quoted 10 lines

drivers/acpi/Kconfig +10 -0

+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.

Quoted 75 lines

drivers/acpi/Kconfig +7 -0

+	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

drivers/acpi/Makefile +3 -0

index 6d11522f0e48..f5938399ac14 100644
@@ -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

drivers/acpi/pmic/intel_pmic.c new file +49 -0

new file mode 100644
index 000000000000..5dbc0fb4d536
@@ -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

drivers/acpi/pmic/intel_pmic.c line 59

Quoted 10 lines

drivers/acpi/pmic/intel_pmic.c new file +10 -0

+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.

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

Quoted 53 lines

drivers/acpi/pmic/intel_pmic.c new file +52 -0

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

drivers/acpi/pmic/intel_pmic.c line 121

Quoted 10 lines

drivers/acpi/pmic/intel_pmic.c new file +10 -0

+	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.

drivers/acpi/pmic/intel_pmic.c line 127

Quoted 7 lines

drivers/acpi/pmic/intel_pmic.c new file +6 -0

+	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.

drivers/acpi/pmic/intel_pmic.c line 129

Quoted 3 lines

drivers/acpi/pmic/intel_pmic.c new file +2 -0

+			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.

Quoted 27 lines

drivers/acpi/pmic/intel_pmic.c new file +26 -0

+	}
+
+	opregion->lpat = (struct acpi_lpat *)lpat;
+	opregion->lpat_count = obj_p->package.count / 2;
+
+out:
+	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;
+

drivers/acpi/pmic/intel_pmic.c line 165

Quoted 10 lines

drivers/acpi/pmic/intel_pmic.c new file +10 -0

+	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->get_power(regmap, preg, value64) :
		d->update_power(regmap, preg, *value64 == 1);

which will be consistent with the "return" statement below.

Quoted 29 lines

drivers/acpi/pmic/intel_pmic.c new file +28 -0

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

drivers/acpi/pmic/intel_pmic.c line 203

Quoted 10 lines

drivers/acpi/pmic/intel_pmic.c new file +10 -0

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

?

Quoted 20 lines

drivers/acpi/pmic/intel_pmic.c new file +19 -0

+}
+
+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 {

drivers/acpi/pmic/intel_pmic.c line 227

Quoted 5 lines

drivers/acpi/pmic/intel_pmic.c new file +5 -0

+		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?

Quoted 9 lines

drivers/acpi/pmic/intel_pmic.c new file +8 -0

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

drivers/acpi/pmic/intel_pmic.c line 245

Quoted 10 lines

drivers/acpi/pmic/intel_pmic.c new file +10 -0

+	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.

Quoted 71 lines

drivers/acpi/pmic/intel_pmic.c new file +71 -0

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

drivers/acpi/pmic/intel_pmic.c line 322

Quoted 6 lines

drivers/acpi/pmic/intel_pmic.c new file +6 -0

+	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.

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

drivers/acpi/pmic/intel_pmic.c line 334

Quoted 13 lines

drivers/acpi/pmic/intel_pmic.c new file +12 -0

+
+	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?

Quoted 169 lines

drivers/acpi/pmic/intel_pmic.c new file +5 -0

+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmic_install_opregion_handler);
+
+MODULE_LICENSE("GPL");

drivers/acpi/pmic/intel_pmic.h new file +34 -0

new file mode 100644
index 000000000000..18b9bb80f8b6
@@ -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

drivers/acpi/pmic/intel_pmic_crc.c new file +117 -0

new file mode 100644
index 000000000000..7629f16d1526
@@ -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 {

drivers/acpi/pmic/intel_pmic_crc.c line 127

Quoted 10 lines

drivers/acpi/pmic/intel_pmic_crc.c new file +10 -0

+		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.

drivers/acpi/pmic/intel_pmic_crc.c line 136

Quoted 10 lines

drivers/acpi/pmic/intel_pmic_crc.c new file +9 -0

+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.

drivers/acpi/pmic/intel_pmic_crc.c line 146

Quoted 10 lines

drivers/acpi/pmic/intel_pmic_crc.c new file +10 -0

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

Quoted 16 lines

drivers/acpi/pmic/intel_pmic_crc.c new file +15 -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)

drivers/acpi/pmic/intel_pmic_crc.c line 167

Quoted 6 lines

drivers/acpi/pmic/intel_pmic_crc.c new file +6 -0

+{
+	int alert0;
+
+	/* Update to policy enable bit requires unlocking a0lock */
+	if (regmap_read(regmap, PMIC_A0LOCK_REG, &alert0))
+		return -EIO;

Empty line here?

Quoted 65 lines

drivers/acpi/pmic/intel_pmic_crc.c new file +49 -0

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

drivers/mfd/intel_soc_pmic_crc.c +3 -0

index 7107cab832e6..48845d636bba 100644
@@ -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 = {

Quoted 12 lines · 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.

Quoted 2 lines

Signed-off-by Aaron Lu <[email protected]>

Acked-by Lee Jones <[email protected]> for the MFD part

Thanks for resending, looks better to me.

Some nitpicking below.

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 <[email protected]>

Acked-by Lee Jones <[email protected]> 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

drivers/acpi/Kconfig +6 -0

index 3e5f2056f946..227f0692cbff 100644
@@ -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

drivers/acpi/Makefile +1 -0

index f5938399ac14..f74317cc1ca9 100644
@@ -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

drivers/acpi/pmic/intel_pmic_xpower.c new file +23 -0

new file mode 100644
index 000000000000..6c4d6ce0cff1
@@ -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"
+

drivers/acpi/pmic/intel_pmic_xpower.c new file +244 -0

+	{
+		.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");

drivers/mfd/axp20x.c +0 -0

index acbb4c3bf98c..daf3c8d33b4d 100644

Review

Quoted 8 lines · Aaron Lu

(quoted diff) +8 -0

+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.

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 <[email protected]>

---
 drivers/acpi/pmic/intel_pmic_xpower.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

drivers/acpi/pmic/intel_pmic_xpower.c +8 -4

index 6c4d6ce0cff1..480c41c36444 100644
@@ -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);

drivers/acpi/pmic/intel_pmic_xpower.c +5 -0

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

Quoted 42 lines · 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 <[email protected]>

---
 drivers/acpi/pmic/intel_pmic_xpower.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

drivers/acpi/pmic/intel_pmic_xpower.c +13 -4

index 6c4d6ce0cff1..480c41c36444 100644
@@ -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,

drivers/acpi/pmic/intel_pmic_xpower.c static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = { · line 274

Quoted 5 lines

drivers/acpi/pmic/intel_pmic_xpower.c +5 -0

+					&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.

Quoted 10 lines

drivers/acpi/pmic/intel_pmic_xpower.c +5 -0

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

Review

Quoted 4 lines · Aaron Lu

(quoted diff) +4 -0

+	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.

Quoted 8 lines · Rafael J. Wysocki

Review

Quoted 4 lines · Aaron Lu

(quoted diff) +4 -0

+	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 region remove is with module unload, it happens like this:

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.

With this said, the updated patch is here:

From: Aaron Lu <[email protected]>
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.

Signed-off-by Aaron Lu <[email protected]>

---
 drivers/acpi/pmic/intel_pmic_xpower.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

drivers/acpi/pmic/intel_pmic_xpower.c +26 -4

index 795ca073db08..9ec57ebd81c9 100644
@@ -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 = {

From: Rafael J. Wysocki [mailto:[email protected]]
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

Quoted 19 lines · Aaron Lu

Quoted 8 lines · Rafael J. Wysocki

Review

Quoted 4 lines · Aaron Lu

(quoted diff) +4 -0

+	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 region remove is with module unload, it happens like this:

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.

Quoted 2 lines

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?

Quoted 55 lines

With this said, the updated patch is here:

From: Aaron Lu <[email protected]>
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.

Signed-off-by Aaron Lu <[email protected]>

---
 drivers/acpi/pmic/intel_pmic_xpower.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

drivers/acpi/pmic/intel_pmic_xpower.c +19 -4

index 795ca073db08..9ec57ebd81c9 100644
@@ -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,

drivers/acpi/pmic/intel_pmic_xpower.c static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = { · line 254

Quoted 10 lines

drivers/acpi/pmic/intel_pmic_xpower.c +7 -0

+					&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.

Quoted 19 lines · Aaron Lu

Quoted 8 lines · Rafael J. Wysocki

Review

Quoted 4 lines · Aaron Lu

(quoted diff) +4 -0

+	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 region remove is with module unload, it happens like this:

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.

Quoted 2 lines

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?

Quoted 20 lines · Rafael J. Wysocki

Quoted 19 lines · Aaron Lu

Quoted 8 lines · Rafael J. Wysocki

Review

Quoted 4 lines · Aaron Lu

(quoted diff) +4 -0

+	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 region remove is with module unload, it happens like this:

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 1 line

Quoted 2 lines

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 <[email protected]>
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 <[email protected]>

---
 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(-)

drivers/acpi/acpica/acevents.h +9 -0

index 7a7811a9fc26..3020ac4ab7a8 100644
@@ -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
  */

drivers/acpi/acpica/acobject.h +1 -0

index 8abb393dafab..a719d9733e6b 100644
@@ -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 */

drivers/acpi/acpica/evhandler.c +117 -0

index 78ac29351c9e..b27cc8e0507f 100644
@@ -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);
+}

drivers/acpi/acpica/evregion.c +9 -2

index 8eb8575e8c16..dcdd779257d0 100644
@@ -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);
 }
 

drivers/acpi/acpica/evxfregn.c +9 -0

index 2d6f187939c7..c8c8538aae40 100644
@@ -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);

include/acpi/actypes.h +2 -0

index 7000e66f768e..a341a9a8157f 100644
@@ -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
 
 /*

v3:
Only some function/variable name changes, no functiona changes:
- Replace the dptf/DPTF word originate from the BIOS ACPI table with more meaningful word thermal/THERMAL in all places;
- Eliminate the soc part in various structure and function names to make them shorter:
intel_soc_pmic_opregion -> intel_pmic_opregion intel_soc_pmic_pmop_handler -> intel_pmic_pmop_handler intel_soc_pmic_install_opregion_handler -> intel_pmic_install_opregion_handler etc.

v2:
Place PMIC operation files under drivers/acpi/pmic instead of drivers/acpi/pmic_opregion as suggested by Rafael;
Rename PMIC operation files to make them shorter as suggested by Rafael.

v1:
On Intel Baytrail-T and Baytrail-T-CR platforms, there are two customized ACPI operation regions defined for the Power Management Integrated Circuit device, one is for power resource handling and one is for thermal: sensor temperature reporting, trip point setting, etc. There are different PMIC chips used on those platforms and though each has the same two operation regions and functionality, their implementation is different so every PMIC will need a driver. But since their functionality is similar, some common code is abstracted into the intel_soc_pmic_opregion.c.

The last version is posted here:
https://lkml.org/lkml/2014/9/8/801

Changes since then:
1 Move to drivers/acpi as discussed on the above thread;
2 Added support for XPower AXP288 PMIC operation region support; 3 Since operation region handler can not be removed(at the moment at least),
use bool for the two operation region driver configs instead of tristate; Another reason to do this is that, with Mika's MFD ACPI support patch, all those MFD cell devices created will have the same modalias as their parent's so it doesn't make much sense to compile these drivers into modules.

Patch 1 applies on top of Rafael's pm-next branch, and then patch 2 and patch 3 needs merge of Lee's mfd/ib-mfd-iio-3.19 branch where the PMIC driver XPower AXP288 and iio driver axp288_adc is located.

Aaron Lu (3):
ACPI / PMIC: support PMIC operation region for CrystalCove ACPI / PMIC: support PMIC operation region for XPower AXP288 ACPI / PMIC: AXP288: support virtual GPIO in ACPI table

Quoted 44 lines · Aaron Lu

v3:
Only some function/variable name changes, no functiona changes:
- Replace the dptf/DPTF word originate from the BIOS ACPI table with more meaningful word thermal/THERMAL in all places;
- Eliminate the soc part in various structure and function names to make them shorter:
intel_soc_pmic_opregion -> intel_pmic_opregion intel_soc_pmic_pmop_handler -> intel_pmic_pmop_handler intel_soc_pmic_install_opregion_handler -> intel_pmic_install_opregion_handler etc.

v2:
Place PMIC operation files under drivers/acpi/pmic instead of drivers/acpi/pmic_opregion as suggested by Rafael;
Rename PMIC operation files to make them shorter as suggested by Rafael.

v1:
On Intel Baytrail-T and Baytrail-T-CR platforms, there are two customized ACPI operation regions defined for the Power Management Integrated Circuit device, one is for power resource handling and one is for thermal: sensor temperature reporting, trip point setting, etc. There are different PMIC chips used on those platforms and though each has the same two operation regions and functionality, their implementation is different so every PMIC will need a driver. But since their functionality is similar, some common code is abstracted into the intel_soc_pmic_opregion.c.

The last version is posted here:
https://lkml.org/lkml/2014/9/8/801

Changes since then:
1 Move to drivers/acpi as discussed on the above thread;
2 Added support for XPower AXP288 PMIC operation region support; 3 Since operation region handler can not be removed(at the moment at least),
use bool for the two operation region driver configs instead of tristate; Another reason to do this is that, with Mika's MFD ACPI support patch, all those MFD cell devices created will have the same modalias as their parent's so it doesn't make much sense to compile these drivers into modules.

Patch 1 applies on top of Rafael's pm-next branch, and then patch 2 and patch 3 needs merge of Lee's mfd/ib-mfd-iio-3.19 branch where the PMIC driver XPower AXP288 and iio driver axp288_adc is located.

Quoted 4 lines

Aaron Lu (3):
ACPI / PMIC: support PMIC operation region for CrystalCove ACPI / PMIC: support PMIC operation region for XPower AXP288 ACPI / PMIC: AXP288: support virtual GPIO in ACPI table

OK

I've pulled the Lee's 'mfd/ib-mfd-iio-3.19' branch and applied your updated three on top of that. Please check the bleeding-edge branch of linux-pm.git for the result.

Quoted 50 lines · Rafael J. Wysocki

Quoted 44 lines · Aaron Lu

v3:
Only some function/variable name changes, no functiona changes:
- Replace the dptf/DPTF word originate from the BIOS ACPI table with more meaningful word thermal/THERMAL in all places;
- Eliminate the soc part in various structure and function names to make them shorter:
intel_soc_pmic_opregion -> intel_pmic_opregion intel_soc_pmic_pmop_handler -> intel_pmic_pmop_handler intel_soc_pmic_install_opregion_handler -> intel_pmic_install_opregion_handler etc.

v2:
Place PMIC operation files under drivers/acpi/pmic instead of drivers/acpi/pmic_opregion as suggested by Rafael;
Rename PMIC operation files to make them shorter as suggested by Rafael.

v1:
On Intel Baytrail-T and Baytrail-T-CR platforms, there are two customized ACPI operation regions defined for the Power Management Integrated Circuit device, one is for power resource handling and one is for thermal: sensor temperature reporting, trip point setting, etc. There are different PMIC chips used on those platforms and though each has the same two operation regions and functionality, their implementation is different so every PMIC will need a driver. But since their functionality is similar, some common code is abstracted into the intel_soc_pmic_opregion.c.

The last version is posted here:
https://lkml.org/lkml/2014/9/8/801

Changes since then:
1 Move to drivers/acpi as discussed on the above thread;
2 Added support for XPower AXP288 PMIC operation region support; 3 Since operation region handler can not be removed(at the moment at least),
use bool for the two operation region driver configs instead of tristate; Another reason to do this is that, with Mika's MFD ACPI support patch, all those MFD cell devices created will have the same modalias as their parent's so it doesn't make much sense to compile these drivers into modules.

Patch 1 applies on top of Rafael's pm-next branch, and then patch 2 and patch 3 needs merge of Lee's mfd/ib-mfd-iio-3.19 branch where the PMIC driver XPower AXP288 and iio driver axp288_adc is located.

Quoted 4 lines

Aaron Lu (3):
ACPI / PMIC: support PMIC operation region for CrystalCove ACPI / PMIC: support PMIC operation region for XPower AXP288 ACPI / PMIC: AXP288: support virtual GPIO in ACPI table

Quoted 5 lines

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 <[email protected]>
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 <[email protected]>

Signed-off-by Aaron Lu <[email protected]>

---
 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(-)

drivers/acpi/Kconfig +3 -3

index 227f0692cbff..f9459ba4ce59 100644
@@ -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.

drivers/acpi/pmic/intel_pmic_crc.c +0 -0

index 8955e5b41195..8a193381b5ee 100644

drivers/acpi/pmic/intel_pmic_crc.c +10 -1

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

drivers/acpi/pmic/intel_pmic_xpower.c +0 -0

index 9ec57ebd81c9..4debcbbd6285 100644

drivers/acpi/pmic/intel_pmic_xpower.c +10 -1

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

Quoted 63 lines · Aaron Lu

Quoted 50 lines · Rafael J. Wysocki

Quoted 44 lines · Aaron Lu

v3:
Only some function/variable name changes, no functiona changes:
- Replace the dptf/DPTF word originate from the BIOS ACPI table with more meaningful word thermal/THERMAL in all places;
- Eliminate the soc part in various structure and function names to make them shorter:
intel_soc_pmic_opregion -> intel_pmic_opregion intel_soc_pmic_pmop_handler -> intel_pmic_pmop_handler intel_soc_pmic_install_opregion_handler -> intel_pmic_install_opregion_handler etc.

v2:
Place PMIC operation files under drivers/acpi/pmic instead of drivers/acpi/pmic_opregion as suggested by Rafael;
Rename PMIC operation files to make them shorter as suggested by Rafael.

v1:
On Intel Baytrail-T and Baytrail-T-CR platforms, there are two customized ACPI operation regions defined for the Power Management Integrated Circuit device, one is for power resource handling and one is for thermal: sensor temperature reporting, trip point setting, etc. There are different PMIC chips used on those platforms and though each has the same two operation regions and functionality, their implementation is different so every PMIC will need a driver. But since their functionality is similar, some common code is abstracted into the intel_soc_pmic_opregion.c.

The last version is posted here:
https://lkml.org/lkml/2014/9/8/801

Changes since then:
1 Move to drivers/acpi as discussed on the above thread;
2 Added support for XPower AXP288 PMIC operation region support; 3 Since operation region handler can not be removed(at the moment at least),
use bool for the two operation region driver configs instead of tristate; Another reason to do this is that, with Mika's MFD ACPI support patch, all those MFD cell devices created will have the same modalias as their parent's so it doesn't make much sense to compile these drivers into modules.

Patch 1 applies on top of Rafael's pm-next branch, and then patch 2 and patch 3 needs merge of Lee's mfd/ib-mfd-iio-3.19 branch where the PMIC driver XPower AXP288 and iio driver axp288_adc is located.

Quoted 4 lines

Aaron Lu (3):
ACPI / PMIC: support PMIC operation region for CrystalCove ACPI / PMIC: support PMIC operation region for XPower AXP288 ACPI / PMIC: AXP288: support virtual GPIO in ACPI table

Quoted 5 lines

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 <[email protected]>
Date: Tue, 25 Nov 2014 14:35:38 +0800
Subject: [PATCH] ACPI / PMIC: Make it possible to build PMIC driver as a module

Quoted 2 lines

This can solve a problem that when axp288_adc driver is built as a module and the PMIC driver is builtin, following error would ocur:

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

Quoted 56 lines

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 <[email protected]>

Signed-off-by Aaron Lu <[email protected]>

---
 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(-)

drivers/acpi/Kconfig +3 -3

index 227f0692cbff..f9459ba4ce59 100644
@@ -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.

drivers/acpi/pmic/intel_pmic_crc.c +0 -0

index 8955e5b41195..8a193381b5ee 100644
@@ -194,13 +194,23 @@ static int intel_crc_pmic_opregion_probe(struct platform_device *pdev)
 			&intel_crc_pmic_opregion_data);
 }
 

drivers/acpi/pmic/intel_pmic_crc.c static int intel_crc_pmic_opregion_probe(struct platform_device *pdev) · line 197

Quoted 1 line

drivers/acpi/pmic/intel_pmic_crc.c +1 -0

+#define DRV_NAME "crystal_cove_region"

This name is just horrible, BTW.

Quoted 26 lines

drivers/acpi/pmic/intel_pmic_crc.c +10 -1

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

drivers/acpi/pmic/intel_pmic_xpower.c +0 -0

index 9ec57ebd81c9..4debcbbd6285 100644

drivers/acpi/pmic/intel_pmic_xpower.c static int intel_xpower_pmic_opregion_probe(struct platform_device *pdev) · line 254

Quoted 4 lines

drivers/acpi/pmic/intel_pmic_xpower.c +1 -0

@@ -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.

Quoted 21 lines

drivers/acpi/pmic/intel_pmic_xpower.c +10 -1

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

Quoted 64 lines · Rafael J. Wysocki

Quoted 63 lines · Aaron Lu

Quoted 50 lines · Rafael J. Wysocki

Quoted 44 lines · Aaron Lu

v3:
Only some function/variable name changes, no functiona changes:
- Replace the dptf/DPTF word originate from the BIOS ACPI table with more meaningful word thermal/THERMAL in all places;
- Eliminate the soc part in various structure and function names to make them shorter:
intel_soc_pmic_opregion -> intel_pmic_opregion intel_soc_pmic_pmop_handler -> intel_pmic_pmop_handler intel_soc_pmic_install_opregion_handler -> intel_pmic_install_opregion_handler etc.

v2:
Place PMIC operation files under drivers/acpi/pmic instead of drivers/acpi/pmic_opregion as suggested by Rafael;
Rename PMIC operation files to make them shorter as suggested by Rafael.

v1:
On Intel Baytrail-T and Baytrail-T-CR platforms, there are two customized ACPI operation regions defined for the Power Management Integrated Circuit device, one is for power resource handling and one is for thermal: sensor temperature reporting, trip point setting, etc. There are different PMIC chips used on those platforms and though each has the same two operation regions and functionality, their implementation is different so every PMIC will need a driver. But since their functionality is similar, some common code is abstracted into the intel_soc_pmic_opregion.c.

The last version is posted here:
https://lkml.org/lkml/2014/9/8/801

Changes since then:
1 Move to drivers/acpi as discussed on the above thread;
2 Added support for XPower AXP288 PMIC operation region support; 3 Since operation region handler can not be removed(at the moment at least),
use bool for the two operation region driver configs instead of tristate; Another reason to do this is that, with Mika's MFD ACPI support patch, all those MFD cell devices created will have the same modalias as their parent's so it doesn't make much sense to compile these drivers into modules.

Patch 1 applies on top of Rafael's pm-next branch, and then patch 2 and patch 3 needs merge of Lee's mfd/ib-mfd-iio-3.19 branch where the PMIC driver XPower AXP288 and iio driver axp288_adc is located.

Quoted 4 lines

Aaron Lu (3):
ACPI / PMIC: support PMIC operation region for CrystalCove ACPI / PMIC: support PMIC operation region for XPower AXP288 ACPI / PMIC: AXP288: support virtual GPIO in ACPI table

Quoted 5 lines

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 <[email protected]>
Date: Tue, 25 Nov 2014 14:35:38 +0800
Subject: [PATCH] ACPI / PMIC: Make it possible to build PMIC driver as a module

Quoted 6 lines

Quoted 2 lines

This can solve a problem that when axp288_adc driver is built as a module and the PMIC driver is builtin, following error would ocur:

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

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

Quoted 56 lines

Quoted 56 lines

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 <[email protected]>

Signed-off-by Aaron Lu <[email protected]>

---
 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(-)

drivers/acpi/Kconfig +3 -3

index 227f0692cbff..f9459ba4ce59 100644
@@ -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.

drivers/acpi/pmic/intel_pmic_crc.c +0 -0

index 8955e5b41195..8a193381b5ee 100644
@@ -194,13 +194,23 @@ static int intel_crc_pmic_opregion_probe(struct platform_device *pdev)
 			&intel_crc_pmic_opregion_data);
 }
 

Quoted 3 lines

drivers/acpi/pmic/intel_pmic_crc.c static int intel_crc_pmic_opregion_probe(struct platform_device *pdev) · line 197

Quoted 1 line

drivers/acpi/pmic/intel_pmic_crc.c +1 -0

+#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?

Quoted 58 lines

Quoted 26 lines

drivers/acpi/pmic/intel_pmic_crc.c +10 -1

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

drivers/acpi/pmic/intel_pmic_xpower.c +0 -0

index 9ec57ebd81c9..4debcbbd6285 100644

drivers/acpi/pmic/intel_pmic_xpower.c static int intel_xpower_pmic_opregion_probe(struct platform_device *pdev) · line 254

Quoted 4 lines

drivers/acpi/pmic/intel_pmic_xpower.c +1 -0

@@ -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.

Quoted 21 lines

drivers/acpi/pmic/intel_pmic_xpower.c +10 -1

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