linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/6] DA9062 PMIC features
@ 2019-11-29 17:25 Marco Felsch
  2019-11-29 17:25 ` [PATCH v3 1/6] gpio: treewide rename gpio_chip_hwgpio to gpiod_to_offset Marco Felsch
                   ` (6 more replies)
  0 siblings, 7 replies; 38+ messages in thread
From: Marco Felsch @ 2019-11-29 17:25 UTC (permalink / raw)
  To: support.opensource, lee.jones, robh+dt, linus.walleij,
	bgolaszewski, joel, andrew, lgirdwood, broonie
  Cc: devicetree, linux-kernel, linux-gpio, linux-aspeed,
	linux-arm-kernel, kernel

Hi,

this series address all comments made on [1]. Patch "gpio: add support
to get local gpio number" is splitted into:
 - "gpio: treewide rename gpio_chip_hwgpio to gpiod_to_offset"
 - "gpio: make gpiod_to_offset() available for other users"
Please check the discussion [1] for more information. You need to apply
[2] to test the new features.

[1] https://lore.kernel.org/lkml/20191127135932.7223-1-m.felsch@pengutronix.de/
[2] https://lore.kernel.org/linux-gpio/20191129165817.20426-1-m.felsch@pengutronix.de/T/#m3da1fb0d16a37979c74bbcebdb29f3da9e89a9ac

Marco Felsch (6):
  gpio: treewide rename gpio_chip_hwgpio to gpiod_to_offset
  gpio: make gpiod_to_offset() available for other users
  dt-bindings: mfd: da9062: add regulator voltage selection
    documentation
  regulator: da9062: add voltage selection gpio support
  dt-bindings: mfd: da9062: add regulator gpio enable/disable
    documentation
  regulator: da9062: add gpio based regulator dis-/enable support

 .../devicetree/bindings/mfd/da9062.txt        |  16 ++
 drivers/gpio/gpio-aspeed.c                    |  15 +-
 drivers/gpio/gpiolib-sysfs.c                  |   9 +-
 drivers/gpio/gpiolib.c                        |  74 +++--
 drivers/gpio/gpiolib.h                        |   8 -
 drivers/regulator/da9062-regulator.c          | 258 ++++++++++++++++++
 include/linux/gpio/private.h                  |  27 ++
 7 files changed, 361 insertions(+), 46 deletions(-)
 create mode 100644 include/linux/gpio/private.h

-- 
2.20.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v3 1/6] gpio: treewide rename gpio_chip_hwgpio to gpiod_to_offset
  2019-11-29 17:25 [PATCH v3 0/6] DA9062 PMIC features Marco Felsch
@ 2019-11-29 17:25 ` Marco Felsch
  2019-12-02  2:59   ` Andrew Jeffery
  2019-11-29 17:25 ` [PATCH v3 2/6] gpio: make gpiod_to_offset() available for other users Marco Felsch
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 38+ messages in thread
From: Marco Felsch @ 2019-11-29 17:25 UTC (permalink / raw)
  To: support.opensource, lee.jones, robh+dt, linus.walleij,
	bgolaszewski, joel, andrew, lgirdwood, broonie
  Cc: devicetree, linux-kernel, linux-gpio, linux-aspeed,
	linux-arm-kernel, kernel

During discussion [1] we decided to rename the gpio subsystem local
helper so the name is more meaningful for users outside the gpio
subsystem. Making the helper public is done by a 2nd patch. The
current users are the gpiolib itself and the aspeed gpio driver.
The renaming is done by the following command:

    find ./drivers/gpio -type f -exec sed -i 's/gpio_chip_hwgpio/gpiod_to_offset/g' {} \;

[1] https://lkml.org/lkml/2019/11/27/357

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:
v3:
- new patch

 drivers/gpio/gpio-aspeed.c   |  6 ++---
 drivers/gpio/gpiolib-sysfs.c |  8 +++---
 drivers/gpio/gpiolib.c       | 52 ++++++++++++++++++------------------
 drivers/gpio/gpiolib.h       |  2 +-
 4 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 09e53c5f3b0a..b1d1d39e5174 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -21,7 +21,7 @@
 
 /*
  * These two headers aren't meant to be used by GPIO drivers. We need
- * them in order to access gpio_chip_hwgpio() which we need to implement
+ * them in order to access gpiod_to_offset() which we need to implement
  * the aspeed specific API which allows the coprocessor to request
  * access to some GPIOs and to arbitrate between coprocessor and ARM.
  */
@@ -1007,7 +1007,7 @@ int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc,
 {
 	struct gpio_chip *chip = gpiod_to_chip(desc);
 	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
-	int rc = 0, bindex, offset = gpio_chip_hwgpio(desc);
+	int rc = 0, bindex, offset = gpiod_to_offset(desc);
 	const struct aspeed_gpio_bank *bank = to_bank(offset);
 	unsigned long flags;
 
@@ -1053,7 +1053,7 @@ int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc)
 {
 	struct gpio_chip *chip = gpiod_to_chip(desc);
 	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
-	int rc = 0, bindex, offset = gpio_chip_hwgpio(desc);
+	int rc = 0, bindex, offset = gpiod_to_offset(desc);
 	const struct aspeed_gpio_bank *bank = to_bank(offset);
 	unsigned long flags;
 
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index fbf6b1a0a4fa..d4cab6a80928 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -192,7 +192,7 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 	 *        Remove this redundant call (along with the corresponding
 	 *        unlock) when those drivers have been fixed.
 	 */
-	ret = gpiochip_lock_as_irq(desc->gdev->chip, gpio_chip_hwgpio(desc));
+	ret = gpiochip_lock_as_irq(desc->gdev->chip, gpiod_to_offset(desc));
 	if (ret < 0)
 		goto err_put_kn;
 
@@ -206,7 +206,7 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 	return 0;
 
 err_unlock:
-	gpiochip_unlock_as_irq(desc->gdev->chip, gpio_chip_hwgpio(desc));
+	gpiochip_unlock_as_irq(desc->gdev->chip, gpiod_to_offset(desc));
 err_put_kn:
 	sysfs_put(data->value_kn);
 
@@ -224,7 +224,7 @@ static void gpio_sysfs_free_irq(struct device *dev)
 
 	data->irq_flags = 0;
 	free_irq(data->irq, data);
-	gpiochip_unlock_as_irq(desc->gdev->chip, gpio_chip_hwgpio(desc));
+	gpiochip_unlock_as_irq(desc->gdev->chip, gpiod_to_offset(desc));
 	sysfs_put(data->value_kn);
 }
 
@@ -622,7 +622,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	else
 		data->direction_can_change = false;
 
-	offset = gpio_chip_hwgpio(desc);
+	offset = gpiod_to_offset(desc);
 	if (chip->names && chip->names[offset])
 		ioname = chip->names[offset];
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 104ed299d5ea..548cf41c6179 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -218,7 +218,7 @@ int gpiod_get_direction(struct gpio_desc *desc)
 	int ret;
 
 	chip = gpiod_to_chip(desc);
-	offset = gpio_chip_hwgpio(desc);
+	offset = gpiod_to_offset(desc);
 
 	if (!chip->get_direction)
 		return -ENOTSUPP;
@@ -2679,7 +2679,7 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	if (chip->request) {
 		/* chip->request may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
-		offset = gpio_chip_hwgpio(desc);
+		offset = gpiod_to_offset(desc);
 		if (gpiochip_line_is_valid(chip, offset))
 			ret = chip->request(chip, offset);
 		else
@@ -2781,7 +2781,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 		if (chip->free) {
 			spin_unlock_irqrestore(&gpio_lock, flags);
 			might_sleep_if(chip->can_sleep);
-			chip->free(chip, gpio_chip_hwgpio(desc));
+			chip->free(chip, gpiod_to_offset(desc));
 			spin_lock_irqsave(&gpio_lock, flags);
 		}
 		kfree_const(desc->label);
@@ -2965,9 +2965,9 @@ int gpiod_direction_input(struct gpio_desc *desc)
 	 * assume we are in input mode after this.
 	 */
 	if (chip->direction_input) {
-		ret = chip->direction_input(chip, gpio_chip_hwgpio(desc));
+		ret = chip->direction_input(chip, gpiod_to_offset(desc));
 	} else if (chip->get_direction &&
-		  (chip->get_direction(chip, gpio_chip_hwgpio(desc)) != 1)) {
+		  (chip->get_direction(chip, gpiod_to_offset(desc)) != 1)) {
 		gpiod_warn(desc,
 			   "%s: missing direction_input() operation and line is output\n",
 			   __func__);
@@ -2977,10 +2977,10 @@ int gpiod_direction_input(struct gpio_desc *desc)
 		clear_bit(FLAG_IS_OUT, &desc->flags);
 
 	if (test_bit(FLAG_PULL_UP, &desc->flags))
-		gpio_set_config(chip, gpio_chip_hwgpio(desc),
+		gpio_set_config(chip, gpiod_to_offset(desc),
 				PIN_CONFIG_BIAS_PULL_UP);
 	else if (test_bit(FLAG_PULL_DOWN, &desc->flags))
-		gpio_set_config(chip, gpio_chip_hwgpio(desc),
+		gpio_set_config(chip, gpiod_to_offset(desc),
 				PIN_CONFIG_BIAS_PULL_DOWN);
 
 	trace_gpio_direction(desc_to_gpio(desc), 1, ret);
@@ -3008,11 +3008,11 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 	}
 
 	if (gc->direction_output) {
-		ret = gc->direction_output(gc, gpio_chip_hwgpio(desc), val);
+		ret = gc->direction_output(gc, gpiod_to_offset(desc), val);
 	} else {
 		/* Check that we are in output mode if we can */
 		if (gc->get_direction &&
-		    gc->get_direction(gc, gpio_chip_hwgpio(desc))) {
+		    gc->get_direction(gc, gpiod_to_offset(desc))) {
 			gpiod_warn(desc,
 				"%s: missing direction_output() operation\n",
 				__func__);
@@ -3022,7 +3022,7 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 		 * If we can't actively set the direction, we are some
 		 * output-only chip, so just drive the output as desired.
 		 */
-		gc->set(gc, gpio_chip_hwgpio(desc), val);
+		gc->set(gc, gpiod_to_offset(desc), val);
 	}
 
 	if (!ret)
@@ -3085,7 +3085,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 	gc = desc->gdev->chip;
 	if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
 		/* First see if we can enable open drain in hardware */
-		ret = gpio_set_config(gc, gpio_chip_hwgpio(desc),
+		ret = gpio_set_config(gc, gpiod_to_offset(desc),
 				      PIN_CONFIG_DRIVE_OPEN_DRAIN);
 		if (!ret)
 			goto set_output_value;
@@ -3096,7 +3096,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 		}
 	}
 	else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) {
-		ret = gpio_set_config(gc, gpio_chip_hwgpio(desc),
+		ret = gpio_set_config(gc, gpiod_to_offset(desc),
 				      PIN_CONFIG_DRIVE_OPEN_SOURCE);
 		if (!ret)
 			goto set_output_value;
@@ -3106,7 +3106,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 			goto set_output_flag;
 		}
 	} else {
-		gpio_set_config(gc, gpio_chip_hwgpio(desc),
+		gpio_set_config(gc, gpiod_to_offset(desc),
 				PIN_CONFIG_DRIVE_PUSH_PULL);
 	}
 
@@ -3150,7 +3150,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 	}
 
 	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce);
-	return chip->set_config(chip, gpio_chip_hwgpio(desc), config);
+	return chip->set_config(chip, gpiod_to_offset(desc), config);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 
@@ -3186,7 +3186,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory)
 
 	packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE,
 					  !transitory);
-	gpio = gpio_chip_hwgpio(desc);
+	gpio = gpiod_to_offset(desc);
 	rc = chip->set_config(chip, gpio, packed);
 	if (rc == -ENOTSUPP) {
 		dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n",
@@ -3240,7 +3240,7 @@ static int gpiod_get_raw_value_commit(const struct gpio_desc *desc)
 	int value;
 
 	chip = desc->gdev->chip;
-	offset = gpio_chip_hwgpio(desc);
+	offset = gpiod_to_offset(desc);
 	value = chip->get ? chip->get(chip, offset) : -EIO;
 	value = value < 0 ? value : !!value;
 	trace_gpio_value(desc_to_gpio(desc), 1, value);
@@ -3329,7 +3329,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 		first = i;
 		do {
 			const struct gpio_desc *desc = desc_array[i];
-			int hwgpio = gpio_chip_hwgpio(desc);
+			int hwgpio = gpiod_to_offset(desc);
 
 			__set_bit(hwgpio, mask);
 			i++;
@@ -3349,7 +3349,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 
 		for (j = first; j < i; ) {
 			const struct gpio_desc *desc = desc_array[j];
-			int hwgpio = gpio_chip_hwgpio(desc);
+			int hwgpio = gpiod_to_offset(desc);
 			int value = test_bit(hwgpio, bits);
 
 			if (!raw && test_bit(FLAG_ACTIVE_LOW, &desc->flags))
@@ -3479,7 +3479,7 @@ static void gpio_set_open_drain_value_commit(struct gpio_desc *desc, bool value)
 {
 	int ret = 0;
 	struct gpio_chip *chip = desc->gdev->chip;
-	int offset = gpio_chip_hwgpio(desc);
+	int offset = gpiod_to_offset(desc);
 
 	if (value) {
 		ret = chip->direction_input(chip, offset);
@@ -3504,7 +3504,7 @@ static void gpio_set_open_source_value_commit(struct gpio_desc *desc, bool value
 {
 	int ret = 0;
 	struct gpio_chip *chip = desc->gdev->chip;
-	int offset = gpio_chip_hwgpio(desc);
+	int offset = gpiod_to_offset(desc);
 
 	if (value) {
 		ret = chip->direction_output(chip, offset, 1);
@@ -3526,7 +3526,7 @@ static void gpiod_set_raw_value_commit(struct gpio_desc *desc, bool value)
 
 	chip = desc->gdev->chip;
 	trace_gpio_value(desc_to_gpio(desc), 0, value);
-	chip->set(chip, gpio_chip_hwgpio(desc), value);
+	chip->set(chip, gpiod_to_offset(desc), value);
 }
 
 /*
@@ -3610,7 +3610,7 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 
 		do {
 			struct gpio_desc *desc = desc_array[i];
-			int hwgpio = gpio_chip_hwgpio(desc);
+			int hwgpio = gpiod_to_offset(desc);
 			int value = test_bit(i, value_bitmap);
 
 			/*
@@ -3822,7 +3822,7 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 		return -EINVAL;
 
 	chip = desc->gdev->chip;
-	offset = gpio_chip_hwgpio(desc);
+	offset = gpiod_to_offset(desc);
 	if (chip->to_irq) {
 		int retirq = chip->to_irq(chip, offset);
 
@@ -4678,7 +4678,7 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 	int ret;
 
 	chip = gpiod_to_chip(desc);
-	hwnum = gpio_chip_hwgpio(desc);
+	hwnum = gpiod_to_offset(desc);
 
 	local_desc = gpiochip_request_own_desc(chip, hwnum, name,
 					       lflags, dflags);
@@ -4759,7 +4759,7 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 		 * If pin hardware number of array member 0 is also 0, select
 		 * its chip as a candidate for fast bitmap processing path.
 		 */
-		if (descs->ndescs == 0 && gpio_chip_hwgpio(desc) == 0) {
+		if (descs->ndescs == 0 && gpiod_to_offset(desc) == 0) {
 			struct gpio_descs *array;
 
 			bitmap_size = BITS_TO_LONGS(chip->ngpio > count ?
@@ -4803,7 +4803,7 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 		 * but their pins are not in hardware order.
 		 */
 		else if (array_info &&
-			   gpio_chip_hwgpio(desc) != descs->ndescs) {
+			   gpiod_to_offset(desc) != descs->ndescs) {
 			/*
 			 * Don't use fast path if all array members processed so
 			 * far belong to the same chip as this one but its pin
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index b8b10a409c7b..a7f93ce6e114 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -127,7 +127,7 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 /*
  * Return the GPIO number of the passed descriptor relative to its chip
  */
-static inline int gpio_chip_hwgpio(const struct gpio_desc *desc)
+static inline int gpiod_to_offet(const struct gpio_desc *desc)
 {
 	return desc - &desc->gdev->descs[0];
 }
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v3 2/6] gpio: make gpiod_to_offset() available for other users
  2019-11-29 17:25 [PATCH v3 0/6] DA9062 PMIC features Marco Felsch
  2019-11-29 17:25 ` [PATCH v3 1/6] gpio: treewide rename gpio_chip_hwgpio to gpiod_to_offset Marco Felsch
@ 2019-11-29 17:25 ` Marco Felsch
  2019-12-02  3:00   ` Andrew Jeffery
  2019-11-29 17:25 ` [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation Marco Felsch
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 38+ messages in thread
From: Marco Felsch @ 2019-11-29 17:25 UTC (permalink / raw)
  To: support.opensource, lee.jones, robh+dt, linus.walleij,
	bgolaszewski, joel, andrew, lgirdwood, broonie
  Cc: devicetree, linux-kernel, linux-gpio, linux-aspeed,
	linux-arm-kernel, kernel

Currently gpiod_to_offset() is a gpio-subsystem private function which
is used by the gpiolib itself and by the aspeed gpio driver. The time
has shown that there are other drivers as well which need this
information in some special cases e.g. MFD drivers. The patch makes the
function public but you need to explicit add the <linux/gpio/private.h>
include. See discussion [1] for more information.

[1] https://lkml.org/lkml/2019/11/27/357

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v3:
- former patch description was "gpio: add support to get local gpio number"
- adapt commit message and description to reflect new state
- don't add wrapper instead use the already existing gpiod_to_offset
- move gpiod_to_offset from gpiolib.h into gpiolib.c
- move declaration into linux/gpio/private.h
---
 drivers/gpio/gpio-aspeed.c   | 11 +++++------
 drivers/gpio/gpiolib-sysfs.c |  1 +
 drivers/gpio/gpiolib.c       | 22 ++++++++++++++++++++++
 drivers/gpio/gpiolib.h       |  8 --------
 include/linux/gpio/private.h | 27 +++++++++++++++++++++++++++
 5 files changed, 55 insertions(+), 14 deletions(-)
 create mode 100644 include/linux/gpio/private.h

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index b1d1d39e5174..e10ebad6853a 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -20,13 +20,12 @@
 #include <linux/string.h>
 
 /*
- * These two headers aren't meant to be used by GPIO drivers. We need
- * them in order to access gpiod_to_offset() which we need to implement
- * the aspeed specific API which allows the coprocessor to request
- * access to some GPIOs and to arbitrate between coprocessor and ARM.
+ * The header isn't meant to be used by GPIO drivers. We need it in order to
+ * access gpiod_to_offset() which we need to implement the aspeed specific API
+ * which allows the coprocessor to request access to some GPIOs and to
+ * arbitrate between coprocessor and ARM.
  */
-#include <linux/gpio/consumer.h>
-#include "gpiolib.h"
+#include <linux/gpio/private.h>
 
 struct aspeed_bank_props {
 	unsigned int bank;
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index d4cab6a80928..367db78bb58c 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -5,6 +5,7 @@
 #include <linux/sysfs.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
+#include <linux/gpio/private.h>
 #include <linux/interrupt.h>
 #include <linux/kdev_t.h>
 #include <linux/slab.h>
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 548cf41c6179..197cac0e3e99 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -150,6 +150,28 @@ struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip,
 	return &gdev->descs[hwnum];
 }
 
+/**
+ * gpiod_to_offset - obtain the local GPIO number from a global unique
+ *		     descriptor
+ * @desc:	     gpio whose local gpio number should be returned
+ *
+ * It converts the global unique descriptor into the gpio chip local gpio
+ * number. This can be useful if you need to do further device configuration
+ * e.g. for a MFD. Use this function with caution. You will get a wrong number
+ * if you pass the wrong descriptor.
+ *
+ * Return:
+ * * the GPIO number of the passed descriptor relative to its chip
+ * * -EINVAL if desc is invalid or NULL
+ */
+int gpiod_to_offset(const struct gpio_desc *desc)
+{
+	if (IS_ERR_OR_NULL(desc))
+		return -EINVAL;
+	return desc - &desc->gdev->descs[0];
+}
+EXPORT_SYMBOL_GPL(gpiod_to_offset);
+
 /**
  * desc_to_gpio - convert a GPIO descriptor to the integer namespace
  * @desc: GPIO descriptor
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index a7f93ce6e114..a586a793b084 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -124,14 +124,6 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 		unsigned long lflags, enum gpiod_flags dflags);
 
-/*
- * Return the GPIO number of the passed descriptor relative to its chip
- */
-static inline int gpiod_to_offet(const struct gpio_desc *desc)
-{
-	return desc - &desc->gdev->descs[0];
-}
-
 /* With descriptor prefix */
 
 #define gpiod_emerg(desc, fmt, ...)					       \
diff --git a/include/linux/gpio/private.h b/include/linux/gpio/private.h
new file mode 100644
index 000000000000..56514bdcfac6
--- /dev/null
+++ b/include/linux/gpio/private.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Functions listed here should be used with caution. Mostly those functions are
+ * used by the gpiolib internally. But the time has shown that some special
+ * drivers needs access to these helpers too.
+ */
+#ifndef __LINUX_GPIO_PRIVATE_H
+#define __LINUX_GPIO_PRIVATE_H
+
+struct gpio_desc;
+
+#ifdef CONFIG_GPIOLIB
+
+int gpiod_to_offset(const struct gpio_desc *desc);
+
+#else /* CONFIG_GPIOLIB */
+
+static inline int gpiod_to_offset(const struct gpio_desc *desc)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(desc);
+	return 0;
+}
+
+#endif /* CONFIG_GPIOLIB */
+
+#endif /* __LINUX_GPIO_PRIVATE_H */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-11-29 17:25 [PATCH v3 0/6] DA9062 PMIC features Marco Felsch
  2019-11-29 17:25 ` [PATCH v3 1/6] gpio: treewide rename gpio_chip_hwgpio to gpiod_to_offset Marco Felsch
  2019-11-29 17:25 ` [PATCH v3 2/6] gpio: make gpiod_to_offset() available for other users Marco Felsch
@ 2019-11-29 17:25 ` Marco Felsch
  2019-12-04 13:46   ` Mark Brown
  2019-11-29 17:25 ` [PATCH v3 4/6] regulator: da9062: add voltage selection gpio support Marco Felsch
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 38+ messages in thread
From: Marco Felsch @ 2019-11-29 17:25 UTC (permalink / raw)
  To: support.opensource, lee.jones, robh+dt, linus.walleij,
	bgolaszewski, joel, andrew, lgirdwood, broonie
  Cc: devicetree, linux-kernel, linux-gpio, linux-aspeed,
	linux-arm-kernel, kernel

Add the documentation which describe the voltage selection gpio support.
This property can be applied to each subnode within the 'regulators'
node so each regulator can be configured differently.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:
v3:
- adapt binding description

 Documentation/devicetree/bindings/mfd/da9062.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt b/Documentation/devicetree/bindings/mfd/da9062.txt
index edca653a5777..b9cccd4c3f76 100644
--- a/Documentation/devicetree/bindings/mfd/da9062.txt
+++ b/Documentation/devicetree/bindings/mfd/da9062.txt
@@ -66,6 +66,14 @@ Sub-nodes:
   details of individual regulator device can be found in:
   Documentation/devicetree/bindings/regulator/regulator.txt
 
+  Optional regulator device-specific properties:
+  - dlg,vsel-sense-gpios : A GPIO reference to a local general purpose input,
+    the datasheet calls it GPI. The regulator sense the input signal and select
+    the active or suspend voltage settings. If the signal is active the
+    active-settings are applied else the suspend-settings are applied.
+    Attention: Sharing the same GPI for other purposes or across multiple
+    regulators is possible but the polarity setting must equal.
+
 - rtc : This node defines settings required for the Real-Time Clock associated
   with the DA9062. There are currently no entries in this binding, however
   compatible = "dlg,da9062-rtc" should be added if a node is created.
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v3 4/6] regulator: da9062: add voltage selection gpio support
  2019-11-29 17:25 [PATCH v3 0/6] DA9062 PMIC features Marco Felsch
                   ` (2 preceding siblings ...)
  2019-11-29 17:25 ` [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation Marco Felsch
@ 2019-11-29 17:25 ` Marco Felsch
  2019-11-29 17:25 ` [PATCH v3 5/6] dt-bindings: mfd: da9062: add regulator gpio enable/disable documentation Marco Felsch
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 38+ messages in thread
From: Marco Felsch @ 2019-11-29 17:25 UTC (permalink / raw)
  To: support.opensource, lee.jones, robh+dt, linus.walleij,
	bgolaszewski, joel, andrew, lgirdwood, broonie
  Cc: devicetree, linux-kernel, linux-gpio, linux-aspeed,
	linux-arm-kernel, kernel

The DA9062/1 devices can switch their regulator voltages between
voltage-A (active) and voltage-B (suspend) settings. Switching the
voltages can be controlled by ther internal state-machine or by a gpio
input signal and can be configured for each individual regulator. This
commit adds the gpio-based voltage switching support.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:
v3:
- make use of <linux/gpio/private.h>
- add comment to vsel_gpi
- append vsel_gpi to da9062_regulator_info instead of insert it in the
  middle

v2:
- use new public api gpiod_to_offset()
- add -ENOENT error check to mimic devm_gpio_optional
- add local gpio check for hardening the code
---
 drivers/regulator/da9062-regulator.c | 174 +++++++++++++++++++++++++++
 1 file changed, 174 insertions(+)

diff --git a/drivers/regulator/da9062-regulator.c b/drivers/regulator/da9062-regulator.c
index 710e67081d53..6117e631236b 100644
--- a/drivers/regulator/da9062-regulator.c
+++ b/drivers/regulator/da9062-regulator.c
@@ -7,6 +7,8 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/private.h>			/* for gpiod_to_offset() */
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -52,6 +54,16 @@ struct da9062_regulator_info {
 	unsigned int suspend_vsel_reg;
 	/* Event detection bit */
 	struct reg_field oc_event;
+	/*
+	 * The DA9062 can use its own general purpose inputs to control the
+	 * regulators. Each regulator can be configured independently. So the
+	 * DA9062 is a consumer of its own provided general purpose inputs.
+	 *
+	 * vsel_gpi:
+	 * The input port which is used by a regulator to select between
+	 * voltage-a/b settings.
+	 */
+	struct reg_field vsel_gpi;
 };
 
 /* Single regulator settings */
@@ -65,6 +77,7 @@ struct da9062_regulator {
 	struct regmap_field			*suspend;
 	struct regmap_field			*sleep;
 	struct regmap_field			*suspend_sleep;
+	struct regmap_field			*vsel_gpi;
 };
 
 /* Encapsulates all information for the regulators driver */
@@ -351,6 +364,81 @@ static const struct regulator_ops da9062_ldo_ops = {
 	.set_suspend_mode	= da9062_ldo_set_suspend_mode,
 };
 
+static int da9062_config_gpi(struct device_node *np,
+			     const struct regulator_desc *desc,
+			     struct regulator_config *cfg, const char *gpi_id)
+{
+	struct da9062_regulator *regl = cfg->driver_data;
+	struct device *hw = regl->hw->dev;
+	struct device_node *gpio_np;
+	struct gpio_desc *gpi;
+	unsigned int nr;
+	int ret;
+	char *prop, *label;
+
+	prop = kasprintf(GFP_KERNEL, "dlg,%s-sense-gpios", gpi_id);
+	if (!prop)
+		return -ENOMEM;
+
+	label = kasprintf(GFP_KERNEL, "%s-%s-gpi", desc->name, gpi_id);
+	if (!label) {
+		ret = -ENOMEM;
+		goto free;
+	}
+
+	/*
+	 * We only must ensure that the gpio device is probed before the
+	 * regulator driver so no need to store the reference global. Luckily
+	 * devm_* releases the gpio upon a remove action. The gpio's are
+	 * optional so we need to check for ENOENT. Also we need to check for
+	 * the GPIOLIB support. Do nothing if the property or the gpiolib is
+	 * missing.
+	 */
+	gpi = devm_gpiod_get_from_of_node(cfg->dev, np, prop, 0, GPIOD_IN |
+					  GPIOD_FLAGS_BIT_NONEXCLUSIVE, label);
+	if (IS_ERR(gpi)) {
+		ret = PTR_ERR(gpi);
+		if (ret == -ENOENT || ret == -ENOSYS)
+			ret = 0;
+		goto free;
+	}
+
+	/*
+	 * Only local gpios are valid. The gpio-controller is within the
+	 * mfd-root node.
+	 */
+	gpio_np = of_parse_phandle(np, prop, 0);
+	if (gpio_np != hw->of_node) {
+		of_node_put(gpio_np);
+		dev_err(hw, "Failed to request %s.\n", prop);
+		ret = -EINVAL;
+		goto free;
+	}
+	of_node_put(gpio_np);
+
+	/* We need the local number */
+	nr = gpiod_to_offset(gpi);
+	if (nr < 1 || nr > 3) {
+		ret = -EINVAL;
+		goto free;
+	}
+
+	ret = regmap_field_write(regl->vsel_gpi, nr);
+
+free:
+	kfree(prop);
+	kfree(label);
+
+	return ret;
+}
+
+static int da9062_parse_dt(struct device_node *np,
+			   const struct regulator_desc *desc,
+			   struct regulator_config *cfg)
+{
+	return da9062_config_gpi(np, desc, cfg, "vsel");
+}
+
 /* DA9061 Regulator information */
 static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 	{
@@ -358,6 +446,7 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 		.desc.name = "DA9061 BUCK1",
 		.desc.of_match = of_match_ptr("buck1"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_buck_ops,
 		.desc.min_uV = (300) * 1000,
 		.desc.uV_step = (10) * 1000,
@@ -388,12 +477,17 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_BUCK1_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_BUCK1_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_BUCK1_CONT,
+			__builtin_ffs((int)DA9062AA_VBUCK1_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VBUCK1_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9061_ID_BUCK2,
 		.desc.name = "DA9061 BUCK2",
 		.desc.of_match = of_match_ptr("buck2"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_buck_ops,
 		.desc.min_uV = (800) * 1000,
 		.desc.uV_step = (20) * 1000,
@@ -424,12 +518,17 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_BUCK3_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_BUCK3_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_BUCK3_CONT,
+			__builtin_ffs((int)DA9062AA_VBUCK3_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VBUCK3_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9061_ID_BUCK3,
 		.desc.name = "DA9061 BUCK3",
 		.desc.of_match = of_match_ptr("buck3"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_buck_ops,
 		.desc.min_uV = (530) * 1000,
 		.desc.uV_step = (10) * 1000,
@@ -460,12 +559,17 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_BUCK4_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_BUCK4_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_BUCK4_CONT,
+			__builtin_ffs((int)DA9062AA_VBUCK4_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VBUCK4_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9061_ID_LDO1,
 		.desc.name = "DA9061 LDO1",
 		.desc.of_match = of_match_ptr("ldo1"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_ldo_ops,
 		.desc.min_uV = (900) * 1000,
 		.desc.uV_step = (50) * 1000,
@@ -489,6 +593,10 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_LDO1_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_LDO1_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_LDO1_CONT,
+			__builtin_ffs((int)DA9062AA_VLDO1_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VLDO1_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO1_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -499,6 +607,7 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 		.desc.name = "DA9061 LDO2",
 		.desc.of_match = of_match_ptr("ldo2"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_ldo_ops,
 		.desc.min_uV = (900) * 1000,
 		.desc.uV_step = (50) * 1000,
@@ -522,6 +631,10 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_LDO2_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_LDO2_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_LDO2_CONT,
+			__builtin_ffs((int)DA9062AA_VLDO2_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VLDO2_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO2_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -532,6 +645,7 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 		.desc.name = "DA9061 LDO3",
 		.desc.of_match = of_match_ptr("ldo3"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_ldo_ops,
 		.desc.min_uV = (900) * 1000,
 		.desc.uV_step = (50) * 1000,
@@ -555,6 +669,10 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_LDO3_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_LDO3_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_LDO3_CONT,
+			__builtin_ffs((int)DA9062AA_VLDO3_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VLDO3_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO3_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -565,6 +683,7 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 		.desc.name = "DA9061 LDO4",
 		.desc.of_match = of_match_ptr("ldo4"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_ldo_ops,
 		.desc.min_uV = (900) * 1000,
 		.desc.uV_step = (50) * 1000,
@@ -588,6 +707,10 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_LDO4_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_LDO4_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_LDO4_CONT,
+			__builtin_ffs((int)DA9062AA_VLDO4_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VLDO4_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO4_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -602,6 +725,7 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 		.desc.name = "DA9062 BUCK1",
 		.desc.of_match = of_match_ptr("buck1"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_buck_ops,
 		.desc.min_uV = (300) * 1000,
 		.desc.uV_step = (10) * 1000,
@@ -632,12 +756,17 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_BUCK1_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_BUCK1_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_BUCK1_CONT,
+			__builtin_ffs((int)DA9062AA_VBUCK1_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VBUCK1_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9062_ID_BUCK2,
 		.desc.name = "DA9062 BUCK2",
 		.desc.of_match = of_match_ptr("buck2"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_buck_ops,
 		.desc.min_uV = (300) * 1000,
 		.desc.uV_step = (10) * 1000,
@@ -668,12 +797,17 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_BUCK2_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_BUCK2_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_BUCK2_CONT,
+			__builtin_ffs((int)DA9062AA_VBUCK2_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VBUCK2_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9062_ID_BUCK3,
 		.desc.name = "DA9062 BUCK3",
 		.desc.of_match = of_match_ptr("buck3"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_buck_ops,
 		.desc.min_uV = (800) * 1000,
 		.desc.uV_step = (20) * 1000,
@@ -704,12 +838,17 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_BUCK3_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_BUCK3_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_BUCK3_CONT,
+			__builtin_ffs((int)DA9062AA_VBUCK3_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VBUCK3_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9062_ID_BUCK4,
 		.desc.name = "DA9062 BUCK4",
 		.desc.of_match = of_match_ptr("buck4"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_buck_ops,
 		.desc.min_uV = (530) * 1000,
 		.desc.uV_step = (10) * 1000,
@@ -740,12 +879,17 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_BUCK4_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_BUCK4_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_BUCK4_CONT,
+			__builtin_ffs((int)DA9062AA_VBUCK4_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VBUCK4_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9062_ID_LDO1,
 		.desc.name = "DA9062 LDO1",
 		.desc.of_match = of_match_ptr("ldo1"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_ldo_ops,
 		.desc.min_uV = (900) * 1000,
 		.desc.uV_step = (50) * 1000,
@@ -769,6 +913,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_LDO1_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_LDO1_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_LDO1_CONT,
+			__builtin_ffs((int)DA9062AA_VLDO1_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VLDO1_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO1_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -779,6 +927,7 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 		.desc.name = "DA9062 LDO2",
 		.desc.of_match = of_match_ptr("ldo2"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_ldo_ops,
 		.desc.min_uV = (900) * 1000,
 		.desc.uV_step = (50) * 1000,
@@ -802,6 +951,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_LDO2_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_LDO2_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_LDO2_CONT,
+			__builtin_ffs((int)DA9062AA_VLDO2_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VLDO2_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO2_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -812,6 +965,7 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 		.desc.name = "DA9062 LDO3",
 		.desc.of_match = of_match_ptr("ldo3"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_ldo_ops,
 		.desc.min_uV = (900) * 1000,
 		.desc.uV_step = (50) * 1000,
@@ -835,6 +989,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_LDO3_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_LDO3_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_LDO3_CONT,
+			__builtin_ffs((int)DA9062AA_VLDO3_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VLDO3_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO3_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -845,6 +1003,7 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 		.desc.name = "DA9062 LDO4",
 		.desc.of_match = of_match_ptr("ldo4"),
 		.desc.regulators_node = of_match_ptr("regulators"),
+		.desc.of_parse_cb = da9062_parse_dt,
 		.desc.ops = &da9062_ldo_ops,
 		.desc.min_uV = (900) * 1000,
 		.desc.uV_step = (50) * 1000,
@@ -868,6 +1027,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_LDO4_CONF_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_LDO4_CONF_MASK) - 1),
+		.vsel_gpi = REG_FIELD(DA9062AA_LDO4_CONT,
+			__builtin_ffs((int)DA9062AA_VLDO4_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_VLDO4_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO4_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -988,6 +1151,15 @@ static int da9062_regulator_probe(struct platform_device *pdev)
 				return PTR_ERR(regl->suspend_sleep);
 		}
 
+		if (regl->info->vsel_gpi.reg) {
+			regl->vsel_gpi = devm_regmap_field_alloc(
+					&pdev->dev,
+					chip->regmap,
+					regl->info->vsel_gpi);
+			if (IS_ERR(regl->vsel_gpi))
+				return PTR_ERR(regl->vsel_gpi);
+		}
+
 		/* Register regulator */
 		memset(&config, 0, sizeof(config));
 		config.dev = chip->dev;
@@ -997,6 +1169,8 @@ static int da9062_regulator_probe(struct platform_device *pdev)
 		regl->rdev = devm_regulator_register(&pdev->dev, &regl->desc,
 						     &config);
 		if (IS_ERR(regl->rdev)) {
+			if (PTR_ERR(regl->rdev) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
 			dev_err(&pdev->dev,
 				"Failed to register %s regulator\n",
 				regl->desc.name);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v3 5/6] dt-bindings: mfd: da9062: add regulator gpio enable/disable documentation
  2019-11-29 17:25 [PATCH v3 0/6] DA9062 PMIC features Marco Felsch
                   ` (3 preceding siblings ...)
  2019-11-29 17:25 ` [PATCH v3 4/6] regulator: da9062: add voltage selection gpio support Marco Felsch
@ 2019-11-29 17:25 ` Marco Felsch
  2019-12-13 22:23   ` Rob Herring
  2019-12-16 16:31   ` Lee Jones
  2019-11-29 17:25 ` [PATCH v3 6/6] regulator: da9062: add gpio based regulator dis-/enable support Marco Felsch
  2019-12-02 11:44 ` [PATCH v3 0/6] DA9062 PMIC features Linus Walleij
  6 siblings, 2 replies; 38+ messages in thread
From: Marco Felsch @ 2019-11-29 17:25 UTC (permalink / raw)
  To: support.opensource, lee.jones, robh+dt, linus.walleij,
	bgolaszewski, joel, andrew, lgirdwood, broonie
  Cc: devicetree, linux-kernel, linux-gpio, linux-aspeed,
	linux-arm-kernel, kernel

At the gpio-based regulator enable/disable documentation. This property
can be applied to each subnode within the 'regulators' node so each
regulator can be configured differently.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:
v3:
- adapt binding description

 Documentation/devicetree/bindings/mfd/da9062.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt b/Documentation/devicetree/bindings/mfd/da9062.txt
index b9cccd4c3f76..863b1199d875 100644
--- a/Documentation/devicetree/bindings/mfd/da9062.txt
+++ b/Documentation/devicetree/bindings/mfd/da9062.txt
@@ -74,6 +74,13 @@ Sub-nodes:
     Attention: Sharing the same GPI for other purposes or across multiple
     regulators is possible but the polarity setting must equal.
 
+  - dlg,ena-sense-gpios : A GPIO reference to a local general purpose input,
+    the datasheet calls it GPI. The regulator sense the input signal and enable
+    or disable the regulator output. The regulator output is enabled if the
+    the signal is active else the output is disabled.
+    Attention: Sharing the same GPI for other purposes or across multiple
+    regulators is possible but the polarity setting must equal.
+
 - rtc : This node defines settings required for the Real-Time Clock associated
   with the DA9062. There are currently no entries in this binding, however
   compatible = "dlg,da9062-rtc" should be added if a node is created.
@@ -111,6 +118,7 @@ Example:
 				regulator-min-microvolt = <900000>;
 				regulator-max-microvolt = <3600000>;
 				regulator-boot-on;
+				dlg,ena-sense-gpios = <&pmic0 2 GPIO_ACTIVE_LOW>;
 			};
 		};
 	};
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v3 6/6] regulator: da9062: add gpio based regulator dis-/enable support
  2019-11-29 17:25 [PATCH v3 0/6] DA9062 PMIC features Marco Felsch
                   ` (4 preceding siblings ...)
  2019-11-29 17:25 ` [PATCH v3 5/6] dt-bindings: mfd: da9062: add regulator gpio enable/disable documentation Marco Felsch
@ 2019-11-29 17:25 ` Marco Felsch
  2019-12-02 11:44 ` [PATCH v3 0/6] DA9062 PMIC features Linus Walleij
  6 siblings, 0 replies; 38+ messages in thread
From: Marco Felsch @ 2019-11-29 17:25 UTC (permalink / raw)
  To: support.opensource, lee.jones, robh+dt, linus.walleij,
	bgolaszewski, joel, andrew, lgirdwood, broonie
  Cc: devicetree, linux-kernel, linux-gpio, linux-aspeed,
	linux-arm-kernel, kernel

Each regulator can be enabeld/disabled by the internal pmic state
machine or by a gpio input signal. Typically the OTP configures the
regulators to be enabled/disabled on a specific sequence number which is
most the time fine. Sometimes we need to reconfigure that due to a PCB
bug. This patch adds the support to disable/enable the regulator based
on a gpio input signal.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v3:
- add comment on reg_field
- append ena_gpi to da9062_regulator_info instead of insert it in the
  middle
---
 drivers/regulator/da9062-regulator.c | 88 +++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/da9062-regulator.c b/drivers/regulator/da9062-regulator.c
index 6117e631236b..79b08029282a 100644
--- a/drivers/regulator/da9062-regulator.c
+++ b/drivers/regulator/da9062-regulator.c
@@ -62,8 +62,13 @@ struct da9062_regulator_info {
 	 * vsel_gpi:
 	 * The input port which is used by a regulator to select between
 	 * voltage-a/b settings.
+	 *
+	 * ena_gpi:
+	 * The input port which is used by a regulator to en-/disable its
+	 * output.
 	 */
 	struct reg_field vsel_gpi;
+	struct reg_field ena_gpi;
 };
 
 /* Single regulator settings */
@@ -78,6 +83,7 @@ struct da9062_regulator {
 	struct regmap_field			*sleep;
 	struct regmap_field			*suspend_sleep;
 	struct regmap_field			*vsel_gpi;
+	struct regmap_field			*ena_gpi;
 };
 
 /* Encapsulates all information for the regulators driver */
@@ -423,7 +429,10 @@ static int da9062_config_gpi(struct device_node *np,
 		goto free;
 	}
 
-	ret = regmap_field_write(regl->vsel_gpi, nr);
+	if (!strncmp(gpi_id, "ena", 3))
+		ret = regmap_field_write(regl->ena_gpi, nr);
+	else
+		ret = regmap_field_write(regl->vsel_gpi, nr);
 
 free:
 	kfree(prop);
@@ -436,7 +445,13 @@ static int da9062_parse_dt(struct device_node *np,
 			   const struct regulator_desc *desc,
 			   struct regulator_config *cfg)
 {
-	return da9062_config_gpi(np, desc, cfg, "vsel");
+	int error;
+
+	error = da9062_config_gpi(np, desc, cfg, "vsel");
+	if (error)
+		return error;
+
+	return da9062_config_gpi(np, desc, cfg, "ena");
 }
 
 /* DA9061 Regulator information */
@@ -481,6 +496,10 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VBUCK1_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VBUCK1_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_BUCK1_CONT,
+			__builtin_ffs((int)DA9062AA_BUCK1_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_BUCK1_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9061_ID_BUCK2,
@@ -522,6 +541,10 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VBUCK3_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VBUCK3_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_BUCK3_CONT,
+			__builtin_ffs((int)DA9062AA_BUCK3_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_BUCK3_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9061_ID_BUCK3,
@@ -563,6 +586,10 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VBUCK4_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VBUCK4_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_BUCK4_CONT,
+			__builtin_ffs((int)DA9062AA_BUCK4_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_BUCK4_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9061_ID_LDO1,
@@ -597,6 +624,10 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VLDO1_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VLDO1_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_LDO1_CONT,
+			__builtin_ffs((int)DA9062AA_LDO1_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_LDO1_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO1_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -635,6 +666,10 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VLDO2_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VLDO2_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_LDO2_CONT,
+			__builtin_ffs((int)DA9062AA_LDO2_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_LDO2_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO2_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -673,6 +708,10 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VLDO3_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VLDO3_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_LDO3_CONT,
+			__builtin_ffs((int)DA9062AA_LDO3_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_LDO3_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO3_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -711,6 +750,10 @@ static const struct da9062_regulator_info local_da9061_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VLDO4_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VLDO4_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_LDO4_CONT,
+			__builtin_ffs((int)DA9062AA_LDO4_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_LDO4_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO4_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -760,6 +803,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VBUCK1_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VBUCK1_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_BUCK1_CONT,
+			__builtin_ffs((int)DA9062AA_BUCK1_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_BUCK1_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9062_ID_BUCK2,
@@ -801,6 +848,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VBUCK2_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VBUCK2_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_BUCK2_CONT,
+			__builtin_ffs((int)DA9062AA_BUCK2_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_BUCK2_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9062_ID_BUCK3,
@@ -842,6 +893,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VBUCK3_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VBUCK3_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_BUCK3_CONT,
+			__builtin_ffs((int)DA9062AA_BUCK3_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_BUCK3_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9062_ID_BUCK4,
@@ -883,6 +938,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VBUCK4_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VBUCK4_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_BUCK4_CONT,
+			__builtin_ffs((int)DA9062AA_BUCK4_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_BUCK4_GPI_MASK) - 1),
 	},
 	{
 		.desc.id = DA9062_ID_LDO1,
@@ -917,6 +976,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VLDO1_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VLDO1_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_LDO1_CONT,
+			__builtin_ffs((int)DA9062AA_LDO1_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_LDO1_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO1_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -955,6 +1018,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VLDO2_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VLDO2_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_LDO2_CONT,
+			__builtin_ffs((int)DA9062AA_LDO2_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_LDO2_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO2_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -993,6 +1060,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VLDO3_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VLDO3_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_LDO3_CONT,
+			__builtin_ffs((int)DA9062AA_LDO3_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_LDO3_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO3_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -1031,6 +1102,10 @@ static const struct da9062_regulator_info local_da9062_regulator_info[] = {
 			__builtin_ffs((int)DA9062AA_VLDO4_GPI_MASK) - 1,
 			sizeof(unsigned int) * 8 -
 			__builtin_clz(DA9062AA_VLDO4_GPI_MASK) - 1),
+		.ena_gpi = REG_FIELD(DA9062AA_LDO4_CONT,
+			__builtin_ffs((int)DA9062AA_LDO4_GPI_MASK) - 1,
+			sizeof(unsigned int) * 8 -
+			__builtin_clz(DA9062AA_LDO4_GPI_MASK) - 1),
 		.oc_event = REG_FIELD(DA9062AA_STATUS_D,
 			__builtin_ffs((int)DA9062AA_LDO4_ILIM_MASK) - 1,
 			sizeof(unsigned int) * 8 -
@@ -1160,6 +1235,15 @@ static int da9062_regulator_probe(struct platform_device *pdev)
 				return PTR_ERR(regl->vsel_gpi);
 		}
 
+		if (regl->info->ena_gpi.reg) {
+			regl->ena_gpi = devm_regmap_field_alloc(
+					&pdev->dev,
+					chip->regmap,
+					regl->info->ena_gpi);
+			if (IS_ERR(regl->ena_gpi))
+				return PTR_ERR(regl->ena_gpi);
+		}
+
 		/* Register regulator */
 		memset(&config, 0, sizeof(config));
 		config.dev = chip->dev;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 1/6] gpio: treewide rename gpio_chip_hwgpio to gpiod_to_offset
  2019-11-29 17:25 ` [PATCH v3 1/6] gpio: treewide rename gpio_chip_hwgpio to gpiod_to_offset Marco Felsch
@ 2019-12-02  2:59   ` Andrew Jeffery
  0 siblings, 0 replies; 38+ messages in thread
From: Andrew Jeffery @ 2019-12-02  2:59 UTC (permalink / raw)
  To: Marco Felsch, support.opensource, Lee Jones, Rob Herring,
	Linus Walleij, Bartosz Golaszewski, Joel Stanley, lgirdwood,
	broonie
  Cc: devicetree, linux-kernel, linux-gpio, linux-aspeed,
	linux-arm-kernel, Pengutronix Kernel Team



On Sat, 30 Nov 2019, at 03:55, Marco Felsch wrote:
> During discussion [1] we decided to rename the gpio subsystem local
> helper so the name is more meaningful for users outside the gpio
> subsystem. Making the helper public is done by a 2nd patch. The
> current users are the gpiolib itself and the aspeed gpio driver.
> The renaming is done by the following command:
> 
>     find ./drivers/gpio -type f -exec sed -i 
> 's/gpio_chip_hwgpio/gpiod_to_offset/g' {} \;
> 
> [1] https://lkml.org/lkml/2019/11/27/357
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Changelog:
> v3:
> - new patch
> 
>  drivers/gpio/gpio-aspeed.c   |  6 ++---

For the aspeed portion:

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 2/6] gpio: make gpiod_to_offset() available for other users
  2019-11-29 17:25 ` [PATCH v3 2/6] gpio: make gpiod_to_offset() available for other users Marco Felsch
@ 2019-12-02  3:00   ` Andrew Jeffery
  0 siblings, 0 replies; 38+ messages in thread
From: Andrew Jeffery @ 2019-12-02  3:00 UTC (permalink / raw)
  To: Marco Felsch, support.opensource, Lee Jones, Rob Herring,
	Linus Walleij, Bartosz Golaszewski, Joel Stanley, lgirdwood,
	broonie
  Cc: devicetree, linux-kernel, linux-gpio, linux-aspeed,
	linux-arm-kernel, Pengutronix Kernel Team



On Sat, 30 Nov 2019, at 03:55, Marco Felsch wrote:
> Currently gpiod_to_offset() is a gpio-subsystem private function which
> is used by the gpiolib itself and by the aspeed gpio driver. The time
> has shown that there are other drivers as well which need this
> information in some special cases e.g. MFD drivers. The patch makes the
> function public but you need to explicit add the <linux/gpio/private.h>
> include. See discussion [1] for more information.
> 
> [1] https://lkml.org/lkml/2019/11/27/357
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v3:
> - former patch description was "gpio: add support to get local gpio number"
> - adapt commit message and description to reflect new state
> - don't add wrapper instead use the already existing gpiod_to_offset
> - move gpiod_to_offset from gpiolib.h into gpiolib.c
> - move declaration into linux/gpio/private.h
> ---
>  drivers/gpio/gpio-aspeed.c   | 11 +++++------

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 0/6] DA9062 PMIC features
  2019-11-29 17:25 [PATCH v3 0/6] DA9062 PMIC features Marco Felsch
                   ` (5 preceding siblings ...)
  2019-11-29 17:25 ` [PATCH v3 6/6] regulator: da9062: add gpio based regulator dis-/enable support Marco Felsch
@ 2019-12-02 11:44 ` Linus Walleij
  2019-12-02 12:04   ` Lee Jones
  6 siblings, 1 reply; 38+ messages in thread
From: Linus Walleij @ 2019-12-02 11:44 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Support Opensource, Lee Jones, Rob Herring, Bartosz Golaszewski,
	Joel Stanley, Andrew Jeffery, Liam Girdwood, Mark Brown,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, open list:GPIO SUBSYSTEM, linux-aspeed, Linux ARM,
	Sascha Hauer

On Fri, Nov 29, 2019 at 6:25 PM Marco Felsch <m.felsch@pengutronix.de> wrote:

> this series address all comments made on [1]. Patch "gpio: add support
> to get local gpio number" is splitted into:
>  - "gpio: treewide rename gpio_chip_hwgpio to gpiod_to_offset"
>  - "gpio: make gpiod_to_offset() available for other users"
> Please check the discussion [1] for more information. You need to apply
> [2] to test the new features.

I am very happy with the shape of patches (1) and (2).

I can apply these on an immutable branch and merge into the
GPIO tree at v5.5-rc1 and offer to other subsystem maintainers
to pull in so they can merge the rest of the patch series on
top.

Alternatively I can merge all the patches into the GPIO tree.

I suppose this is not so much of a MFD business at this
point so whatever the regulator maintainer prefers I suppose?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 0/6] DA9062 PMIC features
  2019-12-02 11:44 ` [PATCH v3 0/6] DA9062 PMIC features Linus Walleij
@ 2019-12-02 12:04   ` Lee Jones
  0 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2019-12-02 12:04 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Marco Felsch, Support Opensource, Rob Herring,
	Bartosz Golaszewski, Joel Stanley, Andrew Jeffery, Liam Girdwood,
	Mark Brown,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, open list:GPIO SUBSYSTEM, linux-aspeed, Linux ARM,
	Sascha Hauer

On Mon, 02 Dec 2019, Linus Walleij wrote:

> On Fri, Nov 29, 2019 at 6:25 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
> > this series address all comments made on [1]. Patch "gpio: add support
> > to get local gpio number" is splitted into:
> >  - "gpio: treewide rename gpio_chip_hwgpio to gpiod_to_offset"
> >  - "gpio: make gpiod_to_offset() available for other users"
> > Please check the discussion [1] for more information. You need to apply
> > [2] to test the new features.
> 
> I am very happy with the shape of patches (1) and (2).
> 
> I can apply these on an immutable branch and merge into the
> GPIO tree at v5.5-rc1 and offer to other subsystem maintainers
> to pull in so they can merge the rest of the patch series on
> top.
> 
> Alternatively I can merge all the patches into the GPIO tree.
> 
> I suppose this is not so much of a MFD business at this
> point so whatever the regulator maintainer prefers I suppose?

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-11-29 17:25 ` [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation Marco Felsch
@ 2019-12-04 13:46   ` Mark Brown
  2019-12-10  9:41     ` Marco Felsch
  0 siblings, 1 reply; 38+ messages in thread
From: Mark Brown @ 2019-12-04 13:46 UTC (permalink / raw)
  To: Marco Felsch
  Cc: support.opensource, lee.jones, robh+dt, linus.walleij,
	bgolaszewski, joel, andrew, lgirdwood, devicetree, linux-kernel,
	linux-gpio, linux-aspeed, linux-arm-kernel, kernel

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

On Fri, Nov 29, 2019 at 06:25:34PM +0100, Marco Felsch wrote:

> +  Optional regulator device-specific properties:
> +  - dlg,vsel-sense-gpios : A GPIO reference to a local general purpose input,
> +    the datasheet calls it GPI. The regulator sense the input signal and select
> +    the active or suspend voltage settings. If the signal is active the
> +    active-settings are applied else the suspend-settings are applied.
> +    Attention: Sharing the same GPI for other purposes or across multiple
> +    regulators is possible but the polarity setting must equal.

I'm really confused by this.  As far as I understand it it seems
to be doing pinmuxing on the chip using the GPIO bindings which
is itself a bit odd and I don't see anything here that configures
whatever sets the state of the pins.  Don't we need another GPIO
to set the vsel-sense inputs on the PMIC?

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-04 13:46   ` Mark Brown
@ 2019-12-10  9:41     ` Marco Felsch
  2019-12-11 16:14       ` Adam Thomson
  0 siblings, 1 reply; 38+ messages in thread
From: Marco Felsch @ 2019-12-10  9:41 UTC (permalink / raw)
  To: Mark Brown
  Cc: support.opensource, lee.jones, robh+dt, linus.walleij,
	bgolaszewski, joel, andrew, lgirdwood, devicetree, linux-kernel,
	linux-gpio, linux-aspeed, linux-arm-kernel, kernel

Hi Mark,

On 19-12-04 13:46, Mark Brown wrote:
> On Fri, Nov 29, 2019 at 06:25:34PM +0100, Marco Felsch wrote:
> 
> > +  Optional regulator device-specific properties:
> > +  - dlg,vsel-sense-gpios : A GPIO reference to a local general purpose input,
> > +    the datasheet calls it GPI. The regulator sense the input signal and select
> > +    the active or suspend voltage settings. If the signal is active the
> > +    active-settings are applied else the suspend-settings are applied.
> > +    Attention: Sharing the same GPI for other purposes or across multiple
> > +    regulators is possible but the polarity setting must equal.
> 
> I'm really confused by this.  As far as I understand it it seems
> to be doing pinmuxing on the chip using the GPIO bindings which
> is itself a bit odd and I don't see anything here that configures
> whatever sets the state of the pins.  Don't we need another GPIO
> to set the vsel-sense inputs on the PMIC?

Yes the PMIC is very configurable and it took a while till I understand
it.. @Adam please correct me if I'm wrong.

The PMIC regulators regardless of the type: ldo or buck can be
simplified drawn as:



da9062-gpio               da9062-regulator
    
  +-------------------------------------------------------
  |                  PMIC
  |    
  > GPIO0            +--------------------------+
  |                  |         REGULATOR-0      |
  > GPIO1 -------+   |                          |
  |              +-- > vsel-in    voltage-a-out <
  > GPIO2        |   |                          |
  |              |   > enable-in  voltage-b-out <
  |              |   |                          |
  |              |   +--------------------------+
  |              |                              
  |              |   +--------------------------+                          
  |              |   |         REGULATOR-1      |                          
  |              |   |                          |                          
  |              +-- > vsel-in    voltage-a-out <                          
  |                  |                          |                          
  |                  > enable-in  voltage-b-out <
  |                  |                          |
  |                  +--------------------------+
  |

The 'vsel-in' and 'enable-in' regulator inputs must be routed to the
PMIC GPIOs which must be configured as input. If this is a pinmux in
your opinion, then yes we need to do that. IMHO it isn't a pinmux
because from the regulator point of view it is just a GPIO which comes
from our own gpio-dev (da9062-gpio). So the abstraction is vald. Anyway
I'm with you that this isn't the typical use-case.

Regards,
  Marco 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-10  9:41     ` Marco Felsch
@ 2019-12-11 16:14       ` Adam Thomson
  2019-12-11 17:09         ` Marco Felsch
  0 siblings, 1 reply; 38+ messages in thread
From: Adam Thomson @ 2019-12-11 16:14 UTC (permalink / raw)
  To: Marco Felsch, Mark Brown
  Cc: Support Opensource, lee.jones, robh+dt, linus.walleij,
	bgolaszewski, joel, andrew, lgirdwood, devicetree, linux-kernel,
	linux-gpio, linux-aspeed, linux-arm-kernel, kernel

On 10 December 2019 09:42, Marco Felsch wrote:

> Hi Mark,
> 
> On 19-12-04 13:46, Mark Brown wrote:
> > On Fri, Nov 29, 2019 at 06:25:34PM +0100, Marco Felsch wrote:
> >
> > > +  Optional regulator device-specific properties:
> > > +  - dlg,vsel-sense-gpios : A GPIO reference to a local general purpose input,
> > > +    the datasheet calls it GPI. The regulator sense the input signal and select
> > > +    the active or suspend voltage settings. If the signal is active the
> > > +    active-settings are applied else the suspend-settings are applied.
> > > +    Attention: Sharing the same GPI for other purposes or across multiple
> > > +    regulators is possible but the polarity setting must equal.
> >
> > I'm really confused by this.  As far as I understand it it seems
> > to be doing pinmuxing on the chip using the GPIO bindings which
> > is itself a bit odd and I don't see anything here that configures
> > whatever sets the state of the pins.  Don't we need another GPIO
> > to set the vsel-sense inputs on the PMIC?
> 
> Yes the PMIC is very configurable and it took a while till I understand
> it.. @Adam please correct me if I'm wrong.
> 
> The PMIC regulators regardless of the type: ldo or buck can be
> simplified drawn as:
> 
> 
> 
> da9062-gpio               da9062-regulator
> 
>   +-------------------------------------------------------
>   |                  PMIC
>   |
>   > GPIO0            +--------------------------+
>   |                  |         REGULATOR-0      |
>   > GPIO1 -------+   |                          |
>   |              +-- > vsel-in    voltage-a-out <
>   > GPIO2        |   |                          |
>   |              |   > enable-in  voltage-b-out <
>   |              |   |                          |
>   |              |   +--------------------------+
>   |              |
>   |              |   +--------------------------+
>   |              |   |         REGULATOR-1      |
>   |              |   |                          |
>   |              +-- > vsel-in    voltage-a-out <
>   |                  |                          |
>   |                  > enable-in  voltage-b-out <
>   |                  |                          |
>   |                  +--------------------------+
>   |
> 
> The 'vsel-in' and 'enable-in' regulator inputs must be routed to the
> PMIC GPIOs which must be configured as input. If this is a pinmux in
> your opinion, then yes we need to do that. IMHO it isn't a pinmux
> because from the regulator point of view it is just a GPIO which comes
> from our own gpio-dev (da9062-gpio). So the abstraction is vald. Anyway
> I'm with you that this isn't the typical use-case.

We've had this discussion before and to me it felt more like pinmux than GPIO
although I understand we're configuring the GPIO pin as input before then
configuring a regulator to take that specific internal GPIO as the control
signal. We're defining a specific role to this pin in HW rather than it being a
general software handled GPI so it feels like this would be neater under pinmux.
There does still need to be a mapping between that pin and the regulator which I
guess would be served by passing the pin to the regulator through generic pinmux
bindings and then in the regulator code you're simply just enabling the
regulator to be controlled from that pin. The HW lets you control multiple
regulators from the same input pin so there's a flexibility there to be
captured, as you mention.

> 
> Regards,
>   Marco
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-11 16:14       ` Adam Thomson
@ 2019-12-11 17:09         ` Marco Felsch
  2019-12-12 15:08           ` Linus Walleij
  2019-12-12 16:10           ` Mark Brown
  0 siblings, 2 replies; 38+ messages in thread
From: Marco Felsch @ 2019-12-11 17:09 UTC (permalink / raw)
  To: Adam Thomson
  Cc: Mark Brown, Support Opensource, lee.jones, robh+dt,
	linus.walleij, bgolaszewski, joel, andrew, lgirdwood, devicetree,
	linux-kernel, linux-gpio, linux-aspeed, linux-arm-kernel, kernel

Hi Adam,

On 19-12-11 16:14, Adam Thomson wrote:
> On 10 December 2019 09:42, Marco Felsch wrote:
> 
> > Hi Mark,
> > 
> > On 19-12-04 13:46, Mark Brown wrote:
> > > On Fri, Nov 29, 2019 at 06:25:34PM +0100, Marco Felsch wrote:
> > >
> > > > +  Optional regulator device-specific properties:
> > > > +  - dlg,vsel-sense-gpios : A GPIO reference to a local general purpose input,
> > > > +    the datasheet calls it GPI. The regulator sense the input signal and select
> > > > +    the active or suspend voltage settings. If the signal is active the
> > > > +    active-settings are applied else the suspend-settings are applied.
> > > > +    Attention: Sharing the same GPI for other purposes or across multiple
> > > > +    regulators is possible but the polarity setting must equal.
> > >
> > > I'm really confused by this.  As far as I understand it it seems
> > > to be doing pinmuxing on the chip using the GPIO bindings which
> > > is itself a bit odd and I don't see anything here that configures
> > > whatever sets the state of the pins.  Don't we need another GPIO
> > > to set the vsel-sense inputs on the PMIC?
> > 
> > Yes the PMIC is very configurable and it took a while till I understand
> > it.. @Adam please correct me if I'm wrong.
> > 
> > The PMIC regulators regardless of the type: ldo or buck can be
> > simplified drawn as:
> > 
> > 
> > 
> > da9062-gpio               da9062-regulator
> > 
> >   +-------------------------------------------------------
> >   |                  PMIC
> >   |
> >   > GPIO0            +--------------------------+
> >   |                  |         REGULATOR-0      |
> >   > GPIO1 -------+   |                          |
> >   |              +-- > vsel-in    voltage-a-out <
> >   > GPIO2        |   |                          |
> >   |              |   > enable-in  voltage-b-out <
> >   |              |   |                          |
> >   |              |   +--------------------------+
> >   |              |
> >   |              |   +--------------------------+
> >   |              |   |         REGULATOR-1      |
> >   |              |   |                          |
> >   |              +-- > vsel-in    voltage-a-out <
> >   |                  |                          |
> >   |                  > enable-in  voltage-b-out <
> >   |                  |                          |
> >   |                  +--------------------------+
> >   |
> > 
> > The 'vsel-in' and 'enable-in' regulator inputs must be routed to the
> > PMIC GPIOs which must be configured as input. If this is a pinmux in
> > your opinion, then yes we need to do that. IMHO it isn't a pinmux
> > because from the regulator point of view it is just a GPIO which comes
> > from our own gpio-dev (da9062-gpio). So the abstraction is vald. Anyway
> > I'm with you that this isn't the typical use-case.
> 
> We've had this discussion before and to me it felt more like pinmux than GPIO
> although I understand we're configuring the GPIO pin as input before then
> configuring a regulator to take that specific internal GPIO as the control
> signal. We're defining a specific role to this pin in HW rather than it being a
> general software handled GPI so it feels like this would be neater under pinmux.
> There does still need to be a mapping between that pin and the regulator which I
> guess would be served by passing the pin to the regulator through generic pinmux
> bindings and then in the regulator code you're simply just enabling the
> regulator to be controlled from that pin. The HW lets you control multiple
> regulators from the same input pin so there's a flexibility there to be
> captured, as you mention.

I know that we already had this discussion but the result was to wait
for the maintainers input. Since Linus is the pinctrl/gpio maintainer
and Mark the regulator maintainer we now have some input so we can move
forward. Linus made some comments on the dt-bindings and on the code but
he didn't pointed out that this usage is wrong. So I guessed it would be
fine for him. Mark did his first comments now and I explained the
current state..

I discussed it with a colleague again and he mentioned that pinctrl
should be named pinctrl instead it should be named padctrl. We don't
reconfigure the pad to a other function it is still a device general
purpose input pad. The hw-signal flow goes always trough the gpio block
so one argument more for my solution. Also we don't configure the "pad"
to be a vsel/ena-pin. The hw-pad can only be a gpio or has an alternate
function (WDKICK for GPIO0, Seq. SYS_EN for GPIO2, Seq. PWR_EN for GPIO4).
Instead we tell the regulator to use _this_ GPIO e.g. for voltage
selection so we go the other way around. My last argument why pinctrl
isn't the correct place is that the GPIO1 can be used for
regulator-0:vsel-in and for regulator-1:enable-in. So this pad would
have different states which is invalid IMHO.

Regards,
  Marco

> > Regards,
> >   Marco
> > 
> > --
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-11 17:09         ` Marco Felsch
@ 2019-12-12 15:08           ` Linus Walleij
  2019-12-12 15:55             ` Marco Felsch
  2019-12-12 16:10           ` Mark Brown
  1 sibling, 1 reply; 38+ messages in thread
From: Linus Walleij @ 2019-12-12 15:08 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Adam Thomson, Mark Brown, Support Opensource, lee.jones, robh+dt,
	bgolaszewski, joel, andrew, lgirdwood, devicetree, linux-kernel,
	linux-gpio, linux-aspeed, linux-arm-kernel, kernel

On Wed, Dec 11, 2019 at 6:09 PM Marco Felsch <m.felsch@pengutronix.de> wrote:

> I discussed it with a colleague again and he mentioned that pinctrl
> should be named pinctrl instead it should be named padctrl.

Quoting Documentation/driver-api/pinctl.rst:

(...)
Definition of PIN:

- PINS are equal to pads, fingers, balls or whatever packaging input or
  output line you want to control and these are denoted by unsigned integers
  in the range 0..maxpin.
(...)

> We don't
> reconfigure the pad to a other function it is still a device general
> purpose input pad. The hw-signal flow goes always trough the gpio block
> so one argument more for my solution. Also we don't configure the "pad"
> to be a vsel/ena-pin. The hw-pad can only be a gpio or has an alternate
> function (WDKICK for GPIO0, Seq. SYS_EN for GPIO2, Seq. PWR_EN for GPIO4).
> Instead we tell the regulator to use _this_ GPIO e.g. for voltage
> selection so we go the other way around. My last argument why pinctrl
> isn't the correct place is that the GPIO1 can be used for
> regulator-0:vsel-in and for regulator-1:enable-in. So this pad would
> have different states which is invalid IMHO.

Yeah it is just one of these cases where the silicon designer pulled
a line of polysilicone over to the regulator enable signal and put a
switch on it and say "so you can also enable the regulator
with a signal from here", it can be used in parallel with anything
else, which is especially messy.

Special cases require special handling, since the electronic design
of this thing is a bit Rube Goldberg.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-12 15:08           ` Linus Walleij
@ 2019-12-12 15:55             ` Marco Felsch
  0 siblings, 0 replies; 38+ messages in thread
From: Marco Felsch @ 2019-12-12 15:55 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Adam Thomson, Mark Brown, Support Opensource, lee.jones, robh+dt,
	bgolaszewski, joel, andrew, lgirdwood, devicetree, linux-kernel,
	linux-gpio, linux-aspeed, linux-arm-kernel, kernel

Hi,

On 19-12-12 16:08, Linus Walleij wrote:
> On Wed, Dec 11, 2019 at 6:09 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
> > I discussed it with a colleague again and he mentioned that pinctrl
> > should be named pinctrl instead it should be named padctrl.
> 
> Quoting Documentation/driver-api/pinctl.rst:
> 
> (...)
> Definition of PIN:
> 
> - PINS are equal to pads, fingers, balls or whatever packaging input or
>   output line you want to control and these are denoted by unsigned integers
>   in the range 0..maxpin.
> (...)

Okay there is the definition.

> > We don't
> > reconfigure the pad to a other function it is still a device general
> > purpose input pad. The hw-signal flow goes always trough the gpio block
> > so one argument more for my solution. Also we don't configure the "pad"
> > to be a vsel/ena-pin. The hw-pad can only be a gpio or has an alternate
> > function (WDKICK for GPIO0, Seq. SYS_EN for GPIO2, Seq. PWR_EN for GPIO4).
> > Instead we tell the regulator to use _this_ GPIO e.g. for voltage
> > selection so we go the other way around. My last argument why pinctrl
> > isn't the correct place is that the GPIO1 can be used for
> > regulator-0:vsel-in and for regulator-1:enable-in. So this pad would
> > have different states which is invalid IMHO.
> 
> Yeah it is just one of these cases where the silicon designer pulled
> a line of polysilicone over to the regulator enable signal and put a
> switch on it and say "so you can also enable the regulator
> with a signal from here", it can be used in parallel with anything
> else, which is especially messy.

I didn't say that the design isn't messy ;) I just wanna make the right
abstraction and IMHO this is the correct abstraction.

Regards,
  Marco

> Special cases require special handling, since the electronic design
> of this thing is a bit Rube Goldberg.
> 
> Yours,
> Linus Walleij
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-11 17:09         ` Marco Felsch
  2019-12-12 15:08           ` Linus Walleij
@ 2019-12-12 16:10           ` Mark Brown
  2019-12-12 16:21             ` Marco Felsch
  1 sibling, 1 reply; 38+ messages in thread
From: Mark Brown @ 2019-12-12 16:10 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Adam Thomson, Support Opensource, lee.jones, robh+dt,
	linus.walleij, bgolaszewski, joel, andrew, lgirdwood, devicetree,
	linux-kernel, linux-gpio, linux-aspeed, linux-arm-kernel, kernel

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

On Wed, Dec 11, 2019 at 06:09:18PM +0100, Marco Felsch wrote:

> so one argument more for my solution. Also we don't configure the "pad"
> to be a vsel/ena-pin. The hw-pad can only be a gpio or has an alternate
> function (WDKICK for GPIO0, Seq. SYS_EN for GPIO2, Seq. PWR_EN for GPIO4).
> Instead we tell the regulator to use _this_ GPIO e.g. for voltage
> selection so we go the other way around. My last argument why pinctrl
> isn't the correct place is that the GPIO1 can be used for
> regulator-0:vsel-in and for regulator-1:enable-in. So this pad would
> have different states which is invalid IMHO.

Note that there's two bits to my concern - one is if we should be using
gpiolib or pinctrl, the other is what's driving the input (whichever API
it's configured through) which didn't seem to be mentioned.

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-12 16:10           ` Mark Brown
@ 2019-12-12 16:21             ` Marco Felsch
  2019-12-12 16:51               ` Mark Brown
  2019-12-16 12:28               ` Linus Walleij
  0 siblings, 2 replies; 38+ messages in thread
From: Marco Felsch @ 2019-12-12 16:21 UTC (permalink / raw)
  To: Mark Brown
  Cc: Adam Thomson, Support Opensource, lee.jones, robh+dt,
	linus.walleij, bgolaszewski, joel, andrew, lgirdwood, devicetree,
	linux-kernel, linux-gpio, linux-aspeed, linux-arm-kernel, kernel

On 19-12-12 16:10, Mark Brown wrote:
> On Wed, Dec 11, 2019 at 06:09:18PM +0100, Marco Felsch wrote:
> 
> > so one argument more for my solution. Also we don't configure the "pad"
> > to be a vsel/ena-pin. The hw-pad can only be a gpio or has an alternate
> > function (WDKICK for GPIO0, Seq. SYS_EN for GPIO2, Seq. PWR_EN for GPIO4).
> > Instead we tell the regulator to use _this_ GPIO e.g. for voltage
> > selection so we go the other way around. My last argument why pinctrl
> > isn't the correct place is that the GPIO1 can be used for
> > regulator-0:vsel-in and for regulator-1:enable-in. So this pad would
> > have different states which is invalid IMHO.
> 
> Note that there's two bits to my concern - one is if we should be using
> gpiolib or pinctrl, the other is what's driving the input (whichever API
> it's configured through) which didn't seem to be mentioned.

gpiolib or pinctrl:
I pointed out all my arguments above so I think it is time for Linus.

"... what's driving the input ..":
Sorry I didn't get you here. What did you mean? The input is driven by
the host. This can be any gpio line and in my case it is a gpio line
driven by the soc-hw during a suspend operation.

Regards,
  Marco

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-12 16:21             ` Marco Felsch
@ 2019-12-12 16:51               ` Mark Brown
  2019-12-16  8:55                 ` Marco Felsch
  2019-12-16 12:28               ` Linus Walleij
  1 sibling, 1 reply; 38+ messages in thread
From: Mark Brown @ 2019-12-12 16:51 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Adam Thomson, Support Opensource, lee.jones, robh+dt,
	linus.walleij, bgolaszewski, joel, andrew, lgirdwood, devicetree,
	linux-kernel, linux-gpio, linux-aspeed, linux-arm-kernel, kernel

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

On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:

> "... what's driving the input ..":
> Sorry I didn't get you here. What did you mean? The input is driven by
> the host. This can be any gpio line and in my case it is a gpio line
> driven by the soc-hw during a suspend operation.

Something needs to say what that thing is, especially if it's runtime
controllable.  In your case from the point of view of software there is
actually no enable control so we shouldn't be providing an enable
operation to the framework.

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 5/6] dt-bindings: mfd: da9062: add regulator gpio enable/disable documentation
  2019-11-29 17:25 ` [PATCH v3 5/6] dt-bindings: mfd: da9062: add regulator gpio enable/disable documentation Marco Felsch
@ 2019-12-13 22:23   ` Rob Herring
  2019-12-16 16:31   ` Lee Jones
  1 sibling, 0 replies; 38+ messages in thread
From: Rob Herring @ 2019-12-13 22:23 UTC (permalink / raw)
  To: Marco Felsch
  Cc: support.opensource, lee.jones, robh+dt, linus.walleij,
	bgolaszewski, joel, andrew, lgirdwood, broonie, devicetree,
	linux-kernel, linux-gpio, linux-aspeed, linux-arm-kernel, kernel

On Fri, 29 Nov 2019 18:25:36 +0100, Marco Felsch wrote:
> At the gpio-based regulator enable/disable documentation. This property
> can be applied to each subnode within the 'regulators' node so each
> regulator can be configured differently.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Changelog:
> v3:
> - adapt binding description
> 
>  Documentation/devicetree/bindings/mfd/da9062.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-12 16:51               ` Mark Brown
@ 2019-12-16  8:55                 ` Marco Felsch
  2019-12-16 11:44                   ` Mark Brown
                                     ` (2 more replies)
  0 siblings, 3 replies; 38+ messages in thread
From: Marco Felsch @ 2019-12-16  8:55 UTC (permalink / raw)
  To: Mark Brown
  Cc: devicetree, Support Opensource, linux-aspeed, linux-gpio, andrew,
	linus.walleij, lgirdwood, linux-kernel, bgolaszewski, robh+dt,
	joel, kernel, Adam Thomson, lee.jones, linux-arm-kernel

On 19-12-12 16:51, Mark Brown wrote:
> On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:
> 
> > "... what's driving the input ..":
> > Sorry I didn't get you here. What did you mean? The input is driven by
> > the host. This can be any gpio line and in my case it is a gpio line
> > driven by the soc-hw during a suspend operation.
> 
> Something needs to say what that thing is, especially if it's runtime
> controllable.  In your case from the point of view of software there is
> actually no enable control so we shouldn't be providing an enable
> operation to the framework.

The enabel control signal is always available, please check [1] table
63. There is a mux in front of the enable pin so:

             +-------------
 Seq. |\     |   Regulator
 GPI1 | \    |
 GPI2 | | -- > Enable
 GPI3 | /    |
      |/     .
             .
             .

Adam please correct me if this is wrong.

[1] https://www.dialog-semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf

Regards,
  Marco

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-16  8:55                 ` Marco Felsch
@ 2019-12-16 11:44                   ` Mark Brown
  2019-12-17  7:35                     ` Marco Felsch
  2019-12-16 16:32                   ` Adam Thomson
  2019-12-16 16:32                   ` Adam Thomson
  2 siblings, 1 reply; 38+ messages in thread
From: Mark Brown @ 2019-12-16 11:44 UTC (permalink / raw)
  To: Marco Felsch
  Cc: devicetree, Support Opensource, linux-aspeed, linux-gpio, andrew,
	linus.walleij, lgirdwood, linux-kernel, bgolaszewski, robh+dt,
	joel, kernel, Adam Thomson, lee.jones, linux-arm-kernel

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

On Mon, Dec 16, 2019 at 09:55:25AM +0100, Marco Felsch wrote:
> On 19-12-12 16:51, Mark Brown wrote:

> > Something needs to say what that thing is, especially if it's runtime
> > controllable.  In your case from the point of view of software there is
> > actually no enable control so we shouldn't be providing an enable
> > operation to the framework.

> The enabel control signal is always available, please check [1] table
> 63. There is a mux in front of the enable pin so:

What I'm saying is that I think the binding needs to explicitly talk
about that since at the minute it's really confusing reading it as it
is, it sounds very much like it's trying to override that in a chip
specific fashion as using gpiolib and the GPIO bindings for pinmuxing is
really quite unusual.

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-12 16:21             ` Marco Felsch
  2019-12-12 16:51               ` Mark Brown
@ 2019-12-16 12:28               ` Linus Walleij
  1 sibling, 0 replies; 38+ messages in thread
From: Linus Walleij @ 2019-12-16 12:28 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Mark Brown, Adam Thomson, Support Opensource, lee.jones, robh+dt,
	bgolaszewski, joel, andrew, lgirdwood, devicetree, linux-kernel,
	linux-gpio, linux-aspeed, linux-arm-kernel, kernel

On Thu, Dec 12, 2019 at 5:21 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
> On 19-12-12 16:10, Mark Brown wrote:

> > Note that there's two bits to my concern - one is if we should be using
> > gpiolib or pinctrl, the other is what's driving the input (whichever API
> > it's configured through) which didn't seem to be mentioned.
>
> gpiolib or pinctrl:
> I pointed out all my arguments above so I think it is time for Linus.

I think I've elaborated on this?

The API is fine with me, because this thing does some autonomous
control and I don't know any better way to share that same line
with the GPIO subsystem than to make this offset available to
the regulator driver.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 5/6] dt-bindings: mfd: da9062: add regulator gpio enable/disable documentation
  2019-11-29 17:25 ` [PATCH v3 5/6] dt-bindings: mfd: da9062: add regulator gpio enable/disable documentation Marco Felsch
  2019-12-13 22:23   ` Rob Herring
@ 2019-12-16 16:31   ` Lee Jones
  1 sibling, 0 replies; 38+ messages in thread
From: Lee Jones @ 2019-12-16 16:31 UTC (permalink / raw)
  To: Marco Felsch
  Cc: support.opensource, robh+dt, linus.walleij, bgolaszewski, joel,
	andrew, lgirdwood, broonie, devicetree, linux-kernel, linux-gpio,
	linux-aspeed, linux-arm-kernel, kernel

On Fri, 29 Nov 2019, Marco Felsch wrote:

> At the gpio-based regulator enable/disable documentation. This property
> can be applied to each subnode within the 'regulators' node so each
> regulator can be configured differently.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Changelog:
> v3:
> - adapt binding description
> 
>  Documentation/devicetree/bindings/mfd/da9062.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-16  8:55                 ` Marco Felsch
  2019-12-16 11:44                   ` Mark Brown
  2019-12-16 16:32                   ` Adam Thomson
@ 2019-12-16 16:32                   ` Adam Thomson
  2 siblings, 0 replies; 38+ messages in thread
From: Adam Thomson @ 2019-12-16 16:32 UTC (permalink / raw)
  To: Marco Felsch, Mark Brown
  Cc: devicetree, Support Opensource, linux-aspeed, linux-gpio, andrew,
	linus.walleij, lgirdwood, linux-kernel, bgolaszewski, robh+dt,
	joel, kernel, Adam Thomson, lee.jones, linux-arm-kernel



> -----Original Message-----
> From: Marco Felsch <m.felsch@pengutronix.de>
> Sent: 16 December 2019 08:55
> To: Mark Brown <broonie@kernel.org>
> Cc: devicetree@vger.kernel.org; Support Opensource
> <Support.Opensource@diasemi.com>; linux-aspeed@lists.ozlabs.org; linux-
> gpio@vger.kernel.org; andrew@aj.id.au; linus.walleij@linaro.org;
> lgirdwood@gmail.com; linux-kernel@vger.kernel.org;
> bgolaszewski@baylibre.com; robh+dt@kernel.org; joel@jms.id.au;
> kernel@pengutronix.de; Adam Thomson
> <Adam.Thomson.Opensource@diasemi.com>; lee.jones@linaro.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage
> selection documentation
> 
> On 19-12-12 16:51, Mark Brown wrote:
> > On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:
> >
> > > "... what's driving the input ..":
> > > Sorry I didn't get you here. What did you mean? The input is driven by
> > > the host. This can be any gpio line and in my case it is a gpio line
> > > driven by the soc-hw during a suspend operation.
> >
> > Something needs to say what that thing is, especially if it's runtime
> > controllable.  In your case from the point of view of software there is
> > actually no enable control so we shouldn't be providing an enable
> > operation to the framework.
> 
> The enabel control signal is always available, please check [1] table
> 63. There is a mux in front of the enable pin so:
> 
>              +-------------
>  Seq. |\     |   Regulator
>  GPI1 | \    |
>  GPI2 | | -- > Enable
>  GPI3 | /    |
>       |/     .
>              .
>              .
> 
> Adam please correct me if this is wrong.
> 
> [1] https://www.dialog-
> semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf
> 
> Regards,
>   Marco
> 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-16  8:55                 ` Marco Felsch
  2019-12-16 11:44                   ` Mark Brown
@ 2019-12-16 16:32                   ` Adam Thomson
  2019-12-17  9:00                     ` Marco Felsch
  2019-12-16 16:32                   ` Adam Thomson
  2 siblings, 1 reply; 38+ messages in thread
From: Adam Thomson @ 2019-12-16 16:32 UTC (permalink / raw)
  To: Marco Felsch, Mark Brown
  Cc: devicetree, Support Opensource, linux-aspeed, linux-gpio, andrew,
	linus.walleij, lgirdwood, linux-kernel, bgolaszewski, robh+dt,
	joel, kernel, Adam Thomson, lee.jones, linux-arm-kernel

On 16 December 2019 08:55, Marco Felsch wrote:

> On 19-12-12 16:51, Mark Brown wrote:
> > On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:
> >
> > > "... what's driving the input ..":
> > > Sorry I didn't get you here. What did you mean? The input is driven by
> > > the host. This can be any gpio line and in my case it is a gpio line
> > > driven by the soc-hw during a suspend operation.
> >
> > Something needs to say what that thing is, especially if it's runtime
> > controllable.  In your case from the point of view of software there is
> > actually no enable control so we shouldn't be providing an enable
> > operation to the framework.
> 
> The enabel control signal is always available, please check [1] table
> 63. There is a mux in front of the enable pin so:
> 
>              +-------------
>  Seq. |\     |   Regulator
>  GPI1 | \    |
>  GPI2 | | -- > Enable
>  GPI3 | /    |
>       |/     .
>              .
>              .
> 
> Adam please correct me if this is wrong.

Yes the register can always be configured regardless of the associated pin
configuration, but if say GPIO1 was configured as a GPO but a regulator was
configured to use GPIO1 as its GPI control mechanism, the output signal from
GPIO1 would be ignored, the sequencer control would not have any effect and
you're simply left with manual I2C control. Really we shouldn't be getting into
that situation though. If a GPIO is to be used as a regulator control signal
then it should be marked as such and I don't think we should be able to use that
pin for anything other than regulator control.

> 
> [1] https://www.dialog-
> semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf
> 
> Regards,
>   Marco
> 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-16 11:44                   ` Mark Brown
@ 2019-12-17  7:35                     ` Marco Felsch
  2019-12-17 12:58                       ` Mark Brown
  0 siblings, 1 reply; 38+ messages in thread
From: Marco Felsch @ 2019-12-17  7:35 UTC (permalink / raw)
  To: Mark Brown
  Cc: devicetree, Support Opensource, linux-aspeed, bgolaszewski,
	andrew, linus.walleij, lgirdwood, linux-kernel, linux-gpio,
	robh+dt, joel, kernel, Adam Thomson, lee.jones, linux-arm-kernel

On 19-12-16 11:44, Mark Brown wrote:
> On Mon, Dec 16, 2019 at 09:55:25AM +0100, Marco Felsch wrote:
> > On 19-12-12 16:51, Mark Brown wrote:
> 
> > > Something needs to say what that thing is, especially if it's runtime
> > > controllable.  In your case from the point of view of software there is
> > > actually no enable control so we shouldn't be providing an enable
> > > operation to the framework.
> 
> > The enabel control signal is always available, please check [1] table
> > 63. There is a mux in front of the enable pin so:
> 
> What I'm saying is that I think the binding needs to explicitly talk
> about that since at the minute it's really confusing reading it as it
> is, it sounds very much like it's trying to override that in a chip
> specific fashion as using gpiolib and the GPIO bindings for pinmuxing is
> really quite unusual.

Hm.. I still think that we don't mux the pin to some special function.
It is still a gpio input pin and if we don't request the pin we could
read the input from user-space too and get a 'valid' value. Muxing would
happen if we change the pad to so called _alternate_ function. Anyway,
lets find a binding description:

name:
 - dlg,vsel-sense-gpios

IMHO this is very descriptive and needs no update.

description:
 - A GPIO reference to a local general purpose input, [1] calls it GPI.
   The DA9062 regulators can select between voltage-a/-b settings.
   Each regulator has a VBUCK*_GPI or VLDO*_GPI input to determine the
   active setting. In front of the VBUCK*_GPI/VLDO*_GPI input is a mux
   to select between different signal sources, valid sources are: the
   internal sequencer, GPI1, GPI2 and GPI3. See [1] table 63 for more
   information. Most the time the internal sequencer is fine but
   sometimes it is necessary to use the signal from the DA9062 GPI
   pads. This binding covers the second use case.
   Attention: Sharing the same GPI for other purposes or across multiple
   regulators is possible but the polarity setting must equal.

[1] https://www.dialog-semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf

Regards,
  Marco


> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-16 16:32                   ` Adam Thomson
@ 2019-12-17  9:00                     ` Marco Felsch
  2019-12-17  9:12                       ` Marco Felsch
  2019-12-17  9:53                       ` Adam Thomson
  0 siblings, 2 replies; 38+ messages in thread
From: Marco Felsch @ 2019-12-17  9:00 UTC (permalink / raw)
  To: Adam Thomson
  Cc: Mark Brown, devicetree, Support Opensource, linux-aspeed,
	bgolaszewski, andrew, linus.walleij, lgirdwood, linux-kernel,
	linux-gpio, robh+dt, joel, kernel, lee.jones, linux-arm-kernel

On 19-12-16 16:32, Adam Thomson wrote:
> On 16 December 2019 08:55, Marco Felsch wrote:
> 
> > On 19-12-12 16:51, Mark Brown wrote:
> > > On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:
> > >
> > > > "... what's driving the input ..":
> > > > Sorry I didn't get you here. What did you mean? The input is driven by
> > > > the host. This can be any gpio line and in my case it is a gpio line
> > > > driven by the soc-hw during a suspend operation.
> > >
> > > Something needs to say what that thing is, especially if it's runtime
> > > controllable.  In your case from the point of view of software there is
> > > actually no enable control so we shouldn't be providing an enable
> > > operation to the framework.
> > 
> > The enabel control signal is always available, please check [1] table
> > 63. There is a mux in front of the enable pin so:
> > 
> >              +-------------
> >  Seq. |\     |   Regulator
> >  GPI1 | \    |
> >  GPI2 | | -- > Enable
> >  GPI3 | /    |
> >       |/     .
> >              .
> >              .
> > 
> > Adam please correct me if this is wrong.
> 
> Yes the register can always be configured regardless of the associated pin
> configuration, but if say GPIO1 was configured as a GPO but a regulator was
> configured to use GPIO1 as its GPI control mechanism, the output signal from
> GPIO1 would be ignored, the sequencer control would not have any effect and
> you're simply left with manual I2C control. Really we shouldn't be getting into
> that situation though. If a GPIO is to be used as a regulator control signal
> then it should be marked as such and I don't think we should be able to use that
> pin for anything other than regulator control.

I see, so we have to guarantee that the requested gpio is configured as
input. This can be done by:

  if (gpi->flags & FLAG_IS_OUT)
  	return -EINVAL;

Regards,
  Marco

> > 
> > [1] https://www.dialog-
> > semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf
> > 
> > Regards,
> >   Marco
> > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-17  9:00                     ` Marco Felsch
@ 2019-12-17  9:12                       ` Marco Felsch
  2019-12-17  9:53                       ` Adam Thomson
  1 sibling, 0 replies; 38+ messages in thread
From: Marco Felsch @ 2019-12-17  9:12 UTC (permalink / raw)
  To: Adam Thomson
  Cc: devicetree, Support Opensource, linux-aspeed, linux-gpio, andrew,
	linus.walleij, lgirdwood, robh+dt, linux-kernel, bgolaszewski,
	Mark Brown, joel, kernel, lee.jones, linux-arm-kernel

On 19-12-17 10:00, Marco Felsch wrote:
> On 19-12-16 16:32, Adam Thomson wrote:
> > On 16 December 2019 08:55, Marco Felsch wrote:
> > 
> > > On 19-12-12 16:51, Mark Brown wrote:
> > > > On Thu, Dec 12, 2019 at 05:21:53PM +0100, Marco Felsch wrote:
> > > >
> > > > > "... what's driving the input ..":
> > > > > Sorry I didn't get you here. What did you mean? The input is driven by
> > > > > the host. This can be any gpio line and in my case it is a gpio line
> > > > > driven by the soc-hw during a suspend operation.
> > > >
> > > > Something needs to say what that thing is, especially if it's runtime
> > > > controllable.  In your case from the point of view of software there is
> > > > actually no enable control so we shouldn't be providing an enable
> > > > operation to the framework.
> > > 
> > > The enabel control signal is always available, please check [1] table
> > > 63. There is a mux in front of the enable pin so:
> > > 
> > >              +-------------
> > >  Seq. |\     |   Regulator
> > >  GPI1 | \    |
> > >  GPI2 | | -- > Enable
> > >  GPI3 | /    |
> > >       |/     .
> > >              .
> > >              .
> > > 
> > > Adam please correct me if this is wrong.
> > 
> > Yes the register can always be configured regardless of the associated pin
> > configuration, but if say GPIO1 was configured as a GPO but a regulator was
> > configured to use GPIO1 as its GPI control mechanism, the output signal from
> > GPIO1 would be ignored, the sequencer control would not have any effect and
> > you're simply left with manual I2C control. Really we shouldn't be getting into
> > that situation though. If a GPIO is to be used as a regulator control signal
> > then it should be marked as such and I don't think we should be able to use that
> > pin for anything other than regulator control.
> 
> I see, so we have to guarantee that the requested gpio is configured as
> input. This can be done by:
> 
>   if (gpi->flags & FLAG_IS_OUT)
>   	return -EINVAL;

Sorry didn't noticed that the flags are only used internally. The
correct check must be:

	/* GPIO must be configured as input */
	if (!gpiod_get_direction(gpi)) {
		ret = -EINVAL;
		goto free;
	}

> 
> Regards,
>   Marco
> 
> > > 
> > > [1] https://www.dialog-
> > > semiconductor.com/sites/default/files/da9062_datasheet_3v6.pdf
> > > 
> > > Regards,
> > >   Marco
> > > 
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-17  9:00                     ` Marco Felsch
  2019-12-17  9:12                       ` Marco Felsch
@ 2019-12-17  9:53                       ` Adam Thomson
  2019-12-17 12:31                         ` Marco Felsch
  1 sibling, 1 reply; 38+ messages in thread
From: Adam Thomson @ 2019-12-17  9:53 UTC (permalink / raw)
  To: Marco Felsch, Adam Thomson
  Cc: Mark Brown, devicetree, Support Opensource, linux-aspeed,
	bgolaszewski, andrew, linus.walleij, lgirdwood, linux-kernel,
	linux-gpio, robh+dt, joel, kernel, lee.jones, linux-arm-kernel

On 17 December 2019 09:01, Marco Felsch wrote:

> > > The enabel control signal is always available, please check [1] table
> > > 63. There is a mux in front of the enable pin so:
> > >
> > >              +-------------
> > >  Seq. |\     |   Regulator
> > >  GPI1 | \    |
> > >  GPI2 | | -- > Enable
> > >  GPI3 | /    |
> > >       |/     .
> > >              .
> > >              .
> > >
> > > Adam please correct me if this is wrong.
> >
> > Yes the register can always be configured regardless of the associated pin
> > configuration, but if say GPIO1 was configured as a GPO but a regulator was
> > configured to use GPIO1 as its GPI control mechanism, the output signal from
> > GPIO1 would be ignored, the sequencer control would not have any effect and
> > you're simply left with manual I2C control. Really we shouldn't be getting into
> > that situation though. If a GPIO is to be used as a regulator control signal
> > then it should be marked as such and I don't think we should be able to use that
> > pin for anything other than regulator control.
> 
> I see, so we have to guarantee that the requested gpio is configured as
> input. This can be done by:

This is one of the reasons I thought this was better suited to being done in the
pinctrl/pinmux side. If you configure the GPIO as for regulator control then
the code can automatically configure the GPIO for input. That doesn't then need
to be in the regulator driver.

But yes we wouldn't really want to configure a regulator to be controlled via a
GPI when it's configured as a GPO as it makes no sense.

> 
>   if (gpi->flags & FLAG_IS_OUT)
>   	return -EINVAL;
> 
> Regards,
>   Marco

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-17  9:53                       ` Adam Thomson
@ 2019-12-17 12:31                         ` Marco Felsch
  2019-12-17 13:13                           ` Adam Thomson
  0 siblings, 1 reply; 38+ messages in thread
From: Marco Felsch @ 2019-12-17 12:31 UTC (permalink / raw)
  To: Adam Thomson
  Cc: Mark Brown, devicetree, Support Opensource, linux-aspeed,
	bgolaszewski, andrew, linus.walleij, lgirdwood, linux-kernel,
	linux-gpio, robh+dt, joel, kernel, lee.jones, linux-arm-kernel

On 19-12-17 09:53, Adam Thomson wrote:
> On 17 December 2019 09:01, Marco Felsch wrote:
> 
> > > > The enabel control signal is always available, please check [1] table
> > > > 63. There is a mux in front of the enable pin so:
> > > >
> > > >              +-------------
> > > >  Seq. |\     |   Regulator
> > > >  GPI1 | \    |
> > > >  GPI2 | | -- > Enable
> > > >  GPI3 | /    |
> > > >       |/     .
> > > >              .
> > > >              .
> > > >
> > > > Adam please correct me if this is wrong.
> > >
> > > Yes the register can always be configured regardless of the associated pin
> > > configuration, but if say GPIO1 was configured as a GPO but a regulator was
> > > configured to use GPIO1 as its GPI control mechanism, the output signal from
> > > GPIO1 would be ignored, the sequencer control would not have any effect and
> > > you're simply left with manual I2C control. Really we shouldn't be getting into
> > > that situation though. If a GPIO is to be used as a regulator control signal
> > > then it should be marked as such and I don't think we should be able to use that
> > > pin for anything other than regulator control.
> > 
> > I see, so we have to guarantee that the requested gpio is configured as
> > input. This can be done by:
> 
> This is one of the reasons I thought this was better suited to being done in the
> pinctrl/pinmux side. If you configure the GPIO as for regulator control then
> the code can automatically configure the GPIO for input. That doesn't then need
> to be in the regulator driver.

I still don't prefer that way.. pls check my arguments I already made
and I don't wanna repeat it again.

> But yes we wouldn't really want to configure a regulator to be controlled via a
> GPI when it's configured as a GPO as it makes no sense.

Okay, so the check is all we need to hardening the driver against wrong
usage :)

Regards,
  Marco

> 
> > 
> >   if (gpi->flags & FLAG_IS_OUT)
> >   	return -EINVAL;
> > 
> > Regards,
> >   Marco
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-17  7:35                     ` Marco Felsch
@ 2019-12-17 12:58                       ` Mark Brown
  2020-01-07  8:36                         ` Marco Felsch
  0 siblings, 1 reply; 38+ messages in thread
From: Mark Brown @ 2019-12-17 12:58 UTC (permalink / raw)
  To: Marco Felsch
  Cc: devicetree, Support Opensource, linux-aspeed, bgolaszewski,
	andrew, linus.walleij, lgirdwood, linux-kernel, linux-gpio,
	robh+dt, joel, kernel, Adam Thomson, lee.jones, linux-arm-kernel

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

On Tue, Dec 17, 2019 at 08:35:33AM +0100, Marco Felsch wrote:
> On 19-12-16 11:44, Mark Brown wrote:

> > What I'm saying is that I think the binding needs to explicitly talk
> > about that since at the minute it's really confusing reading it as it
> > is, it sounds very much like it's trying to override that in a chip
> > specific fashion as using gpiolib and the GPIO bindings for pinmuxing is
> > really quite unusual.

> Hm.. I still think that we don't mux the pin to some special function.
> It is still a gpio input pin and if we don't request the pin we could
> read the input from user-space too and get a 'valid' value. Muxing would
> happen if we change the pad to so called _alternate_ function. Anyway,
> lets find a binding description:

I don't think any of this makes much difference from a user point of
view.

> IMHO this is very descriptive and needs no update.

> description:
>  - A GPIO reference to a local general purpose input, [1] calls it GPI.
>    The DA9062 regulators can select between voltage-a/-b settings.
>    Each regulator has a VBUCK*_GPI or VLDO*_GPI input to determine the
>    active setting. In front of the VBUCK*_GPI/VLDO*_GPI input is a mux
>    to select between different signal sources, valid sources are: the
>    internal sequencer, GPI1, GPI2 and GPI3. See [1] table 63 for more
>    information. Most the time the internal sequencer is fine but
>    sometimes it is necessary to use the signal from the DA9062 GPI
>    pads. This binding covers the second use case.
>    Attention: Sharing the same GPI for other purposes or across multiple
>    regulators is possible but the polarity setting must equal.

This doesn't say anything about how the GPIO input is expected to be
controlled, for voltage setting any runtime control would need to be
done by the driver and it sounds like that's all that can be controlled.
The way this reads I'd expect one use of this to be for fast voltage
setting for example (you could even combine that with suspend sequencing
using the internal sequencer if you mux back to the sequencer during
suspend).

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-17 12:31                         ` Marco Felsch
@ 2019-12-17 13:13                           ` Adam Thomson
  0 siblings, 0 replies; 38+ messages in thread
From: Adam Thomson @ 2019-12-17 13:13 UTC (permalink / raw)
  To: Marco Felsch, Adam Thomson
  Cc: Mark Brown, devicetree, Support Opensource, linux-aspeed,
	bgolaszewski, andrew, linus.walleij, lgirdwood, linux-kernel,
	linux-gpio, robh+dt, joel, kernel, lee.jones, linux-arm-kernel

On 17 December 2019 12:31, Marco Felsch wrote:

> On 19-12-17 09:53, Adam Thomson wrote:
> > On 17 December 2019 09:01, Marco Felsch wrote:
> >
> > > > > The enabel control signal is always available, please check [1] table
> > > > > 63. There is a mux in front of the enable pin so:
> > > > >
> > > > >              +-------------
> > > > >  Seq. |\     |   Regulator
> > > > >  GPI1 | \    |
> > > > >  GPI2 | | -- > Enable
> > > > >  GPI3 | /    |
> > > > >       |/     .
> > > > >              .
> > > > >              .
> > > > >
> > > > > Adam please correct me if this is wrong.
> > > >
> > > > Yes the register can always be configured regardless of the associated pin
> > > > configuration, but if say GPIO1 was configured as a GPO but a regulator was
> > > > configured to use GPIO1 as its GPI control mechanism, the output signal
> from
> > > > GPIO1 would be ignored, the sequencer control would not have any effect
> and
> > > > you're simply left with manual I2C control. Really we shouldn't be getting
> into
> > > > that situation though. If a GPIO is to be used as a regulator control signal
> > > > then it should be marked as such and I don't think we should be able to use
> that
> > > > pin for anything other than regulator control.
> > >
> > > I see, so we have to guarantee that the requested gpio is configured as
> > > input. This can be done by:
> >
> > This is one of the reasons I thought this was better suited to being done in the
> > pinctrl/pinmux side. If you configure the GPIO as for regulator control then
> > the code can automatically configure the GPIO for input. That doesn't then
> need
> > to be in the regulator driver.
> 
> I still don't prefer that way.. pls check my arguments I already made
> and I don't wanna repeat it again.

Yes, I read your arguments but still can't agree :)

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2019-12-17 12:58                       ` Mark Brown
@ 2020-01-07  8:36                         ` Marco Felsch
  2020-01-07 13:09                           ` Mark Brown
  0 siblings, 1 reply; 38+ messages in thread
From: Marco Felsch @ 2020-01-07  8:36 UTC (permalink / raw)
  To: Mark Brown
  Cc: devicetree, Support Opensource, linux-aspeed, bgolaszewski,
	andrew, linus.walleij, lgirdwood, linux-kernel, linux-gpio,
	robh+dt, joel, kernel, Adam Thomson, lee.jones, linux-arm-kernel

Hi Mark,

On 19-12-17 12:58, Mark Brown wrote:
> On Tue, Dec 17, 2019 at 08:35:33AM +0100, Marco Felsch wrote:
> > On 19-12-16 11:44, Mark Brown wrote:
> 
> > > What I'm saying is that I think the binding needs to explicitly talk
> > > about that since at the minute it's really confusing reading it as it
> > > is, it sounds very much like it's trying to override that in a chip
> > > specific fashion as using gpiolib and the GPIO bindings for pinmuxing is
> > > really quite unusual.
> 
> > Hm.. I still think that we don't mux the pin to some special function.
> > It is still a gpio input pin and if we don't request the pin we could
> > read the input from user-space too and get a 'valid' value. Muxing would
> > happen if we change the pad to so called _alternate_ function. Anyway,
> > lets find a binding description:
> 
> I don't think any of this makes much difference from a user point of
> view.
> 
> > IMHO this is very descriptive and needs no update.
> 
> > description:
> >  - A GPIO reference to a local general purpose input, [1] calls it GPI.
> >    The DA9062 regulators can select between voltage-a/-b settings.
> >    Each regulator has a VBUCK*_GPI or VLDO*_GPI input to determine the
> >    active setting. In front of the VBUCK*_GPI/VLDO*_GPI input is a mux
> >    to select between different signal sources, valid sources are: the
> >    internal sequencer, GPI1, GPI2 and GPI3. See [1] table 63 for more
> >    information. Most the time the internal sequencer is fine but
> >    sometimes it is necessary to use the signal from the DA9062 GPI
> >    pads. This binding covers the second use case.
> >    Attention: Sharing the same GPI for other purposes or across multiple
> >    regulators is possible but the polarity setting must equal.
> 
> This doesn't say anything about how the GPIO input is expected to be
> controlled, for voltage setting any runtime control would need to be
> done by the driver and it sounds like that's all that can be controlled.
> The way this reads I'd expect one use of this to be for fast voltage
> setting for example (you could even combine that with suspend sequencing
> using the internal sequencer if you mux back to the sequencer during
> suspend).

The input signal is routed trough the da9062 gpio block to the
regualtors. You can't set any voltage value using a gpio instead you
decide which voltage setting is applied. The voltage values for
runtime/suspend comes from the dt-data. No it's not just a fast
switching option imagine the system suspend case where the cpu and soc
voltage can be reduced to a very low value. Older soc's like the imx6
signaling this state by a hard wired gpio line because the soc and
cpu cores don't work properly on such low voltage values. This is
my use case and I can't use the sequencer.

Regards,
  Marco


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2020-01-07  8:36                         ` Marco Felsch
@ 2020-01-07 13:09                           ` Mark Brown
  2020-01-07 13:38                             ` Marco Felsch
  0 siblings, 1 reply; 38+ messages in thread
From: Mark Brown @ 2020-01-07 13:09 UTC (permalink / raw)
  To: Marco Felsch
  Cc: devicetree, Support Opensource, linux-aspeed, bgolaszewski,
	andrew, linus.walleij, lgirdwood, linux-kernel, linux-gpio,
	robh+dt, joel, kernel, Adam Thomson, lee.jones, linux-arm-kernel

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

On Tue, Jan 07, 2020 at 09:36:54AM +0100, Marco Felsch wrote:
> On 19-12-17 12:58, Mark Brown wrote:

> > This doesn't say anything about how the GPIO input is expected to be
> > controlled, for voltage setting any runtime control would need to be
> > done by the driver and it sounds like that's all that can be controlled.
> > The way this reads I'd expect one use of this to be for fast voltage
> > setting for example (you could even combine that with suspend sequencing
> > using the internal sequencer if you mux back to the sequencer during
> > suspend).

> The input signal is routed trough the da9062 gpio block to the
> regualtors. You can't set any voltage value using a gpio instead you
> decide which voltage setting is applied. The voltage values for
> runtime/suspend comes from the dt-data. No it's not just a fast
> switching option imagine the system suspend case where the cpu and soc
> voltage can be reduced to a very low value. Older soc's like the imx6
> signaling this state by a hard wired gpio line because the soc and
> cpu cores don't work properly on such low voltage values. This is
> my use case and I can't use the sequencer.

My point is that I can't tell any of this from the description.

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2020-01-07 13:09                           ` Mark Brown
@ 2020-01-07 13:38                             ` Marco Felsch
  2020-01-14 15:43                               ` Mark Brown
  0 siblings, 1 reply; 38+ messages in thread
From: Marco Felsch @ 2020-01-07 13:38 UTC (permalink / raw)
  To: Mark Brown
  Cc: devicetree, Support Opensource, linux-aspeed, bgolaszewski,
	andrew, linus.walleij, lgirdwood, linux-kernel, linux-gpio,
	robh+dt, joel, kernel, Adam Thomson, lee.jones, linux-arm-kernel

On 20-01-07 13:09, Mark Brown wrote:
> On Tue, Jan 07, 2020 at 09:36:54AM +0100, Marco Felsch wrote:
> > On 19-12-17 12:58, Mark Brown wrote:
> 
> > > This doesn't say anything about how the GPIO input is expected to be
> > > controlled, for voltage setting any runtime control would need to be
> > > done by the driver and it sounds like that's all that can be controlled.
> > > The way this reads I'd expect one use of this to be for fast voltage
> > > setting for example (you could even combine that with suspend sequencing
> > > using the internal sequencer if you mux back to the sequencer during
> > > suspend).
> 
> > The input signal is routed trough the da9062 gpio block to the
> > regualtors. You can't set any voltage value using a gpio instead you
> > decide which voltage setting is applied. The voltage values for
> > runtime/suspend comes from the dt-data. No it's not just a fast
> > switching option imagine the system suspend case where the cpu and soc
> > voltage can be reduced to a very low value. Older soc's like the imx6
> > signaling this state by a hard wired gpio line because the soc and
> > cpu cores don't work properly on such low voltage values. This is
> > my use case and I can't use the sequencer.
> 
> My point is that I can't tell any of this from the description.

Therefore I want to discuss the dt-binding documentation with you and
the others to get this done. Is the above description better to
understand the dt-binding?

Regards,
  Marco

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation
  2020-01-07 13:38                             ` Marco Felsch
@ 2020-01-14 15:43                               ` Mark Brown
  0 siblings, 0 replies; 38+ messages in thread
From: Mark Brown @ 2020-01-14 15:43 UTC (permalink / raw)
  To: Marco Felsch
  Cc: devicetree, Support Opensource, linux-aspeed, bgolaszewski,
	andrew, linus.walleij, lgirdwood, linux-kernel, linux-gpio,
	robh+dt, joel, kernel, Adam Thomson, lee.jones, linux-arm-kernel

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

On Tue, Jan 07, 2020 at 02:38:11PM +0100, Marco Felsch wrote:
> On 20-01-07 13:09, Mark Brown wrote:
> > On Tue, Jan 07, 2020 at 09:36:54AM +0100, Marco Felsch wrote:

> > > The input signal is routed trough the da9062 gpio block to the
> > > regualtors. You can't set any voltage value using a gpio instead you
> > > decide which voltage setting is applied. The voltage values for
> > > runtime/suspend comes from the dt-data. No it's not just a fast
> > > switching option imagine the system suspend case where the cpu and soc
> > > voltage can be reduced to a very low value. Older soc's like the imx6
> > > signaling this state by a hard wired gpio line because the soc and
> > > cpu cores don't work properly on such low voltage values. This is
> > > my use case and I can't use the sequencer.

> > My point is that I can't tell any of this from the description.

> Therefore I want to discuss the dt-binding documentation with you and
> the others to get this done. Is the above description better to
> understand the dt-binding?

That text really doesn't feel like text that'd be idiomatic
directly in a binding document but some of those ideas probably
do need to be in the text I think.

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2020-01-14 15:43 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29 17:25 [PATCH v3 0/6] DA9062 PMIC features Marco Felsch
2019-11-29 17:25 ` [PATCH v3 1/6] gpio: treewide rename gpio_chip_hwgpio to gpiod_to_offset Marco Felsch
2019-12-02  2:59   ` Andrew Jeffery
2019-11-29 17:25 ` [PATCH v3 2/6] gpio: make gpiod_to_offset() available for other users Marco Felsch
2019-12-02  3:00   ` Andrew Jeffery
2019-11-29 17:25 ` [PATCH v3 3/6] dt-bindings: mfd: da9062: add regulator voltage selection documentation Marco Felsch
2019-12-04 13:46   ` Mark Brown
2019-12-10  9:41     ` Marco Felsch
2019-12-11 16:14       ` Adam Thomson
2019-12-11 17:09         ` Marco Felsch
2019-12-12 15:08           ` Linus Walleij
2019-12-12 15:55             ` Marco Felsch
2019-12-12 16:10           ` Mark Brown
2019-12-12 16:21             ` Marco Felsch
2019-12-12 16:51               ` Mark Brown
2019-12-16  8:55                 ` Marco Felsch
2019-12-16 11:44                   ` Mark Brown
2019-12-17  7:35                     ` Marco Felsch
2019-12-17 12:58                       ` Mark Brown
2020-01-07  8:36                         ` Marco Felsch
2020-01-07 13:09                           ` Mark Brown
2020-01-07 13:38                             ` Marco Felsch
2020-01-14 15:43                               ` Mark Brown
2019-12-16 16:32                   ` Adam Thomson
2019-12-17  9:00                     ` Marco Felsch
2019-12-17  9:12                       ` Marco Felsch
2019-12-17  9:53                       ` Adam Thomson
2019-12-17 12:31                         ` Marco Felsch
2019-12-17 13:13                           ` Adam Thomson
2019-12-16 16:32                   ` Adam Thomson
2019-12-16 12:28               ` Linus Walleij
2019-11-29 17:25 ` [PATCH v3 4/6] regulator: da9062: add voltage selection gpio support Marco Felsch
2019-11-29 17:25 ` [PATCH v3 5/6] dt-bindings: mfd: da9062: add regulator gpio enable/disable documentation Marco Felsch
2019-12-13 22:23   ` Rob Herring
2019-12-16 16:31   ` Lee Jones
2019-11-29 17:25 ` [PATCH v3 6/6] regulator: da9062: add gpio based regulator dis-/enable support Marco Felsch
2019-12-02 11:44 ` [PATCH v3 0/6] DA9062 PMIC features Linus Walleij
2019-12-02 12:04   ` Lee Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).