[PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Furquan Shaikh]

Until now, the regulator framework assumed that regulators are being
passed in using device tree(OF) only. However, with the recent change
to add ACPI fixed regulator, it is necessary to have all the regulator
init data and regulation constraints parsing performed for ACPI
regulators as well. This is important to ensure that drivers can
properly enable and disable the regulators.

Since regulator properties remain the same across OF and ACPI
regulators, this series of patches provides common routine for
obtaining regulation constraints from device tree and ACPI nodes. In
order to support these common routines, certain device property
functions had to be exported to make the node parsing logic generic
across OF and ACPI.

Since the changes in this patch series are cross-system, it will
require some amount of co-ordination to ensure that it can be applied
at once.

Dmitry Torokhov (3):
  ACPI / property: have acpi_get_next_subnode take fwnode_handle
  device property: introduce fwnode_for_each_child()
  device property: introduce fwnode_get_named_child_node()

Furquan Shaikh (4):
  drivers/regulator: Rename of_map_mode to map_mode in regulator desc
  device property: Export dev_fwnode
  drivers/gpio: Add and export gpiod_lookup[_index]
  drivers/regulator: Initialize regulator init data for ACPI regulators

 drivers/acpi/property.c                 |   8 +-
 drivers/base/property.c                 |  61 ++++++++---
 drivers/gpio/gpiolib.c                  |  75 ++++++++++---
 drivers/regulator/Makefile              |   2 +-
 drivers/regulator/fixed.c               | 100 ++++++-----------
 drivers/regulator/internal.h            |   3 +
 drivers/regulator/max77802-regulator.c  |  12 +-
 drivers/regulator/of_regulator.c        | 156 +-------------------------
 drivers/regulator/qcom_spmi-regulator.c |   2 +-
 drivers/regulator/regulator_props.c     | 189 ++++++++++++++++++++++++++++++++
 drivers/regulator/twl-regulator.c       |   6 +-
 drivers/regulator/twl6030-regulator.c   |   2 +-
 include/linux/acpi.h                    |   5 +-
 include/linux/fwnode.h                  |  10 ++
 include/linux/gpio/consumer.h           |  21 ++++
 include/linux/property.h                |   1 +
 include/linux/regulator/driver.h        |   5 +-
 17 files changed, 386 insertions(+), 272 deletions(-)
 create mode 100644 drivers/regulator/regulator_props.c

-- 
2.11.0.483.g087da7b7c-goog

[PATCH 1/7] drivers/regulator: Rename of_map_mode to map_mode in regulator desc

[Furquan Shaikh]

In order to extend regulator support to ACPI, it is necessary that all
the regulator descriptor structure members are generic and not tied to
just DeviceTree (OF). Thus, rename of_map_mode to map_mode to allow
drivers using ACPI regulators to define required map_mode callback.

Signed-off-by: Furquan Shaikh <furquan@chromium.org>
---
 drivers/regulator/max77802-regulator.c  | 12 ++++++------
 drivers/regulator/of_regulator.c        |  8 ++++----
 drivers/regulator/qcom_spmi-regulator.c |  2 +-
 drivers/regulator/twl-regulator.c       |  6 +++---
 drivers/regulator/twl6030-regulator.c   |  2 +-
 include/linux/regulator/driver.h        |  5 +++--
 6 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/regulator/max77802-regulator.c b/drivers/regulator/max77802-regulator.c
index 1d3539324d9a..ccacc492f6ff 100644
--- a/drivers/regulator/max77802-regulator.c
+++ b/drivers/regulator/max77802-regulator.c
@@ -379,7 +379,7 @@ static struct regulator_ops max77802_buck_dvs_ops = {
 	.vsel_mask	= MAX77802_VSEL_MASK,				\
 	.enable_reg	= MAX77802_REG_LDO1CTRL1 + num - 1,		\
 	.enable_mask	= MAX77802_OPMODE_MASK << MAX77802_OPMODE_SHIFT_LDO, \
-	.of_map_mode	= max77802_map_mode,				\
+	.map_mode	= max77802_map_mode,				\
 }
 
 /* LDOs 1, 2, 8, 15, 17, 27, 30, 35 */
@@ -400,7 +400,7 @@ static struct regulator_ops max77802_buck_dvs_ops = {
 	.vsel_mask	= MAX77802_VSEL_MASK,				\
 	.enable_reg	= MAX77802_REG_LDO1CTRL1 + num - 1,		\
 	.enable_mask	= MAX77802_OPMODE_MASK << MAX77802_OPMODE_SHIFT_LDO, \
-	.of_map_mode	= max77802_map_mode,				\
+	.map_mode	= max77802_map_mode,				\
 }
 
 /* BUCKs 1, 6 */
@@ -421,7 +421,7 @@ static struct regulator_ops max77802_buck_dvs_ops = {
 	.vsel_mask	= MAX77802_DVS_VSEL_MASK,			\
 	.enable_reg	= MAX77802_REG_BUCK ## num ## CTRL,		\
 	.enable_mask	= MAX77802_OPMODE_MASK,				\
-	.of_map_mode	= max77802_map_mode,				\
+	.map_mode	= max77802_map_mode,				\
 }
 
 /* BUCKS 2-4 */
@@ -443,7 +443,7 @@ static struct regulator_ops max77802_buck_dvs_ops = {
 	.enable_reg	= MAX77802_REG_BUCK ## num ## CTRL1,		\
 	.enable_mask	= MAX77802_OPMODE_MASK <<			\
 				MAX77802_OPMODE_BUCK234_SHIFT,		\
-	.of_map_mode	= max77802_map_mode,				\
+	.map_mode	= max77802_map_mode,				\
 }
 
 /* BUCK 5 */
@@ -464,7 +464,7 @@ static struct regulator_ops max77802_buck_dvs_ops = {
 	.vsel_mask	= MAX77802_VSEL_MASK,				\
 	.enable_reg	= MAX77802_REG_BUCK5CTRL,			\
 	.enable_mask	= MAX77802_OPMODE_MASK,				\
-	.of_map_mode	= max77802_map_mode,				\
+	.map_mode	= max77802_map_mode,				\
 }
 
 /* BUCKs 7-10 */
@@ -485,7 +485,7 @@ static struct regulator_ops max77802_buck_dvs_ops = {
 	.vsel_mask	= MAX77802_VSEL_MASK,				\
 	.enable_reg	= MAX77802_REG_BUCK7CTRL + (num - 7) * 3,	\
 	.enable_mask	= MAX77802_OPMODE_MASK,				\
-	.of_map_mode	= max77802_map_mode,				\
+	.map_mode	= max77802_map_mode,				\
 }
 
 static const struct regulator_desc regulators[] = {
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 4f613ec99500..13d4dc2c287e 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -100,8 +100,8 @@ static void of_get_regulation_constraints(struct device_node *np,
 	}
 
 	if (!of_property_read_u32(np, "regulator-initial-mode", &pval)) {
-		if (desc && desc->of_map_mode) {
-			ret = desc->of_map_mode(pval);
+		if (desc && desc->map_mode) {
+			ret = desc->map_mode(pval);
 			if (ret == -EINVAL)
 				pr_err("%s: invalid mode %u\n", np->name, pval);
 			else
@@ -139,8 +139,8 @@ static void of_get_regulation_constraints(struct device_node *np,
 
 		if (!of_property_read_u32(suspend_np, "regulator-mode",
 					  &pval)) {
-			if (desc && desc->of_map_mode) {
-				ret = desc->of_map_mode(pval);
+			if (desc && desc->map_mode) {
+				ret = desc->map_mode(pval);
 				if (ret == -EINVAL)
 					pr_err("%s: invalid mode %u\n",
 					       np->name, pval);
diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index 16c5f84e06a7..54fe4aadd350 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -1679,7 +1679,7 @@ static int qcom_spmi_regulator_probe(struct platform_device *pdev)
 		vreg->desc.supply_name = reg->supply;
 		vreg->desc.of_match = reg->name;
 		vreg->desc.of_parse_cb = spmi_regulator_of_parse;
-		vreg->desc.of_map_mode = spmi_regulator_of_map_mode;
+		vreg->desc.map_mode = spmi_regulator_of_map_mode;
 
 		ret = spmi_regulator_match(vreg, reg->force_type);
 		if (ret)
diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 6c9ec84121bd..023aa28c8a0f 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -466,7 +466,7 @@ static const struct twlreg_info TWL4030_INFO_##label = { \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
 		.enable_time = turnon_delay, \
-		.of_map_mode = twl4030reg_map_mode, \
+		.map_mode = twl4030reg_map_mode, \
 		}, \
 	}
 
@@ -482,7 +482,7 @@ static const struct twlreg_info TWL4030_INFO_##label = { \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
 		.enable_time = turnon_delay, \
-		.of_map_mode = twl4030reg_map_mode, \
+		.map_mode = twl4030reg_map_mode, \
 		}, \
 	}
 
@@ -501,7 +501,7 @@ static const struct twlreg_info TWLFIXED_INFO_##label = { \
 		.owner = THIS_MODULE, \
 		.min_uV = mVolts * 1000, \
 		.enable_time = turnon_delay, \
-		.of_map_mode = twl4030reg_map_mode, \
+		.map_mode = twl4030reg_map_mode, \
 		}, \
 	}
 
diff --git a/drivers/regulator/twl6030-regulator.c b/drivers/regulator/twl6030-regulator.c
index 4864b9d742c0..d7be72869f52 100644
--- a/drivers/regulator/twl6030-regulator.c
+++ b/drivers/regulator/twl6030-regulator.c
@@ -569,7 +569,7 @@ static const struct twlreg_info TWLFIXED_INFO_##label = { \
 		.owner = THIS_MODULE, \
 		.min_uV = mVolts * 1000, \
 		.enable_time = turnon_delay, \
-		.of_map_mode = NULL, \
+		.map_mode = NULL, \
 		}, \
 	}
 
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index dac8e7b16bc6..b50592a6f050 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -296,7 +296,8 @@ enum regulator_type {
  * @enable_time: Time taken for initial enable of regulator (in uS).
  * @off_on_delay: guard time (in uS), before re-enabling a regulator
  *
- * @of_map_mode: Maps a hardware mode defined in a DeviceTree to a standard mode
+ * @map_mode: Maps a hardware mode defined in device properties to a standard
+ *            mode.
  */
 struct regulator_desc {
 	const char *name;
@@ -350,7 +351,7 @@ struct regulator_desc {
 
 	unsigned int off_on_delay;
 
-	unsigned int (*of_map_mode)(unsigned int mode);
+	unsigned int (*map_mode)(unsigned int mode);
 };
 
 /**
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 2/7] ACPI / property: have acpi_get_next_subnode take fwnode_handle

[Furquan Shaikh]

From: Dmitry Torokhov <dtor@chromium.org>

In preparation to introducing fwnode_for_each_chil() iterator we need
acpi_get_next_subnode take fwnode_handle argument instead of dev.

Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Furquan Shaikh <furquan@chromium.org>
---
 drivers/acpi/property.c | 8 +++++---
 drivers/base/property.c | 2 +-
 include/linux/acpi.h    | 5 +++--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 3afddcd834ef..b87e822e3a51 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -866,15 +866,17 @@ int acpi_node_prop_read(struct fwnode_handle *fwnode,  const char *propname,
 
 /**
  * acpi_get_next_subnode - Return the next child node handle for a device.
- * @dev: Device to find the next child node for.
+ * @node: node to find the next child node for. Must be ACPI device node,
+ *	not data node.
  * @child: Handle to one of the device's child nodes or a null handle.
  */
-struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
+struct fwnode_handle *acpi_get_next_subnode(struct fwnode_handle *node,
 					    struct fwnode_handle *child)
 {
-	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct acpi_device *adev;
 	struct list_head *head, *next;
 
+	adev = to_acpi_device_node(node);
 	if (!adev)
 		return NULL;
 
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 43a36d68c3fd..f2f8220534c4 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -881,7 +881,7 @@ struct fwnode_handle *device_get_next_child_node(struct device *dev,
 		if (node)
 			return &node->fwnode;
 	} else if (IS_ENABLED(CONFIG_ACPI)) {
-		return acpi_get_next_subnode(dev, child);
+		return acpi_get_next_subnode(dev->fwnode, child);
 	}
 	return NULL;
 }
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 5b36974ed60a..0331a9dbe4f2 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -999,7 +999,7 @@ int acpi_node_prop_read(struct fwnode_handle *fwnode, const char *propname,
 int acpi_dev_prop_read(struct acpi_device *adev, const char *propname,
 		       enum dev_prop_type proptype, void *val, size_t nval);
 
-struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
+struct fwnode_handle *acpi_get_next_subnode(struct fwnode_handle *node,
 					    struct fwnode_handle *subnode);
 
 struct acpi_probe_entry;
@@ -1117,7 +1117,8 @@ static inline int acpi_dev_prop_read(struct acpi_device *adev,
 	return -ENXIO;
 }
 
-static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
+static inline struct fwnode_handle *acpi_get_next_subnode(
+						struct fwnode_handle *node,
 						struct fwnode_handle *subnode)
 {
 	return NULL;
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 3/7] device property: introduce fwnode_for_each_child()

[Furquan Shaikh]

From: Dmitry Torokhov <dtor@chromium.org>

Generic code, that wishes to work with ACPI, device tree, and pset
properties needs iterator that can work with fwnode_handle.

Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Furquan Shaikh <furquan@chromium.org>
---
 drivers/base/property.c | 33 +++++++++++++++++++++++----------
 include/linux/fwnode.h  |  7 +++++++
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index f2f8220534c4..fbb05a4a7595 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -867,6 +867,28 @@ int device_add_properties(struct device *dev, struct property_entry *properties)
 EXPORT_SYMBOL_GPL(device_add_properties);
 
 /**
+ * fwnode_get_next_child_node - Return the next child node handle for a device
+ * @node: Node to find the next child node for.
+ * @child: Handle to one of the device's child nodes or a null handle.
+ */
+struct fwnode_handle *fwnode_get_next_child_node(struct fwnode_handle *node,
+						 struct fwnode_handle *child)
+{
+	if (IS_ENABLED(CONFIG_OF) && is_of_node(node)) {
+		struct device_node *np;
+
+		np = of_get_next_available_child(to_of_node(node),
+						   to_of_node(child));
+		if (np)
+			return &np->fwnode;
+	} else if (IS_ENABLED(CONFIG_ACPI) && is_acpi_node(node)) {
+		return acpi_get_next_subnode(node, child);
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_next_child_node);
+
+/**
  * device_get_next_child_node - Return the next child node handle for a device
  * @dev: Device to find the next child node for.
  * @child: Handle to one of the device's child nodes or a null handle.
@@ -874,16 +896,7 @@ EXPORT_SYMBOL_GPL(device_add_properties);
 struct fwnode_handle *device_get_next_child_node(struct device *dev,
 						 struct fwnode_handle *child)
 {
-	if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
-		struct device_node *node;
-
-		node = of_get_next_available_child(dev->of_node, to_of_node(child));
-		if (node)
-			return &node->fwnode;
-	} else if (IS_ENABLED(CONFIG_ACPI)) {
-		return acpi_get_next_subnode(dev->fwnode, child);
-	}
-	return NULL;
+	return fwnode_get_next_child_node(dev_fwnode(dev), child);
 }
 EXPORT_SYMBOL_GPL(device_get_next_child_node);
 
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 8bd28ce6d76e..b4efe2a088ae 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -27,4 +27,11 @@ struct fwnode_handle {
 	struct fwnode_handle *secondary;
 };
 
+struct fwnode_handle *fwnode_get_next_child_node(struct fwnode_handle *node,
+						 struct fwnode_handle *child);
+
+#define fwnode_for_each_child_node(node, child) \
+	for (child = fwnode_get_next_child_node(node, NULL); child; \
+	     child = fwnode_get_next_child_node(node, child))
+
 #endif
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 4/7] device property: introduce fwnode_get_named_child_node()

[Furquan Shaikh]

From: Dmitry Torokhov <dtor@chromium.org>

Generic code that works with ACPI, device tree, and pset properties may
want to fetch named child node of fwnode_handle.

Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Furquan Shaikh <furquan@chromium.org>
---
 drivers/base/property.c | 45 +++++++++++++++++++++++++++++----------------
 include/linux/fwnode.h  |  3 +++
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index fbb05a4a7595..3556c9fbdbf7 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -889,23 +889,11 @@ struct fwnode_handle *fwnode_get_next_child_node(struct fwnode_handle *node,
 EXPORT_SYMBOL_GPL(fwnode_get_next_child_node);
 
 /**
- * device_get_next_child_node - Return the next child node handle for a device
- * @dev: Device to find the next child node for.
- * @child: Handle to one of the device's child nodes or a null handle.
- */
-struct fwnode_handle *device_get_next_child_node(struct device *dev,
-						 struct fwnode_handle *child)
-{
-	return fwnode_get_next_child_node(dev_fwnode(dev), child);
-}
-EXPORT_SYMBOL_GPL(device_get_next_child_node);
-
-/**
- * device_get_named_child_node - Return first matching named child node handle
- * @dev: Device to find the named child node for.
+ * fwnode_get_named_child_node - Return first matching named child node handle
+ * @node: Node to find the named child node for.
  * @childname: String to match child node name against.
  */
-struct fwnode_handle *device_get_named_child_node(struct device *dev,
+struct fwnode_handle *fwnode_get_named_child_node(struct fwnode_handle *node,
 						  const char *childname)
 {
 	struct fwnode_handle *child;
@@ -914,7 +902,7 @@ struct fwnode_handle *device_get_named_child_node(struct device *dev,
 	 * Find first matching named child node of this device.
 	 * For ACPI this will be a data only sub-node.
 	 */
-	device_for_each_child_node(dev, child) {
+	fwnode_for_each_child_node(node, child) {
 		if (is_of_node(child)) {
 			if (!of_node_cmp(to_of_node(child)->name, childname))
 				return child;
@@ -926,6 +914,31 @@ struct fwnode_handle *device_get_named_child_node(struct device *dev,
 
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(fwnode_get_named_child_node);
+
+
+/**
+ * device_get_next_child_node - Return the next child node handle for a device
+ * @dev: Device to find the next child node for.
+ * @child: Handle to one of the device's child nodes or a null handle.
+ */
+struct fwnode_handle *device_get_next_child_node(struct device *dev,
+						 struct fwnode_handle *child)
+{
+	return fwnode_get_next_child_node(dev_fwnode(dev), child);
+}
+EXPORT_SYMBOL_GPL(device_get_next_child_node);
+
+/**
+ * device_get_named_child_node - Return first matching named child node handle
+ * @dev: Device to find the named child node for.
+ * @childname: String to match child node name against.
+ */
+struct fwnode_handle *device_get_named_child_node(struct device *dev,
+						  const char *childname)
+{
+	return fwnode_get_named_child_node(dev_fwnode(dev), childname);
+}
 EXPORT_SYMBOL_GPL(device_get_named_child_node);
 
 /**
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index b4efe2a088ae..d501e0b967bd 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -34,4 +34,7 @@ struct fwnode_handle *fwnode_get_next_child_node(struct fwnode_handle *node,
 	for (child = fwnode_get_next_child_node(node, NULL); child; \
 	     child = fwnode_get_next_child_node(node, child))
 
+struct fwnode_handle *fwnode_get_named_child_node(struct fwnode_handle *node,
+						  const char *childname);
+
 #endif
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 5/7] device property: Export dev_fwnode

[Furquan Shaikh]

Export dev_fwnode so that it can be used by other drivers like regulator.

Signed-off-by: Furquan Shaikh <furquan@chromium.org>
---
 drivers/base/property.c  | 3 ++-
 include/linux/property.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 3556c9fbdbf7..39eaeec0f0ea 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -182,11 +182,12 @@ static int pset_prop_read_string(struct property_set *pset,
 	return 0;
 }
 
-static inline struct fwnode_handle *dev_fwnode(struct device *dev)
+struct fwnode_handle *dev_fwnode(struct device *dev)
 {
 	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
 		&dev->of_node->fwnode : dev->fwnode;
 }
+EXPORT_SYMBOL_GPL(dev_fwnode);
 
 /**
  * device_property_present - check if a property of a device is present
diff --git a/include/linux/property.h b/include/linux/property.h
index 856e50b2140c..9e2b7e1416cd 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -49,6 +49,7 @@ int device_property_read_string(struct device *dev, const char *propname,
 int device_property_match_string(struct device *dev,
 				 const char *propname, const char *string);
 
+struct fwnode_handle *dev_fwnode(struct device *dev);
 bool fwnode_property_present(struct fwnode_handle *fwnode, const char *propname);
 int fwnode_property_read_u8_array(struct fwnode_handle *fwnode,
 				  const char *propname, u8 *val,
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 6/7] drivers/gpio: Add and export gpiod_lookup[_index]

[Furquan Shaikh]

Sometimes (as the case with fixed regulator) we only want to look up,
but not necessarily reserve right away, GPIO. gpiod_lookup() and
gpiod_lookup_by_index() allow us doing just that.

Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Furquan Shaikh <furquan@chromium.org>
---
 drivers/gpio/gpiolib.c        | 75 ++++++++++++++++++++++++++++++++++---------
 include/linux/gpio/consumer.h | 21 ++++++++++++
 2 files changed, 80 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a07ae9e37930..7f8ac6c229e8 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3165,6 +3165,22 @@ int gpiod_count(struct device *dev, const char *con_id)
 EXPORT_SYMBOL_GPL(gpiod_count);
 
 /**
+ * gpiod_lookup - look up a GPIO for a given GPIO function
+ * @dev:	GPIO consumer, can be NULL for system-global GPIOs
+ * @con_id:	function within the GPIO consumer
+ *
+ * Return the GPIO descriptor corresponding to the function con_id of device
+ * dev, -ENOENT if no GPIO has been assigned to the requested function, or
+ * another IS_ERR() code.
+ */
+struct gpio_desc *__must_check gpiod_lookup(struct device *dev,
+					    const char *con_id)
+{
+	return gpiod_lookup_index(dev, con_id, 0);
+}
+EXPORT_SYMBOL_GPL(gpiod_lookup);
+
+/**
  * gpiod_get - obtain a GPIO for a given GPIO function
  * @dev:	GPIO consumer, can be NULL for system-global GPIOs
  * @con_id:	function within the GPIO consumer
@@ -3241,26 +3257,19 @@ static int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 }
 
 /**
- * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
+ * gpiod_lookup_index - look up a GPIO from a multi-index GPIO function
  * @dev:	GPIO consumer, can be NULL for system-global GPIOs
  * @con_id:	function within the GPIO consumer
  * @idx:	index of the GPIO to obtain in the consumer
- * @flags:	optional GPIO initialization flags
- *
- * This variant of gpiod_get() allows to access GPIOs other than the first
- * defined one for functions that define several GPIOs.
  *
- * Return a valid GPIO descriptor, -ENOENT if no GPIO has been assigned to the
- * requested function and/or index, or another IS_ERR() code if an error
- * occurred while trying to acquire the GPIO.
+ * Return a valid GPIO descriptor, or -ENOENT if no GPIO has been assigned to
+ * the requested function and/or index, or another IS_ERR() code.
  */
-struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
-					       const char *con_id,
-					       unsigned int idx,
-					       enum gpiod_flags flags)
+struct gpio_desc *__must_check gpiod_lookup_index(struct device *dev,
+						  const char *con_id,
+						  unsigned int idx)
 {
 	struct gpio_desc *desc = NULL;
-	int status;
 	enum gpio_lookup_flags lookupflags = 0;
 
 	dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
@@ -3285,16 +3294,50 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 		desc = gpiod_find(dev, con_id, idx, &lookupflags);
 	}
 
-	if (IS_ERR(desc)) {
+	if (IS_ERR(desc))
 		dev_dbg(dev, "lookup for GPIO %s failed\n", con_id);
+
+	/*
+	 * Configure static flags based on lookup data (such as
+	 * "active low", "open drain", etc.)
+	 */
+	gpiod_configure_flags(desc, con_id, lookupflags, 0);
+
+	return desc;
+}
+EXPORT_SYMBOL_GPL(gpiod_lookup_index);
+
+/**
+ * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
+ * @dev:	GPIO consumer, can be NULL for system-global GPIOs
+ * @con_id:	function within the GPIO consumer
+ * @idx:	index of the GPIO to obtain in the consumer
+ * @flags:	optional GPIO initialization flags
+ *
+ * This variant of gpiod_get() allows to access GPIOs other than the first
+ * defined one for functions that define several GPIOs.
+ *
+ * Return a valid GPIO descriptor, -ENOENT if no GPIO has been assigned to the
+ * requested function and/or index, or another IS_ERR() code if an error
+ * occurred while trying to acquire the GPIO.
+ */
+struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
+					       const char *con_id,
+					       unsigned int idx,
+					       enum gpiod_flags flags)
+{
+	struct gpio_desc *desc;
+	int status;
+
+	desc = gpiod_lookup_index(dev, con_id, idx);
+	if (IS_ERR(desc))
 		return desc;
-	}
 
 	status = gpiod_request(desc, con_id);
 	if (status < 0)
 		return ERR_PTR(status);
 
-	status = gpiod_configure_flags(desc, con_id, lookupflags, flags);
+	status = gpiod_configure_flags(desc, con_id, 0, flags);
 	if (status < 0) {
 		dev_dbg(dev, "setup of GPIO %s failed\n", con_id);
 		gpiod_put(desc);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index fb0fde686cb1..8fcb638f5568 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -46,6 +46,13 @@ enum gpiod_flags {
 /* Return the number of GPIOs associated with a device / function */
 int gpiod_count(struct device *dev, const char *con_id);
 
+/* Look up GPIOs */
+struct gpio_desc *__must_check gpiod_lookup(struct device *dev,
+					    const char *con_id);
+struct gpio_desc *__must_check gpiod_lookup_index(struct device *dev,
+						  const char *con_id,
+						  unsigned int idx);
+
 /* Acquire and dispose GPIOs */
 struct gpio_desc *__must_check gpiod_get(struct device *dev,
 					 const char *con_id,
@@ -146,6 +153,20 @@ static inline int gpiod_count(struct device *dev, const char *con_id)
 	return 0;
 }
 
+static inline struct gpio_desc *__must_check gpiod_lookup(struct device *dev,
+							  const char *con_id)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline struct gpio_desc *__must_check
+gpiod_lookup_index(struct device *dev,
+		   const char *con_id,
+		   unsigned int idx)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline struct gpio_desc *__must_check gpiod_get(struct device *dev,
 						       const char *con_id,
 						       enum gpiod_flags flags)
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 7/7] drivers/regulator: Initialize regulator init data for ACPI regulators

[Furquan Shaikh]

Regulator init data and regulation constraints need to be setup by
ACPI regulators similar to OF regulators. This is required to ensure
that drivers can properly enable/disable the regulators.

Since, regulator properties remain the same across OF and ACPI
regulators, provide common routines for obtaining the regulation
constraints from device tree/ACPI node. Update fixed regulator driver
to use this newly added routine.

Signed-off-by: Furquan Shaikh <furquan@chromium.org>
---
 drivers/regulator/Makefile          |   2 +-
 drivers/regulator/fixed.c           | 100 +++++++------------
 drivers/regulator/internal.h        |   3 +
 drivers/regulator/of_regulator.c    | 156 +----------------------------
 drivers/regulator/regulator_props.c | 189 ++++++++++++++++++++++++++++++++++++
 5 files changed, 229 insertions(+), 221 deletions(-)
 create mode 100644 drivers/regulator/regulator_props.c

diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 14294692beb9..7701cb9e9bec 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -3,7 +3,7 @@
 #
 
 
-obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o helpers.o devres.o
+obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o helpers.o devres.o regulator_props.o
 obj-$(CONFIG_OF) += of_regulator.o
 obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
 obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index a43b0e8a438d..572f352095c3 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -34,39 +34,40 @@
 #include <linux/property.h>
 #include <linux/gpio/consumer.h>
 
+#include "internal.h"
+
 struct fixed_voltage_data {
 	struct regulator_desc desc;
 	struct regulator_dev *dev;
 };
 
-
 /**
- * of_get_fixed_voltage_config - extract fixed_voltage_config structure info
+ * reg_get_fixed_voltage_config - extract fixed_voltage_config structure info
  * @dev: device requesting for fixed_voltage_config
  * @desc: regulator description
  *
  * Populates fixed_voltage_config structure by extracting data from device
- * tree node, returns a pointer to the populated structure of NULL if memory
- * alloc fails.
+ * tree or ACPI node, returns a pointer to the populated structure or NULL if
+ * memory alloc fails.
  */
 static struct fixed_voltage_config *
-of_get_fixed_voltage_config(struct device *dev,
-			    const struct regulator_desc *desc)
+reg_fixed_voltage_get_config(struct device *dev,
+			const struct regulator_desc *desc)
 {
 	struct fixed_voltage_config *config;
-	struct device_node *np = dev->of_node;
 	struct regulator_init_data *init_data;
+	struct gpio_desc *gpiod;
 
-	config = devm_kzalloc(dev, sizeof(struct fixed_voltage_config),
-								 GFP_KERNEL);
+	config = devm_kzalloc(dev, sizeof(*config), GFP_KERNEL);
 	if (!config)
 		return ERR_PTR(-ENOMEM);
 
-	config->init_data = of_get_regulator_init_data(dev, dev->of_node, desc);
-	if (!config->init_data)
+	init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL);
+	if (!init_data)
 		return ERR_PTR(-EINVAL);
 
-	init_data = config->init_data;
+	device_get_regulation_constraints(dev_fwnode(dev), init_data, desc);
+
 	init_data->constraints.apply_uV = 0;
 
 	config->supply_name = init_data->constraints.name;
@@ -74,63 +75,35 @@ of_get_fixed_voltage_config(struct device *dev,
 		config->microvolts = init_data->constraints.min_uV;
 	} else {
 		dev_err(dev,
-			 "Fixed regulator specified with variable voltages\n");
+			"Fixed regulator specified with variable voltages\n");
 		return ERR_PTR(-EINVAL);
 	}
 
 	if (init_data->constraints.boot_on)
 		config->enabled_at_boot = true;
 
-	config->gpio = of_get_named_gpio(np, "gpio", 0);
-	if ((config->gpio < 0) && (config->gpio != -ENOENT))
-		return ERR_PTR(config->gpio);
-
-	of_property_read_u32(np, "startup-delay-us", &config->startup_delay);
+	gpiod = gpiod_lookup(dev, NULL);
 
-	config->enable_high = of_property_read_bool(np, "enable-active-high");
-	config->gpio_is_open_drain = of_property_read_bool(np,
-							   "gpio-open-drain");
+	if (gpiod == ERR_PTR(-EPROBE_DEFER))
+		return ERR_PTR(-EPROBE_DEFER);
 
-	if (of_find_property(np, "vin-supply", NULL))
-		config->input_supply = "vin";
+	if (!IS_ERR(gpiod))
+		config->gpio = desc_to_gpio(gpiod);
+	else
+		config->gpio = -1;
 
-	return config;
-}
+	device_property_read_u32(dev, "startup-delay-us",
+				&config->startup_delay);
 
-/**
- * acpi_get_fixed_voltage_config - extract fixed_voltage_config structure info
- * @dev: device requesting for fixed_voltage_config
- * @desc: regulator description
- *
- * Populates fixed_voltage_config structure by extracting data through ACPI
- * interface, returns a pointer to the populated structure of NULL if memory
- * alloc fails.
- */
-static struct fixed_voltage_config *
-acpi_get_fixed_voltage_config(struct device *dev,
-			      const struct regulator_desc *desc)
-{
-	struct fixed_voltage_config *config;
-	const char *supply_name;
-	struct gpio_desc *gpiod;
-	int ret;
-
-	config = devm_kzalloc(dev, sizeof(*config), GFP_KERNEL);
-	if (!config)
-		return ERR_PTR(-ENOMEM);
-
-	ret = device_property_read_string(dev, "supply-name", &supply_name);
-	if (!ret)
-		config->supply_name = supply_name;
-
-	gpiod = gpiod_get(dev, "gpio", GPIOD_ASIS);
-	if (IS_ERR(gpiod))
-		return ERR_PTR(-ENODEV);
-
-	config->gpio = desc_to_gpio(gpiod);
 	config->enable_high = device_property_read_bool(dev,
 							"enable-active-high");
-	gpiod_put(gpiod);
+	config->gpio_is_open_drain = device_property_read_bool(dev,
+							"gpio-open-drain");
+
+	if (device_property_present(dev, "vin-supply"))
+		config->input_supply = "vin";
+
+	config->init_data = init_data;
 
 	return config;
 }
@@ -150,14 +123,9 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 	if (!drvdata)
 		return -ENOMEM;
 
-	if (pdev->dev.of_node) {
-		config = of_get_fixed_voltage_config(&pdev->dev,
-						     &drvdata->desc);
-		if (IS_ERR(config))
-			return PTR_ERR(config);
-	} else if (ACPI_HANDLE(&pdev->dev)) {
-		config = acpi_get_fixed_voltage_config(&pdev->dev,
-						       &drvdata->desc);
+	if (pdev->dev.of_node || ACPI_HANDLE(&pdev->dev)) {
+		config = reg_fixed_voltage_get_config(&pdev->dev,
+						      &drvdata->desc);
 		if (IS_ERR(config))
 			return PTR_ERR(config);
 	} else {
@@ -198,7 +166,7 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 
 	if (gpio_is_valid(config->gpio)) {
 		cfg.ena_gpio = config->gpio;
-		if (pdev->dev.of_node)
+		if (pdev->dev.of_node || ACPI_HANDLE(&pdev->dev))
 			cfg.ena_gpio_initialized = true;
 	}
 	cfg.ena_gpio_invert = !config->enable_high;
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index c74ac8734023..3a699b2cbea9 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -51,4 +51,7 @@ regulator_of_get_init_data(struct device *dev,
 }
 #endif
 
+void device_get_regulation_constraints(struct fwnode_handle *fwnode,
+					struct regulator_init_data *init_data,
+					const struct regulator_desc *desc);
 #endif
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 13d4dc2c287e..0ff5259c34f0 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -19,159 +19,6 @@
 
 #include "internal.h"
 
-static const char *const regulator_states[PM_SUSPEND_MAX + 1] = {
-	[PM_SUSPEND_MEM]	= "regulator-state-mem",
-	[PM_SUSPEND_MAX]	= "regulator-state-disk",
-};
-
-static void of_get_regulation_constraints(struct device_node *np,
-					struct regulator_init_data **init_data,
-					const struct regulator_desc *desc)
-{
-	struct regulation_constraints *constraints = &(*init_data)->constraints;
-	struct regulator_state *suspend_state;
-	struct device_node *suspend_np;
-	int ret, i;
-	u32 pval;
-
-	constraints->name = of_get_property(np, "regulator-name", NULL);
-
-	if (!of_property_read_u32(np, "regulator-min-microvolt", &pval))
-		constraints->min_uV = pval;
-
-	if (!of_property_read_u32(np, "regulator-max-microvolt", &pval))
-		constraints->max_uV = pval;
-
-	/* Voltage change possible? */
-	if (constraints->min_uV != constraints->max_uV)
-		constraints->valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
-
-	/* Do we have a voltage range, if so try to apply it? */
-	if (constraints->min_uV && constraints->max_uV)
-		constraints->apply_uV = true;
-
-	if (!of_property_read_u32(np, "regulator-microvolt-offset", &pval))
-		constraints->uV_offset = pval;
-	if (!of_property_read_u32(np, "regulator-min-microamp", &pval))
-		constraints->min_uA = pval;
-	if (!of_property_read_u32(np, "regulator-max-microamp", &pval))
-		constraints->max_uA = pval;
-
-	if (!of_property_read_u32(np, "regulator-input-current-limit-microamp",
-				  &pval))
-		constraints->ilim_uA = pval;
-
-	/* Current change possible? */
-	if (constraints->min_uA != constraints->max_uA)
-		constraints->valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
-
-	constraints->boot_on = of_property_read_bool(np, "regulator-boot-on");
-	constraints->always_on = of_property_read_bool(np, "regulator-always-on");
-	if (!constraints->always_on) /* status change should be possible. */
-		constraints->valid_ops_mask |= REGULATOR_CHANGE_STATUS;
-
-	constraints->pull_down = of_property_read_bool(np, "regulator-pull-down");
-
-	if (of_property_read_bool(np, "regulator-allow-bypass"))
-		constraints->valid_ops_mask |= REGULATOR_CHANGE_BYPASS;
-
-	if (of_property_read_bool(np, "regulator-allow-set-load"))
-		constraints->valid_ops_mask |= REGULATOR_CHANGE_DRMS;
-
-	ret = of_property_read_u32(np, "regulator-ramp-delay", &pval);
-	if (!ret) {
-		if (pval)
-			constraints->ramp_delay = pval;
-		else
-			constraints->ramp_disable = true;
-	}
-
-	ret = of_property_read_u32(np, "regulator-enable-ramp-delay", &pval);
-	if (!ret)
-		constraints->enable_time = pval;
-
-	constraints->soft_start = of_property_read_bool(np,
-					"regulator-soft-start");
-	ret = of_property_read_u32(np, "regulator-active-discharge", &pval);
-	if (!ret) {
-		constraints->active_discharge =
-				(pval) ? REGULATOR_ACTIVE_DISCHARGE_ENABLE :
-					REGULATOR_ACTIVE_DISCHARGE_DISABLE;
-	}
-
-	if (!of_property_read_u32(np, "regulator-initial-mode", &pval)) {
-		if (desc && desc->map_mode) {
-			ret = desc->map_mode(pval);
-			if (ret == -EINVAL)
-				pr_err("%s: invalid mode %u\n", np->name, pval);
-			else
-				constraints->initial_mode = ret;
-		} else {
-			pr_warn("%s: mapping for mode %d not defined\n",
-				np->name, pval);
-		}
-	}
-
-	if (!of_property_read_u32(np, "regulator-system-load", &pval))
-		constraints->system_load = pval;
-
-	constraints->over_current_protection = of_property_read_bool(np,
-					"regulator-over-current-protection");
-
-	for (i = 0; i < ARRAY_SIZE(regulator_states); i++) {
-		switch (i) {
-		case PM_SUSPEND_MEM:
-			suspend_state = &constraints->state_mem;
-			break;
-		case PM_SUSPEND_MAX:
-			suspend_state = &constraints->state_disk;
-			break;
-		case PM_SUSPEND_ON:
-		case PM_SUSPEND_FREEZE:
-		case PM_SUSPEND_STANDBY:
-		default:
-			continue;
-		}
-
-		suspend_np = of_get_child_by_name(np, regulator_states[i]);
-		if (!suspend_np || !suspend_state)
-			continue;
-
-		if (!of_property_read_u32(suspend_np, "regulator-mode",
-					  &pval)) {
-			if (desc && desc->map_mode) {
-				ret = desc->map_mode(pval);
-				if (ret == -EINVAL)
-					pr_err("%s: invalid mode %u\n",
-					       np->name, pval);
-				else
-					suspend_state->mode = ret;
-			} else {
-				pr_warn("%s: mapping for mode %d not defined\n",
-					np->name, pval);
-			}
-		}
-
-		if (of_property_read_bool(suspend_np,
-					"regulator-on-in-suspend"))
-			suspend_state->enabled = true;
-		else if (of_property_read_bool(suspend_np,
-					"regulator-off-in-suspend"))
-			suspend_state->disabled = true;
-
-		if (!of_property_read_u32(suspend_np,
-					"regulator-suspend-microvolt", &pval))
-			suspend_state->uV = pval;
-
-		if (i == PM_SUSPEND_MEM)
-			constraints->initial_state = PM_SUSPEND_MEM;
-
-		of_node_put(suspend_np);
-		suspend_state = NULL;
-		suspend_np = NULL;
-	}
-}
-
 /**
  * of_get_regulator_init_data - extract regulator_init_data structure info
  * @dev: device requesting for regulator_init_data
@@ -195,7 +42,8 @@ struct regulator_init_data *of_get_regulator_init_data(struct device *dev,
 	if (!init_data)
 		return NULL; /* Out of memory? */
 
-	of_get_regulation_constraints(node, &init_data, desc);
+	device_get_regulation_constraints(&node->fwnode, init_data, desc);
+
 	return init_data;
 }
 EXPORT_SYMBOL_GPL(of_get_regulator_init_data);
diff --git a/drivers/regulator/regulator_props.c b/drivers/regulator/regulator_props.c
new file mode 100644
index 000000000000..65e78ad1f494
--- /dev/null
+++ b/drivers/regulator/regulator_props.c
@@ -0,0 +1,189 @@
+/*
+ * ACPI helpers for regulator framework
+ *
+ * Copyright (C) 2011 Texas Instruments, Inc
+ * Copyright (C) 2016 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/export.h>
+#include <linux/of.h>
+#include <linux/property.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+static const char *const regulator_states[PM_SUSPEND_MAX + 1] = {
+	[PM_SUSPEND_MEM]	= "regulator-state-mem",
+	[PM_SUSPEND_MAX]	= "regulator-state-disk",
+};
+
+/**
+ * device_get_regulation_constraints - extract regulation_constraints info
+ * @fwnode: fwnode of device requesting for regulation_constraints
+ * @init_data: regulator_init_data structure containing regulation_constraints
+ * @desc: regulator description
+ *
+ * Populates regulation_constraints structure by extracting data from device
+ * tree or ACPI node.
+ */
+void device_get_regulation_constraints(struct fwnode_handle *fwnode,
+					struct regulator_init_data *init_data,
+					const struct regulator_desc *desc)
+{
+	struct regulation_constraints *constraints = &init_data->constraints;
+	int ret, i;
+	u32 pval;
+	struct regulator_state *suspend_state = NULL;
+	struct fwnode_handle *suspend_fwnode;
+
+	fwnode_property_read_string(fwnode, "regulator-name",
+					&constraints->name);
+
+	if (!fwnode_property_read_u32(fwnode, "regulator-min-microvolt", &pval))
+		constraints->min_uV = pval;
+
+	if (!fwnode_property_read_u32(fwnode, "regulator-max-microvolt", &pval))
+		constraints->max_uV = pval;
+
+	/* Voltage change possible? */
+	if (constraints->min_uV != constraints->max_uV)
+		constraints->valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
+
+	/* Do we have a voltage range, if so try to apply it? */
+	if (constraints->min_uV && constraints->max_uV)
+		constraints->apply_uV = true;
+
+	if (!fwnode_property_read_u32(fwnode, "regulator-microvolt-offset",
+					&pval))
+		constraints->uV_offset = pval;
+	if (!fwnode_property_read_u32(fwnode, "regulator-min-microamp", &pval))
+		constraints->min_uA = pval;
+	if (!fwnode_property_read_u32(fwnode, "regulator-max-microamp", &pval))
+		constraints->max_uA = pval;
+
+	if (!fwnode_property_read_u32(fwnode,
+				"regulator-input-current-limit-microamp",
+				&pval))
+		constraints->ilim_uA = pval;
+
+	/* Current change possible? */
+	if (constraints->min_uA != constraints->max_uA)
+		constraints->valid_ops_mask |= REGULATOR_CHANGE_CURRENT;
+
+	constraints->boot_on = fwnode_property_read_bool(fwnode,
+							"regulator-boot-on");
+	constraints->always_on = fwnode_property_read_bool(fwnode,
+							"regulator-always-on");
+	if (!constraints->always_on) /* status change should be possible. */
+		constraints->valid_ops_mask |= REGULATOR_CHANGE_STATUS;
+
+	constraints->pull_down = fwnode_property_read_bool(fwnode,
+							"regulator-pull-down");
+
+	if (fwnode_property_read_bool(fwnode, "regulator-allow-bypass"))
+		constraints->valid_ops_mask |= REGULATOR_CHANGE_BYPASS;
+
+	if (fwnode_property_read_bool(fwnode, "regulator-allow-set-load"))
+		constraints->valid_ops_mask |= REGULATOR_CHANGE_DRMS;
+
+	ret = fwnode_property_read_u32(fwnode, "regulator-ramp-delay", &pval);
+	if (!ret) {
+		if (pval)
+			constraints->ramp_delay = pval;
+		else
+			constraints->ramp_disable = true;
+	}
+
+	ret = fwnode_property_read_u32(fwnode, "regulator-enable-ramp-delay",
+					&pval);
+	if (!ret)
+		constraints->enable_time = pval;
+
+	constraints->soft_start = fwnode_property_read_bool(fwnode,
+							"regulator-soft-start");
+	ret = fwnode_property_read_u32(fwnode, "regulator-active-discharge",
+					&pval);
+	if (!ret) {
+		constraints->active_discharge =
+				(pval) ? REGULATOR_ACTIVE_DISCHARGE_ENABLE :
+					REGULATOR_ACTIVE_DISCHARGE_DISABLE;
+	}
+
+	if (!fwnode_property_read_u32(fwnode, "regulator-initial-mode",
+					&pval)) {
+		if (desc && desc->map_mode) {
+			ret = desc->map_mode(pval);
+			if (ret == -EINVAL)
+				pr_err("invalid mode %u\n", pval);
+			else
+				constraints->initial_mode = ret;
+		} else {
+			pr_warn("mapping for mode %d not defined\n", pval);
+		}
+	}
+
+	if (!fwnode_property_read_u32(fwnode, "regulator-system-load", &pval))
+		constraints->system_load = pval;
+
+	constraints->over_current_protection = fwnode_property_read_bool(fwnode,
+					"regulator-over-current-protection");
+
+	for (i = 0; i < ARRAY_SIZE(regulator_states); i++) {
+		switch (i) {
+		case PM_SUSPEND_MEM:
+			suspend_state = &constraints->state_mem;
+			break;
+		case PM_SUSPEND_MAX:
+			suspend_state = &constraints->state_disk;
+			break;
+		case PM_SUSPEND_ON:
+		case PM_SUSPEND_FREEZE:
+		case PM_SUSPEND_STANDBY:
+		default:
+			continue;
+		}
+
+		suspend_fwnode = fwnode_get_named_child_node(fwnode,
+							regulator_states[i]);
+		if (!suspend_fwnode || !suspend_state)
+			continue;
+
+		if (!fwnode_property_read_u32(suspend_fwnode, "regulator-mode",
+						&pval)) {
+			if (desc && desc->map_mode) {
+				ret = desc->map_mode(pval);
+				if (ret == -EINVAL)
+					pr_err("invalid mode %u\n", pval);
+				else
+					suspend_state->mode = ret;
+			} else {
+				pr_warn("mapping for mode %d not defined\n",
+					pval);
+			}
+		}
+
+		if (fwnode_property_read_bool(suspend_fwnode,
+					"regulator-on-in-suspend"))
+			suspend_state->enabled = true;
+		else if (fwnode_property_read_bool(suspend_fwnode,
+						"regulator-off-in-suspend"))
+			suspend_state->disabled = true;
+
+		if (!fwnode_property_read_u32(suspend_fwnode,
+					"regulator-suspend-microvolt", &pval))
+			suspend_state->uV = pval;
+
+		if (i == PM_SUSPEND_MEM)
+			constraints->initial_state = PM_SUSPEND_MEM;
+
+		fwnode_handle_put(suspend_fwnode);
+		suspend_state = NULL;
+		suspend_fwnode = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(device_get_regulation_constraints);
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH 2/7] ACPI / property: have acpi_get_next_subnode take fwnode_handle

[kbuild test robot]

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

Hi Dmitry,

[auto build test ERROR on regulator/for-next]
[also build test ERROR on v4.10-rc5 next-20170125]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Furquan-Shaikh/Implement-generic-regulator-constraints-parsing-for-ACPI-and-OF/20170125-171807
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: i386-randconfig-x004-201704 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from drivers/acpi/property.c:16:0:
   drivers/acpi/property.c: In function 'acpi_get_next_subnode':
>> drivers/acpi/property.c:840:27: error: 'dev' undeclared (first use in this function)
        adev = ACPI_COMPANION(dev);
                              ^
   include/linux/acpi.h:54:51: note: in definition of macro 'ACPI_COMPANION'
    #define ACPI_COMPANION(dev)  to_acpi_device_node((dev)->fwnode)
                                                      ^~~
   drivers/acpi/property.c:840:27: note: each undeclared identifier is reported only once for each function it appears in
        adev = ACPI_COMPANION(dev);
                              ^
   include/linux/acpi.h:54:51: note: in definition of macro 'ACPI_COMPANION'
    #define ACPI_COMPANION(dev)  to_acpi_device_node((dev)->fwnode)
                                                      ^~~

vim +/dev +840 drivers/acpi/property.c

504a3374 Rafael J. Wysocki 2015-08-27  834  
504a3374 Rafael J. Wysocki 2015-08-27  835  		if (child) {
504a3374 Rafael J. Wysocki 2015-08-27  836  			adev = to_acpi_device_node(child);
504a3374 Rafael J. Wysocki 2015-08-27  837  			next = adev->node.next;
504a3374 Rafael J. Wysocki 2015-08-27  838  			if (next == head) {
504a3374 Rafael J. Wysocki 2015-08-27  839  				child = NULL;
bf4703fd Irina Tirdea      2016-03-13 @840  				adev = ACPI_COMPANION(dev);
504a3374 Rafael J. Wysocki 2015-08-27  841  				goto nondev;
504a3374 Rafael J. Wysocki 2015-08-27  842  			}
504a3374 Rafael J. Wysocki 2015-08-27  843  			adev = list_entry(next, struct acpi_device, node);

:::::: The code at line 840 was first introduced by commit
:::::: bf4703fdd166fffd5b2e4c42d2ebbf708b94748e ACPI / property: fix data node parsing in acpi_get_next_subnode()

:::::: TO: Irina Tirdea <irina.tirdea@intel.com>
:::::: CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29821 bytes --]

Re: [PATCH 6/7] drivers/gpio: Add and export gpiod_lookup[_index]

[kbuild test robot]

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

Hi Furquan,

[auto build test ERROR on regulator/for-next]
[also build test ERROR on v4.10-rc5 next-20170125]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Furquan-Shaikh/Implement-generic-regulator-constraints-parsing-for-ACPI-and-OF/20170125-171807
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: i386-randconfig-x004-201704 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/gpio/gpiolib.c: In function 'gpiod_lookup_index':
>> drivers/gpio/gpiolib.c:3221:44: error: 'flags' undeclared (first use in this function)
       desc = acpi_find_gpio(dev, con_id, idx, flags, &lookupflags);
                                               ^~~~~
   drivers/gpio/gpiolib.c:3221:44: note: each undeclared identifier is reported only once for each function it appears in

vim +/flags +3221 drivers/gpio/gpiolib.c

bae48da23 Alexandre Courbot 2013-10-17  3215  		/* Using device tree? */
4d8440b93 Rafael J. Wysocki 2015-03-10  3216  		if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
bae48da23 Alexandre Courbot 2013-10-17  3217  			dev_dbg(dev, "using device tree for GPIO lookup\n");
39b2bbe3d Alexandre Courbot 2014-07-25  3218  			desc = of_find_gpio(dev, con_id, idx, &lookupflags);
4d8440b93 Rafael J. Wysocki 2015-03-10  3219  		} else if (ACPI_COMPANION(dev)) {
81f59e9d1 Mika Westerberg   2013-10-10  3220  			dev_dbg(dev, "using ACPI for GPIO lookup\n");
25487533f Dmitry Torokhov   2016-03-24 @3221  			desc = acpi_find_gpio(dev, con_id, idx, flags, &lookupflags);
35c5d7fdc Alexandre Courbot 2013-11-23  3222  		}
4d8440b93 Rafael J. Wysocki 2015-03-10  3223  	}
35c5d7fdc Alexandre Courbot 2013-11-23  3224  

:::::: The code at line 3221 was first introduced by commit
:::::: 25487533ff311014b0cc39ed7fd721c114e1b11f gpio / ACPI: ignore GpioInt() GPIOs when requesting GPIO_OUT_*

:::::: TO: Dmitry Torokhov <dtor@chromium.org>
:::::: CC: Linus Walleij <linus.walleij@linaro.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29821 bytes --]

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Lorenzo Pieralisi]

[+MarkR]

On Tue, Jan 24, 2017 at 04:06:34PM -0800, Furquan Shaikh wrote:
> Until now, the regulator framework assumed that regulators are being
> passed in using device tree(OF) only. However, with the recent change
> to add ACPI fixed regulator, it is necessary to have all the regulator
> init data and regulation constraints parsing performed for ACPI
> regulators as well. This is important to ensure that drivers can
> properly enable and disable the regulators.
> 
> Since regulator properties remain the same across OF and ACPI
> regulators, this series of patches provides common routine for
> obtaining regulation constraints from device tree and ACPI nodes. In
> order to support these common routines, certain device property
> functions had to be exported to make the node parsing logic generic
> across OF and ACPI.
> 
> Since the changes in this patch series are cross-system, it will
> require some amount of co-ordination to ensure that it can be applied
> at once.

The DT bindings you are enabling in ACPI through this patch series
clash with the ACPI specifications and its way of handling power
states for devices, which clearly goes against the DSD properties
rules as stated in:

Documentation/acpi/DSD-properties-rules.txt

If the current ACPI bindings (and I mean ACPI specifications not
_PRP0001) are not sufficient to allow firmware to control power
management (and the regulators control that is part of it) through
standard ACPI methods we should request changes to the specifications,
enabling regulator control with ACPI through DT bindings is just a
recipe for disaster.

What's the best course of action should and can be debated, in the
interim on the series:

NAKed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> Dmitry Torokhov (3):
>   ACPI / property: have acpi_get_next_subnode take fwnode_handle
>   device property: introduce fwnode_for_each_child()
>   device property: introduce fwnode_get_named_child_node()
> 
> Furquan Shaikh (4):
>   drivers/regulator: Rename of_map_mode to map_mode in regulator desc
>   device property: Export dev_fwnode
>   drivers/gpio: Add and export gpiod_lookup[_index]
>   drivers/regulator: Initialize regulator init data for ACPI regulators
> 
>  drivers/acpi/property.c                 |   8 +-
>  drivers/base/property.c                 |  61 ++++++++---
>  drivers/gpio/gpiolib.c                  |  75 ++++++++++---
>  drivers/regulator/Makefile              |   2 +-
>  drivers/regulator/fixed.c               | 100 ++++++-----------
>  drivers/regulator/internal.h            |   3 +
>  drivers/regulator/max77802-regulator.c  |  12 +-
>  drivers/regulator/of_regulator.c        | 156 +-------------------------
>  drivers/regulator/qcom_spmi-regulator.c |   2 +-
>  drivers/regulator/regulator_props.c     | 189 ++++++++++++++++++++++++++++++++
>  drivers/regulator/twl-regulator.c       |   6 +-
>  drivers/regulator/twl6030-regulator.c   |   2 +-
>  include/linux/acpi.h                    |   5 +-
>  include/linux/fwnode.h                  |  10 ++
>  include/linux/gpio/consumer.h           |  21 ++++
>  include/linux/property.h                |   1 +
>  include/linux/regulator/driver.h        |   5 +-
>  17 files changed, 386 insertions(+), 272 deletions(-)
>  create mode 100644 drivers/regulator/regulator_props.c
> 
> -- 
> 2.11.0.483.g087da7b7c-goog
> 

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Brown]

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

On Tue, Jan 24, 2017 at 04:06:34PM -0800, Furquan Shaikh wrote:

> Since regulator properties remain the same across OF and ACPI
> regulators, this series of patches provides common routine for
> obtaining regulation constraints from device tree and ACPI nodes. In

As Lorenzo explained this is really not a good idea, ACPI has a power
management model which is just being ignored here.  If you're producing
systems using ACPI you really need to use ACPI in a joined up fashion
rather than just blindly importing all DT bindings into ACPI.  While
there are many areas where ACPI just doesn't do anything and as a result
it does make sense to reuse existing DT bindings there are also plenty
of areas where work has been done on the ACPI side.  Regulators and
power management are definitely one of those areas.

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

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Rafael J. Wysocki]

On Wed, Jan 25, 2017 at 1:49 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jan 24, 2017 at 04:06:34PM -0800, Furquan Shaikh wrote:
>
>> Since regulator properties remain the same across OF and ACPI
>> regulators, this series of patches provides common routine for
>> obtaining regulation constraints from device tree and ACPI nodes. In
>
> As Lorenzo explained this is really not a good idea, ACPI has a power
> management model which is just being ignored here.  If you're producing
> systems using ACPI you really need to use ACPI in a joined up fashion
> rather than just blindly importing all DT bindings into ACPI.  While
> there are many areas where ACPI just doesn't do anything and as a result
> it does make sense to reuse existing DT bindings there are also plenty
> of areas where work has been done on the ACPI side.  Regulators and
> power management are definitely one of those areas.

This is exactly right. :-)

Thanks,
Rafael

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Furquan Shaikh]

On Wed, Jan 25, 2017 at 4:55 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jan 25, 2017 at 1:49 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Jan 24, 2017 at 04:06:34PM -0800, Furquan Shaikh wrote:
> >
> >> Since regulator properties remain the same across OF and ACPI
> >> regulators, this series of patches provides common routine for
> >> obtaining regulation constraints from device tree and ACPI nodes. In
> >
> > As Lorenzo explained this is really not a good idea, ACPI has a power
> > management model which is just being ignored here.  If you're producing
> > systems using ACPI you really need to use ACPI in a joined up fashion
> > rather than just blindly importing all DT bindings into ACPI.  While
> > there are many areas where ACPI just doesn't do anything and as a result
> > it does make sense to reuse existing DT bindings there are also plenty
> > of areas where work has been done on the ACPI side.  Regulators and
> > power management are definitely one of those areas.
>
> This is exactly right. :-)
>
> Thanks,
> Rafael

I understand that ACPI provides its own bindings to allow firmware to
control power management and thus regulators have been a part of the
firmware control. However, there are use cases where the kernel driver
wishes to control the regulator to manage power to the device
irrespective of the way regulator is passed in (ACPI/OF).

That is the reason why the recent change to add ACPI support to fixed
regulators was done
(https://github.com/torvalds/linux/blob/master/drivers/regulator/fixed.c#L100).
It needs regulators for USB driver. Similarly, other drivers like ELAN
touchscreen that plan to control power to the device in a generic way
irrespective of OF/ACPI need to control regulators in kernel itself.
The above change for adding ACPI support to fixed regulators is
currently parsing only limited parameters and also does not work the
same way for ACPI and OF, though it ends to introduce the regulators
in a way similar to OF.

We need to support existing drivers and use cases for power management
in both OF and ACPI environments (keeping in mind that suspend to idle
bypasses parts of firmware) without needing to change all the drivers.
How can we achieve this?

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Rutland]

On Wed, Jan 25, 2017 at 08:56:42AM -0800, Furquan Shaikh wrote:
> I understand that ACPI provides its own bindings to allow firmware to
> control power management and thus regulators have been a part of the
> firmware control. However, there are use cases where the kernel driver
> wishes to control the regulator to manage power to the device
> irrespective of the way regulator is passed in (ACPI/OF).

It sounds like there is a deficiency in the ACPI spec, then.  That being
the case, this should be addressed within the ACPI spec (or at least in
conjunction with it), rather than attaching something unrelated onto the
side.

There are a number of problems with this approach, and other OSs are
facing the same set of problems. The ACPI spec and the ASWG are supposed
to provide a common ground where such issues get dealt with.

Using DSD to bypass the ASWG is creating a large set of problems for a
trivial expedience.

> That is the reason why the recent change to add ACPI support to fixed
> regulators was done
> (https://github.com/torvalds/linux/blob/master/drivers/regulator/fixed.c#L100).

To be honest, I'm surprised this got merged.

Mark, this was added in this cycle; can we please rip that out for now?

> It needs regulators for USB driver. Similarly, other drivers like ELAN
> touchscreen that plan to control power to the device in a generic way
> irrespective of OF/ACPI need to control regulators in kernel itself.
> The above change for adding ACPI support to fixed regulators is
> currently parsing only limited parameters and also does not work the
> same way for ACPI and OF, though it ends to introduce the regulators
> in a way similar to OF.

Having a consistent API is desirable. That does not imply that we should
invent a non-standard FW representation in ACPI, nor does it imply that
an API we have today is necessarily appropriate.

There are a number of different ways this could be addressed.

> We need to support existing drivers and use cases for power management
> in both OF and ACPI environments (keeping in mind that suspend to idle
> bypasses parts of firmware) without needing to change all the drivers.

I think that the goal here is broken, given existing model differences
between ACPI and DT.

We can certainly come up with something that allows drivers to support
both, but trying to do this without updating drivers opens a huge set of
problems.

Thanks,
Mark.

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Brown]

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

On Wed, Jan 25, 2017 at 08:56:42AM -0800, Furquan Shaikh wrote:

> I understand that ACPI provides its own bindings to allow firmware to
> control power management and thus regulators have been a part of the
> firmware control. However, there are use cases where the kernel driver
> wishes to control the regulator to manage power to the device
> irrespective of the way regulator is passed in (ACPI/OF).

You're missing the point here.  What we're saying is that if you want to
do this using ACPI you should extend ACPI to support this directly so
where regulators need to be controlled by the OS there's a clear
understanding of how this interacts with the rest of the ACPI power
management.

> We need to support existing drivers and use cases for power management
> in both OF and ACPI environments (keeping in mind that suspend to idle
> bypasses parts of firmware) without needing to change all the drivers.
> How can we achieve this?

Consumers already don't know they're using DT, there's no reason they
should have any impact from ACPI either except possibly a bit of name
translation for ACPI's idiomatic naming conventions.

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

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Brown]

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

On Wed, Jan 25, 2017 at 06:23:20PM +0000, Mark Rutland wrote:
> On Wed, Jan 25, 2017 at 08:56:42AM -0800, Furquan Shaikh wrote:

> > That is the reason why the recent change to add ACPI support to fixed
> > regulators was done
> > (https://github.com/torvalds/linux/blob/master/drivers/regulator/fixed.c#L100).

> To be honest, I'm surprised this got merged.

My understanding was that it was instantiated from another device as an
implementation detail of that device, letting it say "this GPIO should
be handled as a regulator".

> Mark, this was added in this cycle; can we please rip that out for now?

If it's instantiated directly we probably should.

> We can certainly come up with something that allows drivers to support
> both, but trying to do this without updating drivers opens a huge set of
> problems.

I think there's a reasonable chance that any ACPI specs could be written
in such a way as to allow transparent support in Linux, the main thing
I'd worry about is naming issues.

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

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Rutland]

On Wed, Jan 25, 2017 at 06:29:55PM +0000, Mark Brown wrote:
> On Wed, Jan 25, 2017 at 06:23:20PM +0000, Mark Rutland wrote:
> > On Wed, Jan 25, 2017 at 08:56:42AM -0800, Furquan Shaikh wrote:
> 
> > > That is the reason why the recent change to add ACPI support to fixed
> > > regulators was done
> > > (https://github.com/torvalds/linux/blob/master/drivers/regulator/fixed.c#L100).
> 
> > To be honest, I'm surprised this got merged.
> 
> My understanding was that it was instantiated from another device as an
> implementation detail of that device, letting it say "this GPIO should
> be handled as a regulator".
> 
> > Mark, this was added in this cycle; can we please rip that out for now?
> 
> If it's instantiated directly we probably should.

I think that given the larger problem that needs to be addressed here,
and how the us of DSD properties muddies the water, it would be
preferable to remove it until we have some consensus.

> > We can certainly come up with something that allows drivers to support
> > both, but trying to do this without updating drivers opens a huge set of
> > problems.
> 
> I think there's a reasonable chance that any ACPI specs could be written
> in such a way as to allow transparent support in Linux, the main thing
> I'd worry about is naming issues.

I think it's certainly possible to handle this so that drivers don't
largely have to care. I also think there is some massaging the needs to
be done (e.g. tables of names or some indirection for ACPI/DT
differences), and a unified API that tries to completely hide that is
not truly possible.

Thanks,
Mark.

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Dmitry Torokhov]

On Wed, Jan 25, 2017 at 06:29:55PM +0000, Mark Brown wrote:
> On Wed, Jan 25, 2017 at 06:23:20PM +0000, Mark Rutland wrote:
> > On Wed, Jan 25, 2017 at 08:56:42AM -0800, Furquan Shaikh wrote:
> 
> > > That is the reason why the recent change to add ACPI support to fixed
> > > regulators was done
> > > (https://github.com/torvalds/linux/blob/master/drivers/regulator/fixed.c#L100).
> 
> > To be honest, I'm surprised this got merged.
> 
> My understanding was that it was instantiated from another device as an
> implementation detail of that device, letting it say "this GPIO should
> be handled as a regulator".
> 
> > Mark, this was added in this cycle; can we please rip that out for now?
> 
> If it's instantiated directly we probably should.
> 
> > We can certainly come up with something that allows drivers to support
> > both, but trying to do this without updating drivers opens a huge set of
> > problems.
> 
> I think there's a reasonable chance that any ACPI specs could be written
> in such a way as to allow transparent support in Linux, the main thing
> I'd worry about is naming issues.

So if I am reading this correctly, currently ACPI does not expose power
supplies directly, but rather ties them to the device power state (D0,
D3cold, etc). Linux drivers do not usually follow that state model and
expect to have all their power supplies be given to them and then
figures out what to do with them itself. Given that, what do we do? Do
we map only entries from _PR3 so they are available to drivers via
regulator_get()? Or we ask the standard to add method enumerating all
supplies?

Thanks.

-- 
Dmitry

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Brown]

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

On Wed, Jan 25, 2017 at 06:34:20PM +0000, Mark Rutland wrote:
> On Wed, Jan 25, 2017 at 06:29:55PM +0000, Mark Brown wrote:

> > > Mark, this was added in this cycle; can we please rip that out for now?

> > If it's instantiated directly we probably should.

> I think that given the larger problem that needs to be addressed here,
> and how the us of DSD properties muddies the water, it would be
> preferable to remove it until we have some consensus.

Can you send a patch with a writeup please?

> > I think there's a reasonable chance that any ACPI specs could be written
> > in such a way as to allow transparent support in Linux, the main thing
> > I'd worry about is naming issues.

> I think it's certainly possible to handle this so that drivers don't
> largely have to care. I also think there is some massaging the needs to
> be done (e.g. tables of names or some indirection for ACPI/DT
> differences), and a unified API that tries to completely hide that is
> not truly possible.

Given how little consumers can assume about what they'll be allowed/able
to do on a given system the naming should be about it - if anything else
leaks through I'd be a bit worried.

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

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Lorenzo Pieralisi]

On Wed, Jan 25, 2017 at 06:29:55PM +0000, Mark Brown wrote:
> On Wed, Jan 25, 2017 at 06:23:20PM +0000, Mark Rutland wrote:
> > On Wed, Jan 25, 2017 at 08:56:42AM -0800, Furquan Shaikh wrote:
> 
> > > That is the reason why the recent change to add ACPI support to fixed
> > > regulators was done
> > > (https://github.com/torvalds/linux/blob/master/drivers/regulator/fixed.c#L100).
> 
> > To be honest, I'm surprised this got merged.
> 
> My understanding was that it was instantiated from another device as an
> implementation detail of that device, letting it say "this GPIO should
> be handled as a regulator".
> 
> > Mark, this was added in this cycle; can we please rip that out for now?
> 
> If it's instantiated directly we probably should.
> 
> > We can certainly come up with something that allows drivers to support
> > both, but trying to do this without updating drivers opens a huge set of
> > problems.
> 
> I think there's a reasonable chance that any ACPI specs could be written
> in such a way as to allow transparent support in Linux, the main thing
> I'd worry about is naming issues.

I think that the difference between ACPI and DT firmware models,
in particular in relation to power states handling (and what piece
of SW is in charge of power management) is significant and goes beyond
naming conventions, therefore the code (and reasoning behind it - ie
to have an identical driver interface to a completely different FW
model) in this series is just not acceptable, that's a plain shortcut.

We will see how this should be implemented in ACPI, not with this
code (and FW bindings).

Thanks,
Lorenzo

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Dmitry Torokhov]

On Wed, Jan 25, 2017 at 10:44:32AM -0800, Dmitry Torokhov wrote:
> On Wed, Jan 25, 2017 at 06:29:55PM +0000, Mark Brown wrote:
> > On Wed, Jan 25, 2017 at 06:23:20PM +0000, Mark Rutland wrote:
> > > On Wed, Jan 25, 2017 at 08:56:42AM -0800, Furquan Shaikh wrote:
> > 
> > > > That is the reason why the recent change to add ACPI support to fixed
> > > > regulators was done
> > > > (https://github.com/torvalds/linux/blob/master/drivers/regulator/fixed.c#L100).
> > 
> > > To be honest, I'm surprised this got merged.
> > 
> > My understanding was that it was instantiated from another device as an
> > implementation detail of that device, letting it say "this GPIO should
> > be handled as a regulator".
> > 
> > > Mark, this was added in this cycle; can we please rip that out for now?
> > 
> > If it's instantiated directly we probably should.
> > 
> > > We can certainly come up with something that allows drivers to support
> > > both, but trying to do this without updating drivers opens a huge set of
> > > problems.
> > 
> > I think there's a reasonable chance that any ACPI specs could be written
> > in such a way as to allow transparent support in Linux, the main thing
> > I'd worry about is naming issues.
> 
> So if I am reading this correctly, currently ACPI does not expose power
> supplies directly, but rather ties them to the device power state (D0,
> D3cold, etc). Linux drivers do not usually follow that state model and
> expect to have all their power supplies be given to them and then
> figures out what to do with them itself. Given that, what do we do? Do
> we map only entries from _PR3 so they are available to drivers via
> regulator_get()? Or we ask the standard to add method enumerating all
> supplies?

For the record, the main issue for the drivers, which is being solved by
exposing power supplies to the driver, is the following:

1. We suspend the device. Since there is no regulators the driver
assumes that it will retain it's state upon resume
2. System goes into some sleep state
3. System wakes up
4. Device goes through resume, normally disabling wakeup interrupt and
enabling normal processing
5. We end up with non functioning device because the firmware actually
cut the power off without the driver knowing anything about it.

I would really hate to go through _every_ driver and add the following
code to the resume path:

#if IS_ENABLED(CONFIG_ACPI)
	if (acpi_device_was_powered_off_between_suspend_and_now(dev)) {
		completely_reinitialize_device(dev);
	}
#endif

Thanks.

-- 
Dmitry

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Rutland]

On Wed, Jan 25, 2017 at 06:49:49PM +0000, Mark Brown wrote:
> On Wed, Jan 25, 2017 at 06:34:20PM +0000, Mark Rutland wrote:
> > On Wed, Jan 25, 2017 at 06:29:55PM +0000, Mark Brown wrote:
> 
> > > > Mark, this was added in this cycle; can we please rip that out for now?
> 
> > > If it's instantiated directly we probably should.
> 
> > I think that given the larger problem that needs to be addressed here,
> > and how the us of DSD properties muddies the water, it would be
> > preferable to remove it until we have some consensus.
> 
> Can you send a patch with a writeup please?

Sure. I've sent [1].

Thanks,
Mark.

[1] http://lkml.kernel.org/r/1485372609-2498-1-git-send-email-mark.rutland@arm.com

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Brown]

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

On Wed, Jan 25, 2017 at 11:27:11AM -0800, Dmitry Torokhov wrote:

> For the record, the main issue for the drivers, which is being solved by
> exposing power supplies to the driver, is the following:

> 1. We suspend the device. Since there is no regulators the driver
> assumes that it will retain it's state upon resume

That's *not* a sensible thing for drivers to assume regardless of the
presence or absence of explicitly controlled regulators, that just seems
like a plain old driver bug.  Even if there are regulators that doesn't
mean there isn't a suspend mode configuration that disables those
regulators.

> I would really hate to go through _every_ driver and add the following
> code to the resume path:
> 
> #if IS_ENABLED(CONFIG_ACPI)
> 	if (acpi_device_was_powered_off_between_suspend_and_now(dev)) {
> 		completely_reinitialize_device(dev);
> 	}
> #endif

That's not an ACPI thing, unless the device thinks it's actively
providing a wakeup source then if the system suspends the driver should
not be surprised to have power pulled - that's pretty normal.  If the
driver is a wakeup source then it's a bit different.

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

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Brown]

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

On Wed, Jan 25, 2017 at 07:21:35PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Jan 25, 2017 at 06:29:55PM +0000, Mark Brown wrote:

> > I think there's a reasonable chance that any ACPI specs could be written
> > in such a way as to allow transparent support in Linux, the main thing
> > I'd worry about is naming issues.

> I think that the difference between ACPI and DT firmware models,
> in particular in relation to power states handling (and what piece
> of SW is in charge of power management) is significant and goes beyond
> naming conventions, therefore the code (and reasoning behind it - ie
> to have an identical driver interface to a completely different FW
> model) in this series is just not acceptable, that's a plain shortcut.

> We will see how this should be implemented in ACPI, not with this
> code (and FW bindings).

Oh, absolutely - what I'm saying is that once that's done I'd expect
implementing it to be almost entirely a regulator core change in the
same way that implementing DT support was.

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

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Dmitry Torokhov]

On Wed, Jan 25, 2017 at 08:39:07PM +0000, Mark Brown wrote:
> On Wed, Jan 25, 2017 at 11:27:11AM -0800, Dmitry Torokhov wrote:
> 
> > For the record, the main issue for the drivers, which is being solved by
> > exposing power supplies to the driver, is the following:
> 
> > 1. We suspend the device. Since there is no regulators the driver
> > assumes that it will retain it's state upon resume
> 
> That's *not* a sensible thing for drivers to assume regardless of the
> presence or absence of explicitly controlled regulators, that just seems
> like a plain old driver bug.  Even if there are regulators that doesn't
> mean there isn't a suspend mode configuration that disables those
> regulators.

That means your platform description is incomplete, AKA a plain old
device tree bug.

> 
> > I would really hate to go through _every_ driver and add the following
> > code to the resume path:
> > 
> > #if IS_ENABLED(CONFIG_ACPI)
> > 	if (acpi_device_was_powered_off_between_suspend_and_now(dev)) {
> > 		completely_reinitialize_device(dev);
> > 	}
> > #endif
> 
> That's not an ACPI thing, unless the device thinks it's actively
> providing a wakeup source then if the system suspends the driver should
> not be surprised to have power pulled - that's pretty normal.  If the
> driver is a wakeup source then it's a bit different.

>From the practical standpoint many drivers make this assumption because
this shortens resume time for device. I.e. it makes difference whether
we go through full controller reset, possibly reloading firmware, and
applying desired configuration, versus taking the chip out of deep sleep
mode.

The decision on resume time vs power draw is up to the platform
designers of course.

So if we indeed saying that drivers should expect handling power loss
even when devices are not on a hot-pluggable buses then we need a
generic API for querying whether device lost it's state or not.

Thanks.

-- 
Dmitry

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Brown]

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

On Wed, Jan 25, 2017 at 01:17:14PM -0800, Dmitry Torokhov wrote:
> On Wed, Jan 25, 2017 at 08:39:07PM +0000, Mark Brown wrote:

> > That's *not* a sensible thing for drivers to assume regardless of the
> > presence or absence of explicitly controlled regulators, that just seems
> > like a plain old driver bug.  Even if there are regulators that doesn't
> > mean there isn't a suspend mode configuration that disables those
> > regulators.

> That means your platform description is incomplete, AKA a plain old
> device tree bug.

What makes you say that?  Like I say even with an explicitly described
regulator we might power it off during suspend (perhaps even through
explicit device tree configuration).

> > That's not an ACPI thing, unless the device thinks it's actively
> > providing a wakeup source then if the system suspends the driver should
> > not be surprised to have power pulled - that's pretty normal.  If the
> > driver is a wakeup source then it's a bit different.

> From the practical standpoint many drivers make this assumption because
> this shortens resume time for device. I.e. it makes difference whether
> we go through full controller reset, possibly reloading firmware, and
> applying desired configuration, versus taking the chip out of deep sleep
> mode.

Really?  They've been used on a limited set of systems and if they're
attempting to use the presence of a regulator to figure out if this is
going to happen they are just completely broken in this regard and need
to be fixed before anyone copies that code.  Even ignoring the suspend
mode configuration dummy regulators mean such a check would return true
in essentially all systems.

> So if we indeed saying that drivers should expect handling power loss
> even when devices are not on a hot-pluggable buses then we need a
> generic API for querying whether device lost it's state or not.

Yes.  Querying the suspend mode configuration would be one way to do it
already.

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

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Al Stone]

On 01/25/2017 12:27 PM, Dmitry Torokhov wrote:
> On Wed, Jan 25, 2017 at 10:44:32AM -0800, Dmitry Torokhov wrote:
>> On Wed, Jan 25, 2017 at 06:29:55PM +0000, Mark Brown wrote:
>>> On Wed, Jan 25, 2017 at 06:23:20PM +0000, Mark Rutland wrote:
>>>> On Wed, Jan 25, 2017 at 08:56:42AM -0800, Furquan Shaikh wrote:
>>>
>>>>> That is the reason why the recent change to add ACPI support to fixed
>>>>> regulators was done
>>>>> (https://github.com/torvalds/linux/blob/master/drivers/regulator/fixed.c#L100).
>>>
>>>> To be honest, I'm surprised this got merged.
>>>
>>> My understanding was that it was instantiated from another device as an
>>> implementation detail of that device, letting it say "this GPIO should
>>> be handled as a regulator".
>>>
>>>> Mark, this was added in this cycle; can we please rip that out for now?
>>>
>>> If it's instantiated directly we probably should.
>>>
>>>> We can certainly come up with something that allows drivers to support
>>>> both, but trying to do this without updating drivers opens a huge set of
>>>> problems.
>>>
>>> I think there's a reasonable chance that any ACPI specs could be written
>>> in such a way as to allow transparent support in Linux, the main thing
>>> I'd worry about is naming issues.
>>
>> So if I am reading this correctly, currently ACPI does not expose power
>> supplies directly, but rather ties them to the device power state (D0,
>> D3cold, etc). Linux drivers do not usually follow that state model and
>> expect to have all their power supplies be given to them and then
>> figures out what to do with them itself. Given that, what do we do? Do
>> we map only entries from _PR3 so they are available to drivers via
>> regulator_get()? Or we ask the standard to add method enumerating all
>> supplies?
> 
> For the record, the main issue for the drivers, which is being solved by
> exposing power supplies to the driver, is the following:
> 
> 1. We suspend the device. Since there is no regulators the driver
> assumes that it will retain it's state upon resume
> 2. System goes into some sleep state
> 3. System wakes up
> 4. Device goes through resume, normally disabling wakeup interrupt and
> enabling normal processing
> 5. We end up with non functioning device because the firmware actually
> cut the power off without the driver knowing anything about it.
> 
> I would really hate to go through _every_ driver and add the following
> code to the resume path:
> 
> #if IS_ENABLED(CONFIG_ACPI)
> 	if (acpi_device_was_powered_off_between_suspend_and_now(dev)) {
> 		completely_reinitialize_device(dev);
> 	}
> #endif
> 
> Thanks.
> 

Please see Sections 3.2-3.5 (3.6, too, for a broader picture) in the ACPI spec
[0] for an overview of ACPI power management.  Section 7 of the spec [0] adds
the details on how the firmware and OS are to cooperate in managing power.

In those sections, ACPI defines a PowerResource object, aka a power supply of
some flavor.  ACPI devices are then connected to that power resource.  So, the
spec may already have what you need defined.  Further, the code in drivers/acpi
/device_pm.c and drivers/acpi/power.c may handle the situations described -- I
believe that's their intent, at any rate.   If the ACPI ASL is written
properly, I don't think the device driver would have to add the code shown
above; it would just be handled via the acpi driver and ASL.  The ACPI model is
specifically designed so that drivers don't have to know these sorts of things,
so that the hardware and firmware underneath the OS can change over time
without having to change the OS -- the OS just needs to know how to talk to
ACPI.

But, to the point of some of the other discussion on the thread, this ACPI sort
of power management is a very, very different model than DT so that intertwining
the two models is highly unlikely to work, IMHO.



[0] http://www.uefi.org/specifications -- version 6.1, specifically.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Dmitry Torokhov]

On Wed, Jan 25, 2017 at 09:30:40PM +0000, Mark Brown wrote:
> On Wed, Jan 25, 2017 at 01:17:14PM -0800, Dmitry Torokhov wrote:
> > On Wed, Jan 25, 2017 at 08:39:07PM +0000, Mark Brown wrote:
> 
> > > That's *not* a sensible thing for drivers to assume regardless of the
> > > presence or absence of explicitly controlled regulators, that just seems
> > > like a plain old driver bug.  Even if there are regulators that doesn't
> > > mean there isn't a suspend mode configuration that disables those
> > > regulators.
> 
> > That means your platform description is incomplete, AKA a plain old
> > device tree bug.
> 
> What makes you say that?  Like I say even with an explicitly described
> regulator we might power it off during suspend (perhaps even through
> explicit device tree configuration).
> 
> > > That's not an ACPI thing, unless the device thinks it's actively
> > > providing a wakeup source then if the system suspends the driver should
> > > not be surprised to have power pulled - that's pretty normal.  If the
> > > driver is a wakeup source then it's a bit different.
> 
> > From the practical standpoint many drivers make this assumption because
> > this shortens resume time for device. I.e. it makes difference whether
> > we go through full controller reset, possibly reloading firmware, and
> > applying desired configuration, versus taking the chip out of deep sleep
> > mode.
> 
> Really?  They've been used on a limited set of systems and if they're
> attempting to use the presence of a regulator to figure out if this is
> going to happen they are just completely broken in this regard and need
> to be fixed before anyone copies that code.  Even ignoring the suspend
> mode configuration dummy regulators mean such a check would return true
> in essentially all systems.

I think many of them do not use regulators and just expect the power not
be cut off. Or use regulator + reset gpio combo to make the decision.
For example, we actually keying the need of doing power sequence in Elan
touchscreen driver off presence of GPIO because having GPIO is essential
in implementing required sequence.

> 
> > So if we indeed saying that drivers should expect handling power loss
> > even when devices are not on a hot-pluggable buses then we need a
> > generic API for querying whether device lost it's state or not.
> 
> Yes.  Querying the suspend mode configuration would be one way to do it
> already.

Suspend mode of what though? Regulator? I would not mind cleaning up at
least some of the drivers if we had a generic API for doing such
query... Would it be something like:

	bool device_has_lost_state(struct device *dev,
				   struct regulator *supplies)

with DT systems going strictly off regulator data and ACPI throwing
its internal state in the mix?

Or you meant something else?

-- 
Dmitry

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Brown]

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

On Wed, Jan 25, 2017 at 02:05:58PM -0800, Dmitry Torokhov wrote:

> > Yes.  Querying the suspend mode configuration would be one way to do it
> > already.

> Suspend mode of what though? Regulator? I would not mind cleaning up at
> least some of the drivers if we had a generic API for doing such
> query... Would it be something like:

Yes, though there's some fun working out exactly when to query them.
Early resume should be safe.

> 	bool device_has_lost_state(struct device *dev,
> 				   struct regulator *supplies)

> with DT systems going strictly off regulator data and ACPI throwing
> its internal state in the mix?

> Or you meant something else?

I meant looking at individual regulators, though I guess that does have
a list of supplied as an argument.

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

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Dmitry Torokhov]

On Wed, Jan 25, 2017 at 02:44:10PM -0700, Al Stone wrote:
> 
> But, to the point of some of the other discussion on the thread, this ACPI sort
> of power management is a very, very different model than DT so that intertwining
> the two models is highly unlikely to work, IMHO.

And yet this is something that is sorely needed. If you look, for
example, at drivers in drivers/input/*, then all non-SOC-specific
devices can easily find their way onto both ACPI-based and DT-based
systems (not mentioning legacy-style boards). Having two distinct power
schemes implemented in drivers will lead to many problems.

Having unified way of describing hardware is how _DSD came about, right?
Nobody wanted to write and maintain and test two separate ways of
describing properties when one was already implemented and working.

Thanks.

-- 
Dmitry

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Al Stone]

On 01/25/2017 04:27 PM, Dmitry Torokhov wrote:
> On Wed, Jan 25, 2017 at 02:44:10PM -0700, Al Stone wrote:
>>
>> But, to the point of some of the other discussion on the thread, this ACPI sort
>> of power management is a very, very different model than DT so that intertwining
>> the two models is highly unlikely to work, IMHO.
> 
> And yet this is something that is sorely needed. If you look, for
> example, at drivers in drivers/input/*, then all non-SOC-specific
> devices can easily find their way onto both ACPI-based and DT-based
> systems (not mentioning legacy-style boards). Having two distinct power
> schemes implemented in drivers will lead to many problems.

I really can't speak to those sorts of systems; where I deal with ACPI
is on enterprise-class server systems which seldom have a graphics card,
much less input devices other than a keyboard.  And in general, those
systems are required to use only ACPI.  If a vendor wants their device to
work on such a system, they need to provide a driver that works with ACPI.
It may also work with DT, but in this environment it doesn't matter.

Whether or not there are two power schemes is a moot point.  We have DT
and we have ACPI, and they have very different schemes for power management,
so we're already there.  And so far, my experience has been that as long as
the ACPI and DT parts of the driver are kept disjoint when the models diverge,
and share code when they are semantically absolutely identical, things work
pretty well.

> Having unified way of describing hardware is how _DSD came about, right?
> Nobody wanted to write and maintain and test two separate ways of
> describing properties when one was already implemented and working.

I can't speak for those that proposed _DSD to be part of the ACPI spec,
but no, it was not meant as a unified way of describing hardware, as far
as I can remember from the ASWG discussions I was part of.  The intent,
as I recall it, was to provide some of the same flexibility to ASL that
was available in DT.  At the time, power managment was even discussed as
one of the areas where the DT model and the ACPI model clashed.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Dmitry Torokhov]

On Wed, Jan 25, 2017 at 05:15:52PM -0700, Al Stone wrote:
> On 01/25/2017 04:27 PM, Dmitry Torokhov wrote:
> > On Wed, Jan 25, 2017 at 02:44:10PM -0700, Al Stone wrote:
> >>
> >> But, to the point of some of the other discussion on the thread, this ACPI sort
> >> of power management is a very, very different model than DT so that intertwining
> >> the two models is highly unlikely to work, IMHO.
> > 
> > And yet this is something that is sorely needed. If you look, for
> > example, at drivers in drivers/input/*, then all non-SOC-specific
> > devices can easily find their way onto both ACPI-based and DT-based
> > systems (not mentioning legacy-style boards). Having two distinct power
> > schemes implemented in drivers will lead to many problems.
> 
> I really can't speak to those sorts of systems; where I deal with ACPI
> is on enterprise-class server systems which seldom have a graphics card,
> much less input devices other than a keyboard. 

Yeah, so basically no power management except for CPU states ;)

> And in general, those
> systems are required to use only ACPI.  If a vendor wants their device to
> work on such a system, they need to provide a driver that works with ACPI.
> It may also work with DT, but in this environment it doesn't matter.
> 
> Whether or not there are two power schemes is a moot point.  We have DT
> and we have ACPI, and they have very different schemes for power management,
> so we're already there.  And so far, my experience has been that as long as
> the ACPI and DT parts of the driver are kept disjoint when the models diverge,
> and share code when they are semantically absolutely identical, things work
> pretty well.

As someone who's actually shipping both ACPI and DT-based devices
reusing the same peripherals I can assure you that it is really PITA to
have different PM behavior in a single driver and if we can converge on
something sane that would be great.

> 
> > Having unified way of describing hardware is how _DSD came about, right?
> > Nobody wanted to write and maintain and test two separate ways of
> > describing properties when one was already implemented and working.
> 
> I can't speak for those that proposed _DSD to be part of the ACPI spec,
> but no, it was not meant as a unified way of describing hardware, as far
> as I can remember from the ASWG discussions I was part of.  The intent,
> as I recall it, was to provide some of the same flexibility to ASL that
> was available in DT.  At the time, power managment was even discussed as
> one of the areas where the DT model and the ACPI model clashed.

That's not what I remember from discussions at Plumbers/KS... Binding
compatibility, even if for simplest properties, was one of the points.

Thanks.

-- 
Dmitry

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Rafael J. Wysocki]

On Wednesday, January 25, 2017 04:33:42 PM Dmitry Torokhov wrote:
> On Wed, Jan 25, 2017 at 05:15:52PM -0700, Al Stone wrote:
> > On 01/25/2017 04:27 PM, Dmitry Torokhov wrote:
> > > On Wed, Jan 25, 2017 at 02:44:10PM -0700, Al Stone wrote:
> > >>
> > >> But, to the point of some of the other discussion on the thread, this ACPI sort
> > >> of power management is a very, very different model than DT so that intertwining
> > >> the two models is highly unlikely to work, IMHO.
> > > 
> > > And yet this is something that is sorely needed. If you look, for
> > > example, at drivers in drivers/input/*, then all non-SOC-specific
> > > devices can easily find their way onto both ACPI-based and DT-based
> > > systems (not mentioning legacy-style boards). Having two distinct power
> > > schemes implemented in drivers will lead to many problems.
> > 
> > I really can't speak to those sorts of systems; where I deal with ACPI
> > is on enterprise-class server systems which seldom have a graphics card,
> > much less input devices other than a keyboard. 
> 
> Yeah, so basically no power management except for CPU states ;)
> 
> > And in general, those
> > systems are required to use only ACPI.  If a vendor wants their device to
> > work on such a system, they need to provide a driver that works with ACPI.
> > It may also work with DT, but in this environment it doesn't matter.
> > 
> > Whether or not there are two power schemes is a moot point.  We have DT
> > and we have ACPI, and they have very different schemes for power management,
> > so we're already there.  And so far, my experience has been that as long as
> > the ACPI and DT parts of the driver are kept disjoint when the models diverge,
> > and share code when they are semantically absolutely identical, things work
> > pretty well.
> 
> As someone who's actually shipping both ACPI and DT-based devices
> reusing the same peripherals I can assure you that it is really PITA to
> have different PM behavior in a single driver and if we can converge on
> something sane that would be great.

Agreed.

However, the problem is that PM in ACPI and in the DT-based code is done at
different levels.

ACPI uses the power resource abstraction and D-states to be able to combine
HW control over regulators (think things like MWAIT on Intel CPUs or generally
C-state management done via register wrtites on the CPU) with something that
can be exposed to software.

On the other hand, doing PM at the regulator level pretty much requires that
regulators be only maniplulated directly by software.

The common abstraction can sort of be provided through PM domains, which in the
ACPI case basically map to ACPI power resources and in the OF case genpd is used,
but that doesn't work if drivers expect to be able to control regulators directly
(which BTW isn't an good idea overall IMO, because that may need to be done
differently on different platforms even without ACPI AFAICS).

> > 
> > > Having unified way of describing hardware is how _DSD came about, right?
> > > Nobody wanted to write and maintain and test two separate ways of
> > > describing properties when one was already implemented and working.

So technically, _DSD itself is just a simplified _DSM.  It provides a simplified
interface for what has been possible all the time already anyway.

On top of that, there are _DSD properties, which are defined by the special
UUIDs that we support, and here the idea was to be able to provide information
in a way that resembles DT to be able to avoid artificial code branching in
drivers among other things.

> > I can't speak for those that proposed _DSD to be part of the ACPI spec,
> > but no, it was not meant as a unified way of describing hardware, as far
> > as I can remember from the ASWG discussions I was part of.  The intent,
> > as I recall it, was to provide some of the same flexibility to ASL that
> > was available in DT.  At the time, power managment was even discussed as
> > one of the areas where the DT model and the ACPI model clashed.
> 
> That's not what I remember from discussions at Plumbers/KS... Binding
> compatibility, even if for simplest properties, was one of the points.

The idea was that in the cases in which ACPI didn't provide any specific
frameworks for the given use cases, that would simply help to handle things
in a unified fashion.

Power management is not one of those cases, though.

Thanks,
Rafael

Re: [PATCH 6/7] drivers/gpio: Add and export gpiod_lookup[_index]

[Linus Walleij]

On Wed, Jan 25, 2017 at 1:06 AM, Furquan Shaikh <furquan@chromium.org> wrote:

> Sometimes (as the case with fixed regulator) we only want to look up,
> but not necessarily reserve right away, GPIO. gpiod_lookup() and
> gpiod_lookup_by_index() allow us doing just that.
>
> Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Furquan Shaikh <furquan@chromium.org>

Overall if this solves the GPIO regulator mess, I'd be happy
to apply it.

But overall it is a bit scary with looking up something struct gpiod *
that does not at all have the same semantics as a gpiod *
you get from gpiod_get().

We need to make sure that a gpiod looked up this way does not
work and tick and cannot perform operations as one obtained
with a proper gpiod_get().

>  /**
> + * gpiod_lookup - look up a GPIO for a given GPIO function
> + * @dev:       GPIO consumer, can be NULL for system-global GPIOs
> + * @con_id:    function within the GPIO consumer
> + *
> + * Return the GPIO descriptor corresponding to the function con_id of device
> + * dev, -ENOENT if no GPIO has been assigned to the requested function, or
> + * another IS_ERR() code.
> + */
> +struct gpio_desc *__must_check gpiod_lookup(struct device *dev,
> +                                           const char *con_id)
> +{
> +       return gpiod_lookup_index(dev, con_id, 0);
> +}
> +EXPORT_SYMBOL_GPL(gpiod_lookup);

Make this a static inline in the <linux/gpio/consumer.h> header instead.
Saves a lot of trouble.

>  /**
> - * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
> + * gpiod_lookup_index - look up a GPIO from a multi-index GPIO function
>   * @dev:       GPIO consumer, can be NULL for system-global GPIOs
>   * @con_id:    function within the GPIO consumer
>   * @idx:       index of the GPIO to obtain in the consumer
> - * @flags:     optional GPIO initialization flags
> - *
> - * This variant of gpiod_get() allows to access GPIOs other than the first
> - * defined one for functions that define several GPIOs.
>   *
> - * Return a valid GPIO descriptor, -ENOENT if no GPIO has been assigned to the
> - * requested function and/or index, or another IS_ERR() code if an error
> - * occurred while trying to acquire the GPIO.
> + * Return a valid GPIO descriptor, or -ENOENT if no GPIO has been assigned to
> + * the requested function and/or index, or another IS_ERR() code.
>   */

What a mess in the patch. Not your fault but format-patch created this
mess.0

> -struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
> -                                              const char *con_id,
> -                                              unsigned int idx,
> -                                              enum gpiod_flags flags)
> +struct gpio_desc *__must_check gpiod_lookup_index(struct device *dev,
> +                                                 const char *con_id,
> +                                                 unsigned int idx)
>  {
>         struct gpio_desc *desc = NULL;
> -       int status;
>         enum gpio_lookup_flags lookupflags = 0;
>
>         dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
> @@ -3285,16 +3294,50 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>                 desc = gpiod_find(dev, con_id, idx, &lookupflags);
>         }
>
> -       if (IS_ERR(desc)) {
> +       if (IS_ERR(desc))
>                 dev_dbg(dev, "lookup for GPIO %s failed\n", con_id);
> +
> +       /*
> +        * Configure static flags based on lookup data (such as
> +        * "active low", "open drain", etc.)
> +        */
> +       gpiod_configure_flags(desc, con_id, lookupflags, 0);

So what about setting some special flag from
gpiolib.h like
set_bit(FLAG_ONLY_LOOKUP, &desc->flags);
here numbing all
operations on the returned gpiod, by a check in
VALIDATE_DESC() and VALIDATE_DESC_VOID()
so that no real operations fall through?

Then that flag will just be forcibly cleared by
gpiod_get[_index]() later.

Yours,
Linus Walleij

Re: [PATCH 0/7] Implement generic regulator constraints parsing for ACPI and OF

[Mark Brown]

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

On Thu, Jan 26, 2017 at 11:35:55AM +0100, Rafael J. Wysocki wrote:

> but that doesn't work if drivers expect to be able to control regulators directly
> (which BTW isn't an good idea overall IMO, because that may need to be done
> differently on different platforms even without ACPI AFAICS).

We've really not encountered anything that has any issues here outside
of what's supported in the constraints already (basically what voltages
can be set and if the supply can be turned on and off).  I'd be pretty
surprised if we did TBH, it's hard to do something radically different
with a single piece of silicon which is the abstraction level we tend to
be operating at if we are controlling regulators.  AFAICT a higher level
of abstraction is generally going to go along with physically hiding the
regulators even in a DT world.

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