[PATCH v4 0/2] ACPI / PMIC: Intel CHT Whiskey Cove opregion driver + xpower opregion bugfix

[PATCH v4 0/2] ACPI / PMIC: Intel CHT Whiskey Cove opregion driver + xpower opregion bugfix

[Hans de Goede]

Hi All,

These 2 seem to have fallen through the cracks, hence this resend,
note the 1st patch has a minor change compared to the last time
it was send, its Kconfig option has been changed from a tristate
to a bool.

Regards,

Hans

p.s.

I'm going on vacation / offline starting tomorrow I will be back
May 1st, so if I'm responding to email that is why.

[PATCH v4 1/2] ACPI / PMIC: Add opregion driver for Intel CHT Whiskey Cove PMIC

[Hans de Goede]

Add opregion driver for Intel CHT Whiskey Cove PMIC, based on various
non upstreamed CHT Whiskey Cove PMIC patches. This does not include
support for the Thermal opregion (DPTF) due to lacking documentation.

Cc: Bin Gao <bin.gao@intel.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Changes in v2:
-s/WhiskeyCove/Whiskey Cove/
-Some minor style tweaks
-Allow building as module
Changes in v3:
-Drop filename from copyright header
-Simplify intel_cht_wc_pmic_update_power to 1 line
-Move defines of regulator register addresses to intel_pmic_chtwc.c,
 it is the only place where they are used
Changes in v4:
-Make the config option a bool, ACPI OPRegion handlers must be available
 before other drivers using them are loaded, which can only be ensured if
 both the mfd and opregion drivers are built in.
---
 drivers/acpi/Kconfig                 |   6 +
 drivers/acpi/Makefile                |   1 +
 drivers/acpi/pmic/intel_pmic_chtwc.c | 280 +++++++++++++++++++++++++++++++++++
 3 files changed, 287 insertions(+)
 create mode 100644 drivers/acpi/pmic/intel_pmic_chtwc.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 83e5f7e..4f12fe0 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -516,6 +516,12 @@ config BXT_WC_PMIC_OPREGION
 	help
 	  This config adds ACPI operation region support for BXT WhiskeyCove PMIC.
 
+config CHT_WC_PMIC_OPREGION
+	bool "ACPI operation region support for CHT Whiskey Cove PMIC"
+	depends on INTEL_SOC_PMIC_CHTWC
+	help
+	  This config adds ACPI operation region support for CHT Whiskey Cove PMIC.
+
 endif
 
 config ACPI_CONFIGFS
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index d94f92f..d78065c 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -101,6 +101,7 @@ 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
 obj-$(CONFIG_BXT_WC_PMIC_OPREGION) += pmic/intel_pmic_bxtwc.o
+obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o
 
 obj-$(CONFIG_ACPI_CONFIGFS)	+= acpi_configfs.o
 
diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
new file mode 100644
index 0000000..85636d7
--- /dev/null
+++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
@@ -0,0 +1,280 @@
+/*
+ * Intel CHT Whiskey Cove PMIC operation region driver
+ * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
+ *
+ * Based on various non upstream patches to support the CHT Whiskey Cove PMIC:
+ * Copyright (C) 2013-2015 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/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"
+
+#define CHT_WC_V1P05A_CTRL		0x6e3b
+#define CHT_WC_V1P15_CTRL		0x6e3c
+#define CHT_WC_V1P05A_VSEL		0x6e3d
+#define CHT_WC_V1P15_VSEL		0x6e3e
+#define CHT_WC_V1P8A_CTRL		0x6e56
+#define CHT_WC_V1P8SX_CTRL		0x6e57
+#define CHT_WC_VDDQ_CTRL		0x6e58
+#define CHT_WC_V1P2A_CTRL		0x6e59
+#define CHT_WC_V1P2SX_CTRL		0x6e5a
+#define CHT_WC_V1P8A_VSEL		0x6e5b
+#define CHT_WC_VDDQ_VSEL		0x6e5c
+#define CHT_WC_V2P8SX_CTRL		0x6e5d
+#define CHT_WC_V3P3A_CTRL		0x6e5e
+#define CHT_WC_V3P3SD_CTRL		0x6e5f
+#define CHT_WC_VSDIO_CTRL		0x6e67
+#define CHT_WC_V3P3A_VSEL		0x6e68
+#define CHT_WC_VPROG1A_CTRL		0x6e90
+#define CHT_WC_VPROG1B_CTRL		0x6e91
+#define CHT_WC_VPROG1F_CTRL		0x6e95
+#define CHT_WC_VPROG2D_CTRL		0x6e99
+#define CHT_WC_VPROG3A_CTRL		0x6e9a
+#define CHT_WC_VPROG3B_CTRL		0x6e9b
+#define CHT_WC_VPROG4A_CTRL		0x6e9c
+#define CHT_WC_VPROG4B_CTRL		0x6e9d
+#define CHT_WC_VPROG4C_CTRL		0x6e9e
+#define CHT_WC_VPROG4D_CTRL		0x6e9f
+#define CHT_WC_VPROG5A_CTRL		0x6ea0
+#define CHT_WC_VPROG5B_CTRL		0x6ea1
+#define CHT_WC_VPROG6A_CTRL		0x6ea2
+#define CHT_WC_VPROG6B_CTRL		0x6ea3
+#define CHT_WC_VPROG1A_VSEL		0x6ec0
+#define CHT_WC_VPROG1B_VSEL		0x6ec1
+#define CHT_WC_V1P8SX_VSEL		0x6ec2
+#define CHT_WC_V1P2SX_VSEL		0x6ec3
+#define CHT_WC_V1P2A_VSEL		0x6ec4
+#define CHT_WC_VPROG1F_VSEL		0x6ec5
+#define CHT_WC_VSDIO_VSEL		0x6ec6
+#define CHT_WC_V2P8SX_VSEL		0x6ec7
+#define CHT_WC_V3P3SD_VSEL		0x6ec8
+#define CHT_WC_VPROG2D_VSEL		0x6ec9
+#define CHT_WC_VPROG3A_VSEL		0x6eca
+#define CHT_WC_VPROG3B_VSEL		0x6ecb
+#define CHT_WC_VPROG4A_VSEL		0x6ecc
+#define CHT_WC_VPROG4B_VSEL		0x6ecd
+#define CHT_WC_VPROG4C_VSEL		0x6ece
+#define CHT_WC_VPROG4D_VSEL		0x6ecf
+#define CHT_WC_VPROG5A_VSEL		0x6ed0
+#define CHT_WC_VPROG5B_VSEL		0x6ed1
+#define CHT_WC_VPROG6A_VSEL		0x6ed2
+#define CHT_WC_VPROG6B_VSEL		0x6ed3
+
+/*
+ * Regulator support is based on the non upstream patch:
+ * "regulator: whiskey_cove: implements Whiskey Cove pmic VRF support"
+ * https://github.com/intel-aero/meta-intel-aero/blob/master/recipes-kernel/linux/linux-yocto/0019-regulator-whiskey_cove-implements-WhiskeyCove-pmic-V.patch
+ */
+static struct pmic_table power_table[] = {
+	{
+		.address = 0x0,
+		.reg = CHT_WC_V1P8A_CTRL,
+		.bit = 0x01,
+	}, /* V18A */
+	{
+		.address = 0x04,
+		.reg = CHT_WC_V1P8SX_CTRL,
+		.bit = 0x07,
+	}, /* V18X */
+	{
+		.address = 0x08,
+		.reg = CHT_WC_VDDQ_CTRL,
+		.bit = 0x01,
+	}, /* VDDQ */
+	{
+		.address = 0x0c,
+		.reg = CHT_WC_V1P2A_CTRL,
+		.bit = 0x07,
+	}, /* V12A */
+	{
+		.address = 0x10,
+		.reg = CHT_WC_V1P2SX_CTRL,
+		.bit = 0x07,
+	}, /* V12X */
+	{
+		.address = 0x14,
+		.reg = CHT_WC_V2P8SX_CTRL,
+		.bit = 0x07,
+	}, /* V28X */
+	{
+		.address = 0x18,
+		.reg = CHT_WC_V3P3A_CTRL,
+		.bit = 0x01,
+	}, /* V33A */
+	{
+		.address = 0x1c,
+		.reg = CHT_WC_V3P3SD_CTRL,
+		.bit = 0x07,
+	}, /* V3SD */
+	{
+		.address = 0x20,
+		.reg = CHT_WC_VSDIO_CTRL,
+		.bit = 0x07,
+	}, /* VSD */
+/*	{
+		.address = 0x24,
+		.reg = ??,
+		.bit = ??,
+	}, ** VSW2 */
+/*	{
+		.address = 0x28,
+		.reg = ??,
+		.bit = ??,
+	}, ** VSW1 */
+/*	{
+		.address = 0x2c,
+		.reg = ??,
+		.bit = ??,
+	}, ** VUPY */
+/*	{
+		.address = 0x30,
+		.reg = ??,
+		.bit = ??,
+	}, ** VRSO */
+	{
+		.address = 0x34,
+		.reg = CHT_WC_VPROG1A_CTRL,
+		.bit = 0x07,
+	}, /* VP1A */
+	{
+		.address = 0x38,
+		.reg = CHT_WC_VPROG1B_CTRL,
+		.bit = 0x07,
+	}, /* VP1B */
+	{
+		.address = 0x3c,
+		.reg = CHT_WC_VPROG1F_CTRL,
+		.bit = 0x07,
+	}, /* VP1F */
+	{
+		.address = 0x40,
+		.reg = CHT_WC_VPROG2D_CTRL,
+		.bit = 0x07,
+	}, /* VP2D */
+	{
+		.address = 0x44,
+		.reg = CHT_WC_VPROG3A_CTRL,
+		.bit = 0x07,
+	}, /* VP3A */
+	{
+		.address = 0x48,
+		.reg = CHT_WC_VPROG3B_CTRL,
+		.bit = 0x07,
+	}, /* VP3B */
+	{
+		.address = 0x4c,
+		.reg = CHT_WC_VPROG4A_CTRL,
+		.bit = 0x07,
+	}, /* VP4A */
+	{
+		.address = 0x50,
+		.reg = CHT_WC_VPROG4B_CTRL,
+		.bit = 0x07,
+	}, /* VP4B */
+	{
+		.address = 0x54,
+		.reg = CHT_WC_VPROG4C_CTRL,
+		.bit = 0x07,
+	}, /* VP4C */
+	{
+		.address = 0x58,
+		.reg = CHT_WC_VPROG4D_CTRL,
+		.bit = 0x07,
+	}, /* VP4D */
+	{
+		.address = 0x5c,
+		.reg = CHT_WC_VPROG5A_CTRL,
+		.bit = 0x07,
+	}, /* VP5A */
+	{
+		.address = 0x60,
+		.reg = CHT_WC_VPROG5B_CTRL,
+		.bit = 0x07,
+	}, /* VP5B */
+	{
+		.address = 0x64,
+		.reg = CHT_WC_VPROG6A_CTRL,
+		.bit = 0x07,
+	}, /* VP6A */
+	{
+		.address = 0x68,
+		.reg = CHT_WC_VPROG6B_CTRL,
+		.bit = 0x07,
+	}, /* VP6B */
+/*	{
+		.address = 0x6c,
+		.reg = ??,
+		.bit = ??,
+	}  ** VP7A */
+};
+
+static int intel_cht_wc_pmic_get_power(struct regmap *regmap, int reg,
+		int bit, u64 *value)
+{
+	int data;
+
+	if (regmap_read(regmap, reg, &data))
+		return -EIO;
+
+	*value = (data & bit) ? 1 : 0;
+	return 0;
+}
+
+static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg,
+		int bitmask, bool on)
+{
+	return regmap_update_bits(regmap, reg, bitmask, on ? 1 : 0);
+}
+
+/*
+ * The thermal table and ops are empty, we do not support the Thermal opregion
+ * (DPTF) due to lacking documentation.
+ */
+static struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
+	.get_power		= intel_cht_wc_pmic_get_power,
+	.update_power		= intel_cht_wc_pmic_update_power,
+	.power_table		= power_table,
+	.power_table_count	= ARRAY_SIZE(power_table),
+};
+
+static int intel_cht_wc_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_cht_wc_pmic_opregion_data);
+}
+
+static struct platform_device_id cht_wc_opregion_id_table[] = {
+	{ .name = "cht_wcove_region" },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, cht_wc_opregion_id_table);
+
+static struct platform_driver intel_cht_wc_pmic_opregion_driver = {
+	.probe = intel_cht_wc_pmic_opregion_probe,
+	.driver = {
+		.name = "cht_whiskey_cove_pmic",
+	},
+	.id_table = cht_wc_opregion_id_table,
+};
+module_platform_driver(intel_cht_wc_pmic_opregion_driver);
+
+MODULE_DESCRIPTION("Intel CHT Whiskey Cove PMIC operation region driver");
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
+MODULE_LICENSE("GPL");
-- 
2.9.3

[PATCH v4 2/2] ACPI / PMIC: Stop xpower OPRegion handler relying on IIO

[Hans de Goede]

The intel_pmic_xpower code provides an OPRegion handler, which must be
available before other drivers using it are loaded, which can only be
ensured if both the mfd and opregion drivers are built in, which is why
the Kconfig option for intel_pmic_xpower is a bool.

The use of IIO is causing trouble for generic distro configs here as
distros will typically want to build IIO drivers as modules and there
really is no reason to use IIO here. The reading of the ADC value is a
single regmap_bulk_read, which is already protected against races by
the regmap-lock.

This commit removes the use of IIO, allowing distros to enable the
driver without needing to built IIO in and also actually simplifies
the code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/Kconfig                  |  2 +-
 drivers/acpi/pmic/intel_pmic_xpower.c | 21 ++++-----------------
 2 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 4f12fe0..842530f 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -506,7 +506,7 @@ config CRC_PMIC_OPREGION
 
 config XPOWER_PMIC_OPREGION
 	bool "ACPI operation region support for XPower AXP288 PMIC"
-	depends on AXP288_ADC = y
+	depends on MFD_AXP20X_I2C
 	help
 	  This config adds ACPI operation region support for XPower AXP288 PMIC.
 
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index e6e991a..55f5111 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -18,7 +18,6 @@
 #include <linux/mfd/axp20x.h>
 #include <linux/regmap.h>
 #include <linux/platform_device.h>
-#include <linux/iio/consumer.h>
 #include "intel_pmic.h"
 
 #define XPOWER_GPADC_LOW	0x5b
@@ -186,28 +185,16 @@ static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
  * @regmap: regmap of the PMIC device
  * @reg: register to get the reading
  *
- * We could get the sensor value by manipulating the HW regs here, but since
- * the axp288 IIO driver may also access the same regs at the same time, the
- * APIs provided by IIO subsystem are used here instead to avoid problems. As
- * a result, the two passed in params are of no actual use.
- *
  * Return a positive value on success, errno on failure.
  */
 static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
 {
-	struct iio_channel *gpadc_chan;
-	int ret, val;
-
-	gpadc_chan = iio_channel_get(NULL, "axp288-system-temp");
-	if (IS_ERR_OR_NULL(gpadc_chan))
-		return -EACCES;
+	u8 buf[2];
 
-	ret = iio_read_channel_raw(gpadc_chan, &val);
-	if (ret < 0)
-		val = ret;
+	if (regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2))
+		return -EIO;
 
-	iio_channel_release(gpadc_chan);
-	return val;
+	return (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
 }
 
 static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
-- 
2.9.3

Re: [PATCH v4 1/2] ACPI / PMIC: Add opregion driver for Intel CHT Whiskey Cove PMIC

[Rafael J. Wysocki]

On Wed, Apr 19, 2017 at 3:06 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Add opregion driver for Intel CHT Whiskey Cove PMIC, based on various
> non upstreamed CHT Whiskey Cove PMIC patches. This does not include
> support for the Thermal opregion (DPTF) due to lacking documentation.
>
> Cc: Bin Gao <bin.gao@intel.com>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Felipe, Srinivas, any concerns here?

> ---
> Changes in v2:
> -s/WhiskeyCove/Whiskey Cove/
> -Some minor style tweaks
> -Allow building as module
> Changes in v3:
> -Drop filename from copyright header
> -Simplify intel_cht_wc_pmic_update_power to 1 line
> -Move defines of regulator register addresses to intel_pmic_chtwc.c,
>  it is the only place where they are used
> Changes in v4:
> -Make the config option a bool, ACPI OPRegion handlers must be available
>  before other drivers using them are loaded, which can only be ensured if
>  both the mfd and opregion drivers are built in.
> ---
>  drivers/acpi/Kconfig                 |   6 +
>  drivers/acpi/Makefile                |   1 +
>  drivers/acpi/pmic/intel_pmic_chtwc.c | 280 +++++++++++++++++++++++++++++++++++
>  3 files changed, 287 insertions(+)
>  create mode 100644 drivers/acpi/pmic/intel_pmic_chtwc.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 83e5f7e..4f12fe0 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -516,6 +516,12 @@ config BXT_WC_PMIC_OPREGION
>         help
>           This config adds ACPI operation region support for BXT WhiskeyCove PMIC.
>
> +config CHT_WC_PMIC_OPREGION
> +       bool "ACPI operation region support for CHT Whiskey Cove PMIC"
> +       depends on INTEL_SOC_PMIC_CHTWC
> +       help
> +         This config adds ACPI operation region support for CHT Whiskey Cove PMIC.
> +
>  endif
>
>  config ACPI_CONFIGFS
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index d94f92f..d78065c 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -101,6 +101,7 @@ 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
>  obj-$(CONFIG_BXT_WC_PMIC_OPREGION) += pmic/intel_pmic_bxtwc.o
> +obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o
>
>  obj-$(CONFIG_ACPI_CONFIGFS)    += acpi_configfs.o
>
> diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
> new file mode 100644
> index 0000000..85636d7
> --- /dev/null
> +++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
> @@ -0,0 +1,280 @@
> +/*
> + * Intel CHT Whiskey Cove PMIC operation region driver
> + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
> + *
> + * Based on various non upstream patches to support the CHT Whiskey Cove PMIC:
> + * Copyright (C) 2013-2015 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/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"
> +
> +#define CHT_WC_V1P05A_CTRL             0x6e3b
> +#define CHT_WC_V1P15_CTRL              0x6e3c
> +#define CHT_WC_V1P05A_VSEL             0x6e3d
> +#define CHT_WC_V1P15_VSEL              0x6e3e
> +#define CHT_WC_V1P8A_CTRL              0x6e56
> +#define CHT_WC_V1P8SX_CTRL             0x6e57
> +#define CHT_WC_VDDQ_CTRL               0x6e58
> +#define CHT_WC_V1P2A_CTRL              0x6e59
> +#define CHT_WC_V1P2SX_CTRL             0x6e5a
> +#define CHT_WC_V1P8A_VSEL              0x6e5b
> +#define CHT_WC_VDDQ_VSEL               0x6e5c
> +#define CHT_WC_V2P8SX_CTRL             0x6e5d
> +#define CHT_WC_V3P3A_CTRL              0x6e5e
> +#define CHT_WC_V3P3SD_CTRL             0x6e5f
> +#define CHT_WC_VSDIO_CTRL              0x6e67
> +#define CHT_WC_V3P3A_VSEL              0x6e68
> +#define CHT_WC_VPROG1A_CTRL            0x6e90
> +#define CHT_WC_VPROG1B_CTRL            0x6e91
> +#define CHT_WC_VPROG1F_CTRL            0x6e95
> +#define CHT_WC_VPROG2D_CTRL            0x6e99
> +#define CHT_WC_VPROG3A_CTRL            0x6e9a
> +#define CHT_WC_VPROG3B_CTRL            0x6e9b
> +#define CHT_WC_VPROG4A_CTRL            0x6e9c
> +#define CHT_WC_VPROG4B_CTRL            0x6e9d
> +#define CHT_WC_VPROG4C_CTRL            0x6e9e
> +#define CHT_WC_VPROG4D_CTRL            0x6e9f
> +#define CHT_WC_VPROG5A_CTRL            0x6ea0
> +#define CHT_WC_VPROG5B_CTRL            0x6ea1
> +#define CHT_WC_VPROG6A_CTRL            0x6ea2
> +#define CHT_WC_VPROG6B_CTRL            0x6ea3
> +#define CHT_WC_VPROG1A_VSEL            0x6ec0
> +#define CHT_WC_VPROG1B_VSEL            0x6ec1
> +#define CHT_WC_V1P8SX_VSEL             0x6ec2
> +#define CHT_WC_V1P2SX_VSEL             0x6ec3
> +#define CHT_WC_V1P2A_VSEL              0x6ec4
> +#define CHT_WC_VPROG1F_VSEL            0x6ec5
> +#define CHT_WC_VSDIO_VSEL              0x6ec6
> +#define CHT_WC_V2P8SX_VSEL             0x6ec7
> +#define CHT_WC_V3P3SD_VSEL             0x6ec8
> +#define CHT_WC_VPROG2D_VSEL            0x6ec9
> +#define CHT_WC_VPROG3A_VSEL            0x6eca
> +#define CHT_WC_VPROG3B_VSEL            0x6ecb
> +#define CHT_WC_VPROG4A_VSEL            0x6ecc
> +#define CHT_WC_VPROG4B_VSEL            0x6ecd
> +#define CHT_WC_VPROG4C_VSEL            0x6ece
> +#define CHT_WC_VPROG4D_VSEL            0x6ecf
> +#define CHT_WC_VPROG5A_VSEL            0x6ed0
> +#define CHT_WC_VPROG5B_VSEL            0x6ed1
> +#define CHT_WC_VPROG6A_VSEL            0x6ed2
> +#define CHT_WC_VPROG6B_VSEL            0x6ed3
> +
> +/*
> + * Regulator support is based on the non upstream patch:
> + * "regulator: whiskey_cove: implements Whiskey Cove pmic VRF support"
> + * https://github.com/intel-aero/meta-intel-aero/blob/master/recipes-kernel/linux/linux-yocto/0019-regulator-whiskey_cove-implements-WhiskeyCove-pmic-V.patch
> + */
> +static struct pmic_table power_table[] = {
> +       {
> +               .address = 0x0,
> +               .reg = CHT_WC_V1P8A_CTRL,
> +               .bit = 0x01,
> +       }, /* V18A */
> +       {
> +               .address = 0x04,
> +               .reg = CHT_WC_V1P8SX_CTRL,
> +               .bit = 0x07,
> +       }, /* V18X */
> +       {
> +               .address = 0x08,
> +               .reg = CHT_WC_VDDQ_CTRL,
> +               .bit = 0x01,
> +       }, /* VDDQ */
> +       {
> +               .address = 0x0c,
> +               .reg = CHT_WC_V1P2A_CTRL,
> +               .bit = 0x07,
> +       }, /* V12A */
> +       {
> +               .address = 0x10,
> +               .reg = CHT_WC_V1P2SX_CTRL,
> +               .bit = 0x07,
> +       }, /* V12X */
> +       {
> +               .address = 0x14,
> +               .reg = CHT_WC_V2P8SX_CTRL,
> +               .bit = 0x07,
> +       }, /* V28X */
> +       {
> +               .address = 0x18,
> +               .reg = CHT_WC_V3P3A_CTRL,
> +               .bit = 0x01,
> +       }, /* V33A */
> +       {
> +               .address = 0x1c,
> +               .reg = CHT_WC_V3P3SD_CTRL,
> +               .bit = 0x07,
> +       }, /* V3SD */
> +       {
> +               .address = 0x20,
> +               .reg = CHT_WC_VSDIO_CTRL,
> +               .bit = 0x07,
> +       }, /* VSD */
> +/*     {
> +               .address = 0x24,
> +               .reg = ??,
> +               .bit = ??,
> +       }, ** VSW2 */
> +/*     {
> +               .address = 0x28,
> +               .reg = ??,
> +               .bit = ??,
> +       }, ** VSW1 */
> +/*     {
> +               .address = 0x2c,
> +               .reg = ??,
> +               .bit = ??,
> +       }, ** VUPY */
> +/*     {
> +               .address = 0x30,
> +               .reg = ??,
> +               .bit = ??,
> +       }, ** VRSO */
> +       {
> +               .address = 0x34,
> +               .reg = CHT_WC_VPROG1A_CTRL,
> +               .bit = 0x07,
> +       }, /* VP1A */
> +       {
> +               .address = 0x38,
> +               .reg = CHT_WC_VPROG1B_CTRL,
> +               .bit = 0x07,
> +       }, /* VP1B */
> +       {
> +               .address = 0x3c,
> +               .reg = CHT_WC_VPROG1F_CTRL,
> +               .bit = 0x07,
> +       }, /* VP1F */
> +       {
> +               .address = 0x40,
> +               .reg = CHT_WC_VPROG2D_CTRL,
> +               .bit = 0x07,
> +       }, /* VP2D */
> +       {
> +               .address = 0x44,
> +               .reg = CHT_WC_VPROG3A_CTRL,
> +               .bit = 0x07,
> +       }, /* VP3A */
> +       {
> +               .address = 0x48,
> +               .reg = CHT_WC_VPROG3B_CTRL,
> +               .bit = 0x07,
> +       }, /* VP3B */
> +       {
> +               .address = 0x4c,
> +               .reg = CHT_WC_VPROG4A_CTRL,
> +               .bit = 0x07,
> +       }, /* VP4A */
> +       {
> +               .address = 0x50,
> +               .reg = CHT_WC_VPROG4B_CTRL,
> +               .bit = 0x07,
> +       }, /* VP4B */
> +       {
> +               .address = 0x54,
> +               .reg = CHT_WC_VPROG4C_CTRL,
> +               .bit = 0x07,
> +       }, /* VP4C */
> +       {
> +               .address = 0x58,
> +               .reg = CHT_WC_VPROG4D_CTRL,
> +               .bit = 0x07,
> +       }, /* VP4D */
> +       {
> +               .address = 0x5c,
> +               .reg = CHT_WC_VPROG5A_CTRL,
> +               .bit = 0x07,
> +       }, /* VP5A */
> +       {
> +               .address = 0x60,
> +               .reg = CHT_WC_VPROG5B_CTRL,
> +               .bit = 0x07,
> +       }, /* VP5B */
> +       {
> +               .address = 0x64,
> +               .reg = CHT_WC_VPROG6A_CTRL,
> +               .bit = 0x07,
> +       }, /* VP6A */
> +       {
> +               .address = 0x68,
> +               .reg = CHT_WC_VPROG6B_CTRL,
> +               .bit = 0x07,
> +       }, /* VP6B */
> +/*     {
> +               .address = 0x6c,
> +               .reg = ??,
> +               .bit = ??,
> +       }  ** VP7A */
> +};
> +
> +static int intel_cht_wc_pmic_get_power(struct regmap *regmap, int reg,
> +               int bit, u64 *value)
> +{
> +       int data;
> +
> +       if (regmap_read(regmap, reg, &data))
> +               return -EIO;
> +
> +       *value = (data & bit) ? 1 : 0;
> +       return 0;
> +}
> +
> +static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg,
> +               int bitmask, bool on)
> +{
> +       return regmap_update_bits(regmap, reg, bitmask, on ? 1 : 0);
> +}
> +
> +/*
> + * The thermal table and ops are empty, we do not support the Thermal opregion
> + * (DPTF) due to lacking documentation.
> + */
> +static struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
> +       .get_power              = intel_cht_wc_pmic_get_power,
> +       .update_power           = intel_cht_wc_pmic_update_power,
> +       .power_table            = power_table,
> +       .power_table_count      = ARRAY_SIZE(power_table),
> +};
> +
> +static int intel_cht_wc_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_cht_wc_pmic_opregion_data);
> +}
> +
> +static struct platform_device_id cht_wc_opregion_id_table[] = {
> +       { .name = "cht_wcove_region" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(platform, cht_wc_opregion_id_table);
> +
> +static struct platform_driver intel_cht_wc_pmic_opregion_driver = {
> +       .probe = intel_cht_wc_pmic_opregion_probe,
> +       .driver = {
> +               .name = "cht_whiskey_cove_pmic",
> +       },
> +       .id_table = cht_wc_opregion_id_table,
> +};
> +module_platform_driver(intel_cht_wc_pmic_opregion_driver);
> +
> +MODULE_DESCRIPTION("Intel CHT Whiskey Cove PMIC operation region driver");
> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> +MODULE_LICENSE("GPL");
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] ACPI / PMIC: Stop xpower OPRegion handler relying on IIO

[Rafael J. Wysocki]

On Wed, Apr 19, 2017 at 3:07 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> The intel_pmic_xpower code provides an OPRegion handler, which must be
> available before other drivers using it are loaded, which can only be
> ensured if both the mfd and opregion drivers are built in, which is why
> the Kconfig option for intel_pmic_xpower is a bool.
>
> The use of IIO is causing trouble for generic distro configs here as
> distros will typically want to build IIO drivers as modules and there
> really is no reason to use IIO here. The reading of the ADC value is a
> single regmap_bulk_read, which is already protected against races by
> the regmap-lock.
>
> This commit removes the use of IIO, allowing distros to enable the
> driver without needing to built IIO in and also actually simplifies
> the code.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Andy, Srinivas, any concerns?

> ---
>  drivers/acpi/Kconfig                  |  2 +-
>  drivers/acpi/pmic/intel_pmic_xpower.c | 21 ++++-----------------
>  2 files changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 4f12fe0..842530f 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -506,7 +506,7 @@ config CRC_PMIC_OPREGION
>
>  config XPOWER_PMIC_OPREGION
>         bool "ACPI operation region support for XPower AXP288 PMIC"
> -       depends on AXP288_ADC = y
> +       depends on MFD_AXP20X_I2C
>         help
>           This config adds ACPI operation region support for XPower AXP288 PMIC.
>
> diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
> index e6e991a..55f5111 100644
> --- a/drivers/acpi/pmic/intel_pmic_xpower.c
> +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
> @@ -18,7 +18,6 @@
>  #include <linux/mfd/axp20x.h>
>  #include <linux/regmap.h>
>  #include <linux/platform_device.h>
> -#include <linux/iio/consumer.h>
>  #include "intel_pmic.h"
>
>  #define XPOWER_GPADC_LOW       0x5b
> @@ -186,28 +185,16 @@ static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
>   * @regmap: regmap of the PMIC device
>   * @reg: register to get the reading
>   *
> - * We could get the sensor value by manipulating the HW regs here, but since
> - * the axp288 IIO driver may also access the same regs at the same time, the
> - * APIs provided by IIO subsystem are used here instead to avoid problems. As
> - * a result, the two passed in params are of no actual use.
> - *
>   * Return a positive value on success, errno on failure.
>   */
>  static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
>  {
> -       struct iio_channel *gpadc_chan;
> -       int ret, val;
> -
> -       gpadc_chan = iio_channel_get(NULL, "axp288-system-temp");
> -       if (IS_ERR_OR_NULL(gpadc_chan))
> -               return -EACCES;
> +       u8 buf[2];
>
> -       ret = iio_read_channel_raw(gpadc_chan, &val);
> -       if (ret < 0)
> -               val = ret;
> +       if (regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2))
> +               return -EIO;
>
> -       iio_channel_release(gpadc_chan);
> -       return val;
> +       return (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
>  }
>
>  static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = {
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] ACPI / PMIC: Add opregion driver for Intel CHT Whiskey Cove PMIC

[Srinivas Pandruvada]

On Wed, 2017-04-19 at 22:17 +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 19, 2017 at 3:06 PM, Hans de Goede <hdegoede@redhat.com>
> wrote:
> > 
> > Add opregion driver for Intel CHT Whiskey Cove PMIC, based on
> > various
> > non upstreamed CHT Whiskey Cove PMIC patches. This does not include
> > support for the Thermal opregion (DPTF) due to lacking
> > documentation.
> > 
> > Cc: Bin Gao <bin.gao@intel.com>
> > Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Felipe, Srinivas, any concerns here?
No.
The DPTF part I will try to add later.

Thanks,
Srinivas

> 
> > 
> > ---
> > Changes in v2:
> > -s/WhiskeyCove/Whiskey Cove/
> > -Some minor style tweaks
> > -Allow building as module
> > Changes in v3:
> > -Drop filename from copyright header
> > -Simplify intel_cht_wc_pmic_update_power to 1 line
> > -Move defines of regulator register addresses to
> > intel_pmic_chtwc.c,
> >  it is the only place where they are used
> > Changes in v4:
> > -Make the config option a bool, ACPI OPRegion handlers must be
> > available
> >  before other drivers using them are loaded, which can only be
> > ensured if
> >  both the mfd and opregion drivers are built in.
> > ---
> >  drivers/acpi/Kconfig                 |   6 +
> >  drivers/acpi/Makefile                |   1 +
> >  drivers/acpi/pmic/intel_pmic_chtwc.c | 280
> > +++++++++++++++++++++++++++++++++++
> >  3 files changed, 287 insertions(+)
> >  create mode 100644 drivers/acpi/pmic/intel_pmic_chtwc.c
> > 
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index 83e5f7e..4f12fe0 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -516,6 +516,12 @@ config BXT_WC_PMIC_OPREGION
> >         help
> >           This config adds ACPI operation region support for BXT
> > WhiskeyCove PMIC.
> > 
> > +config CHT_WC_PMIC_OPREGION
> > +       bool "ACPI operation region support for CHT Whiskey Cove
> > PMIC"
> > +       depends on INTEL_SOC_PMIC_CHTWC
> > +       help
> > +         This config adds ACPI operation region support for CHT
> > Whiskey Cove PMIC.
> > +
> >  endif
> > 
> >  config ACPI_CONFIGFS
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index d94f92f..d78065c 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -101,6 +101,7 @@ 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
> >  obj-$(CONFIG_BXT_WC_PMIC_OPREGION) += pmic/intel_pmic_bxtwc.o
> > +obj-$(CONFIG_CHT_WC_PMIC_OPREGION) += pmic/intel_pmic_chtwc.o
> > 
> >  obj-$(CONFIG_ACPI_CONFIGFS)    += acpi_configfs.o
> > 
> > diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c
> > b/drivers/acpi/pmic/intel_pmic_chtwc.c
> > new file mode 100644
> > index 0000000..85636d7
> > --- /dev/null
> > +++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
> > @@ -0,0 +1,280 @@
> > +/*
> > + * Intel CHT Whiskey Cove PMIC operation region driver
> > + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
> > + *
> > + * Based on various non upstream patches to support the CHT
> > Whiskey Cove PMIC:
> > + * Copyright (C) 2013-2015 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/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"
> > +
> > +#define CHT_WC_V1P05A_CTRL             0x6e3b
> > +#define CHT_WC_V1P15_CTRL              0x6e3c
> > +#define CHT_WC_V1P05A_VSEL             0x6e3d
> > +#define CHT_WC_V1P15_VSEL              0x6e3e
> > +#define CHT_WC_V1P8A_CTRL              0x6e56
> > +#define CHT_WC_V1P8SX_CTRL             0x6e57
> > +#define CHT_WC_VDDQ_CTRL               0x6e58
> > +#define CHT_WC_V1P2A_CTRL              0x6e59
> > +#define CHT_WC_V1P2SX_CTRL             0x6e5a
> > +#define CHT_WC_V1P8A_VSEL              0x6e5b
> > +#define CHT_WC_VDDQ_VSEL               0x6e5c
> > +#define CHT_WC_V2P8SX_CTRL             0x6e5d
> > +#define CHT_WC_V3P3A_CTRL              0x6e5e
> > +#define CHT_WC_V3P3SD_CTRL             0x6e5f
> > +#define CHT_WC_VSDIO_CTRL              0x6e67
> > +#define CHT_WC_V3P3A_VSEL              0x6e68
> > +#define CHT_WC_VPROG1A_CTRL            0x6e90
> > +#define CHT_WC_VPROG1B_CTRL            0x6e91
> > +#define CHT_WC_VPROG1F_CTRL            0x6e95
> > +#define CHT_WC_VPROG2D_CTRL            0x6e99
> > +#define CHT_WC_VPROG3A_CTRL            0x6e9a
> > +#define CHT_WC_VPROG3B_CTRL            0x6e9b
> > +#define CHT_WC_VPROG4A_CTRL            0x6e9c
> > +#define CHT_WC_VPROG4B_CTRL            0x6e9d
> > +#define CHT_WC_VPROG4C_CTRL            0x6e9e
> > +#define CHT_WC_VPROG4D_CTRL            0x6e9f
> > +#define CHT_WC_VPROG5A_CTRL            0x6ea0
> > +#define CHT_WC_VPROG5B_CTRL            0x6ea1
> > +#define CHT_WC_VPROG6A_CTRL            0x6ea2
> > +#define CHT_WC_VPROG6B_CTRL            0x6ea3
> > +#define CHT_WC_VPROG1A_VSEL            0x6ec0
> > +#define CHT_WC_VPROG1B_VSEL            0x6ec1
> > +#define CHT_WC_V1P8SX_VSEL             0x6ec2
> > +#define CHT_WC_V1P2SX_VSEL             0x6ec3
> > +#define CHT_WC_V1P2A_VSEL              0x6ec4
> > +#define CHT_WC_VPROG1F_VSEL            0x6ec5
> > +#define CHT_WC_VSDIO_VSEL              0x6ec6
> > +#define CHT_WC_V2P8SX_VSEL             0x6ec7
> > +#define CHT_WC_V3P3SD_VSEL             0x6ec8
> > +#define CHT_WC_VPROG2D_VSEL            0x6ec9
> > +#define CHT_WC_VPROG3A_VSEL            0x6eca
> > +#define CHT_WC_VPROG3B_VSEL            0x6ecb
> > +#define CHT_WC_VPROG4A_VSEL            0x6ecc
> > +#define CHT_WC_VPROG4B_VSEL            0x6ecd
> > +#define CHT_WC_VPROG4C_VSEL            0x6ece
> > +#define CHT_WC_VPROG4D_VSEL            0x6ecf
> > +#define CHT_WC_VPROG5A_VSEL            0x6ed0
> > +#define CHT_WC_VPROG5B_VSEL            0x6ed1
> > +#define CHT_WC_VPROG6A_VSEL            0x6ed2
> > +#define CHT_WC_VPROG6B_VSEL            0x6ed3
> > +
> > +/*
> > + * Regulator support is based on the non upstream patch:
> > + * "regulator: whiskey_cove: implements Whiskey Cove pmic VRF
> > support"
> > + * https://github.com/intel-aero/meta-intel-aero/blob/master/recip
> > es-kernel/linux/linux-yocto/0019-regulator-whiskey_cove-implements-
> > WhiskeyCove-pmic-V.patch
> > + */
> > +static struct pmic_table power_table[] = {
> > +       {
> > +               .address = 0x0,
> > +               .reg = CHT_WC_V1P8A_CTRL,
> > +               .bit = 0x01,
> > +       }, /* V18A */
> > +       {
> > +               .address = 0x04,
> > +               .reg = CHT_WC_V1P8SX_CTRL,
> > +               .bit = 0x07,
> > +       }, /* V18X */
> > +       {
> > +               .address = 0x08,
> > +               .reg = CHT_WC_VDDQ_CTRL,
> > +               .bit = 0x01,
> > +       }, /* VDDQ */
> > +       {
> > +               .address = 0x0c,
> > +               .reg = CHT_WC_V1P2A_CTRL,
> > +               .bit = 0x07,
> > +       }, /* V12A */
> > +       {
> > +               .address = 0x10,
> > +               .reg = CHT_WC_V1P2SX_CTRL,
> > +               .bit = 0x07,
> > +       }, /* V12X */
> > +       {
> > +               .address = 0x14,
> > +               .reg = CHT_WC_V2P8SX_CTRL,
> > +               .bit = 0x07,
> > +       }, /* V28X */
> > +       {
> > +               .address = 0x18,
> > +               .reg = CHT_WC_V3P3A_CTRL,
> > +               .bit = 0x01,
> > +       }, /* V33A */
> > +       {
> > +               .address = 0x1c,
> > +               .reg = CHT_WC_V3P3SD_CTRL,
> > +               .bit = 0x07,
> > +       }, /* V3SD */
> > +       {
> > +               .address = 0x20,
> > +               .reg = CHT_WC_VSDIO_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VSD */
> > +/*     {
> > +               .address = 0x24,
> > +               .reg = ??,
> > +               .bit = ??,
> > +       }, ** VSW2 */
> > +/*     {
> > +               .address = 0x28,
> > +               .reg = ??,
> > +               .bit = ??,
> > +       }, ** VSW1 */
> > +/*     {
> > +               .address = 0x2c,
> > +               .reg = ??,
> > +               .bit = ??,
> > +       }, ** VUPY */
> > +/*     {
> > +               .address = 0x30,
> > +               .reg = ??,
> > +               .bit = ??,
> > +       }, ** VRSO */
> > +       {
> > +               .address = 0x34,
> > +               .reg = CHT_WC_VPROG1A_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP1A */
> > +       {
> > +               .address = 0x38,
> > +               .reg = CHT_WC_VPROG1B_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP1B */
> > +       {
> > +               .address = 0x3c,
> > +               .reg = CHT_WC_VPROG1F_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP1F */
> > +       {
> > +               .address = 0x40,
> > +               .reg = CHT_WC_VPROG2D_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP2D */
> > +       {
> > +               .address = 0x44,
> > +               .reg = CHT_WC_VPROG3A_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP3A */
> > +       {
> > +               .address = 0x48,
> > +               .reg = CHT_WC_VPROG3B_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP3B */
> > +       {
> > +               .address = 0x4c,
> > +               .reg = CHT_WC_VPROG4A_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP4A */
> > +       {
> > +               .address = 0x50,
> > +               .reg = CHT_WC_VPROG4B_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP4B */
> > +       {
> > +               .address = 0x54,
> > +               .reg = CHT_WC_VPROG4C_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP4C */
> > +       {
> > +               .address = 0x58,
> > +               .reg = CHT_WC_VPROG4D_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP4D */
> > +       {
> > +               .address = 0x5c,
> > +               .reg = CHT_WC_VPROG5A_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP5A */
> > +       {
> > +               .address = 0x60,
> > +               .reg = CHT_WC_VPROG5B_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP5B */
> > +       {
> > +               .address = 0x64,
> > +               .reg = CHT_WC_VPROG6A_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP6A */
> > +       {
> > +               .address = 0x68,
> > +               .reg = CHT_WC_VPROG6B_CTRL,
> > +               .bit = 0x07,
> > +       }, /* VP6B */
> > +/*     {
> > +               .address = 0x6c,
> > +               .reg = ??,
> > +               .bit = ??,
> > +       }  ** VP7A */
> > +};
> > +
> > +static int intel_cht_wc_pmic_get_power(struct regmap *regmap, int
> > reg,
> > +               int bit, u64 *value)
> > +{
> > +       int data;
> > +
> > +       if (regmap_read(regmap, reg, &data))
> > +               return -EIO;
> > +
> > +       *value = (data & bit) ? 1 : 0;
> > +       return 0;
> > +}
> > +
> > +static int intel_cht_wc_pmic_update_power(struct regmap *regmap,
> > int reg,
> > +               int bitmask, bool on)
> > +{
> > +       return regmap_update_bits(regmap, reg, bitmask, on ? 1 :
> > 0);
> > +}
> > +
> > +/*
> > + * The thermal table and ops are empty, we do not support the
> > Thermal opregion
> > + * (DPTF) due to lacking documentation.
> > + */
> > +static struct intel_pmic_opregion_data
> > intel_cht_wc_pmic_opregion_data = {
> > +       .get_power              = intel_cht_wc_pmic_get_power,
> > +       .update_power           = intel_cht_wc_pmic_update_power,
> > +       .power_table            = power_table,
> > +       .power_table_count      = ARRAY_SIZE(power_table),
> > +};
> > +
> > +static int intel_cht_wc_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_cht_wc_pmic_opregion_data);
> > +}
> > +
> > +static struct platform_device_id cht_wc_opregion_id_table[] = {
> > +       { .name = "cht_wcove_region" },
> > +       {},
> > +};
> > +MODULE_DEVICE_TABLE(platform, cht_wc_opregion_id_table);
> > +
> > +static struct platform_driver intel_cht_wc_pmic_opregion_driver =
> > {
> > +       .probe = intel_cht_wc_pmic_opregion_probe,
> > +       .driver = {
> > +               .name = "cht_whiskey_cove_pmic",
> > +       },
> > +       .id_table = cht_wc_opregion_id_table,
> > +};
> > +module_platform_driver(intel_cht_wc_pmic_opregion_driver);
> > +
> > +MODULE_DESCRIPTION("Intel CHT Whiskey Cove PMIC operation region
> > driver");
> > +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.9.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] ACPI / PMIC: Stop xpower OPRegion handler relying on IIO

[Srinivas Pandruvada]

On Wed, 2017-04-19 at 22:18 +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 19, 2017 at 3:07 PM, Hans de Goede <hdegoede@redhat.com>
> wrote:
> > 
> > The intel_pmic_xpower code provides an OPRegion handler, which must
> > be
> > available before other drivers using it are loaded, which can only
> > be
> > ensured if both the mfd and opregion drivers are built in, which is
> > why
> > the Kconfig option for intel_pmic_xpower is a bool.
> > 
> > The use of IIO is causing trouble for generic distro configs here
> > as
> > distros will typically want to build IIO drivers as modules and
> > there
> > really is no reason to use IIO here. The reading of the ADC value
> > is a
> > single regmap_bulk_read, which is already protected against races
> > by
> > the regmap-lock.
> > 
> > This commit removes the use of IIO, allowing distros to enable the
> > driver without needing to built IIO in and also actually simplifies
> > the code.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Andy, Srinivas, any concerns?
Added Jacob. 
If regmap_bulk_read is protected, we don't need to go through IIO. So
change looks fine to me.

Thanks,
Srinivas
> 
> > 
> > ---
> >  drivers/acpi/Kconfig                  |  2 +-
> >  drivers/acpi/pmic/intel_pmic_xpower.c | 21 ++++-----------------
> >  2 files changed, 5 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index 4f12fe0..842530f 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -506,7 +506,7 @@ config CRC_PMIC_OPREGION
> > 
> >  config XPOWER_PMIC_OPREGION
> >         bool "ACPI operation region support for XPower AXP288 PMIC"
> > -       depends on AXP288_ADC = y
> > +       depends on MFD_AXP20X_I2C
> >         help
> >           This config adds ACPI operation region support for XPower
> > AXP288 PMIC.
> > 
> > diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c
> > b/drivers/acpi/pmic/intel_pmic_xpower.c
> > index e6e991a..55f5111 100644
> > --- a/drivers/acpi/pmic/intel_pmic_xpower.c
> > +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
> > @@ -18,7 +18,6 @@
> >  #include <linux/mfd/axp20x.h>
> >  #include <linux/regmap.h>
> >  #include <linux/platform_device.h>
> > -#include <linux/iio/consumer.h>
> >  #include "intel_pmic.h"
> > 
> >  #define XPOWER_GPADC_LOW       0x5b
> > @@ -186,28 +185,16 @@ static int
> > intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
> >   * @regmap: regmap of the PMIC device
> >   * @reg: register to get the reading
> >   *
> > - * We could get the sensor value by manipulating the HW regs here,
> > but since
> > - * the axp288 IIO driver may also access the same regs at the same
> > time, the
> > - * APIs provided by IIO subsystem are used here instead to avoid
> > problems. As
> > - * a result, the two passed in params are of no actual use.
> > - *
> >   * Return a positive value on success, errno on failure.
> >   */
> >  static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap,
> > int reg)
> >  {
> > -       struct iio_channel *gpadc_chan;
> > -       int ret, val;
> > -
> > -       gpadc_chan = iio_channel_get(NULL, "axp288-system-temp");
> > -       if (IS_ERR_OR_NULL(gpadc_chan))
> > -               return -EACCES;
> > +       u8 buf[2];
> > 
> > -       ret = iio_read_channel_raw(gpadc_chan, &val);
> > -       if (ret < 0)
> > -               val = ret;
> > +       if (regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2))
> > +               return -EIO;
> > 
> > -       iio_channel_release(gpadc_chan);
> > -       return val;
> > +       return (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
> >  }
> > 
> >  static struct intel_pmic_opregion_data
> > intel_xpower_pmic_opregion_data = {
> > --
> > 2.9.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] ACPI / PMIC: Stop xpower OPRegion handler relying on IIO

[Andy Shevchenko]

On Wed, 2017-04-19 at 22:18 +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 19, 2017 at 3:07 PM, Hans de Goede <hdegoede@redhat.com>
> wrote:
> > The intel_pmic_xpower code provides an OPRegion handler, which must
> > be
> > available before other drivers using it are loaded, which can only
> > be
> > ensured if both the mfd and opregion drivers are built in, which is
> > why
> > the Kconfig option for intel_pmic_xpower is a bool.
> > 
> > The use of IIO is causing trouble for generic distro configs here as
> > distros will typically want to build IIO drivers as modules and
> > there
> > really is no reason to use IIO here. The reading of the ADC value is
> > a
> > single regmap_bulk_read, which is already protected against races by
> > the regmap-lock.
> > 
> > This commit removes the use of IIO, allowing distros to enable the
> > driver without needing to built IIO in and also actually simplifies
> > the code.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Andy, Srinivas, any concerns?

I would prefer to have this magic shifts in a common header between
drivers, but I'm not insisting (I admit it might take much more work for
no clear benefit, except keeping ADC bits in one place).

So, I'm fine with this change.

> 
> > ---
> >  drivers/acpi/Kconfig                  |  2 +-
> >  drivers/acpi/pmic/intel_pmic_xpower.c | 21 ++++-----------------
> >  2 files changed, 5 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index 4f12fe0..842530f 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -506,7 +506,7 @@ config CRC_PMIC_OPREGION
> > 
> >  config XPOWER_PMIC_OPREGION
> >         bool "ACPI operation region support for XPower AXP288 PMIC"
> > -       depends on AXP288_ADC = y
> > +       depends on MFD_AXP20X_I2C
> >         help
> >           This config adds ACPI operation region support for XPower
> > AXP288 PMIC.
> > 
> > diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c
> > b/drivers/acpi/pmic/intel_pmic_xpower.c
> > index e6e991a..55f5111 100644
> > --- a/drivers/acpi/pmic/intel_pmic_xpower.c
> > +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
> > @@ -18,7 +18,6 @@
> >  #include <linux/mfd/axp20x.h>
> >  #include <linux/regmap.h>
> >  #include <linux/platform_device.h>
> > -#include <linux/iio/consumer.h>
> >  #include "intel_pmic.h"
> > 
> >  #define XPOWER_GPADC_LOW       0x5b
> > @@ -186,28 +185,16 @@ static int
> > intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
> >   * @regmap: regmap of the PMIC device
> >   * @reg: register to get the reading
> >   *
> > - * We could get the sensor value by manipulating the HW regs here,
> > but since
> > - * the axp288 IIO driver may also access the same regs at the same
> > time, the
> > - * APIs provided by IIO subsystem are used here instead to avoid
> > problems. As
> > - * a result, the two passed in params are of no actual use.
> > - *
> >   * Return a positive value on success, errno on failure.
> >   */
> >  static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap,
> > int reg)
> >  {
> > -       struct iio_channel *gpadc_chan;
> > -       int ret, val;
> > -
> > -       gpadc_chan = iio_channel_get(NULL, "axp288-system-temp");
> > -       if (IS_ERR_OR_NULL(gpadc_chan))
> > -               return -EACCES;
> > +       u8 buf[2];
> > 
> > -       ret = iio_read_channel_raw(gpadc_chan, &val);
> > -       if (ret < 0)
> > -               val = ret;
> > +       if (regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2))
> > +               return -EIO;
> > 
> > -       iio_channel_release(gpadc_chan);
> > -       return val;
> > +       return (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
> >  }
> > 
> >  static struct intel_pmic_opregion_data
> > intel_xpower_pmic_opregion_data = {
> > --
> > 2.9.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v4 2/2] ACPI / PMIC: Stop xpower OPRegion handler relying on IIO

[Rafael J. Wysocki]

On Thu, Apr 20, 2017 at 10:19 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, 2017-04-19 at 22:18 +0200, Rafael J. Wysocki wrote:
>> On Wed, Apr 19, 2017 at 3:07 PM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>> > The intel_pmic_xpower code provides an OPRegion handler, which must
>> > be
>> > available before other drivers using it are loaded, which can only
>> > be
>> > ensured if both the mfd and opregion drivers are built in, which is
>> > why
>> > the Kconfig option for intel_pmic_xpower is a bool.
>> >
>> > The use of IIO is causing trouble for generic distro configs here as
>> > distros will typically want to build IIO drivers as modules and
>> > there
>> > really is no reason to use IIO here. The reading of the ADC value is
>> > a
>> > single regmap_bulk_read, which is already protected against races by
>> > the regmap-lock.
>> >
>> > This commit removes the use of IIO, allowing distros to enable the
>> > driver without needing to built IIO in and also actually simplifies
>> > the code.
>> >
>> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Andy, Srinivas, any concerns?
>
> I would prefer to have this magic shifts in a common header between
> drivers, but I'm not insisting (I admit it might take much more work for
> no clear benefit, except keeping ADC bits in one place).
>
> So, I'm fine with this change.

OK

I'm going to apply the series then.

Thanks,
Rafael