ACPI / PMIC: Add partial support for Cherry Trail Crystal Cove PMIC

For some model PMIC's used on Intel boards we do not know how to handle the power or thermal opregions because we have no documentation.

For example in the intel_pmic_chtwc.c driver thermal_table_count is 0, which means that our PMIC_THERMAL_OPREGION_ID handler will always fail with AE_BAD_PARAMETER, in this case it is better to simply not register the handler at all.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/acpi/pmic/intel_pmic.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

drivers/acpi/pmic/intel_pmic.c +13 -7

index 452041398b34..a371f273f99d 100644
@@ -252,7 +252,7 @@ int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
 					struct regmap *regmap,
 					struct intel_pmic_opregion_data *d)
 {
-	acpi_status status;
+	acpi_status status = AE_OK;
 	struct intel_pmic_opregion *opregion;
 	int ret;
 

@@ -270,7 +270,8 @@ int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
 	opregion->regmap = regmap;
 	opregion->lpat_table = acpi_lpat_get_conversion_table(handle);
 
-	status = acpi_install_address_space_handler(handle,
+	if (d->power_table_count)
+		status = acpi_install_address_space_handler(handle,
 						    PMIC_POWER_OPREGION_ID,
 						    intel_pmic_power_handler,
 						    NULL, opregion);

@@ -279,7 +280,8 @@ int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
 		goto out_error;
 	}
 
-	status = acpi_install_address_space_handler(handle,
+	if (d->thermal_table_count)
+		status = acpi_install_address_space_handler(handle,
 						    PMIC_THERMAL_OPREGION_ID,
 						    intel_pmic_thermal_handler,
 						    NULL, opregion);

@@ -301,12 +303,16 @@ int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
 	return 0;
 
 out_remove_thermal_handler:
-	acpi_remove_address_space_handler(handle, PMIC_THERMAL_OPREGION_ID,
-					  intel_pmic_thermal_handler);
+	if (d->thermal_table_count)
+		acpi_remove_address_space_handler(handle,
+						  PMIC_THERMAL_OPREGION_ID,
+						  intel_pmic_thermal_handler);
 
 out_remove_power_handler:
-	acpi_remove_address_space_handler(handle, PMIC_POWER_OPREGION_ID,
-					  intel_pmic_power_handler);
+	if (d->power_table_count)
+		acpi_remove_address_space_handler(handle,
+						  PMIC_POWER_OPREGION_ID,
+						  intel_pmic_power_handler);
 
 out_error:
 	acpi_lpat_free_conversion_table(opregion->lpat_table);

Signature · 1 line

2.23.0

Our current Crystal Cove OpRegion driver is only valid for the Crystal Cove PMIC variant found on Bay Trail (BYT) boards, Cherry Trail (CHT) based boards use another variant.

At least the regulator registers are different on CHT and these registers are one of the things controlled by the custom PMIC OpRegion.

Commit 4d9ed62ab142 ("mfd: intel_soc_pmic: Export separate mfd-cell configs for BYT and CHT") has disabled the intel_pmic_crc.c code for CHT devices by removing the "crystal_cove_pmic" MFD cell on CHT devices.

This commit renames the intel_pmic_crc.c driver and the cell to be prefixed with "byt" to indicate that this code is for BYT devices only.

This is a preparation patch for adding a separate PMIC OpRegion driver for the CHT variant of the Crystal Cove PMIC (sometimes called Crystal Cove Plus in Android kernel sources).

 .../acpi/pmic/{intel_pmic_crc.c => intel_pmic_bytcrc.c}    | 4 ++--
 drivers/mfd/intel_soc_pmic_crc.c                           | 2 +-

Quoted 14 lines · Hans de Goede

Our current Crystal Cove OpRegion driver is only valid for the Crystal Cove PMIC variant found on Bay Trail (BYT) boards, Cherry Trail (CHT) based boards use another variant.

At least the regulator registers are different on CHT and these registers are one of the things controlled by the custom PMIC OpRegion.

Commit 4d9ed62ab142 ("mfd: intel_soc_pmic: Export separate mfd-cell configs for BYT and CHT") has disabled the intel_pmic_crc.c code for CHT devices by removing the "crystal_cove_pmic" MFD cell on CHT devices.

This commit renames the intel_pmic_crc.c driver and the cell to be prefixed with "byt" to indicate that this code is for BYT devices only.

Quoted 6 lines

This is a preparation patch for adding a separate PMIC OpRegion driver for the CHT variant of the Crystal Cove PMIC (sometimes called Crystal Cove Plus in Android kernel sources).

 .../acpi/pmic/{intel_pmic_crc.c => intel_pmic_bytcrc.c}    | 4 ++--
 drivers/mfd/intel_soc_pmic_crc.c                           | 2 +-

I would go with previously established pattern, i.e. intel_pmic_bytcc.c.

drivers/mfd/intel_soc_pmic_crc.c static struct mfd_cell crystal_cove_byt_dev[] = { · line 81

Quoted 11 lines

+++ b/drivers/mfd/intel_soc_pmic_crc.c

drivers/mfd/intel_soc_pmic_crc.c +1 -1

@@ -75,7 +75,7 @@ static struct mfd_cell crystal_cove_byt_dev[] = {
 		.resources = gpio_resources,
 	},
 	{
-		.name = "crystal_cove_pmic",
+		.name = "byt_crystal_cove_pmic",
 	},
 	{
 		.name = "crystal_cove_pwm",

I'm wondering shouldn't we rename the PWM and GPIO for the sake of consistency? Yes, if a driver is used on both CHT and BYT, let it provide two names.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 14 lines · Hans de Goede

Our current Crystal Cove OpRegion driver is only valid for the Crystal Cove PMIC variant found on Bay Trail (BYT) boards, Cherry Trail (CHT) based boards use another variant.

At least the regulator registers are different on CHT and these registers are one of the things controlled by the custom PMIC OpRegion.

Commit 4d9ed62ab142 ("mfd: intel_soc_pmic: Export separate mfd-cell configs for BYT and CHT") has disabled the intel_pmic_crc.c code for CHT devices by removing the "crystal_cove_pmic" MFD cell on CHT devices.

This commit renames the intel_pmic_crc.c driver and the cell to be prefixed with "byt" to indicate that this code is for BYT devices only.

Quoted 6 lines

This is a preparation patch for adding a separate PMIC OpRegion driver for the CHT variant of the Crystal Cove PMIC (sometimes called Crystal Cove Plus in Android kernel sources).

  .../acpi/pmic/{intel_pmic_crc.c => intel_pmic_bytcrc.c}    | 4 ++--
  drivers/mfd/intel_soc_pmic_crc.c                           | 2 +-

I would go with previously established pattern, i.e. intel_pmic_bytcc.c.

Quoted 5 lines

+++ b/drivers/mfd/intel_soc_pmic_crc.c

drivers/mfd/intel_soc_pmic_crc.c +0 -0

@@ -75,7 +75,7 @@ static struct mfd_cell crystal_cove_byt_dev[] = {
  		.resources = gpio_resources,
  	},
  	{

drivers/mfd/intel_soc_pmic_crc.c static struct mfd_cell crystal_cove_byt_dev[] = { · line 81

Quoted 5 lines

drivers/mfd/intel_soc_pmic_crc.c +1 -1

-		.name = "crystal_cove_pmic",
+		.name = "byt_crystal_cove_pmic",
  	},
  	{
  		.name = "crystal_cove_pwm",

I'm wondering shouldn't we rename the PWM and GPIO for the sake of consistency? Yes, if a driver is used on both CHT and BYT, let it provide two names.

Quoted 3 lines · Andy Shevchenko

On Thu, Oct 24, 2019 at 11:38:25PM +0200, Hans de Goede wrote:

I would go with previously established pattern, i.e. intel_pmic_bytcc.c.

Well that would be consistent with the chtwc for the Whiskey Cove, but Crystal Cove related files are shortened to crc in many places already:

Filenames before this patch:
drivers/acpi/pmic/intel_pmic_crc.c
drivers/pwm/pwm-crc.c
drivers/mfd/intel_soc_pmic_crc.c

And to me "cc" stands for the Type-C cc lines, or for Cc: from email, so IMHO it is best to stick with crc here.

Quoted 2 lines

I'm wondering shouldn't we rename the PWM and GPIO for the sake of consistency? Yes, if a driver is used on both CHT and BYT, let it provide two names.

I believe it is fine to keep the blocks which are identical between the 2 versions as just "crystal_cove_foo", but renaming them is fine with me too, but that follows outside the scope of this series and should be done in a follow-up series IMHO.

Quoted 13 lines · Hans de Goede

Quoted 3 lines · Andy Shevchenko

On Thu, Oct 24, 2019 at 11:38:25PM +0200, Hans de Goede wrote:

I would go with previously established pattern, i.e. intel_pmic_bytcc.c.

Well that would be consistent with the chtwc for the Whiskey Cove, but Crystal Cove related files are shortened to crc in many places already:

Filenames before this patch:
drivers/acpi/pmic/intel_pmic_crc.c
drivers/pwm/pwm-crc.c
drivers/mfd/intel_soc_pmic_crc.c

Quoted 2 lines

And to me "cc" stands for the Type-C cc lines, or for Cc: from email, so IMHO it is best to stick with crc here.

Okay, let's do an exception here due to the fact the code and name already exists and spreads enough thru sources.

It means you may use mine tags.

We have no docs for the CHT Crystal Cove PMIC. The Asus Zenfone-2 kernel code has 2 Crystal Cove regulator drivers, one calls the PMIC a "Crystal Cove Plus" PMIC and talks about Cherry Trail, so presuambly that one could be used to get register info for the regulators if we need to implement regulator support in the future.

For now the sole purpose of this driver is to make intel_soc_pmic_exec_mipi_pmic_seq_element work on devices with a CHT Crystal Cove PMIC.

Specifically this fixes the following MIPI PMIC sequence related errors on e.g. an Asus T100HA:

[  178.211801] intel_soc_pmic_exec_mipi_pmic_seq_element: No PMIC registered
[  178.211897] [drm:intel_dsi_dcs_init_backlight_funcs [i915]] *ERROR* mipi_exec_pmic failed, error: -6

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/acpi/Kconfig                  |  7 +++++
 drivers/acpi/Makefile                 |  1 +
 drivers/acpi/pmic/intel_pmic_chtcrc.c | 44 +++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)
 create mode 100644 drivers/acpi/pmic/intel_pmic_chtcrc.c

drivers/acpi/Kconfig +7 -0

index 089f7f8e1be7..0f23d8b22c42 100644
@@ -520,6 +520,13 @@ config BYTCRC_PMIC_OPREGION
 	  This config adds ACPI operation region support for the Bay Trail
 	  version of the Crystal Cove PMIC.
 
+config CHTCRC_PMIC_OPREGION
+	bool "ACPI operation region support for Cherry Trail Crystal Cove PMIC"
+	depends on INTEL_SOC_PMIC
+	help
+	  This config adds ACPI operation region support for the Cherry Trail
+	  version of the Crystal Cove PMIC.
+
 config XPOWER_PMIC_OPREGION
 	bool "ACPI operation region support for XPower AXP288 PMIC"
 	depends on MFD_AXP20X_I2C && IOSF_MBI=y

drivers/acpi/Makefile +1 -0

index ee59b1db69a1..68853f23e901 100644
@@ -110,6 +110,7 @@ obj-$(CONFIG_ACPI_EXTLOG)	+= acpi_extlog.o
 
 obj-$(CONFIG_PMIC_OPREGION)	+= pmic/intel_pmic.o
 obj-$(CONFIG_BYTCRC_PMIC_OPREGION) += pmic/intel_pmic_bytcrc.o
+obj-$(CONFIG_CHTCRC_PMIC_OPREGION) += pmic/intel_pmic_chtcrc.o
 obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
 obj-$(CONFIG_BXT_WC_PMIC_OPREGION) += pmic/intel_pmic_bxtwc.o
 obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o

drivers/acpi/pmic/intel_pmic_chtcrc.c new file +44 -0

new file mode 100644
index 000000000000..ebf8d3187df1
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Cherry Trail Crystal Cove PMIC operation region driver
+ *
+ * Copyright (C) 2019 Hans de Goede <[email protected]>
+ */
+
+#include <linux/acpi.h>
+#include <linux/init.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include "intel_pmic.h"
+
+/*
+ * We have no docs for the CHT Crystal Cove PMIC. The Asus Zenfone-2 kernel
+ * code has 2 Crystal Cove regulator drivers, one calls the PMIC a "Crystal
+ * Cove Plus" PMIC and talks about Cherry Trail, so presuambly that one
+ * could be used to get register info for the regulators if we need to
+ * implement regulator support in the future.
+ *
+ * For now the sole purpose of this driver is to make
+ * intel_soc_pmic_exec_mipi_pmic_seq_element work on devices with a
+ * CHT Crystal Cove PMIC.
+ */
+static struct intel_pmic_opregion_data intel_chtcrc_pmic_opregion_data = {
+	.pmic_i2c_address = 0x6e,
+};
+
+static int intel_chtcrc_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_chtcrc_pmic_opregion_data);
+}
+
+static struct platform_driver intel_chtcrc_pmic_opregion_driver = {
+	.probe = intel_chtcrc_pmic_opregion_probe,
+	.driver = {
+		.name = "cht_crystal_cove_pmic",
+	},
+};
+builtin_platform_driver(intel_chtcrc_pmic_opregion_driver);

Signature · 1 line

2.23.0

Quoted 13 lines · Hans de Goede

We have no docs for the CHT Crystal Cove PMIC. The Asus Zenfone-2 kernel code has 2 Crystal Cove regulator drivers, one calls the PMIC a "Crystal Cove Plus" PMIC and talks about Cherry Trail, so presuambly that one could be used to get register info for the regulators if we need to implement regulator support in the future.

For now the sole purpose of this driver is to make intel_soc_pmic_exec_mipi_pmic_seq_element work on devices with a CHT Crystal Cove PMIC.

Specifically this fixes the following MIPI PMIC sequence related errors on e.g. an Asus T100HA:

Quoted 3 lines

[  178.211801] intel_soc_pmic_exec_mipi_pmic_seq_element: No PMIC registered
[  178.211897] [drm:intel_dsi_dcs_init_backlight_funcs [i915]] *ERROR* mipi_exec_pmic failed, error: -6

Reviewed-by Andy Shevchenko <[email protected]>

as long as name pattern uses "chtcc".

Quoted 93 lines

Signed-off-by Hans de Goede <[email protected]>

---
  drivers/acpi/Kconfig                  |  7 +++++
  drivers/acpi/Makefile                 |  1 +
  drivers/acpi/pmic/intel_pmic_chtcrc.c | 44 +++++++++++++++++++++++++++
  3 files changed, 52 insertions(+)

create mode 100644 drivers/acpi/pmic/intel_pmic_chtcrc.c

drivers/acpi/Kconfig +7 -0

index 089f7f8e1be7..0f23d8b22c42 100644
@@ -520,6 +520,13 @@ config BYTCRC_PMIC_OPREGION
  	  This config adds ACPI operation region support for the Bay Trail
  	  version of the Crystal Cove PMIC.
 
+config CHTCRC_PMIC_OPREGION
+	bool "ACPI operation region support for Cherry Trail Crystal Cove PMIC"
+	depends on INTEL_SOC_PMIC
+	help
+	  This config adds ACPI operation region support for the Cherry Trail
+	  version of the Crystal Cove PMIC.
+
  config XPOWER_PMIC_OPREGION
  	bool "ACPI operation region support for XPower AXP288 PMIC"
  	depends on MFD_AXP20X_I2C && IOSF_MBI=y

drivers/acpi/Makefile +1 -0

index ee59b1db69a1..68853f23e901 100644
@@ -110,6 +110,7 @@ obj-$(CONFIG_ACPI_EXTLOG)	+= acpi_extlog.o
 
  obj-$(CONFIG_PMIC_OPREGION)	+= pmic/intel_pmic.o
  obj-$(CONFIG_BYTCRC_PMIC_OPREGION) += pmic/intel_pmic_bytcrc.o
+obj-$(CONFIG_CHTCRC_PMIC_OPREGION) += pmic/intel_pmic_chtcrc.o
  obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
  obj-$(CONFIG_BXT_WC_PMIC_OPREGION) += pmic/intel_pmic_bxtwc.o
  obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o

drivers/acpi/pmic/intel_pmic_chtcrc.c new file +44 -0

new file mode 100644
index 000000000000..ebf8d3187df1
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Cherry Trail Crystal Cove PMIC operation region driver
+ *
+ * Copyright (C) 2019 Hans de Goede <[email protected]>
+ */
+
+#include <linux/acpi.h>
+#include <linux/init.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include "intel_pmic.h"
+
+/*
+ * We have no docs for the CHT Crystal Cove PMIC. The Asus Zenfone-2 kernel
+ * code has 2 Crystal Cove regulator drivers, one calls the PMIC a "Crystal
+ * Cove Plus" PMIC and talks about Cherry Trail, so presuambly that one
+ * could be used to get register info for the regulators if we need to
+ * implement regulator support in the future.
+ *
+ * For now the sole purpose of this driver is to make
+ * intel_soc_pmic_exec_mipi_pmic_seq_element work on devices with a
+ * CHT Crystal Cove PMIC.
+ */
+static struct intel_pmic_opregion_data intel_chtcrc_pmic_opregion_data = {
+	.pmic_i2c_address = 0x6e,
+};
+
+static int intel_chtcrc_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_chtcrc_pmic_opregion_data);
+}
+
+static struct platform_driver intel_chtcrc_pmic_opregion_driver = {
+	.probe = intel_chtcrc_pmic_opregion_probe,
+	.driver = {
+		.name = "cht_crystal_cove_pmic",
+	},
+};
+builtin_platform_driver(intel_chtcrc_pmic_opregion_driver);

Signature · 1 line

2.23.0

Quoted 5 lines · Andy Shevchenko

On Thu, Oct 24, 2019 at 11:38:26PM +0200, Hans de Goede wrote:

Reviewed-by Andy Shevchenko <[email protected]>

as long as name pattern uses "chtcc".

As I already replied to your other similar remark, I would really like to stick with crc, crc to me means either crystal-cove or well a crc, cc triggers different associations in my mind. Also other crystal-cove files also use crc in their filename.

Add a "cht_crystal_cove_pmic" cell to the cells for the Cherry Trail variant of the Crystal Cove PMIC. Adding this cell enables / hooks-up the new Cherry Trail Crystal Cove PMIC OpRegion driver.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/mfd/intel_soc_pmic_crc.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/mfd/intel_soc_pmic_crc.c +3 -0

index ab09b8225b76..429efa1f8e55 100644
@@ -88,6 +88,9 @@ static struct mfd_cell crystal_cove_cht_dev[] = {
 		.num_resources = ARRAY_SIZE(gpio_resources),
 		.resources = gpio_resources,
 	},
+	{
+		.name = "cht_crystal_cove_pmic",
+	},
 	{
 		.name = "crystal_cove_pwm",
 	},

Signature · 1 line

2.23.0

Add a "cht_crystal_cove_pmic" cell to the cells for the Cherry Trail variant of the Crystal Cove PMIC. Adding this cell enables / hooks-up the new Cherry Trail Crystal Cove PMIC OpRegion driver.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/mfd/intel_soc_pmic_crc.c | 3 +++
 1 file changed, 3 insertions(+)

Quoted 4 lines · Hans de Goede

Add a "cht_crystal_cove_pmic" cell to the cells for the Cherry Trail variant of the Crystal Cove PMIC. Adding this cell enables / hooks-up the new Cherry Trail Crystal Cove PMIC OpRegion driver.

Quoted 4 lines

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/mfd/intel_soc_pmic_crc.c | 3 +++
 1 file changed, 3 insertions(+)

Applied, thanks.