The newly appeared gpio-delay module enables external signal delay lines
that may be connected to the GPIOs. But at the same time it copies the
GPIO forwarder functionality. Besides that the approach does not scale.
If we would have another external component, we would need yet another
driver. That's why I think, and seems others support me, better to
enable such a functionality inside GPIO aggregator driver.
Patch 1 is a cleanup that may be applied independently on the decision
about the rest.
Please, test and comment!
In v2:
- split as a series
- covered CONFIG_OF_GPIO=n case
- removed the gpio-delay
- moved gpio-delay Kconfig help to the comment in the code
Not in v2:
- updated DT description to note about long timeouts in atomic mode
- sysfs hiding for gpio-delay functionality
- used msleep(): the documentation is _against_ this
Andy Shevchenko (4):
gpio: aggregator: Remove CONFIG_OF and of_match_ptr() protections
gpio: aggregator: Support delay for setting up individual GPIOs
gpio: aggregator: Set up a parser of delay line parameters
gpio: delay: Remove duplicative functionality
drivers/gpio/Kconfig | 9 --
drivers/gpio/Makefile | 1 -
drivers/gpio/gpio-aggregator.c | 109 ++++++++++++++++++++--
drivers/gpio/gpio-delay.c | 164 ---------------------------------
4 files changed, 103 insertions(+), 180 deletions(-)
delete mode 100644 drivers/gpio/gpio-delay.c
--
2.40.0.1.gaa8946217a0b
Now that GPIO aggregator supports a delay line, drop the duplicative
functionality, i.e. the entire gpio-delay driver.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/Kconfig | 9 ---
drivers/gpio/Makefile | 1 -
drivers/gpio/gpio-delay.c | 164 --------------------------------------
3 files changed, 174 deletions(-)
delete mode 100644 drivers/gpio/gpio-delay.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 209738ef1446..abaae68c88a4 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1748,15 +1748,6 @@ config GPIO_AGGREGATOR
industrial control context, to be operated from userspace using
the GPIO chardev interface.
-config GPIO_DELAY
- tristate "GPIO delay"
- depends on OF_GPIO
- help
- Say yes here to enable the GPIO delay, which provides a way to
- configure platform specific delays for GPIO ramp-up or ramp-down
- delays. This can serve the following purposes:
- - Open-drain output using an RC filter
-
config GPIO_LATCH
tristate "GPIO latch driver"
help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 947c9cf9aba8..7843b16f5d59 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -52,7 +52,6 @@ obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
obj-$(CONFIG_GPIO_DA9055) += gpio-da9055.o
obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o
obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o
-obj-$(CONFIG_GPIO_DELAY) += gpio-delay.o
obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o
obj-$(CONFIG_GPIO_EIC_SPRD) += gpio-eic-sprd.o
obj-$(CONFIG_GPIO_ELKHARTLAKE) += gpio-elkhartlake.o
diff --git a/drivers/gpio/gpio-delay.c b/drivers/gpio/gpio-delay.c
deleted file mode 100644
index b489b561b225..000000000000
--- a/drivers/gpio/gpio-delay.c
+++ /dev/null
@@ -1,164 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright 2022 TQ-Systems GmbH
- * Author: Alexander Stein <[email protected]>
- */
-
-#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_delay_timing {
- unsigned long ramp_up_delay_us;
- unsigned long ramp_down_delay_us;
-};
-
-struct gpio_delay_priv {
- struct gpio_chip gc;
- struct gpio_descs *input_gpio;
- struct gpio_delay_timing *delay_timings;
-};
-
-static int gpio_delay_get_direction(struct gpio_chip *gc, unsigned int offset)
-{
- return GPIO_LINE_DIRECTION_OUT;
-}
-
-static void gpio_delay_set(struct gpio_chip *gc, unsigned int offset, int val)
-{
- struct gpio_delay_priv *priv = gpiochip_get_data(gc);
- struct gpio_desc *gpio_desc = priv->input_gpio->desc[offset];
- const struct gpio_delay_timing *delay_timings;
- bool ramp_up;
-
- gpiod_set_value(gpio_desc, val);
-
- delay_timings = &priv->delay_timings[offset];
- ramp_up = (!gpiod_is_active_low(gpio_desc) && val) ||
- (gpiod_is_active_low(gpio_desc) && !val);
-
- if (ramp_up && delay_timings->ramp_up_delay_us)
- udelay(delay_timings->ramp_up_delay_us);
- if (!ramp_up && delay_timings->ramp_down_delay_us)
- udelay(delay_timings->ramp_down_delay_us);
-}
-
-static void gpio_delay_set_can_sleep(struct gpio_chip *gc, unsigned int offset, int val)
-{
- struct gpio_delay_priv *priv = gpiochip_get_data(gc);
- struct gpio_desc *gpio_desc = priv->input_gpio->desc[offset];
- const struct gpio_delay_timing *delay_timings;
- bool ramp_up;
-
- gpiod_set_value_cansleep(gpio_desc, val);
-
- delay_timings = &priv->delay_timings[offset];
- ramp_up = (!gpiod_is_active_low(gpio_desc) && val) ||
- (gpiod_is_active_low(gpio_desc) && !val);
-
- if (ramp_up && delay_timings->ramp_up_delay_us)
- fsleep(delay_timings->ramp_up_delay_us);
- if (!ramp_up && delay_timings->ramp_down_delay_us)
- fsleep(delay_timings->ramp_down_delay_us);
-}
-
-static int gpio_delay_of_xlate(struct gpio_chip *gc,
- const struct of_phandle_args *gpiospec,
- u32 *flags)
-{
- struct gpio_delay_priv *priv = gpiochip_get_data(gc);
- struct gpio_delay_timing *timings;
- u32 line;
-
- if (gpiospec->args_count != gc->of_gpio_n_cells)
- return -EINVAL;
-
- line = gpiospec->args[0];
- if (line >= gc->ngpio)
- return -EINVAL;
-
- timings = &priv->delay_timings[line];
- timings->ramp_up_delay_us = gpiospec->args[1];
- timings->ramp_down_delay_us = gpiospec->args[2];
-
- return line;
-}
-
-static bool gpio_delay_can_sleep(const struct gpio_delay_priv *priv)
-{
- int i;
-
- for (i = 0; i < priv->input_gpio->ndescs; i++)
- if (gpiod_cansleep(priv->input_gpio->desc[i]))
- return true;
-
- return false;
-}
-
-static int gpio_delay_probe(struct platform_device *pdev)
-{
- struct gpio_delay_priv *priv;
-
- priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
-
- priv->input_gpio = devm_gpiod_get_array(&pdev->dev, NULL, GPIOD_OUT_LOW);
- if (IS_ERR(priv->input_gpio))
- return dev_err_probe(&pdev->dev, PTR_ERR(priv->input_gpio),
- "Failed to get input-gpios");
-
- priv->delay_timings = devm_kcalloc(&pdev->dev,
- priv->input_gpio->ndescs,
- sizeof(*priv->delay_timings),
- GFP_KERNEL);
- if (!priv->delay_timings)
- return -ENOMEM;
-
- if (gpio_delay_can_sleep(priv)) {
- priv->gc.can_sleep = true;
- priv->gc.set = gpio_delay_set_can_sleep;
- } else {
- priv->gc.can_sleep = false;
- priv->gc.set = gpio_delay_set;
- }
-
- priv->gc.get_direction = gpio_delay_get_direction;
- priv->gc.of_xlate = gpio_delay_of_xlate;
- priv->gc.of_gpio_n_cells = 3;
- priv->gc.ngpio = priv->input_gpio->ndescs;
- 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_delay_ids[] = {
- {
- .compatible = "gpio-delay",
- },
- { /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, gpio_delay_ids);
-
-static struct platform_driver gpio_delay_driver = {
- .driver = {
- .name = "gpio-delay",
- .of_match_table = gpio_delay_ids,
- },
- .probe = gpio_delay_probe,
-};
-module_platform_driver(gpio_delay_driver);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Alexander Stein <[email protected]>");
-MODULE_DESCRIPTION("GPIO delay driver");
--
2.40.0.1.gaa8946217a0b
They stop the driver being used with ACPI PRP0001 and are something
I want to avoid being cut and paste into new drivers. Also include
mod_devicetable.h as we struct of_device_id is defined in there.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpio-aggregator.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 20a686f12df7..1aa7455672d3 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -12,6 +12,7 @@
#include <linux/ctype.h>
#include <linux/idr.h>
#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/overflow.h>
@@ -500,23 +501,21 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_OF
static const struct of_device_id gpio_aggregator_dt_ids[] = {
/*
* Add GPIO-operated devices controlled from userspace below,
- * or use "driver_override" in sysfs
+ * or use "driver_override" in sysfs.
*/
{}
};
MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
-#endif
static struct platform_driver gpio_aggregator_driver = {
.probe = gpio_aggregator_probe,
.driver = {
.name = DRV_NAME,
.groups = gpio_aggregator_groups,
- .of_match_table = of_match_ptr(gpio_aggregator_dt_ids),
+ .of_match_table = gpio_aggregator_dt_ids,
},
};
--
2.40.0.1.gaa8946217a0b
On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<[email protected]> wrote:
> They stop the driver being used with ACPI PRP0001 and are something
> I want to avoid being cut and paste into new drivers. Also include
> mod_devicetable.h as we struct of_device_id is defined in there.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<[email protected]> wrote:
> Now that GPIO aggregator supports a delay line, drop the duplicative
> functionality, i.e. the entire gpio-delay driver.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
This is the most pleasing technical solution for sure.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<[email protected]> wrote:
> They stop the driver being used with ACPI PRP0001 and are something
> I want to avoid being cut and paste into new drivers. Also include
> mod_devicetable.h as we struct of_device_id is defined in there.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko
<[email protected]> wrote:
> Now that GPIO aggregator supports a delay line, drop the duplicative
> functionality, i.e. the entire gpio-delay driver.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Jun 15, 2023 at 2:14 AM Andy Shevchenko
<[email protected]> wrote:
>
> The newly appeared gpio-delay module enables external signal delay lines
> that may be connected to the GPIOs. But at the same time it copies the
> GPIO forwarder functionality. Besides that the approach does not scale.
> If we would have another external component, we would need yet another
> driver. That's why I think, and seems others support me, better to
> enable such a functionality inside GPIO aggregator driver.
>
> Patch 1 is a cleanup that may be applied independently on the decision
> about the rest.
>
> Please, test and comment!
Alexander, I have slightly changed the code, can you test this and
give your formal Tested-by tag?
--
With Best Regards,
Andy Shevchenko