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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdXhcR0MTVVpm-2PiSZkCc+BnU6oEp_aVRj+rqvBbWLHMA@mail.gmail.com>
Date:   Fri, 9 Jun 2023 09:11:04 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Andy Shevchenko <andy@...nel.org>,
        Alexander Stein <linux@...tq-group.com>
Subject: Re: [rfc, rft, PATCH v1 1/1] gpio: aggregator: Introduce delay
 support for individual output pins

Hi Andy,

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