On 09/06/2022 18:43, Andy Shevchenko wrote:
On Thu, Jun 9, 2022 at 6:29 PM Jean-Jacques Hiblot
<jjhiblot@xxxxxxxxxxxxxxx> 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.
