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;
From:   Alexander Stein <alexander.stein@ew.tq-group.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Geert Uytterhoeven <geert@glider.be>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Andy Shevchenko <andy@kernel.org>,
        "linux@ew.tq-group.com" <linux@ew.tq-group.com>
Subject: Re: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay support for individual output pins
Date:   Fri, 09 Jun 2023 08:40:15 +0200
Message-ID: <4808746.GXAFRqVoOG@steina-w>
Organization: TQ-Systems GmbH
In-Reply-To: <20230608162130.55015-1-andriy.shevchenko@linux.intel.com>
References: <20230608162130.55015-1-andriy.shevchenko@linux.intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

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.
>=20
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>=20
> I don't like the idea of gpio-delay or similar. We have already GPIO
> aggregator that incorporates the GPIO proxy / forwarder functionality.
>=20
> 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...
>=20
> Any comments are appreciated, and tests are esp. welcome!

=46WIW: Replacing CONFIG_GPIO_DELAY=3Dm with CONFIG_GPIO_AGGREGATOR=3Dm wor=
ks as=20
well on my platform.
But I'm not sure if it's worth the additional complexity for gpio-aggregato=
r=20
to replace gpio-delay.

Regards,
Alexander

>  drivers/gpio/gpio-aggregator.c | 84 ++++++++++++++++++++++++++++++----
>  1 file changed, 74 insertions(+), 10 deletions(-)
>=20
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregato=
r.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
>   */
>=20
> +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=20
*/
>  };
>=20
> @@ -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 =3D gpiochip_get_data(chip);
> +	const struct gpiochip_fwd_timing *delay_timings;
> +	struct gpio_desc *desc =3D fwd->descs[offset];
> +	bool is_active_low =3D gpiod_is_active_low(desc);
> +	bool ramp_up;
>=20
> -	if (chip->can_sleep)
> -		gpiod_set_value_cansleep(fwd->descs[offset], value);
> -	else
> -		gpiod_set_value(fwd->descs[offset], value);
> +	delay_timings =3D &fwd->delay_timings[offset];
> +	ramp_up =3D (!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);
> +	}
>  }
>=20
>  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]);
>  }
>=20
> +static int gpiochip_fwd_delay_of_xlate(struct gpio_chip *chip,
> +				       const struct of_phandle_args=20
*gpiospec,
> +				       u32 *flags)
> +{
> +	struct gpiochip_fwd *fwd =3D gpiochip_get_data(chip);
> +	struct gpiochip_fwd_timing *timings;
> +	u32 line;
> +
> +	if (gpiospec->args_count !=3D chip->of_gpio_n_cells)
> +		return -EINVAL;
> +
> +	line =3D gpiospec->args[0];
> +	if (line >=3D chip->ngpio)
> +		return -EINVAL;
> +
> +	timings =3D &fwd->delay_timings[line];
> +	timings->ramp_up_us =3D gpiospec->args[1];
> +	timings->ramp_down_us =3D 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 operati=
ons
> 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=20
ngpios,
> -						struct gpio_desc=20
*descs[])
> +						struct gpio_desc=20
*descs[],
> +						bool delay)
>  {
>  	const char *label =3D dev_name(dev);
>  	struct gpiochip_fwd *fwd;
> @@ -459,6 +508,17 @@ static struct gpiochip_fwd *gpiochip_fwd_create(stru=
ct
> device *dev, else
>  		spin_lock_init(&fwd->slock);
>=20
> +	if (delay) {
> +		fwd->delay_timings =3D devm_kcalloc(dev, ngpios,
> +						  sizeof(*fwd-
>delay_timings),
> +						  GFP_KERNEL);
> +		if (!fwd->delay_timings)
> +			return ERR_PTR(-ENOMEM);
> +
> +		chip->of_xlate =3D gpiochip_fwd_delay_of_xlate;
> +		chip->of_gpio_n_cells =3D 3;
> +	}
> +
>  	error =3D devm_gpiochip_add_data(dev, chip, fwd);
>  	if (error)
>  		return ERR_PTR(error);
> @@ -476,6 +536,7 @@ static int gpio_aggregator_probe(struct platform_devi=
ce
> *pdev) struct device *dev =3D &pdev->dev;
>  	struct gpio_desc **descs;
>  	struct gpiochip_fwd *fwd;
> +	bool delay;
>  	int i, n;
>=20
>  	n =3D gpiod_count(dev, NULL);
> @@ -492,7 +553,9 @@ static int gpio_aggregator_probe(struct platform_devi=
ce
> *pdev) return PTR_ERR(descs[i]);
>  	}
>=20
> -	fwd =3D gpiochip_fwd_create(dev, n, descs);
> +	delay =3D fwnode_device_is_compatible(dev_fwnode(dev), "gpio-delay");
> +
> +	fwd =3D gpiochip_fwd_create(dev, n, descs, delay);
>  	if (IS_ERR(fwd))
>  		return PTR_ERR(fwd);
>=20
> @@ -500,23 +563,24 @@ static int gpio_aggregator_probe(struct
> platform_device *pdev) return 0;
>  }
>=20
> -#ifdef CONFIG_OF
>  static const struct of_device_id gpio_aggregator_dt_ids[] =3D {
>  	/*
>  	 * Add GPIO-operated devices controlled from userspace below,
> -	 * or use "driver_override" in sysfs
> +	 * or use "driver_override" in sysfs.
>  	 */
> +	{
> +		.compatible =3D "gpio-delay",
> +	},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
> -#endif
>=20
>  static struct platform_driver gpio_aggregator_driver =3D {
>  	.probe =3D gpio_aggregator_probe,
>  	.driver =3D {
>  		.name =3D DRV_NAME,
>  		.groups =3D gpio_aggregator_groups,
> -		.of_match_table =3D of_match_ptr(gpio_aggregator_dt_ids),
> +		.of_match_table =3D gpio_aggregator_dt_ids,
>  	},
>  };


=2D-=20
TQ-Systems GmbH | M=FChlstra=DFe 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht M=FCnchen, HRB 105018
Gesch=E4ftsf=FChrer: Detlef Schneider, R=FCdiger Stahl, Stefan Schneider
http://www.tq-group.com/