[PATCH v3 0/4] GPIO "active low", hogging and usb-nop-xceive 'reset-gpio' support

[PATCH v3 0/4] GPIO "active low", hogging and usb-nop-xceive 'reset-gpio' support

[Andrey Smirnov]

Hi everyone,

This is a v3 of my original patchset. Changes in this version are the following:

  - new flags, GPIOF_INIT_[IN]ACTIVE and GPIOF_OUT_INIT_[IN]ACTIVE are
    added to avoid semantically overloading the meaing of "HIGH/LOW"
    variants of those

  - added commentary about GPIO 'hog' interpretation of the meaning of
    'output-high', 'output-low'

Changes since v1:

  - 'active low' support is moved inot a dedicated API
  - of_gpiochip_scan_gpios renamed to of_gpiochip_scan_hogs
  - nop_usbphy_init doesn't try to needlessly configure reset gpio's directionality

Let me know if anything else needs changing.

Thanks,
Andrey Smirnov

Andrey Smirnov (4):
  gpio-imx: Do not use gpio_set_value()
  gpiolib: Add code to support "active low" GPIOs
  gpiolib: Add support for GPIO "hog" nodes
  usb-nop-xceiv: Add support for 'reset-gpios' binding

 drivers/gpio/gpio-imx.c     |   2 +-
 drivers/gpio/gpiolib.c      | 146 ++++++++++++++++++++++++++++++++++++++++++--
 drivers/phy/usb-nop-xceiv.c |  45 ++++++++++++--
 include/gpio.h              |  25 ++++++++
 4 files changed, 208 insertions(+), 10 deletions(-)

-- 
2.9.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v3 1/4] gpio-imx: Do not use gpio_set_value()

[Andrey Smirnov]

Do not use gpio_set_value() in imx_gpio_direction_output().  We don't
check gpio_set_value's result, so calling imx_gpio_set_value()
directly instead should be as good.

Cc: cphealy@gmail.com
Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Acked-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/gpio/gpio-imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-imx.c b/drivers/gpio/gpio-imx.c
index ab4f596..a9d44d4 100644
--- a/drivers/gpio/gpio-imx.c
+++ b/drivers/gpio/gpio-imx.c
@@ -93,7 +93,7 @@ static int imx_gpio_direction_output(struct gpio_chip *chip, unsigned gpio, int
 	void __iomem *base = imxgpio->base;
 	u32 val;
 
-	gpio_set_value(gpio + chip->base, value);
+	imx_gpio_set_value(chip, gpio, value);
 
 	val = readl(base + imxgpio->regs->gdir);
 	val |= 1 << gpio;
-- 
2.9.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v3 2/4] gpiolib: Add code to support "active low" GPIOs

[Andrey Smirnov]

So far this particular aspect of various DT-bindings has been handled
on a per-driver basis. With this change, hopefully, we'll have a
single place to handle necessary logic inversions and eventually
would be able to migrate existing users as well as avoiding adding
redundant code to new drivers.

Cc: cphealy@gmail.com
Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/gpio/gpiolib.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++----
 include/gpio.h         | 25 +++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1f57c76..28f6a5e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -13,6 +13,7 @@ static LIST_HEAD(chip_list);
 struct gpio_info {
 	struct gpio_chip *chip;
 	bool requested;
+	bool active_low;
 	char *label;
 };
 
@@ -45,6 +46,15 @@ static struct gpio_info *gpio_to_desc(unsigned gpio)
 	return NULL;
 }
 
+static int gpio_adjust_value(struct gpio_info *gi,
+			     int value)
+{
+	if (value < 0)
+		return value;
+
+	return !!value ^ gi->active_low;
+}
+
 int gpio_request(unsigned gpio, const char *label)
 {
 	struct gpio_info *gi = gpio_to_desc(gpio);
@@ -69,6 +79,7 @@ int gpio_request(unsigned gpio, const char *label)
 	}
 
 	gi->requested = true;
+	gi->active_low = false;
 	gi->label = xstrdup(label);
 
 done:
@@ -93,6 +104,7 @@ void gpio_free(unsigned gpio)
 		gi->chip->ops->free(gi->chip, gpio - gi->chip->base);
 
 	gi->requested = false;
+	gi->active_low = false;
 	free(gi->label);
 	gi->label = NULL;
 }
@@ -111,11 +123,20 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 	if (err)
 		return err;
 
-	if (flags & GPIOF_DIR_IN)
+	if (flags & GPIOF_ACTIVE_LOW) {
+		struct gpio_info *gi = gpio_to_desc(gpio);
+		gi->active_low = true;
+	}
+
+	if (flags & GPIOF_DIR_IN) {
 		err = gpio_direction_input(gpio);
-	else
+	} else if (flags & GPIOF_LOGICAL) {
+		err = gpio_direction_active(gpio,
+					    !!(flags & GPIOF_INIT_ACTIVE));
+	} else {
 		err = gpio_direction_output(gpio,
-				(flags & GPIOF_INIT_HIGH) ? 1 : 0);
+					    !!(flags & GPIOF_INIT_HIGH));
+	}
 
 	if (err)
 		goto free_gpio;
@@ -178,6 +199,13 @@ void gpio_set_value(unsigned gpio, int value)
 }
 EXPORT_SYMBOL(gpio_set_value);
 
+void gpio_set_active(unsigned gpio, bool value)
+{
+	struct gpio_info *gi = gpio_to_desc(gpio);
+	gpio_set_value(gpio, gpio_adjust_value(gi, value));
+}
+EXPORT_SYMBOL(gpio_set_active);
+
 int gpio_get_value(unsigned gpio)
 {
 	struct gpio_info *gi = gpio_to_desc(gpio);
@@ -196,6 +224,13 @@ int gpio_get_value(unsigned gpio)
 }
 EXPORT_SYMBOL(gpio_get_value);
 
+int gpio_is_active(unsigned gpio)
+{
+	struct gpio_info *gi = gpio_to_desc(gpio);
+	return gpio_adjust_value(gi, gpio_get_value(gpio));
+}
+EXPORT_SYMBOL(gpio_is_active);
+
 int gpio_direction_output(unsigned gpio, int value)
 {
 	struct gpio_info *gi = gpio_to_desc(gpio);
@@ -215,6 +250,13 @@ int gpio_direction_output(unsigned gpio, int value)
 }
 EXPORT_SYMBOL(gpio_direction_output);
 
+int gpio_direction_active(unsigned gpio, bool value)
+{
+	struct gpio_info *gi = gpio_to_desc(gpio);
+	return gpio_direction_output(gpio, gpio_adjust_value(gi, value));
+}
+EXPORT_SYMBOL(gpio_direction_active);
+
 int gpio_direction_input(unsigned gpio)
 {
 	struct gpio_info *gi = gpio_to_desc(gpio);
@@ -334,7 +376,7 @@ static int do_gpiolib(int argc, char *argv[])
 		printf("  GPIO %*d: %*s %*s %*s  %s\n", 4, i,
 			3, (dir < 0) ? "unk" : ((dir == GPIOF_DIR_IN) ? "in" : "out"),
 			3, (val < 0) ? "unk" : ((val == 0) ? "lo" : "hi"),
-			9, gi->requested ? "true" : "false",
+		        12, gi->requested ? (gi->active_low ? "active low" : "true") : "false",
 			(gi->requested && gi->label) ? gi->label : "");
 	}
 
diff --git a/include/gpio.h b/include/gpio.h
index 7b3f512..56aae22 100644
--- a/include/gpio.h
+++ b/include/gpio.h
@@ -6,6 +6,11 @@ void gpio_set_value(unsigned gpio, int value);
 int gpio_get_value(unsigned gpio);
 int gpio_direction_output(unsigned gpio, int value);
 int gpio_direction_input(unsigned gpio);
+
+void gpio_set_active(unsigned gpio, bool state);
+int gpio_is_active(unsigned gpio);
+int gpio_direction_active(unsigned gpio, bool state);
+
 #else
 static inline void gpio_set_value(unsigned gpio, int value)
 {
@@ -22,6 +27,18 @@ static inline int gpio_direction_input(unsigned gpio)
 {
 	return -EINVAL;
 }
+
+static inline void gpio_set_active(unsigned gpio, int value)
+{
+}
+static inline int gpio_is_active(unsigned gpio)
+{
+	return 0;
+}
+static inline int gpio_direction_active(unsigned gpio, int value)
+{
+	return -EINVAL;
+}
 #endif
 
 #define ARCH_NR_GPIOS 256
@@ -45,6 +62,14 @@ static inline int gpio_is_valid(int gpio)
 #define GPIOF_OUT_INIT_LOW	(GPIOF_DIR_OUT | GPIOF_INIT_LOW)
 #define GPIOF_OUT_INIT_HIGH	(GPIOF_DIR_OUT | GPIOF_INIT_HIGH)
 
+#define GPIOF_LOGICAL		BIT(2)
+#define GPIOF_ACTIVE_HIGH	GPIOF_LOGICAL
+#define GPIOF_ACTIVE_LOW	(BIT(3) | GPIOF_LOGICAL)
+#define GPIOF_INIT_INACTIVE	GPIOF_LOGICAL
+#define GPIOF_INIT_ACTIVE	(GPIOF_LOGICAL | GPIOF_INIT_HIGH)
+#define GPIOF_OUT_INIT_ACTIVE	(GPIOF_DIR_OUT | GPIOF_INIT_ACTIVE)
+#define GPIOF_OUT_INIT_INACTIVE	(GPIOF_DIR_OUT | GPIOF_INIT_INACTIVE)
+
 /**
  * struct gpio - a structure describing a GPIO with configuration
  * @gpio:	the GPIO number
-- 
2.9.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v3 3/4] gpiolib: Add support for GPIO "hog" nodes

[Andrey Smirnov]

Add code to support 'gpio-hog' nodes used in some .dts files in Linux
kernel.

Cc: cphealy@gmail.com
Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/gpio/gpiolib.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 28f6a5e..a3e17ad 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -5,6 +5,7 @@
 #include <command.h>
 #include <complete.h>
 #include <gpio.h>
+#include <of_gpio.h>
 #include <errno.h>
 #include <malloc.h>
 
@@ -304,6 +305,99 @@ static int gpiochip_find_base(int start, int ngpio)
 	return base;
 }
 
+static int of_hog_gpio(struct device_node *np, struct gpio_chip *chip,
+		       unsigned int idx)
+{
+	struct device_node *chip_np = chip->dev->device_node;
+	unsigned long flags = 0;
+	u32 gpio_cells, gpio_num, gpio_flags;
+	int ret, gpio;
+	const char *name = NULL;
+
+	ret = of_property_read_u32(chip_np, "#gpio-cells", &gpio_cells);
+	if (ret)
+		return ret;
+
+	/*
+	 * Support for GPIOs that don't have #gpio-cells set to 2 is
+	 * not implemented
+	 */
+	if (WARN_ON(gpio_cells != 2))
+		return -ENOTSUPP;
+
+	ret = of_property_read_u32_index(np, "gpios", idx * gpio_cells,
+					 &gpio_num);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32_index(np, "gpios", idx * gpio_cells + 1,
+					 &gpio_flags);
+	if (ret)
+		return ret;
+
+	if (gpio_flags & OF_GPIO_ACTIVE_LOW)
+		flags |= GPIOF_ACTIVE_LOW;
+
+	gpio = gpio_get_num(chip->dev, gpio_num);
+	if (ret == -EPROBE_DEFER)
+		return ret;
+
+	if (ret < 0) {
+		dev_err(chip->dev, "unable to get gpio %u\n", gpio_num);
+		return ret;
+	}
+
+
+	/*
+	 * Note that, in order to be compatible with Linux, the code
+	 * below interprets 'output-high' as to mean 'output-active'.
+	 * That is, when processed for active-low GPIO, it will result
+	 * in output being asserted logically 'active', but physically
+	 * 'low'.
+	 *
+	 * Conversely it means that specifying 'output-low' for
+	 * 'active-low' GPIO would result in 'high' level observed on
+	 * the corresponding pin
+	 *
+	 */
+	if (of_property_read_bool(np, "input"))
+		flags |= GPIOF_DIR_IN;
+	else if (of_property_read_bool(np, "output-low"))
+		flags |= GPIOF_OUT_INIT_INACTIVE;
+	else if (of_property_read_bool(np, "output-high"))
+		flags |= GPIOF_OUT_INIT_ACTIVE;
+	else
+		return -EINVAL;
+
+	of_property_read_string(np, "line-name", &name);
+
+	return gpio_request_one(gpio, flags, name);
+}
+
+static int of_gpiochip_scan_hogs(struct gpio_chip *chip)
+{
+	struct device_node *np;
+	int ret, i;
+
+	for_each_available_child_of_node(chip->dev->device_node, np) {
+		if (!of_property_read_bool(np, "gpio-hog"))
+			continue;
+
+		for (ret = 0, i = 0;
+		     !ret;
+		     ret = of_hog_gpio(np, chip, i), i++)
+			;
+		/*
+		 * We ignore -EOVERFLOW because it serves us as an
+		 * indicator that there's no more GPIOs to handle.
+		 */
+		if (ret < 0 && ret != -EOVERFLOW)
+			return ret;
+	}
+
+	return 0;
+}
+
 int gpiochip_add(struct gpio_chip *chip)
 {
 	int base, i;
@@ -322,7 +416,7 @@ int gpiochip_add(struct gpio_chip *chip)
 	for (i = chip->base; i < chip->base + chip->ngpio; i++)
 		gpio_desc[i].chip = chip;
 
-	return 0;
+	return of_gpiochip_scan_hogs(chip);
 }
 
 void gpiochip_remove(struct gpio_chip *chip)
-- 
2.9.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v3 4/4] usb-nop-xceiv: Add support for 'reset-gpios' binding

[Andrey Smirnov]

Cc: cphealy@gmail.com
Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/phy/usb-nop-xceiv.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/usb-nop-xceiv.c b/drivers/phy/usb-nop-xceiv.c
index d403fe4..b124e6c 100644
--- a/drivers/phy/usb-nop-xceiv.c
+++ b/drivers/phy/usb-nop-xceiv.c
@@ -22,12 +22,15 @@
 #include <linux/phy/phy.h>
 #include <linux/clk.h>
 #include <linux/err.h>
+#include <gpio.h>
+#include <of_gpio.h>
 
 struct nop_usbphy {
 	struct usb_phy usb_phy;
 	struct phy *phy;
 	struct phy_provider *provider;
 	struct clk *clk;
+	int reset;
 };
 
 static struct phy *nop_usbphy_xlate(struct device_d *dev,
@@ -40,9 +43,22 @@ static struct phy *nop_usbphy_xlate(struct device_d *dev,
 
 static int nop_usbphy_init(struct phy *phy)
 {
+	int ret;
 	struct nop_usbphy *nopphy = phy_get_drvdata(phy);
 
-	return clk_enable(nopphy->clk);
+	ret = clk_enable(nopphy->clk);
+	if (ret < 0)
+		return ret;
+
+	if (gpio_is_valid(nopphy->reset)) {
+		/*
+		 * Let's wait for 100 ms before deasserting reset line
+		 */
+		mdelay(100);
+		gpio_set_active(nopphy->reset, false);
+	}
+
+	return 0;
 }
 
 static struct usb_phy *nop_usbphy_to_usbphy(struct phy *phy)
@@ -61,6 +77,9 @@ static int nop_usbphy_probe(struct device_d *dev)
 {
 	int ret;
 	struct nop_usbphy *nopphy;
+	enum of_gpio_flags of_flags;
+	char *name = NULL;
+	unsigned long flags = GPIOF_OUT_INIT_ACTIVE;
 
 	nopphy = xzalloc(sizeof(*nopphy));
 
@@ -70,6 +89,20 @@ static int nop_usbphy_probe(struct device_d *dev)
 	if (IS_ERR(nopphy->clk))
 		nopphy->clk = NULL;
 
+	nopphy->reset = of_get_named_gpio_flags(dev->device_node,
+						"reset-gpios", 0, &of_flags);
+	if (gpio_is_valid(nopphy->reset)) {
+		/* assert reset */
+
+		if (of_flags & OF_GPIO_ACTIVE_LOW)
+			flags |= GPIOF_ACTIVE_LOW;
+
+		name = basprintf("%s reset", dev_name(dev));
+		ret  = gpio_request_one(nopphy->reset, flags, name);
+		if (ret < 0)
+			goto err_free;
+	}
+
 	/* FIXME: Add vbus regulator support */
 	/* FIXME: Add vbus-detect-gpio support */
 
@@ -77,7 +110,7 @@ static int nop_usbphy_probe(struct device_d *dev)
 	nopphy->phy = phy_create(dev, NULL, &nop_phy_ops, NULL);
 	if (IS_ERR(nopphy->phy)) {
 		ret = PTR_ERR(nopphy->phy);
-		goto err_free;
+		goto release_gpio;
 	}
 
 	phy_set_drvdata(nopphy->phy, nopphy);
@@ -85,13 +118,17 @@ static int nop_usbphy_probe(struct device_d *dev)
 	nopphy->provider = of_phy_provider_register(dev, nop_usbphy_xlate);
 	if (IS_ERR(nopphy->provider)) {
 		ret = PTR_ERR(nopphy->provider);
-		goto err_free;
+		goto release_gpio;
 	}
 
 	return 0;
+
+release_gpio:
+	if (gpio_is_valid(nopphy->reset))
+		gpio_free(nopphy->reset);
 err_free:
 	free(nopphy);
-
+	free(name);
 	return ret;
 };
 
-- 
2.9.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v3 0/4] GPIO "active low", hogging and usb-nop-xceive 'reset-gpio' support

[Sascha Hauer]

On Sat, Jun 03, 2017 at 08:11:50PM -0700, Andrey Smirnov wrote:
> Hi everyone,
> 
> This is a v3 of my original patchset. Changes in this version are the following:
> 
>   - new flags, GPIOF_INIT_[IN]ACTIVE and GPIOF_OUT_INIT_[IN]ACTIVE are
>     added to avoid semantically overloading the meaing of "HIGH/LOW"
>     variants of those
> 
>   - added commentary about GPIO 'hog' interpretation of the meaning of
>     'output-high', 'output-low'
> 
> Changes since v1:
> 
>   - 'active low' support is moved inot a dedicated API
>   - of_gpiochip_scan_gpios renamed to of_gpiochip_scan_hogs
>   - nop_usbphy_init doesn't try to needlessly configure reset gpio's directionality
> 
> Let me know if anything else needs changing.
> 
> Thanks,
> Andrey Smirnov

Applied, thanks

Sascha

> 
> Andrey Smirnov (4):
>   gpio-imx: Do not use gpio_set_value()
>   gpiolib: Add code to support "active low" GPIOs
>   gpiolib: Add support for GPIO "hog" nodes
>   usb-nop-xceiv: Add support for 'reset-gpios' binding
> 
>  drivers/gpio/gpio-imx.c     |   2 +-
>  drivers/gpio/gpiolib.c      | 146 ++++++++++++++++++++++++++++++++++++++++++--
>  drivers/phy/usb-nop-xceiv.c |  45 ++++++++++++--
>  include/gpio.h              |  25 ++++++++
>  4 files changed, 208 insertions(+), 10 deletions(-)
> 
> -- 
> 2.9.4
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox