[PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Andrey Zhizhikin]

This patchset introduces additional regulator driver for Intel Cherry
Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
PMIC, which is used to instantiate this regulator.

Regulator support for this PMIC was present in kernel release from Intel
targeted Aero platform, but was not entirely ported upstream and has
been omitted in mainline kernel releases. Consecutively, absence of
regulator caused the SD Card interface not to be provided with Vqcc
voltage source needed to operate with UHS-I cards.

Following patches are addessing this issue and making sd card interface
to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
of the SD Card interface in consumers and exposes optional "vqmmc"
voltage source, which mmc driver uses to switch signalling voltages
between 1.8V and 3.3V. 

This set contains of 2 patches: one is implementing the regulator driver
(based on a non upstreamed version from Intel Aero), and another patch
registers this driver as mfd cell in exising Whiskey Cove PMIC driver.


Andrey Zhizhikin (2):
  regulator: add support for Intel Cherry Whiskey Cove regulator
  mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC

 drivers/mfd/intel_soc_pmic_chtwc.c            |  15 +-
 drivers/regulator/Kconfig                     |  10 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/intel-cht-wc-regulator.c    | 433 ++++++++++++++++++
 .../linux/regulator/intel-cht-wc-regulator.h  |  64 +++
 5 files changed, 521 insertions(+), 2 deletions(-)
 create mode 100644 drivers/regulator/intel-cht-wc-regulator.c
 create mode 100644 include/linux/regulator/intel-cht-wc-regulator.h

-- 
2.17.1

[PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Andrey Zhizhikin]

Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which
supplies various voltage rails.

This initial version supports only vsdio, which is required to source
vqmmc for sd card interface.

Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com>
---
 drivers/regulator/Kconfig                     |  10 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/intel-cht-wc-regulator.c    | 433 ++++++++++++++++++
 .../linux/regulator/intel-cht-wc-regulator.h  |  64 +++
 4 files changed, 508 insertions(+)
 create mode 100644 drivers/regulator/intel-cht-wc-regulator.c
 create mode 100644 include/linux/regulator/intel-cht-wc-regulator.h

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 3ee63531f6d5..ae3d57f572df 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1077,6 +1077,16 @@ config REGULATOR_VEXPRESS
 	  This driver provides support for voltage regulators available
 	  on the ARM Ltd's Versatile Express platform.
 
+config REGULATOR_WHISKEY_COVE
+	tristate "PMIC Whiskey Cove voltage regulator"
+	depends on INTEL_SOC_PMIC_CHTWC
+	help
+	  This driver provides support for the voltage regulators of the
+	  Intel Whiskey Cove PMIC. It is provided as an mfd cell of the
+	  Intel Cherry Trail PMIC driver.
+	  Only select this regulator driver if the MFD part is selected
+	  in the Kernel configuration, it is meant to be operable as a cell.
+
 config REGULATOR_WM831X
 	tristate "Wolfson Microelectronics WM831x PMIC regulators"
 	depends on MFD_WM831X
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 2210ba56f9bd..380d536126c6 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -132,6 +132,7 @@ obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o
 obj-$(CONFIG_REGULATOR_UNIPHIER) += uniphier-regulator.o
 obj-$(CONFIG_REGULATOR_VCTRL) += vctrl-regulator.o
 obj-$(CONFIG_REGULATOR_VEXPRESS) += vexpress-regulator.o
+obj-$(CONFIG_REGULATOR_WHISKEY_COVE) += intel-cht-wc-regulator.o
 obj-$(CONFIG_REGULATOR_WM831X) += wm831x-dcdc.o
 obj-$(CONFIG_REGULATOR_WM831X) += wm831x-isink.o
 obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
diff --git a/drivers/regulator/intel-cht-wc-regulator.c b/drivers/regulator/intel-cht-wc-regulator.c
new file mode 100644
index 000000000000..5a66a6b37dc6
--- /dev/null
+++ b/drivers/regulator/intel-cht-wc-regulator.c
@@ -0,0 +1,433 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * intel-cht-wc-regulator.c - CherryTrail regulator driver
+ *
+ * Copyright (c) 2019, Leica Geosystems AG
+ *
+ * Base on the non-upstreamed version from Intel,
+ * which was introduced for Intel Aero board.
+ * Copyright (c) 2014, Intel Corporation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/regulator/intel-cht-wc-regulator.h>
+
+
+/*
+ * Voltage Control regulator offsets
+ */
+
+/* buck boost regulators */
+#define CHT_WC_V3P3A_CTRL	0x6e5e
+/* buck regulators */
+#define CHT_WC_V1P8A_CTRL	0x6e56
+#define CHT_WC_V1P05A_CTRL	0x6e3b
+#define CHT_WC_V1P15_CTRL	0x6e3c
+#define CHT_WC_VDDQ_CTRL	0x6e58
+/* boot regulators */
+/* ldo regulators */
+#define CHT_WC_VPROG1A_CTRL	0x6e90
+#define CHT_WC_VPROG1B_CTRL	0x6e91
+#define CHT_WC_VPROG1F_CTRL	0x6e95
+#define CHT_WC_V1P8SX_CTRL	0x6e57
+#define CHT_WC_V1P2A_CTRL	0x6e59
+#define CHT_WC_V1P2SX_CTRL	0x6e5a
+#define CHT_WC_VSDIO_CTRL	0x6e67
+#define CHT_WC_V2P8SX_CTRL	0x6e5d
+#define CHT_WC_V3P3SD_CTRL	0x6e5f
+#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
+
+
+/*
+ * Voltage Selector regulator offsets
+ */
+
+/* buck boost regulators */
+#define CHT_WC_V3P3A_VSEL	0x6e68
+/* buck regulators */
+#define CHT_WC_V1P8A_VSEL	0x6e5b
+#define CHT_WC_V1P05A_VSEL	0x6e3d
+#define CHT_WC_V1P15_VSEL	0x6e3e
+#define CHT_WC_VDDQ_VSEL		0x6e5c
+/* boot regulators */
+/* ldo regulators */
+#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
+
+
+/* number of voltage variations exposed */
+
+/* buck boost regulators */
+#define CHT_WC_V3P3A_VRANGE	8
+/* buck regulators */
+#define CHT_WC_V1P8A_VRANGE	256
+#define CHT_WC_V1P05A_VRANGE	256
+#define CHT_WC_VDDQ_VRANGE	120
+/* boot regulators */
+/* ldo regulators */
+#define CHT_WC_VPROG1A_VRANGE	53
+#define CHT_WC_VPROG1B_VRANGE	53
+#define CHT_WC_VPROG1F_VRANGE	53
+#define CHT_WC_V1P8SX_VRANGE	53
+#define CHT_WC_V1P2SX_VRANGE	53
+#define CHT_WC_V1P2A_VRANGE	53
+#define CHT_WC_VSDIO_VRANGE	53
+#define CHT_WC_V2P8SX_VRANGE	53
+#define CHT_WC_V3P3SD_VRANGE	53
+#define CHT_WC_VPROG2D_VRANGE	53
+#define CHT_WC_VPROG3A_VRANGE	53
+#define CHT_WC_VPROG3B_VRANGE	53
+#define CHT_WC_VPROG4A_VRANGE	53
+#define CHT_WC_VPROG4B_VRANGE	53
+#define CHT_WC_VPROG4C_VRANGE	53
+#define CHT_WC_VPROG4D_VRANGE	53
+#define CHT_WC_VPROG5A_VRANGE	53
+#define CHT_WC_VPROG5B_VRANGE	53
+#define CHT_WC_VPROG6A_VRANGE	53
+#define CHT_WC_VPROG6B_VRANGE	53
+
+
+/* voltage tables */
+static unsigned int CHT_WC_V3P3A_VSEL_TABLE[CHT_WC_V3P3A_VRANGE],
+		    CHT_WC_V1P8A_VSEL_TABLE[CHT_WC_V1P8A_VRANGE],
+		    CHT_WC_V1P05A_VSEL_TABLE[CHT_WC_V1P05A_VRANGE],
+		    CHT_WC_VDDQ_VSEL_TABLE[CHT_WC_VDDQ_VRANGE],
+		    CHT_WC_V1P8SX_VSEL_TABLE[CHT_WC_V1P8SX_VRANGE],
+		    CHT_WC_V1P2SX_VSEL_TABLE[CHT_WC_V1P2SX_VRANGE],
+		    CHT_WC_V1P2A_VSEL_TABLE[CHT_WC_V1P2A_VRANGE],
+		    CHT_WC_V2P8SX_VSEL_TABLE[CHT_WC_V2P8SX_VRANGE],
+		    CHT_WC_V3P3SD_VSEL_TABLE[CHT_WC_V3P3SD_VRANGE],
+		    CHT_WC_VPROG1A_VSEL_TABLE[CHT_WC_VPROG1A_VRANGE],
+		    CHT_WC_VPROG1B_VSEL_TABLE[CHT_WC_VPROG1B_VRANGE],
+		    CHT_WC_VPROG1F_VSEL_TABLE[CHT_WC_VPROG1F_VRANGE],
+		    CHT_WC_VPROG2D_VSEL_TABLE[CHT_WC_VPROG2D_VRANGE],
+		    CHT_WC_VPROG3A_VSEL_TABLE[CHT_WC_VPROG3A_VRANGE],
+		    CHT_WC_VPROG3B_VSEL_TABLE[CHT_WC_VPROG3B_VRANGE],
+		    CHT_WC_VPROG4A_VSEL_TABLE[CHT_WC_VPROG4A_VRANGE],
+		    CHT_WC_VPROG4B_VSEL_TABLE[CHT_WC_VPROG4B_VRANGE],
+		    CHT_WC_VPROG4C_VSEL_TABLE[CHT_WC_VPROG4C_VRANGE],
+		    CHT_WC_VPROG4D_VSEL_TABLE[CHT_WC_VPROG4D_VRANGE],
+		    CHT_WC_VPROG5A_VSEL_TABLE[CHT_WC_VPROG5A_VRANGE],
+		    CHT_WC_VPROG5B_VSEL_TABLE[CHT_WC_VPROG5B_VRANGE],
+		    CHT_WC_VPROG6A_VSEL_TABLE[CHT_WC_VPROG6A_VRANGE],
+		    CHT_WC_VPROG6B_VSEL_TABLE[CHT_WC_VPROG6B_VRANGE];
+
+/*
+ * VSDIO regulator description, voltage tables, consumers and constraints
+ */
+/*
+ * The VSDIO regulator should only support 1.8V and 3.3V. All other
+ * voltages are invalid for sd card, so disable them here.
+ */
+static unsigned int CHT_WC_VSDIO_VSEL_TABLE[CHT_WC_VSDIO_VRANGE] = {
+	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+	0, 0, 0, 0, 0, 0, 0, 0, 1800000, 0, 0, 0,
+	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+	0, 0, 3300000, 0, 0
+};
+
+/* List the SD card interface as a consumer of vqmmc voltage source from VSDIO
+ * regulator. This is the only interface that requires this source on Cherry
+ * Trail to operate with UHS-I cards.
+ */
+static struct regulator_consumer_supply cht_wc_vqmmc_supply_consumer[] = {
+	REGULATOR_SUPPLY("vqmmc", "80860F14:02"),
+};
+
+static struct regulator_init_data vqmmc_init_data = {
+	.constraints = {
+		.min_uV			= 1800000,
+		.max_uV			= 3300000,
+		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE |
+					REGULATOR_CHANGE_STATUS,
+		.valid_modes_mask	= REGULATOR_MODE_NORMAL,
+		.settling_time		= 20000,
+	},
+	.num_consumer_supplies	= ARRAY_SIZE(cht_wc_vqmmc_supply_consumer),
+	.consumer_supplies	= cht_wc_vqmmc_supply_consumer,
+};
+
+static int cht_wc_regulator_enable(struct regulator_dev *rdev)
+{
+	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
+
+	int ret = regmap_update_bits(rdev->regmap, pmic_info->vctl_reg,
+			pmic_info->vctl_mask, pmic_info->reg_enbl_mask);
+	if (ret < 0)
+		dev_err(&rdev->dev, "Failed to enable regulator %s: %d\n",
+			pmic_info->desc.name, ret);
+
+	return ret;
+}
+
+static int cht_wc_regulator_disable(struct regulator_dev *rdev)
+{
+	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
+
+	int ret = regmap_update_bits(rdev->regmap, pmic_info->vctl_reg,
+			pmic_info->vctl_mask, pmic_info->reg_dsbl_mask);
+	if (ret < 0)
+		dev_err(&rdev->dev, "Failed to disable regulator %s: %d\n",
+			pmic_info->desc.name, ret);
+
+	return ret;
+}
+
+static int cht_wc_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
+	int rval;
+
+	int ret = regmap_read(rdev->regmap, pmic_info->vctl_reg, &rval);
+
+	if (ret < 0) {
+		dev_err(&rdev->dev, "Register 0x%04x read failed: %d\n",
+			pmic_info->vsel_reg, ret);
+		return ret;
+	}
+
+	rval &= pmic_info->vctl_mask;
+
+	return rval & pmic_info->reg_enbl_mask;
+}
+
+static int cht_wc_regulator_read_voltage_sel(struct regulator_dev *rdev)
+{
+	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
+	int rval;
+
+	int ret = regmap_read(rdev->regmap, pmic_info->vsel_reg, &rval);
+
+	if (ret < 0) {
+		dev_err(&rdev->dev, "Register 0x%04x read failed: %d\n",
+			pmic_info->vsel_reg, ret);
+		return ret;
+	}
+
+	return (rval & pmic_info->vsel_mask) - pmic_info->start;
+}
+
+static int cht_wc_regulator_set_voltage_sel(struct regulator_dev *rdev,
+		unsigned int selector)
+{
+	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
+
+	int ret = regmap_update_bits(rdev->regmap, pmic_info->vsel_reg,
+			pmic_info->vsel_mask, selector + pmic_info->start);
+	if (ret < 0)
+		dev_err(&rdev->dev, "Failed to set voltage for regulator %s: %d\n",
+			pmic_info->desc.name, ret);
+
+	return ret;
+}
+
+/* regulator ops */
+static struct regulator_ops wcove_regulator_ops = {
+	.enable			= cht_wc_regulator_enable,
+	.disable		= cht_wc_regulator_disable,
+	.is_enabled		= cht_wc_regulator_is_enabled,
+	.get_voltage_sel	= cht_wc_regulator_read_voltage_sel,
+	.set_voltage_sel	= cht_wc_regulator_set_voltage_sel,
+	.list_voltage		= regulator_list_voltage_table,
+};
+
+#define CHT_WC_REGULATOR(_id, minv, maxv, strt, vselmsk, vscale, \
+				vctlmsk, enbl, dsbl, rt_flag, _init_data) \
+{\
+	.desc = {\
+		.name		= ""#_id,\
+		.ops		= &wcove_regulator_ops,\
+		.type		= REGULATOR_VOLTAGE,\
+		.id		= CHT_WC_REGULATOR_##_id,\
+		.owner		= THIS_MODULE,\
+	},\
+	.rdev		= NULL, \
+	.init_data	= (_init_data), \
+	.vctl_reg	= CHT_WC_##_id##_CTRL,\
+	.vsel_reg	= CHT_WC_##_id##_VSEL,\
+	.min_mV		= (minv),\
+	.max_mV		= (maxv),\
+	.start		= (strt),\
+	.vsel_mask	= (vselmsk),\
+	.scale		= (vscale),\
+	.nvolts		= CHT_WC_##_id##_VRANGE,\
+	.vctl_mask	= (vctlmsk),\
+	.reg_enbl_mask	= (enbl),\
+	.reg_dsbl_mask	= (dsbl),\
+	.vtable		= CHT_WC_##_id##_VSEL_TABLE,\
+	.runtime_table	= (rt_flag),\
+}
+
+/* Regulator descriptions */
+static struct ch_wc_regulator_info regulators_info[] = {
+	CHT_WC_REGULATOR(V3P3A,    3000, 3350, 0x00, 0x07, 50, 0x01, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(V1P8A,    250,  2100, 0x00, 0xff, 10, 0x01, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(V1P05A,   250,  2100, 0x00, 0xff, 10, 0x01, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VDDQ,     250,  1440, 0x00, 0x7f, 10, 0x01, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(V1P8SX,   800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(V1P2A,    800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(V1P2SX,   800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(V2P8SX,   800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VSDIO,    800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, false, &vqmmc_init_data),
+	CHT_WC_REGULATOR(V3P3SD,   800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG1A,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG1B,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG1F,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG2D,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG3A,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG3B,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG4A,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG4B,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG4C,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG4D,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG5A,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG5B,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG6A,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+	CHT_WC_REGULATOR(VPROG6B,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
+};
+
+static inline struct ch_wc_regulator_info *cht_wc_find_regulator_info(int id)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(regulators_info); i++) {
+		if (regulators_info[i].desc.id == id)
+			return &regulators_info[i];
+	}
+	return NULL;
+}
+
+static void initialize_vtable(struct ch_wc_regulator_info *reg_info)
+{
+	unsigned int i, volt;
+
+	if (reg_info->runtime_table == true) {
+		for (i = 0; i < reg_info->nvolts; i++) {
+			volt = reg_info->min_mV + (i * reg_info->scale);
+			if (volt < reg_info->min_mV)
+				volt = reg_info->min_mV;
+			if (volt > reg_info->max_mV)
+				volt = reg_info->max_mV;
+			/* set value in uV */
+			reg_info->vtable[i] = volt*1000;
+		}
+	}
+	reg_info->desc.volt_table = reg_info->vtable;
+	reg_info->desc.n_voltages = reg_info->nvolts;
+}
+
+static int cht_wc_regulator_probe(struct platform_device *pdev)
+{
+	struct ch_wc_regulator_info *regulator;
+	struct regulator_config config = { };
+
+	struct ch_wc_regulator_info *reg_info =
+		cht_wc_find_regulator_info(pdev->id);
+
+	if (reg_info == NULL)
+		return -EINVAL;
+
+	regulator = devm_kzalloc(&pdev->dev, sizeof(*regulator), GFP_KERNEL);
+	if (!regulator)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, regulator);
+
+	initialize_vtable(reg_info);
+
+	config.dev = &pdev->dev;
+	config.driver_data = reg_info;
+	config.init_data = reg_info->init_data;
+
+	regulator->rdev = regulator_register(&reg_info->desc, &config);
+
+	if (IS_ERR(regulator->rdev)) {
+		dev_err(&pdev->dev, "failed to register regulator as %s\n",
+				reg_info->desc.name);
+		return PTR_ERR(regulator->rdev);
+	}
+
+	dev_dbg(&pdev->dev, "registered Whiskey Cove regulator as %s\n",
+			dev_name(&regulator->rdev->dev));
+
+	return 0;
+}
+
+static int cht_wc_regulator_remove(struct platform_device *pdev)
+{
+	regulator_unregister(platform_get_drvdata(pdev));
+	return 0;
+}
+
+static const struct platform_device_id cht_wc_regulator_id_table[] = {
+	{
+		.name = "cht_wcove_regulator"
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, wcove_regulator_id_table);
+
+static struct platform_driver cht_wc_regulator_driver = {
+	.driver = {
+		.name = "cht_wcove_regulator",
+		.owner = THIS_MODULE,
+	},
+	.probe    = cht_wc_regulator_probe,
+	.remove   = cht_wc_regulator_remove,
+	.id_table = cht_wc_regulator_id_table,
+};
+
+static int __init cht_wc_regulator_init(void)
+{
+	return platform_driver_register(&cht_wc_regulator_driver);
+}
+subsys_initcall_sync(cht_wc_regulator_init);
+
+static void __exit cht_wc_regulator_exit(void)
+{
+	platform_driver_unregister(&cht_wc_regulator_driver);
+}
+module_exit(cht_wc_regulator_exit);
+
+MODULE_ALIAS("platform:cht-wc-regulator");
+MODULE_DESCRIPTION("Intel Cherry Trail Whiskey Cove Regulator driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com");
+MODULE_AUTHOR("Nitheesh K L <nitheesh.k.l@intel.com");
diff --git a/include/linux/regulator/intel-cht-wc-regulator.h b/include/linux/regulator/intel-cht-wc-regulator.h
new file mode 100644
index 000000000000..0f6b3be11fea
--- /dev/null
+++ b/include/linux/regulator/intel-cht-wc-regulator.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * intel-cht-wc-regulator.h - Cherry Trail Whiskey Cove PMIC regulator
+ *
+ * Copyright (c) 2019, Leica Geosystems AG
+ *
+ * Base on the non-upstreamed version from Intel, which was
+ * introduced for Intel Aero board.
+ * Copyright (c) 2014, Intel Corporation.
+ */
+
+#ifndef __LINUX_REGULATOR_INTEL_CHT_WC_H_
+#define __LINUX_REGULATOR_INTEL_CHT_WC_H_
+
+#include <linux/regulator/driver.h>
+
+enum {
+	CHT_WC_REGULATOR_V1P8A = 1,
+	CHT_WC_REGULATOR_V1P05A,
+	CHT_WC_REGULATOR_V1P15,
+	CHT_WC_REGULATOR_VDDQ,
+	CHT_WC_REGULATOR_V3P3A,
+	CHT_WC_REGULATOR_VPROG1A,
+	CHT_WC_REGULATOR_VPROG1B,
+	CHT_WC_REGULATOR_VPROG1F,
+	CHT_WC_REGULATOR_V1P8SX,
+	CHT_WC_REGULATOR_V1P2SX,
+	CHT_WC_REGULATOR_V1P2A,
+	CHT_WC_REGULATOR_VSDIO,
+	CHT_WC_REGULATOR_V2P8SX,
+	CHT_WC_REGULATOR_V3P3SD,
+	CHT_WC_REGULATOR_VPROG2D,
+	CHT_WC_REGULATOR_VPROG3A,
+	CHT_WC_REGULATOR_VPROG3B,
+	CHT_WC_REGULATOR_VPROG4A,
+	CHT_WC_REGULATOR_VPROG4B,
+	CHT_WC_REGULATOR_VPROG4C,
+	CHT_WC_REGULATOR_VPROG4D,
+	CHT_WC_REGULATOR_VPROG5A,
+	CHT_WC_REGULATOR_VPROG5B,
+	CHT_WC_REGULATOR_VPROG6A,
+	CHT_WC_REGULATOR_VPROG6B,
+};
+
+struct ch_wc_regulator_info {
+	struct regulator_desc desc;
+	struct regulator_dev *rdev;
+	struct regulator_init_data *init_data;
+	unsigned int vctl_reg;
+	unsigned int vsel_reg;
+	unsigned int min_mV;
+	unsigned int max_mV;
+	unsigned int start;
+	unsigned int vsel_mask;
+	unsigned int scale;
+	unsigned int nvolts;
+	unsigned int vctl_mask;
+	unsigned int reg_enbl_mask;
+	unsigned int reg_dsbl_mask;
+	unsigned int *vtable;
+	bool runtime_table;
+};
+
+#endif /* __LINUX_REGULATOR_INTEL_CHT_WC_H_ */
-- 
2.17.1

Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Andy Shevchenko]

On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:
> Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which
> supplies various voltage rails.
> 
> This initial version supports only vsdio, which is required to source
> vqmmc for sd card interface.

This patch has some style issues. I will wait for Adrian and Hans to comment on
the approach as a whole and then we will see how to improve the rest.

> Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com>
> ---
>  drivers/regulator/Kconfig                     |  10 +
>  drivers/regulator/Makefile                    |   1 +
>  drivers/regulator/intel-cht-wc-regulator.c    | 433 ++++++++++++++++++
>  .../linux/regulator/intel-cht-wc-regulator.h  |  64 +++
>  4 files changed, 508 insertions(+)
>  create mode 100644 drivers/regulator/intel-cht-wc-regulator.c
>  create mode 100644 include/linux/regulator/intel-cht-wc-regulator.h
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 3ee63531f6d5..ae3d57f572df 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1077,6 +1077,16 @@ config REGULATOR_VEXPRESS
>  	  This driver provides support for voltage regulators available
>  	  on the ARM Ltd's Versatile Express platform.
>  
> +config REGULATOR_WHISKEY_COVE
> +	tristate "PMIC Whiskey Cove voltage regulator"
> +	depends on INTEL_SOC_PMIC_CHTWC
> +	help
> +	  This driver provides support for the voltage regulators of the
> +	  Intel Whiskey Cove PMIC. It is provided as an mfd cell of the
> +	  Intel Cherry Trail PMIC driver.
> +	  Only select this regulator driver if the MFD part is selected
> +	  in the Kernel configuration, it is meant to be operable as a cell.
> +
>  config REGULATOR_WM831X
>  	tristate "Wolfson Microelectronics WM831x PMIC regulators"
>  	depends on MFD_WM831X
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 2210ba56f9bd..380d536126c6 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -132,6 +132,7 @@ obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o
>  obj-$(CONFIG_REGULATOR_UNIPHIER) += uniphier-regulator.o
>  obj-$(CONFIG_REGULATOR_VCTRL) += vctrl-regulator.o
>  obj-$(CONFIG_REGULATOR_VEXPRESS) += vexpress-regulator.o
> +obj-$(CONFIG_REGULATOR_WHISKEY_COVE) += intel-cht-wc-regulator.o
>  obj-$(CONFIG_REGULATOR_WM831X) += wm831x-dcdc.o
>  obj-$(CONFIG_REGULATOR_WM831X) += wm831x-isink.o
>  obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
> diff --git a/drivers/regulator/intel-cht-wc-regulator.c b/drivers/regulator/intel-cht-wc-regulator.c
> new file mode 100644
> index 000000000000..5a66a6b37dc6
> --- /dev/null
> +++ b/drivers/regulator/intel-cht-wc-regulator.c
> @@ -0,0 +1,433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel-cht-wc-regulator.c - CherryTrail regulator driver
> + *
> + * Copyright (c) 2019, Leica Geosystems AG
> + *
> + * Base on the non-upstreamed version from Intel,
> + * which was introduced for Intel Aero board.
> + * Copyright (c) 2014, Intel Corporation.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/regulator/intel-cht-wc-regulator.h>
> +
> +
> +/*
> + * Voltage Control regulator offsets
> + */
> +
> +/* buck boost regulators */
> +#define CHT_WC_V3P3A_CTRL	0x6e5e
> +/* buck regulators */
> +#define CHT_WC_V1P8A_CTRL	0x6e56
> +#define CHT_WC_V1P05A_CTRL	0x6e3b
> +#define CHT_WC_V1P15_CTRL	0x6e3c
> +#define CHT_WC_VDDQ_CTRL	0x6e58
> +/* boot regulators */
> +/* ldo regulators */
> +#define CHT_WC_VPROG1A_CTRL	0x6e90
> +#define CHT_WC_VPROG1B_CTRL	0x6e91
> +#define CHT_WC_VPROG1F_CTRL	0x6e95
> +#define CHT_WC_V1P8SX_CTRL	0x6e57
> +#define CHT_WC_V1P2A_CTRL	0x6e59
> +#define CHT_WC_V1P2SX_CTRL	0x6e5a
> +#define CHT_WC_VSDIO_CTRL	0x6e67
> +#define CHT_WC_V2P8SX_CTRL	0x6e5d
> +#define CHT_WC_V3P3SD_CTRL	0x6e5f
> +#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
> +
> +
> +/*
> + * Voltage Selector regulator offsets
> + */
> +
> +/* buck boost regulators */
> +#define CHT_WC_V3P3A_VSEL	0x6e68
> +/* buck regulators */
> +#define CHT_WC_V1P8A_VSEL	0x6e5b
> +#define CHT_WC_V1P05A_VSEL	0x6e3d
> +#define CHT_WC_V1P15_VSEL	0x6e3e
> +#define CHT_WC_VDDQ_VSEL		0x6e5c
> +/* boot regulators */
> +/* ldo regulators */
> +#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
> +
> +
> +/* number of voltage variations exposed */
> +
> +/* buck boost regulators */
> +#define CHT_WC_V3P3A_VRANGE	8
> +/* buck regulators */
> +#define CHT_WC_V1P8A_VRANGE	256
> +#define CHT_WC_V1P05A_VRANGE	256
> +#define CHT_WC_VDDQ_VRANGE	120
> +/* boot regulators */
> +/* ldo regulators */
> +#define CHT_WC_VPROG1A_VRANGE	53
> +#define CHT_WC_VPROG1B_VRANGE	53
> +#define CHT_WC_VPROG1F_VRANGE	53
> +#define CHT_WC_V1P8SX_VRANGE	53
> +#define CHT_WC_V1P2SX_VRANGE	53
> +#define CHT_WC_V1P2A_VRANGE	53
> +#define CHT_WC_VSDIO_VRANGE	53
> +#define CHT_WC_V2P8SX_VRANGE	53
> +#define CHT_WC_V3P3SD_VRANGE	53
> +#define CHT_WC_VPROG2D_VRANGE	53
> +#define CHT_WC_VPROG3A_VRANGE	53
> +#define CHT_WC_VPROG3B_VRANGE	53
> +#define CHT_WC_VPROG4A_VRANGE	53
> +#define CHT_WC_VPROG4B_VRANGE	53
> +#define CHT_WC_VPROG4C_VRANGE	53
> +#define CHT_WC_VPROG4D_VRANGE	53
> +#define CHT_WC_VPROG5A_VRANGE	53
> +#define CHT_WC_VPROG5B_VRANGE	53
> +#define CHT_WC_VPROG6A_VRANGE	53
> +#define CHT_WC_VPROG6B_VRANGE	53
> +
> +
> +/* voltage tables */
> +static unsigned int CHT_WC_V3P3A_VSEL_TABLE[CHT_WC_V3P3A_VRANGE],
> +		    CHT_WC_V1P8A_VSEL_TABLE[CHT_WC_V1P8A_VRANGE],
> +		    CHT_WC_V1P05A_VSEL_TABLE[CHT_WC_V1P05A_VRANGE],
> +		    CHT_WC_VDDQ_VSEL_TABLE[CHT_WC_VDDQ_VRANGE],
> +		    CHT_WC_V1P8SX_VSEL_TABLE[CHT_WC_V1P8SX_VRANGE],
> +		    CHT_WC_V1P2SX_VSEL_TABLE[CHT_WC_V1P2SX_VRANGE],
> +		    CHT_WC_V1P2A_VSEL_TABLE[CHT_WC_V1P2A_VRANGE],
> +		    CHT_WC_V2P8SX_VSEL_TABLE[CHT_WC_V2P8SX_VRANGE],
> +		    CHT_WC_V3P3SD_VSEL_TABLE[CHT_WC_V3P3SD_VRANGE],
> +		    CHT_WC_VPROG1A_VSEL_TABLE[CHT_WC_VPROG1A_VRANGE],
> +		    CHT_WC_VPROG1B_VSEL_TABLE[CHT_WC_VPROG1B_VRANGE],
> +		    CHT_WC_VPROG1F_VSEL_TABLE[CHT_WC_VPROG1F_VRANGE],
> +		    CHT_WC_VPROG2D_VSEL_TABLE[CHT_WC_VPROG2D_VRANGE],
> +		    CHT_WC_VPROG3A_VSEL_TABLE[CHT_WC_VPROG3A_VRANGE],
> +		    CHT_WC_VPROG3B_VSEL_TABLE[CHT_WC_VPROG3B_VRANGE],
> +		    CHT_WC_VPROG4A_VSEL_TABLE[CHT_WC_VPROG4A_VRANGE],
> +		    CHT_WC_VPROG4B_VSEL_TABLE[CHT_WC_VPROG4B_VRANGE],
> +		    CHT_WC_VPROG4C_VSEL_TABLE[CHT_WC_VPROG4C_VRANGE],
> +		    CHT_WC_VPROG4D_VSEL_TABLE[CHT_WC_VPROG4D_VRANGE],
> +		    CHT_WC_VPROG5A_VSEL_TABLE[CHT_WC_VPROG5A_VRANGE],
> +		    CHT_WC_VPROG5B_VSEL_TABLE[CHT_WC_VPROG5B_VRANGE],
> +		    CHT_WC_VPROG6A_VSEL_TABLE[CHT_WC_VPROG6A_VRANGE],
> +		    CHT_WC_VPROG6B_VSEL_TABLE[CHT_WC_VPROG6B_VRANGE];
> +
> +/*
> + * VSDIO regulator description, voltage tables, consumers and constraints
> + */
> +/*
> + * The VSDIO regulator should only support 1.8V and 3.3V. All other
> + * voltages are invalid for sd card, so disable them here.
> + */
> +static unsigned int CHT_WC_VSDIO_VSEL_TABLE[CHT_WC_VSDIO_VRANGE] = {
> +	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +	0, 0, 0, 0, 0, 0, 0, 0, 1800000, 0, 0, 0,
> +	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +	0, 0, 3300000, 0, 0
> +};
> +
> +/* List the SD card interface as a consumer of vqmmc voltage source from VSDIO
> + * regulator. This is the only interface that requires this source on Cherry
> + * Trail to operate with UHS-I cards.
> + */
> +static struct regulator_consumer_supply cht_wc_vqmmc_supply_consumer[] = {
> +	REGULATOR_SUPPLY("vqmmc", "80860F14:02"),
> +};
> +
> +static struct regulator_init_data vqmmc_init_data = {
> +	.constraints = {
> +		.min_uV			= 1800000,
> +		.max_uV			= 3300000,
> +		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE |
> +					REGULATOR_CHANGE_STATUS,
> +		.valid_modes_mask	= REGULATOR_MODE_NORMAL,
> +		.settling_time		= 20000,
> +	},
> +	.num_consumer_supplies	= ARRAY_SIZE(cht_wc_vqmmc_supply_consumer),
> +	.consumer_supplies	= cht_wc_vqmmc_supply_consumer,
> +};
> +
> +static int cht_wc_regulator_enable(struct regulator_dev *rdev)
> +{
> +	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
> +
> +	int ret = regmap_update_bits(rdev->regmap, pmic_info->vctl_reg,
> +			pmic_info->vctl_mask, pmic_info->reg_enbl_mask);
> +	if (ret < 0)
> +		dev_err(&rdev->dev, "Failed to enable regulator %s: %d\n",
> +			pmic_info->desc.name, ret);
> +
> +	return ret;
> +}
> +
> +static int cht_wc_regulator_disable(struct regulator_dev *rdev)
> +{
> +	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
> +
> +	int ret = regmap_update_bits(rdev->regmap, pmic_info->vctl_reg,
> +			pmic_info->vctl_mask, pmic_info->reg_dsbl_mask);
> +	if (ret < 0)
> +		dev_err(&rdev->dev, "Failed to disable regulator %s: %d\n",
> +			pmic_info->desc.name, ret);
> +
> +	return ret;
> +}
> +
> +static int cht_wc_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
> +	int rval;
> +
> +	int ret = regmap_read(rdev->regmap, pmic_info->vctl_reg, &rval);
> +
> +	if (ret < 0) {
> +		dev_err(&rdev->dev, "Register 0x%04x read failed: %d\n",
> +			pmic_info->vsel_reg, ret);
> +		return ret;
> +	}
> +
> +	rval &= pmic_info->vctl_mask;
> +
> +	return rval & pmic_info->reg_enbl_mask;
> +}
> +
> +static int cht_wc_regulator_read_voltage_sel(struct regulator_dev *rdev)
> +{
> +	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
> +	int rval;
> +
> +	int ret = regmap_read(rdev->regmap, pmic_info->vsel_reg, &rval);
> +
> +	if (ret < 0) {
> +		dev_err(&rdev->dev, "Register 0x%04x read failed: %d\n",
> +			pmic_info->vsel_reg, ret);
> +		return ret;
> +	}
> +
> +	return (rval & pmic_info->vsel_mask) - pmic_info->start;
> +}
> +
> +static int cht_wc_regulator_set_voltage_sel(struct regulator_dev *rdev,
> +		unsigned int selector)
> +{
> +	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
> +
> +	int ret = regmap_update_bits(rdev->regmap, pmic_info->vsel_reg,
> +			pmic_info->vsel_mask, selector + pmic_info->start);
> +	if (ret < 0)
> +		dev_err(&rdev->dev, "Failed to set voltage for regulator %s: %d\n",
> +			pmic_info->desc.name, ret);
> +
> +	return ret;
> +}
> +
> +/* regulator ops */
> +static struct regulator_ops wcove_regulator_ops = {
> +	.enable			= cht_wc_regulator_enable,
> +	.disable		= cht_wc_regulator_disable,
> +	.is_enabled		= cht_wc_regulator_is_enabled,
> +	.get_voltage_sel	= cht_wc_regulator_read_voltage_sel,
> +	.set_voltage_sel	= cht_wc_regulator_set_voltage_sel,
> +	.list_voltage		= regulator_list_voltage_table,
> +};
> +
> +#define CHT_WC_REGULATOR(_id, minv, maxv, strt, vselmsk, vscale, \
> +				vctlmsk, enbl, dsbl, rt_flag, _init_data) \
> +{\
> +	.desc = {\
> +		.name		= ""#_id,\
> +		.ops		= &wcove_regulator_ops,\
> +		.type		= REGULATOR_VOLTAGE,\
> +		.id		= CHT_WC_REGULATOR_##_id,\
> +		.owner		= THIS_MODULE,\
> +	},\
> +	.rdev		= NULL, \
> +	.init_data	= (_init_data), \
> +	.vctl_reg	= CHT_WC_##_id##_CTRL,\
> +	.vsel_reg	= CHT_WC_##_id##_VSEL,\
> +	.min_mV		= (minv),\
> +	.max_mV		= (maxv),\
> +	.start		= (strt),\
> +	.vsel_mask	= (vselmsk),\
> +	.scale		= (vscale),\
> +	.nvolts		= CHT_WC_##_id##_VRANGE,\
> +	.vctl_mask	= (vctlmsk),\
> +	.reg_enbl_mask	= (enbl),\
> +	.reg_dsbl_mask	= (dsbl),\
> +	.vtable		= CHT_WC_##_id##_VSEL_TABLE,\
> +	.runtime_table	= (rt_flag),\
> +}
> +
> +/* Regulator descriptions */
> +static struct ch_wc_regulator_info regulators_info[] = {
> +	CHT_WC_REGULATOR(V3P3A,    3000, 3350, 0x00, 0x07, 50, 0x01, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(V1P8A,    250,  2100, 0x00, 0xff, 10, 0x01, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(V1P05A,   250,  2100, 0x00, 0xff, 10, 0x01, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VDDQ,     250,  1440, 0x00, 0x7f, 10, 0x01, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(V1P8SX,   800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(V1P2A,    800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(V1P2SX,   800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(V2P8SX,   800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VSDIO,    800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, false, &vqmmc_init_data),
> +	CHT_WC_REGULATOR(V3P3SD,   800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG1A,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG1B,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG1F,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG2D,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG3A,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG3B,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG4A,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG4B,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG4C,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG4D,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG5A,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG5B,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG6A,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +	CHT_WC_REGULATOR(VPROG6B,  800,  3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true,  NULL),
> +};
> +
> +static inline struct ch_wc_regulator_info *cht_wc_find_regulator_info(int id)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(regulators_info); i++) {
> +		if (regulators_info[i].desc.id == id)
> +			return &regulators_info[i];
> +	}
> +	return NULL;
> +}
> +
> +static void initialize_vtable(struct ch_wc_regulator_info *reg_info)
> +{
> +	unsigned int i, volt;
> +
> +	if (reg_info->runtime_table == true) {
> +		for (i = 0; i < reg_info->nvolts; i++) {
> +			volt = reg_info->min_mV + (i * reg_info->scale);
> +			if (volt < reg_info->min_mV)
> +				volt = reg_info->min_mV;
> +			if (volt > reg_info->max_mV)
> +				volt = reg_info->max_mV;
> +			/* set value in uV */
> +			reg_info->vtable[i] = volt*1000;
> +		}
> +	}
> +	reg_info->desc.volt_table = reg_info->vtable;
> +	reg_info->desc.n_voltages = reg_info->nvolts;
> +}
> +
> +static int cht_wc_regulator_probe(struct platform_device *pdev)
> +{
> +	struct ch_wc_regulator_info *regulator;
> +	struct regulator_config config = { };
> +
> +	struct ch_wc_regulator_info *reg_info =
> +		cht_wc_find_regulator_info(pdev->id);
> +
> +	if (reg_info == NULL)
> +		return -EINVAL;
> +
> +	regulator = devm_kzalloc(&pdev->dev, sizeof(*regulator), GFP_KERNEL);
> +	if (!regulator)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, regulator);
> +
> +	initialize_vtable(reg_info);
> +
> +	config.dev = &pdev->dev;
> +	config.driver_data = reg_info;
> +	config.init_data = reg_info->init_data;
> +
> +	regulator->rdev = regulator_register(&reg_info->desc, &config);
> +
> +	if (IS_ERR(regulator->rdev)) {
> +		dev_err(&pdev->dev, "failed to register regulator as %s\n",
> +				reg_info->desc.name);
> +		return PTR_ERR(regulator->rdev);
> +	}
> +
> +	dev_dbg(&pdev->dev, "registered Whiskey Cove regulator as %s\n",
> +			dev_name(&regulator->rdev->dev));
> +
> +	return 0;
> +}
> +
> +static int cht_wc_regulator_remove(struct platform_device *pdev)
> +{
> +	regulator_unregister(platform_get_drvdata(pdev));
> +	return 0;
> +}
> +
> +static const struct platform_device_id cht_wc_regulator_id_table[] = {
> +	{
> +		.name = "cht_wcove_regulator"
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, wcove_regulator_id_table);
> +
> +static struct platform_driver cht_wc_regulator_driver = {
> +	.driver = {
> +		.name = "cht_wcove_regulator",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe    = cht_wc_regulator_probe,
> +	.remove   = cht_wc_regulator_remove,
> +	.id_table = cht_wc_regulator_id_table,
> +};
> +
> +static int __init cht_wc_regulator_init(void)
> +{
> +	return platform_driver_register(&cht_wc_regulator_driver);
> +}
> +subsys_initcall_sync(cht_wc_regulator_init);
> +
> +static void __exit cht_wc_regulator_exit(void)
> +{
> +	platform_driver_unregister(&cht_wc_regulator_driver);
> +}
> +module_exit(cht_wc_regulator_exit);
> +
> +MODULE_ALIAS("platform:cht-wc-regulator");
> +MODULE_DESCRIPTION("Intel Cherry Trail Whiskey Cove Regulator driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com");
> +MODULE_AUTHOR("Nitheesh K L <nitheesh.k.l@intel.com");
> diff --git a/include/linux/regulator/intel-cht-wc-regulator.h b/include/linux/regulator/intel-cht-wc-regulator.h
> new file mode 100644
> index 000000000000..0f6b3be11fea
> --- /dev/null
> +++ b/include/linux/regulator/intel-cht-wc-regulator.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * intel-cht-wc-regulator.h - Cherry Trail Whiskey Cove PMIC regulator
> + *
> + * Copyright (c) 2019, Leica Geosystems AG
> + *
> + * Base on the non-upstreamed version from Intel, which was
> + * introduced for Intel Aero board.
> + * Copyright (c) 2014, Intel Corporation.
> + */
> +
> +#ifndef __LINUX_REGULATOR_INTEL_CHT_WC_H_
> +#define __LINUX_REGULATOR_INTEL_CHT_WC_H_
> +
> +#include <linux/regulator/driver.h>
> +
> +enum {
> +	CHT_WC_REGULATOR_V1P8A = 1,
> +	CHT_WC_REGULATOR_V1P05A,
> +	CHT_WC_REGULATOR_V1P15,
> +	CHT_WC_REGULATOR_VDDQ,
> +	CHT_WC_REGULATOR_V3P3A,
> +	CHT_WC_REGULATOR_VPROG1A,
> +	CHT_WC_REGULATOR_VPROG1B,
> +	CHT_WC_REGULATOR_VPROG1F,
> +	CHT_WC_REGULATOR_V1P8SX,
> +	CHT_WC_REGULATOR_V1P2SX,
> +	CHT_WC_REGULATOR_V1P2A,
> +	CHT_WC_REGULATOR_VSDIO,
> +	CHT_WC_REGULATOR_V2P8SX,
> +	CHT_WC_REGULATOR_V3P3SD,
> +	CHT_WC_REGULATOR_VPROG2D,
> +	CHT_WC_REGULATOR_VPROG3A,
> +	CHT_WC_REGULATOR_VPROG3B,
> +	CHT_WC_REGULATOR_VPROG4A,
> +	CHT_WC_REGULATOR_VPROG4B,
> +	CHT_WC_REGULATOR_VPROG4C,
> +	CHT_WC_REGULATOR_VPROG4D,
> +	CHT_WC_REGULATOR_VPROG5A,
> +	CHT_WC_REGULATOR_VPROG5B,
> +	CHT_WC_REGULATOR_VPROG6A,
> +	CHT_WC_REGULATOR_VPROG6B,
> +};
> +
> +struct ch_wc_regulator_info {
> +	struct regulator_desc desc;
> +	struct regulator_dev *rdev;
> +	struct regulator_init_data *init_data;
> +	unsigned int vctl_reg;
> +	unsigned int vsel_reg;
> +	unsigned int min_mV;
> +	unsigned int max_mV;
> +	unsigned int start;
> +	unsigned int vsel_mask;
> +	unsigned int scale;
> +	unsigned int nvolts;
> +	unsigned int vctl_mask;
> +	unsigned int reg_enbl_mask;
> +	unsigned int reg_dsbl_mask;
> +	unsigned int *vtable;
> +	bool runtime_table;
> +};
> +
> +#endif /* __LINUX_REGULATOR_INTEL_CHT_WC_H_ */
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Andrey Zhizhikin]

On Fri, Oct 25, 2019 at 10:01 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:
> > Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which
> > supplies various voltage rails.
> >
> > This initial version supports only vsdio, which is required to source
> > vqmmc for sd card interface.
>
> This patch has some style issues. I will wait for Adrian and Hans to comment on
> the approach as a whole and then we will see how to improve the rest.

Agreed, styling issues are coming from definition of CHT_WC_REGULATOR
elements in regulators_info array, and they are mainly related to 80
characters per line. I decided to leave it like this since it is more
readable. But if the 80 characters rule is to be enforced here - I can
go with something like this for every element:
CHT_WC_REGULATOR(V3P3A,    3000, 3350, 0x00, 0x07,\
                                     50, 0x01, 0x01, 0x0, true,  NULL),

Let's wait for other people's comments here and I can then come up
with v2 of this patch.

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

--
Regards,
Andrey.

Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 10:58:05AM +0200, Andrey Zhizhikin wrote:
> On Fri, Oct 25, 2019 at 10:01 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:
> > > Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which
> > > supplies various voltage rails.
> > >
> > > This initial version supports only vsdio, which is required to source
> > > vqmmc for sd card interface.
> >
> > This patch has some style issues. I will wait for Adrian and Hans to comment on
> > the approach as a whole and then we will see how to improve the rest.
> 
> Agreed, styling issues are coming from definition of CHT_WC_REGULATOR
> elements in regulators_info array, and they are mainly related to 80
> characters per line. I decided to leave it like this since it is more
> readable. But if the 80 characters rule is to be enforced here - I can
> go with something like this for every element:
> CHT_WC_REGULATOR(V3P3A,    3000, 3350, 0x00, 0x07,\
>                                      50, 0x01, 0x01, 0x0, true,  NULL),

No, I'm not talking about these. Actually I hate this 80 limit in 21st century.
But let's not discuss it right now.

> Let's wait for other people's comments here and I can then come up
> with v2 of this patch.

Exactly!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Andrey Zhizhikin]

On Fri, Oct 25, 2019 at 11:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Oct 25, 2019 at 10:58:05AM +0200, Andrey Zhizhikin wrote:
> > On Fri, Oct 25, 2019 at 10:01 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:
> > > > Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which
> > > > supplies various voltage rails.
> > > >
> > > > This initial version supports only vsdio, which is required to source
> > > > vqmmc for sd card interface.
> > >
> > > This patch has some style issues. I will wait for Adrian and Hans to comment on
> > > the approach as a whole and then we will see how to improve the rest.
> >
> > Agreed, styling issues are coming from definition of CHT_WC_REGULATOR
> > elements in regulators_info array, and they are mainly related to 80
> > characters per line. I decided to leave it like this since it is more
> > readable. But if the 80 characters rule is to be enforced here - I can
> > go with something like this for every element:
> > CHT_WC_REGULATOR(V3P3A,    3000, 3350, 0x00, 0x07,\
> >                                      50, 0x01, 0x01, 0x0, true,  NULL),
>
> No, I'm not talking about these. Actually I hate this 80 limit in 21st century.
> But let's not discuss it right now.

I just ran a checkpatch and I think I know what you're referring to:
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?

In this case I really have to wait what others have to say, since I do
not know who and how the maintainer would be assigned.

>
> > Let's wait for other people's comments here and I can then come up
> > with v2 of this patch.
>
> Exactly!
>
> --
> With Best Regards,
> Andy Shevchenko
>

-- 
Regards,
Andrey.

Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 11:31:50AM +0200, Andrey Zhizhikin wrote:
> On Fri, Oct 25, 2019 at 11:14 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Oct 25, 2019 at 10:58:05AM +0200, Andrey Zhizhikin wrote:
> > > On Fri, Oct 25, 2019 at 10:01 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:
> > > > > Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which
> > > > > supplies various voltage rails.
> > > > >
> > > > > This initial version supports only vsdio, which is required to source
> > > > > vqmmc for sd card interface.
> > > >
> > > > This patch has some style issues. I will wait for Adrian and Hans to comment on
> > > > the approach as a whole and then we will see how to improve the rest.
> > >
> > > Agreed, styling issues are coming from definition of CHT_WC_REGULATOR
> > > elements in regulators_info array, and they are mainly related to 80
> > > characters per line. I decided to leave it like this since it is more
> > > readable. But if the 80 characters rule is to be enforced here - I can
> > > go with something like this for every element:
> > > CHT_WC_REGULATOR(V3P3A,    3000, 3350, 0x00, 0x07,\
> > >                                      50, 0x01, 0x01, 0x0, true,  NULL),
> >
> > No, I'm not talking about these. Actually I hate this 80 limit in 21st century.
> > But let's not discuss it right now.
> 
> I just ran a checkpatch and I think I know what you're referring to:
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> 
> In this case I really have to wait what others have to say, since I do
> not know who and how the maintainer would be assigned.

Neither this :-) Whoever it's an interesting warning from checkpatch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 6522 bytes --]

On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:

> +	  Only select this regulator driver if the MFD part is selected
> +	  in the Kernel configuration, it is meant to be operable as a cell.

This i what Kconfig dependencies are for.

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel-cht-wc-regulator.c - CherryTrail regulator driver
> + *

Please use C++ style for the entire comment so things look more
consistent.

> +#define CHT_WC_VPROG1A_VRANGE	53
> +#define CHT_WC_VPROG1B_VRANGE	53
> +#define CHT_WC_VPROG1F_VRANGE	53
> +#define CHT_WC_V1P8SX_VRANGE	53
> +#define CHT_WC_V1P2SX_VRANGE	53
> +#define CHT_WC_V1P2A_VRANGE	53
> +#define CHT_WC_VSDIO_VRANGE	53
> +#define CHT_WC_V2P8SX_VRANGE	53
> +#define CHT_WC_V3P3SD_VRANGE	53
> +#define CHT_WC_VPROG2D_VRANGE	53
> +#define CHT_WC_VPROG3A_VRANGE	53
> +#define CHT_WC_VPROG3B_VRANGE	53
> +#define CHT_WC_VPROG4A_VRANGE	53
> +#define CHT_WC_VPROG4B_VRANGE	53
> +#define CHT_WC_VPROG4C_VRANGE	53
> +#define CHT_WC_VPROG4D_VRANGE	53
> +#define CHT_WC_VPROG5A_VRANGE	53
> +#define CHT_WC_VPROG5B_VRANGE	53
> +#define CHT_WC_VPROG6A_VRANGE	53
> +#define CHT_WC_VPROG6B_VRANGE	53

These appear to be identical - is this not just a bunch of
instantiations of the same IPs

> +/* voltage tables */
> +static unsigned int CHT_WC_V3P3A_VSEL_TABLE[CHT_WC_V3P3A_VRANGE],
> +		    CHT_WC_V1P8A_VSEL_TABLE[CHT_WC_V1P8A_VRANGE],
> +		    CHT_WC_V1P05A_VSEL_TABLE[CHT_WC_V1P05A_VRANGE],
> +		    CHT_WC_VDDQ_VSEL_TABLE[CHT_WC_VDDQ_VRANGE],
> +		    CHT_WC_V1P8SX_VSEL_TABLE[CHT_WC_V1P8SX_VRANGE],
> +		    CHT_WC_V1P2SX_VSEL_TABLE[CHT_WC_V1P2SX_VRANGE],
> +		    CHT_WC_V1P2A_VSEL_TABLE[CHT_WC_V1P2A_VRANGE],
> +		    CHT_WC_V2P8SX_VSEL_TABLE[CHT_WC_V2P8SX_VRANGE],
> +		    CHT_WC_V3P3SD_VSEL_TABLE[CHT_WC_V3P3SD_VRANGE],
> +		    CHT_WC_VPROG1A_VSEL_TABLE[CHT_WC_VPROG1A_VRANGE],
> +		    CHT_WC_VPROG1B_VSEL_TABLE[CHT_WC_VPROG1B_VRANGE],
> +		    CHT_WC_VPROG1F_VSEL_TABLE[CHT_WC_VPROG1F_VRANGE],
> +		    CHT_WC_VPROG2D_VSEL_TABLE[CHT_WC_VPROG2D_VRANGE],
> +		    CHT_WC_VPROG3A_VSEL_TABLE[CHT_WC_VPROG3A_VRANGE],
> +		    CHT_WC_VPROG3B_VSEL_TABLE[CHT_WC_VPROG3B_VRANGE],
> +		    CHT_WC_VPROG4A_VSEL_TABLE[CHT_WC_VPROG4A_VRANGE],
> +		    CHT_WC_VPROG4B_VSEL_TABLE[CHT_WC_VPROG4B_VRANGE],
> +		    CHT_WC_VPROG4C_VSEL_TABLE[CHT_WC_VPROG4C_VRANGE],
> +		    CHT_WC_VPROG4D_VSEL_TABLE[CHT_WC_VPROG4D_VRANGE],
> +		    CHT_WC_VPROG5A_VSEL_TABLE[CHT_WC_VPROG5A_VRANGE],
> +		    CHT_WC_VPROG5B_VSEL_TABLE[CHT_WC_VPROG5B_VRANGE],
> +		    CHT_WC_VPROG6A_VSEL_TABLE[CHT_WC_VPROG6A_VRANGE],
> +		    CHT_WC_VPROG6B_VSEL_TABLE[CHT_WC_VPROG6B_VRANGE];

Please write a series of individual variable declarations, the
above way of writing things is very unusual and hence confusing
to read.  Though like I say it looks like the tables are mostly
identical so you probably only need a much smaller number of
tables, one per IP.

> +/*
> + * The VSDIO regulator should only support 1.8V and 3.3V. All other
> + * voltages are invalid for sd card, so disable them here.
> + */
> +static unsigned int CHT_WC_VSDIO_VSEL_TABLE[CHT_WC_VSDIO_VRANGE] = {
> +	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +	0, 0, 0, 0, 0, 0, 0, 0, 1800000, 0, 0, 0,
> +	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +	0, 0, 3300000, 0, 0
> +};

Is this really a limitation of the *regulator* or is it a
limitation of the consumer?  The combination of the way this is
written and the register layout makes it look like it's a
consumer limitation in which case leave it up to the consumer to
figure out what constraints it has.

> +/* List the SD card interface as a consumer of vqmmc voltage source from VSDIO
> + * regulator. This is the only interface that requires this source on Cherry
> + * Trail to operate with UHS-I cards.
> + */
> +static struct regulator_consumer_supply cht_wc_vqmmc_supply_consumer[] = {
> +	REGULATOR_SUPPLY("vqmmc", "80860F14:02"),
> +};

This looks like board specific configuration so should be defined
in the board.

> +static struct regulator_init_data vqmmc_init_data = {
> +	.constraints = {
> +		.min_uV			= 1800000,
> +		.max_uV			= 3300000,
> +		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE |
> +					REGULATOR_CHANGE_STATUS,
> +		.valid_modes_mask	= REGULATOR_MODE_NORMAL,
> +		.settling_time		= 20000,
> +	},
> +	.num_consumer_supplies	= ARRAY_SIZE(cht_wc_vqmmc_supply_consumer),
> +	.consumer_supplies	= cht_wc_vqmmc_supply_consumer,
> +};

This *definitely* appears to be board specific configuration and
should be defined for the board.

> +static int cht_wc_regulator_enable(struct regulator_dev *rdev)
> +{
> +	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);

regulator_enable_regmap()

> +static int cht_wc_regulator_disable(struct regulator_dev *rdev)
> +{
> +	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);

regulator_disable_regmap()

> +static int cht_wc_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
> +	int rval;

This looks like it's a get_status() operation (reading back the
actual staus of the regulator rather than if we asked for it to
be enabled or disabled).

> +static int cht_wc_regulator_read_voltage_sel(struct regulator_dev *rdev)
> +{

regulator_get_voltage_sel_regmap()

> +static int cht_wc_regulator_set_voltage_sel(struct regulator_dev *rdev,
> +		unsigned int selector)

regulator_set_voltage_sel_regmap()

> +static void initialize_vtable(struct ch_wc_regulator_info *reg_info)
> +{
> +	unsigned int i, volt;
> +
> +	if (reg_info->runtime_table == true) {
> +		for (i = 0; i < reg_info->nvolts; i++) {
> +			volt = reg_info->min_mV + (i * reg_info->scale);
> +			if (volt < reg_info->min_mV)
> +				volt = reg_info->min_mV;
> +			if (volt > reg_info->max_mV)
> +				volt = reg_info->max_mV;
> +			/* set value in uV */
> +			reg_info->vtable[i] = volt*1000;
> +		}
> +	}
> +	reg_info->desc.volt_table = reg_info->vtable;
> +	reg_info->desc.n_voltages = reg_info->nvolts;
> +}

This looks like you've got a linear range so you should be using
regulator_map_voltage_linear and regulator_list_voltage_linear.

> +	regulator->rdev = regulator_register(&reg_info->desc, &config);

devm_regulator_register()

> +static int __init cht_wc_regulator_init(void)
> +{
> +	return platform_driver_register(&cht_wc_regulator_driver);
> +}
> +subsys_initcall_sync(cht_wc_regulator_init);

Why subsys_initcall() and not just module_platform_driver?
Deferred probe should work fine for regulators.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Andrey Zhizhikin]

Something went wrong (mainly with me pressing wrong buttons), so I
managed to pull the conversation off the list. I'd duplicate my
answers here with comments from Mark, first being:

On Fri, Oct 25, 2019 at 4:43 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Oct 25, 2019 at 03:55:17PM +0200, Andrey Zhizhikin wrote:
> > On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:
>
> Please don't take things off-list unless there is a really strong reason
> to do so.  Sending things to the list ensures that everyone gets a
> chance to read and comment on things.  (From some of the things
> in your mail I think this might've been unintentional.)
>

Sorry for mess, sometimes it happens but I try not to create it...

On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:
>
> > +       Only select this regulator driver if the MFD part is selected
> > +       in the Kernel configuration, it is meant to be operable as a cell.
>
> This i what Kconfig dependencies are for.

True, would re-phrase to description.

>
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * intel-cht-wc-regulator.c - CherryTrail regulator driver
> > + *
>
> Please use C++ style for the entire comment so things look more
> consistent.

This is what I'm puzzled about - which style to use for the file
header since the introduction of SPDX and a rule that it should be
"C++ style" commented for source files and "C style" for header files.
After this introduction, should the more-or-less standard header be
also done in C++ style? I saw different source files are doing
different things... But all-in-all I would follow you advise here with
converting entire block to C++.

[Mark]: The only thing SPDX cares about is the first line, the making
the rest of the block a C++ one is mostly a preference I have.

Got it, would be done!

>
> > +#define CHT_WC_VPROG1A_VRANGE        53
> > +#define CHT_WC_VPROG1B_VRANGE        53
> > +#define CHT_WC_VPROG1F_VRANGE        53
> > +#define CHT_WC_V1P8SX_VRANGE 53
> > +#define CHT_WC_V1P2SX_VRANGE 53
> > +#define CHT_WC_V1P2A_VRANGE  53
> > +#define CHT_WC_VSDIO_VRANGE  53
> > +#define CHT_WC_V2P8SX_VRANGE 53
> > +#define CHT_WC_V3P3SD_VRANGE 53
> > +#define CHT_WC_VPROG2D_VRANGE        53
> > +#define CHT_WC_VPROG3A_VRANGE        53
> > +#define CHT_WC_VPROG3B_VRANGE        53
> > +#define CHT_WC_VPROG4A_VRANGE        53
> > +#define CHT_WC_VPROG4B_VRANGE        53
> > +#define CHT_WC_VPROG4C_VRANGE        53
> > +#define CHT_WC_VPROG4D_VRANGE        53
> > +#define CHT_WC_VPROG5A_VRANGE        53
> > +#define CHT_WC_VPROG5B_VRANGE        53
> > +#define CHT_WC_VPROG6A_VRANGE        53
> > +#define CHT_WC_VPROG6B_VRANGE        53
>
> These appear to be identical - is this not just a bunch of
> instantiations of the same IPs

That was done for "macro convenience". I would definitely re-work this part.

>
> > +/* voltage tables */
> > +static unsigned int CHT_WC_V3P3A_VSEL_TABLE[CHT_WC_V3P3A_VRANGE],
> > +                 CHT_WC_V1P8A_VSEL_TABLE[CHT_WC_V1P8A_VRANGE],
> > +                 CHT_WC_V1P05A_VSEL_TABLE[CHT_WC_V1P05A_VRANGE],
> > +                 CHT_WC_VDDQ_VSEL_TABLE[CHT_WC_VDDQ_VRANGE],
> > +                 CHT_WC_V1P8SX_VSEL_TABLE[CHT_WC_V1P8SX_VRANGE],
> > +                 CHT_WC_V1P2SX_VSEL_TABLE[CHT_WC_V1P2SX_VRANGE],
> > +                 CHT_WC_V1P2A_VSEL_TABLE[CHT_WC_V1P2A_VRANGE],
> > +                 CHT_WC_V2P8SX_VSEL_TABLE[CHT_WC_V2P8SX_VRANGE],
> > +                 CHT_WC_V3P3SD_VSEL_TABLE[CHT_WC_V3P3SD_VRANGE],
> > +                 CHT_WC_VPROG1A_VSEL_TABLE[CHT_WC_VPROG1A_VRANGE],
> > +                 CHT_WC_VPROG1B_VSEL_TABLE[CHT_WC_VPROG1B_VRANGE],
> > +                 CHT_WC_VPROG1F_VSEL_TABLE[CHT_WC_VPROG1F_VRANGE],
> > +                 CHT_WC_VPROG2D_VSEL_TABLE[CHT_WC_VPROG2D_VRANGE],
> > +                 CHT_WC_VPROG3A_VSEL_TABLE[CHT_WC_VPROG3A_VRANGE],
> > +                 CHT_WC_VPROG3B_VSEL_TABLE[CHT_WC_VPROG3B_VRANGE],
> > +                 CHT_WC_VPROG4A_VSEL_TABLE[CHT_WC_VPROG4A_VRANGE],
> > +                 CHT_WC_VPROG4B_VSEL_TABLE[CHT_WC_VPROG4B_VRANGE],
> > +                 CHT_WC_VPROG4C_VSEL_TABLE[CHT_WC_VPROG4C_VRANGE],
> > +                 CHT_WC_VPROG4D_VSEL_TABLE[CHT_WC_VPROG4D_VRANGE],
> > +                 CHT_WC_VPROG5A_VSEL_TABLE[CHT_WC_VPROG5A_VRANGE],
> > +                 CHT_WC_VPROG5B_VSEL_TABLE[CHT_WC_VPROG5B_VRANGE],
> > +                 CHT_WC_VPROG6A_VSEL_TABLE[CHT_WC_VPROG6A_VRANGE],
> > +                 CHT_WC_VPROG6B_VSEL_TABLE[CHT_WC_VPROG6B_VRANGE];
>
> Please write a series of individual variable declarations, the
> above way of writing things is very unusual and hence confusing
> to read.  Though like I say it looks like the tables are mostly
> identical so you probably only need a much smaller number of
> tables, one per IP.

Agreed, would be re-worked as well.

>
> > +/*
> > + * The VSDIO regulator should only support 1.8V and 3.3V. All other
> > + * voltages are invalid for sd card, so disable them here.
> > + */
> > +static unsigned int CHT_WC_VSDIO_VSEL_TABLE[CHT_WC_VSDIO_VRANGE] = {
> > +     0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > +     0, 0, 0, 0, 0, 0, 0, 0, 1800000, 0, 0, 0,
> > +     0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > +     0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > +     0, 0, 3300000, 0, 0
> > +};
>
> Is this really a limitation of the *regulator* or is it a
> limitation of the consumer?  The combination of the way this is
> written and the register layout makes it look like it's a
> consumer limitation in which case leave it up to the consumer to
> figure out what constraints it has.

This is a tricky point. Since there is no datasheet available from
Intel on this IP - I went with a safe option of taking this part from
original Intel patch, which they did for Aero board as the range was
described there in exactly this way. I am totally unsure if the
regulator does not provide all voltages, or this limit is purely
artificial here. Nevertheless, according to SD specification this
regulator should only operate with only those two voltage levels: 1v8
and 3v3 and consumers (mmc) should be well aware of this.

[Mark]: There's always the possibility that someone builds a board
with something else attached to the regulator if they don't want to
use SD cards for example.

I would re-work this to map all voltage ranges here properly, so there
would be no further confusions.

[Mark]: OK

>
> > +/* List the SD card interface as a consumer of vqmmc voltage source from VSDIO
> > + * regulator. This is the only interface that requires this source on Cherry
> > + * Trail to operate with UHS-I cards.
> > + */
> > +static struct regulator_consumer_supply cht_wc_vqmmc_supply_consumer[] = {
> > +     REGULATOR_SUPPLY("vqmmc", "80860F14:02"),
> > +};
>
> This looks like board specific configuration so should be defined
> in the board.
>
> > +static struct regulator_init_data vqmmc_init_data = {
> > +     .constraints = {
> > +             .min_uV                 = 1800000,
> > +             .max_uV                 = 3300000,
> > +             .valid_ops_mask         = REGULATOR_CHANGE_VOLTAGE |
> > +                                     REGULATOR_CHANGE_STATUS,
> > +             .valid_modes_mask       = REGULATOR_MODE_NORMAL,
> > +             .settling_time          = 20000,
> > +     },
> > +     .num_consumer_supplies  = ARRAY_SIZE(cht_wc_vqmmc_supply_consumer),
> > +     .consumer_supplies      = cht_wc_vqmmc_supply_consumer,
> > +};
>
> This *definitely* appears to be board specific configuration and
> should be defined for the board.

Above those two points above: I totally agree this is not the
regulator configuration but rather a specific board one. The only
thing I was not able to locate is a correct board file to put this
into.

Maybe you or Hans can guide me here on where to have this code as best?

[Mark]: I *think* drivers/platform/x86 might be what you're looking
for but I'm not super familiar with x86.  There's also
arch/x86/platform but I think they're also trying to push things out
of arch/.

>
> > +static int cht_wc_regulator_enable(struct regulator_dev *rdev)
> > +{
> > +     struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
>
> regulator_enable_regmap()

Agreed, would re-work this.

>
> > +static int cht_wc_regulator_disable(struct regulator_dev *rdev)
> > +{
> > +     struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
>
> regulator_disable_regmap()

... and this as well.

>
> > +static int cht_wc_regulator_is_enabled(struct regulator_dev *rdev)
> > +{
> > +     struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
> > +     int rval;
>
> This looks like it's a get_status() operation (reading back the
> actual staus of the regulator rather than if we asked for it to
> be enabled or disabled).

Yes, this is actually a get_status. Would re-work it.

>
> > +static int cht_wc_regulator_read_voltage_sel(struct regulator_dev *rdev)
> > +{
>
> regulator_get_voltage_sel_regmap()
>
> > +static int cht_wc_regulator_set_voltage_sel(struct regulator_dev *rdev,
> > +             unsigned int selector)
>
> regulator_set_voltage_sel_regmap()

I'd revise the whole regulator core API usage from comments above, as
I see now the driver code could be simplified dramatically.

>
> > +static void initialize_vtable(struct ch_wc_regulator_info *reg_info)
> > +{
> > +     unsigned int i, volt;
> > +
> > +     if (reg_info->runtime_table == true) {
> > +             for (i = 0; i < reg_info->nvolts; i++) {
> > +                     volt = reg_info->min_mV + (i * reg_info->scale);
> > +                     if (volt < reg_info->min_mV)
> > +                             volt = reg_info->min_mV;
> > +                     if (volt > reg_info->max_mV)
> > +                             volt = reg_info->max_mV;
> > +                     /* set value in uV */
> > +                     reg_info->vtable[i] = volt*1000;
> > +             }
> > +     }
> > +     reg_info->desc.volt_table = reg_info->vtable;
> > +     reg_info->desc.n_voltages = reg_info->nvolts;
> > +}
>
> This looks like you've got a linear range so you should be using
> regulator_map_voltage_linear and regulator_list_voltage_linear.

Yes, you're absolutely right, I've missed this one out. Would use a
proper API here as well.

>
> > +     regulator->rdev = regulator_register(&reg_info->desc, &config);
>
> devm_regulator_register()

Yeap, and consecutively devm_regulator_unregister() in
cht_wc_regulator_remove call.

[Mark]: With devm_ you don't even need to explicitly unregister so the
entire remove function can go.

>
> > +static int __init cht_wc_regulator_init(void)
> > +{
> > +     return platform_driver_register(&cht_wc_regulator_driver);
> > +}
> > +subsys_initcall_sync(cht_wc_regulator_init);
>
> Why subsys_initcall() and not just module_platform_driver?
> Deferred probe should work fine for regulators.

Got it, would correct this as well.

Overall, I would definitely work on a v2 here for both the regulator
and mfd cell part. If anyone has additional comment - please share
them here so I would gladly take them in!

Thanks a lot to all of you for reviewing this set!

--
Regards,
Andrey.

Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 05:26:56PM +0200, Andrey Zhizhikin wrote:
> On Fri, Oct 25, 2019 at 4:43 PM Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Oct 25, 2019 at 03:55:17PM +0200, Andrey Zhizhikin wrote:
> > > On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote:
> > > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:
> >
> > Please don't take things off-list unless there is a really strong reason
> > to do so.  Sending things to the list ensures that everyone gets a
> > chance to read and comment on things.  (From some of the things
> > in your mail I think this might've been unintentional.)

> Sorry for mess, sometimes it happens but I try not to create it...

Script nodded... :)

> On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:

> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * intel-cht-wc-regulator.c - CherryTrail regulator driver
> > > + *
> >
> > Please use C++ style for the entire comment so things look more
> > consistent.
> 
> This is what I'm puzzled about - which style to use for the file
> header since the introduction of SPDX and a rule that it should be
> "C++ style" commented for source files and "C style" for header files.
> After this introduction, should the more-or-less standard header be
> also done in C++ style? I saw different source files are doing
> different things... But all-in-all I would follow you advise here with
> converting entire block to C++.
> 
> [Mark]: The only thing SPDX cares about is the first line, the making
> the rest of the block a C++ one is mostly a preference I have.
> 
> Got it, would be done!

Also remove the file name(s) from file(s). If we would ever rename the file,
its name will be additional churn inside file (or often being forgotten).

> > Is this really a limitation of the *regulator* or is it a
> > limitation of the consumer?  The combination of the way this is
> > written and the register layout makes it look like it's a
> > consumer limitation in which case leave it up to the consumer to
> > figure out what constraints it has.
> 
> This is a tricky point. Since there is no datasheet available from

Key word "publicly".

I may look for it some like in November and perhaps be able to answer to
(some) questions.

> Intel on this IP - I went with a safe option of taking this part from
> original Intel patch, which they did for Aero board as the range was
> described there in exactly this way.

> > This *definitely* appears to be board specific configuration and
> > should be defined for the board.
> 
> Above those two points above: I totally agree this is not the
> regulator configuration but rather a specific board one. The only
> thing I was not able to locate is a correct board file to put this
> into.

See below.

> Maybe you or Hans can guide me here on where to have this code as best?
> 
> [Mark]: I *think* drivers/platform/x86 might be what you're looking
> for but I'm not super familiar with x86.  There's also
> arch/x86/platform but I think they're also trying to push things out
> of arch/.

It's more likely drivers/acpi/acpi_lpss.c.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Andrey Zhizhikin]

On Fri, Oct 25, 2019 at 6:02 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Oct 25, 2019 at 05:26:56PM +0200, Andrey Zhizhikin wrote:
> > On Fri, Oct 25, 2019 at 4:43 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Fri, Oct 25, 2019 at 03:55:17PM +0200, Andrey Zhizhikin wrote:
> > > > On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote:
> > > > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:
> > >
> > > Please don't take things off-list unless there is a really strong reason
> > > to do so.  Sending things to the list ensures that everyone gets a
> > > chance to read and comment on things.  (From some of the things
> > > in your mail I think this might've been unintentional.)
>
> > Sorry for mess, sometimes it happens but I try not to create it...
>
> Script nodded... :)

Point taken! :)

>
> > On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:
>
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * intel-cht-wc-regulator.c - CherryTrail regulator driver
> > > > + *
> > >
> > > Please use C++ style for the entire comment so things look more
> > > consistent.
> >
> > This is what I'm puzzled about - which style to use for the file
> > header since the introduction of SPDX and a rule that it should be
> > "C++ style" commented for source files and "C style" for header files.
> > After this introduction, should the more-or-less standard header be
> > also done in C++ style? I saw different source files are doing
> > different things... But all-in-all I would follow you advise here with
> > converting entire block to C++.
> >
> > [Mark]: The only thing SPDX cares about is the first line, the making
> > the rest of the block a C++ one is mostly a preference I have.
> >
> > Got it, would be done!
>
> Also remove the file name(s) from file(s). If we would ever rename the file,
> its name will be additional churn inside file (or often being forgotten).

OK, would use this pattern for all my future work now.

>
> > > Is this really a limitation of the *regulator* or is it a
> > > limitation of the consumer?  The combination of the way this is
> > > written and the register layout makes it look like it's a
> > > consumer limitation in which case leave it up to the consumer to
> > > figure out what constraints it has.
> >
> > This is a tricky point. Since there is no datasheet available from
>
> Key word "publicly".
>
> I may look for it some like in November and perhaps be able to answer to
> (some) questions.

That would be really great! The main point that needs to be clarified
at this moment is about voltages provided by vsdio cell, and if it
safe to include all of them all in the vsel table.

>
> > Intel on this IP - I went with a safe option of taking this part from
> > original Intel patch, which they did for Aero board as the range was
> > described there in exactly this way.
>
> > > This *definitely* appears to be board specific configuration and
> > > should be defined for the board.
> >
> > Above those two points above: I totally agree this is not the
> > regulator configuration but rather a specific board one. The only
> > thing I was not able to locate is a correct board file to put this
> > into.
>
> See below.
>
> > Maybe you or Hans can guide me here on where to have this code as best?
> >
> > [Mark]: I *think* drivers/platform/x86 might be what you're looking
> > for but I'm not super familiar with x86.  There's also
> > arch/x86/platform but I think they're also trying to push things out
> > of arch/.
>
> It's more likely drivers/acpi/acpi_lpss.c.

Thanks for the hint, I'd definitely would have a look at it!

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

-- 
Regards,
Andrey.

[PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC

[Andrey Zhizhikin]

Add a regulator mfd cell to Whiskey Cove PMIC driver, which is used to
supply various voltage rails.

In addition, make the initialization of this mfd driver early enough in
order to provide regulator cell to mmc sub-system when it is initialized.

Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com>
---
 drivers/mfd/intel_soc_pmic_chtwc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
index be84bb2aa837..47c0d9994784 100644
--- a/drivers/mfd/intel_soc_pmic_chtwc.c
+++ b/drivers/mfd/intel_soc_pmic_chtwc.c
@@ -17,6 +17,7 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/intel_soc_pmic.h>
 #include <linux/regmap.h>
+#include <linux/regulator/intel-cht-wc-regulator.h>
 
 /* PMIC device registers */
 #define REG_OFFSET_MASK		GENMASK(7, 0)
@@ -58,6 +59,11 @@ static struct mfd_cell cht_wc_dev[] = {
 		.name = "cht_wcove_ext_chgr",
 		.num_resources = ARRAY_SIZE(cht_wc_ext_charger_resources),
 		.resources = cht_wc_ext_charger_resources,
+	}, {
+		.name = "cht_wcove_regulator",
+		.id = CHT_WC_REGULATOR_VSDIO + 1,
+		.num_resources = 0,
+		.resources = NULL,
 	},
 	{	.name = "cht_wcove_region", },
 	{	.name = "cht_wcove_leds", },
@@ -215,7 +221,7 @@ static const struct acpi_device_id cht_wc_acpi_ids[] = {
 	{ }
 };
 
-static struct i2c_driver cht_wc_driver = {
+static struct i2c_driver cht_wc_i2c_driver = {
 	.driver	= {
 		.name	= "CHT Whiskey Cove PMIC",
 		.pm     = &cht_wc_pm_ops,
@@ -225,4 +231,9 @@ static struct i2c_driver cht_wc_driver = {
 	.shutdown = cht_wc_shutdown,
 	.id_table = cht_wc_i2c_id,
 };
-builtin_i2c_driver(cht_wc_driver);
+
+static int __init cht_wc_i2c_init(void)
+{
+	return i2c_add_driver(&cht_wc_i2c_driver);
+}
+subsys_initcall(cht_wc_i2c_init);
-- 
2.17.1

Re: [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC

[Andy Shevchenko]

On Thu, Oct 24, 2019 at 02:29:39PM +0000, Andrey Zhizhikin wrote:
> Add a regulator mfd cell to Whiskey Cove PMIC driver, which is used to
> supply various voltage rails.
> 
> In addition, make the initialization of this mfd driver early enough in
> order to provide regulator cell to mmc sub-system when it is initialized.

Doesn't deferred probe mechanism work for you?
MMC core returns that error till we have the driver initialized.

> +	}, {
> +		.name = "cht_wcove_regulator",
> +		.id = CHT_WC_REGULATOR_VSDIO + 1,

> +		.num_resources = 0,
> +		.resources = NULL,

No need to put these.

> -static struct i2c_driver cht_wc_driver = {
> +static struct i2c_driver cht_wc_i2c_driver = {

Renaming is not explained in the commit message.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC

[Andrey Zhizhikin]

On Fri, Oct 25, 2019 at 10:07 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Oct 24, 2019 at 02:29:39PM +0000, Andrey Zhizhikin wrote:
> > Add a regulator mfd cell to Whiskey Cove PMIC driver, which is used to
> > supply various voltage rails.
> >
> > In addition, make the initialization of this mfd driver early enough in
> > order to provide regulator cell to mmc sub-system when it is initialized.
>
> Doesn't deferred probe mechanism work for you?
> MMC core returns that error till we have the driver initialized.

This would work for mmc sub-system, but my idea was that later when
more cells are added to this mfd - it might turn out that we would
require an early initialization anyway. So I decided to take an
opportunity to adjust it with this patch as well, and this is what I
roughly explained in the commit message. When I'm reading it now,
exactly this point was not mentioned in commit message at all, and I
rather coupled the early init with mmc sub-system, which creates a
source of confusion here. I guess if there would be no other
objections about early init - I'd go with v2 of this patch, where I
would clean-up the point below and adjust the commit description.

Thanks a lot for pointing this out!

>
> > +     }, {
> > +             .name = "cht_wcove_regulator",
> > +             .id = CHT_WC_REGULATOR_VSDIO + 1,
>
> > +             .num_resources = 0,
> > +             .resources = NULL,
>
> No need to put these.

Agreed, this was forgotten. Sorry for that.

>
> > -static struct i2c_driver cht_wc_driver = {
> > +static struct i2c_driver cht_wc_i2c_driver = {
>
> Renaming is not explained in the commit message.

True, this point I forgot to mention. Actually, this is tightly
coupled with the fact that mfd driver has been moved to an earlier
init stage and since it does belong to I2C sub-system (and represented
by i2c_device structure) - I decided to make a name sound more
logical.

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

-- 
Regards,
Andrey.

Re: [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 11:16:22AM +0200, Andrey Zhizhikin wrote:
> On Fri, Oct 25, 2019 at 10:07 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Oct 24, 2019 at 02:29:39PM +0000, Andrey Zhizhikin wrote:
> > > Add a regulator mfd cell to Whiskey Cove PMIC driver, which is used to
> > > supply various voltage rails.
> > >
> > > In addition, make the initialization of this mfd driver early enough in
> > > order to provide regulator cell to mmc sub-system when it is initialized.
> >
> > Doesn't deferred probe mechanism work for you?
> > MMC core returns that error till we have the driver initialized.
> 
> This would work for mmc sub-system, but my idea was that later when
> more cells are added to this mfd - it might turn out that we would
> require an early initialization anyway. So I decided to take an
> opportunity to adjust it with this patch as well, and this is what I
> roughly explained in the commit message. When I'm reading it now,
> exactly this point was not mentioned in commit message at all, and I
> rather coupled the early init with mmc sub-system, which creates a
> source of confusion here. I guess if there would be no other
> objections about early init - I'd go with v2 of this patch, where I
> would clean-up the point below and adjust the commit description.
> 
> Thanks a lot for pointing this out!

> > > -static struct i2c_driver cht_wc_driver = {
> > > +static struct i2c_driver cht_wc_i2c_driver = {
> >
> > Renaming is not explained in the commit message.
> 
> True, this point I forgot to mention. Actually, this is tightly
> coupled with the fact that mfd driver has been moved to an earlier
> init stage and since it does belong to I2C sub-system (and represented
> by i2c_device structure) - I decided to make a name sound more
> logical.

So, seems you have three changes in one patch. Please, split accordingly.

In v2 also Cc all stakeholders I mentioned.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC

[Andrey Zhizhikin]

On Fri, Oct 25, 2019 at 12:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Oct 25, 2019 at 11:16:22AM +0200, Andrey Zhizhikin wrote:
> > On Fri, Oct 25, 2019 at 10:07 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Oct 24, 2019 at 02:29:39PM +0000, Andrey Zhizhikin wrote:
> > > > Add a regulator mfd cell to Whiskey Cove PMIC driver, which is used to
> > > > supply various voltage rails.
> > > >
> > > > In addition, make the initialization of this mfd driver early enough in
> > > > order to provide regulator cell to mmc sub-system when it is initialized.
> > >
> > > Doesn't deferred probe mechanism work for you?
> > > MMC core returns that error till we have the driver initialized.
> >
> > This would work for mmc sub-system, but my idea was that later when
> > more cells are added to this mfd - it might turn out that we would
> > require an early initialization anyway. So I decided to take an
> > opportunity to adjust it with this patch as well, and this is what I
> > roughly explained in the commit message. When I'm reading it now,
> > exactly this point was not mentioned in commit message at all, and I
> > rather coupled the early init with mmc sub-system, which creates a
> > source of confusion here. I guess if there would be no other
> > objections about early init - I'd go with v2 of this patch, where I
> > would clean-up the point below and adjust the commit description.
> >
> > Thanks a lot for pointing this out!
>
> > > > -static struct i2c_driver cht_wc_driver = {
> > > > +static struct i2c_driver cht_wc_i2c_driver = {
> > >
> > > Renaming is not explained in the commit message.
> >
> > True, this point I forgot to mention. Actually, this is tightly
> > coupled with the fact that mfd driver has been moved to an earlier
> > init stage and since it does belong to I2C sub-system (and represented
> > by i2c_device structure) - I decided to make a name sound more
> > logical.
>
> So, seems you have three changes in one patch. Please, split accordingly.

Got it, thanks! Now I see it would be more logical to have 3 patches
in the following order:
- i2c driver renaming
- additional regulator cell
- early init

If you don't mind - I'd wait for other people to comment on this
patch, collect all points, and then come up with v2 of this patch (and
whole series in fact).

>
> In v2 also Cc all stakeholders I mentioned.

Would certainly do!

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

-- 
Regards,
Andrey.

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Andy Shevchenko]

On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote:
> This patchset introduces additional regulator driver for Intel Cherry
> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
> PMIC, which is used to instantiate this regulator.
> 
> Regulator support for this PMIC was present in kernel release from Intel
> targeted Aero platform, but was not entirely ported upstream and has
> been omitted in mainline kernel releases. Consecutively, absence of
> regulator caused the SD Card interface not to be provided with Vqcc
> voltage source needed to operate with UHS-I cards.
> 
> Following patches are addessing this issue and making sd card interface
> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
> of the SD Card interface in consumers and exposes optional "vqmmc"
> voltage source, which mmc driver uses to switch signalling voltages
> between 1.8V and 3.3V. 
> 
> This set contains of 2 patches: one is implementing the regulator driver
> (based on a non upstreamed version from Intel Aero), and another patch
> registers this driver as mfd cell in exising Whiskey Cove PMIC driver.

Thank you.
Hans, Cc'ed, has quite interested in these kind of patches.
Am I right, Hans?

> 
> 
> Andrey Zhizhikin (2):
>   regulator: add support for Intel Cherry Whiskey Cove regulator
>   mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC
> 
>  drivers/mfd/intel_soc_pmic_chtwc.c            |  15 +-
>  drivers/regulator/Kconfig                     |  10 +
>  drivers/regulator/Makefile                    |   1 +
>  drivers/regulator/intel-cht-wc-regulator.c    | 433 ++++++++++++++++++
>  .../linux/regulator/intel-cht-wc-regulator.h  |  64 +++
>  5 files changed, 521 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/regulator/intel-cht-wc-regulator.c
>  create mode 100644 include/linux/regulator/intel-cht-wc-regulator.h
> 
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 10:53:35AM +0300, Andy Shevchenko wrote:
> On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote:
> > This patchset introduces additional regulator driver for Intel Cherry
> > Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
> > PMIC, which is used to instantiate this regulator.
> > 
> > Regulator support for this PMIC was present in kernel release from Intel
> > targeted Aero platform, but was not entirely ported upstream and has
> > been omitted in mainline kernel releases. Consecutively, absence of
> > regulator caused the SD Card interface not to be provided with Vqcc
> > voltage source needed to operate with UHS-I cards.
> > 
> > Following patches are addessing this issue and making sd card interface
> > to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
> > of the SD Card interface in consumers and exposes optional "vqmmc"
> > voltage source, which mmc driver uses to switch signalling voltages
> > between 1.8V and 3.3V. 
> > 
> > This set contains of 2 patches: one is implementing the regulator driver
> > (based on a non upstreamed version from Intel Aero), and another patch
> > registers this driver as mfd cell in exising Whiskey Cove PMIC driver.
> 
> Thank you.
> Hans, Cc'ed, has quite interested in these kind of patches.
> Am I right, Hans?

Since it's about UHS/SD, Cc to Adrian as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Andrey Zhizhikin]

On Fri, Oct 25, 2019 at 9:55 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Oct 25, 2019 at 10:53:35AM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote:
> > > This patchset introduces additional regulator driver for Intel Cherry
> > > Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
> > > PMIC, which is used to instantiate this regulator.
> > >
> > > Regulator support for this PMIC was present in kernel release from Intel
> > > targeted Aero platform, but was not entirely ported upstream and has
> > > been omitted in mainline kernel releases. Consecutively, absence of
> > > regulator caused the SD Card interface not to be provided with Vqcc
> > > voltage source needed to operate with UHS-I cards.
> > >
> > > Following patches are addessing this issue and making sd card interface
> > > to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
> > > of the SD Card interface in consumers and exposes optional "vqmmc"
> > > voltage source, which mmc driver uses to switch signalling voltages
> > > between 1.8V and 3.3V.
> > >
> > > This set contains of 2 patches: one is implementing the regulator driver
> > > (based on a non upstreamed version from Intel Aero), and another patch
> > > registers this driver as mfd cell in exising Whiskey Cove PMIC driver.
> >
> > Thank you.
> > Hans, Cc'ed, has quite interested in these kind of patches.
> > Am I right, Hans?
>
> Since it's about UHS/SD, Cc to Adrian as well.

Thanks for looking into this, and including also interested people
here. I've included only maintainers, but all interested parties are
also very welcomed.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

-- 
Regards,
Andrey.

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Adrian Hunter]

On 25/10/19 10:55 AM, Andy Shevchenko wrote:
> On Fri, Oct 25, 2019 at 10:53:35AM +0300, Andy Shevchenko wrote:
>> On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote:
>>> This patchset introduces additional regulator driver for Intel Cherry
>>> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
>>> PMIC, which is used to instantiate this regulator.
>>>
>>> Regulator support for this PMIC was present in kernel release from Intel
>>> targeted Aero platform, but was not entirely ported upstream and has
>>> been omitted in mainline kernel releases. Consecutively, absence of
>>> regulator caused the SD Card interface not to be provided with Vqcc
>>> voltage source needed to operate with UHS-I cards.
>>>
>>> Following patches are addessing this issue and making sd card interface
>>> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
>>> of the SD Card interface in consumers and exposes optional "vqmmc"
>>> voltage source, which mmc driver uses to switch signalling voltages
>>> between 1.8V and 3.3V. 
>>>
>>> This set contains of 2 patches: one is implementing the regulator driver
>>> (based on a non upstreamed version from Intel Aero), and another patch
>>> registers this driver as mfd cell in exising Whiskey Cove PMIC driver.
>>
>> Thank you.
>> Hans, Cc'ed, has quite interested in these kind of patches.
>> Am I right, Hans?
> 
> Since it's about UHS/SD, Cc to Adrian as well.
> 

My only concern is that the driver might conflict with ACPI methods trying
to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT
with code like this:

  If ((Arg2 == 0x03))
   {
       ADBG ("DSM 1p8")
       If ((^^I2C7.AVBL == One))
       {
           If ((PMID == One))
           {
               DATA = 0x59
               ^^I2C7.DL03 = BUFF /* \_SB_.PCI0.SHC1.BUFF */
           }
           ElseIf ((PMID == 0x02))
           {
               BUFF = ^^I2C7.XD31 /* \_SB_.PCI0.I2C7.XD31 */
               If ((STAT == Zero))
               {
                   DATA |= 0x10
                   ^^I2C7.XD31 = BUFF /* \_SB_.PCI0.SHC1.BUFF */
               }

               BUFF = ^^I2C7.XD32 /* \_SB_.PCI0.I2C7.XD32 */
               If ((STAT == Zero))
               {
                   DATA |= 0x0B
                   DATA &= 0xEB
                   ^^I2C7.XD32 = BUFF /* \_SB_.PCI0.SHC1.BUFF */
               }

               Sleep (0x0A)
               BUFF = ^^I2C7.XD31 /* \_SB_.PCI0.I2C7.XD31 */
               If ((STAT == Zero))
               {
                   DATA |= 0x20
                   ^^I2C7.XD31 = BUFF /* \_SB_.PCI0.SHC1.BUFF */
               }
           }
           ElseIf ((PMID == 0x03))
           {
               Local0 = ^^I2C7.PMI5.GET (One, 0x6E, 0x67)
               Sleep (0x0A)
               Local0 &= 0xF8
               ^^I2C7.PMI5.SET (One, 0x6E, 0x67, Local0)
               Sleep (0x64)
               Local0 = ^^I2C7.PMI5.GET (One, 0x6E, 0x67)
               Sleep (0x0A)
               Local0 |= One
               Local0 &= 0xF9
               ^^I2C7.PMI5.SET (One, 0x6E, 0x67, Local0)
               Sleep (0x0A)
               ^^I2C7.PMI5.SET (One, 0x6E, 0xC6, 0x1F)
               Sleep (0x0A)
           }
       }

       SDVL = One
       Return (0x03)
   }

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote:
> On 25/10/19 10:55 AM, Andy Shevchenko wrote:

> > Since it's about UHS/SD, Cc to Adrian as well.

> My only concern is that the driver might conflict with ACPI methods trying
> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT
> with code like this:

That's certainly what's idiomatic for ACPI (though machine specific
quirks are too!).  The safe thing to do would be to only register the
supply on systems where we know there's no ACPI method.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Hans de Goede]

Hi,

On 28-10-2019 13:45, Mark Brown wrote:
> On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote:
>> On 25/10/19 10:55 AM, Andy Shevchenko wrote:
> 
>>> Since it's about UHS/SD, Cc to Adrian as well.
> 
>> My only concern is that the driver might conflict with ACPI methods trying
>> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT
>> with code like this:

Oh, right that is a very good point.

> That's certainly what's idiomatic for ACPI (though machine specific
> quirks are too!).  The safe thing to do would be to only register the
> supply on systems where we know there's no ACPI method.

Right, so as I mentioned before Andrey told me about the evaluation
board he is using I was aware of only 3 Cherry Trail devices using
the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo
Yoga book. I've checked the DSDT of all 3 and all 3 of them offer
voltage control through the Intel _DSM method for voltage control.

I've also actually tested this on the GPD win and 1.8V signalling
works fine there without needing Andrey's patch.

So it seems that Andrey's patch should only be active on his
dev-board, as actual production hardware ships with the _DSM method.

I believe that the best solution is for the Whiskey Cove MFD driver:
drivers/mfd/intel_soc_pmic_chtwc.c

To only register the new cell on Andrey's evaluation board model
(based in a DMI match I guess). Another option would be to do
the DMI check in the regulator driver, but that would mean
udev will needlessly modprobe the regulator driver on production
hardware, so doing it in the MFD driver and not registering the cell
seems best,

Regards,

Hans

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Andrey Zhizhikin]

Hi Hans,

On Mon, Oct 28, 2019 at 2:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 28-10-2019 13:45, Mark Brown wrote:
> > On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote:
> >> On 25/10/19 10:55 AM, Andy Shevchenko wrote:
> >
> >>> Since it's about UHS/SD, Cc to Adrian as well.
> >
> >> My only concern is that the driver might conflict with ACPI methods trying
> >> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT
> >> with code like this:
>
> Oh, right that is a very good point.
>
> > That's certainly what's idiomatic for ACPI (though machine specific
> > quirks are too!).  The safe thing to do would be to only register the
> > supply on systems where we know there's no ACPI method.
>
> Right, so as I mentioned before Andrey told me about the evaluation
> board he is using I was aware of only 3 Cherry Trail devices using
> the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo
> Yoga book. I've checked the DSDT of all 3 and all 3 of them offer
> voltage control through the Intel _DSM method for voltage control.
>
> I've also actually tested this on the GPD win and 1.8V signalling
> works fine there without needing Andrey's patch.

Thanks a lot for checking this one out! At least this proves now that
the only platform affected is in fact Intel Aero board, and the patch
as it is might not be necessary to accommodate for all CHT-based
products with Whiskey Cove.

>
> So it seems that Andrey's patch should only be active on his
> dev-board, as actual production hardware ships with the _DSM method.
>
> I believe that the best solution is for the Whiskey Cove MFD driver:
> drivers/mfd/intel_soc_pmic_chtwc.c
>
> To only register the new cell on Andrey's evaluation board model
> (based in a DMI match I guess). Another option would be to do
> the DMI check in the regulator driver, but that would mean
> udev will needlessly modprobe the regulator driver on production
> hardware, so doing it in the MFD driver and not registering the cell
> seems best,

I tend to lean to a solution to perform a DMI check in MFD rather than
in the regulator driver, since this would keep the regulator
more-or-less agnostic to the where it is running on.

Or maybe it would even make more sense to create a board-specific hook
(like it was suggested for vqmmc voltage and sdmmc ACPI d of
consumer), and then only register a cell for Aero match? This would
actually keep the regulator consumer and mfd cell together and would
not allow the device-specific code to leak into generic driver
implementation. In this case I'd go with mfd_add_cell() if I get a DMI
match and register a vqmmc consumer in there.

In that case, can you please tell me what you think about it and if
the drivers/acpi/acpi_lpss.c would still be an appropriate location to
put this code to?

Thanks a lot!

>
> Regards,
>
> Hans
>


--
Regards,
Andrey.

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Hans de Goede]

Hi,

On 28-10-2019 16:01, Andrey Zhizhikin wrote:
> Hi Hans,
> 
> On Mon, Oct 28, 2019 at 2:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 28-10-2019 13:45, Mark Brown wrote:
>>> On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote:
>>>> On 25/10/19 10:55 AM, Andy Shevchenko wrote:
>>>
>>>>> Since it's about UHS/SD, Cc to Adrian as well.
>>>
>>>> My only concern is that the driver might conflict with ACPI methods trying
>>>> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT
>>>> with code like this:
>>
>> Oh, right that is a very good point.
>>
>>> That's certainly what's idiomatic for ACPI (though machine specific
>>> quirks are too!).  The safe thing to do would be to only register the
>>> supply on systems where we know there's no ACPI method.
>>
>> Right, so as I mentioned before Andrey told me about the evaluation
>> board he is using I was aware of only 3 Cherry Trail devices using
>> the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo
>> Yoga book. I've checked the DSDT of all 3 and all 3 of them offer
>> voltage control through the Intel _DSM method for voltage control.
>>
>> I've also actually tested this on the GPD win and 1.8V signalling
>> works fine there without needing Andrey's patch.
> 
> Thanks a lot for checking this one out! At least this proves now that
> the only platform affected is in fact Intel Aero board, and the patch
> as it is might not be necessary to accommodate for all CHT-based
> products with Whiskey Cove.
> 
>>
>> So it seems that Andrey's patch should only be active on his
>> dev-board, as actual production hardware ships with the _DSM method.
>>
>> I believe that the best solution is for the Whiskey Cove MFD driver:
>> drivers/mfd/intel_soc_pmic_chtwc.c
>>
>> To only register the new cell on Andrey's evaluation board model
>> (based in a DMI match I guess). Another option would be to do
>> the DMI check in the regulator driver, but that would mean
>> udev will needlessly modprobe the regulator driver on production
>> hardware, so doing it in the MFD driver and not registering the cell
>> seems best,
> 
> I tend to lean to a solution to perform a DMI check in MFD rather than
> in the regulator driver, since this would keep the regulator
> more-or-less agnostic to the where it is running on.
> 
> Or maybe it would even make more sense to create a board-specific hook
> (like it was suggested for vqmmc voltage and sdmmc ACPI d of
> consumer), and then only register a cell for Aero match? This would
> actually keep the regulator consumer and mfd cell together and would
> not allow the device-specific code to leak into generic driver
> implementation. In this case I'd go with mfd_add_cell() if I get a DMI
> match and register a vqmmc consumer in there.
> 
> In that case, can you please tell me what you think about it and if
> the drivers/acpi/acpi_lpss.c would still be an appropriate location to
> put this code to?

I do not think that drivers/acpi/acpi_lpss.c is a good place.

Thinking a bit more about this, my preferred solutions would be:

1. A BIOS update fixing the DSDT, as Andy suggested. Note we can
lso use an overlay DSDT in the initrd, but that will only help users
which take manual steps to add this to their initrd...

2. A new drivers/platform/x86 driver binding to the dmi-ids of the Areo
board, like e.g. drivers/platform/x86/intel_oaktrail.c is doing,
unlike that one you do not need to register a platform_device from
the module_init() function, you can just add the mfd-cell and the
regulator constraints from the module_init() function.

Assuming 1. is not an option (I do not know if Intel still
supports the Aero), then 2 will nicely isolate all the Aero
specific code into a driver which will only auto-load on
the Aero.

Regards,

Hans

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Andrey Zhizhikin]

Hi Hans!

On Tue, Oct 29, 2019 at 1:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 28-10-2019 16:01, Andrey Zhizhikin wrote:
> > Hi Hans,
> >
> > On Mon, Oct 28, 2019 at 2:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 28-10-2019 13:45, Mark Brown wrote:
> >>> On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote:
> >>>> On 25/10/19 10:55 AM, Andy Shevchenko wrote:
> >>>
> >>>>> Since it's about UHS/SD, Cc to Adrian as well.
> >>>
> >>>> My only concern is that the driver might conflict with ACPI methods trying
> >>>> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT
> >>>> with code like this:
> >>
> >> Oh, right that is a very good point.
> >>
> >>> That's certainly what's idiomatic for ACPI (though machine specific
> >>> quirks are too!).  The safe thing to do would be to only register the
> >>> supply on systems where we know there's no ACPI method.
> >>
> >> Right, so as I mentioned before Andrey told me about the evaluation
> >> board he is using I was aware of only 3 Cherry Trail devices using
> >> the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo
> >> Yoga book. I've checked the DSDT of all 3 and all 3 of them offer
> >> voltage control through the Intel _DSM method for voltage control.
> >>
> >> I've also actually tested this on the GPD win and 1.8V signalling
> >> works fine there without needing Andrey's patch.
> >
> > Thanks a lot for checking this one out! At least this proves now that
> > the only platform affected is in fact Intel Aero board, and the patch
> > as it is might not be necessary to accommodate for all CHT-based
> > products with Whiskey Cove.
> >
> >>
> >> So it seems that Andrey's patch should only be active on his
> >> dev-board, as actual production hardware ships with the _DSM method.
> >>
> >> I believe that the best solution is for the Whiskey Cove MFD driver:
> >> drivers/mfd/intel_soc_pmic_chtwc.c
> >>
> >> To only register the new cell on Andrey's evaluation board model
> >> (based in a DMI match I guess). Another option would be to do
> >> the DMI check in the regulator driver, but that would mean
> >> udev will needlessly modprobe the regulator driver on production
> >> hardware, so doing it in the MFD driver and not registering the cell
> >> seems best,
> >
> > I tend to lean to a solution to perform a DMI check in MFD rather than
> > in the regulator driver, since this would keep the regulator
> > more-or-less agnostic to the where it is running on.
> >
> > Or maybe it would even make more sense to create a board-specific hook
> > (like it was suggested for vqmmc voltage and sdmmc ACPI d of
> > consumer), and then only register a cell for Aero match? This would
> > actually keep the regulator consumer and mfd cell together and would
> > not allow the device-specific code to leak into generic driver
> > implementation. In this case I'd go with mfd_add_cell() if I get a DMI
> > match and register a vqmmc consumer in there.
> >
> > In that case, can you please tell me what you think about it and if
> > the drivers/acpi/acpi_lpss.c would still be an appropriate location to
> > put this code to?
>
> I do not think that drivers/acpi/acpi_lpss.c is a good place.

Thanks a lot for clarifying this point, I was also not sure whether I
would need combine the platform-specific functionality with LPSS
implementation.

>
> Thinking a bit more about this, my preferred solutions would be:
>
> 1. A BIOS update fixing the DSDT, as Andy suggested. Note we can
> lso use an overlay DSDT in the initrd, but that will only help users
> which take manual steps to add this to their initrd...

This I believe would not be an option since the Aero platform has been
phased-out from Intel, and Insyde most probably would not do an update
on the BIOS. I can go with DSDT overlay, but I was not sure whether
this is a good way to solve this.

>
> 2. A new drivers/platform/x86 driver binding to the dmi-ids of the Areo
> board, like e.g. drivers/platform/x86/intel_oaktrail.c is doing,
> unlike that one you do not need to register a platform_device from
> the module_init() function, you can just add the mfd-cell and the
> regulator constraints from the module_init() function.

This would be my preferred solution, since in this case I can contain
all the Aero-specific modifications to it's own board file. If there
would be further modifications needed for it - they would be nicely
contained within that board file.

>
> Assuming 1. is not an option (I do not know if Intel still
> supports the Aero), then 2 will nicely isolate all the Aero
> specific code into a driver which will only auto-load on
> the Aero.

Just as I indicated above, chances that BIOS would receive an update
are between slim and nil. If no one have any objection, I'd prefer to
go with the approach [2] from above.

>
> Regards,
>
> Hans
>


--
Regards,
Andrey.

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Hans de Goede]

Hi,

On 29-10-2019 17:57, Andrey Zhizhikin wrote:
> Hi Hans!
> 
> On Tue, Oct 29, 2019 at 1:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 28-10-2019 16:01, Andrey Zhizhikin wrote:
>>> Hi Hans,
>>>
>>> On Mon, Oct 28, 2019 at 2:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 28-10-2019 13:45, Mark Brown wrote:
>>>>> On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote:
>>>>>> On 25/10/19 10:55 AM, Andy Shevchenko wrote:
>>>>>
>>>>>>> Since it's about UHS/SD, Cc to Adrian as well.
>>>>>
>>>>>> My only concern is that the driver might conflict with ACPI methods trying
>>>>>> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT
>>>>>> with code like this:
>>>>
>>>> Oh, right that is a very good point.
>>>>
>>>>> That's certainly what's idiomatic for ACPI (though machine specific
>>>>> quirks are too!).  The safe thing to do would be to only register the
>>>>> supply on systems where we know there's no ACPI method.
>>>>
>>>> Right, so as I mentioned before Andrey told me about the evaluation
>>>> board he is using I was aware of only 3 Cherry Trail devices using
>>>> the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo
>>>> Yoga book. I've checked the DSDT of all 3 and all 3 of them offer
>>>> voltage control through the Intel _DSM method for voltage control.
>>>>
>>>> I've also actually tested this on the GPD win and 1.8V signalling
>>>> works fine there without needing Andrey's patch.
>>>
>>> Thanks a lot for checking this one out! At least this proves now that
>>> the only platform affected is in fact Intel Aero board, and the patch
>>> as it is might not be necessary to accommodate for all CHT-based
>>> products with Whiskey Cove.
>>>
>>>>
>>>> So it seems that Andrey's patch should only be active on his
>>>> dev-board, as actual production hardware ships with the _DSM method.
>>>>
>>>> I believe that the best solution is for the Whiskey Cove MFD driver:
>>>> drivers/mfd/intel_soc_pmic_chtwc.c
>>>>
>>>> To only register the new cell on Andrey's evaluation board model
>>>> (based in a DMI match I guess). Another option would be to do
>>>> the DMI check in the regulator driver, but that would mean
>>>> udev will needlessly modprobe the regulator driver on production
>>>> hardware, so doing it in the MFD driver and not registering the cell
>>>> seems best,
>>>
>>> I tend to lean to a solution to perform a DMI check in MFD rather than
>>> in the regulator driver, since this would keep the regulator
>>> more-or-less agnostic to the where it is running on.
>>>
>>> Or maybe it would even make more sense to create a board-specific hook
>>> (like it was suggested for vqmmc voltage and sdmmc ACPI d of
>>> consumer), and then only register a cell for Aero match? This would
>>> actually keep the regulator consumer and mfd cell together and would
>>> not allow the device-specific code to leak into generic driver
>>> implementation. In this case I'd go with mfd_add_cell() if I get a DMI
>>> match and register a vqmmc consumer in there.
>>>
>>> In that case, can you please tell me what you think about it and if
>>> the drivers/acpi/acpi_lpss.c would still be an appropriate location to
>>> put this code to?
>>
>> I do not think that drivers/acpi/acpi_lpss.c is a good place.
> 
> Thanks a lot for clarifying this point, I was also not sure whether I
> would need combine the platform-specific functionality with LPSS
> implementation.
> 
>>
>> Thinking a bit more about this, my preferred solutions would be:
>>
>> 1. A BIOS update fixing the DSDT, as Andy suggested. Note we can
>> lso use an overlay DSDT in the initrd, but that will only help users
>> which take manual steps to add this to their initrd...
> 
> This I believe would not be an option since the Aero platform has been
> phased-out from Intel, and Insyde most probably would not do an update
> on the BIOS. I can go with DSDT overlay, but I was not sure whether
> this is a good way to solve this.
> 
>>
>> 2. A new drivers/platform/x86 driver binding to the dmi-ids of the Areo
>> board, like e.g. drivers/platform/x86/intel_oaktrail.c is doing,
>> unlike that one you do not need to register a platform_device from
>> the module_init() function, you can just add the mfd-cell and the
>> regulator constraints from the module_init() function.
> 
> This would be my preferred solution, since in this case I can contain
> all the Aero-specific modifications to it's own board file. If there
> would be further modifications needed for it - they would be nicely
> contained within that board file.
> 
>>
>> Assuming 1. is not an option (I do not know if Intel still
>> supports the Aero), then 2 will nicely isolate all the Aero
>> specific code into a driver which will only auto-load on
>> the Aero.
> 
> Just as I indicated above, chances that BIOS would receive an update
> are between slim and nil. If no one have any objection, I'd prefer to
> go with the approach [2] from above.

I agree that 2. probably is the best, so I say go for it :)

Regards,

Hans

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Andy Shevchenko]

On Mon, Oct 28, 2019 at 02:26:21PM +0100, Hans de Goede wrote:
> On 28-10-2019 13:45, Mark Brown wrote:
> > On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote:
> > > On 25/10/19 10:55 AM, Andy Shevchenko wrote:
> > 
> > > > Since it's about UHS/SD, Cc to Adrian as well.
> > 
> > > My only concern is that the driver might conflict with ACPI methods trying
> > > to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT
> > > with code like this:
> 
> Oh, right that is a very good point.
> 
> > That's certainly what's idiomatic for ACPI (though machine specific
> > quirks are too!).  The safe thing to do would be to only register the
> > supply on systems where we know there's no ACPI method.
> 
> Right, so as I mentioned before Andrey told me about the evaluation
> board he is using I was aware of only 3 Cherry Trail devices using
> the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo
> Yoga book. I've checked the DSDT of all 3 and all 3 of them offer
> voltage control through the Intel _DSM method for voltage control.
> 
> I've also actually tested this on the GPD win and 1.8V signalling
> works fine there without needing Andrey's patch.
> 
> So it seems that Andrey's patch should only be active on his
> dev-board, as actual production hardware ships with the _DSM method.
> 
> I believe that the best solution is for the Whiskey Cove MFD driver:
> drivers/mfd/intel_soc_pmic_chtwc.c
> 
> To only register the new cell on Andrey's evaluation board model
> (based in a DMI match I guess). Another option would be to do
> the DMI check in the regulator driver, but that would mean
> udev will needlessly modprobe the regulator driver on production
> hardware, so doing it in the MFD driver and not registering the cell
> seems best,

I'm wondering if we can upgrade DSDT to provide _DSM for Aero platform.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Andrey Zhizhikin]

On Mon, Oct 28, 2019 at 1:42 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 25/10/19 10:55 AM, Andy Shevchenko wrote:
> > On Fri, Oct 25, 2019 at 10:53:35AM +0300, Andy Shevchenko wrote:
> >> On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote:
> >>> This patchset introduces additional regulator driver for Intel Cherry
> >>> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
> >>> PMIC, which is used to instantiate this regulator.
> >>>
> >>> Regulator support for this PMIC was present in kernel release from Intel
> >>> targeted Aero platform, but was not entirely ported upstream and has
> >>> been omitted in mainline kernel releases. Consecutively, absence of
> >>> regulator caused the SD Card interface not to be provided with Vqcc
> >>> voltage source needed to operate with UHS-I cards.
> >>>
> >>> Following patches are addessing this issue and making sd card interface
> >>> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
> >>> of the SD Card interface in consumers and exposes optional "vqmmc"
> >>> voltage source, which mmc driver uses to switch signalling voltages
> >>> between 1.8V and 3.3V.
> >>>
> >>> This set contains of 2 patches: one is implementing the regulator driver
> >>> (based on a non upstreamed version from Intel Aero), and another patch
> >>> registers this driver as mfd cell in exising Whiskey Cove PMIC driver.
> >>
> >> Thank you.
> >> Hans, Cc'ed, has quite interested in these kind of patches.
> >> Am I right, Hans?
> >
> > Since it's about UHS/SD, Cc to Adrian as well.
> >
>
> My only concern is that the driver might conflict with ACPI methods trying
> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT
> with code like this:
>
[... cut ...]

That's a good point, and this is what I tried at first when I started
to work on this problem. In my case INTEL_DSM_V18_SWITCH (fn=0x3) got
executed in the mmc sub-system, but without the regulator or DSDT
support the card did not get the proper voltage.

The DSDT on Intel Aero reference platform looks different here.
Currently I don't have it on hands, but would be able to look it up
next week.

-- 
Regards,
Andrey.

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Hans de Goede]

Hi,

On 25-10-2019 09:53, Andy Shevchenko wrote:
> On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote:
>> This patchset introduces additional regulator driver for Intel Cherry
>> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
>> PMIC, which is used to instantiate this regulator.
>>
>> Regulator support for this PMIC was present in kernel release from Intel
>> targeted Aero platform, but was not entirely ported upstream and has
>> been omitted in mainline kernel releases. Consecutively, absence of
>> regulator caused the SD Card interface not to be provided with Vqcc
>> voltage source needed to operate with UHS-I cards.
>>
>> Following patches are addessing this issue and making sd card interface
>> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
>> of the SD Card interface in consumers and exposes optional "vqmmc"
>> voltage source, which mmc driver uses to switch signalling voltages
>> between 1.8V and 3.3V.
>>
>> This set contains of 2 patches: one is implementing the regulator driver
>> (based on a non upstreamed version from Intel Aero), and another patch
>> registers this driver as mfd cell in exising Whiskey Cove PMIC driver.
> 
> Thank you.
> Hans, Cc'ed, has quite interested in these kind of patches.
> Am I right, Hans?

Yes since I do a lot of work on Bay and Cherry Trail hw enablement I'm
always interested in CHT specific patches.

Overall this series looks good (from a high level view I did not
do a detailed review) but I wonder if we really want to export all the
regulators when we just need the vsdio one?

Most regulators are controlled by either the P-Unit inside the CHT SoC
or through ACPI OpRegion accesses.  Luckily the regulator subsys does not
expose a sysfs interface for users to directly poke regulators, but this will
still make it somewhat easier for users to poke regulators which they should
leave alone.

Note I'm not saying this is wrong, having support for all regulators in place
in case we need it in the future is also kinda nice. OTOH if we just need the
one now, maybe we should just support the one for now ?

Andy do you have an opinion on this?

Andrey, may I ask which device you are testing this on?

Anyways overall good work, thank you for doing this!

Regards,

Hans

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Andrey Zhizhikin]

On Fri, Oct 25, 2019 at 11:38 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 25-10-2019 09:53, Andy Shevchenko wrote:
> > On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote:
> >> This patchset introduces additional regulator driver for Intel Cherry
> >> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
> >> PMIC, which is used to instantiate this regulator.
> >>
> >> Regulator support for this PMIC was present in kernel release from Intel
> >> targeted Aero platform, but was not entirely ported upstream and has
> >> been omitted in mainline kernel releases. Consecutively, absence of
> >> regulator caused the SD Card interface not to be provided with Vqcc
> >> voltage source needed to operate with UHS-I cards.
> >>
> >> Following patches are addessing this issue and making sd card interface
> >> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
> >> of the SD Card interface in consumers and exposes optional "vqmmc"
> >> voltage source, which mmc driver uses to switch signalling voltages
> >> between 1.8V and 3.3V.
> >>
> >> This set contains of 2 patches: one is implementing the regulator driver
> >> (based on a non upstreamed version from Intel Aero), and another patch
> >> registers this driver as mfd cell in exising Whiskey Cove PMIC driver.
> >
> > Thank you.
> > Hans, Cc'ed, has quite interested in these kind of patches.
> > Am I right, Hans?
>
> Yes since I do a lot of work on Bay and Cherry Trail hw enablement I'm
> always interested in CHT specific patches.
>
> Overall this series looks good (from a high level view I did not
> do a detailed review) but I wonder if we really want to export all the
> regulators when we just need the vsdio one?

I thought about this point, and actually came to a personal conclusion
that if I do this as a new driver - then it is better to list all
possible regulators, creating some sort of "skeleton" which people
could then work on if need be. I do agree that at the present moment
the one regulator which is exposed is the one for vsdio, but listing
all possibilities should not hurt. This was my motivation to put them
all into the driver on the first place.

If you believe additional regulator elements should be removed from
this version of the driver - I can clean them up and come up with v2
here.

>
> Most regulators are controlled by either the P-Unit inside the CHT SoC
> or through ACPI OpRegion accesses.  Luckily the regulator subsys does not
> expose a sysfs interface for users to directly poke regulators, but this will
> still make it somewhat easier for users to poke regulators which they should
> leave alone.

Agree, this is a valid point. But honestly I would really be surprised
if a user would directly touch something which can burn his silicon to
pieces. Regulators are usually not approached by users; unless they
are really HW engineers and know what they are doing.

>
> Note I'm not saying this is wrong, having support for all regulators in place
> in case we need it in the future is also kinda nice. OTOH if we just need the
> one now, maybe we should just support the one for now ?

This I've already covered above I guess.

> Andrey, may I ask which device you are testing this on?

Sure, I use the original Intel Aero board. It used to have an official
image from Aero team with a heavily patched 4.4.y kernel, but when I
decided to have this updated to the latest stable branch - I've faced
the issue of missing core functionality, which led me to this patch.

>
> Anyways overall good work, thank you for doing this!

You're welcome, and thanks for looking into this!

>
> Regards,
>
> Hans
>

-- 
Regards,
Andrey.

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Hans de Goede]

Hi,

On 25-10-2019 12:11, Andrey Zhizhikin wrote:
> On Fri, Oct 25, 2019 at 11:38 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 25-10-2019 09:53, Andy Shevchenko wrote:
>>> On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote:
>>>> This patchset introduces additional regulator driver for Intel Cherry
>>>> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
>>>> PMIC, which is used to instantiate this regulator.
>>>>
>>>> Regulator support for this PMIC was present in kernel release from Intel
>>>> targeted Aero platform, but was not entirely ported upstream and has
>>>> been omitted in mainline kernel releases. Consecutively, absence of
>>>> regulator caused the SD Card interface not to be provided with Vqcc
>>>> voltage source needed to operate with UHS-I cards.
>>>>
>>>> Following patches are addessing this issue and making sd card interface
>>>> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
>>>> of the SD Card interface in consumers and exposes optional "vqmmc"
>>>> voltage source, which mmc driver uses to switch signalling voltages
>>>> between 1.8V and 3.3V.
>>>>
>>>> This set contains of 2 patches: one is implementing the regulator driver
>>>> (based on a non upstreamed version from Intel Aero), and another patch
>>>> registers this driver as mfd cell in exising Whiskey Cove PMIC driver.
>>>
>>> Thank you.
>>> Hans, Cc'ed, has quite interested in these kind of patches.
>>> Am I right, Hans?
>>
>> Yes since I do a lot of work on Bay and Cherry Trail hw enablement I'm
>> always interested in CHT specific patches.
>>
>> Overall this series looks good (from a high level view I did not
>> do a detailed review) but I wonder if we really want to export all the
>> regulators when we just need the vsdio one?
> 
> I thought about this point, and actually came to a personal conclusion
> that if I do this as a new driver - then it is better to list all
> possible regulators, creating some sort of "skeleton" which people
> could then work on if need be. I do agree that at the present moment
> the one regulator which is exposed is the one for vsdio, but listing
> all possibilities should not hurt. This was my motivation to put them
> all into the driver on the first place.
> 
> If you believe additional regulator elements should be removed from
> this version of the driver - I can clean them up and come up with v2
> here.
> 
>>
>> Most regulators are controlled by either the P-Unit inside the CHT SoC
>> or through ACPI OpRegion accesses.  Luckily the regulator subsys does not
>> expose a sysfs interface for users to directly poke regulators, but this will
>> still make it somewhat easier for users to poke regulators which they should
>> leave alone.
> 
> Agree, this is a valid point. But honestly I would really be surprised
> if a user would directly touch something which can burn his silicon to
> pieces. Regulators are usually not approached by users; unless they
> are really HW engineers and know what they are doing.

True.

Hmm, I do just realize that the regulator subsystem turns off regulators
which are not in use from its pov, which would be kinda bad here.

It does this when the kernel is done initializing / switches to the
initrd or directly to the rootfs. Its been a while since I've last touched
this. Have you tried building the regulator driver (and mfd stuff and i2c
controller, etc.) into the kernel, so that the regulators work without modules?

I'm afraid that if you do that they will all get turned off, with expecting
results... Although I guess the regulator subsystem may skip this step for
regulators without any constrains, to be sure I would have to re-read the code.

Anyways this is a serious concern which we need to resolve.

>> Andrey, may I ask which device you are testing this on?
> 
> Sure, I use the original Intel Aero board. It used to have an official
> image from Aero team with a heavily patched 4.4.y kernel, but when I
> decided to have this updated to the latest stable branch - I've faced
> the issue of missing core functionality, which led me to this patch.

Ah I see, the reason I was asking is because I only know of three CHT
devices using a Whiskey Cove PMIC (instead of the AXP288, Crystal Cove or
TI one): the GPD Win, GPD pocket and one of the Lenovo Book versions
(the one with the CHT SoC).

Lets say I want to test this patch-set on one of the GPD devices,
I guess I need an UHS-I capable card and then what should I see
in sysfs for the mmc settings ?

Regards,

Hans

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Andrey Zhizhikin]

Hi,
On Fri, Oct 25, 2019 at 12:45 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 25-10-2019 12:11, Andrey Zhizhikin wrote:
> > On Fri, Oct 25, 2019 at 11:38 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 25-10-2019 09:53, Andy Shevchenko wrote:
> >>> On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote:
> >>>> This patchset introduces additional regulator driver for Intel Cherry
> >>>> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
> >>>> PMIC, which is used to instantiate this regulator.
> >>>>
> >>>> Regulator support for this PMIC was present in kernel release from Intel
> >>>> targeted Aero platform, but was not entirely ported upstream and has
> >>>> been omitted in mainline kernel releases. Consecutively, absence of
> >>>> regulator caused the SD Card interface not to be provided with Vqcc
> >>>> voltage source needed to operate with UHS-I cards.
> >>>>
> >>>> Following patches are addessing this issue and making sd card interface
> >>>> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
> >>>> of the SD Card interface in consumers and exposes optional "vqmmc"
> >>>> voltage source, which mmc driver uses to switch signalling voltages
> >>>> between 1.8V and 3.3V.
> >>>>
> >>>> This set contains of 2 patches: one is implementing the regulator driver
> >>>> (based on a non upstreamed version from Intel Aero), and another patch
> >>>> registers this driver as mfd cell in exising Whiskey Cove PMIC driver.
> >>>
> >>> Thank you.
> >>> Hans, Cc'ed, has quite interested in these kind of patches.
> >>> Am I right, Hans?
> >>
> >> Yes since I do a lot of work on Bay and Cherry Trail hw enablement I'm
> >> always interested in CHT specific patches.
> >>
> >> Overall this series looks good (from a high level view I did not
> >> do a detailed review) but I wonder if we really want to export all the
> >> regulators when we just need the vsdio one?
> >
> > I thought about this point, and actually came to a personal conclusion
> > that if I do this as a new driver - then it is better to list all
> > possible regulators, creating some sort of "skeleton" which people
> > could then work on if need be. I do agree that at the present moment
> > the one regulator which is exposed is the one for vsdio, but listing
> > all possibilities should not hurt. This was my motivation to put them
> > all into the driver on the first place.
> >
> > If you believe additional regulator elements should be removed from
> > this version of the driver - I can clean them up and come up with v2
> > here.
> >
> >>
> >> Most regulators are controlled by either the P-Unit inside the CHT SoC
> >> or through ACPI OpRegion accesses.  Luckily the regulator subsys does not
> >> expose a sysfs interface for users to directly poke regulators, but this will
> >> still make it somewhat easier for users to poke regulators which they should
> >> leave alone.
> >
> > Agree, this is a valid point. But honestly I would really be surprised
> > if a user would directly touch something which can burn his silicon to
> > pieces. Regulators are usually not approached by users; unless they
> > are really HW engineers and know what they are doing.
>
> True.
>
> Hmm, I do just realize that the regulator subsystem turns off regulators
> which are not in use from its pov, which would be kinda bad here.

Interesting point, this I did not know.

>
> It does this when the kernel is done initializing / switches to the
> initrd or directly to the rootfs. Its been a while since I've last touched
> this. Have you tried building the regulator driver (and mfd stuff and i2c
> controller, etc.) into the kernel, so that the regulators work without modules?

This is the first thing that I've tested - I've selected the new
regulator as built-in, and it is fully operable.

>
> I'm afraid that if you do that they will all get turned off, with expecting
> results... Although I guess the regulator subsystem may skip this step for
> regulators without any constrains, to be sure I would have to re-read the code.

I guess (although to 100% sure), that this is exactly the reason why
this regulator remains operable - it does have neither source nor
constraints, therefore it remains enabled.

>
> Anyways this is a serious concern which we need to resolve.
>
> >> Andrey, may I ask which device you are testing this on?
> >
> > Sure, I use the original Intel Aero board. It used to have an official
> > image from Aero team with a heavily patched 4.4.y kernel, but when I
> > decided to have this updated to the latest stable branch - I've faced
> > the issue of missing core functionality, which led me to this patch.
>
> Ah I see, the reason I was asking is because I only know of three CHT
> devices using a Whiskey Cove PMIC (instead of the AXP288, Crystal Cove or
> TI one): the GPD Win, GPD pocket and one of the Lenovo Book versions
> (the one with the CHT SoC).

I've stumbled upon those platforms, specifically when I was searching
for information about the sdmmc interface. It looks like that they all
are affected by this missing vqmmc piece, since a lot of people have
raised complaints some SD Cards are not recognized.

>
> Lets say I want to test this patch-set on one of the GPD devices,
> I guess I need an UHS-I capable card and then what should I see
> in sysfs for the mmc settings ?

This is rather trivial: with the absence of 1.8v on the interface,
UHS-I cards are failing during the speed switch sped in
initialization. If you would plug in the UHS-I SD Card in the slot,
then driver outputs the error message in dmesg:
mmc2: error -110 whilst initialising SD card

Error codes returned are either ETIMEDOUT or EILSEQ, depending on how
the SD Card controller handles improper voltages on interface.

With the patch applied, when UHS-I card is inserted, following message
in recorded in dmesg:
mmc2: new ultra high speed SDR104 SDHC card at address aaaa

SDR104 mode is only for UHS-capable cards (Grade I and above); that
message is for example recorded when SanDisk Extreme 32GB card is
inserted. I've also tested different other cards from Verbatim,
Swissbit and SanDisk, and they all were recognized OK.

BTW, as a side note: there is a way to disable 1.8v switching in mmc
sub-system via quirks2, this makes all UHS-I cards to operate as High
Speed (HS) cards at 3.3v IO level. This is rather a hack, but people
using the CHT-based products tend to do exactly that (or at least this
is a common solution proposed in Internet).

As for sysfs attributes: I actually didn't look into those to really
tell you which values for which parameters would definitely
distinguish UHS cards and settings from HS ones.

>
> Regards,
>
> Hans
>

-- 
Regards,
Andrey.

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 588 bytes --]

On Fri, Oct 25, 2019 at 12:45:30PM +0200, Hans de Goede wrote:

> Hmm, I do just realize that the regulator subsystem turns off regulators
> which are not in use from its pov, which would be kinda bad here.

It will only do this for regulators which have constraints which
enable the kernel to change the power state of the regulator, the
regulator core will never make any change to hardware that was
not explicitly permitted by constraints so should be perfectly
safe unless constraints are provided in which case it's up to
whoever provides the constraints to make sure they are safe.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Hans de Goede]

Hi,

On 25-10-2019 14:05, Mark Brown wrote:
> On Fri, Oct 25, 2019 at 12:45:30PM +0200, Hans de Goede wrote:
> 
>> Hmm, I do just realize that the regulator subsystem turns off regulators
>> which are not in use from its pov, which would be kinda bad here.
> 
> It will only do this for regulators which have constraints which
> enable the kernel to change the power state of the regulator, the
> regulator core will never make any change to hardware that was
> not explicitly permitted by constraints so should be perfectly
> safe unless constraints are provided in which case it's up to
> whoever provides the constraints to make sure they are safe.

Ok, thank you for the explanation.

Then I believe it is fine to keep the driver as is, with support
for all regulators in the PMIC.

Regards,

Hans

Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 11:38:52AM +0200, Hans de Goede wrote:
> On 25-10-2019 09:53, Andy Shevchenko wrote:
> > On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote:
> > > This patchset introduces additional regulator driver for Intel Cherry
> > > Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
> > > PMIC, which is used to instantiate this regulator.
> > > 
> > > Regulator support for this PMIC was present in kernel release from Intel
> > > targeted Aero platform, but was not entirely ported upstream and has
> > > been omitted in mainline kernel releases. Consecutively, absence of
> > > regulator caused the SD Card interface not to be provided with Vqcc
> > > voltage source needed to operate with UHS-I cards.
> > > 
> > > Following patches are addessing this issue and making sd card interface
> > > to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
> > > of the SD Card interface in consumers and exposes optional "vqmmc"
> > > voltage source, which mmc driver uses to switch signalling voltages
> > > between 1.8V and 3.3V.
> > > 
> > > This set contains of 2 patches: one is implementing the regulator driver
> > > (based on a non upstreamed version from Intel Aero), and another patch
> > > registers this driver as mfd cell in exising Whiskey Cove PMIC driver.
> > 
> > Thank you.
> > Hans, Cc'ed, has quite interested in these kind of patches.
> > Am I right, Hans?
> 
> Yes since I do a lot of work on Bay and Cherry Trail hw enablement I'm
> always interested in CHT specific patches.
> 
> Overall this series looks good (from a high level view I did not
> do a detailed review) but I wonder if we really want to export all the
> regulators when we just need the vsdio one?
> 
> Most regulators are controlled by either the P-Unit inside the CHT SoC
> or through ACPI OpRegion accesses.  Luckily the regulator subsys does not
> expose a sysfs interface for users to directly poke regulators, but this will
> still make it somewhat easier for users to poke regulators which they should
> leave alone.
> 
> Note I'm not saying this is wrong, having support for all regulators in place
> in case we need it in the future is also kinda nice. OTOH if we just need the
> one now, maybe we should just support the one for now ?
> 
> Andy do you have an opinion on this?

Usually on such patches I have two (a bit contradicted) wishes:
- be as little as needed
- for sake of documentation purposes keep as many definitions as we can

But you know the dangling code / definitions not accepted by all maintainers.

So, that said, I have no strong opinion here, basically rely on maintainer's
point of view.

Perhaps also good idea is to put a reference as URL to the published code from
which this has been derived.

> 
> Andrey, may I ask which device you are testing this on?
> 
> Anyways overall good work, thank you for doing this!

+1 to the cheering up!

-- 
With Best Regards,
Andy Shevchenko