diff mbox series

[U-Boot,06/13] gpio: add support for new flags on gpio configuration

Message ID 20191023134448.20149-7-patrick.delaunay@st.com
State Changes Requested
Delegated to: Simon Glass
Headers show
Series dm: add support of new binding in gpio and pincontrol | expand

Commit Message

Patrick DELAUNAY Oct. 23, 2019, 1:44 p.m. UTC 
This commit manages the flags that can be used in GPIO specifiers to
indicate if a pull-up resistor or pull-down resistor should be
enabled for output GPIO and the Open Drain/Open Source configuration
for input GPIO.

It is managed in driver with a new ops in gpio uclass set_config.

These flags are already support in Linux kernel in gpiolib.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 drivers/gpio/gpio-uclass.c | 62 +++++++++++++++++++++++++++++++++++++-
 include/asm-generic/gpio.h | 56 ++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+), 1 deletion(-)

Comments

Simon Glass Oct. 30, 2019, 1:48 a.m. UTC | #1 
Hi Patrick,

On Wed, 23 Oct 2019 at 07:45, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> This commit manages the flags that can be used in GPIO specifiers to
> indicate if a pull-up resistor or pull-down resistor should be
> enabled for output GPIO and the Open Drain/Open Source configuration
> for input GPIO.
>
> It is managed in driver with a new ops in gpio uclass set_config.
>
> These flags are already support in Linux kernel in gpiolib.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  drivers/gpio/gpio-uclass.c | 62 +++++++++++++++++++++++++++++++++++++-
>  include/asm-generic/gpio.h | 56 ++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+), 1 deletion(-)
>

To me this seems like a pretty annoying interface. The uclass has to
call the driver multiple times with each enum and the driver may end
up reprogramming the pins multiple times to get it right.

Normally we want to program things correctly once, before enabling the function.

On the other handle I think what you have is better than adding new
methods like set_open_drain().

But overall I think it would be better to define a new struct like
gpio_config that holds some flags and perhaps a few other things. Then
the uclass can set up that struct and call the driver once with it, to
set up the pin. It could include input/output too, so that if
set_config() is defined, the uclass uses that instead of
direction_output(), etc.

What do you think?

Also we should update the sandbox driver to include tests for new
methods. It looks like you have done pinctrl but not this?

Regards,
Simon
Patrick DELAUNAY Oct. 31, 2019, 2:59 p.m. UTC | #2 
Hi Simon,

> From: Simon Glass <sjg@chromium.org>
> Sent: mercredi 30 octobre 2019 02:49
> 
> Hi Patrick,
> 
> On Wed, 23 Oct 2019 at 07:45, Patrick Delaunay <patrick.delaunay@st.com>
> wrote:
> >
> > This commit manages the flags that can be used in GPIO specifiers to
> > indicate if a pull-up resistor or pull-down resistor should be enabled
> > for output GPIO and the Open Drain/Open Source configuration for input
> > GPIO.
> >
> > It is managed in driver with a new ops in gpio uclass set_config.
> >
> > These flags are already support in Linux kernel in gpiolib.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > ---
> >
> >  drivers/gpio/gpio-uclass.c | 62
> > +++++++++++++++++++++++++++++++++++++-
> >  include/asm-generic/gpio.h | 56 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 117 insertions(+), 1 deletion(-)
> >
> 
> To me this seems like a pretty annoying interface. The uclass has to call the driver
> multiple times with each enum and the driver may end up reprogramming the pins
> multiple times to get it right.
> 
> Normally we want to program things correctly once, before enabling the function.
> 
> On the other handle I think what you have is better than adding new methods like
> set_open_drain().
> 
> But overall I think it would be better to define a new struct like gpio_config that
> holds some flags and perhaps a few other things. Then the uclass can set up that
> struct and call the driver once with it, to set up the pin. It could include input/output
> too, so that if
> set_config() is defined, the uclass uses that instead of direction_output(), etc.
> 
> What do you think?

I understand the issue.... 
You think something like the serial ops setconfig/getconfig.

So the API can evaluate without add new ops for each new parameter... 
It is clearly better.

I will think about and try to propose something without break nothing.

> Also we should update the sandbox driver to include tests for new methods. It
> looks like you have done pinctrl but not this?

I think test are done in 
[PATCH 12/13] test: dm: update test for pins configuration in gpio
- dm_test_gpio_pin_config

Do think about other tests ?

> Regards,
> Simon

Regards

Patrick
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index 90fbed455b..c6c6e8b343 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -128,7 +128,19 @@  int gpio_xlate_offs_flags(struct udevice *dev, struct gpio_desc *desc,
 		return 0;
 
 	if (args->args[1] & GPIO_ACTIVE_LOW)
-		desc->flags = GPIOD_ACTIVE_LOW;
+		desc->flags |= GPIOD_ACTIVE_LOW;
+
+	if ((args->args[1] & GPIO_OPEN_DRAIN) == GPIO_OPEN_DRAIN)
+		desc->flags |= GPIOD_OPEN_DRAIN;
+
+	if ((args->args[1] & GPIO_OPEN_SOURCE) == GPIO_OPEN_SOURCE)
+		desc->flags |= GPIOD_OPEN_SOURCE;
+
+	if (args->args[1] & GPIO_PULL_UP)
+		desc->flags |= GPIOD_PULL_UP;
+
+	if (args->args[1] & GPIO_PULL_DOWN)
+		desc->flags |= GPIOD_PULL_DOWN;
 
 	return 0;
 }
@@ -517,12 +529,31 @@  int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
 
 	if (ops->set_open_drain)
 		ret = ops->set_open_drain(desc->dev, desc->offset, value);
+	else if (ops->set_config)
+		ret = ops->set_config(desc->dev, desc->offset,
+				      value ? GPIO_CONF_DRIVE_OPEN_DRAIN :
+					      GPIO_CONF_DRIVE_PULL_PUSH);
 	else
 		return 0; /* feature not supported -> ignore setting */
 
 	return ret;
 }
 
+int gpio_get_config(const struct gpio_desc *desc)
+{
+	struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
+	int ret;
+
+	ret = check_reserved(desc, "get_config");
+	if (ret)
+		return ret;
+
+	if (ops->set_config)
+		return ops->get_config(desc->dev, desc->offset);
+	else
+		return -ENOSYS;
+}
+
 int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
 {
 	struct udevice *dev = desc->dev;
@@ -533,14 +564,39 @@  int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
 	if (ret)
 		return ret;
 
+	/* both pull-up and pull-down enabled, invalid configuration */
+	if ((flags & GPIOD_PULL_UP) && (flags & GPIOD_PULL_DOWN))
+		return -EINVAL;
+
 	if (flags & GPIOD_IS_OUT) {
 		int value = flags & GPIOD_IS_OUT_ACTIVE ? 1 : 0;
 
+		if (ops->set_config) {
+			if (flags & GPIOD_OPEN_DRAIN)
+				ops->set_config(dev, desc->offset,
+						GPIO_CONF_DRIVE_OPEN_DRAIN);
+			else if (flags & GPIOD_OPEN_SOURCE)
+				ops->set_config(dev, desc->offset,
+						GPIO_CONF_DRIVE_OPEN_SOURCE);
+			else
+				ops->set_config(dev, desc->offset,
+						GPIO_CONF_DRIVE_PULL_PUSH);
+		}
+
 		if (flags & GPIOD_ACTIVE_LOW)
 			value = !value;
 		ret = ops->direction_output(dev, desc->offset, value);
 	} else  if (flags & GPIOD_IS_IN) {
 		ret = ops->direction_input(dev, desc->offset);
+
+		if (ops->set_config) {
+			if (flags & GPIOD_PULL_UP)
+				ops->set_config(dev, desc->offset,
+						GPIO_CONF_BIAS_PULL_UP);
+			else if (flags & GPIOD_PULL_DOWN)
+				ops->set_config(dev, desc->offset,
+						GPIO_CONF_BIAS_PULL_DOWN);
+		}
 	}
 	if (ret)
 		return ret;
@@ -1061,6 +1117,10 @@  static int gpio_post_bind(struct udevice *dev)
 			ops->get_function += gd->reloc_off;
 		if (ops->xlate)
 			ops->xlate += gd->reloc_off;
+		if (ops->set_config)
+			ops->set_config += gd->reloc_off;
+		if (ops->get_config)
+			ops->get_config += gd->reloc_off;
 
 		reloc_done++;
 	}
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index d6cf18744f..9441e7dccc 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -112,6 +112,17 @@  enum gpio_func_t {
 	GPIOF_COUNT,
 };
 
+enum gpio_config {
+	GPIO_CONF_NONE,
+	GPIO_CONF_DRIVE_OPEN_DRAIN,
+	GPIO_CONF_DRIVE_OPEN_SOURCE,
+	GPIO_CONF_DRIVE_PULL_PUSH,
+	GPIO_CONF_BIAS_PULL_DOWN,
+	GPIO_CONF_BIAS_PULL_UP,
+
+	GPIO_CONF_COUNT,
+};
+
 struct udevice;
 
 struct gpio_desc {
@@ -122,6 +133,10 @@  struct gpio_desc {
 #define GPIOD_IS_IN		(1 << 2)	/* GPIO is an input */
 #define GPIOD_ACTIVE_LOW	(1 << 3)	/* value has active low */
 #define GPIOD_IS_OUT_ACTIVE	(1 << 4)	/* set output active */
+#define GPIOD_OPEN_DRAIN	(1 << 5)	/* GPIO is open drain type */
+#define GPIOD_OPEN_SOURCE	(1 << 6)	/* GPIO is open source type */
+#define GPIOD_PULL_UP		(1 << 7)	/* GPIO has pull up enabled */
+#define GPIOD_PULL_DOWN		(1 << 8)	/* GPIO has pull down enabled */
 
 	uint offset;		/* GPIO offset within the device */
 	/*
@@ -290,6 +305,36 @@  struct dm_gpio_ops {
 	 */
 	int (*xlate)(struct udevice *dev, struct gpio_desc *desc,
 		     struct ofnode_phandle_args *args);
+
+	/**
+	 * set_config() - Set GPIO configuration
+	 *
+	 * This function should set up the GPIO configuration according to the
+	 * information in the arguments.
+	 *
+	 * This method is optional.
+	 *
+	 * @dev:	GPIO device
+	 * @offset:	GPIO offset within that device
+	 * @config:	GPIO configuration
+	 * @return 0 if OK, -ve on error
+	 */
+	int (*set_config)(struct udevice *dev, unsigned offset,
+			  enum gpio_config config);
+
+	/**
+	 * set_config() - Set GPIO configuration
+	 *
+	 * This function return the GPIO configuration according to the
+	 * information in the arguments.
+	 *
+	 * This method is optional.
+	 *
+	 * @dev:	GPIO device
+	 * @offset:	GPIO offset within that device
+	 * @return GPIO configuration (GPIO_CONF_) if OK, -ve on error
+	 */
+	int (*get_config)(struct udevice *dev, unsigned offset);
 };
 
 /**
@@ -657,4 +702,15 @@  int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags);
  */
 int gpio_get_number(const struct gpio_desc *desc);
 
+/**
+ * gpio_get_config() - get the current GPIO pin configuration
+ *
+ * Obtain the current GPIO pin configuration
+ *
+ * @desc:	GPIO description containing device, offset and flags,
+ *		previously returned by gpio_request_by_name()
+ * @return GPIO configuration (GPIO_CONF_) if OK, -ve on error
+ */
+int gpio_get_config(const struct gpio_desc *desc);
+
 #endif	/* _ASM_GENERIC_GPIO_H_ */