[rfc,rft,v1,1/1] gpio: aggregator: Introduce delay support for individual output pins
Message ID | 20230608162130.55015-1-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp400578vqr; Thu, 8 Jun 2023 09:33:30 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4CPo903uooThjdFIWMpC3S9WQpIbF8D8O/F+FcFP+XIZnyaZs4kzBD9bDapJMLYiKX8HSD X-Received: by 2002:a17:90a:db95:b0:259:aa15:80e6 with SMTP id h21-20020a17090adb9500b00259aa1580e6mr7208739pjv.23.1686242010516; Thu, 08 Jun 2023 09:33:30 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1686242010; cv=none; d=google.com; s=arc-20160816; b=v7wtxoa4/kXui2lyhjeWyALO9qdAXZ8gHTMgrJBu06qF5fOeuCc7y+sehMEfVbaDN3 2GImkiwMk7WQFAcjTOuXAmRbsvqtRxRMJDIYPRhvIMwNoV76yUMAna7CCSyfb5rPSCjK ZXxfhV/yNNGzOaXeJidupeKTMznm8jAMlYjJECul0+rrcXAqhFWc9P9E/4w4iJkVEGnF CY3dM0UFTK9o9byJiOHu/b0UYtp2ER8QKhYOtKIeUZ3+jMrl2lGcpUWfSJSo5ZHBwZS1 lmqS7UMg6WT/fGyAIRFxRX+2ZG/5n86/BJig6I6L+0M9i8LM/llGb14McnwDqY/DvZyI +xFA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=CguRzEWjCI7nIxORqVQBuHehcvBi6aI6lujJT4QlYKU=; b=qb1gL7+ziRmmNavNYZvhjKHCsKoBx7LShSRfzwYFgLQ4tGkeINC83wt5nZ/qr+J9fR lsPdp3t7PKOdiDZLhH8rFklNJkF6m0sq4WQp82tSyYeGA487VNaR3DuBHqAJrIXEGb3f 9hND1o9Ju6NpJATkqVplvIe4/Z6DO6bELxYHfjO4Z1vB4GNuRiOZ5Hr8uN45pnsylTqJ uE1ZQPZEBIH29irFZOYGVKjM9+VTm7Y/JGOq78JZ2aQ3SpNXQCuZUQBGN3MYXBSnSdhT EAk7jZmO6YtE0jEVTje2NFQg1KmXlYLrgXFT12okU8gA9ZvT+FVH1ZC+nbF8qv5GCdHh apEg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=ib3NjQA9; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id x16-20020a17090a789000b00253160141c7si2977477pjk.83.2023.06.08.09.33.18; Thu, 08 Jun 2023 09:33:30 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=ib3NjQA9; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234539AbjFHQXb (ORCPT <rfc822;literming00@gmail.com> + 99 others); Thu, 8 Jun 2023 12:23:31 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33388 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234524AbjFHQXG (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 8 Jun 2023 12:23:06 -0400 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 869702D72; Thu, 8 Jun 2023 09:22:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1686241377; x=1717777377; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=0EWXB2OkiFb2hh+8AxZy+RiHPeZQVtYstodxAyYGW94=; b=ib3NjQA9e0047FXHvdxmTUdGmpTyIrzcTtSFt06oE3BaHwj0csg+bO+p 0eq/dBQAqZsi5Khnrvc5USVxIGdKP+zPmSrbfc6zIPqzZONoHMlCcAaxe tvqLiRmVZZttp89pyROkpeOJgBrf0mjTskSwOXY6GgE5OmmzGklzerVO/ cn08LblaUwGiHchoBVTjuvMhI0KXekufmDTZi0Z0jJYYVIbK3XhlIfYBO tZd25pUmpbymFbYQHXqQsiyjwc8htFbEy5w1WktnK0B7+5yf0BnIPgnpl 2Yg48r8puu8jx5blBQb3m8Sm9TNUqzeXJ+lUc9/8WXehU1rEMkjFGyIlI w==; X-IronPort-AV: E=McAfee;i="6600,9927,10735"; a="385701292" X-IronPort-AV: E=Sophos;i="6.00,227,1681196400"; d="scan'208";a="385701292" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jun 2023 09:21:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10735"; a="884252675" X-IronPort-AV: E=Sophos;i="6.00,227,1681196400"; d="scan'208";a="884252675" Received: from black.fi.intel.com ([10.237.72.28]) by orsmga005.jf.intel.com with ESMTP; 08 Jun 2023 09:21:26 -0700 Received: by black.fi.intel.com (Postfix, from userid 1003) id 45B8C1B4; Thu, 8 Jun 2023 19:21:33 +0300 (EEST) From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Geert Uytterhoeven <geert+renesas@glider.be>, Linus Walleij <linus.walleij@linaro.org>, Bartosz Golaszewski <brgl@bgdev.pl>, Andy Shevchenko <andy@kernel.org>, Alexander Stein <linux@ew.tq-group.com> Subject: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins Date: Thu, 8 Jun 2023 19:21:30 +0300 Message-Id: <20230608162130.55015-1-andriy.shevchenko@linux.intel.com> X-Mailer: git-send-email 2.40.0.1.gaa8946217a0b MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1768152902008596083?= X-GMAIL-MSGID: =?utf-8?q?1768152902008596083?= |
Series |
[rfc,rft,v1,1/1] gpio: aggregator: Introduce delay support for individual output pins
|
|
Commit Message
Andy Shevchenko
June 8, 2023, 4:21 p.m. UTC
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(-)
Comments
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
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
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, > }, > };
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
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
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.
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?
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).
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, }, };