[U-Boot] [PATCH v3 0/9] Add pinmux command

[U-Boot] [PATCH v3 0/9] Add pinmux command

[Patrice Chotard]

For debug purpose, it's useful to know the pins muxing
to check if a pin is configured as a GPIO or as an alternate
function and to get information about this alternate function
configuration. For this purpose a new command pinmux is implemented.

This series adds:
  - Add get_pin_muxing ops to UCLASS pinctrl
  - Add pinmux command
  - Add get_function() support to stm32 gpio driver
  - Add get_pins_count() support to stm32 pinctrl driver
  - Add get_pin_name() support to stm32 pinctrl driver
  - Add get_pin_muxing() support to stm32 pinctrl driver

Changes in v3:
 - Replace const char **buf parameter by char *buf, int size parameters
   for pinctrl_get_pin_muxing()
 - Replace const char **buf parameter by char *buf, int size parameters
   for pinctrl_get_pin_name()
 - Update calls to pinctrl_get_pin_name() and pinctrl_get_pin_muxing
   due to prototype update.
 - Fix typo

Changes in v2:
 - Replace pinmux_show ops which displayed the complete pin-controller
   muxing by get_pin_muxing ops which displays the muxing of one pin
 - In order to make pin muxing display less SoC specific,
   use pinctrl_pins_count(), pinctrl_get_pin_name() and
   pinctrl_get_pin_muxing() methods instead of
   previous pinctrl_pinmux_show() method.

Patrice Chotard (9):
  dm: pinctrl: Add get_pin_muxing() ops
  dm: pinctrl: Add pinctrl_get_pin_name and pinctrl_get_pins_count
  dm: uclass: Add uclass_foreach_dev_probe
  cmd: pinmux: Add pinmux command
  pinctrl: stm32: Add get_pins_count() ops
  pinctrl: stm32: Add get_pin_name() ops
  pinctrl: stm32: Add get_pin_muxing() ops
  gpio: stm32f7: Add ops get_function
  pinctrl: sandbox: Add get_pin_muxing ops support

 cmd/Kconfig                       |   8 ++
 cmd/Makefile                      |   1 +
 cmd/pinmux.c                      | 144 +++++++++++++++++++++++++
 drivers/gpio/stm32f7_gpio.c       |  20 ++++
 drivers/pinctrl/pinctrl-sandbox.c |  15 +++
 drivers/pinctrl/pinctrl-uclass.c  |  36 +++++++
 drivers/pinctrl/pinctrl_stm32.c   | 218 +++++++++++++++++++++++++++++++++++++-
 include/dm/pinctrl.h              |  55 ++++++++++
 include/dm/uclass.h               |  16 +++
 9 files changed, 508 insertions(+), 5 deletions(-)
 create mode 100644 cmd/pinmux.c

-- 
1.9.1

[U-Boot] [PATCH v3 1/9] dm: pinctrl: Add get_pin_muxing() ops

[Patrice Chotard]

Add get_pin_muxing() which allows to display the muxing
of a given pin belonging to a pin-controller.

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

Changes in v3:
 - Replace const char **buf parameter by char *buf, int size parameters
   for pinctrl_get_pin_muxing()

Changes in v2:
 - Replace pinmux_show ops which displayed the complete pin-controller
   muxing by get_pin_muxing ops which displays the muxing of one pin

 drivers/pinctrl/pinctrl-uclass.c | 13 +++++++++++++
 include/dm/pinctrl.h             | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
index c38bb212ed74..cd2ca4e4950e 100644
--- a/drivers/pinctrl/pinctrl-uclass.c
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -249,6 +249,19 @@ int pinctrl_get_gpio_mux(struct udevice *dev, int banknum, int index)
 	return ops->get_gpio_mux(dev, banknum, index);
 }
 
+int pinctrl_get_pin_muxing(struct udevice *dev, int selector, char *buf,
+			   int size)
+{
+	struct pinctrl_ops *ops = pinctrl_get_ops(dev);
+
+	if (!ops->get_pin_muxing)
+		return -ENOSYS;
+
+	snprintf(buf, size, ops->get_pin_muxing(dev, selector));
+
+	return 0;
+}
+
 /**
  * pinconfig_post_bind() - post binding for PINCTRL uclass
  * Recursively bind child nodes as pinconfig devices in case of full pinctrl.
diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
index 80de3f3fed2b..4cfaddcaebbf 100644
--- a/include/dm/pinctrl.h
+++ b/include/dm/pinctrl.h
@@ -66,6 +66,7 @@ struct pinconf_param {
  *	pointing a config node. (necessary for pinctrl_full)
  * @set_state_simple: do needed pinctrl operations for a peripherl @periph.
  *	(necessary for pinctrl_simple)
+ * @get_pin_muxing: display the muxing of a given pin.
  */
 struct pinctrl_ops {
 	int (*get_pins_count)(struct udevice *dev);
@@ -129,6 +130,22 @@ struct pinctrl_ops {
 	* @return mux value (SoC-specific, e.g. 0 for input, 1 for output)
 	 */
 	int (*get_gpio_mux)(struct udevice *dev, int banknum, int index);
+
+	/**
+	 * get_pin_muxing() - show pin muxing
+	 *
+	 * This allows to display the muxing of a given pin. It's useful for
+	 * debug purpose to know if a pin is configured as GPIO or as an
+	 * alternate function and which one.
+	 * Typically it is used by a PINCTRL driver with knowledge of the SoC
+	 * pinctrl setup.
+	 *
+	 * @dev:	Pinctrl device to use
+	 * @selector:	Pin selector
+	 * @return a string wich contains the muxing description
+	 */
+	const char *(*get_pin_muxing)(struct udevice *dev,
+				      unsigned int selector);
 };
 
 #define pinctrl_get_ops(dev)	((struct pinctrl_ops *)(dev)->driver->ops)
@@ -348,4 +365,19 @@ int pinctrl_decode_pin_config(const void *blob, int node);
 */
 int pinctrl_get_gpio_mux(struct udevice *dev, int banknum, int index);
 
+/**
+ * pinctrl_get_pin_muxing() - Returns the muxing description
+ *
+ * This allows to display the muxing description of the given pin for
+ * debug purpose
+ *
+ * @dev:	Pinctrl device to use
+ * @selector	Pin index within pin-controller
+ * @buf		Pin's muxing description
+ * @size	Pin's muxing description length
+ * @return 0 if OK, -ve on error
+ */
+int pinctrl_get_pin_muxing(struct udevice *dev, int selector, char *buf,
+			   int size);
+
 #endif /* __PINCTRL_H */
-- 
1.9.1

[U-Boot] [PATCH v3 2/9] dm: pinctrl: Add pinctrl_get_pin_name and pinctrl_get_pins_count

[Patrice Chotard]

Add pinctrl_get_pin_name() and pinctrl_get_pins_count() methods
to obtain pin's name and pin's muxing given a pin reference.

This will be used by the new pinmux command.

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

Changes in v3:
 - Replace const char **buf parameter by char *buf, int size parameters
   for pinctrl_get_pin_name()

Changes in v2: None

 drivers/pinctrl/pinctrl-uclass.c | 23 +++++++++++++++++++++++
 include/dm/pinctrl.h             | 23 +++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
index cd2ca4e4950e..bbaf830d65b9 100644
--- a/drivers/pinctrl/pinctrl-uclass.c
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -249,6 +249,29 @@ int pinctrl_get_gpio_mux(struct udevice *dev, int banknum, int index)
 	return ops->get_gpio_mux(dev, banknum, index);
 }
 
+int pinctrl_get_pins_count(struct udevice *dev)
+{
+	struct pinctrl_ops *ops = pinctrl_get_ops(dev);
+
+	if (!ops->get_pins_count)
+		return -ENOSYS;
+
+	return ops->get_pins_count(dev);
+}
+
+int pinctrl_get_pin_name(struct udevice *dev, int selector, char *buf,
+			 int size)
+{
+	struct pinctrl_ops *ops = pinctrl_get_ops(dev);
+
+	if (!ops->get_pin_name)
+		return -ENOSYS;
+
+	snprintf(buf, size, ops->get_pin_name(dev, selector));
+
+	return 0;
+}
+
 int pinctrl_get_pin_muxing(struct udevice *dev, int selector, char *buf,
 			   int size)
 {
diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
index 4cfaddcaebbf..0b73f28eae97 100644
--- a/include/dm/pinctrl.h
+++ b/include/dm/pinctrl.h
@@ -380,4 +380,27 @@ int pinctrl_get_gpio_mux(struct udevice *dev, int banknum, int index);
 int pinctrl_get_pin_muxing(struct udevice *dev, int selector, char *buf,
 			   int size);
 
+/**
+ * pinctrl_get_pins_count() - display pin-controller pins number
+ *
+ * This allows to know the number of pins owned by a given pin-controller
+ *
+ * @dev:	Pinctrl device to use
+ * @return pins number if OK, -ve on error
+ */
+int pinctrl_get_pins_count(struct udevice *dev);
+
+/**
+ * pinctrl_get_pin_name() - Returns the pin's name
+ *
+ * This allows to display the pin's name for debug purpose
+ *
+ * @dev:	Pinctrl device to use
+ * @selector	Pin index within pin-controller
+ * @buf		Pin's name
+ * @return 0 if OK, -ve on error
+ */
+int pinctrl_get_pin_name(struct udevice *dev, int selector, char *buf,
+			 int size);
+
 #endif /* __PINCTRL_H */
-- 
1.9.1

[U-Boot] [PATCH v3 3/9] dm: uclass: Add uclass_foreach_dev_probe

[Patrice Chotard]

Add uclass_foreach_dev_probe() which iterates through
devices of a given uclass. Devices are probed if necessary
and are ready to use.

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v3: None
Changes in v2: None

 include/dm/uclass.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/dm/uclass.h b/include/dm/uclass.h
index 6e7c1cd3e8bc..10ccfdce951e 100644
--- a/include/dm/uclass.h
+++ b/include/dm/uclass.h
@@ -376,4 +376,20 @@ int uclass_resolve_seq(struct udevice *dev);
 #define uclass_foreach_dev_safe(pos, next, uc)	\
 	list_for_each_entry_safe(pos, next, &uc->dev_head, uclass_node)
 
+/**
+ * uclass_foreach_dev_probe() - Helper function to iteration through devices
+ * of given uclass
+ *
+ * This creates a for() loop which works through the available devices in
+ * a uclass in order from start to end. Devices are probed if necessary,
+ * and ready for use.
+ *
+ * @id: Uclass ID
+ * @dev: struct udevice * to hold the current device. Set to NULL when there
+ * are no more devices.
+ */
+#define uclass_foreach_dev_probe(id, dev)	\
+	for (uclass_first_device(id, &dev); dev; \
+	     uclass_next_device(&dev))
+
 #endif
-- 
1.9.1

[U-Boot] [PATCH v3 4/9] cmd: pinmux: Add pinmux command

[Patrice Chotard]

pinmux command allows to :
 - list all pin-controllers available on platforms
 - select a pin-controller
 - display the muxing of all pins of the current pin-controller
   or all pin-controllers depending of given options

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

Changes in v3:
 - Update calls to pinctrl_get_pin_name() and pinctrl_get_pin_muxing
   due to prototype update.

Changes in v2:
 - In order to make pin muxing display less SoC specific,
   use pinctrl_pins_count(), pinctrl_get_pin_name() and
   pinctrl_get_pin_muxing() methods instead of
   previous pinctrl_pinmux_show() method.

 cmd/Kconfig  |   8 ++++
 cmd/Makefile |   1 +
 cmd/pinmux.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+)
 create mode 100644 cmd/pinmux.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 7ed3c9c3b30b..f041e57a7ff3 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -953,6 +953,14 @@ config CMD_PCMCIA
 	  about 1990. These devices are typically removable memory or network
 	  cards using a standard 68-pin connector.
 
+config CMD_PINMUX
+	bool "pinmux - show pins muxing"
+	default y if PINCTRL
+	help
+	  Parse all available pin-controllers and show pins muxing. This
+	  is useful for debug purpoer to check the pin muxing and to know if
+	  a pin is configured as a GPIO or as an alternate function.
+
 config CMD_POWEROFF
 	bool "poweroff"
 	help
diff --git a/cmd/Makefile b/cmd/Makefile
index d9cdaf6064b8..4e562ae32c5b 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -104,6 +104,7 @@ ifdef CONFIG_PCI
 obj-$(CONFIG_CMD_PCI) += pci.o
 endif
 obj-y += pcmcia.o
+obj-$(CONFIG_CMD_PINMUX) += pinmux.o
 obj-$(CONFIG_CMD_PXE) += pxe.o
 obj-$(CONFIG_CMD_WOL) += wol.o
 obj-$(CONFIG_CMD_QFW) += qfw.o
diff --git a/cmd/pinmux.c b/cmd/pinmux.c
new file mode 100644
index 000000000000..55d5230a1657
--- /dev/null
+++ b/cmd/pinmux.c
@@ -0,0 +1,144 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018, STMicroelectronics - All Rights Reserved
+ */
+
+#include <common.h>
+#include <command.h>
+#include <dm.h>
+#include <errno.h>
+#include <dm/pinctrl.h>
+#include <dm/uclass-internal.h>
+
+#define LIMIT_DEVNAME	30
+#define LIMIT_PINNAME	10
+#define LIMIT_PINMUX	40
+
+static struct udevice *currdev;
+
+static int do_dev(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	const char *name;
+	int ret;
+
+	switch (argc) {
+	case 2:
+		name = argv[1];
+		ret = uclass_get_device_by_name(UCLASS_PINCTRL, name, &currdev);
+		if (ret) {
+			printf("Can't get the pin-controller: %s!\n", name);
+			return CMD_RET_FAILURE;
+		}
+	case 1:
+		if (!currdev) {
+			printf("Pin-controller device is not set!\n");
+			return CMD_RET_USAGE;
+		}
+
+		printf("dev: %s\n", currdev->name);
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static int show_pinmux(struct udevice *dev)
+{
+	char pin_name[LIMIT_PINNAME];
+	char pin_mux[LIMIT_PINMUX];
+	int pins_count;
+	int i;
+	int ret;
+
+	pins_count = pinctrl_get_pins_count(dev);
+
+	if (pins_count == -ENOSYS) {
+		printf("Ops get_pins_count not supported\n");
+		return pins_count;
+	}
+
+	for (i = 0; i < pins_count; i++) {
+		ret = pinctrl_get_pin_name(dev, i, pin_name, LIMIT_PINNAME);
+		if (ret == -ENOSYS) {
+			printf("Ops get_pin_name not supported\n");
+			return ret;
+		}
+
+		ret = pinctrl_get_pin_muxing(dev, i, pin_mux, LIMIT_PINMUX);
+		if (ret == -ENOSYS) {
+			printf("Ops get_pin_muxing not supported\n");
+			return ret;
+		}
+
+		printf("%-*s: %-*s\n", LIMIT_PINNAME, pin_name,
+		       LIMIT_PINMUX, pin_mux);
+	}
+
+	return 0;
+}
+
+static int do_status(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	struct udevice *dev;
+	int ret = 0;
+
+	if (currdev && (argc < 2 || strcmp(argv[1], "-a")))
+		return show_pinmux(currdev);
+
+	uclass_foreach_dev_probe(UCLASS_PINCTRL, dev) {
+		/* insert a separator between each pin-controller display */
+		printf("--------------------------\n");
+		printf("%s:\n", dev->name);
+		ret = show_pinmux(dev);
+		if (ret)
+			printf("Can't display pin muxing for %s\n", dev->name);
+	}
+
+	return ret;
+}
+
+static int do_list(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	struct udevice *dev;
+
+	printf("| %-*.*s| %-*.*s| %s\n",
+	       LIMIT_DEVNAME, LIMIT_DEVNAME, "Device",
+	       LIMIT_DEVNAME, LIMIT_DEVNAME, "Driver",
+	       "Parent");
+
+	uclass_foreach_dev_probe(UCLASS_PINCTRL, dev) {
+		printf("| %-*.*s| %-*.*s| %s\n",
+		       LIMIT_DEVNAME, LIMIT_DEVNAME, dev->name,
+		       LIMIT_DEVNAME, LIMIT_DEVNAME, dev->driver->name,
+		       dev->parent->name);
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static cmd_tbl_t pinmux_subcmd[] = {
+	U_BOOT_CMD_MKENT(dev, 2, 1, do_dev, "", ""),
+	U_BOOT_CMD_MKENT(list, 1, 1, do_list, "", ""),
+	U_BOOT_CMD_MKENT(status, 2, 1, do_status, "", ""),
+};
+
+static int do_pinmux(cmd_tbl_t *cmdtp, int flag, int argc,
+		     char * const argv[])
+{
+	cmd_tbl_t *cmd;
+
+	argc--;
+	argv++;
+
+	cmd = find_cmd_tbl(argv[0], pinmux_subcmd, ARRAY_SIZE(pinmux_subcmd));
+	if (!cmd || argc > cmd->maxargs)
+		return CMD_RET_USAGE;
+
+	return cmd->cmd(cmdtp, flag, argc, argv);
+}
+
+U_BOOT_CMD(pinmux, CONFIG_SYS_MAXARGS, 1, do_pinmux,
+	   "show pin-controller muxing",
+	   "list                     - list UCLASS_PINCTRL devices\n"
+	   "pinmux dev [pincontroller-name] - select pin-controller device\n"
+	   "pinmux status [-a]              - print pin-controller muxing [for all]\n"
+)
-- 
1.9.1

[U-Boot] [PATCH v3 5/9] pinctrl: stm32: Add get_pins_count() ops

[Patrice Chotard]

Add get_pins_count ops to obtain the number of pins
owns by a pin-controller.
On STM32 SoCs bindings, each pin-controller owns
several gpio banks. Each GPIO bank can own up to 16 pins.

To obtain the total pins count, walk through each sub-nodes
(ie GPIO banks) and sum each GPIO banks pins number. For that
in probe() we build a list with each GPIO device reference found.
This list will also be used with future get_pin_muxing and get_pin_name
ops to speed up and optimize walk through all GPIO banks.

As this code is common to all STM32 SoCs, this code is put
under SPL_BUILD compilation flag to avoid to increase SPL code size
for STM32F7 which is limited to 32Ko.

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

Changes in v3: None
Changes in v2: None

 drivers/pinctrl/pinctrl_stm32.c | 94 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 89 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/pinctrl_stm32.c b/drivers/pinctrl/pinctrl_stm32.c
index 31285cdd5784..82ac261dafea 100644
--- a/drivers/pinctrl/pinctrl_stm32.c
+++ b/drivers/pinctrl/pinctrl_stm32.c
@@ -14,6 +14,83 @@ DECLARE_GLOBAL_DATA_PTR;
 #define OTYPE_MSK			1
 #define AFR_MASK			0xF
 
+#ifndef CONFIG_SPL_BUILD
+struct stm32_pinctrl_priv {
+	int pinctrl_ngpios;
+	struct list_head gpio_dev;
+};
+
+struct stm32_gpio_bank {
+	struct udevice *gpio_dev;
+	struct list_head list;
+};
+
+static int stm32_pinctrl_get_pins_count(struct udevice *dev)
+{
+	struct stm32_pinctrl_priv *priv = dev_get_priv(dev);
+	struct gpio_dev_priv *uc_priv;
+	struct stm32_gpio_bank *gpio_bank;
+
+	/*
+	 * if get_pins_count has already been executed once on this
+	 * pin-controller, no need to run it again
+	 */
+	if (priv->pinctrl_ngpios)
+		return priv->pinctrl_ngpios;
+
+	/*
+	 * walk through all banks to retrieve the pin-controller
+	 * pins number
+	 */
+	list_for_each_entry(gpio_bank, &priv->gpio_dev, list) {
+		uc_priv = dev_get_uclass_priv(gpio_bank->gpio_dev);
+
+		priv->pinctrl_ngpios += uc_priv->gpio_count;
+	}
+
+	return priv->pinctrl_ngpios;
+}
+
+int stm32_pinctrl_probe(struct udevice *dev)
+{
+	struct stm32_pinctrl_priv *priv = dev_get_priv(dev);
+	struct udevice *gpio_dev;
+	struct udevice *child;
+	struct stm32_gpio_bank *gpio_bank;
+	int ret;
+
+	INIT_LIST_HEAD(&priv->gpio_dev);
+
+	/*
+	 * parse pin-controller sub-nodes (ie gpio bank nodes) and fill
+	 * a list with all gpio device reference which belongs to the
+	 * current pin-controller. This list is used to find pin_name and
+	 * pin muxing
+	 */
+	list_for_each_entry(child, &dev->child_head, sibling_node) {
+		ret = uclass_get_device_by_name(UCLASS_GPIO, child->name,
+						&gpio_dev);
+
+		if (ret < 0 && ret != -ENODEV) {
+			dev_err(dev, "Failed to find %s device ret = %d\n",
+				child->name, ret);
+			return ret;
+		}
+
+		gpio_bank = malloc(sizeof(*gpio_bank));
+		if (!gpio_bank) {
+			dev_err(dev, "Not enough memory\n");
+			return -ENOMEM;
+		}
+
+		gpio_bank->gpio_dev = gpio_dev;
+		list_add_tail(&gpio_bank->list, &priv->gpio_dev);
+	}
+
+	return 0;
+}
+#endif
+
 static int stm32_gpio_config(struct gpio_desc *desc,
 			     const struct stm32_gpio_ctl *ctl)
 {
@@ -182,6 +259,9 @@ static struct pinctrl_ops stm32_pinctrl_ops = {
 #else /* PINCTRL_FULL */
 	.set_state_simple	= stm32_pinctrl_set_state_simple,
 #endif /* PINCTRL_FULL */
+#ifndef CONFIG_SPL_BUILD
+	.get_pins_count		= stm32_pinctrl_get_pins_count,
+#endif
 };
 
 static const struct udevice_id stm32_pinctrl_ids[] = {
@@ -195,9 +275,13 @@ static const struct udevice_id stm32_pinctrl_ids[] = {
 };
 
 U_BOOT_DRIVER(pinctrl_stm32) = {
-	.name		= "pinctrl_stm32",
-	.id		= UCLASS_PINCTRL,
-	.of_match	= stm32_pinctrl_ids,
-	.ops		= &stm32_pinctrl_ops,
-	.bind		= dm_scan_fdt_dev,
+	.name			= "pinctrl_stm32",
+	.id			= UCLASS_PINCTRL,
+	.of_match		= stm32_pinctrl_ids,
+	.ops			= &stm32_pinctrl_ops,
+	.bind			= dm_scan_fdt_dev,
+#ifndef CONFIG_SPL_BUILD
+	.probe			= stm32_pinctrl_probe,
+	.priv_auto_alloc_size	= sizeof(struct stm32_pinctrl_priv),
+#endif
 };
-- 
1.9.1

[U-Boot] [PATCH v3 6/9] pinctrl: stm32: Add get_pin_name() ops

[Patrice Chotard]

Add get_pin_name ops to obtain a pin name given a
pin index of a specified pin-controller.

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

Changes in v3: None
Changes in v2: None

 drivers/pinctrl/pinctrl_stm32.c | 47 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/pinctrl/pinctrl_stm32.c b/drivers/pinctrl/pinctrl_stm32.c
index 82ac261dafea..e0bee4b97bd5 100644
--- a/drivers/pinctrl/pinctrl_stm32.c
+++ b/drivers/pinctrl/pinctrl_stm32.c
@@ -25,6 +25,10 @@ struct stm32_gpio_bank {
 	struct list_head list;
 };
 
+#define MAX_PIN_PER_BANK		16
+
+#define MAX_PIN_NAME_LEN		12
+static char pin_name[MAX_PIN_NAME_LEN];
 static int stm32_pinctrl_get_pins_count(struct udevice *dev)
 {
 	struct stm32_pinctrl_priv *priv = dev_get_priv(dev);
@@ -51,6 +55,48 @@ static int stm32_pinctrl_get_pins_count(struct udevice *dev)
 	return priv->pinctrl_ngpios;
 }
 
+static struct udevice *stm32_pinctrl_get_gpio_dev(struct udevice *dev,
+						  unsigned int selector)
+{
+	struct stm32_pinctrl_priv *priv = dev_get_priv(dev);
+	struct stm32_gpio_bank *gpio_bank;
+	struct gpio_dev_priv *uc_priv;
+	int first_pin = 0;
+
+	/* look up for the bank which owns the requested pin */
+	list_for_each_entry(gpio_bank, &priv->gpio_dev, list) {
+		uc_priv = dev_get_uclass_priv(gpio_bank->gpio_dev);
+
+		if (selector < (first_pin + uc_priv->gpio_count))
+			/* we found the bank */
+			return gpio_bank->gpio_dev;
+
+		first_pin += uc_priv->gpio_count;
+	}
+
+	return NULL;
+}
+
+static const char *stm32_pinctrl_get_pin_name(struct udevice *dev,
+					      unsigned int selector)
+{
+	struct gpio_dev_priv *uc_priv;
+	struct udevice *gpio_dev;
+
+	/* look up for the bank which owns the requested pin */
+	gpio_dev = stm32_pinctrl_get_gpio_dev(dev, selector);
+	if (!gpio_dev) {
+		snprintf(pin_name, MAX_PIN_NAME_LEN, "Error");
+	} else {
+		uc_priv = dev_get_uclass_priv(gpio_dev);
+
+		snprintf(pin_name, MAX_PIN_NAME_LEN, "%s%d",
+			 uc_priv->bank_name,
+			 selector % MAX_PIN_PER_BANK);
+	}
+
+	return pin_name;
+}
 int stm32_pinctrl_probe(struct udevice *dev)
 {
 	struct stm32_pinctrl_priv *priv = dev_get_priv(dev);
@@ -260,6 +306,7 @@ static struct pinctrl_ops stm32_pinctrl_ops = {
 	.set_state_simple	= stm32_pinctrl_set_state_simple,
 #endif /* PINCTRL_FULL */
 #ifndef CONFIG_SPL_BUILD
+	.get_pin_name		= stm32_pinctrl_get_pin_name,
 	.get_pins_count		= stm32_pinctrl_get_pins_count,
 #endif
 };
-- 
1.9.1

[U-Boot] [PATCH v3 7/9] pinctrl: stm32: Add get_pin_muxing() ops

[Patrice Chotard]

Add get_pin_muxing() ops to obtain the pin muxing description
a given pin index.

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

Changes in v3: None
Changes in v2: None

 drivers/pinctrl/pinctrl_stm32.c | 77 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/drivers/pinctrl/pinctrl_stm32.c b/drivers/pinctrl/pinctrl_stm32.c
index e0bee4b97bd5..588efda8df4b 100644
--- a/drivers/pinctrl/pinctrl_stm32.c
+++ b/drivers/pinctrl/pinctrl_stm32.c
@@ -27,8 +27,34 @@ struct stm32_gpio_bank {
 
 #define MAX_PIN_PER_BANK		16
 
+#define MAX_PIN_MUX_LEN			40
+static char pin_mux[MAX_PIN_MUX_LEN];
+
 #define MAX_PIN_NAME_LEN		12
 static char pin_name[MAX_PIN_NAME_LEN];
+#define PINMUX_MODE_COUNT		5
+static const char * const pinmux_mode[PINMUX_MODE_COUNT] = {
+	"gpio input",
+	"gpio output",
+	"analog",
+	"unknown",
+	"alt function",
+};
+
+static int stm32_pinctrl_get_af(struct udevice *dev, unsigned int offset)
+{
+	struct stm32_gpio_priv *priv = dev_get_priv(dev);
+	struct stm32_gpio_regs *regs = priv->regs;
+	u32 af;
+	u32 alt_shift = (offset % 8) * 4;
+	u32 alt_index =  offset / 8;
+
+	af = (readl(&regs->afr[alt_index]) &
+	      GENMASK(alt_shift + 3, alt_shift)) >> alt_shift;
+
+	return af;
+}
+
 static int stm32_pinctrl_get_pins_count(struct udevice *dev)
 {
 	struct stm32_pinctrl_priv *priv = dev_get_priv(dev);
@@ -97,6 +123,56 @@ static const char *stm32_pinctrl_get_pin_name(struct udevice *dev,
 
 	return pin_name;
 }
+
+static const char *stm32_pinctrl_get_pin_muxing(struct udevice *dev,
+						unsigned int selector)
+{
+	struct udevice *gpio_dev;
+	const char *label;
+	int gpio_pin;
+	int mode;
+	int af_num;
+
+	/* look up for the bank which owns the requested pin */
+	gpio_dev = stm32_pinctrl_get_gpio_dev(dev, selector);
+
+	if (!gpio_dev) {
+		snprintf(pin_mux, MAX_PIN_MUX_LEN, "Error");
+		return pin_mux;
+	}
+
+	/* translate pin-controller pin number to gpio pin number */
+	gpio_pin = selector % MAX_PIN_PER_BANK;
+
+	mode = gpio_get_raw_function(gpio_dev, gpio_pin, &label);
+
+	dev_dbg(dev, "selector = %d gpio_pin = %d mode = %d\n",
+		selector, gpio_pin, mode);
+
+	switch (mode) {
+	case GPIOF_UNKNOWN:
+		/* should never happen */
+		snprintf(pin_mux, MAX_PIN_MUX_LEN, "Error !!");
+		break;
+	case GPIOF_UNUSED:
+		snprintf(pin_mux, MAX_PIN_MUX_LEN, "%s",
+			 pinmux_mode[mode]);
+		break;
+	case GPIOF_FUNC:
+		af_num = stm32_pinctrl_get_af(gpio_dev, gpio_pin);
+		snprintf(pin_mux, MAX_PIN_MUX_LEN, "%s %d",
+			 pinmux_mode[mode], af_num);
+		break;
+	case GPIOF_OUTPUT:
+	case GPIOF_INPUT:
+		snprintf(pin_mux, MAX_PIN_MUX_LEN, "%s %s",
+			 pinmux_mode[mode], label ? label : "");
+		break;
+	}
+
+	return pin_mux;
+}
+
 int stm32_pinctrl_probe(struct udevice *dev)
 {
 	struct stm32_pinctrl_priv *priv = dev_get_priv(dev);
@@ -308,6 +384,7 @@ static struct pinctrl_ops stm32_pinctrl_ops = {
 #ifndef CONFIG_SPL_BUILD
 	.get_pin_name		= stm32_pinctrl_get_pin_name,
 	.get_pins_count		= stm32_pinctrl_get_pins_count,
+	.get_pin_muxing		= stm32_pinctrl_get_pin_muxing,
 #endif
 };
 
-- 
1.9.1

[U-Boot] [PATCH v3 8/9] gpio: stm32f7: Add ops get_function

[Patrice Chotard]

This patch adds gpio get_function ops support.
This function reports the state of a gpio.

Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

Changes in v3: None
Changes in v2: None

 drivers/gpio/stm32f7_gpio.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpio/stm32f7_gpio.c b/drivers/gpio/stm32f7_gpio.c
index 4c0786fff88d..82c8b8d23ae6 100644
--- a/drivers/gpio/stm32f7_gpio.c
+++ b/drivers/gpio/stm32f7_gpio.c
@@ -65,11 +65,31 @@ static int stm32_gpio_set_value(struct udevice *dev, unsigned offset, int value)
 	return 0;
 }
 
+static int stm32_gpio_get_function(struct udevice *dev, unsigned int offset)
+{
+	struct stm32_gpio_priv *priv = dev_get_priv(dev);
+	struct stm32_gpio_regs *regs = priv->regs;
+	int bits_index = MODE_BITS(offset);
+	int mask = MODE_BITS_MASK << bits_index;
+	u32 mode;
+
+	mode = (readl(&regs->moder) & mask) >> bits_index;
+	if (mode == STM32_GPIO_MODE_OUT)
+		return GPIOF_OUTPUT;
+	if (mode == STM32_GPIO_MODE_IN)
+		return GPIOF_INPUT;
+	if (mode == STM32_GPIO_MODE_AN)
+		return GPIOF_UNUSED;
+
+	return GPIOF_FUNC;
+}
+
 static const struct dm_gpio_ops gpio_stm32_ops = {
 	.direction_input	= stm32_gpio_direction_input,
 	.direction_output	= stm32_gpio_direction_output,
 	.get_value		= stm32_gpio_get_value,
 	.set_value		= stm32_gpio_set_value,
+	.get_function		= stm32_gpio_get_function,
 };
 
 static int gpio_stm32_probe(struct udevice *dev)
-- 
1.9.1

[U-Boot] [PATCH v3 9/9] pinctrl: sandbox: Add get_pin_muxing ops support

[Patrice Chotard]

Add get_pin_mux ops support to display the pin muxing
description of the sandbox_pins[]

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

Changes in v3:
 - Fix typo

Changes in v2: None

 drivers/pinctrl/pinctrl-sandbox.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-sandbox.c b/drivers/pinctrl/pinctrl-sandbox.c
index 755ac08bdf72..c09e0f2d0e37 100644
--- a/drivers/pinctrl/pinctrl-sandbox.c
+++ b/drivers/pinctrl/pinctrl-sandbox.c
@@ -17,6 +17,14 @@ static const char * const sandbox_pins[] = {
 	"W1"
 };
 
+static const char * const sandbox_pins_muxing[] = {
+	"I2C SCL",
+	"I2C SDA",
+	"Uart TX",
+	"Uart RX",
+	"1-wire gpio",
+};
+
 static const char * const sandbox_groups[] = {
 	"i2c",
 	"serial_a",
@@ -56,6 +64,12 @@ static const char *sandbox_get_pin_name(struct udevice *dev, unsigned selector)
 	return sandbox_pins[selector];
 }
 
+static const char *sandbox_get_pin_muxing(struct udevice *dev,
+					  unsigned int selector)
+{
+	return sandbox_pins_muxing[selector];
+}
+
 static int sandbox_get_groups_count(struct udevice *dev)
 {
 	return ARRAY_SIZE(sandbox_groups);
@@ -123,6 +137,7 @@ static int sandbox_pinconf_group_set(struct udevice *dev,
 const struct pinctrl_ops sandbox_pinctrl_ops = {
 	.get_pins_count = sandbox_get_pins_count,
 	.get_pin_name = sandbox_get_pin_name,
+	.get_pin_muxing = sandbox_get_pin_muxing,
 	.get_groups_count = sandbox_get_groups_count,
 	.get_group_name = sandbox_get_group_name,
 	.get_functions_count = sandbox_get_functions_count,
-- 
1.9.1

[U-Boot] [PATCH v3 3/9] dm: uclass: Add uclass_foreach_dev_probe

[Bin Meng]

Hi Patrice,

On Tue, Oct 9, 2018 at 9:41 PM Patrice Chotard <patrice.chotard@st.com> wrote:
>
> Add uclass_foreach_dev_probe() which iterates through
> devices of a given uclass. Devices are probed if necessary
> and are ready to use.
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  include/dm/uclass.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
> index 6e7c1cd3e8bc..10ccfdce951e 100644
> --- a/include/dm/uclass.h
> +++ b/include/dm/uclass.h
> @@ -376,4 +376,20 @@ int uclass_resolve_seq(struct udevice *dev);
>  #define uclass_foreach_dev_safe(pos, next, uc) \
>         list_for_each_entry_safe(pos, next, &uc->dev_head, uclass_node)
>
> +/**
> + * uclass_foreach_dev_probe() - Helper function to iteration through devices
> + * of given uclass
> + *
> + * This creates a for() loop which works through the available devices in
> + * a uclass in order from start to end. Devices are probed if necessary,
> + * and ready for use.
> + *
> + * @id: Uclass ID
> + * @dev: struct udevice * to hold the current device. Set to NULL when there
> + * are no more devices.
> + */
> +#define uclass_foreach_dev_probe(id, dev)      \
> +       for (uclass_first_device(id, &dev); dev; \
> +            uclass_next_device(&dev))

Shouldn't we check the return value of uclass_first_device() and
uclass_next_device()?

> +
>  #endif

Regards,
Bin

[U-Boot] [PATCH v3 3/9] dm: uclass: Add uclass_foreach_dev_probe

[Patrice CHOTARD]

Hi Bin

On 10/11/2018 11:06 AM, Bin Meng wrote:
> Hi Patrice,
> 
> On Tue, Oct 9, 2018 at 9:41 PM Patrice Chotard <patrice.chotard@st.com> wrote:
>>
>> Add uclass_foreach_dev_probe() which iterates through
>> devices of a given uclass. Devices are probed if necessary
>> and are ready to use.
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>  include/dm/uclass.h | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
>> index 6e7c1cd3e8bc..10ccfdce951e 100644
>> --- a/include/dm/uclass.h
>> +++ b/include/dm/uclass.h
>> @@ -376,4 +376,20 @@ int uclass_resolve_seq(struct udevice *dev);
>>  #define uclass_foreach_dev_safe(pos, next, uc) \
>>         list_for_each_entry_safe(pos, next, &uc->dev_head, uclass_node)
>>
>> +/**
>> + * uclass_foreach_dev_probe() - Helper function to iteration through devices
>> + * of given uclass
>> + *
>> + * This creates a for() loop which works through the available devices in
>> + * a uclass in order from start to end. Devices are probed if necessary,
>> + * and ready for use.
>> + *
>> + * @id: Uclass ID
>> + * @dev: struct udevice * to hold the current device. Set to NULL when there
>> + * are no more devices.
>> + */
>> +#define uclass_foreach_dev_probe(id, dev)      \
>> +       for (uclass_first_device(id, &dev); dev; \
>> +            uclass_next_device(&dev))
> 
> Shouldn't we check the return value of uclass_first_device() and
> uclass_next_device()?

It's not necessary to check the return value of uclass_first_device(id,
&dev) because in any error case, dev is NULL, which is the loop output
condition. This is only for the first iteration.

For the other iteration, dev comes from uclass_next_device(&dev),
similarly to uclass_first_device(), in any error case dev is NULL.

Thanks

Patrice


> 
>> +
>>  #endif
> 
> Regards,
> Bin
> 

[U-Boot] [PATCH v3 3/9] dm: uclass: Add uclass_foreach_dev_probe

[Bin Meng]

Hi Patrice,

On Fri, Oct 12, 2018 at 3:51 PM Patrice CHOTARD <patrice.chotard@st.com> wrote:
>
> Hi Bin
>
> On 10/11/2018 11:06 AM, Bin Meng wrote:
> > Hi Patrice,
> >
> > On Tue, Oct 9, 2018 at 9:41 PM Patrice Chotard <patrice.chotard@st.com> wrote:
> >>
> >> Add uclass_foreach_dev_probe() which iterates through
> >> devices of a given uclass. Devices are probed if necessary
> >> and are ready to use.
> >>
> >> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> >> ---
> >>
> >> Changes in v3: None
> >> Changes in v2: None
> >>
> >>  include/dm/uclass.h | 16 ++++++++++++++++
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
> >> index 6e7c1cd3e8bc..10ccfdce951e 100644
> >> --- a/include/dm/uclass.h
> >> +++ b/include/dm/uclass.h
> >> @@ -376,4 +376,20 @@ int uclass_resolve_seq(struct udevice *dev);
> >>  #define uclass_foreach_dev_safe(pos, next, uc) \
> >>         list_for_each_entry_safe(pos, next, &uc->dev_head, uclass_node)
> >>
> >> +/**
> >> + * uclass_foreach_dev_probe() - Helper function to iteration through devices
> >> + * of given uclass
> >> + *
> >> + * This creates a for() loop which works through the available devices in
> >> + * a uclass in order from start to end. Devices are probed if necessary,
> >> + * and ready for use.
> >> + *
> >> + * @id: Uclass ID
> >> + * @dev: struct udevice * to hold the current device. Set to NULL when there
> >> + * are no more devices.
> >> + */
> >> +#define uclass_foreach_dev_probe(id, dev)      \
> >> +       for (uclass_first_device(id, &dev); dev; \
> >> +            uclass_next_device(&dev))
> >
> > Shouldn't we check the return value of uclass_first_device() and
> > uclass_next_device()?
>
> It's not necessary to check the return value of uclass_first_device(id,
> &dev) because in any error case, dev is NULL, which is the loop output
> condition. This is only for the first iteration.
>

Please see the comment of uclass_first_device().

 * @devp: Returns pointer to the first device in that uclass if no error
 * occurred, or NULL if there is no first device, or an error occurred with
 * that device.
 * @return 0 if OK (found or not found), other -ve on error

So it can return error with a valid dev.

> For the other iteration, dev comes from uclass_next_device(&dev),
> similarly to uclass_first_device(), in any error case dev is NULL.
>

Similar please see the comment of uclass_next_device().

Regards,
Bin

[U-Boot] [PATCH v3 3/9] dm: uclass: Add uclass_foreach_dev_probe

[Patrice CHOTARD]

Hi Bin

On 10/12/2018 12:13 PM, Bin Meng wrote:
> Hi Patrice,
> 
> On Fri, Oct 12, 2018 at 3:51 PM Patrice CHOTARD <patrice.chotard@st.com> wrote:
>>
>> Hi Bin
>>
>> On 10/11/2018 11:06 AM, Bin Meng wrote:
>>> Hi Patrice,
>>>
>>> On Tue, Oct 9, 2018 at 9:41 PM Patrice Chotard <patrice.chotard@st.com> wrote:
>>>>
>>>> Add uclass_foreach_dev_probe() which iterates through
>>>> devices of a given uclass. Devices are probed if necessary
>>>> and are ready to use.
>>>>
>>>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>
>>>> Changes in v3: None
>>>> Changes in v2: None
>>>>
>>>>  include/dm/uclass.h | 16 ++++++++++++++++
>>>>  1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
>>>> index 6e7c1cd3e8bc..10ccfdce951e 100644
>>>> --- a/include/dm/uclass.h
>>>> +++ b/include/dm/uclass.h
>>>> @@ -376,4 +376,20 @@ int uclass_resolve_seq(struct udevice *dev);
>>>>  #define uclass_foreach_dev_safe(pos, next, uc) \
>>>>         list_for_each_entry_safe(pos, next, &uc->dev_head, uclass_node)
>>>>
>>>> +/**
>>>> + * uclass_foreach_dev_probe() - Helper function to iteration through devices
>>>> + * of given uclass
>>>> + *
>>>> + * This creates a for() loop which works through the available devices in
>>>> + * a uclass in order from start to end. Devices are probed if necessary,
>>>> + * and ready for use.
>>>> + *
>>>> + * @id: Uclass ID
>>>> + * @dev: struct udevice * to hold the current device. Set to NULL when there
>>>> + * are no more devices.
>>>> + */
>>>> +#define uclass_foreach_dev_probe(id, dev)      \
>>>> +       for (uclass_first_device(id, &dev); dev; \
>>>> +            uclass_next_device(&dev))
>>>
>>> Shouldn't we check the return value of uclass_first_device() and
>>> uclass_next_device()?
>>
>> It's not necessary to check the return value of uclass_first_device(id,
>> &dev) because in any error case, dev is NULL, which is the loop output
>> condition. This is only for the first iteration.
>>
> 
> Please see the comment of uclass_first_device().
> 
>  * @devp: Returns pointer to the first device in that uclass if no error
>  * occurred, or NULL if there is no first device, or an error occurred with
>  * that device.
>  * @return 0 if OK (found or not found), other -ve on error
> 
> So it can return error with a valid dev.

I checked the uclass_first_device() implementation and the comment seems
not aligned with what is coded. When uclass_first_device() returns a
valid dev, return value is always 0. (see uclass_get_device_tail()
return value).

In any case uclass_first_device() returns a valid dev with an error.

Simon, Bin do you confirm ?

> 
>> For the other iteration, dev comes from uclass_next_device(&dev),
>> similarly to uclass_first_device(), in any error case dev is NULL.
>>
> 
> Similar please see the comment of uclass_next_device().

I have the same remark regarding uclass_next_device().

Thanks

Patrice

> 
> Regards,
> Bin
> 

[U-Boot] [PATCH v3 5/9] pinctrl: stm32: Add get_pins_count() ops

[Simon Glass]

On 9 October 2018 at 07:31, Patrice Chotard <patrice.chotard@st.com> wrote:
> Add get_pins_count ops to obtain the number of pins
> owns by a pin-controller.
> On STM32 SoCs bindings, each pin-controller owns
> several gpio banks. Each GPIO bank can own up to 16 pins.
>
> To obtain the total pins count, walk through each sub-nodes
> (ie GPIO banks) and sum each GPIO banks pins number. For that
> in probe() we build a list with each GPIO device reference found.
> This list will also be used with future get_pin_muxing and get_pin_name
> ops to speed up and optimize walk through all GPIO banks.
>
> As this code is common to all STM32 SoCs, this code is put
> under SPL_BUILD compilation flag to avoid to increase SPL code size
> for STM32F7 which is limited to 32Ko.
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/pinctrl/pinctrl_stm32.c | 94 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 89 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v3 6/9] pinctrl: stm32: Add get_pin_name() ops

[Simon Glass]

On 9 October 2018 at 07:31, Patrice Chotard <patrice.chotard@st.com> wrote:
> Add get_pin_name ops to obtain a pin name given a
> pin index of a specified pin-controller.
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/pinctrl/pinctrl_stm32.c | 47 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v3 7/9] pinctrl: stm32: Add get_pin_muxing() ops

[Simon Glass]

Hi Patrice,

On 9 October 2018 at 07:31, Patrice Chotard <patrice.chotard@st.com> wrote:
> Add get_pin_muxing() ops to obtain the pin muxing description
> a given pin index.
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/pinctrl/pinctrl_stm32.c | 77 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl_stm32.c b/drivers/pinctrl/pinctrl_stm32.c
> index e0bee4b97bd5..588efda8df4b 100644
> --- a/drivers/pinctrl/pinctrl_stm32.c
> +++ b/drivers/pinctrl/pinctrl_stm32.c
> @@ -27,8 +27,34 @@ struct stm32_gpio_bank {
>
>  #define MAX_PIN_PER_BANK               16
>
> +#define MAX_PIN_MUX_LEN                        40
> +static char pin_mux[MAX_PIN_MUX_LEN];
> +
>  #define MAX_PIN_NAME_LEN               12
>  static char pin_name[MAX_PIN_NAME_LEN];
> +#define PINMUX_MODE_COUNT              5
> +static const char * const pinmux_mode[PINMUX_MODE_COUNT] = {
> +       "gpio input",
> +       "gpio output",
> +       "analog",
> +       "unknown",
> +       "alt function",
> +};
> +
> +static int stm32_pinctrl_get_af(struct udevice *dev, unsigned int offset)
> +{
> +       struct stm32_gpio_priv *priv = dev_get_priv(dev);
> +       struct stm32_gpio_regs *regs = priv->regs;
> +       u32 af;
> +       u32 alt_shift = (offset % 8) * 4;
> +       u32 alt_index =  offset / 8;
> +
> +       af = (readl(&regs->afr[alt_index]) &
> +             GENMASK(alt_shift + 3, alt_shift)) >> alt_shift;
> +
> +       return af;
> +}
> +
>  static int stm32_pinctrl_get_pins_count(struct udevice *dev)
>  {
>         struct stm32_pinctrl_priv *priv = dev_get_priv(dev);
> @@ -97,6 +123,56 @@ static const char *stm32_pinctrl_get_pin_name(struct udevice *dev,
>
>         return pin_name;
>  }
> +
> +static const char *stm32_pinctrl_get_pin_muxing(struct udevice *dev,
> +                                               unsigned int selector)

Again I think this should be passed a buffer to write into.

Regards,
Simon

[U-Boot] [PATCH v3 9/9] pinctrl: sandbox: Add get_pin_muxing ops support

[Simon Glass]

On 9 October 2018 at 07:31, Patrice Chotard <patrice.chotard@st.com> wrote:
> Add get_pin_mux ops support to display the pin muxing
> description of the sandbox_pins[]
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> Changes in v3:
>  - Fix typo
>
> Changes in v2: None
>
>  drivers/pinctrl/pinctrl-sandbox.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl-sandbox.c b/drivers/pinctrl/pinctrl-sandbox.c
> index 755ac08bdf72..c09e0f2d0e37 100644
> --- a/drivers/pinctrl/pinctrl-sandbox.c
> +++ b/drivers/pinctrl/pinctrl-sandbox.c
> @@ -17,6 +17,14 @@ static const char * const sandbox_pins[] = {
>         "W1"
>  };
>
> +static const char * const sandbox_pins_muxing[] = {
> +       "I2C SCL",
> +       "I2C SDA",
> +       "Uart TX",
> +       "Uart RX",
> +       "1-wire gpio",
> +};
> +
>  static const char * const sandbox_groups[] = {
>         "i2c",
>         "serial_a",
> @@ -56,6 +64,12 @@ static const char *sandbox_get_pin_name(struct udevice *dev, unsigned selector)
>         return sandbox_pins[selector];
>  }
>
> +static const char *sandbox_get_pin_muxing(struct udevice *dev,
> +                                         unsigned int selector)
> +{
> +       return sandbox_pins_muxing[selector];
> +}
> +
>  static int sandbox_get_groups_count(struct udevice *dev)
>  {
>         return ARRAY_SIZE(sandbox_groups);
> @@ -123,6 +137,7 @@ static int sandbox_pinconf_group_set(struct udevice *dev,
>  const struct pinctrl_ops sandbox_pinctrl_ops = {
>         .get_pins_count = sandbox_get_pins_count,
>         .get_pin_name = sandbox_get_pin_name,
> +       .get_pin_muxing = sandbox_get_pin_muxing,
>         .get_groups_count = sandbox_get_groups_count,
>         .get_group_name = sandbox_get_group_name,
>         .get_functions_count = sandbox_get_functions_count,
> --
> 1.9.1
>

Reviewed-by: Simon Glass <sjg@chromium.org>

I suggest adding a test that executes this command on sandbox.

[U-Boot] [PATCH v3 3/9] dm: uclass: Add uclass_foreach_dev_probe

[Simon Glass]

Hi Patrick,

On 15 October 2018 at 03:01, Patrice CHOTARD <patrice.chotard@st.com> wrote:
> Hi Bin
>
> On 10/12/2018 12:13 PM, Bin Meng wrote:
>> Hi Patrice,
>>
>> On Fri, Oct 12, 2018 at 3:51 PM Patrice CHOTARD <patrice.chotard@st.com> wrote:
>>>
>>> Hi Bin
>>>
>>> On 10/11/2018 11:06 AM, Bin Meng wrote:
>>>> Hi Patrice,
>>>>
>>>> On Tue, Oct 9, 2018 at 9:41 PM Patrice Chotard <patrice.chotard@st.com> wrote:
>>>>>
>>>>> Add uclass_foreach_dev_probe() which iterates through
>>>>> devices of a given uclass. Devices are probed if necessary
>>>>> and are ready to use.
>>>>>
>>>>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>> ---
>>>>>
>>>>> Changes in v3: None
>>>>> Changes in v2: None
>>>>>
>>>>>  include/dm/uclass.h | 16 ++++++++++++++++
>>>>>  1 file changed, 16 insertions(+)
>>>>>
>>>>> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
>>>>> index 6e7c1cd3e8bc..10ccfdce951e 100644
>>>>> --- a/include/dm/uclass.h
>>>>> +++ b/include/dm/uclass.h
>>>>> @@ -376,4 +376,20 @@ int uclass_resolve_seq(struct udevice *dev);
>>>>>  #define uclass_foreach_dev_safe(pos, next, uc) \
>>>>>         list_for_each_entry_safe(pos, next, &uc->dev_head, uclass_node)
>>>>>
>>>>> +/**
>>>>> + * uclass_foreach_dev_probe() - Helper function to iteration through devices
>>>>> + * of given uclass
>>>>> + *
>>>>> + * This creates a for() loop which works through the available devices in
>>>>> + * a uclass in order from start to end. Devices are probed if necessary,
>>>>> + * and ready for use.
>>>>> + *
>>>>> + * @id: Uclass ID
>>>>> + * @dev: struct udevice * to hold the current device. Set to NULL when there
>>>>> + * are no more devices.
>>>>> + */
>>>>> +#define uclass_foreach_dev_probe(id, dev)      \
>>>>> +       for (uclass_first_device(id, &dev); dev; \
>>>>> +            uclass_next_device(&dev))
>>>>
>>>> Shouldn't we check the return value of uclass_first_device() and
>>>> uclass_next_device()?
>>>
>>> It's not necessary to check the return value of uclass_first_device(id,
>>> &dev) because in any error case, dev is NULL, which is the loop output
>>> condition. This is only for the first iteration.
>>>
>>
>> Please see the comment of uclass_first_device().
>>
>>  * @devp: Returns pointer to the first device in that uclass if no error
>>  * occurred, or NULL if there is no first device, or an error occurred with
>>  * that device.
>>  * @return 0 if OK (found or not found), other -ve on error
>>
>> So it can return error with a valid dev.
>
> I checked the uclass_first_device() implementation and the comment seems
> not aligned with what is coded. When uclass_first_device() returns a
> valid dev, return value is always 0. (see uclass_get_device_tail()
> return value).

I actually don't like this function. Let's use
uclass_first_device_err() when we care about errors. I am wondering if
we should delete uclass_first_device().
>
> In any case uclass_first_device() returns a valid dev with an error.
>
> Simon, Bin do you confirm ?

I don't see how, no.
>
>>
>>> For the other iteration, dev comes from uclass_next_device(&dev),
>>> similarly to uclass_first_device(), in any error case dev is NULL.
>>>
>>
>> Similar please see the comment of uclass_next_device().
>
> I have the same remark regarding uclass_next_device().


Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v3 1/9] dm: pinctrl: Add get_pin_muxing() ops

[Simon Glass]

Hi Patrice,

On 9 October 2018 at 07:31, Patrice Chotard <patrice.chotard@st.com> wrote:
> Add get_pin_muxing() which allows to display the muxing
> of a given pin belonging to a pin-controller.
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> Changes in v3:
>  - Replace const char **buf parameter by char *buf, int size parameters
>    for pinctrl_get_pin_muxing()
>
> Changes in v2:
>  - Replace pinmux_show ops which displayed the complete pin-controller
>    muxing by get_pin_muxing ops which displays the muxing of one pin
>
>  drivers/pinctrl/pinctrl-uclass.c | 13 +++++++++++++
>  include/dm/pinctrl.h             | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

But please see below.

>
> diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
> index c38bb212ed74..cd2ca4e4950e 100644
> --- a/drivers/pinctrl/pinctrl-uclass.c
> +++ b/drivers/pinctrl/pinctrl-uclass.c
> @@ -249,6 +249,19 @@ int pinctrl_get_gpio_mux(struct udevice *dev, int banknum, int index)
>         return ops->get_gpio_mux(dev, banknum, index);
>  }
>
> +int pinctrl_get_pin_muxing(struct udevice *dev, int selector, char *buf,
> +                          int size)
> +{
> +       struct pinctrl_ops *ops = pinctrl_get_ops(dev);
> +
> +       if (!ops->get_pin_muxing)
> +               return -ENOSYS;
> +
> +       snprintf(buf, size, ops->get_pin_muxing(dev, selector));

I wonder if it would be easy to return -ENOSPC if the string is too long?

Regards,
Simon

[U-Boot] [PATCH v3 2/9] dm: pinctrl: Add pinctrl_get_pin_name and pinctrl_get_pins_count

[Simon Glass]

On 9 October 2018 at 07:31, Patrice Chotard <patrice.chotard@st.com> wrote:
> Add pinctrl_get_pin_name() and pinctrl_get_pins_count() methods
> to obtain pin's name and pin's muxing given a pin reference.
>
> This will be used by the new pinmux command.
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> Changes in v3:
>  - Replace const char **buf parameter by char *buf, int size parameters
>    for pinctrl_get_pin_name()
>
> Changes in v2: None
>
>  drivers/pinctrl/pinctrl-uclass.c | 23 +++++++++++++++++++++++
>  include/dm/pinctrl.h             | 23 +++++++++++++++++++++++
>  2 files changed, 46 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v3 4/9] cmd: pinmux: Add pinmux command

[Simon Glass]

On 9 October 2018 at 07:31, Patrice Chotard <patrice.chotard@st.com> wrote:
> pinmux command allows to :
>  - list all pin-controllers available on platforms
>  - select a pin-controller
>  - display the muxing of all pins of the current pin-controller
>    or all pin-controllers depending of given options
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> Changes in v3:
>  - Update calls to pinctrl_get_pin_name() and pinctrl_get_pin_muxing
>    due to prototype update.
>
> Changes in v2:
>  - In order to make pin muxing display less SoC specific,
>    use pinctrl_pins_count(), pinctrl_get_pin_name() and
>    pinctrl_get_pin_muxing() methods instead of
>    previous pinctrl_pinmux_show() method.
>
>  cmd/Kconfig  |   8 ++++
>  cmd/Makefile |   1 +
>  cmd/pinmux.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 153 insertions(+)
>  create mode 100644 cmd/pinmux.c

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v3 9/9] pinctrl: sandbox: Add get_pin_muxing ops support

[Patrice CHOTARD]

Hi Simon

On 10/19/18 5:25 AM, Simon Glass wrote:
> On 9 October 2018 at 07:31, Patrice Chotard <patrice.chotard@st.com> wrote:
>> Add get_pin_mux ops support to display the pin muxing
>> description of the sandbox_pins[]
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> Changes in v3:
>>  - Fix typo
>>
>> Changes in v2: None
>>
>>  drivers/pinctrl/pinctrl-sandbox.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/pinctrl/pinctrl-sandbox.c b/drivers/pinctrl/pinctrl-sandbox.c
>> index 755ac08bdf72..c09e0f2d0e37 100644
>> --- a/drivers/pinctrl/pinctrl-sandbox.c
>> +++ b/drivers/pinctrl/pinctrl-sandbox.c
>> @@ -17,6 +17,14 @@ static const char * const sandbox_pins[] = {
>>         "W1"
>>  };
>>
>> +static const char * const sandbox_pins_muxing[] = {
>> +       "I2C SCL",
>> +       "I2C SDA",
>> +       "Uart TX",
>> +       "Uart RX",
>> +       "1-wire gpio",
>> +};
>> +
>>  static const char * const sandbox_groups[] = {
>>         "i2c",
>>         "serial_a",
>> @@ -56,6 +64,12 @@ static const char *sandbox_get_pin_name(struct udevice *dev, unsigned selector)
>>         return sandbox_pins[selector];
>>  }
>>
>> +static const char *sandbox_get_pin_muxing(struct udevice *dev,
>> +                                         unsigned int selector)
>> +{
>> +       return sandbox_pins_muxing[selector];
>> +}
>> +
>>  static int sandbox_get_groups_count(struct udevice *dev)
>>  {
>>         return ARRAY_SIZE(sandbox_groups);
>> @@ -123,6 +137,7 @@ static int sandbox_pinconf_group_set(struct udevice *dev,
>>  const struct pinctrl_ops sandbox_pinctrl_ops = {
>>         .get_pins_count = sandbox_get_pins_count,
>>         .get_pin_name = sandbox_get_pin_name,
>> +       .get_pin_muxing = sandbox_get_pin_muxing,
>>         .get_groups_count = sandbox_get_groups_count,
>>         .get_group_name = sandbox_get_group_name,
>>         .get_functions_count = sandbox_get_functions_count,
>> --
>> 1.9.1
>>
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> I suggest adding a test that executes this command on sandbox.
> 

Ok i will add a test for that.

Thanks

Patrice

[U-Boot] [PATCH v3 1/9] dm: pinctrl: Add get_pin_muxing() ops

[Patrice CHOTARD]

Hi Simon

On 10/19/18 5:28 AM, Simon Glass wrote:
> Hi Patrice,
> 
> On 9 October 2018 at 07:31, Patrice Chotard <patrice.chotard@st.com> wrote:
>> Add get_pin_muxing() which allows to display the muxing
>> of a given pin belonging to a pin-controller.
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> Changes in v3:
>>  - Replace const char **buf parameter by char *buf, int size parameters
>>    for pinctrl_get_pin_muxing()
>>
>> Changes in v2:
>>  - Replace pinmux_show ops which displayed the complete pin-controller
>>    muxing by get_pin_muxing ops which displays the muxing of one pin
>>
>>  drivers/pinctrl/pinctrl-uclass.c | 13 +++++++++++++
>>  include/dm/pinctrl.h             | 32 ++++++++++++++++++++++++++++++++
>>  2 files changed, 45 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> But please see below.
> 
>>
>> diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
>> index c38bb212ed74..cd2ca4e4950e 100644
>> --- a/drivers/pinctrl/pinctrl-uclass.c
>> +++ b/drivers/pinctrl/pinctrl-uclass.c
>> @@ -249,6 +249,19 @@ int pinctrl_get_gpio_mux(struct udevice *dev, int banknum, int index)
>>         return ops->get_gpio_mux(dev, banknum, index);
>>  }
>>
>> +int pinctrl_get_pin_muxing(struct udevice *dev, int selector, char *buf,
>> +                          int size)
>> +{
>> +       struct pinctrl_ops *ops = pinctrl_get_ops(dev);
>> +
>> +       if (!ops->get_pin_muxing)
>> +               return -ENOSYS;
>> +
>> +       snprintf(buf, size, ops->get_pin_muxing(dev, selector));
> 
> I wonder if it would be easy to return -ENOSPC if the string is too long?

Not so easy, for example in pinctrl_stm32.c, depending of the pin's
muxing mode, i not only copy but also, in some case, concatenate
additional information. It would complexify the pinctrl code.


Patrice

> 
> Regards,
> Simon
> 

[U-Boot] [PATCH v3 7/9] pinctrl: stm32: Add get_pin_muxing() ops

[Patrice CHOTARD]

Hi Simon

On 10/19/18 5:25 AM, Simon Glass wrote:
> Hi Patrice,
> 
> On 9 October 2018 at 07:31, Patrice Chotard <patrice.chotard@st.com> wrote:
>> Add get_pin_muxing() ops to obtain the pin muxing description
>> a given pin index.
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>  drivers/pinctrl/pinctrl_stm32.c | 77 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 77 insertions(+)
>>
>> diff --git a/drivers/pinctrl/pinctrl_stm32.c b/drivers/pinctrl/pinctrl_stm32.c
>> index e0bee4b97bd5..588efda8df4b 100644
>> --- a/drivers/pinctrl/pinctrl_stm32.c
>> +++ b/drivers/pinctrl/pinctrl_stm32.c
>> @@ -27,8 +27,34 @@ struct stm32_gpio_bank {
>>
>>  #define MAX_PIN_PER_BANK               16
>>
>> +#define MAX_PIN_MUX_LEN                        40
>> +static char pin_mux[MAX_PIN_MUX_LEN];
>> +
>>  #define MAX_PIN_NAME_LEN               12
>>  static char pin_name[MAX_PIN_NAME_LEN];
>> +#define PINMUX_MODE_COUNT              5
>> +static const char * const pinmux_mode[PINMUX_MODE_COUNT] = {
>> +       "gpio input",
>> +       "gpio output",
>> +       "analog",
>> +       "unknown",
>> +       "alt function",
>> +};
>> +
>> +static int stm32_pinctrl_get_af(struct udevice *dev, unsigned int offset)
>> +{
>> +       struct stm32_gpio_priv *priv = dev_get_priv(dev);
>> +       struct stm32_gpio_regs *regs = priv->regs;
>> +       u32 af;
>> +       u32 alt_shift = (offset % 8) * 4;
>> +       u32 alt_index =  offset / 8;
>> +
>> +       af = (readl(&regs->afr[alt_index]) &
>> +             GENMASK(alt_shift + 3, alt_shift)) >> alt_shift;
>> +
>> +       return af;
>> +}
>> +
>>  static int stm32_pinctrl_get_pins_count(struct udevice *dev)
>>  {
>>         struct stm32_pinctrl_priv *priv = dev_get_priv(dev);
>> @@ -97,6 +123,56 @@ static const char *stm32_pinctrl_get_pin_name(struct udevice *dev,
>>
>>         return pin_name;
>>  }
>> +
>> +static const char *stm32_pinctrl_get_pin_muxing(struct udevice *dev,
>> +                                               unsigned int selector)
> 
> Again I think this should be passed a buffer to write into.

Ok

Thanks

Patrice

> 
> Regards,
> Simon
> 

[U-Boot] [PATCH v3 3/9] dm: uclass: Add uclass_foreach_dev_probe

[Patrice CHOTARD]

Hi Simon

On 10/19/18 5:27 AM, Simon Glass wrote:
> Hi Patrick,
> 
> On 15 October 2018 at 03:01, Patrice CHOTARD <patrice.chotard@st.com> wrote:
>> Hi Bin
>>
>> On 10/12/2018 12:13 PM, Bin Meng wrote:
>>> Hi Patrice,
>>>
>>> On Fri, Oct 12, 2018 at 3:51 PM Patrice CHOTARD <patrice.chotard@st.com> wrote:
>>>>
>>>> Hi Bin
>>>>
>>>> On 10/11/2018 11:06 AM, Bin Meng wrote:
>>>>> Hi Patrice,
>>>>>
>>>>> On Tue, Oct 9, 2018 at 9:41 PM Patrice Chotard <patrice.chotard@st.com> wrote:
>>>>>>
>>>>>> Add uclass_foreach_dev_probe() which iterates through
>>>>>> devices of a given uclass. Devices are probed if necessary
>>>>>> and are ready to use.
>>>>>>
>>>>>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>> ---
>>>>>>
>>>>>> Changes in v3: None
>>>>>> Changes in v2: None
>>>>>>
>>>>>>  include/dm/uclass.h | 16 ++++++++++++++++
>>>>>>  1 file changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
>>>>>> index 6e7c1cd3e8bc..10ccfdce951e 100644
>>>>>> --- a/include/dm/uclass.h
>>>>>> +++ b/include/dm/uclass.h
>>>>>> @@ -376,4 +376,20 @@ int uclass_resolve_seq(struct udevice *dev);
>>>>>>  #define uclass_foreach_dev_safe(pos, next, uc) \
>>>>>>         list_for_each_entry_safe(pos, next, &uc->dev_head, uclass_node)
>>>>>>
>>>>>> +/**
>>>>>> + * uclass_foreach_dev_probe() - Helper function to iteration through devices
>>>>>> + * of given uclass
>>>>>> + *
>>>>>> + * This creates a for() loop which works through the available devices in
>>>>>> + * a uclass in order from start to end. Devices are probed if necessary,
>>>>>> + * and ready for use.
>>>>>> + *
>>>>>> + * @id: Uclass ID
>>>>>> + * @dev: struct udevice * to hold the current device. Set to NULL when there
>>>>>> + * are no more devices.
>>>>>> + */
>>>>>> +#define uclass_foreach_dev_probe(id, dev)      \
>>>>>> +       for (uclass_first_device(id, &dev); dev; \
>>>>>> +            uclass_next_device(&dev))
>>>>>
>>>>> Shouldn't we check the return value of uclass_first_device() and
>>>>> uclass_next_device()?
>>>>
>>>> It's not necessary to check the return value of uclass_first_device(id,
>>>> &dev) because in any error case, dev is NULL, which is the loop output
>>>> condition. This is only for the first iteration.
>>>>
>>>
>>> Please see the comment of uclass_first_device().
>>>
>>>  * @devp: Returns pointer to the first device in that uclass if no error
>>>  * occurred, or NULL if there is no first device, or an error occurred with
>>>  * that device.
>>>  * @return 0 if OK (found or not found), other -ve on error
>>>
>>> So it can return error with a valid dev.
>>
>> I checked the uclass_first_device() implementation and the comment seems
>> not aligned with what is coded. When uclass_first_device() returns a
>> valid dev, return value is always 0. (see uclass_get_device_tail()
>> return value).
> 
> I actually don't like this function. Let's use
> uclass_first_device_err() when we care about errors. I am wondering if
> we should delete uclass_first_device().

Ok i will use uclass_first_device_err() instead

>>
>> In any case uclass_first_device() returns a valid dev with an error.
>>
>> Simon, Bin do you confirm ?
> 
> I don't see how, no.
>>
>>>
>>>> For the other iteration, dev comes from uclass_next_device(&dev),
>>>> similarly to uclass_first_device(), in any error case dev is NULL.
>>>>
>>>
>>> Similar please see the comment of uclass_next_device().
>>
>> I have the same remark regarding uclass_next_device().

I will add a new uclass_next_device_err()

Thanks

Patrice

> 
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 

[U-Boot] [PATCH v3 1/9] dm: pinctrl: Add get_pin_muxing() ops

[Simon Glass]

Hi Patrice,

On 23 October 2018 at 00:41, Patrice CHOTARD <patrice.chotard@st.com> wrote:
> Hi Simon
>
> On 10/19/18 5:28 AM, Simon Glass wrote:
>> Hi Patrice,
>>
>> On 9 October 2018 at 07:31, Patrice Chotard <patrice.chotard@st.com> wrote:
>>> Add get_pin_muxing() which allows to display the muxing
>>> of a given pin belonging to a pin-controller.
>>>
>>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>>> ---
>>>
>>> Changes in v3:
>>>  - Replace const char **buf parameter by char *buf, int size parameters
>>>    for pinctrl_get_pin_muxing()
>>>
>>> Changes in v2:
>>>  - Replace pinmux_show ops which displayed the complete pin-controller
>>>    muxing by get_pin_muxing ops which displays the muxing of one pin
>>>
>>>  drivers/pinctrl/pinctrl-uclass.c | 13 +++++++++++++
>>>  include/dm/pinctrl.h             | 32 ++++++++++++++++++++++++++++++++
>>>  2 files changed, 45 insertions(+)
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> But please see below.
>>
>>>
>>> diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
>>> index c38bb212ed74..cd2ca4e4950e 100644
>>> --- a/drivers/pinctrl/pinctrl-uclass.c
>>> +++ b/drivers/pinctrl/pinctrl-uclass.c
>>> @@ -249,6 +249,19 @@ int pinctrl_get_gpio_mux(struct udevice *dev, int banknum, int index)
>>>         return ops->get_gpio_mux(dev, banknum, index);
>>>  }
>>>
>>> +int pinctrl_get_pin_muxing(struct udevice *dev, int selector, char *buf,
>>> +                          int size)
>>> +{
>>> +       struct pinctrl_ops *ops = pinctrl_get_ops(dev);
>>> +
>>> +       if (!ops->get_pin_muxing)
>>> +               return -ENOSYS;
>>> +
>>> +       snprintf(buf, size, ops->get_pin_muxing(dev, selector));
>>
>> I wonder if it would be easy to return -ENOSPC if the string is too long?
>
> Not so easy, for example in pinctrl_stm32.c, depending of the pin's
> muxing mode, i not only copy but also, in some case, concatenate
> additional information. It would complexify the pinctrl code.

OK. Well snprintf() should return a useful value, but I'm not sure if
it tells you when the buffer is exhausted.

Regards,
Simon