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

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

[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 <andriy.shevchenko@linux.intel.com>
---

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

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

[kernel test robot]

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: i386-randconfig-i051-20230608 (https://download.01.org/0day-ci/archive/20230609/202306090307.hZlCud1x-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build):
        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/20230608162130.55015-1-andriy.shevchenko@linux.intel.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 olddefconfig
        make W=1 O=build_dir ARCH=i386 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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306090307.hZlCud1x-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-aggregator.c: In function 'gpiochip_fwd_delay_of_xlate':
>> drivers/gpio/gpio-aggregator.c:426:41: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'
     426 |         if (gpiospec->args_count != chip->of_gpio_n_cells)
         |                                         ^~
   drivers/gpio/gpio-aggregator.c: In function 'gpiochip_fwd_create':
>> drivers/gpio/gpio-aggregator.c:518:21: error: 'struct gpio_chip' has no member named 'of_xlate'
     518 |                 chip->of_xlate = gpiochip_fwd_delay_of_xlate;
         |                     ^~
   drivers/gpio/gpio-aggregator.c:519:21: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'
     519 |                 chip->of_gpio_n_cells = 3;
         |                     ^~


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

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

[kernel test robot]

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/202306090344.UJNc4HFx-lkp@intel.com/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/20230608162130.55015-1-andriy.shevchenko@linux.intel.com
        # 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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306090344.UJNc4HFx-lkp@intel.com/

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

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

[Alexander Stein]

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 <andriy.shevchenko@linux.intel.com>
> ---
> 
> 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/

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

[Geert Uytterhoeven]

Hi Andy,

On Thu, Jun 8, 2023 at 6:23 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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 <andriy.shevchenko@linux.intel.com>
> ---
>
> 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 -- geert@linux-m68k.org

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

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

[Linus Walleij]

On Thu, Jun 8, 2023 at 6:22 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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

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

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
> > ---
> > 
> > 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

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

[Andy Shevchenko]

On Fri, Jun 09, 2023 at 09:11:04AM +0200, Geert Uytterhoeven wrote:
> On Thu, Jun 8, 2023 at 6:23 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> 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 <andriy.shevchenko@linux.intel.com>
> > ---
> >
> > 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

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

[Andy Shevchenko]

On Fri, Jun 09, 2023 at 09:50:37AM +0200, Linus Walleij wrote:
> On Thu, Jun 8, 2023 at 6:22 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> 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