Re: [PATCH 1/3] gpio: Add GPIO delay driver

[Michal Simek <michal.simek@amd.com>]

On 1/30/26 08:21, Peng Fan wrote:
> Hi Michal,
> 
> On Thu, Jan 22, 2026 at 01:50:33PM +0100, Michal Simek wrote:
>> Add a GPIO controller driver that provides configurable delays when
>> setting GPIO output values. This is useful for hardware that requires
>> specific timing delays during power sequencing or GPIO state changes.
>>
>> The driver wraps underlying GPIO controllers and adds programmable
>> ramp-up and ramp-down delays specified in microseconds through the
>> device tree. Each GPIO can have independent delay timings.
>>
>> Device tree binding matches Linux.
>>
>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>> ---
>>
>> drivers/gpio/Kconfig      |   8 +++
>> drivers/gpio/Makefile     |   1 +
>> drivers/gpio/gpio-delay.c | 133 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 142 insertions(+)
>> create mode 100644 drivers/gpio/gpio-delay.c
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index 60c5c54688e6..f69919abc05b 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -1,3 +1,11 @@
>> +config GPIO_DELAY
>> +	bool "GPIO delay driver"
>> +	depends on DM_GPIO
>> +	help
>> +	  Enable the GPIO delay driver.
>> +	  This driver allows wrapping another GPIO controller and inserting
>> +	  ramp-up/ramp-down delays on output changes, as described in the
>> +	  Linux gpio-delay binding.
> 
> Add an entry for SPL?

Why? I don't have a need for it and someone should test it if this should be 
used in SPL. That can be done on the top of this when tested.


>> #
>> # GPIO infrastructure and drivers
>> #
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index 910478c0c7a9..fec258f59f52 100644
>> --- a/drivers/gpio/Makefile
>> +++ b/drivers/gpio/Makefile
>> @@ -21,6 +21,7 @@ obj-$(CONFIG_ATMEL_PIO4)	+= atmel_pio4.o
>> obj-$(CONFIG_BCM6345_GPIO)	+= bcm6345_gpio.o
>> obj-$(CONFIG_CORTINA_GPIO)      += cortina_gpio.o
>> obj-$(CONFIG_FXL6408_GPIO)	+= gpio-fxl6408.o
>> +obj-$(CONFIG_GPIO_DELAY)	+= gpio-delay.o
>> obj-$(CONFIG_INTEL_GPIO)	+= intel_gpio.o
>> obj-$(CONFIG_INTEL_ICH6_GPIO)	+= intel_ich6_gpio.o
>> obj-$(CONFIG_INTEL_BROADWELL_GPIO)	+= intel_broadwell_gpio.o
>> diff --git a/drivers/gpio/gpio-delay.c b/drivers/gpio/gpio-delay.c
>> new file mode 100644
>> index 000000000000..0c0d05ccb493
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-delay.c
>> @@ -0,0 +1,133 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2025 - 2026, Advanced Micro Devices, Inc.
> 
> Drop 2025?

Why? Driver was developed in 2025 but upstreaming happens in 2026.

> 
>> + *
>> + * Michal Simek <michal.simek@amd.com>
>> + */
>> +
>> +#include <dm.h>
>> +#include <dm/device_compat.h>
>> +#include <dm/devres.h>
>> +#include <asm/gpio.h>
>> +#include <linux/delay.h>
>> +
>> +struct gpio_delay_desc {
>> +	struct gpio_desc real_gpio;
>> +	u32 ramp_up_us;
>> +	u32 ramp_down_us;
>> +};
>> +
>> +struct gpio_delay_priv {
>> +	struct gpio_delay_desc *descs;
>> +};
>> +
>> +static int gpio_delay_direction_input(struct udevice *dev, unsigned int offset)
>> +{
>> +	return -ENOSYS;
>> +}
>> +
>> +static int gpio_delay_get_value(struct udevice *dev, unsigned int offset)
>> +{
>> +	return -ENOSYS;
>> +}
>> +
>> +static int gpio_delay_set_value(struct udevice *dev, unsigned int offset,
>> +				int value)
>> +{
>> +	struct gpio_delay_priv *priv = dev_get_priv(dev);
>> +	struct gpio_delay_desc *desc = &priv->descs[offset];
>> +	int ret = dm_gpio_set_value(&desc->real_gpio, value);
>> +	u32 wait;
> 
> 
> Check return value.
> 	if (ret) {
> 		dev_err(dev, "Failed to set gpio %d\n", offset);
> 	}

will add.

> 
>> +
>> +	dev_dbg(dev, "gpio %d set to %d\n", offset, value);
>> +
>> +	if (value)
>> +		wait = desc->ramp_up_us;
>> +	else
>> +		wait = desc->ramp_down_us;
>> +
>> +	udelay(wait);
>> +
>> +	dev_dbg(dev, "waited for %d us\n", wait);
>> +
>> +	return ret;
>> +}
>> +
>> +static int gpio_delay_direction_output(struct udevice *dev, unsigned int offset,
>> +				       int value)
>> +{
>> +	return gpio_delay_set_value(dev, offset, value);
>> +}
>> +
>> +static int gpio_delay_xlate(struct udevice *dev, struct gpio_desc *desc,
>> +			    struct ofnode_phandle_args *args)
>> +{
>> +	struct gpio_delay_priv *priv = dev_get_priv(dev);
>> +
>> +	if (args->args_count < 3)
>> +		return -EINVAL;
>> +
>> +	if (args->args[0] >= 32)
>> +		return -EINVAL;
>> +
>> +	struct gpio_delay_desc *d = &priv->descs[args->args[0]];
>> +
>> +	d->ramp_up_us = args->args[1];
>> +	d->ramp_down_us = args->args[2];
>> +
>> +	dev_dbg(dev, "pin: %d, ramp_up_us: %d, ramp_down_us: %d\n",
>> +		args->args[0], d->ramp_up_us, d->ramp_down_us);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct dm_gpio_ops gpio_delay_ops = {
>> +	.direction_output = gpio_delay_direction_output,
>> +	.direction_input = gpio_delay_direction_input,
>> +	.get_value = gpio_delay_get_value,
>> +	.set_value = gpio_delay_set_value,
>> +	.xlate = gpio_delay_xlate,
>> +};
>> +
>> +static int gpio_delay_probe(struct udevice *dev)
>> +{
>> +	struct gpio_delay_priv *priv = dev_get_priv(dev);
>> +	struct gpio_delay_desc *d;
>> +	ofnode node = dev_ofnode(dev);
>> +	int i = 0, ret, ngpio;
>> +
>> +	ngpio = gpio_get_list_count(dev, "gpios");
>> +	if (ngpio < 0)
>> +		return ngpio;
>> +
>> +	dev_dbg(dev, "gpios: %d\n", ngpio);
>> +
>> +	priv->descs = devm_kmalloc_array(dev, ngpio, sizeof(*d), GFP_KERNEL);
>> +	if (!priv->descs)
>> +		return -ENOMEM;
>> +
>> +	/* Request all GPIOs described in the controller node */
>> +	for (i = 0; i < ngpio; i++) {
>> +		d = &priv->descs[i];
>> +		ret = gpio_request_by_name_nodev(node, "gpios", i,
>> +						 &d->real_gpio, GPIOD_IS_OUT);
> 
> This will configure the GPIO as output and ACTIVE high/low flag will also
> be used per my understanding.
> 
> Should the dir and value be deferred until set_value is invoked?

It is clear that it should be output because you just setting it up later.
But I don't have any issue not to pass any flag here.
Please let me know if you want me to change it to 0.

Thanks,
Michal