[PATCH v4 0/2] gpio: Add gpio-latch driver

[PATCH v4 0/2] gpio: Add gpio-latch driver

[Sascha Hauer]

This round includes the changes suggested by Serge: Instead of hammering
the GPIOs as fast as we can we now have device tree properties to limit
the speed so that slower hardware can work with the driver.

Sascha

Sascha Hauer (2):
  gpio: Add gpio latch driver
  dt-bindings: gpio: Add gpio-latch binding document

 .../devicetree/bindings/gpio/gpio-latch.yaml  |  94 ++++++++
 drivers/gpio/Kconfig                          |   6 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-latch.c                     | 220 ++++++++++++++++++
 4 files changed, 321 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-latch.yaml
 create mode 100644 drivers/gpio/gpio-latch.c

-- 
2.30.2

[PATCH v4 1/2] gpio: Add gpio latch driver

[Sascha Hauer]

This driver implements a GPIO multiplexer based on latches connected to
other GPIOs. A set of data GPIOs is connected to the data input of
multiple latches. The clock input of each latch is driven by another
set of GPIOs. With two 8-bit latches 10 GPIOs can be multiplexed into
16 GPIOs. GPOs might be a better term as in fact the multiplexed pins
are output only.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

Notes:
    Changes since v3:
    - Introduce delays between GPIO toggles as suggested by Serge Semin
    
    Changes since v2:
    - Fix inconsistent licensing
    - document locks
    - use regular bit operations
    - include linux/mod_devicetable.h rather than linux/of_device.h
    - Put spinlock and mutex into a union to make clear that only one of them is used
    - rename __gpio_latch_set to gpio_latch_set_unlocked
    
    Changes since v1:
    - Use gpiod_set_value_cansleep when the underlying GPIOs might sleep
    - Move MODULE_DEVICE_TABLE near to the end

 drivers/gpio/Kconfig      |   6 ++
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/gpio-latch.c | 220 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 227 insertions(+)
 create mode 100644 drivers/gpio/gpio-latch.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0642f579196f2..e4603810ec910 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1690,6 +1690,12 @@ config GPIO_AGGREGATOR
 	      industrial control context, to be operated from userspace using
 	      the GPIO chardev interface.
 
+config GPIO_LATCH
+	tristate "GPIO latch driver"
+	help
+	  Say yes here to enable a driver for GPIO multiplexers based on latches
+	  connected to other GPIOs.
+
 config GPIO_MOCKUP
 	tristate "GPIO Testing Driver"
 	select IRQ_SIM
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a0985d30f51bb..310fa08decc69 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_GPIO_IT87)			+= gpio-it87.o
 obj-$(CONFIG_GPIO_IXP4XX)		+= gpio-ixp4xx.o
 obj-$(CONFIG_GPIO_JANZ_TTL)		+= gpio-janz-ttl.o
 obj-$(CONFIG_GPIO_KEMPLD)		+= gpio-kempld.o
+obj-$(CONFIG_GPIO_LATCH)		+= gpio-latch.o
 obj-$(CONFIG_GPIO_LOGICVC)		+= gpio-logicvc.o
 obj-$(CONFIG_GPIO_LOONGSON1)		+= gpio-loongson1.o
 obj-$(CONFIG_GPIO_LOONGSON)		+= gpio-loongson.o
diff --git a/drivers/gpio/gpio-latch.c b/drivers/gpio/gpio-latch.c
new file mode 100644
index 0000000000000..a4ab8f1240450
--- /dev/null
+++ b/drivers/gpio/gpio-latch.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GPIO latch driver
+ *
+ *  Copyright (C) 2022 Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * This driver implements a GPIO (or better GPO as there is no input)
+ * multiplexer based on latches like this:
+ *
+ * CLK0 ----------------------.        ,--------.
+ * CLK1 -------------------.  `--------|>    #0 |
+ *                         |           |        |
+ * OUT0 ----------------+--|-----------|D0    Q0|-----|<
+ * OUT1 --------------+-|--|-----------|D1    Q1|-----|<
+ * OUT2 ------------+-|-|--|-----------|D2    Q2|-----|<
+ * OUT3 ----------+-|-|-|--|-----------|D3    Q3|-----|<
+ * OUT4 --------+-|-|-|-|--|-----------|D4    Q4|-----|<
+ * OUT5 ------+-|-|-|-|-|--|-----------|D5    Q5|-----|<
+ * OUT6 ----+-|-|-|-|-|-|--|-----------|D6    Q6|-----|<
+ * OUT7 --+-|-|-|-|-|-|-|--|-----------|D7    Q7|-----|<
+ *        | | | | | | | |  |           `--------'
+ *        | | | | | | | |  |
+ *        | | | | | | | |  |           ,--------.
+ *        | | | | | | | |  `-----------|>    #1 |
+ *        | | | | | | | |              |        |
+ *        | | | | | | | `--------------|D0    Q0|-----|<
+ *        | | | | | | `----------------|D1    Q1|-----|<
+ *        | | | | | `------------------|D2    Q2|-----|<
+ *        | | | | `--------------------|D3    Q3|-----|<
+ *        | | | `----------------------|D4    Q4|-----|<
+ *        | | `------------------------|D5    Q5|-----|<
+ *        | `--------------------------|D6    Q6|-----|<
+ *        `----------------------------|D7    Q7|-----|<
+ *                                     `--------'
+ *
+ * The above is just an example. The actual number of number of latches and
+ * the number of inputs per latch is derived from the number of GPIOs given
+ * in the corresponding device tree properties.
+ */
+
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+
+#include "gpiolib.h"
+
+struct gpio_latch_priv {
+	struct gpio_chip gc;
+	struct gpio_descs *clk_gpios;
+	struct gpio_descs *latched_gpios;
+	int n_latched_gpios;
+	unsigned int setup_duration_ns;
+	unsigned int clock_duration_ns;
+	unsigned long *shadow;
+	/*
+	 * Depending on whether any of the underlying GPIOs may sleep we either
+	 * use a mutex or a spinlock to protect our shadow map.
+	 */
+	union {
+		struct mutex mutex; /* protects @shadow */
+		spinlock_t spinlock; /* protects @shadow */
+	};
+};
+
+static int gpio_latch_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static void gpio_latch_set_unlocked(struct gpio_latch_priv *priv,
+				    void (*set)(struct gpio_desc *desc, int value),
+				    unsigned int offset, bool val)
+{
+	int latch = offset / priv->n_latched_gpios;
+	int i;
+
+	assign_bit(offset, priv->shadow, val);
+
+	for (i = 0; i < priv->n_latched_gpios; i++)
+		set(priv->latched_gpios->desc[i],
+		    test_bit(latch * priv->n_latched_gpios + i, priv->shadow));
+
+	ndelay(priv->setup_duration_ns);
+	set(priv->clk_gpios->desc[latch], 1);
+	udelay(priv->clock_duration_ns);
+	set(priv->clk_gpios->desc[latch], 0);
+	udelay(priv->clock_duration_ns);
+}
+
+static void gpio_latch_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	struct gpio_latch_priv *priv = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->spinlock, flags);
+
+	gpio_latch_set_unlocked(priv, gpiod_set_value, offset, val);
+
+	spin_unlock_irqrestore(&priv->spinlock, flags);
+}
+
+static void gpio_latch_set_can_sleep(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	struct gpio_latch_priv *priv = gpiochip_get_data(gc);
+
+	mutex_lock(&priv->mutex);
+
+	gpio_latch_set_unlocked(priv, gpiod_set_value_cansleep, offset, val);
+
+	mutex_unlock(&priv->mutex);
+}
+
+static bool gpio_latch_can_sleep(struct gpio_latch_priv *priv, unsigned int n_latches)
+{
+	int i;
+
+	for (i = 0; i < n_latches; i++)
+		if (gpiod_cansleep(priv->clk_gpios->desc[i]))
+			return true;
+
+	for (i = 0; i < priv->n_latched_gpios; i++)
+		if (gpiod_cansleep(priv->latched_gpios->desc[i]))
+			return true;
+
+	return false;
+}
+
+static int gpio_latch_probe(struct platform_device *pdev)
+{
+	struct gpio_latch_priv *priv;
+	unsigned int n_latches;
+	struct device_node *np = pdev->dev.of_node;
+	/*
+	 * Some value which is still acceptable to delay in atomic context.
+	 * If we need to go higher we might have to switch to usleep_range(),
+	 * but that cannot ne used in atomic context and the driver would have
+	 * to be adjusted to support that.
+	 */
+	unsigned int duration_ns_max = 5000;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk_gpios = devm_gpiod_get_array(&pdev->dev, "clk", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->clk_gpios))
+		return PTR_ERR(priv->clk_gpios);
+
+	priv->latched_gpios = devm_gpiod_get_array(&pdev->dev, "latched", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->latched_gpios))
+		return PTR_ERR(priv->latched_gpios);
+
+	n_latches = priv->clk_gpios->ndescs;
+	priv->n_latched_gpios = priv->latched_gpios->ndescs;
+
+	priv->shadow = devm_bitmap_zalloc(&pdev->dev, n_latches * priv->n_latched_gpios,
+					  GFP_KERNEL);
+	if (!priv->shadow)
+		return -ENOMEM;
+
+	if (gpio_latch_can_sleep(priv, n_latches)) {
+		priv->gc.can_sleep = true;
+		priv->gc.set = gpio_latch_set_can_sleep;
+		mutex_init(&priv->mutex);
+	} else {
+		priv->gc.can_sleep = false;
+		priv->gc.set = gpio_latch_set;
+		spin_lock_init(&priv->spinlock);
+	}
+
+	of_property_read_u32(np, "setup-duration-ns", &priv->setup_duration_ns);
+	if (priv->setup_duration_ns > duration_ns_max) {
+		dev_warn(&pdev->dev, "setup-duration-ns too high, limit to %d\n",
+			 duration_ns_max);
+		priv->setup_duration_ns = duration_ns_max;
+	}
+
+	of_property_read_u32(np, "clock-duration-ns", &priv->clock_duration_ns);
+	if (priv->clock_duration_ns > duration_ns_max) {
+		dev_warn(&pdev->dev, "clock-duration-ns too high, limit to %d\n",
+			 duration_ns_max);
+		priv->clock_duration_ns = duration_ns_max;
+	}
+
+	priv->gc.get_direction = gpio_latch_get_direction;
+	priv->gc.ngpio = n_latches * priv->n_latched_gpios;
+	priv->gc.owner = THIS_MODULE;
+	priv->gc.base = -1;
+	priv->gc.parent = &pdev->dev;
+
+	platform_set_drvdata(pdev, priv);
+
+	return devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv);
+}
+
+static const struct of_device_id gpio_latch_ids[] = {
+	{
+		.compatible	= "gpio-latch",
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, gpio_latch_ids);
+
+static struct platform_driver gpio_latch_driver = {
+	.driver	= {
+		.name		= "gpio-latch",
+		.of_match_table	= gpio_latch_ids,
+	},
+	.probe	= gpio_latch_probe,
+};
+module_platform_driver(gpio_latch_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
+MODULE_DESCRIPTION("GPIO latch driver");
-- 
2.30.2

[PATCH v4 2/2] dt-bindings: gpio: Add gpio-latch binding document

[Sascha Hauer]

This adds a binding for a GPIO multiplexer driver based on latches
connected to other GPIOs.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

Notes:
    Changes since v3:
    - Introduce delays between GPIO toggles as suggested by Serge Semin
    
    Changes since v1:
    - Add license to binding file

 .../devicetree/bindings/gpio/gpio-latch.yaml  | 94 +++++++++++++++++++
 1 file changed, 94 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-latch.yaml

diff --git a/Documentation/devicetree/bindings/gpio/gpio-latch.yaml b/Documentation/devicetree/bindings/gpio/gpio-latch.yaml
new file mode 100644
index 0000000000000..1ed82a2cebdaa
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-latch.yaml
@@ -0,0 +1,94 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-latch.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO latch controller
+
+maintainers:
+  - Sascha Hauer <s.hauer@pengutronix.de>
+
+description: |
+  This binding describes a GPIO multiplexer based on latches connected to
+  other GPIOs, like this:
+
+  CLK0 ----------------------.        ,--------.
+  CLK1 -------------------.  `--------|>    #0 |
+                          |           |        |
+  OUT0 ----------------+--|-----------|D0    Q0|-----|<
+  OUT1 --------------+-|--|-----------|D1    Q1|-----|<
+  OUT2 ------------+-|-|--|-----------|D2    Q2|-----|<
+  OUT3 ----------+-|-|-|--|-----------|D3    Q3|-----|<
+  OUT4 --------+-|-|-|-|--|-----------|D4    Q4|-----|<
+  OUT5 ------+-|-|-|-|-|--|-----------|D5    Q5|-----|<
+  OUT6 ----+-|-|-|-|-|-|--|-----------|D6    Q6|-----|<
+  OUT7 --+-|-|-|-|-|-|-|--|-----------|D7    Q7|-----|<
+         | | | | | | | |  |           `--------'
+         | | | | | | | |  |
+         | | | | | | | |  |           ,--------.
+         | | | | | | | |  `-----------|>    #1 |
+         | | | | | | | |              |        |
+         | | | | | | | `--------------|D0    Q0|-----|<
+         | | | | | | `----------------|D1    Q1|-----|<
+         | | | | | `------------------|D2    Q2|-----|<
+         | | | | `--------------------|D3    Q3|-----|<
+         | | | `----------------------|D4    Q4|-----|<
+         | | `------------------------|D5    Q5|-----|<
+         | `--------------------------|D6    Q6|-----|<
+         `----------------------------|D7    Q7|-----|<
+                                      `--------'
+
+  The number of clk-gpios and latched-gpios is not fixed. The actual number
+  of number of latches and the number of inputs per latch is derived from
+  the number of GPIOs given in the corresponding device tree properties.
+
+properties:
+  compatible:
+    const: gpio-latch
+  "#gpio-cells":
+    const: 2
+
+  clk-gpios:
+    description: Array of GPIOs to be used to clock a latch
+
+  latched-gpios:
+    description: Array of GPIOs to be used as inputs per latch
+
+  setup-duration-ns:
+    description: Delay in nanoseconds to wait after the latch inputs have been
+      set up
+
+  clock-duration-ns:
+    description: Delay in nanoseconds to wait between clock output changes
+
+  gpio-controller: true
+
+  gpio-line-names: true
+
+required:
+  - compatible
+  - "#gpio-cells"
+  - gpio-controller
+  - clk-gpios
+  - latched-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio-latch {
+        #gpio-cells = <2>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_di_do_leds>;
+        compatible = "gpio-latch";
+        gpio-controller;
+        setup-duration-ns = <100>;
+        clock-duration-ns = <100>;
+
+        clk-gpios = <&gpio3 7 0>, <&gpio3 8 0>;
+        latched-gpios = <&gpio3 21 0>, <&gpio3 22 0>,
+                       <&gpio3 23 0>, <&gpio3 24 0>,
+                       <&gpio3 25 0>, <&gpio3 26 0>,
+                       <&gpio3 27 0>, <&gpio3 28 0>;
+    };
-- 
2.30.2

Re: [PATCH v4 1/2] gpio: Add gpio latch driver

[Serge Semin]

On Thu, Oct 06, 2022 at 10:30:30AM +0200, Sascha Hauer wrote:
> This driver implements a GPIO multiplexer based on latches connected to
> other GPIOs. A set of data GPIOs is connected to the data input of
> multiple latches. The clock input of each latch is driven by another
> set of GPIOs. With two 8-bit latches 10 GPIOs can be multiplexed into
> 16 GPIOs. GPOs might be a better term as in fact the multiplexed pins
> are output only.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> Notes:
>     Changes since v3:
>     - Introduce delays between GPIO toggles as suggested by Serge Semin
>     
>     Changes since v2:
>     - Fix inconsistent licensing
>     - document locks
>     - use regular bit operations
>     - include linux/mod_devicetable.h rather than linux/of_device.h
>     - Put spinlock and mutex into a union to make clear that only one of them is used
>     - rename __gpio_latch_set to gpio_latch_set_unlocked
>     
>     Changes since v1:
>     - Use gpiod_set_value_cansleep when the underlying GPIOs might sleep
>     - Move MODULE_DEVICE_TABLE near to the end
> 
>  drivers/gpio/Kconfig      |   6 ++
>  drivers/gpio/Makefile     |   1 +
>  drivers/gpio/gpio-latch.c | 220 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 227 insertions(+)
>  create mode 100644 drivers/gpio/gpio-latch.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 0642f579196f2..e4603810ec910 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1690,6 +1690,12 @@ config GPIO_AGGREGATOR
>  	      industrial control context, to be operated from userspace using
>  	      the GPIO chardev interface.
>  
> +config GPIO_LATCH
> +	tristate "GPIO latch driver"
> +	help
> +	  Say yes here to enable a driver for GPIO multiplexers based on latches
> +	  connected to other GPIOs.
> +
>  config GPIO_MOCKUP
>  	tristate "GPIO Testing Driver"
>  	select IRQ_SIM
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index a0985d30f51bb..310fa08decc69 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_GPIO_IT87)			+= gpio-it87.o
>  obj-$(CONFIG_GPIO_IXP4XX)		+= gpio-ixp4xx.o
>  obj-$(CONFIG_GPIO_JANZ_TTL)		+= gpio-janz-ttl.o
>  obj-$(CONFIG_GPIO_KEMPLD)		+= gpio-kempld.o
> +obj-$(CONFIG_GPIO_LATCH)		+= gpio-latch.o
>  obj-$(CONFIG_GPIO_LOGICVC)		+= gpio-logicvc.o
>  obj-$(CONFIG_GPIO_LOONGSON1)		+= gpio-loongson1.o
>  obj-$(CONFIG_GPIO_LOONGSON)		+= gpio-loongson.o
> diff --git a/drivers/gpio/gpio-latch.c b/drivers/gpio/gpio-latch.c
> new file mode 100644
> index 0000000000000..a4ab8f1240450
> --- /dev/null
> +++ b/drivers/gpio/gpio-latch.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * GPIO latch driver
> + *
> + *  Copyright (C) 2022 Sascha Hauer <s.hauer@pengutronix.de>
> + *
> + * This driver implements a GPIO (or better GPO as there is no input)
> + * multiplexer based on latches like this:
> + *
> + * CLK0 ----------------------.        ,--------.
> + * CLK1 -------------------.  `--------|>    #0 |
> + *                         |           |        |
> + * OUT0 ----------------+--|-----------|D0    Q0|-----|<
> + * OUT1 --------------+-|--|-----------|D1    Q1|-----|<
> + * OUT2 ------------+-|-|--|-----------|D2    Q2|-----|<
> + * OUT3 ----------+-|-|-|--|-----------|D3    Q3|-----|<
> + * OUT4 --------+-|-|-|-|--|-----------|D4    Q4|-----|<
> + * OUT5 ------+-|-|-|-|-|--|-----------|D5    Q5|-----|<
> + * OUT6 ----+-|-|-|-|-|-|--|-----------|D6    Q6|-----|<
> + * OUT7 --+-|-|-|-|-|-|-|--|-----------|D7    Q7|-----|<
> + *        | | | | | | | |  |           `--------'
> + *        | | | | | | | |  |
> + *        | | | | | | | |  |           ,--------.
> + *        | | | | | | | |  `-----------|>    #1 |
> + *        | | | | | | | |              |        |
> + *        | | | | | | | `--------------|D0    Q0|-----|<
> + *        | | | | | | `----------------|D1    Q1|-----|<
> + *        | | | | | `------------------|D2    Q2|-----|<
> + *        | | | | `--------------------|D3    Q3|-----|<
> + *        | | | `----------------------|D4    Q4|-----|<
> + *        | | `------------------------|D5    Q5|-----|<
> + *        | `--------------------------|D6    Q6|-----|<
> + *        `----------------------------|D7    Q7|-----|<
> + *                                     `--------'
> + *
> + * The above is just an example. The actual number of number of latches and
> + * the number of inputs per latch is derived from the number of GPIOs given
> + * in the corresponding device tree properties.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +
> +#include "gpiolib.h"
> +
> +struct gpio_latch_priv {
> +	struct gpio_chip gc;
> +	struct gpio_descs *clk_gpios;
> +	struct gpio_descs *latched_gpios;
> +	int n_latched_gpios;
> +	unsigned int setup_duration_ns;
> +	unsigned int clock_duration_ns;
> +	unsigned long *shadow;
> +	/*
> +	 * Depending on whether any of the underlying GPIOs may sleep we either
> +	 * use a mutex or a spinlock to protect our shadow map.
> +	 */
> +	union {
> +		struct mutex mutex; /* protects @shadow */
> +		spinlock_t spinlock; /* protects @shadow */
> +	};
> +};
> +
> +static int gpio_latch_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +	return GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static void gpio_latch_set_unlocked(struct gpio_latch_priv *priv,
> +				    void (*set)(struct gpio_desc *desc, int value),
> +				    unsigned int offset, bool val)
> +{
> +	int latch = offset / priv->n_latched_gpios;
> +	int i;
> +
> +	assign_bit(offset, priv->shadow, val);
> +
> +	for (i = 0; i < priv->n_latched_gpios; i++)
> +		set(priv->latched_gpios->desc[i],
> +		    test_bit(latch * priv->n_latched_gpios + i, priv->shadow));
> +
> +	ndelay(priv->setup_duration_ns);
> +	set(priv->clk_gpios->desc[latch], 1);

> +	udelay(priv->clock_duration_ns);

These are supposed to be !n!delay()'s. Aren't they?

> +	set(priv->clk_gpios->desc[latch], 0);

> +	udelay(priv->clock_duration_ns);

Why do you need the second clock-duration? AFAICS at least from the
TI SNx4LV74A specification the outputs get updated on the positive
edge of the pulse. So the clock-duration value shall contain both
the "CLK pulse duration" and "CLK hold time", which should be enough
for the latches states being perceived by the device.

> +}
> +
> +static void gpio_latch_set(struct gpio_chip *gc, unsigned int offset, int val)
> +{
> +	struct gpio_latch_priv *priv = gpiochip_get_data(gc);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->spinlock, flags);
> +
> +	gpio_latch_set_unlocked(priv, gpiod_set_value, offset, val);
> +
> +	spin_unlock_irqrestore(&priv->spinlock, flags);
> +}
> +
> +static void gpio_latch_set_can_sleep(struct gpio_chip *gc, unsigned int offset, int val)
> +{
> +	struct gpio_latch_priv *priv = gpiochip_get_data(gc);
> +
> +	mutex_lock(&priv->mutex);
> +
> +	gpio_latch_set_unlocked(priv, gpiod_set_value_cansleep, offset, val);
> +
> +	mutex_unlock(&priv->mutex);
> +}
> +
> +static bool gpio_latch_can_sleep(struct gpio_latch_priv *priv, unsigned int n_latches)
> +{
> +	int i;
> +
> +	for (i = 0; i < n_latches; i++)
> +		if (gpiod_cansleep(priv->clk_gpios->desc[i]))
> +			return true;
> +
> +	for (i = 0; i < priv->n_latched_gpios; i++)
> +		if (gpiod_cansleep(priv->latched_gpios->desc[i]))
> +			return true;
> +
> +	return false;
> +}
> +
> +static int gpio_latch_probe(struct platform_device *pdev)
> +{
> +	struct gpio_latch_priv *priv;
> +	unsigned int n_latches;
> +	struct device_node *np = pdev->dev.of_node;

> +	/*
> +	 * Some value which is still acceptable to delay in atomic context.
> +	 * If we need to go higher we might have to switch to usleep_range(),
> +	 * but that cannot ne used in atomic context and the driver would have
> +	 * to be adjusted to support that.
> +	 */
> +	unsigned int duration_ns_max = 5000;

I don't see you changing this parameter. So it's better to be a macro.

-Sergey

> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->clk_gpios = devm_gpiod_get_array(&pdev->dev, "clk", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->clk_gpios))
> +		return PTR_ERR(priv->clk_gpios);
> +
> +	priv->latched_gpios = devm_gpiod_get_array(&pdev->dev, "latched", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->latched_gpios))
> +		return PTR_ERR(priv->latched_gpios);
> +
> +	n_latches = priv->clk_gpios->ndescs;
> +	priv->n_latched_gpios = priv->latched_gpios->ndescs;
> +
> +	priv->shadow = devm_bitmap_zalloc(&pdev->dev, n_latches * priv->n_latched_gpios,
> +					  GFP_KERNEL);
> +	if (!priv->shadow)
> +		return -ENOMEM;
> +
> +	if (gpio_latch_can_sleep(priv, n_latches)) {
> +		priv->gc.can_sleep = true;
> +		priv->gc.set = gpio_latch_set_can_sleep;
> +		mutex_init(&priv->mutex);
> +	} else {
> +		priv->gc.can_sleep = false;
> +		priv->gc.set = gpio_latch_set;
> +		spin_lock_init(&priv->spinlock);
> +	}
> +
> +	of_property_read_u32(np, "setup-duration-ns", &priv->setup_duration_ns);
> +	if (priv->setup_duration_ns > duration_ns_max) {
> +		dev_warn(&pdev->dev, "setup-duration-ns too high, limit to %d\n",
> +			 duration_ns_max);
> +		priv->setup_duration_ns = duration_ns_max;
> +	}
> +
> +	of_property_read_u32(np, "clock-duration-ns", &priv->clock_duration_ns);
> +	if (priv->clock_duration_ns > duration_ns_max) {
> +		dev_warn(&pdev->dev, "clock-duration-ns too high, limit to %d\n",
> +			 duration_ns_max);
> +		priv->clock_duration_ns = duration_ns_max;
> +	}
> +
> +	priv->gc.get_direction = gpio_latch_get_direction;
> +	priv->gc.ngpio = n_latches * priv->n_latched_gpios;
> +	priv->gc.owner = THIS_MODULE;
> +	priv->gc.base = -1;
> +	priv->gc.parent = &pdev->dev;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv);
> +}
> +
> +static const struct of_device_id gpio_latch_ids[] = {
> +	{
> +		.compatible	= "gpio-latch",
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +MODULE_DEVICE_TABLE(of, gpio_latch_ids);
> +
> +static struct platform_driver gpio_latch_driver = {
> +	.driver	= {
> +		.name		= "gpio-latch",
> +		.of_match_table	= gpio_latch_ids,
> +	},
> +	.probe	= gpio_latch_probe,
> +};
> +module_platform_driver(gpio_latch_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
> +MODULE_DESCRIPTION("GPIO latch driver");
> -- 
> 2.30.2
> 

Re: [PATCH v4 1/2] gpio: Add gpio latch driver

[Sascha Hauer]

On Thu, Oct 06, 2022 at 12:15:06PM +0300, Serge Semin wrote:
> On Thu, Oct 06, 2022 at 10:30:30AM +0200, Sascha Hauer wrote:
> > This driver implements a GPIO multiplexer based on latches connected to
> > other GPIOs. A set of data GPIOs is connected to the data input of
> > multiple latches. The clock input of each latch is driven by another
> > set of GPIOs. With two 8-bit latches 10 GPIOs can be multiplexed into
> > 16 GPIOs. GPOs might be a better term as in fact the multiplexed pins
> > are output only.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> > 
> > Notes:
> >     Changes since v3:
> >     - Introduce delays between GPIO toggles as suggested by Serge Semin
> >     
> >     Changes since v2:
> >     - Fix inconsistent licensing
> >     - document locks
> >     - use regular bit operations
> >     - include linux/mod_devicetable.h rather than linux/of_device.h
> >     - Put spinlock and mutex into a union to make clear that only one of them is used
> >     - rename __gpio_latch_set to gpio_latch_set_unlocked
> >     
> >     Changes since v1:
> >     - Use gpiod_set_value_cansleep when the underlying GPIOs might sleep
> >     - Move MODULE_DEVICE_TABLE near to the end
> > 
> >  drivers/gpio/Kconfig      |   6 ++
> >  drivers/gpio/Makefile     |   1 +
> >  drivers/gpio/gpio-latch.c | 220 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 227 insertions(+)
> >  create mode 100644 drivers/gpio/gpio-latch.c
> > 
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 0642f579196f2..e4603810ec910 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -1690,6 +1690,12 @@ config GPIO_AGGREGATOR
> >  	      industrial control context, to be operated from userspace using
> >  	      the GPIO chardev interface.
> >  
> > +config GPIO_LATCH
> > +	tristate "GPIO latch driver"
> > +	help
> > +	  Say yes here to enable a driver for GPIO multiplexers based on latches
> > +	  connected to other GPIOs.
> > +
> >  config GPIO_MOCKUP
> >  	tristate "GPIO Testing Driver"
> >  	select IRQ_SIM
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index a0985d30f51bb..310fa08decc69 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -75,6 +75,7 @@ obj-$(CONFIG_GPIO_IT87)			+= gpio-it87.o
> >  obj-$(CONFIG_GPIO_IXP4XX)		+= gpio-ixp4xx.o
> >  obj-$(CONFIG_GPIO_JANZ_TTL)		+= gpio-janz-ttl.o
> >  obj-$(CONFIG_GPIO_KEMPLD)		+= gpio-kempld.o
> > +obj-$(CONFIG_GPIO_LATCH)		+= gpio-latch.o
> >  obj-$(CONFIG_GPIO_LOGICVC)		+= gpio-logicvc.o
> >  obj-$(CONFIG_GPIO_LOONGSON1)		+= gpio-loongson1.o
> >  obj-$(CONFIG_GPIO_LOONGSON)		+= gpio-loongson.o
> > diff --git a/drivers/gpio/gpio-latch.c b/drivers/gpio/gpio-latch.c
> > new file mode 100644
> > index 0000000000000..a4ab8f1240450
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-latch.c
> > @@ -0,0 +1,220 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * GPIO latch driver
> > + *
> > + *  Copyright (C) 2022 Sascha Hauer <s.hauer@pengutronix.de>
> > + *
> > + * This driver implements a GPIO (or better GPO as there is no input)
> > + * multiplexer based on latches like this:
> > + *
> > + * CLK0 ----------------------.        ,--------.
> > + * CLK1 -------------------.  `--------|>    #0 |
> > + *                         |           |        |
> > + * OUT0 ----------------+--|-----------|D0    Q0|-----|<
> > + * OUT1 --------------+-|--|-----------|D1    Q1|-----|<
> > + * OUT2 ------------+-|-|--|-----------|D2    Q2|-----|<
> > + * OUT3 ----------+-|-|-|--|-----------|D3    Q3|-----|<
> > + * OUT4 --------+-|-|-|-|--|-----------|D4    Q4|-----|<
> > + * OUT5 ------+-|-|-|-|-|--|-----------|D5    Q5|-----|<
> > + * OUT6 ----+-|-|-|-|-|-|--|-----------|D6    Q6|-----|<
> > + * OUT7 --+-|-|-|-|-|-|-|--|-----------|D7    Q7|-----|<
> > + *        | | | | | | | |  |           `--------'
> > + *        | | | | | | | |  |
> > + *        | | | | | | | |  |           ,--------.
> > + *        | | | | | | | |  `-----------|>    #1 |
> > + *        | | | | | | | |              |        |
> > + *        | | | | | | | `--------------|D0    Q0|-----|<
> > + *        | | | | | | `----------------|D1    Q1|-----|<
> > + *        | | | | | `------------------|D2    Q2|-----|<
> > + *        | | | | `--------------------|D3    Q3|-----|<
> > + *        | | | `----------------------|D4    Q4|-----|<
> > + *        | | `------------------------|D5    Q5|-----|<
> > + *        | `--------------------------|D6    Q6|-----|<
> > + *        `----------------------------|D7    Q7|-----|<
> > + *                                     `--------'
> > + *
> > + * The above is just an example. The actual number of number of latches and
> > + * the number of inputs per latch is derived from the number of GPIOs given
> > + * in the corresponding device tree properties.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/delay.h>
> > +
> > +#include "gpiolib.h"
> > +
> > +struct gpio_latch_priv {
> > +	struct gpio_chip gc;
> > +	struct gpio_descs *clk_gpios;
> > +	struct gpio_descs *latched_gpios;
> > +	int n_latched_gpios;
> > +	unsigned int setup_duration_ns;
> > +	unsigned int clock_duration_ns;
> > +	unsigned long *shadow;
> > +	/*
> > +	 * Depending on whether any of the underlying GPIOs may sleep we either
> > +	 * use a mutex or a spinlock to protect our shadow map.
> > +	 */
> > +	union {
> > +		struct mutex mutex; /* protects @shadow */
> > +		spinlock_t spinlock; /* protects @shadow */
> > +	};
> > +};
> > +
> > +static int gpio_latch_get_direction(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +	return GPIO_LINE_DIRECTION_OUT;
> > +}
> > +
> > +static void gpio_latch_set_unlocked(struct gpio_latch_priv *priv,
> > +				    void (*set)(struct gpio_desc *desc, int value),
> > +				    unsigned int offset, bool val)
> > +{
> > +	int latch = offset / priv->n_latched_gpios;
> > +	int i;
> > +
> > +	assign_bit(offset, priv->shadow, val);
> > +
> > +	for (i = 0; i < priv->n_latched_gpios; i++)
> > +		set(priv->latched_gpios->desc[i],
> > +		    test_bit(latch * priv->n_latched_gpios + i, priv->shadow));
> > +
> > +	ndelay(priv->setup_duration_ns);
> > +	set(priv->clk_gpios->desc[latch], 1);
> 
> > +	udelay(priv->clock_duration_ns);
> 
> These are supposed to be !n!delay()'s. Aren't they?

Yes, indeed.

> 
> > +	set(priv->clk_gpios->desc[latch], 0);
> 
> > +	udelay(priv->clock_duration_ns);
> 
> Why do you need the second clock-duration? AFAICS at least from the
> TI SNx4LV74A specification the outputs get updated on the positive
> edge of the pulse. So the clock-duration value shall contain both
> the "CLK pulse duration" and "CLK hold time", which should be enough
> for the latches states being perceived by the device.

The rationale was that there should be some delay between two subsequent
calls to gpio_latch_set_unlocked(). You are right though, on my latch
the update happens on the positive edge as well, so this shouldn't be
needed.

Sascha

> > +	/*
> > +	 * Some value which is still acceptable to delay in atomic context.
> > +	 * If we need to go higher we might have to switch to usleep_range(),
> > +	 * but that cannot ne used in atomic context and the driver would have
> > +	 * to be adjusted to support that.
> > +	 */
> > +	unsigned int duration_ns_max = 5000;
> 
> I don't see you changing this parameter. So it's better to be a macro.

Why should it?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v4 1/2] gpio: Add gpio latch driver

[Serge Semin]

On Thu, Oct 06, 2022 at 02:26:34PM +0200, Sascha Hauer wrote:
> On Thu, Oct 06, 2022 at 12:15:06PM +0300, Serge Semin wrote:
> > On Thu, Oct 06, 2022 at 10:30:30AM +0200, Sascha Hauer wrote:
> > > This driver implements a GPIO multiplexer based on latches connected to
> > > other GPIOs. A set of data GPIOs is connected to the data input of
> > > multiple latches. The clock input of each latch is driven by another
> > > set of GPIOs. With two 8-bit latches 10 GPIOs can be multiplexed into
> > > 16 GPIOs. GPOs might be a better term as in fact the multiplexed pins
> > > are output only.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > > 
> > > Notes:
> > >     Changes since v3:
> > >     - Introduce delays between GPIO toggles as suggested by Serge Semin
> > >     
> > >     Changes since v2:
> > >     - Fix inconsistent licensing
> > >     - document locks
> > >     - use regular bit operations
> > >     - include linux/mod_devicetable.h rather than linux/of_device.h
> > >     - Put spinlock and mutex into a union to make clear that only one of them is used
> > >     - rename __gpio_latch_set to gpio_latch_set_unlocked
> > >     
> > >     Changes since v1:
> > >     - Use gpiod_set_value_cansleep when the underlying GPIOs might sleep
> > >     - Move MODULE_DEVICE_TABLE near to the end
> > > 
> > >  drivers/gpio/Kconfig      |   6 ++
> > >  drivers/gpio/Makefile     |   1 +
> > >  drivers/gpio/gpio-latch.c | 220 ++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 227 insertions(+)
> > >  create mode 100644 drivers/gpio/gpio-latch.c
> > > 
> > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > > index 0642f579196f2..e4603810ec910 100644
> > > --- a/drivers/gpio/Kconfig
> > > +++ b/drivers/gpio/Kconfig
> > > @@ -1690,6 +1690,12 @@ config GPIO_AGGREGATOR
> > >  	      industrial control context, to be operated from userspace using
> > >  	      the GPIO chardev interface.
> > >  
> > > +config GPIO_LATCH
> > > +	tristate "GPIO latch driver"
> > > +	help
> > > +	  Say yes here to enable a driver for GPIO multiplexers based on latches
> > > +	  connected to other GPIOs.
> > > +
> > >  config GPIO_MOCKUP
> > >  	tristate "GPIO Testing Driver"
> > >  	select IRQ_SIM
> > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > > index a0985d30f51bb..310fa08decc69 100644
> > > --- a/drivers/gpio/Makefile
> > > +++ b/drivers/gpio/Makefile
> > > @@ -75,6 +75,7 @@ obj-$(CONFIG_GPIO_IT87)			+= gpio-it87.o
> > >  obj-$(CONFIG_GPIO_IXP4XX)		+= gpio-ixp4xx.o
> > >  obj-$(CONFIG_GPIO_JANZ_TTL)		+= gpio-janz-ttl.o
> > >  obj-$(CONFIG_GPIO_KEMPLD)		+= gpio-kempld.o
> > > +obj-$(CONFIG_GPIO_LATCH)		+= gpio-latch.o
> > >  obj-$(CONFIG_GPIO_LOGICVC)		+= gpio-logicvc.o
> > >  obj-$(CONFIG_GPIO_LOONGSON1)		+= gpio-loongson1.o
> > >  obj-$(CONFIG_GPIO_LOONGSON)		+= gpio-loongson.o
> > > diff --git a/drivers/gpio/gpio-latch.c b/drivers/gpio/gpio-latch.c
> > > new file mode 100644
> > > index 0000000000000..a4ab8f1240450
> > > --- /dev/null
> > > +++ b/drivers/gpio/gpio-latch.c
> > > @@ -0,0 +1,220 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * GPIO latch driver
> > > + *
> > > + *  Copyright (C) 2022 Sascha Hauer <s.hauer@pengutronix.de>
> > > + *
> > > + * This driver implements a GPIO (or better GPO as there is no input)
> > > + * multiplexer based on latches like this:
> > > + *
> > > + * CLK0 ----------------------.        ,--------.
> > > + * CLK1 -------------------.  `--------|>    #0 |
> > > + *                         |           |        |
> > > + * OUT0 ----------------+--|-----------|D0    Q0|-----|<
> > > + * OUT1 --------------+-|--|-----------|D1    Q1|-----|<
> > > + * OUT2 ------------+-|-|--|-----------|D2    Q2|-----|<
> > > + * OUT3 ----------+-|-|-|--|-----------|D3    Q3|-----|<
> > > + * OUT4 --------+-|-|-|-|--|-----------|D4    Q4|-----|<
> > > + * OUT5 ------+-|-|-|-|-|--|-----------|D5    Q5|-----|<
> > > + * OUT6 ----+-|-|-|-|-|-|--|-----------|D6    Q6|-----|<
> > > + * OUT7 --+-|-|-|-|-|-|-|--|-----------|D7    Q7|-----|<
> > > + *        | | | | | | | |  |           `--------'
> > > + *        | | | | | | | |  |
> > > + *        | | | | | | | |  |           ,--------.
> > > + *        | | | | | | | |  `-----------|>    #1 |
> > > + *        | | | | | | | |              |        |
> > > + *        | | | | | | | `--------------|D0    Q0|-----|<
> > > + *        | | | | | | `----------------|D1    Q1|-----|<
> > > + *        | | | | | `------------------|D2    Q2|-----|<
> > > + *        | | | | `--------------------|D3    Q3|-----|<
> > > + *        | | | `----------------------|D4    Q4|-----|<
> > > + *        | | `------------------------|D5    Q5|-----|<
> > > + *        | `--------------------------|D6    Q6|-----|<
> > > + *        `----------------------------|D7    Q7|-----|<
> > > + *                                     `--------'
> > > + *
> > > + * The above is just an example. The actual number of number of latches and
> > > + * the number of inputs per latch is derived from the number of GPIOs given
> > > + * in the corresponding device tree properties.
> > > + */
> > > +
> > > +#include <linux/err.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/gpio/driver.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/delay.h>
> > > +
> > > +#include "gpiolib.h"
> > > +
> > > +struct gpio_latch_priv {
> > > +	struct gpio_chip gc;
> > > +	struct gpio_descs *clk_gpios;
> > > +	struct gpio_descs *latched_gpios;
> > > +	int n_latched_gpios;
> > > +	unsigned int setup_duration_ns;
> > > +	unsigned int clock_duration_ns;
> > > +	unsigned long *shadow;
> > > +	/*
> > > +	 * Depending on whether any of the underlying GPIOs may sleep we either
> > > +	 * use a mutex or a spinlock to protect our shadow map.
> > > +	 */
> > > +	union {
> > > +		struct mutex mutex; /* protects @shadow */
> > > +		spinlock_t spinlock; /* protects @shadow */
> > > +	};
> > > +};
> > > +
> > > +static int gpio_latch_get_direction(struct gpio_chip *gc, unsigned int offset)
> > > +{
> > > +	return GPIO_LINE_DIRECTION_OUT;
> > > +}
> > > +
> > > +static void gpio_latch_set_unlocked(struct gpio_latch_priv *priv,
> > > +				    void (*set)(struct gpio_desc *desc, int value),
> > > +				    unsigned int offset, bool val)
> > > +{
> > > +	int latch = offset / priv->n_latched_gpios;
> > > +	int i;
> > > +
> > > +	assign_bit(offset, priv->shadow, val);
> > > +
> > > +	for (i = 0; i < priv->n_latched_gpios; i++)
> > > +		set(priv->latched_gpios->desc[i],
> > > +		    test_bit(latch * priv->n_latched_gpios + i, priv->shadow));
> > > +
> > > +	ndelay(priv->setup_duration_ns);
> > > +	set(priv->clk_gpios->desc[latch], 1);
> > 
> > > +	udelay(priv->clock_duration_ns);
> > 
> > These are supposed to be !n!delay()'s. Aren't they?
> 
> Yes, indeed.
> 
> > 
> > > +	set(priv->clk_gpios->desc[latch], 0);
> > 
> > > +	udelay(priv->clock_duration_ns);
> > 
> > Why do you need the second clock-duration? AFAICS at least from the
> > TI SNx4LV74A specification the outputs get updated on the positive
> > edge of the pulse. So the clock-duration value shall contain both
> > the "CLK pulse duration" and "CLK hold time", which should be enough
> > for the latches states being perceived by the device.
> 
> The rationale was that there should be some delay between two subsequent
> calls to gpio_latch_set_unlocked(). You are right though, on my latch
> the update happens on the positive edge as well, so this shouldn't be
> needed.
> 
> Sascha
> 
> > > +	/*
> > > +	 * Some value which is still acceptable to delay in atomic context.
> > > +	 * If we need to go higher we might have to switch to usleep_range(),
> > > +	 * but that cannot ne used in atomic context and the driver would have
> > > +	 * to be adjusted to support that.
> > > +	 */
> > > +	unsigned int duration_ns_max = 5000;
> > 

> > I don't see you changing this parameter. So it's better to be a macro.
> 
> Why should it?

Well because of its semantics. It's a constant and is definitely a
common parameter for any device. So const-macro-based parametrization
like

/*
...
 */
#define GPIO_LATCH_DURATION_NS_MAX		5000

placed above the struct gpio_latch_priv declaration with the same
comment would be more appropriate than a pre-initialized local
variable. Thus the capitalized name could be right away read that the
parameter is a constant and common for all the devices. In your
implementation it isn't obvious from the variable usage context. I had
to parse the whole gpio_latch_probe() method to understand that.

-Sergey

> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v4 2/2] dt-bindings: gpio: Add gpio-latch binding document

[Rob Herring]

On Thu, 06 Oct 2022 10:30:31 +0200, Sascha Hauer wrote:
> This adds a binding for a GPIO multiplexer driver based on latches
> connected to other GPIOs.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> Notes:
>     Changes since v3:
>     - Introduce delays between GPIO toggles as suggested by Serge Semin
> 
>     Changes since v1:
>     - Add license to binding file
> 
>  .../devicetree/bindings/gpio/gpio-latch.yaml  | 94 +++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-latch.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>