linux-kernel - Re: [PATCH v4 3/3] leds: tlc5925: Add support for non blocking operations

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: <CAHp75VdwiptUUHwDT8bZmY4r7beiiT9NFD9o_-=+KiqMcqGhcg@mail.gmail.com>
Date:   Tue, 14 Jun 2022 16:51:53 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Jean-Jacques Hiblot <jjhiblot@...phandler.com>
Cc:     Pavel Machek <pavel@....cz>, krzk+dt@...nel.org,
        Rob Herring <robh+dt@...nel.org>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/3] leds: tlc5925: Add support for non blocking operations

On Tue, Jun 14, 2022 at 4:27 PM Jean-Jacques Hiblot
<jjhiblot@...phandler.com> wrote:
>
> Settings multiple LEDs in a row can be a slow operation because of the
> time required to acquire the bus and prepare the transfer.
> And, in most cases, it is not required that the operation is synchronous.
>
> Implementing the non-blocking brightness_set() for such cases.
> A work queue is used to perform the actual SPI transfer.
>
> The blocking method is still available in case someone needs to perform
> this operation synchronously (ie by calling led_set_brightness_sync()).
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@...phandler.com>
> ---
>  drivers/leds/leds-tlc5925.c | 39 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/leds-tlc5925.c b/drivers/leds/leds-tlc5925.c
> index eeab1138ba2c..2c083b04d7f6 100644
> --- a/drivers/leds/leds-tlc5925.c
> +++ b/drivers/leds/leds-tlc5925.c
> @@ -14,6 +14,7 @@
>  #include <linux/leds.h>
>  #include <linux/module.h>
>  #include <linux/property.h>
> +#include <linux/workqueue.h>
>  #include <linux/spi/spi.h>
>
>  struct single_led_priv {
> @@ -24,10 +25,24 @@ struct single_led_priv {
>  struct tlc5925_leds_priv {
>         int max_num_leds;
>         unsigned long *state;
> +       struct spi_device *spi;
> +       struct work_struct xmit_work;
>         struct single_led_priv leds[];
>  };
>
> -static int tlc5925_brightness_set_blocking(struct led_classdev *cdev,
> +static int xmit(struct tlc5925_leds_priv *priv)
> +{
> +       return spi_write(priv->spi, priv->state, BITS_TO_BYTES(priv->max_num_leds));
> +}
> +
> +static void xmit_work(struct work_struct *ws)
> +{
> +       struct tlc5925_leds_priv *priv =
> +               container_of(ws, struct tlc5925_leds_priv, xmit_work);

+ Blank line.

> +       xmit(priv);
> +};
> +
> +static void tlc5925_brightness_set(struct led_classdev *cdev,
>                                             enum led_brightness brightness)
>  {
>         struct spi_device *spi = to_spi_device(cdev->dev->parent);
> @@ -40,9 +55,25 @@ static int tlc5925_brightness_set_blocking(struct led_classdev *cdev,
>         // assign_bit() is atomic, no need for lock
>         assign_bit(index, priv->state, !!brightness);
>
> -       return spi_write(spi, priv->state, BITS_TO_BYTES(priv->max_num_leds));
> +       schedule_work(&priv->xmit_work);
>  }
>
> +static int tlc5925_brightness_set_blocking(struct led_classdev *cdev,
> +                                           enum led_brightness brightness)
> +{
> +       struct spi_device *spi = to_spi_device(cdev->dev->parent);
> +       struct tlc5925_leds_priv *priv = spi_get_drvdata(spi);

> +       struct single_led_priv *led = container_of(cdev,
> +                                                  struct single_led_priv,
> +                                                  cdev);

Better:

       struct single_led_priv *led =
                        container_of(cdev, struct single_led_priv, cdev);

...

> +       // assign_bit() is atomic, no need for lock

The useless comment.

-- 
With Best Regards,
Andy Shevchenko