LinuxLists.cc - [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins

2023-06-08 16:34:03

Subject: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins

The aggregator mode can also handle properties of the platform, that
do not belong to the GPIO controller itself. One of such a property
is signal delay line. Intdoduce support of it.

Signed-off-by: Andy Shevchenko <[email protected]>
---

I don't like the idea of gpio-delay or similar. We have already GPIO
aggregator that incorporates the GPIO proxy / forwarder functionality.

This one is RFC because:
1) just compile tested;
2) has obvious issues with CONFIG_OF_GPIO;
3) contains ~5 patches in a single change for now;
4) requires additional work with blocking sysfs for this;
5) requires some DT bindings work;
6) ...whatever I forgot...

Any comments are appreciated, and tests are esp. welcome!

drivers/gpio/gpio-aggregator.c | 84 ++++++++++++++++++++++++++++++----
1 file changed, 74 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 20a686f12df7..802d123f0188 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -10,12 +10,14 @@
#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/module.h>
#include <linux/mutex.h>
#include <linux/overflow.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/string.h>
@@ -239,6 +241,11 @@ static void __exit gpio_aggregator_remove_all(void)
* GPIO Forwarder
*/

+struct gpiochip_fwd_timing {
+ unsigned long ramp_up_us;
+ unsigned long ramp_down_us;
+};
+
struct gpiochip_fwd {
struct gpio_chip chip;
struct gpio_desc **descs;
@@ -246,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 */
};

@@ -333,11 +341,28 @@ static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
{
struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+ const struct gpiochip_fwd_timing *delay_timings;
+ struct gpio_desc *desc = fwd->descs[offset];
+ bool is_active_low = gpiod_is_active_low(desc);
+ bool ramp_up;

- if (chip->can_sleep)
- gpiod_set_value_cansleep(fwd->descs[offset], value);
- else
- gpiod_set_value(fwd->descs[offset], value);
+ delay_timings = &fwd->delay_timings[offset];
+ ramp_up = (!is_active_low && value) || (is_active_low && !value);
+ if (chip->can_sleep) {
+ gpiod_set_value_cansleep(desc, value);
+
+ if (ramp_up && delay_timings->ramp_up_us)
+ fsleep(delay_timings->ramp_up_us);
+ if (!ramp_up && delay_timings->ramp_down_us)
+ fsleep(delay_timings->ramp_down_us);
+ } else {
+ gpiod_set_value(desc, value);
+
+ if (ramp_up && delay_timings->ramp_up_us)
+ udelay(delay_timings->ramp_up_us);
+ if (!ramp_up && delay_timings->ramp_down_us)
+ udelay(delay_timings->ramp_down_us);
+ }
}

static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
@@ -390,6 +415,28 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset)
return gpiod_to_irq(fwd->descs[offset]);
}

+static int gpiochip_fwd_delay_of_xlate(struct gpio_chip *chip,
+ const struct of_phandle_args *gpiospec,
+ u32 *flags)
+{
+ struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+ struct gpiochip_fwd_timing *timings;
+ u32 line;
+
+ if (gpiospec->args_count != chip->of_gpio_n_cells)
+ return -EINVAL;
+
+ line = gpiospec->args[0];
+ if (line >= chip->ngpio)
+ return -EINVAL;
+
+ timings = &fwd->delay_timings[line];
+ timings->ramp_up_us = gpiospec->args[1];
+ timings->ramp_down_us = gpiospec->args[2];
+
+ return line;
+}
+
/**
* gpiochip_fwd_create() - Create a new GPIO forwarder
* @dev: Parent device pointer
@@ -397,6 +444,7 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset)
* @descs: Array containing the GPIO descriptors to forward to.
* This array must contain @ngpios entries, and must not be deallocated
* before the forwarder has been destroyed again.
+ * @delay: True if the pins have an external delay line.
*
* This function creates a new gpiochip, which forwards all GPIO operations to
* the passed GPIO descriptors.
@@ -406,7 +454,8 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset)
*/
static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
unsigned int ngpios,
- struct gpio_desc *descs[])
+ struct gpio_desc *descs[],
+ bool delay)
{
const char *label = dev_name(dev);
struct gpiochip_fwd *fwd;
@@ -459,6 +508,17 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
else
spin_lock_init(&fwd->slock);

+ if (delay) {
+ fwd->delay_timings = devm_kcalloc(dev, ngpios,
+ sizeof(*fwd->delay_timings),
+ GFP_KERNEL);
+ if (!fwd->delay_timings)
+ return ERR_PTR(-ENOMEM);
+
+ chip->of_xlate = gpiochip_fwd_delay_of_xlate;
+ chip->of_gpio_n_cells = 3;
+ }
+
error = devm_gpiochip_add_data(dev, chip, fwd);
if (error)
return ERR_PTR(error);
@@ -476,6 +536,7 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct gpio_desc **descs;
struct gpiochip_fwd *fwd;
+ bool delay;
int i, n;

n = gpiod_count(dev, NULL);
@@ -492,7 +553,9 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
return PTR_ERR(descs[i]);
}

- fwd = gpiochip_fwd_create(dev, n, descs);
+ delay = fwnode_device_is_compatible(dev_fwnode(dev), "gpio-delay");
+
+ fwd = gpiochip_fwd_create(dev, n, descs, delay);
if (IS_ERR(fwd))
return PTR_ERR(fwd);

@@ -500,23 +563,24 @@ 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.
*/
+ {
+ .compatible = "gpio-delay",
+ },
{}
};
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

2023-06-08 19:49:33

by kernel test robot

[permalink] [raw]

Subject: Re: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins

2023-06-08 20:25:35

by kernel test robot

[permalink] [raw]

Subject: Re: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins

Hi Andy,

kernel test robot noticed the following build errors:

[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on linus/master v6.4-rc5 next-20230608]
[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/Andy-Shevchenko/gpio-aggregator-Introduce-delay-support-for-individual-output-pins/20230609-002703
base: https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link: https://lore.kernel.org/r/20230608162130.55015-1-andriy.shevchenko%40linux.intel.com
patch subject: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins
config: hexagon-randconfig-r023-20230608 (https://download.01.org/0day-ci/archive/20230609/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git remote add brgl https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
git fetch brgl gpio/for-next
git checkout brgl/gpio/for-next
b4 shazam https://lore.kernel.org/r/[email protected]
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/gpio/

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

In file included from drivers/gpio/gpio-aggregator.c:26:
In file included from include/linux/gpio/driver.h:6:
In file included from include/linux/irqchip/chained_irq.h:10:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/gpio/gpio-aggregator.c:26:
In file included from include/linux/gpio/driver.h:6:
In file included from include/linux/irqchip/chained_irq.h:10:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/gpio/gpio-aggregator.c:26:
In file included from include/linux/gpio/driver.h:6:
In file included from include/linux/irqchip/chained_irq.h:10:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
>> drivers/gpio/gpio-aggregator.c:426:36: error: no member named 'of_gpio_n_cells' in 'struct gpio_chip'
426 | if (gpiospec->args_count != chip->of_gpio_n_cells)
| ~~~~ ^
>> drivers/gpio/gpio-aggregator.c:518:9: error: no member named 'of_xlate' in 'struct gpio_chip'
518 | chip->of_xlate = gpiochip_fwd_delay_of_xlate;
| ~~~~ ^
drivers/gpio/gpio-aggregator.c:519:9: error: no member named 'of_gpio_n_cells' in 'struct gpio_chip'
519 | chip->of_gpio_n_cells = 3;
| ~~~~ ^
6 warnings and 3 errors generated.

vim +426 drivers/gpio/gpio-aggregator.c

417
418 static int gpiochip_fwd_delay_of_xlate(struct gpio_chip *chip,
419 const struct of_phandle_args *gpiospec,
420 u32 *flags)
421 {
422 struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
423 struct gpiochip_fwd_timing *timings;
424 u32 line;
425
> 426 if (gpiospec->args_count != chip->of_gpio_n_cells)
427 return -EINVAL;
428
429 line = gpiospec->args[0];
430 if (line >= chip->ngpio)
431 return -EINVAL;
432
433 timings = &fwd->delay_timings[line];
434 timings->ramp_up_us = gpiospec->args[1];
435 timings->ramp_down_us = gpiospec->args[2];
436
437 return line;
438 }
439
440 /**
441 * gpiochip_fwd_create() - Create a new GPIO forwarder
442 * @dev: Parent device pointer
443 * @ngpios: Number of GPIOs in the forwarder.
444 * @descs: Array containing the GPIO descriptors to forward to.
445 * This array must contain @ngpios entries, and must not be deallocated
446 * before the forwarder has been destroyed again.
447 * @delay: True if the pins have an external delay line.
448 *
449 * This function creates a new gpiochip, which forwards all GPIO operations to
450 * the passed GPIO descriptors.
451 *
452 * Return: An opaque object pointer, or an ERR_PTR()-encoded negative error
453 * code on failure.
454 */
455 static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
456 unsigned int ngpios,
457 struct gpio_desc *descs[],
458 bool delay)
459 {
460 const char *label = dev_name(dev);
461 struct gpiochip_fwd *fwd;
462 struct gpio_chip *chip;
463 unsigned int i;
464 int error;
465
466 fwd = devm_kzalloc(dev, struct_size(fwd, tmp, fwd_tmp_size(ngpios)),
467 GFP_KERNEL);
468 if (!fwd)
469 return ERR_PTR(-ENOMEM);
470
471 chip = &fwd->chip;
472
473 /*
474 * If any of the GPIO lines are sleeping, then the entire forwarder
475 * will be sleeping.
476 * If any of the chips support .set_config(), then the forwarder will
477 * support setting configs.
478 */
479 for (i = 0; i < ngpios; i++) {
480 struct gpio_chip *parent = gpiod_to_chip(descs[i]);
481
482 dev_dbg(dev, "%u => gpio %d irq %d\n", i,
483 desc_to_gpio(descs[i]), gpiod_to_irq(descs[i]));
484
485 if (gpiod_cansleep(descs[i]))
486 chip->can_sleep = true;
487 if (parent && parent->set_config)
488 chip->set_config = gpio_fwd_set_config;
489 }
490
491 chip->label = label;
492 chip->parent = dev;
493 chip->owner = THIS_MODULE;
494 chip->get_direction = gpio_fwd_get_direction;
495 chip->direction_input = gpio_fwd_direction_input;
496 chip->direction_output = gpio_fwd_direction_output;
497 chip->get = gpio_fwd_get;
498 chip->get_multiple = gpio_fwd_get_multiple_locked;
499 chip->set = gpio_fwd_set;
500 chip->set_multiple = gpio_fwd_set_multiple_locked;
501 chip->to_irq = gpio_fwd_to_irq;
502 chip->base = -1;
503 chip->ngpio = ngpios;
504 fwd->descs = descs;
505
506 if (chip->can_sleep)
507 mutex_init(&fwd->mlock);
508 else
509 spin_lock_init(&fwd->slock);
510
511 if (delay) {
512 fwd->delay_timings = devm_kcalloc(dev, ngpios,
513 sizeof(*fwd->delay_timings),
514 GFP_KERNEL);
515 if (!fwd->delay_timings)
516 return ERR_PTR(-ENOMEM);
517
> 518 chip->of_xlate = gpiochip_fwd_delay_of_xlate;
519 chip->of_gpio_n_cells = 3;
520 }
521
522 error = devm_gpiochip_add_data(dev, chip, fwd);
523 if (error)
524 return ERR_PTR(error);
525
526 return fwd;
527 }
528

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-06-09 06:55:20

by Alexander Stein

[permalink] [raw]

Subject: Re: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins

Am Donnerstag, 8. Juni 2023, 18:23:08 CEST schrieb Andy Shevchenko:
> The aggregator mode can also handle properties of the platform, that
> do not belong to the GPIO controller itself. One of such a property
> is signal delay line. Intdoduce support of it.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
>
> I don't like the idea of gpio-delay or similar. We have already GPIO
> aggregator that incorporates the GPIO proxy / forwarder functionality.
>
> This one is RFC because:
> 1) just compile tested;
> 2) has obvious issues with CONFIG_OF_GPIO;
> 3) contains ~5 patches in a single change for now;
> 4) requires additional work with blocking sysfs for this;
> 5) requires some DT bindings work;
> 6) ...whatever I forgot...
>
> Any comments are appreciated, and tests are esp. welcome!

FWIW: Replacing CONFIG_GPIO_DELAY=m with CONFIG_GPIO_AGGREGATOR=m works as
well on my platform.
But I'm not sure if it's worth the additional complexity for gpio-aggregator
to replace gpio-delay.

Regards,
Alexander

> drivers/gpio/gpio-aggregator.c | 84 ++++++++++++++++++++++++++++++----
> 1 file changed, 74 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> index 20a686f12df7..802d123f0188 100644
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -10,12 +10,14 @@
> #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/module.h>
> #include <linux/mutex.h>
> #include <linux/overflow.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/string.h>
> @@ -239,6 +241,11 @@ static void __exit gpio_aggregator_remove_all(void)
> * GPIO Forwarder
> */
>
> +struct gpiochip_fwd_timing {
> + unsigned long ramp_up_us;
> + unsigned long ramp_down_us;
> +};
> +
> struct gpiochip_fwd {
> struct gpio_chip chip;
> struct gpio_desc **descs;
> @@ -246,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
*/
> };
>
> @@ -333,11 +341,28 @@ static int gpio_fwd_get_multiple_locked(struct
> gpio_chip *chip, static void gpio_fwd_set(struct gpio_chip *chip, unsigned
> int offset, int value) {
> struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> + const struct gpiochip_fwd_timing *delay_timings;
> + struct gpio_desc *desc = fwd->descs[offset];
> + bool is_active_low = gpiod_is_active_low(desc);
> + bool ramp_up;
>
> - if (chip->can_sleep)
> - gpiod_set_value_cansleep(fwd->descs[offset], value);
> - else
> - gpiod_set_value(fwd->descs[offset], value);
> + delay_timings = &fwd->delay_timings[offset];
> + ramp_up = (!is_active_low && value) || (is_active_low && !value);
> + if (chip->can_sleep) {
> + gpiod_set_value_cansleep(desc, value);
> +
> + if (ramp_up && delay_timings->ramp_up_us)
> + fsleep(delay_timings->ramp_up_us);
> + if (!ramp_up && delay_timings->ramp_down_us)
> + fsleep(delay_timings->ramp_down_us);
> + } else {
> + gpiod_set_value(desc, value);
> +
> + if (ramp_up && delay_timings->ramp_up_us)
> + udelay(delay_timings->ramp_up_us);
> + if (!ramp_up && delay_timings->ramp_down_us)
> + udelay(delay_timings->ramp_down_us);
> + }
> }
>
> static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long
> *mask, @@ -390,6 +415,28 @@ static int gpio_fwd_to_irq(struct gpio_chip
> *chip, unsigned int offset) return gpiod_to_irq(fwd->descs[offset]);
> }
>
> +static int gpiochip_fwd_delay_of_xlate(struct gpio_chip *chip,
> + const struct of_phandle_args
*gpiospec,
> + u32 *flags)
> +{
> + struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> + struct gpiochip_fwd_timing *timings;
> + u32 line;
> +
> + if (gpiospec->args_count != chip->of_gpio_n_cells)
> + return -EINVAL;
> +
> + line = gpiospec->args[0];
> + if (line >= chip->ngpio)
> + return -EINVAL;
> +
> + timings = &fwd->delay_timings[line];
> + timings->ramp_up_us = gpiospec->args[1];
> + timings->ramp_down_us = gpiospec->args[2];
> +
> + return line;
> +}
> +
> /**
> * gpiochip_fwd_create() - Create a new GPIO forwarder
> * @dev: Parent device pointer
> @@ -397,6 +444,7 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip,
> unsigned int offset) * @descs: Array containing the GPIO descriptors to
> forward to.
> * This array must contain @ngpios entries, and must not be
> deallocated * before the forwarder has been destroyed again.
> + * @delay: True if the pins have an external delay line.
> *
> * This function creates a new gpiochip, which forwards all GPIO operations
> to * the passed GPIO descriptors.
> @@ -406,7 +454,8 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip,
> unsigned int offset) */
> static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
> unsigned int
ngpios,
> - struct gpio_desc
*descs[])
> + struct gpio_desc
*descs[],
> + bool delay)
> {
> const char *label = dev_name(dev);
> struct gpiochip_fwd *fwd;
> @@ -459,6 +508,17 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct
> device *dev, else
> spin_lock_init(&fwd->slock);
>
> + if (delay) {
> + fwd->delay_timings = devm_kcalloc(dev, ngpios,
> + sizeof(*fwd-
>delay_timings),
> + GFP_KERNEL);
> + if (!fwd->delay_timings)
> + return ERR_PTR(-ENOMEM);
> +
> + chip->of_xlate = gpiochip_fwd_delay_of_xlate;
> + chip->of_gpio_n_cells = 3;
> + }
> +
> error = devm_gpiochip_add_data(dev, chip, fwd);
> if (error)
> return ERR_PTR(error);
> @@ -476,6 +536,7 @@ static int gpio_aggregator_probe(struct platform_device
> *pdev) struct device *dev = &pdev->dev;
> struct gpio_desc **descs;
> struct gpiochip_fwd *fwd;
> + bool delay;
> int i, n;
>
> n = gpiod_count(dev, NULL);
> @@ -492,7 +553,9 @@ static int gpio_aggregator_probe(struct platform_device
> *pdev) return PTR_ERR(descs[i]);
> }
>
> - fwd = gpiochip_fwd_create(dev, n, descs);
> + delay = fwnode_device_is_compatible(dev_fwnode(dev), "gpio-delay");
> +
> + fwd = gpiochip_fwd_create(dev, n, descs, delay);
> if (IS_ERR(fwd))
> return PTR_ERR(fwd);
>
> @@ -500,23 +563,24 @@ 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.
> */
> + {
> + .compatible = "gpio-delay",
> + },
> {}
> };
> 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,
> },
> };

--
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/

2023-06-09 07:15:49

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins

Hi Andy,

On Thu, Jun 8, 2023 at 6:23 PM Andy Shevchenko
<[email protected]> wrote:
> The aggregator mode can also handle properties of the platform, that
> do not belong to the GPIO controller itself. One of such a property
> is signal delay line. Intdoduce support of it.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
>
> I don't like the idea of gpio-delay or similar. We have already GPIO
> aggregator that incorporates the GPIO proxy / forwarder functionality.

I think this makes sense.

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c

> @@ -333,11 +341,28 @@ static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
> static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
> {
> struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> + const struct gpiochip_fwd_timing *delay_timings;
> + struct gpio_desc *desc = fwd->descs[offset];
> + bool is_active_low = gpiod_is_active_low(desc);
> + bool ramp_up;
>
> - if (chip->can_sleep)
> - gpiod_set_value_cansleep(fwd->descs[offset], value);
> - else
> - gpiod_set_value(fwd->descs[offset], value);
> + delay_timings = &fwd->delay_timings[offset];
> + ramp_up = (!is_active_low && value) || (is_active_low && !value);
> + if (chip->can_sleep) {
> + gpiod_set_value_cansleep(desc, value);
> +
> + if (ramp_up && delay_timings->ramp_up_us)
> + fsleep(delay_timings->ramp_up_us);
> + if (!ramp_up && delay_timings->ramp_down_us)
> + fsleep(delay_timings->ramp_down_us);
> + } else {
> + gpiod_set_value(desc, value);
> +
> + if (ramp_up && delay_timings->ramp_up_us)
> + udelay(delay_timings->ramp_up_us);
> + if (!ramp_up && delay_timings->ramp_down_us)
> + udelay(delay_timings->ramp_down_us);

I hope no one ever needs to use the values from the example in the
bindings

enable-gpios = <&enable_delay 0 130000 30000>;

on a non-sleepable GPIO. Not only is a looping delay of 130 ms very bad
for system responsiveness, such large delays may not even be supported
on all systems (e.g. ARM implementation says < 2 ms).
So for large values, this should use mdelay().

This also applies to gpio-delay, of course.

> + }
> }

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

2023-06-09 08:16:24

by Linus Walleij

[permalink] [raw]

Subject: Re: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins

On Thu, Jun 8, 2023 at 6:22 PM Andy Shevchenko
<[email protected]> wrote:

> The aggregator mode can also handle properties of the platform, that
> do not belong to the GPIO controller itself. One of such a property
> is signal delay line. Intdoduce support of it.
(...)
> I don't like the idea of gpio-delay or similar. We have already GPIO
> aggregator that incorporates the GPIO proxy / forwarder functionality.

You are right. This is the right solution going forward IMO.

Yours,
Linus Walleij

2023-06-12 17:55:57

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins

On Fri, Jun 09, 2023 at 08:40:15AM +0200, Alexander Stein wrote:
> Am Donnerstag, 8. Juni 2023, 18:23:08 CEST schrieb Andy Shevchenko:
> > The aggregator mode can also handle properties of the platform, that
> > do not belong to the GPIO controller itself. One of such a property
> > is signal delay line. Intdoduce support of it.
> >
> > Signed-off-by: Andy Shevchenko <[email protected]>
> > ---
> >
> > I don't like the idea of gpio-delay or similar. We have already GPIO
> > aggregator that incorporates the GPIO proxy / forwarder functionality.
> >
> > This one is RFC because:
> > 1) just compile tested;
> > 2) has obvious issues with CONFIG_OF_GPIO;
> > 3) contains ~5 patches in a single change for now;
> > 4) requires additional work with blocking sysfs for this;
> > 5) requires some DT bindings work;
> > 6) ...whatever I forgot...
> >
> > Any comments are appreciated, and tests are esp. welcome!
>
> FWIW: Replacing CONFIG_GPIO_DELAY=m with CONFIG_GPIO_AGGREGATOR=m works as
> well on my platform.

Thank you for testing!

> But I'm not sure if it's worth the additional complexity for gpio-aggregator
> to replace gpio-delay.

The (main) problem is that it does not scale. Today we have RC chain, tomorrow
(hypothetically) LC or LRC. Are we going to create (repeat) the similar approach
for each single case?

I would probably tolerate the existence of the gpio-delay, but we already have
GPIO forwarder, which implements 70% (?) percent of gpio-delay already.

--
With Best Regards,
Andy Shevchenko

2023-06-12 18:00:36

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins

On Fri, Jun 09, 2023 at 09:50:37AM +0200, Linus Walleij wrote:
> On Thu, Jun 8, 2023 at 6:22 PM Andy Shevchenko
> <[email protected]> wrote:
>
> > The aggregator mode can also handle properties of the platform, that
> > do not belong to the GPIO controller itself. One of such a property
> > is signal delay line. Intdoduce support of it.
> (...)
> > I don't like the idea of gpio-delay or similar. We have already GPIO
> > aggregator that incorporates the GPIO proxy / forwarder functionality.
>
> You are right. This is the right solution going forward IMO.

Thank you for the support.

Take into consideration 1 kinda neutral and 2 for votes, I'll going to split
and improve the patch (series).

--
With Best Regards,
Andy Shevchenko

2023-06-12 18:04:41

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins

On Fri, Jun 09, 2023 at 09:11:04AM +0200, Geert Uytterhoeven wrote:
> On Thu, Jun 8, 2023 at 6:23 PM Andy Shevchenko
> <[email protected]> wrote:
> > The aggregator mode can also handle properties of the platform, that
> > do not belong to the GPIO controller itself. One of such a property
> > is signal delay line. Intdoduce support of it.
> >
> > Signed-off-by: Andy Shevchenko <[email protected]>
> > ---
> >
> > I don't like the idea of gpio-delay or similar. We have already GPIO
> > aggregator that incorporates the GPIO proxy / forwarder functionality.
>
> I think this makes sense.

Thank you for the support of the idea.

...

> I hope no one ever needs to use the values from the example in the
> bindings
>
> enable-gpios = <&enable_delay 0 130000 30000>;
>
> on a non-sleepable GPIO. Not only is a looping delay of 130 ms very bad
> for system responsiveness, such large delays may not even be supported
> on all systems (e.g. ARM implementation says < 2 ms).
> So for large values, this should use mdelay().
>
> This also applies to gpio-delay, of course.

Thank you for pointing this out. I will think about better approach.
Shan't we add a comment into DT bindings to warn users about this?

--
With Best Regards,
Andy Shevchenko