Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 9 Jun 2022 18:43:04 +0200 | Subject | Re: [PATCH v3 3/3] leds: tlc5925: Add support for non blocking operations |
| |
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.
...
> + 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.
-- With Best Regards, Andy Shevchenko
| |