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

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

[Andrey Smirnov]

Do not use gpio_set_value() in imx_gpio_direction_output() for two
reasons:

	- Since we don't check gpio_set_value's result, using it
          instead of imx_gpio_set_value doesn't seem to have any
          advantages

	- Using gpiolib's function at this level makes it hard to
          implement 'active low' support (commit that follows) since
          gpio driver is dealing with physical GPIO levels, whereas
          gpiolib is explected to accept logical levels.

As a remedy swithch imx_gpio_direction_output() to using
imx_gpio_set_value().

Cc: cphealy@gmail.com
Cc: 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.3


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

[PATCH 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 | 37 +++++++++++++++++++++++++++++++------
 include/gpio.h         |  3 +++
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1f57c76..9e9df355 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,6 +123,11 @@ 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
@@ -173,8 +190,10 @@ void gpio_set_value(unsigned gpio, int value)
 	if (gpio_ensure_requested(gi, gpio))
 		return;
 
-	if (gi->chip->ops->set)
-		gi->chip->ops->set(gi->chip, gpio - gi->chip->base, value);
+	if (gi->chip->ops->set) {
+		gi->chip->ops->set(gi->chip, gpio - gi->chip->base,
+				   gpio_adjust_value(gi, value));
+	}
 }
 EXPORT_SYMBOL(gpio_set_value);
 
@@ -192,7 +211,10 @@ int gpio_get_value(unsigned gpio)
 
 	if (!gi->chip->ops->get)
 		return -ENOSYS;
-	return gi->chip->ops->get(gi->chip, gpio - gi->chip->base);
+
+	ret = gi->chip->ops->get(gi->chip, gpio - gi->chip->base);
+
+	return gpio_adjust_value(gi, ret);
 }
 EXPORT_SYMBOL(gpio_get_value);
 
@@ -211,7 +233,7 @@ int gpio_direction_output(unsigned gpio, int value)
 	if (!gi->chip->ops->direction_output)
 		return -ENOSYS;
 	return gi->chip->ops->direction_output(gi->chip, gpio - gi->chip->base,
-					       value);
+					       gpio_adjust_value(gi, value));
 }
 EXPORT_SYMBOL(gpio_direction_output);
 
@@ -229,7 +251,10 @@ int gpio_direction_input(unsigned gpio)
 
 	if (!gi->chip->ops->direction_input)
 		return -ENOSYS;
-	return gi->chip->ops->direction_input(gi->chip, gpio - gi->chip->base);
+
+	ret = gi->chip->ops->direction_input(gi->chip, gpio - gi->chip->base);
+
+	return gpio_adjust_value(gi, ret);
 }
 EXPORT_SYMBOL(gpio_direction_input);
 
@@ -334,7 +359,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..9432543 100644
--- a/include/gpio.h
+++ b/include/gpio.h
@@ -41,6 +41,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.3


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

[PATCH 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 | 79 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9e9df355..959adba 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>
 
@@ -287,6 +288,82 @@ 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;
+
+	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_gpios(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;
@@ -305,7 +382,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_gpios(chip);
 }
 
 void gpiochip_remove(struct gpio_chip *chip)
-- 
2.9.3


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

[PATCH 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..bd28af4 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_direction_output(nopphy->reset, 0);
+	}
+
+	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_output(nopphy->reset, 1);
+	}
+
 	/* 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.3


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

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

[Nikita Yushchenko]

> Do not use gpio_set_value() in imx_gpio_direction_output() for two
> reasons:
> 
> 	- Since we don't check gpio_set_value's result, using it
>           instead of imx_gpio_set_value doesn't seem to have any
>           advantages
> 
> 	- Using gpiolib's function at this level makes it hard to
>           implement 'active low' support (commit that follows) since
>           gpio driver is dealing with physical GPIO levels, whereas
>           gpiolib is explected to accept logical levels.
> 
> As a remedy swithch imx_gpio_direction_output() to using
> imx_gpio_set_value().

I'd say calling back into subsystem from driver is generally invalid
pattern, and this patch fixes that.

Acked-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>

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

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

[Nikita Yushchenko]

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

Do we have at least single case when same pin of the same chip is active
high in one use and active low in other use?

I'd say that "logic values" of gpiolib is a major source of confusion,
at least in it's current form. The fact that
  gpio_set_value(..., 1)
does not set gpio value to 1 but instead sets gpio value to what is
configured as active, is non-intuitive at least. Maybe with different
API names (e.g. gpio_activate() / gpio_deactivate()) it could be better.

I understand that we have these logic values in linux and may want
similarity between linux and barebox. But far not sure at which side it
should be fixed.

Right now I'm trying to handle a situation (in linux) where chip has
signal active low, but driver author just used gpiolib calls with
physical values, inverted.  So device trees are plain wrong (stating
gpio is active high which is not true) but changing that is about to
break existing working setups.

I don't know how many similar cases exist, but I guess that quite a few.
 And all this is caused by enforcement of logical gpio values concept
everywhere - while it is useful only in rare case of setup-dependent
signal polarity.  In vast majority of cases signal polarity is
chip-dependent, not setup-dependent.

Nikita

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

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

[Nikita Yushchenko]

> +	ret = of_property_read_u32(chip_np, "#gpio-cells", &gpio_cells);
> +	if (ret)
> +		return ret;
> +
> +	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;

Doesn't this hardcode interpretation of device tree words in gpio
specification - while this is intended to be gpio-provider specific and
that's why #gpio-cells exist?


> +static int of_gpiochip_scan_gpios(struct gpio_chip *chip)

Not best choice of name for routine that scans hogs?

(although I understand that it comes from linux counterpart)

> -	return 0;
> +	return of_gpiochip_scan_gpios(chip);

Should we fail gpiochip registration on failure to claim hogs?
I don't know.

Nikita

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

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

[Nikita Yushchenko]

> +
> +	if (gpio_is_valid(nopphy->reset)) {
> +		/*
> +		 * Let's wait for 100 ms before deasserting reset line
> +		 */
> +		mdelay(100);
> +		gpio_direction_output(nopphy->reset, 0);
> +	}

Why not gpio_set_value() here?  Direction is already configured at this
stage.

Nikita

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

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

[Sascha Hauer]

On Tue, May 23, 2017 at 09:30:27AM +0300, Nikita Yushchenko 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.
> 
> Do we have at least single case when same pin of the same chip is active
> high in one use and active low in other use?
> 
> I'd say that "logic values" of gpiolib is a major source of confusion,
> at least in it's current form. The fact that
>   gpio_set_value(..., 1)
> does not set gpio value to 1 but instead sets gpio value to what is
> configured as active, is non-intuitive at least. Maybe with different
> API names (e.g. gpio_activate() / gpio_deactivate()) it could be better.

Plain gpio_set_value() in Linux does not honour any ACTIVE_LOW flags,
only gpiod_set_value() does. But anyway, you are right, it *is*
confusing. I agree that we should have a different set of functions
which honour the ACTIVE_LOW flag. Besides of being more consistent
in the end I think it's the only way to not break any existing gpio
setups in barebox. With a different API set we can review each driver
change for unwanted side effects.

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 3/4] gpiolib: Add support for GPIO "hog" nodes

[Andrey Smirnov]

On Mon, May 22, 2017 at 11:52 PM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
>> +     ret = of_property_read_u32(chip_np, "#gpio-cells", &gpio_cells);
>> +     if (ret)
>> +             return ret;
>> +
>> +     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;
>
> Doesn't this hardcode interpretation of device tree words in gpio
> specification - while this is intended to be gpio-provider specific and
> that's why #gpio-cells exist?
>

It does and yes that's my understanding of the purpose of #gpio-cells
as well. The reason I did in such a primitive way was because
Barebox's GPIO subsystem doesn't have any translation plumbing to be
able to handle anything more than a simple one dimensional offset.
Given the fact that of_get_named_gpio_flags() make similar assumption
I thought that there are no real consumers of that functionality and
left proper implementation as a future improvement that can be made
once the need arises.

>
>> +static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>
> Not best choice of name for routine that scans hogs?
>
> (although I understand that it comes from linux counterpart)
>

Eh, I don't have any strong opinion on this one, I am more than happy
to rename it if you think there are better alternatives.

>> -     return 0;
>> +     return of_gpiochip_scan_gpios(chip);
>
> Should we fail gpiochip registration on failure to claim hogs?
> I don't know.

I couldn't think of a use-case where it wasn't basically all or
nothing: either I get everything working or I need to go back and fix
my DT. Sascha, do you have an opinion on this one?

Thanks,
Andrey Smirnov

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

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

[Andrey Smirnov]

On Mon, May 22, 2017 at 11:30 PM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> 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.
>
> Do we have at least single case when same pin of the same chip is active
> high in one use and active low in other use?
>

Same pin on the same chip? Probably no. But that's not really the
use-case for this change, there are a number of drivers that control
more or less an abstract device with logical operations rather that
specific chip. Consider the case of "gpio-leds" driver, for example,
there isn't a specific chip that this diver is written in mind with,
but depending on particular electrical design same pin ("enable") can
activate that LED either when it's low or high. Same for
"usp-nop-xceiv", it can be any USB PHY chip that doesn't require any
configuration and "reset" pin can be implemented as active low or
active high, but you'd still would want to do the same thing for both:
assert and then deassert reset whatever it means electrically.

> I'd say that "logic values" of gpiolib is a major source of confusion,
> at least in it's current form. The fact that
>   gpio_set_value(..., 1)
> does not set gpio value to 1 but instead sets gpio value to what is
> configured as active, is non-intuitive at least. Maybe with different
> API names (e.g. gpio_activate() / gpio_deactivate()) it could be better.
>

I have no problem moving this functionality into a separate API.

> I understand that we have these logic values in linux and may want
> similarity between linux and barebox. But far not sure at which side it
> should be fixed.
>
> Right now I'm trying to handle a situation (in linux) where chip has
> signal active low, but driver author just used gpiolib calls with
> physical values, inverted.  So device trees are plain wrong (stating
> gpio is active high which is not true) but changing that is about to
> break existing working setups.
>
> I don't know how many similar cases exist, but I guess that quite a few.
>  And all this is caused by enforcement of logical gpio values concept
> everywhere - while it is useful only in rare case of setup-dependent
> signal polarity.  In vast majority of cases signal polarity is
> chip-dependent, not setup-dependent.
>

I am not sure I really see a problem with the situation you describe
and why you'd want to change device tree description (perhaps because
I don't know all of the details). I agree, that, depending on you
personal preferences, one might find it annoying that driver uses
physical values, but other than that I don't see a technical problem
with using physical values in the driver, since as you said, it's not
very likely that same pin on the same chip would be active low/vs
active high.

Thanks,
Andrey Smrinov

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

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

[Andrey Smirnov]

On Tue, May 23, 2017 at 1:33 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, May 23, 2017 at 09:30:27AM +0300, Nikita Yushchenko 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.
>>
>> Do we have at least single case when same pin of the same chip is active
>> high in one use and active low in other use?
>>
>> I'd say that "logic values" of gpiolib is a major source of confusion,
>> at least in it's current form. The fact that
>>   gpio_set_value(..., 1)
>> does not set gpio value to 1 but instead sets gpio value to what is
>> configured as active, is non-intuitive at least. Maybe with different
>> API names (e.g. gpio_activate() / gpio_deactivate()) it could be better.
>
> Plain gpio_set_value() in Linux does not honour any ACTIVE_LOW flags,
> only gpiod_set_value() does. But anyway, you are right, it *is*
> confusing. I agree that we should have a different set of functions
> which honour the ACTIVE_LOW flag. Besides of being more consistent
> in the end I think it's the only way to not break any existing gpio
> setups in barebox. With a different API set we can review each driver
> change for unwanted side effects.

Sounds reasonable. I'll add the API and convert the rest of the
patches to use it appropriately in v2 of the patch.

Thanks,
Andrey Smirnov

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

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

[Andrey Smirnov]

On Mon, May 22, 2017 at 11:55 PM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
>> +
>> +     if (gpio_is_valid(nopphy->reset)) {
>> +             /*
>> +              * Let's wait for 100 ms before deasserting reset line
>> +              */
>> +             mdelay(100);
>> +             gpio_direction_output(nopphy->reset, 0);
>> +     }
>
> Why not gpio_set_value() here?  Direction is already configured at this
> stage.

No reason, really. I'll fix it in v2.

Thanks,
Andrey Smirnov

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

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

[Nikita Yushchenko]

24.05.2017 02:25, Andrey Smirnov wrote:
> On Mon, May 22, 2017 at 11:52 PM, Nikita Yushchenko
> <nikita.yoush@cogentembedded.com> wrote:
>>> +     ret = of_property_read_u32(chip_np, "#gpio-cells", &gpio_cells);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     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;
>>
>> Doesn't this hardcode interpretation of device tree words in gpio
>> specification - while this is intended to be gpio-provider specific and
>> that's why #gpio-cells exist?
>>
> 
> It does and yes that's my understanding of the purpose of #gpio-cells
> as well. The reason I did in such a primitive way was because
> Barebox's GPIO subsystem doesn't have any translation plumbing to be
> able to handle anything more than a simple one dimensional offset.
> Given the fact that of_get_named_gpio_flags() make similar assumption
> I thought that there are no real consumers of that functionality and
> left proper implementation as a future improvement that can be made
> once the need arises.

Maybe then at least make this [wrong] thing done in single place?  I.e.
extract relevant code from of_get_named_gpio_flags() into separate
routine and call it from two places?  (And add a comment there, that it
is a stub assuming dump representation)

>>> +static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>>
>> Not best choice of name for routine that scans hogs?
>>
>> (although I understand that it comes from linux counterpart)
>>
> 
> Eh, I don't have any strong opinion on this one, I am more than happy
> to rename it if you think there are better alternatives.

of_gpiochip_scan_hogs() ?


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

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

[Sascha Hauer]

On Tue, May 23, 2017 at 04:25:03PM -0700, Andrey Smirnov wrote:
> On Mon, May 22, 2017 at 11:52 PM, Nikita Yushchenko
> <nikita.yoush@cogentembedded.com> wrote:
> >> +     ret = of_property_read_u32(chip_np, "#gpio-cells", &gpio_cells);
> >> +     if (ret)
> >> +             return ret;
> >> +
> >> +     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;
> >
> > Doesn't this hardcode interpretation of device tree words in gpio
> > specification - while this is intended to be gpio-provider specific and
> > that's why #gpio-cells exist?
> >
> 
> It does and yes that's my understanding of the purpose of #gpio-cells
> as well. The reason I did in such a primitive way was because
> Barebox's GPIO subsystem doesn't have any translation plumbing to be
> able to handle anything more than a simple one dimensional offset.
> Given the fact that of_get_named_gpio_flags() make similar assumption
> I thought that there are no real consumers of that functionality and
> left proper implementation as a future improvement that can be made
> once the need arises.
> 
> >
> >> +static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
> >
> > Not best choice of name for routine that scans hogs?
> >
> > (although I understand that it comes from linux counterpart)
> >
> 
> Eh, I don't have any strong opinion on this one, I am more than happy
> to rename it if you think there are better alternatives.
> 
> >> -     return 0;
> >> +     return of_gpiochip_scan_gpios(chip);
> >
> > Should we fail gpiochip registration on failure to claim hogs?
> > I don't know.
> 
> I couldn't think of a use-case where it wasn't basically all or
> nothing: either I get everything working or I need to go back and fix
> my DT. Sascha, do you have an opinion on this one?

I can't think of any good reason why the hog initialization should fail
and we still want to keep the gpio chip.
Let's keep it like this until someone delivers us a good reason.

Sasch

-- 
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 2/4] gpiolib: Add code to support "active low" GPIOs

[Nikita Yushchenko]

> I am not sure I really see a problem with the situation you describe
> and why you'd want to change device tree description (perhaps because
> I don't know all of the details). I agree, that, depending on you
> personal preferences, one might find it annoying that driver uses
> physical values, but other than that I don't see a technical problem
> with using physical values in the driver, since as you said, it's not
> very likely that same pin on the same chip would be active low/vs
> active high.

First point is that words "active high" (or "active low") have very
clear meaning. And situation when chip's signal is active low, but one
has to write "active high" in signal definition to make things working,
is not something I welcome in systems I deal with.

Second point is that by cleaning up the above, you make drivers depend
on correct polarity settings in dts. Which means that when writing dts,
you get need to dig over datasheets (which may be under NDA) to find out
polarity of each signal. This looks like breakage of information
locality - knowledge of chip's signals polarity belongs to chip's
driver. Common case of signals connected directly to gpio providers
should just work.  It's nice to have a way to override driver's default,
to handle signal inversion by board logic and other special cases, but
that should be purely optional.

Third point is code clearance. If routine is called set_value, has
argument that very looks like value to be set, but instead of setting
given value does something different, it's source of confusion and errors.

But perhaps this thread is wrong place to discuss all that.

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

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

[Trent Piepho]

On Wed, 2017-05-24 at 10:26 +0300, Nikita Yushchenko wrote:
> First point is that words "active high" (or "active low") have very
> clear meaning. And situation when chip's signal is active low, but one
> has to write "active high" in signal definition to make things working,
> is not something I welcome in systems I deal with.
> 
> Second point is that by cleaning up the above, you make drivers depend
> on correct polarity settings in dts. Which means that when writing dts,
> you get need to dig over datasheets (which may be under NDA) to find out
> polarity of each signal. This looks like breakage of information
> locality - knowledge of chip's signals polarity belongs to chip's
> driver. Common case of signals connected directly to gpio providers
> should just work.  It's nice to have a way to override driver's default,

I agree with this.  It's pretty much random if a given signal will want
a high value to mean asserted or not.  And plenty of signals switch
"modes" and it's not even clear which mode should be considered
"asserted".  If drivers expect me to put active low/high in the
bindings, then it means for every signal one must get the datasheet and
figure out what a high signal means.  And then likely look though the
driver code to make sure the driver sets 1 to mean that.

In cases where the polarity is arbitrary, it should be documented in the
bindings (like I did for the gpio leds binding) that one is expected to
provide the polarity.

The reason gpiolib doesn't naively handle polarity in gpio_set_value()
is because of speed.  Originally gpio_set_value() was an arch specific
inline function that could check if the numeric gpio number was in the
range reserved for on-SoC gpio lines (__builtin_constant_p() lets you do
that at compile time).  If so, the inline function could expand directly
to the code for setting the gpio, which could be just a single machine
instruction.  You lose that speed if you need to look up a gpio
descriptor and a driver state struct and check a bunch of flags and
acquire a spinlock.

This doesn't work anymore since even built in gpios use the Linux driver
model and dynamically allocate their IDs.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

[Andrey Smirnov]

On Wed, May 24, 2017 at 11:16 AM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> On Wed, 2017-05-24 at 10:26 +0300, Nikita Yushchenko wrote:
>> First point is that words "active high" (or "active low") have very
>> clear meaning. And situation when chip's signal is active low, but one
>> has to write "active high" in signal definition to make things working,
>> is not something I welcome in systems I deal with.
>>
>> Second point is that by cleaning up the above, you make drivers depend
>> on correct polarity settings in dts. Which means that when writing dts,
>> you get need to dig over datasheets (which may be under NDA) to find out
>> polarity of each signal. This looks like breakage of information
>> locality - knowledge of chip's signals polarity belongs to chip's
>> driver. Common case of signals connected directly to gpio providers
>> should just work.  It's nice to have a way to override driver's default,
>
> I agree with this.  It's pretty much random if a given signal will want
> a high value to mean asserted or not.

Yes, and that the point of having "active low" being configurable in
device tree so it would be possible to use exactly the same driver
code for slightly different setups.

> And plenty of signals switch
> "modes" and it's not even clear which mode should be considered
> "asserted".

First this statement is so vague that it is hard to make any argument
about it. Second, just because a feature doesn't cover every possible
use-case doesn't mean that it doesn't have a place in the code base at
all.

> If drivers expect me to put active low/high in the
> bindings, then it means for every signal one must get the datasheet and
> figure out what a high signal means.  And then likely look though the
> driver code to make sure the driver sets 1 to mean that.
>

I don't see how "active low" changes the way you troubleshoot things
at all. If you are not lucky and you code didn't just work from the
first try, wouldn't you want to verify that you specified correct GPIO
number by looking at the schematic? And if so wouldn't you see what
high/low means by looking at the chip's symbol? If you don't have
access to a schematic, the only way I see to proceed with debugging it
is to probe correct pin on the chip with a scope, for which you'd need
at least an abridged datasheet that would have pinout documented.

Regardless of any of that, I seems to me that this is an argument
about personal preferences (I find the feature in question useful and
don't think it is confusing, you guys have dislike it) so I don't
think we'd resolve this any time soon.

IMHO, whether any of likes it or not, OF_GPIO_ACTIVE_LOW is an
existing feature and the only technical question is if it should be
supported by Barebox on per-driver basis or if there should be a
central API for it.

Sascha, are you still OK with me proceeding with making and using an
API, or should I just scrap that and handle this as a part of my
"usb-no-xcev"?

Thanks,
Andrey Smirnov

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

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

[Nikita Yushchenko]

> IMHO, whether any of likes it or not, OF_GPIO_ACTIVE_LOW is an
> existing feature and the only technical question is if it should be
> supported by Barebox on per-driver basis or if there should be a
> central API for it.
> 
> Sascha, are you still OK with me proceeding with making and using an
> API, or should I just scrap that and handle this as a part of my
> "usb-no-xcev"?

Is support for case when this usb-no-xcev's gpio is active high needed?

If yes, then your questions above are valid.

If no, then there is third option, completely ignore this aspect of
device tree and just assume that gpio is active low.

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

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

[Andrey Smirnov]

On Wed, May 24, 2017 at 11:36 PM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
>> IMHO, whether any of likes it or not, OF_GPIO_ACTIVE_LOW is an
>> existing feature and the only technical question is if it should be
>> supported by Barebox on per-driver basis or if there should be a
>> central API for it.
>>
>> Sascha, are you still OK with me proceeding with making and using an
>> API, or should I just scrap that and handle this as a part of my
>> "usb-no-xcev"?
>
> Is support for case when this usb-no-xcev's gpio is active high needed?
>
> If yes, then your questions above are valid.
>
> If no, then there is third option, completely ignore this aspect of
> device tree and just assume that gpio is active low.

That is a trivial/easily testable feature to implement, and I am not
interested in pursuing "third option" you propose.

Thanks,
Andrey Smirnov

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

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

[Sascha Hauer]

On Wed, May 24, 2017 at 01:36:48PM -0700, Andrey Smirnov wrote:
> On Wed, May 24, 2017 at 11:16 AM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> > On Wed, 2017-05-24 at 10:26 +0300, Nikita Yushchenko wrote:
> >> First point is that words "active high" (or "active low") have very
> >> clear meaning. And situation when chip's signal is active low, but one
> >> has to write "active high" in signal definition to make things working,
> >> is not something I welcome in systems I deal with.
> >>
> >> Second point is that by cleaning up the above, you make drivers depend
> >> on correct polarity settings in dts. Which means that when writing dts,
> >> you get need to dig over datasheets (which may be under NDA) to find out
> >> polarity of each signal. This looks like breakage of information
> >> locality - knowledge of chip's signals polarity belongs to chip's
> >> driver. Common case of signals connected directly to gpio providers
> >> should just work.  It's nice to have a way to override driver's default,
> >
> > I agree with this.  It's pretty much random if a given signal will want
> > a high value to mean asserted or not.
> 
> Yes, and that the point of having "active low" being configurable in
> device tree so it would be possible to use exactly the same driver
> code for slightly different setups.
> 
> > And plenty of signals switch
> > "modes" and it's not even clear which mode should be considered
> > "asserted".
> 
> First this statement is so vague that it is hard to make any argument
> about it. Second, just because a feature doesn't cover every possible
> use-case doesn't mean that it doesn't have a place in the code base at
> all.
> 
> > If drivers expect me to put active low/high in the
> > bindings, then it means for every signal one must get the datasheet and
> > figure out what a high signal means.  And then likely look though the
> > driver code to make sure the driver sets 1 to mean that.
> >
> 
> I don't see how "active low" changes the way you troubleshoot things
> at all. If you are not lucky and you code didn't just work from the
> first try, wouldn't you want to verify that you specified correct GPIO
> number by looking at the schematic? And if so wouldn't you see what
> high/low means by looking at the chip's symbol? If you don't have
> access to a schematic, the only way I see to proceed with debugging it
> is to probe correct pin on the chip with a scope, for which you'd need
> at least an abridged datasheet that would have pinout documented.
> 
> Regardless of any of that, I seems to me that this is an argument
> about personal preferences (I find the feature in question useful and
> don't think it is confusing, you guys have dislike it) so I don't
> think we'd resolve this any time soon.
> 
> IMHO, whether any of likes it or not, OF_GPIO_ACTIVE_LOW is an
> existing feature and the only technical question is if it should be
> supported by Barebox on per-driver basis or if there should be a
> central API for it.

I still think that gpio_[gs]et_value should set the GPIOs to the actual
logical value and not take any GPIO_ACTIVE_* flags into account. Also I
still think that having an additional gpio_set_[in]active API would be
useful.
It's a bit unfortunate that in Linux gpio_set_value and gpiod_set_value
behave differently, despite the similar name.

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 3/4] gpiolib: Add support for GPIO "hog" nodes

[Andrey Smirnov]

On Tue, May 23, 2017 at 11:43 PM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
>
>
> 24.05.2017 02:25, Andrey Smirnov wrote:
>> On Mon, May 22, 2017 at 11:52 PM, Nikita Yushchenko
>> <nikita.yoush@cogentembedded.com> wrote:
>>>> +     ret = of_property_read_u32(chip_np, "#gpio-cells", &gpio_cells);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     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;
>>>
>>> Doesn't this hardcode interpretation of device tree words in gpio
>>> specification - while this is intended to be gpio-provider specific and
>>> that's why #gpio-cells exist?
>>>
>>
>> It does and yes that's my understanding of the purpose of #gpio-cells
>> as well. The reason I did in such a primitive way was because
>> Barebox's GPIO subsystem doesn't have any translation plumbing to be
>> able to handle anything more than a simple one dimensional offset.
>> Given the fact that of_get_named_gpio_flags() make similar assumption
>> I thought that there are no real consumers of that functionality and
>> left proper implementation as a future improvement that can be made
>> once the need arises.
>
> Maybe then at least make this [wrong] thing done in single place?  I.e.
> extract relevant code from of_get_named_gpio_flags() into separate
> routine and call it from two places?  (And add a comment there, that it
> is a stub assuming dump representation)
>

The code of the two doesn't have much, if anything, in common.
Of_get_named_gpio_flags is expecting a phandle to the gpio node be a
part of the field it parses, whereas gpio specifier in hog nodes omits
that. I don't think I can have any meaningful code sharing here.

>>>> +static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>>>
>>> Not best choice of name for routine that scans hogs?
>>>
>>> (although I understand that it comes from linux counterpart)
>>>
>>
>> Eh, I don't have any strong opinion on this one, I am more than happy
>> to rename it if you think there are better alternatives.
>
> of_gpiochip_scan_hogs() ?
>

Sure, I'll do that in v2.

Thanks,
Andrey Smirnov

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