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

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

[Andrey Smirnov]

Hi everyone,

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

  - '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      | 123 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/phy/usb-nop-xceiv.c |  46 +++++++++++++++--
 include/gpio.h              |  20 +++++++
 4 files changed, 184 insertions(+), 7 deletions(-)

-- 
2.9.4


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

[PATCH v2 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 v2 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 | 42 ++++++++++++++++++++++++++++++++++++++++--
 include/gpio.h         | 20 ++++++++++++++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1f57c76..36d8874 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,10 +123,15 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 	if (err)
 		return err;
 
+	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
-		err = gpio_direction_output(gpio,
+		err = gpio_direction_active(gpio,
 				(flags & GPIOF_INIT_HIGH) ? 1 : 0);
 
 	if (err)
@@ -178,6 +195,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 +220,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 +246,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 +372,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..bd61269 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
@@ -41,6 +58,9 @@ static inline int gpio_is_valid(int gpio)
 #define GPIOF_INIT_LOW	(0 << 1)
 #define GPIOF_INIT_HIGH	(1 << 1)
 
+#define GPIOF_ACTIVE_HIGH	(0 << 2)
+#define GPIOF_ACTIVE_LOW	(1 << 2)
+
 #define GPIOF_IN		(GPIOF_DIR_IN)
 #define GPIOF_OUT_INIT_LOW	(GPIOF_DIR_OUT | GPIOF_INIT_LOW)
 #define GPIOF_OUT_INIT_HIGH	(GPIOF_DIR_OUT | GPIOF_INIT_HIGH)
-- 
2.9.4


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

[PATCH v2 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 | 83 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 82 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 36d8874..8089c4f 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>
 
@@ -300,6 +301,86 @@ 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;
+	}
+
+	if (of_property_read_bool(np, "input"))
+		flags |= GPIOF_DIR_IN;
+	else if (of_property_read_bool(np, "output-low"))
+		flags |= GPIOF_OUT_INIT_LOW;
+	else if (of_property_read_bool(np, "output-high"))
+		flags |= GPIOF_OUT_INIT_HIGH;
+	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;
@@ -318,7 +399,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 v2 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 | 46 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/usb-nop-xceiv.c b/drivers/phy/usb-nop-xceiv.c
index d403fe4..c193a10 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_DIR_OUT;
 
 	nopphy = xzalloc(sizeof(*nopphy));
 
@@ -70,6 +89,21 @@ 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)) {
+		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;
+
+		/* assert reset */
+		gpio_direction_active(nopphy->reset, true);
+	}
+
 	/* FIXME: Add vbus regulator support */
 	/* FIXME: Add vbus-detect-gpio support */
 
@@ -77,7 +111,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 +119,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 v2 2/4] gpiolib: Add code to support "active low" GPIOs

[Sascha Hauer]

On Tue, May 30, 2017 at 07:52:26AM -0700, Andrey Smirnov wrote:
> 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 | 42 ++++++++++++++++++++++++++++++++++++++++--
>  include/gpio.h         | 20 ++++++++++++++++++++
>  2 files changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 1f57c76..36d8874 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,10 +123,15 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
>  	if (err)
>  		return err;
>  
> +	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
> -		err = gpio_direction_output(gpio,
> +		err = gpio_direction_active(gpio,
>  				(flags & GPIOF_INIT_HIGH) ? 1 : 0);

And here things get messy.

For me 'high' and 'low' represent the physical values of a GPIO whereas
"active" and "inactive" represent the logical values of a GPIO. The flag
is named GPIOF_INIT_*HIGH*, not GPIOF_INIT_*ACTIVE*, which means a GPIO
with this flag should get the physical 'high' value, not the logical
'active' value.

They goofed the binding in the kernel, so I'm afraid there's nothing we
can do about this :(

Sascha


-- 
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

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

[Andrey Smirnov]

On Thu, Jun 1, 2017 at 12:19 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, May 30, 2017 at 07:52:26AM -0700, Andrey Smirnov wrote:
>> 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 | 42 ++++++++++++++++++++++++++++++++++++++++--
>>  include/gpio.h         | 20 ++++++++++++++++++++
>>  2 files changed, 60 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 1f57c76..36d8874 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,10 +123,15 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
>>       if (err)
>>               return err;
>>
>> +     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
>> -             err = gpio_direction_output(gpio,
>> +             err = gpio_direction_active(gpio,
>>                               (flags & GPIOF_INIT_HIGH) ? 1 : 0);
>
> And here things get messy.
>
> For me 'high' and 'low' represent the physical values of a GPIO whereas
> "active" and "inactive" represent the logical values of a GPIO. The flag
> is named GPIOF_INIT_*HIGH*, not GPIOF_INIT_*ACTIVE*, which means a GPIO
> with this flag should get the physical 'high' value, not the logical
> 'active' value.
>
> They goofed the binding in the kernel, so I'm afraid there's nothing we
> can do about this :(

So do we want to:

a) Keep things as is in v2(I am assuming that is not really an option)
b) Improve the optics by introducing GPIOF_INIT_ACTIVE, but keeping
the behavior of hog nodes consistent with Linux kernel
c) Drop this particular chunk of code, not support keep using
gpio_direciton_output(), and print a warning when we encounter a hog
with 'active low' set
d) Drop active low API patch and handle that aspect inside of code of
the driver that needs it
e) Something else

?

Thanks,
Andrey Smirnov

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

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

[Sascha Hauer]

On Thu, Jun 01, 2017 at 01:33:55PM -0700, Andrey Smirnov wrote:
> On Thu, Jun 1, 2017 at 12:19 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Tue, May 30, 2017 at 07:52:26AM -0700, Andrey Smirnov wrote:
> >> 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 | 42 ++++++++++++++++++++++++++++++++++++++++--
> >>  include/gpio.h         | 20 ++++++++++++++++++++
> >>  2 files changed, 60 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >> index 1f57c76..36d8874 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,10 +123,15 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
> >>       if (err)
> >>               return err;
> >>
> >> +     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
> >> -             err = gpio_direction_output(gpio,
> >> +             err = gpio_direction_active(gpio,
> >>                               (flags & GPIOF_INIT_HIGH) ? 1 : 0);
> >
> > And here things get messy.
> >
> > For me 'high' and 'low' represent the physical values of a GPIO whereas
> > "active" and "inactive" represent the logical values of a GPIO. The flag
> > is named GPIOF_INIT_*HIGH*, not GPIOF_INIT_*ACTIVE*, which means a GPIO
> > with this flag should get the physical 'high' value, not the logical
> > 'active' value.
> >
> > They goofed the binding in the kernel, so I'm afraid there's nothing we
> > can do about this :(
> 
> So do we want to:
> 
> a) Keep things as is in v2(I am assuming that is not really an option)
> b) Improve the optics by introducing GPIOF_INIT_ACTIVE, but keeping
> the behavior of hog nodes consistent with Linux kernel

We must keep the behaviour consistent with the Kernel, everything else
is not an option. A GPIOF_INIT_ACTIVE flag sounds like a good idea. The
place where "output-[high|low]" is translated into this flag seems a
good place to put a big comment what is going on.

Sascha

-- 
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

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

[Andrey Smirnov]

On Thu, Jun 1, 2017 at 10:28 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Thu, Jun 01, 2017 at 01:33:55PM -0700, Andrey Smirnov wrote:
>> On Thu, Jun 1, 2017 at 12:19 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > On Tue, May 30, 2017 at 07:52:26AM -0700, Andrey Smirnov wrote:
>> >> 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 | 42 ++++++++++++++++++++++++++++++++++++++++--
>> >>  include/gpio.h         | 20 ++++++++++++++++++++
>> >>  2 files changed, 60 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> >> index 1f57c76..36d8874 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,10 +123,15 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
>> >>       if (err)
>> >>               return err;
>> >>
>> >> +     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
>> >> -             err = gpio_direction_output(gpio,
>> >> +             err = gpio_direction_active(gpio,
>> >>                               (flags & GPIOF_INIT_HIGH) ? 1 : 0);
>> >
>> > And here things get messy.
>> >
>> > For me 'high' and 'low' represent the physical values of a GPIO whereas
>> > "active" and "inactive" represent the logical values of a GPIO. The flag
>> > is named GPIOF_INIT_*HIGH*, not GPIOF_INIT_*ACTIVE*, which means a GPIO
>> > with this flag should get the physical 'high' value, not the logical
>> > 'active' value.
>> >
>> > They goofed the binding in the kernel, so I'm afraid there's nothing we
>> > can do about this :(
>>
>> So do we want to:
>>
>> a) Keep things as is in v2(I am assuming that is not really an option)
>> b) Improve the optics by introducing GPIOF_INIT_ACTIVE, but keeping
>> the behavior of hog nodes consistent with Linux kernel
>
> We must keep the behaviour consistent with the Kernel, everything else
> is not an option. A GPIOF_INIT_ACTIVE flag sounds like a good idea. The
> place where "output-[high|low]" is translated into this flag seems a
> good place to put a big comment what is going on.
>

Sounds good. I'll update the patchset accordingly.

Thanks,
Andrey Smirnov

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