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@xxxxxxxxx>:
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