acpi/pmic: Add support for PMIC regs operation region

PATCH In review Bin Gao 5 participants 6 messages Jun 24, 2016 – Jun 27, 2016

Subsystems (2)

ACPI ACPI PMIC drivers

lore

Share & tools

lore

Bin Gao

author

2016-06-24 00:48 UTC #0 lore raw

PATCH v4 3/3 acpi/pmic: Add support for PMIC regs operation region

Broxton platform firmware has defined new customized operation regions called regs for PMIC chip - regs op region is used to handle the PMIC gpio mainly intended for the TYPE-C VBUS and Orientation.

The intel_gpio_ctx structure is created for the purpose of handling the PMIC gpio register read and write.

Signed-off-by Felipe Balbi <[email protected]>

Signed-off-by Chandra Sekhar Anagani <[email protected]>

Signed-off-by Bin Gao <[email protected]>

---
Changes in v4:
- various fixes to address Aaron's comments.
Changes in v3: none
Changes in v2: none

 drivers/acpi/pmic/intel_pmic.c | 74 ++++++++++++++++++++++++++++++++++++++++--
 drivers/acpi/pmic/intel_pmic.h |  5 +++
 2 files changed, 76 insertions(+), 3 deletions(-)

drivers/acpi/pmic/intel_pmic.c +71 -3

index 410e96f..e11d1e0 100644
@@ -21,12 +21,14 @@
 
 #define PMIC_POWER_OPREGION_ID		0x8d
 #define PMIC_THERMAL_OPREGION_ID	0x8c
+#define PMIC_REGS_OPREGION_ID		0x8f
 
 struct intel_pmic_opregion {
 	struct mutex lock;
 	struct acpi_lpat_conversion_table *lpat_table;
 	struct regmap *regmap;
 	struct intel_pmic_opregion_data *data;
+	struct pmic_gpio_ctx	ctx;
 };
 
 static int pmic_get_reg_bit(int address, struct pmic_table *table,

@@ -204,6 +206,56 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
 	return AE_OK;
 }
 
+static acpi_status intel_pmic_gpio_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 result;
+
+	switch (address) {
+	case 0:
+		return AE_OK;
+	case 1:
+		opregion->ctx.address |= (*value64 & 0xff) << 8;
+		return AE_OK;
+	case 2:
+		opregion->ctx.address |= *value64 & 0xff;
+		return AE_OK;
+	case 3:
+		opregion->ctx.value = *value64 & 0xff;
+		return AE_OK;
+	case 4:
+		if (*value64) {
+			result = regmap_write(opregion->regmap,
+					 opregion->ctx.address,
+					opregion->ctx.value);
+		} else {
+			result = regmap_read(opregion->regmap,
+					opregion->ctx.address,
+					&opregion->ctx.value);
+			if (result == 0)
+				*value64 = opregion->ctx.value;
+		}
+		memset(&opregion->ctx, 0x00, sizeof(opregion->ctx));
+		break;
+	default:
+		pr_err("%s(): Inavlid address %llu, please check the BIOS\n",
+							 __func__, address);
+		return AE_ERROR;
+	}
+
+	if (result < 0) {
+		if (result == -EINVAL)
+			return AE_BAD_PARAMETER;
+		else
+			return AE_ERROR;
+	}
+
+	return AE_OK;
+}
+
+
 int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
 					struct regmap *regmap,
 					struct intel_pmic_opregion_data *d)

@@ -240,15 +292,31 @@ int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
 						    intel_pmic_thermal_handler,
 						    NULL, opregion);
 	if (ACPI_FAILURE(status)) {
-		acpi_remove_address_space_handler(handle, PMIC_POWER_OPREGION_ID,
-						  intel_pmic_power_handler);
 		ret = -ENODEV;
-		goto out_error;
+		goto remove_pmic_power_handler;
+	}
+
+	status = acpi_install_address_space_handler(handle,
+						    PMIC_REGS_OPREGION_ID,
+						    intel_pmic_gpio_handler,
+						    NULL, opregion);
+	if (ACPI_FAILURE(status)) {
+		ret = -ENODEV;
+		goto remove_pmic_thermal_handler;
 	}
 
 	opregion->data = d;
 	return 0;
 
+remove_pmic_thermal_handler:
+		acpi_remove_address_space_handler(handle,
+						PMIC_POWER_OPREGION_ID,
+						intel_pmic_thermal_handler);
+remove_pmic_power_handler:
+		acpi_remove_address_space_handler(handle,
+						PMIC_POWER_OPREGION_ID,
+						intel_pmic_power_handler);
+
 out_error:
 	acpi_lpat_free_conversion_table(opregion->lpat_table);
 	return ret;

drivers/acpi/pmic/intel_pmic.h +5 -0

index e8bfa7b..c87c2ef 100644
@@ -7,6 +7,11 @@ struct pmic_table {
 	int bit;	/* control bit for power */
 };
 
+struct pmic_gpio_ctx {
+	u16 address;		/* pmic regs address */
+	unsigned int value;	/* value to write @regs address */
+};
+
 struct intel_pmic_opregion_data {
 	int (*get_power)(struct regmap *r, int reg, int bit, u64 *value);
 	int (*update_power)(struct regmap *r, int reg, int bit, bool on);

Signature · 1 line

1.9.1

Heikki Krogerus

2016-06-27 09:27 UTC #1 lore raw

Whoa! Hold on!

Quoted 25 lines · Bin Gao

Broxton platform firmware has defined new customized operation regions called regs for PMIC chip - regs op region is used to handle the PMIC gpio mainly intended for the TYPE-C VBUS and Orientation.
The intel_gpio_ctx structure is created for the purpose of handling the PMIC gpio register read and write.
Signed-off-by Felipe Balbi <[email protected]>
Signed-off-by Chandra Sekhar Anagani <[email protected]>
Signed-off-by Bin Gao <[email protected]>
---
Changes in v4:
- various fixes to address Aaron's comments.
Changes in v3: none
Changes in v2: none
 drivers/acpi/pmic/intel_pmic.c | 74 ++++++++++++++++++++++++++++++++++++++++--
 drivers/acpi/pmic/intel_pmic.h |  5 +++
 2 files changed, 76 insertions(+), 3 deletions(-)
drivers/acpi/pmic/intel_pmic.c +0 -0
index 410e96f..e11d1e0 100644
@@ -21,12 +21,14 @@
 

drivers/acpi/pmic/intel_pmic.c line 31

Quoted 10 lines

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

 #define PMIC_POWER_OPREGION_ID		0x8d
 #define PMIC_THERMAL_OPREGION_ID	0x8c
+#define PMIC_REGS_OPREGION_ID		0x8f
 
 struct intel_pmic_opregion {
 	struct mutex lock;
 	struct acpi_lpat_conversion_table *lpat_table;
 	struct regmap *regmap;
 	struct intel_pmic_opregion_data *data;
+	struct pmic_gpio_ctx	ctx;

What gpio?

drivers/acpi/pmic/intel_pmic.c static acpi_status intel_pmic_thermal_handler(u32 function, · line 211

Quoted 10 lines

drivers/acpi/pmic/intel_pmic.c +3 -0

 };
 
 static int pmic_get_reg_bit(int address, struct pmic_table *table,

@@ -204,6 +206,56 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
 	return AE_OK;
 }
 
+static acpi_status intel_pmic_gpio_handler(u32 function,
+	acpi_physical_address address, u32 bits, u64 *value64,
+		void *handler_context, void *region_context)

What the heck is this? Why is this suddenly a gpio handler?

This is handler for an operation region, not some gpio!

Signature · 1 line

heikki

Rafael J. Wysocki

maintainer

2016-06-27 13:25 UTC #2 lore raw

Quoted 48 lines · Heikki Krogerus

Whoa! Hold on!
Quoted 25 lines · Bin Gao
Broxton platform firmware has defined new customized operation regions called regs for PMIC chip - regs op region is used to handle the PMIC gpio mainly intended for the TYPE-C VBUS and Orientation.
The intel_gpio_ctx structure is created for the purpose of handling the PMIC gpio register read and write.
Signed-off-by Felipe Balbi <[email protected]>
Signed-off-by Chandra Sekhar Anagani <[email protected]>
Signed-off-by Bin Gao <[email protected]>
---
Changes in v4:
- various fixes to address Aaron's comments.
Changes in v3: none
Changes in v2: none
 drivers/acpi/pmic/intel_pmic.c | 74 ++++++++++++++++++++++++++++++++++++++++--
 drivers/acpi/pmic/intel_pmic.h |  5 +++
 2 files changed, 76 insertions(+), 3 deletions(-)
drivers/acpi/pmic/intel_pmic.c +0 -0
index 410e96f..e11d1e0 100644
@@ -21,12 +21,14 @@
 
drivers/acpi/pmic/intel_pmic.c line 31
Quoted 10 lines
drivers/acpi/pmic/intel_pmic.c +2 -0
 #define PMIC_POWER_OPREGION_ID               0x8d
 #define PMIC_THERMAL_OPREGION_ID     0x8c
+#define PMIC_REGS_OPREGION_ID                0x8f
 
 struct intel_pmic_opregion {
      struct mutex lock;
      struct acpi_lpat_conversion_table *lpat_table;
      struct regmap *regmap;
      struct intel_pmic_opregion_data *data;
+     struct pmic_gpio_ctx    ctx;
What gpio?
Quoted 7 lines
drivers/acpi/pmic/intel_pmic.c +0 -0
 };
 
 static int pmic_get_reg_bit(int address, struct pmic_table *table,
@@ -204,6 +206,56 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
      return AE_OK;
 }
 

Quoted 7 lines

drivers/acpi/pmic/intel_pmic.c static acpi_status intel_pmic_thermal_handler(u32 function, · line 211
Quoted 3 lines
drivers/acpi/pmic/intel_pmic.c +3 -0
+static acpi_status intel_pmic_gpio_handler(u32 function,
+     acpi_physical_address address, u32 bits, u64 *value64,
+             void *handler_context, void *region_context)
What the heck is this? Why is this suddenly a gpio handler?
This is handler for an operation region, not some gpio!

What about if I replaced this one with the original one from Felipe you sent?

Thanks,
Rafael

Heikki Krogerus

2016-06-27 14:37 UTC #3 lore raw

Quoted 56 lines · Rafael J. Wysocki

Quoted 54 lines · Heikki Krogerus
Whoa! Hold on!
Quoted 25 lines · Bin Gao
Broxton platform firmware has defined new customized operation regions called regs for PMIC chip - regs op region is used to handle the PMIC gpio mainly intended for the TYPE-C VBUS and Orientation.
The intel_gpio_ctx structure is created for the purpose of handling the PMIC gpio register read and write.
Signed-off-by Felipe Balbi <[email protected]>
Signed-off-by Chandra Sekhar Anagani <[email protected]>
Signed-off-by Bin Gao <[email protected]>
---
Changes in v4:
- various fixes to address Aaron's comments.
Changes in v3: none
Changes in v2: none
 drivers/acpi/pmic/intel_pmic.c | 74 ++++++++++++++++++++++++++++++++++++++++--
 drivers/acpi/pmic/intel_pmic.h |  5 +++
 2 files changed, 76 insertions(+), 3 deletions(-)
drivers/acpi/pmic/intel_pmic.c +0 -0
index 410e96f..e11d1e0 100644
@@ -21,12 +21,14 @@
 
drivers/acpi/pmic/intel_pmic.c line 31
Quoted 10 lines
drivers/acpi/pmic/intel_pmic.c +2 -0
 #define PMIC_POWER_OPREGION_ID               0x8d
 #define PMIC_THERMAL_OPREGION_ID     0x8c
+#define PMIC_REGS_OPREGION_ID                0x8f
 
 struct intel_pmic_opregion {
      struct mutex lock;
      struct acpi_lpat_conversion_table *lpat_table;
      struct regmap *regmap;
      struct intel_pmic_opregion_data *data;
+     struct pmic_gpio_ctx    ctx;
What gpio?
drivers/acpi/pmic/intel_pmic.c static acpi_status intel_pmic_thermal_handler(u32 function, · line 211
Quoted 10 lines
drivers/acpi/pmic/intel_pmic.c +3 -0
 };
 
 static int pmic_get_reg_bit(int address, struct pmic_table *table,
@@ -204,6 +206,56 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
      return AE_OK;
 }
 
+static acpi_status intel_pmic_gpio_handler(u32 function,
+     acpi_physical_address address, u32 bits, u64 *value64,
+             void *handler_context, void *region_context)
What the heck is this? Why is this suddenly a gpio handler?

Quoted 5 lines

Quoted 1 line
This is handler for an operation region, not some gpio!
OK
What about if I replaced this one with the original one from Felipe you sent?

That works for me.

Thanks,

Signature · 1 line

heikki

Rafael J. Wysocki

2016-06-27 20:24 UTC #4 lore raw

Quoted 57 lines · Heikki Krogerus

Quoted 56 lines · Rafael J. Wysocki
Quoted 54 lines · Heikki Krogerus
Whoa! Hold on!
Quoted 25 lines · Bin Gao
Broxton platform firmware has defined new customized operation regions called regs for PMIC chip - regs op region is used to handle the PMIC gpio mainly intended for the TYPE-C VBUS and Orientation.
The intel_gpio_ctx structure is created for the purpose of handling the PMIC gpio register read and write.
Signed-off-by Felipe Balbi <[email protected]>
Signed-off-by Chandra Sekhar Anagani <[email protected]>
Signed-off-by Bin Gao <[email protected]>
---
Changes in v4:
- various fixes to address Aaron's comments.
Changes in v3: none
Changes in v2: none
 drivers/acpi/pmic/intel_pmic.c | 74 ++++++++++++++++++++++++++++++++++++++++--
 drivers/acpi/pmic/intel_pmic.h |  5 +++
 2 files changed, 76 insertions(+), 3 deletions(-)
drivers/acpi/pmic/intel_pmic.c +0 -0
index 410e96f..e11d1e0 100644
@@ -21,12 +21,14 @@
 
drivers/acpi/pmic/intel_pmic.c line 31
Quoted 10 lines
drivers/acpi/pmic/intel_pmic.c +2 -0
 #define PMIC_POWER_OPREGION_ID               0x8d
 #define PMIC_THERMAL_OPREGION_ID     0x8c
+#define PMIC_REGS_OPREGION_ID                0x8f
 
 struct intel_pmic_opregion {
      struct mutex lock;
      struct acpi_lpat_conversion_table *lpat_table;
      struct regmap *regmap;
      struct intel_pmic_opregion_data *data;
+     struct pmic_gpio_ctx    ctx;
What gpio?
drivers/acpi/pmic/intel_pmic.c static acpi_status intel_pmic_thermal_handler(u32 function, · line 211
Quoted 10 lines
drivers/acpi/pmic/intel_pmic.c +3 -0
 };
 
 static int pmic_get_reg_bit(int address, struct pmic_table *table,
@@ -204,6 +206,56 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
      return AE_OK;
 }
 
+static acpi_status intel_pmic_gpio_handler(u32 function,
+     acpi_physical_address address, u32 bits, u64 *value64,
+             void *handler_context, void *region_context)
What the heck is this? Why is this suddenly a gpio handler?

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
This is handler for an operation region, not some gpio!
OK
What about if I replaced this one with the original one from Felipe you sent?
That works for me.

OK, let's do that, then.

Thanks,
Rafael

Gao, Bin

2016-06-27 21:25 UTC #5 lore raw

I'm fine with that. Thanks Rafael.

-Bin

-----Original Message-----
From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Monday, June 27, 2016 1:29 PM
To: Heikki Krogerus
Cc: Rafael J. Wysocki; Bin Gao; Lu, Aaron; Gortmaker, Paul (Wind River); ACPI Devel Maling List; Linux Kernel Mailing List; Iyer, Yegnesh S; David Rajamanickam, Ajay Thomas; Gao, Bin
Subject: Re: [PATCH v4 3/3] acpi/pmic: Add support for PMIC regs operation region

Quoted 59 lines · Heikki Krogerus

Quoted 58 lines · Rafael J. Wysocki
Quoted 56 lines · Heikki Krogerus
Whoa! Hold on!
Quoted 27 lines · Bin Gao
Broxton platform firmware has defined new customized operation regions called regs for PMIC chip - regs op region is used to handle the PMIC gpio mainly intended for the TYPE-C VBUS and Orientation.
The intel_gpio_ctx structure is created for the purpose of handling the PMIC gpio register read and write.
Signed-off-by Felipe Balbi <[email protected]>
Signed-off-by Chandra Sekhar Anagani
<[email protected]>
Signed-off-by Bin Gao <[email protected]>
---
Changes in v4:
- various fixes to address Aaron's comments.
Changes in v3: none
Changes in v2: none
 drivers/acpi/pmic/intel_pmic.c | 74 
++++++++++++++++++++++++++++++++++++++++--
 drivers/acpi/pmic/intel_pmic.h |  5 +++
 2 files changed, 76 insertions(+), 3 deletions(-)
(quoted diff) +0 -0
b/drivers/acpi/pmic/intel_pmic.c index 410e96f..e11d1e0 100644
drivers/acpi/pmic/intel_pmic.c +0 -0
@@ -21,12 +21,14 @@
 
drivers/acpi/pmic/intel_pmic.c line 31
Quoted 10 lines
drivers/acpi/pmic/intel_pmic.c +2 -0
 #define PMIC_POWER_OPREGION_ID               0x8d
 #define PMIC_THERMAL_OPREGION_ID     0x8c
+#define PMIC_REGS_OPREGION_ID                0x8f
 
 struct intel_pmic_opregion {
      struct mutex lock;
      struct acpi_lpat_conversion_table *lpat_table;
      struct regmap *regmap;
      struct intel_pmic_opregion_data *data;
+     struct pmic_gpio_ctx    ctx;
What gpio?
drivers/acpi/pmic/intel_pmic.c static acpi_status intel_pmic_thermal_handler(u32 function, · line 211
Quoted 10 lines
drivers/acpi/pmic/intel_pmic.c +0 -0
 };
 
 static int pmic_get_reg_bit(int address, struct pmic_table 
*table, @@ -204,6 +206,56 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
      return AE_OK;
}
drivers/acpi/pmic/intel_pmic.c +3 -0
+static acpi_status intel_pmic_gpio_handler(u32 function,
+     acpi_physical_address address, u32 bits, u64 *value64,
+             void *handler_context, void *region_context)
What the heck is this? Why is this suddenly a gpio handler?

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
This is handler for an operation region, not some gpio!
OK
What about if I replaced this one with the original one from Felipe you sent?
That works for me.

OK, let's do that, then.

Thanks,
Rafael