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 would appreciate your tag.
In v3:
- added new patch 3 to prevent device removal from sysfs
- switched to feature in driver data instead of "compatible" (Geert)
- applied tags (Geert, Linus)
- left DT bindings untouched, can be amended later on
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
- left udelay() call untouched as recommended by documentation
Andy Shevchenko (5):
gpio: aggregator: Remove CONFIG_OF and of_match_ptr() protections
gpio: aggregator: Support delay for setting up individual GPIOs
gpio: aggregator: Prevent collisions between DT and user device IDs
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 | 113 +++++++++++++++++++++--
drivers/gpio/gpio-delay.c | 164 ---------------------------------
4 files changed, 106 insertions(+), 181 deletions(-)
delete mode 100644 drivers/gpio/gpio-delay.c
--
2.40.0.1.gaa8946217a0b
In case we have a device instantiated via DT or other means than
via new_device sysfs node, the collision with the latter is possible.
Prevent such collisions by allocating user instantiated devices with
higher IDs, currently set to 1024.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpio-aggregator.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 4a470dd8b75d..8892cb37ad79 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -26,6 +26,7 @@
#include <linux/gpio/driver.h>
#include <linux/gpio/machine.h>
+#define AGGREGATOR_MIN_DEVID 1024
#define AGGREGATOR_MAX_GPIOS 512
/*
@@ -135,7 +136,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
}
mutex_lock(&gpio_aggregator_lock);
- id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
+ id = idr_alloc(&gpio_aggregator_idr, aggr, AGGREGATOR_MIN_DEVID, 0, GFP_KERNEL);
mutex_unlock(&gpio_aggregator_lock);
if (id < 0) {
--
2.40.0.1.gaa8946217a0b
In some cases the GPIO may require an additional delay after setting
its value. Add support for that into the GPIO forwarder code.
This will be fully enabled for use in the following changes.
Reviewed-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpio-aggregator.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 1aa7455672d3..4a470dd8b75d 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -10,6 +10,7 @@
#include <linux/bitmap.h>
#include <linux/bitops.h>
#include <linux/ctype.h>
+#include <linux/delay.h>
#include <linux/idr.h>
#include <linux/kernel.h>
#include <linux/mod_devicetable.h>
@@ -240,6 +241,11 @@ static void __exit gpio_aggregator_remove_all(void)
* GPIO Forwarder
*/
+struct gpiochip_fwd_timing {
+ u32 ramp_up_us;
+ u32 ramp_down_us;
+};
+
struct gpiochip_fwd {
struct gpio_chip chip;
struct gpio_desc **descs;
@@ -247,6 +253,7 @@ struct gpiochip_fwd {
struct mutex mlock; /* protects tmp[] if can_sleep */
spinlock_t slock; /* protects tmp[] if !can_sleep */
};
+ struct gpiochip_fwd_timing *delay_timings;
unsigned long tmp[]; /* values and descs for multiple ops */
};
@@ -331,6 +338,27 @@ static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
return error;
}
+static void gpio_fwd_delay(struct gpio_chip *chip, unsigned int offset, int value)
+{
+ struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+ const struct gpiochip_fwd_timing *delay_timings;
+ bool is_active_low = gpiod_is_active_low(fwd->descs[offset]);
+ u32 delay_us;
+
+ delay_timings = &fwd->delay_timings[offset];
+ if ((!is_active_low && value) || (is_active_low && !value))
+ delay_us = delay_timings->ramp_up_us;
+ else
+ delay_us = delay_timings->ramp_down_us;
+ if (!delay_us)
+ return;
+
+ if (chip->can_sleep)
+ fsleep(delay_us);
+ else
+ udelay(delay_us);
+}
+
static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
{
struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
@@ -339,6 +367,9 @@ static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
gpiod_set_value_cansleep(fwd->descs[offset], value);
else
gpiod_set_value(fwd->descs[offset], value);
+
+ if (fwd->delay_timings)
+ gpio_fwd_delay(chip, offset, value);
}
static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
--
2.40.0.1.gaa8946217a0b
Hi Andy,
Thanks for your patch!
On Thu, Jun 15, 2023 at 3:51 PM Andy Shevchenko
<[email protected]> wrote:
> In case we have a device instantiated via DT or other means than
> via new_device sysfs node, the collision with the latter is possible.
> Prevent such collisions by allocating user instantiated devices with
> higher IDs, currently set to 1024.
Can you please elaborate? How exactly is this possible?
Aggregators instantiated through sysfs are named "gpio-aggregator.<n>",
and are IDR-based.
Aggregators instantiated from DT are named "<unit-address>.<node-name>".
How can this conflict? When instantiated from ACPI?
What am I missing?
> Signed-off-by: Andy Shevchenko <[email protected]>
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -26,6 +26,7 @@
> #include <linux/gpio/driver.h>
> #include <linux/gpio/machine.h>
>
> +#define AGGREGATOR_MIN_DEVID 1024
> #define AGGREGATOR_MAX_GPIOS 512
>
> /*
> @@ -135,7 +136,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> }
>
> mutex_lock(&gpio_aggregator_lock);
> - id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> + id = idr_alloc(&gpio_aggregator_idr, aggr, AGGREGATOR_MIN_DEVID, 0, GFP_KERNEL);
Iff this would solve an issue, it would be only temporarily, until someone
instantiates 1024 aggregators through some other means ;-)
> mutex_unlock(&gpio_aggregator_lock);
>
> if (id < 0) {
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 04:54:14PM +0200, Geert Uytterhoeven wrote:
> On Thu, Jun 15, 2023 at 3:51 PM Andy Shevchenko
> <[email protected]> wrote:
> > In case we have a device instantiated via DT or other means than
> > via new_device sysfs node, the collision with the latter is possible.
> > Prevent such collisions by allocating user instantiated devices with
> > higher IDs, currently set to 1024.
>
> Can you please elaborate? How exactly is this possible?
>
> Aggregators instantiated through sysfs are named "gpio-aggregator.<n>",
> and are IDR-based.
> Aggregators instantiated from DT are named "<unit-address>.<node-name>".
> How can this conflict? When instantiated from ACPI?
> What am I missing?
Nothing. It's me who misunderstood how OF platform device naming schema works.
So this patch can be discarded as we never will have gpio-delay available for
removal via delete_device sysfs node.
Bart, tell me if you need a new version w/o this patch (but note that b4 can
handle this case with
b4 -slt -P1,2,4,5 ...
).
--
With Best Regards,
Andy Shevchenko
Hi Andy,
Am Donnerstag, 15. Juni 2023, 15:20:18 CEST schrieb Andy Shevchenko:
> 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 would appreciate your tag.
This works on my platform:
Tested-by: Alexander Stein <[email protected]>
> In v3:
> - added new patch 3 to prevent device removal from sysfs
> - switched to feature in driver data instead of "compatible" (Geert)
> - applied tags (Geert, Linus)
> - left DT bindings untouched, can be amended later on
>
> 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
> - left udelay() call untouched as recommended by documentation
>
> Andy Shevchenko (5):
> gpio: aggregator: Remove CONFIG_OF and of_match_ptr() protections
> gpio: aggregator: Support delay for setting up individual GPIOs
> gpio: aggregator: Prevent collisions between DT and user device IDs
> 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 | 113 +++++++++++++++++++++--
> drivers/gpio/gpio-delay.c | 164 ---------------------------------
> 4 files changed, 106 insertions(+), 181 deletions(-)
> delete mode 100644 drivers/gpio/gpio-delay.c
--
TQ-Systems GmbH | M?hlstra?e 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht M?nchen, HRB 105018
Gesch?ftsf?hrer: Detlef Schneider, R?diger Stahl, Stefan Schneider
http://www.tq-group.com/
On Thu, Jun 15, 2023 at 3:51 PM 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 would appreciate your tag.
>
> In v3:
> - added new patch 3 to prevent device removal from sysfs
> - switched to feature in driver data instead of "compatible" (Geert)
> - applied tags (Geert, Linus)
> - left DT bindings untouched, can be amended later on
>
> 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
> - left udelay() call untouched as recommended by documentation
>
> Andy Shevchenko (5):
> gpio: aggregator: Remove CONFIG_OF and of_match_ptr() protections
> gpio: aggregator: Support delay for setting up individual GPIOs
> gpio: aggregator: Prevent collisions between DT and user device IDs
> 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 | 113 +++++++++++++++++++++--
> drivers/gpio/gpio-delay.c | 164 ---------------------------------
> 4 files changed, 106 insertions(+), 181 deletions(-)
> delete mode 100644 drivers/gpio/gpio-delay.c
>
> --
> 2.40.0.1.gaa8946217a0b
>
Applied patches 1, 2, 4 and 5. Thanks!
Bart
On Fri, Jun 16, 2023 at 11:01:17AM +0200, Alexander Stein wrote:
> Am Donnerstag, 15. Juni 2023, 15:20:18 CEST schrieb Andy Shevchenko:
> > 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 would appreciate your tag.
>
> This works on my platform:
> Tested-by: Alexander Stein <[email protected]>
Thank you!
--
With Best Regards,
Andy Shevchenko