Re: [PATCH v2 00/12] LED core improvements

[Jacek Anaszewski <j.anaszewski@samsung.com>]

Hi Sakari,

On 10/01/2015 03:22 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> Jacek Anaszewski wrote:
>> This is a second version of the patch set preparing the
>> ground for removing work queues from LED class drivers.
>> Andrew and Sakari - thanks for a review.
>>
>> ================
>> Changes from v1:
>> ================
>> - removed useless use of else in led_set_brightness()
>> - switched to removing error code instead of emitting
>>    warning from led_set_brightness_sync() in case software
>>    blink fallback is enabled
>> - moved __led_set_brightness() contents to set_brightness_delayed()
>> - removed description of internal led_set_brightness_nosleep() from
>>    led-class.txt documentation, and modified description of
>>    led_set_brightness() and led_set_brightness_sync()
>> - renamed LED_BLINK_CHANGE flag to more meaningful
>>    LED_BLINK_BRIGHTNESS_CHANGE
>> - Fixed stylistic issues in a few comments
>> - Adopted old brightness_set_sync() op description to
>>    a new brightness_set_blocking() op
>> - Fixed led_set_brightness_nosleep to avoid scheduling
>>    spurious set_brightness_work
>> - made LED core using EXPORT_SYMBOL_GPL consistently
>> - switched v4l2-flash-led-class wrapper to using
>>    led_set_brightness_sync API for setting torch brightness
>> - merged with patch set
>>    "LED flash: Set brightness in a sync way on demand"
>
> Thanks for the update.
>
> A few comments on no particular patch ---
>
> - Synchronous brightness setting is only possible with drivers that
> implement blocking brigtness setting op. Should the work queue be
> removed from the rest of the drivers, this would be easy to fix.

Work queues are removed from drivers in the patch set [1], and
I plan on picking them after merging this series. Those changes
replace brightness_set op initialization with brightness_set_blocking.
What needs modifications to support led_set_brightness_sync API are
drivers with non-blocking brightness_set op. The question is whether
it should be accomplished by simply adding brightness_set_blocking
op initialization, with the same function as in case of brightness_set
op. Alternatively we could avoid removing SET_BRIGHTNESS_SYNC flag,
and mark non-blocking drivers with it to inform the core that they
set brightness synchronously.

> - led_classdev_suspend() and led_classdev_resume() still unconditionally
> call ->brightness_set() which may now be NULL. This needs to be fixed.
>

Right, I noticed that at some point but finally forgot to address.
Will fix in the next version.

[1] https://lkml.org/lkml/2015/8/20/426

-- 
Best Regards,
Jacek Anaszewski