[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <84da79a6-9515-2b42-b34a-0c089d7a41b7@traphandler.com>
Date: Tue, 14 Jun 2022 15:44:14 +0200
From: Jean-Jacques Hiblot <jjhiblot@...phandler.com>
To: Andy Shevchenko <andy.shevchenko@...il.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 v3 3/3] leds: tlc5925: Add support for non blocking
operations
On 09/06/2022 18:43, Andy Shevchenko wrote:
> On Thu, Jun 9, 2022 at 6:29 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()).
> 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.
>
Powered by blists - more mailing lists