Third round of the gpio-latch driver. The review comments I received
from v2 are integrated, for a changelog see the individual patches.
This time there's also [email protected] on Cc which I forgot
to add the last two rounds.
Sascha
Sascha Hauer (2):
gpio: Add gpio latch driver
dt-bindings: gpio: Add gpio-latch binding document
.../devicetree/bindings/gpio/gpio-latch.yaml | 85 ++++++++
drivers/gpio/Kconfig | 6 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-latch.c | 192 ++++++++++++++++++
4 files changed, 284 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-latch.yaml
create mode 100644 drivers/gpio/gpio-latch.c
--
2.30.2
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 <[email protected]>
---
Notes:
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 | 192 ++++++++++++++++++++++++++++++++++++++
3 files changed, 199 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..4134cba1c88a8
--- /dev/null
+++ b/drivers/gpio/gpio-latch.c
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GPIO latch driver
+ *
+ * Copyright (C) 2022 Sascha Hauer <[email protected]>
+ *
+ * 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 "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 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));
+
+ set(priv->clk_gpios->desc[latch], 1);
+ set(priv->clk_gpios->desc[latch], 0);
+}
+
+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;
+
+ 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);
+ }
+
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("GPIO latch driver");
--
2.30.2
This adds a binding for a GPIO multiplexer driver based on latches
connected to other GPIOs.
Signed-off-by: Sascha Hauer <[email protected]>
---
Notes:
Changes since v1:
- Add license to binding file
.../devicetree/bindings/gpio/gpio-latch.yaml | 85 +++++++++++++++++++
1 file changed, 85 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..856a9e1e43b45
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-latch.yaml
@@ -0,0 +1,85 @@
+# 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 <[email protected]>
+
+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
+
+ 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;
+
+ 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
On Wed, Sep 14, 2022 at 09:13:05AM +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 <[email protected]>
> ---
>
> Notes:
> 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 | 192 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 199 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..4134cba1c88a8
> --- /dev/null
> +++ b/drivers/gpio/gpio-latch.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * GPIO latch driver
> + *
> + * Copyright (C) 2022 Sascha Hauer <[email protected]>
> + *
> + * 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 "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 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));
-> duration?
> +
> + set(priv->clk_gpios->desc[latch], 1);
-> duration?
> + set(priv->clk_gpios->desc[latch], 0);
I am pretty much sure there must be some duration between the actions
above *. See for instance the tw and (tsu + th) timing requirements in
the next edge-triggered flip-flops:
https://www.ti.com/lit/ds/symlink/sn74lv74a.pdf?ts=1663163389954&ref_url=https%253A%252F%252Fwww.google.com%252F
The durations are normally small (ns or a bit smaller) but still need
to be added anyway.
Note since the durations are device-specific an additional DT-property array
with durations should be added too.
* already faced weird problems in this regard.
-Sergey
> +}
> +
> +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;
> +
> + 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);
> + }
> +
> + 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 <[email protected]>");
> +MODULE_DESCRIPTION("GPIO latch driver");
> --
> 2.30.2
>
On Wed, 14 Sep 2022 09:13:06 +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 <[email protected]>
> ---
>
> Notes:
> Changes since v1:
> - Add license to binding file
>
> .../devicetree/bindings/gpio/gpio-latch.yaml | 85 +++++++++++++++++++
> 1 file changed, 85 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-latch.yaml
>
Reviewed-by: Rob Herring <[email protected]>
On Wed, Sep 14, 2022 at 9:13 AM Sascha Hauer <[email protected]> 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 <[email protected]>
I see Serge has additional comments but apart from that this
looks acceptable to me:
Reviewed-by: Linus Walleij <[email protected]>
You may want to add set_multiple[_cansleep] later, but it
will survive without it.
Yours,
Linus Walleij
On Wed, Sep 14, 2022 at 9:13 AM Sascha Hauer <[email protected]> wrote:
> This adds a binding for a GPIO multiplexer driver based on latches
> connected to other GPIOs.
>
> Signed-off-by: Sascha Hauer <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, Sep 14, 2022 at 05:03:10PM +0300, Serge Semin wrote:
> > + 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));
>
> -> duration?
>
> > +
> > + set(priv->clk_gpios->desc[latch], 1);
>
> -> duration?
>
> > + set(priv->clk_gpios->desc[latch], 0);
>
> I am pretty much sure there must be some duration between the actions
> above *. See for instance the tw and (tsu + th) timing requirements in
> the next edge-triggered flip-flops:
> https://www.ti.com/lit/ds/symlink/sn74lv74a.pdf?ts=1663163389954&ref_url=https%253A%252F%252Fwww.google.com%252F
>
> The durations are normally small (ns or a bit smaller) but still need
> to be added anyway.
>
> Note since the durations are device-specific an additional DT-property array
> with durations should be added too.
Do you think a fixed udelay(1) would be enough for now? Bigger delays
shouldn't be needed and smaller delays expand to udelay(1) anyway on
architectures not providing an architecture specific ndelay().
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 |
To += @Rob, @Linus
On Thu, Sep 22, 2022 at 03:31:05PM +0200, Sascha Hauer wrote:
> On Wed, Sep 14, 2022 at 05:03:10PM +0300, Serge Semin wrote:
> > > + 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));
> >
> > -> duration?
> >
> > > +
> > > + set(priv->clk_gpios->desc[latch], 1);
> >
> > -> duration?
> >
> > > + set(priv->clk_gpios->desc[latch], 0);
> >
> > I am pretty much sure there must be some duration between the actions
> > above *. See for instance the tw and (tsu + th) timing requirements in
> > the next edge-triggered flip-flops:
> > https://www.ti.com/lit/ds/symlink/sn74lv74a.pdf?ts=1663163389954&ref_url=https%253A%252F%252Fwww.google.com%252F
> >
> > The durations are normally small (ns or a bit smaller) but still need
> > to be added anyway.
> >
> > Note since the durations are device-specific an additional DT-property array
> > with durations should be added too.
>
> Do you think a fixed udelay(1) would be enough for now? Bigger delays
> shouldn't be needed and smaller delays expand to udelay(1) anyway on
> architectures not providing an architecture specific ndelay().
I am sure you shouldn't assume what the particular architecture
provide and what it doesn't. When it comes to the GPIOs the switching
timings can have a critical value in a lot of applications (like i2c
bitbang, or real-time systems). There is no point in waiting for
micro seconds in the fast-path like this when there is only a few
nano seconds delay required.
I couldn't find any generic ready-to-use DT-property for this case.
So IMO instead the next properties would work:
1. "setup-duration-ns" - data input timing after which the clock input
can be asserted (Tsu in the hw-manual above).
2. "clock-duration-ns" - clock input timing for which the CLK signal
must be kept asserted so the device would perceive the input
states, the outputs would be updated and the clock signal could be
driven back to low (Tw including Th in the hw-manual above).
@Rob, @Linus, any suggestion regarding the properties and their naming?
-Serge
>
> 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 |
On Sun, Sep 25, 2022 at 5:30 PM Serge Semin <[email protected]> wrote:
> I couldn't find any generic ready-to-use DT-property for this case.
> So IMO instead the next properties would work:
> 1. "setup-duration-ns" - data input timing after which the clock input
> can be asserted (Tsu in the hw-manual above).
> 2. "clock-duration-ns" - clock input timing for which the CLK signal
> must be kept asserted so the device would perceive the input
> states, the outputs would be updated and the clock signal could be
> driven back to low (Tw including Th in the hw-manual above).
>
> @Rob, @Linus, any suggestion regarding the properties and their naming?
I think your suggestions look fine!
Yours,
Linus Walleij