diff mbox series

[v3,3/3] leds: tlc5925: Add support for non blocking operations

Message ID 20220609162734.1462625-4-jjhiblot@traphandler.com
State New
Headers show
Series Add support for the TLC5925 | expand

Commit Message

Jean-Jacques Hiblot June 9, 2022, 4:27 p.m. UTC 
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@traphandler.com>
---
 drivers/leds/leds-tlc5925.c | 76 +++++++++++++++++++++++++++++++++----
 1 file changed, 68 insertions(+), 8 deletions(-)

Comments

Jean-Jacques Hiblot June 14, 2022, 1:44 p.m. UTC | #1 
On 09/06/2022 18:43, Andy Shevchenko wrote:
> On Thu, Jun 9, 2022 at 6:29 PM Jean-Jacques Hiblot
> <jjhiblot@traphandler.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()).
> i.e.
>
>> +#define BITS_PER_ATOMIC (sizeof(atomic_t) * 8)
> We have BITS_PER_TYPE(). Use it directly in the code, no need for a
> whole new macro.
>
> ...
>
>> +static int xmit(struct tlc5925_leds_priv *priv)
>> +{
>> +       int i;
>> +
>> +       spin_lock(&priv->lock);
> This can't be called during IRQ?
>
>> +       for (i = 0; i < priv->max_state / (sizeof(atomic_t) * 8) ; i++)
> BITS_PER_TYPE() ?
>
>> +               priv->spi_buffer[i] = atomic_read(&priv->state[i]);
>> +       spin_unlock(&priv->lock);
>> +
>> +       return spi_write(priv->spi, priv->spi_buffer, priv->max_num_leds / 8);
>> +}
> ...
>
>> +static void xmit_work(struct work_struct *ws)
>> +{
>> +       struct tlc5925_leds_priv *priv =
>> +               container_of(ws, struct tlc5925_leds_priv, xmit_work);
> One line?
>
> Missed blank line here.
>
>> +       xmit(priv);
>> +};
> ...
>
>>          if (brightness)
>> -               priv->state[index / 8] |= (1 << (index % 8));
>> +               atomic_or(1 << (index % BITS_PER_ATOMIC),
>> +                         &priv->state[index / BITS_PER_ATOMIC]);
>>          else
>> -               priv->state[index / 8] &= ~(1 << (index % 8));
>> -       spin_unlock(&priv->lock);
>> +               atomic_and(~(1 << (index % BITS_PER_ATOMIC)),
>> +                          &priv->state[index / BITS_PER_ATOMIC]);
> The whole bunch looks like reinventing the bitmap / bitops.
> Use unsigned long (or DECLARE_BITMAP() if it can be higher than 32)
> for state and set_bit() / clear_bit() / assign_bit() that are atomic.

Thanks for pointing this out.

It will drastically simplify the code.

>
> ...
>
>> +       if (brightness)
>> +               atomic_or(1 << (index % BITS_PER_ATOMIC),
>> +                         &priv->state[index / BITS_PER_ATOMIC]);
>> +       else
>> +               atomic_and(~(1 << (index % BITS_PER_ATOMIC)),
>> +                          &priv->state[index / BITS_PER_ATOMIC]);
> assign_bit()
>
> ...
>
>> +       // Allocate the buffer used to hold the state of each LED
>> +       priv->max_state = round_up(max_num_leds, BITS_PER_ATOMIC);
>> +       priv->state = devm_kzalloc(dev,
>> +                                  priv->max_state / 8,
>> +                                  GFP_KERNEL);
>>          if (!priv->state)
>>                  return -ENOMEM;
> devm_bitmap_zalloc() ?
>
> ...
>
>> +       // Allocate a second buffer for the communication on the SPI bus
>> +       priv->spi_buffer = devm_kzalloc(dev,
>> +                                  priv->max_state / 8,
>> +                                  GFP_KERNEL);
> Not sure I understand the output, but perhaps here the BITS_TO_BYTES()
> should be used.
>
diff mbox series

Patch

diff --git a/drivers/leds/leds-tlc5925.c b/drivers/leds/leds-tlc5925.c
index 2c50ba10bdbf..943514d2c459 100644
--- a/drivers/leds/leds-tlc5925.c
+++ b/drivers/leds/leds-tlc5925.c
@@ -18,6 +18,8 @@ 
 #include <linux/property.h>
 #include <linux/workqueue.h>
 
+#define BITS_PER_ATOMIC (sizeof(atomic_t) * 8)
+
 struct single_led_priv {
 	int idx;
 	struct led_classdev cdev;
@@ -25,12 +27,35 @@  struct single_led_priv {
 
 struct tlc5925_leds_priv {
 	int max_num_leds;
-	u8 *state;
+	int max_state;
+	atomic_t *state;
+	int *spi_buffer;
 	spinlock_t lock;
+	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)
+{
+	int i;
+
+	spin_lock(&priv->lock);
+	for (i = 0; i < priv->max_state / (sizeof(atomic_t) * 8) ; i++)
+		priv->spi_buffer[i] = atomic_read(&priv->state[i]);
+	spin_unlock(&priv->lock);
+
+	return spi_write(priv->spi, priv->spi_buffer, priv->max_num_leds / 8);
+}
+
+static void xmit_work(struct work_struct *ws)
+{
+	struct tlc5925_leds_priv *priv =
+		container_of(ws, struct tlc5925_leds_priv, xmit_work);
+	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,16 +65,36 @@  static int tlc5925_brightness_set_blocking(struct led_classdev *cdev,
 						   cdev);
 	int index = led->idx;
 
-	spin_lock(&priv->lock);
 	if (brightness)
-		priv->state[index / 8] |= (1 << (index % 8));
+		atomic_or(1 << (index % BITS_PER_ATOMIC),
+			  &priv->state[index / BITS_PER_ATOMIC]);
 	else
-		priv->state[index / 8] &= ~(1 << (index % 8));
-	spin_unlock(&priv->lock);
+		atomic_and(~(1 << (index % BITS_PER_ATOMIC)),
+			   &priv->state[index / BITS_PER_ATOMIC]);
 
-	return spi_write(spi, priv->state, priv->max_num_leds / 8);
+	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);
+	int index = led->idx;
+
+	if (brightness)
+		atomic_or(1 << (index % BITS_PER_ATOMIC),
+			  &priv->state[index / BITS_PER_ATOMIC]);
+	else
+		atomic_and(~(1 << (index % BITS_PER_ATOMIC)),
+			   &priv->state[index / BITS_PER_ATOMIC]);
+
+	cancel_work_sync(&priv->xmit_work);
+	return xmit(priv);
+}
 
 static int tlc5925_probe(struct spi_device *spi)
 {
@@ -96,10 +141,24 @@  static int tlc5925_probe(struct spi_device *spi)
 
 	spin_lock_init(&priv->lock);
 
-	priv->state = devm_kzalloc(dev, DIV_ROUND_UP(max_num_leds, 8), GFP_KERNEL);
+	priv->spi = spi;
+	INIT_WORK(&priv->xmit_work, xmit_work);
+
+	// Allocate the buffer used to hold the state of each LED
+	priv->max_state = round_up(max_num_leds, BITS_PER_ATOMIC);
+	priv->state = devm_kzalloc(dev,
+				   priv->max_state / 8,
+				   GFP_KERNEL);
 	if (!priv->state)
 		return -ENOMEM;
 
+	// Allocate a second buffer for the communication on the SPI bus
+	priv->spi_buffer = devm_kzalloc(dev,
+				   priv->max_state / 8,
+				   GFP_KERNEL);
+	if (!priv->spi_buffer)
+		return -ENOMEM;
+
 	priv->max_num_leds = max_num_leds;
 
 	device_for_each_child_node(dev, child) {
@@ -120,6 +179,7 @@  static int tlc5925_probe(struct spi_device *spi)
 		cdev = &(priv->leds[count].cdev);
 		cdev->brightness = LED_OFF;
 		cdev->max_brightness = 1;
+		cdev->brightness_set = tlc5925_brightness_set;
 		cdev->brightness_set_blocking = tlc5925_brightness_set_blocking;
 
 		ret = devm_led_classdev_register_ext(dev, cdev, &init_data);