Hi, finally, I found the time to update the driver according to the many comments received a while ago.
Most of them have been worked in, including the regmap idea and brightness_set_blocking().
The driver works on our system, so that I will mail [PATCH v2] as a followup.
There is only one aspect of the new solution I am not sure if it is really better than our old proposal (see below).
Am 20.04.2016 um 23:04 schrieb Jacek Anaszewski jacek.anaszewski@gmail.com:
On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote:
I believe the LEDS core now handles the workqueues generically for blocking operations, so it's no longer needed in the individual drivers.
We had a lot of trouble with locking and blocking especially if we want to indicate CPU or (root) disk activity.
What kind of troubles you had? Could you share more details? Does it mean that current LED core design doesn't fit for your use cases?
The system started to flicker the LEDs irregularily and sometimes the whole kernel stalled.
So it is implemented in a way that changes can be requested faster than the I2C bus can write new values to the chip.
Only after one sequence of I2C writes is done, another work function can be scheduled. And each group of writes updates as many LEDs in parallel if necessary.
You can serialize the operations in brightness_set_blocking with a mutex.
Yes, that works fine in our (incomplete) test setup.
But I think it assumes that the i2c bus is never congested by other i2c traffic.
I have not found code that obviously takes care of the situation if led trigger events (e.g. mmc or cpu triggers) are coming in faster than the i2c (even using regmap) can write out over i2c.
If I understand the led core code correctly, it will just do another schedule_work for every single change of led brightness.
So I wonder if Is there some guarantee that this work queue will not fill up memory and is really processed faster than being filled? I.e. can the queue overflow?
To reduce this risk, my original implementation strategy was different. The update speed was limited by i2c writing. A new register update batch job was only scheduled if the previous one is finished. If i2c was blocked/congested, the writing worker thread would come to a halt.
All incoming led brightness changes were simply accumulated until a new batch job is started, because LEDs would anyways flicker invisibly fast.
Tests with the new driver have shown that it seems not to run into this situation on our system but it might depend on factors we have not yet tested (slow i2c, other i2c traffic on the same bus, CPU speed, event types choosen).
So I am a little in doubt about this risk. But I may have simply missed the reason why the standard approach works and can never overflow.
BR, Nikolaus