diff mbox series

[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 show
Series [rfc,rft,v1,1/1] gpio: aggregator: Introduce delay support for individual output pins | expand

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

Andy Shevchenko June 12, 2023, 5:32 p.m. UTC | #1
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?
diff mbox series

Patch

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,
 	},
 };