* [PATCH v2 0/2] gpio: Add gpio-latch driver
@ 2022-08-31 5:58 Sascha Hauer
2022-08-31 5:58 ` [PATCH v2 1/2] gpio: Add gpio latch driver Sascha Hauer
2022-08-31 5:58 ` [PATCH v2 2/2] dt-bindings: gpio: Add gpio-latch binding document Sascha Hauer
0 siblings, 2 replies; 12+ messages in thread
From: Sascha Hauer @ 2022-08-31 5:58 UTC (permalink / raw)
To: linux-gpio
Cc: linux-kernel, Geert Uytterhoeven, Linus Walleij,
Bartosz Golaszewski, kernel, Sascha Hauer
This adds a gpio-driver which multiplexes existing GPIOs using latches.
Uwe asked [1] if that would be accectable as a new gpio driver, and here
is the result. For a better description what this is all about have a
look at the drawings in the patches.
Sascha
[1] https://lore.kernel.org/all/CACRpkdaBO=JzokGUF6uXZc7ASVD7LjqBxTLGwX-FShM=A9gw9A@mail.gmail.com/t/
Changes since v1:
- Use gpiod_set_value_cansleep when the underlying GPIOs might sleep
- Move MODULE_DEVICE_TABLE near to the end
- Add license to binding file
- remove trailing whitespaces
Sascha Hauer (2):
gpio: Add gpio latch driver
dt-bindings: gpio: Add gpio-latch binding document
.../devicetree/bindings/gpio/gpio-latch.yaml | 84 ++++++++
drivers/gpio/Kconfig | 6 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-latch.c | 190 ++++++++++++++++++
4 files changed, 281 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-latch.yaml
create mode 100644 drivers/gpio/gpio-latch.c
--
2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] gpio: Add gpio latch driver
2022-08-31 5:58 [PATCH v2 0/2] gpio: Add gpio-latch driver Sascha Hauer
@ 2022-08-31 5:58 ` Sascha Hauer
2022-08-31 8:01 ` Marco Felsch
` (4 more replies)
2022-08-31 5:58 ` [PATCH v2 2/2] dt-bindings: gpio: Add gpio-latch binding document Sascha Hauer
1 sibling, 5 replies; 12+ messages in thread
From: Sascha Hauer @ 2022-08-31 5:58 UTC (permalink / raw)
To: linux-gpio
Cc: linux-kernel, Geert Uytterhoeven, Linus Walleij,
Bartosz Golaszewski, kernel, 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>
---
drivers/gpio/Kconfig | 6 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-latch.c | 190 ++++++++++++++++++++++++++++++++++++++
3 files changed, 197 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..4068ed4066272
--- /dev/null
+++ b/drivers/gpio/gpio-latch.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * 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/module.h>
+#include <linux/of_device.h>
+#include <linux/gpio/driver.h>
+#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+
+#include "gpiolib.h"
+
+struct gpio_latch_priv {
+ struct gpio_chip gc;
+ struct gpio_descs *clk_gpios;
+ struct gpio_descs *data_gpios;
+ spinlock_t lock;
+ int n_ports;
+ int n_pins;
+ unsigned int *shadow;
+ struct mutex mutex;
+ spinlock_t spinlock;
+};
+
+static int gpio_latch_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+ return GPIO_LINE_DIRECTION_OUT;
+}
+
+static void __gpio_latch_set(struct gpio_latch_priv *priv,
+ void (* set)(struct gpio_desc *desc, int value),
+ unsigned int offset, int val)
+{
+ int latch = offset / priv->n_pins;
+ int i;
+
+ if (val)
+ priv->shadow[latch] |= BIT(offset % priv->n_pins);
+ else
+ priv->shadow[latch] &= ~BIT(offset % priv->n_pins);
+
+ for (i = 0; i < priv->n_pins; i++)
+ set(priv->data_gpios->desc[i], priv->shadow[latch] & BIT(i));
+
+ 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(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(priv, gpiod_set_value_cansleep, offset, val);
+
+ mutex_unlock(&priv->mutex);
+}
+
+static bool gpio_latch_can_sleep(struct gpio_latch_priv *priv)
+{
+ int i;
+
+ for (i = 0; i < priv->n_ports; i++)
+ if (gpiod_cansleep(priv->clk_gpios->desc[i]))
+ return true;
+
+ for (i = 0; i < priv->n_pins; i++)
+ if (gpiod_cansleep(priv->data_gpios->desc[i]))
+ return true;
+
+ return false;
+}
+
+static int gpio_latch_probe(struct platform_device *pdev)
+{
+ struct gpio_latch_priv *priv;
+
+ 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->data_gpios = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_OUT_LOW);
+ if (IS_ERR(priv->data_gpios))
+ return PTR_ERR(priv->data_gpios);
+
+ priv->n_ports = priv->clk_gpios->ndescs;
+ priv->n_pins = priv->data_gpios->ndescs;
+
+ priv->shadow = devm_kcalloc(&pdev->dev, priv->n_ports, sizeof(*priv->shadow),
+ GFP_KERNEL);
+ if (!priv->shadow)
+ return -ENOMEM;
+
+ if (gpio_latch_can_sleep(priv)) {
+ 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 = priv->n_ports * priv->n_pins;
+ priv->gc.owner = THIS_MODULE;
+ priv->gc.base = -1;
+ priv->gc.parent = &pdev->dev;
+ priv->gc.of_node = pdev->dev.of_node;
+
+ 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] dt-bindings: gpio: Add gpio-latch binding document
2022-08-31 5:58 [PATCH v2 0/2] gpio: Add gpio-latch driver Sascha Hauer
2022-08-31 5:58 ` [PATCH v2 1/2] gpio: Add gpio latch driver Sascha Hauer
@ 2022-08-31 5:58 ` Sascha Hauer
1 sibling, 0 replies; 12+ messages in thread
From: Sascha Hauer @ 2022-08-31 5:58 UTC (permalink / raw)
To: linux-gpio
Cc: linux-kernel, Geert Uytterhoeven, Linus Walleij,
Bartosz Golaszewski, kernel, 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>
---
.../devicetree/bindings/gpio/gpio-latch.yaml | 84 +++++++++++++++++++
1 file changed, 84 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..7aa513c10df1a
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-latch.yaml
@@ -0,0 +1,84 @@
+# 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 data-gpios is not fixed. The actual number of
+ GPIOs used for clk and data are taken from the corresponding array lengths.
+
+properties:
+ compatible:
+ const: gpio-latch
+ "#gpio-cells":
+ const: 2
+
+ clk-gpios:
+ description: Array of GPIOs to be used to clock a latch
+
+ data-gpios:
+ description: Array of GPIOs to be used as data GPIOs
+
+ gpio-controller: true
+
+ gpio-line-names: true
+
+required:
+ - compatible
+ - "#gpio-cells"
+ - gpio-controller
+ - clk-gpios
+ - data-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>;
+ data-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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] gpio: Add gpio latch driver
2022-08-31 5:58 ` [PATCH v2 1/2] gpio: Add gpio latch driver Sascha Hauer
@ 2022-08-31 8:01 ` Marco Felsch
2022-08-31 9:07 ` Sascha Hauer
2022-08-31 11:50 ` Bartosz Golaszewski
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Marco Felsch @ 2022-08-31 8:01 UTC (permalink / raw)
To: Sascha Hauer
Cc: linux-gpio, linux-kernel, Geert Uytterhoeven, kernel,
Bartosz Golaszewski, Linus Walleij
Hi Sascha,
On 22-08-31, 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>
apart the one minor nit (see end) feel free to add my
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> drivers/gpio/Kconfig | 6 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-latch.c | 190 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 197 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..4068ed4066272
> --- /dev/null
> +++ b/drivers/gpio/gpio-latch.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * 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/module.h>
> +#include <linux/of_device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> +
> +#include "gpiolib.h"
> +
> +struct gpio_latch_priv {
> + struct gpio_chip gc;
> + struct gpio_descs *clk_gpios;
> + struct gpio_descs *data_gpios;
> + spinlock_t lock;
> + int n_ports;
> + int n_pins;
> + unsigned int *shadow;
> + struct mutex mutex;
> + spinlock_t spinlock;
> +};
> +
> +static int gpio_latch_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + return GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static void __gpio_latch_set(struct gpio_latch_priv *priv,
> + void (* set)(struct gpio_desc *desc, int value),
> + unsigned int offset, int val)
> +{
> + int latch = offset / priv->n_pins;
> + int i;
> +
> + if (val)
> + priv->shadow[latch] |= BIT(offset % priv->n_pins);
> + else
> + priv->shadow[latch] &= ~BIT(offset % priv->n_pins);
> +
> + for (i = 0; i < priv->n_pins; i++)
> + set(priv->data_gpios->desc[i], priv->shadow[latch] & BIT(i));
> +
> + 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(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(priv, gpiod_set_value_cansleep, offset, val);
> +
> + mutex_unlock(&priv->mutex);
> +}
> +
> +static bool gpio_latch_can_sleep(struct gpio_latch_priv *priv)
> +{
> + int i;
> +
> + for (i = 0; i < priv->n_ports; i++)
> + if (gpiod_cansleep(priv->clk_gpios->desc[i]))
> + return true;
> +
> + for (i = 0; i < priv->n_pins; i++)
> + if (gpiod_cansleep(priv->data_gpios->desc[i]))
> + return true;
> +
> + return false;
> +}
> +
> +static int gpio_latch_probe(struct platform_device *pdev)
> +{
> + struct gpio_latch_priv *priv;
> +
> + 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->data_gpios = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_OUT_LOW);
> + if (IS_ERR(priv->data_gpios))
> + return PTR_ERR(priv->data_gpios);
> +
> + priv->n_ports = priv->clk_gpios->ndescs;
> + priv->n_pins = priv->data_gpios->ndescs;
> +
> + priv->shadow = devm_kcalloc(&pdev->dev, priv->n_ports, sizeof(*priv->shadow),
> + GFP_KERNEL);
> + if (!priv->shadow)
> + return -ENOMEM;
> +
> + if (gpio_latch_can_sleep(priv)) {
> + 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 = priv->n_ports * priv->n_pins;
> + priv->gc.owner = THIS_MODULE;
> + priv->gc.base = -1;
> + priv->gc.parent = &pdev->dev;
> + priv->gc.of_node = pdev->dev.of_node;
> +
> + 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");
checkpatch.pl should complain about this since it it now GPL, see commit
bf7fbeeae6db. Sorry for not spotted that earlier.
Regards,
Marco
> +MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
> +MODULE_DESCRIPTION("GPIO latch driver");
> --
> 2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] gpio: Add gpio latch driver
2022-08-31 8:01 ` Marco Felsch
@ 2022-08-31 9:07 ` Sascha Hauer
0 siblings, 0 replies; 12+ messages in thread
From: Sascha Hauer @ 2022-08-31 9:07 UTC (permalink / raw)
To: Marco Felsch
Cc: linux-gpio, linux-kernel, Geert Uytterhoeven, kernel,
Bartosz Golaszewski, Linus Walleij
On Wed, Aug 31, 2022 at 10:01:09AM +0200, Marco Felsch wrote:
> Hi Sascha,
>
> On 22-08-31, 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>
>
> apart the one minor nit (see end) feel free to add my
>
> Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
>
> > +// SPDX-License-Identifier: GPL-2.0-or-later
[snip]
> > +MODULE_LICENSE("GPL v2");
>
> checkpatch.pl should complain about this since it it now GPL, see commit
> bf7fbeeae6db. Sorry for not spotted that earlier.
Just checked, checkpatch doesn't complain. Will fix in v3
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 |
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] gpio: Add gpio latch driver
2022-08-31 5:58 ` [PATCH v2 1/2] gpio: Add gpio latch driver Sascha Hauer
2022-08-31 8:01 ` Marco Felsch
@ 2022-08-31 11:50 ` Bartosz Golaszewski
2022-08-31 20:50 ` Andy Shevchenko
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Bartosz Golaszewski @ 2022-08-31 11:50 UTC (permalink / raw)
To: Sascha Hauer
Cc: open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Geert Uytterhoeven, Linus Walleij, Sascha Hauer
On Wed, Aug 31, 2022 at 7:58 AM Sascha Hauer <s.hauer@pengutronix.de> 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>
> ---
> drivers/gpio/Kconfig | 6 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-latch.c | 190 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 197 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..4068ed4066272
> --- /dev/null
> +++ b/drivers/gpio/gpio-latch.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * 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/module.h>
> +#include <linux/of_device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> +
> +#include "gpiolib.h"
> +
> +struct gpio_latch_priv {
> + struct gpio_chip gc;
> + struct gpio_descs *clk_gpios;
> + struct gpio_descs *data_gpios;
> + spinlock_t lock;
> + int n_ports;
> + int n_pins;
> + unsigned int *shadow;
> + struct mutex mutex;
> + spinlock_t spinlock;
> +};
> +
> +static int gpio_latch_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + return GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static void __gpio_latch_set(struct gpio_latch_priv *priv,
I would love to see that renamed to gpio_latch_set_unlocked(). Other
than that, looks good to me.
Bart
> + void (* set)(struct gpio_desc *desc, int value),
> + unsigned int offset, int val)
> +{
> + int latch = offset / priv->n_pins;
> + int i;
> +
> + if (val)
> + priv->shadow[latch] |= BIT(offset % priv->n_pins);
> + else
> + priv->shadow[latch] &= ~BIT(offset % priv->n_pins);
> +
> + for (i = 0; i < priv->n_pins; i++)
> + set(priv->data_gpios->desc[i], priv->shadow[latch] & BIT(i));
> +
> + 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(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(priv, gpiod_set_value_cansleep, offset, val);
> +
> + mutex_unlock(&priv->mutex);
> +}
> +
> +static bool gpio_latch_can_sleep(struct gpio_latch_priv *priv)
> +{
> + int i;
> +
> + for (i = 0; i < priv->n_ports; i++)
> + if (gpiod_cansleep(priv->clk_gpios->desc[i]))
> + return true;
> +
> + for (i = 0; i < priv->n_pins; i++)
> + if (gpiod_cansleep(priv->data_gpios->desc[i]))
> + return true;
> +
> + return false;
> +}
> +
> +static int gpio_latch_probe(struct platform_device *pdev)
> +{
> + struct gpio_latch_priv *priv;
> +
> + 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->data_gpios = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_OUT_LOW);
> + if (IS_ERR(priv->data_gpios))
> + return PTR_ERR(priv->data_gpios);
> +
> + priv->n_ports = priv->clk_gpios->ndescs;
> + priv->n_pins = priv->data_gpios->ndescs;
> +
> + priv->shadow = devm_kcalloc(&pdev->dev, priv->n_ports, sizeof(*priv->shadow),
> + GFP_KERNEL);
> + if (!priv->shadow)
> + return -ENOMEM;
> +
> + if (gpio_latch_can_sleep(priv)) {
> + 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 = priv->n_ports * priv->n_pins;
> + priv->gc.owner = THIS_MODULE;
> + priv->gc.base = -1;
> + priv->gc.parent = &pdev->dev;
> + priv->gc.of_node = pdev->dev.of_node;
> +
> + 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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] gpio: Add gpio latch driver
2022-08-31 5:58 ` [PATCH v2 1/2] gpio: Add gpio latch driver Sascha Hauer
2022-08-31 8:01 ` Marco Felsch
2022-08-31 11:50 ` Bartosz Golaszewski
@ 2022-08-31 20:50 ` Andy Shevchenko
2022-09-01 8:20 ` Sascha Hauer
2022-08-31 20:52 ` kernel test robot
2022-09-02 6:42 ` Andy Shevchenko
4 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2022-08-31 20:50 UTC (permalink / raw)
To: Sascha Hauer
Cc: open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
Sascha Hauer
On Wed, Aug 31, 2022 at 9:02 AM Sascha Hauer <s.hauer@pengutronix.de> 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.
I'm still unsure it shouldn't be a part of the (not yet in upstream)
driver that I have mentioned before. But let's leave this apart right
now.
...
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
Why?
It seems you misplaced it instead of mod_devicetable.h.
> +#include <linux/gpio/driver.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
Keep above sorted?
...
> + struct mutex mutex;
> + spinlock_t spinlock;
Checkpatch usually complains if locks are not commented. Looking at
the below code, why it's not an (anonymous) union?
...
> + if (val)
> + priv->shadow[latch] |= BIT(offset % priv->n_pins);
> + else
> + priv->shadow[latch] &= ~BIT(offset % priv->n_pins);
I believe shadow should be defined as unsigned long * and hence normal
bit operations can be applied. For example here is assign_bit().
...
> + priv->shadow = devm_kcalloc(&pdev->dev, priv->n_ports, sizeof(*priv->shadow),
> + GFP_KERNEL);
bitmap_zalloc()
> + if (!priv->shadow)
> + return -ENOMEM;
...
> + priv->gc.parent = &pdev->dev;
> + priv->gc.of_node = pdev->dev.of_node;
Redundant as repeating parent above.
...
> +static const struct of_device_id gpio_latch_ids[] = {
> + {
> + .compatible = "gpio-latch",
> + }, {
> + /* sentinel */
> + }
You may compress this to the 2 LoCs.
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] gpio: Add gpio latch driver
2022-08-31 5:58 ` [PATCH v2 1/2] gpio: Add gpio latch driver Sascha Hauer
` (2 preceding siblings ...)
2022-08-31 20:50 ` Andy Shevchenko
@ 2022-08-31 20:52 ` kernel test robot
2022-09-02 6:42 ` Andy Shevchenko
4 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2022-08-31 20:52 UTC (permalink / raw)
To: Sascha Hauer, linux-gpio
Cc: kbuild-all, linux-kernel, Geert Uytterhoeven, Linus Walleij,
Bartosz Golaszewski, kernel, Sascha Hauer
Hi Sascha,
I love your patch! Yet something to improve:
[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on linus/master v6.0-rc3 next-20220830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sascha-Hauer/gpio-Add-gpio-latch-driver/20220831-135855
base: https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
config: s390-randconfig-r021-20220831 (https://download.01.org/0day-ci/archive/20220901/202209010432.NKyeVosI-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/8fab8b8c35fd4faebb003751d6d34fc06093c19e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Sascha-Hauer/gpio-Add-gpio-latch-driver/20220831-135855
git checkout 8fab8b8c35fd4faebb003751d6d34fc06093c19e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/gpio/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/gpio/gpio-latch.c: In function 'gpio_latch_probe':
>> drivers/gpio/gpio-latch.c:163:18: error: 'struct gpio_chip' has no member named 'of_node'; did you mean 'fwnode'?
163 | priv->gc.of_node = pdev->dev.of_node;
| ^~~~~~~
| fwnode
vim +163 drivers/gpio/gpio-latch.c
123
124 static int gpio_latch_probe(struct platform_device *pdev)
125 {
126 struct gpio_latch_priv *priv;
127
128 priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
129 if (!priv)
130 return -ENOMEM;
131
132 priv->clk_gpios = devm_gpiod_get_array(&pdev->dev, "clk", GPIOD_OUT_LOW);
133 if (IS_ERR(priv->clk_gpios))
134 return PTR_ERR(priv->clk_gpios);
135
136 priv->data_gpios = devm_gpiod_get_array(&pdev->dev, "data", GPIOD_OUT_LOW);
137 if (IS_ERR(priv->data_gpios))
138 return PTR_ERR(priv->data_gpios);
139
140 priv->n_ports = priv->clk_gpios->ndescs;
141 priv->n_pins = priv->data_gpios->ndescs;
142
143 priv->shadow = devm_kcalloc(&pdev->dev, priv->n_ports, sizeof(*priv->shadow),
144 GFP_KERNEL);
145 if (!priv->shadow)
146 return -ENOMEM;
147
148 if (gpio_latch_can_sleep(priv)) {
149 priv->gc.can_sleep = true;
150 priv->gc.set = gpio_latch_set_can_sleep;
151 mutex_init(&priv->mutex);
152 } else {
153 priv->gc.can_sleep = false;
154 priv->gc.set = gpio_latch_set;
155 spin_lock_init(&priv->spinlock);
156 }
157
158 priv->gc.get_direction = gpio_latch_get_direction;
159 priv->gc.ngpio = priv->n_ports * priv->n_pins;
160 priv->gc.owner = THIS_MODULE;
161 priv->gc.base = -1;
162 priv->gc.parent = &pdev->dev;
> 163 priv->gc.of_node = pdev->dev.of_node;
164
165 platform_set_drvdata(pdev, priv);
166
167 return devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv);
168 }
169
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] gpio: Add gpio latch driver
2022-08-31 20:50 ` Andy Shevchenko
@ 2022-09-01 8:20 ` Sascha Hauer
0 siblings, 0 replies; 12+ messages in thread
From: Sascha Hauer @ 2022-09-01 8:20 UTC (permalink / raw)
To: Andy Shevchenko
Cc: open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
Sascha Hauer
On Wed, Aug 31, 2022 at 11:50:47PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 31, 2022 at 9:02 AM Sascha Hauer <s.hauer@pengutronix.de> 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.
>
> I'm still unsure it shouldn't be a part of the (not yet in upstream)
> driver that I have mentioned before. But let's leave this apart right
> now.
I don't see how this could be done. The before mentioned driver depends
on a gpio-mux which is a binary decoder. This doesn't have a
correspondence in this driver.
>
> ...
>
> > +#include <linux/err.h>
> > +#include <linux/module.h>
>
> > +#include <linux/of_device.h>
>
> Why?
> It seems you misplaced it instead of mod_devicetable.h.
Ok.
>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/gpio/consumer.h>
>
> Keep above sorted?
>
> ...
>
> > + struct mutex mutex;
> > + spinlock_t spinlock;
>
> Checkpatch usually complains if locks are not commented. Looking at
> the below code, why it's not an (anonymous) union?
checkpatch only complains here when given a --subjective. Anyway,
commenting it is a good thing, and a union can be used here.
>
> ...
>
> > + if (val)
> > + priv->shadow[latch] |= BIT(offset % priv->n_pins);
> > + else
> > + priv->shadow[latch] &= ~BIT(offset % priv->n_pins);
>
> I believe shadow should be defined as unsigned long * and hence normal
> bit operations can be applied. For example here is assign_bit().
Good point.
> > +static const struct of_device_id gpio_latch_ids[] = {
> > + {
> > + .compatible = "gpio-latch",
> > + }, {
> > + /* sentinel */
> > + }
>
> You may compress this to the 2 LoCs.
I usually prefer not doing that as it means that we have to reformat it
once we initialize other fields as well, like here for example .data.
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 |
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] gpio: Add gpio latch driver
2022-08-31 5:58 ` [PATCH v2 1/2] gpio: Add gpio latch driver Sascha Hauer
` (3 preceding siblings ...)
2022-08-31 20:52 ` kernel test robot
@ 2022-09-02 6:42 ` Andy Shevchenko
2022-09-02 7:16 ` Sascha Hauer
4 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2022-09-02 6:42 UTC (permalink / raw)
To: Sascha Hauer
Cc: open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
Sascha Hauer
On Wed, Aug 31, 2022 at 9:02 AM Sascha Hauer <s.hauer@pengutronix.de> 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.
So, this is for only one type of latches, now I'm wondering why
gpio-74xx-mmio can't cover this case (with probably small
modifications to the code)?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] gpio: Add gpio latch driver
2022-09-02 6:42 ` Andy Shevchenko
@ 2022-09-02 7:16 ` Sascha Hauer
2022-09-02 7:20 ` Andy Shevchenko
0 siblings, 1 reply; 12+ messages in thread
From: Sascha Hauer @ 2022-09-02 7:16 UTC (permalink / raw)
To: Andy Shevchenko
Cc: open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
Sascha Hauer
On Fri, Sep 02, 2022 at 09:42:21AM +0300, Andy Shevchenko wrote:
> On Wed, Aug 31, 2022 at 9:02 AM Sascha Hauer <s.hauer@pengutronix.de> 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.
>
> So, this is for only one type of latches, now I'm wondering why
> gpio-74xx-mmio can't cover this case (with probably small
> modifications to the code)?
gpio-74xx-mmio is about latches connected to a parallel bus. You can
access the GPIOs by doing readl/writel operations. The latches are
driven by the bus logic and likely an additional address decoder.
What I have here instead is a latch fully driven by GPIOs.
Yes, with enough force you could implement it in the gpio-74xx-mmio
driver, but that wouldn't be mmio at all and likely completely different
code pathes.
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 |
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] gpio: Add gpio latch driver
2022-09-02 7:16 ` Sascha Hauer
@ 2022-09-02 7:20 ` Andy Shevchenko
0 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2022-09-02 7:20 UTC (permalink / raw)
To: Sascha Hauer
Cc: open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
Sascha Hauer
On Fri, Sep 2, 2022 at 10:16 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Fri, Sep 02, 2022 at 09:42:21AM +0300, Andy Shevchenko wrote:
> > On Wed, Aug 31, 2022 at 9:02 AM Sascha Hauer <s.hauer@pengutronix.de> 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.
> >
> > So, this is for only one type of latches, now I'm wondering why
> > gpio-74xx-mmio can't cover this case (with probably small
> > modifications to the code)?
>
> gpio-74xx-mmio is about latches connected to a parallel bus. You can
> access the GPIOs by doing readl/writel operations. The latches are
> driven by the bus logic and likely an additional address decoder.
>
> What I have here instead is a latch fully driven by GPIOs.
But this reminds me of some kind of gpio-aggregator with a specific
layer on top. To me it really feels that we are (semi-)reinventing a
wheel between the lines...
> Yes, with enough force you could implement it in the gpio-74xx-mmio
> driver, but that wouldn't be mmio at all and likely completely different
> code pathes.
Got it, thanks for elaboration.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-09-02 7:20 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-31 5:58 [PATCH v2 0/2] gpio: Add gpio-latch driver Sascha Hauer
2022-08-31 5:58 ` [PATCH v2 1/2] gpio: Add gpio latch driver Sascha Hauer
2022-08-31 8:01 ` Marco Felsch
2022-08-31 9:07 ` Sascha Hauer
2022-08-31 11:50 ` Bartosz Golaszewski
2022-08-31 20:50 ` Andy Shevchenko
2022-09-01 8:20 ` Sascha Hauer
2022-08-31 20:52 ` kernel test robot
2022-09-02 6:42 ` Andy Shevchenko
2022-09-02 7:16 ` Sascha Hauer
2022-09-02 7:20 ` Andy Shevchenko
2022-08-31 5:58 ` [PATCH v2 2/2] dt-bindings: gpio: Add gpio-latch binding document Sascha Hauer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.